From 86538b5af93ffea7ce991f2fa498cd71ee16c2a2 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 15 Mar 2024 16:36:12 +1100 Subject: [PATCH 1/2] Address most of Sean's review comments --- beacon_node/beacon_chain/src/attestation_rewards.rs | 11 ++++------- beacon_node/operation_pool/src/lib.rs | 12 +++++------- consensus/state_processing/src/all_caches.rs | 2 +- consensus/state_processing/src/common/base.rs | 12 ------------ consensus/state_processing/src/epoch_cache.rs | 5 ++++- consensus/state_processing/src/metrics.rs | 4 ---- .../src/per_epoch_processing/altair.rs | 2 +- .../src/per_epoch_processing/base.rs | 2 +- consensus/types/src/beacon_state.rs | 6 +++--- consensus/types/src/epoch_cache.rs | 5 +++-- 10 files changed, 22 insertions(+), 39 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_rewards.rs b/beacon_node/beacon_chain/src/attestation_rewards.rs index 1fb90d934cf..299f516fdf8 100644 --- a/beacon_node/beacon_chain/src/attestation_rewards.rs +++ b/beacon_node/beacon_chain/src/attestation_rewards.rs @@ -5,7 +5,7 @@ use eth2::types::ValidatorId; use safe_arith::SafeArith; use serde_utils::quoted_u64::Quoted; use slog::debug; -use state_processing::common::base::get_base_reward_from_effective_balance; +use state_processing::common::base::{self, SqrtTotalActiveBalance}; use state_processing::per_epoch_processing::altair::{ process_inactivity_updates_slow, process_justification_and_finalization, }; @@ -376,15 +376,12 @@ 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()? { let effective_balance = effective_balance_step.safe_mul(spec.effective_balance_increment)?; - let base_reward = get_base_reward_from_effective_balance::( - effective_balance, - total_balances.current_epoch(), - spec, - )?; + let base_reward = + base::get_base_reward(effective_balance, sqrt_total_active_balance, spec)?; // compute ideal head rewards let head = get_attestation_component_delta( diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index 4651ea92988..9de7b8d77ed 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -256,16 +256,14 @@ impl OperationPool { curr_epoch_validity_filter: impl for<'a> FnMut(&AttestationRef<'a, T>) -> bool + Send, spec: &ChainSpec, ) -> Result>, OpPoolError> { - if let BeaconState::Base(_) = state { - // Ok - } else { - // epoch cache must be initialized to fetch base_reward values in the max_cover score - // function. Currently max_cover ignores items on errors. If epoch cache is not - // initialized, this function returns Ok([]). + if !matches!(state, BeaconState::Base(_)) { + // Epoch cache must be initialized to fetch base reward values in the max cover `score` + // function. Currently max cover ignores items on errors. If epoch cache is not + // initialized, this function returns an error. if !is_epoch_cache_initialized(state).map_err(OpPoolError::EpochCacheError)? { return Err(OpPoolError::EpochCacheNotInitialized); } - }; + } // Attestations for the current fork, which may be from the current or previous epoch. let (prev_epoch_key, curr_epoch_key) = CheckpointKey::keys_for_state(state); diff --git a/consensus/state_processing/src/all_caches.rs b/consensus/state_processing/src/all_caches.rs index 41382813f1c..106692c63aa 100644 --- a/consensus/state_processing/src/all_caches.rs +++ b/consensus/state_processing/src/all_caches.rs @@ -46,7 +46,7 @@ impl AllCaches for BeaconState { && self.slashings_cache_is_initialized() && self .epoch_cache() - .check_validity::(current_epoch, epoch_cache_decision_block_root) + .check_validity(current_epoch, epoch_cache_decision_block_root) .is_ok() } } diff --git a/consensus/state_processing/src/common/base.rs b/consensus/state_processing/src/common/base.rs index cb6ea6c04cd..d582e0bea2d 100644 --- a/consensus/state_processing/src/common/base.rs +++ b/consensus/state_processing/src/common/base.rs @@ -28,15 +28,3 @@ pub fn get_base_reward( .safe_div(sqrt_total_active_balance.as_u64())? .safe_div(spec.base_rewards_per_epoch) } - -pub fn get_base_reward_from_effective_balance( - effective_balance: u64, - total_active_balance: u64, - spec: &ChainSpec, -) -> Result { - effective_balance - .safe_mul(spec.base_reward_factor)? - .safe_div(total_active_balance.integer_sqrt())? - .safe_div(spec.base_rewards_per_epoch) - .map_err(Into::into) -} diff --git a/consensus/state_processing/src/epoch_cache.rs b/consensus/state_processing/src/epoch_cache.rs index 954837d1977..05f8d8f79f5 100644 --- a/consensus/state_processing/src/epoch_cache.rs +++ b/consensus/state_processing/src/epoch_cache.rs @@ -19,6 +19,9 @@ impl PreEpochCache { // State root should already have been filled in by `process_slot`, except in the case // of a `partial_state_advance`. let decision_block_root = latest_block_header.canonical_root(); + if decision_block_root.is_zero() { + return Err(EpochCacheError::ZeroDecisionBlock); + } let epoch_key = EpochCacheKey { epoch: state.next_epoch()?, @@ -82,7 +85,7 @@ pub fn is_epoch_cache_initialized( .map_err(EpochCacheError::BeaconState)?; Ok(epoch_cache - .check_validity::(current_epoch, decision_block_root) + .check_validity(current_epoch, decision_block_root) .is_ok()) } diff --git a/consensus/state_processing/src/metrics.rs b/consensus/state_processing/src/metrics.rs index 891689921ad..e163f3b76b8 100644 --- a/consensus/state_processing/src/metrics.rs +++ b/consensus/state_processing/src/metrics.rs @@ -17,10 +17,6 @@ lazy_static! { "beacon_participation_prev_epoch_source_attesting_gwei_total", "Total effective balance (gwei) of validators who attested to the source in the previous epoch" ); - pub static ref PARTICIPATION_PREV_EPOCH_ACTIVE_GWEI_TOTAL: Result = try_create_int_gauge( - "beacon_participation_prev_epoch_active_gwei_total", - "Total effective balance (gwei) of validators active in the previous epoch" - ); /* * Processing metrics */ diff --git a/consensus/state_processing/src/per_epoch_processing/altair.rs b/consensus/state_processing/src/per_epoch_processing/altair.rs index be2bba405bf..d892e905399 100644 --- a/consensus/state_processing/src/per_epoch_processing/altair.rs +++ b/consensus/state_processing/src/per_epoch_processing/altair.rs @@ -78,7 +78,7 @@ pub fn process_epoch( process_sync_committee_updates(state, spec)?; // Rotate the epoch caches to suit the epoch transition. - state.advance_caches(spec)?; + state.advance_caches()?; update_progressive_balances_on_epoch_transition(state, spec)?; Ok(EpochProcessingSummary::Altair { diff --git a/consensus/state_processing/src/per_epoch_processing/base.rs b/consensus/state_processing/src/per_epoch_processing/base.rs index 796379ae756..6b31b3e8d21 100644 --- a/consensus/state_processing/src/per_epoch_processing/base.rs +++ b/consensus/state_processing/src/per_epoch_processing/base.rs @@ -70,7 +70,7 @@ pub fn process_epoch( process_participation_record_updates(state)?; // Rotate the epoch caches to suit the epoch transition. - state.advance_caches(spec)?; + state.advance_caches()?; Ok(EpochProcessingSummary::Base { total_balances: validator_statuses.total_balances, diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 7a5337b7b6f..94b011fb9b0 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -1719,9 +1719,9 @@ impl BeaconState { /// /// This should be used if the `slot` of this state is advanced beyond an epoch boundary. /// - /// Note: this function will not build any new committee caches, but will build the total - /// balance cache if the (new) current committee cache is initialized. - pub fn advance_caches(&mut self, _spec: &ChainSpec) -> Result<(), Error> { + /// Note: this function will not build any new committee caches, nor will it update the total + /// active balance cache. The total active balance cache must be updated separately. + pub fn advance_caches(&mut self) -> Result<(), Error> { self.committee_caches_mut().rotate_left(1); let next = Self::committee_cache_index(RelativeEpoch::Next); diff --git a/consensus/types/src/epoch_cache.rs b/consensus/types/src/epoch_cache.rs index 1f717f2fcaf..60266fe2fb3 100644 --- a/consensus/types/src/epoch_cache.rs +++ b/consensus/types/src/epoch_cache.rs @@ -1,4 +1,4 @@ -use crate::{ActivationQueue, BeaconStateError, ChainSpec, Epoch, EthSpec, Hash256, Slot}; +use crate::{ActivationQueue, BeaconStateError, ChainSpec, Epoch, Hash256, Slot}; use safe_arith::{ArithError, SafeArith}; use std::sync::Arc; @@ -40,6 +40,7 @@ pub struct EpochCacheKey { pub enum EpochCacheError { IncorrectEpoch { cache: Epoch, state: Epoch }, IncorrectDecisionBlock { cache: Hash256, state: Hash256 }, + ZeroDecisionBlock, ValidatorIndexOutOfBounds { validator_index: usize }, EffectiveBalanceOutOfBounds { effective_balance_eth: usize }, InvalidSlot { slot: Slot }, @@ -79,7 +80,7 @@ impl EpochCache { } } - pub fn check_validity( + pub fn check_validity( &self, current_epoch: Epoch, state_decision_root: Hash256, From 22cade34d3e73a49281192310271e4012db2e2d5 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 18 Mar 2024 13:49:50 +1100 Subject: [PATCH 2/2] Simplify total balance cache building --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 +- consensus/state_processing/src/epoch_cache.rs | 2 +- .../src/per_epoch_processing/altair.rs | 2 +- .../src/per_epoch_processing/base.rs | 2 +- consensus/types/src/beacon_state.rs | 36 ++++++++----------- 5 files changed, 18 insertions(+), 26 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index d524853f91e..21a2afc086a 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4953,7 +4953,7 @@ impl BeaconChain { metrics::start_timer(&metrics::BLOCK_PRODUCTION_ATTESTATION_TIMES); // Epoch cache and total balance cache are required for op pool packing. - state.build_total_active_balance_cache_at(state.current_epoch(), &self.spec)?; + state.build_total_active_balance_cache(&self.spec)?; initialize_epoch_cache(&mut state, &self.spec)?; let mut prev_filter_cache = HashMap::new(); diff --git a/consensus/state_processing/src/epoch_cache.rs b/consensus/state_processing/src/epoch_cache.rs index 05f8d8f79f5..be641d2398f 100644 --- a/consensus/state_processing/src/epoch_cache.rs +++ b/consensus/state_processing/src/epoch_cache.rs @@ -104,7 +104,7 @@ pub fn initialize_epoch_cache( .proposer_shuffling_decision_root(Hash256::zero()) .map_err(EpochCacheError::BeaconState)?; - state.build_total_active_balance_cache_at(current_epoch, spec)?; + state.build_total_active_balance_cache(spec)?; let total_active_balance = state.get_total_active_balance_at_epoch(current_epoch)?; // Collect effective balances and compute activation queue. diff --git a/consensus/state_processing/src/per_epoch_processing/altair.rs b/consensus/state_processing/src/per_epoch_processing/altair.rs index d892e905399..144483649f1 100644 --- a/consensus/state_processing/src/per_epoch_processing/altair.rs +++ b/consensus/state_processing/src/per_epoch_processing/altair.rs @@ -30,7 +30,7 @@ pub fn process_epoch( state.build_committee_cache(RelativeEpoch::Previous, spec)?; state.build_committee_cache(RelativeEpoch::Current, spec)?; state.build_committee_cache(RelativeEpoch::Next, spec)?; - state.build_total_active_balance_cache_at(state.current_epoch(), spec)?; + state.build_total_active_balance_cache(spec)?; initialize_epoch_cache(state, spec)?; initialize_progressive_balances_cache::(state, spec)?; diff --git a/consensus/state_processing/src/per_epoch_processing/base.rs b/consensus/state_processing/src/per_epoch_processing/base.rs index 6b31b3e8d21..361bb75f416 100644 --- a/consensus/state_processing/src/per_epoch_processing/base.rs +++ b/consensus/state_processing/src/per_epoch_processing/base.rs @@ -24,7 +24,7 @@ pub fn process_epoch( state.build_committee_cache(RelativeEpoch::Previous, spec)?; state.build_committee_cache(RelativeEpoch::Current, spec)?; state.build_committee_cache(RelativeEpoch::Next, spec)?; - state.build_total_active_balance_cache_at(state.current_epoch(), spec)?; + state.build_total_active_balance_cache(spec)?; initialize_epoch_cache(state, spec)?; // Load the struct we use to assign validators into sets based on their participation. diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 94b011fb9b0..231a8820323 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -1480,19 +1480,13 @@ impl BeaconState { /// /// This method should rarely be invoked because single-pass epoch processing keeps the total /// active balance cache up to date. - pub fn compute_total_active_balance_slow( - &self, - epoch: Epoch, - spec: &ChainSpec, - ) -> Result { - if epoch != self.current_epoch() && epoch != self.next_epoch()? { - return Err(Error::EpochOutOfBounds); - } + pub fn compute_total_active_balance_slow(&self, spec: &ChainSpec) -> Result { + let current_epoch = self.current_epoch(); let mut total_active_balance = 0; for validator in self.validators() { - if validator.is_active_at(epoch) { + if validator.is_active_at(current_epoch) { total_active_balance.safe_add_assign(validator.effective_balance)?; } } @@ -1536,26 +1530,24 @@ impl BeaconState { *self.total_active_balance_mut() = Some((epoch, balance)); } - /// Build the total active balance cache. - pub fn build_total_active_balance_cache_at( - &mut self, - epoch: Epoch, - spec: &ChainSpec, - ) -> Result<(), Error> { - if self.get_total_active_balance_at_epoch(epoch).is_err() { - self.force_build_total_active_balance_cache_at(epoch, spec)?; + /// Build the total active balance cache for the current epoch if it is not already built. + pub fn build_total_active_balance_cache(&mut self, spec: &ChainSpec) -> Result<(), Error> { + if self + .get_total_active_balance_at_epoch(self.current_epoch()) + .is_err() + { + self.force_build_total_active_balance_cache(spec)?; } Ok(()) } /// Build the total active balance cache, even if it is already built. - pub fn force_build_total_active_balance_cache_at( + pub fn force_build_total_active_balance_cache( &mut self, - epoch: Epoch, spec: &ChainSpec, ) -> Result<(), Error> { - let total_active_balance = self.compute_total_active_balance_slow(epoch, spec)?; - *self.total_active_balance_mut() = Some((epoch, total_active_balance)); + let total_active_balance = self.compute_total_active_balance_slow(spec)?; + *self.total_active_balance_mut() = Some((self.current_epoch(), total_active_balance)); Ok(()) } @@ -1685,7 +1677,7 @@ impl BeaconState { } if self.total_active_balance().is_none() && relative_epoch == RelativeEpoch::Current { - self.build_total_active_balance_cache_at(self.current_epoch(), spec)?; + self.build_total_active_balance_cache(spec)?; } Ok(()) }