calculate next slot's withdrawals properly even across epoch boundary (#6470)

beacon_chain/nimbus_beacon_node.nim

@@ -1244,8 +1244,6 @@ proc doppelgangerChecked(node: BeaconNode, epoch: Epoch) =
     for validator in node.attachedValidators[]:
       validator.doppelgangerChecked(epoch - 1)
 
-from ./spec/state_transition_epoch import effective_balance_might_update
-
 proc maybeUpdateActionTrackerNextEpoch(
     node: BeaconNode, forkyState: ForkyHashedBeaconState, nextEpoch: Epoch) =
   if node.consensusManager[].actionTracker.needsUpdate(

beacon_chain/spec/beaconstate.nim

@@ -1277,21 +1277,60 @@ func get_pending_balance_to_withdraw*(
 
   pending_balance
 
+# https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.7/specs/phase0/beacon-chain.md#effective-balances-updates
+template effective_balance_might_update*(
+    balance: Gwei, effective_balance: Gwei): bool =
+  const
+    HYSTERESIS_INCREMENT =
+      EFFECTIVE_BALANCE_INCREMENT.Gwei div HYSTERESIS_QUOTIENT
+    DOWNWARD_THRESHOLD = HYSTERESIS_INCREMENT * HYSTERESIS_DOWNWARD_MULTIPLIER
+    UPWARD_THRESHOLD = HYSTERESIS_INCREMENT * HYSTERESIS_UPWARD_MULTIPLIER
+  balance + DOWNWARD_THRESHOLD < effective_balance or
+    effective_balance + UPWARD_THRESHOLD < balance
+
+# https://github.com/ethereum/consensus-specs/blob/v1.4.0/specs/phase0/beacon-chain.md#effective-balances-updates
+# https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.1/specs/electra/beacon-chain.md#updated-process_effective_balance_updates
+template get_effective_balance_update*(
+    consensusFork: static ConsensusFork, balance: Gwei,
+    effective_balance: Gwei, vidx: uint64): Gwei =
+  when consensusFork <= ConsensusFork.Deneb:
+    min(
+      balance - balance mod EFFECTIVE_BALANCE_INCREMENT.Gwei,
+      MAX_EFFECTIVE_BALANCE.Gwei)
+  else:
+    debugComment "amortize validator read access"
+    let effective_balance_limit =
+      if has_compounding_withdrawal_credential(state.validators.item(vidx)):
+        MAX_EFFECTIVE_BALANCE_ELECTRA.Gwei
+      else:
+        MIN_ACTIVATION_BALANCE.Gwei
+    min(
+      balance - balance mod EFFECTIVE_BALANCE_INCREMENT.Gwei,
+      effective_balance_limit)
+
+template get_updated_effective_balance*(
+    consensusFork: static ConsensusFork, balance: Gwei,
+    effective_balance: Gwei, vidx: uint64): Gwei =
+  if effective_balance_might_update(balance, effective_balance):
+    get_effective_balance_update(consensusFork, balance, effective_balance, vidx)
+  else:
+    balance
+
 # https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.5/specs/capella/beacon-chain.md#new-get_expected_withdrawals
-func get_expected_withdrawals*(
+template get_expected_withdrawals_aux*(
-    state: capella.BeaconState | deneb.BeaconState): seq[Withdrawal] =
+    state: capella.BeaconState | deneb.BeaconState, epoch: Epoch,
+    fetch_balance: untyped): seq[Withdrawal] =
   let
-    epoch = get_current_epoch(state)
     num_validators = lenu64(state.validators)
     bound = min(len(state.validators), MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP)
   var
     withdrawal_index = state.next_withdrawal_index
-    validator_index = state.next_withdrawal_validator_index
+    validator_index {.inject.} = state.next_withdrawal_validator_index
     withdrawals: seq[Withdrawal] = @[]
   for _ in 0 ..< bound:
     let
       validator = state.validators[validator_index]
-      balance = state.balances[validator_index]
+      balance = fetch_balance
     if is_fully_withdrawable_validator(
         typeof(state).kind, validator, balance, epoch):
       var w = Withdrawal(
@@ -1315,13 +1354,20 @@ func get_expected_withdrawals*(
     validator_index = (validator_index + 1) mod num_validators
   withdrawals
 
+func get_expected_withdrawals*(
+    state: capella.BeaconState | deneb.BeaconState): seq[Withdrawal] =
+  get_expected_withdrawals_aux(state, get_current_epoch(state)) do:
+    state.balances[validator_index]
+
 # https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.3/specs/electra/beacon-chain.md#updated-get_expected_withdrawals
 # This partials count is used in exactly one place, while in general being able
 # to cleanly treat the results of get_expected_withdrawals as a seq[Withdrawal]
 # are valuable enough to make that the default version of this spec function.
-func get_expected_withdrawals_with_partial_count*(state: electra.BeaconState):
+template get_expected_withdrawals_with_partial_count_aux*(
+    state: electra.BeaconState, epoch: Epoch, fetch_balance: untyped):
     (seq[Withdrawal], uint64) =
-  let epoch = get_current_epoch(state)
+  doAssert epoch - get_current_epoch(state) in [0'u64, 1'u64]
+
   var
     withdrawal_index = state.next_withdrawal_index
     withdrawals: seq[Withdrawal] = @[]
@@ -1333,16 +1379,31 @@ func get_expected_withdrawals_with_partial_count*(state: electra.BeaconState):
       break
 
     let
-      validator = state.validators[withdrawal.index]
+      validator = state.validators.item(withdrawal.index)
+
+      # Keep a uniform variable name available for injected code
+      validator_index {.inject.} = withdrawal.index
+
+      # Here, can't use the pre-stored effective balance because this template
+      # might be called on the next slot and therefore next epoch, after which
+      # the effective balance might have updated.
+      effective_balance_at_slot =
+        if epoch == get_current_epoch(state):
+          validator.effective_balance
+        else:
+          get_updated_effective_balance(
+            typeof(state).kind, fetch_balance, validator.effective_balance,
+            validator_index)
+
       has_sufficient_effective_balance =
-        validator.effective_balance >= static(MIN_ACTIVATION_BALANCE.Gwei)
+        effective_balance_at_slot >= static(MIN_ACTIVATION_BALANCE.Gwei)
-      has_excess_balance =
+      has_excess_balance = fetch_balance > static(MIN_ACTIVATION_BALANCE.Gwei)
-        state.balances[withdrawal.index] > static(MIN_ACTIVATION_BALANCE.Gwei)
     if  validator.exit_epoch == FAR_FUTURE_EPOCH and
         has_sufficient_effective_balance and has_excess_balance:
-      let withdrawable_balance = min(
+      let
-        state.balances[withdrawal.index] - static(MIN_ACTIVATION_BALANCE.Gwei),
+        withdrawable_balance = min(
-        withdrawal.amount)
+          fetch_balance - static(MIN_ACTIVATION_BALANCE.Gwei),
+          withdrawal.amount)
       var w = Withdrawal(
         index: withdrawal_index,
         validator_index: withdrawal.index,
@@ -1356,13 +1417,13 @@ func get_expected_withdrawals_with_partial_count*(state: electra.BeaconState):
   let
     bound = min(len(state.validators), MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP)
     num_validators = lenu64(state.validators)
-  var validator_index = state.next_withdrawal_validator_index
+  var validator_index {.inject.} = state.next_withdrawal_validator_index
 
   # Sweep for remaining.
   for _ in 0 ..< bound:
     let
-      validator = state.validators[validator_index]
+      validator = state.validators.item(validator_index)
-      balance = state.balances[validator_index]
+      balance = fetch_balance
     if is_fully_withdrawable_validator(
         typeof(state).kind, validator, balance, epoch):
       var w = Withdrawal(
@@ -1388,6 +1449,12 @@ func get_expected_withdrawals_with_partial_count*(state: electra.BeaconState):
 
   (withdrawals, partial_withdrawals_count)
 
+template get_expected_withdrawals_with_partial_count*(
+    state: electra.BeaconState): (seq[Withdrawal], uint64) =
+  get_expected_withdrawals_with_partial_count_aux(
+      state, get_current_epoch(state)) do:
+    state.balances.item(validator_index)
+
 func get_expected_withdrawals*(state: electra.BeaconState): seq[Withdrawal] =
   get_expected_withdrawals_with_partial_count(state)[0]
 

beacon_chain/spec/state_transition_epoch.nim

@@ -1075,61 +1075,18 @@ func process_eth1_data_reset*(state: var ForkyBeaconState) =
   if next_epoch mod EPOCHS_PER_ETH1_VOTING_PERIOD == 0:
     state.eth1_data_votes = default(type state.eth1_data_votes)
 
-# https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.7/specs/phase0/beacon-chain.md#effective-balances-updates
-template effective_balance_might_update*(
-    balance: Gwei, effective_balance: Gwei): bool =
-  const
-    HYSTERESIS_INCREMENT =
-      EFFECTIVE_BALANCE_INCREMENT.Gwei div HYSTERESIS_QUOTIENT
-    DOWNWARD_THRESHOLD = HYSTERESIS_INCREMENT * HYSTERESIS_DOWNWARD_MULTIPLIER
-    UPWARD_THRESHOLD = HYSTERESIS_INCREMENT * HYSTERESIS_UPWARD_MULTIPLIER
-  balance + DOWNWARD_THRESHOLD < effective_balance or
-    effective_balance + UPWARD_THRESHOLD < balance
-
 # https://github.com/ethereum/consensus-specs/blob/v1.4.0/specs/phase0/beacon-chain.md#effective-balances-updates
-func process_effective_balance_updates*(
-    state: var (phase0.BeaconState | altair.BeaconState |
-                bellatrix.BeaconState | capella.BeaconState |
-                deneb.BeaconState)) =
-  # Update effective balances with hysteresis
-  for vidx in state.validators.vindices:
-    let
-      balance = state.balances.item(vidx)
-      effective_balance = state.validators.item(vidx).effective_balance
-    if effective_balance_might_update(balance, effective_balance):
-      let new_effective_balance =
-        min(
-          balance - balance mod EFFECTIVE_BALANCE_INCREMENT.Gwei,
-          MAX_EFFECTIVE_BALANCE.Gwei)
-      # Protect against unnecessary cache invalidation
-      if new_effective_balance != effective_balance:
-        state.validators.mitem(vidx).effective_balance = new_effective_balance
-
 # https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.1/specs/electra/beacon-chain.md#updated-process_effective_balance_updates
-func process_effective_balance_updates*(state: var electra.BeaconState) =
+func process_effective_balance_updates*(state: var ForkyBeaconState) =
   # Update effective balances with hysteresis
   for vidx in state.validators.vindices:
     let
       balance = state.balances.item(vidx)
       effective_balance = state.validators.item(vidx).effective_balance
+
     if effective_balance_might_update(balance, effective_balance):
-      debugComment "amortize validator read access"
+      let new_effective_balance = get_effective_balance_update(
-      # Wrapping MAX_EFFECTIVE_BALANCE_ELECTRA.Gwei and
+        typeof(state).kind, balance, effective_balance, vidx.distinctBase)
-      # MIN_ACTIVATION_BALANCE.Gwei in static() results
-      # in
-      # beacon_chain/spec/state_transition_epoch.nim(1067, 20) Error: expected: ':', but got: '('
-      # even though it'd be better to statically verify safety
-      let
-        effective_balance_limit =
-          if has_compounding_withdrawal_credential(
-              state.validators.item(vidx)):
-            MAX_EFFECTIVE_BALANCE_ELECTRA.Gwei
-          else:
-            MIN_ACTIVATION_BALANCE.Gwei
-        new_effective_balance =
-          min(
-            balance - balance mod EFFECTIVE_BALANCE_INCREMENT.Gwei,
-            effective_balance_limit)
       # Protect against unnecessary cache invalidation
       if new_effective_balance != effective_balance:
         state.validators.mitem(vidx).effective_balance = new_effective_balance
@@ -1564,9 +1521,8 @@ proc process_epoch*(
   ok()
 
 proc get_validator_balance_after_epoch*(
-    cfg: RuntimeConfig,
+    cfg: RuntimeConfig, state: deneb.BeaconState | electra.BeaconState,
-    state: deneb.BeaconState | electra.BeaconState,
+    cache: var StateCache, info: var altair.EpochInfo,
-    flags: UpdateFlags, cache: var StateCache, info: var altair.EpochInfo,
     index: ValidatorIndex): Gwei =
   # Run a subset of process_epoch() which affects an individual validator,
   # without modifying state itself
@@ -1586,7 +1542,7 @@ proc get_validator_balance_after_epoch*(
       weigh_justification_and_finalization(
         state, info.balances.current_epoch,
         info.balances.previous_epoch[TIMELY_TARGET_FLAG_INDEX],
-        info.balances.current_epoch_TIMELY_TARGET, flags)
+        info.balances.current_epoch_TIMELY_TARGET, {})
 
   # Used as part of process_rewards_and_penalties
   let inactivity_score =
@@ -1667,3 +1623,21 @@ proc get_validator_balance_after_epoch*(
           processed_amount += deposit.amount
 
   post_epoch_balance
+
+proc get_next_slot_expected_withdrawals*(
+    cfg: RuntimeConfig, state: deneb.BeaconState, cache: var StateCache,
+    info: var altair.EpochInfo): seq[Withdrawal] =
+  get_expected_withdrawals_aux(state, (state.slot + 1).epoch) do:
+    # validator_index is defined by an injected symbol within the template
+    get_validator_balance_after_epoch(
+      cfg, state, cache, info, validator_index.ValidatorIndex)
+
+proc get_next_slot_expected_withdrawals*(
+    cfg: RuntimeConfig, state: electra.BeaconState, cache: var StateCache,
+    info: var altair.EpochInfo): seq[Withdrawal] =
+  let (res, _) = get_expected_withdrawals_with_partial_count_aux(
+      state, (state.slot + 1).epoch) do:
+    # validator_index is defined by an injected symbol within the template
+    get_validator_balance_after_epoch(
+      cfg, state, cache, info, validator_index.ValidatorIndex)
+  res

beacon_chain/validator_bucket_sort.nim

@@ -75,7 +75,7 @@ func findValidatorIndex*(
     pubkey: ValidatorPubKey): Opt[ValidatorIndex] =
   for validatorIndex in bsv.extraItems:
     if validators[validatorIndex.distinctBase].pubkey == pubkey:
-      return Opt.some validatorIndex.ValidatorIndex
+      return Opt.some validatorIndex
   let
     bucketNumber = getBucketNumber(pubkey)
     lowerBounds =

beacon_chain/validators/beacon_validators.nim

@@ -443,8 +443,8 @@ proc getExecutionPayload(
     feeRecipient = $feeRecipient
 
   node.elManager.getPayload(
-      PayloadType, beaconHead.blck.bid.root, executionHead, latestSafe,
+    PayloadType, beaconHead.blck.bid.root, executionHead, latestSafe,
-      latestFinalized, timestamp, random, feeRecipient, withdrawals)
+    latestFinalized, timestamp, random, feeRecipient, withdrawals)
 
 # BlockRewards has issues resolving somehow otherwise
 import ".."/spec/state_transition_block

tests/test_toblindedblock.nim

@@ -128,4 +128,4 @@ suite "Blinded block conversions":
           deneb_steps
         when consensusFork >= ConsensusFork.Electra:
           debugComment "add electra_steps"
-        static: doAssert consensusFork.high == ConsensusFork.Electra
+        static: doAssert high(ConsensusFork) == ConsensusFork.Electra

tests/teststateutil.nim

@@ -14,7 +14,8 @@ import
 
 from ".."/beacon_chain/validator_bucket_sort import sortValidatorBuckets
 from ".."/beacon_chain/spec/state_transition_epoch import
-  get_validator_balance_after_epoch, process_epoch
+  get_validator_balance_after_epoch, get_next_slot_expected_withdrawals,
+  process_epoch
 
 func round_multiple_down(x: Gwei, n: Gwei): Gwei =
   ## Round the input to the previous multiple of "n"
@@ -100,6 +101,9 @@ proc getTestStates*(
     if tmpState[].kind == consensusFork:
       result.add assignClone(tmpState[])
 
+from std/sequtils import allIt
+from ".."/beacon_chain/spec/beaconstate import get_expected_withdrawals
+
 proc checkPerValidatorBalanceCalc*(
     state: deneb.BeaconState | electra.BeaconState): bool =
   var
@@ -107,10 +111,9 @@ proc checkPerValidatorBalanceCalc*(
     cache: StateCache
   let tmpState = newClone(state)  # slow, but tolerable for tests
   discard process_epoch(defaultRuntimeConfig, tmpState[], {}, cache, info)
-  for i in 0 ..< tmpState.balances.len:
-    if tmpState.balances.item(i) != get_validator_balance_after_epoch(
-        defaultRuntimeConfig, state, default(UpdateFlags), cache, info,
-        i.ValidatorIndex):
-      return false
 
-  true
+  allIt(0 ..< tmpState.balances.len,
+      tmpState.balances.item(it) == get_validator_balance_after_epoch(
+        defaultRuntimeConfig, state, cache, info, it.ValidatorIndex)) and
+    get_expected_withdrawals(tmpState[]) == get_next_slot_expected_withdrawals(
+      defaultRuntimeConfig, state, cache, info)