feat: add recursive aggregation module [skip-line-limit]#1365
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughRefactors recursive-aggregation circuits and prover to support wrapper + fold aggregation: switches wrappers from ZK to non‑ZK verification, introduces per-proof fold inputs and VK-hash chaining, adds VK artifact loading and utils, exposes new prover APIs for wrapper/fold generation & verification, and updates build tooling to emit multiple VK artifacts. Changes
sequenceDiagram
participant User
participant Provable as Provable
participant ZkProver as ZkProver
participant VKLoader as VK Loader
participant RecAgg as RecAgg Module
participant BB as BBBinary
User->>Provable: aggregate_proof(inputs, optional_aggregated_proof)
Provable->>ZkProver: generate_recursive_proof (per input)
ZkProver->>VKLoader: load_vk_artifacts(circuit)
VKLoader-->>ZkProver: VkArtifacts (verification_key, key_hash)
ZkProver->>BB: invoke bb (witness, vk[_recursive], -t verifier_target)
BB-->>ZkProver: recursive proof
ZkProver-->>Provable: recursive proofs
Provable->>RecAgg: generate_wrapper_proof(recursive_proofs)
RecAgg->>VKLoader: load_wrapper_vk_artifacts
VKLoader-->>RecAgg: VkArtifacts
RecAgg->>ZkProver: generate_wrapper_proof
ZkProver->>BB: invoke bb (wrapper vk[_recursive])
BB-->>ZkProver: wrapper proof
ZkProver-->>Provable: wrapper proof
alt aggregated_proof provided
Provable->>RecAgg: generate_fold_proof(aggregated_proof, wrapper_proof)
RecAgg->>VKLoader: load_vk_for_fold_input
VKLoader-->>RecAgg: VkArtifacts
RecAgg->>ZkProver: generate_fold_proof
ZkProver->>BB: invoke bb (fold vk[_recursive], verifier_target=no-zk)
BB-->>ZkProver: fold proof
ZkProver-->>Provable: final fold proof
else
Provable-->>User: wrapper proof
end
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 |
cedoor
changed the title
![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: 3
🧹 Nitpick comments (5)
crates/zk-prover/src/circuits/recursive_aggregation/mod.rs (1)
173-180: Make commitment extraction strict instead of usinglast().Lines 173-180 silently take the last field from
public_signals. Require the exact expected shape (single commitment) and fail fast when it drifts.♻️ Proposed hardening
- let commitment1 = bytes_to_field_strings(&proof1.public_signals)? - .into_iter() - .last() - .ok_or_else(|| ZkError::InvalidInput("proof1 public_signals is empty".into()))?; - let commitment2 = bytes_to_field_strings(&proof2.public_signals)? - .into_iter() - .last() - .ok_or_else(|| ZkError::InvalidInput("proof2 public_signals is empty".into()))?; + let commitment1_fields = bytes_to_field_strings(&proof1.public_signals)?; + if commitment1_fields.len() != 1 { + return Err(ZkError::InvalidInput(format!( + "expected proof1 to expose exactly 1 commitment field, got {}", + commitment1_fields.len() + ))); + } + let commitment1 = commitment1_fields[0].clone(); + + let commitment2_fields = bytes_to_field_strings(&proof2.public_signals)?; + if commitment2_fields.len() != 1 { + return Err(ZkError::InvalidInput(format!( + "expected proof2 to expose exactly 1 commitment field, got {}", + commitment2_fields.len() + ))); + } + let commitment2 = commitment2_fields[0].clone();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/circuits/recursive_aggregation/mod.rs` around lines 173 - 180, Replace the permissive "take last" extraction of commitments with a strict check that each proof's public_signals decodes to exactly one field; for both commitment1 and commitment2 use bytes_to_field_strings(&proofX.public_signals)?, assert the resulting Vec has length == 1 (or pattern-match it to a single element) and return ZkError::InvalidInput with a clear message if not exactly one element, so any shape drift fails fast instead of silently picking the last element.crates/zk-prover/src/traits.rs (2)
85-86: Doc comment uses stale parameter name.Lines 85-86 mention
fold_proof, but the API usesaggregated_proof. Align wording to avoid caller confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/traits.rs` around lines 85 - 86, The doc comment currently refers to a stale parameter name `fold_proof`; update the comment to reference the actual parameter `aggregated_proof` and adjust the wording to match its behavior (e.g., "When `aggregated_proof` is provided: if it is a wrapper proof, does initial fold (two wrappers → fold); if it is a fold proof, folds wrapper with it. When `None`, returns the wrapper proof."). Locate the comment near the trait/function in crates/zk-prover/src/traits.rs that documents this behavior (search for the existing text mentioning `fold_proof`) and replace the parameter name and any related phrasing to avoid caller confusion while preserving the described logic.
105-129: Pre-validate circuit compatibility before proving inner inputs.Line 125 rejects mixed circuits only after Lines 109-123 already generated recursive proofs. Move the “same resolved circuit” check before witness/proof generation to avoid wasted proving work and extra artifacts on invalid input.
♻️ Proposed refactor
- let mut recursive_proofs = Vec::with_capacity(inputs.len()); - let mut resolved_names = Vec::with_capacity(inputs.len()); + let mut resolved_names = Vec::with_capacity(inputs.len()); + for input in inputs { + resolved_names.push(self.resolve_circuit_name(params, input)); + } + if resolved_names.len() == 2 && resolved_names[0] != resolved_names[1] { + return Err(ZkError::InvalidInput( + "aggregate_proof requires both inputs to use the same circuit".into(), + )); + } + + let mut recursive_proofs = Vec::with_capacity(inputs.len()); let witness_gen = WitnessGenerator::new(); for (i, input) in inputs.iter().enumerate() { let input_map = self.build_inputs(params, input)?; - let resolved_name = self.resolve_circuit_name(params, input); - resolved_names.push(resolved_name); + let resolved_name = resolved_names[i]; let circuit_path = prover .circuits_dir() - .join(resolved_names[i].dir_path()) - .join(format!("{}.json", resolved_names[i].as_str())); + .join(resolved_name.dir_path()) + .join(format!("{}.json", resolved_name.as_str())); let circuit = CompiledCircuit::from_file(&circuit_path)?; let witness = witness_gen.generate_witness(&circuit, input_map)?; let inner_e3_id = format!("{}_inner_{}", e3_id, i); - let proof = - prover.generate_recursive_proof(resolved_names[i], &witness, &inner_e3_id)?; + let proof = prover.generate_recursive_proof(resolved_name, &witness, &inner_e3_id)?; recursive_proofs.push(proof); } - - if recursive_proofs.len() == 2 && resolved_names[0] != resolved_names[1] { - return Err(ZkError::InvalidInput( - "aggregate_proof requires both inputs to use the same circuit".into(), - )); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/traits.rs` around lines 105 - 129, Pre-validate that all inputs reference the same compiled circuit by resolving circuit names up-front: first iterate inputs to call resolve_circuit_name (collect into resolved_names) and check the compatibility rule (e.g., if recursive_proofs will be length 2 ensure resolved_names[0] == resolved_names[1]) and return ZkError::InvalidInput if they differ; only after this check proceed to build inputs (build_inputs), generate witnesses with WitnessGenerator::new() and call prover.generate_recursive_proof to populate recursive_proofs. This moves the equality check before any witness/proof generation (resolve_circuit_name, build_inputs, generate_witness, generate_recursive_proof) to avoid wasted work and artifacts.crates/zk-prover/src/prover.rs (2)
122-125: Extract VK suffix mapping into a helper to avoid drift.The same
verifier_target -> vk_suffixmatch appears twice; centralizing it reduces maintenance risk.♻️ Proposed refactor
impl ZkProver { + fn vk_suffix_for_target(verifier_target: Option<&str>) -> &'static str { + match verifier_target { + Some("noir-recursive") | Some("noir-recursive-no-zk") => "_recursive", + _ => "", + } + } + fn generate_proof_impl( &self, circuit: CircuitName, witness_data: &[u8], e3_id: &str, dir_path: &str, verifier_target: Option<&str>, ) -> Result<Proof, ZkError> { - let vk_suffix = match verifier_target { - Some("noir-recursive") | Some("noir-recursive-no-zk") => "_recursive", - _ => "", - }; + let vk_suffix = Self::vk_suffix_for_target(verifier_target); ... } fn verify_proof_impl( &self, circuit: CircuitName, proof_data: &[u8], public_signals: &[u8], dir_path: String, e3_id: &str, party_id: u64, verifier_target: Option<&str>, ) -> Result<bool, ZkError> { - let vk_suffix = match verifier_target { - Some("noir-recursive") | Some("noir-recursive-no-zk") => "_recursive", - _ => "", - }; + let vk_suffix = Self::vk_suffix_for_target(verifier_target); ... } }Also applies to: 280-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/prover.rs` around lines 122 - 125, Extract the repeated match logic that maps verifier_target to the vk_suffix into a single helper (e.g., a function like vk_suffix_for or vk_suffix_from_verifier_target) and replace both occurrences where you set vk_suffix (the match on verifier_target that yields "_recursive" or "") with calls to that helper; ensure the helper accepts the same type as verifier_target (Option<&str> or Option<String> as used) and returns a &'static str or String consistent with how vk_suffix is later used so both sites in prover.rs use the centralized mapping.
71-84: Fail fast when wrapper APIs are called withCircuitName::Fold.Right now, misuse falls through to file-not-found errors. Return
ZkError::InvalidInputimmediately for clearer behavior and easier debugging.♻️ Proposed refactor
pub fn generate_wrapper_proof( &self, circuit: CircuitName, witness_data: &[u8], e3_id: &str, ) -> Result<Proof, ZkError> { + if circuit == CircuitName::Fold { + return Err(ZkError::InvalidInput( + "Fold circuit has no wrapper circuit".to_string(), + )); + } self.generate_proof_impl( circuit, witness_data, e3_id, &circuit.wrapper_dir_path(), Some("noir-recursive-no-zk"), ) } pub fn verify_wrapper_proof( &self, proof: &Proof, e3_id: &str, party_id: u64, ) -> Result<bool, ZkError> { + if proof.circuit == CircuitName::Fold { + return Err(ZkError::InvalidInput( + "Fold circuit has no wrapper circuit".to_string(), + )); + } self.verify_proof_impl( proof.circuit, &proof.data, &proof.public_signals, proof.circuit.wrapper_dir_path(), e3_id, party_id, Some("noir-recursive-no-zk"), ) }Also applies to: 223-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/prover.rs` around lines 71 - 84, Add an explicit early check for CircuitName::Fold and return ZkError::InvalidInput in the wrapper-proof paths: at the start of generate_wrapper_proof, if circuit == CircuitName::Fold return Err(ZkError::InvalidInput("wrapper API does not support Fold".into())); likewise, update any other wrapper-API call sites that invoke generate_proof_impl with &circuit.wrapper_dir_path() and the "noir-recursive-no-zk" profile (the code region around the other wrapper implementation that falls through to file-not-found) to perform the same guard and return ZkError::InvalidInput immediately instead of allowing file-not-found errors to surface.
🤖 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_encryption/src/main.nr`:
- Around line 21-25: The key_hash parameter is currently a private Field which
prevents external VK binding; update the function signature to make key_hash a
public input (change key_hash: Field to key_hash: pub Field) wherever this
wrapper's entry function defines it, ensure calls to
verify_honk_proof_non_zk(...) still pass the same symbol, and if other wrapper
circuits follow the private pattern, align them by either making their key_hash
parameters pub or by including key_hash in the aggregated_public_inputs and
validating it downstream (e.g., in the fold circuit) so the external VK can be
verified consistently.
In `@scripts/build-circuits.ts`:
- Around line 305-345: runWriteVk can fail silently and cause the build to be
reported successful despite missing VKs; change the callers of runWriteVk (the
invocations for 'evm' and 'noir-recursive-no-zk' near the use of variables
packageName, result, isWrapper) to fail fast by throwing an Error (or exiting
non-zero) when runWriteVk returns false instead of continuing; update both
branches (isWrapper true/false) so a returned false from runWriteVk results in
an immediate throw like new Error(`VK generation failed for ${packageName}
(${verifierTarget})`) so the overall build stops and the failure is reported.
- Around line 305-308: The execSync call inside runWriteVk doesn't pass the
parsed --oracle-hash flag; create an oracleHashFlag string (like in
scripts/generate-verifiers.ts) based on this.options.oracleHash (empty string
when undefined) and append it to the bb write_vk command invocation in
runWriteVk so the command includes the --oracle_hash option when provided.
---
Nitpick comments:
In `@crates/zk-prover/src/circuits/recursive_aggregation/mod.rs`:
- Around line 173-180: Replace the permissive "take last" extraction of
commitments with a strict check that each proof's public_signals decodes to
exactly one field; for both commitment1 and commitment2 use
bytes_to_field_strings(&proofX.public_signals)?, assert the resulting Vec has
length == 1 (or pattern-match it to a single element) and return
ZkError::InvalidInput with a clear message if not exactly one element, so any
shape drift fails fast instead of silently picking the last element.
In `@crates/zk-prover/src/prover.rs`:
- Around line 122-125: Extract the repeated match logic that maps
verifier_target to the vk_suffix into a single helper (e.g., a function like
vk_suffix_for or vk_suffix_from_verifier_target) and replace both occurrences
where you set vk_suffix (the match on verifier_target that yields "_recursive"
or "") with calls to that helper; ensure the helper accepts the same type as
verifier_target (Option<&str> or Option<String> as used) and returns a &'static
str or String consistent with how vk_suffix is later used so both sites in
prover.rs use the centralized mapping.
- Around line 71-84: Add an explicit early check for CircuitName::Fold and
return ZkError::InvalidInput in the wrapper-proof paths: at the start of
generate_wrapper_proof, if circuit == CircuitName::Fold return
Err(ZkError::InvalidInput("wrapper API does not support Fold".into()));
likewise, update any other wrapper-API call sites that invoke
generate_proof_impl with &circuit.wrapper_dir_path() and the
"noir-recursive-no-zk" profile (the code region around the other wrapper
implementation that falls through to file-not-found) to perform the same guard
and return ZkError::InvalidInput immediately instead of allowing file-not-found
errors to surface.
In `@crates/zk-prover/src/traits.rs`:
- Around line 85-86: The doc comment currently refers to a stale parameter name
`fold_proof`; update the comment to reference the actual parameter
`aggregated_proof` and adjust the wording to match its behavior (e.g., "When
`aggregated_proof` is provided: if it is a wrapper proof, does initial fold (two
wrappers → fold); if it is a fold proof, folds wrapper with it. When `None`,
returns the wrapper proof."). Locate the comment near the trait/function in
crates/zk-prover/src/traits.rs that documents this behavior (search for the
existing text mentioning `fold_proof`) and replace the parameter name and any
related phrasing to avoid caller confusion while preserving the described logic.
- Around line 105-129: Pre-validate that all inputs reference the same compiled
circuit by resolving circuit names up-front: first iterate inputs to call
resolve_circuit_name (collect into resolved_names) and check the compatibility
rule (e.g., if recursive_proofs will be length 2 ensure resolved_names[0] ==
resolved_names[1]) and return ZkError::InvalidInput if they differ; only after
this check proceed to build inputs (build_inputs), generate witnesses with
WitnessGenerator::new() and call prover.generate_recursive_proof to populate
recursive_proofs. This moves the equality check before any witness/proof
generation (resolve_circuit_name, build_inputs, generate_witness,
generate_recursive_proof) to avoid wasted work and artifacts.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
circuits/bin/recursive_aggregation/fold/src/main.nrcircuits/bin/recursive_aggregation/wrapper/dkg/pk/src/main.nrcircuits/bin/recursive_aggregation/wrapper/dkg/share_computation/src/main.nrcircuits/bin/recursive_aggregation/wrapper/dkg/share_decryption/src/main.nrcircuits/bin/recursive_aggregation/wrapper/dkg/share_encryption/src/main.nrcircuits/bin/recursive_aggregation/wrapper/threshold/decrypted_shares_aggregation/src/main.nrcircuits/bin/recursive_aggregation/wrapper/threshold/pk_aggregation/src/main.nrcircuits/bin/recursive_aggregation/wrapper/threshold/pk_generation/src/main.nrcircuits/bin/recursive_aggregation/wrapper/threshold/share_decryption/src/main.nrcrates/events/src/enclave_event/proof.rscrates/zk-prover/src/actors/zk_actor.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/recursive_aggregation/vk.rscrates/zk-prover/src/error.rscrates/zk-prover/src/lib.rscrates/zk-prover/src/prover.rscrates/zk-prover/src/traits.rsscripts/build-circuits.ts
0xjei
reviewed
Mar 2, 2026
0xjei
left a comment
0xjei
left a comment
Contributor
There was a problem hiding this comment.
did a quick review, amazing progress so far - looking forward to all when ready!
- Recurse into subdirs to find circuits, skip workspace-only Nargo.toml - Add parent target dirs to artifact search for workspace members - Log failed circuit errors at end of build Made-with: Cursor
- Rename bb output vk_hash to {packageName}.vk_hash to avoid overwrite
- Add vk_hash to dist artifacts and checksums
Made-with: Cursor
- Non-wrappers: generate both EVM and recursive VK artifacts - Wrappers: generate only recursive VK artifacts - Add isWrapper() helper to detect circuits under recursive_aggregation/wrapper/ Made-with: Cursor
- Throw Error when runWriteVk returns false for evm or noir-recursive-no-zk - Apply to both wrapper and non-wrapper circuit branches - Prevent build reporting success despite missing VKs Made-with: Cursor
- Remove oracleHash option from build-circuits.ts (already used -t evm/noir-recursive-no-zk) - Replace --oracle-hash with -t evm in generate-verifiers.ts - Update scripts/README.md to reflect new bb write_vk API Made-with: Cursor
cedoor
force-pushed
the
feat/aggregation-module
branch
from
4dabc87 to
b6b614e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
scripts/generate-verifiers.ts (1)
260-261: Verify bb version/feature support before generating verification keys.The code suppresses
bb write_vk -t evmstderr output with{ stdio: 'pipe' }, so failures from unsupported-tflag (on older bb versions) produce a generic "Failed to generate verification key" error. Whilebb --versionis checked at startup (line 82), it only validates the tool exists, not feature support. Adding a preflight check for-tflag support would provide clearer diagnostics if an incompatible bb version is used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-verifiers.ts` around lines 260 - 261, Add a preflight check that verifies the installed `bb` supports the `write_vk -t evm` option before calling execSync to generate VKs: run `bb write_vk --help` (or an equivalent non-destructive help/dry-run) and inspect the output for either the `-t`/`--target` flag or the `evm` target string; if the help output does not mention that flag/target, throw a clear error message indicating the installed `bb` lacks `-t evm` support. Place this check immediately before the existing execSync call that runs `bb write_vk -b "${jsonFile}" -o "${targetDir}" -t evm` in generate-verifiers.ts so failures surface with a descriptive diagnostic rather than the generic "Failed to generate verification key" message.crates/zk-prover/src/traits.rs (1)
109-129: Validate mixed-circuit inputs before generating recursive proofs.At Line 125, the same-circuit check happens after both inner proofs are already generated. For invalid pairs, this does unnecessary proving work before returning an error.
♻️ Proposed refactor
- let mut recursive_proofs = Vec::with_capacity(inputs.len()); - let mut resolved_names = Vec::with_capacity(inputs.len()); + let resolved_names: Vec<CircuitName> = inputs + .iter() + .map(|input| self.resolve_circuit_name(params, input)) + .collect(); + if resolved_names.len() == 2 && resolved_names[0] != resolved_names[1] { + return Err(ZkError::InvalidInput( + "aggregate_proof requires both inputs to use the same circuit".into(), + )); + } + + let mut recursive_proofs = Vec::with_capacity(inputs.len()); let witness_gen = WitnessGenerator::new(); for (i, input) in inputs.iter().enumerate() { let input_map = self.build_inputs(params, input)?; - let resolved_name = self.resolve_circuit_name(params, input); - resolved_names.push(resolved_name); + let resolved_name = resolved_names[i].clone(); let circuit_path = prover .circuits_dir() - .join(resolved_names[i].dir_path()) - .join(format!("{}.json", resolved_names[i].as_str())); + .join(resolved_name.dir_path()) + .join(format!("{}.json", resolved_name.as_str())); let circuit = CompiledCircuit::from_file(&circuit_path)?; let witness = witness_gen.generate_witness(&circuit, input_map)?; let inner_e3_id = format!("{}_inner_{}", e3_id, i); - let proof = - prover.generate_recursive_proof(resolved_names[i], &witness, &inner_e3_id)?; + let proof = prover.generate_recursive_proof(resolved_name, &witness, &inner_e3_id)?; recursive_proofs.push(proof); } - - if recursive_proofs.len() == 2 && resolved_names[0] != resolved_names[1] { - return Err(ZkError::InvalidInput( - "aggregate_proof requires both inputs to use the same circuit".into(), - )); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/traits.rs` around lines 109 - 129, Resolve circuit identities for all inputs first (use resolve_circuit_name and collect into resolved_names) and perform the same-circuit validation immediately after that; if there are two inputs and resolved_names[0] != resolved_names[1], return the ZkError::InvalidInput error before calling CompiledCircuit::from_file, witness_gen.generate_witness, or prover.generate_recursive_proof. In other words, move the mixed-circuit check to occur after resolve_circuit_name but before invoking CompiledCircuit::from_file / witness generation / generate_recursive_proof to avoid doing unnecessary work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/events/src/enclave_event/proof.rs`:
- Around line 100-103: The wrapper_dir_path() currently builds
"recursive_aggregation/wrapper/{}" using dir_path(), which duplicates the
"recursive_aggregation" prefix for CircuitName::Fold (producing
recursive_aggregation/wrapper/recursive_aggregation/fold); update
wrapper_dir_path() to special-case Fold (or detect if dir_path() already starts
with "recursive_aggregation/") and return "recursive_aggregation/wrapper/fold"
for CircuitName::Fold, otherwise keep the existing
format!("recursive_aggregation/wrapper/{}", self.dir_path()); reference
wrapper_dir_path(), dir_path(), and CircuitName::Fold when making the change.
In `@crates/zk-prover/src/circuits/recursive_aggregation/mod.rs`:
- Around line 486-507: The test creates two inner proofs but only removes the
first inner artifact; update the cleanup to also remove the second inner
artifact by calling prover.cleanup for the "_inner_1" artifact (use the same
pattern as format!("{}_inner_0", e3_id) but with "_inner_1") after verifying the
fold proof so both inner artifacts are cleaned up; reference the
ShareEncryptionCircuit::aggregate_proof call that produced the inner proofs and
the existing prover.cleanup(e3_id) call to place the new cleanup call.
In `@crates/zk-prover/src/circuits/recursive_aggregation/vk.rs`:
- Around line 50-56: load_wrapper_vk_artifacts currently accepts
CircuitName::Fold and lets load_vk_from_dir produce a confusing CircuitNotFound
error; add an explicit guard at the top of load_wrapper_vk_artifacts to reject
CircuitName::Fold and return a clear ZkError indicating fold VKs are not wrapper
artifacts (e.g., return Err(ZkError::InvalidInput(...)) when circuit ==
CircuitName::Fold) before constructing circuit_dir or calling load_vk_from_dir.
In `@crates/zk-prover/src/prover.rs`:
- Around line 71-84: The wrapper APIs must reject Fold circuits up-front: add an
explicit InvalidInput check in generate_wrapper_proof that returns an
Err(ZkError::InvalidInput(...)) if the provided circuit equals
CircuitName::Fold, instead of letting generate_proof_impl fail later; do the
same for the corresponding wrapper verify method (the other wrapper method
around generate_verify_impl / the wrapper verify path at the 223-239 region).
Reference CircuitName::Fold, generate_wrapper_proof, the corresponding wrapper
verify function, and the existing calls to .wrapper_dir_path() / the
"noir-recursive-no-zk" wrapper path when implementing the guard. Ensure the
error message clearly states that Fold circuits are not supported by wrapper
APIs.
- Around line 110-117: generate_proof_impl and verify_proof_impl currently write
artifacts to deterministic paths (e3_id-based directories and files like
witness.gz, out, verify_party_<id>) which allows parallel runs to clobber each
other; change both functions to create a per-invocation unique subdirectory
(e.g., append a short UUID or timestamp to dir_path or e3_id to form a
temp_run_dir) and use that temp_run_dir for all artifact writes/reads
(witness.gz, out, verify_party_<id>), passing the unique path into any helper
routines that currently expect dir_path; ensure the temp dir is created
atomically, used for the entire proof/verify flow, and optionally cleaned up on
success or preserved on error for debugging.
In `@crates/zk-prover/src/traits.rs`:
- Around line 83-86: The doc comment refers to a stale parameter name
`fold_proof` while the method actually uses `aggregated_proof`; update the
documentation in crates/zk-prover/src/traits.rs to mention `aggregated_proof`
(and its semantics: when provided, if it's a wrapper proof perform an initial
fold, if it's a fold proof fold the wrapper with it; when `None` return the
wrapper proof) so the text matches the method signature and behavior (reference:
the doc block above the method using aggregated_proof).
In `@scripts/build-circuits.ts`:
- Around line 303-323: The runWriteVk function currently returns true even when
the produced artifacts are missing; change its post-exec behavior so that
immediately after execSync completes you validate that both defaultVk
(join(targetDir, 'vk')) and defaultVkHash (join(targetDir, 'vk_hash')) exist,
and if either is missing log a clear error referencing verifierTarget and
jsonFile and return false; only proceed to perform the rm/copy operations to
vkOut and vkHashOut when both files exist, and keep the existing try/catch to
handle exec errors.
---
Nitpick comments:
In `@crates/zk-prover/src/traits.rs`:
- Around line 109-129: Resolve circuit identities for all inputs first (use
resolve_circuit_name and collect into resolved_names) and perform the
same-circuit validation immediately after that; if there are two inputs and
resolved_names[0] != resolved_names[1], return the ZkError::InvalidInput error
before calling CompiledCircuit::from_file, witness_gen.generate_witness, or
prover.generate_recursive_proof. In other words, move the mixed-circuit check to
occur after resolve_circuit_name but before invoking CompiledCircuit::from_file
/ witness generation / generate_recursive_proof to avoid doing unnecessary work.
In `@scripts/generate-verifiers.ts`:
- Around line 260-261: Add a preflight check that verifies the installed `bb`
supports the `write_vk -t evm` option before calling execSync to generate VKs:
run `bb write_vk --help` (or an equivalent non-destructive help/dry-run) and
inspect the output for either the `-t`/`--target` flag or the `evm` target
string; if the help output does not mention that flag/target, throw a clear
error message indicating the installed `bb` lacks `-t evm` support. Place this
check immediately before the existing execSync call that runs `bb write_vk -b
"${jsonFile}" -o "${targetDir}" -t evm` in generate-verifiers.ts so failures
surface with a descriptive diagnostic rather than the generic "Failed to
generate verification key" message.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
circuits/bin/recursive_aggregation/fold/src/main.nrcircuits/bin/recursive_aggregation/wrapper/dkg/pk/src/main.nrcircuits/bin/recursive_aggregation/wrapper/dkg/share_computation/src/main.nrcircuits/bin/recursive_aggregation/wrapper/dkg/share_decryption/src/main.nrcircuits/bin/recursive_aggregation/wrapper/dkg/share_encryption/src/main.nrcircuits/bin/recursive_aggregation/wrapper/threshold/decrypted_shares_aggregation/src/main.nrcircuits/bin/recursive_aggregation/wrapper/threshold/pk_aggregation/src/main.nrcircuits/bin/recursive_aggregation/wrapper/threshold/pk_generation/src/main.nrcircuits/bin/recursive_aggregation/wrapper/threshold/share_decryption/src/main.nrcircuits/lib/src/math/commitments.nrcrates/events/src/enclave_event/proof.rscrates/zk-prover/src/actors/zk_actor.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/recursive_aggregation/vk.rscrates/zk-prover/src/error.rscrates/zk-prover/src/lib.rscrates/zk-prover/src/prover.rscrates/zk-prover/src/traits.rsscripts/README.mdscripts/build-circuits.tsscripts/generate-verifiers.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/zk-prover/src/lib.rs
- crates/zk-prover/src/actors/zk_actor.rs
- crates/zk-prover/src/circuits/recursive_aggregation/utils.rs
- crates/zk-prover/src/error.rs
- circuits/bin/recursive_aggregation/wrapper/threshold/share_decryption/src/main.nr
- add _inner_1 cleanup in fold proof test (both inner artifacts) - fix aggregate_proof doc to use aggregated_proof param name - validate VK artifacts exist after bb write_vk before copy; return false on missing Made-with: Cursor
- Replace prover.verify() with prover.verify_proof() in multithread crate - Fixes E0599 compilation error Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/zk-prover/src/circuits/recursive_aggregation/mod.rs`:
- Around line 177-194: The current slicing of proof1_public_inputs and
proof2_public_inputs silently truncates inputs; change the validation to require
exactly two public inputs for each proof: replace the get(0..2).ok_or_else(...)
usage with an explicit check that proof1_public_inputs.len() == 2 and
proof2_public_inputs.len() == 2, returning ZkError::InvalidInput with a clear
message if the length differs, then proceed to clone/to_vec the inputs only
after the exact-length check (reference symbols: proof1_public_inputs,
proof2_public_inputs, ZkError::InvalidInput).
In `@crates/zk-prover/src/traits.rs`:
- Around line 109-129: Compute and validate circuit compatibility before
generating any recursive proofs: first iterate inputs to call
resolve_circuit_name (and/or build_inputs/CompiledCircuit::from_file if needed)
to collect resolved_names for all inputs, then if resolved_names.len() == 2 and
resolved_names[0] != resolved_names[1] return the ZkError::InvalidInput
immediately; only after this early check proceed to generate witnesses
(witness_gen.generate_witness), create inner_e3_id, call
prover.generate_recursive_proof, and push into recursive_proofs so no proof work
or _inner_* artifacts are produced on the failure path.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/multithread/src/multithread.rscrates/zk-prover/src/circuits/recursive_aggregation/mod.rscrates/zk-prover/src/traits.rsscripts/build-circuits.ts
- Replace get(0..2).ok_or_else with explicit len() == 2 checks - Return ZkError::InvalidInput for both too few and too many inputs - Prevent silent truncation when proofs have more than 2 public inputs Made-with: Cursor
… generation - Collect resolved_names via resolve_circuit_name before any proof work - Return InvalidInput immediately when 2 inputs use different circuits - Avoid witness generation and recursive proof work on failure path Made-with: Cursor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/zk-prover/src/circuits/recursive_aggregation/mod.rs (1)
390-395: Prefere3_id-derived cleanup keys instead of hardcoded literals.These hardcoded strings can drift from the test’s
e3_idvalue and make maintenance brittle.♻️ Suggested refactor
- prover - .cleanup("aggregation-2proof-wrapper_inner_0") - .unwrap(); - prover - .cleanup("aggregation-2proof-wrapper_inner_1") - .unwrap(); + prover.cleanup(&format!("{}_inner_0", e3_id)).unwrap(); + prover.cleanup(&format!("{}_inner_1", e3_id)).unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/circuits/recursive_aggregation/mod.rs` around lines 390 - 395, The two prover.cleanup calls use hardcoded keys ("aggregation-2proof-wrapper_inner_0" and "_1") which can diverge from the test's e3_id; change these to derive the keys from the existing e3_id variable (use the same formatting used elsewhere when constructing aggregation keys) and call prover.cleanup with those formatted strings (e.g., build the key using e3_id and the inner index for both cleanup calls) so the cleanup always matches the produced artifact names; update both occurrences where prover.cleanup("aggregation-2proof-wrapper_inner_0") and prover.cleanup("aggregation-2proof-wrapper_inner_1") appear to use the e3_id-derived key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/zk-prover/src/circuits/recursive_aggregation/mod.rs`:
- Around line 390-395: The two prover.cleanup calls use hardcoded keys
("aggregation-2proof-wrapper_inner_0" and "_1") which can diverge from the
test's e3_id; change these to derive the keys from the existing e3_id variable
(use the same formatting used elsewhere when constructing aggregation keys) and
call prover.cleanup with those formatted strings (e.g., build the key using
e3_id and the inner index for both cleanup calls) so the cleanup always matches
the produced artifact names; update both occurrences where
prover.cleanup("aggregation-2proof-wrapper_inner_0") and
prover.cleanup("aggregation-2proof-wrapper_inner_1") appear to use the
e3_id-derived key.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/zk-prover/src/circuits/recursive_aggregation/mod.rscrates/zk-prover/src/traits.rs
This was referenced Mar 11, 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 adds a single
aggregate_proofAPI that progressively (sequentially) aggregates ZK proofs into one fold proof for on-chain verification. Nodes and the aggregator reuse the same call: the first call produces a wrapper proof, later calls fold it with new proofs, and the latest result is passed into the next call.API
Nodes and the aggregator use a single function to progressively aggregate ZK proofs into one final proof suitable for on-chain submission.
aggregated_proof = None): Generates inner proof(s) → wrapper proof. Returns the wrapper.aggregated_proof = Some(prev)): Generates inner proof(s) → wrapper → fold withprev. Returns the fold proof.Each call replaces the previous aggregated proof. Store the returned proof and pass it as
aggregated_proofin the next call.Node Flow (C0, C1, C2, C3, C4, C6)
Nodes produce and aggregate 6 proof types in sequence. Some of them have two variants: sk and e_sm, and each node will need to generate H proofs for C3 (one per honest party).
flowchart LR subgraph step1["Step 1: C0 (pk)"] c0_inner[inner C0] --> c0_wrap[wrapper C0] end c0_wrap --> agg1[agg = wrapper C0] subgraph step2["Step 2: C1 (pk_generation)"] c1_inner[inner C1] --> c1_wrap[wrapper C1] c1_wrap --> c1_fold[fold agg, wrapper C1] end agg1 --> c1_fold c1_fold --> agg2[agg = fold] subgraph step3["Step 3: C2 (share_computation)"] c2_inner[inner C2a, C2b] --> c2_wrap[wrapper C2] c2_wrap --> c2_fold[fold agg, wrapper C2] end agg2 --> c2_fold c2_fold --> agg3[agg = fold] subgraph step4["Step 4: C3 (share_encryption)"] c3_inner[inner C3a, C3b] --> c3_wrap[wrapper C3] c3_wrap --> c3_fold[fold agg, wrapper C3] end agg3 --> c3_fold c3_fold --> agg4[agg = fold] subgraph step5["Step 5: C4 (share_decryption) — H proofs"] c4_inner[inner C4a₁..C4a_H, C4b₁..C4b_H] --> c4_wrap[wrapper C4 × H] c4_wrap --> c4_fold[fold agg, wrapper C4] end agg4 --> c4_fold c4_fold --> agg5[agg = fold] subgraph step6["Step 6: C6 (threshold share_decryption)"] c6_inner[inner C6] --> c6_wrap[wrapper C6] c6_wrap --> c6_fold[fold agg, wrapper C6] end agg5 --> c6_fold c6_fold --> final[Send agg to aggregator]Aggregator Flow (node_agg + C5 + C7)
The aggregator receives H node aggregated proofs and aggregates with its own proofs (C5, C7):
flowchart TB subgraph receive["Receive"] node_agg[H node_aggregated_proofs] end subgraph fold_nodes["Fold H node proofs (H-1 folds)"] f1[fold node_1, node_2] f2[fold ... node_H] f1 --> f2 end node_agg --> fold_nodes subgraph step1["Step 1: C5 (pk_aggregation)"] c5_inner[inner C5] --> c5_wrap[wrapper C5] c5_wrap --> c5_fold[fold node_agg, wrapper C5] end fold_nodes --> c5_fold c5_fold --> agg1[agg = fold] subgraph step2["Step 2: C7 (decrypted_shares_aggregation)"] c7_inner[inner C7] --> c7_wrap[wrapper C7] c7_wrap --> c7_fold[fold agg, wrapper C7] end agg1 --> c7_fold c7_fold --> onchain[Post final fold on-chain]Summary by CodeRabbit
New Features
Improvements