Skip to content

Commit 805c2dc

Browse files
michaelsproulmichaelsproul
authored
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.
1 parent af87135 commit 805c2dc

File tree

3 files changed

+29
-20
lines changed

3 files changed

+29
-20
lines changed

‎beacon_node/beacon_chain/src/block_reward.rs‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
use crate::{BeaconChain, BeaconChainError, BeaconChainTypes};
22
use eth2::lighthouse::{AttestationRewards, BlockReward, BlockRewardMeta};
3-
use operation_pool::{AttMaxCover, MaxCover, RewardCache, SplitAttestation};
3+
use operation_pool::{
4+
AttMaxCover, MaxCover, RewardCache, SplitAttestation, PROPOSER_REWARD_DENOMINATOR,
5+
};
46
use state_processing::{
57
common::get_attesting_indices_from_state,
68
per_block_processing::altair::sync_committee::compute_sync_aggregate_rewards,
@@ -65,13 +67,10 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
6567
let mut curr_epoch_total = 0;
6668

6769
for cover in &per_attestation_rewards {
68-
for &reward in cover.fresh_validators_rewards.values() {
69-
if cover.att.data.slot.epoch(T::EthSpec::slots_per_epoch()) == state.current_epoch()
70-
{
71-
curr_epoch_total += reward;
72-
} else {
73-
prev_epoch_total += reward;
74-
}
70+
if cover.att.data.slot.epoch(T::EthSpec::slots_per_epoch()) == state.current_epoch() {
71+
curr_epoch_total += cover.score() as u64;
72+
} else {
73+
prev_epoch_total += cover.score() as u64;
7574
}
7675
}
7776

@@ -80,7 +79,16 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
8079
// Drop the covers.
8180
let per_attestation_rewards = per_attestation_rewards
8281
.into_iter()
83-
.map(|cover| cover.fresh_validators_rewards)
82+
.map(|cover| {
83+
// Divide each reward numerator by the denominator. This can lead to the total being
84+
// less than the sum of the individual rewards due to the fact that integer division
85+
// does not distribute over addition.
86+
let mut rewards = cover.fresh_validators_rewards;
87+
rewards
88+
.values_mut()
89+
.for_each(|reward| *reward /= PROPOSER_REWARD_DENOMINATOR);
90+
rewards
91+
})
8492
.collect();
8593

8694
// Add the attestation data if desired.

‎beacon_node/operation_pool/src/attestation.rs‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,18 @@ use state_processing::common::{
77
use std::collections::HashMap;
88
use types::{
99
beacon_state::BeaconStateBase,
10-
consts::altair::{PARTICIPATION_FLAG_WEIGHTS, WEIGHT_DENOMINATOR},
10+
consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR},
1111
Attestation, BeaconState, BitList, ChainSpec, EthSpec,
1212
};
1313

14+
pub const PROPOSER_REWARD_DENOMINATOR: u64 =
15+
(WEIGHT_DENOMINATOR - PROPOSER_WEIGHT) * WEIGHT_DENOMINATOR / PROPOSER_WEIGHT;
16+
1417
#[derive(Debug, Clone)]
1518
pub struct AttMaxCover<'a, E: EthSpec> {
1619
/// Underlying attestation.
1720
pub att: CompactAttestationRef<'a, E>,
18-
/// Mapping of validator indices and their rewards.
21+
/// Mapping of validator indices and their reward *numerators*.
1922
pub fresh_validators_rewards: HashMap<u64, u64>,
2023
}
2124

@@ -30,7 +33,7 @@ impl<'a, E: EthSpec> AttMaxCover<'a, E> {
3033
if let BeaconState::Base(ref base_state) = state {
3134
Self::new_for_base(att, state, base_state, total_active_balance, spec)
3235
} else {
33-
Self::new_for_altair_deneb(att, state, reward_cache, spec)
36+
Self::new_for_altair_or_later(att, state, reward_cache, spec)
3437
}
3538
}
3639

@@ -68,7 +71,7 @@ impl<'a, E: EthSpec> AttMaxCover<'a, E> {
6871
}
6972

7073
/// Initialise an attestation cover object for Altair or later.
71-
pub fn new_for_altair_deneb(
74+
pub fn new_for_altair_or_later(
7275
att: CompactAttestationRef<'a, E>,
7376
state: &BeaconState<E>,
7477
reward_cache: &'a RewardCache,
@@ -103,10 +106,7 @@ impl<'a, E: EthSpec> AttMaxCover<'a, E> {
103106
}
104107
}
105108

106-
let proposer_reward = proposer_reward_numerator
107-
.checked_div(WEIGHT_DENOMINATOR.checked_mul(spec.proposer_reward_quotient)?)?;
108-
109-
Some((index, proposer_reward)).filter(|_| proposer_reward != 0)
109+
Some((index, proposer_reward_numerator)).filter(|_| proposer_reward_numerator != 0)
110110
})
111111
.collect();
112112

@@ -163,7 +163,7 @@ impl<'a, E: EthSpec> MaxCover for AttMaxCover<'a, E> {
163163
}
164164

165165
fn score(&self) -> usize {
166-
self.fresh_validators_rewards.values().sum::<u64>() as usize
166+
(self.fresh_validators_rewards.values().sum::<u64>() / PROPOSER_REWARD_DENOMINATOR) as usize
167167
}
168168
}
169169

‎beacon_node/operation_pool/src/lib.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ mod reward_cache;
99
mod sync_aggregate_id;
1010

1111
pub use crate::bls_to_execution_changes::ReceivedPreCapella;
12-
pub use attestation::{earliest_attestation_validators, AttMaxCover};
12+
pub use attestation::{earliest_attestation_validators, AttMaxCover, PROPOSER_REWARD_DENOMINATOR};
1313
pub use attestation_storage::{CompactAttestationRef, SplitAttestation};
1414
pub use max_cover::MaxCover;
1515
pub use persistence::{
@@ -1402,7 +1402,8 @@ mod release_tests {
14021402
.retain(|validator_index, _| !seen_indices.contains(validator_index));
14031403

14041404
// Check that rewards are in decreasing order
1405-
let rewards = fresh_validators_rewards.values().sum();
1405+
let rewards =
1406+
fresh_validators_rewards.values().sum::<u64>() / PROPOSER_REWARD_DENOMINATOR;
14061407
assert!(prev_reward >= rewards);
14071408
prev_reward = rewards;
14081409
seen_indices.extend(fresh_validators_rewards.keys());

0 commit comments

Comments
 (0)