feat(cli): benchmark block building with real XMSS and leanVM crypto

[pablodeymo]

🗒️ Description / Motivation

ethlambda benchmark synthetic only ran with --mock-crypto, so it measured selection, compaction and the state transition but none of the signing or aggregation that dominates a real proposal. This makes real crypto the default and measures the proposer's seal.

Without --mock-crypto the run derives an attestation and a proposal XMSS key per validator from the seed (sized to the slots the run signs, so keygen costs seconds; --key-cache <dir> stores them for reruns), puts the real pubkeys in the synthetic genesis, has every validator sign each slot's attestation data through the production KeyManager, aggregates each participant group into a real leanVM type-1 proof, and seals the built block with seal_block. The measured span is build plus seal, exactly the node's lean_block_building_time_seconds boundary.

What Changed

File	Change
crates/blockchain/src/block_builder.rs	seal_block + SealError, lifted verbatim from propose_block; observes sign_proposer, wrap_proposer, merge_type2
crates/blockchain/src/lib.rs	propose_block calls seal_block, same logging and failure counter on error
crates/blockchain/src/metrics.rs	BLOCK_PROPOSAL_SEAL_PHASES; the histogram now spans the same work as lean_block_building_time_seconds
bin/ethlambda/src/benchmark/keys.rs	Seed-derived XMSS keys sized to the run, optional --key-cache <dir> keyed by leansig rev/seed/index/window
bin/ethlambda/src/benchmark/corpus.rs	CryptoMode::{Mock, Real}: real genesis pubkeys, real type-1 proofs signed through the production KeyManager
bin/ethlambda/src/benchmark/mod.rs	Drop the mock requirement; seal in the measured span; verified on_block import in real mode
bin/ethlambda/src/benchmark/report.rs	aggregate and import columns (outside the span)
docs/benchmarking.md, docs/metrics.md	Real-mode docs, sample output, new phase labels

Correctness / Behavior Guarantees

• propose_block behavior is unchanged; the seal is the same code behind a function.
• Mock mode is unchanged and still what CI and make bench run; it skips the seal since there are no keys, so its report carries only the build phases.
• Real mode imports every built block through on_block, so an invalid proof fails the run instead of producing a report about invalid blocks.
• Same seed and parameters reproduce the same roots: keys are seed-derived and XMSS signing is deterministic.
• Node metric change: three new phase label values on lean_block_proposal_attestation_build_phase_seconds.

Tests Added / Run

• keys.rs: determinism per seed and role; cache round-trip that signs through the KeyManager.
• corpus.rs: #[ignore = "too slow"] test that a seeded proof passes verify_aggregated_signature (passes locally).
• Real run, 2 validators, 1 warmup, 2 iterations: keys in 6.8s, ~0.1s per slot of type-1 aggregation, 0.5–1.3s type-2 merges, 20ms verified imports; a rerun loaded the keys from cache and produced identical block roots. --enable-proposer-aggregation --proofs-per-data 2 exercised a real recursive compaction (compact = 0.56s).
• Mock CI smoke contract still holds (schema_version == 1, one sample per iteration, build phases only).

ethlambda benchmark synthetic --num-validators 2 --warmup-slots 1 --iterations 2 --key-cache /tmp/bench-keys

Related Issues / PRs

• Follows up feat(cli): add offline block-building benchmark sub-command #595 / feat(cli): make benchmark reports comparable across runs and machines #596 (benchmark harness and report), which left real crypto as a documented limitation.

✅ 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 on the latest leanSpec fixtures

[github-actions]

🤖 Kimi Code Review

I'll review this PR which adds real XMSS/leanVM cryptography to the benchmark harness, replacing the previous mock-only mode. Let me analyze the changes systematically.

Overall Assessment

This is a well-structured PR that extends the benchmark to support real cryptographic operations. The code is generally clean, but I've identified several issues ranging from minor to potentially serious.

---

Critical Issues

1. Potential Panic on Out-of-Bounds Access in genesis_store (bin/ethlambda/src/benchmark/corpus.rs:74)

CryptoMode::Real {
    genesis_pubkeys, ..
} => genesis_pubkeys[index as usize],

Problem: No bounds check. If num_validators doesn't match genesis_pubkeys.len(), this panics. The CryptoMode::Real is constructed externally in run_synthetic where num_validators is passed to both KeySet::generate and SyntheticCorpus::new, but this invariant isn't enforced or documented in SyntheticCorpus.

Fix: Add debug_assert! or proper error handling:

genesis_pubkeys.get(index as usize)
    .copied()
    .unwrap_or_else(|| panic!("genesis_pubkeys length {} < num_validators {}", genesis_pubkeys.len(), self.num_validators))

Or better, validate at construction time in SyntheticCorpus::new.

---

2. Incorrect Epoch-to-Slot Conversion in Key Generation (bin/ethlambda/src/benchmark/mod.rs:179)

let keys = keys::KeySet::generate(
    options.seed,
    options.num_validators,
    total_slots + 1,  // Claimed as "total_slots + 1 epochs"
    options.key_cache.as_deref(),
)?;

Problem: The comment says "keys must be active for total_slots + 1 epochs" but KeySet::generate takes num_slots (not epochs), and generate_key passes this directly to LeanSignatureScheme::key_gen(&mut rng, 0, num_active_epochs) where the parameter is named num_active_epochs.

Looking at KeySet::generate (line 67):

let num_active_epochs = usize::try_from(num_slots)

This is slot/epoch confusion. If num_slots is 1000, you're requesting 1000 epochs of keys, not 1000 slots. This could cause:

• Massive key generation overhead (1000 epochs × 65,536 slots/epoch worth of OTS keys)
• Or, if leansig interprets this differently, potential runtime failures

Fix: Clarify the API. If KeySet::generate truly takes epochs, pass total_slots / SLOTS_PER_EPOCH + 1. If it takes slots, rename the parameter and fix the comment.

---

3. Unsafe RNG Seeding in generate_key (bin/ethlambda/src/benchmark/keys.rs:183)

let mut rng = StdRng::seed_from_u64(seed ^ (index << 1 | role_bit).rotate_left(32));

Problem:

1. index << 1 can overflow for large validator indices (though u64::MAX << 1 would wrap in debug mode, it's still poor practice)
2. The seed construction is fragile—rotate_left(32) on a 64-bit value with small index values means the high 32 bits are mostly zero, reducing entropy

Fix: Use a more robust combiner:

use std::hash::{Hash, Hasher, DefaultHasher};
let mut hasher = DefaultHasher::new();
seed.hash(&mut hasher);
index.hash(&mut hasher);
role_bit.hash(&mut hasher);
let mut rng = StdRng::seed_from_u64(hasher.finish());

Or use rand::SeedableRng::from_seed with a [u8; 32] derived from a cryptographic hash.

---

Security Issues

4. Cache File Has No Integrity Protection (bin/ethlambda/src/benchmark/keys.rs:89-96)

let mut bytes = pubkey.to_vec();
bytes.extend_from_slice(&secret);
std::fs::write(file, bytes)

Problem: Secret keys are written to disk with no authentication, integrity check, or encryption. A corrupted or maliciously modified cache file will be loaded and used.

The load_cached function only checks bytes.len() > PUBKEY_LEN but doesn't verify the pubkey matches the secret, or that the file wasn't truncated at a valid boundary.

Fix:

1. Add a checksum (BLAKE2b or similar) over the entire entry
2. Verify pubkey-secret correspondence on load (derive pubkey from secret and check)
3. Consider file permissions (though this is benchmark tooling, not production)

At minimum, add this validation in load_cached:

// After loading, verify secret decodes to the claimed pubkey
let sk = ValidatorSecretKey::from_bytes(secret)
    .map_err(...)?;
let derived_pk = sk.to_public_key(); // if available
let claimed_pk = ValidatorPublicKey::from_bytes(pubkey)
    .map_err(...)?;
assert_eq!(derived_pk.to_bytes(), claimed_pk.to_bytes(), "cache corruption detected");

---

5. Cache Directory Race Condition (bin/ethlambda/src/benchmark/keys.rs:58-60)

if let Some(dir) = cache {
    std::fs::create_dir_all(dir)
        .wrap_err_with(|| format!("failed to create key cache {}", dir.display()))?;
}

Problem: create_dir_all followed by is_file() checks in a loop is racy. Two benchmark processes with the same cache could both decide to generate keys, then one overwrites the other's file.

Fix: Use atomic file creation (std::fs::OpenOptions::new().create_new(true)) or a lock file. For benchmark tooling this may be acceptable, but document it.

---

Correctness Issues

6. Missing Slot Check in seal_block (crates/blockchain/src/block_builder.rs:940)

let slot: u32 = block.slot.try_into().expect("slot exceeds u32");

Problem: This expect can panic. While unlikely in practice, consensus code should never panic on untrusted inputs. The block here is built locally, but this function is now public and could be called with arbitrary blocks.

Fix: Return SealError instead:

let slot: u32 = block.slot.try_into()
    .map_err(|_| SealError::SlotOverflow(block.slot))?;

Add #[error("slot {0} exceeds u32")] SlotOverflow(u64), to SealError.

---

7. Incorrect wrap_err_with Usage (bin/ethlambda/src/benchmark/corpus.rs:127)

let bytes = key_manager
    .sign_attestation(validator, &data)
    .wrap_err_with(|| {
        format!("validator {validator} failed to sign slot {slot}")
    })?;

Problem: wrap_err_with captures the closure's environment. Here slot is already computed, but this pattern is used correctly. However, note that wrap_err_with is from eyre::WrapErr which is imported—this is fine.

Actually, looking more carefully: the slot variable is attestation_slot.try_into().expect(...) at line 119. If that expect fires, the benchmark crashes. This should be handled properly.

---

8. Potential Integer Overflow in total_slots Calculation (bin/ethlambda/src/benchmark/mod.rs:162-165)

let total_slots = options
    .warmup_slots
    .checked_add(common.iterations)
    .ok_or_else(|| eyre::eyre!("--warmup-slots plus --iterations overflows u64"))?;

Good—this is checked. But then:

total_slots + 1  // line 179

This can overflow! total_slots is already warmup_slots + iterations, and + 1 can wrap.

Fix:

let key_window_slots = total_slots.checked_add(1)
    .ok_or_else(|| eyre::eyre!("key window overflows u64"))?;

---

Performance Issues

9. Repeated hash_tree_root() Calls (bin/ethlambda/src/benchmark/corpus.rs:118)

let data = produce_attestation_data(store, attestation_slot);
// ...
let message = data.hash_tree_root();  // in Real branch
// ...
let data = produce_attestation_data(&store, 0);  // in test
// later: data.hash_tree_root() again

Problem: hash_tree_root() is called multiple times on the same data. SSZ hashing is expensive.

Fix: Cache the result. In the test at line 280 and line 302, data is recreated and re-hashed.

---

10. Inefficient Vec Construction in seal_block (crates/blockchain/src/block_builder.rs:985-990)

let mut merge_inputs = Vec::with_capacity(single_message_aggregates.len() + 1);
for sma in single_message_aggregates {
    let pubkeys = sma
        .participant_indices()
        .map(|vid| { /* ... */ })
        .collect::<Result<Vec<_>, _>>()?;
    merge_inputs.push((pubkeys, sma.proof.clone()));
}
merge_inputs.push((vec![proposer_pubkey], proposer_proof_bytes));

Problem: sma.proof.clone() clones every proof. merge_type_1s_into_type_2 likely consumes these; check if it can take ownership.

Actually, looking at the signature: merge_type_1s_into_type_2(merge_inputs)—if this takes Vec<(Vec<ValidatorPublicKey>, ByteList512KiB)>, the clones are necessary unless SingleMessageAggregate::proof can be moved out. But in the current context, single_message_aggregates is &[SingleMessageAggregate], so clone is required.

However, in BlockChainServer::propose_block, the original code had sma.proof.clone() too, so this isn't a regression.

---

Code Quality Issues

11. Error Type Inconsistency (crates/blockchain/src/block_builder.rs:900-921)

#[derive(Debug, thiserror::Error)]
pub enum SealError {
    #[error("failed to sign block root: {0}")]
    Signing(#[from] KeyManagerError),
    // ...
    #[error("failed to decode proposer signature bytes: {0}")]
    ProposerSignature(String),
    // ...
    #[error("failed to build multi-message aggregate: {0}")]
    Decode(String),
}

Problem: Some variants wrap String, others wrap structured errors. The String variants lose error context and make programmatic handling impossible.

Fix: Consider more structured errors, or at minimum document that these strings are opaque. The Decode(String) variant taking err.to_string() at line 1002 is particularly lossy.

---

12. Missing Documentation on seal_block Panic Conditions

The function has expect("slot exceeds u32") which is a panic condition not documented in the doc comment. See Point 6.

---

13. Test Uses unwrap() Heavily (bin/ethlambda/src/benchmark/keys.rs:247-250)

let mut key_manager = second.into_key_manager().unwrap();
// ...
key_manager.sign_block_root(...).expect("cached key signs within its window");
std::fs::remove_dir_all(&dir).unwrap();

Acceptable for tests, but the remove_dir_all at the end—if prior code panicked, this won't run. Use tempfile crate or a RAII guard.

---

14. Inconsistent ValidatorPubkeyBytes Type Assumptions (bin/ethlambda/src/benchmark/keys.rs:16)

const PUBKEY_LEN: usize = std::mem::size_of::<ValidatorPubkeyBytes>();

Problem: This assumes ValidatorPubkeyBytes is a fixed-size array, not a Vec<u8> or other dynamically-sized type. If ValidatorPubkeyBytes is H256 or similar, this works; if it's ever changed to Vec<u8>, size_of returns 24 (pointer + length + capacity on 64-bit), not the actual length.

Fix: Use ValidatorPubkeyBytes::len() or a constant from the type definition:

const PUBKEY_LEN: usize = ValidatorPubkeyBytes::len(); // if available
// or
const PUBKEY_LEN: usize = 32; // if it's H256, use the actual constant

---

Minor Issues

15. Unused Import (crates/blockchain/src/lib.rs diff)

The diff removes ValidatorPublicKey and ValidatorSignature imports, and ByteList512KiB, MultiMessageAggregate. Verify no other code in the module needs these—looks correct since seal_block now handles all crypto.

16. Metric Name Now Misleading (crates/blockchain/src/metrics.rs:507)

The histogram is still named lean_block_proposal_attestation_build_phase_seconds but now includes seal phases. The docs are updated, but the metric name is misleading. Consider if this is acceptable for backward compatibility.

---

Positive Observations

1. Good error context: The wrap_err_with calls provide clear operation context
2. Deterministic key generation: The seeded RNG approach enables reproducible benchmarks
3. Cache invalidation: Including leansig revision in cache keys is correct
4. Phase timing separation: aggregate_seconds and import_seconds are correctly kept outside the measured wall span
5. Real verification in real mode: Using on_block instead of on_block_without_verification for real crypto catches seal bugs
6. Clean extraction of seal_block: The refactoring from inline in BlockChainServer to a testable function is well-done

---

Summary Table

Priority	Item	Location	Issue
Critical	2	benchmark/mod.rs:179	Slot/epoch confusion in key window
Critical	6	block_builder.rs:940	expect can panic on slot conversion
High	1	corpus.rs:74	Unbounded indexing into genesis_pubkeys
High	8	benchmark/mod.rs:179	total_slots + 1 can overflow
High	3	keys.rs:183	Weak RNG seed construction
Security	4	keys.rs:89-96	Unauthenticated secret key cache
Security	5	keys.rs:58-60	Cache directory race condition
Medium	14	keys.rs:16	size_of assumption on pubkey type
Medium	11	block_builder.rs:900	String-based error variants
Low	9	corpus.rs	Redundant hash_tree_root calls
Low	13	keys.rs:247	Test cleanup not guaranteed on panic

The most important fix is Item 2 (slot/epoch confusion), as it fundamentally misunderstands the leansig API and could cause massive performance degradation or incorrect key generation.

---

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

[github-actions]

🤖 Claude Code Review

Review: Benchmark block building with real XMSS and leanVM crypto

Overall this is a well-scoped, well-tested change. The seal_block extraction is a clean, behavior-preserving lift out of propose_block, the new keys.rs module has sensible determinism/cache-invalidation design (keyed by leansig revision + seed + index + role + window), and the benchmark now exercises the real signing/aggregation path with verified import (on_block) rather than skipping validation. Tests cover determinism and cache round-trips, and the expensive crypto-verifying test is correctly marked #[ignore] per project convention.

A few points worth a look, none blocking:

1. crates/blockchain/src/lib.rs:1144-1157 — consolidated error logging loses granularity. The original code logged a distinct message per failure mode ("Failed to decode proposer proposal pubkey", "Failed to merge Type-1s into Type-2", etc.). Now every seal failure logs the single message "Failed to seal block" with detail folded into %err via SealError's Display. The information is still present, but if any dashboards/alerts grep on the old distinct strings, they'll stop matching. Worth a mention in the PR/changelog if such alerting exists.

2. crates/blockchain/src/block_builder.rs:1006 — block.slot.try_into().expect("slot exceeds u32") replaces the old slot as u32 truncating cast. This is actually a correctness improvement (a silent truncation of the slot before it's used as the XMSS signing epoch could theoretically cause epoch collisions across slots N and N + 2^32), but it does introduce a new panic path in propose_block's hot loop where none existed before. Since slot numbers won't realistically approach u32::MAX for centuries at 4s/slot, this is a non-issue in practice — just flagging the behavior change for the record.

3. bin/ethlambda/src/benchmark/keys.rs:390-395 — cached secret key material is written with std::fs::write and no explicit file permissions, so cache files inherit the process umask (often world-readable, e.g. 0644). These are synthetic, seed-derived keys rather than production validator secrets, so the risk is low, but since this establishes a "cache raw secret key bytes to disk" pattern that could be copy-pasted elsewhere, restricting permissions (e.g. 0600 on Unix) would be a reasonable defense-in-depth addition.

4. Minor/nit — bin/ethlambda/src/benchmark/corpus.rs:167: aggregate_start is created unconditionally before the match self.crypto, but only read in the Real arm (aggregate_seconds is hardcoded to 0.0 in Mock). Harmless (one extra Instant::now() call), but could be moved inside the Real arm for clarity.

Nothing found in the fork-choice/state-transition/attestation-processing logic itself, since this PR only touches the benchmark harness and the (behavior-preserving) seal_block extraction — no changes to consensus-critical paths beyond that lift.

---

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

[github-actions]

🤖 Codex Code Review

⚠️ The review job did not complete (result: cancelled). See the job log

---

Automated review by OpenAI Codex