From 805c2dc831e6d07a8f97dbb3862cfae146aa98d3 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 20 May 2025 11:06:40 +1000 Subject: [PATCH] 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 | 26 ++++++++++++------- beacon_node/operation_pool/src/attestation.rs | 18 ++++++------- beacon_node/operation_pool/src/lib.rs | 5 ++-- 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_reward.rs b/beacon_node/beacon_chain/src/block_reward.rs index 69eecc89b8..0809ce34ef 100644 --- a/beacon_node/beacon_chain/src/block_reward.rs +++ b/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 BeaconChain { 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 BeaconChain { // 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. diff --git a/beacon_node/operation_pool/src/attestation.rs b/beacon_node/operation_pool/src/attestation.rs index 97d0583e34..78280278e0 100644 --- a/beacon_node/operation_pool/src/attestation.rs +++ b/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, } @@ -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, 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::() as usize + (self.fresh_validators_rewards.values().sum::() / PROPOSER_REWARD_DENOMINATOR) as usize } } diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index ec8c6640b1..7481aa896a 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/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::() / PROPOSER_REWARD_DENOMINATOR; assert!(prev_reward >= rewards); prev_reward = rewards; seen_indices.extend(fresh_validators_rewards.keys());