perf(verifier): amortize per-table/per-query verifier work (Tier 0 + Tier 1) - #830
perf(verifier): amortize per-table/per-query verifier work (Tier 0 + Tier 1)#830diegokingston wants to merge 1 commit into
Conversation
|
/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.582s | 3.581s | -0.01% ⚪ |
| Proof size | 204.30 MiB | 204.30 MiB | +0.00% ⚪ |
pairs: 20 mean A (PR): 3.581s mean B (main): 3.582s
[parametric] paired-t mean -0.01% sd 0.65% se 0.15%
95% CI: [-0.32%, +0.30%] (t df=19 = 2.093)
[robust] median +0.10% Wilcoxon W+=132 W-=78 p(exact)=0.3300 (z=+0.99)
run-to-run jitter: A CV 0.30% B CV 0.48% (lower = steadier)
within-session drift: +0.12% over the run, 1st->2nd half +0.14%
⚪ INCONCLUSIVE — effect not separable from 0 at n=20 (point estimate ~+0.10%). Add pairs to resolve.
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 a864832 guest=recursion-min.elf
REF_A (PR) 1ea7876 1ea7876 guest=recursion-min.elf
| Metric | REF_B (baseline) | REF_A (PR) | Δ (A-B) |
|---|---|---|---|
| Guest cycles | 43.6M | 38.3M | -5.3M (-12.21%) |
| 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=a8648320867f7f4242fe286a5b266ffed1fb5519 ref_b_elf=recursion-min.elf ref_b_cycles=43608950 ref_b_keccak=3025 ref_b_execute_wall_s=1
ref_a_sha=1ea787697917bf6e4b85620c8913907b5b5a3683 ref_a_elf=recursion-min.elf ref_a_cycles=38284840 ref_a_keccak=3025 ref_a_execute_wall_s=1
delta_cycles=-5324110 delta_keccak=0
|
/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. |
|
/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. |
9b78618 to
4d4d886
Compare
Two recursion-cycle savings in the verifier (= serial recursion guest): 1. Drop VerifierDomain::root_order. The DEEP driver recomputed the trace primitive root via get_primitive_root_of_unity (repeated squarings) every proof; reuse the precomputed domain.trace_primitive_root instead. 2. Compute the Q primary FRI evaluation points once in step_3 and share them between the DEEP reconstruction and the FRI fold. The DEEP driver previously recomputed all 2Q (primary+symmetric) points and the fold recomputed Q more. Now: Q pow() total; each symmetric point is -primary (bit-reversed index primary+N/2, lde_root^(N/2)=-1), so negate instead of a second pow(); the fold moves the same Q points into its inverses. Byte-identical. 204 stark tests pass, clippy clean.
2084def to
1ea7876
Compare
|
/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. |
There was a problem hiding this comment.
⚠️ AI-generated review. Produced by an automated agent and posted from this account — not written by hand. Findings below are leads to verify, not confirmed conclusions.
The reviewable diff contains two Tier-0 wins: the DEEP driver reuses domain.trace_primitive_root instead of re-deriving it per table (dropping the now-dead VerifierDomain::root_order and its .expect), and the Q primary FRI points are computed once in step 3 and shared between the DEEP denominators (symmetric = −primary) and the fold's batch-inverted inverses. Both substitutions hold: trace_primitive_root is built from exactly the trace_length.trailing_zeros() order that root_order held, and reverse_index(2i+1, N) == reverse_index(2i, N) + N/2 with lde_root^(N/2) = −1 — sample_query_indexes bounds iota < N/2, so the low-bit argument holds — making sym = −primary byte-identical to the old pow. Denominator ordering into the whole-proof batch inverse is unchanged, step_3_verify_fri has no transcript absorbs/squeezes to reorder, and no new proof-reachable panic appears. The code is fine; the problems are in what the PR claims.
Description vs. diff — of the seven bullets, only "Redundant primitive root" and "DEEP eval points" are in the branch. Two are not implemented anywhere in it: crypto/stark/src/verifier.rs:674 still builds vec![evaluation.clone(), evaluation_sym.clone()] and calls verify_merkle_path (the hash_data_from_slices + verify_merkle_path_from_leaf_hash pair serves only trace/composition openings and was already on main), and crypto/stark/src/verifier.rs:729 still .collect()s evaluation_point_vec via iter::successors. Of the rest: the zerofier item is the base commit (#829, 4d4d8868), and the OOD-grid and Tier-2 items are already on main — layout.reconstruct_full is built once in verify_rounds_2_to_4 and threaded into both step 2 and step 3 (main verifier.rs:1600), reconstruct_ood_grid does not exist anywhere in the tree, and trace_term += &denoms_trace[row_idx] * &(&base_row_sum - ood_row_sum) landed with #826 (main verifier.rs:1035). With two stacks rewriting these same functions, this is how wins get counted twice. Trim the description to trace-root reuse + shared Q points, attribute the zerofier to #829, and either implement or delete the two Tier-0 bullets — the FRI-layer leaf hash is a real remaining cost (two extension clones + a Vec per layer per query per table) and can mirror verify_opening_pair with the iota-parity ordering.
Benchmark — the posted −12.21% guest cycles is 1ea78769 vs main, i.e. the whole two-commit stack rather than this PR's increment, and it is measured at blowup 2 with 1 query. At Q=1 the per-table fixed savings (one get_primitive_root_of_unity, one extension inversion) are maximally visible while the unchanged dominant costs — per-query Merkle/Keccak authentication and per-query DEEP arithmetic — are at their smallest; at the ~110–219 queries a real base/inner proof uses, the denominator grows by two orders of magnitude while much of the numerator does not. The native A/B on the same run came back at −0.01%, consistent with that. Before merge, post the incremental delta against pr/829 and either re-run the cycle comparison at a realistic query count or state the Q=1 caveat in the description, so −12% does not get quoted as the production win.
crypto/stark/src/verifier.rs:918 — denominators is now sized from query_points.len(), but the consumption loop at :935 walks challenges.iotas and slices denominators[2*i*stride..]. A short query_points is a slice-index panic. It is unreachable today (the single call site maps verifier-sampled iotas), but every other length assumption in this function fails closed with an early return None, including the one directly above. Add if query_points.len() != num_queries { return None; }, or derive num_queries from query_points.len() and use it for both loops.
crypto/stark/src/verifier.rs:526 — after this change query_challenge_to_evaluation_point has one caller (:440, sym = false) and the iota*2+1 branch is unreachable, so the now load-bearing negation identity has no reference implementation to be cross-checked against, and the doc describes a mechanism the verifier no longer uses. Either drop the sym parameter and reword the doc to say the symmetric point is the negation, or keep it and add a unit test — crypto/stark/src/tests/fri_tests.rs already imports reverse_index and builds coset domains — asserting f(iota, true, &domain) == -f(iota, false, &domain) for a few domain sizes and iotas. As it stands, a change to the leaf-pairing or bit-reversal convention would surface only as an unexplained honest-proof rejection in the roundtrip suite.
Nits:
- Comments narrate the diff rather than the code: :435 "Previously the driver recomputed all 2Q…", :499 "(moved in)" (parses only while reading the diff), :883 "(which needed
root_orderand aget_primitive_root_of_unity…)" referencing a field this PR deletes — use present tense and leave history in the commit message. - :522 reads "Returns the field element element of the domain…" and is then restated by the next line.
Stacked on #829. Removes redundancies from the per-table verify path. Each runs
Ttimes for the ~100–140 un-batched sub-proofs of an ethrex proof, andverifier.rsdoubles as the recursion guest, so cycles count as well as wall-clock.Tier 0 (micro)
hash_data_from_slices+verify_merkle_path_from_leaf_hash, mirroringverify_opening_pair) instead of allocating avec![a, b]per layer per query.𝜐^(-2ⁱ)lazily and zip them into the fold, dropping the pre-collectedevaluation_point_vec.gviaget_primitive_root_of_unity(root_order)(~log₂N squarings/table) although the verifier domain already stores it astrace_primitive_root. Use that; drop the now-deadVerifierDomain::root_orderfield.Qprimary FRI points once in step 3 and reuse them for both the DEEP denominators and the FRI eval-point inverses. The symmetric point is the primary's negation (sym_index = primary + N/2,lde_root^(N/2) = −1), so no secondpowper point.Tier 1 (structural)
1/(zᴺ − 1)into the boundary-constraint batch inverse as one trailing element (~3 muls) instead of a separate extension inversion (~70 base muls) per table.verify_rounds_2_to_4(with the soundness-I3 shape validation, now inreconstruct_ood_grid) and thread it into both step 2 and step 3 instead of each rebuilding it.Tier 2 (per-query DEEP arithmetic)
Σ_col Σ_row (t_col(x) − ood[row][col])·inv_den[row]·γmultiplied the shared row denominator1/(x − z·gʳᵒʷ)in per(col,row)element. Factor it out of the column sum (Σ_row inv_den[row]·Σ_col (…)·γ): one extension multiply per row instead of per cell, ~halving the trace-term extension muls over 2·Q queries × T tables. Byte-identical (field add/mul are associative + commutative); the g·z pruning skip is preserved and thetrace_term_coeffsshape guard is strengthened to exact per-dimension equality so the row-major indexing is panic-safe.Soundness
No checks removed. The I3 OOD-shape validation still runs (moved into
reconstruct_ood_grid, returnsNone→ reject). The folded1/(zᴺ − 1)still fails closed: a zero denominator (z on a boundary step or on the trace domain) fails the batch inverse and rejects. Thesym = −primaryidentity is the same one the openings already rely on. The DEEP factoring reorders a finite-field sum/product, so it is exact.Testing
starktests pass — thetest_prove_verify_*suite generates real proofs and runs them through the fullmulti_verify_views → verify_rounds_2_to_4 → step_2/3/4path, exercising every changed line; any DEEP drift would desync the codeword from the FRI folding.cargo clippy -p starkclean; no new warnings.Considered but deferred
multi_verify_views— unsafe when tables have different trace lengths (table splitting), and structural; belongs with the batched-FRI lane.boundary_constraintsper table (counted in replay, rebuilt in step 2) — a small per-table alloc, not arithmetic; avoiding it needs a new AIR-trait count method orChallengesplumbing not justified by the saving.