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

Fix phase 1 withdrawable and refactor phase 0 withdrawable setting #1066

Closed
wants to merge 5 commits into from

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented May 9, 2019

Issue

After #850, process_penalties_and_exits and eligible helpers are removed and then phase 1 "eligible to exit" logic is broken.

Solution

  1. The phase 1 eligible check was added on preparing withdrawable_epoch. This PR extracts initiate_validator_withdrawable from initiate_validator_exit and makes phase 1 eligible check inside initiate_validator_withdrawable. I think it may help Make phase 1 spec executable #1061 be easier?
  2. One refactoring benefit is that before this PR, slashed_validator.withdrawable_epoch would be set twice when executing slash_validator function (first time: in initiate_validator_exit; second time: overwrite in slash_validator). Now it's only set once.
  3. Question: the phase 1 eligible check was on withdrawal queue, not exit queue; but should it be changed to check in initiate_validator_exit due to Withdrawal queue -> exit queue #850 logic change?

else:
return current_epoch >= validator.exit_epoch + MIN_VALIDATOR_WITHDRAWAL_EPOCHS
validator.withdrawable_epoch = epoch
Copy link
Contributor

Choose a reason for hiding this comment

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

If a validator has not responded to all challenges and revealed all keys at the point where they are slashed or request an exit, there is no means by which their withdrawable_epoch can be updated. This means that an honest validator cannot withdraw as they cannot have revealed all of their custody keys all the way up to their exit epoch, but this is what is required to set their withdrawable_epoch.

@djrtwo
Copy link
Contributor

djrtwo commented May 23, 2019

hi @hwwhww. Sorry this has sat so long. Can you handle the merge conflict as I review?

@CarlBeek
Copy link
Contributor

Sorry this has sat so long.

Part of the reason this has sat so long is my not having resolved what I mentioned in my review comment. Basically I don't think this solves withdrawability for a few reasons:

  • If an honest validator exits, because they have cannot have revealed their custody keys up to and including their exit epoch, they are stopped from becoming withdrawable.
  • If anyone has a challenge open against them at the time of requesting an exit, they will not be able to withdraw. Note: this can happen even at the last instant as there could be a challenge included just before the exit is requested.

The problem is that initiate_validator_withdrawable is only called directly after requesting an exit. (Although this is missing in this PR too, as far as I can see.)

There are 2 ways I can see for solving this, either we introduce a InitiateWithdrawable object that can be sent by an exited validator to become withdrawable or we automate the process and after every challenge response and key-reveal, we check if the validator is exited and try to initiate withdrawable for them.

In addition, I think this PR is missing the logic around how long a validator remains exited-but-not-withdrawable as a function of open challenges, resolved challenges and key reveals.

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