feat(cli): add offline block-building benchmark subcommand - #497
Closed
pablodeymo wants to merge 8 commits into
Closed
feat(cli): add offline block-building benchmark subcommand#497pablodeymo wants to merge 8 commits into
pablodeymo wants to merge 8 commits into
Conversation
`extend_proofs_greedily` kept its remaining candidate proofs in a `HashSet<usize>` and picked the best-coverage proof via `max_by_key` over the set's randomized iteration order, so equal-coverage ties were broken arbitrarily per process: the same store state could produce blocks with different aggregation bits from one run to the next. Iterate candidates in index order and break coverage ties toward the lowest index (pool insertion order), making block building reproducible for a given pool. Found by the offline block-building benchmark's same-seed determinism gate.
`ethlambda benchmark synthetic --mock-crypto` measures block building as executed when the node proposes, without a devnet: it builds a synthetic in-memory chain (seed-derived validators, fixed genesis time, no wall-clock dependence), seeds the pending attestation pool each slot the way gossip aggregates arrive, and drives produce_block_with_signatures — the same entry BlockChainServer::propose_block uses — importing each built block so every iteration builds one slot ahead of head like a live proposer. Supports the 'Optimize block building' roadmap item (#465) with reproducible offline measurements; design and roadmap (real-crypto pools, replay-from-datadir) in docs/plans/block-building-benchmark.md. Per-iteration select_payloads/compact/stf_simulate durations come from delta-ing the existing phase histogram's sample sums between iterations (exact: sums accumulate raw f64 seconds and each phase observes exactly once per build; a count-delta assertion turns accounting drift into a hard error). The report (human table or pipe-clean JSON on stdout, logs on stderr) has min/mean/p50/p90/max per phase, the unattributed preamble overhead, per-iteration block roots as a determinism checksum, and environment capture including the resolved leansig revision parsed from Cargo.lock at build time, since leansig tracks a moving branch. The CLI keeps every existing flat node invocation working unchanged: subcommand_negates_reqs + args_conflicts_with_subcommands with the seven node-required arguments as Option<T> + required = true preserves clap's native missing-argument errors for the node path while letting the subcommand parse without them (pinned by unit tests). NEW_PAYLOAD_CAP is exported from ethlambda-storage so the harness rejects --proofs-per-data batches the pending pool would silently evict whole. Adds a 'make bench' target and a seconds-fast mock-crypto smoke step to the CI Test job validating the JSON output contract.
pablodeymo
marked this pull request as draft
July 3, 2026 14:31
Contributor
Greptile SummaryThis PR adds
Confidence Score: 4/5Safe to merge; the node runtime path is unchanged except the tie-break fix, which only affects previously-random equal-coverage cases, and all existing CLI invocations are pinned by tests. The benchmark harness is additive and isolated from production code paths. The block_builder.rs change is small, well-justified, and covered by the determinism contract enforced by the harness itself. The CLI restructuring is thoroughly tested. Three comments were left: a suggested .max(0.0) guard on the overhead_seconds computation, a more order-independent Cargo.lock parser in build.rs, and a note about calling blocking benchmark work on the async executor thread. bin/ethlambda/src/benchmark/mod.rs (overhead clamp), bin/ethlambda/build.rs (Cargo.lock field-order assumption). All other files are straightforward.
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/block_builder.rs | Fixes non-deterministic tie-breaking in extend_proofs_greedily by switching remaining_indices from HashSet to Vec and using (count, Reverse(idx)) in max_by_key; the logic is correct and the O(n) retain is fine given pool caps. |
| bin/ethlambda/src/benchmark/mod.rs | Core benchmark harness; phase-delta accounting with hard count assertions is robust; overhead_seconds computed as wall minus sum-of-phases without a max(0) guard could theoretically surface a tiny negative value in the report. |
| bin/ethlambda/src/benchmark/corpus.rs | Deterministic synthetic corpus; participant_groups correctly partitions validators with step_by; splitmix64 PRNG avoids an external rand dependency cleanly. |
| bin/ethlambda/src/benchmark/report.rs | Statistics and report serialization are correct; population variance (÷n) is appropriate for a closed sample set; nearest-rank percentile is well-tested. |
| bin/ethlambda/src/cli.rs | CLI restructuring with subcommand_negates_reqs + args_conflicts_with_subcommands is idiomatic clap 4; six unit tests thoroughly pin backwards-compat for all invocation shapes. |
| bin/ethlambda/src/main.rs | Early benchmark dispatch mirrors the existing HIVE test-driver pattern; require_arg helper correctly surfaces CLI contract violations; node path unwraps are safe after the benchmark branch returns. |
| bin/ethlambda/build.rs | Leansig revision extraction from Cargo.lock via manual line parsing assumes name appears before source in each [[package]] block; TOML field order in Cargo.lock is stable in practice but not formally guaranteed; falls back gracefully to unknown. |
| crates/storage/src/store.rs | NEW_PAYLOAD_CAP visibility widened from private to pub; change is minimal and the added doc comment explains the rationale clearly. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ethlambda start] --> B{CLI parse}
B -->|subcommand present| C[Set WARN log to stderr]
B -->|no subcommand| D[Set INFO log to stdout]
C --> E[metrics::init]
D --> E
E -->|benchmark subcommand| F[benchmark::run - synchronous]
E -->|no subcommand| G[unwrap required node args]
F --> H[run_synthetic]
H --> I{mock_crypto?}
I -->|no| J[Runtime error: not implemented]
I -->|yes| K[Build SyntheticCorpus + Store]
K --> L[Loop: warmup + measured slots]
L --> M[seed_pool slot-1]
M --> N[phase_snapshot BEFORE]
N --> O[produce_block_with_signatures]
O --> P[phase_snapshot AFTER]
P --> Q[phase_deltas: assert count==1 per phase]
Q --> R[on_block_without_verification import]
R --> S{measured slot?}
S -->|yes| T[Push Sample with overhead=wall-sum phases]
S -->|no warmup| U[Next slot]
T --> U
U --> L
L --> V[Build Report]
V --> W{format?}
W -->|human| X[println human table]
W -->|json| Y[println JSON]
X --> Z{output path?}
Y --> Z
Z -->|yes| AA[fs::write JSON to file]
Z -->|no| AB[Done]
G --> AC[node startup...]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[ethlambda start] --> B{CLI parse}
B -->|subcommand present| C[Set WARN log to stderr]
B -->|no subcommand| D[Set INFO log to stdout]
C --> E[metrics::init]
D --> E
E -->|benchmark subcommand| F[benchmark::run - synchronous]
E -->|no subcommand| G[unwrap required node args]
F --> H[run_synthetic]
H --> I{mock_crypto?}
I -->|no| J[Runtime error: not implemented]
I -->|yes| K[Build SyntheticCorpus + Store]
K --> L[Loop: warmup + measured slots]
L --> M[seed_pool slot-1]
M --> N[phase_snapshot BEFORE]
N --> O[produce_block_with_signatures]
O --> P[phase_snapshot AFTER]
P --> Q[phase_deltas: assert count==1 per phase]
Q --> R[on_block_without_verification import]
R --> S{measured slot?}
S -->|yes| T[Push Sample with overhead=wall-sum phases]
S -->|no warmup| U[Next slot]
T --> U
U --> L
L --> V[Build Report]
V --> W{format?}
W -->|human| X[println human table]
W -->|json| Y[println JSON]
X --> Z{output path?}
Y --> Z
Z -->|yes| AA[fs::write JSON to file]
Z -->|no| AB[Done]
G --> AC[node startup...]
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
bin/ethlambda/src/benchmark/mod.rs:184-185
The overhead is computed as `wall_seconds − Σphases`. Both values derive from `Instant` (the histogram observations record `elapsed.as_secs_f64()` durations captured inside `produce_block_with_signatures`), and the preamble work not covered by any phase should always make the remainder positive. However, floating-point subtraction is not associative: with many small phase values, the accumulated sum can round slightly above the wall measurement in pathological cases, producing a small negative `overhead_seconds` that would show up misleadingly in the JSON report and the statistics table. Clamping to zero costs nothing and prevents confusing outputs.
```suggestion
if measured {
let overhead_seconds = (wall_seconds - phases.values().sum::<f64>()).max(0.0);
```
### Issue 2 of 3
bin/ethlambda/build.rs:40-58
The parser sets `in_leansig_package = true` only after seeing `name = "leansig"`, then looks for `source = ...` in subsequent lines. TOML does not mandate field order within a table, so a future `cargo` version or lock-file reformatter that emits `source` before `name` would silently produce "unknown" for `ETHLAMBDA_LEANSIG_REV`. The failure is graceful but invisible — a benchmark report with `"leansig_rev": "unknown"` is still compared against other reports, potentially leading to false equivalence. A two-pass parse (collect both fields per package block before extracting the rev) would be more robust.
```suggestion
fn leansig_rev_from_lockfile() -> Option<String> {
let lockfile = std::fs::read_to_string(workspace_lockfile()?).ok()?;
// Collect both fields per [[package]] block before extracting the rev,
// so the result is independent of TOML field order within a table.
let mut pending_name: Option<String> = None;
let mut pending_source: Option<String> = None;
for line in lockfile.lines() {
let line = line.trim();
if line == "[[package]]" {
pending_name = None;
pending_source = None;
} else if let Some(name) = line.strip_prefix("name = ") {
pending_name = Some(name.trim_matches('"').to_string());
} else if let Some(source) = line.strip_prefix("source = ") {
pending_source = Some(source.trim_matches('"').to_string());
}
if pending_name.as_deref() == Some("leansig") {
if let Some(ref src) = pending_source {
// source = "git+https://github.com/leanEthereum/leanSig?branch=devnet4#<rev>"
let rev = src.rsplit('#').next()?;
return Some(rev.to_string());
}
}
}
None
}
```
### Issue 3 of 3
bin/ethlambda/src/main.rs:105-107
**Blocking work on the async executor thread**
`benchmark::run` is a purely synchronous, compute-intensive function (it loops for `warmup_slots + iterations` block-building cycles, each potentially involving CPU-heavy state transitions) called directly inside an `async fn main` decorated with `#[tokio::main]`. This parks the tokio worker thread for the entire benchmark duration. It is safe today because no other async tasks are alive at this point, but it violates the tokio contract that work on executor threads should yield promptly. Consider wrapping the call in `tokio::task::block_in_place(|| benchmark::run(benchmark_options))` or refactoring `main` to have a synchronous entry point for benchmark mode.
Reviews (1): Last reviewed commit: "feat(cli): add offline block-building be..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+184
to
+185
| if measured { | ||
| let overhead_seconds = wall_seconds - phases.values().sum::<f64>(); |
Contributor
There was a problem hiding this comment.
wall_seconds − Σphases. Both values derive from Instant (the histogram observations record elapsed.as_secs_f64() durations captured inside produce_block_with_signatures), and the preamble work not covered by any phase should always make the remainder positive. However, floating-point subtraction is not associative: with many small phase values, the accumulated sum can round slightly above the wall measurement in pathological cases, producing a small negative overhead_seconds that would show up misleadingly in the JSON report and the statistics table. Clamping to zero costs nothing and prevents confusing outputs.
Suggested change
| if measured { | |
| let overhead_seconds = wall_seconds - phases.values().sum::<f64>(); | |
| if measured { | |
| let overhead_seconds = (wall_seconds - phases.values().sum::<f64>()).max(0.0); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/ethlambda/src/benchmark/mod.rs
Line: 184-185
Comment:
The overhead is computed as `wall_seconds − Σphases`. Both values derive from `Instant` (the histogram observations record `elapsed.as_secs_f64()` durations captured inside `produce_block_with_signatures`), and the preamble work not covered by any phase should always make the remainder positive. However, floating-point subtraction is not associative: with many small phase values, the accumulated sum can round slightly above the wall measurement in pathological cases, producing a small negative `overhead_seconds` that would show up misleadingly in the JSON report and the statistics table. Clamping to zero costs nothing and prevents confusing outputs.
```suggestion
if measured {
let overhead_seconds = (wall_seconds - phases.values().sum::<f64>()).max(0.0);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+40
to
+58
| fn leansig_rev_from_lockfile() -> Option<String> { | ||
| let lockfile = std::fs::read_to_string(workspace_lockfile()?).ok()?; | ||
| let mut in_leansig_package = false; | ||
| for line in lockfile.lines() { | ||
| let line = line.trim(); | ||
| if line == "[[package]]" { | ||
| in_leansig_package = false; | ||
| } else if line == "name = \"leansig\"" { | ||
| in_leansig_package = true; | ||
| } else if in_leansig_package { | ||
| // source = "git+https://github.com/leanEthereum/leanSig?branch=devnet4#<rev>" | ||
| if let Some(source) = line.strip_prefix("source = ") { | ||
| let rev = source.trim_matches('"').rsplit('#').next()?; | ||
| return Some(rev.to_string()); | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } |
Contributor
There was a problem hiding this comment.
in_leansig_package = true only after seeing name = "leansig", then looks for source = ... in subsequent lines. TOML does not mandate field order within a table, so a future cargo version or lock-file reformatter that emits source before name would silently produce "unknown" for ETHLAMBDA_LEANSIG_REV. The failure is graceful but invisible — a benchmark report with "leansig_rev": "unknown" is still compared against other reports, potentially leading to false equivalence. A two-pass parse (collect both fields per package block before extracting the rev) would be more robust.
Suggested change
| fn leansig_rev_from_lockfile() -> Option<String> { | |
| let lockfile = std::fs::read_to_string(workspace_lockfile()?).ok()?; | |
| let mut in_leansig_package = false; | |
| for line in lockfile.lines() { | |
| let line = line.trim(); | |
| if line == "[[package]]" { | |
| in_leansig_package = false; | |
| } else if line == "name = \"leansig\"" { | |
| in_leansig_package = true; | |
| } else if in_leansig_package { | |
| // source = "git+https://github.com/leanEthereum/leanSig?branch=devnet4#<rev>" | |
| if let Some(source) = line.strip_prefix("source = ") { | |
| let rev = source.trim_matches('"').rsplit('#').next()?; | |
| return Some(rev.to_string()); | |
| } | |
| } | |
| } | |
| None | |
| } | |
| fn leansig_rev_from_lockfile() -> Option<String> { | |
| let lockfile = std::fs::read_to_string(workspace_lockfile()?).ok()?; | |
| // Collect both fields per [[package]] block before extracting the rev, | |
| // so the result is independent of TOML field order within a table. | |
| let mut pending_name: Option<String> = None; | |
| let mut pending_source: Option<String> = None; | |
| for line in lockfile.lines() { | |
| let line = line.trim(); | |
| if line == "[[package]]" { | |
| pending_name = None; | |
| pending_source = None; | |
| } else if let Some(name) = line.strip_prefix("name = ") { | |
| pending_name = Some(name.trim_matches('"').to_string()); | |
| } else if let Some(source) = line.strip_prefix("source = ") { | |
| pending_source = Some(source.trim_matches('"').to_string()); | |
| } | |
| if pending_name.as_deref() == Some("leansig") { | |
| if let Some(ref src) = pending_source { | |
| // source = "git+https://github.com/leanEthereum/leanSig?branch=devnet4#<rev>" | |
| let rev = src.rsplit('#').next()?; | |
| return Some(rev.to_string()); | |
| } | |
| } | |
| } | |
| None | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/ethlambda/build.rs
Line: 40-58
Comment:
The parser sets `in_leansig_package = true` only after seeing `name = "leansig"`, then looks for `source = ...` in subsequent lines. TOML does not mandate field order within a table, so a future `cargo` version or lock-file reformatter that emits `source` before `name` would silently produce "unknown" for `ETHLAMBDA_LEANSIG_REV`. The failure is graceful but invisible — a benchmark report with `"leansig_rev": "unknown"` is still compared against other reports, potentially leading to false equivalence. A two-pass parse (collect both fields per package block before extracting the rev) would be more robust.
```suggestion
fn leansig_rev_from_lockfile() -> Option<String> {
let lockfile = std::fs::read_to_string(workspace_lockfile()?).ok()?;
// Collect both fields per [[package]] block before extracting the rev,
// so the result is independent of TOML field order within a table.
let mut pending_name: Option<String> = None;
let mut pending_source: Option<String> = None;
for line in lockfile.lines() {
let line = line.trim();
if line == "[[package]]" {
pending_name = None;
pending_source = None;
} else if let Some(name) = line.strip_prefix("name = ") {
pending_name = Some(name.trim_matches('"').to_string());
} else if let Some(source) = line.strip_prefix("source = ") {
pending_source = Some(source.trim_matches('"').to_string());
}
if pending_name.as_deref() == Some("leansig") {
if let Some(ref src) = pending_source {
// source = "git+https://github.com/leanEthereum/leanSig?branch=devnet4#<rev>"
let rev = src.rsplit('#').next()?;
return Some(rev.to_string());
}
}
}
None
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+105
to
+107
| if let Some(cli::Command::Benchmark(benchmark_options)) = options.command { | ||
| return benchmark::run(benchmark_options); | ||
| } |
Contributor
There was a problem hiding this comment.
benchmark::run is a purely synchronous, compute-intensive function (it loops for warmup_slots + iterations block-building cycles, each potentially involving CPU-heavy state transitions) called directly inside an async fn main decorated with #[tokio::main]. This parks the tokio worker thread for the entire benchmark duration. It is safe today because no other async tasks are alive at this point, but it violates the tokio contract that work on executor threads should yield promptly. Consider wrapping the call in tokio::task::block_in_place(|| benchmark::run(benchmark_options)) or refactoring main to have a synchronous entry point for benchmark mode.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/ethlambda/src/main.rs
Line: 105-107
Comment:
**Blocking work on the async executor thread**
`benchmark::run` is a purely synchronous, compute-intensive function (it loops for `warmup_slots + iterations` block-building cycles, each potentially involving CPU-heavy state transitions) called directly inside an `async fn main` decorated with `#[tokio::main]`. This parks the tokio worker thread for the entire benchmark duration. It is safe today because no other async tasks are alive at this point, but it violates the tokio contract that work on executor threads should yield promptly. Consider wrapping the call in `tokio::task::block_in_place(|| benchmark::run(benchmark_options))` or refactoring `main` to have a synchronous entry point for benchmark mode.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Resolves conflicts with #484 (shadow sim-cost flags): keep both new bin dependencies (ethlambda-crypto for shadow, ethlambda-metrics for the benchmark), and run init_shadow_cost after the relocated tracing-subscriber setup so its info log stays captured, matching main's ordering.
MegaRedHand
reviewed
Jul 8, 2026
| /// measured crypto with zero ethlambda diff; benchmark reports embed this | ||
| /// revision to keep results interpretable across lock bumps. | ||
| fn emit_leansig_rev() { | ||
| let rev = leansig_rev_from_lockfile().unwrap_or_else(|| "unknown".to_string()); |
Collaborator
There was a problem hiding this comment.
We should add leanVM to this too
Collaborator
Author
There was a problem hiding this comment.
Done in 9f7a311. Reports now carry both revisions, resolved from Cargo.lock — leansig for leanSig and lean-multisig for leanVM (the direct dependency ethlambda-crypto builds against; every other leanVM crate resolves to the same rev).
While in there I also made the parse order-independent per greptile's note: name and source are collected per [[package]] block before the rev is extracted, so a lockfile that emitted source first would no longer silently yield unknown.
Sample line from a run: leansig=15cbdd43ec85… leanvm=e2592df4e30f….
Comment on lines
+144
to
+150
| let config_path = require_arg(options.genesis, "--genesis")?; | ||
| let validators_path = require_arg(options.validators, "--validators")?; | ||
| let bootnodes_path = require_arg(options.bootnodes, "--bootnodes")?; | ||
| let validator_config = require_arg(options.validator_config, "--validator-config")?; | ||
| let validator_keys_dir = require_arg(options.hash_sig_keys_dir, "--hash-sig-keys-dir")?; | ||
| let node_key_path = require_arg(options.node_key, "--node-key")?; | ||
| let node_id = require_arg(options.node_id, "--node-id")?; |
Collaborator
There was a problem hiding this comment.
Why do we need this change?
Collaborator
Author
There was a problem hiding this comment.
Because the seven node args have to be Option<T> for the sub-command to parse at all. benchmark passes none of --genesis/--validators/…, and subcommand_negates_reqs only switches off clap's requirement check — the derive still extracts every field, and extraction of a non-Option<PathBuf> with no value on the command line fails.
So they are Option<T> + #[arg(required = true)]: clap keeps emitting its own missing-argument errors for the flat node invocation (pinned by the cli.rs tests), and require_arg on the node path is just the unwrap of an invariant clap already enforced.
The alternative that keeps them non-optional is moving the benchmark into its own bin target, which means splitting the crate into a lib + two bins so the benchmark/cli modules can be shared — happy to do that instead if you would rather not have the Option churn.
Note this hunk moved in 20fab1b: main is now synchronous (parse, logging, metrics, benchmark dispatch) and the node path lives in run_node, so the benchmark no longer parks a tokio worker thread.
…enchmark # Conflicts: # crates/storage/src/lib.rs
The report already recorded the resolved leanSig revision; leanVM does the signature aggregation, so a rev bump moves the measured crypto too and belongs in the report for results to stay interpretable. Also collect name and source per [[package]] block before extracting the revision, so the lookup no longer depends on TOML field order within the table.
Overhead is wall time minus the sum of the phase deltas. The unattributed preamble keeps the remainder positive in practice, but summing many small phase values can round just above the wall measurement, and a negative overhead in the report would read as an accounting bug.
The benchmark is synchronous, CPU-bound work: dispatching it from inside the async main parked a tokio worker thread for the whole run. Split the startup preamble into a synchronous main that returns before the runtime starts, and move the node path into run_node, which now carries the #[tokio::main] attributes.
This was referenced Aug 26, 2026
Collaborator
Author
|
Superseded by #593, which carries the same harness split into reviewable pieces:
Closing in favour of that stack; the review comments here are addressed there. |
This was referenced Aug 26, 2026
Sahilgill24
pushed a commit
to Sahilgill24/ethlambda
that referenced
this pull request
Sep 1, 2026
…bdaclass#590) ## 🗒️ Description / Motivation `extend_proofs_greedily` kept its remaining candidate proofs in a `HashSet<usize>` and picked the best-coverage proof with `max_by_key` over the set's randomized iteration order, so equal-coverage ties were broken arbitrarily per process: the same store state could produce blocks with different aggregation bits from one run to the next. Found while building the offline block-building benchmark (lambdaclass#497), whose same-seed determinism check reported differing block roots across runs of an identical workload. Split out of that PR because it is a standalone node-behavior fix, unrelated to the harness. ## What Changed | File | Change | |------|--------| | `crates/blockchain/src/block_builder.rs` | `remaining_indices` is a `Vec<usize>` iterated in index order; coverage ties break toward the lowest index (pool insertion order) via `max_by_key((count, Reverse(idx)))` | ## Correctness / Behavior Guarantees - Selection quality is unchanged: the greedy still picks maximum marginal coverage every round. Only the choice *among equal-coverage candidates* changes, and that choice was previously random. - Block building is now reproducible for a given pool, which is what makes a baseline-vs-optimized benchmark comparison meaningful. ## Tests Added / Run - `extend_proofs_greedily_breaks_coverage_ties_by_pool_order`: six disjoint proofs of identical coverage, so every round is again a six-way tie and selection order is decided purely by the tie-break. An arbitrary order cannot match pool order by luck (1 in 720); against the previous code the test fails on most runs. - `make fmt`, `make lint`, `make test` — all clean. ## Related Issues / PRs - Split out of lambdaclass#497 - 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
Sahilgill24
pushed a commit
to Sahilgill24/ethlambda
that referenced
this pull request
Sep 1, 2026
…ass#591) ## 🗒️ 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 lambdaclass#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 lambdaclass#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 lambdaclass#497 - Design doc in lambdaclass#594; the benchmark stacks on this in lambdaclass#595 → lambdaclass#596 - 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 --------- Co-authored-by: Tomás Grüner <47506558+MegaRedHand@users.noreply.github.com>
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
Sahilgill24
pushed a commit
to Sahilgill24/ethlambda
that referenced
this pull request
Sep 1, 2026
…lambdaclass#596) ## 🗒️ Description / Motivation Per-iteration rows show what one build cost. Comparing an optimization against a baseline needs three more things, and this adds them: aggregate statistics, build provenance, and machine-readable output. Third of three (design doc → harness → **this**). > **Stacked on the harness PR.** Its diff here is additive — the per-iteration rows stay, > the summary is appended below them. ``` 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 phase count min mean p50 p90 max compact 3 0.000ms 0.000ms 0.000ms 0.000ms 0.000ms select_payloads 3 0.002ms 0.002ms 0.002ms 0.002ms 0.002ms stf_simulate 3 0.015ms 0.015ms 0.015ms 0.015ms 0.015ms overhead 3 0.064ms 0.066ms 0.066ms 0.068ms 0.068ms wall 3 0.081ms 0.083ms 0.083ms 0.085ms 0.085ms ``` ## What Changed | File | Change | |------|--------| | `bin/ethlambda/src/benchmark/report.rs` | `Stats`/`Summary` plus `stats()`, `percentile()` and the aggregate table: count, min, mean, p50, p90, max per phase, and a CV flagged above 10%. `schema_version` + `to_json()`. `Environment` gains the two resolved crypto revisions | | `bin/ethlambda/build.rs` | Resolve the leansig and leanVM revisions from `Cargo.lock` into `rustc-env` vars. The per-`[[package]]` parse collects `name` and `source` before extracting the rev, so it does not depend on TOML field order | | `bin/ethlambda/src/benchmark/mod.rs` | `--format human\|json` and `--output <path>` | | `bin/ethlambda/Cargo.toml`, `Cargo.lock` | `serde_json` | | `.github/workflows/ci.yml` | Seconds-fast mock smoke step in the Test job asserting the JSON contract | ## Correctness / Behavior Guarantees - **Nearest-rank percentiles, no interpolation.** Sample counts are small, so an exact observed value beats a blend of two neighbours. - **Outliers are never discarded** and the raw per-iteration rows stay above the summary, so a heavy tail stays visible instead of being averaged away. A CV above 10% is flagged so a noisy run is not read as a result. - **Provenance is a comparability guard, not decoration.** leansig is pinned to a moving branch and leanVM does the signature aggregation, so either revision moving moves the measured crypto. Two reports that disagree on them are not comparable, and without this the report cannot say so. - **The JSON shape is pinned by CI**, so a change to the report contract cannot land unnoticed. Logs already go to stderr, so the JSON pipes straight into `jq`. - Node behavior is untouched; `build.rs` only adds env vars consumed by the report. ## Tests Added / Run - `report.rs`: percentile on a single sample and on odd/even lengths; `stats()` against a known set whose population stddev gives CV = 0.4; `stats()` on empty input is zeroed. - Verified by hand: the CI assertion `jq -e '.schema_version == 1 and (.samples | length == 3)'` passes, and reports carry both resolved revisions. - `make fmt`, `make lint`, `make test` (580 tests, 30 suites) — all clean. ## Related Issues / PRs - Stacked on the harness PR; design doc in the accompanying docs 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 --------- Co-authored-by: Tomás Grüner <47506558+MegaRedHand@users.noreply.github.com>
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
2 participants
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
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."Optimize block building" (#465) is the top roadmap item, but today the only observability is Prometheus histograms on a live devnet: noisy, not reproducible, and unable to compare an optimization against a baseline. This PR is milestone M1 (mock-crypto mode) of the design in
docs/plans/block-building-benchmark.md; M2 adds real XMSS/leanVM pools and post-build seal-phase measurement, M3 adds replay-from-datadir.What Changed
bin/ethlambda/src/benchmark/{mod,corpus,report}.rsInMemoryBackend, per-slot pool seeding into the pending pool (promoted by the proposal tick, as in production), iteration loop drivingproduce_block_with_signatures+ block import; stats + human/JSON reportbin/ethlambda/src/cli.rssubcommand_negates_reqs+args_conflicts_with_subcommands; the 7 node-required args becomeOption<T>+required = true(clap's missing-arg errors preserved); unit tests pin flat-invocation compatbin/ethlambda/src/main.rsmainis now synchronous: it parses, sets up logging/metrics and dispatches the benchmark (logs to stderr so JSON on stdout stays pipe-clean) without ever starting the tokio runtime; the node path moved intorun_node, which carries the#[tokio::main]attributes and unwraps the required args oncecrates/blockchain/src/block_builder.rsextend_proofs_greedilybroke equal-coverage proof ties by randomizedHashSetiteration order — block aggregation bits differed run-to-run on a live node; ties now break to the lowest pool indexcrates/storage/{lib,store}.rsNEW_PAYLOAD_CAPso the harness rejects--proofs-per-databatches the pending pool would evict wholebin/ethlambda/build.rsCargo.lockinto reports (leansig tracks the movingdevnet4branch; leanVM does the aggregation, so its pinned rev moves the measured crypto too). The per-[[package]]parse collectsnameandsourcebefore extracting the rev, so it does not depend on TOML field orderMakefile,.github/workflows/ci.ymlmake bench; seconds-fast mock smoke step in the Test job validating the JSON contractdocs/plans/block-building-benchmark.mdCorrectness / Behavior Guarantees
cli.rsunit tests covering parsing, missing-arg errors, and mixed-invocation rejection.select_payloads/compact/stf_simulatecome from sample-sum deltas of the existing phase histogram (sums accumulate raw f64 seconds; each phase observes once per build); a count-delta assertion turns any accounting drift into a hard error. Skipped/unattributed time is reported asoverhead, never hidden.--proofs-per-databeyond the pool cap, warmup+iterations overflow, post-seed eviction.Tests Added / Run
make fmt,make lint— clean;cargo test --workspace --release— all passing (spec tests included).... --format json | jq -e '.schema_version == 1 and (.samples | length == 3)'.Related Issues / PRs
docs/plans/block-building-benchmark.md.✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleancargo test --workspace --release— all passing