add custom flag parser, make rate limiter flags clap friendly

beacon_node/src/cli.rs

@@ -1,4 +1,4 @@
-use clap::{builder::ArgPredicate, Arg, ArgAction, ArgGroup, Command, crate_version};
+use clap::{builder::ArgPredicate, crate_version, Arg, ArgAction, ArgGroup, Command};
 use clap_utils::get_color_style;
 use strum::VariantNames;
 use types::ProgressiveBalancesMode;
@@ -311,6 +311,18 @@ pub fn cli_app() -> Command {
         .arg(
             Arg::new("self-limiter")
             .long("self-limiter")
+            .help(
+                "Enables the outbound rate limiter (requests made by this node). \
+                Use the self-limiter-protocol flag to set per protocol configurations. \
+                If the self rate limiter is enabled and a protocol is not \
+                present in the configuration, the quotas used for the inbound rate limiter will be \
+                used."
+            )
+            .action(ArgAction::SetTrue)
+        )
+        .arg(
+            Arg::new("self-limiter-protocols")
+            .long("self-limiter-protocols")
             .help(
                 "Enables the outbound rate limiter (requests made by this node).\
                 \
@@ -320,11 +332,9 @@ pub fn cli_app() -> Command {
                 present in the configuration, the quotas used for the inbound rate limiter will be \
                 used."
             )
-            .default_missing_value("true")
-            .require_equals(false)
-            // TODO this is dangerous without requires_equals = true
-            .num_args(0..1)
-            .hide(true)
+            .action(ArgAction::Append)
+            .value_delimiter(';')
+            .requires("self-limiter")
         )
         .arg(
             Arg::new("proposer-only")
@@ -335,21 +345,28 @@ pub fn cli_app() -> Command {
                 .action(ArgAction::SetTrue)
         )
         .arg(
-            Arg::new("inbound-rate-limiter")
-            .long("inbound-rate-limiter")
+            Arg::new("disable-inbound-rate-limiter")
+            .long("disable-inbound-rate-limiter")
+            .help(
+                "Disables the inbound rate limiter (requests received by this node)."
+            )
+            .action(ArgAction::SetTrue)
+        )
+        .arg(
+            Arg::new("inbound-rate-limiter-protocols")
+            .long("inbound-rate-limiter-protocols")
             .help(
                 "Configures the inbound rate limiter (requests received by this node).\
                 \
                 Rate limit quotas per protocol can be set in the form of \
                 <protocol_name>:<tokens>/<time_in_seconds>. To set quotas for multiple protocols, \
-                separate them by ';'. If the inbound rate limiter is enabled and a protocol is not \
-                present in the configuration, the default quotas will be used. \
+                separate them by ';'. \
                 \
-                This is enabled by default, using default quotas. To disable rate limiting pass \
-                `disabled` to this option instead."
+                This is enabled by default, using default quotas. To disable rate limiting use \
+                the disable-inbound-rate-limiter flag instead."
             )
             .action(ArgAction::Set)
-            .hide(true)
+            .conflicts_with("disable-inbound-rate-limiter")
         )
         .arg(
             Arg::new("disable-backfill-rate-limiting")

beacon_node/src/config.rs

@@ -5,7 +5,7 @@ use beacon_chain::chain_config::{
 use beacon_chain::TrustedSetup;
 use clap::ArgMatches;
 use clap_utils::flags::DISABLE_MALLOC_TUNING_FLAG;
-use clap_utils::parse_required;
+use clap_utils::{parse_flag, parse_required};
 use client::{ClientConfig, ClientGenesis};
 use directory::{DEFAULT_BEACON_NODE_DIR, DEFAULT_NETWORK_DIR, DEFAULT_ROOT_DIR};
 use environment::RuntimeContext;
@@ -103,7 +103,7 @@ pub fn get_config<E: EthSpec>(
      * Http API server
      */
 
-    if cli_args.contains_id("enable_http") {
+    if parse_flag(cli_args, "enable_http") {
         client_config.http_api.enabled = true;
 
         if let Some(address) = cli_args.get_one::<String>("http-address") {
@@ -324,7 +324,7 @@ pub fn get_config<E: EthSpec>(
                 clap_utils::parse_optional(cli_args, "builder-user-agent")?;
         }
 
-        if cli_args.contains_id("builder-profit-threshold") {
+        if parse_flag(cli_args, "builder-profit-threshold") {
             warn!(
                 log,
                 "Ignoring --builder-profit-threshold";
@@ -555,7 +555,7 @@ pub fn get_config<E: EthSpec>(
             ClientGenesis::GenesisState
         }
     } else {
-        if cli_args.contains_id("checkpoint-state") || cli_args.contains_id("checkpoint-sync-url") {
+        if parse_flag(cli_args, "checkpoint-state") || parse_flag(cli_args, "checkpoint-sync-url") {
             return Err(
                 "Checkpoint sync is not available for this network as no genesis state is known"
                     .to_string(),
@@ -882,7 +882,7 @@ pub fn parse_listening_addresses(
     let listen_addresses_str = cli_args
         .get_many::<String>("listen-address")
         .expect("--listen_addresses has a default value");
-    let use_zero_ports = cli_args.get_flag("zero-ports");
+    let use_zero_ports = parse_flag(cli_args, "zero-ports");
 
     // parse the possible ips
     let mut maybe_ipv4 = None;
@@ -972,7 +972,7 @@ pub fn parse_listening_addresses(
         (None, Some(ipv6)) => {
             // A single ipv6 address was provided. Set the ports
 
-            if cli_args.contains_id("port6") {
+            if parse_flag(cli_args, "port6") {
                 warn!(log, "When listening only over IPv6, use the --port flag. The value of --port6 will be ignored.")
             }
             // use zero ports if required. If not, use the given port.
@@ -1114,15 +1114,15 @@ pub fn set_network_config(
         config.network_dir = data_dir.join(DEFAULT_NETWORK_DIR);
     };
 
-    if cli_args.get_flag("subscribe-all-subnets") {
+    if parse_flag(cli_args, "subscribe-all-subnets") {
         config.subscribe_all_subnets = true;
     }
 
-    if cli_args.get_flag("import-all-attestations") {
+    if parse_flag(cli_args, "import-all-attestations") {
         config.import_all_attestations = true;
     }
 
-    if cli_args.get_flag("shutdown-after-sync") {
+    if parse_flag(cli_args, "shutdown-after-sync") {
         config.shutdown_after_sync = true;
     }
 
@@ -1178,7 +1178,7 @@ pub fn set_network_config(
             .collect::<Result<Vec<Multiaddr>, _>>()?;
     }
 
-    if cli_args.get_flag("disable-peer-scoring") {
+    if parse_flag(cli_args, "disable-peer-scoring") {
         config.disable_peer_scoring = true;
     }
 
@@ -1244,7 +1244,7 @@ pub fn set_network_config(
         );
     }
 
-    if cli_args.get_flag("enr-match") {
+    if parse_flag(cli_args, "enr-match") {
         // Match the IP and UDP port in the ENR.
 
         if let Some(ipv4_addr) = config.listen_addrs().v4().cloned() {
@@ -1360,56 +1360,58 @@ pub fn set_network_config(
         }
     }
 
-    if cli_args.get_flag("disable-enr-auto-update") {
+    if parse_flag(cli_args, "disable-enr-auto-update") {
         config.discv5_config.enr_update = false;
     }
 
-    if cli_args.get_flag("disable-packet-filter") {
+    if parse_flag(cli_args, "disable-packet-filter") {
         warn!(log, "Discv5 packet filter is disabled");
         config.discv5_config.enable_packet_filter = false;
     }
 
-    if cli_args.get_flag("disable-discovery") {
+    if parse_flag(cli_args, "disable-discovery") {
         config.disable_discovery = true;
         warn!(log, "Discovery is disabled. New peers will not be found");
     }
 
-    if cli_args.get_flag("disable-quic") {
+    if parse_flag(cli_args, "disable-quic") {
         config.disable_quic_support = true;
     }
 
-    if cli_args.get_flag("disable-upnp") {
+    if parse_flag(cli_args, "disable-upnp") {
         config.upnp_enabled = false;
     }
 
-    if cli_args.get_flag("private") {
+    if parse_flag(cli_args, "private") {
         config.private = true;
     }
 
-    if cli_args.get_flag("metrics") {
+    if parse_flag(cli_args, "metrics") {
         config.metrics_enabled = true;
     }
 
-    if cli_args.get_flag("enable-private-discovery") {
+    if parse_flag(cli_args, "enable-private-discovery") {
         config.discv5_config.table_filter = |_| true;
     }
 
     // Light client server config.
-    config.enable_light_client_server = cli_args.get_flag("light-client-server");
-
-    // The self limiter is disabled by default.
-    // This flag can be used both with or without a value. Try to parse it first with a value, if
-    // no value is defined but the flag is present, use the default params.
-    if cli_args.try_get_one::<bool>("self_limiter").is_ok() {
-        config.outbound_rate_limiter_config = Some(Default::default());
-    } else {
-        config.outbound_rate_limiter_config = clap_utils::parse_optional(cli_args, "self-limiter")?;
+    config.enable_light_client_server = parse_flag(cli_args, "light-client-server");
+
+    // The self limiter is disabled by default. If the `self-limiter` flag is provided
+    // without the `self-limiter-protocols` flag, the default params will be used.
+    if parse_flag(cli_args, "self-limiter") {
+        config.outbound_rate_limiter_config =
+            if let Some(protocols) = cli_args.get_one::<String>("self-limiter-protocols") {
+                Some(protocols.parse()?)
+            } else {
+                Some(Default::default())
+            };
     }
 
     // Proposer-only mode overrides a number of previous configuration parameters.
     // Specifically, we avoid subscribing to long-lived subnets and wish to maintain a minimal set
     // of peers.
-    if cli_args.get_flag("proposer-only") {
+    if parse_flag(cli_args, "proposer-only") {
         config.subscribe_all_subnets = false;
 
         if cli_args.get_one::<String>("target-peers").is_none() {
@@ -1419,21 +1421,17 @@ pub fn set_network_config(
         config.proposer_only = true;
         warn!(log, "Proposer-only mode enabled"; "info"=> "Do not connect a validator client to this node unless via the --proposer-nodes flag");
     }
-    // The inbound rate limiter is enabled by default unless `disabled` is passed to the
-    // `inbound-rate-limiter` flag. Any other value should be parsed as a configuration string.
-    config.inbound_rate_limiter_config = match cli_args.get_one::<String>("inbound-rate-limiter") {
-        None => {
-            // Enabled by default, with default values
+    // The inbound rate limiter is enabled by default unless `disabled` via the
+    // `disable-inbound-rate-limiter` flag.
+    config.inbound_rate_limiter_config = if parse_flag(cli_args, "disable-inbound-rate-limiter") {
+        None
+    } else {
+        // Use the default unless values are provided via the `inbound-rate-limiter-protocols`
+        if let Some(protocols) = cli_args.get_one::<String>("inbound-rate-limiter-protocols") {
+            Some(protocols.parse()?)
+        } else {
             Some(Default::default())
         }
-        Some(config_str) => {
-            if config_str.as_str() == "disabled" {
-                None
-            } else {
-                // Enabled with a custom configuration
-                Some(config_str.parse()?)
-            }
-        }
     };
     Ok(())
 }

boot_node/src/cli.rs

@@ -106,111 +106,4 @@ pub fn cli_app() -> Command {
                 .help("The directory which contains the enr and it's associated private key")
                 .action(ArgAction::Set)
         )
-        .arg(
-            Arg::new("subscribe-all-subnets")
-                .long("subscribe-all-subnets")
-                .action(ArgAction::SetTrue)
-                .help("Subscribe to all subnets regardless of validator count. \
-                       This will also advertise the beacon node as being long-lived subscribed to all subnets.")
-        )
-        .arg(
-            Arg::new("import-all-attestations")
-                .long("import-all-attestations")
-                .help("Import and aggregate all attestations, regardless of validator subscriptions. \
-                       This will only import attestations from already-subscribed subnets, use with \
-                       --subscribe-all-subnets to ensure all attestations are received for import.")
-                .action(ArgAction::SetTrue)
-        )
-        .arg(
-            Arg::new("shutdown-after-sync")
-                .long("shutdown-after-sync")
-                .help("Shutdown beacon node as soon as sync is completed. Backfill sync will \
-                       not be performed before shutdown.")
-                .action(ArgAction::SetTrue)
-        )
-        .arg(
-            Arg::new("zero-ports")
-                .long("zero-ports")
-                .short('z')
-                .help("Sets all listening TCP/UDP ports to 0, allowing the OS to choose some \
-                       arbitrary free ports.")
-                .action(ArgAction::SetTrue)
-        )
-        .arg(
-            Arg::new("disable-peer-scoring")
-                .long("disable-peer-scoring")
-                .help("Disables peer scoring in lighthouse. WARNING: This is a dev only flag is only meant to be used in local testing scenarios \
-                        Using this flag on a real network may cause your node to become eclipsed and see a different view of the network")
-                .action(ArgAction::SetTrue)
-                .hide(true)
-        )
-        .arg(
-            Arg::new("enr-match")
-                .short('e')
-                .long("enr-match")
-                .action(ArgAction::SetTrue)
-                .help("Sets the local ENR IP address and port to match those set for lighthouse. \
-                      Specifically, the IP address will be the value of --listen-address and the \
-                      UDP port will be --discovery-port.")
-        )
-        .arg(
-            Arg::new("disable-enr-auto-update")
-                .short('x')
-                .action(ArgAction::SetTrue)
-                .long("disable-enr-auto-update")
-                .help("Discovery automatically updates the nodes local ENR with an external IP address and port as seen by other peers on the network. \
-                This disables this feature, fixing the ENR's IP/PORT to those specified on boot.")
-        )
-        .arg(
-            Arg::new("disable-discovery")
-                .long("disable-discovery")
-                .action(ArgAction::SetTrue)
-                .help("Disables the discv5 discovery protocol. The node will not search for new peers or participate in the discovery protocol.")
-                .hide(true)
-        )
-        .arg(
-            Arg::new("disable-quic")
-                .long("disable-quic")
-                .action(ArgAction::SetTrue)
-                .help("Disables the quic transport. The node will rely solely on the TCP transport for libp2p connections.")
-        )
-        .arg(
-            Arg::new("disable-upnp")
-                .long("disable-upnp")
-                .help("Disables UPnP support. Setting this will prevent Lighthouse from attempting to automatically establish external port mappings.")
-                .action(ArgAction::SetTrue)
-        )
-        .arg(
-            Arg::new("private")
-                .long("private")
-                .help("Prevents sending various client identification information.")
-                .action(ArgAction::SetTrue)
-        )
-        .arg(
-            Arg::new("metrics")
-                .long("metrics")
-                .help("Enable the Prometheus metrics HTTP server. Disabled by default.")
-                .action(ArgAction::SetTrue)
-        )
-        .arg(
-            Arg::new("enable-private-discovery")
-                .long("enable-private-discovery")
-                .help("Lighthouse by default does not discover private IP addresses. Set this flag to enable connection attempts to local addresses.")
-                .action(ArgAction::SetTrue)
-        )
-        .arg(
-            Arg::new("light-client-server")
-                .long("light-client-server")
-                .help("Act as a full node supporting light clients on the p2p network \
-                       [experimental]")
-                .action(ArgAction::SetTrue)
-        )
-        .arg(
-            Arg::new("proposer-only")
-                .long("proposer-only")
-                .help("Sets this beacon node at be a block proposer only node. \
-                       This will run the beacon node in a minimal configuration that is sufficient for block publishing only. This flag should be used \
-                       for a beacon node being referenced by validator client using the --proposer-node flag. This configuration is for enabling more secure setups.")
-                .action(ArgAction::SetTrue)
-        )
 }

boot_node/src/config.rs

@@ -29,7 +29,6 @@ impl<E: EthSpec> BootNodeConfig<E> {
         eth2_network_config: &Eth2NetworkConfig,
     ) -> Result<Self, String> {
         let data_dir = get_data_dir(matches);
-
         // Try and obtain bootnodes
 
         let boot_nodes = {