eip7732: add tests for process_withdrawals block processing
#4468
Conversation
terencechain
force-pushed
the
process-wd-tests
branch
3 times, most recently
from
5f30233 to
6836408
Compare
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as duplicate.
This comment was marked as duplicate.
| if is_post_eip7732(spec): | ||
| assert len(expected_withdrawals_result[0]) == 0 | ||
| else: | ||
| assert len(expected_withdrawals_result) == 0 |
There was a problem hiding this comment.
Why is the else condition necessary if we use @with_eip7732_and_later?
| else: | ||
| expected_withdrawals = spec.get_expected_withdrawals(state) |
There was a problem hiding this comment.
Same thing with this else block. We don't need to check is_post_eip7732.
| # Verify priority ordering: builder -> pending -> sweep | ||
| assert len(expected_withdrawals) == 3 | ||
| assert expected_withdrawals[0].validator_index == builder_index # Builder first | ||
| assert expected_withdrawals[1].validator_index == pending_index # Pending second | ||
| assert expected_withdrawals[2].validator_index == sweep_index # Sweep third |
There was a problem hiding this comment.
The naming here is a bit confusing. Maybe these would be better:
- Builder payments
- Pending partial withdrawals
- Exit/excess withdrawals
|
|
||
| @with_eip7732_and_later | ||
| @spec_state_test | ||
| def test_maximum_withdrawals_per_payload_limit(spec, state): |
There was a problem hiding this comment.
We should also test for:
- There are more than
MAX_WITHDRAWALS_PER_PAYLOADbuilder payments and the pending partial withdrawals queue is not empty. Assert that thebuilder_pending_withdrawalsqueue is still not empty. Assert that none of the other withdrawals were processed. - No builder payments,
MAX_WITHDRAWALS_PER_PAYLOADpending partial withdrawals, some sweep withdrawals. There would be sweep withdrawals because ofMAX_PENDING_PARTIALS_PER_WITHDRAWALS_SWEEP. - No builder payments, no pending partial withdrawals,
MAX_WITHDRAWALS_PER_PAYLOADsweep withdrawals.
| # Add multiple pending withdrawals | ||
| for i in range(3): | ||
| prepare_pending_withdrawal(spec, state, i) | ||
|
|
||
| # EIP-7732 limits pending withdrawals to min(MAX_PENDING_PARTIALS_PER_WITHDRAWALS_SWEEP, MAX_WITHDRAWALS_PER_PAYLOAD - 1) | ||
| # In minimal config: min(2, 4-1) = 2 | ||
| expected_withdrawals = min(3, spec.MAX_PENDING_PARTIALS_PER_WITHDRAWALS_SWEEP) |
There was a problem hiding this comment.
Maybe add enough pending partial withdrawals for this to test the mainnet value (8) too. So maybe queue MAX_PENDING_PARTIALS_PER_WITHDRAWALS_SWEEP withdrawals and set num_expected to that too?
| Test withdrawal processing with validator not yet active. | ||
| """ | ||
| set_parent_block_full(spec, state) | ||
| validator_index = min(len(state.validators) // 2, spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP - 1) |
| validator_index = min( | ||
| len(state.validators) // 2 + 1, spec.MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP - 1 | ||
| ) |
There was a problem hiding this comment.
Same here? Why not just index 0?
There was a problem hiding this comment.
We can do this in a follow up PR, but there are some missing tests that I would like to see here. We need to ensure that builders can perform the full range of "regular" withdrawals too. Eg a builder can do partial withdrawals.
| set_parent_block_full(spec, state) | ||
| current_epoch = spec.get_current_epoch(state) | ||
| set_validator_fully_withdrawable(spec, state, 3, current_epoch) | ||
| state.validators[3].effective_balance = 10000000000 |
There was a problem hiding this comment.
I'd prefer we use MIN_ACTIVATION_BALANCE here.
| current_epoch = spec.get_current_epoch(state) | ||
| set_validator_fully_withdrawable(spec, state, 4, current_epoch) | ||
| state.validators[4].effective_balance = 0 | ||
| state.balances[4] = 100000000 |
There was a problem hiding this comment.
I'd prefer we use MIN_ACTIVATION_BALANCE here too.
|
@terencechain currently failing lint checks. |
terencechain
force-pushed
the
process-wd-tests
branch
3 times, most recently
from
1870941 to
f07705d
Compare
jtraglia
force-pushed
the
process-wd-tests
branch
from
f07705d to
fa1b5c5
Compare
jtraglia
changed the title
process_withdrawals block processing
2 participants
No description provided.