Correct reward denominator in op pool

beacon_node/beacon_chain/src/block_reward.rs

@@ -1,6 +1,8 @@
 use crate::{BeaconChain, BeaconChainError, BeaconChainTypes};
 use eth2::lighthouse::{AttestationRewards, BlockReward, BlockRewardMeta};
-use operation_pool::{AttMaxCover, MaxCover, RewardCache, SplitAttestation};
+use operation_pool::{
+    AttMaxCover, MaxCover, RewardCache, SplitAttestation, PROPOSER_REWARD_DENOMINATOR,
+};
 use state_processing::{
     common::get_attesting_indices_from_state,
     per_block_processing::altair::sync_committee::compute_sync_aggregate_rewards,
@@ -65,13 +67,10 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         let mut curr_epoch_total = 0;
 
         for cover in &per_attestation_rewards {
-            for &reward in cover.fresh_validators_rewards.values() {
-                if cover.att.data.slot.epoch(T::EthSpec::slots_per_epoch()) == state.current_epoch()
-                {
-                    curr_epoch_total += reward;
-                } else {
-                    prev_epoch_total += reward;
-                }
+            if cover.att.data.slot.epoch(T::EthSpec::slots_per_epoch()) == state.current_epoch() {
+                curr_epoch_total += cover.score() as u64;
+            } else {
+                prev_epoch_total += cover.score() as u64;
             }
         }
 
@@ -80,7 +79,16 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         // Drop the covers.
         let per_attestation_rewards = per_attestation_rewards
             .into_iter()
-            .map(|cover| cover.fresh_validators_rewards)
+            .map(|cover| {
+                // Divide each reward numerator by the denominator. This can lead to the total being
+                // less than the sum of the individual rewards due to the fact that integer division
+                // does not distribute over addition.
+                let mut rewards = cover.fresh_validators_rewards;
+                rewards
+                    .values_mut()
+                    .for_each(|reward| *reward /= PROPOSER_REWARD_DENOMINATOR);
+                rewards
+            })
             .collect();
 
         // Add the attestation data if desired.

beacon_node/operation_pool/src/attestation.rs

@@ -7,15 +7,18 @@ use state_processing::common::{
 use std::collections::HashMap;
 use types::{
     beacon_state::BeaconStateBase,
-    consts::altair::{PARTICIPATION_FLAG_WEIGHTS, WEIGHT_DENOMINATOR},
+    consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR},
     Attestation, BeaconState, BitList, ChainSpec, EthSpec,
 };
 
+pub const PROPOSER_REWARD_DENOMINATOR: u64 =
+    (WEIGHT_DENOMINATOR - PROPOSER_WEIGHT) * WEIGHT_DENOMINATOR / PROPOSER_WEIGHT;
+
 #[derive(Debug, Clone)]
 pub struct AttMaxCover<'a, E: EthSpec> {
     /// Underlying attestation.
     pub att: CompactAttestationRef<'a, E>,
-    /// Mapping of validator indices and their rewards.
+    /// Mapping of validator indices and their reward *numerators*.
     pub fresh_validators_rewards: HashMap<u64, u64>,
 }
 
@@ -30,7 +33,7 @@ impl<'a, E: EthSpec> AttMaxCover<'a, E> {
         if let BeaconState::Base(ref base_state) = state {
             Self::new_for_base(att, state, base_state, total_active_balance, spec)
         } else {
-            Self::new_for_altair_deneb(att, state, reward_cache, spec)
+            Self::new_for_altair_or_later(att, state, reward_cache, spec)
         }
     }
 
@@ -68,7 +71,7 @@ impl<'a, E: EthSpec> AttMaxCover<'a, E> {
     }
 
     /// Initialise an attestation cover object for Altair or later.
-    pub fn new_for_altair_deneb(
+    pub fn new_for_altair_or_later(
         att: CompactAttestationRef<'a, E>,
         state: &BeaconState<E>,
         reward_cache: &'a RewardCache,
@@ -103,10 +106,7 @@ impl<'a, E: EthSpec> AttMaxCover<'a, E> {
                     }
                 }
 
-                let proposer_reward = proposer_reward_numerator
-                    .checked_div(WEIGHT_DENOMINATOR.checked_mul(spec.proposer_reward_quotient)?)?;
-
-                Some((index, proposer_reward)).filter(|_| proposer_reward != 0)
+                Some((index, proposer_reward_numerator)).filter(|_| proposer_reward_numerator != 0)
             })
             .collect();
 
@@ -163,7 +163,7 @@ impl<'a, E: EthSpec> MaxCover for AttMaxCover<'a, E> {
     }
 
     fn score(&self) -> usize {
-        self.fresh_validators_rewards.values().sum::<u64>() as usize
+        (self.fresh_validators_rewards.values().sum::<u64>() / PROPOSER_REWARD_DENOMINATOR) as usize
     }
 }
 

beacon_node/operation_pool/src/lib.rs

@@ -9,7 +9,7 @@ mod reward_cache;
 mod sync_aggregate_id;
 
 pub use crate::bls_to_execution_changes::ReceivedPreCapella;
-pub use attestation::{earliest_attestation_validators, AttMaxCover};
+pub use attestation::{earliest_attestation_validators, AttMaxCover, PROPOSER_REWARD_DENOMINATOR};
 pub use attestation_storage::{CompactAttestationRef, SplitAttestation};
 pub use max_cover::MaxCover;
 pub use persistence::{
@@ -1402,7 +1402,8 @@ mod release_tests {
                 .retain(|validator_index, _| !seen_indices.contains(validator_index));
 
             // Check that rewards are in decreasing order
-            let rewards = fresh_validators_rewards.values().sum();
+            let rewards =
+                fresh_validators_rewards.values().sum::<u64>() / PROPOSER_REWARD_DENOMINATOR;
             assert!(prev_reward >= rewards);
             prev_reward = rewards;
             seen_indices.extend(fresh_validators_rewards.keys());