Validator manager import to allow overriding fields with CLI flag (#7684)

* #7651


  


Co-Authored-By: Tan Chee Keong <tanck@sigmaprime.io>

Co-Authored-By: chonghe <44791194+chong-he@users.noreply.github.com>

Co-Authored-By: Lion - dapplion <35266934+dapplion@users.noreply.github.com>

book/src/help_vm_import.md

@@ -24,6 +24,9 @@ Options:
       --debug-level <LEVEL>
           Specifies the verbosity level used when emitting logs to the terminal.
           [default: info] [possible values: info, debug, trace, warn, error]
+      --enabled <enabled>
+          When provided, the imported validator will be enabled or disabled.
+          [possible values: true, false]
       --gas-limit <UINT64>
           When provided, the imported validator will use this gas limit. It is
           recommended to leave this as the default value by not specifying this

lighthouse/tests/validator_manager.rs

@@ -16,6 +16,7 @@ use validator_manager::{
     list_validators::ListConfig,
     move_validators::{MoveConfig, PasswordSource, Validators},
 };
+use zeroize::Zeroizing;
 
 const EXAMPLE_ETH1_ADDRESS: &str = "0x00000000219ab540356cBB839Cbe05303d7705Fa";
 
@@ -280,6 +281,40 @@ pub fn validator_import_using_both_file_flags() {
         .assert_failed();
 }
 
+#[test]
+pub fn validator_import_keystore_file_without_password_flag_should_fail() {
+    CommandLineTest::validators_import()
+        .flag("--vc-token", Some("./token.json"))
+        .flag("--keystore-file", Some("./keystore.json"))
+        .assert_failed();
+}
+
+#[test]
+pub fn validator_import_keystore_file_with_password_flag_should_pass() {
+    CommandLineTest::validators_import()
+        .flag("--vc-token", Some("./token.json"))
+        .flag("--keystore-file", Some("./keystore.json"))
+        .flag("--password", Some("abcd"))
+        .assert_success(|config| {
+            let expected = ImportConfig {
+                validators_file_path: None,
+                keystore_file_path: Some(PathBuf::from("./keystore.json")),
+                vc_url: SensitiveUrl::parse("http://localhost:5062").unwrap(),
+                vc_token_path: PathBuf::from("./token.json"),
+                ignore_duplicates: false,
+                password: Some(Zeroizing::new("abcd".into())),
+                fee_recipient: None,
+                builder_boost_factor: None,
+                gas_limit: None,
+                builder_proposals: None,
+                enabled: None,
+                prefer_builder_proposals: None,
+            };
+            assert_eq!(expected, config);
+            println!("{:?}", expected);
+        });
+}
+
 #[test]
 pub fn validator_import_missing_both_file_flags() {
     CommandLineTest::validators_import()
@@ -287,6 +322,36 @@ pub fn validator_import_missing_both_file_flags() {
         .assert_failed();
 }
 
+#[test]
+pub fn validator_import_fee_recipient_override() {
+    CommandLineTest::validators_import()
+        .flag("--validators-file", Some("./vals.json"))
+        .flag("--vc-token", Some("./token.json"))
+        .flag("--suggested-fee-recipient", Some(EXAMPLE_ETH1_ADDRESS))
+        .flag("--gas-limit", Some("1337"))
+        .flag("--builder-proposals", Some("true"))
+        .flag("--builder-boost-factor", Some("150"))
+        .flag("--prefer-builder-proposals", Some("true"))
+        .flag("--enabled", Some("false"))
+        .assert_success(|config| {
+            let expected = ImportConfig {
+                validators_file_path: Some(PathBuf::from("./vals.json")),
+                keystore_file_path: None,
+                vc_url: SensitiveUrl::parse("http://localhost:5062").unwrap(),
+                vc_token_path: PathBuf::from("./token.json"),
+                ignore_duplicates: false,
+                password: None,
+                fee_recipient: Some(Address::from_str(EXAMPLE_ETH1_ADDRESS).unwrap()),
+                builder_boost_factor: Some(150),
+                gas_limit: Some(1337),
+                builder_proposals: Some(true),
+                enabled: Some(false),
+                prefer_builder_proposals: Some(true),
+            };
+            assert_eq!(expected, config);
+        });
+}
+
 #[test]
 pub fn validator_move_defaults() {
     CommandLineTest::validators_move()

validator_manager/src/import_validators.rs

@@ -112,8 +112,7 @@ pub fn cli_app() -> Command {
                 .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)
+                .display_order(0),
-                .requires(KEYSTORE_FILE_FLAG),
         )
         .arg(
             Arg::new(GAS_LIMIT)
@@ -122,8 +121,7 @@ pub fn cli_app() -> Command {
                 .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)
+                .display_order(0),
-                .requires(KEYSTORE_FILE_FLAG),
         )
         .arg(
             Arg::new(BUILDER_PROPOSALS)
@@ -132,8 +130,7 @@ pub fn cli_app() -> Command {
                 blocks via builder rather than the local EL.",)
                 .value_parser(["true","false"])
                 .action(ArgAction::Set)
-                .display_order(0)
+                .display_order(0),
-                .requires(KEYSTORE_FILE_FLAG),
         )
         .arg(
             Arg::new(BUILDER_BOOST_FACTOR)
@@ -144,8 +141,7 @@ pub fn cli_app() -> Command {
                 when choosing between a builder payload header and payload from \
                 the local execution node.",)
                 .action(ArgAction::Set)
-                .display_order(0)
+                .display_order(0),
-                .requires(KEYSTORE_FILE_FLAG),
         )
         .arg(
             Arg::new(PREFER_BUILDER_PROPOSALS)
@@ -154,8 +150,16 @@ pub fn cli_app() -> Command {
                 constructed by builders, regardless of payload value.",)
                 .value_parser(["true","false"])
                 .action(ArgAction::Set)
-                .display_order(0)
+                .display_order(0),
-                .requires(KEYSTORE_FILE_FLAG),
+        )
+        .arg(
+            Arg::new(ENABLED)
+                .long(ENABLED)
+                .help("When provided, the imported validator will be \
+                enabled or disabled.",)
+                .value_parser(["true","false"])
+                .action(ArgAction::Set)
+                .display_order(0),
         )
 }
 
@@ -225,48 +229,113 @@ async fn run(config: ImportConfig) -> Result<(), String> {
         enabled,
     } = config;
 
-    let validators: Vec<ValidatorSpecification> =
+    let validators: Vec<ValidatorSpecification> = if let Some(validators_format_path) =
-        if let Some(validators_format_path) = &validators_file_path {
+        &validators_file_path
-            if !validators_format_path.exists() {
+    {
-                return Err(format!(
+        if !validators_format_path.exists() {
-                    "Unable to find file at {:?}",
+            return Err(format!(
-                    validators_format_path
+                "Unable to find file at {:?}",
-                ));
+                validators_format_path
-            }
+            ));
+        }
 
-            let validators_file = fs::OpenOptions::new()
+        let validators_file = fs::OpenOptions::new()
-                .read(true)
+            .read(true)
-                .create(false)
+            .create(false)
-                .open(validators_format_path)
+            .open(validators_format_path)
-                .map_err(|e| format!("Unable to open {:?}: {:?}", validators_format_path, e))?;
+            .map_err(|e| format!("Unable to open {:?}: {:?}", validators_format_path, e))?;
 
-            serde_json::from_reader(&validators_file).map_err(|e| {
+        // Define validators as mutable so that if a relevant flag is supplied, the fields can be overridden.
+        let mut validators: Vec<ValidatorSpecification> = 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 {
+        // Log the overridden note when one or more flags is supplied
-                voting_keystore: KeystoreJsonStr(
+        if let Some(override_fee_recipient) = fee_recipient {
-                    Keystore::from_json_file(keystore_format_path).map_err(|e| format!("{e:?}"))?,
+            eprintln!(
-                ),
+                "Please note! --suggested-fee-recipient is provided. This will override existing fee recipient defined in validators.json with: {:?}",
-                voting_keystore_password: password.ok_or_else(|| {
+                override_fee_recipient
-                    "The --password flag is required to supply the keystore password".to_string()
+            );
-                })?,
+        }
-                slashing_protection: None,
+        if let Some(override_gas_limit) = gas_limit {
-                fee_recipient,
+            eprintln!(
-                gas_limit,
+                "Please note! --gas-limit is provided. This will override existing gas limit defined in validators.json with: {}",
-                builder_proposals,
+                override_gas_limit
-                builder_boost_factor,
+            );
-                prefer_builder_proposals,
+        }
-                enabled,
+        if let Some(override_builder_proposals) = builder_proposals {
-            }]
+            eprintln!(
-        } else {
+                "Please note! --builder-proposals is provided. This will override existing builder proposal setting defined in validators.json with: {}",
-            return Err(format!(
+                override_builder_proposals
-                "One of the flag --{VALIDATORS_FILE_FLAG} or --{KEYSTORE_FILE_FLAG} is required."
+            );
-            ));
+        }
-        };
+        if let Some(override_builder_boost_factor) = builder_boost_factor {
+            eprintln!(
+                "Please note! --builder-boost-factor is provided. This will override existing builder boost factor defined in validators.json with: {}",
+                override_builder_boost_factor
+            );
+        }
+        if let Some(override_prefer_builder_proposals) = prefer_builder_proposals {
+            eprintln!(
+                "Please note! --prefer-builder-proposals is provided. This will override existing prefer builder proposal setting defined in validators.json with: {}",
+                override_prefer_builder_proposals
+            );
+        }
+        if let Some(override_enabled) = enabled {
+            eprintln!(
+                "Please note! --enabled flag is provided. This will override existing setting defined in validators.json with: {}",
+                override_enabled
+            );
+        }
+
+        // Override the fields in validators.json file if the flag is supplied
+        for validator in &mut validators {
+            if let Some(override_fee_recipient) = fee_recipient {
+                validator.fee_recipient = Some(override_fee_recipient);
+            }
+            if let Some(override_gas_limit) = gas_limit {
+                validator.gas_limit = Some(override_gas_limit);
+            }
+            if let Some(override_builder_proposals) = builder_proposals {
+                validator.builder_proposals = Some(override_builder_proposals);
+            }
+            if let Some(override_builder_boost_factor) = builder_boost_factor {
+                validator.builder_boost_factor = Some(override_builder_boost_factor);
+            }
+            if let Some(override_prefer_builder_proposals) = prefer_builder_proposals {
+                validator.prefer_builder_proposals = Some(override_prefer_builder_proposals);
+            }
+            if let Some(override_enabled) = enabled {
+                validator.enabled = Some(override_enabled);
+            }
+        }
+
+        validators
+    } 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();