Feat/higher arity fri#464
Conversation
New configuration fields for higher-arity FRI folding: - fri_log_arity: log2 of folding arity per round (1=arity-2 default) - fri_log_final_poly_len: log2 of final polynomial length (0=fold to constant) Defaults reproduce current arity-2 behavior exactly.
- StarkProof/Round4: fri_last_value -> fri_final_poly: Vec<FE> - FriDecommitment: layers_evaluations_sym -> Vec<Vec<FE>> (K-1 siblings) - FriLayerMerkleTreeBackend: PairKeccak256Backend -> BatchKeccak256Backend - Leaf construction: fixed [FE; 2] arrays -> Vec<FE> Defaults produce vec![single_value] and vec![single_sibling], preserving arity-2 behavior. All 129 tests pass unchanged.
Rewrite commit_phase_from_evaluations to support configurable folding arity: - Each round samples ONE challenge, applies log_arity sequential binary folds with squaring challenges, then builds one Merkle tree - Leaf groups are 2^log_arity elements (variable-size Vec leaves) - Final polynomial sent as Vec of coefficients (supports early termination) Rewrite query_phase for variable-size siblings: - Each layer returns K-1 siblings (all group elements except the queried one) - Leaf index = query_index / group_size With default log_arity=1, this produces identical results to the previous arity-2 implementation. All 129 tests pass.
Rewrite verify_query_and_sym_openings to support configurable folding arity: - Initial fold remains arity-2 (combining DEEP evaluations) - Each committed layer reconstructs the full 2^log_arity element group from the queried value + K-1 siblings - Verifies Merkle opening against the reconstructed group - fold_group_verifier applies log_arity sequential binary folds with squaring challenges and domain-computed inverse twiddles - Twiddles computed on-the-fly from coset_offset + primitive root + bit-reversal - Tracks domain state (log_size, offset) across layers With default log_arity=1, this produces identical verification to the previous arity-2 implementation. All 129 tests pass.
Three new tests: - test_fri_arity_4_prove_verify: 64-row Fibonacci AIR with log_arity=2 - test_fri_arity_8_prove_verify: same with log_arity=3 - test_fri_arity_4_has_fewer_merkle_roots_than_arity_2: verifies arity-4 produces strictly fewer FRI layer commitments Also fixes two bugs found by the tests: - Prover: round 0 must fold exactly 1x (initial arity-2), not log_arity - Verifier: fold_group_verifier must not halve leaf_index in sub-fold loop
Change GoldilocksCubicProofOptions default fri_log_arity from 1 to 2 (arity-4). This halves the number of FRI Merkle trees committed during the proof, reducing FRI Merkle hashing work by ~50%. All 132 tests pass including arity-specific roundtrip tests.
|
/bench |
Codex Code ReviewFindings
No other concrete security, correctness, or significant performance issues stood out in the diff. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // the final value directly here. | ||
| if fri_layers_merkle_roots.is_empty() { | ||
| return v == proof.fri_last_value; | ||
| return v == proof.fri_final_poly[index]; |
There was a problem hiding this comment.
High: Panic on malicious proof input
proof.fri_final_poly[index] panics if a malicious prover sends a fri_final_poly that is shorter than index. Since index at this point is still the original iota (unchanged — the initial fold does not update index), and iota can be up to lde_length/2 - 1, any proof with fri_final_poly.len() < iota + 1 panics the verifier.
The same issue exists at line 698 (after the committed-layers loop).
| return v == proof.fri_final_poly[index]; | |
| return proof.fri_final_poly.get(index).map_or(false, |val| &v == val); |
| 0 | ||
| } else { | ||
| // Largest T ≤ max_folds with T = 1 + n*log_arity for integer n ≥ 0. | ||
| let n = (max_folds - 1) / log_arity; |
There was a problem hiding this comment.
High: Division by zero when fri_log_arity = 0
If ProofOptions::fri_log_arity is set to 0, log_arity = 0 here and this line panics (integer division by zero). There is also a second division at line 73 ((total_folds - 1) / log_arity) with the same problem. Additionally, if somehow execution reached the while loop with log_arity = 0, all rounds after the first would have folds_this_round = 0, making folds_done never increase — an infinite loop.
fri_log_arity should be validated to be >= 1 in ProofOptions, or guarded here:
assert!(log_arity >= 1, "fri_log_arity must be at least 1 (arity >= 2)");
| } | ||
|
|
||
| // Check that the final value matches the final polynomial at the residual index. | ||
| result &= v == proof.fri_final_poly[index]; |
There was a problem hiding this comment.
High: Same unchecked index panic as line 642
After the committed-layers loop, index is the leaf_index from the last layer. A malicious prover can send fri_final_poly with fewer elements than index + 1, causing a panic.
| result &= v == proof.fri_final_poly[index]; | |
| result &= proof.fri_final_poly.get(index).map_or(false, |val| &v == val); |
| let mut result = true; | ||
|
|
||
| for (layer_idx, merkle_root) in fri_layers_merkle_roots.iter().enumerate() { | ||
| let siblings = &fri_decommitment.layers_evaluations_sym[layer_idx]; |
There was a problem hiding this comment.
Medium: Panic on malformed decommitment
layers_evaluations_sym[layer_idx] and layers_auth_paths[layer_idx] (line 654) both panic if a malicious or malformed proof has fewer decommitment entries than fri_layers_merkle_roots. Consider returning false instead:
let Some(siblings) = fri_decommitment.layers_evaluations_sym.get(layer_idx) else { return false; };
let Some(auth_path) = fri_decommitment.layers_auth_paths.get(layer_idx) else { return false; };
| // Each successive sub-fold halves `half`, so the product `leaf_index * half` correctly | ||
| // tracks the pair position in the correspondingly smaller domain. | ||
| let mut inv_twiddles: Vec<FieldElement<Field>> = Vec::with_capacity(half); | ||
| let half_log_bits = current_domain_size / 2; |
There was a problem hiding this comment.
Low: Misleading variable name
half_log_bits sounds like a log or bit count, but it's actually the half-domain size (current_domain_size / 2). The reverse_index function takes a size (not a bit count), so the value passed is correct — but the name will confuse readers. Consider half_domain_size or just inline the expression.
|
Review: Feat/higher arity FRI. Good implementation overall - the protocol structure is clearly documented and the roundtrip tests are a nice addition. Issues found: (1) HIGH - Verifier panics on malicious proof input: proof.fri_final_poly[index] at lines 642 and 698 has no bounds check, and fri_decommitment.layers_evaluations_sym[layer_idx] at 653-654 can also panic. A crafted proof can crash the verifier. All should return false rather than panicking. (2) HIGH - Division by zero when fri_log_arity=0: commit_phase_from_evaluations divides by log_arity at lines 65 and 73 with no guard. Also causes an infinite loop in the while loop since folds_this_round=0. Add assert or validate in ProofOptions. (3) LOW - Misleading name half_log_bits at verifier line 747: holds current_domain_size/2 which is a domain SIZE not a log value. Suggest half_domain_size. |
|
/bench 10 |
|
/bench 5 |
Benchmark — fib_iterative_8M (median of 3)Table parallelism: 32 (auto = cores / 3)
Commit: d7318ad · Baseline: cached · Runner: self-hosted bench |
|
/bench |
|
/bench |
|
/bench 3 |
|
/bench 3 |
2 participants
No description provided.