-
Notifications
You must be signed in to change notification settings - Fork 220
Clarify pre-activation validator statuses and add deposit_inqueue for the post-Electra/Fulu deposit pipeline #618
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,8 +43,8 @@ ValidatorIdentityResponse: | |
| ValidatorStatus: | ||
| description: | | ||
| Possible statuses: | ||
| - **pending_initialized** - When the first deposit is processed, but not enough funds are available (or not yet the end of the first epoch) to get validator into the activation queue. | ||
| - **pending_queued** - When validator is waiting to get activated, and have enough funds etc. while in the queue, validator activation epoch keeps changing until it gets to the front and make it through (finalization is a requirement here too). | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what status do we use now if I will probably need @rolfyone to chime in here he knows the Electra changes much better than I do
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| - **pending_minBalance** - The validator exists in `state.validators` but its effective balance has not reached `MIN_ACTIVATION_BALANCE` (32 ETH), so it is not yet eligible for activation. Condition: `activation_eligibility_epoch == FAR_FUTURE_EPOCH`. (Formerly `pending_initialized`.) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what status do we return in the case that the validator has deposited 32 ETH but was not activated yet (before next epoch boundary)? There will be one epoch where no valid status exists now. Returning that the validator has not reached the min balance is wrong. as a side note, we probably wanna follow snake_case naming, ie. rename this to |
||
| - **pending_lookahead** - The validator is eligible (`activation_eligibility_epoch` is set and finalized) and its `activation_epoch` is assigned but in the future. Post-Electra (EIP-7251) this is a fixed `1 + MAX_SEED_LOOKAHEAD` delay, not a churn-limited queue: the `activation_epoch` does not change. Condition: `(activation_eligibility_epoch < FAR_FUTURE_EPOCH) and (activation_epoch > current_epoch)`. (Formerly `pending_queued`.) | ||
| - **active_ongoing** - When validator must be attesting, and have not initiated any exit. | ||
| - **active_exiting** - When validator is still active, but filed a voluntary request to exit. | ||
| - **active_slashed** - When validator is still active, but have a slashed status and is scheduled to exit. | ||
|
|
@@ -53,8 +53,10 @@ ValidatorStatus: | |
| - **withdrawal_possible** - After validator has exited, a while later is permitted to move funds, and is truly out of the system. | ||
| - **withdrawal_done** - (not possible in phase0, except slashing full balance) - actually having moved funds away | ||
|
|
||
| Note: post-Electra (EIP-6110), a deposit that is in `state.pending_deposits` but not yet in `state.validators` is not represented by a `ValidatorStatus` (there is no validator object yet). This deposit-pipeline state — referred to as `deposit_inqueue` — is the real churn-limited wait and is observable via the `getPendingDeposits` endpoint, not via this enum. | ||
|
|
||
| [Validator status specification](https://hackmd.io/ofFJ5gOmQpu1jjHilHbdQQ) | ||
| enum: ["pending_initialized", "pending_queued", "active_ongoing", "active_exiting", "active_slashed", "exited_unslashed", "exited_slashed", "withdrawal_possible", "withdrawal_done"] | ||
| enum: ["pending_minBalance", "pending_lookahead", "active_ongoing", "active_exiting", "active_slashed", "exited_unslashed", "exited_slashed", "withdrawal_possible", "withdrawal_done"] | ||
|
Comment on lines
-57
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this would be breaking in this format. The 2 ideas that may be better would be either
Maybe the extra doc with new enums is adequate in this case - thoughts @nflaig ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @rolfyone, agreed. Renaming the existing values outright would break anything matching on the strings (and the ?status= filter); that's the breaking trade-off I called out under Backward compatibility in the description (option A vs B). Going additive is basically option B there: add pending_minBalance and pending_lookahead as the post-Electra values, keep pending_initialized / pending_queued in the enum as deprecated aliases, and document the pre/post-Electra flow in the description. Nothing breaks for current consumers and we get a clean deprecation window, with the old names removable in a future major once everyone has moved over. Does that work for you and @nflaig? If you're both good with it I'll update the PR accordingly.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renaming existing values seems definitely like it requires a v2, adding new value might be debatable, but if we want to allow filtering by these new values then we would also need a v2 this api is heavily used by many consumers outside of clients so it worries me to do any such change without a v2 I also haven't looked into the actual new values in detail, maybe depending on how they work this could be different, a tldr of what this PR does would be helpful and why we need it, the PR description is way to verbose and seems AI generated to me
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
could be warranted, or we add the details to the spec itself, it's not great to rely on an external source (like a hackmd) for more details |
||
| example: "active_ongoing" | ||
|
|
||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.