feat: split C2 with wrapper [skip-line-limit]#1400
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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:
📝 WalkthroughWalkthroughRefactors DKG share-computation into base and chunk circuits with chunking configs and commitments, removes monolithic circuits, adds inner→outer wrapper proof flow, updates workspace manifests and Noir binaries, and propagates changes through zk-helpers, zk-prover, events, multithread, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant BaseGen as "Base Generator"
participant ChunkGen as "Chunk Generator"
participant InnerWrap as "Inner Wrapper"
participant OuterWrap as "Outer Wrapper"
participant Prover
CLI->>BaseGen: prepare base inputs
BaseGen->>Prover: prove base circuit
Prover-->>BaseGen: base_proof
CLI->>ChunkGen: prepare y chunks
ChunkGen->>Prover: prove chunk circuits (per chunk)
Prover-->>ChunkGen: chunk_proofs[]
CLI->>InnerWrap: call generate_share_computation_wrapper_proof(base_proof, chunk_proofs)
InnerWrap->>InnerWrap: verify base & chunk proofs, extract commitments
InnerWrap->>Prover: prove inner wrapper
Prover-->>InnerWrap: inner_proof
InnerWrap->>OuterWrap: assemble outer input (inner_proof)
OuterWrap->>Prover: prove outer wrapper
Prover-->>OuterWrap: final_wrapper_proof
OuterWrap-->>CLI: deliver final wrapper proof
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
📝 Coding Plan
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 |
Closed
0xjei
force-pushed
the
circuits/split-c2-wrapper
branch
from
eca4610 to
fea0e9c
Compare
0xjei
force-pushed
the
circuits/split-c2-wrapper
branch
from
1e8c991 to
c1ce9c9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/enclave-contracts/test/Slashing/SlashingManager.spec.ts (1)
62-65: Optional: Remove unused_mockVerifierparameter fromsetupPolicies.The
_mockVerifierparameter is declared but never referenced within the function body. TheproofVerifierfield is always set toethers.ZeroAddressin all three policies. If this parameter was intended for future use, consider removing it until needed to reduce confusion.♻️ Suggested cleanup
async function setupPolicies( slashingManager: SlashingManager, - _mockVerifier?: MockCircuitVerifier, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/Slashing/SlashingManager.spec.ts` around lines 62 - 65, The _mockVerifier parameter in the setupPolicies function is unused; remove the unused parameter declaration (_mockVerifier?: MockCircuitVerifier) from the setupPolicies signature and update any calls to setupPolicies accordingly, keeping the function body as-is (it already sets proofVerifier to ethers.ZeroAddress for each policy); if you intended to use a mock verifier later, leave a TODO comment instead of an unused parameter to avoid confusion.circuits/bin/dkg/share_computation_chunk_batch/Nargo.toml (1)
8-8: Consider pinningbb_proof_verificationwith commit SHA for stronger supply-chain guarantees.While tag-based pinning is the established convention across Nargo.toml files in this repository, using
revwith the commit SHA provides immutability guarantees that tags cannot provide. This aligns with your stated preference for reproducible builds and is particularly valuable for external dependencies.The current
tag = "v3.0.0-nightly.20260102"pinning is functional and consistent with other dependencies in the codebase; this is an optional improvement rather than a breaking concern.Suggested change
-bb_proof_verification = { git = "https://github.com/AztecProtocol/aztec-packages/", tag = "v3.0.0-nightly.20260102", directory = "barretenberg/noir/bb_proof_verification" } +bb_proof_verification = { git = "https://github.com/AztecProtocol/aztec-packages/", rev = "<commit_sha_for_v3.0.0-nightly.20260102>", directory = "barretenberg/noir/bb_proof_verification" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/bin/dkg/share_computation_chunk_batch/Nargo.toml` at line 8, Update the bb_proof_verification dependency entry in Nargo.toml to pin it by commit SHA instead of the tag; locate the bb_proof_verification line and replace the tag = "v3.0.0-nightly.20260102" attribute with rev = "<commit-sha>" (using the precise commit SHA for that release) so the dependency is immutable and reproducible while keeping the same git URL and directory values.circuits/bin/dkg/share_computation/Nargo.toml (2)
4-4: Replace placeholder author metadata.
authors = [""]is empty metadata; use a real value (or remove the field if your tooling permits).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/bin/dkg/share_computation/Nargo.toml` at line 4, The authors metadata in Nargo.toml currently contains an empty placeholder (`authors = [""]`); update the authors field to include a real author name or organization (e.g., replace the empty string with a valid identifier) or remove the authors line entirely if your build/tooling supports omitting it; locate the `authors` entry in Nargo.toml and replace or delete it accordingly to provide valid metadata.
8-8: Replace tag with the immutable commit hash of v3.0.0-nightly.20260102.Named tags like
v3.0.0-nightly.20260102can be retagged or moved, making them less tamper-resistant. Nargo supports any string in thetagfield, including commit hashes. Replace the tag with the full commit SHA of that release for immutable pinning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/bin/dkg/share_computation/Nargo.toml` at line 8, The dependency entry bb_proof_verification uses a mutable tag string; replace the tag value ("v3.0.0-nightly.20260102") with the full immutable commit SHA for that release so the git reference is pinned; update the bb_proof_verification entry in Nargo.toml to set tag = "<full-commit-sha>" (the 40-char SHA) ensuring the git, directory, and other keys remain unchanged.crates/zk-helpers/src/circuits/dkg/share_computation/circuit.rs (1)
17-17: Track TODO debt with an issue reference before merge.These TODOs are understandable, but adding an issue/ID in the comments will make follow-up explicit and prevent drift.
Also applies to: 49-50
🤖 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/circuit.rs` at line 17, The inline TODO comments ("todo: remove this (keep it until we update zk-prover)" and the other TODOs around lines 49-50) must be annotated with a tracking issue reference so the technical debt is explicit; update those TODO comments to include a short issue ID or URL (e.g. "TODO: remove this — tracked in ISSUE-XXXX / https://...") and ensure all matching TODO occurrences in this module/circuit (the comments containing "todo: remove this" and the ones at 49-50) are updated consistently with the same issue identifier.crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs (1)
32-33: Centralize chunk geometry and forbid short last chunks.
SHARE_COMPUTATION_CHUNK_SIZE,compute_chunks_per_batch(), and the ceil-basedcompute_n_chunks()are helper-local copies of values that already live incircuits/lib/src/configs/insecure/dkg.nrandcircuits/lib/src/configs/secure/dkg.nr. Combined withsplit_into_chunks()usingmin(...), a future config change will silently produce a shorter finaly_chunk, even though the Noir chunk circuit still expects a fixed-size array (crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs:270-274checks for 512 today). Please source the geometry from one place, or at least match exhaustively on supported presets and assert divisibility/pad explicitly.Also applies to: 153-172, 317-337
🤖 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/computation.rs` around lines 32 - 33, The hard-coded SHARE_COMPUTATION_CHUNK_SIZE and local helpers compute_chunks_per_batch() and compute_n_chunks() can diverge from the Noir circuit presets and allow a short final y_chunk; replace these local copies by sourcing the chunk geometry from the central config (the same presets in circuits/lib/src/configs/{insecure,secure}/dkg.nr) or exhaustively match the supported presets inside crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs, and add an explicit assert/validation that total_size % chunk_size == 0 (or pad deterministically) before calling split_into_chunks(); update any callers and ensure the array size expectations checked in crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (the fixed-size array check around lines ~270-274) are satisfied.crates/zk-prover/tests/local_e2e_tests.rs (1)
541-555: These “commitment consistency” tests no longer check commitments.After the wrapper split, both tests only assert a 160-byte public-signal blob and that one field is non-zero. That would still pass if the final wrapper exported the wrong batch tuple or ignored later batches, so it no longer protects the contract implied by the test names. Please compare the final public outputs against the expected values derived from the assembled base/chunk/batch pipeline.
Also applies to: 613-627
🤖 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 541 - 555, The test currently only checks size and non-zeroness of proof.public_signals (converted via public_signals_to_fields), but must assert that the final wrapper's public outputs match the expected batch tuple derived from the assembled base/chunk/batch pipeline; update the test that uses proof.public_signals/public_signals_to_fields (and the similar block at the other location) to compute the expected 5 field values from the assembled pipeline (using the same functions/values used to build base/chunk/batch commitments) and then assert equality field-by-field (e.g., compare expected_fields[i] == fields[i] for all 5 fields) instead of only length and non-zero checks so the final wrapper's exports are verified exactly.crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (1)
112-114: Consider removing unused parameter_n_batches.The
_n_batchesparameter is passed togenerate_configsbut never used, asN_BATCHESis computed in the generated Noir code fromN_CHUNKS / CHUNKS_PER_BATCH. Consider removing this parameter to reduce confusion, or add a consistency assertion if validation is intended.🤖 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 112 - 114, The _n_batches parameter is unused in generate_configs; either remove the _n_batches parameter from generate_configs' signature and all call sites, or keep it but add a consistency check inside generate_configs (e.g., assert that _n_batches == chunk_size / chunks_per_batch) and propagate any mismatch as an error; reference the generate_configs function and the _n_batches symbol and ensure the generated Noir constant N_BATCHES remains computed from N_CHUNKS / CHUNKS_PER_BATCH or is validated against the provided _n_batches for consistency.
🤖 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/dkg/share_computation/src/main.nr`:
- Around line 11-13: The code currently exposes batch_public_inputs[0] (from
share_computation_chunk_batch) which hardcodes batch 0 and drops
aggregated_commitment values from later batches; instead, change the top-level
proof to fold all per-batch aggregated_commitment values into a single final
public output (e.g., hash/commit/aggregate the vector of aggregated_commitment)
and include that folded value as the public C2 output (instead of
batch_public_inputs[0]); additionally, add assertions in the top-level circuit
that shared fields (base_key_hash, chunk_key_hash and any other shared public
slots defined by BATCH_WRAPPER_PUBLIC_INPUTS) are identical across every entry
in batch_public_inputs and that batch_idx values are consistent/ordered as
required before performing the fold so the final public output is bound to the
whole computation.
In `@crates/events/src/enclave_event/signed_proof.rs`:
- Around line 58-59: The current mapping makes ProofType::C2aSkShareComputation
and ProofType::C2bESmShareComputation both map to CircuitName::ShareComputation,
so add an explicit semantic check in the verification path (e.g., in
SignedProof::verify or the verify_proof routine that consumes public signals)
that inspects the public signals' dkg_input_type and asserts it matches the
expected value for each ProofType (SK vs ESm); implement: parse the
dkg_input_type from the public signals, compare against the expected
enum/constant for ProofType::C2aSkShareComputation and
ProofType::C2bESmShareComputation, and return a verification error when they
differ, leaving the CircuitName mapping unchanged.
In `@crates/multithread/src/multithread.rs`:
- Around line 779-786: The loop that calls generate_chunk_batch_proof currently
slices chunk_proofs[start..end] without bounds checks, which can panic; before
creating start/end compute and validate that start < chunk_proofs.len() and end
<= chunk_proofs.len() (or clamp end = usize::min(end, chunk_proofs.len())) and
handle the mismatched case by returning an Err or skipping the batch with a
clear log; use safe indexing (e.g., chunk_proofs.get(start..end)) and propagate
or convert the error from the surrounding function so generate_chunk_batch_proof
never receives an out-of-bounds slice.
---
Nitpick comments:
In `@circuits/bin/dkg/share_computation_chunk_batch/Nargo.toml`:
- Line 8: Update the bb_proof_verification dependency entry in Nargo.toml to pin
it by commit SHA instead of the tag; locate the bb_proof_verification line and
replace the tag = "v3.0.0-nightly.20260102" attribute with rev = "<commit-sha>"
(using the precise commit SHA for that release) so the dependency is immutable
and reproducible while keeping the same git URL and directory values.
In `@circuits/bin/dkg/share_computation/Nargo.toml`:
- Line 4: The authors metadata in Nargo.toml currently contains an empty
placeholder (`authors = [""]`); update the authors field to include a real
author name or organization (e.g., replace the empty string with a valid
identifier) or remove the authors line entirely if your build/tooling supports
omitting it; locate the `authors` entry in Nargo.toml and replace or delete it
accordingly to provide valid metadata.
- Line 8: The dependency entry bb_proof_verification uses a mutable tag string;
replace the tag value ("v3.0.0-nightly.20260102") with the full immutable commit
SHA for that release so the git reference is pinned; update the
bb_proof_verification entry in Nargo.toml to set tag = "<full-commit-sha>" (the
40-char SHA) ensuring the git, directory, and other keys remain unchanged.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/circuit.rs`:
- Line 17: The inline TODO comments ("todo: remove this (keep it until we update
zk-prover)" and the other TODOs around lines 49-50) must be annotated with a
tracking issue reference so the technical debt is explicit; update those TODO
comments to include a short issue ID or URL (e.g. "TODO: remove this — tracked
in ISSUE-XXXX / https://...") and ensure all matching TODO occurrences in this
module/circuit (the comments containing "todo: remove this" and the ones at
49-50) are updated consistently with the same issue identifier.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs`:
- Around line 112-114: The _n_batches parameter is unused in generate_configs;
either remove the _n_batches parameter from generate_configs' signature and all
call sites, or keep it but add a consistency check inside generate_configs
(e.g., assert that _n_batches == chunk_size / chunks_per_batch) and propagate
any mismatch as an error; reference the generate_configs function and the
_n_batches symbol and ensure the generated Noir constant N_BATCHES remains
computed from N_CHUNKS / CHUNKS_PER_BATCH or is validated against the provided
_n_batches for consistency.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs`:
- Around line 32-33: The hard-coded SHARE_COMPUTATION_CHUNK_SIZE and local
helpers compute_chunks_per_batch() and compute_n_chunks() can diverge from the
Noir circuit presets and allow a short final y_chunk; replace these local copies
by sourcing the chunk geometry from the central config (the same presets in
circuits/lib/src/configs/{insecure,secure}/dkg.nr) or exhaustively match the
supported presets inside
crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs, and add an
explicit assert/validation that total_size % chunk_size == 0 (or pad
deterministically) before calling split_into_chunks(); update any callers and
ensure the array size expectations checked in
crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (the fixed-size
array check around lines ~270-274) are satisfied.
In `@crates/zk-prover/tests/local_e2e_tests.rs`:
- Around line 541-555: The test currently only checks size and non-zeroness of
proof.public_signals (converted via public_signals_to_fields), but must assert
that the final wrapper's public outputs match the expected batch tuple derived
from the assembled base/chunk/batch pipeline; update the test that uses
proof.public_signals/public_signals_to_fields (and the similar block at the
other location) to compute the expected 5 field values from the assembled
pipeline (using the same functions/values used to build base/chunk/batch
commitments) and then assert equality field-by-field (e.g., compare
expected_fields[i] == fields[i] for all 5 fields) instead of only length and
non-zero checks so the final wrapper's exports are verified exactly.
In `@packages/enclave-contracts/test/Slashing/SlashingManager.spec.ts`:
- Around line 62-65: The _mockVerifier parameter in the setupPolicies function
is unused; remove the unused parameter declaration (_mockVerifier?:
MockCircuitVerifier) from the setupPolicies signature and update any calls to
setupPolicies accordingly, keeping the function body as-is (it already sets
proofVerifier to ethers.ZeroAddress for each policy); if you intended to use a
mock verifier later, leave a TODO comment instead of an unused parameter to
avoid confusion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5fe538c-129e-4aaf-a725-c7ada5c84e1e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
.github/workflows/ci.ymlcircuits/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/src/main.nrcircuits/bin/dkg/share_computation/Nargo.tomlcircuits/bin/dkg/share_computation/src/main.nrcircuits/bin/dkg/share_computation_chunk/Nargo.tomlcircuits/bin/dkg/share_computation_chunk/src/main.nrcircuits/bin/dkg/share_computation_chunk_batch/Nargo.tomlcircuits/bin/dkg/share_computation_chunk_batch/src/main.nrcircuits/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/src/main.nrcircuits/bin/recursive_aggregation/wrapper/dkg/share_computation/src/main.nrcircuits/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.nrcrates/events/src/enclave_event/proof.rscrates/events/src/enclave_event/signed_proof.rscrates/keyshare/src/threshold_share_collector.rscrates/multithread/src/multithread.rscrates/tests/tests/integration.rscrates/zk-helpers/README.mdcrates/zk-helpers/src/bin/zk_cli.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-prover/Cargo.tomlcrates/zk-prover/src/circuits/dkg/mod.rscrates/zk-prover/src/circuits/dkg/share_computation.rscrates/zk-prover/src/circuits/mod.rscrates/zk-prover/src/circuits/recursive_aggregation/mod.rscrates/zk-prover/src/circuits/recursive_aggregation/utils.rscrates/zk-prover/src/circuits/utils.rscrates/zk-prover/src/circuits/vk.rscrates/zk-prover/src/lib.rscrates/zk-prover/src/prover.rscrates/zk-prover/tests/local_e2e_tests.rspackage.jsonpackages/enclave-contracts/test/Slashing/SlashingManager.spec.ts
💤 Files with no reviewable changes (7)
- circuits/bin/dkg/e_sm_share_computation/README.md
- crates/zk-prover/src/circuits/recursive_aggregation/utils.rs
- circuits/lib/src/core/dkg/share_computation.nr
- circuits/bin/dkg/e_sm_share_computation/src/main.nr
- circuits/bin/dkg/sk_share_computation/README.md
- circuits/bin/dkg/sk_share_computation/src/main.nr
- circuits/bin/dkg/e_sm_share_computation/Nargo.toml
🚧 Files skipped from review as they are similar to previous changes (11)
- circuits/lib/src/core/dkg/share_computation/mod.nr
- crates/tests/tests/integration.rs
- crates/zk-prover/src/circuits/recursive_aggregation/mod.rs
- circuits/bin/dkg/share_computation_chunk/Nargo.toml
- circuits/bin/dkg/Nargo.toml
- circuits/bin/dkg/e_sm_share_computation_base/Nargo.toml
- circuits/bin/dkg/sk_share_computation_base/Nargo.toml
- circuits/lib/src/core/dkg/share_computation/base.nr
- crates/zk-prover/src/prover.rs
- circuits/bin/dkg/sk_share_computation_base/src/main.nr
- circuits/bin/dkg/e_sm_share_computation_base/src/main.nr
ctrlc03
reviewed
Mar 17, 2026
cedoor
suggested changes
Mar 17, 2026
cedoor
reviewed
Mar 17, 2026
ctrlc03
reviewed
Mar 17, 2026
cedoor
reviewed
Mar 17, 2026
cedoor
previously approved these changes
Mar 17, 2026
cedoor
approved these changes
Mar 17, 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
4 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 splits DKG C2 (
share_computation) circuit in two separate circuits:base: base circuit for C2 (SecretKeyShareComputation and SmudgingNoiseShareComputation). verifies secret commitment and consistency, and outputs chunk and party commitments to bind the chunk circuits and provide share commitments for downstream C3/C4.chunk: verifies y_chunk is consistent with chunk_commitment from base circuit, range bounds and parity check with the matrix.tasks
baseandchunk@zahrajavarzk-helpersscript to generate the inputs for these two new circuits @0xjeiuser_data_encryption@0xjeiSummary by CodeRabbit
New Features
Bug Fixes
Chores