diff --git a/beacon_node/operation_pool/src/attestation.rs b/beacon_node/operation_pool/src/attestation.rs index 11537e6ec3b..2f7fba45401 100644 --- a/beacon_node/operation_pool/src/attestation.rs +++ b/beacon_node/operation_pool/src/attestation.rs @@ -90,6 +90,8 @@ impl<'a, T: EthSpec> AttMaxCover<'a, T> { let att_participation_flags = get_attestation_participation_flag_indices(state, &att.data, inclusion_delay, spec) .ok()?; + let base_reward_per_increment = + altair::BaseRewardPerIncrement::new(total_active_balance, spec).ok()?; let fresh_validators_rewards = attesting_indices .iter() @@ -98,7 +100,7 @@ impl<'a, T: EthSpec> AttMaxCover<'a, T> { let participation = participation_list.get(index)?; let base_reward = - altair::get_base_reward(state, index, total_active_balance, spec).ok()?; + altair::get_base_reward(state, index, base_reward_per_increment, spec).ok()?; for (flag_index, weight) in PARTICIPATION_FLAG_WEIGHTS.iter().enumerate() { if att_participation_flags.contains(&flag_index) diff --git a/consensus/state_processing/src/common/altair.rs b/consensus/state_processing/src/common/altair.rs index 6cf80bdd9ed..8943ef2f40b 100644 --- a/consensus/state_processing/src/common/altair.rs +++ b/consensus/state_processing/src/common/altair.rs @@ -2,27 +2,45 @@ use integer_sqrt::IntegerSquareRoot; use safe_arith::{ArithError, SafeArith}; use types::*; +/// This type exists to avoid confusing `total_active_balance` with `base_reward_per_increment`, +/// since they are used in close proximity and the same type (`u64`). +#[derive(Copy, Clone)] +pub struct BaseRewardPerIncrement(u64); + +impl BaseRewardPerIncrement { + pub fn new(total_active_balance: u64, spec: &ChainSpec) -> Result { + get_base_reward_per_increment(total_active_balance, spec).map(Self) + } + + pub fn as_u64(&self) -> u64 { + self.0 + } +} + /// Returns the base reward for some validator. /// +/// The function has a different interface to the spec since it accepts the +/// `base_reward_per_increment` without computing it each time. Avoiding the re computation has +/// shown to be a significant optimisation. +/// /// Spec v1.1.0 pub fn get_base_reward( state: &BeaconState, index: usize, - // Should be == get_total_active_balance(state, spec) - total_active_balance: u64, + base_reward_per_increment: BaseRewardPerIncrement, spec: &ChainSpec, ) -> Result { state .get_effective_balance(index)? .safe_div(spec.effective_balance_increment)? - .safe_mul(get_base_reward_per_increment(total_active_balance, spec)?) + .safe_mul(base_reward_per_increment.as_u64()) .map_err(Into::into) } /// Returns the base reward for some validator. /// /// Spec v1.1.0 -pub fn get_base_reward_per_increment( +fn get_base_reward_per_increment( total_active_balance: u64, spec: &ChainSpec, ) -> Result { diff --git a/consensus/state_processing/src/per_block_processing/altair/sync_committee.rs b/consensus/state_processing/src/per_block_processing/altair/sync_committee.rs index 8358003e4b4..306e86714c6 100644 --- a/consensus/state_processing/src/per_block_processing/altair/sync_committee.rs +++ b/consensus/state_processing/src/per_block_processing/altair/sync_committee.rs @@ -1,4 +1,4 @@ -use crate::common::{altair::get_base_reward_per_increment, decrease_balance, increase_balance}; +use crate::common::{altair::BaseRewardPerIncrement, decrease_balance, increase_balance}; use crate::per_block_processing::errors::{BlockProcessingError, SyncAggregateInvalid}; use crate::{signature_sets::sync_aggregate_signature_set, VerifySignatures}; use safe_arith::SafeArith; @@ -72,7 +72,8 @@ pub fn compute_sync_aggregate_rewards( let total_active_balance = state.get_total_active_balance()?; let total_active_increments = total_active_balance.safe_div(spec.effective_balance_increment)?; - let total_base_rewards = get_base_reward_per_increment(total_active_balance, spec)? + let total_base_rewards = BaseRewardPerIncrement::new(total_active_balance, spec)? + .as_u64() .safe_mul(total_active_increments)?; let max_participant_rewards = total_base_rewards .safe_mul(SYNC_REWARD_WEIGHT)? diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs index 3bf22d004a4..31a4ac1fb42 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -1,7 +1,8 @@ use super::*; use crate::common::{ - altair::get_base_reward, get_attestation_participation_flag_indices, increase_balance, - initiate_validator_exit, slash_validator, + altair::{get_base_reward, BaseRewardPerIncrement}, + get_attestation_participation_flag_indices, increase_balance, initiate_validator_exit, + slash_validator, }; use crate::per_block_processing::errors::{BlockProcessingError, IntoWithIndex}; use crate::VerifySignatures; @@ -128,6 +129,7 @@ pub mod altair { // Update epoch participation flags. let total_active_balance = state.get_total_active_balance()?; + let base_reward_per_increment = BaseRewardPerIncrement::new(total_active_balance, spec)?; let mut proposer_reward_numerator = 0; for index in &indexed_attestation.attesting_indices { let index = *index as usize; @@ -143,7 +145,7 @@ pub mod altair { { validator_participation.add_flag(flag_index)?; proposer_reward_numerator.safe_add_assign( - get_base_reward(state, index, total_active_balance, spec)? + get_base_reward(state, index, base_reward_per_increment, spec)? .safe_mul(weight)?, )?; } diff --git a/consensus/state_processing/src/per_epoch_processing/altair/inactivity_updates.rs b/consensus/state_processing/src/per_epoch_processing/altair/inactivity_updates.rs index 038fe770440..967f642e85d 100644 --- a/consensus/state_processing/src/per_epoch_processing/altair/inactivity_updates.rs +++ b/consensus/state_processing/src/per_epoch_processing/altair/inactivity_updates.rs @@ -14,6 +14,7 @@ pub fn process_inactivity_updates( participation_cache: &ParticipationCache, spec: &ChainSpec, ) -> Result<(), EpochProcessingError> { + let previous_epoch = state.previous_epoch(); // Score updates based on previous epoch participation, skip genesis epoch if state.current_epoch() == T::genesis_epoch() { return Ok(()); @@ -33,7 +34,7 @@ pub fn process_inactivity_updates( .safe_add_assign(spec.inactivity_score_bias)?; } // Decrease the score of all validators for forgiveness when not during a leak - if !state.is_in_inactivity_leak(spec) { + if !state.is_in_inactivity_leak(previous_epoch, spec) { let inactivity_score = state.get_inactivity_score_mut(index)?; inactivity_score .safe_sub_assign(min(spec.inactivity_score_recovery_rate, *inactivity_score))?; diff --git a/consensus/state_processing/src/per_epoch_processing/altair/participation_cache.rs b/consensus/state_processing/src/per_epoch_processing/altair/participation_cache.rs index 503dadfc708..004726923e9 100644 --- a/consensus/state_processing/src/per_epoch_processing/altair/participation_cache.rs +++ b/consensus/state_processing/src/per_epoch_processing/altair/participation_cache.rs @@ -12,7 +12,6 @@ //! to get useful summaries about the validator participation in an epoch. use safe_arith::{ArithError, SafeArith}; -use std::collections::HashMap; use types::{ consts::altair::{ NUM_FLAG_INDICES, TIMELY_HEAD_FLAG_INDEX, TIMELY_SOURCE_FLAG_INDEX, @@ -24,6 +23,7 @@ use types::{ #[derive(Debug, PartialEq)] pub enum Error { InvalidFlagIndex(usize), + InvalidValidatorIndex(usize), } /// A balance which will never be below the specified `minimum`. @@ -64,7 +64,7 @@ struct SingleEpochParticipationCache { /// It would be ideal to maintain a reference to the `BeaconState` here rather than copying the /// `ParticipationFlags`, however that would cause us to run into mutable reference limitations /// upstream. - unslashed_participating_indices: HashMap, + unslashed_participating_indices: Vec>, /// Stores the sum of the balances for all validators in `self.unslashed_participating_indices` /// for all flags in `NUM_FLAG_INDICES`. /// @@ -76,11 +76,12 @@ struct SingleEpochParticipationCache { } impl SingleEpochParticipationCache { - fn new(hashmap_len: usize, spec: &ChainSpec) -> Self { + fn new(state: &BeaconState, spec: &ChainSpec) -> Self { + let num_validators = state.validators().len(); let zero_balance = Balance::zero(spec.effective_balance_increment); Self { - unslashed_participating_indices: HashMap::with_capacity(hashmap_len), + unslashed_participating_indices: vec![None; num_validators], total_flag_balances: [zero_balance; NUM_FLAG_INDICES], total_active_balance: zero_balance, } @@ -100,7 +101,11 @@ impl SingleEpochParticipationCache { /// /// May return an error if `flag_index` is out-of-bounds. fn has_flag(&self, val_index: usize, flag_index: usize) -> Result { - if let Some(participation_flags) = self.unslashed_participating_indices.get(&val_index) { + let participation_flags = self + .unslashed_participating_indices + .get(val_index) + .ok_or(Error::InvalidValidatorIndex(val_index))?; + if let Some(participation_flags) = participation_flags { participation_flags .has_flag(flag_index) .map_err(|_| Error::InvalidFlagIndex(flag_index)) @@ -121,13 +126,14 @@ impl SingleEpochParticipationCache { &mut self, val_index: usize, state: &BeaconState, + current_epoch: Epoch, relative_epoch: RelativeEpoch, ) -> Result<(), BeaconStateError> { let val_balance = state.get_effective_balance(val_index)?; let validator = state.get_validator(val_index)?; // Sanity check to ensure the validator is active. - let epoch = relative_epoch.into_epoch(state.current_epoch()); + let epoch = relative_epoch.into_epoch(current_epoch); if !validator.is_active_at(epoch) { return Err(BeaconStateError::ValidatorIsInactive { val_index }); } @@ -149,8 +155,10 @@ impl SingleEpochParticipationCache { } // Add their `ParticipationFlags` to the map. - self.unslashed_participating_indices - .insert(val_index, *epoch_participation); + *self + .unslashed_participating_indices + .get_mut(val_index) + .ok_or(BeaconStateError::UnknownValidator(val_index))? = Some(*epoch_participation); // Iterate through all the flags and increment the total flag balances for whichever flags // are set for `val_index`. @@ -190,19 +198,10 @@ impl ParticipationCache { let current_epoch = state.current_epoch(); let previous_epoch = state.previous_epoch(); - let num_previous_epoch_active_vals = state - .get_cached_active_validator_indices(RelativeEpoch::Previous)? - .len(); - let num_current_epoch_active_vals = state - .get_cached_active_validator_indices(RelativeEpoch::Current)? - .len(); - // Both the current/previous epoch participations are set to a capacity that is slightly // larger than required. The difference will be due slashed-but-active validators. - let mut current_epoch_participation = - SingleEpochParticipationCache::new(num_current_epoch_active_vals, spec); - let mut previous_epoch_participation = - SingleEpochParticipationCache::new(num_previous_epoch_active_vals, spec); + let mut current_epoch_participation = SingleEpochParticipationCache::new(state, spec); + let mut previous_epoch_participation = SingleEpochParticipationCache::new(state, spec); // Contains the set of validators which are either: // // - Active in the previous epoch. @@ -224,6 +223,7 @@ impl ParticipationCache { current_epoch_participation.process_active_validator( val_index, state, + current_epoch, RelativeEpoch::Current, )?; } @@ -232,13 +232,14 @@ impl ParticipationCache { previous_epoch_participation.process_active_validator( val_index, state, + current_epoch, RelativeEpoch::Previous, )?; } // Note: a validator might still be "eligible" whilst returning `false` to // `Validator::is_active_at`. - if state.is_eligible_validator(val_index)? { + if state.is_eligible_validator(previous_epoch, val_index)? { eligible_indices.push(val_index) } } @@ -313,16 +314,20 @@ impl ParticipationCache { * Active/Unslashed */ - pub fn is_active_unslashed_in_previous_epoch(&self, val_index: usize) -> bool { + /// Returns `None` for an unknown `val_index`. + pub fn is_active_unslashed_in_previous_epoch(&self, val_index: usize) -> Option { self.previous_epoch_participation .unslashed_participating_indices - .contains_key(&val_index) + .get(val_index) + .map(|flags| flags.is_some()) } - pub fn is_active_unslashed_in_current_epoch(&self, val_index: usize) -> bool { + /// Returns `None` for an unknown `val_index`. + pub fn is_active_unslashed_in_current_epoch(&self, val_index: usize) -> Option { self.current_epoch_participation .unslashed_participating_indices - .contains_key(&val_index) + .get(val_index) + .map(|flags| flags.is_some()) } /* 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 ce102694f58..ccebbcb3a29 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 @@ -6,7 +6,10 @@ use types::consts::altair::{ }; use types::{BeaconState, ChainSpec, EthSpec}; -use crate::common::{altair::get_base_reward, decrease_balance, increase_balance}; +use crate::common::{ + altair::{get_base_reward, BaseRewardPerIncrement}, + decrease_balance, increase_balance, +}; use crate::per_epoch_processing::{Delta, Error}; /// Apply attester and proposer rewards. @@ -67,13 +70,14 @@ pub fn get_flag_index_deltas( 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 base_reward_per_increment = BaseRewardPerIncrement::new(total_active_balance, spec)?; for &index in participation_cache.eligible_validator_indices() { - let base_reward = get_base_reward(state, index, total_active_balance, spec)?; + let base_reward = get_base_reward(state, index, base_reward_per_increment, spec)?; let mut delta = Delta::default(); if unslashed_participating_indices.contains(index as usize)? { - if !state.is_in_inactivity_leak(spec) { + if !state.is_in_inactivity_leak(previous_epoch, spec) { let reward_numerator = base_reward .safe_mul(weight)? .safe_mul(unslashed_participating_increments)?; diff --git a/consensus/state_processing/src/per_epoch_processing/base/rewards_and_penalties.rs b/consensus/state_processing/src/per_epoch_processing/base/rewards_and_penalties.rs index 99d08a6db33..87e4261e0a1 100644 --- a/consensus/state_processing/src/per_epoch_processing/base/rewards_and_penalties.rs +++ b/consensus/state_processing/src/per_epoch_processing/base/rewards_and_penalties.rs @@ -78,6 +78,7 @@ pub fn get_attestation_deltas( validator_statuses: &ValidatorStatuses, spec: &ChainSpec, ) -> Result, Error> { + let previous_epoch = state.previous_epoch(); let finality_delay = state .previous_epoch() .safe_sub(state.finalized_checkpoint().epoch)? @@ -92,7 +93,7 @@ pub fn get_attestation_deltas( // `get_inclusion_delay_deltas`. It's safe to do so here because any validator that is in // the unslashed indices of the matching source attestations is active, and therefore // eligible. - if !state.is_eligible_validator(index)? { + if !state.is_eligible_validator(previous_epoch, index)? { continue; } diff --git a/consensus/state_processing/src/per_epoch_processing/epoch_processing_summary.rs b/consensus/state_processing/src/per_epoch_processing/epoch_processing_summary.rs index 81487474236..5e15aa3e1b2 100644 --- a/consensus/state_processing/src/per_epoch_processing/epoch_processing_summary.rs +++ b/consensus/state_processing/src/per_epoch_processing/epoch_processing_summary.rs @@ -101,7 +101,9 @@ impl EpochProcessingSummary { EpochProcessingSummary::Altair { participation_cache, .. - } => participation_cache.is_active_unslashed_in_current_epoch(val_index), + } => participation_cache + .is_active_unslashed_in_current_epoch(val_index) + .unwrap_or(false), } } @@ -197,7 +199,9 @@ impl EpochProcessingSummary { EpochProcessingSummary::Altair { participation_cache, .. - } => participation_cache.is_active_unslashed_in_previous_epoch(val_index), + } => participation_cache + .is_active_unslashed_in_previous_epoch(val_index) + .unwrap_or(false), } } diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index d182ab9ae77..3a0f7d02e80 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -1602,17 +1602,23 @@ impl BeaconState { self.clone_with(CloneConfig::committee_caches_only()) } - pub fn is_eligible_validator(&self, val_index: usize) -> Result { - let previous_epoch = self.previous_epoch(); + /// Passing `previous_epoch` to this function rather than computing it internally provides + /// a tangible speed improvement in state processing. + pub fn is_eligible_validator( + &self, + previous_epoch: Epoch, + val_index: usize, + ) -> Result { self.get_validator(val_index).map(|val| { val.is_active_at(previous_epoch) || (val.slashed && previous_epoch + Epoch::new(1) < val.withdrawable_epoch) }) } - pub fn is_in_inactivity_leak(&self, spec: &ChainSpec) -> bool { - (self.previous_epoch() - self.finalized_checkpoint().epoch) - > spec.min_epochs_to_inactivity_penalty + /// Passing `previous_epoch` to this function rather than computing it internally provides + /// a tangible speed improvement in state processing. + pub fn is_in_inactivity_leak(&self, previous_epoch: Epoch, spec: &ChainSpec) -> bool { + (previous_epoch - self.finalized_checkpoint().epoch) > spec.min_epochs_to_inactivity_penalty } /// Get the `SyncCommittee` associated with the next slot. Useful because sync committees