Skip to content

Commit 61c296e

Browse files
james-prysmsaolyn
james-prysm
and
saolyn
authored
eip7251: Bugfix and more withdrawal tests (#14578)
* addressing bug with withdrawals for devnet 5 * changelog * fixing if statement check * adding test * terence's review comments * attempting to fix weird comment formatting * moving back more comments * Update CHANGELOG.md Co-authored-by: Sammy Rosso <[email protected]> --------- Co-authored-by: Sammy Rosso <[email protected]>
1 parent f264680 commit 61c296e

File tree

4 files changed

+136
-73
lines changed

4 files changed

+136
-73
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,8 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve
5858
- Fix keymanager API should return corrected error format for malformed tokens
5959
- Fix keymanager API so that get keys returns an empty response instead of a 500 error when using an unsupported keystore.
6060
- Small log imporvement, removing some redundant or duplicate logs
61+
- EIP7521 - Fixes withdrawal bug by accounting for pending partial withdrawals and deducting already withdrawn amounts from the sweep balance. [PR](https://github.com/prysmaticlabs/prysm/pull/14578)
62+
6163

6264
### Security
6365

‎beacon-chain/core/blocks/withdrawals.go‎

Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -120,35 +120,36 @@ func ValidateBLSToExecutionChange(st state.ReadOnlyBeaconState, signed *ethpb.Si
120120
//
121121
// Spec pseudocode definition:
122122
//
123-
// def process_withdrawals(state: BeaconState, payload: ExecutionPayload) -> None:
124-
// expected_withdrawals, partial_withdrawals_count = get_expected_withdrawals(state) # [Modified in Electra:EIP7251]
123+
// def process_withdrawals(state: BeaconState, payload: ExecutionPayload) -> None:
125124
//
126-
// assert len(payload.withdrawals) == len(expected_withdrawals)
125+
// expected_withdrawals, processed_partial_withdrawals_count = get_expected_withdrawals(state) # [Modified in Electra:EIP7251]
127126
//
128-
// for expected_withdrawal, withdrawal in zip(expected_withdrawals, payload.withdrawals):
129-
// assert withdrawal == expected_withdrawal
130-
// decrease_balance(state, withdrawal.validator_index, withdrawal.amount)
127+
// assert len(payload.withdrawals) == len(expected_withdrawals)
131128
//
132-
// # Update pending partial withdrawals [New in Electra:EIP7251]
133-
// state.pending_partial_withdrawals = state.pending_partial_withdrawals[partial_withdrawals_count:]
129+
// for expected_withdrawal, withdrawal in zip(expected_withdrawals, payload.withdrawals):
130+
// assert withdrawal == expected_withdrawal
131+
// decrease_balance(state, withdrawal.validator_index, withdrawal.amount)
134132
//
135-
// # Update the next withdrawal index if this block contained withdrawals
136-
// if len(expected_withdrawals) != 0:
137-
// latest_withdrawal = expected_withdrawals[-1]
138-
// state.next_withdrawal_index = WithdrawalIndex(latest_withdrawal.index + 1)
133+
// # Update pending partial withdrawals [New in Electra:EIP7251]
134+
// state.pending_partial_withdrawals = state.pending_partial_withdrawals[processed_partial_withdrawals_count:]
139135
//
140-
// # Update the next validator index to start the next withdrawal sweep
141-
// if len(expected_withdrawals) == MAX_WITHDRAWALS_PER_PAYLOAD:
142-
// # Next sweep starts after the latest withdrawal's validator index
143-
// next_validator_index = ValidatorIndex((expected_withdrawals[-1].validator_index + 1) % len(state.validators))
144-
// state.next_withdrawal_validator_index = next_validator_index
145-
// else:
146-
// # Advance sweep by the max length of the sweep if there was not a full set of withdrawals
147-
// next_index = state.next_withdrawal_validator_index + MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP
148-
// next_validator_index = ValidatorIndex(next_index % len(state.validators))
149-
// state.next_withdrawal_validator_index = next_validator_index
136+
// # Update the next withdrawal index if this block contained withdrawals
137+
// if len(expected_withdrawals) != 0:
138+
// latest_withdrawal = expected_withdrawals[-1]
139+
// state.next_withdrawal_index = WithdrawalIndex(latest_withdrawal.index + 1)
140+
//
141+
// # Update the next validator index to start the next withdrawal sweep
142+
// if len(expected_withdrawals) == MAX_WITHDRAWALS_PER_PAYLOAD:
143+
// # Next sweep starts after the latest withdrawal's validator index
144+
// next_validator_index = ValidatorIndex((expected_withdrawals[-1].validator_index + 1) % len(state.validators))
145+
// state.next_withdrawal_validator_index = next_validator_index
146+
// else:
147+
// # Advance sweep by the max length of the sweep if there was not a full set of withdrawals
148+
// next_index = state.next_withdrawal_validator_index + MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP
149+
// next_validator_index = ValidatorIndex(next_index % len(state.validators))
150+
// state.next_withdrawal_validator_index = next_validator_index
150151
func ProcessWithdrawals(st state.BeaconState, executionData interfaces.ExecutionData) (state.BeaconState, error) {
151-
expectedWithdrawals, partialWithdrawalsCount, err := st.ExpectedWithdrawals()
152+
expectedWithdrawals, processedPartialWithdrawalsCount, err := st.ExpectedWithdrawals()
152153
if err != nil {
153154
return nil, errors.Wrap(err, "could not get expected withdrawals")
154155
}
@@ -192,7 +193,7 @@ func ProcessWithdrawals(st state.BeaconState, executionData interfaces.Execution
192193
}
193194

194195
if st.Version() >= version.Electra {
195-
if err := st.DequeuePartialWithdrawals(partialWithdrawalsCount); err != nil {
196+
if err := st.DequeuePartialWithdrawals(processedPartialWithdrawalsCount); err != nil {
196197
return nil, fmt.Errorf("unable to dequeue partial withdrawals from state: %w", err)
197198
}
198199
}

‎beacon-chain/state/state-native/getters_withdrawal.go‎

Lines changed: 61 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -49,56 +49,59 @@ func (b *BeaconState) NextWithdrawalValidatorIndex() (primitives.ValidatorIndex,
4949
// Spec definition:
5050
//
5151
// def get_expected_withdrawals(state: BeaconState) -> Tuple[Sequence[Withdrawal], uint64]:
52-
// epoch = get_current_epoch(state)
53-
// withdrawal_index = state.next_withdrawal_index
54-
// validator_index = state.next_withdrawal_validator_index
55-
// withdrawals: List[Withdrawal] = []
52+
// epoch = get_current_epoch(state)
53+
// withdrawal_index = state.next_withdrawal_index
54+
// validator_index = state.next_withdrawal_validator_index
55+
// withdrawals: List[Withdrawal] = []
56+
// processed_partial_withdrawals_count = 0
5657
//
57-
// # [New in Electra:EIP7251] Consume pending partial withdrawals
58-
// for withdrawal in state.pending_partial_withdrawals:
59-
// if withdrawal.withdrawable_epoch > epoch or len(withdrawals) == MAX_PENDING_PARTIALS_PER_WITHDRAWALS_SWEEP:
60-
// break
58+
// # [New in Electra:EIP7251] Consume pending partial withdrawals
59+
// for withdrawal in state.pending_partial_withdrawals:
60+
// if withdrawal.withdrawable_epoch > epoch or len(withdrawals) == MAX_PENDING_PARTIALS_PER_WITHDRAWALS_SWEEP:
61+
// break
6162
//
62-
// validator = state.validators[withdrawal.index]
63-
// has_sufficient_effective_balance = validator.effective_balance >= MIN_ACTIVATION_BALANCE
64-
// has_excess_balance = state.balances[withdrawal.index] > MIN_ACTIVATION_BALANCE
65-
// if validator.exit_epoch == FAR_FUTURE_EPOCH and has_sufficient_effective_balance and has_excess_balance:
66-
// withdrawable_balance = min(state.balances[withdrawal.index] - MIN_ACTIVATION_BALANCE, withdrawal.amount)
67-
// withdrawals.append(Withdrawal(
68-
// index=withdrawal_index,
69-
// validator_index=withdrawal.index,
70-
// address=ExecutionAddress(validator.withdrawal_credentials[12:]),
71-
// amount=withdrawable_balance,
72-
// ))
73-
// withdrawal_index += WithdrawalIndex(1)
63+
// validator = state.validators[withdrawal.index]
64+
// has_sufficient_effective_balance = validator.effective_balance >= MIN_ACTIVATION_BALANCE
65+
// has_excess_balance = state.balances[withdrawal.index] > MIN_ACTIVATION_BALANCE
66+
// if validator.exit_epoch == FAR_FUTURE_EPOCH and has_sufficient_effective_balance and has_excess_balance:
67+
// withdrawable_balance = min(state.balances[withdrawal.index] - MIN_ACTIVATION_BALANCE, withdrawal.amount)
68+
// withdrawals.append(Withdrawal(
69+
// index=withdrawal_index,
70+
// validator_index=withdrawal.index,
71+
// address=ExecutionAddress(validator.withdrawal_credentials[12:]),
72+
// amount=withdrawable_balance,
73+
// ))
74+
// withdrawal_index += WithdrawalIndex(1)
7475
//
75-
// partial_withdrawals_count = len(withdrawals)
76+
// processed_partial_withdrawals_count += 1
7677
//
77-
// # Sweep for remaining.
78-
// bound = min(len(state.validators), MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP)
79-
// for _ in range(bound):
80-
// validator = state.validators[validator_index]
81-
// balance = state.balances[validator_index]
82-
// if is_fully_withdrawable_validator(validator, balance, epoch):
83-
// withdrawals.append(Withdrawal(
84-
// index=withdrawal_index,
85-
// validator_index=validator_index,
86-
// address=ExecutionAddress(validator.withdrawal_credentials[12:]),
87-
// amount=balance,
88-
// ))
89-
// withdrawal_index += WithdrawalIndex(1)
90-
// elif is_partially_withdrawable_validator(validator, balance):
91-
// withdrawals.append(Withdrawal(
92-
// index=withdrawal_index,
93-
// validator_index=validator_index,
94-
// address=ExecutionAddress(validator.withdrawal_credentials[12:]),
95-
// amount=balance - get_validator_max_effective_balance(validator), # [Modified in Electra:EIP7251]
96-
// ))
97-
// withdrawal_index += WithdrawalIndex(1)
98-
// if len(withdrawals) == MAX_WITHDRAWALS_PER_PAYLOAD:
99-
// break
100-
// validator_index = ValidatorIndex((validator_index + 1) % len(state.validators))
101-
// return withdrawals, partial_withdrawals_count
78+
// # Sweep for remaining.
79+
// bound = min(len(state.validators), MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP)
80+
// for _ in range(bound):
81+
// validator = state.validators[validator_index]
82+
// # [Modified in Electra:EIP7251]
83+
// partially_withdrawn_balance = sum(withdrawal.amount for withdrawal in withdrawals if withdrawal.validator_index == validator_index)
84+
// balance = state.balances[validator_index] - partially_withdrawn_balance
85+
// if is_fully_withdrawable_validator(validator, balance, epoch):
86+
// withdrawals.append(Withdrawal(
87+
// index=withdrawal_index,
88+
// validator_index=validator_index,
89+
// address=ExecutionAddress(validator.withdrawal_credentials[12:]),
90+
// amount=balance,
91+
// ))
92+
// withdrawal_index += WithdrawalIndex(1)
93+
// elif is_partially_withdrawable_validator(validator, balance):
94+
// withdrawals.append(Withdrawal(
95+
// index=withdrawal_index,
96+
// validator_index=validator_index,
97+
// address=ExecutionAddress(validator.withdrawal_credentials[12:]),
98+
// amount=balance - get_validator_max_effective_balance(validator), # [Modified in Electra:EIP7251]
99+
// ))
100+
// withdrawal_index += WithdrawalIndex(1)
101+
// if len(withdrawals) == MAX_WITHDRAWALS_PER_PAYLOAD:
102+
// break
103+
// validator_index = ValidatorIndex((validator_index + 1) % len(state.validators))
104+
// return withdrawals, processed_partial_withdrawals_count
102105
func (b *BeaconState) ExpectedWithdrawals() ([]*enginev1.Withdrawal, uint64, error) {
103106
if b.version < version.Capella {
104107
return nil, 0, errNotSupported("ExpectedWithdrawals", b.version)
@@ -113,7 +116,7 @@ func (b *BeaconState) ExpectedWithdrawals() ([]*enginev1.Withdrawal, uint64, err
113116
epoch := slots.ToEpoch(b.slot)
114117

115118
// Electra partial withdrawals functionality.
116-
var partialWithdrawalsCount uint64
119+
var processedPartialWithdrawalsCount uint64
117120
if b.version >= version.Electra {
118121
for _, w := range b.pendingPartialWithdrawals {
119122
if w.WithdrawableEpoch > epoch || len(withdrawals) >= int(params.BeaconConfig().MaxPendingPartialsPerWithdrawalsSweep) {
@@ -140,7 +143,7 @@ func (b *BeaconState) ExpectedWithdrawals() ([]*enginev1.Withdrawal, uint64, err
140143
})
141144
withdrawalIndex++
142145
}
143-
partialWithdrawalsCount++
146+
processedPartialWithdrawalsCount++
144147
}
145148
}
146149

@@ -155,6 +158,15 @@ func (b *BeaconState) ExpectedWithdrawals() ([]*enginev1.Withdrawal, uint64, err
155158
if err != nil {
156159
return nil, 0, errors.Wrapf(err, "could not retrieve balance at index %d", validatorIndex)
157160
}
161+
if b.version >= version.Electra {
162+
var partiallyWithdrawnBalance uint64
163+
for _, w := range withdrawals {
164+
if w.ValidatorIndex == validatorIndex {
165+
partiallyWithdrawnBalance += w.Amount
166+
}
167+
}
168+
balance = balance - partiallyWithdrawnBalance
169+
}
158170
if helpers.IsFullyWithdrawableValidator(val, balance, epoch, b.version) {
159171
withdrawals = append(withdrawals, &enginev1.Withdrawal{
160172
Index: withdrawalIndex,
@@ -181,7 +193,7 @@ func (b *BeaconState) ExpectedWithdrawals() ([]*enginev1.Withdrawal, uint64, err
181193
}
182194
}
183195

184-
return withdrawals, partialWithdrawalsCount, nil
196+
return withdrawals, processedPartialWithdrawalsCount, nil
185197
}
186198

187199
func (b *BeaconState) PendingPartialWithdrawals() ([]*ethpb.PendingPartialWithdrawal, error) {

‎beacon-chain/state/state-native/getters_withdrawal_test.go‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,4 +367,52 @@ func TestExpectedWithdrawals(t *testing.T) {
367367
require.NoError(t, err)
368368
require.Equal(t, uint64(10), partialWithdrawalsCount)
369369
})
370+
t.Run("electra same validator has one partially and one fully withdrawable", func(t *testing.T) {
371+
s, _ := util.DeterministicGenesisStateElectra(t, 1)
372+
vals := make([]*ethpb.Validator, 100)
373+
balances := make([]uint64, 100)
374+
for i := range vals {
375+
balances[i] = params.BeaconConfig().MaxEffectiveBalance
376+
val := &ethpb.Validator{
377+
WithdrawalCredentials: make([]byte, 32),
378+
EffectiveBalance: params.BeaconConfig().MaxEffectiveBalance,
379+
WithdrawableEpoch: primitives.Epoch(1),
380+
ExitEpoch: params.BeaconConfig().FarFutureEpoch,
381+
}
382+
val.WithdrawalCredentials[0] = params.BeaconConfig().ETH1AddressWithdrawalPrefixByte
383+
val.WithdrawalCredentials[31] = byte(i)
384+
vals[i] = val
385+
}
386+
balances[1] += params.BeaconConfig().MinDepositAmount
387+
vals[1].WithdrawableEpoch = primitives.Epoch(0)
388+
require.NoError(t, s.SetValidators(vals))
389+
require.NoError(t, s.SetBalances(balances))
390+
// Give validator a pending balance to withdraw.
391+
require.NoError(t, s.AppendPendingPartialWithdrawal(&ethpb.PendingPartialWithdrawal{
392+
Index: 1,
393+
Amount: balances[1], // will only deduct excess even though balance is more that minimum activation
394+
WithdrawableEpoch: primitives.Epoch(0),
395+
}))
396+
p, err := s.PendingPartialWithdrawals()
397+
require.NoError(t, err)
398+
require.Equal(t, 1, len(p))
399+
expected, _, err := s.ExpectedWithdrawals()
400+
require.NoError(t, err)
401+
require.Equal(t, 2, len(expected))
402+
403+
withdrawalFull := &enginev1.Withdrawal{
404+
Index: 1,
405+
ValidatorIndex: 1,
406+
Address: vals[1].WithdrawalCredentials[12:],
407+
Amount: balances[1] - params.BeaconConfig().MinDepositAmount, // subtract the partial from this
408+
}
409+
withdrawalPartial := &enginev1.Withdrawal{
410+
Index: 0,
411+
ValidatorIndex: 1,
412+
Address: vals[1].WithdrawalCredentials[12:],
413+
Amount: params.BeaconConfig().MinDepositAmount,
414+
}
415+
require.DeepEqual(t, withdrawalPartial, expected[0])
416+
require.DeepEqual(t, withdrawalFull, expected[1])
417+
})
370418
}

0 commit comments

Comments
 (0)