From 9c00b06966bc11aa841e7983d3a65ffa5f779673 Mon Sep 17 00:00:00 2001 From: james-prysm <90280386+james-prysm@users.noreply.github.com> Date: Thu, 17 Apr 2025 13:17:53 -0500 Subject: [PATCH] fix expected withdrawals (#15191) * fixed underflow with expected withdrawals * update comment * Revert "update comment" This reverts commit e07da541ac80a28b7fe84b4ff436622cff92b046. * attempting to fix comment indents * fixing another missed tab in comments * trying tabs one more time for fixing tabs * adding undeflow safety * fixing error typo * missed wrapping the error --- .../state/state-native/getters_withdrawal.go | 25 +++++++++++--- .../state-native/getters_withdrawal_test.go | 34 +++++++++++++++++++ changelog/james-prysm_withdrawal-change.md | 3 ++ 3 files changed, 57 insertions(+), 5 deletions(-) create mode 100644 changelog/james-prysm_withdrawal-change.md diff --git a/beacon-chain/state/state-native/getters_withdrawal.go b/beacon-chain/state/state-native/getters_withdrawal.go index b84357dcad..8f2ac7defa 100644 --- a/beacon-chain/state/state-native/getters_withdrawal.go +++ b/beacon-chain/state/state-native/getters_withdrawal.go @@ -62,9 +62,11 @@ func (b *BeaconState) NextWithdrawalValidatorIndex() (primitives.ValidatorIndex, // // validator = state.validators[withdrawal.index] // has_sufficient_effective_balance = validator.effective_balance >= MIN_ACTIVATION_BALANCE -// has_excess_balance = state.balances[withdrawal.index] > MIN_ACTIVATION_BALANCE +// total_withdrawn = sum(w.amount for w in withdrawals if w.validator_index == withdrawal.validator_index) +// balance = state.balances[withdrawal.validator_index] - total_withdrawn +// has_excess_balance = balance > MIN_ACTIVATION_BALANCE // if validator.exit_epoch == FAR_FUTURE_EPOCH and has_sufficient_effective_balance and has_excess_balance: -// withdrawable_balance = min(state.balances[withdrawal.index] - MIN_ACTIVATION_BALANCE, withdrawal.amount) +// withdrawable_balance = min(balance - MIN_ACTIVATION_BALANCE, withdrawal.amount) // withdrawals.append(Withdrawal( // index=withdrawal_index, // validator_index=withdrawal.index, @@ -132,9 +134,19 @@ func (b *BeaconState) ExpectedWithdrawals() ([]*enginev1.Withdrawal, uint64, err return nil, 0, fmt.Errorf("could not retrieve balance at index %d: %w", w.Index, err) } hasSufficientEffectiveBalance := v.EffectiveBalance() >= params.BeaconConfig().MinActivationBalance - hasExcessBalance := vBal > params.BeaconConfig().MinActivationBalance + var totalWithdrawn uint64 + for _, wi := range withdrawals { + if wi.ValidatorIndex == w.Index { + totalWithdrawn += wi.Amount + } + } + balance, err := mathutil.Sub64(vBal, totalWithdrawn) + if err != nil { + return nil, 0, errors.Wrapf(err, "failed to subtract balance %d with total withdrawn %d", vBal, totalWithdrawn) + } + hasExcessBalance := balance > params.BeaconConfig().MinActivationBalance if v.ExitEpoch() == params.BeaconConfig().FarFutureEpoch && hasSufficientEffectiveBalance && hasExcessBalance { - amount := min(vBal-params.BeaconConfig().MinActivationBalance, w.Amount) + amount := min(balance-params.BeaconConfig().MinActivationBalance, w.Amount) withdrawals = append(withdrawals, &enginev1.Withdrawal{ Index: withdrawalIndex, ValidatorIndex: w.Index, @@ -165,7 +177,10 @@ func (b *BeaconState) ExpectedWithdrawals() ([]*enginev1.Withdrawal, uint64, err partiallyWithdrawnBalance += w.Amount } } - balance = balance - partiallyWithdrawnBalance + balance, err = mathutil.Sub64(balance, partiallyWithdrawnBalance) + if err != nil { + return nil, 0, errors.Wrapf(err, "could not subtract balance %d with partial withdrawn balance %d", balance, partiallyWithdrawnBalance) + } } if helpers.IsFullyWithdrawableValidator(val, balance, epoch, b.version) { withdrawals = append(withdrawals, &enginev1.Withdrawal{ diff --git a/beacon-chain/state/state-native/getters_withdrawal_test.go b/beacon-chain/state/state-native/getters_withdrawal_test.go index fd7f4e9eba..d9d3d7513e 100644 --- a/beacon-chain/state/state-native/getters_withdrawal_test.go +++ b/beacon-chain/state/state-native/getters_withdrawal_test.go @@ -416,3 +416,37 @@ func TestExpectedWithdrawals(t *testing.T) { require.DeepEqual(t, withdrawalFull, expected[1]) }) } + +func TestExpectedWithdrawals_underflow_electra(t *testing.T) { + s, err := state_native.InitializeFromProtoUnsafeElectra(ðpb.BeaconStateElectra{}) + require.NoError(t, err) + vals := make([]*ethpb.Validator, 1) + balances := make([]uint64, 1) + balances[0] = 2015_000_000_000 //Validator A begins leaking ETH due to inactivity, and over time, its balance decreases to 2,015 ETH + val := ðpb.Validator{ + WithdrawalCredentials: make([]byte, 32), + EffectiveBalance: params.BeaconConfig().MaxEffectiveBalanceElectra, + WithdrawableEpoch: primitives.Epoch(0), + ExitEpoch: params.BeaconConfig().FarFutureEpoch, + } + val.WithdrawalCredentials[0] = params.BeaconConfig().CompoundingWithdrawalPrefixByte + val.WithdrawalCredentials[31] = byte(0) + vals[0] = val + + require.NoError(t, s.SetValidators(vals)) + require.NoError(t, s.SetBalances(balances)) + require.NoError(t, s.AppendPendingPartialWithdrawal(ðpb.PendingPartialWithdrawal{ + Amount: 1008_000_000_000, + WithdrawableEpoch: primitives.Epoch(0), + })) + require.NoError(t, s.AppendPendingPartialWithdrawal(ðpb.PendingPartialWithdrawal{ + Amount: 1008_000_000_000, + WithdrawableEpoch: primitives.Epoch(0), + })) + expected, _, err := s.ExpectedWithdrawals() + require.NoError(t, err) + require.Equal(t, 3, len(expected)) // is a fully withdrawable validator + require.Equal(t, uint64(1008_000_000_000), expected[0].Amount) + require.Equal(t, uint64(975_000_000_000), expected[1].Amount) + require.Equal(t, uint64(32_000_000_000), expected[2].Amount) +} diff --git a/changelog/james-prysm_withdrawal-change.md b/changelog/james-prysm_withdrawal-change.md new file mode 100644 index 0000000000..3532683260 --- /dev/null +++ b/changelog/james-prysm_withdrawal-change.md @@ -0,0 +1,3 @@ +### Fixed + +- fixed underflow with balances in leaking edge case with expected withdrawals. \ No newline at end of file