From 0ca51def20ab3625a545cbd8702bb156926a4e90 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 2 Feb 2024 15:44:54 +1100 Subject: [PATCH 1/5] Delete unused epoch processing code --- .../altair/rewards_and_penalties.rs | 82 +------------- testing/ef_tests/src/cases/rewards.rs | 102 ++++-------------- 2 files changed, 23 insertions(+), 161 deletions(-) diff --git a/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs b/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs index 9510f2eaec4..d71648796f4 100644 --- a/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs +++ b/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs @@ -1,12 +1,10 @@ -use super::ParticipationCache; + use crate::per_epoch_processing::{ - single_pass::{process_epoch_single_pass, SinglePassConfig}, - Delta, Error, + single_pass::{process_epoch_single_pass, SinglePassConfig}, Error, }; -use safe_arith::SafeArith; + use types::consts::altair::{ - PARTICIPATION_FLAG_WEIGHTS, TIMELY_HEAD_FLAG_INDEX, TIMELY_TARGET_FLAG_INDEX, - WEIGHT_DENOMINATOR, + PARTICIPATION_FLAG_WEIGHTS, }; use types::{BeaconState, ChainSpec, EthSpec}; @@ -28,51 +26,6 @@ pub fn process_rewards_and_penalties_slow( Ok(()) } -/// Return the deltas for a given flag index by scanning through the participation flags. -/// -/// Spec v1.1.0 -pub fn get_flag_index_deltas( - deltas: &mut [Delta], - state: &BeaconState, - flag_index: usize, - total_active_balance: u64, - participation_cache: &ParticipationCache, - spec: &ChainSpec, -) -> Result<(), Error> { - let weight = get_flag_weight(flag_index)?; - let unslashed_participating_balance = - participation_cache.previous_epoch_flag_attesting_balance(flag_index)?; - let unslashed_participating_increments = - unslashed_participating_balance.safe_div(spec.effective_balance_increment)?; - let active_increments = total_active_balance.safe_div(spec.effective_balance_increment)?; - let previous_epoch = state.previous_epoch(); - - for &index in participation_cache.eligible_validator_indices() { - let validator = participation_cache.get_validator(index)?; - let base_reward = validator.base_reward; - - let mut delta = Delta::default(); - - if validator.is_unslashed_participating_index(flag_index)? { - if !state.is_in_inactivity_leak(previous_epoch, spec)? { - let reward_numerator = base_reward - .safe_mul(weight)? - .safe_mul(unslashed_participating_increments)?; - delta.reward( - reward_numerator.safe_div(active_increments.safe_mul(WEIGHT_DENOMINATOR)?)?, - )?; - } - } else if flag_index != TIMELY_HEAD_FLAG_INDEX { - delta.penalize(base_reward.safe_mul(weight)?.safe_div(WEIGHT_DENOMINATOR)?)?; - } - deltas - .get_mut(index) - .ok_or(Error::DeltaOutOfBounds(index))? - .combine(delta)?; - } - Ok(()) -} - /// Get the weight for a `flag_index` from the constant list of all weights. pub fn get_flag_weight(flag_index: usize) -> Result { PARTICIPATION_FLAG_WEIGHTS @@ -80,30 +33,3 @@ pub fn get_flag_weight(flag_index: usize) -> Result { .copied() .ok_or(Error::InvalidFlagIndex(flag_index)) } - -pub fn get_inactivity_penalty_deltas( - deltas: &mut [Delta], - state: &BeaconState, - participation_cache: &ParticipationCache, - spec: &ChainSpec, -) -> Result<(), Error> { - for &index in participation_cache.eligible_validator_indices() { - let validator = participation_cache.get_validator(index)?; - let mut delta = Delta::default(); - - if !validator.is_unslashed_participating_index(TIMELY_TARGET_FLAG_INDEX)? { - let penalty_numerator = validator - .effective_balance - .safe_mul(state.get_inactivity_score(index)?)?; - let penalty_denominator = spec - .inactivity_score_bias - .safe_mul(spec.inactivity_penalty_quotient_for_state(state))?; - delta.penalize(penalty_numerator.safe_div(penalty_denominator)?)?; - } - deltas - .get_mut(index) - .ok_or(Error::DeltaOutOfBounds(index))? - .combine(delta)?; - } - Ok(()) -} diff --git a/testing/ef_tests/src/cases/rewards.rs b/testing/ef_tests/src/cases/rewards.rs index bb41f6fe12f..0b06e183f11 100644 --- a/testing/ef_tests/src/cases/rewards.rs +++ b/testing/ef_tests/src/cases/rewards.rs @@ -7,17 +7,13 @@ use ssz::four_byte_option_impl; use ssz_derive::{Decode, Encode}; use state_processing::{ per_epoch_processing::{ - altair::{self, rewards_and_penalties::get_flag_index_deltas, ParticipationCache}, base::{self, rewards_and_penalties::AttestationDelta, ValidatorStatuses}, Delta, }, EpochProcessingError, }; use std::path::{Path, PathBuf}; -use types::{ - consts::altair::{TIMELY_HEAD_FLAG_INDEX, TIMELY_SOURCE_FLAG_INDEX, TIMELY_TARGET_FLAG_INDEX}, - BeaconState, EthSpec, ForkName, -}; +use types::{BeaconState, EthSpec, ForkName}; #[derive(Debug, Clone, PartialEq, Decode, Encode, CompareFields)] pub struct Deltas { @@ -108,53 +104,30 @@ impl Case for RewardsTest { fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> { let mut state = self.pre.clone(); + + // NOTE: We cannot run these tests for forks other than phase0 because single-pass epoch + // processing cannot expose these individual deltas. There is no point maintaining a + // separate implementation of rewards processing that will not be used in prod. + if fork_name != ForkName::Base { + return Ok(()); + } + let spec = &testing_spec::(fork_name); let deltas: Result = (|| { // Processing requires the committee caches. state.build_all_committee_caches(spec)?; - if let BeaconState::Base(_) = state { - let mut validator_statuses = ValidatorStatuses::new(&state, spec)?; - validator_statuses.process_attestations(&state)?; - - let deltas = base::rewards_and_penalties::get_attestation_deltas_all( - &state, - &validator_statuses, - spec, - )?; - - Ok(convert_all_base_deltas(&deltas)) - } else { - let total_active_balance = state.get_total_active_balance()?; - - let source_deltas = compute_altair_flag_deltas( - &state, - TIMELY_SOURCE_FLAG_INDEX, - total_active_balance, - spec, - )?; - let target_deltas = compute_altair_flag_deltas( - &state, - TIMELY_TARGET_FLAG_INDEX, - total_active_balance, - spec, - )?; - let head_deltas = compute_altair_flag_deltas( - &state, - TIMELY_HEAD_FLAG_INDEX, - total_active_balance, - spec, - )?; - let inactivity_penalty_deltas = compute_altair_inactivity_deltas(&state, spec)?; - Ok(AllDeltas { - source_deltas, - target_deltas, - head_deltas, - inclusion_delay_deltas: None, - inactivity_penalty_deltas, - }) - } + let mut validator_statuses = ValidatorStatuses::new(&state, spec)?; + validator_statuses.process_attestations(&state)?; + + let deltas = base::rewards_and_penalties::get_attestation_deltas_all( + &state, + &validator_statuses, + spec, + )?; + + Ok(convert_all_base_deltas(&deltas)) })(); compare_result_detailed(&deltas, &Some(self.deltas.clone()))?; @@ -181,40 +154,3 @@ fn convert_base_deltas(attestation_deltas: &[AttestationDelta], accessor: Access .unzip(); Deltas { rewards, penalties } } - -fn compute_altair_flag_deltas( - state: &BeaconState, - flag_index: usize, - total_active_balance: u64, - spec: &ChainSpec, -) -> Result { - let mut deltas = vec![Delta::default(); state.validators().len()]; - get_flag_index_deltas( - &mut deltas, - state, - flag_index, - total_active_balance, - &ParticipationCache::new(state, spec).unwrap(), - spec, - )?; - Ok(convert_altair_deltas(deltas)) -} - -fn compute_altair_inactivity_deltas( - state: &BeaconState, - spec: &ChainSpec, -) -> Result { - let mut deltas = vec![Delta::default(); state.validators().len()]; - altair::rewards_and_penalties::get_inactivity_penalty_deltas( - &mut deltas, - state, - &ParticipationCache::new(state, spec).unwrap(), - spec, - )?; - Ok(convert_altair_deltas(deltas)) -} - -fn convert_altair_deltas(deltas: Vec) -> Deltas { - let (rewards, penalties) = deltas.into_iter().map(|d| (d.rewards, d.penalties)).unzip(); - Deltas { rewards, penalties } -} From 2f32fa6f77e85a4bb0022c376a67a82834b9ecf4 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 5 Feb 2024 12:01:08 +1100 Subject: [PATCH 2/5] Compare total deltas --- testing/ef_tests/src/cases/rewards.rs | 109 +++++++++++++++++++++----- 1 file changed, 90 insertions(+), 19 deletions(-) diff --git a/testing/ef_tests/src/cases/rewards.rs b/testing/ef_tests/src/cases/rewards.rs index 0b06e183f11..217e898e983 100644 --- a/testing/ef_tests/src/cases/rewards.rs +++ b/testing/ef_tests/src/cases/rewards.rs @@ -7,6 +7,7 @@ use ssz::four_byte_option_impl; use ssz_derive::{Decode, Encode}; use state_processing::{ per_epoch_processing::{ + altair, base::{self, rewards_and_penalties::AttestationDelta, ValidatorStatuses}, Delta, }, @@ -37,6 +38,11 @@ pub struct AllDeltas { inactivity_penalty_deltas: Deltas, } +#[derive(Debug, Clone, PartialEq, CompareFields)] +pub struct TotalDeltas { + deltas: Vec, +} + #[derive(Debug, Clone, Default, Deserialize)] pub struct Metadata { pub description: Option, @@ -104,33 +110,44 @@ impl Case for RewardsTest { fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> { let mut state = self.pre.clone(); + let spec = &testing_spec::(fork_name); - // NOTE: We cannot run these tests for forks other than phase0 because single-pass epoch - // processing cannot expose these individual deltas. There is no point maintaining a - // separate implementation of rewards processing that will not be used in prod. - if fork_name != ForkName::Base { - return Ok(()); + // Single-pass epoch processing doesn't compute rewards in the genesis epoch because that's + // what the spec for `process_rewards_and_penalties` says to do. We skip these tests for now. + // + // See: https://github.com/ethereum/consensus-specs/issues/3593 + if fork_name != ForkName::Base && state.current_epoch() == 0 { + return Err(Error::SkippedKnownFailure); } - let spec = &testing_spec::(fork_name); + if let BeaconState::Base(_) = state { + let deltas: Result = (|| { + // Processing requires the committee caches. + state.build_all_committee_caches(spec)?; - let deltas: Result = (|| { - // Processing requires the committee caches. - state.build_all_committee_caches(spec)?; + let mut validator_statuses = ValidatorStatuses::new(&state, spec)?; + validator_statuses.process_attestations(&state)?; - let mut validator_statuses = ValidatorStatuses::new(&state, spec)?; - validator_statuses.process_attestations(&state)?; + let deltas = base::rewards_and_penalties::get_attestation_deltas_all( + &state, + &validator_statuses, + spec, + )?; - let deltas = base::rewards_and_penalties::get_attestation_deltas_all( - &state, - &validator_statuses, - spec, - )?; + Ok(convert_all_base_deltas(&deltas)) + })(); + compare_result_detailed(&deltas, &Some(self.deltas.clone()))?; + } else { + let deltas: Result = (|| { + // Processing requires the committee caches. + state.build_all_committee_caches(spec)?; + compute_altair_deltas(&mut state, spec) + })(); - Ok(convert_all_base_deltas(&deltas)) - })(); + let expected = all_deltas_to_total_deltas(&self.deltas); - compare_result_detailed(&deltas, &Some(self.deltas.clone()))?; + compare_result_detailed(&deltas, &Some(expected))?; + }; Ok(()) } @@ -154,3 +171,57 @@ fn convert_base_deltas(attestation_deltas: &[AttestationDelta], accessor: Access .unzip(); Deltas { rewards, penalties } } + +fn deltas_to_total_deltas(d: &Deltas) -> impl Iterator + '_ { + d.rewards + .iter() + .zip(&d.penalties) + .map(|(&reward, &penalty)| reward as i64 - penalty as i64) +} + +fn optional_deltas_to_total_deltas(d: &Option, len: usize) -> TotalDeltas { + let deltas = if let Some(d) = d { + deltas_to_total_deltas(d).collect() + } else { + vec![0i64; len] + }; + TotalDeltas { deltas } +} + +fn all_deltas_to_total_deltas(d: &AllDeltas) -> TotalDeltas { + let len = d.source_deltas.rewards.len(); + let deltas = deltas_to_total_deltas(&d.source_deltas) + .zip(deltas_to_total_deltas(&d.target_deltas)) + .zip(deltas_to_total_deltas(&d.head_deltas)) + .zip(optional_deltas_to_total_deltas(&d.inclusion_delay_deltas, len).deltas) + .zip(deltas_to_total_deltas(&d.inactivity_penalty_deltas)) + .map( + |((((source, target), head), inclusion_delay), inactivity_penalty)| { + source + target + head + inclusion_delay + inactivity_penalty + }, + ) + .collect::>(); + TotalDeltas { deltas } +} + +fn compute_altair_deltas( + state: &mut BeaconState, + spec: &ChainSpec, +) -> Result { + // Initialise deltas to pre-state balances. + let mut deltas = state + .balances() + .iter() + .map(|x| *x as i64) + .collect::>(); + altair::process_rewards_and_penalties_slow(state, spec)?; + + state.apply_pending_mutations().unwrap(); + + for (delta, new_balance) in deltas.iter_mut().zip(state.balances()) { + let old_balance = *delta; + *delta = *new_balance as i64 - old_balance; + } + + Ok(TotalDeltas { deltas }) +} From 7cc15060e44fd7cd8866fe1e633e1e9c92aa1f5a Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 5 Feb 2024 12:02:41 +1100 Subject: [PATCH 3/5] Remove unnecessary apply_pending --- testing/ef_tests/src/cases/rewards.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/testing/ef_tests/src/cases/rewards.rs b/testing/ef_tests/src/cases/rewards.rs index 217e898e983..1a8d5b0f539 100644 --- a/testing/ef_tests/src/cases/rewards.rs +++ b/testing/ef_tests/src/cases/rewards.rs @@ -216,8 +216,6 @@ fn compute_altair_deltas( .collect::>(); altair::process_rewards_and_penalties_slow(state, spec)?; - state.apply_pending_mutations().unwrap(); - for (delta, new_balance) in deltas.iter_mut().zip(state.balances()) { let old_balance = *delta; *delta = *new_balance as i64 - old_balance; From 81181d1d2a513f3cea732f41cbf7886bad4277f6 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 5 Feb 2024 13:23:16 +1100 Subject: [PATCH 4/5] cargo fmt --- .../per_epoch_processing/altair/rewards_and_penalties.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs b/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs index d71648796f4..4eeb93cf450 100644 --- a/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs +++ b/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs @@ -1,11 +1,9 @@ - use crate::per_epoch_processing::{ - single_pass::{process_epoch_single_pass, SinglePassConfig}, Error, + single_pass::{process_epoch_single_pass, SinglePassConfig}, + Error, }; -use types::consts::altair::{ - PARTICIPATION_FLAG_WEIGHTS, -}; +use types::consts::altair::PARTICIPATION_FLAG_WEIGHTS; use types::{BeaconState, ChainSpec, EthSpec}; /// Apply attester and proposer rewards. From 9057a974b2f181e11e2057b18e4634d79bc0ef67 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 5 Feb 2024 15:02:51 +1100 Subject: [PATCH 5/5] Remove newline --- .../src/per_epoch_processing/altair/rewards_and_penalties.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs b/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs index 4eeb93cf450..6b36346847b 100644 --- a/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs +++ b/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs @@ -2,7 +2,6 @@ use crate::per_epoch_processing::{ single_pass::{process_epoch_single_pass, SinglePassConfig}, Error, }; - use types::consts::altair::PARTICIPATION_FLAG_WEIGHTS; use types::{BeaconState, ChainSpec, EthSpec};