feat(cli): accept a `node` sub-command for running the node by pablodeymo · Pull Request #591 · lambdaclass/ethlambda

pablodeymo · 2026-08-26T16:05:46Z

🗒️ Description / Motivation

The binary has only ever run the node, so an invocation is a bare list of node flags.
The offline block-building benchmark adds a second entry point, which means the node
first needs a name of its own.

node is an ordinary clap sub-command on a top-level parser that owns the binary's name,
version and about. NodeOptions (renamed from CliOptions) becomes a plain clap::Args
group and keeps every field
exactly as it is — no Option<T>, no required = true, no unwrap helper on the node
path, which is what the review of #497 objected to.

The flat ethlambda --genesis ... form keeps working, because that is what the
Dockerfile, lean-quickstart, the hive shim and the devnet skills all pass. clap has no
default_subcommand, so exactly one thing sits in front of the parser: a command line
that names no sub-command gets node inserted.

What Changed

File	Change
`bin/ethlambda/src/command.rs`	New. Top-level `Cli` parser + `Command` sub-command enum, and `default_subcommand`, which inserts `node` unless the first token is a sub-command, `-h/--help/-V/--version`, or clap's generated `help`
`bin/ethlambda/src/cli.rs`	`clap::Parser` → `clap::Args`, and `CliOptions` renamed to `NodeOptions`; the `#[command(...)]` attribute moves to the top-level parser. No field changes
`bin/ethlambda/src/main.rs`	Parses through `command::parse()` and matches on `Command`

command.rs also carries a test-only parse_node_options helper. Merging main brought
#579's cli.rs tests, which called CliOptions::parse_from — a clap::Parser method the
group lost when it became clap::Args. Git merged both sides cleanly, so nothing flagged
it; the test build was broken until a3d7e52, and both test modules now parse a node
command line through the real dispatch.

Correctness / Behavior Guarantees

Every existing invocation keeps working, and clap owns everything a reader should
not have to trust us for: --help lists the sub-commands itself, usage lines name the
sub-command, and an unknown sub-command produces clap's error rather than a
stray-positional 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.
--version after node flags still works and still prints the same string. It used
to live on the node options, so it was accepted anywhere; propagate_version keeps that,
and display_name = "ethlambda" keeps the output byte-identical rather than
ethlambda-node. All three forms are asserted equal.
One deliberate change: a bare ethlambda now prints clap's top-level help, listing
the sub-commands, instead of a missing-argument list. It still exits non-zero, and the
test asserts both.

Tests Added / Run

Unit tests in command.rs pin: the flat parse; the two forms agreeing field for field; a
--node-id value that is literally node; a trailing node token still rejected; a
second node token rejected; missing required flags in both forms; the bare invocation's
error kind and non-zero exit; --help/--version staying top-level; --version printing
one identical string across all three forms; and --help listing the sub-commands.

make fmt, make lint, make test (574 tests, 30 suites) — all clean.

Related Issues / PRs

Replaces the CLI approach reviewed in the now-closed feat(cli): add offline block-building benchmark subcommand #497
Design doc in docs: document the block-building benchmark #594; the benchmark stacks on this in feat(cli): add offline block-building benchmark sub-command #595 → feat(cli): make benchmark reports comparable across runs and machines #596
Related to Block building optimizations #465

✅ Verification Checklist

Ran make fmt — clean
Ran make lint (clippy with -D warnings) — clean
Ran make test (cargo test --workspace --profile release-fast) — all passing

The binary has only ever run the node, so an invocation is a bare list of node flags. An upcoming offline block-building benchmark adds a second entry point, which means the node first needs a name of its own. `node` is the default sub-command: `ethlambda node --genesis ...` and the existing flat `ethlambda --genesis ...` both run the node. A leading `node` token is stripped before parsing and the same CliOptions parser then sees exactly the arguments it saw before, so for the flat form the help text, error messages, exit codes and --version are unchanged by construction rather than by convention. That form is what the Dockerfile, lean-quickstart, the hive shim and the devnet skills all use, and none of them has to move. Tests pin the flat parse, the two forms agreeing field for field, a --node-id value that is literally "node", a trailing `node` token still being rejected, missing required flags in both forms, the bare invocation, and --help/--version staying top-level flags.

Review follow-up on the sub-command module. The token is stripped before clap ever sees it, so `--help` advertised no sub-command at all and `node` was undiscoverable from the help output. It is now listed there, from a HELP_NOTE const this module owns and cli.rs only points at, and a test pins it. Invocation had a single variant, so it bought nothing that returning CliOptions does not: main.rs destructured it irrefutably, and a second variant would force that line to become a match either way. Gone until there is a second entry point to name. Also pin that a *second* `node` token is left for clap to reject like any other stray positional, and record why the two parses are compared through Debug rather than PartialEq.

github-actions · 2026-08-26T16:25:44Z

🤖 Kimi Code Review

The diff is clean, well-tested, and correctly handles the sub-command dispatch logic. No critical issues found.

Minor suggestions:

bin/ethlambda/src/main.rs:83

let Invocation::Node(options) = command::parse();

While irrefutable today (only one enum variant), consider using if let or adding an else with unreachable!() to make the code resilient to future Invocation variants. This ensures a compile-time reminder if new subcommands are added.

bin/ethlambda/src/command.rs:28-30
The logic relies on the invariant that CliOptions declares no positional arguments. Consider adding a compile-time or runtime assertion in cli.rs to guard against accidental addition of positional arguments, which would break the sub-command detection logic at line 48.

Style nit:
Line 49 uses Vec::remove(1) which is O(n) for the remaining args. For CLI argument counts this is negligible, but if you prefer to avoid the shift, you could use args.into_iter().enumerate().filter(|(i, arg)| !(*i == 1 && arg == NODE)).map(|(_, arg)| arg).

Acknowledgments:

Good test coverage for edge cases (flag value vs subcommand, trailing tokens, missing required flags).
Proper handling of OsString for cross-platform compatibility.
Clear documentation explaining why the token removal approach preserves backward compatibility.

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

github-actions · 2026-08-26T16:26:15Z

🤖 Codex Code Review

No findings.

This PR is CLI-only and does not touch fork choice, attestation handling, state transition, XMSS, or SSZ paths. The node dispatch is narrowly scoped: it only strips a leading node token before handing argv to the existing CliOptions parser, so the legacy flat invocation remains on the same codepath (bin/ethlambda/src/command.rs, bin/ethlambda/src/main.rs). The added tests cover the main regression cases around flat-vs-explicit invocation, preserving missing-required-flag errors, and not swallowing node when it appears as a later value or stray token (bin/ethlambda/src/command.rs).

Residual risk is low and mostly around CLI UX, not consensus correctness. I could not run cargo test in this sandbox because cargo/rustup need write access under /home/runner, which is read-only here, so this review is based on static inspection rather than executed tests.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

github-actions · 2026-08-26T16:27:12Z

🤖 Claude Code Review

Review Summary

The core mechanism in bin/ethlambda/src/command.rs is well-designed and correct:

try_parse_from correctly strips a leading node token only at index 1 (right after the program name), leaving the rest of the argument list untouched for CliOptions::try_parse_from. This achieves the low-churn goal — the existing parser's behavior, error messages, and exit codes are genuinely unchanged for the flat invocation form.
The is_some_and(|arg| arg == NODE) + single remove(1) correctly handles the edge cases the tests target: a --node-id node value isn't mistaken for the sub-command (it's at a different index), a trailing node isn't swallowed, and only the first of two leading node tokens is stripped.
main.rs's let Invocation::Node(options) = command::parse(); is fine as an irrefutable pattern given Invocation currently has exactly one variant, and cleanly sets up room for the offline block-building sub-command from the follow-up PR.
Test coverage is thorough for a CLI parsing shim: flat vs. scoped equivalence, value-vs-subcommand disambiguation, trailing/duplicate node, missing required flags, bare invocation, and --help/--version short-circuiting.

One confirmed discrepancy (reported via ReportFindings): the PR description explicitly claims cli.rs gains after_help = crate::command::HELP_NOTE so the sub-command remains discoverable via --help, and that command.rs "owns the HELP_NOTE." Neither exists in the actual diff (only command.rs and main.rs are touched; no HELP_NOTE symbol appears in the diff or in the resulting cli.rs). As merged, ethlambda --help gives no indication that ethlambda node ... is a supported invocation — this directly undercuts the PR's own stated correctness rationale.

No security, memory-safety, or consensus-layer concerns — this PR is pure CLI plumbing and doesn't touch fork choice, attestations, state transition, or signature code.

Automated review by Claude (Anthropic) · sonnet · custom prompt

MegaRedHand · 2026-08-26T20:00:27Z

+    author = "LambdaClass",
+    version = version::CLIENT_VERSION,
+    about = "ethlambda consensus client",
+    after_help = crate::command::HELP_NOTE


I don't like how we're essentially hacking our way around clap here. We should find a way to use clap's subcommands for this: https://docs.rs/clap/latest/clap/trait.Subcommand.html

You're right, and the argv surgery is gone. node is a real Subcommand now.

CliOptions became a plain clap::Args group and keeps every field exactly as it was — no Option<T>, no required = true, no unwrap on the node path — with the binary's name/version/about moved to a top-level parser holding #[command(subcommand)]. So clap owns the parts we were hand-rolling: --help lists the sub-commands itself (the hand-written after_help note is deleted), usage lines name the sub-command, and an unknown sub-command gets clap's own error instead of a stray-positional one.

The one thing I could not get from clap is a default sub-command. There is no default_subcommand, and the flat ethlambda --genesis … form has to keep working — the Dockerfile, lean-quickstart, the hive shim and the devnet skills all pass exactly that. So a single check remains in front of the parser: if the first token names no sub-command and is not -h/--help/-V/--version or clap's generated help, node is inserted. That is the whole of it, four lines in default_subcommand.

Two behaviour changes fell out of moving the parser identity up a level, both pinned by tests:

--version used to sit on CliOptions, so it was accepted after node flags. propagate_version keeps that working, and display_name = "ethlambda" keeps the printed string byte-identical instead of ethlambda-node; a test asserts all three forms produce the same output.

A bare ethlambda now prints clap's top-level help listing the sub-commands, rather than a missing-argument list for the node. Still exits non-zero.

Worth saying plainly: this is longer than the token stripping it replaces — a sub-command enum plus a top-level parser costs more lines than a Vec::remove — but the parts a reader has to trust are clap's now instead of ours.

If you would rather have no argv handling whatsoever, the alternative is dropping the flat form and requiring ethlambda node … everywhere. That is genuinely clean, but it breaks every running devnet and needs a lean-quickstart change, so I did not take it on unilaterally — say the word and I will.

Unrelated, so it does not mislead anyone reading down the thread: the Claude review bot above reports after_help/HELP_NOTE as missing from the diff. That was true of the commit it ran on, not of the branch head — its run predates a force-push. Moot either way now, since clap lists the sub-commands on its own and HELP_NOTE is deleted.

MegaRedHand

.

The previous version removed a leading `node` token from argv and handed the rest to CliOptions, so clap never knew a sub-command existed: `--help` could not list it (a hand-written HELP_NOTE had to), usage lines could not name it, and an unknown sub-command produced a stray-positional error instead of clap's own. `node` is now an ordinary clap sub-command on a top-level parser that owns the binary's name, version and about. CliOptions becomes a plain `clap::Args` group and keeps every field exactly as it is — no `Option<T>`, no `required = true`, no unwrap on the node path. What argv manipulation remains is one thing clap cannot express: a default sub-command. The Dockerfile, lean-quickstart, the hive shim and the devnet skills all invoke the binary as a bare list of node flags, so a command line that names no sub-command gets `node` inserted. A bare invocation is left alone, and so are `-h/--help/-V/--version` and clap's generated `help`, which is the whole of the special-casing. Two behaviours needed care to keep: - `--version` used to live on the node options, so it was accepted after node flags. `propagate_version` keeps that working, and `display_name = "ethlambda"` keeps the printed string identical rather than `ethlambda-node`; a test pins all three forms against each other. - A bare invocation now prints clap's top-level help (listing the sub-commands) instead of a missing-argument list, still exiting non-zero. The test asserts both. This is longer in lines than the token stripping it replaces — a sub-command enum and a top-level parser cost more than a `Vec::remove` — but the parts a reader must trust are now clap's, not ours.

Merging main brought #579's cli.rs tests, which call `CliOptions::parse_from`. `CliOptions` has been a `clap::Args` group since `node` became a sub-command, so that constructor no longer exists and the test build stopped compiling. Git merged both sides cleanly — the conflict is semantic, so nothing flagged it. The tests now go through `command::parse_node_options`, a single test-only helper that runs the real dispatch and hands back the node options. The command.rs tests use it too, so there is one way to parse a node command line in tests rather than two.

The struct is the `node` sub-command's option group now, not the whole CLI — the CLI is the top-level `Cli` parser. Rename only; no field or attribute changes. Also takes the review's wording for the sub-command's help line: "default when no sub-command is given" rather than "assumed".

`parse_node_options` existed only so two test modules could destructure the `node` command, which is one line each. `try_parse_from` becomes `pub(crate)` — it is already the testable core that `parse()` wraps — and both callers destructure inline, so there is no test-only function in the module's surface.

…ass#595) ## 🗒️ Description / Motivation Adds `ethlambda benchmark synthetic` — an offline harness that measures block building **exactly as executed when the node proposes**, against a reproducible synthetic workload, with no devnet required. Second of three (design doc → **this** → comparable reports). What lands here is the smallest thing that runs: the real proposer path, driven deterministically, reporting one row per measured iteration. Aggregate statistics, build provenance and machine-readable output follow in the next PR, so this one can be reviewed for *what it measures* rather than how it formats. > **Stacked on lambdaclass#591**, which adds the `node`/`benchmark` token dispatch this uses. Review > that first; the base moves to `main` once it lands. ``` $ make bench Block-building benchmark — synthetic workload (mock crypto) validators=8 warmup_slots=8 iterations=10 proofs_per_data=1 seed=42 enable_proposer_aggregation=false max_attestations_per_block=3 ethlambda/v0.1.0/aarch64-apple-darwin/rustc-v1.97.1 os=macos arch=aarch64 threads=14 iter compact select_payloads stf_simulate overhead wall root 1 0.000ms 0.002ms 0.015ms 0.068ms 0.085ms 0x7282cc99 2 0.000ms 0.002ms 0.015ms 0.066ms 0.083ms 0xb9065af0 3 0.000ms 0.002ms 0.015ms 0.064ms 0.081ms 0x303f6b0f ``` ## How to read this In this order — each piece is understandable without the next: 1. **`corpus.rs`** — the workload. Deterministic validators, a genesis store over `InMemoryBackend`, and `seed_pool`, which fills the pending pool for one slot and reports how many entries the next build will see. 2. **`build_one_slot`** (`mod.rs`) — one slot end to end: seed, time the build, import the block. Warmup and measured slots run this same path; only whether the sample is kept differs, so there is no "am I warming up?" branching inside. 3. **`PhaseTimer`** (`mod.rs`) — `start()` before the build, `finish()` after. Two readings of the existing phase histogram; the difference between their sample *sums* is the build's phase time, so nothing is added to the hot path. 4. **`run_synthetic`** (`mod.rs`) — validate, set up, loop over slots, report. 46 lines. 5. **`report.rs`** — the types and the per-iteration table. ## What Changed | File | Change | |------|--------| | `bin/ethlambda/src/benchmark/mod.rs` | Harness driver: clap options + validation, `run_synthetic`'s slot loop, `build_one_slot` for one slot's work, and `PhaseTimer` for per-phase attribution | | `bin/ethlambda/src/benchmark/corpus.rs` | Seeded synthetic corpus: genesis store over `InMemoryBackend`, deterministic pubkeys via splitmix64, and per-slot pool seeding in fixed insertion order, which also rejects a batch the pool would evict whole | | `bin/ethlambda/src/benchmark/report.rs` | Params/Environment/Sample types and the human-readable per-iteration table | | `bin/ethlambda/src/command.rs` | `benchmark` joins `node` as a second clap sub-command, so clap lists it in `--help` and names it in its own usage lines. The node payload becomes `Box<CliOptions>` now that a much smaller variant sits beside it | | `bin/ethlambda/src/main.rs` | `main` becomes synchronous and dispatches; only the node path enters the tokio runtime (`run_node` carries the `#[tokio::main]` attributes). Benchmark logs go to stderr at WARN so the report owns stdout | | `crates/storage/{lib,store}.rs` | Export `NEW_PAYLOAD_CAP` so the harness rejects a `--proofs-per-data` batch the pending pool would evict whole | | `Makefile` | `make bench` (override `BENCH_ARGS` to customize) | ## Correctness / Behavior Guarantees - **`cli.rs` is not touched by this PR and the node runtime is unchanged.** The harness arguments live in their own `Args` group; the node's stay plain `PathBuf`/`String`, so clap keeps emitting its own missing-argument errors. - **It measures the production path**, not a copy: the harness enters through `produce_block_with_signatures`, the same function `BlockChainServer::propose_block` calls, and seeds the *pending* pool so the proposal tick promotes it exactly as on a live node. - **Determinism:** same seed + params → identical per-iteration block roots. Verified across repeated runs; the roots are printed so a baseline-vs-optimized diff proves an optimization changed only speed, not attestation selection. The harness never reads the wall clock into results. - **Exact phase attribution with zero hot-path changes:** per-iteration `select_payloads`/`compact`/`stf_simulate` come from the sample sums of the existing `lean_block_proposal_attestation_build_phase_seconds` histogram, deltaed between iterations, with a per-phase assertion that the count advanced by exactly one. `overhead` is the clamped remainder of wall minus the phases. - The benchmark never starts the tokio runtime, so it cannot park a worker thread for the duration of a CPU-bound run. ## Tests Added / Run - `corpus.rs`: participant groups partition every validator; synthetic pubkeys are deterministic for a seed. - `command.rs`: the benchmark token parses with no node argument, rejects node flags, and its usage line names the sub-command. - Verified by hand: `make bench`; identical block-root sequences across two runs at the same seed; `ethlambda --genesis config.yaml` still failing with clap's own missing-argument list. - `make fmt`, `make lint`, `make test` (576 tests, 30 suites) — all clean. ## Related Issues / PRs - Stacked on lambdaclass#591; design doc in the accompanying docs PR - Followed by the comparable-reports PR - Splits the now-closed lambdaclass#497 / lambdaclass#593 - Related to lambdaclass#465 ## ✅ Verification Checklist - [x] Ran `make fmt` — clean - [x] Ran `make lint` (clippy with `-D warnings`) — clean - [x] Ran `make test` (`cargo test --workspace --profile release-fast`) — all passing

pablodeymo added 2 commits August 26, 2026 13:01

MegaRedHand approved these changes Aug 26, 2026

View reviewed changes

MegaRedHand requested changes Aug 26, 2026

View reviewed changes

pablodeymo added 2 commits August 26, 2026 17:17

Merge branch 'main' into feat/cli-node-subcommand

4fb7fe1

MegaRedHand approved these changes Aug 26, 2026

View reviewed changes

Comment thread bin/ethlambda/src/command.rs Outdated

Comment thread bin/ethlambda/src/cli.rs Outdated

pablodeymo added 2 commits August 26, 2026 18:49

MegaRedHand reviewed Aug 26, 2026

View reviewed changes

Comment thread bin/ethlambda/src/command.rs Outdated

MegaRedHand approved these changes Aug 26, 2026

View reviewed changes

MegaRedHand enabled auto-merge August 26, 2026 22:42

Merge branch 'main' into feat/cli-node-subcommand

d0e38ef

MegaRedHand added this pull request to the merge queue Aug 26, 2026

Merged via the queue into main with commit d54044c Aug 26, 2026
2 checks passed

MegaRedHand deleted the feat/cli-node-subcommand branch August 26, 2026 23:08

Navigation Menu

Uh oh!

feat(cli): accept a `node` sub-command for running the node - #591

feat(cli): accept a `node` sub-command for running the node#591
MegaRedHand merged 8 commits into
mainfrom
feat/cli-node-subcommand

pablodeymo commented Aug 26, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 26, 2026

Uh oh!

github-actions Bot commented Aug 26, 2026

Uh oh!

github-actions Bot commented Aug 26, 2026

Uh oh!

MegaRedHand Aug 26, 2026

Uh oh!

pablodeymo Aug 26, 2026

Uh oh!

MegaRedHand left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

pablodeymo commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🗒️ Description / Motivation

What Changed

Correctness / Behavior Guarantees

Tests Added / Run

Related Issues / PRs

✅ Verification Checklist

Uh oh!

github-actions Bot commented Aug 26, 2026

🤖 Kimi Code Review

Uh oh!

github-actions Bot commented Aug 26, 2026

🤖 Codex Code Review

Uh oh!

github-actions Bot commented Aug 26, 2026

🤖 Claude Code Review

Review Summary

Uh oh!

MegaRedHand Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

pablodeymo Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

MegaRedHand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pablodeymo commented Aug 26, 2026 •

edited

Loading