Repository navigation
perf: cheaper decoding and challenge setup for RISC-V guest verifiers - #188
Conversation
|
Warning This PR has more than 500 changed lines and does not include a spec. Large features and architectural changes benefit from a short spec before implementation review. If this PR is a bug fix, small cleanup, or otherwise does not warrant a spec, feel free to ignore this message. |
Documentation blast radius (advisory)These regions may need doc/spec/book updates based on changed paths. Changed files in this PR: 9
|
PCS Profile Benchmark
13 of 13 profiles passed. Times are medians of Each sample verifies the same proof first with the configured multi-threaded pool and then with one thread. Both timings reuse the same verifier setup. Merge-base comparisons are available for Benchmark shards
Public opening statements
One-hot profiles generate deterministic witnesses with one Direct evaluates the public setup contribution during Stage 2. Recursive carries the same check through a Stage 3 setup-product sumcheck. Both modes execute the complete fold schedule and terminal verification. The chunked profiles Generated profiles may select different A, B, and D ring dimensions at different fold levels. The short profile names omit those dimensions. Each sample generates deterministic witnesses and opening points, prepares setup, commits, proves, serializes the proof, checks its size, prepares verifier setup, and verifies the claimed openings. It does not test malformed proofs. Phase time
Proof size and protocol shape
Grinding retries are rejected attempts at each fold, listed in measured-run order. Zero means the first sampled nonce was accepted. Each multi-group profile has two precommitted nv16 singleton polynomials and two polynomials in the displayed final group. Native proof accounting
Memory and setup size
Deltas are shown only for profiles with a matching merge-base case. Negative is smaller or faster. The uploaded |
f3ee260 to
d202349
Compare
CI test timing
Run summary
Wall time spans 2 parallel nextest slice shards. Slowest tests
Regressions vs main
New slow testsNo new tests ≥30s vs main baseline. |
d202349 to
8f5f6ad
Compare
Verifier-side changes that cut instruction counts on a RISC-V zkVM guest, where sub-word memory operations and software floating point are expensive. Proofs, transcripts, and outputs are unchanged. - Golomb-Rice decoding reads codes that fit one aligned 64-bit word from that word, and counts unary runs a byte at a time. - Proof atoms encode into an inline, word-aligned 64-byte buffer instead of a heap Vec. - SparseChallengeConfig::log2_support_bits memoizes per (D, counts), and the op-norm trig tables compute each certified first-octant enclosure once. - Paired tensor evaluation looks up per-bit factor pairs, and dense-left materialization collects the residual offsets once instead of recursing per output coordinate.
8f5f6ad to
22e2a32
Compare
|
/ai-review |
|
AI review attempt 1/3 reserved. Failed or incomplete attempts also count toward this PR's limit. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Usefulness: benefit supported. The guest-verifier motivation is supported by the code: these changes remove repeated logarithm and Taylor evaluations, per-field encoding allocations, and duplicate tensor recurrence work on existing consumers. They add no dependencies or public format changes. The main costs are cache synchronization and residual-table memory; the latter needs the correction below. Exact guest instruction savings were not independently verified.
Approval not recommended: new P0-P3 findings in this review.
| for view in &mut views { | ||
| if !view.family.scalar.is_zero() { | ||
| view.residual | ||
| .try_reserve_exact(view.residual_len) |
There was a problem hiding this comment.
[P2] This reserves the full Cartesian product even when zero weights eliminate every residual. For example, a scalar-one family with axes unit(2, 1, 1), dense(0, 0, vec![F::zero(); 2]), and unit(1 << 26, 0, 1) passes the 1 << 28 work bound but reserves 1 << 27 entries before collecting none—2 GiB for an eight-byte field on a 64-bit target. The base implementation skips the zero-weight branches and returns a two-element zero output with negligible scratch space. This turns a cheap valid input into an allocation failure on bounded-memory guests. Can we count live residuals or grow the table fallibly as entries are emitted, and add a zero-heavy regression test?
There was a problem hiding this comment.
Fixed in b7f51c0. Each axis reports its nonzero-weight count in closed form: len for unit axes, the count of nonzero entries for dense axes, and the product of per-bit nonzero factor counts for bit-product axes (fields have no zero divisors). The residual table reserves exactly the product of those counts, so your example reserves nothing. The regression test eq_pair_tensor_materialization_of_zero_weight_residuals_is_zero uses that family. On hosts that overcommit memory the old reservation was never touched, so the test checks the result and the code path; the memory saving only shows on hosts that commit memory up front.
github-actions
Bot
added
the
ai-reviewed
The residual table reserved the full Cartesian product of the non-destination axes even when zero weights removed every entry, so a cheap valid family could reserve 2^27 entries and collect none. Each axis now reports its nonzero-weight count in closed form, and the table reserves the product of those counts.
RadNi
left a comment
RadNi
left a comment
There was a problem hiding this comment.
Inline feedback: one malformed-metadata correctness issue, plus suggestions on residual memory budgeting, exact-output regression coverage, and isolated guest performance measurements.
| EqPairTensorWeights::BitProduct(bit_factors) => { | ||
| bit_factors.iter().fold(1, |count, factors| { | ||
| count * factors.iter().filter(|factor| !factor.is_zero()).count() |
There was a problem hiding this comment.
[P2] Validate axis geometry before counting nonzero entries
The preceding materialization work bound uses axis.len, while this count trusts the backing factors. Since axis fields are publicly mutable, a residual axis with len = 2 and 64 bit-factor pairs of [1, 1], alongside a destination axis unit(2, 1, 3), passes a nominal work check of four coordinates but computes 2^64 here on a 64-bit target. This panics with overflow checks enabled and wraps otherwise. I reproduced the panic using the branch's tensor/materialization source in a small harness with substitute field/equality operations; the base path completed. This demonstrates the arithmetic regression, not reachability from malicious proof bytes.
Please validate dense-weight lengths and bit-product factor counts against axis.len before allocation sizing, reject inconsistent geometry with AkitaError, and add a malformed residual-axis regression test. The outer product of live counts also needs to rely on that validated invariant.
| .fold(1, |count, (_, axis)| count * axis.nonzero_len()); | ||
| view.residual.try_reserve_exact(live).map_err(|_| { | ||
| AkitaError::InvalidInput("paired tensor residual table allocation failed".into()) | ||
| })?; |
There was a problem hiding this comment.
Suggestion: bound residual-table bytes separately from arithmetic work
Counting only live entries addresses the earlier zero-weight allocation issue, but a valid all-nonzero family with axes unit(2, 1, 3) and unit(1 << 27, 0, 1) still passes the 1 << 28 work limit and reserves 1 << 27 residual entries. On a 64-bit target with an eight-byte field, the (usize, F) table alone is about 2 GiB, despite the output having only two elements. The previous recursive contraction used much less auxiliary memory. Fallible reservation does not by itself establish a practical memory budget for a bounded-memory guest.
Could we cap precomputation bytes separately and retain streaming contraction when that budget is exceeded? A regression test can exercise the budget/fallback decision using a small test budget rather than allocating gigabytes.
| } | ||
|
|
||
| #[test] | ||
| fn atom_bytes_encode_inline_and_spilled_lengths() { |
There was a problem hiding this comment.
Suggestion: pin actual atom encodings and a transcript challenge
This test verifies the inline/heap container with synthetic bytes, but it does not exercise the changed field-atom and extension-atom encoding paths. Existing prover/verifier round trips can still succeed if both sides change identically.
Please add byte-for-byte comparisons of actual FieldAtom encodings against canonical field bytes and ExtensionAtom encodings against concatenated canonical coefficient encodings. A pinned transcript challenge for a fixed message sequence would also independently check the PR's claim that transcript bytes and challenges remain unchanged. Keep the current length-boundary test as coverage for the storage choice.
| let cos_phi = taylor_cos((phi_lo, phi_hi)); | ||
| let sin_phi = taylor_sin((phi_lo, phi_hi)); | ||
| // octant_reduce bounds r by d/4 for every degree. | ||
| let (cos_phi, sin_phi) = first_octant[r]; |
There was a problem hiding this comment.
Suggestion: compare certified tables exactly with the previous construction
The existing approximate-reference checks verify enclosure accuracy, but do not establish that this optimization preserves every fixed-point entry and the global eps_root exactly. Those values affect the acceptance boundary used by challenge sampling.
Could we retain the previous per-angle construction as a test-only reference and assert exact equality of base_cos, base_sin, and eps_root across the supported degrees and representative scales? That would directly substantiate the unchanged-output/rejection claim for this refactor.
| // families; the bound keeps arbitrary callers from growing the table. | ||
| const MAX_CACHED_FAMILIES: usize = 64; | ||
| type SupportCacheEntry = ((usize, usize, usize), f64); | ||
| static CACHE: std::sync::Mutex<Vec<SupportCacheEntry>> = std::sync::Mutex::new(Vec::new()); |
There was a problem hiding this comment.
Suggestion: include isolated guest performance measurements
The PR description reports validation and combined-series guest integration, but does not give isolated before/after guest instruction or trace-row counts for these optimizations. The new memo also makes cold-cache and warm-cache costs different.
Please include reproducible before/after measurements for this PR, ideally broken down by optimization, with cold-cache versus warm-cache results and peak memory for residual materialization. Including the fixture, guest revision, and build configuration would make the claimed benefit and the memory tradeoff directly reviewable.
|
/ai-review |
|
AI review attempt 2/3 reserved. Failed or incomplete attempts also count toward this PR's limit. |
There was a problem hiding this comment.
Usefulness: benefit supported. The guest-verifier motivation is supported by existing consumers: these changes remove per-atom encoding allocations, repeated Taylor evaluations, repeated support calculations, and redundant tensor work without adding dependencies or intentionally changing public formats. The main costs are cache synchronization and residual-table memory. The optimization premise remains useful, but the confirmed residual-sizing issues below need resolution. Exact guest instruction savings were not independently verified.
Approval not recommended: unresolved feedback from existing discussion.
Unresolved feedback from existing discussion:
- RadNi’s comment 4331364217 remains unresolved. EqPairTensorAxis fields are public, but materialize_eq_tensor_left does not revalidate their geometry. The work check in tensor_pair/materialize.rs:251–280 uses axis.len, while nonzero_len in tensor_pair/mod.rs:107–119 trusts the backing factors. A residual axis with len=2 and 64 pairs of [1,1] passes a four-coordinate work check alongside unit(2,1,3), then computes 2^64, panicking with overflow checks or wrapping otherwise. Validate weight geometry before counting and sizing allocations, including the outer live-count product.
- The memory regression raised in RadNi’s comment 4331364224 remains unresolved. tensor_pair/materialize.rs:291–302 reserves every live residual with no separate scratch-memory budget or streaming alternative. Valid axes unit(2,1,3) and unit(1<<27,0,1) pass the 1<<28 work limit but require approximately 2 GiB of residual storage for an eight-byte field on a 64-bit target, despite producing only two output elements. The base implementation streamed these residuals. Fallible reservation reports allocation failure rather than preserving support on bounded-memory guests; bound precomputation memory and retain a bounded-memory contraction path.
Revalidate every axis's weight table against its public len at both tensor entry points, so live-residual counting cannot overflow on mutated axes. Cap dense-left residual tables at 1 MiB and fall back to the streaming contraction from main above that budget. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed RadNi's review (4331364217, 4331364224) and the transcript suggestion:
|
|
/ai-review |
|
AI review attempt 3/3 reserved. Failed or incomplete attempts also count toward this PR's limit. |
There was a problem hiding this comment.
Usefulness: benefit supported. Existing decoder, transcript, challenge-sampling and tensor consumers benefit from fewer allocations and repeated computations without dependency or wire-format changes. The new residual-table budget preserves a bounded-memory streaming path. Costs include global cache synchronization and maintaining two residual evaluation modes; their duplicated traversal is avoidable. Exact guest instruction savings were not independently verified.
Approval not recommended: new P0-P3 findings in this review.
| } | ||
| } | ||
|
|
||
| fn collect_residual<F: Field>( |
There was a problem hiding this comment.
[P2] The tabulated and streaming modes now have separate implementations of the same residual-axis walk here and in contract_residual_tensor_axes: skipping the destination axis, reading coordinate weights, pruning zeros, multiplying weights and checking offsets are all duplicated. Changes to this geometry/error-handling logic consequently need matching edits in both paths. Can we share one residual-term visitor with an initial offset/weight and a leaf callback—appending to the table or accumulating the weighted equality value—and delete the second recursive traversal? That keeps the memory-budget fallback without duplicating its policy.
…aming modes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Verifier-side changes that cut instruction counts on a RISC-V zkVM guest, where sub-word memory operations and software floating point are expensive. Proofs, transcripts, and verifier outputs are unchanged.
Vec. Longer encodings spill to the heap; a test covers both cases.SparseChallengeConfig::log2_support_bitsmemoizes per(D, counts), and the op-norm trig tables compute each certified first-octant enclosure once.The support-bits memo holds at most 64 families and takes its lock once per call. A test pins the word-window path's quotient cap at every bit offset and byte alignment (it fails if the cap check is removed).
Review guide
Read in this order (one commit; every change must leave outputs and rejections unchanged):
crates/akita-params/src/golomb_rice.rs:BitReader::word_windowand the fast path ingolomb_rice_decode_one_from.crates/akita-transcript/src/proof_stream.rs:AtomBytes, which writes the same bytes as the previousVecencoding.crates/akita-algebra/src/offset_eq/tensor_pair/{evaluate,materialize}.rs:basis_pair_factorsandcollect_residual/evaluate_residual, replacingcontract_residual_tensor_axes. Allocation is fallible and happens after the work bound.crates/akita-challenges/src/config.rs(the bounded support-bits memo) andsampler/op_norm.rs(the first-octant table;octant_reducebounds the index for every degree).These are standalone; none depends on the other PRs.
Validation
On this branch, rebased on main
401e79728: fmt, the three CI clippy lanes, the guardrail and dependency scripts, cargo machete, and taplo, and the workspace tests (1,861 passed). On the combined series atmarkos/recursion-integration78d31e124, a16z/jolt#1911's recursion example accepts a fresh proof in the guest, rejects one with a flipped opening-proof bit, and accepts in input mode.Series
These PRs upstream the Akita changes that a16z/jolt#1911 needs to verify Akita proofs inside a Jolt RISC-V guest in under 2^26 trace rows. That PR now pins
markos/recursion-integrationin this repository (mainplus these five) instead of a personal fork, and repins tomainonce they merge.The first three are independent and build against the Jolt revision
mainpins today. The last two use Jolt APIs added in a16z/jolt#1911 (jolt-inlines-nttand jolt-field's row kernels), so they share one commit pinning Jolt at that PR's head, a commit on a16z/jolt that is not yet on itsmain.Merge order: once these land, a16z/jolt#1911 pins Akita
maininstead of the fork, and a follow-up here repins Jolt to its merge commit. Alternatively, the Jolt-side APIs can land first in a smaller a16z/jolt PR, so the last two pin a Joltmaincommit from the start.A per-change row ladder for the guest is in
specs/recursion-guest.mdon a16z/jolt#1911.