From 266b24112306355bdfa64cdc0d7b63f2e3b4572a Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 22 Jan 2025 16:34:22 -0800 Subject: [PATCH] Electra minor refactorings (#6839) N/A Fix some typos and other minor refactorings in the electra code. Thanks @jtraglia for bringing them up. Note to reviewiers: 47803496dedf1d0e6e4b11f527afff0119976ff0 is the commit that needs looking into in detail. The rest are very minor refactorings --- .../operation_pool/src/attestation_storage.rs | 2 +- .../src/per_block_processing.rs | 26 +++++------- .../src/per_epoch_processing/single_pass.rs | 10 ++--- .../state_processing/src/upgrade/electra.rs | 3 +- consensus/types/src/beacon_state.rs | 42 +++++-------------- consensus/types/src/validator.rs | 12 +++--- 6 files changed, 33 insertions(+), 62 deletions(-) diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 083c1170f07..49ef5c279c5 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -214,7 +214,7 @@ impl CompactIndexedAttestationElectra { .is_zero() } - /// Returns `true` if aggregated, otherwise `false`. + /// Returns `true` if aggregated, otherwise `false`. pub fn aggregate_same_committee(&mut self, other: &Self) -> bool { if self.committee_bits != other.committee_bits { return false; diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index 502ad25838e..ef4799c245b 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -523,9 +523,9 @@ pub fn get_expected_withdrawals( // [New in Electra:EIP7251] // Consume pending partial withdrawals let processed_partial_withdrawals_count = - if let Ok(partial_withdrawals) = state.pending_partial_withdrawals() { + if let Ok(pending_partial_withdrawals) = state.pending_partial_withdrawals() { let mut processed_partial_withdrawals_count = 0; - for withdrawal in partial_withdrawals { + for withdrawal in pending_partial_withdrawals { if withdrawal.withdrawable_epoch > epoch || withdrawals.len() == spec.max_pending_partials_per_withdrawals_sweep as usize { @@ -552,7 +552,7 @@ pub fn get_expected_withdrawals( validator_index: withdrawal.validator_index, address: validator .get_execution_withdrawal_address(spec) - .ok_or(BeaconStateError::NonExecutionAddresWithdrawalCredential)?, + .ok_or(BeaconStateError::NonExecutionAddressWithdrawalCredential)?, amount: withdrawable_balance, }); withdrawal_index.safe_add_assign(1)?; @@ -583,7 +583,7 @@ pub fn get_expected_withdrawals( validator_index as usize, ))? .safe_sub(partially_withdrawn_balance)?; - if validator.is_fully_withdrawable_at(balance, epoch, spec, fork_name) { + if validator.is_fully_withdrawable_validator(balance, epoch, spec, fork_name) { withdrawals.push(Withdrawal { index: withdrawal_index, validator_index, @@ -600,9 +600,7 @@ pub fn get_expected_withdrawals( address: validator .get_execution_withdrawal_address(spec) .ok_or(BlockProcessingError::WithdrawalCredentialsInvalid)?, - amount: balance.safe_sub( - validator.get_max_effective_balance(spec, state.fork_name_unchecked()), - )?, + amount: balance.safe_sub(validator.get_max_effective_balance(spec, fork_name))?, }); withdrawal_index.safe_add_assign(1)?; } @@ -624,7 +622,7 @@ pub fn process_withdrawals>( spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { if state.fork_name_unchecked().capella_enabled() { - let (expected_withdrawals, partial_withdrawals_count) = + let (expected_withdrawals, processed_partial_withdrawals_count) = get_expected_withdrawals(state, spec)?; let expected_root = expected_withdrawals.tree_hash_root(); let withdrawals_root = payload.withdrawals_root()?; @@ -645,14 +643,10 @@ pub fn process_withdrawals>( } // Update pending partial withdrawals [New in Electra:EIP7251] - if let Some(partial_withdrawals_count) = partial_withdrawals_count { - // TODO(electra): Use efficient pop_front after milhouse release https://github.com/sigp/milhouse/pull/38 - let new_partial_withdrawals = state - .pending_partial_withdrawals()? - .iter_from(partial_withdrawals_count)? - .cloned() - .collect::>(); - *state.pending_partial_withdrawals_mut()? = List::new(new_partial_withdrawals)?; + if let Some(processed_partial_withdrawals_count) = processed_partial_withdrawals_count { + state + .pending_partial_withdrawals_mut()? + .pop_front(processed_partial_withdrawals_count)?; } // Update the next withdrawal index if this block contained withdrawals diff --git a/consensus/state_processing/src/per_epoch_processing/single_pass.rs b/consensus/state_processing/src/per_epoch_processing/single_pass.rs index a4a81c8eeff..5c31669a600 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -1075,13 +1075,9 @@ fn process_pending_consolidations( next_pending_consolidation.safe_add_assign(1)?; } - let new_pending_consolidations = List::try_from_iter( - state - .pending_consolidations()? - .iter_from(next_pending_consolidation)? - .cloned(), - )?; - *state.pending_consolidations_mut()? = new_pending_consolidations; + state + .pending_consolidations_mut()? + .pop_front(next_pending_consolidation)?; // the spec tests require we don't perform effective balance updates when testing pending_consolidations if !perform_effective_balance_updates { diff --git a/consensus/state_processing/src/upgrade/electra.rs b/consensus/state_processing/src/upgrade/electra.rs index 0f32e1553d9..258b28a45bd 100644 --- a/consensus/state_processing/src/upgrade/electra.rs +++ b/consensus/state_processing/src/upgrade/electra.rs @@ -47,10 +47,11 @@ pub fn upgrade_to_electra( .enumerate() .filter(|(_, validator)| validator.activation_epoch == spec.far_future_epoch) .sorted_by_key(|(index, validator)| (validator.activation_eligibility_epoch, *index)) + .map(|(index, _)| index) .collect::>(); // Process validators to queue entire balance and reset them - for (index, _) in pre_activation { + for index in pre_activation { let balance = post .balances_mut() .get_mut(index) diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 6f44998cdff..157271b2272 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -161,7 +161,7 @@ pub enum Error { InvalidFlagIndex(usize), MerkleTreeError(merkle_proof::MerkleTreeError), PartialWithdrawalCountInvalid(usize), - NonExecutionAddresWithdrawalCredential, + NonExecutionAddressWithdrawalCredential, NoCommitteeFound(CommitteeIndex), InvalidCommitteeIndex(CommitteeIndex), InvalidSelectionProof { @@ -2214,7 +2214,7 @@ impl BeaconState { // ******* Electra accessors ******* - /// Return the churn limit for the current epoch. + /// Return the churn limit for the current epoch. pub fn get_balance_churn_limit(&self, spec: &ChainSpec) -> Result { let total_active_balance = self.get_total_active_balance()?; let churn = std::cmp::max( @@ -2329,21 +2329,12 @@ impl BeaconState { | BeaconState::Bellatrix(_) | BeaconState::Capella(_) | BeaconState::Deneb(_) => Err(Error::IncorrectStateVariant), - BeaconState::Electra(_) => { - let state = self.as_electra_mut()?; - + BeaconState::Electra(_) | BeaconState::Fulu(_) => { // Consume the balance and update state variables - state.exit_balance_to_consume = exit_balance_to_consume.safe_sub(exit_balance)?; - state.earliest_exit_epoch = earliest_exit_epoch; - Ok(state.earliest_exit_epoch) - } - BeaconState::Fulu(_) => { - let state = self.as_fulu_mut()?; - - // Consume the balance and update state variables - state.exit_balance_to_consume = exit_balance_to_consume.safe_sub(exit_balance)?; - state.earliest_exit_epoch = earliest_exit_epoch; - Ok(state.earliest_exit_epoch) + *self.exit_balance_to_consume_mut()? = + exit_balance_to_consume.safe_sub(exit_balance)?; + *self.earliest_exit_epoch_mut()? = earliest_exit_epoch; + self.earliest_exit_epoch() } } } @@ -2385,23 +2376,12 @@ impl BeaconState { | BeaconState::Bellatrix(_) | BeaconState::Capella(_) | BeaconState::Deneb(_) => Err(Error::IncorrectStateVariant), - BeaconState::Electra(_) => { - let state = self.as_electra_mut()?; - - // Consume the balance and update state variables. - state.consolidation_balance_to_consume = - consolidation_balance_to_consume.safe_sub(consolidation_balance)?; - state.earliest_consolidation_epoch = earliest_consolidation_epoch; - Ok(state.earliest_consolidation_epoch) - } - BeaconState::Fulu(_) => { - let state = self.as_fulu_mut()?; - + BeaconState::Electra(_) | BeaconState::Fulu(_) => { // Consume the balance and update state variables. - state.consolidation_balance_to_consume = + *self.consolidation_balance_to_consume_mut()? = consolidation_balance_to_consume.safe_sub(consolidation_balance)?; - state.earliest_consolidation_epoch = earliest_consolidation_epoch; - Ok(state.earliest_consolidation_epoch) + *self.earliest_consolidation_epoch_mut()? = earliest_consolidation_epoch; + self.earliest_consolidation_epoch() } } } diff --git a/consensus/types/src/validator.rs b/consensus/types/src/validator.rs index 222b9292a2a..5aed90d2c1b 100644 --- a/consensus/types/src/validator.rs +++ b/consensus/types/src/validator.rs @@ -56,7 +56,7 @@ impl Validator { }; let max_effective_balance = validator.get_max_effective_balance(spec, fork_name); - // safe math is unnecessary here since the spec.effecive_balance_increment is never <= 0 + // safe math is unnecessary here since the spec.effective_balance_increment is never <= 0 validator.effective_balance = std::cmp::min( amount - (amount % spec.effective_balance_increment), max_effective_balance, @@ -195,7 +195,7 @@ impl Validator { /// Returns `true` if the validator is fully withdrawable at some epoch. /// /// Calls the correct function depending on the provided `fork_name`. - pub fn is_fully_withdrawable_at( + pub fn is_fully_withdrawable_validator( &self, balance: u64, epoch: Epoch, @@ -203,14 +203,14 @@ impl Validator { current_fork: ForkName, ) -> bool { if current_fork.electra_enabled() { - self.is_fully_withdrawable_at_electra(balance, epoch, spec) + self.is_fully_withdrawable_validator_electra(balance, epoch, spec) } else { - self.is_fully_withdrawable_at_capella(balance, epoch, spec) + self.is_fully_withdrawable_validator_capella(balance, epoch, spec) } } /// Returns `true` if the validator is fully withdrawable at some epoch. - fn is_fully_withdrawable_at_capella( + fn is_fully_withdrawable_validator_capella( &self, balance: u64, epoch: Epoch, @@ -222,7 +222,7 @@ impl Validator { /// Returns `true` if the validator is fully withdrawable at some epoch. /// /// Modified in electra as part of EIP 7251. - fn is_fully_withdrawable_at_electra( + fn is_fully_withdrawable_validator_electra( &self, balance: u64, epoch: Epoch,