Fix phase0 block reward in rewards API (#5101)

* Added Block Rewards

* added new type

* added enum

* Fix phase0 block reward in rewards API (#4929)

* Merge 'guav00a/proposer-rewards-api'

* Merge unstable

* Revamp phase0 reward API tests

- Add test_rewards_base_slashings (testing #5101)
- Improve fix to not include proposer reward in attestation reward API calculation (#4856)
- Adjust test approach for phase0 tests: Pad with empty epochs to include all rewards in calculation
- Simplify and unify code across all reward tests

* Merge branch 'unstable' into fix-4929

* Merge branch 'unstable' into fix-4929

* Merge remote-tracking branch 'origin/unstable' into fix-4929

* Fix merge fallout

* Remove junk revived in merge

* Address review

- check for attestations with lower inclusion delay
- check for double attestations in block
- add test

* Merge branch 'unstable' into fix-4929

* Merge branch 'unstable' into fix-4929

beacon_node/beacon_chain/src/attestation_rewards.rs

@@ -11,7 +11,7 @@ use state_processing::per_epoch_processing::altair::{
 };
 use state_processing::per_epoch_processing::base::rewards_and_penalties::{
     get_attestation_component_delta, get_attestation_deltas_all, get_attestation_deltas_subset,
-    get_inactivity_penalty_delta, get_inclusion_delay_delta,
+    get_inactivity_penalty_delta, get_inclusion_delay_delta, ProposerRewardCalculation,
 };
 use state_processing::per_epoch_processing::base::validator_statuses::InclusionInfo;
 use state_processing::per_epoch_processing::base::{
@@ -81,13 +81,24 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             self.compute_ideal_rewards_base(&state, &validator_statuses.total_balances)?;
 
         let indices_to_attestation_delta = if validators.is_empty() {
-            get_attestation_deltas_all(&state, &validator_statuses, spec)?
+            get_attestation_deltas_all(
+                &state,
+                &validator_statuses,
+                ProposerRewardCalculation::Exclude,
+                spec,
+            )?
             .into_iter()
             .enumerate()
             .collect()
         } else {
             let validator_indices = Self::validators_ids_to_indices(&mut state, validators)?;
-            get_attestation_deltas_subset(&state, &validator_statuses, &validator_indices, spec)?
+            get_attestation_deltas_subset(
+                &state,
+                &validator_statuses,
+                ProposerRewardCalculation::Exclude,
+                &validator_indices,
+                spec,
+            )?
         };
 
         let mut total_rewards = vec![];

beacon_node/beacon_chain/src/beacon_block_reward.rs

@@ -1,20 +1,25 @@
-use crate::{BeaconChain, BeaconChainError, BeaconChainTypes};
+use crate::{BeaconChain, BeaconChainError, BeaconChainTypes, StateSkipConfig};
+use attesting_indices_base::get_attesting_indices;
 use eth2::lighthouse::StandardBlockReward;
-use operation_pool::RewardCache;
 use safe_arith::SafeArith;
 use slog::error;
+use state_processing::common::attesting_indices_base;
 use state_processing::{
-    common::{get_attestation_participation_flag_indices, get_attesting_indices_from_state},
+    common::{
+        base::{self, SqrtTotalActiveBalance},
+        get_attestation_participation_flag_indices, get_attesting_indices_from_state,
+    },
     epoch_cache::initialize_epoch_cache,
     per_block_processing::{
         altair::sync_committee::compute_sync_aggregate_rewards, get_slashable_indices,
     },
 };
+use std::collections::HashSet;
 use store::{
     consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR},
     RelativeEpoch,
 };
-use types::{AbstractExecPayload, BeaconBlockRef, BeaconState, BeaconStateError, Hash256};
+use types::{AbstractExecPayload, BeaconBlockRef, BeaconState, BeaconStateError, EthSpec};
 
 type BeaconBlockSubRewardValue = u64;
 
@@ -22,7 +27,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     pub fn compute_beacon_block_reward<Payload: AbstractExecPayload<T::EthSpec>>(
         &self,
         block: BeaconBlockRef<'_, T::EthSpec, Payload>,
-        block_root: Hash256,
         state: &mut BeaconState<T::EthSpec>,
     ) -> Result<StandardBlockReward, BeaconChainError> {
         if block.slot() != state.slot() {
@@ -33,7 +37,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         state.build_committee_cache(RelativeEpoch::Current, &self.spec)?;
         initialize_epoch_cache(state, &self.spec)?;
 
-        self.compute_beacon_block_reward_with_cache(block, block_root, state)
+        self.compute_beacon_block_reward_with_cache(block, state)
     }
 
     // This should only be called after a committee cache has been built
@@ -41,7 +45,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     fn compute_beacon_block_reward_with_cache<Payload: AbstractExecPayload<T::EthSpec>>(
         &self,
         block: BeaconBlockRef<'_, T::EthSpec, Payload>,
-        block_root: Hash256,
         state: &BeaconState<T::EthSpec>,
     ) -> Result<StandardBlockReward, BeaconChainError> {
         let proposer_index = block.proposer_index();
@@ -72,7 +75,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             })?;
 
         let block_attestation_reward = if let BeaconState::Base(_) = state {
-            self.compute_beacon_block_attestation_reward_base(block, block_root, state)
+            self.compute_beacon_block_attestation_reward_base(block, state)
                 .map_err(|e| {
                     error!(
                         self.log,
@@ -169,19 +172,85 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     fn compute_beacon_block_attestation_reward_base<Payload: AbstractExecPayload<T::EthSpec>>(
         &self,
         block: BeaconBlockRef<'_, T::EthSpec, Payload>,
-        block_root: Hash256,
         state: &BeaconState<T::EthSpec>,
     ) -> Result<BeaconBlockSubRewardValue, BeaconChainError> {
-        // Call compute_block_reward in the base case
+        // In phase0, rewards for including attestations are awarded at epoch boundaries when the corresponding
-        // Since base does not have sync aggregate, we only grab attesation portion of the returned
+        // attestations are contained in state.previous_epoch_attestations. So, if an attestation within this block has
-        // value
+        // target = previous_epoch, it is directly inserted into previous_epoch_attestations and we need the state at
-        let mut reward_cache = RewardCache::default();
+        // the end of this epoch, or the attestation has target = current_epoch and thus we need the state at the end
-        let block_attestation_reward = self
+        // of the next epoch.
-            .compute_block_reward(block, block_root, state, &mut reward_cache, true)?
+        // We fetch these lazily, as only one might be needed depending on the block's content.
-            .attestation_rewards
+        let mut current_epoch_end = None;
-            .total;
+        let mut next_epoch_end = None;
 
-        Ok(block_attestation_reward)
+        let epoch = block.epoch();
+        let mut block_reward = 0;
+
+        let mut rewarded_attesters = HashSet::new();
+
+        for attestation in block.body().attestations() {
+            let processing_epoch_end = if attestation.data().target.epoch == epoch {
+                let next_epoch_end = match &mut next_epoch_end {
+                    Some(next_epoch_end) => next_epoch_end,
+                    None => {
+                        let state = self.state_at_slot(
+                            epoch.safe_add(1)?.end_slot(T::EthSpec::slots_per_epoch()),
+                            StateSkipConfig::WithoutStateRoots,
+                        )?;
+                        next_epoch_end.get_or_insert(state)
+                    }
+                };
+
+                // If the next epoch end is no longer phase0, no proposer rewards are awarded, as Altair epoch boundry
+                // processing kicks in. We check this here, as we know that current_epoch_end will always be phase0.
+                if !matches!(next_epoch_end, BeaconState::Base(_)) {
+                    continue;
+                }
+
+                next_epoch_end
+            } else if attestation.data().target.epoch == epoch.safe_sub(1)? {
+                match &mut current_epoch_end {
+                    Some(current_epoch_end) => current_epoch_end,
+                    None => {
+                        let state = self.state_at_slot(
+                            epoch.end_slot(T::EthSpec::slots_per_epoch()),
+                            StateSkipConfig::WithoutStateRoots,
+                        )?;
+                        current_epoch_end.get_or_insert(state)
+                    }
+                }
+            } else {
+                return Err(BeaconChainError::BlockRewardAttestationError);
+            };
+
+            let inclusion_delay = state.slot().safe_sub(attestation.data().slot)?.as_u64();
+            let sqrt_total_active_balance =
+                SqrtTotalActiveBalance::new(processing_epoch_end.get_total_active_balance()?);
+            for attester in get_attesting_indices_from_state(state, attestation)? {
+                let validator = processing_epoch_end.get_validator(attester as usize)?;
+                if !validator.slashed
+                    && !rewarded_attesters.contains(&attester)
+                    && !has_earlier_attestation(
+                        state,
+                        processing_epoch_end,
+                        inclusion_delay,
+                        attester,
+                    )?
+                {
+                    let base_reward = base::get_base_reward(
+                        validator.effective_balance,
+                        sqrt_total_active_balance,
+                        &self.spec,
+                    )?;
+                    let proposer_reward =
+                        base_reward.safe_div(self.spec.proposer_reward_quotient)?;
+                    block_reward.safe_add_assign(proposer_reward)?;
+                    rewarded_attesters.insert(attester);
+                }
+            }
+        }
+
+        Ok(block_reward)
     }
 
     fn compute_beacon_block_attestation_reward_altair_deneb<
@@ -244,3 +313,25 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         Ok(total_proposer_reward)
     }
 }
+
+fn has_earlier_attestation<E: EthSpec>(
+    state: &BeaconState<E>,
+    processing_epoch_end: &BeaconState<E>,
+    inclusion_delay: u64,
+    attester: u64,
+) -> Result<bool, BeaconChainError> {
+    if inclusion_delay > 1 {
+        for epoch_att in processing_epoch_end.previous_epoch_attestations()? {
+            if epoch_att.inclusion_delay < inclusion_delay {
+                let committee =
+                    state.get_beacon_committee(epoch_att.data.slot, epoch_att.data.index)?;
+                let earlier_attesters =
+                    get_attesting_indices::<E>(committee.committee, &epoch_att.aggregation_bits)?;
+                if earlier_attesters.contains(&attester) {
+                    return Ok(true);
+                }
+            }
+        }
+    }
+    Ok(false)
+}

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -5639,7 +5639,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         let mut ctxt = ConsensusContext::new(block.slot());
 
         let consensus_block_value = self
-            .compute_beacon_block_reward(block.message(), Hash256::zero(), &mut state)
+            .compute_beacon_block_reward(block.message(), &mut state)
             .map(|reward| reward.total)
             .unwrap_or(0);
 

beacon_node/beacon_chain/tests/rewards.rs

@@ -1,20 +1,22 @@
 #![cfg(test)]
 
-use std::collections::HashMap;
+use beacon_chain::block_verification_types::AsBlock;
-use std::sync::LazyLock;
-
 use beacon_chain::test_utils::{
     generate_deterministic_keypairs, BeaconChainHarness, EphemeralHarnessType,
 };
 use beacon_chain::{
     test_utils::{AttestationStrategy, BlockStrategy, RelativeSyncCommittee},
     types::{Epoch, EthSpec, Keypair, MinimalEthSpec},
+    BlockError, ChainConfig, StateSkipConfig, WhenSlotSkipped,
 };
 use eth2::lighthouse::attestation_rewards::TotalAttestationRewards;
 use eth2::lighthouse::StandardAttestationRewards;
 use eth2::types::ValidatorId;
-use types::beacon_state::Error as BeaconStateError;
+use state_processing::{BlockReplayError, BlockReplayer};
-use types::{BeaconState, ChainSpec, ForkName, Slot};
+use std::array::IntoIter;
+use std::collections::HashMap;
+use std::sync::{Arc, LazyLock};
+use types::{ChainSpec, ForkName, Slot};
 
 pub const VALIDATOR_COUNT: usize = 64;
 
@@ -24,10 +26,16 @@ static KEYPAIRS: LazyLock<Vec<Keypair>> =
     LazyLock::new(|| generate_deterministic_keypairs(VALIDATOR_COUNT));
 
 fn get_harness(spec: ChainSpec) -> BeaconChainHarness<EphemeralHarnessType<E>> {
+    let chain_config = ChainConfig {
+        reconstruct_historic_states: true,
+        ..Default::default()
+    };
+
     let harness = BeaconChainHarness::builder(E::default())
         .spec(spec)
         .keypairs(KEYPAIRS.to_vec())
         .fresh_ephemeral_store()
+        .chain_config(chain_config)
         .build();
 
     harness.advance_slot();
@@ -37,9 +45,7 @@ fn get_harness(spec: ChainSpec) -> BeaconChainHarness<EphemeralHarnessType<E>> {
 
 #[tokio::test]
 async fn test_sync_committee_rewards() {
-    let mut spec = E::default_spec();
+    let spec = ForkName::Altair.make_genesis_spec(E::default_spec());
-    spec.altair_fork_epoch = Some(Epoch::new(0));
-
     let harness = get_harness(spec);
     let num_block_produced = E::slots_per_epoch();
 
@@ -126,121 +132,63 @@ async fn test_sync_committee_rewards() {
 }
 
 #[tokio::test]
-async fn test_verify_attestation_rewards_base() {
+async fn test_rewards_base() {
-    let harness = get_harness(E::default_spec());
+    let spec = ForkName::Base.make_genesis_spec(E::default_spec());
+    let harness = get_harness(spec);
+    let initial_balances = harness.get_current_state().balances().to_vec();
 
-    // epoch 0 (N), only two thirds of validators vote.
-    let two_thirds = (VALIDATOR_COUNT / 3) * 2;
-    let two_thirds_validators: Vec<usize> = (0..two_thirds).collect();
     harness
-        .extend_chain(
+        .extend_slots(E::slots_per_epoch() as usize * 2 - 1)
-            E::slots_per_epoch() as usize,
-            BlockStrategy::OnCanonicalHead,
-            AttestationStrategy::SomeValidators(two_thirds_validators),
-        )
         .await;
 
-    let initial_balances: Vec<u64> = harness.get_current_state().balances().to_vec();
+    check_all_base_rewards(&harness, initial_balances).await;
-
-    // extend slots to beginning of epoch N + 2
-    harness.extend_slots(E::slots_per_epoch() as usize).await;
-
-    // compute reward deltas for all validators in epoch N
-    let StandardAttestationRewards {
-        ideal_rewards,
-        total_rewards,
-    } = harness
-        .chain
-        .compute_attestation_rewards(Epoch::new(0), vec![])
-        .unwrap();
-
-    // assert no inactivity penalty for both ideal rewards and individual validators
-    assert!(ideal_rewards.iter().all(|reward| reward.inactivity == 0));
-    assert!(total_rewards.iter().all(|reward| reward.inactivity == 0));
-
-    // apply attestation rewards to initial balances
-    let expected_balances = apply_attestation_rewards(&initial_balances, total_rewards);
-
-    // verify expected balances against actual balances
-    let balances: Vec<u64> = harness.get_current_state().balances().to_vec();
-    assert_eq!(expected_balances, balances);
 }
 
 #[tokio::test]
-async fn test_verify_attestation_rewards_base_inactivity_leak() {
+async fn test_rewards_base_inactivity_leak() {
-    let spec = E::default_spec();
+    let spec = ForkName::Base.make_genesis_spec(E::default_spec());
     let harness = get_harness(spec.clone());
+    let initial_balances = harness.get_current_state().balances().to_vec();
 
     let half = VALIDATOR_COUNT / 2;
     let half_validators: Vec<usize> = (0..half).collect();
     // target epoch is the epoch where the chain enters inactivity leak
     let target_epoch = &spec.min_epochs_to_inactivity_penalty + 1;
 
-    // advance until beginning of epoch N + 1 and get balances
+    // advance until end of target epoch
     harness
-        .extend_chain(
+        .extend_slots_some_validators(
-            (E::slots_per_epoch() * (target_epoch + 1)) as usize,
+            ((E::slots_per_epoch() * target_epoch) - 1) as usize,
-            BlockStrategy::OnCanonicalHead,
+            half_validators.clone(),
-            AttestationStrategy::SomeValidators(half_validators.clone()),
         )
         .await;
-    let initial_balances: Vec<u64> = harness.get_current_state().balances().to_vec();
 
-    // extend slots to beginning of epoch N + 2
+    check_all_base_rewards(&harness, initial_balances).await;
-    harness.advance_slot();
-    harness
-        .extend_chain(
-            E::slots_per_epoch() as usize,
-            BlockStrategy::OnCanonicalHead,
-            AttestationStrategy::SomeValidators(half_validators),
-        )
-        .await;
-    let _slot = harness.get_current_slot();
-
-    // 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 inactivity penalty for both ideal rewards and individual validators
-    assert!(ideal_rewards.iter().all(|reward| reward.inactivity < 0));
-    assert!(total_rewards.iter().all(|reward| reward.inactivity < 0));
-
-    // apply attestation rewards to initial balances
-    let expected_balances = apply_attestation_rewards(&initial_balances, total_rewards);
-
-    // verify expected balances against actual balances
-    let balances: Vec<u64> = harness.get_current_state().balances().to_vec();
-    assert_eq!(expected_balances, balances);
 }
 
 #[tokio::test]
-async fn test_verify_attestation_rewards_base_inactivity_leak_justification_epoch() {
+async fn test_rewards_base_inactivity_leak_justification_epoch() {
-    let spec = E::default_spec();
+    let spec = ForkName::Base.make_genesis_spec(E::default_spec());
     let harness = get_harness(spec.clone());
+    let initial_balances = harness.get_current_state().balances().to_vec();
 
     let half = VALIDATOR_COUNT / 2;
     let half_validators: Vec<usize> = (0..half).collect();
     // target epoch is the epoch where the chain enters inactivity leak
-    let mut target_epoch = &spec.min_epochs_to_inactivity_penalty + 2;
+    let mut target_epoch = &spec.min_epochs_to_inactivity_penalty + 1;
 
-    // advance until beginning of epoch N + 2
+    // advance until end of target epoch
     harness
         .extend_chain(
-            (E::slots_per_epoch() * (target_epoch + 1)) as usize,
+            ((E::slots_per_epoch() * target_epoch) - 1) as usize,
             BlockStrategy::OnCanonicalHead,
             AttestationStrategy::SomeValidators(half_validators.clone()),
         )
         .await;
 
-    // advance to create first justification epoch and get initial balances
+    // advance to create first justification epoch
     harness.extend_slots(E::slots_per_epoch() as usize).await;
     target_epoch += 1;
-    let initial_balances: Vec<u64> = harness.get_current_state().balances().to_vec();
 
     // assert previous_justified_checkpoint matches 0 as we were in inactivity leak from beginning
     assert_eq!(
@@ -252,9 +200,11 @@ async fn test_verify_attestation_rewards_base_inactivity_leak_justification_epoc
             .as_u64()
     );
 
-    // extend slots to beginning of epoch N + 1
+    // extend slots to end of epoch target_epoch + 2
     harness.extend_slots(E::slots_per_epoch() as usize).await;
 
+    check_all_base_rewards(&harness, initial_balances).await;
+
     // assert target epoch and previous_justified_checkpoint match
     assert_eq!(
         target_epoch,
@@ -264,31 +214,94 @@ async fn test_verify_attestation_rewards_base_inactivity_leak_justification_epoc
             .epoch
             .as_u64()
     );
-
-    // 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 we successfully get ideal rewards for justified epoch out of inactivity leak
-    assert!(ideal_rewards
-        .iter()
-        .all(|reward| reward.head > 0 && reward.target > 0 && reward.source > 0));
-
-    // apply attestation rewards to initial balances
-    let expected_balances = apply_attestation_rewards(&initial_balances, total_rewards);
-
-    // verify expected balances against actual balances
-    let balances: Vec<u64> = harness.get_current_state().balances().to_vec();
-    assert_eq!(expected_balances, balances);
 }
 
 #[tokio::test]
-async fn test_verify_attestation_rewards_altair() {
+async fn test_rewards_base_slashings() {
+    let spec = ForkName::Base.make_genesis_spec(E::default_spec());
+    let harness = get_harness(spec);
+    let mut initial_balances = harness.get_current_state().balances().to_vec();
+
+    harness
+        .extend_slots(E::slots_per_epoch() as usize - 1)
+        .await;
+
+    harness.add_attester_slashing(vec![0]).unwrap();
+    let slashed_balance = initial_balances.get_mut(0).unwrap();
+    *slashed_balance -= *slashed_balance / harness.spec.min_slashing_penalty_quotient;
+
+    harness.extend_slots(E::slots_per_epoch() as usize).await;
+
+    check_all_base_rewards(&harness, initial_balances).await;
+}
+
+#[tokio::test]
+async fn test_rewards_base_multi_inclusion() {
+    let spec = ForkName::Base.make_genesis_spec(E::default_spec());
+    let harness = get_harness(spec);
+    let initial_balances = harness.get_current_state().balances().to_vec();
+
+    harness.extend_slots(2).await;
+
+    let prev_block = harness.chain.head_beacon_block();
+
+    harness.extend_slots(1).await;
+
+    harness.advance_slot();
+    let slot = harness.get_current_slot();
+    let mut block =
+        // pin to reduce stack size for clippy
+        Box::pin(
+            harness.make_block_with_modifier(harness.get_current_state(), slot, |block| {
+                // add one attestation from the same block
+                let attestations = &mut block.body_base_mut().unwrap().attestations;
+                attestations
+                    .push(attestations.first().unwrap().clone())
+                    .unwrap();
+
+                // add one attestation from the previous block
+                let attestation = prev_block
+                    .as_block()
+                    .message_base()
+                    .unwrap()
+                    .body
+                    .attestations
+                    .first()
+                    .unwrap()
+                    .clone();
+                attestations.push(attestation).unwrap();
+            }),
+        )
+        .await
+        .0;
+
+    // funky hack: on first try, the state root will mismatch due to our modification
+    // thankfully, the correct state root is reported back, so we just take that one :^)
+    // there probably is a better way...
+    let Err(BlockError::StateRootMismatch { local, .. }) = harness
+        .process_block(slot, block.0.canonical_root(), block.clone())
+        .await
+    else {
+        panic!("unexpected match of state root");
+    };
+    let mut new_block = block.0.message_base().unwrap().clone();
+    new_block.state_root = local;
+    block.0 = Arc::new(harness.sign_beacon_block(new_block.into(), &harness.get_current_state()));
+    harness
+        .process_block(slot, block.0.canonical_root(), block.clone())
+        .await
+        .unwrap();
+
+    harness
+        .extend_slots(E::slots_per_epoch() as usize * 2 - 4)
+        .await;
+
+    // pin to reduce stack size for clippy
+    Box::pin(check_all_base_rewards(&harness, initial_balances)).await;
+}
+
+#[tokio::test]
+async fn test_rewards_altair() {
     let spec = ForkName::Altair.make_genesis_spec(E::default_spec());
     let harness = get_harness(spec.clone());
     let target_epoch = 0;
@@ -297,11 +310,11 @@ async fn test_verify_attestation_rewards_altair() {
     harness
         .extend_slots((E::slots_per_epoch() * (target_epoch + 1)) as usize)
         .await;
-    let initial_balances: Vec<u64> = harness.get_current_state().balances().to_vec();
+    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<u64, u64> = HashMap::new();
+    let mut proposal_rewards_map = HashMap::new();
-    let mut sync_committee_rewards_map: HashMap<u64, i64> = 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);
@@ -311,19 +324,13 @@ async fn test_verify_attestation_rewards_altair() {
             harness.make_block_return_pre_state(state, slot).await;
         let beacon_block_reward = harness
             .chain
-            .compute_beacon_block_reward(
+            .compute_beacon_block_reward(signed_block.message(), &mut state)
-                signed_block.message(),
-                signed_block.canonical_root(),
-                &mut state,
-            )
             .unwrap();
 
         let total_proposer_reward = proposal_rewards_map
-            .get(&beacon_block_reward.proposer_index)
+            .entry(beacon_block_reward.proposer_index)
-            .unwrap_or(&0u64)
+            .or_insert(0);
-            + beacon_block_reward.total;
+        *total_proposer_reward += beacon_block_reward.total as i64;
-
-        proposal_rewards_map.insert(beacon_block_reward.proposer_index, total_proposer_reward);
 
         // calculate sync committee rewards / penalties
         let reward_payload = harness
@@ -331,13 +338,12 @@ async fn test_verify_attestation_rewards_altair() {
             .compute_sync_committee_rewards(signed_block.message(), &mut state)
             .unwrap();
 
-        reward_payload.iter().for_each(|reward| {
+        for reward in reward_payload {
-            let mut amount = *sync_committee_rewards_map
+            let total_sync_reward = sync_committee_rewards_map
-                .get(&reward.validator_index)
+                .entry(reward.validator_index)
-                .unwrap_or(&0);
+                .or_insert(0);
-            amount += reward.reward;
+            *total_sync_reward += reward.reward;
-            sync_committee_rewards_map.insert(reward.validator_index, amount);
+        }
-        });
 
         harness.extend_slots(1).await;
     }
@@ -357,10 +363,9 @@ async fn test_verify_attestation_rewards_altair() {
         .all(|reward| reward.head > 0 && reward.target > 0 && reward.source > 0));
 
     // apply attestation, proposal, and sync committee rewards and penalties to initial balances
-    let expected_balances = apply_attestation_rewards(&initial_balances, total_rewards);
+    apply_attestation_rewards(&mut expected_balances, total_rewards);
-    let expected_balances = apply_beacon_block_rewards(&proposal_rewards_map, expected_balances);
+    apply_other_rewards(&mut expected_balances, &proposal_rewards_map);
-    let expected_balances =
+    apply_other_rewards(&mut expected_balances, &sync_committee_rewards_map);
-        apply_sync_committee_rewards(&sync_committee_rewards_map, expected_balances);
 
     // verify expected balances against actual balances
     let balances: Vec<u64> = harness.get_current_state().balances().to_vec();
@@ -369,7 +374,7 @@ async fn test_verify_attestation_rewards_altair() {
 }
 
 #[tokio::test]
-async fn test_verify_attestation_rewards_altair_inactivity_leak() {
+async fn test_rewards_altair_inactivity_leak() {
     let spec = ForkName::Altair.make_genesis_spec(E::default_spec());
     let harness = get_harness(spec.clone());
 
@@ -385,11 +390,11 @@ async fn test_verify_attestation_rewards_altair_inactivity_leak() {
             half_validators.clone(),
         )
         .await;
-    let initial_balances: Vec<u64> = harness.get_current_state().balances().to_vec();
+    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<u64, u64> = HashMap::new();
+    let mut proposal_rewards_map = HashMap::new();
-    let mut sync_committee_rewards_map: HashMap<u64, i64> = 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);
@@ -399,19 +404,13 @@ async fn test_verify_attestation_rewards_altair_inactivity_leak() {
             harness.make_block_return_pre_state(state, slot).await;
         let beacon_block_reward = harness
             .chain
-            .compute_beacon_block_reward(
+            .compute_beacon_block_reward(signed_block.message(), &mut state)
-                signed_block.message(),
-                signed_block.canonical_root(),
-                &mut state,
-            )
             .unwrap();
 
         let total_proposer_reward = proposal_rewards_map
-            .get(&beacon_block_reward.proposer_index)
+            .entry(beacon_block_reward.proposer_index)
-            .unwrap_or(&0u64)
+            .or_insert(0i64);
-            + beacon_block_reward.total;
+        *total_proposer_reward += beacon_block_reward.total as i64;
-
-        proposal_rewards_map.insert(beacon_block_reward.proposer_index, total_proposer_reward);
 
         // calculate sync committee rewards / penalties
         let reward_payload = harness
@@ -419,13 +418,12 @@ async fn test_verify_attestation_rewards_altair_inactivity_leak() {
             .compute_sync_committee_rewards(signed_block.message(), &mut state)
             .unwrap();
 
-        reward_payload.iter().for_each(|reward| {
+        for reward in reward_payload {
-            let mut amount = *sync_committee_rewards_map
+            let total_sync_reward = sync_committee_rewards_map
-                .get(&reward.validator_index)
+                .entry(reward.validator_index)
-                .unwrap_or(&0);
+                .or_insert(0);
-            amount += reward.reward;
+            *total_sync_reward += reward.reward;
-            sync_committee_rewards_map.insert(reward.validator_index, amount);
+        }
-        });
 
         harness
             .extend_slots_some_validators(1, half_validators.clone())
@@ -451,10 +449,9 @@ async fn test_verify_attestation_rewards_altair_inactivity_leak() {
         .all(|reward| reward.inactivity < 0));
 
     // apply attestation, proposal, and sync committee rewards and penalties to initial balances
-    let expected_balances = apply_attestation_rewards(&initial_balances, total_rewards);
+    apply_attestation_rewards(&mut expected_balances, total_rewards);
-    let expected_balances = apply_beacon_block_rewards(&proposal_rewards_map, expected_balances);
+    apply_other_rewards(&mut expected_balances, &proposal_rewards_map);
-    let expected_balances =
+    apply_other_rewards(&mut expected_balances, &sync_committee_rewards_map);
-        apply_sync_committee_rewards(&sync_committee_rewards_map, expected_balances);
 
     // verify expected balances against actual balances
     let balances: Vec<u64> = harness.get_current_state().balances().to_vec();
@@ -463,7 +460,7 @@ async fn test_verify_attestation_rewards_altair_inactivity_leak() {
 }
 
 #[tokio::test]
-async fn test_verify_attestation_rewards_altair_inactivity_leak_justification_epoch() {
+async fn test_rewards_altair_inactivity_leak_justification_epoch() {
     let spec = ForkName::Altair.make_genesis_spec(E::default_spec());
     let harness = get_harness(spec.clone());
 
@@ -491,11 +488,11 @@ async fn test_verify_attestation_rewards_altair_inactivity_leak_justification_ep
     // advance for first justification epoch and get balances
     harness.extend_slots(E::slots_per_epoch() as usize).await;
     target_epoch += 1;
-    let initial_balances: Vec<u64> = harness.get_current_state().balances().to_vec();
+    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<u64, u64> = HashMap::new();
+    let mut proposal_rewards_map = HashMap::new();
-    let mut sync_committee_rewards_map: HashMap<u64, i64> = 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);
@@ -505,19 +502,13 @@ async fn test_verify_attestation_rewards_altair_inactivity_leak_justification_ep
             harness.make_block_return_pre_state(state, slot).await;
         let beacon_block_reward = harness
             .chain
-            .compute_beacon_block_reward(
+            .compute_beacon_block_reward(signed_block.message(), &mut state)
-                signed_block.message(),
-                signed_block.canonical_root(),
-                &mut state,
-            )
             .unwrap();
 
         let total_proposer_reward = proposal_rewards_map
-            .get(&beacon_block_reward.proposer_index)
+            .entry(beacon_block_reward.proposer_index)
-            .unwrap_or(&0u64)
+            .or_insert(0);
-            + beacon_block_reward.total;
+        *total_proposer_reward += beacon_block_reward.total as i64;
-
-        proposal_rewards_map.insert(beacon_block_reward.proposer_index, total_proposer_reward);
 
         // calculate sync committee rewards / penalties
         let reward_payload = harness
@@ -525,13 +516,12 @@ async fn test_verify_attestation_rewards_altair_inactivity_leak_justification_ep
             .compute_sync_committee_rewards(signed_block.message(), &mut state)
             .unwrap();
 
-        reward_payload.iter().for_each(|reward| {
+        for reward in reward_payload {
-            let mut amount = *sync_committee_rewards_map
+            let total_sync_reward = sync_committee_rewards_map
-                .get(&reward.validator_index)
+                .entry(reward.validator_index)
-                .unwrap_or(&0);
+                .or_insert(0);
-            amount += reward.reward;
+            *total_sync_reward += reward.reward;
-            sync_committee_rewards_map.insert(reward.validator_index, amount);
+        }
-        });
 
         harness.extend_slots(1).await;
     }
@@ -561,10 +551,9 @@ async fn test_verify_attestation_rewards_altair_inactivity_leak_justification_ep
         .all(|reward| reward.head > 0 && reward.target > 0 && reward.source > 0));
 
     // apply attestation, proposal, and sync committee rewards and penalties to initial balances
-    let expected_balances = apply_attestation_rewards(&initial_balances, total_rewards);
+    apply_attestation_rewards(&mut expected_balances, total_rewards);
-    let expected_balances = apply_beacon_block_rewards(&proposal_rewards_map, expected_balances);
+    apply_other_rewards(&mut expected_balances, &proposal_rewards_map);
-    let expected_balances =
+    apply_other_rewards(&mut expected_balances, &sync_committee_rewards_map);
-        apply_sync_committee_rewards(&sync_committee_rewards_map, expected_balances);
 
     // verify expected balances against actual balances
     let balances: Vec<u64> = harness.get_current_state().balances().to_vec();
@@ -572,109 +561,130 @@ async fn test_verify_attestation_rewards_altair_inactivity_leak_justification_ep
 }
 
 #[tokio::test]
-async fn test_verify_attestation_rewards_base_subset_only() {
+async fn test_rewards_base_subset_only() {
-    let harness = get_harness(E::default_spec());
+    let spec = ForkName::Base.make_genesis_spec(E::default_spec());
+    let harness = get_harness(spec);
+    let initial_balances = harness.get_current_state().balances().to_vec();
+
+    // a subset of validators to compute attestation rewards for
+    let validators_subset = (0..16).chain(56..64).collect::<Vec<_>>();
 
     // epoch 0 (N), only two thirds of validators vote.
     let two_thirds = (VALIDATOR_COUNT / 3) * 2;
     let two_thirds_validators: Vec<usize> = (0..two_thirds).collect();
     harness
-        .extend_chain(
+        .extend_slots_some_validators(E::slots_per_epoch() as usize, two_thirds_validators.clone())
-            E::slots_per_epoch() as usize,
-            BlockStrategy::OnCanonicalHead,
-            AttestationStrategy::SomeValidators(two_thirds_validators),
-        )
         .await;
 
-    // a small subset of validators to compute attestation rewards for
+    check_all_base_rewards_for_subset(&harness, initial_balances, validators_subset).await;
-    let validators_subset = [0, VALIDATOR_COUNT / 2, VALIDATOR_COUNT - 1];
+}
 
-    // capture balances before transitioning to N + 2
+async fn check_all_base_rewards(
-    let initial_balances = get_validator_balances(harness.get_current_state(), &validators_subset);
+    harness: &BeaconChainHarness<EphemeralHarnessType<E>>,
+    balances: Vec<u64>,
+) {
+    check_all_base_rewards_for_subset(harness, balances, vec![]).await;
+}
 
-    // extend slots to beginning of epoch N + 2
+async fn check_all_base_rewards_for_subset(
-    harness.extend_slots(E::slots_per_epoch() as usize).await;
+    harness: &BeaconChainHarness<EphemeralHarnessType<E>>,
-
+    mut balances: Vec<u64>,
-    let validators_subset_ids: Vec<ValidatorId> = validators_subset
+    validator_subset: Vec<u64>,
-        .into_iter()
+) {
-        .map(|idx| ValidatorId::Index(idx as u64))
+    let validator_subset_ids: Vec<ValidatorId> = validator_subset
+        .iter()
+        .map(|&idx| ValidatorId::Index(idx))
         .collect();
 
-    // compute reward deltas for the subset of validators in epoch N
+    // capture the amount of epochs generated by the caller
-    let StandardAttestationRewards {
+    let epochs = harness.get_current_slot().epoch(E::slots_per_epoch()) + 1;
-        ideal_rewards: _,
+
-        total_rewards,
+    // advance two empty epochs to ensure balances are updated by the epoch boundaries
-    } = harness
+    for _ in 0..E::slots_per_epoch() * 2 {
+        harness.advance_slot();
+    }
+    // fill one slot to ensure state is updated
+    harness.extend_slots(1).await;
+
+    // calculate proposal awards
+    let mut proposal_rewards_map = HashMap::new();
+    for slot in 1..(E::slots_per_epoch() * epochs.as_u64()) {
+        if let Some(block) = harness
             .chain
-        .compute_attestation_rewards(Epoch::new(0), validators_subset_ids)
+            .block_at_slot(Slot::new(slot), WhenSlotSkipped::None)
+            .unwrap()
+        {
+            let parent_state = harness
+                .chain
+                .state_at_slot(Slot::new(slot - 1), StateSkipConfig::WithoutStateRoots)
                 .unwrap();
 
-    // apply attestation rewards to initial balances
+            let mut pre_state = BlockReplayer::<E, BlockReplayError, IntoIter<_, 0>>::new(
-    let expected_balances = apply_attestation_rewards(&initial_balances, total_rewards);
+                parent_state,
+                &harness.spec,
+            )
+            .no_signature_verification()
+            .minimal_block_root_verification()
+            .apply_blocks(vec![], Some(block.slot()))
+            .unwrap()
+            .into_state();
+
+            let beacon_block_reward = harness
+                .chain
+                .compute_beacon_block_reward(block.message(), &mut pre_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;
+        }
+    }
+    apply_other_rewards(&mut balances, &proposal_rewards_map);
+
+    for epoch in 0..epochs.as_u64() {
+        // compute reward deltas in epoch
+        let total_rewards = harness
+            .chain
+            .compute_attestation_rewards(Epoch::new(epoch), validator_subset_ids.clone())
+            .unwrap()
+            .total_rewards;
+
+        // apply attestation rewards to balances
+        apply_attestation_rewards(&mut balances, total_rewards);
+    }
 
     // verify expected balances against actual balances
-    let balances = get_validator_balances(harness.get_current_state(), &validators_subset);
+    let actual_balances: Vec<u64> = harness.get_current_state().balances().to_vec();
-    assert_eq!(expected_balances, balances);
+    if validator_subset.is_empty() {
+        assert_eq!(balances, actual_balances);
+    } else {
+        for validator in validator_subset {
+            assert_eq!(
+                balances[validator as usize],
+                actual_balances[validator as usize]
+            );
+        }
+    }
 }
 
 /// Apply a vec of `TotalAttestationRewards` to initial balances, and return
 fn apply_attestation_rewards(
-    initial_balances: &[u64],
+    balances: &mut [u64],
     attestation_rewards: Vec<TotalAttestationRewards>,
-) -> Vec<u64> {
+) {
-    initial_balances
+    for rewards in attestation_rewards {
-        .iter()
+        let balance = balances.get_mut(rewards.validator_index as usize).unwrap();
-        .zip(attestation_rewards)
+        *balance = (*balance as i64
-        .map(|(&initial_balance, rewards)| {
-            let expected_balance = initial_balance as i64
             + rewards.head
             + rewards.source
             + rewards.target
             + rewards.inclusion_delay.map(|q| q.value).unwrap_or(0) as i64
-                + rewards.inactivity;
+            + rewards.inactivity) as u64;
-            expected_balance as u64
+    }
-        })
-        .collect::<Vec<u64>>()
 }
 
-fn get_validator_balances(state: BeaconState<E>, validators: &[usize]) -> Vec<u64> {
+fn apply_other_rewards(balances: &mut [u64], rewards_map: &HashMap<u64, i64>) {
-    validators
+    for (i, balance) in balances.iter_mut().enumerate() {
-        .iter()
+        *balance = balance.saturating_add_signed(*rewards_map.get(&(i as u64)).unwrap_or(&0));
-        .flat_map(|&id| {
-            state
-                .balances()
-                .get(id)
-                .cloned()
-                .ok_or(BeaconStateError::BalancesOutOfBounds(id))
-        })
-        .collect()
     }
-
-fn apply_beacon_block_rewards(
-    proposal_rewards_map: &HashMap<u64, u64>,
-    expected_balances: Vec<u64>,
-) -> Vec<u64> {
-    let calculated_balances = expected_balances
-        .iter()
-        .enumerate()
-        .map(|(i, balance)| balance + proposal_rewards_map.get(&(i as u64)).unwrap_or(&0u64))
-        .collect();
-
-    calculated_balances
-}
-
-fn apply_sync_committee_rewards(
-    sync_committee_rewards_map: &HashMap<u64, i64>,
-    expected_balances: Vec<u64>,
-) -> Vec<u64> {
-    let calculated_balances = expected_balances
-        .iter()
-        .enumerate()
-        .map(|(i, balance)| {
-            (*balance as i64 + sync_committee_rewards_map.get(&(i as u64)).unwrap_or(&0i64))
-                .unsigned_abs()
-        })
-        .collect();
-
-    calculated_balances
 }

beacon_node/http_api/src/standard_block_rewards.rs

@@ -15,12 +15,10 @@ pub fn compute_beacon_block_rewards<T: BeaconChainTypes>(
 
     let block_ref = block.message();
 
-    let block_root = block.canonical_root();
-
     let mut state = get_state_before_applying_block(chain.clone(), &block)?;
 
     let rewards = chain
-        .compute_beacon_block_reward(block_ref, block_root, &mut state)
+        .compute_beacon_block_reward(block_ref, &mut state)
         .map_err(beacon_chain_error)?;
 
     Ok((rewards, execution_optimistic, finalized))

consensus/state_processing/src/per_epoch_processing/base/rewards_and_penalties.rs

@@ -45,6 +45,12 @@ impl AttestationDelta {
     }
 }
 
+#[derive(Debug)]
+pub enum ProposerRewardCalculation {
+    Include,
+    Exclude,
+}
+
 /// Apply attester and proposer rewards.
 pub fn process_rewards_and_penalties<E: EthSpec>(
     state: &mut BeaconState<E>,
@@ -62,7 +68,12 @@ pub fn process_rewards_and_penalties<E: EthSpec>(
         return Err(Error::ValidatorStatusesInconsistent);
     }
 
-    let deltas = get_attestation_deltas_all(state, validator_statuses, spec)?;
+    let deltas = get_attestation_deltas_all(
+        state,
+        validator_statuses,
+        ProposerRewardCalculation::Include,
+        spec,
+    )?;
 
     // Apply the deltas, erroring on overflow above but not on overflow below (saturating at 0
     // instead).
@@ -79,9 +90,10 @@ pub fn process_rewards_and_penalties<E: EthSpec>(
 pub fn get_attestation_deltas_all<E: EthSpec>(
     state: &BeaconState<E>,
     validator_statuses: &ValidatorStatuses,
+    proposer_reward: ProposerRewardCalculation,
     spec: &ChainSpec,
 ) -> Result<Vec<AttestationDelta>, Error> {
-    get_attestation_deltas(state, validator_statuses, None, spec)
+    get_attestation_deltas(state, validator_statuses, proposer_reward, None, spec)
 }
 
 /// Apply rewards for participation in attestations during the previous epoch, and only compute
@@ -89,10 +101,18 @@ pub fn get_attestation_deltas_all<E: EthSpec>(
 pub fn get_attestation_deltas_subset<E: EthSpec>(
     state: &BeaconState<E>,
     validator_statuses: &ValidatorStatuses,
+    proposer_reward: ProposerRewardCalculation,
     validators_subset: &Vec<usize>,
     spec: &ChainSpec,
 ) -> Result<Vec<(usize, AttestationDelta)>, Error> {
-    get_attestation_deltas(state, validator_statuses, Some(validators_subset), spec).map(|deltas| {
+    get_attestation_deltas(
+        state,
+        validator_statuses,
+        proposer_reward,
+        Some(validators_subset),
+        spec,
+    )
+    .map(|deltas| {
         deltas
             .into_iter()
             .enumerate()
@@ -109,6 +129,7 @@ pub fn get_attestation_deltas_subset<E: EthSpec>(
 fn get_attestation_deltas<E: EthSpec>(
     state: &BeaconState<E>,
     validator_statuses: &ValidatorStatuses,
+    proposer_reward: ProposerRewardCalculation,
     maybe_validators_subset: Option<&Vec<usize>>,
     spec: &ChainSpec,
 ) -> Result<Vec<AttestationDelta>, Error> {
@@ -169,6 +190,7 @@ fn get_attestation_deltas<E: EthSpec>(
                 .combine(inactivity_penalty_delta)?;
         }
 
+        if let ProposerRewardCalculation::Include = proposer_reward {
             if let Some((proposer_index, proposer_delta)) = proposer_delta {
                 if include_validator_delta(proposer_index) {
                     deltas
@@ -179,6 +201,7 @@ fn get_attestation_deltas<E: EthSpec>(
                 }
             }
         }
+    }
 
     Ok(deltas)
 }

testing/ef_tests/src/cases/rewards.rs

@@ -5,6 +5,7 @@ use compare_fields_derive::CompareFields;
 use serde::Deserialize;
 use ssz::four_byte_option_impl;
 use ssz_derive::{Decode, Encode};
+use state_processing::per_epoch_processing::base::rewards_and_penalties::ProposerRewardCalculation;
 use state_processing::{
     per_epoch_processing::{
         altair,
@@ -130,6 +131,7 @@ impl<E: EthSpec> Case for RewardsTest<E> {
                 let deltas = base::rewards_and_penalties::get_attestation_deltas_all(
                     &state,
                     &validator_statuses,
+                    ProposerRewardCalculation::Include,
                     spec,
                 )?;