tooling(recursion): count keccak hashes to verify a proof (excl. grin… - #987
tooling(recursion): count keccak hashes to verify a proof (excl. grin…#987ColoCarletti wants to merge 1 commit into
Conversation
…ding) A host-only metric for the recursion guest's dominant cost. crypto::hash_metrics counters fire on every keccak-256 finalize (the host PlatformKeccak256 wrapper) — Merkle trees, the Fiat-Shamir transcript, the program-id/ELF fold — EXCEPT the grinding proof-of-work check, excluded at its call site in the verifier. The Merkle backend splits its share into node (auth-path) vs leaf finalizes. test_count_recursion_hashes verifies the dumped recursion blob (/tmp/recursion_input.bin) and prints total(excl. grinding) / merkle(nodes,leaves) / transcript+other. Loop: change the prover, re-dump, re-count, compare. Zero-cost on the riscv64 guest (compiled out); disabled by default on the host (one relaxed atomic load per hash when on).
|
/ai-review |
Codex Code Review
No security or execution-correctness issues found in the reviewed changes. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| impl Update for PlatformKeccak256 { | ||
| fn update(&mut self, data: &[u8]) { | ||
| Update::update(&mut self.0, data); | ||
| } | ||
| } | ||
|
|
||
| impl FixedOutput for PlatformKeccak256 { | ||
| fn finalize_into(self, out: &mut Output<Self>) { | ||
| crate::hash_metrics::count_total(); | ||
| FixedOutput::finalize_into(self.0, out); | ||
| } | ||
| } | ||
|
|
||
| impl Reset for PlatformKeccak256 { | ||
| fn reset(&mut self) { | ||
| Reset::reset(&mut self.0); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Medium (performance): none of the forwarding methods are #[inline]. They are non-generic inherent trait impls defined in crypto, so downstream crates (stark, prover) call them as real cross-crate calls unless LTO kicks in — whereas today PlatformKeccak256 = sha3::Keccak256 resolves to digest's CoreWrapper methods, which are #[inline].
That matters most for update: the Merkle leaf path streams field elements 8 bytes at a time (element.stream_bytes(sink)), so this adds one call per chunk, not per hash, in the prover's hottest loop. And finalize_into now moves the ~200-byte sponge by value through an extra newtype layer — the exact shape the DO-NOT-REFACTOR note in field_element_vector.rs:32-41 says was measured slower.
At minimum add #[inline(always)] to all six forwarding methods. Better, since this is a host-only diagnostic: keep pub type PlatformKeccak256 = sha3::Keccak256; as the default and put the counting wrapper behind a cargo feature, so a normal prover build is provably unchanged.
| // Metric: a Merkle finalize (leaf or node) — the total keccak count is taken | ||
| // at the primitive; this is the Merkle sub-count. No-op on guest / disabled. | ||
| crate::hash_metrics::count_merkle(); |
There was a problem hiding this comment.
Low/Medium (metric accuracy): the Merkle sub-counters only cover this file. FieldElementBackend (backends/field_element.rs, aliased as Keccak256Backend / FriMerkleTreeBackend in stark/src/config.rs:10) hashes via its own hasher.finalize() and is never counted — those hashes land in total and therefore get reported as transcript+other. It looks unused in the current verify path, but the split will silently misreport the day a tree switches backends.
Also, count_merkle here fires for any D, while count_total only fires for keccak. With a non-keccak backend (Poseidon, Sha3_256, Keccak512 in the crypto tests) merkle can exceed total, and the test's total.saturating_sub(merkle) prints a plausible-looking 0 instead of flagging the inconsistency.
Cheapest fix: count in field_element.rs too, and have the test assert merkle <= total / nodes <= merkle rather than saturating.
| // Exclude the proof-of-work check from the host hash metric | ||
| // (`crypto::hash_metrics`): the metric asks for "all hashes except | ||
| // grinding". No-op on the guest and whenever counting is off. | ||
| let hm_was = crypto::hash_metrics::is_enabled(); | ||
| crypto::hash_metrics::disable(); | ||
| let nonce_is_valid = proof.nonce().is_some_and(|nonce_value| { | ||
| grinding::is_valid_nonce(&challenges.grinding_seed, nonce_value, grinding_factor) | ||
| }); | ||
| if hm_was { | ||
| crypto::hash_metrics::enable(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Low (simplicity): this whole exclusion — 12 lines of global save/restore in shared verifier code, plus the is_enabled API and its guest stub — removes exactly 2 hashes per proof. is_valid_nonce is get_inner_hash (1 finalize) + is_valid_nonce_for_inner_hash (1 finalize); against the ~900k in the PR description that is noise, and grinding verification is work the recursion guest actually does, so excluding it makes the proxy slightly less faithful, not more.
Suggest dropping the exclusion entirely (and is_enabled with it): it keeps measurement state out of the verifier, and the headline number stays "every keccak the verifier does".
Secondary, if it stays: the toggle is a process-global, so a rayon-parallel verify would drop any hashes other threads finalize inside this window, and a concurrent second verify would race the restore.
| let preset_name = | ||
| std::env::var("RECURSION_DUMP_PRESET").unwrap_or_else(|_| "blowup4".to_string()); |
There was a problem hiding this comment.
Low (usability): two footguns for the documented loop.
- This defaults to
blowup4, buttest_dump_recursion_inputdefaults tomin— running the pair with no env at all produces a verify failure rather than a count. Default to"min"to match the producer. - It always calls
verify_continuation_and_attest, which only accepts a continuation blob. A dump made withoutRECURSION_DUMP_EPOCH_LOG2(the dump test's default branch) fails insiderkyv::accessand surfaces as.expect("verify_continuation_and_attest errored")with an opaque validation message. Worth saying "requires a dump made withRECURSION_DUMP_EPOCH_LOG2set" in the doc comment, and pointing the panic message at that.
Also: enable() is not restored if the verify below panics, leaving counting on for the rest of the test process. Minor, but a disable() before the assert!/expect (or just disabling first thing after snapshot) avoids it.
Review: keccak hash-count metricUseful, well-scoped diagnostic, and the guest-side reasoning checks out: everything is Findings (all inline): Medium — performance: the host Low/Medium — metric accuracy: Low — simplicity: the grinding exclusion (12 lines of global save/restore in shared verifier code + the Low — usability: the new test defaults to Nit: the guest stubs omit |
AI ReviewPR #987 · 6 changed files Findings
Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro). AI-002: Merkle counters fire for non-keccak digests, breaking total/merkle invariant
Claim
Evidence
Suggested fix Only count Merkle activity when the digest is actually the platform keccak hasher. Import AI-004: Hash-metrics disable/enable around grinding is not exception-safe
Claim The verifier disables Evidence crypto/stark/src/verifier.rs:1670-1677 stores the prior state, calls Suggested fix Replace the manual disable/enable pair with a small scope guard whose Reviewer Lanes
Verification Lanes
Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report. Discarded candidates (2) — rejected by the verifier
Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts. |
1 participant
Recursion hash-count metric (excl. grinding)
What
A host-only diagnostic that counts the keccak-256 hashes done to verify a proof —
a fast, deterministic proxy for the recursion guest's dominant cost (Merkle path
hashing + Fiat-Shamir). Lets you tell whether a prover-side change actually reduces
the work the recursion verifier does, without building or running the guest.
How it works
crypto::hash_metricsmodule, incremented at thPlatformKeccak256primitive on every keccak-256finalize— so it catchesall verify-side hashing: Merkle trees, the Fiat-Shamir transcript,
program-id/ELF fold.
is_valid_noncecheck withhash_metrics::disable()/ re-enable().(
nodes= auth-path compressions,leaves) vstranscript+other.(
enable/disable), off by default (one relaxed atomic load per hash whenon). Compiled out entirely on the riscv64 guest
(
#[cfg(not(target_arch = "riscv64"))]): the guest pays nothing and its keccakstays byte-identical (the counter is a pure side effect).
How to use
Dump a proof blob once (the slow step — it proves):
RECURSION_DUMP_PRESET=blowup4 RECURSION_DUMP_EPOCH_LOG2=22
RECURSION_DUMP_INNER_ELF=.elf RECURSION_DUMP_INNER_INPUT=.bin
cargo test --release -p lambda-vm-prover --lib
test_dump_recursion_input -- --ignored
→ writes
/tmp/recursion_input.bin.Count (fast, ~1s — verify only):
RECURSION_DUMP_PRESET=blowup4
cargo test --release -p lambda-vm-prover --lib
test_count_recursion_hashes -- --ignored --nocapture
→ prints, e.g.:
[hash-count] preset=blowup4 blob=B fri_queries=110 | total(excl. grinding)=900311
| merkle=896220 (nodes=810830 leaves=85390) | transcript+other=
Loop: change the prover → re-dump → re-count → compare
total.Env vars:
RECURSION_DUMP_PRESET(must match the dump, else verifyRECURSION_INPUT_PATH(default/tmp/recursion_input.bin).Notes / limitations
internal permutations.
(preset, query count, epochs, and which prover produced it).
Files
crypto/crypto/src/hash_metrics.rs(new) — counters + enable/disacrypto/crypto/src/hash/platform_keccak.rs— hostPlatformKeccak256wrapssha3::Keccak256and counts on finalize.crypto/crypto/src/merkle_tree/backends/field_element_vector.rs— Merklenode/leaf sub-counters.
crypto/stark/src/verifier.rs— grinding exclusion.prover/src/tests/recursion_smoke_test.rs— `test_count_recursion