feat(cli): add offline block-building benchmark sub-command - #595
Conversation
🤖 Kimi Code ReviewThis is a well-structured benchmarking PR that correctly isolates the block-building path. The code is clean, properly documented, and follows Rust idioms. Below are specific observations. Code Correctness & Safety
store.insert_new_aggregated_payloads_batch(entries);The batch insert assumes the store's internal ordering is deterministic. Ensure
eyre::ensure!(
options.proofs_per_data as usize <= NEW_PAYLOAD_CAP,
"--proofs-per-data {} exceeds the pending-pool capacity..."
);Good defensive check. However,
let total_slots = options
.warmup_slots
.checked_add(common.iterations)
.ok_or_else(|| eyre::eyre!("--warmup-slots plus --iterations overflows u64"))?;Correct overflow handling.
proof: MultiMessageAggregate::default(),In mock-crypto mode this is fine, but verify that Consensus & Cryptographic Considerations
const GENESIS_TIME: u64 = 1_700_000_000;Hardcoding genesis time is correct for deterministic benchmarking. Ensure this doesn't conflict with any time-based fork logic if the benchmark is later extended to simulate forks across long timespans.
let validators = (0..self.num_validators)
.map(|index| Validator {
attestation_pubkey: synthetic_pubkey(&mut rng_state),
proposal_pubkey: synthetic_pubkey(&mut rng_state),
index,
})The deterministic RNG is appropriate here. Verify that
on_block_without_verification(&mut store, signed_block)Documented why verification is skipped (self-generated blocks). Ensure this function doesn't skip state-transition validation that could corrupt the store for subsequent iterations. The comment says it imports "outside the measured span," which is correct methodology. Performance & Memory
.flat_map(|family| family.get_metric())
.filter_map(|metric| { ... })This iterates all metrics in the global registry. If the registry grows large, consider caching the histogram lookup by name, though for a benchmark tool this is negligible.
fn splitmix64(state: &mut u64) -> u64 {
*state = state.wrapping_add(0x9e37_79b9_7f4a_7c15);
...
}Consider marking this Error Handling & Robustness
eyre::ensure!(
store.new_aggregated_payloads_count() > 0,
"seeded attestations were evicted from the pending pool at slot {slot}..."
);Excellent check. Silent eviction would produce misleading benchmark results (empty blocks).
eyre::ensure!(
observations == 1,
"phase '{phase}' was observed {observations} times during one build (expected 1)..."
);The assumption of single-threaded execution is valid now but fragile. Add a comment warning that this check will fail if CLI & UX
args.drain(..2);
let argv = std::iter::once(OsString::from("ethlambda benchmark")).chain(args);Clever handling to get clap to render
Testing
Minor Suggestions
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
I didn’t find a production consensus/security regression in the new proposer-path benchmark wiring or the Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewReview:
|
233cc94 to
428174e
Compare
db70dbf to
99adf46
Compare
`ethlambda benchmark synthetic` measures block building exactly as executed when the node proposes, against a reproducible synthetic workload, with no devnet required. "Optimize block building" (#465) is the top roadmap item, but the only observability today is Prometheus histograms on a live devnet: noisy, not reproducible, and unable to compare an optimization against a baseline. The harness drives the production proposer entry point (`produce_block_with_signatures`) over a seeded in-memory chain, seeding the pending pool per slot and letting the proposal tick promote it, as on a live node. Per-phase timings come from the existing `lean_block_proposal_attestation_build_phase_seconds` histogram: the per-label sample sums are deltaed between iterations, so attribution is exact and the hot path is untouched. Each iteration reports its block root, so a baseline-vs-optimized diff proves an optimization changed only speed and not attestation selection. `benchmark` is a second clap sub-command alongside `node`, so clap lists it in `--help` and names it in its own usage lines, and the harness arguments live in their own `Args` group rather than in CliOptions. `main` becomes synchronous and only the node path enters the tokio runtime: the benchmark is synchronous CPU-bound work and would otherwise park a worker thread for its whole run. Its logs go to stderr so the report owns stdout. NEW_PAYLOAD_CAP becomes public so the harness can reject a --proofs-per-data batch the pending pool would evict whole. `make bench` runs it. Reports one row per measured iteration. Aggregate statistics, build provenance and machine-readable output follow separately, as does the real-crypto workload — see docs/plans/block-building-benchmark.md for the milestones. Laid out to be read in one pass: `SyntheticCorpus` builds the chain and seeds the pool, `build_one_slot` is one slot's work end to end (seed, time the build, import), `PhaseTimer` turns two histogram readings into per-phase durations, `run_synthetic` is the loop over slots, and report.rs formats. Warmup and measured slots run the same code path; only whether the sample is kept differs.
99adf46 to
c2da08d
Compare
| }) | ||
| } | ||
|
|
||
| const PHASE_HISTOGRAM: &str = "lean_block_proposal_attestation_build_phase_seconds"; |
There was a problem hiding this comment.
This is a really hacky way of recording phase times. We should record the duration of each phase inside the relevant functions and return a report, which the metrics consume. That way we don't have to do this here.
We can do this in another PR
There was a problem hiding this comment.
Agreed, and filed as #599 so it does not get lost.
Two reasons beyond removing the indirection, which I wrote up there: having the phases return their own timings makes them available to anything that builds a block, rather than only to a process that can read the global registry — which is what a replay-from-datadir mode would want — and it removes the reason the harness is limited to one configuration per process invocation. It also lets the "observed exactly once per build" assertion go away, since that check only exists because a registry diff cannot tell a mis-attribution from a real measurement.
| // Boxed because the node options dwarf every other variant's payload | ||
| // (~312 bytes against ~80), which `clippy::large_enum_variant` rightly | ||
| // flags: one allocation per process is cheaper than carrying that size in | ||
| // every value of this enum. | ||
| #[command(display_name = "ethlambda")] | ||
| Node(NodeOptions), | ||
| // Boxed because the node options dwarf the other variant's payload (~312 | ||
| // bytes against ~80), which `clippy::large_enum_variant` rightly flags: one | ||
| // allocation per process is cheaper than carrying that size in every value | ||
| // of this enum. | ||
| Node(Box<NodeOptions>), |
There was a problem hiding this comment.
The comment is duplicate.
Also, I think we should drop the Box and tag this enum with #[allow(clippy::large_enum_variant)]. Adding the Box on each variant just adds noise, and having the enum be unbalanced isn't a performance problem here really.
There was a problem hiding this comment.
Done in 39aa610: the Box is gone and the enum carries #[allow(clippy::large_enum_variant)] with a line on why the imbalance is fine here — exactly one variant is built per process and main consumes it immediately. Same reasoning req_resp::messages already uses, so it is at least consistent with the rest of the tree.
You were right about the duplicate too: the previous rebase left that comment in the file twice. Both copies are gone with the Box.
Review feedback: the `Box` around the node options adds noise at every use site, and the imbalance is not a performance problem here — exactly one variant is built per process and `main` consumes it immediately. The enum carries `#[allow(clippy::large_enum_variant)]` instead, which is what `req_resp::messages` already does for the same reason. Also drops the boxing rationale, which the previous rebase had left in the file twice.
Brings in the offline block-building benchmark sub-command (#595). Conflicts and adaptations: - `NEW_PAYLOAD_CAP` (crates/storage/src/store.rs): main made it `pub` for the benchmark's pool seeding; this branch dropped the hardcoded "~4s" from its doc comment because the slot duration is now configurable. Kept both. - `benchmark::corpus` builds its synthetic store through `Store::from_anchor_state`, which now takes the slot duration; the harness derives tick timestamps from slot numbers rather than a clock, so it passes `DEFAULT_MILLISECONDS_PER_SLOT`.
2 participants
🗒️ Description / Motivation
Adds
ethlambda benchmark synthetic— an offline harness that measures block buildingexactly 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.
How to read this
In this order — each piece is understandable without the next:
corpus.rs— the workload. Deterministic validators, a genesis store overInMemoryBackend, andseed_pool, which fills the pending pool for one slot andreports how many entries the next build will see.
build_one_slot(mod.rs) — one slot end to end: seed, time the build, import theblock. 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.
PhaseTimer(mod.rs) —start()before the build,finish()after. Tworeadings 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.
run_synthetic(mod.rs) — validate, set up, loop over slots, report. 46 lines.report.rs— the types and the per-iteration table.What Changed
bin/ethlambda/src/benchmark/mod.rsrun_synthetic's slot loop,build_one_slotfor one slot's work, andPhaseTimerfor per-phase attributionbin/ethlambda/src/benchmark/corpus.rsInMemoryBackend, deterministic pubkeys via splitmix64, and per-slot pool seeding in fixed insertion order, which also rejects a batch the pool would evict wholebin/ethlambda/src/benchmark/report.rsbin/ethlambda/src/command.rsbenchmarkjoinsnodeas a second clap sub-command, so clap lists it in--helpand names it in its own usage lines. The node payload becomesBox<CliOptions>now that a much smaller variant sits beside itbin/ethlambda/src/main.rsmainbecomes synchronous and dispatches; only the node path enters the tokio runtime (run_nodecarries the#[tokio::main]attributes). Benchmark logs go to stderr at WARN so the report owns stdoutcrates/storage/{lib,store}.rsNEW_PAYLOAD_CAPso the harness rejects a--proofs-per-databatch the pending pool would evict wholeMakefilemake bench(overrideBENCH_ARGSto customize)Correctness / Behavior Guarantees
cli.rsis not touched by this PR and the node runtime is unchanged. The harnessarguments live in their own
Argsgroup; the node's stay plainPathBuf/String, soclap keeps emitting its own missing-argument errors.
produce_block_with_signatures, the same functionBlockChainServer::propose_blockcalls, and seeds the pending pool so the proposal tick promotes it exactly as on a
live node.
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.
select_payloads/compact/stf_simulatecome from the sample sums of the existinglean_block_proposal_attestation_build_phase_secondshistogram, deltaed betweeniterations, with a per-phase assertion that the count advanced by exactly one.
overheadis the clamped remainder of wall minus the phases.duration of a CPU-bound run.
Tests Added / Run
corpus.rs: participant groups partition every validator; synthetic pubkeys aredeterministic for a seed.
command.rs: the benchmark token parses with no node argument, rejects node flags, andits usage line names the sub-command.
make bench; identical block-root sequences across two runs at thesame seed;
ethlambda --genesis config.yamlstill failing with clap's ownmissing-argument list.
make fmt,make lint,make test(576 tests, 30 suites) — all clean.Related Issues / PRs
nodesub-command for running the node #591; design doc in the accompanying docs PR✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleanmake test(cargo test --workspace --profile release-fast) — all passing