Correct reward denominator in op pool (#5047)

Closes #5016


  The op pool was using the wrong denominator when calculating proposer block rewards! This was mostly inconsequential as our studies of Lighthouse's block profitability already showed that it is very close to optimal. The wrong denominator was leftover from phase0 code, and wasn't properly updated for Altair.

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() {
-                if cover.att.data.slot.epoch(T::EthSpec::slots_per_epoch()) == state.current_epoch()
+                curr_epoch_total += cover.score() as u64;
-                {
+            } else {
-                    curr_epoch_total += reward;
+                prev_epoch_total += cover.score() as u64;
-                } else {
-                    prev_epoch_total += reward;
-                }
             }
         }
 
@@ -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
+                Some((index, proposer_reward_numerator)).filter(|_| proposer_reward_numerator != 0)
-                    .checked_div(WEIGHT_DENOMINATOR.checked_mul(spec.proposer_reward_quotient)?)?;
-
-                Some((index, proposer_reward)).filter(|_| proposer_reward != 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());