-
Notifications
You must be signed in to change notification settings - Fork 973
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.
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
Withdrawals push #2836
Withdrawals push #2836
Conversation
minor Merge and forkchoice updates release
Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>
release v1.1.7
release v1.1.8
We may also consider a separate |
Ensure that the SSZ object classes are ordered
531fdb9
to
1ab5206
Compare
1ab5206
to
3dd83cf
Compare
We've been working on a proposal for execution layer only push with a mechanism closer to how the deposit contract works: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
I've run --preset=mainnet
with this PR successfully.
Should we finish tests/core/pyspec/eth2spec/test/capella/fork/test_capella_fork_random.py
in this PR? Or leave a TODO comment here?
```python | ||
def process_block(state: BeaconState, block: BeaconBlock) -> None: | ||
process_block_header(state, block) | ||
if is_execution_enabled(state, block.body): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this check and other checks in the spec that are related to the Merge transition should be removed in a cleanup fork after the Merge. Not directly related to Cappella but worth keeping in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think so too
will address in separate PR. It needs some testing fixes -- e.g. all transition tests need to be labeled with_only_bellatrix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be reviewed. There is a problem between the account and management account is being reviewed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work! left some comments
edit: i'll make a separate pass on the tests left one comment on a test case, otherwise they are a great start!
tests/core/pyspec/eth2spec/test/capella/block_processing/test_process_withdrawals.py
Show resolved
Hide resolved
Co-authored-by: Alex Stokes <r.alex.stokes@gmail.com>
…nto withdrawals-push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
BeaconState
maintains a queue ofwithdrawal_receipts
(one receipt added for each call towithdraw()
)min(MAX_WITHDRAWAL_TRANSACTIONS_PER_PAYLOAD, len(withdrawal_receipts))
receipts are dequeued into ExecutionPayload at each slotExecutionEngine
interfaceprocess_withdrawals
andprocess_full_withdrawals