refactor: new proof aggregation [skip-line-limit]#1516
Conversation
- C3: private ct limbs, public ct_commitment output (Poseidon DS_CIPHERTEXT) - C6: public ct_commitment input, witness-only ct0/ct1, verify limb hash - c3_fold: inner public inputs [Field; 3]; Rust fold + layout metadata - Flow-trace note on cross-phase ct_commitment vs user_data_encryption Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughReplaces the prior wrapper+fold recursive aggregation with phase-specific aggregation circuits and flows (NodeDkgFold, DkgAggregation, DecryptionAggregation). Adds many new Noir aggregation bins and zk‑prover aggregation APIs, removes wrapper/fold circuits and fold state, and updates multithread, aggregator actors, events, EVM contracts, tooling, and commitment/IO layouts (pk/ct commitments). Changes
Sequence Diagram(s)sequenceDiagram
participant Node as Node/Producer
participant Multithread as Multithread Worker
participant ZkProver as ZkProver
participant Registry as On-chain Registry/Verifier
Node->>Multithread: publish inner proofs (C0..C5 / C6 / C7)
Multithread->>ZkProver: Publish ZkRequest::NodeDkgFold / DkgAggregation / DecryptionAggregation
ZkProver->>ZkProver: generate_recursive_aggregation_bin_proof(...) / sequential folds
ZkProver-->>Multithread: ZkResponse::NodeDkgFold / DkgAggregation / DecryptionAggregation (proof(s))
Multithread->>Node: emit events (PublicKeyAggregated / PlaintextAggregated) with pk_commitment + aggregator proof(s)
Node->>Registry: publishCommittee / publishPlaintextOutput(pkCommitment, proof)
Registry-->>Node: verification result (bool)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
- Redesign c3_fold: one inner ShareEncryption ZK proof + optional prior c3_fold non-ZK proof - Pub outputs: per-party pk, msg, ct slots; skip acc verification when is_first_step - Add generate_c3_fold_step and generate_sequential_c3_fold; inner C3 uses extract_output(ct_commitment) - Remove C3FoldMerge circuit and CircuitName variant; update vk helper and lint-circuits Made-with: Cursor
- Add c2ab_fold, c3ab_fold, c4ab_fold, c6_fold, nodes_fold, node_fold, dkg_aggregator, and decryption_aggregator Noir bins - Remove legacy recursive_aggregation fold and wrapper trees; refresh c3_fold - Document aggregation bins in flow-trace; extend lint-circuits.sh checks
|
Could you do an early review of the new recursive-aggregation circuits? @zahrajavar @0xjei |
cedoor
changed the title
sure, will get a proper look tomorrow 🙏 |
|
agreed in chat on fixing the C3 fold stuff |
- Expand c3_fold and c3ab_fold public IO to N_PARTIES * L_THRESHOLD slots. - Assert C2 share commitments against C3 for every party/limb in node_fold. - Sync nodes_fold C3AB_FOLD_PUBLIC_LEN with c3ab_fold; use ASCII comments for nargo. - Update zk-prover accumulator, VK wiring, events, multithread, and integration tests.
- Add c3_fold_kernel Noir package and wire recursive aggregation build - Implement sequential C3 fold in c3_accumulator; export generate_sequential_c3_fold only - Add recursive_aggregation e2e tests (two inner ShareEncryption proofs) - Update prover, circuit helpers, Proof extract_input/output, multithread, flow-trace
- Add c6_fold_kernel Noir package for genesis accumulator (aligned with c3_fold_kernel). - Make c6_fold always verify the non-ZK accumulator like c3_fold. - Add CircuitName::C6FoldKernel and generate_sequential_c6_fold in aggregation/c6_accumulator.rs. - Move c3 accumulator into circuits/aggregation/ with a small mod.rs. - Copy c6_fold_kernel artifacts in integration test circuit staging. - Extend recursive_aggregation e2e tests (C6 ABI, staged kernel, two-step fold + verify).
- Add NODE_FOLD_PIPELINE (c2ab_fold, c3ab_fold, c4ab_fold, node_fold) JSON load + staged VK tests. - Stage the same bins in integration test default/recursive_aggregation fixture copy.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/aggregator/src/threshold_plaintext_aggregator.rs (1)
512-575:⚠️ Potential issue | 🔴 CriticalBind every C6 proof/share pair, not just index 0.
For multi-ciphertext decryptions,
proofs.first()andshares.first()only validate the first share. Later shares can still feed plaintext aggregation without matching their verified C6d_commitment, especially when recursive proof aggregation is disabled.Suggested fix direction
- let Some(first_proof) = proofs.first() else { - warn!( - "Empty C6 proof list for party {} — marking as mismatched", - party_id - ); - mismatched.insert(*party_id); - continue; - }; - let Some(c6_d_bytes) = c6_output_layout - .extract_field(&first_proof.payload.proof.public_signals, "d_commitment") - else { - warn!( - "Could not extract d_commitment from C6 proof for party {} — marking as mismatched", - party_id - ); - mismatched.insert(*party_id); - continue; - }; - - let Some(share_bytes) = shares.first() else { - warn!( - "No share bytes for party {} — marking as mismatched", - party_id - ); - mismatched.insert(*party_id); - continue; - }; - let Ok(poly) = e3_trbfv::helpers::try_poly_from_bytes(share_bytes, &threshold_params) - else { - warn!( - "Could not deserialize share for party {} — marking as mismatched", - party_id - ); - mismatched.insert(*party_id); - continue; - }; - let crt = e3_polynomial::CrtPolynomial::from_fhe_polynomial(&poly); - - // C6 public `d_commitment` hashes native truncated limbs (same layout as C7), not - // reversed+centered witness `d`. - let computed = compute_threshold_decryption_share_commitment(&crt, d_bit, max_k); - - // Convert to big-endian 32-byte padded format matching - // Barretenberg's public_signals encoding. - let (_, be_bytes) = computed.to_bytes_be(); - let mut computed_padded = [0u8; 32]; - let start = 32usize.saturating_sub(be_bytes.len()); - computed_padded[start..].copy_from_slice(&be_bytes[..be_bytes.len().min(32)]); - - if computed_padded != c6_d_bytes { - warn!( - "d_commitment mismatch for party {}: raw share commitment differs from C6 proof output", - party_id - ); - mismatched.insert(*party_id); - } + if proofs.len() != shares.len() { + warn!( + "C6 proof/share count mismatch for party {}: {} proofs, {} shares", + party_id, + proofs.len(), + shares.len() + ); + mismatched.insert(*party_id); + continue; + } + + for (ct_idx, share_bytes) in shares.iter().enumerate() { + let Some(proof) = proofs.get(ct_idx) else { + mismatched.insert(*party_id); + break; + }; + let Some(c6_d_bytes) = c6_output_layout + .extract_field(&proof.payload.proof.public_signals, "d_commitment") + else { + warn!( + "Could not extract d_commitment from C6 proof for party {} ct index {}", + party_id, ct_idx + ); + mismatched.insert(*party_id); + break; + }; + let Ok(poly) = + e3_trbfv::helpers::try_poly_from_bytes(share_bytes, &threshold_params) + else { + warn!( + "Could not deserialize share for party {} ct index {}", + party_id, ct_idx + ); + mismatched.insert(*party_id); + break; + }; + + let crt = e3_polynomial::CrtPolynomial::from_fhe_polynomial(&poly); + let computed = compute_threshold_decryption_share_commitment(&crt, d_bit, max_k); + let (_, be_bytes) = computed.to_bytes_be(); + let mut computed_padded = [0u8; 32]; + let start = 32usize.saturating_sub(be_bytes.len()); + computed_padded[start..].copy_from_slice(&be_bytes[..be_bytes.len().min(32)]); + + if computed_padded != c6_d_bytes { + warn!( + "d_commitment mismatch for party {} ct index {}", + party_id, ct_idx + ); + mismatched.insert(*party_id); + break; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/threshold_plaintext_aggregator.rs` around lines 512 - 575, The current loop in honest_shares only checks proofs.first() and shares.first(), so only the first ciphertext/share is bound and validated; update the logic to iterate over every proof/share pair for each party_id (use c6_proofs.get(party_id) and the shares Vec in honest_shares) and compare each proof's extracted "d_commitment" against the corresponding share's computed commitment via compute_threshold_decryption_share_commitment (after deserializing each share with e3_trbfv::helpers::try_poly_from_bytes and converting to CrtPolynomial). If any pair mismatches, insert the party_id into mismatched; otherwise accept that party only if all of its per-ciphertext pairs match. Ensure you reference proofs[i].payload.proof.public_signals and shares[i] (or equivalent index-aligned pairing) rather than .first().crates/tests/tests/integration.rs (1)
661-670:⚠️ Potential issue | 🔴 CriticalFix tuple variant pattern syntax.
NodeDkgFold,DkgAggregation, andDecryptionAggregationare tuple variants defined incrates/events/src/enclave_event/compute_request/zk.rs. Using{ .. }patterns will not compile; use tuple patterns(_)instead.Compile fix
- | ComputeRequestKind::Zk(ZkRequest::NodeDkgFold { .. }) - | ComputeRequestKind::Zk(ZkRequest::DkgAggregation { .. }) - | ComputeRequestKind::Zk(ZkRequest::DecryptionAggregation { .. }) + | ComputeRequestKind::Zk(ZkRequest::NodeDkgFold(_)) + | ComputeRequestKind::Zk(ZkRequest::DkgAggregation(_)) + | ComputeRequestKind::Zk(ZkRequest::DecryptionAggregation(_))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/tests/integration.rs` around lines 661 - 670, The match arm for EnclaveEventData::ComputeRequest uses struct-like patterns `{ .. }` for tuple variants from ZkRequest which causes a compile error; inside the matches! for ComputeRequestKind::Zk you should change the patterns ZkRequest::NodeDkgFold { .. }, ZkRequest::DkgAggregation { .. }, and ZkRequest::DecryptionAggregation { .. } to tuple patterns like ZkRequest::NodeDkgFold(_), ZkRequest::DkgAggregation(_), and ZkRequest::DecryptionAggregation(_) so the variant constructors match their actual definitions.circuits/lib/src/core/threshold/share_decryption.nr (1)
186-195:⚠️ Potential issue | 🔴 CriticalAssign the returned
Vecfrompushto update the payload with commitments.In lines 190–191,
inputs.push(...)does not updateinputsunder NoirVecsemantics; the return value must be assigned. Without this fix, the two critical commitment fields are omitted from the Fiat-Shamir payload, breaking the transcript binding.Proposed fix
// Use commitments instead of full polynomials (saves constraints) - inputs.push(self.expected_sk_commitment); - inputs.push(self.expected_e_sm_commitment); + inputs = inputs.push(self.expected_sk_commitment); + inputs = inputs.push(self.expected_e_sm_commitment);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/lib/src/core/threshold/share_decryption.nr` around lines 186 - 195, The payload() function is failing to include the commitments because Noir Vec::push returns a new Vec which must be assigned; change the two lines that call inputs.push(self.expected_sk_commitment) and inputs.push(self.expected_e_sm_commitment) to assign their results back to inputs (e.g., inputs = inputs.push(self.expected_sk_commitment); inputs = inputs.push(self.expected_e_sm_commitment);), so the commitments expected_sk_commitment and expected_e_sm_commitment are actually present before calling flatten::<_, _, BIT_CT>(inputs, self.ct0) and flatten::<_, _, BIT_CT>(inputs, self.ct1).
🧹 Nitpick comments (1)
examples/CRISP/server/src/cli/commands.rs (1)
189-211: Redundant timestamp reassignment at line 189.With the new
let current_timestamp = get_current_timestamp().await?;shadowing at line 205, the reassignment at line 189 is effectively dead — the value is only read by theinfo!at line 195 and then immediately overwritten (via shadowing) at line 205, along withwindow_start/input_window(lines 159-163 vs 207-211). Consider dropping the reassignment at line 189 (and themutat line 153) to simplify, or conversely drop the re-read+shadow at lines 205-211 if the intent was only to log a fresh timestamp before the tx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/CRISP/server/src/cli/commands.rs` around lines 189 - 211, The first call to get_current_timestamp() that assigns current_timestamp (used only for logging) is redundant because current_timestamp is immediately shadowed by a second get_current_timestamp() before window_start/input_window are computed; remove the earlier assignment (and the mut on the original declaration if present) and keep the later fresh get_current_timestamp() used to compute window_start and input_window (which reference CONFIG.e3_duration), or alternatively remove the later shadowing and use the initially fetched current_timestamp consistently—update occurrences of current_timestamp, window_start, and input_window accordingly to avoid dead code and shadowing.
🤖 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/lib/src/core/threshold/share_decryption.nr`:
- Around line 223-226: The commitment for d_native_trunc currently uses BIT_D
packing width causing rejection/ambiguity for values in the upper half; update
compute_threshold_decryption_share_commitment and any C6/C7 commitment
generation/checks to accept a native-width bit bound derived from the modulus
(i.e., pass a NATIVE_BIT_WIDTH parameter instead of BIT_D), ensure
verify_d_native_trunc_binding still enforces the native [0,q) constraint, and
propagate that same native bit-width through the Rust computation/codegen and
the C7 expected commitment comparisons so the hashing/packing uses the full
native modulus width consistently for d_native_trunc.
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 660-668: The code in ThresholdPlaintextAggregator that sets
self.decryption_aggregator_proofs = Some(Vec::new()) when
honest_c6_proofs_for_agg is present but empty (or contains empty inner proofs)
must not silently publish an empty aggregation-proof; instead, detect this
invalid state and return an error to abort aggregation. Replace the branch that
currently logs and sets decryption_aggregator_proofs to Some(Vec::new()) with a
failure return (propagate or construct an appropriate Err) that names the
problem (e.g., "missing C6 inner proofs for aggregation") so callers of
PlaintextAggregated cannot receive an unverifiable decryption_aggregator_proofs
when aggregation is enabled. Ensure the error path uses the function's existing
error type and preserves the info! log for diagnostics.
- Around line 767-771: When handling
ComputeResponseKind::Zk(ZkResponse::DecryptionAggregation(resp)) you must
validate that resp.proofs contains exactly the expected number of
DecryptionAggregator proofs before setting self.decryption_aggregator_proofs and
calling self.try_publish_complete(); check resp.proofs.len() against the
aggregator's expected count (e.g. the number of pending ciphertext/C7 jobs
tracked by this aggregator) and if it does not match, do not overwrite
self.decryption_aggregator_proofs, log/return an error, and leave
self.decryption_aggregation_correlation intact so a partial response is not
published.
---
Outside diff comments:
In `@circuits/lib/src/core/threshold/share_decryption.nr`:
- Around line 186-195: The payload() function is failing to include the
commitments because Noir Vec::push returns a new Vec which must be assigned;
change the two lines that call inputs.push(self.expected_sk_commitment) and
inputs.push(self.expected_e_sm_commitment) to assign their results back to
inputs (e.g., inputs = inputs.push(self.expected_sk_commitment); inputs =
inputs.push(self.expected_e_sm_commitment);), so the commitments
expected_sk_commitment and expected_e_sm_commitment are actually present before
calling flatten::<_, _, BIT_CT>(inputs, self.ct0) and flatten::<_, _,
BIT_CT>(inputs, self.ct1).
In `@crates/aggregator/src/threshold_plaintext_aggregator.rs`:
- Around line 512-575: The current loop in honest_shares only checks
proofs.first() and shares.first(), so only the first ciphertext/share is bound
and validated; update the logic to iterate over every proof/share pair for each
party_id (use c6_proofs.get(party_id) and the shares Vec in honest_shares) and
compare each proof's extracted "d_commitment" against the corresponding share's
computed commitment via compute_threshold_decryption_share_commitment (after
deserializing each share with e3_trbfv::helpers::try_poly_from_bytes and
converting to CrtPolynomial). If any pair mismatches, insert the party_id into
mismatched; otherwise accept that party only if all of its per-ciphertext pairs
match. Ensure you reference proofs[i].payload.proof.public_signals and shares[i]
(or equivalent index-aligned pairing) rather than .first().
In `@crates/tests/tests/integration.rs`:
- Around line 661-670: The match arm for EnclaveEventData::ComputeRequest uses
struct-like patterns `{ .. }` for tuple variants from ZkRequest which causes a
compile error; inside the matches! for ComputeRequestKind::Zk you should change
the patterns ZkRequest::NodeDkgFold { .. }, ZkRequest::DkgAggregation { .. },
and ZkRequest::DecryptionAggregation { .. } to tuple patterns like
ZkRequest::NodeDkgFold(_), ZkRequest::DkgAggregation(_), and
ZkRequest::DecryptionAggregation(_) so the variant constructors match their
actual definitions.
---
Nitpick comments:
In `@examples/CRISP/server/src/cli/commands.rs`:
- Around line 189-211: The first call to get_current_timestamp() that assigns
current_timestamp (used only for logging) is redundant because current_timestamp
is immediately shadowed by a second get_current_timestamp() before
window_start/input_window are computed; remove the earlier assignment (and the
mut on the original declaration if present) and keep the later fresh
get_current_timestamp() used to compute window_start and input_window (which
reference CONFIG.e3_duration), or alternatively remove the later shadowing and
use the initially fetched current_timestamp consistently—update occurrences of
current_timestamp, window_start, and input_window accordingly to avoid dead code
and shadowing.
🪄 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: 3cd58633-c01c-4367-a118-8a2e2e9d5328
📒 Files selected for processing (10)
circuits/bin/threshold/share_decryption/src/main.nrcircuits/lib/src/core/threshold/share_decryption.nrcrates/aggregator/src/threshold_plaintext_aggregator.rscrates/tests/tests/integration.rscrates/zk-helpers/src/circuits/threshold/share_decryption/computation.rscrates/zk-prover/tests/local_e2e_tests.rsexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonexamples/CRISP/server/.env.exampleexamples/CRISP/server/src/cli/commands.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- circuits/bin/threshold/share_decryption/src/main.nr
- examples/CRISP/packages/crisp-contracts/deployed_contracts.json
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 (3)
scripts/generate-verifiers.ts (1)
16-21:⚠️ Potential issue | 🟡 MinorStale example in header docstring.
The header still references the old
pk,foldcircuit names, which were removed in this PR (per thepackage.jsonchange todkg_aggregator,decryption_aggregator). The CLIshowHelp()examples at lines 415-416 were updated; please update this top-of-file usage block too for consistency.📝 Suggested doc fix
* Usage: * pnpm generate:verifiers # All circuits (or --circuits from package.json) - * pnpm generate:verifiers --circuits pk,fold # Specific circuits + * pnpm generate:verifiers --circuits dkg_aggregator,decryption_aggregator # Specific circuits * pnpm generate:verifiers --clean # Remove existing verifiers first * pnpm generate:verifiers --dry-run # Show what would be generated */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-verifiers.ts` around lines 16 - 21, Update the top-of-file usage docstring in scripts/generate-verifiers.ts to match the updated circuit names from package.json: replace the stale "pk,fold" example with "dkg_aggregator,decryption_aggregator" (so the line "pnpm generate:verifiers --circuits pk,fold" becomes "pnpm generate:verifiers --circuits dkg_aggregator,decryption_aggregator"), and ensure this header now matches the CLI showHelp() examples and any other occurrences of the old circuit names in the file.docs/pages/cryptography.mdx (2)
443-446:⚠️ Potential issue | 🟡 MinorStale reference to "wrapper circuits" under
recursive_aggregation/.
recursive_aggregation/wrapper/was removed in this PR; aggregation is now expressed as the ad-hoc bins (c2ab_fold,c3_fold,c3ab_fold,c4ab_fold,node_fold,nodes_fold,dkg_aggregator,decryption_aggregator,c6_fold). Consider rewording to reference the inner circuits and the new aggregation bins instead of "inner UltraHonk proofs, wrapper circuits, and the folding machinery", to avoid pointing readers at a directory that no longer exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/pages/cryptography.mdx` around lines 443 - 446, Update the sentence that mentions "inner UltraHonk proofs, wrapper circuits, and the folding machinery" to remove the stale "wrapper circuits" reference and instead point readers to the inner UltraHonk circuits and the new ad-hoc aggregation bins; specifically mention the aggregation bins like c2ab_fold, c3_fold, c3ab_fold, c4ab_fold, node_fold, nodes_fold, dkg_aggregator, decryption_aggregator, and c6_fold (under recursive_aggregation/) and keep the Noir Circuits link for how to build and verify them.
199-200:⚠️ Potential issue | 🟡 MinorStale "proposed design" note — this PR implements proof aggregation.
This PR adds the
dkg_aggregator/decryption_aggregatorcircuits and updates Solidity, Rust aggregators, and EVM helpers so the on-chain flow verifies a single aggregator proof per phase (DKG/decryption). The note describing the section as a "proposed design that has not been implemented yet" should be removed or rephrased to reflect the now-implemented architecture (with any open follow-ups, e.g.#1524/#1525, called out narrowly instead of blanket-disclaiming the whole section).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/pages/cryptography.mdx` around lines 199 - 200, Remove or update the stale "proposed design" note in docs/pages/cryptography.mdx that states the design "has not been implemented yet" — replace it with a concise statement that the dkg_aggregator and decryption_aggregator circuits plus the Solidity and Rust aggregators and EVM helpers are now implemented and that the on-chain flow verifies a single aggregator proof per phase; if necessary, briefly note outstanding follow-ups (e.g. `#1524`, `#1525`) rather than blanket-disclaiming the section.
♻️ Duplicate comments (2)
packages/enclave-contracts/contracts/Enclave.sol (1)
414-419:⚠️ Potential issue | 🔴 CriticalDecryption proof is still bound only to
keccak256(plaintextOutput).The verifier call does not include any E3-scoped public input (e.g.
e3.ciphertextOutput,e3.committeePublicKey, or an E3-bound domain separator), so a validDecryptionAggregatorproof for a different ciphertext/E3 that yields the same plaintext hash could be replayed here to complete this E3 and trigger reward distribution. TheIDecryptionVerifier.verify(bytes32, bytes)interface inIDecryptionVerifier.solwould need to be extended (or itspublicInputsframing tightened) to accept and check the E3-scoped values against the aggregator's public-input slots.This was previously flagged on an outdated commit; the current code path still has the same shape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/Enclave.sol` around lines 414 - 419, The decryption proof is currently verified only against keccak256(plaintextOutput) via e3.decryptionVerifier.verify, allowing replay across different E3 instances; extend the IDecryptionVerifier.verify interface (and its implementations) to accept E3-scoped public inputs (e.g. e3.ciphertextOutput, e3.committeePublicKey and/or an E3 domain separator) and update the call site in Enclave.sol (where e3.proofAggregationEnabled is checked and e3.decryptionVerifier.verify is invoked) to pass a tightly framed publicInputs bundle that includes those E3-specific values so the verifier can bind the aggregated proof to this exact ciphertext/E3 context before setting success.crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs (1)
187-213:⚠️ Potential issue | 🟠 MajorC0 still not correlated with the C1/C2/C3 chain — test name overpromises.
pk_bfv_data(line 197) is generated as an independent random sample viaPkCircuitData::generate_sample(preset), while the C1/C2 chain comes frompk_generation_sample_with_esiat lines 187‑188. The C0 proof at lines 204‑213 is therefore unrelated to the BFV public key used everywhere else in this "correlated" e2e, so the test cannot fail on a real C0↔chain mismatch (e.g. a C0 commitment for a different pk leaking through).Either:
- derive
pk_bfv_datafrom the BFV public key embedded inpk_gen, or- before constructing the
nfJSON, assertc0_proof'spk_commitmentpublic output equals the corresponding C2ab/C3ab/C4ab pk_commitment used downstream.This was raised previously and is still outstanding in the current code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs` around lines 187 - 213, The test currently generates pk_bfv_data via PkCircuitData::generate_sample which is independent from the BFV public key produced by pk_generation_sample_with_esi (pk_gen), so the C0 proof (c0_proof) is not correlated with the C1/C2 chain; fix by either deriving pk_bfv_data from pk_gen (replace PkCircuitData::generate_sample with a constructor that builds PkCircuitData from pk_gen’s BFV public key) or, if keeping the separate sample, add an assertion after proving C0 that c0_proof's pk_commitment public output equals the pk_commitment used downstream (e.g., the one in share_esm/share_sk or the C2ab/C3ab/C4ab data) before constructing the nf JSON to ensure the proof is genuinely correlated.
🧹 Nitpick comments (10)
packages/enclave-contracts/contracts/interfaces/IEnclave.sol (1)
428-433: Stale NatSpec forproofAggregationEnabledreferences removed wrapper/fold flow.The doc still says ciphernodes "generate and fold wrapper proofs" and that "wrapper/fold proofs are skipped" when disabled, but this PR removes the wrapper/fold trees in favor of the new
dkg_aggregator/decryption_aggregatorcircuits. Consider updating the wording to reflect the new aggregator-based flow (and the fact that C5/C7 verification on-chain is gated by this flag, sinceEnclave.sol::publishPlaintextOutputonly invokes the decryption verifier when the flag is true).📝 Suggested wording update
- /// `@notice` When true, ciphernodes generate and fold wrapper proofs - /// for DKG proof aggregation (public verifiability). When - /// false, wrapper/fold proofs are skipped to reduce latency. - /// C5 and C7 proofs are always generated and verified on-chain - /// regardless of this flag. + /// `@notice` When true, ciphernodes generate the DkgAggregator and + /// DecryptionAggregator proofs for public on-chain + /// verifiability of DKG and threshold decryption. When + /// false, aggregator proof generation/verification is + /// skipped to reduce latency. bool proofAggregationEnabled;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/interfaces/IEnclave.sol` around lines 428 - 433, The NatSpec for the boolean field proofAggregationEnabled is stale: replace references to "wrapper/fold proofs" with the current aggregator-based flow and clarify gating of on-chain verification; update the comment on proofAggregationEnabled to state that when true ciphernodes generate and fold proofs via the dkg_aggregator and decryption_aggregator circuits (and that publishPlaintextOutput only invokes the decryption verifier when this flag is true), and that C5/C7 verification remains performed on-chain regardless of the flag; modify the docstring near proofAggregationEnabled in IEnclave.sol to mention the new aggregator names (dkg_aggregator / decryption_aggregator) and the gating behavior tied to Enclave.sol::publishPlaintextOutput.crates/aggregator/src/publickey_aggregator.rs (1)
72-86: Remove unused carry-through state fields fromGeneratingC5Proof.
keyshare_bytesanddishonest_partiesare assigned when enteringGeneratingC5Proofbut never read afterward. They are threaded through every state mutation (try_receive_c5_proof,try_receive_dkg_node_proof,try_dispatch_dkg_aggregation,try_publish_complete,try_handle_dkg_aggregation_response) via destructure-and-reconstruct patterns without being used in the logic. These appear to be remnants from the removed pairwise-fold flow and can be dropped to reduce the persisted state size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/aggregator/src/publickey_aggregator.rs` around lines 72 - 86, Remove the unused carry-through fields keyshare_bytes and dishonest_parties from the GeneratingC5Proof enum variant and eliminate them from every place that destructures/reconstructs that state: update the constructor/transition that creates GeneratingC5Proof (where those fields were initially assigned) and adjust all state-match and rebuild sites inside try_receive_c5_proof, try_receive_dkg_node_proof, try_dispatch_dkg_aggregation, try_publish_complete, and try_handle_dkg_aggregation_response to stop extracting or reattaching keyshare_bytes and dishonest_parties; ensure no remaining references to those symbols exist and that the remaining fields (public_key, nodes, dkg_node_proofs, honest_party_ids, dkg_aggregation_correlation, dkg_aggregated_proof, c5_proof_pending, last_ec) are preserved in the reconstructed state.scripts/build-circuits.ts (1)
587-587: Stale "wrapper" terminology in comment.The PR removes wrapper circuits, but the comment for the
recursive/variant still describes the artifacts as "inner/base proofs fed into wrapper". Consider updating to reflect the new aggregation pipeline (e.g., "inner/base proofs consumed by the aggregation fold circuits").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-circuits.ts` at line 587, Update the stale comment that references "wrapper" in the recursive/ variant: replace "inner/base proofs fed into wrapper" with wording that reflects the new aggregation pipeline such as "inner/base proofs consumed by the aggregation fold circuits" (or similar wording) in the comment near the recursive/ variant in scripts/build-circuits.ts so it accurately describes the new aggregation flow.crates/zk-prover/src/circuits/aggregation/helpers.rs (2)
63-83: Guardslot_width == 0to avoid a%panic.If a future caller passes
slot_width = 0, the(v.len() - prefix_len) % slot_widthcheck panics on integer division. All current callers pass non-zero widths (3 or 4), but a tiny upfront guard makes this helper robust against future misuse.🛡️ Proposed change
) -> Result<Vec<String>, ZkError> { if proof.circuit != expected_circuit { return Err(ZkError::InvalidInput(format!( "expected {expected_circuit} proof, got {}", proof.circuit ))); } + if slot_width == 0 { + return Err(ZkError::InvalidInput( + "parse_acc_public_field_strings: slot_width must be > 0".into(), + )); + } let v = bytes_to_field_strings(proof.public_signals.as_ref())?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/circuits/aggregation/helpers.rs` around lines 63 - 83, The function parse_acc_public_field_strings should guard against slot_width == 0 to avoid a divide-by-zero panic: add an early validation in parse_acc_public_field_strings that returns Err(ZkError::InvalidInput(...)) when slot_width is zero (include a clear message like "slot_width must be non-zero"), then keep the existing checks (bytes_to_field_strings(proof.public_signals.as_ref())?, length vs prefix_len, and the modulo check) unchanged; reference parse_acc_public_field_strings and slot_width to locate the change.
32-59: Replace stringly-typedkindwith an enum.
kind: &strhere accepts only the magic strings"input"/"output"and falls through to a runtime error otherwise. Since this helper is only used internally (e.g. fromshare_encryption_inner_public_inputsinc3_accumulator.rs), promotingkindto a small enum (e.g.PublicSignalKind::{Input, Output}) eliminates the string-comparison branch, the unreachable error path, and the risk of typos at call sites — fully resolved at compile time.♻️ Proposed change
+pub enum PublicSignalKind { + Input, + Output, +} + /// Extract a single 32-byte public input/output by name. `kind` must be `"input"` or `"output"`. pub fn extract_single_field( proof: &Proof, - kind: &str, + kind: PublicSignalKind, name: &str, context: &str, ) -> Result<String, ZkError> { let bytes = match kind { - "input" => proof + PublicSignalKind::Input => proof .extract_input(name) .ok_or_else(|| ZkError::InvalidInput(format!("{context}: missing input {name}")))?, - "output" => proof + PublicSignalKind::Output => proof .extract_output(name) .ok_or_else(|| ZkError::InvalidInput(format!("{context}: missing output {name}")))?, - _ => { - return Err(ZkError::InvalidInput(format!( - "extract_single_field: kind must be input or output, got {kind}" - ))); - } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/circuits/aggregation/helpers.rs` around lines 32 - 59, extract_single_field currently takes a stringly-typed kind: &str and matches on "input"/"output", allowing runtime typos and unreachable branches; change the API to accept a small enum (e.g. PublicSignalKind with variants Input and Output), update extract_single_field signature to (proof: &Proof, kind: PublicSignalKind, name: &str, context: &str) and replace the string match with match PublicSignalKind::{Input,Output} to call proof.extract_input or proof.extract_output accordingly, and then update all call sites (for example share_encryption_inner_public_inputs in c3_accumulator.rs) to pass the enum variant instead of the string literal so the choice is enforced at compile time.crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs (2)
296-298: Hardcodedtotal_slots == 6assertion couples the test to a single preset.The constant
assert_eq!(total_slots, 6, ...)makes this test silently break (with a confusing assertion message) the momentBfvPreset::InsecureThreshold512orCiphernodesCommitteeSize::Microparameters move. Sincetotal_slotsis already derived dynamically from the compiledc3_fold.jsonABI just above, consider dropping the hard-coded6(the rest of the test already adapts viaslot_indices = (0..total_slots as u32).collect()), or computing the expected value fromN_PARTIES * L_THRESHOLDexposed by the preset rather than a literal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs` around lines 296 - 298, The test asserts total_slots == 6 via a hard-coded literal which couples it to a specific preset; instead either remove that rigid assert or compute the expected value from the preset constants and compare to total_slots. Locate the call to c3_fold_total_slots_from_compiled_json() and the assertion using total_slots, then replace assert_eq!(total_slots, 6, ...) with one of: (A) drop the assertion entirely since slot_indices already uses total_slots, or (B) derive expected_slots from the preset (e.g. use BfvPreset::InsecureThreshold512 and CiphernodesCommitteeSize::Micro semantics or compute expected_slots = N_PARTIES * L_THRESHOLD from the preset values) and assert_eq!(total_slots, expected_slots, "...") so the test adapts when those constants change.
132-148: Test silently passes when artifacts are missing.Three
returnpaths (lines 132‑135, 137‑144, 145‑148) cause the test to be skipped without failing wheneverbb,node_fold.json, orc3_fold.jsonaren't present, only printing aprintln!. That makes a CI environment that forgets to runpnpm build:circuits --group recursive_aggregationlook green even though the entire correlated path is untested. If this is intentional for local dev, consider gating with#[ignore = "..."]or acfg(feature = "circuit_artifacts")so the skip is visible in CI summaries instead of being indistinguishable from a real pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs` around lines 132 - 148, The test currently silently returns when artifacts are missing (in the find_bb() branch and when recursive_aggregation_compiled_json_path(CircuitName::NodeFold) or c3_fold_json_path() don't exist), so change those early-return branches to either (A) assert or panic with a clear message (e.g., replace the println! + return with panic!("missing artifact: ...") or assert!(path.exists(), "...")) so CI fails when artifacts are absent, or (B) gate the whole test with an explicit annotation like #[ignore = "requires circuit artifacts"] or cfg(feature = "circuit_artifacts") so the skip is visible and deliberate; update the checks around find_bb(), recursive_aggregation_compiled_json_path(CircuitName::NodeFold), and c3_fold_json_path() accordingly.crates/zk-prover/src/circuits/aggregation/c3_accumulator.rs (2)
39-92: Document why the kernel proof usesslot_index = 0.Line 69 hardcodes
slot_index: 0in the kernel witness, but the kernel runs with the full first inner proof'sc3_public_inputsand writes them at slot 0 of its public output. This is correct only because the immediate nextc3_foldstep runs withis_first_step = true, in which the siblingc3_foldcircuit ignoresacc_public_inputsentirely and re-derives the slot row from the inner proof. A short comment near line 69 stating "kernel slot index is irrelevant — first c3_fold step has is_first_step=true and ignores acc_public_inputs" would prevent a future contributor from "fixing" this to the realslot_indexand breaking the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/circuits/aggregation/c3_accumulator.rs` around lines 39 - 92, In generate_c3_fold_kernel_genesis_proof, add a brief comment next to the slot_index: 0 assignment explaining that the kernel's slot_index value is irrelevant here because the kernel is fed the full first inner proof's c3_public_inputs and the immediate next C3 fold step runs with is_first_step = true and therefore ignores acc_public_inputs and re-derives the slot row from the inner proof; reference the slot_index field and is_first_step boolean so future contributors won't change slot_index to the "real" value and break the contract.
84-92: Don't silently swallow kernel-cleanup errors.
let _ = prover.cleanup(job_id);discards any cleanup failure for the kernel work directory. If cleanup fails (disk pressure, permissions, etc.) the temp artifacts accumulate silently across E3 runs. At minimum, log the error atwarnlevel so operators see it; bubbling it up to the caller is also reasonable since the proof itself has already been produced.♻️ Proposed change
- let _ = prover.cleanup(job_id); + if let Err(e) = prover.cleanup(job_id) { + tracing::warn!(error = %e, job_id, "c3_fold kernel cleanup failed"); + } Ok(proof)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/circuits/aggregation/c3_accumulator.rs` around lines 84 - 92, The code currently swallows errors from prover.cleanup(job_id) after generating the proof (in the generate_recursive_aggregation_bin_proof -> prover.cleanup sequence), which lets temp kernel artifacts accumulate; change the cleanup call to handle errors instead of ignoring them: call prover.cleanup(job_id) and if it returns Err, either log the error at warn level via the existing logger or propagate the error to the caller (return Err) so failures are visible; ensure this change is made in the function that calls prover.generate_recursive_aggregation_bin_proof (referencing prover.cleanup, job_id, and CircuitName::C3FoldKernel) and keep the successful Ok(proof) return only after handling or forwarding the cleanup error.circuits/bin/recursive_aggregation/nodes_fold_kernel/src/main.nr (1)
15-27: Unused intermediate constants kept for ABI parity?
C2_PUBLIC_LEN,C2AB_FOLD_PUBLIC_LEN,C3_SLOTS,C3AB_FOLD_PUBLIC_LEN,C4_PUBLIC_LEN,C4AB_FOLD_PUBLIC_LENare declaredpub globalhere but not referenced in this kernel'smain. If these are kept solely for parity withnodes_fold(or for external Noir consumers), a brief comment would help future readers; otherwise consider trimming the unused ones to reduce drift risk if the sibling layout changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@circuits/bin/recursive_aggregation/nodes_fold_kernel/src/main.nr` around lines 15 - 27, Several public global constants (C2_PUBLIC_LEN, C2AB_FOLD_PUBLIC_LEN, C3_SLOTS, C3AB_FOLD_PUBLIC_LEN, C4_PUBLIC_LEN, C4AB_FOLD_PUBLIC_LEN) are declared but not used in this kernel's main; either remove them or document why they remain for ABI parity. Fix by either (a) deleting the unused pub global declarations, or (b) adding a concise comment above those declarations stating they are intentionally kept for ABI/layout parity with sibling kernels or external consumers, and ensure NODE_FOLD_PUBLIC_LEN and NODES_FOLD_PUBLIC_LEN (the actually used symbols) remain unchanged; choose one approach and apply it consistently to avoid future drift.
🤖 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/aggregator/src/publickey_aggregator.rs`:
- Around line 627-662: The code currently only checks key presence via
all_honest_proofs_present but then drops None values when building pairs,
allowing silent truncation; change the guard in publickey_aggregator.rs (around
all_honest_proofs_present, dkg_node_proofs, pairs, node_fold_proofs) to
explicitly verify that for every honest_party_id dkg_node_proofs.get(id) is
Some(Some(_)) (i.e., the map contains the key and the value is Some), and if any
honest id maps to None either return early (same as the all-None branch) or
return an error/emit a clear log and skip dispatch — do not proceed to
filter_map and dispatch when there is a mixed Some/None state. Ensure the new
check replaces or augments the existing all_honest_proofs_present logic and add
a clear info/error log indicating mixed proofs when you early-return.
In `@packages/enclave-contracts/tasks/enclave.ts`:
- Around line 318-327: The task must validate that publicKey and pkCommitment
are consistent before calling publishCommittee: when proof aggregation is
disabled (proof equals 0x or equivalent), ensure publicKey is provided and
pkCommitment is a non-zero bytes32 and equals the hash of publicKey (e.g.,
keccak256(publicKey)); reject or normalize malformed/zero pkCommitment values
instead of just checking falsiness. Update the caller that constructs the
arguments for publishCommittee to perform this check (reference variables
publicKey, pkCommitment, proof and the publishCommittee invocation) and throw a
clear error or exit when the consistency check fails.
---
Outside diff comments:
In `@docs/pages/cryptography.mdx`:
- Around line 443-446: Update the sentence that mentions "inner UltraHonk
proofs, wrapper circuits, and the folding machinery" to remove the stale
"wrapper circuits" reference and instead point readers to the inner UltraHonk
circuits and the new ad-hoc aggregation bins; specifically mention the
aggregation bins like c2ab_fold, c3_fold, c3ab_fold, c4ab_fold, node_fold,
nodes_fold, dkg_aggregator, decryption_aggregator, and c6_fold (under
recursive_aggregation/) and keep the Noir Circuits link for how to build and
verify them.
- Around line 199-200: Remove or update the stale "proposed design" note in
docs/pages/cryptography.mdx that states the design "has not been implemented
yet" — replace it with a concise statement that the dkg_aggregator and
decryption_aggregator circuits plus the Solidity and Rust aggregators and EVM
helpers are now implemented and that the on-chain flow verifies a single
aggregator proof per phase; if necessary, briefly note outstanding follow-ups
(e.g. `#1524`, `#1525`) rather than blanket-disclaiming the section.
In `@scripts/generate-verifiers.ts`:
- Around line 16-21: Update the top-of-file usage docstring in
scripts/generate-verifiers.ts to match the updated circuit names from
package.json: replace the stale "pk,fold" example with
"dkg_aggregator,decryption_aggregator" (so the line "pnpm generate:verifiers
--circuits pk,fold" becomes "pnpm generate:verifiers --circuits
dkg_aggregator,decryption_aggregator"), and ensure this header now matches the
CLI showHelp() examples and any other occurrences of the old circuit names in
the file.
---
Duplicate comments:
In `@crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs`:
- Around line 187-213: The test currently generates pk_bfv_data via
PkCircuitData::generate_sample which is independent from the BFV public key
produced by pk_generation_sample_with_esi (pk_gen), so the C0 proof (c0_proof)
is not correlated with the C1/C2 chain; fix by either deriving pk_bfv_data from
pk_gen (replace PkCircuitData::generate_sample with a constructor that builds
PkCircuitData from pk_gen’s BFV public key) or, if keeping the separate sample,
add an assertion after proving C0 that c0_proof's pk_commitment public output
equals the pk_commitment used downstream (e.g., the one in share_esm/share_sk or
the C2ab/C3ab/C4ab data) before constructing the nf JSON to ensure the proof is
genuinely correlated.
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 414-419: The decryption proof is currently verified only against
keccak256(plaintextOutput) via e3.decryptionVerifier.verify, allowing replay
across different E3 instances; extend the IDecryptionVerifier.verify interface
(and its implementations) to accept E3-scoped public inputs (e.g.
e3.ciphertextOutput, e3.committeePublicKey and/or an E3 domain separator) and
update the call site in Enclave.sol (where e3.proofAggregationEnabled is checked
and e3.decryptionVerifier.verify is invoked) to pass a tightly framed
publicInputs bundle that includes those E3-specific values so the verifier can
bind the aggregated proof to this exact ciphertext/E3 context before setting
success.
---
Nitpick comments:
In `@circuits/bin/recursive_aggregation/nodes_fold_kernel/src/main.nr`:
- Around line 15-27: Several public global constants (C2_PUBLIC_LEN,
C2AB_FOLD_PUBLIC_LEN, C3_SLOTS, C3AB_FOLD_PUBLIC_LEN, C4_PUBLIC_LEN,
C4AB_FOLD_PUBLIC_LEN) are declared but not used in this kernel's main; either
remove them or document why they remain for ABI parity. Fix by either (a)
deleting the unused pub global declarations, or (b) adding a concise comment
above those declarations stating they are intentionally kept for ABI/layout
parity with sibling kernels or external consumers, and ensure
NODE_FOLD_PUBLIC_LEN and NODES_FOLD_PUBLIC_LEN (the actually used symbols)
remain unchanged; choose one approach and apply it consistently to avoid future
drift.
In `@crates/aggregator/src/publickey_aggregator.rs`:
- Around line 72-86: Remove the unused carry-through fields keyshare_bytes and
dishonest_parties from the GeneratingC5Proof enum variant and eliminate them
from every place that destructures/reconstructs that state: update the
constructor/transition that creates GeneratingC5Proof (where those fields were
initially assigned) and adjust all state-match and rebuild sites inside
try_receive_c5_proof, try_receive_dkg_node_proof, try_dispatch_dkg_aggregation,
try_publish_complete, and try_handle_dkg_aggregation_response to stop extracting
or reattaching keyshare_bytes and dishonest_parties; ensure no remaining
references to those symbols exist and that the remaining fields (public_key,
nodes, dkg_node_proofs, honest_party_ids, dkg_aggregation_correlation,
dkg_aggregated_proof, c5_proof_pending, last_ec) are preserved in the
reconstructed state.
In `@crates/zk-prover/src/circuits/aggregation/c3_accumulator.rs`:
- Around line 39-92: In generate_c3_fold_kernel_genesis_proof, add a brief
comment next to the slot_index: 0 assignment explaining that the kernel's
slot_index value is irrelevant here because the kernel is fed the full first
inner proof's c3_public_inputs and the immediate next C3 fold step runs with
is_first_step = true and therefore ignores acc_public_inputs and re-derives the
slot row from the inner proof; reference the slot_index field and is_first_step
boolean so future contributors won't change slot_index to the "real" value and
break the contract.
- Around line 84-92: The code currently swallows errors from
prover.cleanup(job_id) after generating the proof (in the
generate_recursive_aggregation_bin_proof -> prover.cleanup sequence), which lets
temp kernel artifacts accumulate; change the cleanup call to handle errors
instead of ignoring them: call prover.cleanup(job_id) and if it returns Err,
either log the error at warn level via the existing logger or propagate the
error to the caller (return Err) so failures are visible; ensure this change is
made in the function that calls prover.generate_recursive_aggregation_bin_proof
(referencing prover.cleanup, job_id, and CircuitName::C3FoldKernel) and keep the
successful Ok(proof) return only after handling or forwarding the cleanup error.
In `@crates/zk-prover/src/circuits/aggregation/helpers.rs`:
- Around line 63-83: The function parse_acc_public_field_strings should guard
against slot_width == 0 to avoid a divide-by-zero panic: add an early validation
in parse_acc_public_field_strings that returns Err(ZkError::InvalidInput(...))
when slot_width is zero (include a clear message like "slot_width must be
non-zero"), then keep the existing checks
(bytes_to_field_strings(proof.public_signals.as_ref())?, length vs prefix_len,
and the modulo check) unchanged; reference parse_acc_public_field_strings and
slot_width to locate the change.
- Around line 32-59: extract_single_field currently takes a stringly-typed kind:
&str and matches on "input"/"output", allowing runtime typos and unreachable
branches; change the API to accept a small enum (e.g. PublicSignalKind with
variants Input and Output), update extract_single_field signature to (proof:
&Proof, kind: PublicSignalKind, name: &str, context: &str) and replace the
string match with match PublicSignalKind::{Input,Output} to call
proof.extract_input or proof.extract_output accordingly, and then update all
call sites (for example share_encryption_inner_public_inputs in
c3_accumulator.rs) to pass the enum variant instead of the string literal so the
choice is enforced at compile time.
In `@crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs`:
- Around line 296-298: The test asserts total_slots == 6 via a hard-coded
literal which couples it to a specific preset; instead either remove that rigid
assert or compute the expected value from the preset constants and compare to
total_slots. Locate the call to c3_fold_total_slots_from_compiled_json() and the
assertion using total_slots, then replace assert_eq!(total_slots, 6, ...) with
one of: (A) drop the assertion entirely since slot_indices already uses
total_slots, or (B) derive expected_slots from the preset (e.g. use
BfvPreset::InsecureThreshold512 and CiphernodesCommitteeSize::Micro semantics or
compute expected_slots = N_PARTIES * L_THRESHOLD from the preset values) and
assert_eq!(total_slots, expected_slots, "...") so the test adapts when those
constants change.
- Around line 132-148: The test currently silently returns when artifacts are
missing (in the find_bb() branch and when
recursive_aggregation_compiled_json_path(CircuitName::NodeFold) or
c3_fold_json_path() don't exist), so change those early-return branches to
either (A) assert or panic with a clear message (e.g., replace the println! +
return with panic!("missing artifact: ...") or assert!(path.exists(), "...")) so
CI fails when artifacts are absent, or (B) gate the whole test with an explicit
annotation like #[ignore = "requires circuit artifacts"] or cfg(feature =
"circuit_artifacts") so the skip is visible and deliberate; update the checks
around find_bb(),
recursive_aggregation_compiled_json_path(CircuitName::NodeFold), and
c3_fold_json_path() accordingly.
In `@packages/enclave-contracts/contracts/interfaces/IEnclave.sol`:
- Around line 428-433: The NatSpec for the boolean field proofAggregationEnabled
is stale: replace references to "wrapper/fold proofs" with the current
aggregator-based flow and clarify gating of on-chain verification; update the
comment on proofAggregationEnabled to state that when true ciphernodes generate
and fold proofs via the dkg_aggregator and decryption_aggregator circuits (and
that publishPlaintextOutput only invokes the decryption verifier when this flag
is true), and that C5/C7 verification remains performed on-chain regardless of
the flag; modify the docstring near proofAggregationEnabled in IEnclave.sol to
mention the new aggregator names (dkg_aggregator / decryption_aggregator) and
the gating behavior tied to Enclave.sol::publishPlaintextOutput.
In `@scripts/build-circuits.ts`:
- Line 587: Update the stale comment that references "wrapper" in the recursive/
variant: replace "inner/base proofs fed into wrapper" with wording that reflects
the new aggregation pipeline such as "inner/base proofs consumed by the
aggregation fold circuits" (or similar wording) in the comment near the
recursive/ variant in scripts/build-circuits.ts so it accurately describes the
new aggregation flow.
🪄 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: 02d6e2d7-219f-4216-9855-f493e059bf61
📒 Files selected for processing (39)
agent/flow-trace/04_DKG_AND_COMPUTATION.mdcircuits/bin/recursive_aggregation/c3_fold_kernel/src/main.nrcircuits/bin/recursive_aggregation/c6_fold_kernel/src/main.nrcircuits/bin/recursive_aggregation/decryption_aggregator/src/main.nrcircuits/bin/recursive_aggregation/node_fold/src/main.nrcircuits/bin/recursive_aggregation/nodes_fold_kernel/src/main.nrcrates/aggregator/src/publickey_aggregator.rscrates/dashboard/src/dashboard.htmlcrates/events/src/enclave_event/keyshare_created.rscrates/zk-prover/src/actors/commitment_links/c4a_to_c6.rscrates/zk-prover/src/actors/commitment_links/c4b_to_c6.rscrates/zk-prover/src/circuits/aggregation/c3_accumulator.rscrates/zk-prover/src/circuits/aggregation/c6_accumulator.rscrates/zk-prover/src/circuits/aggregation/helpers.rscrates/zk-prover/src/circuits/aggregation/mod.rscrates/zk-prover/src/circuits/aggregation/node_dkg_fold.rscrates/zk-prover/src/circuits/aggregation/nodes_fold_accumulator.rscrates/zk-prover/tests/node_fold_correlated_e2e_tests.rsdocs/pages/cryptography.mdxdocs/pages/noir-circuits.mdxdocs/pages/tutorials/deploy-to-testnet.mdxexamples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonpackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/contracts/verifiers/bfv/honk/DecryptionAggregatorVerifier.solpackages/enclave-contracts/contracts/verifiers/bfv/honk/DkgAggregatorVerifier.solpackages/enclave-contracts/tasks/enclave.tspackages/enclave-contracts/test/BfvPkVerifier.spec.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tsscripts/build-circuits.tsscripts/generate-verifiers.tsscripts/lint-circuits.sh
✅ Files skipped from review due to trivial changes (6)
- docs/pages/tutorials/deploy-to-testnet.mdx
- packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json
- crates/dashboard/src/dashboard.html
- docs/pages/noir-circuits.mdx
- packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json
- examples/CRISP/enclave.config.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/zk-prover/src/circuits/aggregation/mod.rs
- scripts/lint-circuits.sh
- crates/zk-prover/src/circuits/aggregation/nodes_fold_accumulator.rs
- crates/zk-prover/src/circuits/aggregation/node_dkg_fold.rs
- crates/zk-prover/src/circuits/aggregation/c6_accumulator.rs
- circuits/bin/recursive_aggregation/node_fold/src/main.nr
- circuits/bin/recursive_aggregation/decryption_aggregator/src/main.nr
|
Did a Quick review with Claude. Here are the items to verify and confirm On-chain BFV wrappers do not pin intermediate VK hashescircuits/bin/recursive_aggregation/dkg_aggregator/src/main.nr declares BfvPkVerifier.verify only validates two things:
Nowhere does it compare Every
So a prover can generate a "valid" Critical layout constants are duplicated across circuits with "Must match" commentsThe same
Every one of these is derived from the same handful of inputs (
|
That are two separate follow-up tasks for this PR defined in #1524 and #1525
True, let's keep as it is tho. Needs more refactoring as there are some circular dependencies on default imports if we use a unified file.
True, tho it wouldn't make any sense for a node to do that and then be unable to fold those proofs. All those proofs are generated at the same step. |
- Add BIT_D_NATIVE / NATIVE_BIT_WIDTH for C6 d_commitment and C7 share commitment checks - Introduce compute_native_crt_coeff_bit and C6 Bits.d_native_bit; C7 uses d_native_bit only - Keep THRESHOLD_SHARE_DECRYPTION_BIT_D for centered d in Fiat–Shamir payload - Regenerate threshold.nr globals, codegen, aggregator, and local e2e expectation
5 participants
Summary
Refactors proof aggregation around ad-hoc Noir bins under
circuits/bin/recursive_aggregation/, replacing the old genericfold/andwrapper/trees. Aggregation is expressed as explicit fold + aggregator circuits with clear public layouts and key hashes.Recursive aggregation layout & circuits
c2ab_foldc3_foldShareEncryption+ optional priorc3_foldnon-ZK proof (is_first_step,slot_index).c3ab_foldc3_foldoutputs.c4ab_foldnode_foldc2ab_fold,c3ab_fold,c4ab_foldwith same-party assertions between stages.nodes_foldHnode_foldproofs (one honest slot per step), chained with priornodes_fold.dkg_aggregatornodes_foldproof + C5 (pk_aggregation) ZK; enforces cross-node grids and C5↔node public links.c6_foldT+1C6 (ThresholdShareDecryption) rows for the phase-7 path.decryption_aggregatorc6_foldproof + C7 ZK; ties folded C6 columns toc7_public.Removed:
recursive_aggregation/fold/andrecursive_aggregation/wrapper/(legacy two-proof fold and generic wrappers).How reviewers can test
Compile recursive-aggregation circuits (needed for zk-prover tests that read
circuits/bin/...):Barretenberg — integration tests expect
bbonPATH; they print a skip message if it’s missing.Optional zk-prover integration targets (not run by default CI for this crate yet):
CI still runs
e3-zk-prover’sintegration_testsandlocal_e2e_testsonly; these two are for local / follow-up CI if you wire them in.What those zk-prover tests cover
fold_accumulators_e2e_testsgenerate_sequential_c3_fold/generate_sequential_c6_fold(prove + verify), slot counts from compiledc3_fold/c6_foldJSON, staging of recursive-aggregation artifacts, and loading the node_fold pipeline (C2ab → C3ab → C4ab →node_fold) without a full correlated DKG witness story.node_fold_correlated_e2e_testsnode_foldprove + verify.Ciphernode flow & on-chain verifier refactor
The latest commit aligns the Solidity stack, the Rust aggregators and the indexer/EVM helpers with the new
dkg_aggregator/decryption_aggregatorlayout. The on-chain flow is simpler: one EVM proof per phase, verified by a single aggregator verifier that internally checks the node/c6 folds and the C5/C7 sub-proofs.Before vs. after
main)IPkVerifier.verify(proof, foldProof) → pkCommitment— on-chain verified C5 proof + optional RecursiveAggregationFold proof; commitment extracted as last public input.IPkVerifier.verify(pkCommitment, proof) → bool— on-chain verifies oneDkgAggregatorproof and binds it to a caller-suppliedpkCommitment.IDecryptionVerifier.verify(plaintextHash, proof, foldProof) → bool— verified C7 + optional fold proof.IDecryptionVerifier.verify(plaintextHash, proof) → bool— verifies oneDecryptionAggregatorproof and binds its exposed plaintext toplaintextHash.publishCommittee(e3Id, nodes, publicKey, proof, foldProof)— registry extracted the commitment fromproofpublic inputs.publishCommittee(e3Id, nodes, publicKey, pkCommitment, proof)—pkCommitmentis explicit;foldProofdropped; eventCommitteePublishednow includespkCommitment.publishPlaintextOutput(e3Id, plaintextOutput, proof, foldProof)publishPlaintextOutput(e3Id, plaintextOutput, proof)e3.proofAggregationEnabled == false,proofmay be empty andpkCommitmentis trusted from the (signed) aggregator.This PR requires working on two follow-up issues:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation