eip7251: Bugfix and more withdrawal tests#3979
Conversation
jtraglia
left a comment
There was a problem hiding this comment.
Thanks for this! It looks really good.
- The name
processed_partial_withdrawals_countmakes sense 👍 - Good catch on the bug, the fix looks correct to me.
- The new tests are nice. I see no issues here, just a few questions.
tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawals.py
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawals.py
Outdated
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawals.py
Outdated
Show resolved
Hide resolved
| balance = state.balances[validator_index] | ||
| # [Modified in Electra:EIP7251] | ||
| partially_withdrawn_balance = sum( | ||
| withdrawal.amount for withdrawal in withdrawals if withdrawal.validator_index == validator_index) |
There was a problem hiding this comment.
Alternatively, it could iterate over withdrawals[:effective_partial_withdrawals_count] which contains partial withdrawals included during the current run. In cases when there are no withdrawal requests eligible for processing this computation will be a noop. But we can also leave the existing computation in the spec for simplicity and defer possible optimisation to the implementations
There was a problem hiding this comment.
What is effective_partial_withdrawals_count? I cannot find its definition.
There was a problem hiding this comment.
@ppopth I think he meant processed_partial_withdrawals_count, the newly renamed variable.
There was a problem hiding this comment.
effective_partial_withdrawals_count = len(withdrawals) computed before the sweep. processed_partial_withdrawals_count may be greater than that number due to skipped partial withdrawals
tests/core/pyspec/eth2spec/test/electra/block_processing/test_process_withdrawals.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
Follow up to #3943 with more withdrawal tests and yet another bugfix.
The PR does also proposes to rename
partial_withdrawals_counttoprocessed_partial_withdrawals_countand withdrawals test helper refactor.The fix
The balance withdrawn upon request, i.e. by processing a pending partial withdrawal, wasn’t taken into account for the sweep. Potential outcome is a validator being considered partially withdrawable while it has no excess balance and effectively is not partially withdrawable.
The proposed fix is simply to check if validator index is already presented in the withdrawals and deduct the already withdrawn balance from the balance considered by the sweep.