feat(cli): make benchmark reports comparable across runs and machines

[pablodeymo]

🗒️ 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 feat(cli): add offline block-building benchmark subcommand #497 / feat(cli): add offline block-building benchmark sub-command #593
• Related to Block building optimizations #465

✅ Verification Checklist

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

[github-actions]

🤖 Kimi Code Review

bin/ethlambda/build.rs

• Lines 70–89: Manual TOML parsing of Cargo.lock is fragile. The format is stable but using toml or cargo-lock crates would be more robust against whitespace variations and future field reordering. If keeping the manual parser, add a comment noting the dependency on the specific lockfile format.
• Line 76: The relative path ../../Cargo.lock assumes a fixed workspace layout. Consider using CARGO_WORKSPACE_DIR (Cargo 1.74+) or traversing upward to find the lockfile to survive crate restructuring.
• Line 89: rsplit_once('#') correctly handles the git revision fragment, but note that if the URL itself contains a # (e.g., in a branch name), this will split incorrectly. Branch names with # are rare but valid; consider documenting this limitation.

bin/ethlambda/src/benchmark/mod.rs

• Lines 227–234: If both --format json and --output are specified, report.to_json() is called twice (once for stdout, once for the file). For large reports, serialize once to a String and reuse it.
• Line 232: Writing to the file uses ? but the stdout print on line 229 uses println!. Consider handling the JSON serialization error consistently (though both ultimately return eyre::Result).

bin/ethlambda/src/benchmark/report.rs

• Lines 105–106: The percentile function uses round() which can be sensitive to floating-point precision. Given the small sample sizes typical in benchmarks, the nearest-rank method is appropriate, but consider documenting why interpolation was avoided.
• Line 134: The CV warning threshold of 10% is reasonable, but consider making it configurable via CLI for noisy CI environments.
• Lines 186–187: Good practice embedding the crypto library revisions in the report; this prevents benchmark result misinterpretation when dependencies move.
• Line 230: Hardcoded schema_version: 1 is good for future compatibility. Document the schema evolution policy (e.g., "bump on breaking JSON structure changes").

General

• CI workflow: The smoke test using jq to validate JSON structure is a good integration check.
• Tests: The unit tests for percentile and stats cover edge cases (empty input, single sample, odd/even lengths). Consider adding a test for the CV calculation when mean is zero to prevent regression of the NaN-guard.

Security/Memory: No unsafe code introduced. File I/O in build.rs is limited to reading the lockfile; paths are constructed from CARGO_MANIFEST_DIR which is controlled by Cargo.

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. bin/ethlambda/src/benchmark/report.rs lines 296-298 implement percentile() as round((len - 1) * q), but the comment says “nearest-rank percentile.” That is not nearest-rank and it biases even-sized samples upward: for 4 values, p50 becomes the 3rd element instead of the 2nd nearest-rank value. Because this summary is now part of the JSON contract, consumers will get systematically wrong p50/p90 values. Either implement true nearest-rank (ceil(n*q) - 1, clamped) or rename the function/docs/tests to the estimator you actually want.

2. bin/ethlambda/build.rs lines 41-45 silently fall back to "unknown" when the lockfile path changes or the ad hoc parser stops matching. For benchmark provenance, that is a correctness problem: reports remain “valid” JSON but lose the crypto revision data that this PR is trying to preserve, and CI only checks schema_version plus sample count. I would at least emit a warning when either targeted package is missing, and preferably fail the build or switch to a real lockfile parser so provenance regressions are not silent.

No consensus-path, fork-choice, attestation-validation, SSZ, or XMSS-processing logic is touched here, so I did not find consensus/security regressions in the Ethereum client code itself.

I could not run the Rust tests in this sandbox because Cargo/rustup attempted to write under read-only ~/.cargo / ~/.rustup.

---

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

[github-actions]

🤖 Claude Code Review

Review: PR 596 — benchmark report comparability (stats, provenance, JSON)

This is benchmarking-tool code, not consensus-critical logic, so the risk profile is low, but there's one finding that materially affects CI cost/time and directly contradicts the PR's own claims.

1. CI smoke step does not reuse the previous build, contrary to its comment (.github/workflows/ci.yml:79-85)

# Reuses the release build from the test step; validates the benchmark
# harness end-to-end and its JSON output contract in a few seconds.
- name: Benchmark smoke (mock crypto)
  run: |
    cargo run --release --bin ethlambda -- benchmark synthetic --mock-crypto \

The preceding "Run fixture-based tests" step runs make test, which builds under --profile release-fast (Makefile:15, no LTO, 16 codegen units, incremental — see Cargo.toml:48-53). This new step invokes cargo run --release, which is [profile.release]: lto = "fat", codegen-units = 1 (Cargo.toml:38-41). These are two entirely separate profile directories under target/; nothing is reused. The result is a from-scratch fat-LTO build of the full binary and its dependency graph (including the XMSS/leanVM crypto stack), which is exactly the slow-build case the release-fast profile exists to avoid, per this repo's own CLAUDE.md note ("rebuilds are much faster than --release").

• The comment and PR description's "seconds-fast" framing are false — this will add a full LTO release build to CI (likely minutes, not seconds).
• Fix: use cargo run --profile release-fast (matching the test job's build) so it genuinely reuses cached artifacts from the preceding step, or run it as an additional command inside make test/the fixture-tests composite action rather than a separate --release invocation.

This is worth confirming/fixing before merge since it's a recurring CI cost on every PR and push to main.

Minor nits

• bin/ethlambda/src/benchmark/mod.rs:226-231: when both --format json and --output <path> are passed, report.to_json() is called twice, redoing the serialization. Cheap in absolute terms (small struct, once per benchmark run) but trivially avoidable by binding it to a local once.
• bin/ethlambda/build.rs lockfile_git_revs(): hand-rolled line-based TOML parsing instead of a proper parser. It's scoped narrowly enough (only name = / source = lines, only within [[package]] blocks) to be safe for a machine-generated Cargo.lock today, but it's a second implicit TOML parser to keep in sync if the lockfile format ever changes shape. Given this only affects benchmark provenance metadata (falls back to "unknown" on any parse miss, not a hard failure), this is acceptable as-is — just flagging as a maintainability tradeoff, not a blocker.
• .github/workflows/ci.yml:82-85: the new step's run: block omits set -euo pipefail (used by the other custom steps in this same action, e.g. run-fixture-tests/action.yml:18). In practice jq -e will fail on non-JSON/empty input if cargo run fails, but adding set -euo pipefail would make that failure mode explicit and consistent with the rest of the workflow.

What looks solid

• stats()/percentile(): population-variance CV, nearest-rank percentiles, empty-input zeroing, and the single/odd/even-length edge cases are all correctly handled and covered by the added unit tests (report.rs:295-345). The sorted.len() - 1 in percentile can't underflow since it's only reached after the is_empty() guard in stats().
• build.rs package-name-to-env-var mapping (leansig, lean-multisig) matches the actual Cargo.lock entries and crates/common/crypto/Cargo.toml dependency names — verified directly against the lockfile.
• rustc-env fallback to "unknown" plus the single cargo:rerun-if-changed for the lockfile is the right call: env!() in report.rs can't fail to compile, and since the git2/rustc Emitter instructions already suppress Cargo's default whole-package rerun-if-changed, explicitly watching Cargo.lock here is actually necessary, not redundant.
• No consensus-layer code paths are touched — build.rs and benchmark/ are excluded from block/attestation/state-transition logic, so there's no fork-choice, justification, or signature-verification risk surface in this diff.

---

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