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

Simplify validator activation flow #3983

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

mkalinin
Copy link
Collaborator

@mkalinin mkalinin commented Oct 18, 2024

Deposit queue finalization proposed in #3818 allows for the following simplification in the validator activation flow:

  • Do not use activation_eligibility_epoch which is currently used by the protocol to finalize new validators before activating them — this is now done by finalizing the deposit queue
  • Activate new validator once it has sufficient effective balance
  • Set activation_eligibility_epoch for backwards compatibility

In future upgrades activation_eligibility_epoch can be deprecated and re-purposed for any other usage e.g. custom EB ceiling etc

ToDo

  • Fix tests

@dapplion
Copy link
Collaborator

Now activation_eligibility_epoch will remain set to far future epoch forever. If that's the case it can break the parsing of the validator statuses for API consumers, see https://hackmd.io/ofFJ5gOmQpu1jjHilHbdQQ. Not a big issue but we should communicate this breaking change.

@jtraglia
Copy link
Member

See the discussion about this PR on discord here:

I agree with @mcdee's idea:

I agree that it is redundant with your change, but setting it retains backwards-compatibility so avoids immediate issues. It could then be deprecated in Electra and use slowly removed downstream, eventually freeing up a slot in the validator struct (exciting!)

@ppopth
Copy link
Member

ppopth commented Oct 20, 2024

Why not just completely remove activation_eligibility_epoch instead of leaving it unset? It doesn't seem to beak any backward-compatibility, right?

If it breaks, it shouldn't break more than just unsetting it, right?

Copy link
Member

@ppopth ppopth left a comment

Choose a reason for hiding this comment

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

The idea looks good for me

@mkalinin
Copy link
Collaborator Author

Updated the proposed change with preserving the assignment of activation_eligibility_epoch

@mcdee
Copy link
Contributor

mcdee commented Oct 21, 2024

LGTM.

@mkalinin
Copy link
Collaborator Author

The following problem emerged during a deeper analysis of this change:

  • Eth1 bridge deposits yield a pending deposit with slot=GENESIS_SLOT to distinguish them from deposit requests, the distinguishing is important for Eth1 bridge deprecation
  • Side effect from the above is that Eth1 bridge deposits bypass finalization check in the deposit queue

Considering that Eth1 bridge deposits are lagging behind by ~11 hrs, it is fine for them to bypass the finality check because effectively they should be finalized many hours prior Electra. The activation_eligibility_epoch mechanism is already redundant today, the only case where it still matters is a period of non-finality which is longer than 11 hours.

Considering the above, I see three options:

  1. Leave the proposed change as is and assume that there will be no long period of non-finality on the mainnet around the Electra fork
  2. Use different flag e.g. signature=ETH1_BRIDGE_DEPOSIT_SIGNATURE to distinguish Eth1 bridge deposits and do finalize them before processing in the activation queue
  3. Postpone the change to the next upgrade to avoid unnecessary risk and additional considerations

@mcdee
Copy link
Contributor

mcdee commented Oct 22, 2024

It sounds like 2) is the better option (I'm assuming that 3) would revert to 2) after the next hard fork anyway).

@mkalinin
Copy link
Collaborator Author

It sounds like 2) is the better option (I'm assuming that 3) would revert to 2) after the next hard fork anyway).

There will be no Eth1 bridge deposits soon after Electra activation. So for the next hard fork we can safely take take the approach proposed in this PR

@mkalinin
Copy link
Collaborator Author

Option (2) has a flaw as anyone can submit a deposit request top-up with that signature and bypass finalization. Although, it shouldn’t be exploited in any adversarial way I would postpone this change to the next upgrade, i.e. option (3), another reason for that is that we want to finalize the spec for Electra asap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants