lighthouse
lighthouse copied to clipboard
Validator manager commands for standard key-manager APIs
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.
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 withenabled
/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 withlighthouse 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.
Thanks for the contribution @pahor167 , could you address the issues @chong-he found?
Thanks for the contribution @pahor167 , could you address the issues @chong-he found?
Thanks for review, I'm looking into it
I've merged CK's PR into this one so we can review here.
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!