From 0344f68cfdea246d49ca9f4cfc641c3d20433270 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Sun, 9 Feb 2025 21:15:33 +1100 Subject: [PATCH] Update attestation rewards API for Electra (#6819) Closes: - https://github.com/sigp/lighthouse/issues/6818 Use `MAX_EFFECTIVE_BALANCE_ELECTRA` (2048) for attestation reward calculations involving Electra. Add a new `InteropGenesisBuilder` that tries to provide a more flexible way to build genesis states. Unfortunately due to lifetime jank, it is quite unergonomic at present. We may want to refactor this builder in future to make it easier to use. --- .../beacon_chain/src/attestation_rewards.rs | 19 +- beacon_node/beacon_chain/src/test_utils.rs | 57 +++-- beacon_node/beacon_chain/tests/rewards.rs | 109 +++++++++ beacon_node/genesis/src/interop.rs | 229 +++++++++++------- beacon_node/genesis/src/lib.rs | 2 +- beacon_node/http_api/tests/fork_tests.rs | 59 +++-- 6 files changed, 341 insertions(+), 134 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_rewards.rs b/beacon_node/beacon_chain/src/attestation_rewards.rs index 3b37b09e402..4f7c480c8c3 100644 --- a/beacon_node/beacon_chain/src/attestation_rewards.rs +++ b/beacon_node/beacon_chain/src/attestation_rewards.rs @@ -175,7 +175,9 @@ impl BeaconChain { let base_reward_per_increment = BaseRewardPerIncrement::new(total_active_balance, spec)?; - for effective_balance_eth in 1..=self.max_effective_balance_increment_steps()? { + for effective_balance_eth in + 1..=self.max_effective_balance_increment_steps(previous_epoch)? + { let effective_balance = effective_balance_eth.safe_mul(spec.effective_balance_increment)?; let base_reward = @@ -321,11 +323,14 @@ impl BeaconChain { }) } - fn max_effective_balance_increment_steps(&self) -> Result { + fn max_effective_balance_increment_steps( + &self, + rewards_epoch: Epoch, + ) -> Result { let spec = &self.spec; - let max_steps = spec - .max_effective_balance - .safe_div(spec.effective_balance_increment)?; + let fork_name = spec.fork_name_at_epoch(rewards_epoch); + let max_effective_balance = spec.max_effective_balance_for_fork(fork_name); + let max_steps = max_effective_balance.safe_div(spec.effective_balance_increment)?; Ok(max_steps) } @@ -386,7 +391,9 @@ impl BeaconChain { let mut ideal_attestation_rewards_list = Vec::new(); let sqrt_total_active_balance = SqrtTotalActiveBalance::new(total_balances.current_epoch()); - for effective_balance_step in 1..=self.max_effective_balance_increment_steps()? { + for effective_balance_step in + 1..=self.max_effective_balance_increment_steps(previous_epoch)? + { let effective_balance = effective_balance_step.safe_mul(spec.effective_balance_increment)?; let base_reward = diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index e61146bfc87..8c9e3929f6b 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -31,7 +31,7 @@ use execution_layer::{ ExecutionLayer, }; use futures::channel::mpsc::Receiver; -pub use genesis::{interop_genesis_state_with_eth1, DEFAULT_ETH1_BLOCK_HASH}; +pub use genesis::{InteropGenesisBuilder, DEFAULT_ETH1_BLOCK_HASH}; use int_to_bytes::int_to_bytes32; use kzg::trusted_setup::get_trusted_setup; use kzg::{Kzg, TrustedSetup}; @@ -232,6 +232,7 @@ pub struct Builder { mock_execution_layer: Option>, testing_slot_clock: Option, validator_monitor_config: Option, + genesis_state_builder: Option>, import_all_data_columns: bool, runtime: TestRuntime, log: Logger, @@ -253,16 +254,22 @@ impl Builder> { ) .unwrap(), ); + let genesis_state_builder = self.genesis_state_builder.take().unwrap_or_else(|| { + // Set alternating withdrawal credentials if no builder is specified. + InteropGenesisBuilder::default().set_alternating_eth1_withdrawal_credentials() + }); + let mutator = move |builder: BeaconChainBuilder<_>| { let header = generate_genesis_header::(builder.get_spec(), false); - let genesis_state = interop_genesis_state_with_eth1::( - &validator_keypairs, - HARNESS_GENESIS_TIME, - Hash256::from_slice(DEFAULT_ETH1_BLOCK_HASH), - header, - builder.get_spec(), - ) - .expect("should generate interop state"); + let genesis_state = genesis_state_builder + .set_opt_execution_payload_header(header) + .build_genesis_state( + &validator_keypairs, + HARNESS_GENESIS_TIME, + Hash256::from_slice(DEFAULT_ETH1_BLOCK_HASH), + builder.get_spec(), + ) + .expect("should generate interop state"); builder .genesis_state(genesis_state) .expect("should build state using recent genesis") @@ -318,16 +325,22 @@ impl Builder> { .clone() .expect("cannot build without validator keypairs"); + let genesis_state_builder = self.genesis_state_builder.take().unwrap_or_else(|| { + // Set alternating withdrawal credentials if no builder is specified. + InteropGenesisBuilder::default().set_alternating_eth1_withdrawal_credentials() + }); + let mutator = move |builder: BeaconChainBuilder<_>| { let header = generate_genesis_header::(builder.get_spec(), false); - let genesis_state = interop_genesis_state_with_eth1::( - &validator_keypairs, - HARNESS_GENESIS_TIME, - Hash256::from_slice(DEFAULT_ETH1_BLOCK_HASH), - header, - builder.get_spec(), - ) - .expect("should generate interop state"); + let genesis_state = genesis_state_builder + .set_opt_execution_payload_header(header) + .build_genesis_state( + &validator_keypairs, + HARNESS_GENESIS_TIME, + Hash256::from_slice(DEFAULT_ETH1_BLOCK_HASH), + builder.get_spec(), + ) + .expect("should generate interop state"); builder .genesis_state(genesis_state) .expect("should build state using recent genesis") @@ -375,6 +388,7 @@ where mock_execution_layer: None, testing_slot_clock: None, validator_monitor_config: None, + genesis_state_builder: None, import_all_data_columns: false, runtime, log, @@ -560,6 +574,15 @@ where self } + pub fn with_genesis_state_builder( + mut self, + f: impl FnOnce(InteropGenesisBuilder) -> InteropGenesisBuilder, + ) -> Self { + let builder = self.genesis_state_builder.take().unwrap_or_default(); + self.genesis_state_builder = Some(f(builder)); + self + } + pub fn build(self) -> BeaconChainHarness> { let (shutdown_tx, shutdown_receiver) = futures::channel::mpsc::channel(1); diff --git a/beacon_node/beacon_chain/tests/rewards.rs b/beacon_node/beacon_chain/tests/rewards.rs index be7045c54a9..41e6467b0fa 100644 --- a/beacon_node/beacon_chain/tests/rewards.rs +++ b/beacon_node/beacon_chain/tests/rewards.rs @@ -36,6 +36,38 @@ fn get_harness(spec: ChainSpec) -> BeaconChainHarness> { .keypairs(KEYPAIRS.to_vec()) .fresh_ephemeral_store() .chain_config(chain_config) + .mock_execution_layer() + .build(); + + harness.advance_slot(); + + harness +} + +fn get_electra_harness(spec: ChainSpec) -> BeaconChainHarness> { + let chain_config = ChainConfig { + reconstruct_historic_states: true, + ..Default::default() + }; + + let spec = Arc::new(spec); + + let harness = BeaconChainHarness::builder(E::default()) + .spec(spec.clone()) + .keypairs(KEYPAIRS.to_vec()) + .with_genesis_state_builder(|builder| { + builder.set_initial_balance_fn(Box::new(move |i| { + // Use a variety of balances between min activation balance and max effective balance. + let balance = spec.max_effective_balance_electra + / (i as u64 + 1) + / spec.effective_balance_increment + * spec.effective_balance_increment; + balance.max(spec.min_activation_balance) + })) + }) + .fresh_ephemeral_store() + .chain_config(chain_config) + .mock_execution_layer() .build(); harness.advance_slot(); @@ -560,6 +592,83 @@ async fn test_rewards_altair_inactivity_leak_justification_epoch() { assert_eq!(expected_balances, balances); } +#[tokio::test] +async fn test_rewards_electra() { + let spec = ForkName::Electra.make_genesis_spec(E::default_spec()); + let harness = get_electra_harness(spec.clone()); + let target_epoch = 0; + + // advance until epoch N + 1 and get initial balances + harness + .extend_slots((E::slots_per_epoch() * (target_epoch + 1)) as usize) + .await; + let mut expected_balances = harness.get_current_state().balances().to_vec(); + + // advance until epoch N + 2 and build proposal rewards map + let mut proposal_rewards_map = HashMap::new(); + let mut sync_committee_rewards_map = HashMap::new(); + for _ in 0..E::slots_per_epoch() { + let state = harness.get_current_state(); + let slot = state.slot() + Slot::new(1); + + // calculate beacon block rewards / penalties + let ((signed_block, _maybe_blob_sidecars), mut state) = + harness.make_block_return_pre_state(state, slot).await; + let beacon_block_reward = harness + .chain + .compute_beacon_block_reward(signed_block.message(), &mut state) + .unwrap(); + + let total_proposer_reward = proposal_rewards_map + .entry(beacon_block_reward.proposer_index) + .or_insert(0); + *total_proposer_reward += beacon_block_reward.total as i64; + + // calculate sync committee rewards / penalties + let reward_payload = harness + .chain + .compute_sync_committee_rewards(signed_block.message(), &mut state) + .unwrap(); + + for reward in reward_payload { + let total_sync_reward = sync_committee_rewards_map + .entry(reward.validator_index) + .or_insert(0); + *total_sync_reward += reward.reward; + } + + harness.extend_slots(1).await; + } + + // compute reward deltas for all validators in epoch N + let StandardAttestationRewards { + ideal_rewards, + total_rewards, + } = harness + .chain + .compute_attestation_rewards(Epoch::new(target_epoch), vec![]) + .unwrap(); + + // assert ideal rewards are greater than 0 + assert_eq!( + ideal_rewards.len() as u64, + spec.max_effective_balance_electra / spec.effective_balance_increment + ); + assert!(ideal_rewards + .iter() + .all(|reward| reward.head > 0 && reward.target > 0 && reward.source > 0)); + + // apply attestation, proposal, and sync committee rewards and penalties to initial balances + apply_attestation_rewards(&mut expected_balances, total_rewards); + apply_other_rewards(&mut expected_balances, &proposal_rewards_map); + apply_other_rewards(&mut expected_balances, &sync_committee_rewards_map); + + // verify expected balances against actual balances + let balances: Vec = harness.get_current_state().balances().to_vec(); + + assert_eq!(expected_balances, balances); +} + #[tokio::test] async fn test_rewards_base_subset_only() { let spec = ForkName::Base.make_genesis_spec(E::default_spec()); diff --git a/beacon_node/genesis/src/interop.rs b/beacon_node/genesis/src/interop.rs index 90c4ad6e665..4fccc0393bf 100644 --- a/beacon_node/genesis/src/interop.rs +++ b/beacon_node/genesis/src/interop.rs @@ -24,10 +24,134 @@ fn eth1_withdrawal_credentials(pubkey: &PublicKey, spec: &ChainSpec) -> Hash256 Hash256::from_slice(&credentials) } +pub type WithdrawalCredentialsFn = + Box Fn(usize, &'a PublicKey, &'a ChainSpec) -> Hash256>; + /// Builds a genesis state as defined by the Eth2 interop procedure (see below). /// /// Reference: /// https://github.com/ethereum/eth2.0-pm/tree/6e41fcf383ebeb5125938850d8e9b4e9888389b4/interop/mocked_start +#[derive(Default)] +pub struct InteropGenesisBuilder { + /// Mapping from validator index to initial balance for each validator. + /// + /// If `None`, then the default balance of 32 ETH will be used. + initial_balance_fn: Option u64>>, + + /// Mapping from validator index and pubkey to withdrawal credentials for each validator. + /// + /// If `None`, then default BLS withdrawal credentials will be used. + withdrawal_credentials_fn: Option, + + /// The execution payload header to embed in the genesis state. + execution_payload_header: Option>, +} + +impl InteropGenesisBuilder { + pub fn new() -> Self { + Self::default() + } + + pub fn set_initial_balance_fn(mut self, initial_balance_fn: Box u64>) -> Self { + self.initial_balance_fn = Some(initial_balance_fn); + self + } + + pub fn set_withdrawal_credentials_fn( + mut self, + withdrawal_credentials_fn: WithdrawalCredentialsFn, + ) -> Self { + self.withdrawal_credentials_fn = Some(withdrawal_credentials_fn); + self + } + + pub fn set_alternating_eth1_withdrawal_credentials(self) -> Self { + self.set_withdrawal_credentials_fn(Box::new(alternating_eth1_withdrawal_credentials_fn)) + } + + pub fn set_execution_payload_header( + self, + execution_payload_header: ExecutionPayloadHeader, + ) -> Self { + self.set_opt_execution_payload_header(Some(execution_payload_header)) + } + + pub fn set_opt_execution_payload_header( + mut self, + execution_payload_header: Option>, + ) -> Self { + self.execution_payload_header = execution_payload_header; + self + } + + pub fn build_genesis_state( + self, + keypairs: &[Keypair], + genesis_time: u64, + eth1_block_hash: Hash256, + spec: &ChainSpec, + ) -> Result, String> { + // Generate withdrawal credentials using provided function, or default BLS. + let withdrawal_credentials_fn = self.withdrawal_credentials_fn.unwrap_or_else(|| { + Box::new(|_, pubkey, spec| bls_withdrawal_credentials(pubkey, spec)) + }); + + let withdrawal_credentials = keypairs + .iter() + .map(|key| &key.pk) + .enumerate() + .map(|(i, pubkey)| withdrawal_credentials_fn(i, pubkey, spec)) + .collect::>(); + + // Generate initial balances. + let initial_balance_fn = self + .initial_balance_fn + .unwrap_or_else(|| Box::new(|_| spec.max_effective_balance)); + + let eth1_timestamp = 2_u64.pow(40); + + let initial_balances = (0..keypairs.len()) + .map(initial_balance_fn) + .collect::>(); + + let datas = keypairs + .into_par_iter() + .zip(withdrawal_credentials.into_par_iter()) + .zip(initial_balances.into_par_iter()) + .map(|((keypair, withdrawal_credentials), amount)| { + let mut data = DepositData { + withdrawal_credentials, + pubkey: keypair.pk.clone().into(), + amount, + signature: Signature::empty().into(), + }; + + data.signature = data.create_signature(&keypair.sk, spec); + + data + }) + .collect::>(); + + let mut state = initialize_beacon_state_from_eth1( + eth1_block_hash, + eth1_timestamp, + genesis_deposits(datas, spec)?, + self.execution_payload_header, + spec, + ) + .map_err(|e| format!("Unable to initialize genesis state: {:?}", e))?; + + *state.genesis_time_mut() = genesis_time; + + // Invalidate all the caches after all the manual state surgery. + state + .drop_all_caches() + .map_err(|e| format!("Unable to drop caches: {:?}", e))?; + + Ok(state) + } +} + pub fn interop_genesis_state( keypairs: &[Keypair], genesis_time: u64, @@ -35,18 +159,21 @@ pub fn interop_genesis_state( execution_payload_header: Option>, spec: &ChainSpec, ) -> Result, String> { - let withdrawal_credentials = keypairs - .iter() - .map(|keypair| bls_withdrawal_credentials(&keypair.pk, spec)) - .collect::>(); - interop_genesis_state_with_withdrawal_credentials::( - keypairs, - &withdrawal_credentials, - genesis_time, - eth1_block_hash, - execution_payload_header, - spec, - ) + InteropGenesisBuilder::new() + .set_opt_execution_payload_header(execution_payload_header) + .build_genesis_state(keypairs, genesis_time, eth1_block_hash, spec) +} + +fn alternating_eth1_withdrawal_credentials_fn<'a>( + index: usize, + pubkey: &'a PublicKey, + spec: &'a ChainSpec, +) -> Hash256 { + if index % 2usize == 0usize { + bls_withdrawal_credentials(pubkey, spec) + } else { + eth1_withdrawal_credentials(pubkey, spec) + } } // returns an interop genesis state except every other @@ -58,80 +185,10 @@ pub fn interop_genesis_state_with_eth1( execution_payload_header: Option>, spec: &ChainSpec, ) -> Result, String> { - let withdrawal_credentials = keypairs - .iter() - .enumerate() - .map(|(index, keypair)| { - if index % 2 == 0 { - bls_withdrawal_credentials(&keypair.pk, spec) - } else { - eth1_withdrawal_credentials(&keypair.pk, spec) - } - }) - .collect::>(); - interop_genesis_state_with_withdrawal_credentials::( - keypairs, - &withdrawal_credentials, - genesis_time, - eth1_block_hash, - execution_payload_header, - spec, - ) -} - -pub fn interop_genesis_state_with_withdrawal_credentials( - keypairs: &[Keypair], - withdrawal_credentials: &[Hash256], - genesis_time: u64, - eth1_block_hash: Hash256, - execution_payload_header: Option>, - spec: &ChainSpec, -) -> Result, String> { - if keypairs.len() != withdrawal_credentials.len() { - return Err(format!( - "wrong number of withdrawal credentials, expected: {}, got: {}", - keypairs.len(), - withdrawal_credentials.len() - )); - } - - let eth1_timestamp = 2_u64.pow(40); - let amount = spec.max_effective_balance; - - let datas = keypairs - .into_par_iter() - .zip(withdrawal_credentials.into_par_iter()) - .map(|(keypair, &withdrawal_credentials)| { - let mut data = DepositData { - withdrawal_credentials, - pubkey: keypair.pk.clone().into(), - amount, - signature: Signature::empty().into(), - }; - - data.signature = data.create_signature(&keypair.sk, spec); - - data - }) - .collect::>(); - - let mut state = initialize_beacon_state_from_eth1( - eth1_block_hash, - eth1_timestamp, - genesis_deposits(datas, spec)?, - execution_payload_header, - spec, - ) - .map_err(|e| format!("Unable to initialize genesis state: {:?}", e))?; - - *state.genesis_time_mut() = genesis_time; - - // Invalidate all the caches after all the manual state surgery. - state - .drop_all_caches() - .map_err(|e| format!("Unable to drop caches: {:?}", e))?; - - Ok(state) + InteropGenesisBuilder::new() + .set_alternating_eth1_withdrawal_credentials() + .set_opt_execution_payload_header(execution_payload_header) + .build_genesis_state(keypairs, genesis_time, eth1_block_hash, spec) } #[cfg(test)] diff --git a/beacon_node/genesis/src/lib.rs b/beacon_node/genesis/src/lib.rs index 3fb053bf880..1fba64aafb3 100644 --- a/beacon_node/genesis/src/lib.rs +++ b/beacon_node/genesis/src/lib.rs @@ -7,6 +7,6 @@ pub use eth1::Eth1Endpoint; pub use eth1_genesis_service::{Eth1GenesisService, Statistics}; pub use interop::{ bls_withdrawal_credentials, interop_genesis_state, interop_genesis_state_with_eth1, - interop_genesis_state_with_withdrawal_credentials, DEFAULT_ETH1_BLOCK_HASH, + InteropGenesisBuilder, DEFAULT_ETH1_BLOCK_HASH, }; pub use types::test_utils::generate_deterministic_keypairs; diff --git a/beacon_node/http_api/tests/fork_tests.rs b/beacon_node/http_api/tests/fork_tests.rs index d6b8df33b3f..10e1d015368 100644 --- a/beacon_node/http_api/tests/fork_tests.rs +++ b/beacon_node/http_api/tests/fork_tests.rs @@ -5,7 +5,7 @@ use beacon_chain::{ }; use eth2::types::{IndexedErrorMessage, StateId, SyncSubcommittee}; use execution_layer::test_utils::generate_genesis_header; -use genesis::{bls_withdrawal_credentials, interop_genesis_state_with_withdrawal_credentials}; +use genesis::{bls_withdrawal_credentials, InteropGenesisBuilder}; use http_api::test_utils::*; use std::collections::HashSet; use types::{ @@ -346,35 +346,46 @@ fn assert_server_indexed_error(error: eth2::Error, status_code: u16, indices: Ve #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn bls_to_execution_changes_update_all_around_capella_fork() { - let validator_count = 128; + const VALIDATOR_COUNT: usize = 128; let fork_epoch = Epoch::new(2); let spec = capella_spec(fork_epoch); let max_bls_to_execution_changes = E::max_bls_to_execution_changes(); // Use a genesis state with entirely BLS withdrawal credentials. - // Offset keypairs by `validator_count` to create keys distinct from the signing keys. - let validator_keypairs = generate_deterministic_keypairs(validator_count); - let withdrawal_keypairs = (0..validator_count) - .map(|i| Some(generate_deterministic_keypair(i + validator_count))) - .collect::>(); - let withdrawal_credentials = withdrawal_keypairs - .iter() - .map(|keypair| bls_withdrawal_credentials(&keypair.as_ref().unwrap().pk, &spec)) + // Offset keypairs by `VALIDATOR_COUNT` to create keys distinct from the signing keys. + let validator_keypairs = generate_deterministic_keypairs(VALIDATOR_COUNT); + let withdrawal_keypairs = (0..VALIDATOR_COUNT) + .map(|i| Some(generate_deterministic_keypair(i + VALIDATOR_COUNT))) .collect::>(); + + fn withdrawal_credentials_fn<'a>( + index: usize, + _: &'a types::PublicKey, + spec: &'a ChainSpec, + ) -> Hash256 { + // It is a bit inefficient to regenerate the whole keypair here, but this is a workaround. + // `InteropGenesisBuilder` requires the `withdrawal_credentials_fn` to have + // a `'static` lifetime. + let keypair = generate_deterministic_keypair(index + VALIDATOR_COUNT); + bls_withdrawal_credentials(&keypair.pk, spec) + } + let header = generate_genesis_header(&spec, true); - let genesis_state = interop_genesis_state_with_withdrawal_credentials( - &validator_keypairs, - &withdrawal_credentials, - HARNESS_GENESIS_TIME, - Hash256::from_slice(DEFAULT_ETH1_BLOCK_HASH), - header, - &spec, - ) - .unwrap(); + + let genesis_state = InteropGenesisBuilder::new() + .set_opt_execution_payload_header(header) + .set_withdrawal_credentials_fn(Box::new(withdrawal_credentials_fn)) + .build_genesis_state( + &validator_keypairs, + HARNESS_GENESIS_TIME, + Hash256::from_slice(DEFAULT_ETH1_BLOCK_HASH), + &spec, + ) + .unwrap(); let tester = InteractiveTester::::new_with_initializer_and_mutator( Some(spec.clone()), - validator_count, + VALIDATOR_COUNT, Some(Box::new(|harness_builder| { harness_builder .keypairs(validator_keypairs) @@ -421,7 +432,7 @@ async fn bls_to_execution_changes_update_all_around_capella_fork() { let pubkey = &harness.get_withdrawal_keypair(validator_index).pk; // And the wrong secret key. let secret_key = &harness - .get_withdrawal_keypair((validator_index + 1) % validator_count as u64) + .get_withdrawal_keypair((validator_index + 1) % VALIDATOR_COUNT as u64) .sk; harness.make_bls_to_execution_change_with_keys( validator_index, @@ -433,7 +444,7 @@ async fn bls_to_execution_changes_update_all_around_capella_fork() { .collect::>(); // Submit some changes before Capella. Just enough to fill two blocks. - let num_pre_capella = validator_count / 4; + let num_pre_capella = VALIDATOR_COUNT / 4; let blocks_filled_pre_capella = 2; assert_eq!( num_pre_capella, @@ -488,7 +499,7 @@ async fn bls_to_execution_changes_update_all_around_capella_fork() { ); // Add Capella blocks which should be full of BLS to execution changes. - for i in 0..validator_count / max_bls_to_execution_changes { + for i in 0..VALIDATOR_COUNT / max_bls_to_execution_changes { let head_block_root = harness.extend_slots(1).await; let head_block = harness .chain @@ -534,7 +545,7 @@ async fn bls_to_execution_changes_update_all_around_capella_fork() { assert_server_indexed_error( error, 400, - (validator_count..3 * validator_count).collect(), + (VALIDATOR_COUNT..3 * VALIDATOR_COUNT).collect(), ); } }