Validator manager commands for standard key-manager APIs #5347

pahor167 · 2024-03-04T21:30:15Z

Issue Addressed

#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.

CLAassistant · 2024-03-04T21:30:31Z

All committers have signed the CLA.

chong-he · 2024-03-19T13:19:23Z

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: #5347 (review)

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.

dapplion · 2024-04-03T02:09:19Z

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

pahor167 · 2024-04-04T08:54:42Z

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

Thanks for review, I'm looking into it

chong-he

@pahor167

I have created a PR on your repo that includes updating the code and some fixes to the errors found (thanks to @michaelsproul). Feel free to merge the PR so that this PR is closer to completion.

I would also like to suggest to rename remove.rs to delete.rs and some wordings in the source code to be consistent with the Keymanager API.

Thanks!

chong-he · 2024-08-07T05:51:45Z

validator_manager/src/list_validators.rs

+    if dump_config.should_exit_early(&config)? {
+        Ok(())
+    } else {
+        println!("{}", run(config).await.unwrap());


This fixes the panic bug in no. 1 here: #5347 (comment)

Thanks to @michaelsproul, he also mentioned that we should remove all unwrap() in the PR

Suggested change

println!("{}", run(config).await.unwrap());

println!("{}", run(config).await?);

Fixed in b80d16e

chong-he · 2024-08-07T05:52:49Z

validator_manager/src/remove_validator.rs

+
+use crate::{common::vc_http_client, DumpConfig};
+
+pub const CMD: &str = "remove";


From my understanding, this is the wording that's shown on the CLI help text. I think it is better to have it named delete to be consistent with the Keymanager API: https://ethereum.github.io/keymanager-APIs/#/Local%20Key%20Manager

Suggested change

pub const CMD: &str = "remove";

pub const CMD: &str = "delete";

Maybe can revise some of the wordings in this file, and also rename the name of this file to delete_validators.rs

Fixed in eadec07

chong-he · 2024-08-07T05:56:38Z

validator_manager/src/import_validator.rs

+pub const FEE_RECIPIENT: &str = "fee-recipient";
+pub const GAS_LIMIT: &str = "gas-limit";
+pub const BUILDER_PROPOSALS: &str = "builder-proposals";
+pub const BUILDER_BOOST_FACTOR: &str = "builder-boost-factor";
+pub const PREFER_BUILDER_PROPOSALS: &str = "prefer-builder-proposals";
+pub const ENABLED: &str = "enabled";


This provides additional optional flags to be used when importing the validators, e.g., fee-recipient etc, which will be added to the validator_definitions.yml directly. I think this is a nice feature to have.

Just noted here that other import commands do not have this

chong-he · 2024-08-07T06:59:25Z

validator_manager/src/import_validator.rs

+                .takes_value(true),
+        )
+        .arg(
+            Arg::with_name(FEE_RECIPIENT)


If we keep all these optional flags, I would recommend to use the same name, for example, suggested-fee-recipient in the validator client:

lighthouse/validator_client/src/cli.rs

Line 161 in 9e12c21

Arg::new("suggested-fee-recipient")

Also recommend to update the help text, for example, can use the same help text from the validator client. The same goes for other flags like builder-proposals

Suggested change

Arg::with_name(FEE_RECIPIENT)

Arg::with_name(SUGGESTED_FEE_RECIPIENT)

Updated in chong-he@100ee22

chong-he · 2024-08-07T14:23:05Z

crypto/eth2_keystore/src/keystore.rs

+    /// Instantiates `self` from a `JsonKeystore`.
+    pub fn from_json_keystore(json_keystore: JsonKeystore) -> Result<Self, Error> {
+        Ok(Keystore { json: json_keystore })
+    }
+


Probably can delete this since the import_validator.rs now uses from_json_file function

Suggested change

/// Instantiates `self` from a `JsonKeystore`.

pub fn from_json_keystore(json_keystore: JsonKeystore) -> Result<Self, Error> {

Ok(Keystore { json: json_keystore })

}

chong-he · 2024-08-08T00:05:34Z

validator_manager/src/import_validator.rs

+                let validators_file = fs::read_to_string(&self.import_config.validator_file_path)
+                .map_err(|e| format!("Unable to open {:?}: {:?}", &self.import_config.validator_file_path, e)).unwrap();
+
+                let local_keystore: Keystore = Keystore::from_json_keystore(serde_json::from_str(&validators_file).expect("JSON was not well formatted")).unwrap();


Suggested change

let local_keystore: Keystore = Keystore::from_json_keystore(serde_json::from_str(&validators_file).expect("JSON was not well formatted")).unwrap();

let local_keystore: Keystore = Keystore::from_json_file(serde_json::from_str(&validators_file_path).expect("JSON was not well formatted")).unwrap();

Try to remove it in 6fb9f83

michaelsproul · 2024-08-08T05:19:46Z

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

…anager-standard-keystore

michaelsproul · 2024-08-15T05:23:30Z

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!

Validator manager commands for standard key-manager APIs

0fa74cd

This was referenced Mar 4, 2024

Validator manager commands for standard key-manager APIs #4854

Open

Add minimal list validators #5334

Closed

chong-he added val-client Relates to the validator client binary ready-for-review The code is ready for review labels Mar 5, 2024

michaelsproul mentioned this pull request Mar 12, 2024

Validator manager move should respect suggested fee recipient #5402

Open

dapplion 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 Apr 3, 2024

chong-he and others added 18 commits July 29, 2024 20:56

Merge latest unstable

f6d11bc

Fix Some in lib.rs

83967a5

Replace Arg::with_name with Arg::new

bf6e3b7

Update takes_value

1337a2f

Remove clap::App

1082426

Change App to Command

526603c

Add command in use

0075fdd

Remove generic in ArgMatches

6803092

Fix matches.get_flag

1630435

Fixes

1c8b3b3

fix error handling

b80d16e

SetTrue in import

509d590

Fix

bb283a2

Fix builder-proposal flag (will delete the flag later)

e1452f8

Minor fix

e0918a8

Fix prefer_builder_proposals

97afc09

Remove unwrap

2212dd4

Error handling from Michael

47382a7

chong-he added 2 commits August 7, 2024 11:44

Delete unwrap

82d578f

Revert flags option

d71cca9

chong-he reviewed Aug 7, 2024

View reviewed changes

chong-he mentioned this pull request Aug 7, 2024

Update PR #5347 in Sigp/lighthouse pahor167/lighthouse#1

Merged

chong-he added 2 commits August 7, 2024 19:36

Simplify help command code

98ac81b

Remove flag header in move

ad7cb23

chong-he reviewed Aug 8, 2024

View reviewed changes

Merge remote-tracking branch 'origin/unstable' into pahor/validator-m…

2f9f0dd

…anager-standard-keystore

michaelsproul added ready-for-review The code is ready for review v6.0.0 New major release for hierarchical state diffs and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 8, 2024

michaelsproul self-requested a review August 8, 2024 05:26

chong-he and others added 14 commits August 12, 2024 14:06

Add log in VC when keystore is deleted

6069c94

Delete duplicated log when validator does not exist

ff0c841

Simplify log code

0e4e9a4

Rename remove to delete

eadec07

cargo-fmt

97196cc

Try to remove a function

6fb9f83

make-cli

bed168c

Error handling

a55cc5b

Merge branch 'vm' of https://github.com/chong-he/lighthouse into vm

b5b9537

Update CLI hel text

100ee22

make-cli

610f971

Fix checks

64a4f67

Merge branch 'vm' of https://github.com/chong-he/lighthouse into vm

9ba60be

Try to fix check errors

e079eb4

michaelsproul closed this Aug 15, 2024

chong-he mentioned this pull request Aug 15, 2024

Validator manager commands for the Keymanager APIs #6261

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validator manager commands for standard key-manager APIs #5347

Validator manager commands for standard key-manager APIs #5347

pahor167 commented Mar 4, 2024

CLAassistant commented Mar 4, 2024 •

edited

Loading

chong-he commented Mar 19, 2024 •

edited

Loading

dapplion commented Apr 3, 2024

pahor167 commented Apr 4, 2024

chong-he left a comment

chong-he Aug 7, 2024

chong-he Aug 13, 2024

chong-he Aug 7, 2024

chong-he Aug 13, 2024

chong-he Aug 7, 2024

chong-he Aug 7, 2024 •

edited

Loading

chong-he Aug 13, 2024

chong-he Aug 7, 2024

chong-he Aug 8, 2024

chong-he Aug 13, 2024

michaelsproul commented Aug 8, 2024

michaelsproul commented Aug 15, 2024

	println!("{}", run(config).await.unwrap());
	println!("{}", run(config).await?);


		use crate::{common::vc_http_client, DumpConfig};

		pub const CMD: &str = "remove";

	pub const CMD: &str = "remove";
	pub const CMD: &str = "delete";

	Arg::with_name(FEE_RECIPIENT)
	Arg::with_name(SUGGESTED_FEE_RECIPIENT)

	let local_keystore: Keystore = Keystore::from_json_keystore(serde_json::from_str(&validators_file).expect("JSON was not well formatted")).unwrap();
	let local_keystore: Keystore = Keystore::from_json_file(serde_json::from_str(&validators_file_path).expect("JSON was not well formatted")).unwrap();

Validator manager commands for standard key-manager APIs #5347

Validator manager commands for standard key-manager APIs #5347

Conversation

pahor167 commented Mar 4, 2024

Issue Addressed

Proposed Changes

Additional Info

CLAassistant commented Mar 4, 2024 • edited Loading

chong-he commented Mar 19, 2024 • edited Loading

1. For list:

2. For import-standard:

4. On documentation:

dapplion commented Apr 3, 2024

pahor167 commented Apr 4, 2024

chong-he left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chong-he Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsproul commented Aug 8, 2024

michaelsproul commented Aug 15, 2024

CLAassistant commented Mar 4, 2024 •

edited

Loading

chong-he commented Mar 19, 2024 •

edited

Loading

1. For `list`:

2. For `import-standard`:

chong-he Aug 7, 2024 •

edited

Loading