Skip to content

perf(crypto): stream field-element bytes into hashers and transcript#773

Open
Oppen wants to merge 3 commits into
perf/rkyv-serializationfrom
perf/fixed-fe-bytes
Open

perf(crypto): stream field-element bytes into hashers and transcript#773
Oppen wants to merge 3 commits into
perf/rkyv-serializationfrom
perf/fixed-fe-bytes

Conversation

@Oppen

@Oppen Oppen commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Eliminates the per-field-element heap allocation in Merkle leaf hashing and Fiat-Shamir transcript appends: AsBytes gains a stream_bytes sink method, overridden zero-alloc for the Goldilocks base field and its degree-3 extension, replacing as_bytes()/to_bytes_be() returning a fresh Vec<u8> per element per hash.
  • Adds FieldElementVectorBackend::hash_data_parts + verify_merkle_path_from_hash so row-pair openings (verify_opening_pair, verify_composition_poly_opening, verify_fri_layer_openings) hash two slices directly instead of concatenating them into a throwaway Vec first; verify_composition_poly_opening and hash_data now delegate to their row-pair/parts siblings instead of duplicating the hashing loop.
  • Follow-up: the degree-3 extension's stream_bytes was calling the base-field override three times (one per limb), landing as three separate Digest::update calls on the guest instead of one. Disassembly confirmed the dyn FnMut sink itself was fully devirtualized (no indirect-call cost), so the actual waste was call count. Now reuses the existing zero-alloc write_bytes_be override into a stack buffer and sinks once.
  • Recursion guest: single-query 89.7M → 71.1M cycles, multi-query 2.21B → 1.78B cycles (handoff's baselines were 89.7M / 2.21B; target was ~1.85–1.95B multi-query, beaten by a further margin after the follow-up).

Test plan

  • cargo test --workspace --exclude math-cuda (492 prover + 137 stark tests, all green) after each commit
  • make test-ethrex
  • cargo test -p lambda-vm-prover --lib test_recursion_execute_1query -- --ignored --nocapture (in-VM verify accepts, guest commits vk_digest ‖ output, byte-identical across every change)
  • make test-profile-recursion-single / -multi cycle counts confirmed against baseline after each commit
  • Guest ELF disassembly (llvm-objdump) confirming stream_bytes devirtualizes and the extension-field batching removes redundant BlockBuffer::digest_blocks/memcpy calls
  • Multiple rounds of independent adversarial review (correctness, performance incl. a dedicated missed-optimization hunt, implementation simplicity) against the diff

@Oppen Oppen force-pushed the perf/rkyv-serialization branch from ea14eeb to 2e68711 Compare July 8, 2026 20:09
Oppen added 2 commits July 8, 2026 17:44
Eliminates the per-element heap allocation in Merkle leaf hashing and
Fiat-Shamir transcript appends. AsBytes gains a stream_bytes sink method,
overridden zero-alloc for Goldilocks base and degree-3 extension; the
Merkle backends and DefaultTranscript use it instead of as_bytes()/
to_bytes_be(). Adds hash_data_parts + verify_merkle_path_from_hash so
row-pair openings hash two slices directly instead of concatenating them
into a throwaway Vec first; verify_composition_poly_opening and
FieldElementVectorBackend::hash_data now delegate to their row-pair/parts
siblings instead of duplicating the same hashing loop.

Recursion guest: single-query 89.7M -> 73.7M cycles, multi-query
2.21B -> 1.82B cycles.
stream_bytes was calling the base-field override three times, one per
limb, each landing as its own Digest::update on the guest — three
BlockBuffer::digest_blocks + memcpy calls instead of one. Disassembly
of the guest ELF showed dyn dispatch itself was fully devirtualized by
the #[inline(always)] chain, so the actual cost was the call count, not
indirection. Reuse the existing write_bytes_be override (already
zero-alloc, byte-identical layout) into a stack buffer and sink once.

Recursion guest: single-query 73.9M -> 71.1M cycles, multi-query
1.82B -> 1.78B cycles.
@Oppen Oppen force-pushed the perf/fixed-fe-bytes branch from aa8d48c to ddb97be Compare July 8, 2026 20:44
@Oppen

Oppen commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

/ai-review

@Oppen

Oppen commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

/bench

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codex Code Review

No findings.

I reviewed only the PR diff and did not identify safety/security, correctness, significant performance, or dead-code issues in the changed code. Tests were not run, per the static-review instructions.

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review: perf(crypto): stream field-element bytes into hashers and transcript

Reviewed the diff focusing on the byte-identity invariant, since the whole refactor rests on stream_bytes producing exactly the same bytes the old as_bytes()/to_bytes_be() path did. It holds:

  • Goldilocks: as_bytes -> to_bytes_be -> canonical_u64().to_be_bytes() (8 bytes). The stream_bytes override sinks canonical_u64().to_be_bytes(). Identical.
  • Degree-3 extension: as_bytes/to_bytes_be concatenates the three components' big-endian canonical bytes (24 bytes). stream_bytes writes the same via write_bytes_be into a [0u8; 24] stack buffer and sinks once. Byte-identical, one update call instead of three.
  • Default trait method falls back to sink(&self.as_bytes()), so non-overridden types are unchanged.

Leaf-hash equivalence in the verifier also checks out:

  • hash_data_parts(&[a, b]) hashes elements of a then b in order — identical to the old value = a.to_vec(); value.extend(b); hash_data(&value).
  • verify_composition_poly_opening delegating to verify_opening_pair::<FieldExtension> is sound: same row-pair leaf layout, same iota, same concatenation order. Good dedup.
  • verify_fri_layer_openings preserves the iota % 2 ordering and iota >> 1 index; only swaps the throwaway Vec for two from_ref slices.

No memory-safety, panic, or unbounded-allocation concerns — the changes remove allocations rather than add them, and the stack buffers are fixed-size and fully written before use.

Minor (Low / cosmetic):

  • crypto/stark/src/verifier.rs: pub use crate::proof::view::{PiDeserializer}; wraps a single import in braces — unnecessary churn (clippy unused_braces style). Revert to pub use crate::proof::view::PiDeserializer;.
  • Same file: &composition_poly_merkle_root (already &Commitment) and &opening.evaluations() rely on deref coercion; dropping the extra & reads cleaner, but harmless.

Nothing blocking. Clean, byte-preserving perf refactor.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Benchmark — ethrex 20 transfers (median of 3)

Table parallelism: auto (cores / 3)

Metric main PR Δ
Peak heap 73267 MB 73020 MB -247 MB (-0.3%) ⚪
Prove time 38.430s 40.461s +2.031s (+5.3%) 🔴

⚠️ Regression detected — heap or time increased by more than 5%.

✅ Low variance (time: 2.4%, heap: 1.3%)

Commit: ddb97be · Baseline: cached · Runner: self-hosted bench

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

AI Review

PR #773 · 8 changed files

Findings

Status Sev Location Finding Found by
candidate low crypto/stark/src/verifier.rs:488 Auxiliary &amp; reborrow in verify_composition_poly_opening call moonmath
zro/minimax-m3

Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro).

AI-001: Auxiliary `&` reborrow in `verify_composition_poly_opening` call
  • Status: candidate
  • Severity: low
  • Location: crypto/stark/src/verifier.rs:488
  • Found by: moonmath:zro/minimax-m3
  • Verified by: -
  • Rejected by: -

Claim

The new body of verify_composition_poly_opening passes &amp;composition_poly_merkle_root (a &amp;&amp;Commitment reborrow) to verify_opening_pair, which expects &amp;Commitment. The auto-reborrow makes this compile but introduces an unnecessary indirection that mirrors the prior inline call inconsistently — the old code passed composition_poly_merkle_root directly to verify_merkle_path. Dropping the leading &amp; is cleaner and matches the prior pattern.

Evidence

In crypto/stark/src/verifier.rs lines 477-491, the refactored body is Self::verify_opening_pair::&lt;FieldExtension&gt;(deep_poly_openings.composition_poly(), &amp;composition_poly_merkle_root, *iota). The parameter composition_poly_merkle_root: &amp;Commitment is already a reference; writing composition_poly_merkle_root would coerce to &amp;Commitment via auto-reborrow without the explicit &amp;&amp;. The pre-refactor code (also shown in the diff) passed composition_poly_merkle_root directly: verify_merkle_path::&lt;BatchedMerkleTreeBackend&lt;FieldExtension&gt;&gt;(composition_poly.merkle_path(), composition_poly_merkle_root, *iota, &amp;value).

Suggested fix

Replace &amp;composition_poly_merkle_root with composition_poly_merkle_root so the argument type is &amp;Commitment rather than the auto-reborrowed &amp;&amp;Commitment.

Reviewer Lanes

Lane Model Prompt Status Findings
glm openrouter/z-ai/glm-5.2 general error: opencode failed (provider/auth/runtime error) and no findings were submitted 0
kimi openrouter/moonshotai/kimi-k2.7-code general error: opencode failed (provider/auth/runtime error) and no findings were submitted 0
minimax minimax/MiniMax-M3 general error: opencode failed (provider/auth/runtime error) and no findings were submitted 0
moonmath zro/minimax-m3 general success 1
nemotron openrouter/nvidia/nemotron-3-ultra-550b-a55b general error: opencode failed (provider/auth/runtime error) and no findings were submitted 0

Verification Lanes

Lane Model Status Confirmed Rejected Uncertain
deepseek-verifier openrouter/deepseek/deepseek-v4-pro error: opencode failed (provider/auth/runtime error) and no verifications were submitted 0 0 0

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant