-
Notifications
You must be signed in to change notification settings - Fork 187
perf(plonk): optimize prover with batch inversion and direct evaluations #1143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
80d1fe7
338f81f
1263466
41921f1
5d17814
ee681f2
62f5931
31b9798
b5d3655
5f4257c
27b6e23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -334,28 +334,41 @@ where | |
| gamma: FieldElement<F>, | ||
| ) -> Round2Result<F, CS::Commitment> { | ||
| let cpi = common_preprocessed_input; | ||
| let mut coefficients: Vec<FieldElement<F>> = vec![FieldElement::one()]; | ||
| let (s1, s2, s3) = (&cpi.s1_lagrange, &cpi.s2_lagrange, &cpi.s3_lagrange); | ||
|
|
||
| let k2 = &cpi.k1 * &cpi.k1; | ||
|
|
||
| let lp = |w: &FieldElement<F>, eta: &FieldElement<F>| w + &beta * eta + γ | ||
|
|
||
| for i in 0..&cpi.n - 1 { | ||
| // Compute all numerators and denominators first. | ||
| // We need n-1 factors to compute n coefficients: z[0]=1, z[i+1]=z[i]*factor[i] for i in 0..n-1. | ||
| // This matches the original loop range `0..cpi.n - 1`. | ||
| let n_minus_1 = cpi.n - 1; | ||
| let mut numerators = Vec::with_capacity(n_minus_1); | ||
| let mut denominators = Vec::with_capacity(n_minus_1); | ||
|
|
||
| for i in 0..n_minus_1 { | ||
| let (a_i, b_i, c_i) = (&witness.a[i], &witness.b[i], &witness.c[i]); | ||
| let num = lp(a_i, &cpi.domain[i]) | ||
| * lp(b_i, &(&cpi.domain[i] * &cpi.k1)) | ||
| * lp(c_i, &(&cpi.domain[i] * &k2)); | ||
| let den = lp(a_i, &s1[i]) * lp(b_i, &s2[i]) * lp(c_i, &s3[i]); | ||
| // den != 0 with overwhelming probability because beta and gamma are random elements. | ||
| let new_factor = (num / den).expect( | ||
| "division by zero in permutation polynomial: beta and gamma should prevent this", | ||
| ); | ||
|
|
||
| let new_term = coefficients | ||
| .last() | ||
| .expect("coefficients vector is non-empty") | ||
| * &new_factor; | ||
| numerators.push(num); | ||
| denominators.push(den); | ||
| } | ||
|
|
||
| // Batch invert all denominators at once (much faster than n-1 individual inversions) | ||
| FieldElement::inplace_batch_inverse(&mut denominators).expect( | ||
| "batch inversion failed in permutation polynomial: beta and gamma should prevent zeros", | ||
| ); | ||
|
|
||
| // Compute coefficients using the inverted denominators | ||
| let mut coefficients: Vec<FieldElement<F>> = Vec::with_capacity(cpi.n); | ||
| coefficients.push(FieldElement::one()); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correctness
Security
Performance
Bugs & Errors
Code Simplicity
Overall, assumptions in mathematical operations need further scrutiny to ensure that the constraints and debug assertions align with real-world and adversarial inputs. Consider potential refactorings or performance enhancements after addressing foundational issues. |
||
| for i in 0..n_minus_1 { | ||
| let factor = &numerators[i] * &denominators[i]; | ||
| let new_term = coefficients.last().expect("coefficients non-empty") * &factor; | ||
| coefficients.push(new_term); | ||
| } | ||
|
|
||
|
|
@@ -386,10 +399,6 @@ where | |
| let cpi = common_preprocessed_input; | ||
| let k2 = &cpi.k1 * &cpi.k1; | ||
|
|
||
| let one = Polynomial::new_monomial(FieldElement::one(), 0); | ||
| let p_x = &Polynomial::new_monomial(FieldElement::<F>::one(), 1); | ||
| let zh = Polynomial::new_monomial(FieldElement::<F>::one(), cpi.n) - &one; | ||
|
|
||
| let z_x_omega_coefficients: Vec<FieldElement<F>> = p_z | ||
| .coefficients() | ||
| .iter() | ||
|
|
@@ -430,8 +439,25 @@ where | |
| .expect("FFT evaluation of qc must be within field's two-adicity limit"); | ||
| let p_pi_eval = Polynomial::evaluate_offset_fft(&p_pi, 1, Some(degree), offset) | ||
| .expect("FFT evaluation of p_pi must be within field's two-adicity limit"); | ||
| let p_x_eval = Polynomial::evaluate_offset_fft(p_x, 1, Some(degree), offset) | ||
| .expect("FFT evaluation of p_x must be within field's two-adicity limit"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correctness
Security
Performance
Bugs & Errors
Code Simplicity
Consider addressing the above issues before merging to ensure robustness and security of the cryptographic operations. |
||
|
|
||
| // Optimization: p_x = X (identity polynomial), so p_x(offset * ω^i) = offset * ω^i. | ||
| // Generate the coset directly instead of using FFT. | ||
| // Note: This uses the same primitive root that evaluate_offset_fft uses internally, | ||
| // since both derive it from F::get_primitive_root_of_unity for the same domain size. | ||
| let omega = F::get_primitive_root_of_unity(degree.trailing_zeros() as u64) | ||
| .expect("primitive root exists for degree"); | ||
| let p_x_eval: Vec<_> = (0..degree) | ||
| .scan(offset.clone(), |current, _| { | ||
| let val = current.clone(); | ||
| *current = &*current * ω | ||
| Some(val) | ||
| }) | ||
| .collect(); | ||
| debug_assert_eq!( | ||
| p_x_eval.len(), | ||
| p_a_eval.len(), | ||
| "p_x_eval length must match FFT evaluation length" | ||
| ); | ||
| let p_z_eval = Polynomial::evaluate_offset_fft(p_z, 1, Some(degree), offset) | ||
| .expect("FFT evaluation of p_z must be within field's two-adicity limit"); | ||
| let p_z_x_omega_eval = Polynomial::evaluate_offset_fft(&z_x_omega, 1, Some(degree), offset) | ||
|
|
@@ -505,11 +531,38 @@ where | |
| .map(|((p2, p1), co)| (p2 * &alpha + p1) * &alpha + co) | ||
| .collect(); | ||
|
|
||
| let mut zh_eval = Polynomial::evaluate_offset_fft(&zh, 1, Some(degree), offset).expect( | ||
| "FFT evaluation of vanishing polynomial must be within field's two-adicity limit", | ||
| // Optimization: Z_H(x) = x^n - 1 has only 4 distinct values on a coset of size 4n. | ||
| // On coset {offset * ω^i : i = 0..4n-1} where ω is primitive 4n-th root: | ||
| // Z_H(offset * ω^i) = offset^n * (ω^n)^i - 1 | ||
| // Since ω^n is a 4th root of unity, (ω^n)^i cycles through 4 values. | ||
| // | ||
| // SAFETY: This optimization assumes degree == 4 * n. If degree changes (see TODO above), | ||
| // this optimization must be revisited. | ||
| debug_assert_eq!( | ||
| degree, | ||
| 4 * cpi.n, | ||
| "Z_H optimization requires degree == 4n; if degree formula changes, update this code" | ||
| ); | ||
|
Comment on lines
+572
to
576
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Debug-only assumption check
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/provers/plonk/src/prover.rs
Line: 541:545
Comment:
**Debug-only assumption check**
`degree == 4 * cpi.n` is only enforced with `debug_assert_eq!`, so in release builds this optimization can silently produce incorrect `Z_H` evaluations if `degree` ever changes (the surrounding code even has a TODO about the factor of 4). This should be a real `assert_eq!` (or a checked branch that falls back to the FFT path) so invalid proofs can’t be produced in optimized builds.
How can I resolve this? If you propose a fix, please make it concise. |
||
| FieldElement::inplace_batch_inverse(&mut zh_eval) | ||
| .expect("vanishing polynomial evaluations are non-zero because evaluated on coset offset from the roots of unity"); | ||
| let omega_4n = F::get_primitive_root_of_unity(degree.trailing_zeros() as u64) | ||
| .expect("primitive root exists for degree"); | ||
| let omega_n = omega_4n.pow(cpi.n as u64); // ω^n where ω is 4n-th root; this is a 4th root of unity | ||
| let offset_to_n = offset.pow(cpi.n as u64); | ||
|
|
||
| // Compute the 4 distinct Z_H values and their inverses | ||
| // Use multiplication chain for small powers (faster than pow) | ||
| let omega_n_sq = &omega_n * &omega_n; | ||
| let omega_n_cubed = &omega_n_sq * &omega_n; | ||
| let mut zh_base = [ | ||
| &offset_to_n - FieldElement::<F>::one(), // i ≡ 0 (mod 4) | ||
| &offset_to_n * &omega_n - FieldElement::<F>::one(), // i ≡ 1 (mod 4) | ||
| &offset_to_n * &omega_n_sq - FieldElement::<F>::one(), // i ≡ 2 (mod 4) | ||
| &offset_to_n * &omega_n_cubed - FieldElement::<F>::one(), // i ≡ 3 (mod 4) | ||
| ]; | ||
| FieldElement::inplace_batch_inverse(&mut zh_base) | ||
| .expect("Z_H evaluations are non-zero on coset offset from roots of unity"); | ||
|
|
||
| // Build full evaluation vector by cycling through the 4 values | ||
| let zh_eval: Vec<_> = (0..degree).map(|i| zh_base[i % 4].clone()).collect(); | ||
| let c: Vec<_> = p_eval | ||
|
Comment on lines
565
to
597
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Z_H shortcut assumes degree=4n The new At minimum, add an assertion that Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/provers/plonk/src/prover.rs
Line: 526:550
Comment:
**Z_H shortcut assumes degree=4n**
The new `Z_H` evaluation shortcut relies on `degree = 4 * cpi.n` so that `omega_4n` is a primitive `4n`-th root and `omega_n = omega_4n.pow(n)` is a 4th root of unity. This is only valid under that exact relationship and when `degree` is exactly `4n` in the FFT calls. If `degree` changes (the code even has a TODO about “factor of 4”), or if `evaluate_offset_fft` internally uses a different root/order, then `zh_eval` will be wrong and the quotient `t` will be computed incorrectly.
At minimum, add an assertion that `degree == 4 * cpi.n` here (and/or compute `omega_n` from the actual evaluation root used by the FFT helper), so the optimization can’t silently produce invalid proofs when `degree` is adjusted.
How can I resolve this? If you propose a fix, please make it concise. |
||
| .iter() | ||
| .zip(zh_eval.iter()) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correctness:
Security:
Performance:
Bugs & Errors:
Code Simplicity:
Conclusion:The code appears to be moving towards performant and efficient optimization but has some concerns around robustness against unexpected cases and randomness guarantees. The reliance on |
||
|
|
@@ -583,11 +636,19 @@ where | |
| let (r1, r2, r3, r4) = (round_1, round_2, round_3, round_4); | ||
| // Precompute variables | ||
| let k2 = &cpi.k1 * &cpi.k1; | ||
| let zeta_raised_n = Polynomial::new_monomial(r4.zeta.pow(cpi.n + 2), 0); // TODO: Paper says n and 2n, but Gnark uses n+2 and 2n+4 | ||
| let zeta_raised_2n = Polynomial::new_monomial(r4.zeta.pow(2 * cpi.n + 4), 0); | ||
|
|
||
| // Compute zeta powers efficiently: zeta^n, zeta^(n+2), zeta^(2n+4) | ||
| // Start with zeta^n, then derive others to avoid redundant exponentiations | ||
| let zeta_n = r4.zeta.pow(cpi.n as u64); | ||
| let zeta_sq = &r4.zeta * &r4.zeta; | ||
| let zeta_n_plus_2 = &zeta_n * &zeta_sq; // zeta^(n+2) = zeta^n * zeta^2 | ||
| let zeta_2n_plus_4 = &zeta_n_plus_2 * &zeta_n_plus_2; // zeta^(2n+4) = (zeta^(n+2))^2 | ||
|
|
||
| let zeta_raised_n = Polynomial::new_monomial(zeta_n_plus_2, 0); // TODO: Paper says n and 2n, but Gnark uses n+2 and 2n+4 | ||
| let zeta_raised_2n = Polynomial::new_monomial(zeta_2n_plus_4, 0); | ||
|
|
||
| // zeta is sampled outside the set of roots of unity so zeta != 1, and n != 0. | ||
| let l1_zeta = ((&r4.zeta.pow(cpi.n as u64) - FieldElement::<F>::one()) | ||
| let l1_zeta = ((&zeta_n - FieldElement::<F>::one()) | ||
| / ((&r4.zeta - FieldElement::<F>::one()) * FieldElement::<F>::from(cpi.n as u64))) | ||
| .expect("zeta is outside roots of unity so denominator is non-zero"); | ||
|
|
||
|
|
@@ -606,10 +667,11 @@ where | |
| * &r2.beta | ||
| * &r4.z_zeta_omega | ||
| * &cpi.s3; | ||
| let alpha_squared = &r3.alpha * &r3.alpha; | ||
| p_non_constant += (r_2_2 - r_2_1) * &r3.alpha; | ||
|
|
||
| let r_3 = &r2.p_z * l1_zeta; | ||
| p_non_constant += r_3 * &r3.alpha * &r3.alpha; | ||
| p_non_constant += r_3 * &alpha_squared; | ||
|
|
||
| let partial_t = &r3.p_t_lo + zeta_raised_n * &r3.p_t_mid + zeta_raised_2n * &r3.p_t_hi; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctness
solve_hintandsolve_constraintare correctly transforming the assignments, maintaining invariant properties for each constraint.Security
solve_hintandsolve_constraintoperate in constant-time, especially if handling secret data to prevent timing side-channel attacks.assignmentsrequires zeroization after processing.Performance
self.constraints. Consider breaking out early if possible on each iteration after an assignment changes.Bugs & Errors
Variableidentifiers are correctly handled and that any possible conversion or retrieval of hash map values does not panic (e.g., due to missing constraints or invalid keys).Code Simplicity
assignments = solve_hint(assignments, constraint);andassignments = solve_constraint(assignments, constraint);may introduce redundancy. If possible, merging these processes could simplify the implementation, reducing cognitive load.Overall Comment
Ensure thorough testing for edge cases and data-related issues. Verify if a more efficient constraint solving methodology can be implemented to enhance performance without compromising correctness and security.