Validator manager commands for the Keymanager APIs (#6261)

* Validator manager commands for standard key-manager APIs

* Merge latest unstable

* Fix Some in lib.rs

* Replace Arg::with_name with Arg::new

* Update takes_value

* Remove clap::App

* Change App to Command

* Add command in use

* Remove generic in ArgMatches

* Fix matches.get_flag

* Fixes

* fix error handling

* SetTrue in import

* Fix

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

* Minor fix

* Fix prefer_builder_proposals

* Remove unwrap

* Error handling from Michael

* Add cli help text

* Use None in import to simplify

* Delete unwrap

* Revert flags option

* Simplify help command code

* Remove flag header in move

* Merge remote-tracking branch 'origin/unstable' into pahor/validator-manager-standard-keystore

* Add log in VC when keystore is deleted

* Delete duplicated log when validator does not exist

* Simplify log code

* Rename remove to delete

* cargo-fmt

* Try to remove a function

* make-cli

* Error handling

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

* Update CLI hel text

* make-cli

* Fix checks

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

* Try to fix check errors

* Fix test

* Remove changes

* Update flag name

* CLI display order

* Move builde_proposals flag

* Add doc

* mdlint

* Update validator_manager/src/list_validators.rs

Co-authored-by: Mac L <mjladson@pm.me>

* Delete empty line

* Fix list

* Simplify delete

* Add support to delete more validators

* Fix test

* Rename response

* Add (s)

* Add test to delete multiple validators

* Book and cli

* Make cli

* Only log when keystore is deleted

* Revise deletion log

* Add validator pubkey to error message

* Merge import

* Thank you Mac

* Test

* Add flags

* Error handling for password

* make cli

* Merge remote-tracking branch 'origin/unstable' into vm

* make cli

* Fix test

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

* Fix test

* vm test

* Debug trait thank you Michael

* Fix test

* Merge branch 'unstable' into vm

* test

* testing

* Combine import validator(s)

* make cli

* Add requires

* Update book

* mdlint

* Only show import log when import is successful

* delete testing

* Test for standard format

* Test standard format

* Test

* fix builder_proposals flag

* Fix test for standard format

* Add requires

* Fix vm test

* make cli

* Remove flag header

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

* make cli

* Delete space

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

* Merge branch 'unstable' into vm

* Rename delete_validator to delete_validators

* Rearrange

* Remove pub in run function

* Fix grammar

* Apply suggestions from code review

Co-authored-by: Michael Sproul <micsproul@gmail.com>

* Remove description

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

* Close bracket

* make cli

* Revise list code and test

* Revise import flag

* make cli

* Comment out test

* Update vm test

* Simplify

* Merge remote-tracking branch 'origin/unstable' into vm

* make cli

* Add test

* Add password as a requirement for keystore file

* Correct flags in docs

* typo

validator_manager/src/import_validators.rs

@@ -1,16 +1,28 @@
 use super::common::*;
 use crate::DumpConfig;
+use account_utils::{eth2_keystore::Keystore, ZeroizeString};
 use clap::{Arg, ArgAction, ArgMatches, Command};
 use clap_utils::FLAG_HEADER;
+use derivative::Derivative;
+use eth2::lighthouse_vc::types::KeystoreJsonStr;
 use eth2::{lighthouse_vc::std_types::ImportKeystoreStatus, SensitiveUrl};
 use serde::{Deserialize, Serialize};
 use std::fs;
 use std::path::PathBuf;
+use types::Address;
 
 pub const CMD: &str = "import";
 pub const VALIDATORS_FILE_FLAG: &str = "validators-file";
+pub const KEYSTORE_FILE_FLAG: &str = "keystore-file";
 pub const VC_URL_FLAG: &str = "vc-url";
 pub const VC_TOKEN_FLAG: &str = "vc-token";
+pub const PASSWORD: &str = "password";
+pub const FEE_RECIPIENT: &str = "suggested-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";
 
 pub const DETECTED_DUPLICATE_MESSAGE: &str = "Duplicate validator detected!";
 
@@ -21,15 +33,6 @@ pub fn cli_app() -> Command {
                 are defined in a JSON file which can be generated using the \"create-validators\" \
                 command.",
         )
-        .arg(
-            Arg::new("help")
-                .long("help")
-                .short('h')
-                .help("Prints help information")
-                .action(ArgAction::HelpLong)
-                .display_order(0)
-                .help_heading(FLAG_HEADER),
-        )
         .arg(
             Arg::new(VALIDATORS_FILE_FLAG)
                 .long(VALIDATORS_FILE_FLAG)
@@ -39,19 +42,32 @@ pub fn cli_app() -> Command {
                     imported to the validator client. This file is usually named \
                     \"validators.json\".",
                 )
-                .required(true)
                 .action(ArgAction::Set)
-                .display_order(0),
+                .display_order(0)
+                .required_unless_present("keystore-file")
+                .conflicts_with("keystore-file"),
+        )
+        .arg(
+            Arg::new(KEYSTORE_FILE_FLAG)
+                .long(KEYSTORE_FILE_FLAG)
+                .value_name("PATH_TO_KEYSTORE_FILE")
+                .help(
+                    "The path to a keystore JSON file to be \
+                    imported to the validator client. This file is usually created \
+                    using staking-deposit-cli or ethstaker-deposit-cli",
+                )
+                .action(ArgAction::Set)
+                .display_order(0)
+                .conflicts_with("validators-file")
+                .required_unless_present("validators-file")
+                .requires(PASSWORD),
         )
         .arg(
             Arg::new(VC_URL_FLAG)
                 .long(VC_URL_FLAG)
                 .value_name("HTTP_ADDRESS")
                 .help(
-                    "A HTTP(S) address of a validator client using the keymanager-API. \
-                    If this value is not supplied then a 'dry run' will be conducted where \
-                    no changes are made to the validator client.",
-                )
+                    "A HTTP(S) address of a validator client using the keymanager-API.")
                 .default_value("http://localhost:5062")
                 .requires(VC_TOKEN_FLAG)
                 .action(ArgAction::Set)
@@ -80,29 +96,111 @@ pub fn cli_app() -> Command {
                 )
                 .display_order(0),
         )
+        .arg(
+            Arg::new(PASSWORD)
+                .long(PASSWORD)
+                .value_name("STRING")
+                .help("Password of the keystore file.")
+                .action(ArgAction::Set)
+                .display_order(0)
+                .requires(KEYSTORE_FILE_FLAG),
+        )
+        .arg(
+            Arg::new(FEE_RECIPIENT)
+                .long(FEE_RECIPIENT)
+                .value_name("ETH1_ADDRESS")
+                .help("When provided, the imported validator will use the suggested fee recipient. Omit this flag to use the default value from the VC.")
+                .action(ArgAction::Set)
+                .display_order(0)
+                .requires(KEYSTORE_FILE_FLAG),
+        )
+        .arg(
+            Arg::new(GAS_LIMIT)
+                .long(GAS_LIMIT)
+                .value_name("UINT64")
+                .help("When provided, the imported validator will use this gas limit. It is recommended \
+                to leave this as the default value by not specifying this flag.",)
+                .action(ArgAction::Set)
+                .display_order(0)
+                .requires(KEYSTORE_FILE_FLAG),
+        )
+        .arg(
+            Arg::new(BUILDER_PROPOSALS)
+                .long(BUILDER_PROPOSALS)
+                .help("When provided, the imported validator will attempt to create \
+                blocks via builder rather than the local EL.",)
+                .value_parser(["true","false"])
+                .action(ArgAction::Set)
+                .display_order(0)
+                .requires(KEYSTORE_FILE_FLAG),
+        )
+        .arg(
+            Arg::new(BUILDER_BOOST_FACTOR)
+                .long(BUILDER_BOOST_FACTOR)
+                .value_name("UINT64")
+                .help("When provided, the imported validator will use this \
+                percentage multiplier to apply to the builder's payload value \
+                when choosing between a builder payload header and payload from \
+                the local execution node.",)
+                .action(ArgAction::Set)
+                .display_order(0)
+                .requires(KEYSTORE_FILE_FLAG),
+        )
+        .arg(
+            Arg::new(PREFER_BUILDER_PROPOSALS)
+                .long(PREFER_BUILDER_PROPOSALS)
+                .help("When provided, the imported validator will always prefer blocks \
+                constructed by builders, regardless of payload value.",)
+                .value_parser(["true","false"])
+                .action(ArgAction::Set)
+                .display_order(0)
+                .requires(KEYSTORE_FILE_FLAG),
+        )
 }
 
-#[derive(Clone, PartialEq, Debug, Serialize, Deserialize)]
+#[derive(Clone, PartialEq, Serialize, Deserialize, Derivative)]
+#[derivative(Debug)]
 pub struct ImportConfig {
-    pub validators_file_path: PathBuf,
+    pub validators_file_path: Option<PathBuf>,
+    pub keystore_file_path: Option<PathBuf>,
     pub vc_url: SensitiveUrl,
     pub vc_token_path: PathBuf,
     pub ignore_duplicates: bool,
+    #[derivative(Debug = "ignore")]
+    pub password: Option<ZeroizeString>,
+    pub fee_recipient: Option<Address>,
+    pub gas_limit: Option<u64>,
+    pub builder_proposals: Option<bool>,
+    pub builder_boost_factor: Option<u64>,
+    pub prefer_builder_proposals: Option<bool>,
+    pub enabled: Option<bool>,
 }
 
 impl ImportConfig {
     fn from_cli(matches: &ArgMatches) -> Result<Self, String> {
         Ok(Self {
-            validators_file_path: clap_utils::parse_required(matches, VALIDATORS_FILE_FLAG)?,
+            validators_file_path: clap_utils::parse_optional(matches, VALIDATORS_FILE_FLAG)?,
+            keystore_file_path: clap_utils::parse_optional(matches, KEYSTORE_FILE_FLAG)?,
             vc_url: clap_utils::parse_required(matches, VC_URL_FLAG)?,
             vc_token_path: clap_utils::parse_required(matches, VC_TOKEN_FLAG)?,
             ignore_duplicates: matches.get_flag(IGNORE_DUPLICATES_FLAG),
+            password: clap_utils::parse_optional(matches, PASSWORD)?,
+            fee_recipient: clap_utils::parse_optional(matches, FEE_RECIPIENT)?,
+            gas_limit: clap_utils::parse_optional(matches, GAS_LIMIT)?,
+            builder_proposals: clap_utils::parse_optional(matches, BUILDER_PROPOSALS)?,
+            builder_boost_factor: clap_utils::parse_optional(matches, BUILDER_BOOST_FACTOR)?,
+            prefer_builder_proposals: clap_utils::parse_optional(
+                matches,
+                PREFER_BUILDER_PROPOSALS,
+            )?,
+            enabled: clap_utils::parse_optional(matches, ENABLED)?,
         })
     }
 }
 
 pub async fn cli_run(matches: &ArgMatches, dump_config: DumpConfig) -> Result<(), String> {
     let config = ImportConfig::from_cli(matches)?;
+
     if dump_config.should_exit_early(&config)? {
         Ok(())
     } else {
@@ -113,27 +211,61 @@ pub async fn cli_run(matches: &ArgMatches, dump_config: DumpConfig) -> Result<()
 async fn run<'a>(config: ImportConfig) -> Result<(), String> {
     let ImportConfig {
         validators_file_path,
+        keystore_file_path,
         vc_url,
         vc_token_path,
         ignore_duplicates,
+        password,
+        fee_recipient,
+        gas_limit,
+        builder_proposals,
+        builder_boost_factor,
+        prefer_builder_proposals,
+        enabled,
     } = config;
 
-    if !validators_file_path.exists() {
-        return Err(format!("Unable to find file at {:?}", validators_file_path));
-    }
+    let validators: Vec<ValidatorSpecification> =
+        if let Some(validators_format_path) = &validators_file_path {
+            if !validators_format_path.exists() {
+                return Err(format!(
+                    "Unable to find file at {:?}",
+                    validators_format_path
+                ));
+            }
 
-    let validators_file = fs::OpenOptions::new()
-        .read(true)
-        .create(false)
-        .open(&validators_file_path)
-        .map_err(|e| format!("Unable to open {:?}: {:?}", validators_file_path, e))?;
-    let validators: Vec<ValidatorSpecification> = serde_json::from_reader(&validators_file)
-        .map_err(|e| {
-            format!(
-                "Unable to parse JSON in {:?}: {:?}",
-                validators_file_path, e
-            )
-        })?;
+            let validators_file = fs::OpenOptions::new()
+                .read(true)
+                .create(false)
+                .open(validators_format_path)
+                .map_err(|e| format!("Unable to open {:?}: {:?}", validators_format_path, e))?;
+
+            serde_json::from_reader(&validators_file).map_err(|e| {
+                format!(
+                    "Unable to parse JSON in {:?}: {:?}",
+                    validators_format_path, e
+                )
+            })?
+        } else if let Some(keystore_format_path) = &keystore_file_path {
+            vec![ValidatorSpecification {
+                voting_keystore: KeystoreJsonStr(
+                    Keystore::from_json_file(keystore_format_path).map_err(|e| format!("{e:?}"))?,
+                ),
+                voting_keystore_password: password.ok_or_else(|| {
+                    "The --password flag is required to supply the keystore password".to_string()
+                })?,
+                slashing_protection: None,
+                fee_recipient,
+                gas_limit,
+                builder_proposals,
+                builder_boost_factor,
+                prefer_builder_proposals,
+                enabled,
+            }]
+        } else {
+            return Err(format!(
+                "One of the flag --{VALIDATORS_FILE_FLAG} or --{KEYSTORE_FILE_FLAG} is required."
+            ));
+        };
 
     let count = validators.len();
 
@@ -250,7 +382,10 @@ async fn run<'a>(config: ImportConfig) -> Result<(), String> {
 pub mod tests {
     use super::*;
     use crate::create_validators::tests::TestBuilder as CreateTestBuilder;
-    use std::fs;
+    use std::{
+        fs::{self, File},
+        str::FromStr,
+    };
     use tempfile::{tempdir, TempDir};
     use validator_client::http_api::{test_utils::ApiTester, Config as HttpConfig};
 
@@ -279,10 +414,18 @@ pub mod tests {
             Self {
                 import_config: ImportConfig {
                     // This field will be overwritten later on.
-                    validators_file_path: dir.path().into(),
+                    validators_file_path: Some(dir.path().into()),
+                    keystore_file_path: Some(dir.path().into()),
                     vc_url: vc.url.clone(),
                     vc_token_path,
                     ignore_duplicates: false,
+                    password: Some(ZeroizeString::from_str("password").unwrap()),
+                    fee_recipient: None,
+                    builder_boost_factor: None,
+                    gas_limit: None,
+                    builder_proposals: None,
+                    enabled: None,
+                    prefer_builder_proposals: None,
                 },
                 vc,
                 create_dir: None,
@@ -295,6 +438,10 @@ pub mod tests {
             self
         }
 
+        pub fn get_import_config(&self) -> ImportConfig {
+            self.import_config.clone()
+        }
+
         pub async fn create_validators(mut self, count: u32, first_index: u32) -> Self {
             let create_result = CreateTestBuilder::default()
                 .mutate_config(|config| {
@@ -307,7 +454,55 @@ pub mod tests {
                 create_result.result.is_ok(),
                 "precondition: validators are created"
             );
-            self.import_config.validators_file_path = create_result.validators_file_path();
+            self.import_config.validators_file_path = Some(create_result.validators_file_path());
+            self.create_dir = Some(create_result.output_dir);
+            self
+        }
+
+        // Keystore JSON requires a different format when creating valdiators
+        pub async fn create_validators_keystore_format(
+            mut self,
+            count: u32,
+            first_index: u32,
+        ) -> Self {
+            let create_result = CreateTestBuilder::default()
+                .mutate_config(|config| {
+                    config.count = count;
+                    config.first_index = first_index;
+                })
+                .run_test()
+                .await;
+            assert!(
+                create_result.result.is_ok(),
+                "precondition: validators are created"
+            );
+
+            let validators_file_path = create_result.validators_file_path();
+
+            let validators_file = fs::OpenOptions::new()
+                .read(true)
+                .create(false)
+                .open(&validators_file_path)
+                .map_err(|e| format!("Unable to open {:?}: {:?}", validators_file_path, e))
+                .unwrap();
+
+            let validators: Vec<ValidatorSpecification> = serde_json::from_reader(&validators_file)
+                .map_err(|e| {
+                    format!(
+                        "Unable to parse JSON in {:?}: {:?}",
+                        validators_file_path, e
+                    )
+                })
+                .unwrap();
+
+            let validator = &validators[0];
+            let validator_json = validator.voting_keystore.0.clone();
+
+            let keystore_file = File::create(&validators_file_path).unwrap();
+            let _ = validator_json.to_json_writer(keystore_file);
+
+            self.import_config.keystore_file_path = Some(create_result.validators_file_path());
+            self.import_config.password = Some(validator.voting_keystore_password.clone());
             self.create_dir = Some(create_result.output_dir);
             self
         }
@@ -327,7 +522,8 @@ pub mod tests {
 
                 let local_validators: Vec<ValidatorSpecification> = {
                     let contents =
-                        fs::read_to_string(&self.import_config.validators_file_path).unwrap();
+                        fs::read_to_string(&self.import_config.validators_file_path.unwrap())
+                            .unwrap();
                     serde_json::from_str(&contents).unwrap()
                 };
                 let list_keystores_response = self.vc.client.get_keystores().await.unwrap().data;
@@ -355,6 +551,39 @@ pub mod tests {
                 vc: self.vc,
             }
         }
+
+        pub async fn run_test_keystore_format(self) -> TestResult {
+            let result = run(self.import_config.clone()).await;
+
+            if result.is_ok() {
+                self.vc.ensure_key_cache_consistency().await;
+
+                let local_keystore: Keystore =
+                    Keystore::from_json_file(&self.import_config.keystore_file_path.unwrap())
+                        .unwrap();
+
+                let list_keystores_response = self.vc.client.get_keystores().await.unwrap().data;
+
+                assert_eq!(
+                    1,
+                    list_keystores_response.len(),
+                    "vc should have exactly the number of validators imported"
+                );
+
+                let local_pubkey = local_keystore.public_key().unwrap().into();
+                let remote_validator = list_keystores_response
+                    .iter()
+                    .find(|validator| validator.validating_pubkey == local_pubkey)
+                    .expect("validator must exist on VC");
+                assert_eq!(&remote_validator.derivation_path, &local_keystore.path());
+                assert_eq!(remote_validator.readonly, Some(false));
+            }
+
+            TestResult {
+                result,
+                vc: self.vc,
+            }
+        }
     }
 
     #[must_use] // Use the `assert_ok` or `assert_err` fns to "use" this value.
@@ -445,4 +674,66 @@ pub mod tests {
             .await
             .assert_ok();
     }
+
+    #[tokio::test]
+    async fn create_one_validator_keystore_format() {
+        TestBuilder::new()
+            .await
+            .mutate_import_config(|config| {
+                // Set validators_file_path to None so that keystore_file_path is used for tests with the keystore format
+                config.validators_file_path = None;
+            })
+            .create_validators_keystore_format(1, 0)
+            .await
+            .run_test_keystore_format()
+            .await
+            .assert_ok();
+    }
+
+    #[tokio::test]
+    async fn create_one_validator_with_offset_keystore_format() {
+        TestBuilder::new()
+            .await
+            .mutate_import_config(|config| {
+                config.validators_file_path = None;
+            })
+            .create_validators_keystore_format(1, 42)
+            .await
+            .run_test_keystore_format()
+            .await
+            .assert_ok();
+    }
+
+    #[tokio::test]
+    async fn import_duplicates_when_disallowed_keystore_format() {
+        TestBuilder::new()
+            .await
+            .mutate_import_config(|config| {
+                config.validators_file_path = None;
+            })
+            .create_validators_keystore_format(1, 0)
+            .await
+            .import_validators_without_checks()
+            .await
+            .run_test_keystore_format()
+            .await
+            .assert_err_contains("DuplicateValidator");
+    }
+
+    #[tokio::test]
+    async fn import_duplicates_when_allowed_keystore_format() {
+        TestBuilder::new()
+            .await
+            .mutate_import_config(|config| {
+                config.ignore_duplicates = true;
+                config.validators_file_path = None;
+            })
+            .create_validators_keystore_format(1, 0)
+            .await
+            .import_validators_without_checks()
+            .await
+            .run_test_keystore_format()
+            .await
+            .assert_ok();
+    }
 }