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

EIP-7044: Lock voluntary exit domain on capella #3288

Merged
merged 14 commits into from
Jun 14, 2023

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Mar 10, 2023

Motivation

For staker UX it's useful that a signed exit is valid in perpetuity. Currently signed voluntary exits are valid for only two forks, due the state only holding current and previous fork.

Description

Compute voluntary exit domain as min(voluntary_exit.epoch, CAPELLA_FORK_EPOCH). This change is backwards compatible if merged before deneb.

I have committed this change in the phase0 spec since "technically" it does not change spec logic, but I can see that leaking CAPELLA_FORK_EPOCH into phase0 directory is not ideal.

@nisdas
Copy link
Contributor

nisdas commented Mar 10, 2023

It is too late at this stage to be introducing a consensus change unless it is a security issue.

@dapplion
Copy link
Collaborator Author

It is too late at this stage to be introducing a consensus change unless it is a security issue.

This PR would qualify as a deneb fork change, not capella.

@dapplion
Copy link
Collaborator Author

Moved the change to /deneb directory. See 1c35eb1 for previous iteration committing the change to phase0

@mcdee
Copy link
Contributor

mcdee commented Mar 11, 2023

I'm not sure this PR does what it wants to do. If we imagine that we are a few hard forks down the line, let's call it G, and someone submits a voluntary exit signed during Capella, then get_domain() is passed the epoch CAPELLA_FORK_EPOCH and the line:

fork_version = state.fork.previous_version if epoch < state.fork.epoch else state.fork.current_version

will return the fork version for F, so the signature will not verify.

Given that this is going to be a change to Deneb, could we achieve the desired result by replacing

domain = get_domain(state, DOMAIN_VOLUNTARY_EXIT, min(voluntary_exit.epoch, CAPELLA_FORK_EPOCH))

with

domain = compute_domain(DOMAIN_VOLUNTARY_EXIT, CAPELLA_FORK_VERSION, state.genesis_validators_root)

@ralexstokes
Copy link
Member

I'd rather move to EL-initiated exits rather than mess w/ the exit via CL operation infrastructure

main reason: we can get much more secure exit authorization strategies via e.g. 4337 smart wallets

@mcdee
Copy link
Contributor

mcdee commented Mar 13, 2023

I would suggest that this change is orthogonal to any sort of EL-initiated exit. It's a simple UX boost with only a minimal change to the consensus layer and no alteration to the execution layer. If EL-initiated exits ultimately replace this then fine, but that seems to me to be a fair few hard forks away compared to this change, which could easily slip in to deneb.

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm!

@zah
Copy link

zah commented May 30, 2023

This change is related to an earlier proposal for reducing the required trust between staking pool operators and their stakers. From conversations with multiple pool operators, I know this is a real problem they face.

#2666

Admittedly, smart contract initiated exits will be able to solve the problem as well.

@hwwhww
Copy link
Contributor

hwwhww commented Jun 7, 2023

@djrtwo @ralexstokes

I git merge #3406 and maked EIP7044 change in this branch:deneb-4844-clean...pr3288-comment

I will push it to this PR once #3406 is merged.

@hwwhww hwwhww changed the title Lock voluntary exit domain on capella EIP-7044: Lock voluntary exit domain on capella Jun 7, 2023
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.

LGTM

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

looks good! just a couple of minor comments

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

Looks good to me. I added a comment about how it breaks valid/invalid convention

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844 general:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants