From 4a8a9793ff9aa6b828d83e9f5c018f8cdb32ad95 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 24 Jun 2024 15:18:37 +1000 Subject: [PATCH 1/2] Update pending balance deposit processing --- .../src/per_epoch_processing/single_pass.rs | 58 +++++++++++++++---- 1 file changed, 48 insertions(+), 10 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 4ab756b561b..eef2251c2ad 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -17,8 +17,8 @@ use types::{ }, milhouse::Cow, ActivationQueue, BeaconState, BeaconStateError, ChainSpec, Checkpoint, Epoch, EthSpec, - ExitCache, ForkName, List, ParticipationFlags, ProgressiveBalancesCache, RelativeEpoch, - Unsigned, Validator, + ExitCache, ForkName, List, ParticipationFlags, PendingBalanceDeposit, ProgressiveBalancesCache, + RelativeEpoch, Unsigned, Validator, }; pub struct SinglePassConfig { @@ -91,6 +91,8 @@ struct PendingBalanceDepositsContext { 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, } struct EffectiveBalancesContext { @@ -342,12 +344,15 @@ pub fn process_epoch_single_pass( // of the `pending_balance_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 new_pending_balance_deposits = List::try_from_iter( + let mut new_pending_balance_deposits = List::try_from_iter( state .pending_balance_deposits()? .iter_from(ctxt.next_deposit_index)? .cloned(), )?; + for deposit in ctxt.deposits_to_postpone { + new_pending_balance_deposits.push(deposit)?; + } *state.pending_balance_deposits_mut()? = new_pending_balance_deposits; *state.deposit_balance_to_consume_mut()? = ctxt.deposit_balance_to_consume; } @@ -805,22 +810,54 @@ impl PendingBalanceDepositsContext { let available_for_processing = state .deposit_balance_to_consume()? .safe_add(state.get_activation_exit_churn_limit(spec)?)?; + let current_epoch = state.current_epoch(); let mut processed_amount = 0; 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() { - if processed_amount.safe_add(deposit.amount)? > available_for_processing { - 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. + // + // We need to work out if `validator.exit_epoch` will be set to a non-default value + // *after* changes applied by `process_registry_updates`, which in our implementation + // does not happen until after this (but in the spec happens before). However it's not + // hard to work out: we don't need to know exactly what value the `exit_epoch` will + // take, just whether it is non-default. Nor do we need to know the value of + // `withdrawable_epoch`, because `current_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; + let will_be_exited = validator.is_active_at(current_epoch) + && validator.effective_balance <= spec.ejection_balance; + if already_exited || will_be_exited { + if state.current_epoch() <= validator.withdrawable_epoch { + deposits_to_postpone.push(deposit.clone()); + } else { + // Deposited balance will never become active. Increase balance but do not + // consume churn. + validator_deposits_to_process + .entry(deposit.index as usize) + .or_insert(0) + .safe_add_assign(deposit.amount)?; + } + } 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 { + 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)?; } - validator_deposits_to_process - .entry(deposit.index as usize) - .or_insert(0) - .safe_add_assign(deposit.amount)?; - processed_amount.safe_add_assign(deposit.amount)?; + // Regardless of how the deposit was handled, we move on in the queue. next_deposit_index.safe_add_assign(1)?; } @@ -834,6 +871,7 @@ impl PendingBalanceDepositsContext { next_deposit_index, deposit_balance_to_consume, validator_deposits_to_process, + deposits_to_postpone, }) } } From d68fef31b803f3bc03dfe7f150c026d539fe05b8 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 1 Jul 2024 18:58:28 +0200 Subject: [PATCH 2/2] Update single_pass.rs --- .../state_processing/src/per_epoch_processing/single_pass.rs | 3 +++ 1 file changed, 3 insertions(+) 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 eef2251c2ad..402c721b592 100644 --- a/consensus/state_processing/src/per_epoch_processing/single_pass.rs +++ b/consensus/state_processing/src/per_epoch_processing/single_pass.rs @@ -831,6 +831,9 @@ impl PendingBalanceDepositsContext { // `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 let will_be_exited = validator.is_active_at(current_epoch) && validator.effective_balance <= spec.ejection_balance; if already_exited || will_be_exited {