Feat/scaling improvements#483
Conversation
Three-item plan to reduce proving slope from ~5.1s/M to ~3-3.5s/M: 1. Uniform 2^20 table cap + twiddle dedup + FRI fold parallelism 2. MMCS batched commitment (Plonky3-style, all tables in shared trees) 3. Shared FRI instance across all tables
Tables with effective width < 42 were sized at 2^21, producing single large chunks. Capping at 2^20 produces 2x more chunks of the standard size, improving parallel throughput and keeping peak memory per chunk uniform across all tables.
Tables sharing the same lde_size now reuse a single Arc<LdeTwiddles> instead of each allocating their own copy. This avoids redundant twiddle computation and allocation when multiple tables happen to have the same trace length (e.g., all 2^19-row tables after the cap change).
Under the `parallel` feature, fold each conjugate pair concurrently using `par_chunks(2).zip(inv_twiddles.par_iter())`, collecting into a new Vec of half length. The sequential path (no `parallel` feature) is unchanged. This avoids the index aliasing issue from an in-place interleaved write.
Replace the per-row `Vec` allocation inside the iterator with a thread-local buffer via Rayon's `map_init` (parallel path) or a single buffer allocated outside the loop (sequential path). This avoids one heap allocation per row during Merkle tree construction.
Codex Code Review
No critical/high security vulnerability was identified in the changed lines. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let trace_length = trace.num_rows(); | ||
| let domain = new_domain(*air, trace_length); | ||
|
|
||
| let lde_size = domain.interpolation_domain_size * domain.blowup_factor; |
There was a problem hiding this comment.
Medium – Fragile deduplication key
lde_size alone is not a sufficient key for LdeTwiddles. Looking at LdeTwiddles::new, the struct depends on three independent inputs from the domain:
domain.interpolation_domain_size→ determinescoset_weightslength andinvtwiddlesdomain.blowup_factor→ determinesfwdtwiddles (only vialde_size)domain.coset_offset→ determinescoset_weightsvalues
Two domains that produce the same lde_size = interpolation_domain_size * blowup_factor but with different ratios (e.g. domain_size=2^19, bf=4 vs domain_size=2^20, bf=2) or different coset_offset values would yield structurally different LdeTwiddles, but the current code would hand them the same Arc. Proofs silently generated from the wrong twiddles would be invalid.
This doesn't bite today because all tables share a single ProofOptions (uniform blowup_factor and coset_offset), so equal lde_size ↔ equal domain_size. But the invariant is implicit and not checked, making it a silent correctness landmine for any future per-table options.
| let lde_size = domain.interpolation_domain_size * domain.blowup_factor; | |
| let twiddle_key = (domain.interpolation_domain_size, domain.blowup_factor); | |
| let twiddles = twiddle_by_size | |
| .entry(twiddle_key) | |
| .or_insert_with(|| Arc::new(LdeTwiddles::new(&domain))); |
And change the HashMap type to HashMap<(usize, usize), Arc<LdeTwiddles<Field>>>. Alternatively add a debug_assert that equal lde_size implies equal interpolation_domain_size.
| // Evaluations are stored interleaved: pairs (evals[2j], evals[2j+1]). | ||
| // Fold each pair in parallel, collecting into a new Vec of half length. | ||
| let folded: Vec<FieldElement<E>> = evals | ||
| .par_chunks(2) |
There was a problem hiding this comment.
Low – Silent truncation vs. panic on length mismatch
The sequential path accesses inv_twiddles[j] for j in 0..half, which panics with a clear index-out-of-bounds if inv_twiddles.len() < half.
The parallel path uses par_chunks(2).zip(inv_twiddles.par_iter()), which silently truncates to the shorter of the two iterators. If inv_twiddles is shorter than expected, fewer elements are folded, folded has the wrong length, and the resulting proof is silently incorrect — no panic, no error.
Since FRI always operates on power-of-2-aligned data the lengths always match in practice. But for a cryptographic primitive, silent wrong-output is strictly worse than a loud panic. Consider adding a debug assertion:
debug_assert_eq!(evals.len(), inv_twiddles.len() * 2,
"fold: evals length must be twice inv_twiddles length");
PR Review: Feat/scaling improvementsSummary: Item 1 of the scaling roadmap — uniform max_rows cap at 2^20, twiddle deduplication via Medium – Twiddle dedup key is incomplete
It doesn't bite today because all tables share one Low – Parallel FRI fold: silent truncation vs. panic
Everything else looks clean — the |
|
/bench 3 |
Benchmark — fib_iterative_8M (median of 3)Table parallelism: 32 (auto = cores / 3)
Commit: 38a8100 · Baseline: built from main · Runner: self-hosted bench |
|
/bench 3 |
1 similar comment
|
/bench 3 |
This reverts commit d566602.
Replace per-table FRI with a single shared FRI cascade when all tables share the same LDE domain size. Tables with different domain sizes fall back to per-table FRI (folding insertion across coset boundaries requires further work). Key changes: - FRI: add `commit_phase_with_insertion` and `FriInsertion` API (for future folding insertion support across matching cosets) - Proof: add `SharedFri` struct and `shared_fri` field on `MultiProof` - Prover: in `multi_prove`, when all tables have the same domain, compute per-table DEEP evals, lambda-batch them, and run one shared FRI instead of N independent FRIs. Per-table Merkle trees (trace, aux, composition) are kept separate. - Verifier: add `verify_shared_fri` which replays the shared FRI transcript, reconstructs batched DEEP values from per-table openings, and verifies the FRI fold chain. Per-table composition polynomial and trace opening verification is done via `verify_rounds_2_to_4_without_fri`.
6ecf144 to
38a8100
Compare
|
/bench 3 |
… sizes Extend all tables' trace and composition polynomial LDE evaluations to the largest LDE domain (max_lde_size) so shared FRI can query all tables at the same coset points without per-table index mapping. Key changes: - Add `extend_pool_columns_to_size` helper: extends LDE columns from their natural size to a target size via iFFT(coset) + zero-pad + FFT(coset) - Phase A/C commits: extend main/aux LDE columns to max_lde_size before building Merkle trees, so all trees share the same height - Round 2 composition poly: extend H0/H1 part evaluations to max_lde_size before committing, matching the unified domain - Round 4 openings: reconstruct extended LDE for opening at shared query indices (no iota>>shift mapping needed) - Remove `all_same_domain` guard and per-table FRI fallback -- the shared FRI path now handles all multi-table proves uniformly - Verifier: use unified max-domain evaluation points for DEEP reconstruction and Merkle proof verification (no per-table index mapping) - Fix stride calculations in Round 3 OOD eval and DEEP composition to handle extended composition poly evaluations correctly
|
/bench 3 |
2 participants
No description provided.