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.

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

Merged
merged 19 commits into from
Jun 4, 2024
Merged

EL-triggered consolidations #3775

merged 19 commits into from
Jun 4, 2024

Conversation

fradamt
Copy link
Contributor

@fradamt fradamt commented May 21, 2024

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.

Copy link
Contributor

@hwwhww hwwhww left a 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.

specs/electra/beacon-chain.md Outdated Show resolved Hide resolved
Comment on lines +1318 to +1319
if source_index == target_index:
return
Copy link
Contributor

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?

Copy link
Contributor Author

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

Comment on lines +1301 to +1302
if get_consolidation_churn_limit(state) <= MIN_ACTIVATION_BALANCE:
return
Copy link
Contributor

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.

specs/electra/beacon-chain.md Show resolved Hide resolved
Copy link
Collaborator

@mkalinin mkalinin left a 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.

Copy link
Contributor

@hwwhww hwwhww left a 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!

Comment on lines -1324 to -1325
# Verify the same withdrawal address
assert source_validator.withdrawal_credentials[12:] == target_validator.withdrawal_credentials[12:]

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?

Copy link
Collaborator

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

Choose a reason for hiding this comment

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

Awesome. Appreciate the details 👍

@mkalinin mkalinin mentioned this pull request Sep 29, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants