Address Sean's review comments

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<T: BeaconChainTypes> BeaconChain<T> {
         };
 
         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::<T::EthSpec>(
-                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(

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -4953,7 +4953,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             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();

beacon_node/operation_pool/src/lib.rs

@@ -256,16 +256,14 @@ impl<T: EthSpec> OperationPool<T> {
         curr_epoch_validity_filter: impl for<'a> FnMut(&AttestationRef<'a, T>) -> bool + Send,
         spec: &ChainSpec,
     ) -> Result<Vec<Attestation<T>>, 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);

consensus/state_processing/src/all_caches.rs

@@ -46,7 +46,7 @@ impl<E: EthSpec> AllCaches for BeaconState<E> {
             && self.slashings_cache_is_initialized()
             && self
                 .epoch_cache()
-                .check_validity::<E>(current_epoch, epoch_cache_decision_block_root)
+                .check_validity(current_epoch, epoch_cache_decision_block_root)
                 .is_ok()
     }
 }

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<T: EthSpec>(
-    effective_balance: u64,
-    total_active_balance: u64,
-    spec: &ChainSpec,
-) -> Result<u64, BeaconStateError> {
-    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)
-}

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<E: EthSpec>(
         .map_err(EpochCacheError::BeaconState)?;
 
     Ok(epoch_cache
-        .check_validity::<E>(current_epoch, decision_block_root)
+        .check_validity(current_epoch, decision_block_root)
         .is_ok())
 }
 
@@ -101,7 +104,7 @@ pub fn initialize_epoch_cache<E: EthSpec>(
         .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.

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<IntGauge> = try_create_int_gauge(
-        "beacon_participation_prev_epoch_active_gwei_total",
-        "Total effective balance (gwei) of validators active in the previous epoch"
-    );
     /*
      * Processing metrics
      */

consensus/state_processing/src/per_epoch_processing/altair.rs

@@ -30,7 +30,7 @@ pub fn process_epoch<T: EthSpec>(
     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::<T>(state, spec)?;
 
@@ -78,7 +78,7 @@ pub fn process_epoch<T: EthSpec>(
     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 {

consensus/state_processing/src/per_epoch_processing/base.rs

@@ -24,7 +24,7 @@ pub fn process_epoch<T: EthSpec>(
     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.
@@ -70,7 +70,7 @@ pub fn process_epoch<T: EthSpec>(
     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,

consensus/types/src/beacon_state.rs

@@ -1480,19 +1480,13 @@ impl<T: EthSpec> BeaconState<T> {
     ///
     /// 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<u64, Error> {
-        if epoch != self.current_epoch() && epoch != self.next_epoch()? {
-            return Err(Error::EpochOutOfBounds);
-        }
+    pub fn compute_total_active_balance_slow(&self, spec: &ChainSpec) -> Result<u64, Error> {
+        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<T: EthSpec> BeaconState<T> {
         *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<T: EthSpec> BeaconState<T> {
         }
 
         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(())
     }
@@ -1719,9 +1711,9 @@ impl<T: EthSpec> BeaconState<T> {
     ///
     /// 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);

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<E: EthSpec>(
+    pub fn check_validity(
         &self,
         current_epoch: Epoch,
         state_decision_root: Hash256,