diff --git a/bin/ethlambda/src/cli.rs b/bin/ethlambda/src/cli.rs index 9a4ae79c..d8bb6963 100644 --- a/bin/ethlambda/src/cli.rs +++ b/bin/ethlambda/src/cli.rs @@ -4,11 +4,8 @@ use ethlambda_p2p::discovery::DEFAULT_DISCOVERY_TARGET_PEERS; use std::net::IpAddr; use std::path::PathBuf; -use crate::version; - -#[derive(Debug, clap::Parser)] -#[command(name = "ethlambda", author = "LambdaClass", version = version::CLIENT_VERSION, about = "ethlambda consensus client")] -pub(crate) struct CliOptions { +#[derive(Debug, clap::Args)] +pub(crate) struct NodeOptions { /// Path to the chain genesis config (e.g., config.yaml). #[arg(long)] pub(crate) genesis: PathBuf, @@ -162,7 +159,7 @@ pub(crate) struct DiscoveryConfig { pub(crate) target_peers: usize, } -impl CliOptions { +impl NodeOptions { /// Reject a discovery port that collides with the QUIC port. /// /// Both are UDP. Without this the collision surfaces at bind time as an @@ -220,10 +217,13 @@ pub(crate) struct ShadowOptions { #[cfg(test)] mod tests { use super::*; - use clap::Parser as _; + use crate::command::{Command, try_parse_from}; /// The required flags, so a test can vary only what it cares about. - fn parse(extra: &[&str]) -> CliOptions { + /// + /// `NodeOptions` is a `clap::Args` group rather than a parser of its own, + /// so this parses through the real dispatch, as the binary does. + fn parse(extra: &[&str]) -> NodeOptions { let mut argv = vec![ "ethlambda", "--genesis", @@ -242,7 +242,8 @@ mod tests { "ethlambda_0", ]; argv.extend_from_slice(extra); - CliOptions::parse_from(argv) + let Command::Node(options) = try_parse_from(argv).expect("node options parse"); + options } /// `--discovery.enable` on its own has to work: a default that is never diff --git a/bin/ethlambda/src/command.rs b/bin/ethlambda/src/command.rs new file mode 100644 index 00000000..8209bc9a --- /dev/null +++ b/bin/ethlambda/src/command.rs @@ -0,0 +1,259 @@ +//! Sub-command definition and dispatch. +//! +//! `node` is an ordinary clap sub-command, so clap owns its help, usage lines +//! and error messages. The one thing clap cannot express is a *default* +//! sub-command, and the node needs one: the Dockerfile, +//! lean-quickstart, the hive shim and the devnet skills all invoke the binary as +//! a bare list of node flags, from before there was anything else to run. That +//! form keeps working because a missing sub-command is filled in as `node` +//! before parsing — see [`default_subcommand`]. + +use std::ffi::OsString; + +use clap::Parser; + +use crate::cli::NodeOptions; +use crate::version; + +/// Tokens that already say what to run, so no default is inserted ahead of +/// them. `help` is clap's own generated sub-command (`ethlambda help node`). +const EXPLICIT: &[&str] = &[NODE, "help", "-h", "--help", "-V", "--version"]; + +const NODE: &str = "node"; + +#[derive(Debug, clap::Parser)] +#[command( + name = "ethlambda", + author = "LambdaClass", + version = version::CLIENT_VERSION, + about = "ethlambda consensus client", + // `--version` used to sit on the node options, so it was accepted after + // node flags; `ethereum/hive` builds its image that way. Propagating it to + // the sub-commands keeps those invocations working. + propagate_version = true +)] +struct Cli { + #[command(subcommand)] + command: Command, +} + +/// What the command line asked the binary to do. +#[derive(Debug, clap::Subcommand)] +pub(crate) enum Command { + /// Run the consensus node (default when no sub-command is given). + /// + /// `ethlambda --genesis ...` and `ethlambda node --genesis ...` are the + /// same invocation. + // + // Deliberately not part of the doc comment above, which clap renders as + // this sub-command's `long_about`: `display_name` keeps `--version` + // printing `ethlambda ` after node flags, as it did when the node + // flags were the whole command line. It is still listed and invoked as + // `node`. + #[command(display_name = "ethlambda")] + Node(NodeOptions), +} + +/// Parse the process arguments, exiting the way clap does on a parse error, +/// `--help` or `--version`. +pub(crate) fn parse() -> Command { + try_parse_from(std::env::args_os()).unwrap_or_else(|err| err.exit()) +} + +pub(crate) fn try_parse_from(args: I) -> Result +where + I: IntoIterator, + I::Item: Into, +{ + let mut args: Vec = args.into_iter().map(Into::into).collect(); + if let Some(token) = default_subcommand(&args) { + args.insert(1, token.into()); + } + Cli::try_parse_from(args).map(|cli| cli.command) +} + +/// The sub-command to insert, if the arguments do not name one. +/// +/// `NodeOptions` declares no positional arguments, so the first token after +/// the program name is either a flag or a sub-command — a flag *value* never +/// lands there and is never mistaken for one. A leading flag therefore means +/// the flat node form, and gets `node` inserted ahead of it; a bare invocation +/// is left alone so clap prints its own "requires a subcommand" help. +fn default_subcommand(args: &[OsString]) -> Option<&'static str> { + let first = args.get(1)?.to_str()?; + (!EXPLICIT.contains(&first)).then_some(NODE) +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use clap::error::ErrorKind; + + use super::*; + + /// The flat invocation shape used by the Dockerfile, lean-quickstart, the + /// hive shim and the devnet skills. It must keep parsing unchanged. + const FLAT: &[&str] = &[ + "ethlambda", + "--genesis", + "config.yaml", + "--validators", + "annotated_validators.yaml", + "--bootnodes", + "nodes.yaml", + "--validator-config", + "validator-config.yaml", + "--hash-sig-keys-dir", + "hash-sig-keys/", + "--node-key", + "node.key", + "--node-id", + "ethlambda_0", + "--gossipsub-port", + "9001", + "--is-aggregator", + ]; + + /// `FLAT` with an explicit `node` sub-command token. + fn with_node_token() -> Vec<&'static str> { + let mut args = vec!["ethlambda", NODE]; + args.extend_from_slice(&FLAT[1..]); + args + } + + fn node_options(args: &[&str]) -> NodeOptions { + let Command::Node(options) = + try_parse_from(args.iter().map(OsString::from)).expect("invocation parses"); + options + } + + #[test] + fn flat_invocation_parses_unchanged() { + let options = node_options(FLAT); + assert_eq!(options.genesis, PathBuf::from("config.yaml")); + assert_eq!(options.hash_sig_keys_dir, PathBuf::from("hash-sig-keys/")); + assert_eq!(options.node_id, "ethlambda_0"); + assert_eq!(options.gossipsub_port, 9001); + assert!(options.is_aggregator); + } + + #[test] + fn node_sub_command_accepts_the_same_flags_as_the_flat_form() { + let flat = node_options(FLAT); + let scoped = node_options(&with_node_token()); + // Compared through `Debug`, which the derive prints field by field, + // because `NodeOptions` derives no `PartialEq` — and deriving one for + // a test would touch the parser this module deliberately leaves alone. + assert_eq!(format!("{flat:?}"), format!("{scoped:?}")); + } + + #[test] + fn a_flag_value_of_node_is_not_taken_for_the_sub_command() { + let mut args: Vec<&str> = FLAT.to_vec(); + let value = args + .iter() + .position(|arg| *arg == "ethlambda_0") + .expect("node id value present"); + args[value] = NODE; + assert_eq!(node_options(&args).node_id, NODE); + } + + #[test] + fn a_node_token_after_the_flags_is_still_rejected() { + // The default is inserted at the front or not at all, so a later token + // stays the stray positional argument it has always been. + let mut args: Vec<&str> = FLAT.to_vec(); + args.push(NODE); + let err = try_parse_from(args.iter().map(OsString::from)) + .expect_err("a trailing token must not be swallowed"); + assert_eq!(err.kind(), ErrorKind::UnknownArgument); + } + + #[test] + fn a_second_node_token_is_rejected_by_clap() { + let mut args = with_node_token(); + args.insert(1, NODE); + let err = try_parse_from(args.iter().map(OsString::from)) + .expect_err("only one sub-command is accepted"); + assert_eq!(err.kind(), ErrorKind::UnknownArgument); + } + + #[test] + fn missing_required_flag_keeps_the_clap_error_in_both_forms() { + // `--genesis config.yaml` dropped from the front of the flag list. + let flat: Vec<&str> = std::iter::once("ethlambda") + .chain(FLAT[3..].iter().copied()) + .collect(); + let mut scoped = vec!["ethlambda", NODE]; + scoped.extend_from_slice(&flat[1..]); + + for args in [flat, scoped] { + let err = try_parse_from(args.iter().map(OsString::from)) + .expect_err("a missing required flag must error"); + assert_eq!(err.kind(), ErrorKind::MissingRequiredArgument); + } + } + + #[test] + fn bare_invocation_asks_for_a_sub_command() { + // Nothing to default: clap prints the top-level help, which lists the + // sub-commands, rather than a missing-argument list for one of them. + let err = try_parse_from(["ethlambda"].iter().map(OsString::from)) + .expect_err("an argument-less invocation must not start a node"); + assert_eq!( + err.kind(), + ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand + ); + assert_ne!(err.exit_code(), 0, "a bare invocation must not exit 0"); + } + + #[test] + fn help_and_version_stay_top_level_flags() { + // `ethereum/hive` builds its ethlambda image by piping + // `ethlambda --version` into a file, with and without flags in front. + let mut version_after_flags: Vec<&str> = FLAT.to_vec(); + version_after_flags.push("--version"); + + for (args, expected) in [ + (vec!["ethlambda", "--help"], ErrorKind::DisplayHelp), + (vec!["ethlambda", "--version"], ErrorKind::DisplayVersion), + (version_after_flags, ErrorKind::DisplayVersion), + ] { + let err = try_parse_from(args.iter().map(OsString::from)) + .expect_err("help and version short-circuit parsing"); + assert_eq!(err.kind(), expected); + } + } + + #[test] + fn version_output_is_identical_for_every_form() { + // `--version` moved from the node options to the top-level command, so + // pin that it still prints one string: `ethereum/hive` records this + // output as the client version. + let mut after_flags: Vec<&str> = FLAT.to_vec(); + after_flags.push("--version"); + let printed: Vec = [ + vec!["ethlambda", "--version"], + vec!["ethlambda", NODE, "--version"], + after_flags, + ] + .into_iter() + .map(|args| { + try_parse_from(args.iter().map(OsString::from)) + .expect_err("--version short-circuits parsing") + .to_string() + }) + .collect(); + assert_eq!(printed[0], printed[1]); + assert_eq!(printed[0], printed[2]); + } + + #[test] + fn help_lists_the_sub_commands() { + // Listed by clap itself, because they are real sub-commands. + let err = try_parse_from(["ethlambda", "--help"].iter().map(OsString::from)) + .expect_err("--help short-circuits parsing"); + assert!(err.to_string().contains(NODE), "{err}"); + } +} diff --git a/bin/ethlambda/src/main.rs b/bin/ethlambda/src/main.rs index c0e83b15..a32ac594 100644 --- a/bin/ethlambda/src/main.rs +++ b/bin/ethlambda/src/main.rs @@ -1,5 +1,6 @@ mod checkpoint_sync; mod cli; +mod command; mod fd_limit; mod version; @@ -31,8 +32,8 @@ use std::{ }; use tokio_util::sync::CancellationToken; -use clap::Parser; -use cli::CliOptions; +use command::Command; + use ethlambda_blockchain::MILLISECONDS_PER_SLOT; use ethlambda_blockchain::block_builder::ProposerConfig; use ethlambda_blockchain::key_manager::ValidatorKeyPair; @@ -81,7 +82,7 @@ async fn main() -> eyre::Result<()> { tracing::subscriber::set_global_default(subscriber) .wrap_err("failed to set global tracing subscriber")?; - let options = CliOptions::parse(); + let Command::Node(options) = command::parse(); options.validate_discovery()?; #[cfg(feature = "shadow-integration")]