perf(syscalls,crypto): in-place keccak sponge + direct finalize + fixed-shape parents (−40% recursion-verifier cycles at real query counts)#847
Conversation
|
/review-ai |
Codex Code ReviewNo actionable issues found in the PR changes. The unsafe aligned lane read is correctly guarded, bounded, and valid for the little-endian RISC-V guest target. |
|
Reviewed the in-place Keccak sponge rewrite (single file, Verified the equivalence reasoning:
The |
AI ReviewPR #847 · 1 changed files FindingsNo non-rejected structured findings were reported. Reviewer Lanes
Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report. Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts. |
|
/bench-verify |
|
⏳ Benchmark started on the bench server. The verifier bench takes ~5 min; the recursion-guest cycle comparison then adds guest builds — a few minutes when cached, up to ~1h on a cold run. The bench server is occupied until it finishes. |
Verifier benchmark —
|
| Metric | main | PR | Δ |
|---|---|---|---|
| Verify time | 3.588s | 3.604s | +0.47% 🔴 |
| Proof size | 204.30 MiB | 204.30 MiB | +0.00% ⚪ |
pairs: 20 mean A (PR): 3.604s mean B (main): 3.588s
[parametric] paired-t mean +0.47% sd 0.86% se 0.19%
95% CI: [+0.06%, +0.87%] (t df=19 = 2.093)
[robust] median +0.35% Wilcoxon W+=159 W-=31 p(exact)=0.0082 (z=+2.56)
run-to-run jitter: A CV 0.67% B CV 0.59% (lower = steadier)
within-session drift: -0.14% over the run, 1st->2nd half -0.02%
🔴 REAL REGRESSION — PR verifies ~0.47% slower (paired-t and Wilcoxon agree).
Drift-free interleaved A/B/B/A measurement. - = PR faster. Trust the verdict when paired-t and Wilcoxon agree.
Recursion guest cycles (main vs PR)
=== Recursion-guest cycle comparison — single query (blowup=2, 1 query) — deterministic to ~±100k cycles ===
REF_B (baseline) origin/main 3be1eed guest=recursion-min.elf
REF_A (PR) c061b35 c061b35 guest=recursion-min.elf
| Metric | REF_B (baseline) | REF_A (PR) | Δ (A-B) |
|---|---|---|---|
| Guest cycles | 43.6M | 42.3M | -1.3M (-3.08%) |
| Keccak calls | 3025 | 3025 | 0 |
note: cycles reproduce to ~±100k (build codegen + proof nondeterminism); treat sub-100k deltas as noise, not signal.
raw (exact integer counts)
ref_b_sha=3be1eed35bd73e96c36a99bf03b58a87cf358af7 ref_b_elf=recursion-min.elf ref_b_cycles=43605029 ref_b_keccak=3025 ref_b_execute_wall_s=1
ref_a_sha=c061b35539996e8c708fbdd934b8e7d22f512863 ref_a_elf=recursion-min.elf ref_a_cycles=42261337 ref_a_keccak=3025 ref_a_execute_wall_s=1
delta_cycles=-1343692 delta_keccak=0
e0c47d1 to
e2a715d
Compare
The state is the only buffer: input bytes XOR directly into the rate lanes at a running offset (tiny-keccak's design), padding lands in place, and the digest is read straight out of the state. This removes the staging buffer and its zeroing, the full-block copy, the 17-lane byte re-extraction, and the separate pad block — the largest slice of the ~1,322 cycles of software plumbing measured per 1-cycle keccak_permute ecall (measured for this commit alone: −1.545B cycles on the 219-query ethrex-block recursion benchmark; the residual is addressed by the follow-up commits). The whole-lane fast path gates on the input pointer's runtime alignment (the VM traps on unaligned doubleword loads); misaligned input falls back to byte-wise absorption, so correctness never depends on alignment. Digests are byte-identical to the previous implementation.
The field-element Merkle backends finalized every leaf and parent hash through the `Digest::finalize` blanket: it allocates a zeroed `GenericArray`, the riscv64 keccak adapter fills a local `[u8; 32]` and copies it into that `Output`, then the caller copies the `Output` once more into its own node array. That is two 32-byte memcpys plus a 32-byte memset of pure plumbing around the single permutation each hash actually costs. Route the leaf (`hash_data`, `hash_data_from_slices`, `hash_bytes`) and parent (`hash_new_parent`) hashes of `FieldElementPairBackend` and `FieldElementVectorBackend` through a shared `hash_streamed` helper. On the riscv64 guest, when `D` is the platform keccak digest and nodes are 32 bytes, it drives the syscall sponge directly and squeezes straight into the result array — no GenericArray, no intermediate buffer, no double copy. Every other digest / node size and the entire host build keep the generic `Digest` path, so output stays byte-identical (blobs still verify). Recursion verifier guest cycles: min 42,185,756 -> 41,991,672 (-0.46%), blowup8 291,147,000 -> 273,789,625 (-5.96%); keccak permutation counts unchanged (3,025 / 134,173).
Every Merkle parent hash is exactly 64 bytes (two concatenated 32-byte nodes), which fits the keccak rate in a single block. Add `keccak256_pair` to the syscall sponge: it loads the eight data lanes straight from the two nodes, XORs the pad10*1 bits in place, runs one permutation, and squeezes four lanes — skipping the incremental sponge's per-byte absorb, running-offset bookkeeping, and separate padding pass. Route `hash_new_parent` of both keccak field-element backends to it on the riscv64 guest via the same TypeId + node-size dispatch used for the direct finalize; every other digest / node size and the host build keep the generic streaming path, so output stays byte-identical (blobs verify, keccak counts unchanged at 3,025 / 134,173). Cumulative recursion verifier guest cycles after this commit: min 41,973,461, blowup8 271,979,593. C's isolated effect vs the prior commit: min -266,369, blowup8 -14,646,310. (The prior commit is a measured regression DROP-candidate; A and C together, with B dropped, are the recommended stack.)
The sponge's absorption chunking, padding, and squeezing had no unit coverage — its only oracle was end-to-end proof-blob acceptance. Host tests now inject a software Keccak-f[1600] in place of the ecall and check digest byte-identity against sha3::Keccak256: every input length through three rate blocks (all padding boundaries), 300 randomized misaligned-chunking cases, and the fixed-shape pair path against both the reference and the streaming sponge. The guest global allocator registration is now gated to riscv64 so a host `cargo test` of this crate uses the system allocator instead of aborting on the never-initialized guest heap. Guest builds unchanged (cycle counts bit-identical).
… critical-section dev-dep The syscalls crate is excluded from the workspace (riscv-only bare-metal allocator/entrypoints that assemble only for the guest target), so the root `cargo test` never reached its host keccak-vs-sha3 differential tests and CI couldn't run them. Add a `test-syscalls` target that runs `cargo test` in the crate dir and make `test` depend on it. Drop the `critical-section` dev-dependency: the embedded_alloc global allocator is gated to `target_arch = "riscv64"` (src/allocator.rs), so on host test builds it is never the active allocator and its critical-section path is never linked. A clean `cargo test` links and passes without it. critical-section stays in the lock transitively (via riscv/embedded-alloc) but needs no host impl. Commit syscalls/Cargo.lock (now that CI builds this crate standalone) to match the repo convention for excluded crates and pin dev-deps for reproducible tests.
…tdRng tests - update(): gate the whole-lane fast path on cfg!(target_endian = "little"). The raw *const u64 lane read equals the required little-endian value only on LE targets; the cfg! folds to a compile-time constant (codegen unchanged on every real target) and the byte fallback is endian-correct everywhere. - Deduplicate the squeeze: extract squeeze32_into(state, out), shared by Keccak256::finalize and keccak256_pair. Write-into (rather than returning [u8; 32]) so finalize fills its output reference in place — verified guest-codegen and cycle-identical to the pre-dedup loops. - Tests: replace the hand-rolled xorshift RNG with rand's StdRng + SeedableRng (matching src/random.rs), keeping fixed seeds for reproducibility. - Document at the byte-wise fallback that a from_le_bytes lane-assembly middle path was measured at +4.7% cycles (dropped commit f6d575ed) so it is not re-proposed on this 1-cycle-per-instruction VM. Digests remain byte-identical to sha3 Keccak-256 (host differential tests); guest cycles unchanged (recursion-min 41,747,766 / recursion-blowup8 260,967,578, keccak call counts identical).
bd408a2 to
6db4c86
Compare
…r code sites Two refactors that reviewers (and optimization-hunting agents) will keep re-proposing were implemented and measured slower on the guest; pin the numbers where the edit would happen so the effort isn't repeated: - replacing the TypeId dispatch in hash_streamed with a generic Digest::finalize_into route: +0.5% guest cycles at blowup8 across every formulation (by-value sponge through the trait layer, not elidable cross-crate without LTO) - return-value squeeze32: +81k cycles from the extra stack temporary The misaligned lane-assembly absorb path (+4.7%) is already documented at the byte fallback in update().
- CI never invokes 'make test', so the syscalls differential tests still did not run in CI despite the Makefile wiring; run 'make test-syscalls' as a dedicated step in the cli-test job. - Swap the test RNG from StdRng (algorithm unstable across rand releases) to ChaCha8Rng so the fixed seeds pin the exact fuzz case streams across versions and checkouts. - Correct the finalize_into dead-end comment to give both preset percentages (+0.14% min / +0.48% blowup8) instead of one figure. - Cite PR #847 instead of a commit hash unreachable from any ref for the dropped lane-assembly measurement.
The _start entry symbol (and its imported main) only exist for the guest; on a Linux host they collide with the C runtime / test harness entry and 'cargo test' fails with "entry symbol `main` declared multiple times" (macOS tolerates the duplicate, which is why local host tests passed). Same treatment as the global-allocator gating: guest builds are unchanged.
…n the adapter passthrough invariant Review feedback (two reviewers independently): the whole-lane fast path was only exercised because Vec<u8> bases happen to be 8-aligned on current platforms. A repr(align(8)) buffer now guarantees the aligned path and a +1-offset view guarantees the byte fallback, across all padding boundaries. Also documents two load-bearing implicit facts: the TypeId specializations depend on PlatformKeccak256 remaining a pure passthrough of the syscall sponge (INVARIANT note in the adapter), and the host tests' coverage boundary (the ecall and riscv64 branches are validated only by the proof-blob oracle).
|
/bench-prove |
Benchmark — ethrex 20 transfers (median of 3)Table parallelism: auto (cores / 3)
Commit: 636ba67 · Baseline: cached · Runner: self-hosted bench |
What
Removes the software byte-plumbing around the guest's 1-cycle
keccak_permuteecall — flamegraph attribution measured it at ~1,322 cycles per permutation, 48% of all recursion-verifier guest cycles at the multi-query regime. Six commits (three optimizations, tests, and a review-fix pass):syscalls/src/keccak.rs): the[u64; 25]state is the only buffer — input XORs directly into the rate lanes at a running offset, padding lands in place, the digest reads straight out of the state (tiny-keccak's design). Deletes the 136-byte staging buffer + zeroing, a full-block copy, the 17-lane byte re-extraction, the separate pad block. The whole-lane fast path gates on runtime pointer alignment (the VM traps on unaligned doubleword loads); misaligned input falls back to byte-wise absorption — correctness never depends on alignment.crypto/crypto/src/merkle_tree/backends/field_element_vector.rs): the hot Merkle backends skip theDigest::finalizeblanket impl (GenericArray alloc + zeroing + two 32-byte copies per hash) and squeeze straight into the caller's[u8; 32]. TypeId +cfg(target_arch = "riscv64")specialization (same pattern as perf(crypto): specialize keccak256 for the fixed-shape Merkle parent hash #774); host and every non-keccak digest keep the generic path.keccak256_pairfor 64-byte Merkle parents: loads the eight data lanes from the two nodes (byte-assembled — never an aligned load on a byte-aligned buffer), XORs thepad10*1bits, one permutation, squeezes. Skips the incremental sponge entirely for the most frequent hash shape.syscalls/): see Testing below.5.–6. Review-fix pass: tests wired into
make test(test-syscallstarget), endianness guard on the fast path, shared squeeze helper, unused dev-dep dropped, StdRng in tests.Measurements (exact, deterministic)
Fixed (guest ELF, input blob) pairs — guest cycle counts are bit-reproducible, so each delta is an exact integer, no statistics. Blobs dumped once from the base, the same blobs fed to base and PR guests. Base =
main(@ 3be1eed).The real-block row was measured on the #825 stack (
bench/recursion-full-queries), which provides the continuation guest and 219-query preset — this PR is independent of it; the min/blowup8 rows above are measured directly againstmainand reproduce main's baseline integers exactly.Per-commit contribution on the real target: in-place sponge −1.545B, direct finalize −0.674B, fixed-shape pair −0.528B; the test commit is measured cycle-neutral (bit-identical counts). Keccak permutation counts are unchanged in every regime (3,025 / 134,173 / 3,150,816) — this removes plumbing around the permutations, not permutations.
A fourth optimization variant (lane-assembling misaligned absorb input) was measured at +4.7% on blowup8 across three formulations and dropped: on a 1-cycle-per-instruction VM the byte-wise absorb is already minimal and wider gathers only add instructions.
The min → blowup8 → real gradient (−4% → −29% → −40%) is expected: the plumbing scales with hash volume, i.e. queries × tables × tree depth.
Testing
cd syscalls && cargo test): a software Keccak-f[1600] is injected in place of the ecall (test-only cfg; guest and host non-test builds untouched) and the sponge is checked for digest byte-identity againstsha3::Keccak256: every input length through three rate blocks (all padding boundaries incl. 135/136/137), 300 randomized misaligned-chunking cases throughupdate, andkeccak256_pairagainst both the reference and the streaming sponge. Atest-syscallsMakefile target runs them undermake test, and CI runs them as a dedicated step in the cli-test job.cfg_attr(target_arch = "riscv64", global_allocator). Guest cycle counts re-measured bit-identical after the change.test_recursion_execute_empty(ignored/slow suite) passes with the committed attestation matching the hostprogram_idrecompute.make lintgreen.Notes
maindirectly (originally developed against bench(recursion): measure the verifier at real query counts, over real blocks #825's benchmark branch for the realistic measurements; rebased with no content changes to the four files)./bench-verifyis now a clean PR-vs-main comparison; note it runs the min regime (1 FRI query), where this PR shows only −3.9% — the plumbing removed here scales with queries × tables, hence −29%/−40% at the realistic regimes.make test, the whole-lane fast path is endianness-guarded, pad/squeeze logic is shared, and two proposed alternatives were rejected by measurement (a misaligned lane-assembly absorb path: +4.7%; a genericfinalize_intoreplacement for the TypeId fast path: +0.5% — the by-value sponge through the trait layer can't be fully elided without LTO).Digesttrait boundary on the non-specialized paths). Follow-ups tracked in the optimization log.