FRI arity-4 folding with BatchProof openings#531
Conversation
|
/bench |
Benchmark — fib_iterative_8M (median of 7)Table parallelism: auto (cores / 3)
Commit: 4d19460 · Baseline: cached · Runner: self-hosted bench |
Codex Code ReviewFindings
No other concrete security/logic/performance issues stood out in the PR diff. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| .map(|chunk| [chunk[0].clone(), chunk[1].clone()]) | ||
| // Commit the doubly-folded evaluations as quad (4-element) Merkle leaves. | ||
| let leaves: Vec<[FieldElement<E>; 4]> = evals | ||
| .chunks_exact(4) |
There was a problem hiding this comment.
High – silent data loss / panic for small domains
chunks_exact(4) silently drops trailing elements when evals.len() is not a multiple of 4, and produces zero chunks (leading to a MerkleTree::build panic on an empty slice) when evals.len() < 4.
For number_layers = 3 (domain_size = 8), the double-fold round reduces evals to 2 elements before this line, so zero leaves are produced and the expect below panics. The query_domain_size == 0 guard in sample_query_indexes only protects against domain_size ≤ 3, not this case.
Consider asserting the invariant explicitly:
debug_assert!(
evals.len() >= 4 && evals.len() % 4 == 0,
"evals length {} is not a multiple of 4 after double fold",
evals.len()
);And add a guard at the top of the function (or at the call site) to skip FRI commit when the folded domain is too small for quad leaves.
| // The "b" pair is {index^2, index^3}; its twiddle position is j_b = (index>>1)^1. | ||
| // twiddle_d[j] = lde_coset_element(reverse_index(2^{d+1}·j, N))^{-2^d} | ||
| // Sign: index^2 is lo (even) when index is even, hi (odd) when index is odd. | ||
| let j_b = (index >> 1) ^ 1; | ||
| let lde_idx_b = | ||
| reverse_index((1usize << (folds_done + 1)) * j_b, domain.lde_length as u64); | ||
| let base_b = domain.lde_coset_element(lde_idx_b); | ||
| let twiddle_b_unsigned = base_b.pow(1u64 << folds_done).inv().unwrap(); | ||
| let layer_eval_inv_b = if index & 1 == 0 { | ||
| twiddle_b_unsigned | ||
| } else { | ||
| -twiddle_b_unsigned | ||
| }; | ||
|
|
||
| // Update `v` with next value pᵢ₊₁(𝜐^(2ⁱ⁺¹)). | ||
| v = (&v + evaluation_sym) + evaluation_point_inv * &zetas[i + 1] * (&v - evaluation_sym); | ||
| let inner_val_b = (sib_b + sib_c) + layer_eval_inv_b * z_a * (sib_b - sib_c); |
There was a problem hiding this comment.
High – hard-to-audit twiddle formula with a suspicious sign condition
This hand-rolled twiddle reconstruction for the "b" pair is the most fragile part of the PR. A few concerns:
-
Sign condition:
if index & 1 == 0applies negation based on the last bit ofindexin the committed layer. But the twiddle sign in FRI depends on which "half" of the coset a domain element falls in — that's determined by a bit in the original domain position, not the last bit of the current layer index. After multiple bit-reversal / halving steps the relationship betweenindex & 1and the correct sign is non-obvious. If this is wrong, the verifier silently accepts or rejects valid proofs with no assertion failure. -
Overflow:
(1usize << (folds_done + 1)) * j_boverflows forfolds_done >= usize::BITS - 1. Unreachable in practice but there's no assertion. -
Not tested for the odd-extra path:
test_prove_fib_64_rows(7 layers, 3 double rounds) hitsfold_is_doublefor roundsi=0..1but never for a case where this path runs with a non-zerofolds_donevalue beyond 4.
Consider extracting the twiddle derivation into a named helper with a doc-comment that spells out the math, and adding a unit test that directly validates it against fold_evaluations_in_place.
| pub proof: Proof<Commitment>, | ||
| pub proof_sym: Proof<Commitment>, | ||
| pub proof_2: Proof<Commitment>, | ||
| pub proof_3: Proof<Commitment>, | ||
| pub evaluations: Vec<FieldElement<F>>, | ||
| pub evaluations_sym: Vec<FieldElement<F>>, | ||
| pub evaluations_2: Vec<FieldElement<F>>, | ||
| pub evaluations_3: Vec<FieldElement<F>>, |
There was a problem hiding this comment.
Low – misleading naming
proof_sym / evaluations_sym now hold data for position index+1 (= index^1 in the arity-4 orbit), not a "symmetric" point in the FRI sense. The _sym suffix has a specific meaning in STARK/FRI literature (the evaluation at -x) which no longer applies here.
Suggest renaming to proof_1 / evaluations_1 to match the proof_2/proof_3 pattern already used below, or at minimum update the struct doc-comment to make clear what each field represents.
| // This is sound for arity-4 FRI: the prover commits *one* combined layer | ||
| // that covers both binary folds, so it fixes its evaluations before either | ||
| // challenge is revealed. It cannot choose zeta2 adaptively after seeing | ||
| // zeta1 because both are sampled from the same transcript state, before | ||
| // the commitment is appended. | ||
| let zeta1 = transcript.sample_field_element(); | ||
| let zeta2 = transcript.sample_field_element(); | ||
|
|
There was a problem hiding this comment.
Medium – both challenges sampled before committing: soundness note
The comment argues this is sound because the prover already fixed f_k in the previous commitment and f_{k+2} is fully determined by f_k, zeta1, zeta2. That argument is correct for a honest prover, but it deviates from the standard Fiat-Shamir assumption used in security proofs.
In standard arity-2 FRI the challenge zeta_k is derived from the transcript state including C_k, so the prover cannot choose C_k after seeing zeta_k. Here, zeta1 and zeta2 are both derived from the state before C_{k+1} is appended. A prover that has some slack in choosing f_{k+1} (e.g., from a weak binding), would learn both challenges before fixing the committed polynomial.
If the security proof for this construction has been written up, it would help to reference it in the comment.
Review: Arity-4 FRI (PR #531)This PR upgrades the FRI commit/query/verify pipeline from arity-2 (binary folding, pair-leaf Merkle trees) to arity-4 (two binary folds per committed layer, quad-leaf trees), halving the number of Merkle commits and proof elements. The structural change is coherent and the new HighPanic for small domain sizes (inline on Fragile hand-rolled twiddle for the "b" pair (inline on MediumBoth zeta challenges sampled before committing (inline on Missing test for even LowMisleading |
nicole-graus
changed the title
|
/bench |
# Conflicts: # crypto/stark/src/prover.rs
|
/bench 7 |
Self-review follow-ups on the arity-4 FRI change:
Dead code
- `config.rs`: the switch to quad FRI layers left `FriLayerMerkleTree` /
`FriLayerMerkleTreeBackend` (the old pair-leaf binary-FRI aliases)
referenced only by their own definitions. Removed them and the stale
`PairKeccak256Backend` import. `math-cuda/tests/keccak_leaves.rs` was
the only other user — repointed at `crypto::merkle_tree::backends::
types::PairKeccak256Backend` directly (same type).
- `verifier.rs`: `DeepPolynomialEvaluations` (a 2-tuple alias) was
orphaned when `reconstruct_deep_composition_poly_evaluations_for_all_
queries` started returning a 4-tuple. Removed.
Wrong comment
- `verify_composition_poly_opening` claimed the composition-poly tree
"has pair leaves (PairKeccak256Backend)". It is a `BatchedMerkleTree`
whose leaves each hold a row-pair. Comment corrected.
Naming
- `PolynomialOpenings` / `CompositionPolyOpenings` had fields
`evaluations`, `evaluations_sym`, `evaluations_2`, `evaluations_3`.
`evaluations_sym` is a binary-FRI carryover ("sym" = the -x point);
in the arity-4 orbit it is just position index^1. Renamed to
`evaluations_1` for consistency with `_2` / `_3`.
Behaviour-preserving: 125 stark lib tests pass; lint clean.
2 participants
Description
Implements FRI arity-4 folding (2 binary folds per committed layer using a quad-leaf Merkle tree) with batch Merkle proofs for the arity-4 trace openings. Each query reveals 4 orbit positions per trace tree via a single shared auth path instead of 4 independent paths.
Benchmark Results
Prover-only (Fibonacci AIR, Criterion median of 10)
Full VM (Lambda VM)