feat: link C2 to C3/C4 [skip-line-limit]#1510
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughConsolidates DKG C2 into two inner recursive circuits (C2a: SK, C2b: ESM) plus an optional recursive wrapper ( Changes
Sequence Diagram(s)sequenceDiagram
participant Prover as ZK Prover
participant Inner as Inner C2 Proof (C2a/C2b)
participant Wrapper as Wrapper Circuit (ShareComputation)
participant Verifier as Verifier/Actor
Prover->>Inner: Generate inner proof (witness, configs)
Inner->>Inner: Verify commitment, consistency, range, parity
Inner-->>Prover: Inner proof (signed inner payload)
alt Wrapper aggregation enabled
Prover->>Wrapper: generate_wrapper_proof(inner proof)
Wrapper->>Wrapper: Re-verify inner proof, compress public inputs
Wrapper-->>Prover: Wrapper proof (ShareComputation)
Prover->>Verifier: Emit DKGInnerProofReady (inner + wrapper)
else No wrapper
Prover->>Verifier: Emit DKGInnerProofReady (inner only)
end
Verifier->>Verifier: Select verification path
alt proof.circuit == ShareComputation (wrapper)
Verifier->>Verifier: verify_wrapper_proof (wrapper VK)
else inner circuit
Verifier->>Verifier: bb verify with inner VK
end
Verifier-->>Verifier: Validation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/dkg/share_decryption/computation.rs (1)
330-344:⚠️ Potential issue | 🔴 CriticalReverse coefficients in test to match implementation's commitment computation.
The
Inputs::computemethod reverses coefficients before computingexpected_commitments(lines 191-198), but this test computesdirect_commitmentusing non-reversedshare_coeffsat line 333. The assertion at line 339 will fail because the two commitment values are computed with different coefficient orderings.Proposed fix
for (party_idx, party_cts) in sample.honest_ciphertexts.iter().enumerate() { for mod_idx in 0..threshold_l { let decrypted_pt = sample.secret_key.try_decrypt(&party_cts[mod_idx]).unwrap(); - let share_coeffs = decrypted_pt.value.deref().to_vec(); + let mut share_coeffs = decrypted_pt.value.deref().to_vec(); + share_coeffs.reverse(); let direct_commitment = compute_share_encryption_commitment_from_message( &Polynomial::from_u64_vector(share_coeffs), msg_bit,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs` around lines 330 - 344, The test computes direct_commitment from share_coeffs in sample.honest_ciphertexts but Inputs::compute reverses coefficients before computing expected_commitments; to fix, reverse the coefficient order the same way as Inputs::compute before creating the Polynomial used in compute_share_encryption_commitment_from_message (i.e., take share_coeffs.deref().to_vec(), reverse it, then call Polynomial::from_u64_vector on the reversed vector) so the commitment comparison in the loop over sample.honest_ciphertexts matches the implementation.
🧹 Nitpick comments (6)
crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs (1)
191-196: Comment references C3 but should reference C2 for consistency with Noir circuit.The Noir circuit comment (lines 55-56 in
share_decryption.nr) correctly states this reversal matches "C2's commit_to_party_shares", but this comment references C3. Since C4 (share_decryption) consumes commitments from C2 per the PR description, the comment should be consistent.📝 Suggested comment fix
- // Reverse to match C3's message witness, which is constructed as - // `pt.value.reversed()` before committing (share_encryption/computation.rs). + // Reverse to match C2's commit_to_party_shares, which stores coefficients + // highest-degree-first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs` around lines 191 - 196, The inline comment that says "Reverse to match C3's message witness..." is incorrect; update it to reference C2 to match the Noir circuit and PR description. Edit the comment immediately above the reversed_coeffs logic and the compute_share_encryption_commitment_from_message call so it reads that the reversal matches C2's commit_to_party_shares (and optionally mention share_decryption.nr lines 55-56), ensuring the comment consistently references C2 instead of C3.circuits/lib/src/core/dkg/share_computation.nr (1)
285-297: Add a regression test for the share-commitment serialization contract.The new linkage relies on two non-obvious invariants here: outputs are serialized as
[party][modulus], and each share polynomial is reversed before committing. A focused cross-circuit test against the C3/C4 commitment path would make this much safer to refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/core/dkg/share_computation.nr` around lines 285 - 297, Add a regression test that asserts the share-commitment serialization contract: construct shares the same way as in the loop (build share_coeffs by reversing y[coeff_idx][mod_idx][party_idx + 1], create Polynomial::new(share_coeffs), and call compute_share_encryption_commitment_from_message::<N, BIT_SHARE>) and verify the commitments matrix is serialized as [party][modulus]; compare those commitments byte-for-byte against the C3/C4 commitment path that builds its `message` witness via `pt.value.reversed()` to ensure both the reversal and the [party][modulus] ordering match exactly.crates/zk-helpers/src/bin/zk_cli.rs (1)
265-271: Redundant condition inshow_input_type.Line 269 checks
circuit_name == ShareComputationCircuit::NAMEbut this is already covered byrequires_inputs_arg(set on line 265). The OR clause is always false whenrequires_inputs_argis true forShareComputationCircuit.Similarly, line 271 duplicates the same check that's already part of
requires_inputs_arg.Simplify the redundant conditions
- let show_input_type = requires_inputs_arg || circuit_name == ShareComputationCircuit::NAME; + let show_input_type = requires_inputs_arg; - let dkg_input_type = if circuit_name == ShareComputationCircuit::NAME || requires_inputs_arg { + let dkg_input_type = if requires_inputs_arg {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/bin/zk_cli.rs` around lines 265 - 271, The conditions are redundant: remove the duplicate checks against ShareComputationCircuit::NAME because requires_inputs_arg already covers them; update the computation of show_input_type to simply use requires_inputs_arg, and update the dkg_input_type condition to use only requires_inputs_arg (and any other distinct checks that remain) so that circuit_name == ShareComputationCircuit::NAME is not repeated; adjust the expressions referencing requires_inputs_arg, show_input_type, and dkg_input_type accordingly.crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (1)
43-49: Minor: Explicit closure in error mapping is equivalent but more verbose.The change from
map_err(CircuitsErrors::SerdeJson)tomap_err(|e| CircuitsErrors::SerdeJson(e))is functionally equivalent sinceCircuitsErrors::SerdeJsonis a tuple variant constructor. Both approaches work, but the original was slightly more concise.♻️ Optional: Revert to concise form
pub fn generate_toml(witness: &Inputs) -> Result<CodegenToml, CircuitsErrors> { let json = witness .to_json() - .map_err(|e| CircuitsErrors::SerdeJson(e))?; + .map_err(CircuitsErrors::SerdeJson)?; Ok(toml::to_string(&json)?) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs` around lines 43 - 49, In generate_toml, revert the verbose closure used in error mapping back to the concise constructor form: replace map_err(|e| CircuitsErrors::SerdeJson(e)) with map_err(CircuitsErrors::SerdeJson) so the CircuitsErrors::SerdeJson tuple-variant is passed directly; this keeps behavior identical but restores the shorter, idiomatic form.crates/zk-prover/tests/local_e2e_tests.rs (2)
565-594: Test name may be misleading after simplification.The test
test_share_computation_sk_commitment_consistencynow only validates that:
- The proof generates successfully
- The circuit tag is correct
- Public signals are non-empty and not all zeros
It no longer actually verifies "commitment consistency" against an independently computed expected value (as other commitment consistency tests in this file do, e.g.,
test_pk_generation_commitment_consistency).Consider either renaming the test to reflect its current scope (e.g.,
test_share_computation_sk_proof_validity) or adding assertions that compare the public signals against expected commitment values fromShareComputationCircuit::compute().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/tests/local_e2e_tests.rs` around lines 565 - 594, The test function test_share_computation_sk_commitment_consistency no longer checks commitment consistency — either rename it to reflect its current checks (e.g., change the function name to test_share_computation_sk_proof_validity and update any references) or restore the missing assertions by computing the expected commitment via ShareComputationCircuit::compute() and comparing it to the produced output: convert proof.public_signals with public_signals_to_fields(&proof.public_signals) and assert equality/consistency against the computed expected fields from ShareComputationCircuit::compute() (or the appropriate method that returns expected commitments), keeping the existing checks (circuit tag, non-empty/non-zero public signals) intact.
206-207: Consider compiling only the required circuit for this test.Both
sk_share_computationande_sm_share_computationcircuits are compiled here, but since the sample usesDkgInputType::SecretKey, only the SK circuit should be needed for proof generation. The same applies tosetup_share_computation_e_sm_testat lines 239-240.If both are required by the
ShareComputationCircuitimplementation, a brief comment explaining why would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/tests/local_e2e_tests.rs` around lines 206 - 207, The test currently compiles both circuits via setup_compiled_circuit(&backend, "dkg", "sk_share_computation") and setup_compiled_circuit(&backend, "dkg", "e_sm_share_computation") even though the test uses DkgInputType::SecretKey; either remove the unnecessary e_sm compilation (leave only the SK compile) or, if ShareComputationCircuit requires both variants, add a brief clarifying comment in the test explaining why both sk_share_computation and e_sm_share_computation are needed for proof generation; update the analogous setup in setup_share_computation_e_sm_test the same way.
🤖 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/bin/recursive_aggregation/wrapper/dkg/share_computation/src/main.nr`:
- Around line 13-17: The comment describing public outputs is stale: update the
comment above the N_PUBLIC_INPUTS declaration to reflect that N_PUBLIC_INPUTS =
(L_THRESHOLD * N_PARTIES) + 1 represents one public batch_key_hash parameter
plus L_THRESHOLD per-party share commitments for each of the N_PARTIES (i.e.,
total commitments = L_THRESHOLD * N_PARTIES, plus the single batch_key_hash),
and remove the obsolete "3 public outputs" / "(key_hash, commitment) return
tuple" wording; reference N_PUBLIC_INPUTS, L_THRESHOLD, and N_PARTIES in the new
comment.
In `@crates/zk-helpers/src/circuits/output_layout.rs`:
- Around line 32-34: The doc comment for CircuitOutputLayout::Dynamic
incorrectly describes the dynamic C2 return shape as `[[Field; L]; N]`; update
the text to reflect that inner circuits now return one commitment vector per
party, i.e. `[[Field; L]; N_PARTIES]`, so the doc comment and signal-layout
contract reference `N_PARTIES` (or the appropriate party-count identifier used
in the file) instead of `N`.
---
Outside diff comments:
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs`:
- Around line 330-344: The test computes direct_commitment from share_coeffs in
sample.honest_ciphertexts but Inputs::compute reverses coefficients before
computing expected_commitments; to fix, reverse the coefficient order the same
way as Inputs::compute before creating the Polynomial used in
compute_share_encryption_commitment_from_message (i.e., take
share_coeffs.deref().to_vec(), reverse it, then call Polynomial::from_u64_vector
on the reversed vector) so the commitment comparison in the loop over
sample.honest_ciphertexts matches the implementation.
---
Nitpick comments:
In `@circuits/lib/src/core/dkg/share_computation.nr`:
- Around line 285-297: Add a regression test that asserts the share-commitment
serialization contract: construct shares the same way as in the loop (build
share_coeffs by reversing y[coeff_idx][mod_idx][party_idx + 1], create
Polynomial::new(share_coeffs), and call
compute_share_encryption_commitment_from_message::<N, BIT_SHARE>) and verify the
commitments matrix is serialized as [party][modulus]; compare those commitments
byte-for-byte against the C3/C4 commitment path that builds its `message`
witness via `pt.value.reversed()` to ensure both the reversal and the
[party][modulus] ordering match exactly.
In `@crates/zk-helpers/src/bin/zk_cli.rs`:
- Around line 265-271: The conditions are redundant: remove the duplicate checks
against ShareComputationCircuit::NAME because requires_inputs_arg already covers
them; update the computation of show_input_type to simply use
requires_inputs_arg, and update the dkg_input_type condition to use only
requires_inputs_arg (and any other distinct checks that remain) so that
circuit_name == ShareComputationCircuit::NAME is not repeated; adjust the
expressions referencing requires_inputs_arg, show_input_type, and dkg_input_type
accordingly.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs`:
- Around line 43-49: In generate_toml, revert the verbose closure used in error
mapping back to the concise constructor form: replace map_err(|e|
CircuitsErrors::SerdeJson(e)) with map_err(CircuitsErrors::SerdeJson) so the
CircuitsErrors::SerdeJson tuple-variant is passed directly; this keeps behavior
identical but restores the shorter, idiomatic form.
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs`:
- Around line 191-196: The inline comment that says "Reverse to match C3's
message witness..." is incorrect; update it to reference C2 to match the Noir
circuit and PR description. Edit the comment immediately above the
reversed_coeffs logic and the compute_share_encryption_commitment_from_message
call so it reads that the reversal matches C2's commit_to_party_shares (and
optionally mention share_decryption.nr lines 55-56), ensuring the comment
consistently references C2 instead of C3.
In `@crates/zk-prover/tests/local_e2e_tests.rs`:
- Around line 565-594: The test function
test_share_computation_sk_commitment_consistency no longer checks commitment
consistency — either rename it to reflect its current checks (e.g., change the
function name to test_share_computation_sk_proof_validity and update any
references) or restore the missing assertions by computing the expected
commitment via ShareComputationCircuit::compute() and comparing it to the
produced output: convert proof.public_signals with
public_signals_to_fields(&proof.public_signals) and assert equality/consistency
against the computed expected fields from ShareComputationCircuit::compute() (or
the appropriate method that returns expected commitments), keeping the existing
checks (circuit tag, non-empty/non-zero public signals) intact.
- Around line 206-207: The test currently compiles both circuits via
setup_compiled_circuit(&backend, "dkg", "sk_share_computation") and
setup_compiled_circuit(&backend, "dkg", "e_sm_share_computation") even though
the test uses DkgInputType::SecretKey; either remove the unnecessary e_sm
compilation (leave only the SK compile) or, if ShareComputationCircuit requires
both variants, add a brief clarifying comment in the test explaining why both
sk_share_computation and e_sm_share_computation are needed for proof generation;
update the analogous setup in setup_share_computation_e_sm_test the same way.
🪄 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: d689e9da-5399-4fc2-875c-bb3e27f10086
📒 Files selected for processing (64)
agent/flow-trace/04_DKG_AND_COMPUTATION.mdcircuits/README.mdcircuits/bin/dkg/Nargo.tomlcircuits/bin/dkg/e_sm_share_computation/Nargo.tomlcircuits/bin/dkg/e_sm_share_computation/README.mdcircuits/bin/dkg/e_sm_share_computation/src/main.nrcircuits/bin/dkg/e_sm_share_computation_base/Nargo.tomlcircuits/bin/dkg/e_sm_share_computation_base/README.mdcircuits/bin/dkg/e_sm_share_computation_base/src/main.nrcircuits/bin/dkg/share_computation/README.mdcircuits/bin/dkg/share_computation/src/main.nrcircuits/bin/dkg/share_computation_chunk/Nargo.tomlcircuits/bin/dkg/share_computation_chunk/README.mdcircuits/bin/dkg/share_computation_chunk/src/main.nrcircuits/bin/dkg/share_computation_chunk_batch/README.mdcircuits/bin/dkg/share_computation_chunk_batch/src/main.nrcircuits/bin/dkg/sk_share_computation/Nargo.tomlcircuits/bin/dkg/sk_share_computation/README.mdcircuits/bin/dkg/sk_share_computation/src/main.nrcircuits/bin/dkg/sk_share_computation_base/Nargo.tomlcircuits/bin/dkg/sk_share_computation_base/README.mdcircuits/bin/dkg/sk_share_computation_base/src/main.nrcircuits/bin/recursive_aggregation/wrapper/README.mdcircuits/bin/recursive_aggregation/wrapper/dkg/share_computation/src/main.nrcircuits/lib/src/README.mdcircuits/lib/src/configs/insecure/dkg.nrcircuits/lib/src/configs/secure/dkg.nrcircuits/lib/src/core/dkg/share_computation.nrcircuits/lib/src/core/dkg/share_computation/base.nrcircuits/lib/src/core/dkg/share_computation/chunk.nrcircuits/lib/src/core/dkg/share_computation/mod.nrcircuits/lib/src/core/dkg/share_decryption.nrcrates/events/src/enclave_event/proof.rscrates/events/src/enclave_event/signed_proof.rscrates/multithread/src/multithread.rscrates/test-helpers/src/lib.rscrates/tests/tests/integration.rscrates/zk-helpers/README.mdcrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/circuits/commitments.rscrates/zk-helpers/src/circuits/dkg/share_computation/circuit.rscrates/zk-helpers/src/circuits/dkg/share_computation/codegen.rscrates/zk-helpers/src/circuits/dkg/share_computation/computation.rscrates/zk-helpers/src/circuits/dkg/share_computation/mod.rscrates/zk-helpers/src/circuits/dkg/share_computation/utils.rscrates/zk-helpers/src/circuits/dkg/share_decryption/computation.rscrates/zk-helpers/src/circuits/output_layout.rscrates/zk-prover/src/actors/commitment_consistency_checker.rscrates/zk-prover/src/actors/commitment_links/c0_to_c3.rscrates/zk-prover/src/actors/commitment_links/c1_to_c5.rscrates/zk-prover/src/actors/commitment_links/c2_to_c3.rscrates/zk-prover/src/actors/commitment_links/c2_to_c4.rscrates/zk-prover/src/actors/commitment_links/c4a_to_c6.rscrates/zk-prover/src/actors/commitment_links/c4b_to_c6.rscrates/zk-prover/src/actors/commitment_links/c6_to_c7.rscrates/zk-prover/src/actors/commitment_links/mod.rscrates/zk-prover/src/circuits/dkg/share_computation.rscrates/zk-prover/src/circuits/recursive_aggregation/mod.rscrates/zk-prover/src/circuits/utils.rscrates/zk-prover/src/lib.rscrates/zk-prover/src/prover.rscrates/zk-prover/src/witness.rscrates/zk-prover/tests/local_e2e_tests.rsdocs/pages/cryptography.mdx
💤 Files with no reviewable changes (20)
- circuits/bin/dkg/share_computation_chunk_batch/README.md
- circuits/bin/dkg/sk_share_computation_base/Nargo.toml
- circuits/bin/dkg/share_computation_chunk/README.md
- circuits/bin/dkg/sk_share_computation_base/README.md
- circuits/bin/dkg/share_computation_chunk/Nargo.toml
- circuits/bin/dkg/e_sm_share_computation_base/README.md
- circuits/bin/dkg/share_computation/README.md
- circuits/bin/dkg/e_sm_share_computation_base/Nargo.toml
- circuits/bin/dkg/share_computation_chunk/src/main.nr
- crates/zk-prover/src/lib.rs
- crates/zk-prover/src/circuits/utils.rs
- circuits/bin/dkg/e_sm_share_computation_base/src/main.nr
- crates/zk-helpers/src/circuits/commitments.rs
- circuits/bin/dkg/sk_share_computation_base/src/main.nr
- circuits/lib/src/core/dkg/share_computation/chunk.nr
- circuits/bin/dkg/share_computation/src/main.nr
- circuits/bin/dkg/share_computation_chunk_batch/src/main.nr
- crates/zk-helpers/src/circuits/dkg/share_computation/circuit.rs
- circuits/lib/src/core/dkg/share_computation/mod.nr
- circuits/lib/src/core/dkg/share_computation/base.nr
cedoor
reviewed
Apr 7, 2026
cedoor
reviewed
Apr 7, 2026
Contributor
Author
|
#1510 (comment) solved via chat |
cedoor
approved these changes
Apr 7, 2026
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
2 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.
this PR aims to complete the link of C2, C3 and C4 DKG circuits. There are a few things worth mentioning about the changes that have been conducted in this PR
base+chunk+batch) with the previous, monholithic circuit. We decided to do this since we are going to run with secure parameters with no more than 20 nodes first.expected_message_commitmentand C4 consumes asexpected_commitments. This closes the gap where a node could have used different share values across these circuits without detection.happy to jump into details for any further review!
Summary by CodeRabbit
Refactor
Bug Fixes / Reliability
Documentation
Tools / CLI