Migrate validator client to clap derive (#6300)

Partially #5900


  Migrate the validator client cli to clap derive

Author: eserilev

Cargo.lock

@@ -2,11 +2,8 @@ mod common;
 pub mod validator;
 pub mod wallet;
 
-use clap::Arg;
-use clap::ArgAction;
 use clap::ArgMatches;
 use clap::Command;
-use clap_utils::FLAG_HEADER;
 use environment::Environment;
 use types::EthSpec;
 
@@ -21,15 +18,6 @@ pub fn cli_app() -> Command {
         .visible_aliases(["a", "am", "account"])
         .about("Utilities for generating and managing Ethereum 2.0 accounts.")
         .display_order(0)
-        .arg(
-            Arg::new("help")
-                .long("help")
-                .short('h')
-                .help("Prints help information")
-                .action(ArgAction::HelpLong)
-                .display_order(0)
-                .help_heading(FLAG_HEADER),
-        )
         .subcommand(wallet::cli_app())
         .subcommand(validator::cli_app())
 }

account_manager/src/lib.rs

@@ -8,7 +8,6 @@ pub mod slashing_protection;
 
 use crate::{VALIDATOR_DIR_FLAG, VALIDATOR_DIR_FLAG_ALIAS};
 use clap::{Arg, ArgAction, ArgMatches, Command};
-use clap_utils::FLAG_HEADER;
 use directory::{parse_path_or_default_with_flag, DEFAULT_VALIDATOR_DIR};
 use environment::Environment;
 use std::path::PathBuf;
@@ -20,16 +19,6 @@ pub fn cli_app() -> Command {
     Command::new(CMD)
         .display_order(0)
         .about("Provides commands for managing Eth2 validators.")
-        .arg(
-            Arg::new("help")
-                .long("help")
-                .short('h')
-                .help("Prints help information")
-                .action(ArgAction::HelpLong)
-                .display_order(0)
-                .help_heading(FLAG_HEADER)
-                .global(true),
-        )
         .arg(
             Arg::new(VALIDATOR_DIR_FLAG)
                 .long(VALIDATOR_DIR_FLAG)

account_manager/src/validator/mod.rs

@@ -4,7 +4,6 @@ pub mod recover;
 
 use crate::WALLETS_DIR_FLAG;
 use clap::{Arg, ArgAction, ArgMatches, Command};
-use clap_utils::FLAG_HEADER;
 use directory::{parse_path_or_default_with_flag, DEFAULT_WALLET_DIR};
 use std::fs::create_dir_all;
 use std::path::PathBuf;
@@ -15,16 +14,6 @@ pub fn cli_app() -> Command {
     Command::new(CMD)
         .about("Manage wallets, from which validator keys can be derived.")
         .display_order(0)
-        .arg(
-            Arg::new("help")
-            .long("help")
-            .short('h')
-            .help("Prints help information")
-            .action(ArgAction::HelpLong)
-            .display_order(0)
-            .help_heading(FLAG_HEADER)
-            .global(true)
-        )
         .arg(
             Arg::new(WALLETS_DIR_FLAG)
                 .long(WALLETS_DIR_FLAG)

account_manager/src/wallet/mod.rs

@@ -18,15 +18,6 @@ pub fn cli_app() -> Command {
         /*
          * Configuration directory locations.
          */
-        .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("network-dir")
                 .long("network-dir")

beacon_node/src/cli.rs

@@ -18,16 +18,16 @@ Options:
           certificate path.
       --broadcast <API_TOPICS>
           Comma-separated list of beacon API topics to broadcast to all beacon
-          nodes. Possible values are: none, attestations, blocks, subscriptions,
-          sync-committee. Default (when flag is omitted) is to broadcast
-          subscriptions only.
+          nodes. Default (when flag is omitted) is to broadcast subscriptions
+          only. [possible values: none, attestations, blocks, subscriptions,
+          sync-committee]
       --builder-boost-factor <UINT64>
           Defines the boost factor, a percentage multiplier to apply to the
           builder's payload value when choosing between a builder payload header
           and payload from the local execution node.
-      --builder-registration-timestamp-override <builder-registration-timestamp-override>
+      --builder-registration-timestamp-override <UNIX-TIMESTAMP>
           This flag takes a unix timestamp value that will be used to override
-          the timestamp used in the builder api registration
+          the timestamp used in the builder api registration.
   -d, --datadir <DIR>
           Used to specify a custom root data directory for lighthouse keys and
           databases. Defaults to $HOME/.lighthouse/{network} where network is
@@ -41,7 +41,7 @@ Options:
           The gas limit to be used in all builder proposals for all validators
           managed by this validator client. Note this will not necessarily be
           used if the gas limit set here moves too far from the previous block's
-          gas limit. [default: 30,000,000]
+          gas limit. [default: 30000000]
       --genesis-state-url <URL>
           A URL of a beacon-API compatible server from which to download the
           genesis state. Checkpoint sync server URLs can generally be used with
@@ -68,7 +68,8 @@ Options:
           is supplied, the CORS allowed origin is set to the listen address of
           this server (e.g., http://localhost:5062).
       --http-port <PORT>
-          Set the listen TCP port for the RESTful HTTP API server.
+          Set the listen TCP port for the RESTful HTTP API server. [default:
+          5062]
       --http-token-path <HTTP_TOKEN_PATH>
           Path to file containing the HTTP API token for validator client
           authentication. If not specified, defaults to
@@ -96,13 +97,15 @@ Options:
           set to 0, background file logging is disabled. [default: 200]
       --metrics-address <ADDRESS>
           Set the listen address for the Prometheus metrics HTTP server.
+          [default: 127.0.0.1]
       --metrics-allow-origin <ORIGIN>
           Set the value of the Access-Control-Allow-Origin response HTTP header.
           Use * to allow any origin (not recommended in production). If no value
           is supplied, the CORS allowed origin is set to the listen address of
           this server (e.g., http://localhost:5064).
       --metrics-port <PORT>
           Set the listen TCP port for the Prometheus metrics HTTP server.
+          [default: 5064]
       --monitoring-endpoint <ADDRESS>
           Enables the monitoring service for sending system metrics to a remote
           endpoint. This can be used to monitor your setup on certain services
@@ -113,7 +116,7 @@ Options:
           provide an untrusted URL.
       --monitoring-endpoint-period <SECONDS>
           Defines how many seconds to wait between each message sent to the
-          monitoring-endpoint. Default: 60s
+          monitoring-endpoint. [default: 60]
       --network <network>
           Name of the Eth2 chain Lighthouse will sync and follow. [possible
           values: mainnet, gnosis, chiado, sepolia, holesky]
@@ -145,8 +148,8 @@ Options:
           each validator along with the common slashing protection database and
           the validator_definitions.yml
       --web3-signer-keep-alive-timeout <MILLIS>
-          Keep-alive timeout for each web3signer connection. Set to 'null' to
-          never timeout [default: 20000]
+          Keep-alive timeout for each web3signer connection. Set to '0' to never
+          timeout. [default: 20000]
       --web3-signer-max-idle-connections <COUNT>
           Maximum number of idle connections to maintain per web3signer host.
           Default is unlimited.

book/src/help_vc.md

@@ -13,15 +13,6 @@ pub fn cli_app() -> Command {
         surface compared to a full beacon node.")
         .styles(get_color_style())
         .display_order(0)
-        .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("enr-address")
                 .long("enr-address")

boot_node/src/cli.rs

@@ -66,16 +66,6 @@ pub struct DatabaseManager {
     )]
     pub backend: store::config::DatabaseBackend,
 
-    #[clap(
-        long,
-        global = true,
-        help = "Prints help information",
-        action = clap::ArgAction::HelpLong,
-        display_order = 0,
-        help_heading = FLAG_HEADER
-    )]
-    help: Option<bool>,
-
     #[clap(subcommand)]
     pub subcommand: DatabaseManagerSubcommand,
 }

database_manager/src/cli.rs

@@ -1,9 +1,12 @@
 use clap::Parser;
 use database_manager::cli::DatabaseManager;
 use serde::{Deserialize, Serialize};
+use validator_client::cli::ValidatorClient;
 
 #[derive(Parser, Clone, Deserialize, Serialize, Debug)]
 pub enum LighthouseSubcommands {
     #[clap(name = "database_manager")]
-    DatabaseManager(DatabaseManager),
+    DatabaseManager(Box<DatabaseManager>),
+    #[clap(name = "validator_client")]
+    ValidatorClient(Box<ValidatorClient>),
 }

lighthouse/src/cli.rs

@@ -399,10 +399,10 @@ fn main() {
             .action(ArgAction::HelpLong)
             .display_order(0)
             .help_heading(FLAG_HEADER)
+            .global(true)
         )
         .subcommand(beacon_node::cli_app())
         .subcommand(boot_node::cli_app())
-        .subcommand(validator_client::cli_app())
         .subcommand(account_manager::cli_app())
         .subcommand(validator_manager::cli_app());
 
@@ -673,12 +673,49 @@ fn run<E: EthSpec>(
         return Ok(());
     }
 
-    if let Ok(LighthouseSubcommands::DatabaseManager(db_manager_config)) =
-        LighthouseSubcommands::from_arg_matches(matches)
-    {
-        info!(log, "Running database manager for {} network", network_name);
-        database_manager::run(matches, &db_manager_config, environment)?;
-        return Ok(());
+    match LighthouseSubcommands::from_arg_matches(matches) {
+        Ok(LighthouseSubcommands::DatabaseManager(db_manager_config)) => {
+            info!(log, "Running database manager for {} network", network_name);
+            database_manager::run(matches, &db_manager_config, environment)?;
+            return Ok(());
+        }
+        Ok(LighthouseSubcommands::ValidatorClient(validator_client_config)) => {
+            let context = environment.core_context();
+            let log = context.log().clone();
+            let executor = context.executor.clone();
+            let config = validator_client::Config::from_cli(
+                matches,
+                &validator_client_config,
+                context.log(),
+            )
+            .map_err(|e| format!("Unable to initialize validator config: {}", e))?;
+            // Dump configs if `dump-config` or `dump-chain-config` flags are set
+            clap_utils::check_dump_configs::<_, E>(matches, &config, &context.eth2_config.spec)?;
+
+            let shutdown_flag = matches.get_flag("immediate-shutdown");
+            if shutdown_flag {
+                info!(log, "Validator client immediate shutdown triggered.");
+                return Ok(());
+            }
+
+            executor.clone().spawn(
+                async move {
+                    if let Err(e) = ProductionValidatorClient::new(context, config)
+                        .and_then(|mut vc| async move { vc.start_service().await })
+                        .await
+                    {
+                        crit!(log, "Failed to start validator client"; "reason" => e);
+                        // Ignore the error since it always occurs during normal operation when
+                        // shutting down.
+                        let _ = executor
+                            .shutdown_sender()
+                            .try_send(ShutdownReason::Failure("Failed to start validator client"));
+                    }
+                },
+                "validator_client",
+            );
+        }
+        Err(_) => (),
     };
 
     info!(log, "Lighthouse started"; "version" => VERSION);
@@ -733,38 +770,9 @@ fn run<E: EthSpec>(
                 "beacon_node",
             );
         }
-        Some(("validator_client", matches)) => {
-            let context = environment.core_context();
-            let log = context.log().clone();
-            let executor = context.executor.clone();
-            let config = validator_client::Config::from_cli(matches, context.log())
-                .map_err(|e| format!("Unable to initialize validator config: {}", e))?;
-            // Dump configs if `dump-config` or `dump-chain-config` flags are set
-            clap_utils::check_dump_configs::<_, E>(matches, &config, &context.eth2_config.spec)?;
-
-            let shutdown_flag = matches.get_flag("immediate-shutdown");
-            if shutdown_flag {
-                info!(log, "Validator client immediate shutdown triggered.");
-                return Ok(());
-            }
-
-            executor.clone().spawn(
-                async move {
-                    if let Err(e) = ProductionValidatorClient::new(context, config)
-                        .and_then(|mut vc| async move { vc.start_service().await })
-                        .await
-                    {
-                        crit!(log, "Failed to start validator client"; "reason" => e);
-                        // Ignore the error since it always occurs during normal operation when
-                        // shutting down.
-                        let _ = executor
-                            .shutdown_sender()
-                            .try_send(ShutdownReason::Failure("Failed to start validator client"));
-                    }
-                },
-                "validator_client",
-            );
-        }
+        // TODO(clap-derive) delete this once we've fully migrated to clap derive.
+        // Qt the moment this needs to exist so that we dont trigger a crit.
+        Some(("validator_client", _)) => (),
         _ => {
             crit!(log, "No subcommand supplied. See --help .");
             return Err("No subcommand supplied.".into());

lighthouse/src/main.rs

@@ -407,6 +407,13 @@ fn metrics_port_flag() {
         .with_config(|config| assert_eq!(config.http_metrics.listen_port, 9090));
 }
 #[test]
+fn metrics_port_flag_default() {
+    CommandLineTest::new()
+        .flag("metrics", None)
+        .run()
+        .with_config(|config| assert_eq!(config.http_metrics.listen_port, 5064));
+}
+#[test]
 fn metrics_allow_origin_flag() {
     CommandLineTest::new()
         .flag("metrics", None)
@@ -458,7 +465,7 @@ fn no_doppelganger_protection_flag() {
 fn no_gas_limit_flag() {
     CommandLineTest::new()
         .run()
-        .with_config(|config| assert!(config.validator_store.gas_limit.is_none()));
+        .with_config(|config| assert!(config.validator_store.gas_limit == Some(30_000_000)));
 }
 #[test]
 fn gas_limit_flag() {
@@ -560,7 +567,7 @@ fn broadcast_flag() {
         });
     // Other valid variants
     CommandLineTest::new()
-        .flag("broadcast", Some("blocks, subscriptions"))
+        .flag("broadcast", Some("blocks,subscriptions"))
         .run()
         .with_config(|config| {
             assert_eq!(
@@ -605,7 +612,7 @@ fn beacon_nodes_sync_tolerances_flag() {
 }
 
 #[test]
-#[should_panic(expected = "Unknown API topic")]
+#[should_panic(expected = "invalid value")]
 fn wrong_broadcast_flag() {
     CommandLineTest::new()
         .flag("broadcast", Some("foo, subscriptions"))

lighthouse/tests/validator_client.rs

@@ -9,6 +9,7 @@ name = "beacon_node_fallback"
 path = "src/lib.rs"
 
 [dependencies]
+clap = { workspace = true }
 environment = { workspace = true }
 eth2 = { workspace = true }
 futures = { workspace = true }