-
Notifications
You must be signed in to change notification settings - Fork 997
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
EL-triggered consolidations #3775
Conversation
Co-authored-by: Mikhail Kalinin <[email protected]>
This reverts commit dc2a2bd.
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 @fradamt
I had a review of the spec part and will review the tests part later.
if source_index == target_index: | ||
return |
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.
would it be more straightforward to use if request_source_pubkey == request_target_pubkey
here?
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 don't have a strong opinion here. Whatever we choose, we do use source_index
and target_index
later and it probably makes sense to define them
if get_consolidation_churn_limit(state) <= MIN_ACTIVATION_BALANCE: | ||
return |
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.
test case idea: have a validator get partial withdrawal and then process its consolidation at the same block.
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! 👍
We already have consolidation smart contract implementation (lightclient/sys-asm#14). The other missing part is the EL specification, and IMO it would be reasonable to wait for the decision on the alternative proposal to EIP-7685, which is drafted and explained here: ethereum/execution-apis#551. The proposal does also affects CL as it would need to keep all requests in the BeaconBlockBody
instead of the ExecutionPayload
structure.
…layer_withdrawal_request`
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 switched the order of process_deposit_receipt and process_execution_layer_withdrawal_request
calls.
The PR LGTM!
# Verify the same withdrawal address | ||
assert source_validator.withdrawal_credentials[12:] == target_validator.withdrawal_credentials[12:] |
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.
@hwwhww it is great to see that the consolidation of validators with different withdrawal addresses is possible now.
Was there any reason why it was not allowed initially (why ever this constraint has existed?), but became possible by this PR?
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.
It was not allowed initially because initially consolidation was authorized by the validator’s pubkey which isn’t secure enough to change the owner (withdrawal creds) of the funds. In order to make this possible, a consolidation request system smart contract has been introduced on EL with the corresponding changes on CL to make it possible to sign requests with withdrawal creds which unlocked moving funds between validators with different creds
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.
Awesome. Appreciate the details 👍
As decided prior to the interop, we want to move to EL-triggered consolidations, to simplify the way this feature affects staking pools. Like withdrawal requests, consolidation requests go in the execution payload, and processing failures result in a no-op instead of an invalid block.