Migrate validator client to clap derive

book/src/help_vc.md

@@ -20,14 +20,15 @@ Options:
           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.
+          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 +42,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 +69,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]
       --log-format <FORMAT>
           Specifies the log format used when emitting logs to the terminal.
           [possible values: JSON]
@@ -92,13 +94,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
@@ -109,7 +113,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]
@@ -141,8 +145,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.

lighthouse/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/main.rs

@@ -395,10 +395,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());
 
@@ -669,12 +669,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);
@@ -729,38 +766,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/tests/validator_client.rs

@@ -379,6 +379,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)
@@ -430,7 +437,7 @@ fn no_doppelganger_protection_flag() {
 fn no_gas_limit_flag() {
     CommandLineTest::new()
         .run()
-        .with_config(|config| assert!(config.gas_limit.is_none()));
+        .with_config(|config| assert!(config.gas_limit == Some(30_000_000)));
 }
 #[test]
 fn gas_limit_flag() {
@@ -532,7 +539,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!(
@@ -577,7 +584,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"))

validator_client/src/beacon_node_fallback.rs

@@ -8,6 +8,7 @@ use crate::beacon_node_health::{
 };
 use crate::check_synced::check_node_health;
 use crate::http_metrics::metrics::{inc_counter_vec, ENDPOINT_ERRORS, ENDPOINT_REQUESTS};
+use clap::ValueEnum;
 use environment::RuntimeContext;
 use eth2::BeaconNodeHttpClient;
 use futures::future;
@@ -21,7 +22,8 @@ use std::future::Future;
 use std::marker::PhantomData;
 use std::sync::Arc;
 use std::time::{Duration, Instant};
-use strum::{EnumString, EnumVariantNames};
+use std::vec::Vec;
+use strum::EnumVariantNames;
 use tokio::{sync::RwLock, time::sleep};
 use types::{ChainSpec, Config as ConfigSpec, EthSpec, Slot};
 
@@ -719,9 +721,10 @@ async fn sort_nodes_by_health<E: EthSpec>(nodes: &mut Vec<CandidateBeaconNode<E>
 }
 
 /// Serves as a cue for `BeaconNodeFallback` to tell which requests need to be broadcasted.
-#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, EnumString, EnumVariantNames)]
+#[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize, EnumVariantNames, ValueEnum)]
 #[strum(serialize_all = "kebab-case")]
 pub enum ApiTopic {
+    None,
     Attestations,
     Blocks,
     Subscriptions,
@@ -741,7 +744,6 @@ mod tests {
     use crate::beacon_node_health::BeaconNodeHealthTier;
     use crate::SensitiveUrl;
     use eth2::Timeouts;
-    use std::str::FromStr;
     use strum::VariantNames;
     use types::{MainnetEthSpec, Slot};
 
@@ -750,10 +752,13 @@ mod tests {
     #[test]
     fn api_topic_all() {
         let all = ApiTopic::all();
-        assert_eq!(all.len(), ApiTopic::VARIANTS.len());
-        assert!(ApiTopic::VARIANTS
+        // ignore NONE variant
+        let mut variants = ApiTopic::VARIANTS.to_vec();
+        variants.retain(|s| *s != "none");
+        assert_eq!(all.len(), variants.len());
+        assert!(variants
             .iter()
-            .map(|topic| ApiTopic::from_str(topic).unwrap())
+            .map(|topic| ApiTopic::from_str(topic, true).unwrap())
             .eq(all.into_iter()));
     }
 

validator_client/src/beacon_node_health.rs

@@ -1,8 +1,6 @@
-use itertools::Itertools;
 use serde::{Deserialize, Serialize};
 use std::cmp::Ordering;
 use std::fmt::{Debug, Display, Formatter};
-use std::str::FromStr;
 use types::Slot;
 
 /// Sync distances between 0 and DEFAULT_SYNC_TOLERANCE are considered `synced`.
@@ -50,29 +48,6 @@ impl Default for BeaconNodeSyncDistanceTiers {
     }
 }
 
-impl FromStr for BeaconNodeSyncDistanceTiers {
-    type Err = String;
-
-    fn from_str(s: &str) -> Result<Self, String> {
-        let values: (u64, u64, u64) = s
-            .split(',')
-            .map(|s| {
-                s.parse()
-                    .map_err(|e| format!("Invalid sync distance modifier: {e:?}"))
-            })
-            .collect::<Result<Vec<_>, _>>()?
-            .into_iter()
-            .collect_tuple()
-            .ok_or("Invalid number of sync distance modifiers".to_string())?;
-
-        Ok(BeaconNodeSyncDistanceTiers {
-            synced: Slot::new(values.0),
-            small: Slot::new(values.0 + values.1),
-            medium: Slot::new(values.0 + values.1 + values.2),
-        })
-    }
-}
-
 impl BeaconNodeSyncDistanceTiers {
     /// Takes a given sync distance and determines its tier based on the `sync_tolerance` defined by
     /// the CLI.
@@ -87,6 +62,17 @@ impl BeaconNodeSyncDistanceTiers {
             SyncDistanceTier::Large
         }
     }
+
+    pub fn from_vec(tiers: &[u64]) -> Result<Self, String> {
+        if tiers.len() != 3 {
+            return Err("Invalid number of sync distance modifiers".to_string());
+        }
+        Ok(BeaconNodeSyncDistanceTiers {
+            synced: Slot::new(tiers[0]),
+            small: Slot::new(tiers[0] + tiers[1]),
+            medium: Slot::new(tiers[0] + tiers[1] + tiers[2]),
+        })
+    }
 }
 
 /// Execution Node health metrics.
@@ -293,7 +279,6 @@ mod tests {
         SyncDistanceTier,
     };
     use crate::beacon_node_fallback::Config;
-    use std::str::FromStr;
     use types::Slot;
 
     #[test]
@@ -396,7 +381,7 @@ mod tests {
         // medium 9..=12
         // large: 13..
 
-        let distance_tiers = BeaconNodeSyncDistanceTiers::from_str("4,4,4").unwrap();
+        let distance_tiers = BeaconNodeSyncDistanceTiers::from_vec(&[4, 4, 4]).unwrap();
 
         let synced_low = new_distance_tier(0, &distance_tiers);
         let synced_high = new_distance_tier(4, &distance_tiers);