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

Withdrawal queue -> exit queue #850

Merged
merged 34 commits into from
Apr 14, 2019
Merged

Withdrawal queue -> exit queue #850

merged 34 commits into from
Apr 14, 2019

Conversation

vbuterin
Copy link
Contributor

No description provided.

@JustinDrake
Copy link
Collaborator

Should we be ignoring attestations of slashed-and-queued validators?

@CarlBeek
Copy link
Contributor

Should we be ignoring attestations of slashed-and-queued validators?

I'd argue so. Including them would seemingly only decrease fork choice stability by allowing slashed validators to attest to forks.

@JustinDrake
Copy link
Collaborator

If MAX_EXIT_DEQUEUES_PER_EPOCH = 4 and we have 1,000,000 active validators then it would take 3 years (!!) to dequeue them all. Why not have a churn queue mechanism which is percentage-based (similar to the MAX_BALANCE_CHURN_QUOTIENT logic)?

@JustinDrake
Copy link
Collaborator

JustinDrake commented Mar 29, 2019

Another consideration is that entering the exit queue as a "hedge" (with the intent of rejoining under a different pubkey after exiting) is close to free. Should we increase MIN_VALIDATOR_WITHDRAWABILITY_DELAY or add an exit fee to disincentivise entering the exit queue prematurely?

@vbuterin
Copy link
Contributor Author

If MAX_EXIT_DEQUEUES_PER_EPOCH = 4 and we have 1,000,000 active validators then it would take 3 years (!!) to dequeue them all. Why not have a churn queue mechanism which is percentage-based (similar to the MAX_BALANCE_CHURN_QUOTIENT logic)?

Personally I'd be inclined to favor a hybrid formula, eg. max_dequeues = max(4, len(validator set) / 65536). That would guarantee that all validators can cycle out in ~9.5 months, giving a weak subjectivity period of ~3 months, except in the case where there is less than 8m ETH validating, the validator set can clear faster, providing a mechanism so that if validating turns out to be unexpectedly unpopular the system compromises in the direction of lower weak subjectivity periods to keep the absolute quantity of ETH validating not-too-low.

Should we increase MIN_VALIDATOR_WITHDRAWABILITY_DELAY or add an exit fee to disincentivise entering the exit queue prematurely?

Premature exit fee seems reasonable to me! Though keep in mind that if we want to do it via min delays instead it's not MIN_VALIDATOR_WITHDRAWABILITY_DELAY that we want to tweak, but rather PERSISTENT_COMMITTEE_PERIOD, due to this line:

    assert get_current_epoch(state) - validator.activation_epoch >= PERSISTENT_COMMITTEE_PERIOD

PERSISTENT_COMMITTEE_PERIOD is currently 9 days, but we were considering extending it to 18 days for efficiency reasons. So you can't exactly exit immediately. But an exit fee to make exiting quickly even more undesirable does seem like a good idea.

@vbuterin
Copy link
Contributor Author

Should we be ignoring attestations of slashed-and-queued validators?

The PR is doing that already, see the get_unslashed_attesting_indices definition. Or do you mean ignoring as in not even allowing the bitfields on chain?

@JustinDrake
Copy link
Collaborator

I think we can get rid of validator_registry_update_epoch because it can be derived from the activation_epochs. (And this is a slight optimisation if indices_for_activation == [] since activations can happen in the next epoch regardless of finality.)

@vbuterin
Copy link
Contributor Author

vbuterin commented Apr 3, 2019

I think we can get rid of validator_registry_update_epoch because it can be derived from the activation_epochs.

By using max(activation_epoch) - ACTIVATION_EXIT_DELAY? Seems reasonable to me!

specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
@djrtwo
Copy link
Contributor

djrtwo commented Apr 13, 2019

@vbuterin Can you link to or add rationale for client implementers?

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.

  • fixed broken test
  • added exit queue test to ensure exit queue mechanics after MAX_EXITS_PER_EPOCH exits
  • a couple cleanups

lgtm

@djrtwo djrtwo merged commit 7840d29 into dev Apr 14, 2019
@djrtwo djrtwo deleted the vbuterin-patch-13 branch April 14, 2019 09:21
@djrtwo djrtwo restored the vbuterin-patch-13 branch April 14, 2019 09:22
@djrtwo djrtwo deleted the vbuterin-patch-13 branch April 14, 2019 09:22
@hwwhww
Copy link
Contributor

hwwhww commented Apr 21, 2019

Hey @JustinDrake

Question: what's the rationale of removing validator.activation_epoch == FAR_FUTURE_EPOCH in
47464f2 ?

It was found by @NIC619 that it seems after removing it, the validator.activation_epoch will be updated again and again.

e.g. when ACTIVATION_EXIT_DELAY: 4, finalized epoch 1 when processing epoch 4. pre/post-activation_epoch represents before/after process_registry_updates.

current_epoch finalized_epoch get_delayed_activation_exit_epoch(state.finalized_epoch) pre-activation_epoch post-activation_epoch
0 0 5 FAR_FUTURE_EPOCH 5
1 0 5 5 6
2 0 5 6 7
3 0 5 7 8
4 1 6 8 9

If so, should we either add validator.activation_epoch == FAR_FUTURE_EPOCH back or modify validator.activation_epoch >= get_delayed_activation_exit_epoch(state.finalized_epoch) condition?

JustinDrake added a commit that referenced this pull request Apr 22, 2019
Fix bug [flagged by @NIC619 and @hwwhww](#850 (comment)) whereby the `activation_epoch` of validators dequeued since the finalized epoch was overwritten.

Cosmetic changes:

1) Remove `activate_validator` (there is no overlap between genesis and non-genesis activations)
2) Improve comments related to activation queue
@djrtwo djrtwo mentioned this pull request Dec 12, 2019
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.

None yet

5 participants