feat: opt/c5 assert zero mod [skip-line-limit]#1579
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@circuits/lib/src/core/threshold/pk_aggregation.nr`:
- Around line 35-68: The Inputs struct and related functions still supply
pk1/pk1_agg and compute commitments using them; update Inputs, compute(), and
to_json() to remove pk1 and pk1_agg, accept and pass the circuit CRP (crp)
instead, and compute expected_threshold_pk_commitments using the same
routine/signature the Noir circuit expects (replace calls that used data.a or
data.public_key.c[1] with the CRP input and pk0/ pk0_agg); specifically, modify
Inputs to include crp and pk0/pk0_agg only, change compute() to call
compute_threshold_pk_commitment(&pk0[..], &crp[..], ...) or the exact updated
helper signature, and emit the updated fields in to_json() so the public input
matches expected_threshold_pk_commitments, pk0, pk0_agg, and crp.
In `@circuits/lib/src/math/commitments.nr`:
- Around line 172-177: The Rust implementation of
compute_threshold_pk_commitment currently flattens both pk0 and pk1, causing a
mismatch with the Noir circuit which computes the commitment from only pk0 with
DS_PK_GENERATION; update the Rust function
(crates/zk-helpers/src/circuits/commitments.rs::compute_threshold_pk_commitment)
and any callers (e.g.,
crates/zk-helpers/src/circuits/threshold/pk_aggregation/computation.rs) to build
the payload from only pk0 (drop pk1 flattening) and use DS_PK_GENERATION as the
domain separator so the computed hash matches the Noir circuit’s
compute_threshold_pk_commitment(pk0).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1284d0e3-7856-4def-9751-f7b80de5db10
📒 Files selected for processing (4)
circuits/bin/threshold/pk_aggregation/src/main.nrcircuits/lib/src/core/threshold/pk_aggregation.nrcircuits/lib/src/core/threshold/pk_generation.nrcircuits/lib/src/math/commitments.nr
0xjei
changed the title
0xjei
force-pushed
the
opt/c5-assert-zero-mod
branch
from
92a1f82 to
3d056cb
Compare
Contributor
|
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:
📝 WalkthroughWalkthroughThe PR removes per-party ChangesPK1 elimination and CRS integration
Sequence DiagramsequenceDiagram
participant PkGeneration
participant Commitments
participant Inputs
participant PkAggregation
PkGeneration->>Commitments: compute_threshold_pk_commitment(pk0)
Inputs->>Commitments: compute_threshold_pk_commitment(pk0[i]) (per party)
Inputs->>PkAggregation: supply expected_threshold_pk_commitments, pk0, pk0_agg, crp
PkAggregation->>PkAggregation: verify pk0 commitments and per-basis aggregation (assert_zero_mod)
PkAggregation->>Commitments: compute_pk_aggregation_commitment(pk0_agg, crp)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
0xjei
force-pushed
the
opt/c5-assert-zero-mod
branch
from
3d056cb to
279878a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-helpers/src/circuits/commitments.rs (1)
223-231:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the helper docs to match the new pk0-only contract.
Line 250 now computes the commitment from
pk0only, but the doc block still says this hashespk0together withCRP (pk1). That makes the public helper contract misleading for downstream audit callers.Diff to align the docs
-/// Deserializes the keyshare, extracts the pk0 polynomial, and hashes it -/// together with the CRP (pk1) to produce the commitment. Returns 32 +/// Deserializes the keyshare, extracts the pk0 polynomial, and hashes only +/// pk0 to produce the commitment. Returns 32 /// big-endian bytes, ready to compare against /// `Proof::extract_output("pk_commitment")` from a C1 proof. /// -/// The caller supplies pre-built `params` and `crp` so that batch calls -/// (multiple keyshares with the same parameters) don't rebuild them each time. +/// The caller supplies pre-built `params` and `crp`; `crp` is only needed to +/// deserialize `PublicKeyShare`, not as commitment input.Also applies to: 250-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/commitments.rs` around lines 223 - 231, The doc comment for the pk commitment helper is out of date: it still claims the commitment hashes "pk0 together with the CRP (pk1)" but the code at the pk_commitment computation now uses only pk0. Update the doc block in crates/zk-helpers/src/circuits/commitments.rs (the comment above the pk commitment helper that references pk0, CRP (pk1), and Proof::extract_output("pk_commitment")) to reflect the new pk0-only contract, clearly stating the function computes the commitment from pk0 alone and that callers supply params and crp for batching but the CRP is not mixed into the hash.
🧹 Nitpick comments (1)
crates/zk-helpers/src/circuits/commitments.rs (1)
699-705: ⚡ Quick winMake the roundtrip expectation independent of
compute_threshold_pk_commitment.Line 704 reuses the same helper that the production path now depends on, so a regression in the new pk0-only hashing contract could still pass this test. Build
expectedfrom the flattenedpk0payload andDS_PK_GENERATIONdirectly, like the other manual-commitment tests in this file.Diff to harden the test
- let expected = compute_threshold_pk_commitment(&pk0, bit_pk); + let mut payload = Vec::new(); + payload = flatten(payload, &pk0.limbs, bit_pk); + let io_pattern = [0x80000000 | payload.len() as u32, 1]; + let expected = + field_to_bigint(compute_commitments(payload, DS_PK_GENERATION, io_pattern)[0]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/commitments.rs` around lines 699 - 705, The test currently builds expected via compute_threshold_pk_commitment which mirrors production logic and can mask regressions; instead construct expected by flattening pk0's payload bytes and hashing them with DS_PK_GENERATION directly (the same pattern used in other manual-commitment tests). Locate the pk0 variable (CrtPolynomial from pk_share.p0_share()), call its flatten/serialization to big-endian bytes (the same output used by to_bytes_be), prepend or combine the DS_PK_GENERATION domain/tag, run the hash/commitment routine used in other tests to produce expected and then call to_bytes_be on that result for be_bytes; replace the compute_threshold_pk_commitment call with this explicit flatten+DS_PK_GENERATION construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/zk-helpers/src/circuits/threshold/pk_aggregation/computation.rs`:
- Around line 160-166: The computation currently derives crp from data.a which
can drift from the circuit's hard-coded CRP; update the code in computation.rs
so crp is obtained from the preset constant (e.g. use preset.crp or
lib::configs::default::threshold::CRP) instead of data.a when building the
commitment for pk0_agg, or alternatively add a fast-fail check that asserts
data.a equals the preset CRP and returns an error if not; adjust the block
around pk0_agg/Cr tPolynomial::from_fhe_polynomial, crp.reverse(), and
crp.center(threshold_params.moduli()) to use the preset-derived crp (or perform
the equality check) so commit(pk0_agg, crp) matches the proof generator.
---
Outside diff comments:
In `@crates/zk-helpers/src/circuits/commitments.rs`:
- Around line 223-231: The doc comment for the pk commitment helper is out of
date: it still claims the commitment hashes "pk0 together with the CRP (pk1)"
but the code at the pk_commitment computation now uses only pk0. Update the doc
block in crates/zk-helpers/src/circuits/commitments.rs (the comment above the pk
commitment helper that references pk0, CRP (pk1), and
Proof::extract_output("pk_commitment")) to reflect the new pk0-only contract,
clearly stating the function computes the commitment from pk0 alone and that
callers supply params and crp for batching but the CRP is not mixed into the
hash.
---
Nitpick comments:
In `@crates/zk-helpers/src/circuits/commitments.rs`:
- Around line 699-705: The test currently builds expected via
compute_threshold_pk_commitment which mirrors production logic and can mask
regressions; instead construct expected by flattening pk0's payload bytes and
hashing them with DS_PK_GENERATION directly (the same pattern used in other
manual-commitment tests). Locate the pk0 variable (CrtPolynomial from
pk_share.p0_share()), call its flatten/serialization to big-endian bytes (the
same output used by to_bytes_be), prepend or combine the DS_PK_GENERATION
domain/tag, run the hash/commitment routine used in other tests to produce
expected and then call to_bytes_be on that result for be_bytes; replace the
compute_threshold_pk_commitment call with this explicit flatten+DS_PK_GENERATION
construction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e0fe746-1979-4f8e-98bc-671cd42a82ba
📒 Files selected for processing (9)
circuits/bin/threshold/pk_aggregation/src/main.nrcircuits/lib/src/configs/default/mod.nrcircuits/lib/src/core/threshold/pk_aggregation.nrcircuits/lib/src/core/threshold/pk_generation.nrcircuits/lib/src/math/commitments.nrcrates/zk-helpers/src/circuits/commitments.rscrates/zk-helpers/src/circuits/threshold/pk_aggregation/computation.rscrates/zk-helpers/src/circuits/threshold/pk_aggregation/sample.rscrates/zk-prover/tests/local_e2e_tests.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- circuits/lib/src/core/threshold/pk_generation.nr
- circuits/lib/src/math/commitments.nr
- circuits/bin/threshold/pk_aggregation/src/main.nr
- circuits/lib/src/core/threshold/pk_aggregation.nr
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
542-554: ⚡ Quick winConsider validating jq output and extracting to a reusable action.
The script reads version values using jq but doesn't validate the extraction succeeded. If a key is missing, jq outputs "null" as a string, leading to an invalid version.json that fails later during setup checks rather than immediately.
Additionally, this exact pattern is duplicated in three jobs (lines 542-554, 815-827, 1259-1271). Consider extracting it to a composite action for maintainability.
✨ Suggested improvements
Add validation after jq extraction:
REQUIRED_BB="$(jq -r '.required_bb_version' "$VERSIONS_JSON")" REQUIRED_CIRCUITS="$(jq -r '.required_circuits_version' "$VERSIONS_JSON")" +if [ "$REQUIRED_BB" = "null" ] || [ -z "$REQUIRED_BB" ] || [ "$REQUIRED_CIRCUITS" = "null" ] || [ -z "$REQUIRED_CIRCUITS" ]; then + echo "Error: Failed to extract required versions from $VERSIONS_JSON" + exit 1 +fiConsider creating
.github/actions/setup-noir-circuits/action.yml:name: 'Setup Noir Circuits' description: 'Fetches circuit artifacts and writes version.json' inputs: noir-dir: description: 'Path to Noir directory' required: true runs: using: "composite" steps: - shell: bash run: | VERSIONS_JSON="crates/zk-prover/versions.json" REQUIRED_BB="$(jq -r '.required_bb_version' "$VERSIONS_JSON")" REQUIRED_CIRCUITS="$(jq -r '.required_circuits_version' "$VERSIONS_JSON")" # ... rest of script using ${{ inputs.noir-dir }}Then replace duplicated blocks with:
- name: 'Setup circuit artifacts' uses: ./.github/actions/setup-noir-circuits with: noir-dir: tests/integration/.enclave/noir # or $HOME/.enclave/noir🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 542 - 554, The jq extraction into REQUIRED_BB and REQUIRED_CIRCUITS (using VERSIONS_JSON) can return "null" strings and currently isn't validated, and the same block manipulating INTEGRATION_NOIR/version.json is duplicated across jobs; update each block to check that jq produced non-empty/non-"null" values (fail early with a clear error if validation fails) before writing version.json and fetching artifacts, and factor the logic into a reusable composite action (e.g., .github/actions/setup-noir-circuits) that accepts a noir-dir input and performs the VERSIONS_JSON read, validation of REQUIRED_BB/REQUIRED_CIRCUITS, directory setup, git fetch/archive, and writing of version.json so the three duplicated blocks can be replaced with a single action use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 542-554: The jq extraction into REQUIRED_BB and REQUIRED_CIRCUITS
(using VERSIONS_JSON) can return "null" strings and currently isn't validated,
and the same block manipulating INTEGRATION_NOIR/version.json is duplicated
across jobs; update each block to check that jq produced non-empty/non-"null"
values (fail early with a clear error if validation fails) before writing
version.json and fetching artifacts, and factor the logic into a reusable
composite action (e.g., .github/actions/setup-noir-circuits) that accepts a
noir-dir input and performs the VERSIONS_JSON read, validation of
REQUIRED_BB/REQUIRED_CIRCUITS, directory setup, git fetch/archive, and writing
of version.json so the three duplicated blocks can be replaced with a single
action use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cc440b0-7f96-466c-86ce-5a54906e493d
📒 Files selected for processing (1)
.github/workflows/ci.yml
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/build-circuits.ts (1)
528-539:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStore the concrete committee in
.active-preset.jsonfor per-committee builds.With
--committee all, the active stamp currently writesthis.options.committee("all") instead of the concrete loop committee. That makesisBinReadyForPreset()fail the committee match and prevents--skip-if-built/hydrate fast-path reuse for specific committees.Suggested fix
- private writeActiveBinPresetStamp(preset: string, sourceHash: string): void { + private writeActiveBinPresetStamp(preset: string, committee: CircuitCommittee, sourceHash: string): void { const stamp: PresetBuildStamp = { preset, - committee: this.options.committee, + committee, sourceHash, builtAt: new Date().toISOString(), } writeFileSync(join(this.circuitsDir, '.active-preset.json'), JSON.stringify(stamp, null, 2) + '\n') } - private hydrateBinFromDist(preset: string, committee: string, sourceHash: string): void { + private hydrateBinFromDist(preset: string, committee: CircuitCommittee, sourceHash: string): void { @@ - this.writeActiveBinPresetStamp(preset, sourceHash) + this.writeActiveBinPresetStamp(preset, committee, sourceHash) } @@ - this.writeActiveBinPresetStamp(preset, sourceHash) + this.writeActiveBinPresetStamp(preset, committee, sourceHash)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-circuits.ts` around lines 528 - 539, The active preset stamp currently records this.options.committee (e.g. "all") instead of the concrete loop variable committee, breaking isBinReadyForPreset() matches; update the call and implementation to store the concrete committee: change the invocation this.writeActiveBinPresetStamp(preset, sourceHash) to pass the loop committee (e.g. this.writeActiveBinPresetStamp(preset, committee, sourceHash)) and adjust the writeActiveBinPresetStamp function to accept a committee parameter and persist that committee into .active-preset.json (instead of using this.options.committee), so per-committee builds can be recognized for --skip-if-built/hydrate reuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/build-circuits.ts`:
- Around line 528-539: The active preset stamp currently records
this.options.committee (e.g. "all") instead of the concrete loop variable
committee, breaking isBinReadyForPreset() matches; update the call and
implementation to store the concrete committee: change the invocation
this.writeActiveBinPresetStamp(preset, sourceHash) to pass the loop committee
(e.g. this.writeActiveBinPresetStamp(preset, committee, sourceHash)) and adjust
the writeActiveBinPresetStamp function to accept a committee parameter and
persist that committee into .active-preset.json (instead of using
this.options.committee), so per-committee builds can be recognized for
--skip-if-built/hydrate reuse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 210591a6-0542-4ed8-b3cb-5a16063dcc6d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/zk-prover/Cargo.tomlcrates/zk-prover/src/backend/download.rscrates/zk-prover/src/circuits/aggregation/node_dkg_fold.rscrates/zk-prover/versions.jsonscripts/build-circuits.ts
ctrlc03
approved these changes
Jun 5, 2026
…C5 verify_pk_for_basis verify_pk_for_basis previously reduced both sides of the congruence check independently and compared them (2x ModU64::reduce_mod + assert == 0). It is equivalent and cheaper to check the difference is divisible by q_l using a single ModU64::assert_zero_mod call. A compile-time offset (H * q_l) is added before the subtraction to guarantee the delta is a small non-negative integer in the Field (not a field-wrapped negative), which is required by __reduce_witness_u64. Bound: delta + H*q_l in [(H-1)*q_l/2, (H+2)*q_l/2); max quotient <= H+2, safely fits in u64 for realistic committee sizes. ACIR opcode count (insecure-512, H=3): 43,830 -> 7,990 (-82%). Expected ~24% reduction in Barretenberg constraint count for secure config (N=8192, L=3) where 2x L*N = 49,152 reduce_mod calls are replaced by L*N = 24,576 assert_zero_mod calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pk1 = CRS a is a public constant shared by all parties. Including it as a per-party committed witness in C5 served only as a chain check back to C1's commitment, but that check is now unnecessary because: 1. C1 still verifies pk1 = CRS internally (circuit constraint unchanged). 2. C5 now passes the circuit-constant CRP directly as pk1_agg, giving the same binding guarantee without any per-party witness. Changes: - compute_threshold_pk_commitment: remove pk1 parameter; hash pk0 only. Domain separator DS_PK_GENERATION is unchanged; the io_pattern encodes input length so old (pk0+pk1) and new (pk0-only) hashes cannot collide. - C1 (pk_generation): update the one call site accordingly. - C5 lib (pk_aggregation): remove pk1/pk1_agg fields; add crp field; remove verify_pk1; pass crp to compute_pk_aggregation_commitment so the user-facing final commitment still covers both pk0_agg and pk1=CRP. - C5 bin (main.nr): remove pk1/pk1_agg circuit inputs; import and pass CRP. ACIR opcodes (insecure-512, H=3): 7,990 -> 4,002 (-50%). Combined with the assert_zero_mod commit: 43,830 -> 4,002 (-91% total). Witness reduction at H=10: removes (H+1)*L*N = 11*3*8192 = 270,336 field elements from the prover witness. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
0xjei
force-pushed
the
opt/c5-assert-zero-mod
branch
from
1f8bf14 to
2736b72
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
C5 pk_aggregation constraint reduction (~40–45% estimated)
Two optimizations to the threshold public key aggregation circuit (C5):
assert_zero_mod in verify_pk_for_basis
Replace the two ModU64::reduce_mod calls + equality assert per coefficient position with a single assert_zero_mod on the difference. A compile-time offset (H × q_l) keeps the delta non-negative in the field. Saves ~37 Barretenberg gates per position across L × N = 24,576 positions.
Remove per-party pk1 witness; pin CRP directly
pk1 = a (CRS) is a public constant — including it as a per-party committed witness only served as an indirect chain check back to C1. Instead:
compute_threshold_pk_commitment now hashes pk0 only (domain separator unchanged; input length is encoded in the SAFE io_pattern so old/new hashes can't collide).
C1 still verifies pk1 = CRS internally — the invariant is unchanged.
C5 passes the circuit-constant CRP directly to compute_pk_aggregation_commitment, so the user-facing output commitment still covers both pk0_agg and pk1 = a.
At H=10 this removes (H+1) × L × N = 270,336 field elements from the prover witness and eliminates the verify_pk1 loop.
Summary by CodeRabbit
New Features
Chores