From 3c24e2b9042fc7ad686ef48935ce65b53c28a821 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 10 Oct 2024 13:03:43 -0700 Subject: [PATCH 01/27] Fix partial withdrawals count --- consensus/state_processing/src/per_block_processing.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index f289b6e0817..436f4934b90 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -514,6 +514,7 @@ pub fn get_expected_withdrawals( // Consume pending partial withdrawals let partial_withdrawals_count = if let Ok(partial_withdrawals) = state.pending_partial_withdrawals() { + let mut partial_withdrawals_count = 0; for withdrawal in partial_withdrawals { if withdrawal.withdrawable_epoch > epoch || withdrawals.len() == spec.max_pending_partials_per_withdrawals_sweep as usize @@ -546,8 +547,9 @@ pub fn get_expected_withdrawals( }); withdrawal_index.safe_add_assign(1)?; } + partial_withdrawals_count.safe_add_assign(1)?; } - Some(withdrawals.len()) + Some(partial_withdrawals_count) } else { None }; From 3fa7790e0dd4b0f21f2dcd1bd12ef40d633c7c7d Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 10 Oct 2024 13:36:23 -0700 Subject: [PATCH 02/27] Remove get_active_balance --- .../src/per_epoch_processing/single_pass.rs | 22 ++++++++++--------- consensus/types/src/beacon_state.rs | 21 ------------------ consensus/types/src/validator.rs | 10 --------- 3 files changed, 12 insertions(+), 41 deletions(-) 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 fcb480a37cf..1d37562a95c 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -941,21 +941,23 @@ fn process_pending_consolidations( break; } - // Calculate the active balance while we have the source validator loaded. This is a safe - // reordering. - let source_balance = *state - .balances() - .get(source_index) - .ok_or(BeaconStateError::UnknownValidator(source_index))?; - let active_balance = - source_validator.get_active_balance(source_balance, spec, state_ctxt.fork_name); + // Calculate the consolidated balance + let max_effective_balance = + source_validator.get_max_effective_balance(spec, state_ctxt.fork_name); + let source_effective_balance = std::cmp::min( + *state + .balances() + .get(source_index) + .ok_or(BeaconStateError::UnknownValidator(source_index))?, + max_effective_balance, + ); // Churn any target excess active balance of target and raise its max. state.switch_to_compounding_validator(target_index, spec)?; // Move active balance to target. Excess balance is withdrawable. - decrease_balance(state, source_index, active_balance)?; - increase_balance(state, target_index, active_balance)?; + decrease_balance(state, source_index, source_effective_balance)?; + increase_balance(state, target_index, source_effective_balance)?; affected_validators.insert(source_index); affected_validators.insert(target_index); diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 8eed790a02b..d96752e0bf6 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -2121,27 +2121,6 @@ impl BeaconState { .map_err(Into::into) } - /// Get active balance for the given `validator_index`. - pub fn get_active_balance( - &self, - validator_index: usize, - spec: &ChainSpec, - current_fork: ForkName, - ) -> Result { - let max_effective_balance = self - .validators() - .get(validator_index) - .map(|validator| validator.get_max_effective_balance(spec, current_fork)) - .ok_or(Error::UnknownValidator(validator_index))?; - Ok(std::cmp::min( - *self - .balances() - .get(validator_index) - .ok_or(Error::UnknownValidator(validator_index))?, - max_effective_balance, - )) - } - pub fn get_pending_balance_to_withdraw(&self, validator_index: usize) -> Result { let mut pending_balance = 0; for withdrawal in self diff --git a/consensus/types/src/validator.rs b/consensus/types/src/validator.rs index 298604d4f3e..e94f4b4a0ab 100644 --- a/consensus/types/src/validator.rs +++ b/consensus/types/src/validator.rs @@ -262,16 +262,6 @@ impl Validator { spec.max_effective_balance } } - - pub fn get_active_balance( - &self, - validator_balance: u64, - spec: &ChainSpec, - current_fork: ForkName, - ) -> u64 { - let max_effective_balance = self.get_max_effective_balance(spec, current_fork); - std::cmp::min(validator_balance, max_effective_balance) - } } impl Default for Validator { From 3d595e6c0f39ea293dff90a966e087a4e993c8d3 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 10 Oct 2024 13:57:42 -0700 Subject: [PATCH 03/27] Remove queue_entire_balance_and_reset_validator --- .../state_processing/src/upgrade/electra.rs | 23 ++++++++++++++-- consensus/types/src/beacon_state.rs | 27 ------------------- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/consensus/state_processing/src/upgrade/electra.rs b/consensus/state_processing/src/upgrade/electra.rs index 1e532d9f107..8d5a2feb11f 100644 --- a/consensus/state_processing/src/upgrade/electra.rs +++ b/consensus/state_processing/src/upgrade/electra.rs @@ -2,7 +2,7 @@ use safe_arith::SafeArith; use std::mem; use types::{ BeaconState, BeaconStateElectra, BeaconStateError as Error, ChainSpec, Epoch, EpochCache, - EthSpec, Fork, + EthSpec, Fork, PendingBalanceDeposit, }; /// Transform a `Deneb` state into an `Electra` state. @@ -57,7 +57,26 @@ pub fn upgrade_to_electra( // Process validators to queue entire balance and reset them for (index, _) in pre_activation { - post.queue_entire_balance_and_reset_validator(index, spec)?; + let balance = post + .balances_mut() + .get_mut(index) + .ok_or(Error::UnknownValidator(index))?; + let balance_copy = *balance; + *balance = 0_u64; + + let validator = post + .validators_mut() + .get_mut(index) + .ok_or(Error::UnknownValidator(index))?; + validator.effective_balance = 0; + validator.activation_eligibility_epoch = spec.far_future_epoch; + + post.pending_balance_deposits_mut()? + .push(PendingBalanceDeposit { + index: index as u64, + amount: balance_copy, + }) + .map_err(Error::MilhouseError)?; } // Ensure early adopters of compounding credentials go through the activation churn diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index d96752e0bf6..0119c6a554e 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -2156,33 +2156,6 @@ impl BeaconState { Ok(()) } - pub fn queue_entire_balance_and_reset_validator( - &mut self, - validator_index: usize, - spec: &ChainSpec, - ) -> Result<(), Error> { - let balance = self - .balances_mut() - .get_mut(validator_index) - .ok_or(Error::UnknownValidator(validator_index))?; - let balance_copy = *balance; - *balance = 0_u64; - - let validator = self - .validators_mut() - .get_mut(validator_index) - .ok_or(Error::UnknownValidator(validator_index))?; - validator.effective_balance = 0; - validator.activation_eligibility_epoch = spec.far_future_epoch; - - self.pending_balance_deposits_mut()? - .push(PendingBalanceDeposit { - index: validator_index as u64, - amount: balance_copy, - }) - .map_err(Into::into) - } - /// Change the withdrawal prefix of the given `validator_index` to the compounding withdrawal validator prefix. pub fn switch_to_compounding_validator( &mut self, From 0a4e6be223f9ba1472a069930115e74f70c81663 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 10 Oct 2024 16:31:36 -0700 Subject: [PATCH 04/27] Switch to compounding when consolidating with source==target --- .../process_operations.rs | 83 +++++++++++++++---- .../src/per_epoch_processing/single_pass.rs | 3 - consensus/types/src/beacon_state.rs | 8 +- 3 files changed, 69 insertions(+), 25 deletions(-) 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 fb1c5c7eee2..97a283300aa 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -7,7 +7,6 @@ use crate::per_block_processing::errors::{BlockProcessingError, IntoWithIndex}; use crate::VerifySignatures; use types::consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR}; use types::typenum::U33; -use types::validator::is_compounding_withdrawal_credential; pub fn process_operations>( state: &mut BeaconState, @@ -452,18 +451,6 @@ pub fn apply_deposit( // [Modified in Electra:EIP7251] if let Ok(pending_balance_deposits) = state.pending_balance_deposits_mut() { pending_balance_deposits.push(PendingBalanceDeposit { index, amount })?; - - let validator = state - .validators() - .get(index as usize) - .ok_or(BeaconStateError::UnknownValidator(index as usize))?; - - if is_compounding_withdrawal_credential(deposit_data.withdrawal_credentials, spec) - && validator.has_eth1_withdrawal_credential(spec) - && is_valid_deposit_signature(&deposit_data, spec).is_ok() - { - state.switch_to_compounding_validator(index as usize, spec)?; - } } else { // Update the existing validator balance. increase_balance(state, index as usize, amount)?; @@ -658,11 +645,72 @@ pub fn process_consolidation_requests( Ok(()) } +fn is_valid_switch_to_compounding_request( + state: &BeaconState, + consolidation_request: &ConsolidationRequest, + spec: &ChainSpec, +) -> Result { + // Switch to compounding requires source and target be equal + if consolidation_request.source_pubkey != consolidation_request.target_pubkey { + return Ok(false); + } + + // Verify pubkey exists + let Some(source_index) = state + .pubkey_cache() + .get(&consolidation_request.source_pubkey) + else { + // source validator doesn't exist + return Ok(false); + }; + + let source_validator = state.get_validator(source_index)?; + // Verify the source withdrawal credentials + if let Some(withdrawal_address) = source_validator.get_execution_withdrawal_address(spec) { + if withdrawal_address != consolidation_request.source_address { + return Ok(false); + } + } else { + // Source doen't have execution withdrawal credentials + return Ok(false); + } + + // Verify the source is active + let current_epoch = state.current_epoch(); + if !source_validator.is_active_at(current_epoch) { + return Ok(false); + } + // Verify exits for source has not been initiated + if source_validator.exit_epoch != spec.far_future_epoch { + return Ok(false); + } + + Ok(true) +} + pub fn process_consolidation_request( state: &mut BeaconState, consolidation_request: &ConsolidationRequest, spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { + if is_valid_switch_to_compounding_request(state, consolidation_request, spec)? { + let Some(source_index) = state + .pubkey_cache() + .get(&consolidation_request.source_pubkey) + else { + // source validator doesn't exist. This is unreachable as `is_valid_switch_to_compounding_request` + // will return false in that case. + return Ok(()); + }; + state.switch_to_compounding_validator(source_index, spec)?; + return Ok(()); + } + + // Verify that source != target, so a consolidation cannot be used as an exit. + if consolidation_request.source_pubkey == consolidation_request.target_pubkey { + return Ok(()); + } + // If the pending consolidations queue is full, consolidation requests are ignored if state.pending_consolidations()?.len() == E::PendingConsolidationsLimit::to_usize() { return Ok(()); @@ -686,10 +734,6 @@ pub fn process_consolidation_request( // target validator doesn't exist return Ok(()); }; - // Verify that source != target, so a consolidation cannot be used as an exit. - if source_index == target_index { - return Ok(()); - } let source_validator = state.get_validator(source_index)?; // Verify the source withdrawal credentials @@ -736,5 +780,10 @@ pub fn process_consolidation_request( target_index: target_index as u64, })?; + let target_validator = state.get_validator(target_index)?; + // Churn any target excess active balance of target and raise its max + if target_validator.has_eth1_withdrawal_credential(spec) { + state.switch_to_compounding_validator(target_index, spec)?; + } Ok(()) } 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 1d37562a95c..2bf7f9feb93 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -952,9 +952,6 @@ fn process_pending_consolidations( max_effective_balance, ); - // Churn any target excess active balance of target and raise its max. - state.switch_to_compounding_validator(target_index, spec)?; - // Move active balance to target. Excess balance is withdrawable. decrease_balance(state, source_index, source_effective_balance)?; increase_balance(state, target_index, source_effective_balance)?; diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 0119c6a554e..2eaf95e02ab 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -2166,12 +2166,10 @@ impl BeaconState { .validators_mut() .get_mut(validator_index) .ok_or(Error::UnknownValidator(validator_index))?; - if validator.has_eth1_withdrawal_credential(spec) { - AsMut::<[u8; 32]>::as_mut(&mut validator.withdrawal_credentials)[0] = - spec.compounding_withdrawal_prefix_byte; + AsMut::<[u8; 32]>::as_mut(&mut validator.withdrawal_credentials)[0] = + spec.compounding_withdrawal_prefix_byte; - self.queue_excess_active_balance(validator_index, spec)?; - } + self.queue_excess_active_balance(validator_index, spec)?; Ok(()) } From c0cac0e2cd006884aade2301783330b49f5b9d79 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 11 Oct 2024 17:56:30 -0700 Subject: [PATCH 05/27] Queue deposit requests and apply them during epoch processing --- beacon_node/store/src/partial_beacon_state.rs | 6 +- consensus/state_processing/src/genesis.rs | 2 + .../process_operations.rs | 110 ++++++--- .../src/per_epoch_processing/single_pass.rs | 210 +++++++++++++----- .../state_processing/src/upgrade/electra.rs | 16 +- consensus/types/src/beacon_state.rs | 15 +- consensus/types/src/eth_spec.rs | 23 +- consensus/types/src/lib.rs | 4 +- ..._balance_deposit.rs => pending_deposit.rs} | 11 +- consensus/types/src/preset.rs | 2 +- testing/ef_tests/Makefile | 2 +- .../ef_tests/src/cases/epoch_processing.rs | 6 +- testing/ef_tests/src/type_name.rs | 2 +- testing/ef_tests/tests/tests.rs | 7 +- 14 files changed, 286 insertions(+), 130 deletions(-) rename consensus/types/src/{pending_balance_deposit.rs => pending_deposit.rs} (69%) diff --git a/beacon_node/store/src/partial_beacon_state.rs b/beacon_node/store/src/partial_beacon_state.rs index 8a66ec121e1..ea71ace53e2 100644 --- a/beacon_node/store/src/partial_beacon_state.rs +++ b/beacon_node/store/src/partial_beacon_state.rs @@ -134,7 +134,7 @@ where pub earliest_consolidation_epoch: Epoch, #[superstruct(only(Electra))] - pub pending_balance_deposits: List, + pub pending_deposits: List, #[superstruct(only(Electra))] pub pending_partial_withdrawals: List, @@ -290,7 +290,7 @@ impl PartialBeaconState { earliest_exit_epoch, consolidation_balance_to_consume, earliest_consolidation_epoch, - pending_balance_deposits, + pending_deposits, pending_partial_withdrawals, pending_consolidations ], @@ -563,7 +563,7 @@ impl TryInto> for PartialBeaconState { earliest_exit_epoch, consolidation_balance_to_consume, earliest_consolidation_epoch, - pending_balance_deposits, + pending_deposits, pending_partial_withdrawals, pending_consolidations ], diff --git a/consensus/state_processing/src/genesis.rs b/consensus/state_processing/src/genesis.rs index 00697def5d2..68309fec225 100644 --- a/consensus/state_processing/src/genesis.rs +++ b/consensus/state_processing/src/genesis.rs @@ -120,6 +120,8 @@ pub fn initialize_beacon_state_from_eth1( let post = upgrade_state_to_electra(&mut state, Epoch::new(0), Epoch::new(0), spec)?; state = post; + // TODO(electra): do we need to iterate over an empty pending_deposits list here and increase balance?! + // Remove intermediate Deneb fork from `state.fork`. state.fork_mut().previous_version = spec.electra_fork_version; 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 97a283300aa..8589287dbf4 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -5,6 +5,7 @@ use crate::common::{ }; use crate::per_block_processing::errors::{BlockProcessingError, IntoWithIndex}; use crate::VerifySignatures; +use errors::DepositInvalid; use types::consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR}; use types::typenum::U33; @@ -449,47 +450,73 @@ pub fn apply_deposit( if let Some(index) = validator_index { // [Modified in Electra:EIP7251] - if let Ok(pending_balance_deposits) = state.pending_balance_deposits_mut() { - pending_balance_deposits.push(PendingBalanceDeposit { index, amount })?; + if let Ok(pending_deposits) = state.pending_deposits_mut() { + pending_deposits.push(PendingDeposit { + pubkey: deposit_data.pubkey, + withdrawal_credentials: deposit_data.withdrawal_credentials, + amount, + signature: deposit_data.signature.decompress().map_err(|_| { + BlockProcessingError::DepositInvalid { + index: deposit_index, + reason: DepositInvalid::BadBlsBytes, + } + })?, + slot: spec.genesis_slot, // Use `genesis_slot` to distinguish from a pending deposit request + })?; } else { // Update the existing validator balance. increase_balance(state, index as usize, amount)?; } - } else { + } + // New validator + else { // The signature should be checked for new validators. Return early for a bad // signature. if is_valid_deposit_signature(&deposit_data, spec).is_err() { return Ok(()); } - let new_validator_index = state.validators().len(); - // [Modified in Electra:EIP7251] - let (effective_balance, state_balance) = if state.fork_name_unchecked() >= ForkName::Electra - { - (0, 0) + if state.fork_name_unchecked() >= ForkName::Electra { + // Create a new validator. + let mut validator = Validator { + pubkey: deposit_data.pubkey, + withdrawal_credentials: deposit_data.withdrawal_credentials, + activation_eligibility_epoch: spec.far_future_epoch, + activation_epoch: spec.far_future_epoch, + exit_epoch: spec.far_future_epoch, + withdrawable_epoch: spec.far_future_epoch, + effective_balance: 0, + slashed: false, + }; + let max_effective_balance = + validator.get_max_effective_balance(spec, state.fork_name_unchecked()); + validator.effective_balance = std::cmp::min( + amount.safe_sub(amount.safe_rem(spec.effective_balance_increment)?)?, + max_effective_balance, + ); + + state.validators_mut().push(validator)?; + // state balances is set to 0 in electra + state.balances_mut().push(0)?; } else { - ( - std::cmp::min( + let validator = Validator { + pubkey: deposit_data.pubkey, + withdrawal_credentials: deposit_data.withdrawal_credentials, + activation_eligibility_epoch: spec.far_future_epoch, + activation_epoch: spec.far_future_epoch, + exit_epoch: spec.far_future_epoch, + withdrawable_epoch: spec.far_future_epoch, + effective_balance: std::cmp::min( amount.safe_sub(amount.safe_rem(spec.effective_balance_increment)?)?, spec.max_effective_balance, ), - amount, - ) - }; - // Create a new validator. - let validator = Validator { - pubkey: deposit_data.pubkey, - withdrawal_credentials: deposit_data.withdrawal_credentials, - activation_eligibility_epoch: spec.far_future_epoch, - activation_epoch: spec.far_future_epoch, - exit_epoch: spec.far_future_epoch, - withdrawable_epoch: spec.far_future_epoch, - effective_balance, - slashed: false, + slashed: false, + }; + state.validators_mut().push(validator)?; + // state balances is set to 0 in electra + state.balances_mut().push(amount)?; }; - state.validators_mut().push(validator)?; - state.balances_mut().push(state_balance)?; // Altair or later initializations. if let Ok(previous_epoch_participation) = state.previous_epoch_participation_mut() { @@ -503,10 +530,18 @@ pub fn apply_deposit( } // [New in Electra:EIP7251] - if let Ok(pending_balance_deposits) = state.pending_balance_deposits_mut() { - pending_balance_deposits.push(PendingBalanceDeposit { - index: new_validator_index as u64, + if let Ok(pending_deposits) = state.pending_deposits_mut() { + pending_deposits.push(PendingDeposit { + pubkey: deposit_data.pubkey, + withdrawal_credentials: deposit_data.withdrawal_credentials, amount, + signature: deposit_data.signature.decompress().map_err(|_| { + BlockProcessingError::DepositInvalid { + index: deposit_index, + reason: DepositInvalid::BadBlsBytes, + } + })?, + slot: spec.genesis_slot, // Use `genesis_slot` to distinguish from a pending deposit request })?; } } @@ -620,13 +655,18 @@ pub fn process_deposit_requests( if state.deposit_requests_start_index()? == spec.unset_deposit_requests_start_index { *state.deposit_requests_start_index_mut()? = request.index } - let deposit_data = DepositData { - pubkey: request.pubkey, - withdrawal_credentials: request.withdrawal_credentials, - amount: request.amount, - signature: request.signature.clone().into(), - }; - apply_deposit(state, deposit_data, None, false, spec)? + let slot = state.slot(); + + // [New in Electra:EIP7251] + if let Ok(pending_deposits) = state.pending_deposits_mut() { + pending_deposits.push(PendingDeposit { + pubkey: request.pubkey, + withdrawal_credentials: request.withdrawal_credentials, + amount: request.amount, + signature: request.signature.clone(), + slot, + })?; + } } Ok(()) 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 2bf7f9feb93..6de4b9936e9 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -4,6 +4,7 @@ use crate::{ update_progressive_balances_cache::initialize_progressive_balances_cache, }, epoch_cache::{initialize_epoch_cache, PreEpochCache}, + per_block_processing::is_valid_deposit_signature, per_epoch_processing::{Delta, Error, ParticipationEpochSummary}, }; use itertools::izip; @@ -16,9 +17,9 @@ use types::{ TIMELY_TARGET_FLAG_INDEX, WEIGHT_DENOMINATOR, }, milhouse::Cow, - ActivationQueue, BeaconState, BeaconStateError, ChainSpec, Checkpoint, Epoch, EthSpec, - ExitCache, ForkName, List, ParticipationFlags, PendingBalanceDeposit, ProgressiveBalancesCache, - RelativeEpoch, Unsigned, Validator, + ActivationQueue, BeaconState, BeaconStateError, ChainSpec, Checkpoint, DepositData, Epoch, + EthSpec, ExitCache, ForkName, List, ParticipationFlags, PendingDeposit, + ProgressiveBalancesCache, RelativeEpoch, Unsigned, Validator, }; pub struct SinglePassConfig { @@ -26,7 +27,7 @@ pub struct SinglePassConfig { pub rewards_and_penalties: bool, pub registry_updates: bool, pub slashings: bool, - pub pending_balance_deposits: bool, + pub pending_deposits: bool, pub pending_consolidations: bool, pub effective_balance_updates: bool, } @@ -44,7 +45,7 @@ impl SinglePassConfig { rewards_and_penalties: true, registry_updates: true, slashings: true, - pending_balance_deposits: true, + pending_deposits: true, pending_consolidations: true, effective_balance_updates: true, } @@ -56,7 +57,7 @@ impl SinglePassConfig { rewards_and_penalties: false, registry_updates: false, slashings: false, - pending_balance_deposits: false, + pending_deposits: false, pending_consolidations: false, effective_balance_updates: false, } @@ -85,15 +86,17 @@ struct SlashingsContext { penalty_per_effective_balance_increment: u64, } -struct PendingBalanceDepositsContext { +struct PendingDepositsContext { /// The value to set `next_deposit_index` to *after* processing completes. next_deposit_index: usize, /// The value to set `deposit_balance_to_consume` to *after* processing completes. deposit_balance_to_consume: u64, /// Total balance increases for each validator due to pending balance deposits. validator_deposits_to_process: HashMap, - /// The deposits to append to `pending_balance_deposits` after processing all applicable deposits. - deposits_to_postpone: Vec, + /// The deposits to append to `pending_deposits` after processing all applicable deposits. + deposits_to_postpone: Vec, + /// New validators to be added to the state *after* processing completes. + new_validator_deposits: Vec, } struct EffectiveBalancesContext { @@ -138,6 +141,7 @@ pub fn process_epoch_single_pass( state.build_exit_cache(spec)?; state.build_committee_cache(RelativeEpoch::Previous, spec)?; state.build_committee_cache(RelativeEpoch::Current, spec)?; + state.update_pubkey_cache()?; let previous_epoch = state.previous_epoch(); let current_epoch = state.current_epoch(); @@ -163,12 +167,11 @@ pub fn process_epoch_single_pass( let slashings_ctxt = &SlashingsContext::new(state, state_ctxt, spec)?; let mut next_epoch_cache = PreEpochCache::new_for_next_epoch(state)?; - let pending_balance_deposits_ctxt = - if fork_name.electra_enabled() && conf.pending_balance_deposits { - Some(PendingBalanceDepositsContext::new(state, spec)?) - } else { - None - }; + let pending_deposits_ctxt = if fork_name.electra_enabled() && conf.pending_deposits { + Some(PendingDepositsContext::new(state, spec)?) + } else { + None + }; let mut earliest_exit_epoch = state.earliest_exit_epoch().ok(); let mut exit_balance_to_consume = state.exit_balance_to_consume().ok(); @@ -303,9 +306,9 @@ pub fn process_epoch_single_pass( process_single_slashing(&mut balance, &validator, slashings_ctxt, state_ctxt, spec)?; } - // `process_pending_balance_deposits` - if let Some(pending_balance_deposits_ctxt) = &pending_balance_deposits_ctxt { - process_pending_balance_deposits_for_validator( + // `process_pending_deposits` + if let Some(pending_balance_deposits_ctxt) = &pending_deposits_ctxt { + process_pending_deposits_for_validator( &mut balance, validator_info, pending_balance_deposits_ctxt, @@ -342,20 +345,66 @@ pub fn process_epoch_single_pass( // Finish processing pending balance deposits if relevant. // // This *could* be reordered after `process_pending_consolidations` which pushes only to the end - // of the `pending_balance_deposits` list. But we may as well preserve the write ordering used + // of the `pending_deposits` list. But we may as well preserve the write ordering used // by the spec and do this first. - if let Some(ctxt) = pending_balance_deposits_ctxt { - let mut new_pending_balance_deposits = List::try_from_iter( + if let Some(ctxt) = pending_deposits_ctxt { + let mut new_balance_deposits = List::try_from_iter( state - .pending_balance_deposits()? + .pending_deposits()? .iter_from(ctxt.next_deposit_index)? .cloned(), )?; for deposit in ctxt.deposits_to_postpone { - new_pending_balance_deposits.push(deposit)?; + new_balance_deposits.push(deposit)?; } - *state.pending_balance_deposits_mut()? = new_pending_balance_deposits; + *state.pending_deposits_mut()? = new_balance_deposits; *state.deposit_balance_to_consume_mut()? = ctxt.deposit_balance_to_consume; + + // Apply the new deposits to the state + for deposit in ctxt.new_validator_deposits.into_iter() { + if is_valid_deposit_signature( + &DepositData { + pubkey: deposit.pubkey, + withdrawal_credentials: deposit.withdrawal_credentials, + amount: deposit.amount, + signature: deposit.signature.into(), + }, + spec, + ) + .is_ok() + { + let mut validator = Validator { + pubkey: deposit.pubkey, + withdrawal_credentials: deposit.withdrawal_credentials, + activation_eligibility_epoch: spec.far_future_epoch, + activation_epoch: spec.far_future_epoch, + exit_epoch: spec.far_future_epoch, + withdrawable_epoch: spec.far_future_epoch, + effective_balance: 0, + slashed: false, + }; + let amount = deposit.amount; + let max_effective_balance = + validator.get_max_effective_balance(spec, state.fork_name_unchecked()); + validator.effective_balance = std::cmp::min( + amount.safe_sub(amount.safe_rem(spec.effective_balance_increment)?)?, + max_effective_balance, + ); + + state.validators_mut().push(validator)?; + state.balances_mut().push(amount)?; + // Altair or later initializations. + if let Ok(previous_epoch_participation) = state.previous_epoch_participation_mut() { + previous_epoch_participation.push(ParticipationFlags::default())?; + } + if let Ok(current_epoch_participation) = state.current_epoch_participation_mut() { + current_epoch_participation.push(ParticipationFlags::default())?; + } + if let Ok(inactivity_scores) = state.inactivity_scores_mut() { + inactivity_scores.push(0)?; + } + } + } } // Process consolidations outside the single-pass loop, as they depend on balances for multiple @@ -819,7 +868,7 @@ fn process_single_slashing( Ok(()) } -impl PendingBalanceDepositsContext { +impl PendingDepositsContext { fn new(state: &BeaconState, spec: &ChainSpec) -> Result { let available_for_processing = state .deposit_balance_to_consume()? @@ -830,10 +879,31 @@ impl PendingBalanceDepositsContext { let mut next_deposit_index = 0; let mut validator_deposits_to_process = HashMap::new(); let mut deposits_to_postpone = vec![]; - - let pending_balance_deposits = state.pending_balance_deposits()?; - - for deposit in pending_balance_deposits.iter() { + let mut new_validator_deposits = vec![]; + let mut is_churn_limit_reached = false; + let finalized_slot = state + .finalized_checkpoint() + .epoch + .start_slot(E::slots_per_epoch()); + + let pending_deposits = state.pending_deposits()?; + + for deposit in pending_deposits.iter() { + // Do not process deposit requests if the Eth1 bridge deposits are not yet applied. + if deposit.slot > spec.genesis_slot + && state.eth1_deposit_index() < state.deposit_requests_start_index()? + { + break; + } + // Do not process is deposit slot has not been finalized. + if deposit.slot > finalized_slot { + break; + } + // Do not process if we have reached the limit for the number of deposits + // processed in an epoch. + if next_deposit_index >= E::max_pending_deposits_per_epoch() { + break; + } // We have to do a bit of indexing into `validators` here, but I can't see any way // around that without changing the spec. // @@ -844,48 +914,71 @@ impl PendingBalanceDepositsContext { // take, just whether it is non-default. Nor do we need to know the value of // `withdrawable_epoch`, because `next_epoch <= withdrawable_epoch` will evaluate to // `true` both for the actual value & the default placeholder value (`FAR_FUTURE_EPOCH`). - let validator = state.get_validator(deposit.index as usize)?; - let already_exited = validator.exit_epoch < spec.far_future_epoch; - // In the spec process_registry_updates is called before process_pending_balance_deposits - // so we must account for process_registry_updates ejecting the validator for low balance - // and setting the exit_epoch to < far_future_epoch. Note that in the spec the effective - // balance update does not happen until *after* the registry update, so we don't need to - // account for changes to the effective balance that would push it below the ejection - // balance here. - let will_be_exited = validator.is_active_at(current_epoch) - && validator.effective_balance <= spec.ejection_balance; - if already_exited || will_be_exited { - if next_epoch <= validator.withdrawable_epoch { - deposits_to_postpone.push(deposit.clone()); - } else { - // Deposited balance will never become active. Increase balance but do not - // consume churn. + let mut is_validator_exited = false; + let mut is_validator_withdrawn = false; + if let Some(validator_index) = state.pubkey_cache().get(&deposit.pubkey) { + let validator = state.get_validator(validator_index)?; + let already_exited = validator.exit_epoch < spec.far_future_epoch; + // In the spec process_registry_updates is called before process_pending_deposits + // so we must account for process_registry_updates ejecting the validator for low balance + // and setting the exit_epoch to < far_future_epoch. Note that in the spec the effective + // balance update does not happen until *after* the registry update, so we don't need to + // account for changes to the effective balance that would push it below the ejection + // balance here. + let will_be_exited = validator.is_active_at(current_epoch) + && validator.effective_balance <= spec.ejection_balance; + is_validator_exited = already_exited || will_be_exited; + is_validator_withdrawn = validator.withdrawable_epoch < next_epoch; + } + + if is_validator_withdrawn { + // Deposited balance will never become active. Queue a balance increase + // but do not consume churn + if let Some(validator_index) = state.pubkey_cache().get(&deposit.pubkey) { validator_deposits_to_process - .entry(deposit.index as usize) + .entry(validator_index) .or_insert(0) .safe_add_assign(deposit.amount)?; + } else { + // The `PendingDeposit` is for a new validator + // We add the new validator to the state at the end of all processing. + // Note that the new validator will not be eligible for activation until + // next epoch, so none of the operations will affect a newly added validator. + new_validator_deposits.push(deposit.clone()); } + } else if is_validator_exited { + // Validator is exiting, postpone the deposit until after withdrawable epoch + deposits_to_postpone.push(deposit.clone()); } else { - // Deposit does not fit in the churn, no more deposit processing in this epoch. - if processed_amount.safe_add(deposit.amount)? > available_for_processing { + // Check if deposit fits in the churn, otherwise, do no more deposit processing in this epoch. + is_churn_limit_reached = + processed_amount.safe_add(deposit.amount)? > available_for_processing; + if is_churn_limit_reached { break; } - // Deposit fits in the churn, process it. Increase balance and consume churn. - validator_deposits_to_process - .entry(deposit.index as usize) - .or_insert(0) - .safe_add_assign(deposit.amount)?; processed_amount.safe_add_assign(deposit.amount)?; + + // Deposit fits in the churn, process it. Increase balance and consume churn. + if let Some(validator_index) = state.pubkey_cache().get(&deposit.pubkey) { + validator_deposits_to_process + .entry(validator_index) + .or_insert(0) + .safe_add_assign(deposit.amount)?; + } else { + // The `PendingDeposit` is for a new validator + new_validator_deposits.push(deposit.clone()); + } } // Regardless of how the deposit was handled, we move on in the queue. next_deposit_index.safe_add_assign(1)?; } - let deposit_balance_to_consume = if next_deposit_index == pending_balance_deposits.len() { - 0 - } else { + // Accumulate churn only if the churn limit has been hit. + let deposit_balance_to_consume = if is_churn_limit_reached { available_for_processing.safe_sub(processed_amount)? + } else { + 0 }; Ok(Self { @@ -893,14 +986,15 @@ impl PendingBalanceDepositsContext { deposit_balance_to_consume, validator_deposits_to_process, deposits_to_postpone, + new_validator_deposits, }) } } -fn process_pending_balance_deposits_for_validator( +fn process_pending_deposits_for_validator( balance: &mut Cow, validator_info: &ValidatorInfo, - pending_balance_deposits_ctxt: &PendingBalanceDepositsContext, + pending_balance_deposits_ctxt: &PendingDepositsContext, ) -> Result<(), Error> { if let Some(deposit_amount) = pending_balance_deposits_ctxt .validator_deposits_to_process diff --git a/consensus/state_processing/src/upgrade/electra.rs b/consensus/state_processing/src/upgrade/electra.rs index 8d5a2feb11f..270e943bd32 100644 --- a/consensus/state_processing/src/upgrade/electra.rs +++ b/consensus/state_processing/src/upgrade/electra.rs @@ -1,8 +1,9 @@ +use bls::Signature; use safe_arith::SafeArith; use std::mem; use types::{ BeaconState, BeaconStateElectra, BeaconStateError as Error, ChainSpec, Epoch, EpochCache, - EthSpec, Fork, PendingBalanceDeposit, + EthSpec, Fork, PendingDeposit, }; /// Transform a `Deneb` state into an `Electra` state. @@ -70,11 +71,16 @@ pub fn upgrade_to_electra( .ok_or(Error::UnknownValidator(index))?; validator.effective_balance = 0; validator.activation_eligibility_epoch = spec.far_future_epoch; + let pubkey = validator.pubkey; + let withdrawal_credentials = validator.withdrawal_credentials; - post.pending_balance_deposits_mut()? - .push(PendingBalanceDeposit { - index: index as u64, + post.pending_deposits_mut()? + .push(PendingDeposit { + pubkey, + withdrawal_credentials, amount: balance_copy, + signature: Signature::infinity()?, + slot: spec.genesis_slot, }) .map_err(Error::MilhouseError)?; } @@ -156,7 +162,7 @@ pub fn upgrade_state_to_electra( earliest_exit_epoch, consolidation_balance_to_consume: 0, earliest_consolidation_epoch, - pending_balance_deposits: Default::default(), + pending_deposits: Default::default(), pending_partial_withdrawals: Default::default(), pending_consolidations: Default::default(), // Caches diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 2eaf95e02ab..c04bc6326c4 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -510,7 +510,7 @@ where #[compare_fields(as_iter)] #[test_random(default)] #[superstruct(only(Electra))] - pub pending_balance_deposits: List, + pub pending_deposits: List, #[compare_fields(as_iter)] #[test_random(default)] #[superstruct(only(Electra))] @@ -2147,11 +2147,14 @@ impl BeaconState { if *balance > spec.min_activation_balance { let excess_balance = balance.safe_sub(spec.min_activation_balance)?; *balance = spec.min_activation_balance; - self.pending_balance_deposits_mut()? - .push(PendingBalanceDeposit { - index: validator_index as u64, - amount: excess_balance, - })?; + let validator = self.get_validator(validator_index)?.clone(); + self.pending_deposits_mut()?.push(PendingDeposit { + pubkey: validator.pubkey, + withdrawal_credentials: validator.withdrawal_credentials, + amount: excess_balance, + signature: Signature::infinity()?, + slot: spec.genesis_slot, + })?; } Ok(()) } diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index 09ef8e3c1a7..23e82762096 100644 --- a/consensus/types/src/eth_spec.rs +++ b/consensus/types/src/eth_spec.rs @@ -151,7 +151,7 @@ pub trait EthSpec: /* * New in Electra */ - type PendingBalanceDepositsLimit: Unsigned + Clone + Sync + Send + Debug + PartialEq; + type PendingDepositsLimit: Unsigned + Clone + Sync + Send + Debug + PartialEq; type PendingPartialWithdrawalsLimit: Unsigned + Clone + Sync + Send + Debug + PartialEq; type PendingConsolidationsLimit: Unsigned + Clone + Sync + Send + Debug + PartialEq; type MaxConsolidationRequestsPerPayload: Unsigned + Clone + Sync + Send + Debug + PartialEq; @@ -159,6 +159,7 @@ pub trait EthSpec: type MaxAttesterSlashingsElectra: Unsigned + Clone + Sync + Send + Debug + PartialEq; type MaxAttestationsElectra: Unsigned + Clone + Sync + Send + Debug + PartialEq; type MaxWithdrawalRequestsPerPayload: Unsigned + Clone + Sync + Send + Debug + PartialEq; + type MaxPendingDepositsPerEpoch: Unsigned + Clone + Sync + Send + Debug + PartialEq; fn default_spec() -> ChainSpec; @@ -331,9 +332,9 @@ pub trait EthSpec: .expect("Preset values are not configurable and never result in non-positive block body depth") } - /// Returns the `PENDING_BALANCE_DEPOSITS_LIMIT` constant for this specification. - fn pending_balance_deposits_limit() -> usize { - Self::PendingBalanceDepositsLimit::to_usize() + /// Returns the `PENDING_DEPOSITS_LIMIT` constant for this specification. + fn pending_deposits_limit() -> usize { + Self::PendingDepositsLimit::to_usize() } /// Returns the `PENDING_PARTIAL_WITHDRAWALS_LIMIT` constant for this specification. @@ -371,6 +372,11 @@ pub trait EthSpec: Self::MaxWithdrawalRequestsPerPayload::to_usize() } + /// Returns the `MAX_PENDING_DEPOSITS_PER_EPOCH` constant for this specification. + fn max_pending_deposits_per_epoch() -> usize { + Self::MaxPendingDepositsPerEpoch::to_usize() + } + fn kzg_commitments_inclusion_proof_depth() -> usize { Self::KzgCommitmentsInclusionProofDepth::to_usize() } @@ -430,7 +436,7 @@ impl EthSpec for MainnetEthSpec { type SlotsPerEth1VotingPeriod = U2048; // 64 epochs * 32 slots per epoch type MaxBlsToExecutionChanges = U16; type MaxWithdrawalsPerPayload = U16; - type PendingBalanceDepositsLimit = U134217728; + type PendingDepositsLimit = U134217728; type PendingPartialWithdrawalsLimit = U134217728; type PendingConsolidationsLimit = U262144; type MaxConsolidationRequestsPerPayload = U1; @@ -438,6 +444,7 @@ impl EthSpec for MainnetEthSpec { type MaxAttesterSlashingsElectra = U1; type MaxAttestationsElectra = U8; type MaxWithdrawalRequestsPerPayload = U16; + type MaxPendingDepositsPerEpoch = U16; fn default_spec() -> ChainSpec { ChainSpec::mainnet() @@ -500,7 +507,8 @@ impl EthSpec for MinimalEthSpec { MaxBlsToExecutionChanges, MaxBlobsPerBlock, BytesPerFieldElement, - PendingBalanceDepositsLimit, + PendingDepositsLimit, + MaxPendingDepositsPerEpoch, MaxConsolidationRequestsPerPayload, MaxAttesterSlashingsElectra, MaxAttestationsElectra @@ -557,7 +565,7 @@ impl EthSpec for GnosisEthSpec { type BytesPerFieldElement = U32; type BytesPerBlob = U131072; type KzgCommitmentInclusionProofDepth = U17; - type PendingBalanceDepositsLimit = U134217728; + type PendingDepositsLimit = U134217728; type PendingPartialWithdrawalsLimit = U134217728; type PendingConsolidationsLimit = U262144; type MaxConsolidationRequestsPerPayload = U1; @@ -565,6 +573,7 @@ impl EthSpec for GnosisEthSpec { type MaxAttesterSlashingsElectra = U1; type MaxAttestationsElectra = U8; type MaxWithdrawalRequestsPerPayload = U16; + type MaxPendingDepositsPerEpoch = U16; type FieldElementsPerCell = U64; type FieldElementsPerExtBlob = U8192; type BytesPerCell = U2048; diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index e168199b98c..c28356930b9 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -54,8 +54,8 @@ pub mod light_client_finality_update; pub mod light_client_optimistic_update; pub mod light_client_update; pub mod pending_attestation; -pub mod pending_balance_deposit; pub mod pending_consolidation; +pub mod pending_deposit; pub mod pending_partial_withdrawal; pub mod proposer_preparation_data; pub mod proposer_slashing; @@ -210,8 +210,8 @@ pub use crate::payload::{ FullPayloadRef, OwnedExecPayload, }; pub use crate::pending_attestation::PendingAttestation; -pub use crate::pending_balance_deposit::PendingBalanceDeposit; pub use crate::pending_consolidation::PendingConsolidation; +pub use crate::pending_deposit::PendingDeposit; pub use crate::pending_partial_withdrawal::PendingPartialWithdrawal; pub use crate::preset::{ AltairPreset, BasePreset, BellatrixPreset, CapellaPreset, DenebPreset, ElectraPreset, diff --git a/consensus/types/src/pending_balance_deposit.rs b/consensus/types/src/pending_deposit.rs similarity index 69% rename from consensus/types/src/pending_balance_deposit.rs rename to consensus/types/src/pending_deposit.rs index a2bce577f87..9ee920f1d9b 100644 --- a/consensus/types/src/pending_balance_deposit.rs +++ b/consensus/types/src/pending_deposit.rs @@ -1,4 +1,5 @@ use crate::test_utils::TestRandom; +use crate::*; use serde::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; use test_random_derive::TestRandom; @@ -18,16 +19,18 @@ use tree_hash_derive::TreeHash; TreeHash, TestRandom, )] -pub struct PendingBalanceDeposit { - #[serde(with = "serde_utils::quoted_u64")] - pub index: u64, +pub struct PendingDeposit { + pub pubkey: PublicKeyBytes, + pub withdrawal_credentials: Hash256, #[serde(with = "serde_utils::quoted_u64")] pub amount: u64, + pub signature: Signature, + pub slot: Slot, } #[cfg(test)] mod tests { use super::*; - ssz_and_tree_hash_tests!(PendingBalanceDeposit); + ssz_and_tree_hash_tests!(PendingDeposit); } diff --git a/consensus/types/src/preset.rs b/consensus/types/src/preset.rs index 2c576ed332c..694ab9af0e9 100644 --- a/consensus/types/src/preset.rs +++ b/consensus/types/src/preset.rs @@ -266,7 +266,7 @@ impl ElectraPreset { whistleblower_reward_quotient_electra: spec.whistleblower_reward_quotient_electra, max_pending_partials_per_withdrawals_sweep: spec .max_pending_partials_per_withdrawals_sweep, - pending_balance_deposits_limit: E::pending_balance_deposits_limit() as u64, + pending_balance_deposits_limit: E::pending_deposits_limit() as u64, pending_partial_withdrawals_limit: E::pending_partial_withdrawals_limit() as u64, pending_consolidations_limit: E::pending_consolidations_limit() as u64, max_consolidation_requests_per_payload: E::max_consolidation_requests_per_payload() diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index 390711079f4..b0f97292a0b 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,4 +1,4 @@ -TESTS_TAG := v1.5.0-alpha.6 +TESTS_TAG := v1.5.0-alpha.7 TESTS = general minimal mainnet TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) diff --git a/testing/ef_tests/src/cases/epoch_processing.rs b/testing/ef_tests/src/cases/epoch_processing.rs index dfd782a22b3..c1adf107704 100644 --- a/testing/ef_tests/src/cases/epoch_processing.rs +++ b/testing/ef_tests/src/cases/epoch_processing.rs @@ -86,7 +86,7 @@ type_name!(RewardsAndPenalties, "rewards_and_penalties"); type_name!(RegistryUpdates, "registry_updates"); type_name!(Slashings, "slashings"); type_name!(Eth1DataReset, "eth1_data_reset"); -type_name!(PendingBalanceDeposits, "pending_balance_deposits"); +type_name!(PendingBalanceDeposits, "pending_deposits"); type_name!(PendingConsolidations, "pending_consolidations"); type_name!(EffectiveBalanceUpdates, "effective_balance_updates"); type_name!(SlashingsReset, "slashings_reset"); @@ -193,7 +193,7 @@ impl EpochTransition for PendingBalanceDeposits { state, spec, SinglePassConfig { - pending_balance_deposits: true, + pending_deposits: true, ..SinglePassConfig::disable_all() }, ) @@ -363,7 +363,7 @@ impl> Case for EpochProcessing { } if !fork_name.electra_enabled() - && (T::name() == "pending_consolidations" || T::name() == "pending_balance_deposits") + && (T::name() == "pending_consolidations" || T::name() == "pending_deposits") { return false; } diff --git a/testing/ef_tests/src/type_name.rs b/testing/ef_tests/src/type_name.rs index a9322e5dd5e..c50032a63de 100644 --- a/testing/ef_tests/src/type_name.rs +++ b/testing/ef_tests/src/type_name.rs @@ -134,7 +134,7 @@ type_name_generic!(LightClientUpdateElectra, "LightClientUpdate"); type_name_generic!(PendingAttestation); type_name!(PendingConsolidation); type_name!(PendingPartialWithdrawal); -type_name!(PendingBalanceDeposit); +type_name!(PendingDeposit); type_name!(ProposerSlashing); type_name_generic!(SignedAggregateAndProof); type_name_generic!(SignedAggregateAndProofBase, "SignedAggregateAndProof"); diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 1812a101ca6..1184fbb5140 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -243,8 +243,7 @@ mod ssz_static { use types::historical_summary::HistoricalSummary; use types::{ AttesterSlashingBase, AttesterSlashingElectra, ConsolidationRequest, DepositRequest, - LightClientBootstrapAltair, PendingBalanceDeposit, PendingPartialWithdrawal, - WithdrawalRequest, *, + LightClientBootstrapAltair, PendingDeposit, PendingPartialWithdrawal, WithdrawalRequest, *, }; ssz_static_test!(attestation_data, AttestationData); @@ -664,8 +663,8 @@ mod ssz_static { #[test] fn pending_balance_deposit() { - SszStaticHandler::::electra_and_later().run(); - SszStaticHandler::::electra_and_later().run(); + SszStaticHandler::::electra_and_later().run(); + SszStaticHandler::::electra_and_later().run(); } #[test] From b28d0106299345b3b72e1fedd2c780c9b393c75f Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 16 Oct 2024 14:13:57 -0700 Subject: [PATCH 06/27] Fix ef tests --- .../process_operations.rs | 20 ++++++------------- .../src/per_epoch_processing/single_pass.rs | 17 +++++++++++----- .../state_processing/src/upgrade/electra.rs | 3 ++- consensus/types/src/beacon_state.rs | 2 +- consensus/types/src/deposit_request.rs | 8 ++++---- consensus/types/src/pending_deposit.rs | 3 +-- consensus/types/src/validator.rs | 11 ++++++++++ testing/ef_tests/Makefile | 2 +- 8 files changed, 38 insertions(+), 28 deletions(-) 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 8589287dbf4..50cf1b8151e 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -5,7 +5,6 @@ use crate::common::{ }; use crate::per_block_processing::errors::{BlockProcessingError, IntoWithIndex}; use crate::VerifySignatures; -use errors::DepositInvalid; use types::consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR}; use types::typenum::U33; @@ -455,12 +454,7 @@ pub fn apply_deposit( pubkey: deposit_data.pubkey, withdrawal_credentials: deposit_data.withdrawal_credentials, amount, - signature: deposit_data.signature.decompress().map_err(|_| { - BlockProcessingError::DepositInvalid { - index: deposit_index, - reason: DepositInvalid::BadBlsBytes, - } - })?, + signature: deposit_data.signature, slot: spec.genesis_slot, // Use `genesis_slot` to distinguish from a pending deposit request })?; } else { @@ -478,6 +472,7 @@ pub fn apply_deposit( // [Modified in Electra:EIP7251] if state.fork_name_unchecked() >= ForkName::Electra { + let amount = 0; // Create a new validator. let mut validator = Validator { pubkey: deposit_data.pubkey, @@ -535,12 +530,7 @@ pub fn apply_deposit( pubkey: deposit_data.pubkey, withdrawal_credentials: deposit_data.withdrawal_credentials, amount, - signature: deposit_data.signature.decompress().map_err(|_| { - BlockProcessingError::DepositInvalid { - index: deposit_index, - reason: DepositInvalid::BadBlsBytes, - } - })?, + signature: deposit_data.signature, slot: spec.genesis_slot, // Use `genesis_slot` to distinguish from a pending deposit request })?; } @@ -706,7 +696,7 @@ fn is_valid_switch_to_compounding_request( let source_validator = state.get_validator(source_index)?; // Verify the source withdrawal credentials - if let Some(withdrawal_address) = source_validator.get_execution_withdrawal_address(spec) { + if let Some(withdrawal_address) = source_validator.get_eth1_withdrawal_credential(spec) { if withdrawal_address != consolidation_request.source_address { return Ok(false); } @@ -721,6 +711,7 @@ fn is_valid_switch_to_compounding_request( return Ok(false); } // Verify exits for source has not been initiated + // TODO(pawan): this could be set by process_registry_updates too if source_validator.exit_epoch != spec.far_future_epoch { return Ok(false); } @@ -733,6 +724,7 @@ pub fn process_consolidation_request( consolidation_request: &ConsolidationRequest, spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { + dbg!("here"); if is_valid_switch_to_compounding_request(state, consolidation_request, spec)? { let Some(source_index) = state .pubkey_cache() 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 6de4b9936e9..e49b98700f7 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -168,7 +168,7 @@ pub fn process_epoch_single_pass( let mut next_epoch_cache = PreEpochCache::new_for_next_epoch(state)?; let pending_deposits_ctxt = if fork_name.electra_enabled() && conf.pending_deposits { - Some(PendingDepositsContext::new(state, spec)?) + Some(PendingDepositsContext::new(state, spec, &conf)?) } else { None }; @@ -367,7 +367,7 @@ pub fn process_epoch_single_pass( pubkey: deposit.pubkey, withdrawal_credentials: deposit.withdrawal_credentials, amount: deposit.amount, - signature: deposit.signature.into(), + signature: deposit.signature, }, spec, ) @@ -869,7 +869,11 @@ fn process_single_slashing( } impl PendingDepositsContext { - fn new(state: &BeaconState, spec: &ChainSpec) -> Result { + fn new( + state: &BeaconState, + spec: &ChainSpec, + config: &SinglePassConfig, + ) -> Result { let available_for_processing = state .deposit_balance_to_consume()? .safe_add(state.get_activation_exit_churn_limit(spec)?)?; @@ -925,8 +929,11 @@ impl PendingDepositsContext { // balance update does not happen until *after* the registry update, so we don't need to // account for changes to the effective balance that would push it below the ejection // balance here. - let will_be_exited = validator.is_active_at(current_epoch) - && validator.effective_balance <= spec.ejection_balance; + // Note: we only consider this if registry_updates are enabled in the config. + // EF tests require us to run epoch_processing functions in isolation. + let will_be_exited = config.registry_updates + && (validator.is_active_at(current_epoch) + && validator.effective_balance <= spec.ejection_balance); is_validator_exited = already_exited || will_be_exited; is_validator_withdrawn = validator.withdrawable_epoch < next_epoch; } diff --git a/consensus/state_processing/src/upgrade/electra.rs b/consensus/state_processing/src/upgrade/electra.rs index 270e943bd32..7287bbff080 100644 --- a/consensus/state_processing/src/upgrade/electra.rs +++ b/consensus/state_processing/src/upgrade/electra.rs @@ -73,13 +73,14 @@ pub fn upgrade_to_electra( validator.activation_eligibility_epoch = spec.far_future_epoch; let pubkey = validator.pubkey; let withdrawal_credentials = validator.withdrawal_credentials; + dbg!("is this getting hit"); post.pending_deposits_mut()? .push(PendingDeposit { pubkey, withdrawal_credentials, amount: balance_copy, - signature: Signature::infinity()?, + signature: Signature::infinity()?.into(), slot: spec.genesis_slot, }) .map_err(Error::MilhouseError)?; diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index c04bc6326c4..38bbf0feee8 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -2152,7 +2152,7 @@ impl BeaconState { pubkey: validator.pubkey, withdrawal_credentials: validator.withdrawal_credentials, amount: excess_balance, - signature: Signature::infinity()?, + signature: Signature::infinity()?.into(), slot: spec.genesis_slot, })?; } diff --git a/consensus/types/src/deposit_request.rs b/consensus/types/src/deposit_request.rs index 7af949fef3a..a21760551b5 100644 --- a/consensus/types/src/deposit_request.rs +++ b/consensus/types/src/deposit_request.rs @@ -1,5 +1,6 @@ use crate::test_utils::TestRandom; -use crate::{Hash256, PublicKeyBytes, Signature}; +use crate::{Hash256, PublicKeyBytes}; +use bls::SignatureBytes; use serde::{Deserialize, Serialize}; use ssz::Encode; use ssz_derive::{Decode, Encode}; @@ -10,7 +11,6 @@ use tree_hash_derive::TreeHash; arbitrary::Arbitrary, Debug, PartialEq, - Eq, Hash, Clone, Serialize, @@ -25,7 +25,7 @@ pub struct DepositRequest { pub withdrawal_credentials: Hash256, #[serde(with = "serde_utils::quoted_u64")] pub amount: u64, - pub signature: Signature, + pub signature: SignatureBytes, #[serde(with = "serde_utils::quoted_u64")] pub index: u64, } @@ -36,7 +36,7 @@ impl DepositRequest { pubkey: PublicKeyBytes::empty(), withdrawal_credentials: Hash256::ZERO, amount: 0, - signature: Signature::empty(), + signature: SignatureBytes::empty(), index: 0, } .as_ssz_bytes() diff --git a/consensus/types/src/pending_deposit.rs b/consensus/types/src/pending_deposit.rs index 9ee920f1d9b..3bee86417de 100644 --- a/consensus/types/src/pending_deposit.rs +++ b/consensus/types/src/pending_deposit.rs @@ -9,7 +9,6 @@ use tree_hash_derive::TreeHash; arbitrary::Arbitrary, Debug, PartialEq, - Eq, Hash, Clone, Serialize, @@ -24,7 +23,7 @@ pub struct PendingDeposit { pub withdrawal_credentials: Hash256, #[serde(with = "serde_utils::quoted_u64")] pub amount: u64, - pub signature: Signature, + pub signature: SignatureBytes, pub slot: Slot, } diff --git a/consensus/types/src/validator.rs b/consensus/types/src/validator.rs index e94f4b4a0ab..941c420b559 100644 --- a/consensus/types/src/validator.rs +++ b/consensus/types/src/validator.rs @@ -152,6 +152,17 @@ impl Validator { .flatten() } + pub fn get_eth1_withdrawal_credential(&self, spec: &ChainSpec) -> Option
{ + self.has_eth1_withdrawal_credential(spec) + .then(|| { + self.withdrawal_credentials + .as_slice() + .get(12..) + .map(Address::from_slice) + }) + .flatten() + } + /// Changes withdrawal credentials to the provided eth1 execution address. /// /// WARNING: this function does NO VALIDATION - it just does it! diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index b0f97292a0b..d5f4997bb7e 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,4 +1,4 @@ -TESTS_TAG := v1.5.0-alpha.7 +TESTS_TAG := v1.5.0-alpha.8 TESTS = general minimal mainnet TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) From c8dfe7d3d71bbad3bf967c69cd24949d73020acc Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 16 Oct 2024 15:40:46 -0700 Subject: [PATCH 07/27] Clear todos --- consensus/state_processing/src/genesis.rs | 3 ++- .../src/per_block_processing/process_operations.rs | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/consensus/state_processing/src/genesis.rs b/consensus/state_processing/src/genesis.rs index 68309fec225..4450b999305 100644 --- a/consensus/state_processing/src/genesis.rs +++ b/consensus/state_processing/src/genesis.rs @@ -120,7 +120,8 @@ pub fn initialize_beacon_state_from_eth1( let post = upgrade_state_to_electra(&mut state, Epoch::new(0), Epoch::new(0), spec)?; state = post; - // TODO(electra): do we need to iterate over an empty pending_deposits list here and increase balance?! + // TODO(electra): do we need to iterate over an empty pending_deposits list here and increase balance? + // in accordance with `initialize_beacon_state_from_eth1` function from the spec // Remove intermediate Deneb fork from `state.fork`. state.fork_mut().previous_version = spec.electra_fork_version; 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 50cf1b8151e..81998906cf2 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -711,7 +711,6 @@ fn is_valid_switch_to_compounding_request( return Ok(false); } // Verify exits for source has not been initiated - // TODO(pawan): this could be set by process_registry_updates too if source_validator.exit_epoch != spec.far_future_epoch { return Ok(false); } From 0dd215c294d3c7c3c05bfc1f438cf9d9c43670ec Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 16 Oct 2024 17:30:10 -0700 Subject: [PATCH 08/27] Fix engine api formatting issues --- beacon_node/execution_layer/src/engine_api/http.rs | 2 +- .../execution_layer/src/engine_api/json_structures.rs | 7 ++++--- .../execution_layer/src/engine_api/new_payload_request.rs | 4 ++-- beacon_node/execution_layer/src/test_utils/handle_rpc.rs | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 9c2c43bcf7c..250b3538792 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -794,7 +794,7 @@ impl HttpJsonRpc { new_payload_request_electra.versioned_hashes, new_payload_request_electra.parent_beacon_block_root, new_payload_request_electra - .execution_requests_list + .execution_requests .get_execution_requests_list(), ]); diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index 753554c149a..1431a81acc5 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -379,7 +379,8 @@ impl TryFrom for ExecutionRequests { for (i, request) in value.0.into_iter().enumerate() { // hex string - let decoded_bytes = hex::decode(request).map_err(|e| format!("Invalid hex {:?}", e))?; + let decoded_bytes = hex::decode(request.strip_prefix("0x").unwrap_or(&request)) + .map_err(|e| format!("Invalid hex {:?}", e))?; match RequestPrefix::from_prefix(i as u8) { Some(RequestPrefix::Deposit) => { requests.deposits = DepositRequests::::from_ssz_bytes(&decoded_bytes) @@ -431,7 +432,7 @@ pub struct JsonGetPayloadResponse { #[superstruct(only(V3, V4))] pub should_override_builder: bool, #[superstruct(only(V4))] - pub requests: JsonExecutionRequests, + pub execution_requests: JsonExecutionRequests, } impl TryFrom> for GetPayloadResponse { @@ -464,7 +465,7 @@ impl TryFrom> for GetPayloadResponse { block_value: response.block_value, blobs_bundle: response.blobs_bundle.into(), should_override_builder: response.should_override_builder, - requests: response.requests.try_into()?, + requests: response.execution_requests.try_into()?, })) } } diff --git a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs index 318779b7f3e..6f2261a5c73 100644 --- a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs +++ b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs @@ -44,7 +44,7 @@ pub struct NewPayloadRequest<'block, E: EthSpec> { #[superstruct(only(Deneb, Electra))] pub parent_beacon_block_root: Hash256, #[superstruct(only(Electra))] - pub execution_requests_list: &'block ExecutionRequests, + pub execution_requests: &'block ExecutionRequests, } impl<'block, E: EthSpec> NewPayloadRequest<'block, E> { @@ -185,7 +185,7 @@ impl<'a, E: EthSpec> TryFrom> for NewPayloadRequest<'a, E> .map(kzg_commitment_to_versioned_hash) .collect(), parent_beacon_block_root: block_ref.parent_root, - execution_requests_list: &block_ref.body.execution_requests, + execution_requests: &block_ref.body.execution_requests, })), } } diff --git a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs index 786ac9ad9c9..9365024ffb7 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -374,7 +374,7 @@ pub async fn handle_rpc( .into(), should_override_builder: false, // TODO(electra): add EL requests in mock el - requests: Default::default(), + execution_requests: Default::default(), }) .unwrap() } From 37c765809a57f0e5a663636aa84bec5e3d10838a Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 29 Oct 2024 12:27:15 -0700 Subject: [PATCH 09/27] Make add_validator_to_registry more in line with the spec --- .../process_operations.rs | 11 +++- .../src/per_epoch_processing/single_pass.rs | 54 +++++-------------- consensus/types/src/beacon_state.rs | 20 +++---- consensus/types/src/validator.rs | 11 ++-- 4 files changed, 40 insertions(+), 56 deletions(-) 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 5d862899bc5..a405156acbb 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -470,7 +470,16 @@ pub fn apply_deposit( return Ok(()); } - state.add_validator_to_registry(&deposit_data, spec)?; + state.add_validator_to_registry( + deposit_data.pubkey, + deposit_data.withdrawal_credentials, + if state.fork_name_unchecked() >= ForkName::Electra { + 0 + } else { + amount + }, + spec, + )?; // [New in Electra:EIP7251] if let Ok(pending_deposits) = state.pending_deposits_mut() { 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 e49b98700f7..e79a6fb6fba 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -362,47 +362,19 @@ pub fn process_epoch_single_pass( // Apply the new deposits to the state for deposit in ctxt.new_validator_deposits.into_iter() { - if is_valid_deposit_signature( - &DepositData { - pubkey: deposit.pubkey, - withdrawal_credentials: deposit.withdrawal_credentials, - amount: deposit.amount, - signature: deposit.signature, - }, - spec, - ) - .is_ok() - { - let mut validator = Validator { - pubkey: deposit.pubkey, - withdrawal_credentials: deposit.withdrawal_credentials, - activation_eligibility_epoch: spec.far_future_epoch, - activation_epoch: spec.far_future_epoch, - exit_epoch: spec.far_future_epoch, - withdrawable_epoch: spec.far_future_epoch, - effective_balance: 0, - slashed: false, - }; - let amount = deposit.amount; - let max_effective_balance = - validator.get_max_effective_balance(spec, state.fork_name_unchecked()); - validator.effective_balance = std::cmp::min( - amount.safe_sub(amount.safe_rem(spec.effective_balance_increment)?)?, - max_effective_balance, - ); - - state.validators_mut().push(validator)?; - state.balances_mut().push(amount)?; - // Altair or later initializations. - if let Ok(previous_epoch_participation) = state.previous_epoch_participation_mut() { - previous_epoch_participation.push(ParticipationFlags::default())?; - } - if let Ok(current_epoch_participation) = state.current_epoch_participation_mut() { - current_epoch_participation.push(ParticipationFlags::default())?; - } - if let Ok(inactivity_scores) = state.inactivity_scores_mut() { - inactivity_scores.push(0)?; - } + let deposit_data = DepositData { + pubkey: deposit.pubkey, + withdrawal_credentials: deposit.withdrawal_credentials, + amount: deposit.amount, + signature: deposit.signature, + }; + if is_valid_deposit_signature(&deposit_data, spec).is_ok() { + state.add_validator_to_registry( + deposit_data.pubkey, + deposit_data.withdrawal_credentials, + deposit_data.amount, + spec, + )?; } } } diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 99f5ea7756a..eddd80e2689 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -1550,17 +1550,19 @@ impl BeaconState { pub fn add_validator_to_registry( &mut self, - deposit_data: &DepositData, + pubkey: PublicKeyBytes, + withdrawal_credentials: Hash256, + amount: u64, spec: &ChainSpec, ) -> Result<(), Error> { - let fork = self.fork_name_unchecked(); - let amount = if fork.electra_enabled() { - 0 - } else { - deposit_data.amount - }; - self.validators_mut() - .push(Validator::from_deposit(deposit_data, amount, fork, spec))?; + let fork_name = self.fork_name_unchecked(); + self.validators_mut().push(Validator::from_deposit( + pubkey, + withdrawal_credentials, + amount, + fork_name, + spec, + ))?; self.balances_mut().push(amount)?; // Altair or later initializations. diff --git a/consensus/types/src/validator.rs b/consensus/types/src/validator.rs index f2a16bacd54..3717297abc0 100644 --- a/consensus/types/src/validator.rs +++ b/consensus/types/src/validator.rs @@ -1,6 +1,6 @@ use crate::{ - test_utils::TestRandom, Address, BeaconState, ChainSpec, Checkpoint, DepositData, Epoch, - EthSpec, FixedBytesExtended, ForkName, Hash256, PublicKeyBytes, + test_utils::TestRandom, Address, BeaconState, ChainSpec, Checkpoint, Epoch, EthSpec, + FixedBytesExtended, ForkName, Hash256, PublicKeyBytes, }; use serde::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; @@ -37,14 +37,15 @@ pub struct Validator { impl Validator { #[allow(clippy::arithmetic_side_effects)] pub fn from_deposit( - deposit_data: &DepositData, + pubkey: PublicKeyBytes, + withdrawal_credentials: Hash256, amount: u64, fork_name: ForkName, spec: &ChainSpec, ) -> Self { let mut validator = Validator { - pubkey: deposit_data.pubkey, - withdrawal_credentials: deposit_data.withdrawal_credentials, + pubkey, + withdrawal_credentials, activation_eligibility_epoch: spec.far_future_epoch, activation_epoch: spec.far_future_epoch, exit_epoch: spec.far_future_epoch, From 9f850748548612f004ff009dc3e8fce7d6829898 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 29 Oct 2024 12:40:13 -0700 Subject: [PATCH 10/27] Address some review comments --- .../process_operations.rs | 26 ++++++++++--------- .../state_processing/src/upgrade/electra.rs | 1 - 2 files changed, 14 insertions(+), 13 deletions(-) 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 a405156acbb..73c0fb8e42b 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -636,10 +636,10 @@ fn is_valid_switch_to_compounding_request( state: &BeaconState, consolidation_request: &ConsolidationRequest, spec: &ChainSpec, -) -> Result { +) -> bool { // Switch to compounding requires source and target be equal if consolidation_request.source_pubkey != consolidation_request.target_pubkey { - return Ok(false); + return false; } // Verify pubkey exists @@ -648,31 +648,34 @@ fn is_valid_switch_to_compounding_request( .get(&consolidation_request.source_pubkey) else { // source validator doesn't exist - return Ok(false); + return false; }; - let source_validator = state.get_validator(source_index)?; + let Ok(source_validator) = state.get_validator(source_index) else { + // Validator must exist in the state else it wouldn't exist in the pubkey cache either + return false; + }; // Verify the source withdrawal credentials if let Some(withdrawal_address) = source_validator.get_eth1_withdrawal_credential(spec) { if withdrawal_address != consolidation_request.source_address { - return Ok(false); + return false; } } else { - // Source doen't have execution withdrawal credentials - return Ok(false); + // Source doesn't have eth1 withdrawal credentials + return false; } // Verify the source is active let current_epoch = state.current_epoch(); if !source_validator.is_active_at(current_epoch) { - return Ok(false); + return false; } // Verify exits for source has not been initiated if source_validator.exit_epoch != spec.far_future_epoch { - return Ok(false); + return false; } - Ok(true) + true } pub fn process_consolidation_request( @@ -680,8 +683,7 @@ pub fn process_consolidation_request( consolidation_request: &ConsolidationRequest, spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { - dbg!("here"); - if is_valid_switch_to_compounding_request(state, consolidation_request, spec)? { + if is_valid_switch_to_compounding_request(state, consolidation_request, spec) { let Some(source_index) = state .pubkey_cache() .get(&consolidation_request.source_pubkey) diff --git a/consensus/state_processing/src/upgrade/electra.rs b/consensus/state_processing/src/upgrade/electra.rs index 7287bbff080..c69c8df5a13 100644 --- a/consensus/state_processing/src/upgrade/electra.rs +++ b/consensus/state_processing/src/upgrade/electra.rs @@ -73,7 +73,6 @@ pub fn upgrade_to_electra( validator.activation_eligibility_epoch = spec.far_future_epoch; let pubkey = validator.pubkey; let withdrawal_credentials = validator.withdrawal_credentials; - dbg!("is this getting hit"); post.pending_deposits_mut()? .push(PendingDeposit { From 707338d6ae09d7e413f2ef86413d06a307f67d2e Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 29 Oct 2024 12:59:01 -0700 Subject: [PATCH 11/27] Cleanup --- .../src/per_block_processing/process_operations.rs | 14 +++++++++++++- consensus/types/src/validator.rs | 11 ----------- 2 files changed, 13 insertions(+), 12 deletions(-) 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 73c0fb8e42b..42a79a6830a 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -656,7 +656,19 @@ fn is_valid_switch_to_compounding_request( return false; }; // Verify the source withdrawal credentials - if let Some(withdrawal_address) = source_validator.get_eth1_withdrawal_credential(spec) { + // Note: We need to specifically check for eth1 withdrawal credentials here + // If the validator is already compounding, the compounding request is not valid. + if let Some(withdrawal_address) = source_validator + .has_eth1_withdrawal_credential(spec) + .then(|| { + source_validator + .withdrawal_credentials + .as_slice() + .get(12..) + .map(Address::from_slice) + }) + .flatten() + { if withdrawal_address != consolidation_request.source_address { return false; } diff --git a/consensus/types/src/validator.rs b/consensus/types/src/validator.rs index 3717297abc0..159f2f48c79 100644 --- a/consensus/types/src/validator.rs +++ b/consensus/types/src/validator.rs @@ -181,17 +181,6 @@ impl Validator { .flatten() } - pub fn get_eth1_withdrawal_credential(&self, spec: &ChainSpec) -> Option
{ - self.has_eth1_withdrawal_credential(spec) - .then(|| { - self.withdrawal_credentials - .as_slice() - .get(12..) - .map(Address::from_slice) - }) - .flatten() - } - /// Changes withdrawal credentials to the provided eth1 execution address. /// /// WARNING: this function does NO VALIDATION - it just does it! From b2b728dc3d8ab7cf743e1a31d03228828a375f80 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 29 Oct 2024 13:41:14 -0700 Subject: [PATCH 12/27] Update initialize_beacon_state_from_eth1 --- consensus/state_processing/src/genesis.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/consensus/state_processing/src/genesis.rs b/consensus/state_processing/src/genesis.rs index 4450b999305..00f055e8c31 100644 --- a/consensus/state_processing/src/genesis.rs +++ b/consensus/state_processing/src/genesis.rs @@ -120,12 +120,22 @@ pub fn initialize_beacon_state_from_eth1( let post = upgrade_state_to_electra(&mut state, Epoch::new(0), Epoch::new(0), spec)?; state = post; - // TODO(electra): do we need to iterate over an empty pending_deposits list here and increase balance? - // in accordance with `initialize_beacon_state_from_eth1` function from the spec - // Remove intermediate Deneb fork from `state.fork`. state.fork_mut().previous_version = spec.electra_fork_version; + // iterate over an empty pending_deposits list here and increase balance + let pending_deposits = state.pending_deposits()?.clone(); + for pending_deposit in pending_deposits.into_iter() { + if let Some(validator_index) = state.pubkey_cache().get(&pending_deposit.pubkey) { + state + .balances_mut() + .get_mut(validator_index) + .ok_or(Error::UnknownValidator(validator_index))? + .safe_add(pending_deposit.amount)?; + } + } + *state.pending_deposits_mut()? = List::empty(); + // TODO(electra): think about this more and determine the best way to // do this. The spec tests will expect that the sync committees are // calculated using the electra value for MAX_EFFECTIVE_BALANCE when From 2aaf9b5dbe9a3a664e0f2c39a6023c11e8b46e38 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 6 Nov 2024 15:31:00 -0800 Subject: [PATCH 13/27] Fix rpc decoding for blobs by range/root --- .../lighthouse_network/src/rpc/codec.rs | 74 +++++++++++++++++-- 1 file changed, 66 insertions(+), 8 deletions(-) diff --git a/beacon_node/lighthouse_network/src/rpc/codec.rs b/beacon_node/lighthouse_network/src/rpc/codec.rs index 9bdecab70b1..5d86936d41d 100644 --- a/beacon_node/lighthouse_network/src/rpc/codec.rs +++ b/beacon_node/lighthouse_network/src/rpc/codec.rs @@ -682,10 +682,15 @@ fn handle_rpc_response( SignedBeaconBlock::Base(SignedBeaconBlockBase::from_ssz_bytes(decoded_buffer)?), )))), SupportedProtocol::BlobsByRangeV1 => match fork_name { - Some(ForkName::Deneb) => Ok(Some(RpcSuccessResponse::BlobsByRange(Arc::new( - BlobSidecar::from_ssz_bytes(decoded_buffer)?, - )))), - Some(_) => Err(RPCError::ErrorResponse( + Some(ForkName::Deneb) | Some(ForkName::Electra) => { + Ok(Some(RpcSuccessResponse::BlobsByRange(Arc::new( + BlobSidecar::from_ssz_bytes(decoded_buffer)?, + )))) + } + Some(ForkName::Base) + | Some(ForkName::Altair) + | Some(ForkName::Bellatrix) + | Some(ForkName::Capella) => Err(RPCError::ErrorResponse( RpcErrorResponse::InvalidRequest, "Invalid fork name for blobs by range".to_string(), )), @@ -698,10 +703,15 @@ fn handle_rpc_response( )), }, SupportedProtocol::BlobsByRootV1 => match fork_name { - Some(ForkName::Deneb) => Ok(Some(RpcSuccessResponse::BlobsByRoot(Arc::new( - BlobSidecar::from_ssz_bytes(decoded_buffer)?, - )))), - Some(_) => Err(RPCError::ErrorResponse( + Some(ForkName::Deneb) | Some(ForkName::Electra) => { + Ok(Some(RpcSuccessResponse::BlobsByRoot(Arc::new( + BlobSidecar::from_ssz_bytes(decoded_buffer)?, + )))) + } + Some(ForkName::Base) + | Some(ForkName::Altair) + | Some(ForkName::Bellatrix) + | Some(ForkName::Capella) => Err(RPCError::ErrorResponse( RpcErrorResponse::InvalidRequest, "Invalid fork name for blobs by root".to_string(), )), @@ -1376,6 +1386,16 @@ mod tests { Ok(Some(RpcSuccessResponse::BlobsByRange(empty_blob_sidecar()))), ); + assert_eq!( + encode_then_decode_response( + SupportedProtocol::BlobsByRangeV1, + RpcResponse::Success(RpcSuccessResponse::BlobsByRange(empty_blob_sidecar())), + ForkName::Electra, + &chain_spec + ), + Ok(Some(RpcSuccessResponse::BlobsByRange(empty_blob_sidecar()))), + ); + assert_eq!( encode_then_decode_response( SupportedProtocol::BlobsByRootV1, @@ -1386,6 +1406,16 @@ mod tests { Ok(Some(RpcSuccessResponse::BlobsByRoot(empty_blob_sidecar()))), ); + assert_eq!( + encode_then_decode_response( + SupportedProtocol::BlobsByRootV1, + RpcResponse::Success(RpcSuccessResponse::BlobsByRoot(empty_blob_sidecar())), + ForkName::Electra, + &chain_spec + ), + Ok(Some(RpcSuccessResponse::BlobsByRoot(empty_blob_sidecar()))), + ); + assert_eq!( encode_then_decode_response( SupportedProtocol::DataColumnsByRangeV1, @@ -1400,6 +1430,20 @@ mod tests { ))), ); + assert_eq!( + encode_then_decode_response( + SupportedProtocol::DataColumnsByRangeV1, + RpcResponse::Success(RpcSuccessResponse::DataColumnsByRange( + empty_data_column_sidecar() + )), + ForkName::Electra, + &chain_spec + ), + Ok(Some(RpcSuccessResponse::DataColumnsByRange( + empty_data_column_sidecar() + ))), + ); + assert_eq!( encode_then_decode_response( SupportedProtocol::DataColumnsByRootV1, @@ -1413,6 +1457,20 @@ mod tests { empty_data_column_sidecar() ))), ); + + assert_eq!( + encode_then_decode_response( + SupportedProtocol::DataColumnsByRootV1, + RpcResponse::Success(RpcSuccessResponse::DataColumnsByRoot( + empty_data_column_sidecar() + )), + ForkName::Electra, + &chain_spec + ), + Ok(Some(RpcSuccessResponse::DataColumnsByRoot( + empty_data_column_sidecar() + ))), + ); } // Test RPCResponse encoding/decoding for V1 messages From 27ce1a08d98e2b3ec1b49af03778d214d9854861 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 6 Nov 2024 19:11:07 -0800 Subject: [PATCH 14/27] Fix block hash computation --- beacon_node/execution_layer/src/block_hash.rs | 44 +++++++++++-------- .../src/engine_api/json_structures.rs | 23 ++-------- .../src/engine_api/new_payload_request.rs | 7 ++- consensus/types/src/execution_block_header.rs | 9 ++++ consensus/types/src/execution_requests.rs | 40 ++++++++++++++++- consensus/types/src/lib.rs | 2 +- 6 files changed, 83 insertions(+), 42 deletions(-) diff --git a/beacon_node/execution_layer/src/block_hash.rs b/beacon_node/execution_layer/src/block_hash.rs index cdc172cff47..d3a32c7929b 100644 --- a/beacon_node/execution_layer/src/block_hash.rs +++ b/beacon_node/execution_layer/src/block_hash.rs @@ -7,7 +7,7 @@ use keccak_hash::KECCAK_EMPTY_LIST_RLP; use triehash::ordered_trie_root; use types::{ EncodableExecutionBlockHeader, EthSpec, ExecutionBlockHash, ExecutionBlockHeader, - ExecutionPayloadRef, Hash256, + ExecutionPayloadRef, ExecutionRequests, Hash256, }; /// Calculate the block hash of an execution block. @@ -17,6 +17,7 @@ use types::{ pub fn calculate_execution_block_hash( payload: ExecutionPayloadRef, parent_beacon_block_root: Option, + execution_requests: Option<&ExecutionRequests>, ) -> (ExecutionBlockHash, Hash256) { // Calculate the transactions root. // We're currently using a deprecated Parity library for this. We should move to a @@ -38,6 +39,7 @@ pub fn calculate_execution_block_hash( let rlp_blob_gas_used = payload.blob_gas_used().ok(); let rlp_excess_blob_gas = payload.excess_blob_gas().ok(); + let requests_root = execution_requests.map(|requests| requests.requests_hash()); // Construct the block header. let exec_block_header = ExecutionBlockHeader::from_payload( @@ -48,6 +50,7 @@ pub fn calculate_execution_block_hash( rlp_blob_gas_used, rlp_excess_blob_gas, parent_beacon_block_root, + requests_root, ); // Hash the RLP encoding of the block header. @@ -118,6 +121,7 @@ mod test { blob_gas_used: None, excess_blob_gas: None, parent_beacon_block_root: None, + requests_root: None, }; let expected_rlp = "f90200a0e0a94a7a3c9617401586b1a27025d2d9671332d22d540e0af72b069170380f2aa01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d4934794ba5e000000000000000000000000000000000000a0ec3c94b18b8a1cff7d60f8d258ec723312932928626b4c9355eb4ab3568ec7f7a050f738580ed699f0469702c7ccc63ed2e51bc034be9479b7bff4e68dee84accfa029b0562f7140574dd0d50dee8a271b22e1a0a7b78fca58f7c60370d8317ba2a9b9010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000830200000188016345785d8a00008301553482079e42a0000000000000000000000000000000000000000000000000000000000000000088000000000000000082036b"; let expected_hash = @@ -149,6 +153,7 @@ mod test { blob_gas_used: None, excess_blob_gas: None, parent_beacon_block_root: None, + requests_root: None, }; let expected_rlp = "f901fda0927ca537f06c783a3a2635b8805eef1c8c2124f7444ad4a3389898dd832f2dbea01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d4934794ba5e000000000000000000000000000000000000a0e97859b065bd8dbbb4519c7cb935024de2484c2b7f881181b4360492f0b06b82a050f738580ed699f0469702c7ccc63ed2e51bc034be9479b7bff4e68dee84accfa029b0562f7140574dd0d50dee8a271b22e1a0a7b78fca58f7c60370d8317ba2a9b9010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000800188016345785d8a00008301553482079e42a0000000000000000000000000000000000000000000000000000000000002000088000000000000000082036b"; let expected_hash = @@ -181,6 +186,7 @@ mod test { blob_gas_used: None, excess_blob_gas: None, parent_beacon_block_root: None, + requests_root: None, }; let expected_hash = Hash256::from_str("6da69709cd5a34079b6604d29cd78fc01dacd7c6268980057ad92a2bede87351") @@ -211,6 +217,7 @@ mod test { blob_gas_used: Some(0x0u64), excess_blob_gas: Some(0x0u64), parent_beacon_block_root: Some(Hash256::from_str("f7d327d2c04e4f12e9cdd492e53d39a1d390f8b1571e3b2a22ac6e1e170e5b1a").unwrap()), + requests_root: None, }; let expected_hash = Hash256::from_str("a7448e600ead0a23d16f96aa46e8dea9eef8a7c5669a5f0a5ff32709afe9c408") @@ -221,29 +228,30 @@ mod test { #[test] fn test_rlp_encode_block_electra() { let header = ExecutionBlockHeader { - parent_hash: Hash256::from_str("172864416698b842f4c92f7b476be294b4ef720202779df194cd225f531053ab").unwrap(), + parent_hash: Hash256::from_str("a628f146df398a339768bd101f7dc41d828be79aca5dd02cc878a51bdbadd761").unwrap(), ommers_hash: Hash256::from_str("1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347").unwrap(), - beneficiary: Address::from_str("878705ba3f8bc32fcf7f4caa1a35e72af65cf766").unwrap(), - state_root: Hash256::from_str("c6457d0df85c84c62d1c68f68138b6e796e8a44fb44de221386fb2d5611c41e0").unwrap(), - transactions_root: Hash256::from_str("56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421").unwrap(), - receipts_root: Hash256::from_str("56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421").unwrap(), - logs_bloom:<[u8; 256]>::from_hex("00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000").unwrap().into(), + beneficiary: Address::from_str("f97e180c050e5ab072211ad2c213eb5aee4df134").unwrap(), + state_root: Hash256::from_str("fdff009f8280bd113ebb4df8ce4e2dcc9322d43184a0b506e70b7f4823ca1253").unwrap(), + transactions_root: Hash256::from_str("452806578b4fa881cafb019c47e767e37e2249accf859159f00cddefb2579bb5").unwrap(), + receipts_root: Hash256::from_str("72ceac0f16a32041c881b3220d39ca506a286bef163c01a4d0821cd4027d31c7").unwrap(), + logs_bloom:<[u8; 256]>::from_hex("10000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000400000000000000000000000020000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008000000000000000000000000").unwrap().into(), difficulty: Uint256::ZERO, - number: Uint256::from(97), - gas_limit: Uint256::from(27482534), - gas_used: Uint256::ZERO, - timestamp: 1692132829u64, - extra_data: hex::decode("d883010d00846765746888676f312e32302e37856c696e7578").unwrap(), - mix_hash: Hash256::from_str("0b493c22d2ad4ca76c77ae6ad916af429b42b1dc98fdcb8e5ddbd049bbc5d623").unwrap(), + number: Uint256::from(8230), + gas_limit: Uint256::from(30000000), + gas_used: Uint256::from(3716848), + timestamp: 1730921268, + extra_data: hex::decode("d883010e0c846765746888676f312e32332e32856c696e7578").unwrap(), + mix_hash: Hash256::from_str("e87ca9a45b2e61bbe9080d897db1d584b5d2367d22e898af901091883b7b96ec").unwrap(), nonce: Hash64::ZERO, - base_fee_per_gas: Uint256::from(2374u64), + base_fee_per_gas: Uint256::from(7u64), withdrawals_root: Some(Hash256::from_str("56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421").unwrap()), - blob_gas_used: Some(0x0u64), - excess_blob_gas: Some(0x0u64), - parent_beacon_block_root: Some(Hash256::from_str("f7d327d2c04e4f12e9cdd492e53d39a1d390f8b1571e3b2a22ac6e1e170e5b1a").unwrap()), + blob_gas_used: Some(786432), + excess_blob_gas: Some(44695552), + parent_beacon_block_root: Some(Hash256::from_str("f3a888fee010ebb1ae083547004e96c254b240437823326fdff8354b1fc25629").unwrap()), + requests_root: Some(Hash256::from_str("9440d3365f07573919e1e9ac5178c20ec6fe267357ee4baf8b6409901f331b62").unwrap()), }; let expected_hash = - Hash256::from_str("a7448e600ead0a23d16f96aa46e8dea9eef8a7c5669a5f0a5ff32709afe9c408") + Hash256::from_str("61e67afc96bf21be6aab52c1ace1db48de7b83f03119b0644deb4b69e87e09e1") .unwrap(); test_rlp_encoding(&header, None, expected_hash); } diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index 1431a81acc5..4a813b24bb4 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -6,7 +6,9 @@ use strum::EnumString; use superstruct::superstruct; use types::beacon_block_body::KzgCommitments; use types::blob_sidecar::BlobsList; -use types::execution_requests::{ConsolidationRequests, DepositRequests, WithdrawalRequests}; +use types::execution_requests::{ + ConsolidationRequests, DepositRequests, RequestPrefix, WithdrawalRequests, +}; use types::{FixedVector, Unsigned}; #[derive(Debug, PartialEq, Serialize, Deserialize)] @@ -339,25 +341,6 @@ impl From> for ExecutionPayload { } } -/// This is used to index into the `execution_requests` array. -#[derive(Debug, Copy, Clone)] -enum RequestPrefix { - Deposit, - Withdrawal, - Consolidation, -} - -impl RequestPrefix { - pub fn from_prefix(prefix: u8) -> Option { - match prefix { - 0 => Some(Self::Deposit), - 1 => Some(Self::Withdrawal), - 2 => Some(Self::Consolidation), - _ => None, - } - } -} - /// Format of `ExecutionRequests` received over the engine api. /// /// Array of ssz-encoded requests list encoded as hex bytes. diff --git a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs index 6f2261a5c73..06d72fe6163 100644 --- a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs +++ b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs @@ -121,8 +121,11 @@ impl<'block, E: EthSpec> NewPayloadRequest<'block, E> { let _timer = metrics::start_timer(&metrics::EXECUTION_LAYER_VERIFY_BLOCK_HASH); - let (header_hash, rlp_transactions_root) = - calculate_execution_block_hash(payload, parent_beacon_block_root); + let (header_hash, rlp_transactions_root) = calculate_execution_block_hash( + payload, + parent_beacon_block_root, + self.execution_requests().ok().cloned(), + ); if header_hash != self.block_hash() { return Err(Error::BlockHashMismatch { diff --git a/consensus/types/src/execution_block_header.rs b/consensus/types/src/execution_block_header.rs index 694162d6ffd..60f2960afbe 100644 --- a/consensus/types/src/execution_block_header.rs +++ b/consensus/types/src/execution_block_header.rs @@ -52,9 +52,11 @@ pub struct ExecutionBlockHeader { pub blob_gas_used: Option, pub excess_blob_gas: Option, pub parent_beacon_block_root: Option, + pub requests_root: Option, } impl ExecutionBlockHeader { + #[allow(clippy::too_many_arguments)] pub fn from_payload( payload: ExecutionPayloadRef, rlp_empty_list_root: Hash256, @@ -63,6 +65,7 @@ impl ExecutionBlockHeader { rlp_blob_gas_used: Option, rlp_excess_blob_gas: Option, rlp_parent_beacon_block_root: Option, + rlp_requests_root: Option, ) -> Self { // Most of these field mappings are defined in EIP-3675 except for `mixHash`, which is // defined in EIP-4399. @@ -87,6 +90,7 @@ impl ExecutionBlockHeader { blob_gas_used: rlp_blob_gas_used, excess_blob_gas: rlp_excess_blob_gas, parent_beacon_block_root: rlp_parent_beacon_block_root, + requests_root: rlp_requests_root, } } } @@ -114,6 +118,7 @@ pub struct EncodableExecutionBlockHeader<'a> { pub blob_gas_used: Option, pub excess_blob_gas: Option, pub parent_beacon_block_root: Option<&'a [u8]>, + pub requests_root: Option<&'a [u8]>, } impl<'a> From<&'a ExecutionBlockHeader> for EncodableExecutionBlockHeader<'a> { @@ -139,6 +144,7 @@ impl<'a> From<&'a ExecutionBlockHeader> for EncodableExecutionBlockHeader<'a> { blob_gas_used: header.blob_gas_used, excess_blob_gas: header.excess_blob_gas, parent_beacon_block_root: None, + requests_root: None, }; if let Some(withdrawals_root) = &header.withdrawals_root { encodable.withdrawals_root = Some(withdrawals_root.as_slice()); @@ -146,6 +152,9 @@ impl<'a> From<&'a ExecutionBlockHeader> for EncodableExecutionBlockHeader<'a> { if let Some(parent_beacon_block_root) = &header.parent_beacon_block_root { encodable.parent_beacon_block_root = Some(parent_beacon_block_root.as_slice()) } + if let Some(requests_root) = &header.requests_root { + encodable.requests_root = Some(requests_root.as_slice()) + } encodable } } diff --git a/consensus/types/src/execution_requests.rs b/consensus/types/src/execution_requests.rs index 778260dd841..96a39054207 100644 --- a/consensus/types/src/execution_requests.rs +++ b/consensus/types/src/execution_requests.rs @@ -1,7 +1,8 @@ use crate::test_utils::TestRandom; -use crate::{ConsolidationRequest, DepositRequest, EthSpec, WithdrawalRequest}; +use crate::{ConsolidationRequest, DepositRequest, EthSpec, Hash256, WithdrawalRequest}; use alloy_primitives::Bytes; use derivative::Derivative; +use ethereum_hashing::{DynamicContext, Sha256Context}; use serde::{Deserialize, Serialize}; use ssz::Encode; use ssz_derive::{Decode, Encode}; @@ -47,6 +48,43 @@ impl ExecutionRequests { let consolidation_bytes = Bytes::from(self.consolidations.as_ssz_bytes()); vec![deposit_bytes, withdrawal_bytes, consolidation_bytes] } + + /// Generate the execution layer `requests_hash` based on EIP-7685. + /// + /// `sha256(sha256(requests_0) ++ sha256(requests_1) ++ ...)` + pub fn requests_hash(&self) -> Hash256 { + let mut hasher = DynamicContext::new(); + + for (i, request) in self.get_execution_requests_list().iter().enumerate() { + let mut request_hasher = DynamicContext::new(); + request_hasher.update(&[i as u8]); + request_hasher.update(request); + let request_hash = request_hasher.finalize(); + + hasher.update(&request_hash); + } + + hasher.finalize().into() + } +} + +/// This is used to index into the `execution_requests` array. +#[derive(Debug, Copy, Clone)] +pub enum RequestPrefix { + Deposit, + Withdrawal, + Consolidation, +} + +impl RequestPrefix { + pub fn from_prefix(prefix: u8) -> Option { + match prefix { + 0 => Some(Self::Deposit), + 1 => Some(Self::Withdrawal), + 2 => Some(Self::Consolidation), + _ => None, + } + } } #[cfg(test)] diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 6eaa5594049..dd304c6296c 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -170,7 +170,7 @@ pub use crate::execution_payload_header::{ ExecutionPayloadHeaderDeneb, ExecutionPayloadHeaderElectra, ExecutionPayloadHeaderRef, ExecutionPayloadHeaderRefMut, }; -pub use crate::execution_requests::ExecutionRequests; +pub use crate::execution_requests::{ExecutionRequests, RequestPrefix}; pub use crate::fork::Fork; pub use crate::fork_context::ForkContext; pub use crate::fork_data::ForkData; From da953b8155704c042a3eb020df4c697ede7d217b Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 7 Nov 2024 21:11:18 -0800 Subject: [PATCH 15/27] Fix process_deposits bug --- .../process_operations.rs | 2 +- consensus/types/src/beacon_state.rs | 13 ------- consensus/types/src/beacon_state/tests.rs | 37 ------------------- 3 files changed, 1 insertion(+), 51 deletions(-) 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 42a79a6830a..81b5bac5e68 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -377,7 +377,7 @@ pub fn process_deposits( if state.eth1_deposit_index() < eth1_deposit_index_limit { let expected_deposit_len = std::cmp::min( E::MaxDeposits::to_u64(), - state.get_outstanding_deposit_len()?, + eth1_deposit_index_limit.safe_sub(state.eth1_deposit_index())?, ); block_verify!( deposits.len() as u64 == expected_deposit_len, diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index eddd80e2689..9ea0ed94c58 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -1783,19 +1783,6 @@ impl BeaconState { } } - /// Get the number of outstanding deposits. - /// - /// Returns `Err` if the state is invalid. - pub fn get_outstanding_deposit_len(&self) -> Result { - self.eth1_data() - .deposit_count - .checked_sub(self.eth1_deposit_index()) - .ok_or(Error::InvalidDepositState { - deposit_count: self.eth1_data().deposit_count, - deposit_index: self.eth1_deposit_index(), - }) - } - /// Build all caches (except the tree hash cache), if they need to be built. pub fn build_caches(&mut self, spec: &ChainSpec) -> Result<(), Error> { self.build_all_committee_caches(spec)?; diff --git a/consensus/types/src/beacon_state/tests.rs b/consensus/types/src/beacon_state/tests.rs index 3ad3ccf5617..bfa7bb86d24 100644 --- a/consensus/types/src/beacon_state/tests.rs +++ b/consensus/types/src/beacon_state/tests.rs @@ -307,43 +307,6 @@ mod committees { } } -mod get_outstanding_deposit_len { - use super::*; - - async fn state() -> BeaconState { - get_harness(16, Slot::new(0)) - .await - .chain - .head_beacon_state_cloned() - } - - #[tokio::test] - async fn returns_ok() { - let mut state = state().await; - assert_eq!(state.get_outstanding_deposit_len(), Ok(0)); - - state.eth1_data_mut().deposit_count = 17; - *state.eth1_deposit_index_mut() = 16; - assert_eq!(state.get_outstanding_deposit_len(), Ok(1)); - } - - #[tokio::test] - async fn returns_err_if_the_state_is_invalid() { - let mut state = state().await; - // The state is invalid, deposit count is lower than deposit index. - state.eth1_data_mut().deposit_count = 16; - *state.eth1_deposit_index_mut() = 17; - - assert_eq!( - state.get_outstanding_deposit_len(), - Err(BeaconStateError::InvalidDepositState { - deposit_count: 16, - deposit_index: 17, - }) - ); - } -} - #[test] fn decode_base_and_altair() { type E = MainnetEthSpec; From def2498bc4114b4bd7933cf3bb9a45a555d24ede Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 25 Nov 2024 20:00:28 -0800 Subject: [PATCH 16/27] Fix topup deposit processing bug --- .../src/per_epoch_processing/single_pass.rs | 63 +++++++++++++++++-- 1 file changed, 58 insertions(+), 5 deletions(-) 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 e79a6fb6fba..bb33284d17a 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -360,7 +360,10 @@ pub fn process_epoch_single_pass( *state.pending_deposits_mut()? = new_balance_deposits; *state.deposit_balance_to_consume_mut()? = ctxt.deposit_balance_to_consume; - // Apply the new deposits to the state + // `new_validator_deposits` may contain multiple deposits with the same pubkey where + // the first deposit creates the new validator and the others are topups. + // Each item in the vec is a (pubkey, validator_index) + let mut added_validators = Vec::new(); for deposit in ctxt.new_validator_deposits.into_iter() { let deposit_data = DepositData { pubkey: deposit.pubkey, @@ -369,10 +372,60 @@ pub fn process_epoch_single_pass( signature: deposit.signature, }; if is_valid_deposit_signature(&deposit_data, spec).is_ok() { - state.add_validator_to_registry( - deposit_data.pubkey, - deposit_data.withdrawal_credentials, - deposit_data.amount, + // If a validator with the same pubkey has already been added, do not add a new + // validator, just update the balance. + // Note: updating the `effective_balance` for the new validator is done after + // adding them to the state. + if let Some((_, validator_index)) = added_validators + .iter() + .find(|(pubkey, _)| *pubkey == deposit_data.pubkey) + { + let balance = state.get_balance_mut(*validator_index)?; + balance.safe_add_assign(deposit_data.amount)?; + } else { + // Apply the new deposit to the state + state.add_validator_to_registry( + deposit_data.pubkey, + deposit_data.withdrawal_credentials, + deposit_data.amount, + spec, + )?; + added_validators + .push((deposit_data.pubkey, state.validators().len().safe_sub(1)?)); + } + } + } + if conf.effective_balance_updates { + // Re-process effective balance updates for validators affected by top-up of new validators. + let ( + validators, + balances, + _, + current_epoch_participation, + _, + progressive_balances, + _, + _, + ) = state.mutable_validator_fields()?; + for (_, validator_index) in added_validators.iter() { + let balance = *balances + .get(*validator_index) + .ok_or(BeaconStateError::UnknownValidator(*validator_index))?; + let mut validator = validators + .get_cow(*validator_index) + .ok_or(BeaconStateError::UnknownValidator(*validator_index))?; + let validator_current_epoch_participation = *current_epoch_participation + .get(*validator_index) + .ok_or(BeaconStateError::UnknownValidator(*validator_index))?; + process_single_effective_balance_update( + *validator_index, + balance, + &mut validator, + validator_current_epoch_participation, + &mut next_epoch_cache, + progressive_balances, + effective_balances_ctxt, + state_ctxt, spec, )?; } From c5c9aca6db201c09c995756a11e9cb6a03b2ea99 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Tue, 3 Dec 2024 17:58:51 -0700 Subject: [PATCH 17/27] Update builder api for electra --- beacon_node/execution_layer/src/lib.rs | 3 +-- .../src/test_utils/mock_builder.rs | 24 ++++++++++--------- .../execution_layer/src/test_utils/mod.rs | 2 +- consensus/types/src/builder_bid.rs | 5 +++- 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 08a00d7bf8d..2963b3f9a7f 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -121,8 +121,7 @@ impl TryFrom> for ProvenancedPayload( let mut message = match payload_response_type { crate::GetPayloadResponseType::Full(payload_response) => { #[allow(clippy::type_complexity)] - let (payload, _block_value, maybe_blobs_bundle, _maybe_requests): ( + let (payload, value, maybe_blobs_bundle, maybe_requests): ( ExecutionPayload, Uint256, Option>, @@ -559,8 +559,9 @@ pub fn serve( blob_kzg_commitments: maybe_blobs_bundle .map(|b| b.commitments) .unwrap_or_default(), - value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), + value, pubkey: builder.builder_sk.public_key().compress(), + execution_requests: maybe_requests.unwrap_or_default(), }), ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { header: payload @@ -570,7 +571,7 @@ pub fn serve( blob_kzg_commitments: maybe_blobs_bundle .map(|b| b.commitments) .unwrap_or_default(), - value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), + value, pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { @@ -578,7 +579,7 @@ pub fn serve( .as_capella() .map_err(|_| reject("incorrect payload variant"))? .into(), - value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), + value, pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { @@ -586,7 +587,7 @@ pub fn serve( .as_bellatrix() .map_err(|_| reject("incorrect payload variant"))? .into(), - value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), + value, pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Base | ForkName::Altair => { @@ -596,7 +597,7 @@ pub fn serve( } crate::GetPayloadResponseType::Blinded(payload_response) => { #[allow(clippy::type_complexity)] - let (payload, _block_value, maybe_blobs_bundle, _maybe_requests): ( + let (payload, value, maybe_blobs_bundle, maybe_requests): ( ExecutionPayload, Uint256, Option>, @@ -611,8 +612,9 @@ pub fn serve( blob_kzg_commitments: maybe_blobs_bundle .map(|b| b.commitments) .unwrap_or_default(), - value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), + value, pubkey: builder.builder_sk.public_key().compress(), + execution_requests: maybe_requests.unwrap_or_default(), }), ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { header: payload @@ -622,7 +624,7 @@ pub fn serve( blob_kzg_commitments: maybe_blobs_bundle .map(|b| b.commitments) .unwrap_or_default(), - value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), + value, pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { @@ -630,7 +632,7 @@ pub fn serve( .as_capella() .map_err(|_| reject("incorrect payload variant"))? .into(), - value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), + value, pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { @@ -638,7 +640,7 @@ pub fn serve( .as_bellatrix() .map_err(|_| reject("incorrect payload variant"))? .into(), - value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), + value, pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Base | ForkName::Altair => { diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index 1e71fde2551..f19f84891e1 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -30,7 +30,7 @@ pub use execution_block_generator::{ static_valid_tx, Block, ExecutionBlockGenerator, }; pub use hook::Hook; -pub use mock_builder::{MockBuilder, Operation}; +pub use mock_builder::{MockBuilder, Operation, serve as serve_mock_builder}; pub use mock_execution_layer::MockExecutionLayer; pub const DEFAULT_TERMINAL_DIFFICULTY: u64 = 6400; diff --git a/consensus/types/src/builder_bid.rs b/consensus/types/src/builder_bid.rs index 9885f78474f..5d405d25401 100644 --- a/consensus/types/src/builder_bid.rs +++ b/consensus/types/src/builder_bid.rs @@ -2,7 +2,8 @@ use crate::beacon_block_body::KzgCommitments; use crate::{ ChainSpec, EthSpec, ExecutionPayloadHeaderBellatrix, ExecutionPayloadHeaderCapella, ExecutionPayloadHeaderDeneb, ExecutionPayloadHeaderElectra, ExecutionPayloadHeaderRef, - ExecutionPayloadHeaderRefMut, ForkName, ForkVersionDeserialize, SignedRoot, Uint256, + ExecutionPayloadHeaderRefMut, ExecutionRequests, ForkName, ForkVersionDeserialize, SignedRoot, + Uint256, }; use bls::PublicKeyBytes; use bls::Signature; @@ -33,6 +34,8 @@ pub struct BuilderBid { pub header: ExecutionPayloadHeaderElectra, #[superstruct(only(Deneb, Electra))] pub blob_kzg_commitments: KzgCommitments, + #[superstruct(only(Electra))] + pub execution_requests: ExecutionRequests, #[serde(with = "serde_utils::quoted_u256")] pub value: Uint256, pub pubkey: PublicKeyBytes, From 6d10456912b3c39b8a8c9089db76e8ead20608a0 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 4 Dec 2024 17:28:00 -0800 Subject: [PATCH 18/27] Refactor mock builder to separate functionality --- .../src/test_utils/mock_builder.rs | 640 ++++++++++-------- .../execution_layer/src/test_utils/mod.rs | 2 +- 2 files changed, 342 insertions(+), 300 deletions(-) diff --git a/beacon_node/execution_layer/src/test_utils/mock_builder.rs b/beacon_node/execution_layer/src/test_utils/mock_builder.rs index 88ca399e17f..d2adbc30783 100644 --- a/beacon_node/execution_layer/src/test_utils/mock_builder.rs +++ b/beacon_node/execution_layer/src/test_utils/mock_builder.rs @@ -287,8 +287,339 @@ impl MockBuilder { op.apply(bid); } } + + /// Return the public key of the builder + pub fn public_key(&self) -> PublicKeyBytes { + self.builder_sk.public_key().compress() + } + + pub async fn register_validators( + &self, + registrations: Vec, + ) -> Result<(), String> { + for registration in registrations { + if !registration.verify_signature(&self.spec) { + return Err("invalid signature".to_string()); + } + self.val_registration_cache + .write() + .insert(registration.message.pubkey, registration); + } + Ok(()) + } + + pub async fn submit_blinded_block( + &self, + block: SignedBlindedBeaconBlock, + ) -> Result, String> { + let root = match &block { + SignedBlindedBeaconBlock::Base(_) | types::SignedBeaconBlock::Altair(_) => { + return Err("invalid fork".to_string()); + } + SignedBlindedBeaconBlock::Bellatrix(block) => { + block.message.body.execution_payload.tree_hash_root() + } + SignedBlindedBeaconBlock::Capella(block) => { + block.message.body.execution_payload.tree_hash_root() + } + SignedBlindedBeaconBlock::Deneb(block) => { + block.message.body.execution_payload.tree_hash_root() + } + SignedBlindedBeaconBlock::Electra(block) => { + block.message.body.execution_payload.tree_hash_root() + } + }; + let payload = self + .el + .get_payload_by_root(&root) + .ok_or_else(|| "missing payload for tx root".to_string())?; + let (payload, _) = payload.deconstruct(); + // TODO(pawan): the reconstruction in the beacon client should propagate the + // blobs as well. If that doesn't happen then its probably a bug here + self.beacon_client + .post_beacon_blinded_blocks(&block) + .await + .map_err(|e| format!("Failed to post blinded block {:?}", e))?; + Ok(payload) + } + + pub async fn get_header( + &self, + slot: Slot, + parent_hash: ExecutionBlockHash, + pubkey: PublicKeyBytes, + ) -> Result, String> { + let fork = self.fork_name_at_slot(slot); + let signed_cached_data = self + .val_registration_cache + .read() + .get(&pubkey) + .ok_or_else(|| "missing registration".to_string())? + .clone(); + let cached_data = signed_cached_data.message; + + let head = self + .beacon_client + .get_beacon_blocks::(BlockId::Head) + .await + .map_err(|_| "couldn't get head".to_string())? + .ok_or_else(|| "missing head block".to_string())?; + + let block = head.data.message(); + let head_block_root = block.tree_hash_root(); + let head_execution_hash = block + .body() + .execution_payload() + .map_err(|_| "pre-merge block".to_string())? + .block_hash(); + if head_execution_hash != parent_hash { + return Err("head mismatch".to_string()); + } + + let finalized_execution_hash = self + .beacon_client + .get_beacon_blocks::(BlockId::Finalized) + .await + .map_err(|_| "couldn't get finalized block".to_string())? + .ok_or_else(|| "missing finalized block".to_string())? + .data + .message() + .body() + .execution_payload() + .map_err(|_| "pre-merge block".to_string())? + .block_hash(); + + let justified_execution_hash = self + .beacon_client + .get_beacon_blocks::(BlockId::Justified) + .await + .map_err(|_| "couldn't get justified block".to_string())? + .ok_or_else(|| "missing justified block".to_string())? + .data + .message() + .body() + .execution_payload() + .map_err(|_| "pre-merge block".to_string())? + .block_hash(); + + let val_index = self + .beacon_client + .get_beacon_states_validator_id(StateId::Head, &ValidatorId::PublicKey(pubkey)) + .await + .map_err(|_| "couldn't get validator".to_string())? + .ok_or_else(|| "missing validator".to_string())? + .data + .index; + let fee_recipient = cached_data.fee_recipient; + let slots_since_genesis = slot.as_u64() - self.spec.genesis_slot.as_u64(); + + // TODO(pawan): cache this call + let genesis_data = self + .beacon_client + .get_beacon_genesis() + .await + .map_err(|_| "couldn't get beacon genesis".to_string())? + .data; + let genesis_time = genesis_data.genesis_time; + let timestamp = (slots_since_genesis * self.spec.seconds_per_slot) + genesis_time; + + let head_state: BeaconState = self + .beacon_client + .get_debug_beacon_states(StateId::Head) + .await + .map_err(|_| "couldn't get state".to_string())? + .ok_or_else(|| "missing state".to_string())? + .data; + let prev_randao = head_state + .get_randao_mix(head_state.current_epoch()) + .map_err(|_| "couldn't get prev randao".to_string())?; + + let expected_withdrawals = if fork.capella_enabled() { + Some( + self.beacon_client + .get_expected_withdrawals(&StateId::Head) + .await + .unwrap() + .data, + ) + } else { + None + }; + + let payload_attributes = match fork { + // the withdrawals root is filled in by operations, but we supply the valid withdrawals + // first to avoid polluting the execution block generator with invalid payload attributes + // NOTE: this was part of an effort to add payload attribute uniqueness checks, + // which was abandoned because it broke too many tests in subtle ways. + ForkName::Bellatrix | ForkName::Capella => PayloadAttributes::new( + timestamp, + *prev_randao, + fee_recipient, + expected_withdrawals, + None, + ), + ForkName::Deneb | ForkName::Electra => PayloadAttributes::new( + timestamp, + *prev_randao, + fee_recipient, + expected_withdrawals, + Some(head_block_root), + ), + ForkName::Base | ForkName::Altair => { + return Err("invalid fork".to_string()); + } + }; + + self.el + .insert_proposer(slot, head_block_root, val_index, payload_attributes.clone()) + .await; + + let forkchoice_update_params = ForkchoiceUpdateParameters { + head_root: Hash256::zero(), + head_hash: None, + justified_hash: Some(justified_execution_hash), + finalized_hash: Some(finalized_execution_hash), + }; + + let payload_response_type = self + .el + .get_full_payload_caching( + head_execution_hash, + &payload_attributes, + forkchoice_update_params, + fork, + ) + .await + .map_err(|_| "couldn't get payload".to_string())?; + + let mut message = match payload_response_type { + crate::GetPayloadResponseType::Full(payload_response) => { + #[allow(clippy::type_complexity)] + let (payload, value, maybe_blobs_bundle, maybe_requests): ( + ExecutionPayload, + Uint256, + Option>, + Option>, + ) = payload_response.into(); + + match fork { + ForkName::Electra => BuilderBid::Electra(BuilderBidElectra { + header: payload + .as_electra() + .map_err(|_| "incorrect payload variant".to_string())? + .into(), + blob_kzg_commitments: maybe_blobs_bundle + .map(|b| b.commitments) + .unwrap_or_default(), + value, + pubkey: self.builder_sk.public_key().compress(), + execution_requests: maybe_requests.unwrap_or_default(), + }), + ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { + header: payload + .as_deneb() + .map_err(|_| "incorrect payload variant".to_string())? + .into(), + blob_kzg_commitments: maybe_blobs_bundle + .map(|b| b.commitments) + .unwrap_or_default(), + value, + pubkey: self.builder_sk.public_key().compress(), + }), + ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { + header: payload + .as_capella() + .map_err(|_| "incorrect payload variant".to_string())? + .into(), + value, + pubkey: self.builder_sk.public_key().compress(), + }), + ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { + header: payload + .as_bellatrix() + .map_err(|_| "incorrect payload variant".to_string())? + .into(), + value, + pubkey: self.builder_sk.public_key().compress(), + }), + ForkName::Base | ForkName::Altair => return Err("invalid fork".to_string()), + } + } + crate::GetPayloadResponseType::Blinded(payload_response) => { + #[allow(clippy::type_complexity)] + let (payload, value, maybe_blobs_bundle, maybe_requests): ( + ExecutionPayload, + Uint256, + Option>, + Option>, + ) = payload_response.into(); + match fork { + ForkName::Electra => BuilderBid::Electra(BuilderBidElectra { + header: payload + .as_electra() + .map_err(|_| "incorrect payload variant".to_string())? + .into(), + blob_kzg_commitments: maybe_blobs_bundle + .map(|b| b.commitments) + .unwrap_or_default(), + value, + pubkey: self.builder_sk.public_key().compress(), + execution_requests: maybe_requests.unwrap_or_default(), + }), + ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { + header: payload + .as_deneb() + .map_err(|_| "incorrect payload variant".to_string())? + .into(), + blob_kzg_commitments: maybe_blobs_bundle + .map(|b| b.commitments) + .unwrap_or_default(), + value, + pubkey: self.builder_sk.public_key().compress(), + }), + ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { + header: payload + .as_capella() + .map_err(|_| "incorrect payload variant".to_string())? + .into(), + value, + pubkey: self.builder_sk.public_key().compress(), + }), + ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { + header: payload + .as_bellatrix() + .map_err(|_| "incorrect payload variant".to_string())? + .into(), + value, + pubkey: self.builder_sk.public_key().compress(), + }), + ForkName::Base | ForkName::Altair => return Err("invalid fork".to_string()), + } + } + }; + + message.set_gas_limit(cached_data.gas_limit); + + self.apply_operations(&mut message); + + let mut signature = message.sign_builder_message(&self.builder_sk, &self.spec); + + if *self.invalidate_signatures.read() { + signature = Signature::empty(); + }; + let signed_bid = SignedBuilderBid { message, signature }; + Ok(signed_bid) + } + + fn fork_name_at_slot(&self, slot: Slot) -> ForkName { + self.spec.fork_name_at_slot::(slot) + } } +/// Serve the builder api using warp. Uses the functions defined in `MockBuilder` to serve +/// the requests. +/// +/// We should eventually move this to axum when we move everything else. pub fn serve( listen_addr: Ipv4Addr, listen_port: u16, @@ -308,18 +639,10 @@ pub fn serve( .and(ctx_filter.clone()) .and_then( |registrations: Vec, builder: MockBuilder| async move { - for registration in registrations { - if !registration.verify_signature(&builder.spec) { - return Err(reject("invalid signature")); - } - builder - .val_registration_cache - .write() - .insert(registration.message.pubkey, registration); - } - Ok(warp::reply()) + builder.register_validators(registrations).await.map_err(|e|warp::reject::custom(Custom(e)))?; + Ok::<_, Rejection>(warp::reply()) }, - ); + ).boxed(); let blinded_block = prefix @@ -332,27 +655,10 @@ pub fn serve( |block: SignedBlindedBeaconBlock, fork_name: ForkName, builder: MockBuilder| async move { - let root = match block { - SignedBlindedBeaconBlock::Base(_) | types::SignedBeaconBlock::Altair(_) => { - return Err(reject("invalid fork")); - } - SignedBlindedBeaconBlock::Bellatrix(block) => { - block.message.body.execution_payload.tree_hash_root() - } - SignedBlindedBeaconBlock::Capella(block) => { - block.message.body.execution_payload.tree_hash_root() - } - SignedBlindedBeaconBlock::Deneb(block) => { - block.message.body.execution_payload.tree_hash_root() - } - SignedBlindedBeaconBlock::Electra(block) => { - block.message.body.execution_payload.tree_hash_root() - } - }; let payload = builder - .el - .get_payload_by_root(&root) - .ok_or_else(|| reject("missing payload for tx root"))?; + .submit_blinded_block(block) + .await + .map_err(|e| warp::reject::custom(Custom(e)))?; let resp: ForkVersionedResponse<_> = ForkVersionedResponse { version: Some(fork_name), metadata: Default::default(), @@ -395,276 +701,12 @@ pub fn serve( parent_hash: ExecutionBlockHash, pubkey: PublicKeyBytes, builder: MockBuilder| async move { - let fork = builder.spec.fork_name_at_slot::(slot); - let signed_cached_data = builder - .val_registration_cache - .read() - .get(&pubkey) - .ok_or_else(|| reject("missing registration"))? - .clone(); - let cached_data = signed_cached_data.message; - - let head = builder - .beacon_client - .get_beacon_blocks::(BlockId::Head) - .await - .map_err(|_| reject("couldn't get head"))? - .ok_or_else(|| reject("missing head block"))?; - - let block = head.data.message(); - let head_block_root = block.tree_hash_root(); - let head_execution_hash = block - .body() - .execution_payload() - .map_err(|_| reject("pre-merge block"))? - .block_hash(); - if head_execution_hash != parent_hash { - return Err(reject("head mismatch")); - } - - let finalized_execution_hash = builder - .beacon_client - .get_beacon_blocks::(BlockId::Finalized) - .await - .map_err(|_| reject("couldn't get finalized block"))? - .ok_or_else(|| reject("missing finalized block"))? - .data - .message() - .body() - .execution_payload() - .map_err(|_| reject("pre-merge block"))? - .block_hash(); - - let justified_execution_hash = builder - .beacon_client - .get_beacon_blocks::(BlockId::Justified) - .await - .map_err(|_| reject("couldn't get justified block"))? - .ok_or_else(|| reject("missing justified block"))? - .data - .message() - .body() - .execution_payload() - .map_err(|_| reject("pre-merge block"))? - .block_hash(); - - let val_index = builder - .beacon_client - .get_beacon_states_validator_id(StateId::Head, &ValidatorId::PublicKey(pubkey)) - .await - .map_err(|_| reject("couldn't get validator"))? - .ok_or_else(|| reject("missing validator"))? - .data - .index; - let fee_recipient = cached_data.fee_recipient; - let slots_since_genesis = slot.as_u64() - builder.spec.genesis_slot.as_u64(); - - let genesis_data = builder - .beacon_client - .get_beacon_genesis() - .await - .map_err(|_| reject("couldn't get beacon genesis"))? - .data; - let genesis_time = genesis_data.genesis_time; - let timestamp = - (slots_since_genesis * builder.spec.seconds_per_slot) + genesis_time; - - let head_state: BeaconState = builder - .beacon_client - .get_debug_beacon_states(StateId::Head) + let fork_name = builder.fork_name_at_slot(slot); + let signed_bid = builder + .get_header(slot, parent_hash, pubkey) .await - .map_err(|_| reject("couldn't get state"))? - .ok_or_else(|| reject("missing state"))? - .data; - let prev_randao = head_state - .get_randao_mix(head_state.current_epoch()) - .map_err(|_| reject("couldn't get prev randao"))?; - - let expected_withdrawals = if fork.capella_enabled() { - Some( - builder - .beacon_client - .get_expected_withdrawals(&StateId::Head) - .await - .unwrap() - .data, - ) - } else { - None - }; - - let payload_attributes = match fork { - // the withdrawals root is filled in by operations, but we supply the valid withdrawals - // first to avoid polluting the execution block generator with invalid payload attributes - // NOTE: this was part of an effort to add payload attribute uniqueness checks, - // which was abandoned because it broke too many tests in subtle ways. - ForkName::Bellatrix | ForkName::Capella => PayloadAttributes::new( - timestamp, - *prev_randao, - fee_recipient, - expected_withdrawals, - None, - ), - ForkName::Deneb | ForkName::Electra => PayloadAttributes::new( - timestamp, - *prev_randao, - fee_recipient, - expected_withdrawals, - Some(head_block_root), - ), - ForkName::Base | ForkName::Altair => { - return Err(reject("invalid fork")); - } - }; - - builder - .el - .insert_proposer(slot, head_block_root, val_index, payload_attributes.clone()) - .await; - - let forkchoice_update_params = ForkchoiceUpdateParameters { - head_root: Hash256::zero(), - head_hash: None, - justified_hash: Some(justified_execution_hash), - finalized_hash: Some(finalized_execution_hash), - }; - - let payload_response_type = builder - .el - .get_full_payload_caching( - head_execution_hash, - &payload_attributes, - forkchoice_update_params, - fork, - ) - .await - .map_err(|_| reject("couldn't get payload"))?; - - let mut message = match payload_response_type { - crate::GetPayloadResponseType::Full(payload_response) => { - #[allow(clippy::type_complexity)] - let (payload, value, maybe_blobs_bundle, maybe_requests): ( - ExecutionPayload, - Uint256, - Option>, - Option>, - ) = payload_response.into(); - - match fork { - ForkName::Electra => BuilderBid::Electra(BuilderBidElectra { - header: payload - .as_electra() - .map_err(|_| reject("incorrect payload variant"))? - .into(), - blob_kzg_commitments: maybe_blobs_bundle - .map(|b| b.commitments) - .unwrap_or_default(), - value, - pubkey: builder.builder_sk.public_key().compress(), - execution_requests: maybe_requests.unwrap_or_default(), - }), - ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { - header: payload - .as_deneb() - .map_err(|_| reject("incorrect payload variant"))? - .into(), - blob_kzg_commitments: maybe_blobs_bundle - .map(|b| b.commitments) - .unwrap_or_default(), - value, - pubkey: builder.builder_sk.public_key().compress(), - }), - ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { - header: payload - .as_capella() - .map_err(|_| reject("incorrect payload variant"))? - .into(), - value, - pubkey: builder.builder_sk.public_key().compress(), - }), - ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { - header: payload - .as_bellatrix() - .map_err(|_| reject("incorrect payload variant"))? - .into(), - value, - pubkey: builder.builder_sk.public_key().compress(), - }), - ForkName::Base | ForkName::Altair => { - return Err(reject("invalid fork")) - } - } - } - crate::GetPayloadResponseType::Blinded(payload_response) => { - #[allow(clippy::type_complexity)] - let (payload, value, maybe_blobs_bundle, maybe_requests): ( - ExecutionPayload, - Uint256, - Option>, - Option>, - ) = payload_response.into(); - match fork { - ForkName::Electra => BuilderBid::Electra(BuilderBidElectra { - header: payload - .as_electra() - .map_err(|_| reject("incorrect payload variant"))? - .into(), - blob_kzg_commitments: maybe_blobs_bundle - .map(|b| b.commitments) - .unwrap_or_default(), - value, - pubkey: builder.builder_sk.public_key().compress(), - execution_requests: maybe_requests.unwrap_or_default(), - }), - ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { - header: payload - .as_deneb() - .map_err(|_| reject("incorrect payload variant"))? - .into(), - blob_kzg_commitments: maybe_blobs_bundle - .map(|b| b.commitments) - .unwrap_or_default(), - value, - pubkey: builder.builder_sk.public_key().compress(), - }), - ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { - header: payload - .as_capella() - .map_err(|_| reject("incorrect payload variant"))? - .into(), - value, - pubkey: builder.builder_sk.public_key().compress(), - }), - ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { - header: payload - .as_bellatrix() - .map_err(|_| reject("incorrect payload variant"))? - .into(), - value, - pubkey: builder.builder_sk.public_key().compress(), - }), - ForkName::Base | ForkName::Altair => { - return Err(reject("invalid fork")) - } - } - } - }; - - message.set_gas_limit(cached_data.gas_limit); - - builder.apply_operations(&mut message); - - let mut signature = - message.sign_builder_message(&builder.builder_sk, &builder.spec); - - if *builder.invalidate_signatures.read() { - signature = Signature::empty(); - } + .map_err(|e| warp::reject::custom(Custom(e)))?; - let fork_name = builder - .spec - .fork_name_at_epoch(slot.epoch(E::slots_per_epoch())); - let signed_bid = SignedBuilderBid { message, signature }; let resp: ForkVersionedResponse<_> = ForkVersionedResponse { version: Some(fork_name), metadata: Default::default(), diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index f19f84891e1..1e71fde2551 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -30,7 +30,7 @@ pub use execution_block_generator::{ static_valid_tx, Block, ExecutionBlockGenerator, }; pub use hook::Hook; -pub use mock_builder::{MockBuilder, Operation, serve as serve_mock_builder}; +pub use mock_builder::{MockBuilder, Operation}; pub use mock_execution_layer::MockExecutionLayer; pub const DEFAULT_TERMINAL_DIFFICULTY: u64 = 6400; From 140d4cf9d7a722799cf9d5ce21aa122a066c2340 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 12 Dec 2024 12:55:55 -0800 Subject: [PATCH 19/27] Address review comments --- .../process_operations.rs | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) 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 81b5bac5e68..22d8592364c 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -636,10 +636,10 @@ fn is_valid_switch_to_compounding_request( state: &BeaconState, consolidation_request: &ConsolidationRequest, spec: &ChainSpec, -) -> bool { +) -> Result { // Switch to compounding requires source and target be equal if consolidation_request.source_pubkey != consolidation_request.target_pubkey { - return false; + return Ok(false); } // Verify pubkey exists @@ -648,13 +648,10 @@ fn is_valid_switch_to_compounding_request( .get(&consolidation_request.source_pubkey) else { // source validator doesn't exist - return false; + return Ok(false); }; - let Ok(source_validator) = state.get_validator(source_index) else { - // Validator must exist in the state else it wouldn't exist in the pubkey cache either - return false; - }; + let source_validator = state.get_validator(source_index)?; // Verify the source withdrawal credentials // Note: We need to specifically check for eth1 withdrawal credentials here // If the validator is already compounding, the compounding request is not valid. @@ -670,24 +667,24 @@ fn is_valid_switch_to_compounding_request( .flatten() { if withdrawal_address != consolidation_request.source_address { - return false; + return Ok(false); } } else { // Source doesn't have eth1 withdrawal credentials - return false; + return Ok(false); } // Verify the source is active let current_epoch = state.current_epoch(); if !source_validator.is_active_at(current_epoch) { - return false; + return Ok(false); } // Verify exits for source has not been initiated if source_validator.exit_epoch != spec.far_future_epoch { - return false; + return Ok(false); } - true + Ok(true) } pub fn process_consolidation_request( @@ -695,7 +692,7 @@ pub fn process_consolidation_request( consolidation_request: &ConsolidationRequest, spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { - if is_valid_switch_to_compounding_request(state, consolidation_request, spec) { + if is_valid_switch_to_compounding_request(state, consolidation_request, spec)? { let Some(source_index) = state .pubkey_cache() .get(&consolidation_request.source_pubkey) From da33bd6dd04d5e76f94c437e5ade772bc693da3c Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 13 Dec 2024 15:51:42 +1100 Subject: [PATCH 20/27] Use copied for reference rather than cloned --- .../execution_layer/src/engine_api/new_payload_request.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs index 06d72fe6163..60bc8489744 100644 --- a/beacon_node/execution_layer/src/engine_api/new_payload_request.rs +++ b/beacon_node/execution_layer/src/engine_api/new_payload_request.rs @@ -124,7 +124,7 @@ impl<'block, E: EthSpec> NewPayloadRequest<'block, E> { let (header_hash, rlp_transactions_root) = calculate_execution_block_hash( payload, parent_beacon_block_root, - self.execution_requests().ok().cloned(), + self.execution_requests().ok().copied(), ); if header_hash != self.block_hash() { From 0af6b0fe88a60ba7cb68500c089723cf2d355991 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 13 Dec 2024 15:52:07 +1100 Subject: [PATCH 21/27] Optimise and simplify PendingDepositsContext::new --- .../src/per_epoch_processing/errors.rs | 1 + .../src/per_epoch_processing/single_pass.rs | 26 ++++++++----------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/consensus/state_processing/src/per_epoch_processing/errors.rs b/consensus/state_processing/src/per_epoch_processing/errors.rs index b6c9dbea521..f45c55a7acf 100644 --- a/consensus/state_processing/src/per_epoch_processing/errors.rs +++ b/consensus/state_processing/src/per_epoch_processing/errors.rs @@ -28,6 +28,7 @@ pub enum EpochProcessingError { SinglePassMissingActivationQueue, MissingEarliestExitEpoch, MissingExitBalanceToConsume, + PendingDepositsLogicError, } impl From for EpochProcessingError { 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 bb33284d17a..01638284c96 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -945,7 +945,8 @@ impl PendingDepositsContext { // `true` both for the actual value & the default placeholder value (`FAR_FUTURE_EPOCH`). let mut is_validator_exited = false; let mut is_validator_withdrawn = false; - if let Some(validator_index) = state.pubkey_cache().get(&deposit.pubkey) { + let opt_validator_index = state.pubkey_cache().get(&deposit.pubkey); + if let Some(validator_index) = opt_validator_index { let validator = state.get_validator(validator_index)?; let already_exited = validator.exit_epoch < spec.far_future_epoch; // In the spec process_registry_updates is called before process_pending_deposits @@ -964,20 +965,15 @@ impl PendingDepositsContext { } if is_validator_withdrawn { - // Deposited balance will never become active. Queue a balance increase - // but do not consume churn - if let Some(validator_index) = state.pubkey_cache().get(&deposit.pubkey) { - validator_deposits_to_process - .entry(validator_index) - .or_insert(0) - .safe_add_assign(deposit.amount)?; - } else { - // The `PendingDeposit` is for a new validator - // We add the new validator to the state at the end of all processing. - // Note that the new validator will not be eligible for activation until - // next epoch, so none of the operations will affect a newly added validator. - new_validator_deposits.push(deposit.clone()); - } + // Deposited balance will never become active. Queue a balance increase but do not + // consume churn. Validator index must be known if the validator is known to be + // withdrawn (see calculation of `is_validator_withdrawn` above). + let validator_index = + opt_validator_index.ok_or(Error::PendingDepositsLogicError)?; + validator_deposits_to_process + .entry(validator_index) + .or_insert(0) + .safe_add_assign(deposit.amount)?; } else if is_validator_exited { // Validator is exiting, postpone the deposit until after withdrawable epoch deposits_to_postpone.push(deposit.clone()); From 21591a332392797f3a37125ce2bcea18ef59c635 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 13 Dec 2024 17:13:12 +1100 Subject: [PATCH 22/27] Fix processing of deposits with invalid signatures --- .../src/per_epoch_processing/single_pass.rs | 39 ++++++++----------- consensus/types/src/beacon_state.rs | 19 ++++++++- 2 files changed, 33 insertions(+), 25 deletions(-) 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 01638284c96..904e68e3686 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -364,35 +364,28 @@ pub fn process_epoch_single_pass( // the first deposit creates the new validator and the others are topups. // Each item in the vec is a (pubkey, validator_index) let mut added_validators = Vec::new(); - for deposit in ctxt.new_validator_deposits.into_iter() { + for deposit in ctxt.new_validator_deposits { let deposit_data = DepositData { pubkey: deposit.pubkey, withdrawal_credentials: deposit.withdrawal_credentials, amount: deposit.amount, signature: deposit.signature, }; - if is_valid_deposit_signature(&deposit_data, spec).is_ok() { - // If a validator with the same pubkey has already been added, do not add a new - // validator, just update the balance. - // Note: updating the `effective_balance` for the new validator is done after - // adding them to the state. - if let Some((_, validator_index)) = added_validators - .iter() - .find(|(pubkey, _)| *pubkey == deposit_data.pubkey) - { - let balance = state.get_balance_mut(*validator_index)?; - balance.safe_add_assign(deposit_data.amount)?; - } else { - // Apply the new deposit to the state - state.add_validator_to_registry( - deposit_data.pubkey, - deposit_data.withdrawal_credentials, - deposit_data.amount, - spec, - )?; - added_validators - .push((deposit_data.pubkey, state.validators().len().safe_sub(1)?)); - } + // Only check the signature if this is the first deposit for the validator, + // following the logic from `apply_pending_deposit` in the spec. + if let Some(validator_index) = state.get_validator_index(&deposit_data.pubkey)? { + state + .get_balance_mut(validator_index)? + .safe_add_assign(deposit_data.amount)?; + } else if is_valid_deposit_signature(&deposit_data, spec).is_ok() { + // Apply the new deposit to the state + let validator_index = state.add_validator_to_registry( + deposit_data.pubkey, + deposit_data.withdrawal_credentials, + deposit_data.amount, + spec, + )?; + added_validators.push((deposit_data.pubkey, validator_index)); } } if conf.effective_balance_updates { diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index a4057143681..77b72b209c8 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -1547,13 +1547,15 @@ impl BeaconState { .ok_or(Error::UnknownValidator(validator_index)) } + /// Add a validator to the registry and return the validator index that was allocated for it. pub fn add_validator_to_registry( &mut self, pubkey: PublicKeyBytes, withdrawal_credentials: Hash256, amount: u64, spec: &ChainSpec, - ) -> Result<(), Error> { + ) -> Result { + let index = self.validators().len(); let fork_name = self.fork_name_unchecked(); self.validators_mut().push(Validator::from_deposit( pubkey, @@ -1575,7 +1577,20 @@ impl BeaconState { inactivity_scores.push(0)?; } - Ok(()) + // Keep the pubkey cache up to date if it was up to date prior to this call. + // + // Doing this here while we know the pubkey and index is marginally quicker than doing it in + // a call to `update_pubkey_cache` later because we don't need to index into the validators + // tree again. + let pubkey_cache = self.pubkey_cache_mut(); + if pubkey_cache.len() == index { + let success = pubkey_cache.insert(pubkey, index); + if !success { + return Err(Error::PubkeyCacheInconsistent); + } + } + + Ok(index) } /// Safe copy-on-write accessor for the `validators` list. From d5ec058034bfccb3f37248834aa1c7b890f77add Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 13 Dec 2024 16:06:08 -0800 Subject: [PATCH 23/27] Remove redundant code in genesis init --- consensus/state_processing/src/genesis.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/consensus/state_processing/src/genesis.rs b/consensus/state_processing/src/genesis.rs index 00f055e8c31..00697def5d2 100644 --- a/consensus/state_processing/src/genesis.rs +++ b/consensus/state_processing/src/genesis.rs @@ -123,19 +123,6 @@ pub fn initialize_beacon_state_from_eth1( // Remove intermediate Deneb fork from `state.fork`. state.fork_mut().previous_version = spec.electra_fork_version; - // iterate over an empty pending_deposits list here and increase balance - let pending_deposits = state.pending_deposits()?.clone(); - for pending_deposit in pending_deposits.into_iter() { - if let Some(validator_index) = state.pubkey_cache().get(&pending_deposit.pubkey) { - state - .balances_mut() - .get_mut(validator_index) - .ok_or(Error::UnknownValidator(validator_index))? - .safe_add(pending_deposit.amount)?; - } - } - *state.pending_deposits_mut()? = List::empty(); - // TODO(electra): think about this more and determine the best way to // do this. The spec tests will expect that the sync committees are // calculated using the electra value for MAX_EFFECTIVE_BALANCE when From 5a3f5dfea1df808b2a34b71019d1c3ad3505e064 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 13 Dec 2024 16:56:20 -0800 Subject: [PATCH 24/27] Revert "Refactor mock builder to separate functionality" This reverts commit 6d10456912b3c39b8a8c9089db76e8ead20608a0. --- .../src/test_utils/mock_builder.rs | 640 ++++++++---------- .../execution_layer/src/test_utils/mod.rs | 2 +- 2 files changed, 300 insertions(+), 342 deletions(-) diff --git a/beacon_node/execution_layer/src/test_utils/mock_builder.rs b/beacon_node/execution_layer/src/test_utils/mock_builder.rs index d2adbc30783..88ca399e17f 100644 --- a/beacon_node/execution_layer/src/test_utils/mock_builder.rs +++ b/beacon_node/execution_layer/src/test_utils/mock_builder.rs @@ -287,339 +287,8 @@ impl MockBuilder { op.apply(bid); } } - - /// Return the public key of the builder - pub fn public_key(&self) -> PublicKeyBytes { - self.builder_sk.public_key().compress() - } - - pub async fn register_validators( - &self, - registrations: Vec, - ) -> Result<(), String> { - for registration in registrations { - if !registration.verify_signature(&self.spec) { - return Err("invalid signature".to_string()); - } - self.val_registration_cache - .write() - .insert(registration.message.pubkey, registration); - } - Ok(()) - } - - pub async fn submit_blinded_block( - &self, - block: SignedBlindedBeaconBlock, - ) -> Result, String> { - let root = match &block { - SignedBlindedBeaconBlock::Base(_) | types::SignedBeaconBlock::Altair(_) => { - return Err("invalid fork".to_string()); - } - SignedBlindedBeaconBlock::Bellatrix(block) => { - block.message.body.execution_payload.tree_hash_root() - } - SignedBlindedBeaconBlock::Capella(block) => { - block.message.body.execution_payload.tree_hash_root() - } - SignedBlindedBeaconBlock::Deneb(block) => { - block.message.body.execution_payload.tree_hash_root() - } - SignedBlindedBeaconBlock::Electra(block) => { - block.message.body.execution_payload.tree_hash_root() - } - }; - let payload = self - .el - .get_payload_by_root(&root) - .ok_or_else(|| "missing payload for tx root".to_string())?; - let (payload, _) = payload.deconstruct(); - // TODO(pawan): the reconstruction in the beacon client should propagate the - // blobs as well. If that doesn't happen then its probably a bug here - self.beacon_client - .post_beacon_blinded_blocks(&block) - .await - .map_err(|e| format!("Failed to post blinded block {:?}", e))?; - Ok(payload) - } - - pub async fn get_header( - &self, - slot: Slot, - parent_hash: ExecutionBlockHash, - pubkey: PublicKeyBytes, - ) -> Result, String> { - let fork = self.fork_name_at_slot(slot); - let signed_cached_data = self - .val_registration_cache - .read() - .get(&pubkey) - .ok_or_else(|| "missing registration".to_string())? - .clone(); - let cached_data = signed_cached_data.message; - - let head = self - .beacon_client - .get_beacon_blocks::(BlockId::Head) - .await - .map_err(|_| "couldn't get head".to_string())? - .ok_or_else(|| "missing head block".to_string())?; - - let block = head.data.message(); - let head_block_root = block.tree_hash_root(); - let head_execution_hash = block - .body() - .execution_payload() - .map_err(|_| "pre-merge block".to_string())? - .block_hash(); - if head_execution_hash != parent_hash { - return Err("head mismatch".to_string()); - } - - let finalized_execution_hash = self - .beacon_client - .get_beacon_blocks::(BlockId::Finalized) - .await - .map_err(|_| "couldn't get finalized block".to_string())? - .ok_or_else(|| "missing finalized block".to_string())? - .data - .message() - .body() - .execution_payload() - .map_err(|_| "pre-merge block".to_string())? - .block_hash(); - - let justified_execution_hash = self - .beacon_client - .get_beacon_blocks::(BlockId::Justified) - .await - .map_err(|_| "couldn't get justified block".to_string())? - .ok_or_else(|| "missing justified block".to_string())? - .data - .message() - .body() - .execution_payload() - .map_err(|_| "pre-merge block".to_string())? - .block_hash(); - - let val_index = self - .beacon_client - .get_beacon_states_validator_id(StateId::Head, &ValidatorId::PublicKey(pubkey)) - .await - .map_err(|_| "couldn't get validator".to_string())? - .ok_or_else(|| "missing validator".to_string())? - .data - .index; - let fee_recipient = cached_data.fee_recipient; - let slots_since_genesis = slot.as_u64() - self.spec.genesis_slot.as_u64(); - - // TODO(pawan): cache this call - let genesis_data = self - .beacon_client - .get_beacon_genesis() - .await - .map_err(|_| "couldn't get beacon genesis".to_string())? - .data; - let genesis_time = genesis_data.genesis_time; - let timestamp = (slots_since_genesis * self.spec.seconds_per_slot) + genesis_time; - - let head_state: BeaconState = self - .beacon_client - .get_debug_beacon_states(StateId::Head) - .await - .map_err(|_| "couldn't get state".to_string())? - .ok_or_else(|| "missing state".to_string())? - .data; - let prev_randao = head_state - .get_randao_mix(head_state.current_epoch()) - .map_err(|_| "couldn't get prev randao".to_string())?; - - let expected_withdrawals = if fork.capella_enabled() { - Some( - self.beacon_client - .get_expected_withdrawals(&StateId::Head) - .await - .unwrap() - .data, - ) - } else { - None - }; - - let payload_attributes = match fork { - // the withdrawals root is filled in by operations, but we supply the valid withdrawals - // first to avoid polluting the execution block generator with invalid payload attributes - // NOTE: this was part of an effort to add payload attribute uniqueness checks, - // which was abandoned because it broke too many tests in subtle ways. - ForkName::Bellatrix | ForkName::Capella => PayloadAttributes::new( - timestamp, - *prev_randao, - fee_recipient, - expected_withdrawals, - None, - ), - ForkName::Deneb | ForkName::Electra => PayloadAttributes::new( - timestamp, - *prev_randao, - fee_recipient, - expected_withdrawals, - Some(head_block_root), - ), - ForkName::Base | ForkName::Altair => { - return Err("invalid fork".to_string()); - } - }; - - self.el - .insert_proposer(slot, head_block_root, val_index, payload_attributes.clone()) - .await; - - let forkchoice_update_params = ForkchoiceUpdateParameters { - head_root: Hash256::zero(), - head_hash: None, - justified_hash: Some(justified_execution_hash), - finalized_hash: Some(finalized_execution_hash), - }; - - let payload_response_type = self - .el - .get_full_payload_caching( - head_execution_hash, - &payload_attributes, - forkchoice_update_params, - fork, - ) - .await - .map_err(|_| "couldn't get payload".to_string())?; - - let mut message = match payload_response_type { - crate::GetPayloadResponseType::Full(payload_response) => { - #[allow(clippy::type_complexity)] - let (payload, value, maybe_blobs_bundle, maybe_requests): ( - ExecutionPayload, - Uint256, - Option>, - Option>, - ) = payload_response.into(); - - match fork { - ForkName::Electra => BuilderBid::Electra(BuilderBidElectra { - header: payload - .as_electra() - .map_err(|_| "incorrect payload variant".to_string())? - .into(), - blob_kzg_commitments: maybe_blobs_bundle - .map(|b| b.commitments) - .unwrap_or_default(), - value, - pubkey: self.builder_sk.public_key().compress(), - execution_requests: maybe_requests.unwrap_or_default(), - }), - ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { - header: payload - .as_deneb() - .map_err(|_| "incorrect payload variant".to_string())? - .into(), - blob_kzg_commitments: maybe_blobs_bundle - .map(|b| b.commitments) - .unwrap_or_default(), - value, - pubkey: self.builder_sk.public_key().compress(), - }), - ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { - header: payload - .as_capella() - .map_err(|_| "incorrect payload variant".to_string())? - .into(), - value, - pubkey: self.builder_sk.public_key().compress(), - }), - ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { - header: payload - .as_bellatrix() - .map_err(|_| "incorrect payload variant".to_string())? - .into(), - value, - pubkey: self.builder_sk.public_key().compress(), - }), - ForkName::Base | ForkName::Altair => return Err("invalid fork".to_string()), - } - } - crate::GetPayloadResponseType::Blinded(payload_response) => { - #[allow(clippy::type_complexity)] - let (payload, value, maybe_blobs_bundle, maybe_requests): ( - ExecutionPayload, - Uint256, - Option>, - Option>, - ) = payload_response.into(); - match fork { - ForkName::Electra => BuilderBid::Electra(BuilderBidElectra { - header: payload - .as_electra() - .map_err(|_| "incorrect payload variant".to_string())? - .into(), - blob_kzg_commitments: maybe_blobs_bundle - .map(|b| b.commitments) - .unwrap_or_default(), - value, - pubkey: self.builder_sk.public_key().compress(), - execution_requests: maybe_requests.unwrap_or_default(), - }), - ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { - header: payload - .as_deneb() - .map_err(|_| "incorrect payload variant".to_string())? - .into(), - blob_kzg_commitments: maybe_blobs_bundle - .map(|b| b.commitments) - .unwrap_or_default(), - value, - pubkey: self.builder_sk.public_key().compress(), - }), - ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { - header: payload - .as_capella() - .map_err(|_| "incorrect payload variant".to_string())? - .into(), - value, - pubkey: self.builder_sk.public_key().compress(), - }), - ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { - header: payload - .as_bellatrix() - .map_err(|_| "incorrect payload variant".to_string())? - .into(), - value, - pubkey: self.builder_sk.public_key().compress(), - }), - ForkName::Base | ForkName::Altair => return Err("invalid fork".to_string()), - } - } - }; - - message.set_gas_limit(cached_data.gas_limit); - - self.apply_operations(&mut message); - - let mut signature = message.sign_builder_message(&self.builder_sk, &self.spec); - - if *self.invalidate_signatures.read() { - signature = Signature::empty(); - }; - let signed_bid = SignedBuilderBid { message, signature }; - Ok(signed_bid) - } - - fn fork_name_at_slot(&self, slot: Slot) -> ForkName { - self.spec.fork_name_at_slot::(slot) - } } -/// Serve the builder api using warp. Uses the functions defined in `MockBuilder` to serve -/// the requests. -/// -/// We should eventually move this to axum when we move everything else. pub fn serve( listen_addr: Ipv4Addr, listen_port: u16, @@ -639,10 +308,18 @@ pub fn serve( .and(ctx_filter.clone()) .and_then( |registrations: Vec, builder: MockBuilder| async move { - builder.register_validators(registrations).await.map_err(|e|warp::reject::custom(Custom(e)))?; - Ok::<_, Rejection>(warp::reply()) + for registration in registrations { + if !registration.verify_signature(&builder.spec) { + return Err(reject("invalid signature")); + } + builder + .val_registration_cache + .write() + .insert(registration.message.pubkey, registration); + } + Ok(warp::reply()) }, - ).boxed(); + ); let blinded_block = prefix @@ -655,10 +332,27 @@ pub fn serve( |block: SignedBlindedBeaconBlock, fork_name: ForkName, builder: MockBuilder| async move { + let root = match block { + SignedBlindedBeaconBlock::Base(_) | types::SignedBeaconBlock::Altair(_) => { + return Err(reject("invalid fork")); + } + SignedBlindedBeaconBlock::Bellatrix(block) => { + block.message.body.execution_payload.tree_hash_root() + } + SignedBlindedBeaconBlock::Capella(block) => { + block.message.body.execution_payload.tree_hash_root() + } + SignedBlindedBeaconBlock::Deneb(block) => { + block.message.body.execution_payload.tree_hash_root() + } + SignedBlindedBeaconBlock::Electra(block) => { + block.message.body.execution_payload.tree_hash_root() + } + }; let payload = builder - .submit_blinded_block(block) - .await - .map_err(|e| warp::reject::custom(Custom(e)))?; + .el + .get_payload_by_root(&root) + .ok_or_else(|| reject("missing payload for tx root"))?; let resp: ForkVersionedResponse<_> = ForkVersionedResponse { version: Some(fork_name), metadata: Default::default(), @@ -701,12 +395,276 @@ pub fn serve( parent_hash: ExecutionBlockHash, pubkey: PublicKeyBytes, builder: MockBuilder| async move { - let fork_name = builder.fork_name_at_slot(slot); - let signed_bid = builder - .get_header(slot, parent_hash, pubkey) + let fork = builder.spec.fork_name_at_slot::(slot); + let signed_cached_data = builder + .val_registration_cache + .read() + .get(&pubkey) + .ok_or_else(|| reject("missing registration"))? + .clone(); + let cached_data = signed_cached_data.message; + + let head = builder + .beacon_client + .get_beacon_blocks::(BlockId::Head) + .await + .map_err(|_| reject("couldn't get head"))? + .ok_or_else(|| reject("missing head block"))?; + + let block = head.data.message(); + let head_block_root = block.tree_hash_root(); + let head_execution_hash = block + .body() + .execution_payload() + .map_err(|_| reject("pre-merge block"))? + .block_hash(); + if head_execution_hash != parent_hash { + return Err(reject("head mismatch")); + } + + let finalized_execution_hash = builder + .beacon_client + .get_beacon_blocks::(BlockId::Finalized) + .await + .map_err(|_| reject("couldn't get finalized block"))? + .ok_or_else(|| reject("missing finalized block"))? + .data + .message() + .body() + .execution_payload() + .map_err(|_| reject("pre-merge block"))? + .block_hash(); + + let justified_execution_hash = builder + .beacon_client + .get_beacon_blocks::(BlockId::Justified) + .await + .map_err(|_| reject("couldn't get justified block"))? + .ok_or_else(|| reject("missing justified block"))? + .data + .message() + .body() + .execution_payload() + .map_err(|_| reject("pre-merge block"))? + .block_hash(); + + let val_index = builder + .beacon_client + .get_beacon_states_validator_id(StateId::Head, &ValidatorId::PublicKey(pubkey)) + .await + .map_err(|_| reject("couldn't get validator"))? + .ok_or_else(|| reject("missing validator"))? + .data + .index; + let fee_recipient = cached_data.fee_recipient; + let slots_since_genesis = slot.as_u64() - builder.spec.genesis_slot.as_u64(); + + let genesis_data = builder + .beacon_client + .get_beacon_genesis() + .await + .map_err(|_| reject("couldn't get beacon genesis"))? + .data; + let genesis_time = genesis_data.genesis_time; + let timestamp = + (slots_since_genesis * builder.spec.seconds_per_slot) + genesis_time; + + let head_state: BeaconState = builder + .beacon_client + .get_debug_beacon_states(StateId::Head) .await - .map_err(|e| warp::reject::custom(Custom(e)))?; + .map_err(|_| reject("couldn't get state"))? + .ok_or_else(|| reject("missing state"))? + .data; + let prev_randao = head_state + .get_randao_mix(head_state.current_epoch()) + .map_err(|_| reject("couldn't get prev randao"))?; + + let expected_withdrawals = if fork.capella_enabled() { + Some( + builder + .beacon_client + .get_expected_withdrawals(&StateId::Head) + .await + .unwrap() + .data, + ) + } else { + None + }; + + let payload_attributes = match fork { + // the withdrawals root is filled in by operations, but we supply the valid withdrawals + // first to avoid polluting the execution block generator with invalid payload attributes + // NOTE: this was part of an effort to add payload attribute uniqueness checks, + // which was abandoned because it broke too many tests in subtle ways. + ForkName::Bellatrix | ForkName::Capella => PayloadAttributes::new( + timestamp, + *prev_randao, + fee_recipient, + expected_withdrawals, + None, + ), + ForkName::Deneb | ForkName::Electra => PayloadAttributes::new( + timestamp, + *prev_randao, + fee_recipient, + expected_withdrawals, + Some(head_block_root), + ), + ForkName::Base | ForkName::Altair => { + return Err(reject("invalid fork")); + } + }; + + builder + .el + .insert_proposer(slot, head_block_root, val_index, payload_attributes.clone()) + .await; + + let forkchoice_update_params = ForkchoiceUpdateParameters { + head_root: Hash256::zero(), + head_hash: None, + justified_hash: Some(justified_execution_hash), + finalized_hash: Some(finalized_execution_hash), + }; + + let payload_response_type = builder + .el + .get_full_payload_caching( + head_execution_hash, + &payload_attributes, + forkchoice_update_params, + fork, + ) + .await + .map_err(|_| reject("couldn't get payload"))?; + + let mut message = match payload_response_type { + crate::GetPayloadResponseType::Full(payload_response) => { + #[allow(clippy::type_complexity)] + let (payload, value, maybe_blobs_bundle, maybe_requests): ( + ExecutionPayload, + Uint256, + Option>, + Option>, + ) = payload_response.into(); + + match fork { + ForkName::Electra => BuilderBid::Electra(BuilderBidElectra { + header: payload + .as_electra() + .map_err(|_| reject("incorrect payload variant"))? + .into(), + blob_kzg_commitments: maybe_blobs_bundle + .map(|b| b.commitments) + .unwrap_or_default(), + value, + pubkey: builder.builder_sk.public_key().compress(), + execution_requests: maybe_requests.unwrap_or_default(), + }), + ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { + header: payload + .as_deneb() + .map_err(|_| reject("incorrect payload variant"))? + .into(), + blob_kzg_commitments: maybe_blobs_bundle + .map(|b| b.commitments) + .unwrap_or_default(), + value, + pubkey: builder.builder_sk.public_key().compress(), + }), + ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { + header: payload + .as_capella() + .map_err(|_| reject("incorrect payload variant"))? + .into(), + value, + pubkey: builder.builder_sk.public_key().compress(), + }), + ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { + header: payload + .as_bellatrix() + .map_err(|_| reject("incorrect payload variant"))? + .into(), + value, + pubkey: builder.builder_sk.public_key().compress(), + }), + ForkName::Base | ForkName::Altair => { + return Err(reject("invalid fork")) + } + } + } + crate::GetPayloadResponseType::Blinded(payload_response) => { + #[allow(clippy::type_complexity)] + let (payload, value, maybe_blobs_bundle, maybe_requests): ( + ExecutionPayload, + Uint256, + Option>, + Option>, + ) = payload_response.into(); + match fork { + ForkName::Electra => BuilderBid::Electra(BuilderBidElectra { + header: payload + .as_electra() + .map_err(|_| reject("incorrect payload variant"))? + .into(), + blob_kzg_commitments: maybe_blobs_bundle + .map(|b| b.commitments) + .unwrap_or_default(), + value, + pubkey: builder.builder_sk.public_key().compress(), + execution_requests: maybe_requests.unwrap_or_default(), + }), + ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { + header: payload + .as_deneb() + .map_err(|_| reject("incorrect payload variant"))? + .into(), + blob_kzg_commitments: maybe_blobs_bundle + .map(|b| b.commitments) + .unwrap_or_default(), + value, + pubkey: builder.builder_sk.public_key().compress(), + }), + ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { + header: payload + .as_capella() + .map_err(|_| reject("incorrect payload variant"))? + .into(), + value, + pubkey: builder.builder_sk.public_key().compress(), + }), + ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { + header: payload + .as_bellatrix() + .map_err(|_| reject("incorrect payload variant"))? + .into(), + value, + pubkey: builder.builder_sk.public_key().compress(), + }), + ForkName::Base | ForkName::Altair => { + return Err(reject("invalid fork")) + } + } + } + }; + + message.set_gas_limit(cached_data.gas_limit); + + builder.apply_operations(&mut message); + + let mut signature = + message.sign_builder_message(&builder.builder_sk, &builder.spec); + + if *builder.invalidate_signatures.read() { + signature = Signature::empty(); + } + let fork_name = builder + .spec + .fork_name_at_epoch(slot.epoch(E::slots_per_epoch())); + let signed_bid = SignedBuilderBid { message, signature }; let resp: ForkVersionedResponse<_> = ForkVersionedResponse { version: Some(fork_name), metadata: Default::default(), diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index 1e71fde2551..f19f84891e1 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -30,7 +30,7 @@ pub use execution_block_generator::{ static_valid_tx, Block, ExecutionBlockGenerator, }; pub use hook::Hook; -pub use mock_builder::{MockBuilder, Operation}; +pub use mock_builder::{MockBuilder, Operation, serve as serve_mock_builder}; pub use mock_execution_layer::MockExecutionLayer; pub const DEFAULT_TERMINAL_DIFFICULTY: u64 = 6400; From ff687ed28253bacee70901f5aa8409b478da33a0 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 13 Dec 2024 16:56:28 -0800 Subject: [PATCH 25/27] Revert "Update builder api for electra" This reverts commit c5c9aca6db201c09c995756a11e9cb6a03b2ea99. --- beacon_node/execution_layer/src/lib.rs | 3 ++- .../src/test_utils/mock_builder.rs | 24 +++++++++---------- .../execution_layer/src/test_utils/mod.rs | 2 +- consensus/types/src/builder_bid.rs | 5 +--- 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 2963b3f9a7f..08a00d7bf8d 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -121,7 +121,8 @@ impl TryFrom> for ProvenancedPayload( let mut message = match payload_response_type { crate::GetPayloadResponseType::Full(payload_response) => { #[allow(clippy::type_complexity)] - let (payload, value, maybe_blobs_bundle, maybe_requests): ( + let (payload, _block_value, maybe_blobs_bundle, _maybe_requests): ( ExecutionPayload, Uint256, Option>, @@ -559,9 +559,8 @@ pub fn serve( blob_kzg_commitments: maybe_blobs_bundle .map(|b| b.commitments) .unwrap_or_default(), - value, + value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), pubkey: builder.builder_sk.public_key().compress(), - execution_requests: maybe_requests.unwrap_or_default(), }), ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { header: payload @@ -571,7 +570,7 @@ pub fn serve( blob_kzg_commitments: maybe_blobs_bundle .map(|b| b.commitments) .unwrap_or_default(), - value, + value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { @@ -579,7 +578,7 @@ pub fn serve( .as_capella() .map_err(|_| reject("incorrect payload variant"))? .into(), - value, + value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { @@ -587,7 +586,7 @@ pub fn serve( .as_bellatrix() .map_err(|_| reject("incorrect payload variant"))? .into(), - value, + value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Base | ForkName::Altair => { @@ -597,7 +596,7 @@ pub fn serve( } crate::GetPayloadResponseType::Blinded(payload_response) => { #[allow(clippy::type_complexity)] - let (payload, value, maybe_blobs_bundle, maybe_requests): ( + let (payload, _block_value, maybe_blobs_bundle, _maybe_requests): ( ExecutionPayload, Uint256, Option>, @@ -612,9 +611,8 @@ pub fn serve( blob_kzg_commitments: maybe_blobs_bundle .map(|b| b.commitments) .unwrap_or_default(), - value, + value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), pubkey: builder.builder_sk.public_key().compress(), - execution_requests: maybe_requests.unwrap_or_default(), }), ForkName::Deneb => BuilderBid::Deneb(BuilderBidDeneb { header: payload @@ -624,7 +622,7 @@ pub fn serve( blob_kzg_commitments: maybe_blobs_bundle .map(|b| b.commitments) .unwrap_or_default(), - value, + value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { @@ -632,7 +630,7 @@ pub fn serve( .as_capella() .map_err(|_| reject("incorrect payload variant"))? .into(), - value, + value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Bellatrix => BuilderBid::Bellatrix(BuilderBidBellatrix { @@ -640,7 +638,7 @@ pub fn serve( .as_bellatrix() .map_err(|_| reject("incorrect payload variant"))? .into(), - value, + value: Uint256::from(DEFAULT_BUILDER_PAYLOAD_VALUE_WEI), pubkey: builder.builder_sk.public_key().compress(), }), ForkName::Base | ForkName::Altair => { diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index f19f84891e1..1e71fde2551 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -30,7 +30,7 @@ pub use execution_block_generator::{ static_valid_tx, Block, ExecutionBlockGenerator, }; pub use hook::Hook; -pub use mock_builder::{MockBuilder, Operation, serve as serve_mock_builder}; +pub use mock_builder::{MockBuilder, Operation}; pub use mock_execution_layer::MockExecutionLayer; pub const DEFAULT_TERMINAL_DIFFICULTY: u64 = 6400; diff --git a/consensus/types/src/builder_bid.rs b/consensus/types/src/builder_bid.rs index 5d405d25401..9885f78474f 100644 --- a/consensus/types/src/builder_bid.rs +++ b/consensus/types/src/builder_bid.rs @@ -2,8 +2,7 @@ use crate::beacon_block_body::KzgCommitments; use crate::{ ChainSpec, EthSpec, ExecutionPayloadHeaderBellatrix, ExecutionPayloadHeaderCapella, ExecutionPayloadHeaderDeneb, ExecutionPayloadHeaderElectra, ExecutionPayloadHeaderRef, - ExecutionPayloadHeaderRefMut, ExecutionRequests, ForkName, ForkVersionDeserialize, SignedRoot, - Uint256, + ExecutionPayloadHeaderRefMut, ForkName, ForkVersionDeserialize, SignedRoot, Uint256, }; use bls::PublicKeyBytes; use bls::Signature; @@ -34,8 +33,6 @@ pub struct BuilderBid { pub header: ExecutionPayloadHeaderElectra, #[superstruct(only(Deneb, Electra))] pub blob_kzg_commitments: KzgCommitments, - #[superstruct(only(Electra))] - pub execution_requests: ExecutionRequests, #[serde(with = "serde_utils::quoted_u256")] pub value: Uint256, pub pubkey: PublicKeyBytes, From 75efd4ffbf9449182a44fdae2a30629a018e1695 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 16 Dec 2024 13:02:19 +1100 Subject: [PATCH 26/27] Simplify pre-activation sorting --- consensus/state_processing/src/upgrade/electra.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/consensus/state_processing/src/upgrade/electra.rs b/consensus/state_processing/src/upgrade/electra.rs index c69c8df5a13..cd6c9e59d4e 100644 --- a/consensus/state_processing/src/upgrade/electra.rs +++ b/consensus/state_processing/src/upgrade/electra.rs @@ -1,4 +1,5 @@ use bls::Signature; +use itertools::Itertools; use safe_arith::SafeArith; use std::mem; use types::{ @@ -39,23 +40,13 @@ pub fn upgrade_to_electra( // Add validators that are not yet active to pending balance deposits let validators = post.validators().clone(); - let mut pre_activation = validators + let pre_activation = validators .iter() .enumerate() .filter(|(_, validator)| validator.activation_epoch == spec.far_future_epoch) + .sorted_by_key(|(index, validator)| (validator.activation_eligibility_epoch, *index)) .collect::>(); - // Sort the indices by activation_eligibility_epoch and then by index - pre_activation.sort_by(|(index_a, val_a), (index_b, val_b)| { - if val_a.activation_eligibility_epoch == val_b.activation_eligibility_epoch { - index_a.cmp(index_b) - } else { - val_a - .activation_eligibility_epoch - .cmp(&val_b.activation_eligibility_epoch) - } - }); - // Process validators to queue entire balance and reset them for (index, _) in pre_activation { let balance = post From ebabd0f83616d2920f69c07fa193af3530ffa30e Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 16 Dec 2024 13:02:40 +1100 Subject: [PATCH 27/27] Fix stale validators used in upgrade_to_electra --- consensus/state_processing/src/upgrade/electra.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/consensus/state_processing/src/upgrade/electra.rs b/consensus/state_processing/src/upgrade/electra.rs index cd6c9e59d4e..1e64ef28978 100644 --- a/consensus/state_processing/src/upgrade/electra.rs +++ b/consensus/state_processing/src/upgrade/electra.rs @@ -77,6 +77,7 @@ pub fn upgrade_to_electra( } // Ensure early adopters of compounding credentials go through the activation churn + let validators = post.validators().clone(); for (index, validator) in validators.iter().enumerate() { if validator.has_compounding_withdrawal_credential(spec) { post.queue_excess_active_balance(index, spec)?;