feat: add C4-C6 and C1-C2 links [skip-line-limit]#1511
Merged
Conversation
C4 sums H parties' share coefficients (each in [0, q_l)), producing values in [0, H*q_l). C6's FHE library delivers sk already reduced to [0, q_l), then applies reverse + center before hashing. normalize_aggregated now: 1. Reduces each coefficient mod q_l (subtracts q up to H-1 times) 2. Reverses coefficient order (high-degree first) 3. Centers into [-(q_l-1)/2, (q_l-1)/2] The H generic parameter is threaded through execute() and the call-site in main.nr passes QIS_THRESHOLD as the moduli array. Rust-side e2e tests updated with normalize_crt_for_commitment so their expected_c4 recomputation mirrors the circuit's normalisation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR updates DKG circuit commitment consistency checking and normalization pipelines. It adds C1→C2 commitment links, re-enables C4→C6 links, updates secret-verification routines to reverse and center coefficients before hashing, modifies share decryption to normalize aggregated polynomials via coefficient reduction and reordering, and adjusts supporting helper computations and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant C1 as C1<br/>(PkGeneration)
participant LinkC1C2 as C1→C2<br/>(CommitmentLink)
participant C2a as C2a<br/>(ShareComputation)
participant C2b as C2b<br/>(ShareComputation)
participant LinkC4C6 as C4→C6<br/>(CommitmentLink)
participant C4 as C4<br/>(ShareDecryption)
participant C6 as C6<br/>(ZK Verify)
C1->>C1: Generate sk_commitment,<br/>e_sm_commitment
C1-->>LinkC1C2: Export commitments<br/>in public signals
LinkC1C2->>C2a: Verify C1 sk_commitment<br/>matches expected input
LinkC1C2->>C2b: Verify C1 e_sm_commitment<br/>matches expected input
C2a->>C2a: Verify secret commitment<br/>(reverse + hash)
C2b->>C2b: Verify secret commitment<br/>(reverse + center + hash)
C2a-->>C4: Aggregated shares<br/>(via C2 commitments)
C2b-->>C4: Aggregated smudging<br/>(via C2 commitments)
C4->>C4: Normalize aggregated<br/>(reduce mod + reverse + center)
C4->>C4: Hash normalized polynomial
C4-->>LinkC4C6: Export normalized<br/>commitments
LinkC4C6->>C6: Verify C4 normalized<br/>commitments match C6 witness
C6->>C6: Verify ZK proof<br/>with matched commitments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
This was
linked to
issues
Apr 7, 2026
Closed
![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: 1
🧹 Nitpick comments (1)
crates/zk-prover/src/actors/commitment_links/c1_to_c2.rs (1)
55-108: Optional: reduce duplication between C2a and C2b link implementations.
extract_source_valuesandcheck_signalsare structurally identical apart from the source field name. A tiny shared helper (or small generic constructor pattern) would reduce maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/actors/commitment_links/c1_to_c2.rs` around lines 55 - 108, Both C1ToC2a/C1ToC2b implementations duplicate extract_source_values and check_signals logic; factor the common logic into shared helpers and call them from each link. Create a helper like fn extract_field_value_from_pk(field_name: &str, public_signals: &[u8]) -> Option<FieldValue> that uses CircuitName::PkGeneration.output_layout().extract_field(...) and copies into a [u8; FIELD_BYTE_LEN], and a helper fn target_matches_field(value: &FieldValue, target_public_signals: &[u8]) -> bool that checks length and byte equality; then update C1ToC2bESmCommitmentLink::extract_source_values and ::check_signals (and the C2a link) to call these helpers instead of duplicating code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md`:
- Around line 464-467: Update the descriptive text for ShareVerificationActor to
reflect the correct number of phases: replace the phrase "same 2-phase
verification" with "3-phase verification" so it matches the subsequent Phase
1/Phase 2/Phase 3 list; locate the occurrence near the ShareVerificationActor
section in 04_DKG_AND_COMPUTATION.md and update that wording only.
---
Nitpick comments:
In `@crates/zk-prover/src/actors/commitment_links/c1_to_c2.rs`:
- Around line 55-108: Both C1ToC2a/C1ToC2b implementations duplicate
extract_source_values and check_signals logic; factor the common logic into
shared helpers and call them from each link. Create a helper like fn
extract_field_value_from_pk(field_name: &str, public_signals: &[u8]) ->
Option<FieldValue> that uses
CircuitName::PkGeneration.output_layout().extract_field(...) and copies into a
[u8; FIELD_BYTE_LEN], and a helper fn target_matches_field(value: &FieldValue,
target_public_signals: &[u8]) -> bool that checks length and byte equality; then
update C1ToC2bESmCommitmentLink::extract_source_values and ::check_signals (and
the C2a link) to call these helpers instead of duplicating code.
🪄 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: 6e3b2869-2fd7-426a-95d3-588131fbdb22
📒 Files selected for processing (10)
agent/flow-trace/04_DKG_AND_COMPUTATION.mdcircuits/bin/dkg/share_decryption/src/main.nrcircuits/lib/src/configs/insecure/dkg.nrcircuits/lib/src/core/dkg/share_computation.nrcircuits/lib/src/core/dkg/share_decryption.nrcrates/zk-helpers/src/circuits/dkg/share_computation/computation.rscrates/zk-helpers/src/circuits/dkg/share_decryption/computation.rscrates/zk-prover/src/actors/commitment_links/c1_to_c2.rscrates/zk-prover/src/actors/commitment_links/mod.rscrates/zk-prover/tests/local_e2e_tests.rs
- Replace iterative mod reduction with ModU128::new(q).reduce_mod(c) - Drop unused H generic from normalize_aggregated; update docs Made-with: Cursor
cedoor
approved these changes
Apr 7, 2026
ctrlc03
approved these changes
Apr 7, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
circuits/lib/src/core/dkg/share_decryption.nr (2)
80-82: Make the no-wrap assumption explicit.Line 80 depends on the unreduced coefficient sum staying below the backend field modulus until
normalize_aggregatedruns. BecauseShareDecryptionis generic overHand the supplied moduli, a future preset bump could silently change this from integer-sum-then-reduce into field-wrap-then-reduce. I’d pin that as an explicit preset-level precondition or test.Also applies to: 92-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/core/dkg/share_decryption.nr` around lines 80 - 82, The comment warns that ShareDecryption’s assumption that the unreduced sum H*q_l < backend field modulus is implicit; make this explicit by adding a preset-level precondition (or invariant) that pins H and q_l versus the circuit/backend field modulus and document it where ShareDecryption is defined, and add a unit/integration test that asserts H * q_l < FIELD_MODULUS for all presets; also update the comment near normalize_aggregated to state the no-wrap requirement explicitly and reference the invariant so future preset bumps will fail the test if they violate the assumption.
123-124: Pick one canonical normalization order.Line 123 documents
reduce -> reverse -> center, while Lines 153-155 implementreverse -> reduce -> center, andcrates/zk-prover/tests/local_e2e_tests.rs:101-116uses the first order. The output is the same, but keeping multiple “canonical” spellings around the C4→C6 link makes future audits harder. I’d either reorder Lines 154-155 to match the Rust helper/docs or make the commutativity explicit.♻️ Suggested note
- // Step 3: Normalize to match C6's representation: reduce mod q, reverse, center. + // Step 3: Normalize to match C6's representation. + // Reverse and per-coefficient reduction commute, so this helper uses + // reverse -> reduce -> center while Rust-side helpers may spell it + // reduce -> reverse -> center.Also applies to: 153-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/core/dkg/share_decryption.nr` around lines 123 - 124, Pick one canonical normalization order and make code + tests consistent: choose the order used by normalize_aggregated::<N, L>(aggregated, moduli) (reduce -> reverse -> center) and update the other implementation that currently does reverse -> reduce -> center so it performs reduce then reverse then center, and adjust the local_e2e_tests that assume the first order if needed; ensure any inline docs/comments describing the steps are updated to match the chosen canonical ordering.
🤖 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/dkg/share_decryption.nr`:
- Around line 80-82: The comment warns that ShareDecryption’s assumption that
the unreduced sum H*q_l < backend field modulus is implicit; make this explicit
by adding a preset-level precondition (or invariant) that pins H and q_l versus
the circuit/backend field modulus and document it where ShareDecryption is
defined, and add a unit/integration test that asserts H * q_l < FIELD_MODULUS
for all presets; also update the comment near normalize_aggregated to state the
no-wrap requirement explicitly and reference the invariant so future preset
bumps will fail the test if they violate the assumption.
- Around line 123-124: Pick one canonical normalization order and make code +
tests consistent: choose the order used by normalize_aggregated::<N,
L>(aggregated, moduli) (reduce -> reverse -> center) and update the other
implementation that currently does reverse -> reduce -> center so it performs
reduce then reverse then center, and adjust the local_e2e_tests that assume the
first order if needed; ensure any inline docs/comments describing the steps are
updated to match the chosen canonical ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9f0add3-d4cd-45f7-af85-c65e4316519b
📒 Files selected for processing (1)
circuits/lib/src/core/dkg/share_decryption.nr
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.
Summary by CodeRabbit
Release Notes
Documentation
New Features
Refactor