Repository navigation
feat(cli): accept a node sub-command for running the node - #591
Merged
Merged
Conversation
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.
🤖 Kimi Code ReviewThe 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 bin/ethlambda/src/command.rs:28-30 Style nit: Acknowledgments:
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewNo findings. This PR is CLI-only and does not touch fork choice, attestation handling, state transition, XMSS, or SSZ paths. The Residual risk is low and mostly around CLI UX, not consensus correctness. I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewReview SummaryThe core mechanism in
One confirmed discrepancy (reported via ReportFindings): the PR description explicitly claims 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 |
This was referenced Aug 26, 2026
MegaRedHand
approved these changes
Aug 26, 2026
| author = "LambdaClass", | ||
| version = version::CLIENT_VERSION, | ||
| about = "ethlambda consensus client", | ||
| after_help = crate::command::HELP_NOTE |
Collaborator
There was a problem hiding this comment.
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
Collaborator
Author
There was a problem hiding this comment.
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:
--versionused to sit onCliOptions, so it was accepted after node flags.propagate_versionkeeps that working, anddisplay_name = "ethlambda"keeps the printed string byte-identical instead ofethlambda-node; a test asserts all three forms produce the same output.- A bare
ethlambdanow 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.
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.
MegaRedHand
approved these changes
Aug 26, 2026
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".
MegaRedHand
reviewed
Aug 26, 2026
`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.
MegaRedHand
approved these changes
Aug 26, 2026
MegaRedHand
enabled auto-merge
August 26, 2026 22:42
Sahilgill24
pushed a commit
to Sahilgill24/ethlambda
that referenced
this pull request
Sep 1, 2026
…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
tcoratger
pushed a commit
to tcoratger/ethlambda
that referenced
this pull request
Oct 30, 2026
* docs: design the unified lean/beacon storage layer
The beacon fork choice holds its whole world in memory, including three
unbounded maps of ~350 MB beacon states, so a mainnet follower can
neither survive a restart nor bound its memory. This designs the move
onto the DB-backed store the lean chain already runs on: one set of
methods over one block enum, one runtime config persisted for both
chains, and a byte-domain state delta that makes recovering an arbitrary
beacon state bounded.
The diff layer is the one deliberate exception to sharing. Lean's
field-shaped StateDiff depends on validators never changing, which a
beacon registry breaks every epoch, and it is the better algorithm for
lean, so beacon gets xdelta3 byte deltas beside it rather than instead
of it.
Working artifacts, not reference docs: both are removed once the work
lands, the way the beacon STF design document was.
* build: check debug assertions under the test profile
Tests run under release-fast, which inherits release and therefore had
debug assertions off, so a debug_assert! in any crate ran nowhere. A dev
build is not a fallback: signature verification and aggregation
stack-overflow without release-grade opt-level.
overflow-checks is pinned rather than inherited so arithmetic behaviour
stays identical to release.
Verified both directions: debug_assertions appears in `cargo rustc
--profile release-fast -- --print cfg` and not under release.
* test(types): pin the genesis state root
`State.config` is hashed into every lean state root and is the field
leanSpec declares as `config: GenesisConfig`. Naming it after the spec was
this commit's original job, and main has since done exactly that under the
name `StateConfig`, keeping `ChainConfig` for the node's runtime
configuration: a different type for a different job.
What is left to add is the guard the rename came with. This pins the
genesis state root, so a field reorder, an added field, or a type change
disguised as a rename fails here rather than in cross-client interop.
* feat(types): let the runtime config persist, and give lean one
Config becomes the store-level configuration for both chains, so it needs
an encoding it did not have and a genesis_time it did not carry.
min_genesis_time could not serve as the clock: it is the earliest the
deposit-driven rules permit, 23 seconds before mainnet actually started.
Config::lean fills every beacon field with a placeholder chosen to fail
closed, FAR_FUTURE_EPOCH for every fork epoch, so a beacon-shaped gate
reading a lean config never sees an activated fork where epoch 0 would
have activated all seven.
blob_schedule becomes a bounded SSZ list because SSZ has no unbounded
one; the bound is a storage limit, which is what the database format
version exists to let us change.
* feat(storage): tag a data directory with its chain and format version
A directory is one chain for its whole life and one on-disk format, and
neither is recoverable when wrong: there is no migration, because the
States value layout is about to change and every state already written
would decode as the wrong shape.
Both checks come after the "has this ever held a chain" reads, so a fresh
directory is still Ok(None) rather than a version mismatch.
Also folds four Store struct literals into one constructor, which is how
the next few fields avoid drifting between the bootstrap paths.
* feat(storage): hold the runtime config, not the in-state one
Metadata["config"] now holds the unified runtime Config for both chains
rather than the two-field container that lives inside a lean State. The
in-state one is still what a state root commits to; this is what the node
runs on, and a beacon directory needs the fork schedule in it.
Returned behind an Arc because every beacon fork-choice call takes
&mut Store alongside the config, so a caller has to hold it across a
mutable borrow of the store it came from.
The config row's layout changes without a DB_VERSION bump on purpose:
the version was introduced one commit ago on this same branch and has
never shipped, so there is no released format to be compatible with.
* feat(types): give the block enum a lean variant
BeaconState already carries Lean; blocks now do too, so the storage layer
can take one block type and split inside its methods instead of growing a
method per chain.
The accessors are macro-generated, so the arm lands in the macros: the
existing message:/outer: split turns out to be exactly the lean boundary,
since lean's Block declares all four message fields under the same names
and has no BLS signature to answer the one outer accessor with.
The enum is not renamed. It names 156 sites across 19 files, eight of
them the concrete per-fork structs that share the identifier, and
BeaconState already carries Lean under a beacon-shaped name.
Adding the variant also required PartialEq on SignedBlock, Block,
BlockBody and AggregatedAttestation, since SignedBeaconBlock derives it
and now has to compare a lean payload too; and a lean_block_unreachable
boundary in ethlambda-state-transition's beacon module, mirroring the one
ethlambda-types already has, since the crate's own beacon process_block
dispatcher matched the enum exhaustively.
* docs: withdraw the block-enum rename, and record how the Lean arm lands
Two things the design got wrong about blocks, found while sizing the work.
The rename to SignedBlock is withdrawn. SignedBeaconBlock names 156 sites
across 19 files and eight of those are the concrete per-fork structs that
share the identifier, so a rename is a large edit with a real corruption
mode and no functional gain. BeaconState already carries Lean under a
beacon-shaped name, which is the precedent that makes the rename
unnecessary rather than merely risky.
The plan also assumed the block accessors were hand-written match arms.
They are macro-generated, and there is a fifth accessor the plan never
accounted for: signature, returning a BlsSignature that a lean block has
no equivalent of. The state side already solved this with a pair of
dispatch macros, one panicking on Lean and one running every arm, so
blocks mirror it. The happy part is that the existing message:/outer:
split in the accessor macro turns out to be exactly the lean boundary,
since lean's Block declares all four message fields under the same names
and cannot answer the one outer accessor.
Also corrects the claim that current_slot() reads genesis_time. It
divides the store clock by INTERVALS_PER_SLOT and never reads the config,
so it is lean-shaped as written and the beacon path needs its own slot
derivation from the Unix clock.
* feat(storage): tag States values with the fork that shapes them
One table now has to hold a lean State and seven beacon shapes, and SSZ
carries no type tag, so the reader cannot recover the shape from the
bytes. A leading ForkName::selector byte is what makes one table serve
both without the caller having to already know which chain it opened.
Safe to change the value layout only because the format version and
chain tag landed first: an older directory is rejected outright rather
than misdecoded, and there is deliberately no migration.
The selector is not a variant index, so it always goes back through
from_selector: Lean takes 255 so beacon forks after fulu can keep taking
the next free value.
* feat(storage): the beacon fork-choice table and its scratch
Splits the beacon fork-choice store's fields by whether they are worth
persisting. Only the unrealized justification is: get_voting_source reads
it for every block from a prior epoch, and recomputing an entry means
replaying epoch processing on a copy of that block's post-state, so it
gets the ninth table.
The rest are per-slot or per-epoch scratch and go in memory. Proposer
boost resets every slot, equivocators come back from replaying attester
slashings, latest messages from one epoch of attestations, and PoW blocks
stand in for an execution-client call a restarted node makes again.
The latest-message read takes a closure because the map is behind a
mutex, and it filters equivocators there because get_weight has to
exclude such a vote entirely rather than let it count for either side of
the fork it created.
* feat(storage): one block insert for both chains
insert_signed_block takes the fork-generic enum and splits inside it,
with both arms spelled out rather than delegating to a helper per chain:
the two write different tables, and that difference is what a reader
needs to see. Because a beacon Root is already H256 there is one key type
and one value codec, so nothing here needs a bridging generic.
BlockRoots and BlockProof stay unwritten on the beacon arm: a slot holds
several blocks once a fork exists, so only the parent links say which is
canonical, and a range request walks those from the fork-choice head.
The lean arm keeps its post-commit attestation-vote recording, which the
beacon arm has nothing to do: a beacon block carries no lean
attestations, so the match hands that decision back out rather than
running it unconditionally.
* feat(storage): the beacon checkpoints, head, and an empty beacon directory
Both bootstrap paths write every metadata key they will later read, so an
absent key is a bug reaching the wrong chain's accessor rather than a
condition a caller could recover from. The reader now panics naming the
key and the chain, which is what says which mistake was made, and the
helpers stop returning a Result they could never populate.
Beacon's accessors return values directly: their callers' error type
lives in ethlambda-types and cannot carry a storage error, so a Result
here would buy a map_err at every call site and nothing else.
The fork-choice head is the one row deliberately left unseeded, since
absent means no head has been computed yet, which is a real state.
Metadata["time"] is shared between the chains with different units, which
is safe exactly because a directory holds one chain for its whole life.
from_db_state's existence gate previously relied on KEY_LATEST_FINALIZED,
a lean-only key init_beacon never writes; a fully-formed beacon directory
would otherwise read as empty and let the lean bootstrap write into it.
The gate now consults the chain tag first.
* feat(storage): read a block back as the fork it was written as
insert_signed_block already took the fork-generic enum, but the read side
still returned the lean type, so a beacon block could be written and
never read back. The fork choice migration needs to read blocks, so the
asymmetry had to go.
Dispatches on the cached chain tag rather than trial-decoding: a data
directory holds one chain for its whole life, so the tag is authoritative
and guessing would be wrong as well as slower.
The lean arm keeps the two synthesis behaviours its callers depend on,
the empty proof for the slot-0 anchor and the empty body for a header
whose body_root says there is none.
* feat(storage): persist and read a beacon post-state
The fork choice migration has post-states to keep, so insert_state and
get_state take the fork-generic state type and split inside, the way the
block methods already do.
The beacon arm writes a full snapshot per block on purpose. That is
wasteful at mainnet scale and is a placeholder: the delta layer and the
bounded cache land in later tasks, and at the fixture presets a
standalone snapshot per block is entirely adequate. Beacon deliberately
does not borrow lean's StateDiff, which omits validators on the
assumption they never change; a beacon registry changes every epoch.
The lean arm is unchanged: same snapshot interval, same diff chain, same
reconstruction, same cache.
* feat(beacon): put the fork choice on the DB-backed store
The spec-shaped in-memory Store held three unbounded maps of whole
beacon states, so a mainnet follower could neither survive a restart nor
bound its memory. Its fields move to the storage layer the lean chain
already runs on: blocks to the block tables, checkpoints and the clock to
Metadata, unrealized justifications to their own table, and the per-slot
scratch to an in-memory struct that is cheap to rebuild.
The children scan takes a block index built once per tree walk rather
than a lookup per hop, since get_weight calls get_ancestor once per
active validator.
checkpoint_state is derived on every call for now; a later task restores
a bounded cache. That also removes the file's only outright whole-state
clone, which existed because two maps each needed their own copy of the
anchor.
The fixture suite passes with an unchanged case count at both presets.
* feat(beacon): record the head fork choice computed, and narrow the rest
set_beacon_head existed with tests but had no caller, so a restarted
node had to recompute a whole weighted tree walk before it could answer
what its head was. get_head now writes the head it just computed, every
time rather than only on change: a value written once and left alone is
a second source of truth a bug can let drift.
That also earns get_head's &mut Store. The six read-only helpers around
it go back to &Store: checkpoint_state was given &mut on the theory that
a future cache would need it, but the cache accessors are &self by
design and use interior mutability precisely so a read-only helper can
record a derived value. The module's invariant is restored, so which
functions can mutate consensus state is visible again at a glance.
Fixture suite unchanged at both presets.
* feat(storage): byte-domain state deltas for the beacon chain
VCDIFF rather than xor-plus-compress: an SSZ list that grows shifts every
offset after it, which an xor delta cannot survive and a COPY/ADD stream
handles natively. That is what lets the three big arrays stay untouched
instead of needing length-aware handling of their own.
Beacon needs its own algorithm because lean's StateDiff omits validators
on the assumption they never change, which a beacon registry breaks every
epoch. Lean keeps its own, which is smaller and cheaper there and keeps a
C dependency off the path every devnet runs.
Sized through the *_with_output_len variants rather than the crate's
convenience wrappers, which ask for (input + src) * 2: at beacon scale
that is a ~1.4 GB transient per call and the u32 cast wraps above a
~1.07 GB source. The frame carries the target length so a decode can size
its output exactly.
default-features = false because the crate's stream feature does not
compile at this rev; the one-shot API used here is outside that module.
* docs: replace the delta layer's guessed costs with measured ones
A standalone probe of the codec, before wiring it in, answered two of
this design's open questions and corrected a third.
The dependency does not build as specified: its default `stream` feature
pulls an async module that fails to compile at the pinned rev, so
`default-features = false` is mandatory rather than tidy. The one-shot
in-memory API this design uses is outside that module.
The source-window risk is deleted. xdelta3's 64 MiB window belongs to its
streaming API; this crate wraps the one-shot memory API, which takes the
whole source as one buffer. A one-byte change 79 MB into an 80 MB base
still yields a tiny delta.
And the cost estimates were pessimistic by roughly eight times. Measured
on a 64 MiB buffer with three quarters of its bytes rewritten to
index-derived values: delta 1 MiB, encode 229 ms, decode 6 ms. Scaled to
a mainnet state that puts encode near 1.3 s, inside a slot, and a
31-delta fold near one second rather than eight. The epoch-sized
snapshot interval survives.
The benchmark task still earns its place: the extrapolation is linear and
xdelta3's hash table need not be, and the probe's change pattern is more
structured than a real reward distribution, so it flatters the delta size
more than the timings.
* feat(storage): wire the byte-domain delta codec into beacon state storage
A mainnet BeaconState is on the order of 350 MB, so a snapshot per block
(the placeholder insert_state/get_state shipped with) is not viable past
the minimal preset the fixture suites run against. insert_state now
snapshots only at fork-scoped intervals (one epoch for beacon, unchanged
for lean) and diffs everything in between with the byte-domain VCDIFF
codec added in the previous commit; get_state folds the resulting delta
chain in the byte domain and SSZ-decodes exactly once, at the end, since
folding bytes is memcpy-speed while a mainnet-scale SSZ decode is not.
ForkName::snapshot_interval centralizes the interval so lean's storage
layer and the new beacon path share one source of truth instead of each
picking its own constant.
* refactor(storage): one bounded state cache for both chains
The beacon fork choice used to hold whole states in maps with no cap,
which is not a tuning question at a few hundred megabytes each. It now
shares the LRU the lean chain already had, so one capacity bounds
everything.
Behind an Arc because a hit must not copy: returning an owned state would
hand back much of what the cache saves, and the fixture presets are too
small for a test to notice.
Block and checkpoint states share one bound through a key enum rather
than getting a map each, since two separate caps overshoot the total. A
checkpoint is keyed by epoch as well as root, because a checkpoint's root
is the last block at or before its boundary slot and the same root can
serve different epochs.
The accessors take &self so the read-only fork-choice helpers can record
what they derive on a miss; that is what lets checkpoint_state stay
&Store instead of widening its callers.
* test(storage): measure beacon delta cost on a real mainnet-scale state
The design's encode-cost risk was marked "largely retired by measurement"
on a synthetic 64 MiB probe whose 75%-rewritten, single-pass change
pattern is far more structured than a real epoch transition, and whose
timings were extrapolated linearly past a hash-table codec that need not
scale that way. Both assumptions mattered: measuring a real 2,000,000-
validator electra state (280.7 MB) shows encode cost tracks how
fragmented a change is, not just buffer size.
One ordinary slot: 924 B delta, 375 ms encode, 26 ms decode, comfortably
inside a 4 s slot. Across an epoch boundary: 26.0 MB delta, 3.04 s
encode, 111 ms decode, roughly 2.3x the earlier linear guess and three
quarters of a slot on its own. The epoch-sized snapshot interval still
survives, but only because `is_anchor` already routes the expensive
shape around xdelta3 entirely (always a full snapshot at that boundary,
never a delta): its safety is contingent on staying epoch-aligned, not
on the codec being fast for any shape.
Recorded in the design doc alongside the probe for contrast.
* docs: price the delta measurements against a beacon slot, not a lean one
The measurement section compared both encode times to a 4 second slot,
which is the lean chain's slot time. Mainnet's seconds_per_slot is 12, so
the ordinary-slot delta is about 3% of a slot rather than "most of it
free", and the epoch-boundary shape is about a quarter of a slot rather
than three quarters.
The conclusion is unchanged, and so is the reason: the epoch-transition
shape never reaches the codec, because is_anchor writes that state as a
raw snapshot. But a quarter of a slot and three quarters read very
differently to whoever has to decide whether the contingency is
acceptable, so the arithmetic is worth getting right.
* chore: remove implementation plan
* refactor(storage): the unrealized justification is scratch, not a table
`get_voting_source` reads a block's unrealized justification for every
block from a prior epoch, and recomputing a missing entry means replaying
epoch processing on a copy of that block's post-state. That is what earned
it a column family of its own, but it does not make the map chain history:
no beacon path resumes from a data directory today, and the one that
eventually will re-imports the unfinalized window from its anchor, refilling
the map as it goes.
So move it into `BeaconScratch` beside the other beacon fork-choice maps and
drop `Table::BeaconForkChoice`, putting the table count back at eight. The
accessors keep their signatures, so fork choice is untouched; a read is now a
mutex rather than a backend read plus an SSZ decode, and a write no longer
commits a batch per block.
The struct's bound claim changes with it. `block_timeliness` and the
justifications are both one entry per block imported and neither is pruned,
so "bounded by the unfinalized window" was already loose; say what is
actually true of each map instead.
A data directory written by an earlier commit of this branch will no longer
open, since RocksDB requires every on-disk column family to be listed.
Nothing outside this branch ever had the table, so no lean directory is
affected.
* refactor(types): peel a lean value off the shared enum in one place
`Store::get_state` and `get_signed_block` hand back the fork-generic
`BeaconState`/`SignedBeaconBlock` for both chains, so every lean-only
caller narrowed the result again by hand: thirteen copies of
`let BeaconState::Lean(x) = y else { unreachable!(..) }` across five
crates, each carrying its own wording for the same impossibility, and one
that logged and skipped rather than panicking at all.
Give both enums an `expect_lean` accessor and put its panic behind
`beacon_value_unreachable`, beside the `lean_state_unreachable` pair that
already answers the opposite direction. The invariant that a data
directory holds one chain for its whole life is now asserted in one place
instead of thirteen, and a new lean caller cannot word it differently or
weaken it into a silent skip.
* refactor(storage): one head and checkpoint writer for both chains
The beacon arm kept its own checkpoint rows because the two chains'
`Checkpoint` types carry different fields, lean's `{root, slot}` against
beacon's `{epoch, root}`. That reason does not survive contact with the
fact that an epoch names its own start slot: storing it that way round
trips exactly through a division, so one slot-denominated row serves
either chain and `Store::update_checkpoints` becomes the single writer of
the realized pair. `KEY_BEACON_JUSTIFIED` and `KEY_BEACON_FINALIZED` go
away, and with them the four setters that fed them.
`KEY_BEACON_HEAD` goes too. The head is `KEY_HEAD` on both chains now, so
`beacon_head` derives its slot from the head's own header row rather than
keeping a second `slot || root` value that could drift from the first.
Recording a head through the shared writer also keeps the canonical
`BlockRoots` index in step on a beacon directory, which it never was:
that index holds one block per slot because it holds only the branch fork
choice picked, so forks are no more an obstacle here than on lean.
What the unification actually needed was a chain-agnostic reader for a
stored block's slot and parent root. `block_root_index_changes` decoded a
lean `BlockHeader`, so it could not have run on a beacon directory at
all; `block_entry` now dispatches on the chain tag and both walks share
it. The beacon fork choice reads through it too, in place of the seven
sites that fully decoded a `SignedBeaconBlock`, execution payload
included, to reach two integers.
Pruning stays lean-only, deliberately. The gossip-signature and payload
buffers hold lean attestations a beacon directory never has, and
`prune_live_chain` would be worse than a no-op: `filter_block_tree` asks
`get_checkpoint_block` for the ancestor at the finalized epoch's start
slot, and `get_ancestor` keeps walking while a parent's slot exceeds the
one asked for, so an empty start slot lands below the horizon. A row
missing there is a hard `SpecAssert`, not a degraded read, so beacon
needs its own horizon before it prunes anything.
Also here, from the same read-through: `block_index` was a byte-for-byte
copy of `get_live_chain` and now delegates; `get_weight` takes the block
index instead of rescanning the whole table once per candidate child at
every level of `get_head`'s walk; `set_metadata_batch` puts related
metadata rows under one commit; and the index diff short-circuits when
the head has not moved, which is what a checkpoint-only advance looks
like on the beacon arm.
* refactor(storage): keep a beacon block in one row, not two
Lean splits a block across `BlockHeaders` and `BlockBodies` for two
reasons: a header-only query need not pay for the body, and a block whose
body is empty can leave the body row out entirely. Neither reason holds
for a beacon block. It has no empty-body case, and nothing reads a beacon
header without wanting the block behind it, so the split bought a second
write per import and a second way for the two rows to disagree.
`BlockHeaders` now holds the whole `SignedBeaconBlock`, fork-selector
tagged the way a `States` value is, and the beacon arm writes no
`BlockBodies` row at all. `BeaconBlockEntry`, the two-field struct that
stood in for a header, goes away with it, and the tag-then-decode that
`get_signed_block` spelled out inline is now `encode_beacon_block_value`
and its inverse, beside the pair that already does this for states.
The cost is that `block_entry` decodes a whole block, execution payload
included, to answer a slot and a parent root. Its own documentation now
says so and points a caller walking a chain of them at
`Store::block_index`, which reads the same links out of `LiveChain`.
Also corrects the comment on why `BlockRoots` is not written during
import. It claimed a slot can hold several blocks once a fork exists, but
that index only ever holds the branch fork choice picked, so it holds one
block per slot on either chain; what makes it wrong to write here is that
import order does not determine which branch is canonical.
`update_checkpoints` maintains it as the head moves.
* refactor(beacon): read a block's slot from the caller that already has it
Three of the seven places the beacon fork choice reached into the store
for a block's slot were asking for a value the caller already held. Now
that a beacon `BlockHeaders` row holds the whole block, each of those was
a full SSZ decode, execution payload included, to recover one integer.
`get_voting_source` was the worst of them: `filter_block_tree` calls it
once per leaf on every `get_head`, and one frame up it already has that
block's `(slot, parent_root)` out of the block index it is walking. It
now takes the index, like `get_ancestor`, `get_checkpoint_block` and
`get_weight` do, which is also the closer reading of the specification,
whose `store.blocks[block_root].slot` this index stands in for.
`compute_pulled_up_tip` is called only by `on_block`, which has the block
itself in hand, so the slot is passed in. `validate_on_attestation` built
the index a few lines below its own point lookup; building it first lets
the known-block check and the LMD walk share one scan.
The four that remain, in `get_proposer_head` and
`should_override_forkchoice_update`, are proposal-time and have no index
in scope, where a whole-table scan to serve two lookups would be the
worse trade.
* chore(storage): drop a comment for an import that is gone
`BeaconBlockEntry` was the only thing in `store.rs` deriving SSZ, and it
took the `libssz_derive` import with it when 5a8f8be1 folded a beacon
block into one row. The comment above that import stayed.
It explained a real hazard: `libssz` exports traits named `SszDecode`
and `SszEncode`, `libssz_derive` exports derive macros of the same two
names, so a file wanting both has to bind one pair under another name.
Nothing here wants both any more, and a comment describing a collision
that no longer exists reads as a warning against a change that is
already made.
* refactor(storage): drop the beacon import watermark
`beacon_highest_imported_slot` is the highest slot any beacon block was
imported at: monotone, advanced only forward, and deliberately not the
fork-choice head, so that a reorg lowering the head does not make
forward sync re-fetch slots it already has.
Nothing on this branch reads it. Its three callers live on
feat/mainnet-network, which forked from lambdaclass#591 and keeps its own copy in
`beacon_store.rs`, the file this branch folded into `store.rs`: the
range-sync origin, the `Status` head fallback before the first head
computation, and the `lean_sync_local_head_slot` metric. So what stood
here was the storage half of a feature whose other half is on a sibling
line, kept alive by tests written for it and by nothing else.
Carrying it costs a metadata key seeded at every beacon bootstrap, a
point read on every `insert_signed_block` including the lean arm that
has no use for it, and a conditional write. Whichever line lands second
has to reconcile the two copies anyway; there is no version of that
merge made harder by this side being empty.
Also drops half of the `batch.commit()` rationale, which justified the
single commit partly by the watermark never getting ahead of the block
it counts. The other half, that a half-written block is visible to the
`LiveChain` scan without being decodable, is the whole reason now.
* feat(storage): one store clock in seconds, plus a preset tag on the directory
`Metadata["time"]` carried two different units under one key and one accessor:
intervals-since-genesis on a lean directory, Unix seconds on a beacon one.
`Store::time`'s own documentation asserted the lean meaning unconditionally,
and `Store::current_slot` divided by `INTERVALS_PER_SLOT`, so it answered a
number that meant nothing on a beacon store. Nothing read it there yet, which
is the only reason that was not already a bug.
`time` is now a Unix second on both chains, which is what the beacon
specification's `Store.time` is, and `current_slot` derives the slot from it
for either chain. Lean keeps its interval clock under a second row,
`intervals_since_genesis`, because it cannot be derived from the first: with
`INTERVALS_PER_SLOT` intervals to a slot most interval boundaries fall
strictly between two whole seconds, and `on_tick` steps that grid one interval
at a time, running a duty per step. Both advance from the same tick, the
shared one forward-only so that `tick_to_slot` handing back an earlier
timestamp cannot rewind it.
Every lean site that meant "which interval are we in" now says so: the
attestation and block future-slot guards, the tick idempotency guard, the
arrival-metrics bound, the early-aggregation gate, the fork-choice fixtures'
`checks.time` and the Hive driver snapshot. The two block/attestation guards
in particular are documented as mirrors of each other, so both read the clock
`on_tick` rewinds to replay a slot rather than one of each.
`get_slots_since_genesis` now goes through `Store::current_slot` instead of
keeping a second copy of the arithmetic. It had been reading `genesis_time`
off the store and `seconds_per_slot` off its `config` parameter, and those are
not the same value: `init_beacon` overwrites `genesis_time` from the anchor
state, so a caller holding a config with a placeholder there got a split
clock.
Separately, a data directory now records which SSZ preset the build that wrote
it used, beside the chain tag and for the same reason: a raw byte, readable
before anything else in the directory is decoded, by a build that would decode
the rest into the wrong shape. `db_version` cannot stand in for it, because
both presets write the same layout at the same version while bounding every
SSZ container differently; a `preset-minimal` build opening a mainnet
directory passed both existing gates and then misread it. `from_db_state`
refuses a mismatch, and refuses a directory with no preset recorded, since the
whole point of the row is that nothing else in the directory says which shapes
it holds.
* refactor(storage): one clock row in milliseconds, every other grid derived
The previous commit gave the two chains one `Metadata["time"]` in seconds and
handed lean a second row for its intervals, because four of every five
interval boundaries fall between two whole seconds and a seconds row cannot
name them. Two rows is the wrong answer to that: they are the same clock, and
anything that must be kept in step can fall out of step.
The row is milliseconds instead, which is fine enough for the finest grid
either chain schedules on, and the intervals row is gone. `intervals_since_
genesis` now derives from the row rather than holding its own copy of it, as
`current_slot` already did, and both go through one `ms_since_genesis` that
owns the genesis subtraction and its saturation. Nothing stores a derived
reading, so no two readings can disagree.
`on_tick` walks that one row forward an interval at a time, which is what it
was already doing to the intervals row, and leaves it on the boundary it
processed rather than on the tick's own timestamp: the leftover milliseconds
carry no duty that has run. The forward-only guard the previous commit needed
for the second row goes away with it, since the only backwards write left is
the deliberate rewind that replays a slot.
The beacon specification denominates its `Store.time` in seconds, so
`on_tick_per_slot` and `get_forkchoice_store` multiply on the way in and the
fixture harness divides on the way out. That is three call sites against a
second stored unit, and it is why the accessor is `time_ms` and not `time`:
the two quantities are one unmarked factor of a thousand apart, which is the
confusion this whole change exists to remove.
`is_proposing_on_time` and `on_block`'s timeliness check get more precise for
free. Both were computing a whole-second difference and multiplying it back up
to compare against a basis-point deadline within the slot; both now read
`ms_since_genesis` directly. The fixtures drive those through whole seconds,
so the suites see no change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🗒️ 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.
nodeis an ordinary clap sub-command on a top-level parser that owns the binary's name,version and about.
NodeOptions(renamed fromCliOptions) becomes a plainclap::Argsgroup and keeps every field
exactly as it is — no
Option<T>, norequired = true, no unwrap helper on the nodepath, which is what the review of #497 objected to.
The flat
ethlambda --genesis ...form keeps working, because that is what theDockerfile, 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 linethat names no sub-command gets
nodeinserted.What Changed
bin/ethlambda/src/command.rsCliparser +Commandsub-command enum, anddefault_subcommand, which insertsnodeunless the first token is a sub-command,-h/--help/-V/--version, or clap's generatedhelpbin/ethlambda/src/cli.rsclap::Parser→clap::Args, andCliOptionsrenamed toNodeOptions; the#[command(...)]attribute moves to the top-level parser. No field changesbin/ethlambda/src/main.rscommand::parse()and matches onCommandcommand.rsalso carries a test-onlyparse_node_optionshelper. Mergingmainbrought#579's
cli.rstests, which calledCliOptions::parse_from— aclap::Parsermethod thegroup lost when it became
clap::Args. Git merged both sides cleanly, so nothing flaggedit; the test build was broken until
a3d7e52, and both test modules now parse a nodecommand line through the real dispatch.
Correctness / Behavior Guarantees
not have to trust us for:
--helplists the sub-commands itself, usage lines name thesub-command, and an unknown sub-command produces clap's error rather than a
stray-positional one.
NodeOptionsdeclares no positional arguments, so the first token after the programname 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.
--versionafter node flags still works and still prints the same string. It usedto live on the node options, so it was accepted anywhere;
propagate_versionkeeps that,and
display_name = "ethlambda"keeps the output byte-identical rather thanethlambda-node. All three forms are asserted equal.ethlambdanow prints clap's top-level help, listingthe 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.rspin: the flat parse; the two forms agreeing field for field; a--node-idvalue that is literallynode; a trailingnodetoken still rejected; asecond
nodetoken rejected; missing required flags in both forms; the bare invocation'serror kind and non-zero exit;
--help/--versionstaying top-level;--versionprintingone identical string across all three forms; and
--helplisting the sub-commands.make fmt,make lint,make test(574 tests, 30 suites) — all clean.Related Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleanmake test(cargo test --workspace --profile release-fast) — all passing