Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

eip7251: Bugfix and more withdrawal tests #3979

Merged
merged 2 commits into from
Oct 30, 2024
Merged

eip7251: Bugfix and more withdrawal tests #3979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dcdf30e
Bugfix and more withdrawal tests
mkalinin Oct 16, 2024
62a3d1e
Apply suggestions by @jtraglia
mkalinin Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions specs/electra/beacon-chain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1056,7 +1056,7 @@ def get_expected_withdrawals(state: BeaconState) -> Tuple[Sequence[Withdrawal],
withdrawal_index = state.next_withdrawal_index
validator_index = state.next_withdrawal_validator_index
withdrawals: List[Withdrawal] = []
partial_withdrawals_count = 0
processed_partial_withdrawals_count = 0

# [New in Electra:EIP7251] Consume pending partial withdrawals
for withdrawal in state.pending_partial_withdrawals:
Expand All @@ -1076,13 +1076,16 @@ def get_expected_withdrawals(state: BeaconState) -> Tuple[Sequence[Withdrawal],
))
withdrawal_index += WithdrawalIndex(1)

partial_withdrawals_count += 1
processed_partial_withdrawals_count += 1

# Sweep for remaining.
bound = min(len(state.validators), MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP)
for _ in range(bound):
validator = state.validators[validator_index]
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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ppopth ppopth Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is effective_partial_withdrawals_count? I cannot find its definition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ppopth I think he meant processed_partial_withdrawals_count, the newly renamed variable.

Copy link
Collaborator Author

@mkalinin mkalinin Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense then.

balance = state.balances[validator_index] - partially_withdrawn_balance
if is_fully_withdrawable_validator(validator, balance, epoch):
withdrawals.append(Withdrawal(
index=withdrawal_index,
Expand All @@ -1102,7 +1105,7 @@ def get_expected_withdrawals(state: BeaconState) -> Tuple[Sequence[Withdrawal],
if len(withdrawals) == MAX_WITHDRAWALS_PER_PAYLOAD:
break
validator_index = ValidatorIndex((validator_index + 1) % len(state.validators))
return withdrawals, partial_withdrawals_count
return withdrawals, processed_partial_withdrawals_count
```

##### Modified `process_withdrawals`
Expand All @@ -1111,15 +1114,16 @@ def get_expected_withdrawals(state: BeaconState) -> Tuple[Sequence[Withdrawal],

```python
def process_withdrawals(state: BeaconState, payload: ExecutionPayload) -> None:
expected_withdrawals, partial_withdrawals_count = get_expected_withdrawals(state) # [Modified in Electra:EIP7251]
# [Modified in Electra:EIP7251]
expected_withdrawals, processed_partial_withdrawals_count = get_expected_withdrawals(state)

assert payload.withdrawals == expected_withdrawals

for withdrawal in expected_withdrawals:
decrease_balance(state, withdrawal.validator_index, withdrawal.amount)

# Update pending partial withdrawals [New in Electra:EIP7251]
state.pending_partial_withdrawals = state.pending_partial_withdrawals[partial_withdrawals_count:]
state.pending_partial_withdrawals = state.pending_partial_withdrawals[processed_partial_withdrawals_count:]

# Update the next withdrawal index if this block contained withdrawals
if len(expected_withdrawals) != 0:
Expand Down
Loading