refactor: pk_generation circuit optimization#1353
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
circuits/lib/src/core/threshold/pk_generation.nr (3)
135-139: Add a comment explaining why a single gamma is sufficient across all L moduli.The design shift from L independent evaluation points to one is not self-evident to future readers or auditors. The Schwartz-Zippel soundness argument still holds per equation (each
assertis an independent polynomial identity; the cheating probability per check isdeg/|F|), but that reasoning should be captured inline.💬 Proposed documentation addition
- // Use single gamma at index 0 for all modulus evaluations - let gamma = gammas.get(0); + // A single Fiat-Shamir evaluation point gamma is used for all L CRT-basis checks. + // Each assertion in verify_public_key_for_modulus is an independent polynomial + // identity test (Schwartz-Zippel), so per-check soundness error is deg(f)/|F| + // regardless of sharing the evaluation point, since gamma is fixed only after + // all witnesses are committed to. + let gamma = gammas.get(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/core/threshold/pk_generation.nr` around lines 135 - 139, Add an inline comment above the gamma usage explaining why reusing a single random evaluation point is sound: note that we take gamma = gammas.get(0) and call verify_public_key_for_modulus(i, gamma) for each i in 0..L because each assert inside verify_public_key_for_modulus is an independent polynomial identity and the Schwartz–Zippel lemma applies to each check separately, so the cheating probability per modulus is bounded by deg/|F| and union-bounds across L checks give the overall soundness; mention L, gamma, gammas.get(0), and verify_public_key_for_modulus by name so readers can locate the logic.
137-139: Missed O(L²) → O(L) constraint reduction enabled by the single-gamma change.
verify_public_key_for_modulusevaluates all five polynomial arrays (a,r1,r2,pk0,pk1) and three shared polynomials (sk,eek,cyclo) in full on every loop iteration, at the samegamma. Before this PR each iteration used a distinctgamma_i, so this was unavoidable. With a shared gamma, these evaluations are now identical across all L iterations — meaning you're paying for L × (L + 3) polynomial evaluations when L + 3 would suffice.The fix is to hoist all evaluations before the loop and pass the indexed values directly:
♻️ Proposed refactor (sketch)
+ // Evaluate all polynomials at gamma once + let a_at_gamma = self.a.map(|p| p.eval(gamma)); + let r1_at_gamma = self.r1.map(|p| p.eval(gamma)); + let r2_at_gamma = self.r2.map(|p| p.eval(gamma)); + let pk0_at_gamma = self.pk0.map(|p| p.eval(gamma)); + let pk1_at_gamma = self.pk1.map(|p| p.eval(gamma)); + let eek_at_gamma = self.eek.eval(gamma); + let sk_at_gamma = self.sk.eval(gamma); + let cyclo_at_gamma = gamma.pow_32(N as Field) + 1; + for i in 0..L { - self.verify_public_key_for_modulus(i, gamma); + // pk0_i = -a_i * sk + eek + r2_i * (X^N+1) + r1_i * q_i + let expected_pk0 = -a_at_gamma[i] * sk_at_gamma + + eek_at_gamma + + r2_at_gamma[i] * cyclo_at_gamma + + r1_at_gamma[i] * self.configs.qis[i]; + assert(pk0_at_gamma[i] == expected_pk0, "Public key equation 1 failed"); + assert(pk1_at_gamma[i] == a_at_gamma[i], "Public key equation 2 failed"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/core/threshold/pk_generation.nr` around lines 137 - 139, The loop calling verify_public_key_for_modulus(i, gamma) re-evaluates all polynomial arrays (a, r1, r2, pk0, pk1) and shared polynomials (sk, eek, cyclo) on every iteration even though gamma is the same; hoist the common evaluations out of the for i in 0..L loop by computing the per-indexed polynomial values for each array once (or compute the shared-evaluation results once) and then call verify_public_key_for_modulus using those precomputed/indexed values (or change its signature to accept the already-evaluated values) so the work is O(L + 3) instead of O(L²).
133-148: Incomplete optimization: sponge still squeezes2 * Lelements despite only usinggammas.get(0).
generate_challengecallscompute_threshold_pk_challenge::<L>, which squeezes2 * Lfield elements from the SAFE sponge (as determined by the IO pattern[..., 2 * L]). Onlygammas.get(0)is ever consumed; the remaining2 * L - 1squeezed values are silently dropped. In a ZK circuit, each sponge squeeze is a full Poseidon2 permutation — one of the most constraint-expensive operations. The optimization is defeated because those2 * L - 1extra permutations still add constraints.The generic parameter should be
1andgenerate_challengeshould return a singleFielddirectly.♻️ Proposed fix
- fn generate_challenge(self, sk_commitment: Field, pk_commitment: Field) -> Vec<Field> { + fn generate_challenge(self, sk_commitment: Field, pk_commitment: Field) -> Field { let inputs = self.payload(sk_commitment, pk_commitment); - compute_threshold_pk_challenge::<L>(inputs) + compute_threshold_pk_challenge::<1>(inputs).get(0) }And update the call site accordingly:
- let gammas = self.generate_challenge(sk_commitment, pk_commitment); - - // Use single gamma at index 0 for all modulus evaluations - let gamma = gammas.get(0); + let gamma = self.generate_challenge(sk_commitment, pk_commitment);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/core/threshold/pk_generation.nr` around lines 133 - 148, generate_challenge currently calls compute_threshold_pk_challenge::<L> which squeezes 2*L elements but only gammas.get(0) is used; change generate_challenge to return a single Field (not Vec<Field>) by calling compute_threshold_pk_challenge::<1> and extracting that single squeezed element, then update the call site where generate_challenge was used: replace let gammas = self.generate_challenge(...) and let gamma = gammas.get(0) with let gamma = self.generate_challenge(...), and keep the loop calling self.verify_public_key_for_modulus(i, gamma) unchanged; update any function signatures and types referencing generate_challenge accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@circuits/lib/src/core/threshold/pk_generation.nr`:
- Around line 135-139: Add an inline comment above the gamma usage explaining
why reusing a single random evaluation point is sound: note that we take gamma =
gammas.get(0) and call verify_public_key_for_modulus(i, gamma) for each i in
0..L because each assert inside verify_public_key_for_modulus is an independent
polynomial identity and the Schwartz–Zippel lemma applies to each check
separately, so the cheating probability per modulus is bounded by deg/|F| and
union-bounds across L checks give the overall soundness; mention L, gamma,
gammas.get(0), and verify_public_key_for_modulus by name so readers can locate
the logic.
- Around line 137-139: The loop calling verify_public_key_for_modulus(i, gamma)
re-evaluates all polynomial arrays (a, r1, r2, pk0, pk1) and shared polynomials
(sk, eek, cyclo) on every iteration even though gamma is the same; hoist the
common evaluations out of the for i in 0..L loop by computing the per-indexed
polynomial values for each array once (or compute the shared-evaluation results
once) and then call verify_public_key_for_modulus using those
precomputed/indexed values (or change its signature to accept the
already-evaluated values) so the work is O(L + 3) instead of O(L²).
- Around line 133-148: generate_challenge currently calls
compute_threshold_pk_challenge::<L> which squeezes 2*L elements but only
gammas.get(0) is used; change generate_challenge to return a single Field (not
Vec<Field>) by calling compute_threshold_pk_challenge::<1> and extracting that
single squeezed element, then update the call site where generate_challenge was
used: replace let gammas = self.generate_challenge(...) and let gamma =
gammas.get(0) with let gamma = self.generate_challenge(...), and keep the loop
calling self.verify_public_key_for_modulus(i, gamma) unchanged; update any
function signatures and types referencing generate_challenge accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@circuits/lib/src/core/threshold/pk_generation.nr`:
- Around line 135-137: The loop calling verify_public_key_for_modulus(i, gamma)
does redundant O(L²) polynomial evaluations—hoist all evaluations at gamma once
before the loop (evaluate self.a, self.r1, self.r2, self.pk0, self.pk1 as arrays
at gamma and the scalars self.eek, self.sk, cyclo) and change
verify_public_key_for_modulus to accept those precomputed arrays/scalars (or a
struct) plus index i so it only indexes into the evaluated arrays instead of
re-mapping every call; update the loop to pass the precomputed values and the
index to reduce complexity to O(L).
0xjei
changed the title
0xjei
changed the title
0xjei
force-pushed
the
circuits/pk-generation
branch
2 times, most recently
from
9b18392 to
4b0da8b
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@circuits/lib/src/core/threshold/pk_generation.nr`:
- Around line 135-137: The loop calling self.verify_public_key_for_modulus(i,
gamma) over 0..L was flagged for redundant polynomial evaluations but the Noir
compiler's CSE already eliminates the repeated .eval(gamma) work; leave the loop
as-is and add a brief inline comment above the loop referencing that compiler
CSE removes duplicate polynomial.eval(gamma) calls to avoid future unnecessary
manual refactors — locate the loop at the call site of
verify_public_key_for_modulus and mention L and gamma by name in the comment.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
circuits/lib/src/core/threshold/pk_generation.nr (1)
178-189: Optional: extracta_i_at_gammato avoid the double evaluation ofself.a[i].eval(gamma)per iteration.
self.a[i].eval(gamma)is evaluated at both lines 179 and 186 within the same loop body. The Noir compiler will CSE this automatically (confirmed by prior empirical testing), so there is no constraint overhead. However, extracting it as a local binding improves readability.♻️ Proposed optional refactor
for i in 0..L { + let a_i_at_gamma = self.a[i].eval(gamma); let expected_pk0 = -self.a[i].eval(gamma) * sk_at_gamma + let expected_pk0 = -a_i_at_gamma * sk_at_gamma + eek_at_gamma + self.r2[i].eval(gamma) * cyclo_at_gamma + self.r1[i].eval(gamma) * self.configs.qis[i]; assert(self.pk0[i].eval(gamma) == expected_pk0, "Public key equation 1 failed"); assert( - self.pk1[i].eval(gamma) == self.a[i].eval(gamma), + self.pk1[i].eval(gamma) == a_i_at_gamma, "Public key equation 2 failed", ); }Based on learnings: the Noir compiler already optimizes these redundant evaluations via CSE, so this is a readability-only improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/core/threshold/pk_generation.nr` around lines 178 - 189, Extract a_i_at_gamma = self.a[i].eval(gamma) at the top of the loop body and use that local binding in both the expected_pk0 computation and the pk1 assertion to avoid evaluating self.a[i].eval(gamma) twice; update the expressions that currently call self.a[i].eval(gamma) (used in expected_pk0 and the self.pk1[i].eval(gamma) == ... assertion) to reference a_i_at_gamma instead for improved readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@circuits/lib/src/core/threshold/pk_generation.nr`:
- Around line 178-189: Extract a_i_at_gamma = self.a[i].eval(gamma) at the top
of the loop body and use that local binding in both the expected_pk0 computation
and the pk1 assertion to avoid evaluating self.a[i].eval(gamma) twice; update
the expressions that currently call self.a[i].eval(gamma) (used in expected_pk0
and the self.pk1[i].eval(gamma) == ... assertion) to reference a_i_at_gamma
instead for improved readability.
9eb965d to
321fd82
Compare
0xjei
force-pushed
the
circuits/pk-generation
branch
from
55a422c to
44ea9eb
Compare
3 participants
Use a single Fiat-Shamir challenge gamma at index 0 for all CRT basis evaluations in Pk Generation, and remove e_sm_commitment from the challenge generation payload as it is not needed.
Summary by CodeRabbit
Refactor
Chores
Tests / Docs