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

[Merged by Bors] - Add cli option for voluntary exits #1781

Closed
wants to merge 19 commits into from

Conversation

pawanjay176
Copy link
Member

@pawanjay176 pawanjay176 commented Oct 16, 2020

Issue Addressed

Resolve #1652

Proposed Changes

Adds a cli option for voluntary exits. The flow is similar to prysm's where after entering the password for the validator keystore (or load password from secrets if present) the user is given multiple warnings about the operation being irreversible, then redirected to the docs webpage(not added yet) which explains what a voluntary exit is and the consequences of exiting and then prompted to enter a phrase from the docs webpage as a final confirmation.

Example usage

$ lighthouse --testnet zinken account validator exit --validator <validator-pubkey> --beacon-node http://localhost:5052

Running account manager for zinken testnet                                                                                                          
validator-dir path: "..."

Enter the keystore password:  for validator in ...

Password is correct

Publishing a voluntary exit for validator: ...              
WARNING: This is an irreversible operation                                                                                                                    
WARNING: Withdrawing staked eth will not be possible until Eth1/Eth2 merge Please visit [website] to make sure you understand the implications of a voluntary exit.            
                                                                                                                                             
Enter the phrase from the above URL to confirm the voluntary exit:
Exit my validator
Published voluntary exit for validator ...

Additional info

Not sure if we should have batch exits (--validator all) option for exiting all the validators in the validators directory. I'm slightly leaning towards having only single exits but don't have a strong preference.

@pawanjay176 pawanjay176 marked this pull request as ready for review October 16, 2020 15:43
@pawanjay176 pawanjay176 added the ready-for-review The code is ready for review label Oct 20, 2020
@pawanjay176
Copy link
Member Author

Tested this out on a few of my medalla validators and it works as expected.

Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

I like the general structure, looking good. I have mostly typo fixes and one major-ish suggestion: to use EIP-2335 keystores directly instead of the "validators dir".

@paulhauner paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Oct 22, 2020
@pawanjay176
Copy link
Member Author

@paulhauner Addressed your comments and added a password-file option.
Also added a check for the remote beacon client to be synced.

I'm not sure how to do end to end testing for this. I think it would require us to have some mock beacon client implementation. Is there any other simpler way ?

@pawanjay176 pawanjay176 added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 27, 2020
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Looking great, super tidy! I only have a few more one-line changes I found after running it again :)

@paulhauner paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Oct 29, 2020
@pawanjay176 pawanjay176 force-pushed the voluntary-exits branch 2 times, most recently from dde00ec to 7b61d4e Compare October 29, 2020 14:28
@pawanjay176 pawanjay176 added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 29, 2020
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Great!

bors r+

@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 29, 2020
bors bot pushed a commit that referenced this pull request Oct 29, 2020
## Issue Addressed

Resolve #1652 

## Proposed Changes

Adds a cli option for voluntary exits. The flow is similar to prysm's where after entering the password for the validator keystore (or load password from `secrets` if present) the user is given multiple warnings about the operation being irreversible, then redirected to the docs webpage(not added yet) which explains what a voluntary exit is and the consequences of exiting and then prompted to enter a phrase from the docs webpage as a final confirmation. 

Example usage
```
$ lighthouse --testnet zinken account validator exit --validator <validator-pubkey> --beacon-node http://localhost:5052

Running account manager for zinken testnet                                                                                                          
validator-dir path: "..."

Enter the keystore password:  for validator in ...

Password is correct

Publishing a voluntary exit for validator: ...              
WARNING: This is an irreversible operation                                                                                                                    
WARNING: Withdrawing staked eth will not be possible until Eth1/Eth2 merge Please visit [website] to make sure you understand the implications of a voluntary exit.            
                                                                                                                                             
Enter the phrase from the above URL to confirm the voluntary exit:
Exit my validator
Published voluntary exit for validator ...
```

## Additional info

Not sure if we should have batch exits (`--validator all`) option for exiting all the validators in the `validators` directory. I'm slightly leaning towards having only single exits but don't have a strong preference.
@bors bors bot changed the title Add cli option for voluntary exits [Merged by Bors] - Add cli option for voluntary exits Oct 30, 2020
@bors bors bot closed this Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a CLI command for exiting validators
2 participants