Fix EIP-7251 tests#3656
Conversation
ralexstokes
force-pushed
the
fix-7251-tests
branch
10 times, most recently
from
68b0aeb to
ebf1502
Compare
ralexstokes
force-pushed
the
fix-7251-tests
branch
3 times, most recently
from
a593987 to
56f087f
Compare
ralexstokes
force-pushed
the
fix-7251-tests
branch
2 times, most recently
from
5d6ffbd to
5447aa9
Compare
ralexstokes
force-pushed
the
fix-7251-tests
branch
7 times, most recently
from
6f4baab to
2085bef
Compare
ralexstokes
force-pushed
the
fix-7251-tests
branch
3 times, most recently
from
1d1b8ae to
57a6032
Compare
ralexstokes
force-pushed
the
fix-7251-tests
branch
from
57a6032 to
163f287
Compare
| source_index = spec.get_active_validator_indices(state, current_epoch)[0] | ||
| # Set source balance higher than consolidation churn limit | ||
| state.balances[source_index] = consolidation_churn_limit + 1 | ||
| state.validators[source_index].effective_balance = 2 * consolidation_churn_limit |
There was a problem hiding this comment.
| state.validators[source_index].effective_balance = 2 * consolidation_churn_limit | |
| state.validators[source_index].effective_balance = consolidation_churn_limit + spec.EFFECTIVE_BALANCE_INCREMENT |
I'd maybe do this instead, just to make it easier to interpret what happens. Here one sees the effective balance being 2x the churn limit, and it can be confusing why then expected_exit_epoch only gets a +1 instead of +2
|
need to review but likely want to merge this in first: ralexstokes#3 |
hwwhww
left a comment
hwwhww
left a comment
There was a problem hiding this comment.
well done 👍
note that some EIP-7251 results don't align with the testing goal/expections we had in the old tests. we'll need to have another scan of the missing edge cases.
|
|
||
| def get_expected_withdrawals(spec, state): | ||
| if is_post_eip7251(spec): | ||
| withdrawals, _ = spec.get_expected_withdrawals(state) |
There was a problem hiding this comment.
hmm, not suggesting changing in this PR, but ideally, it could be renamed into a new function name in Electra as the return values changed. e.g., get_expected_withdrawals_and_partial_withdrawals_count. kinda long though.
There was a problem hiding this comment.
yeah, if we look at how it is used, we could instead just pop() from the list and leave this function alone
update: remerkleable's pop doesn't return the value, just delete's from the list, so moving to this strategy would require changes to the underlying ssz library...
| # ensure we go through an epoch transition, to account for post-EIP-7251 behavior | ||
| block_in_next_epoch = build_empty_block(spec, state, slot=state.slot + spec.SLOTS_PER_EPOCH) | ||
| signed_block_in_next_epoch = state_transition_and_sign_block(spec, state, block_in_next_epoch) | ||
|
|
||
| yield 'blocks', [signed_block, signed_block_in_next_epoch] |
There was a problem hiding this comment.
should it be only with if is_post_eip7251: ... condition to distinguish the post-EIP-7251 behavior?
There was a problem hiding this comment.
I think its fine as extending the blocks is backwards-compatible with the capella and deneb behavior
New consolidation tests, comments
ralexstokes
force-pushed
the
fix-7251-tests
branch
from
084319e to
05a891f
Compare
3 participants
supersedes #3648.
spec fixes pulled out into #3657. will get that merged and rebase this to just the test fixes (may need to discuss some of them..)