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

Bring forward changes to withdrawability from phase 1 #615

Merged
merged 6 commits into from
Feb 14, 2019

Conversation

vbuterin
Copy link
Contributor

  • The WITHDRAWABLE flag is removed; instead, a validator's withdrawability is determined through the withdrawable_epoch field (renamed and re-purposed from withdrawal_epoch which was not used)
  • When a validator passes through the withdrawal queue, the prepare_validator_for_withdrawal function does not let them withdraw immediately; instead, they have to wait MIN_VALIDATOR_WITHDRAWAL_EPOCHS. This extra minimum delay serves no value in phase 0, but is crucial for phase 1 as the period between a validator passing through the queue and the validator being eligible to withdraw is where proof of custody challenges can come in; adding it in phase 0 is only half a line of code so easier to add it now.
  • If a validator is penalized, they are no longer subject to the exit queue; instead, their withdrawable_epoch is set LATEST_PENALIZED_EXIT_LENGTH into the future and this is used to determine when the validator can withdraw
  • Changes the eligibility condition for a transfer to use the withdrawable_epoch

* The `WITHDRAWABLE` flag is removed; instead, a validator's withdrawability is determined through the `withdrawable_epoch` field (renamed and re-purposed from `withdrawal_epoch` which was not used)
* When a validator passes through the withdrawal queue, the `prepare_validator_for_withdrawal` function does not let them withdraw immediately; instead, they have to wait `MIN_VALIDATOR_WITHDRAWAL_EPOCHS`. This extra minimum delay serves no value in phase 0, but is crucial for phase 1 as the period between a validator passing through the queue and the validator being eligible to withdraw is where proof of custody challenges can come in; adding it in phase 0 is only half a line of code so easier to add it now.
* If a validator is penalized, they are no longer subject to the exit queue; instead, their `withdrawable_epoch` is set `LATEST_PENALIZED_EXIT_LENGTH` into the future and this is used to determine when the validator can withdraw
* Changes the eligibility condition for a transfer to use the `withdrawable_epoch`
specs/core/0_beacon-chain.md Show resolved Hide resolved
@@ -1377,18 +1378,20 @@ def penalize_validator(state: BeaconState, index: ValidatorIndex) -> None:
state.validator_balances[whistleblower_index] += whistleblower_reward
state.validator_balances[index] -= whistleblower_reward
validator.penalized_epoch = get_current_epoch(state)
validator.withdrawable_epoch = get_current_epoch(state) + LATEST_PENALIZED_EXIT_LENGTH
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a condition that validators can only be penalized if they have not already entered the withdrawable state? I'm imagining someone exits, enters withdrawable, transfers some amount of funds, and then is found to be slashable. Although it would be nice to penalize them a bit, I think we should not allow this and other unexpected flows because it opens up unexpected state transitions (hard to test, potential attack vectors, etc)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a condition that validators can only be penalized if they have not already entered the withdrawable state?

Automatic withdrawals in phase 2 effectively provide that. I'd support adding extra conditions (to be removed in phase 2) for consistency with phase 2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added assert validator.withdrawable_epoch == FAR_FUTURE_EPOCH # [TO BE REMOVED IN PHASE 2] in penalize_validator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs to be possible to penalize validators that are in the "waiting for PoC challenges" stage. So I'd recommend the simple assert validator.withdrawable_epoch >= state.slot or assert validator.withdrawable_epoch > state.slot + ENTRY_EXIT_DELAY.

@JustinDrake JustinDrake merged commit eadfa20 into dev Feb 14, 2019
vbuterin added a commit that referenced this pull request Feb 14, 2019
@djrtwo djrtwo deleted the vbuterin-patch-5 branch February 14, 2019 18:30
vbuterin added a commit that referenced this pull request Feb 19, 2019
* Updated phase 1: branch challenges

* Removed unnecessary line

* Added early subkey reveal slashing

* Revealing during the active period is still revealing early

* Added....

* Machinery for publishing old subkeys
* Inability to withdraw until you published all subkeys
* After a validator exits the queue there's still a minimum 1-day delay before they can withdraw (in the future this delay will be used as an opportunity to start a PoC challenge game)

* Update 1_shard-data-chains.md

* formatting

* minor edits

* Added masking scheme for reveals

Secure under the aggregate extraction infeasibility assumption described on pages 11-12 of https://crypto.stanford.edu/~dabo/pubs/papers/aggreg.pdf

* Added rewards going to challengers

* Add ToC and reorg the constant tables

* Remove tags

* fix constant formatting

* normalize domain constants in phase 1

* Update 1_shard-data-chains.md

* Update 1_shard-data-chains.md

* Update 1_shard-data-chains.md

* Added transition logic

* Fix ToC

* Fix ToC

* Adjusted for #615

* Added more helpers

* epoch -> slot

* fix some type hints

* clean up `get_attestation_merkle_depth`
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

3 participants