lighthouse icon indicating copy to clipboard operation
lighthouse copied to clipboard

Validator manager commands for standard key-manager APIs

Open pahor167 opened this issue 11 months ago • 4 comments

Issue Addressed

https://github.com/sigp/lighthouse/issues/4854

Proposed Changes

Added ability to

  • import validators from standard key-manager
  • list validators in VC
  • remove validator from VC

Additional Info

This is my first PR, please let me know if anything is missing.

pahor167 avatar Mar 04 '24 21:03 pahor167

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Mar 04 '24 21:03 CLAassistant

I have done some testing for the PR. Generally, it is a nice feature to have for Lighthouse Validator Manager family of commands that are compatible with the KeyManager API.

1. For list:

Update: this is fix as per: https://github.com/sigp/lighthouse/pull/5347#pullrequestreview-2222685299

~~When the VC is not running and we run the list command: lighthouse vm list --VC-token ~/.lighthouse/mainnet/validators/api-token.txt~~

~~It will result in errors, which is a bug:~~

Running validator manager for mainnet network
Mar 18 15:44:30.072 CRIT Task panic. This is a bug!              advice: Please check above for a backtrace and notify the developers, backtrace:    0: lighthouse::run::{{closure}}
   1: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/alloc/src/boxed.rs:2029:9
   2: std::panicking::rust_panic_with_hook
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:783:13
...

~~This can be fixed so that it only shows the error at the bottom (just like what import and remove show when the VC is not running):~~

Failed to list keystores on VC: HttpClient(url: http://localhost:5062/, kind: request, detail: error trying to connect: tcp connect error: Connection refused (os error 111))"

2. For import-standard:

The import-standard imports a validator keystore file that is compatible with the keystore files generated by the staking-deposit-cli. Some findings:

i) The import-standard only imports a single keystore at a time. I am not sure if this is a limit when using the API? Just thought it would be nice if it can import multiple keystore files together, or read from a folder like the lighthouse account validator import does.

ii) The flag to read the keystore file is --validator-file, which is not consistent with the current lighthouse vm import --validators-file. It will be good if we can make this flag consistent.

~~iii) When the VC is not running, the error is:~~

~~Failed to list keystores on VC~~

~~This can be revised to:~~

~~Failed to import keystores to VC~~

~~(This is also the case for the current lighthouse vm import command, so it would be great to revise that too)~~

Edit: I think this is fine as it is the same across all vm commands.

iv) It would be nice to rename the import-standard command to a single word, just like other create, move, list commands. But I am not sure which word to use, maybe importCLI?

~~#### 3. For remove:~~

~~i) Should we use the word delete instead of remove throughout so that it is consistent with the wording used in KeyManager API: https://ethereum.github.io/keymanager-APIs/#/Local%20Key%20Manager/deleteKeys~~

Edit: Fixed in eadec07

4. On documentation:

With this PR, to list validators, we will have:

  • lighthouse account validator list which shows the pubkey alongside with enabled/disabled
  • lighthouse vm list which shows the pubkey

This is fine, the main distinction between the two is the legacy lighthouse account validator list command can be done offline, while the lighthouse vm list is online using the API.

To import validators, we will have:

  • the legacy way of using lighthouse account validator import which accepts keystore file generated with staking-deposit-cli and import must be done when VC is offline. It accepts a folder containing keystore files
  • the lighthouse vm import which accepts keystore file generated with lighthouse vm create
  • the lighthouse vm import-standard which is same as the first method but it can be done when VC is online, however it only supports importing a single pubkey at one time
  • the lighthouse/validators/keystore which uses Lighthouse API to import keystore

All these methods can be quite confusing to users. I think we want to mention this properly in the Lighthouse book.

For the new command, it will also be helpful to have a guide in the Lighthouse book. For example, we can include the minimum command required to run these commands:

For list:

lighthouse vm list --vc-token ~/.lighthouse/mainnet/validators/api-token.txt

For import-standard:

lighthouse vm import-standard --vc-token ~/.lighthouse/mainnet/validators/api-token.txt --validator-file /path/to/json --password keystore_password

For remove (delete):

lighthouse vm remove --vc-token ~/.lighthouse/mainnet/validators/api-token.txt --validator pubkey

Thank you.

chong-he avatar Mar 19 '24 13:03 chong-he

Thanks for the contribution @pahor167 , could you address the issues @chong-he found?

dapplion avatar Apr 03 '24 02:04 dapplion

Thanks for the contribution @pahor167 , could you address the issues @chong-he found?

Thanks for review, I'm looking into it

pahor167 avatar Apr 04 '24 08:04 pahor167

I've merged CK's PR into this one so we can review here.

michaelsproul avatar Aug 08 '24 05:08 michaelsproul

Closing in favour of a PR from CK's repo, as updating this PR manually is becoming time-consuming.

Sorry Pahor, you'll get Co-authored-by: on CK's PR when it merges!

michaelsproul avatar Aug 15 '24 05:08 michaelsproul