docs: add the block-building benchmark design plan

[pablodeymo]

🗒️ Description / Motivation

"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.

This records the design of an offline ethlambda benchmark sub-command ahead of the
implementation, so the parts worth arguing about are on the table before there is code
to argue around. First of a three-PR series (see below); nothing here changes behavior.

What Changed

File	Change
docs/plans/block-building-benchmark.md	New. Design and milestone roadmap

What is worth reviewing

• The measured span. What is inside it and what is deliberately outside (gossip
  publish, slot-alignment sleep, block import) — the same boundary as the node's own
  time_block_building metric.
• Phase attribution with zero hot-path changes. The existing
  lean_block_proposal_attestation_build_phase_seconds histogram accumulates exact f64
  sums, observed once per phase per build, so the harness can delta the per-label sums
  between iterations instead of adding instrumentation.
• Statistics policy. Outliers are never auto-discarded — XMSS rejection sampling and
  OTS window advancement produce legitimate tails — and a CV above 10% is flagged rather
  than smoothed.
• The CLI seam. How a second entry point appears without disturbing the flat node
  invocation the Dockerfile, lean-quickstart, the hive shim and the devnet skills all use,
  and why the clap-native subcommand_negates_reqs approach was rejected.
• Milestones. M1 mock crypto, M2 real XMSS/leanVM pools + seal phase, M3
  replay-from-datadir.

Correctness / Behavior Guarantees

Documentation only.

Tests Added / Run

None; no code changes.

Related Issues / PRs

• Implements the design reviewed in feat(cli): accept a node sub-command for running the node #591 (CLI seam) and the two PRs that follow it
• Replaces the doc that shipped inside the now-closed feat(cli): add offline block-building benchmark subcommand #497
• 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

Review of PR #594 — Block Building Benchmark Plan

Overall Assessment: Well-structured design document with clear milestones. Several implementation details need correction before M2/M3 to avoid performance overhead and consensus risks.

Critical Issues

1. Incorrect RocksDB constraint (Section: Harness design)

• Line 95: Claims "RocksDB has no read-only mode" as justification for mandatory datadir copying.
• Issue: RocksDB does support read-only mode via DB::open_for_read_only(). Copying multi-GB datadirs adds unnecessary I/O overhead and risks copying inconsistent state if the source node is running.
• Recommendation: Use DB::open_for_read_only(&opts, path, error_if_wal_file_exists) for replay mode. Only copy if the user explicitly requests a writeable fork.

2. Mock crypto safety boundary (Section: Harness design)

• Line 92: --mock-crypto produces empty proofs and skips seal phases.
• Risk: Without compile-time guards, mock crypto could accidentally be enabled in production builds if CLI parsing errors occur.
• Recommendation: Gate mock-crypto behind #[cfg(test)] or a dedicated bench-mock feature flag, never available in release binaries.

Consensus & Security Concerns

3. Determinism guarantees (M1 deliverables)

• Line 117: Fixes extend_proofs_greedily HashSet non-determinism by breaking ties to lowest pool index.
• Issue: Other HashSet/HashMap usages in the proposer pipeline (attestation aggregation, fork-choice store) may introduce similar non-determinism under rayon parallel iteration.
• Recommendation: Audit all collections in the hot path. Use BTreeSet/BTreeMap or indexmap with fnv/ahash + seeded hasher for deterministic iteration order across runs.

4. seal_block extraction risks (Library refactor section)

• Lines 101-108: Moving lines 504-631 from lib.rs into a new seal.rs.
• Risk: "Six repeated error-return-with-metric blocks" collapsing into one match could lose granularity in failure mode detection during live consensus.
• Recommendation: Ensure the refactored function preserves distinct error variants (not just SealBlockError) so callers can still distinguish between signing failures vs leanVM failures vs type-2 merge failures for metric attribution.

Performance & Correctness

5. Histogram sampling thread safety (Phase capture section)

• Lines 34-37: Plans to read get_sample_sum() from prometheus HistogramVec between iterations.
• Issue: If rayon worker threads are still updating histograms when get_sample_sum() is called, readings may be inconsistent (prometheus Histogram uses atomic counters but iteration boundaries may race with background aggregation).
• Recommendation: Explicitly drop or sync the rayon threadpool between iterations, or use a local Histogram instance per iteration rather than the global Prometheus registry.

6. Datadir consistency during copy (Replay mode)

• Line 96: Mentions copying datadir before opening with --no-copy opt-out.
• Issue: Copying while the source node is running (even with filesystem snapshots) can result in corrupted SSTables or WAL files.
• Recommendation: Document that replay mode requires the source node to be stopped, or implement RocksDB checkpointing (db.create_checkpoint()) instead of full copy.

Code Quality

7. CLI token dispatch fragility (CLI section)

• Lines 47-54: Manually removes leading token before parsing.
• Issue: This bypasses clap's built-in validation. If argument order changes or subcommands are nested later, manual token manipulation breaks.
• Recommendation: Use clap::Parser with #[command(subcommand)] and Option<Command> as originally considered, accepting the unwrap() churn in the node path. The "churn" is safer than manual argv manipulation.

8. XMSS key window validation (Keys section)

• Line 84: Mentions minimal window of 131,072 epochs.
• Issue: If the benchmark runs longer than expected (high iteration counts), keys might exhaust their OTS windows during long-running benchmarks.
• Recommendation: Add a runtime check that (warmup_slots + iterations) < (window_size * 2 / 3) (safety margin) before keygen.

Minor Suggestions

9. Schema versioning (Report section)

• Line 108: JSON output includes schema_version.
• Suggestion: Pin this to the ethlambda crate version or use a separate benchmark schema version constant. Document backward compatibility guarantees for external tooling consuming these JSONs.

10. Memory backend cleanup (Synthetic corpus)

• Line 89: Uses InMemoryBackend.
• Suggestion: Ensure InMemoryBackend is dropped between iterations to prevent accidental state leakage, or explicitly document that the harness relies on fresh backend instances per iteration.

11. Error handling in synchronous main (CLI section)

• Line 58: Benchmark runs on main thread without tokio.
• Suggestion: Ensure all blockchain crate functions called by the benchmark are block_on compatible or purely synchronous. Async code accidentally called from the benchmark path will panic or deadlock without a runtime.

Summary

The plan is architecturally sound but Item 1 (RocksDB read-only) eliminates a major performance bottleneck in replay mode. Item 3 (determinism) is critical for the "identical block-root sequences" verification gate to be meaningful across different machines. Address these before M2 implementation.

The seal_block extraction (Item 4) is high-risk for consensus; recommend a dedicated PR with property-based testing comparing old vs new function outputs for random valid/invalid inputs.

---

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

[github-actions]

🤖 Codex Code Review

Findings:

1. docs/plans/block-building-benchmark.md contradicts itself about the benchmark corpus shape. At line 80 it says there is no --pool-datas knob, but lines 103-105 still define the synthetic pool as “the last --pool-datas slots”. That leaves the planned workload underspecified and would likely produce mismatched implementation/tests. Either restore the knob in the plan or replace that text with the actual source of K.

2. The plan hard-codes source line numbers that are already stale, which is risky for a consensus-critical refactor doc. Line 20 points to store.rs:788, but produce_block_with_signatures is currently at crates/blockchain/src/store.rs:904. The same applies to the lib.rs:504-631 references at lines 24 and 123. For maintainability, refer to symbols/files instead of exact line numbers.

3. The risk/change surface is understated. Lines 31-35 claim “zero hot-path changes,” but lines 123-129 explicitly propose extracting seal_block and adding new proposer-phase metric labels on the production path. Given the consensus sensitivity of block production, the doc should describe this as a proposer hot-path refactor plus instrumentation change, not a zero-hot-path approach.

No executable code is changed in this PR, so I don’t have code-correctness, security, memory-safety, or consensus-behavior findings beyond the documentation accuracy issues above.

---

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

[github-actions]

🤖 Claude Code Review

Review: docs/plans/block-building-benchmark.md

This is a documentation-only PR (a design plan, no code changes), so the review focuses on whether the plan accurately reflects the current codebase — since two follow-up PRs will implement against it.

Findings

1. Refactor target line range doesn't match the described code (crates/blockchain/src/lib.rs:504-631)

The doc states the "sign → wrap_proposer_type1 (leanVM) → merge_type_2 (leanVM)" logic to be extracted into seal_block lives at lib.rs:504-631. In current main, lines 504-631 are the interval-2 aggregation-session-start logic (snapshot_aggregation_inputs, max_jobs, publish_at deadline setup) — unrelated to signing or sealing. The actual sign/wrap/merge sequence lives in propose_block, roughly lines 745-891 (crates/blockchain/src/lib.rs:745-891), which also uses the function names ethlambda_crypto::aggregate_signatures and ethlambda_crypto::merge_type_1s_into_type_2 — not wrap_proposer_type1/merge_type_2 as named in the doc.

This matters because the "one library refactor" section is presented as verified/scoped ("Verbatim move... own commit") and M2 depends on it — a wrong line range and made-up function names will send the M2 implementer to the wrong place, or at minimum require them to re-derive the plan from scratch.

2. M1's "small library fix" (tie-break determinism) appears to already be implemented

The M1 milestone row describes fixing extend_proofs_greedily's HashSet-based candidate set (nondeterministic tie-breaking) as new work to land in M1, listing block_builder.rs as a touched file. Current main (crates/blockchain/src/block_builder.rs:799-825) already uses a Vec<usize> with max_by_key((count, Reverse(idx))) and has an explicit comment about avoiding HashSet iteration-order nondeterminism, plus a dedicated test extend_proofs_greedily_breaks_coverage_ties_by_pool_order. If this fix has already merged, the M1 scope/file list is stale and should be trimmed before the M1 PR is written against this doc.

3. Minor: line-number citations are a maintenance liability

produce_block_with_signatures is cited at store.rs:788; it's actually at store.rs:904 (confirmed still pub fn, so that part is correct). Combined with Finding 1, this suggests the doc's line references were captured against a different snapshot of the tree than what's in main now. Since this doc will outlive several merges before M1/M2 land, consider anchoring references to function/symbol names only (as most of the doc already does) rather than exact line numbers, or add a note that ranges are approximate.

What's solid

• Verified accurate against current code: clap is indeed 4.6.1 (Cargo.lock), on_block_without_verification exists in crates/blockchain/src/store.rs:577, Store::from_db_state exists in crates/storage/src/store.rs:616, and the lean_block_proposal_attestation_build_phase_seconds HistogramVec with select_payloads/compact/stf_simulate labels exists exactly as described in crates/blockchain/src/metrics.rs / block_builder.rs.
• The design reasoning itself is sound: reusing the existing phase histogram instead of adding hot-path instrumentation, refusing to auto-discard outliers given XMSS's legitimate tails, and the token-based CLI dispatch over subcommand_negates_reqs (avoiding turning seven required args into Option<T>) are all well-justified engineering calls with clear tradeoffs stated.
• Scope boundaries (excluding gossip publish/sleep/import, matching the node's own time_block_building metric) are consistent with how propose_block actually times things (drop(timing) before the alignment sleep in lib.rs).

Recommend fixing Findings 1 and 2 before this doc is used as the basis for the M1/M2 PRs, since both are concrete factual drift between the plan and main rather than stylistic nits.

---

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