From 5369922517c6478f690a7f22358224a447d0a6d3 Mon Sep 17 00:00:00 2001 From: Tan Chee Keong Date: Tue, 1 Jul 2025 12:06:09 +0800 Subject: [PATCH 1/5] enable CLI overridden --- validator_manager/src/import_validators.rs | 140 ++++++++++++++------- 1 file changed, 94 insertions(+), 46 deletions(-) diff --git a/validator_manager/src/import_validators.rs b/validator_manager/src/import_validators.rs index 6cfbf7b54ee..f453a6a5c74 100644 --- a/validator_manager/src/import_validators.rs +++ b/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) - .requires(KEYSTORE_FILE_FLAG), + .display_order(0), ) .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) - .requires(KEYSTORE_FILE_FLAG), + .display_order(0), ) .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) - .requires(KEYSTORE_FILE_FLAG), + .display_order(0), ) .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) - .requires(KEYSTORE_FILE_FLAG), + .display_order(0), ) .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) - .requires(KEYSTORE_FILE_FLAG), + .display_order(0), + ) + .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,92 @@ async fn run(config: ImportConfig) -> Result<(), String> { enabled, } = config; - let validators: Vec = - 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: Vec = 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_format_path) - .map_err(|e| format!("Unable to open {:?}: {:?}", validators_format_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| { + // Define validators as mutable as that if CLI flag is supplied, the fields can be overridden + let mut validators: Vec = 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." - )); - }; + })?; + + // Log the overridden note when one or more flags is supplied + if let Some(override_fee_recipient) = fee_recipient { + eprintln!("Please note! --suggested-fee-recipient is provided. This will override existing fee recipient defined in validators.json with: {:?}", override_fee_recipient); + } + if let Some(override_gas_limit) = gas_limit { + eprintln!("Please note! --gas-limit is provided. This will override existing gas limit defined in validators.json with: {}", override_gas_limit); + } + if let Some(override_builder_proposals) = builder_proposals { + eprintln!("Please note! --builder-proposals is provided. This will override existing builder proposal settings defined in validators.json with: {}", override_builder_proposals); + } + 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 settings 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_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(); From 92e5f12ffd51685737fff5672a858dd062e423c8 Mon Sep 17 00:00:00 2001 From: Tan Chee Keong Date: Tue, 1 Jul 2025 14:55:00 +0800 Subject: [PATCH 2/5] Add test --- lighthouse/tests/validator_manager.rs | 30 +++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lighthouse/tests/validator_manager.rs b/lighthouse/tests/validator_manager.rs index 04e3eafe6eb..829c8ffcb1a 100644 --- a/lighthouse/tests/validator_manager.rs +++ b/lighthouse/tests/validator_manager.rs @@ -279,6 +279,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() From 220fdaa15f3802911c672e65e28541e4f4bcc7e2 Mon Sep 17 00:00:00 2001 From: Tan Chee Keong Date: Tue, 1 Jul 2025 14:59:06 +0800 Subject: [PATCH 3/5] minor --- validator_manager/src/import_validators.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/validator_manager/src/import_validators.rs b/validator_manager/src/import_validators.rs index f453a6a5c74..c133f3a49aa 100644 --- a/validator_manager/src/import_validators.rs +++ b/validator_manager/src/import_validators.rs @@ -262,13 +262,13 @@ async fn run(config: ImportConfig) -> Result<(), String> { eprintln!("Please note! --gas-limit is provided. This will override existing gas limit defined in validators.json with: {}", override_gas_limit); } if let Some(override_builder_proposals) = builder_proposals { - eprintln!("Please note! --builder-proposals is provided. This will override existing builder proposal settings defined in validators.json with: {}", override_builder_proposals); + eprintln!("Please note! --builder-proposals is provided. This will override existing builder proposal setting defined in validators.json with: {}", override_builder_proposals); } 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 settings defined in validators.json with: {}", override_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); From 93a378f31783c3aace914db56956a75edb3c1586 Mon Sep 17 00:00:00 2001 From: Tan Chee Keong Date: Tue, 1 Jul 2025 15:38:50 +0800 Subject: [PATCH 4/5] add more tests --- lighthouse/tests/validator_manager.rs | 35 +++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/lighthouse/tests/validator_manager.rs b/lighthouse/tests/validator_manager.rs index 829c8ffcb1a..ce4d8e4dd3c 100644 --- a/lighthouse/tests/validator_manager.rs +++ b/lighthouse/tests/validator_manager.rs @@ -14,6 +14,7 @@ use validator_manager::{ list_validators::ListConfig, move_validators::{MoveConfig, PasswordSource, Validators}, }; +use zeroize::Zeroizing; const EXAMPLE_ETH1_ADDRESS: &str = "0x00000000219ab540356cBB839Cbe05303d7705Fa"; @@ -272,6 +273,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() From 4541422b2c5db793ad2b96cda5f0af38978682ab Mon Sep 17 00:00:00 2001 From: Tan Chee Keong Date: Tue, 1 Jul 2025 16:05:16 +0800 Subject: [PATCH 5/5] cli check --- book/src/help_vm_import.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/book/src/help_vm_import.md b/book/src/help_vm_import.md index ca635be5f15..cbcc6fda2d3 100644 --- a/book/src/help_vm_import.md +++ b/book/src/help_vm_import.md @@ -24,6 +24,9 @@ Options: --debug-level Specifies the verbosity level used when emitting logs to the terminal. [default: info] [possible values: info, debug, trace, warn, error] + --enabled + When provided, the imported validator will be enabled or disabled. + [possible values: true, false] --gas-limit When provided, the imported validator will use this gas limit. It is recommended to leave this as the default value by not specifying this