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

Add Ability to Specify All Public Keys When Exiting Validators #8399

Merged
merged 16 commits into from
Feb 11, 2021

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Feb 4, 2021

What does this PR do? Why is it needed?

Currently, validators cannot exit all accounts if desired. This PR adds a flag --exit-all to the validator client. The staker still needs to confirm a prompt given the dangerous nature of the action.

Which issues(s) does this PR fix?

Fixes #8383

@rauljordan rauljordan requested a review from a team as a code owner February 4, 2021 20:49
@rauljordan rauljordan self-assigned this Feb 4, 2021
terencechain
terencechain previously approved these changes Feb 4, 2021
Copy link
Contributor

@rkapka rkapka left a comment

Choose a reason for hiding this comment

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

Comment from the issue's creator @makultar:

So basically one command to request exit for all validators at the same time. Maximum a yes/no question but perhaps not even that. The ones who looks up and executes this command surely knows he is about to exit all validators wether its 1 x 32ETH or 500 x 32ETH instantly.

I think what this means is an option to exit all validators without even having to select anything. Just type the command, maybe display a yes/no confirmation and exit everything. Do we want to support that? It's risky.

rkapka
rkapka previously requested changes Feb 4, 2021
Copy link
Contributor

@rkapka rkapka left a comment

Choose a reason for hiding this comment

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

Blocking the PR for now as I don't think this fixes the issue, as this is not what OP wants.

@rkapka rkapka dismissed their stale review February 4, 2021 22:25

Dismissing

@makultar
Copy link

makultar commented Feb 5, 2021

Comment from the issue's creator @makultar:

So basically one command to request exit for all validators at the same time. Maximum a yes/no question but perhaps not even that. The ones who looks up and executes this command surely knows he is about to exit all validators wether its 1 x 32ETH or 500 x 32ETH instantly.

I think what this means is an option to exit all validators without even having to select anything. Just type the command, maybe display a yes/no confirmation and exit everything. Do we want to support that? It's risky.

Blocking the PR for now as I don't think this fixes the issue, as this is not what OP wants.

Holddddddd your horses!

I think something like --skip-voluntary-exit-confirmation is exactly was I am looking for. Or at least on the right path.

Hmm I dont agree with "Its risky". Staking itself is already risky, i.e depositing 50 000 USD+, making sure the system works flawlessly, that you dont get hacked, making sure your mnemonic is with you x years while withdrawal is disabled etc etc. There is just tons of various risks. Making an exit to keep your funds safe surely is not. Very much the opposite. Then lets make sure its well described and with several warnings around that description so no one else can be blamed?

No one is dumb enough to execute a command thats wrapped around

*** WARNING * WARNING * WARNING * WARNING * WARNING * WARNING ***
* THE FOLLOWING COMMAND WILL EXIT ALL YOUR VALIDATORS AT ONCE *
*** WARNING * WARNING * WARNING * WARNING * WARNING * WARNING* **

There is no real risk. If you read this WARNING x 12, and still decide to go ahead with a: YES -- thats on you. We are adults, not kids. Who is there to protect? I doubt there is kids or adults with severe brain damage staking 50k that dont know what they are doing :P

Edit: Typos hehe

@rkapka
Copy link
Contributor

rkapka commented Feb 5, 2021

Copying from Discord:

IIRC, right now you have to manually enter all keys that you want to exit. accounts delete has this nice dropdown that you can choose from, with an Select all option. Maybe we can utilize that, combined with the flag that you introduced?

@makultar
Copy link

makultar commented Feb 5, 2021

Copying from Discord:

IIRC, right now you have to manually enter all keys that you want to exit. accounts delete has this nice dropdown that you can choose from, with an Select all option. Maybe we can utilize that, combined with the flag that you introduced?

I would be very happy if the following command was possible
/usr/local/bin/validator accounts voluntary-exit --wallet-dir=/var/lib/prysm/validator --select-all --skip-voluntary-exit-confirmation

Letting you only confirm with a yes/no:
*** WARNING * WARNING * WARNING * WARNING * WARNING * WARNING ***
* THE FOLLOWING COMMAND WILL EXIT ALL YOUR VALIDATORS AT ONCE *
* ARE YOU SURE? Yes/No *
*** WARNING * WARNING * WARNING * WARNING * WARNING * WARNING* **

=> Exits

@makultar
Copy link

makultar commented Feb 5, 2021

For someone thats not a techie, like my family, wife, or future kids. I dont want this to be a complicated task or menu. My instructions to them will be very simply.

  1. Login into server 127.0..1
  2. Copypaste and press enter:
    /usr/local/bin/validator accounts voluntary-exit --wallet-dir=/var/lib/prysm/validator --select-all --skip-voluntary-exit-confirmation
  3. Type "Yes" when asked to confirm => Enter

Not a long restaurant menu going through all kinds of options, opening up for real real problems.

@rauljordan
Copy link
Contributor Author

Thanks @makultar totally understand your rationale now. All you want is a simple command that allows you specify you want to exit all public keys via a flag, then a simple confirmation that looks scary, and then you're done. We agree this is an important feature to safeguard your eth in case something happens to you and a family member needs to perform the action, thank you for bringing it to our attention, we will amend to do as you requested

@rauljordan rauljordan marked this pull request as draft February 5, 2021 17:08
@rauljordan rauljordan changed the title Add Programmatic Voluntary Exits Add Ability to Specify All Public Keys When Exiting Validators Feb 5, 2021
@rauljordan rauljordan marked this pull request as ready for review February 5, 2021 20:04
@makultar
Copy link

makultar commented Feb 5, 2021

Thanks @makultar totally understand your rationale now. All you want is a simple command that allows you specify you want to exit all public keys via a flag, then a simple confirmation that looks scary, and then you're done. We agree this is an important feature to safeguard your eth in case something happens to you and a family member needs to perform the action, thank you for bringing it to our attention, we will amend to do as you requested

Wow thats great! Thanks a ton for this. Cool and kind of you guys. I will launch a few testnet validators and test this feature for sure.

Copy link
Contributor

@rkapka rkapka left a comment

Choose a reason for hiding this comment

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

Please test the new functionality.

Comment on lines +153 to +159
rawPubKeys = make([][]byte, len(validatingPublicKeys))
formattedPubKeys = make([]string, len(validatingPublicKeys))
for i, pk := range validatingPublicKeys {
rawPubKeys[i] = pk[:]
formattedPubKeys[i] = fmt.Sprintf("%#x", bytesutil.Trunc(pk[:]))
}
fmt.Printf("About to perform a voluntary exit of %d accounts\n", len(rawPubKeys))
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a test.

@rauljordan
Copy link
Contributor Author

Done @rkapka

@rauljordan
Copy link
Contributor Author

Ready again @rkapka

Copy link
Contributor

@rkapka rkapka left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@prylabs-bulldozer prylabs-bulldozer bot merged commit e236ded into develop Feb 11, 2021
@delete-merged-branch delete-merged-branch bot deleted the programmatic-exit branch February 11, 2021 16:50
ahadda5 pushed a commit to ahadda5/prysm that referenced this pull request Feb 14, 2021
…aticlabs#8399)

* add programmatic voluntary exit

* add exit all flag

* test

* lint

* add multiple exits test

* fix test

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
ahadda5 pushed a commit to ahadda5/prysm that referenced this pull request Feb 14, 2021
…aticlabs#8399)

* add programmatic voluntary exit

* add exit all flag

* test

* lint

* add multiple exits test

* fix test

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
prylabs-bulldozer bot added a commit that referenced this pull request Feb 15, 2021
* Added MaskCredentialsLogging to logutil, which masks the user info, path and query. It leaves the hostname and port untouched . Making it more secure during logging

* Added MaskCredentialsLogging to logutil, which masks the user info,path and query. It leaves the hostname and port untouched . Making it more secure during logging

* Added newline based on the PR checks

* Update shared/logutil/logutil.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Update shared/logutil/logutil.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Gazelle

* Update shared/logutil/logutil.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Update shared/logutil/logutil.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* added unit tests

* updated one test case

* added logutil_test.go unit test cases

* Refactor validator subnet subscriptions to be non-blocking (#8319)

* Use response.NextEpochDuties for aggregator subnet subscriptions (credit: @KaanKC PR #8204). Make committee subnet subscriptions method non-blocking call

* Fix test

* Fix test

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>

* Add Ability to Specify All Public Keys When Exiting Validators (#8399)

* add programmatic voluntary exit

* add exit all flag

* test

* lint

* add multiple exits test

* fix test

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* Increase Validation Queue (#8431)

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* Validator: add a DEBUG log to show batch attestation save duration (#8432)

* Add a debug log to show duration

* Autofix issues in 1 file

Resolved issues in validator/db/kv/attester_protection.go via DeepSource Autofix

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>

* Add Mutex and Block Profiling (#8435)

* Implement GetStateRoot in the beacon API (#8402)

* update ethereumapis dependency

* span

* initial implementation

* introduce stategen Service interface and MockService

* Include AddStateForSlot function in the mock service

* return states from mock

* add GenesisState to POWChain mock

* populate roots in helper state

* initialize Slot when creating helper state

* tests

* code refactor - extract helper functions

* gzl

* use SetSlot in tests

* handle SetSlot error

* use new testutil's NewBeaconState

* gzl

* go mod tidy

* rename Service to StateManager

* move regex check to helper

Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* On Block Cleanup (#8438)

* Beacon API: update GetStateRoot  (#8437)

* Address various feedbacks

* Gaz

* More nil check

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* qualifying my unix user ahaddad - no real changes to the files

* Update shared/logutil/logutil.go

* Update shared/logutil/logutil.go

* Update shared/logutil/logutil.go

* Update shared/logutil/logutil.go

* Update shared/logutil/logutil.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Co-authored-by: Nishant Das <nishdas93@gmail.com>
Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
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.

Add Ability to Exit All Validators
4 participants