feat: add chunked C2 proving [skip-line-limit] - #1772
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughChangesThe PR replaces monolithic C2 proving with chunked circuits, batching, terminal finalization, root commitments, and recursive VK binding. It also adds recipient-aware DKG validation, revised ciphertext publication guards, and proposal-scoped refund and slashing behavior. Chunked C2 proving — Recursive verification binding — Committee and settlement lifecycle — Validation and tooling — Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
0xjei
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
crates/zk-prover/src/lib.rs (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider keeping the fold steps internal.
generate_sequential_c2_chunk_foldandfinalize_c2_chunk_foldare the two halves of the flow thatprove_chunked_share_computationalready orchestrates. Exporting them lets callers combine a fold and a finalizer that do not match, and the validation for that combination lives only inside each function. If no external caller needs the steps, restrict them topub(crate).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/lib.rs` around lines 30 - 32, Restrict the re-exports of generate_sequential_c2_chunk_fold and finalize_c2_chunk_fold in the crate root to pub(crate), keeping both fold steps internal while preserving their use by prove_chunked_share_computation.crates/zk-prover/src/circuits/aggregation/c2_chunk_accumulator.rs (3)
424-443: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant length check and the identity match.
Line 424 checks
len() <= 2 * chunk_count. Thechecked_subon lines 429-432 already covers that case with a stricter bound, and both produce the same error text. The match on lines 439-443 maps each variant to itself;accumulator.circuitis already validated on lines 409-416.♻️ Proposed simplification
- let acc_public_inputs = bytes_to_field_strings(accumulator.public_signals.as_ref())?; - if acc_public_inputs.len() <= 2 * chunk_count { - return Err(ZkError::InvalidInput( - "C2 chunk accumulator layout is too short".into(), - )); - } - let c2_public_len = acc_public_inputs + let acc_public_inputs = bytes_to_field_strings(accumulator.public_signals.as_ref())?; + let c2_public_len = acc_public_inputs .len() .checked_sub(C2_CHUNK_FOLD_PUBLIC_PREFIX_LEN + (2 * chunk_count)) .ok_or_else(|| ZkError::InvalidInput("C2 chunk accumulator layout is too short".into()))?; @@ - let acc_vk = match accumulator.circuit { - CircuitName::C2ChunkFold => CircuitName::C2ChunkFold, - CircuitName::C2ChunkFoldKernel => CircuitName::C2ChunkFoldKernel, - _ => unreachable!("circuit checked above"), - }; let acc_vk = vk::load_vk_artifacts( &prover.circuits_dir(CircuitVariant::Default, artifacts_dir), - acc_vk, + accumulator.circuit, )?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/c2_chunk_accumulator.rs` around lines 424 - 443, Remove the redundant acc_public_inputs.len() <= 2 * chunk_count validation and rely on the existing checked_sub error path for layout validation. Simplify the acc_vk assignment by using the already validated accumulator.circuit directly, eliminating the identity match while preserving the surrounding circuit validation.
18-18: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBind the prefix length to the circuit definition.
C2_CHUNK_FOLD_PUBLIC_PREFIX_LENduplicates the Noir globalC2_CHUNK_FOLD_PREFIX_LENin the terminal and fold circuits. If the circuit prefix changes, this constant drifts silently, and every length check in this file validates the wrong layout. Add a comment that names the Noir source of truth, or derive the value from a shared generated constant.♻️ Minimum change: document the source of truth
+/// Must equal `C2_CHUNK_FOLD_PREFIX_LEN` in +/// `circuits/bin/recursive_aggregation/c2_chunk_fold/src/main.nr`. const C2_CHUNK_FOLD_PUBLIC_PREFIX_LEN: usize = 6;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/c2_chunk_accumulator.rs` at line 18, Document that C2_CHUNK_FOLD_PUBLIC_PREFIX_LEN must match the Noir global C2_CHUNK_FOLD_PREFIX_LEN defined in the terminal and fold circuits, identifying that global as the source of truth.
253-285: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftUse the kernel proof as the initial accumulator.
C2ChunkFoldignores the kernel state whenis_first_stepis true, so the first chunk is not folded twice. However, the current code verifies the base and first chunk inc2_chunk_fold_kernel, then verifies them again inC2ChunkFoldand discards the kernel state.Generate the kernel proof once, use it as
prior, and runC2ChunkFoldonly for the remaining chunks. The finalizer already acceptsC2ChunkFoldKernel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/c2_chunk_accumulator.rs` around lines 253 - 285, Update the first-step handling around is_first_step and generate_kernel_genesis_proof so the kernel proof becomes the initial accumulator/prior state instead of being discarded. Ensure C2ChunkFold is invoked only for subsequent chunks, while preserving the existing public-input validation and finalizer flow that accepts C2ChunkFoldKernel.scripts/build-circuits.ts (1)
714-719: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe exclusion also forces an unused EVM verification key.
generateVkusesisFoldOrAggregationfor two separate decisions. The else branch produces thenoir-recursiveVK that the finalize circuits need, but it also runsrunWriteVk('evm', ...)and throws when that fails. The finalize circuits are never published on-chain, becauseisAggregationWithEvmOnChainreturns false for them. The build therefore spends time on an EVM VK that nothing consumes, and an EVM VK failure blocks the whole build.Split the two decisions. Add a predicate such as
needsNoirRecursiveVk(circuit)and gate the EVM VK onisAggregationWithEvmOnChainfor aggregation-group circuits.The method name is also now inaccurate. It returns false for circuits that are in the aggregation group. Rename it to describe the VK profile it selects, for example
usesNonZkAccumulatorVkProfile.♻️ Sketch of the split
- private isFoldOrAggregation(circuit: CircuitInfo): boolean { - // C2 terminal projections are ZK leaves for C2abFold, not non-ZK accumulators. - const name = basename(circuit.path) - return circuit.group === CIRCUIT_GROUPS.AGGREGATION && - !['c2_chunk_finalize', 'sk_c2_chunk_finalize', 'esm_c2_chunk_finalize'].includes(name) - } + /** C2 terminal projections are ZK leaves for C2abFold, not non-ZK accumulators. */ + private static readonly ZK_LEAF_AGGREGATION_CIRCUITS = new Set([ + 'c2_chunk_finalize', + 'sk_c2_chunk_finalize', + 'esm_c2_chunk_finalize', + ]) + + /** True when the circuit only needs the non-ZK accumulator VK profile. */ + private usesNonZkAccumulatorVkProfile(circuit: CircuitInfo): boolean { + return ( + circuit.group === CIRCUIT_GROUPS.AGGREGATION && + !NoirCircuitBuilder.ZK_LEAF_AGGREGATION_CIRCUITS.has(basename(circuit.path)) + ) + } + + /** True when an EVM VK must be produced for this circuit. */ + private needsEvmVk(circuit: CircuitInfo): boolean { + return circuit.group !== CIRCUIT_GROUPS.AGGREGATION || this.isAggregationWithEvmOnChain(circuit) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-circuits.ts` around lines 714 - 719, Split the VK decisions in generateVk: add a needsNoirRecursiveVk(circuit) predicate for selecting the noir-recursive profile, and gate runWriteVk('evm', ...) for aggregation circuits with isAggregationWithEvmOnChain so finalize circuits do not generate or require an unused EVM VK. Rename isFoldOrAggregation to a profile-oriented name such as usesNonZkAccumulatorVkProfile and update all callers.crates/zk-prover/src/circuits/dkg/share_computation.rs (2)
105-127: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLoad the chunk circuit once, outside the loop.
circuit_pathandCompiledCircuit::from_filerun on every iteration. The compiled ACIR JSON is re-read and re-parsed once per chunk. For a degree of 32768 with the default chunk size of 512, that is 64 loads of the same file.Hoist both out of the loop.
♻️ Proposed refactor
+ let chunk_circuit_path = prover + .circuits_dir(e3_events::CircuitVariant::Recursive, artifacts_dir) + .join(CircuitName::ShareComputationChunk.dir_path()) + .join(format!( + "{}.json", + CircuitName::ShareComputationChunk.as_str() + )); + let compiled = crate::witness::CompiledCircuit::from_file(&chunk_circuit_path)?; let mut chunks = Vec::with_capacity(chunk_count); let mut indices = Vec::with_capacity(chunk_count); for chunk_idx in 0..chunk_count { @@ let input_map = inputs_json_to_input_map(&Value::Object(chunk_json))?; - let circuit_path = prover - .circuits_dir(e3_events::CircuitVariant::Recursive, artifacts_dir) - .join(CircuitName::ShareComputationChunk.dir_path()) - .join(format!( - "{}.json", - CircuitName::ShareComputationChunk.as_str() - )); - let compiled = crate::witness::CompiledCircuit::from_file(&circuit_path)?; let witness = crate::witness::WitnessGenerator::new().generate_witness(&compiled, input_map)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/dkg/share_computation.rs` around lines 105 - 127, Hoist the `circuit_path` construction and `CompiledCircuit::from_file` call out of the chunk-processing loop so the shared `compiled` circuit is loaded once before iterating over `chunk_idx`. Keep each iteration’s input-map creation and witness generation in the loop, reusing the preloaded circuit.
92-101: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winValidate
ybefore the base proof is generated.The check on the coefficient count runs after
prove_recursive_circuitproduced the base proof. A malformed or missingytherefore costs one full recursive proof before the error is returned. Move theyextraction and the length check above the base proving step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/dkg/share_computation.rs` around lines 92 - 101, Move the y extraction and degree-length validation in the share computation flow to before the prove_recursive_circuit base-proof generation call. Preserve the existing missing-y serialization error and coefficient-count InvalidInput error, and only invoke prove_recursive_circuit after y has passed both checks.crates/events/src/interfold_event/signed_proof.rs (1)
60-67: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover both chunk-finalizer circuits in the test helper.
Production consumers use membership checks against
proof.circuit. However,workflow_tests/mod.rs:33uses[0]for C2a and C2b proofs, so the tests never constructSkC2ChunkFinalizeorESmC2ChunkFinalize. Select circuits explicitly or add cases for both entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/events/src/interfold_event/signed_proof.rs` around lines 60 - 67, Update the test helper that selects circuits for C2a and C2b proofs to cover both entries in each corresponding circuit list, including SkC2ChunkFinalize and ESmC2ChunkFinalize. Avoid relying only on the first element; select each circuit explicitly or add separate test cases while preserving existing proof construction behavior.crates/events/src/interfold_event/proof.rs (1)
161-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
C2ChunkFinalizevariant and circuit package.Only
SkC2ChunkFinalizeandESmC2ChunkFinalizeare used by the chunked C2 proving path. Remove the untyped variant, its mappings, andcircuits/bin/recursive_aggregation/c2_chunk_finalizeto avoid compiling and generating verification keys for an unused circuit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/events/src/interfold_event/proof.rs` around lines 161 - 162, Remove the unused C2ChunkFinalize variant from the relevant proof enum, then delete its associated mappings and references while preserving SkC2ChunkFinalize and ESmC2ChunkFinalize. Remove the circuits/bin/recursive_aggregation/c2_chunk_finalize circuit package and any build or generation entries that compile it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md`:
- Around line 319-327: Update the chunked C2 documentation near the statement
that each chunk has 512 coefficients to clarify that 512 is the default chunk
size, while noting that --chunk-size allows any nonzero divisor of the preset
polynomial degree.
In `@circuits/bin/recursive_aggregation/c2_chunk_finalize/src/main.nr`:
- Around line 20-26: Bind every nested proof to its expected circuit identity
and variant. In c2_chunk_finalize/src/main.nr (lines 20-26), assert acc_key_hash
matches the computed C2ChunkFold accumulator circuit hash before projecting
state; in c2_chunk_fold/src/main.nr (lines 39-63), bind base_key_hash and
chunk_key_hash to C2Base and C2Chunk hashes, bind non-initial acc_key_hash to
C2ChunkFold, and propagate SK/ESM variant selection through accumulator and
finalizer; in sk_c2_chunk_finalize/src/main.nr (line 29), apply the same
identity check and pass the variant through accumulator verification to enforce
SK-only folding.
In `@circuits/bin/recursive_aggregation/c2_chunk_fold_kernel/src/main.nr`:
- Around line 44-45: The recursive verification paths accept caller-supplied key
hashes without circuit-specific validation. In
circuits/bin/recursive_aggregation/c2_chunk_fold_kernel/src/main.nr:44-45,
circuits/bin/recursive_aggregation/c2ab_chunk_fold/src/main.nr:28-30, and
circuits/bin/recursive_aggregation/esm_c2_chunk_finalize/src/main.nr:29, and
likewise the c2_chunk_fold, c2ab_fold, sk_c2_chunk_finalize, and
c2_chunk_finalize circuits, compare each supplied verification-key hash against
that circuit’s expected hash before calling verify_honk_proof; preserve
verification only after all bindings pass.
In `@circuits/bin/recursive_aggregation/sk_c2_chunk_finalize/src/main.nr`:
- Around line 13-34: Update main to validate the six-field accumulator prefix
and bind the supplied acc_vk and acc_key_hash to the intended C2ChunkFold or
C2ChunkFoldKernel context. Also validate the base_key_hash and chunk_key_hash
fields against the expected SK context, while preserving the existing ESM
variant distinction and flag checks. Reject proofs whose accumulator context or
key hashes do not match the selected circuit variant.
In `@circuits/lib/src/core/dkg/share_computation_base.nr`:
- Around line 51-64: Enforce the complete chunk partition by adding an assertion
that N equals CHUNK_SIZE multiplied by N_CHUNKS before generating commitments in
both circuits/lib/src/core/dkg/share_computation_base.nr lines 51-64 and
125-138. Apply the assertion in the SK commitment path and the ESM commitment
path, before their respective chunk-processing loops.
In `@crates/zk-prover/src/circuits/dkg/share_computation.rs`:
- Around line 49-72: In prove_chunked_share_computation_with_chunk_size, after
deriving chunk_count and before generating the base proof, compare it with the
compile-time SHARE_COMPUTATION_N_CHUNKS value and return ZkError::InvalidInput
on mismatch. In crates/multithread/src/multithread.rs lines 938-952, verify that
the zk_cli-configured chunk size reaches this handler; if it does not, document
that the path always uses DEFAULT_C2_CHUNK_SIZE and preserve that behavior.
In `@crates/zk-prover/tests/local_e2e_tests.rs`:
- Around line 653-689: Update crates/zk-prover/tests/local_e2e_tests.rs:653-689
test_chunked_share_computation_proof to use or construct an SK fixture producing
at least two C2 chunks and assert the expected chunk_count; update
crates/zk-prover/tests/local_e2e_tests.rs:691-728 to do the same for the ESM
fixture; update crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs:589-640
to pass the resulting multi-chunk C2 proofs through the C2abChunkFold NodeFold
verification path.
---
Nitpick comments:
In `@crates/events/src/interfold_event/proof.rs`:
- Around line 161-162: Remove the unused C2ChunkFinalize variant from the
relevant proof enum, then delete its associated mappings and references while
preserving SkC2ChunkFinalize and ESmC2ChunkFinalize. Remove the
circuits/bin/recursive_aggregation/c2_chunk_finalize circuit package and any
build or generation entries that compile it.
In `@crates/events/src/interfold_event/signed_proof.rs`:
- Around line 60-67: Update the test helper that selects circuits for C2a and
C2b proofs to cover both entries in each corresponding circuit list, including
SkC2ChunkFinalize and ESmC2ChunkFinalize. Avoid relying only on the first
element; select each circuit explicitly or add separate test cases while
preserving existing proof construction behavior.
In `@crates/zk-prover/src/circuits/aggregation/c2_chunk_accumulator.rs`:
- Around line 424-443: Remove the redundant acc_public_inputs.len() <= 2 *
chunk_count validation and rely on the existing checked_sub error path for
layout validation. Simplify the acc_vk assignment by using the already validated
accumulator.circuit directly, eliminating the identity match while preserving
the surrounding circuit validation.
- Line 18: Document that C2_CHUNK_FOLD_PUBLIC_PREFIX_LEN must match the Noir
global C2_CHUNK_FOLD_PREFIX_LEN defined in the terminal and fold circuits,
identifying that global as the source of truth.
- Around line 253-285: Update the first-step handling around is_first_step and
generate_kernel_genesis_proof so the kernel proof becomes the initial
accumulator/prior state instead of being discarded. Ensure C2ChunkFold is
invoked only for subsequent chunks, while preserving the existing public-input
validation and finalizer flow that accepts C2ChunkFoldKernel.
In `@crates/zk-prover/src/circuits/dkg/share_computation.rs`:
- Around line 105-127: Hoist the `circuit_path` construction and
`CompiledCircuit::from_file` call out of the chunk-processing loop so the shared
`compiled` circuit is loaded once before iterating over `chunk_idx`. Keep each
iteration’s input-map creation and witness generation in the loop, reusing the
preloaded circuit.
- Around line 92-101: Move the y extraction and degree-length validation in the
share computation flow to before the prove_recursive_circuit base-proof
generation call. Preserve the existing missing-y serialization error and
coefficient-count InvalidInput error, and only invoke prove_recursive_circuit
after y has passed both checks.
In `@crates/zk-prover/src/lib.rs`:
- Around line 30-32: Restrict the re-exports of
generate_sequential_c2_chunk_fold and finalize_c2_chunk_fold in the crate root
to pub(crate), keeping both fold steps internal while preserving their use by
prove_chunked_share_computation.
In `@scripts/build-circuits.ts`:
- Around line 714-719: Split the VK decisions in generateVk: add a
needsNoirRecursiveVk(circuit) predicate for selecting the noir-recursive
profile, and gate runWriteVk('evm', ...) for aggregation circuits with
isAggregationWithEvmOnChain so finalize circuits do not generate or require an
unused EVM VK. Rename isFoldOrAggregation to a profile-oriented name such as
usesNonZkAccumulatorVkProfile and update all callers.
🪄 Autofix
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 Plus
Run ID: 99154c0c-7891-4c5f-8c51-266bd97840af
📒 Files selected for processing (47)
agent/CHUNKED_C2_PLAN.mdagent/flow-trace/04_DKG_AND_COMPUTATION.mdcircuits/benchmarks/results_insecure_minimum/crisp_verify_gas.jsoncircuits/benchmarks/results_secure_minimum/benchmark_run_meta.jsoncircuits/benchmarks/results_secure_minimum/crisp_verify_gas.jsoncircuits/benchmarks/results_secure_minimum/integration_summary.jsoncircuits/benchmarks/results_secure_minimum/report.mdcircuits/bin/dkg/Nargo.tomlcircuits/bin/dkg/e_sm_share_computation_base/Nargo.tomlcircuits/bin/dkg/e_sm_share_computation_base/src/main.nrcircuits/bin/dkg/share_computation_chunk/Nargo.tomlcircuits/bin/dkg/share_computation_chunk/src/main.nrcircuits/bin/dkg/sk_share_computation_base/Nargo.tomlcircuits/bin/dkg/sk_share_computation_base/src/main.nrcircuits/bin/recursive_aggregation/c2_chunk_finalize/Nargo.tomlcircuits/bin/recursive_aggregation/c2_chunk_finalize/src/main.nrcircuits/bin/recursive_aggregation/c2_chunk_fold/Nargo.tomlcircuits/bin/recursive_aggregation/c2_chunk_fold/src/main.nrcircuits/bin/recursive_aggregation/c2_chunk_fold_kernel/Nargo.tomlcircuits/bin/recursive_aggregation/c2_chunk_fold_kernel/src/main.nrcircuits/bin/recursive_aggregation/c2ab_chunk_fold/Nargo.tomlcircuits/bin/recursive_aggregation/c2ab_chunk_fold/src/main.nrcircuits/bin/recursive_aggregation/esm_c2_chunk_finalize/Nargo.tomlcircuits/bin/recursive_aggregation/esm_c2_chunk_finalize/src/main.nrcircuits/bin/recursive_aggregation/sk_c2_chunk_finalize/Nargo.tomlcircuits/bin/recursive_aggregation/sk_c2_chunk_finalize/src/main.nrcircuits/lib/src/configs/insecure/dkg.nrcircuits/lib/src/configs/secure/dkg.nrcircuits/lib/src/core/dkg/mod.nrcircuits/lib/src/core/dkg/share_computation_base.nrcircuits/lib/src/core/dkg/share_computation_chunk.nrcircuits/lib/src/math/commitments.nrcrates/events/src/interfold_event/proof.rscrates/events/src/interfold_event/signed_proof.rscrates/multithread/src/multithread.rscrates/zk-helpers/README.mdcrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/circuits/dkg/share_computation/codegen.rscrates/zk-prover/src/circuits/aggregation/c2_chunk_accumulator.rscrates/zk-prover/src/circuits/aggregation/mod.rscrates/zk-prover/src/circuits/aggregation/node_dkg_fold.rscrates/zk-prover/src/circuits/dkg/share_computation.rscrates/zk-prover/src/lib.rscrates/zk-prover/tests/common/helpers.rscrates/zk-prover/tests/local_e2e_tests.rscrates/zk-prover/tests/node_fold_correlated_e2e_tests.rsscripts/build-circuits.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs (1)
239-259: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject chunk sizes that do not match the committed circuit configuration.
Lines 239 and 256 always commit with chunk size
512. The chunked prover accepts any non-zero divisor and slices the witnesses with that supplied size. A caller can therefore request a non-512 chunk size that passes validation but cannot match the generated chunk circuit or expected root commitment.Reject every size except the configured
SHARE_COMPUTATION_CHUNK_SIZE. If multiple sizes are supported, propagate the selected size through the circuit selection and root commitment calculation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs` around lines 239 - 259, Update the chunk-size validation in the chunked prover to reject every size except the configured SHARE_COMPUTATION_CHUNK_SIZE, rather than accepting any non-zero divisor. Ensure circuit selection, witness slicing, and root commitment calls such as compute_sc_sk_secret_root_commitment and compute_sc_esm_secret_root_commitment all use the same configured size.circuits/bin/recursive_aggregation/c2ab_chunk_fold/src/main.nr (1)
18-29: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftBind every recursive child verification key to its canonical circuit.
The recursive verifiers accept caller-selected verification keys and matching hashes. A replacement circuit can produce the required public-input shape and chosen chunk roots without proving the intended C2 computation.
circuits/bin/recursive_aggregation/c2ab_chunk_fold/src/main.nr#L18-L29: comparec2a_key_hashandc2b_key_hashwith canonical C2 finalizer verification-key hashes before consuming either child public output.circuits/bin/recursive_aggregation/esm_c2_chunk_finalize/src/main.nr#L27-L47: comparebatch_key_hashwith the canonicalc2_chunk_batchverification-key hash before consuming batch public outputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@circuits/bin/recursive_aggregation/c2ab_chunk_fold/src/main.nr` around lines 18 - 29, Bind each recursive child proof to its canonical circuit before using its public outputs: in circuits/bin/recursive_aggregation/c2ab_chunk_fold/src/main.nr lines 18-29, validate c2a_key_hash and c2b_key_hash against the canonical C2 finalizer verification-key hashes before the verify_honk_proof calls or any output consumption; in circuits/bin/recursive_aggregation/esm_c2_chunk_finalize/src/main.nr lines 27-47, validate batch_key_hash against the canonical c2_chunk_batch verification-key hash before consuming batch public outputs.
🧹 Nitpick comments (23)
circuits/bin/recursive_aggregation/node_fold/src/main.nr (1)
154-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the literal indices
3and4with named globals.The file already defines layout globals such as
C2AB_PREFIX_LEN,C2A_INNER_START, andC2B_INNER_START. The two new emitted fields use raw indices3and4. If thec2ab_foldprefix layout changes again, these literals silently select the wrong fields.Define named globals for the two root-commitment slots and use them here.
♻️ Proposed refactor
pub global C2A_SHARES_START: u32 = C2A_INNER_START + 1; pub global C2B_SHARES_START: u32 = C2B_INNER_START + 1; + +/// Root commitments carried by `c2ab_chunk_fold` in the expanded prefix. +pub global C2A_ROOT_INDEX: u32 = 3; +pub global C2B_ROOT_INDEX: u32 = 4;( - key_hash, c2ab_public[3], c2ab_public[4], c0_pk, c1_pk, c3_pk, c2a, c2b, c4a_exp, c4b_exp, + key_hash, c2ab_public[C2A_ROOT_INDEX], c2ab_public[C2B_ROOT_INDEX], c0_pk, c1_pk, c3_pk, c2a, c2b, c4a_exp, c4b_exp, sk_agg, esm_agg, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@circuits/bin/recursive_aggregation/node_fold/src/main.nr` around lines 154 - 157, Define named globals for the root-commitment slots currently represented by indices 3 and 4, alongside the existing c2ab layout globals, then replace c2ab_public[3] and c2ab_public[4] in the emitted tuple with those named globals.crates/keyshare/src/threshold_keyshare/derive_decryption_key.rs (1)
339-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
own_party_iddirectly instead ofparty_id as u64.
party_idisown_party_id as usize(line 65).party_id as u64converts the value back. The round trip is lossy on 32-bit targets and hides that the field is the caller-supplied party identifier.recipient_party_idis bound into the C4 proof, so the value must stay exact.♻️ Proposed refactor
- recipient_party_id: party_id as u64, + recipient_party_id: own_party_id,Apply the same change at line 355.
Also applies to: 355-355
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/keyshare/src/threshold_keyshare/derive_decryption_key.rs` at line 339, In the decryption-key derivation flow, replace both recipient_party_id assignments in the C4 proof construction with own_party_id directly, at the locations currently using party_id as u64. Preserve the exact caller-supplied identifier without converting through party_id.crates/zk-prover/src/circuits/dkg/share_computation.rs (1)
91-97: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the loop-invariant work out of the chunk loop.
Four items do not depend on
chunk_idx:
- The
secret_keyname and thesecretlookup at lines 91-97.circuit_pathat lines 148-151.CompiledCircuit::from_fileat line 152, which re-reads and re-parses the circuit artifact for every chunk.WitnessGenerator::new()at line 153.For the secure preset the circuit artifact is large and
chunk_countis 16, so the artifact is read and parsed 16 times. Compute all four before the loop.♻️ Proposed refactor
+ let secret_key = match data.dkg_input_type { + DkgInputType::SecretKey => "sk_secret", + DkgInputType::SmudgingNoise => "e_sm_secret", + }; + let secret = base_json.get(secret_key).ok_or_else(|| { + ZkError::SerializationError(format!("C2 input is missing {secret_key}")) + })?; + let circuit_path = prover + .circuits_dir(e3_events::CircuitVariant::Recursive, artifacts_dir) + .join(chunk_circuit.dir_path()) + .join(format!("{}.json", chunk_circuit.as_str())); + let compiled = crate::witness::CompiledCircuit::from_file(&circuit_path)?; + let witness_gen = crate::witness::WitnessGenerator::new(); + let mut chunks = Vec::with_capacity(chunk_count); for chunk_idx in 0..chunk_count {Also applies to: 148-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/dkg/share_computation.rs` around lines 91 - 97, Move the chunk-independent setup out of the chunk loop: resolve `secret_key` and `secret` once, compute `circuit_path`, create the `CompiledCircuit` via `CompiledCircuit::from_file`, and initialize `WitnessGenerator::new()` before iterating chunks. Reuse these values for every chunk while preserving the existing per-chunk computation and error behavior.crates/zk-prover/src/circuits/aggregation/c2_chunk_batch.rs (1)
202-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the test module beyond
chunks_per_batch.The tests cover only the batch-size helper. The validation branches in
generate_c2_chunk_batchesandfinalize_c2_chunk_batchesare pure input checks and do not need artifacts: wrongchunk_count, non-divisible chunk count, a mismatchedchunk_circuit, an empty batch list, an invalid finalizer circuit, and unequal batch public-input lengths. Add tests for those branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/c2_chunk_batch.rs` around lines 202 - 215, Extend the tests module around chunks_per_batch with focused tests for validation branches in generate_c2_chunk_batches and finalize_c2_chunk_batches, using lightweight inputs without generating artifacts. Cover wrong and non-divisible chunk_count, mismatched chunk_circuit, empty batches, invalid finalizer circuit, and unequal batch public-input lengths, asserting each input is rejected.crates/zk-prover/src/circuits/aggregation/c3_accumulator.rs (1)
128-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine
field_keysconstants forparty_idxandmod_idx. Theshare_encryptionentry point exposes onlyexpected_pk_commitmentandexpected_message_commitmentas public fields; it does not declareparty_idxormod_idxas circuit inputs. Do not require those names in the circuit's public-input metadata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/c3_accumulator.rs` around lines 128 - 136, Update the field extraction logic near the fields array to use field_keys constants for party_idx and mod_idx, defining those constants alongside the existing field key definitions. Preserve share_encryption’s public-input metadata contract by ensuring these internal indices are not required as declared circuit inputs.crates/program-server/src/lib.rs (1)
307-310: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winClone only the BFV parameters instead of the complete
FHEInputs.Line 309 clones the whole
job.inputs, includingciphertexts. The request body limit is 10 MB, so this duplicates the largest field on every computation. Line 316 uses onlyfhe_inputs.params.♻️ Proposed change to clone only `params`
- let fhe_inputs = job.inputs.clone(); + let params_bytes = job.inputs.params.clone(); match runner(job).await {Then update the commitment call:
- let params = decode_bfv_params_arc(&fhe_inputs.params) + let params = decode_bfv_params_arc(¶ms_bytes) .context("failed to decode BFV params for commitment")?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/program-server/src/lib.rs` around lines 307 - 310, In the computation handler around runner(job).await, replace the full job.inputs clone with a clone of only its BFV params field. Update the later commitment call to use this params value while preserving the existing runner and result flow.examples/CRISP/server/src/cli/commands.rs (1)
191-201: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBound the event query.
Line 196 scans all registry logs from genesis for every readiness check. The range grows indefinitely and can exceed RPC provider log limits. Start the query at the registry deployment block or the E3 request block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/CRISP/server/src/cli/commands.rs` around lines 191 - 201, Update the event query in the readiness-check flow using CommitteePublished_filter to set a bounded starting block instead of from_block(0), choosing the registry deployment block or the E3 request block, while preserving the existing topic filter and readiness result.packages/interfold-contracts/contracts/interfaces/IInterfold.sol (1)
646-655: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn
ICiphertextVerifierfrom the getter for symmetry.
setCiphertextVerifieracceptsICiphertextVerifier, butgetCiphertextVerifierreturnsaddress. The pk-verifier pair uses the interface type on both sides. Use the interface type here so callers do not need a cast.♻️ Proposed change
/// `@notice` Returns the ciphertext verifier configured for future requests. function getCiphertextVerifier( bytes32 encryptionSchemeId - ) external view returns (address); + ) external view returns (ICiphertextVerifier);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/interfaces/IInterfold.sol` around lines 646 - 655, Update the getCiphertextVerifier declaration in IInterfold so its return type is ICiphertextVerifier, matching setCiphertextVerifier and the interface-typed getter contract used by the pk-verifier pair.packages/interfold-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (1)
295-295: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a configuration error for a zero ticket price.
InvalidTicketNumberdescribes a bad caller-supplied ticket index. Here the revert means the bonding registry has no ticket price configured. A distinct error makes the misconfiguration diagnosable from the revert alone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/registry/CiphernodeRegistryOwnable.sol` at line 295, Replace InvalidTicketNumber in the ticketPrice validation with the contract’s distinct configuration error for an unset or zero ticket price, while preserving the existing ticketPrice > 0 condition.packages/interfold-contracts/contracts/interfaces/ICiphertextVerifier.sol (1)
13-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the failure contract of
verify.The return type is
bool, but the NatSpec does not state whether an implementation reverts with a typed error or returnsfalseon rejection.IDecryptionVerifier.verifydocuments this explicitly. Add the same statement here so every implementation and caller agrees on the rejection path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/interfaces/ICiphertextVerifier.sol` around lines 13 - 29, Update the NatSpec for ICiphertextVerifier.verify to explicitly document its rejection behavior, matching the established wording and contract used by IDecryptionVerifier.verify: specify whether invalid verification reverts with the typed error or returns false. Keep the function signature unchanged.packages/interfold-contracts/scripts/deployAndSave/bondingRegistry.ts (1)
50-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReconsider silently defaulting the token decimal values.
BondingRegistryvalidatesexpectedTicketDecimalsandexpectedLicenseDecimalsagainst the tokens' reported ERC-20 decimals and reverts withBondingAssetDecimalsMismatch. These defaults of 6 and 0 fit only thedeployInterfold.tscase, where the license token isethers.ZeroAddress. A caller that supplies a real 18-decimal license token and omitslicenseTokenDecimalsgets a deployment-time revert instead of a clear argument error.Every other value the contract depends on is required in the guard at Lines 64-89. Treat these two the same way, or keep the defaults and document that
0means "no license token configured".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/scripts/deployAndSave/bondingRegistry.ts` around lines 50 - 51, Remove the implicit defaults for ticketTokenDecimals and licenseTokenDecimals in the deployment argument configuration, making both values required and validating them alongside the other contract-dependent inputs in the existing guard. If preserving the defaults is necessary, explicitly document and enforce that licenseTokenDecimals of 0 represents no configured license token, while allowing real license tokens to supply their actual decimals.packages/interfold-contracts/contracts/slashing/SlashingManager.sol (1)
369-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
policy.failureReasonandp.failureReasonare now write-only.
setSlashPolicyvalidatespolicy.failureReason, but both proposal paths ignore the stored value and derivep.failureReasonfromaffectsCommittee._executeSlashthen hardcodesIInterfold.FailureReason.InsufficientCommitteeMembersat Line 871 instead of readingp.failureReason. A policy withaffectsCommittee = trueandfailureReason = 0still producesInsufficientCommitteeMembers, so the validation has no effect on behavior.The comment at Line 369 states the field is retained for ABI and storage compatibility. Make that explicit in code: either read
p.failureReasonin_executeSlash, or drop the derivation at Lines 536 and 627 and mark both fields deprecated so a future reader does not assume the policy value is honored.Also applies to: 536-538, 627-629
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/slashing/SlashingManager.sol` around lines 369 - 379, Make failureReason behavior consistent across setSlashPolicy, both proposal paths, and _executeSlash: either propagate and use p.failureReason through execution, or remove the derivations and explicitly mark policy.failureReason and p.failureReason deprecated because they are compatibility-only fields. Ensure the implementation does not imply that the stored policy value affects the slashing reason when _executeSlash still hardcodes InsufficientCommitteeMembers.packages/interfold-contracts/test/E3Lifecycle/E3Integration.spec.ts (1)
214-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing the shared committee publication helper.
finalizeAndPublishCommitteepublishes only the commitment. The shared fixturepackages/interfold-contracts/test/fixtures/helpers.tssetupAndPublishCommitteepublishes the commitment and then callspublishCommitteePublicKey(e3Id, publicKey). The two helpers now leave the committee in different states.The current tests pass with the commitment alone. A future test that reads the committee public key would fail in a way that is hard to trace. Either delegate to
setupAndPublishCommittee, or add thepublishCommitteePublicKeycall here for parity.♻️ Proposed change to publish the key candidate as well
await registry.publishCommittee( 0, pkCommitment, encodeMockDkgProof(pkCommitment), "0x01", ); + await registry.publishCommitteePublicKey(0, publicKey); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/test/E3Lifecycle/E3Integration.spec.ts` around lines 214 - 228, Update finalizeAndPublishCommittee so it leaves the committee in the same fully published state as setupAndPublishCommittee: after publishCommittee, also publish the corresponding public key using publishCommitteePublicKey with e3Id 0 and the same publicKey, or delegate to the shared helper while preserving the existing setup flow.packages/interfold-contracts/contracts/interfaces/IBondingRegistry.sol (1)
542-552: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe function name still says "AtBlock" while the parameter is a timestamp.
The documentation now describes
timepointas a timestamp-mode checkpoint, but the exported namegetTicketBalanceAtBlocksays block number. An integrator that passesblock.numbergets a wrong balance without any revert.Rename the function to
getTicketBalanceAtin the same change set that already breaks the interface, or state the timestamp unit in the name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/interfaces/IBondingRegistry.sol` around lines 542 - 552, Rename the IBondingRegistry function getTicketBalanceAtBlock to getTicketBalanceAt so its exported name matches the timestamp-based timepoint parameter and documentation. Update all corresponding declarations, implementations, and call sites in the same change set, preserving the existing behavior.packages/interfold-contracts/contracts/E3RefundManager.sol (1)
895-899: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit the treasury credit event in
_redistributeHeldSuccess.The treasury fallback in
_creditBaseTopUpsemitsTreasurySlashedCredited. This fallback increments_pendingTreasurywithout any event, so indexers cannot observe the credit.♻️ Proposed change
if (eligible == 0) { address treasuryAddress = _treasuryFor(e3Id); _pendingTreasury[treasuryAddress][token] += amount; + emit TreasurySlashedCredited(treasuryAddress, token, amount); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/E3RefundManager.sol` around lines 895 - 899, Update the eligible == 0 treasury fallback in _redistributeHeldSuccess to emit the same TreasurySlashedCredited event used by _creditBaseTopUps after incrementing _pendingTreasury, preserving the existing treasury address, token, and amount values.packages/interfold-contracts/contracts/lib/BondingAssetLib.sol (1)
85-102: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReturn the new configuration version instead of only emitting it.
validateBondingAssetConfigcomputesconfigurationVersion + (assetChanged ? 1 : 0)for the event. It returns onlyassetChanged. The caller inBondingRegistrymust reproduce the same increment rule to store the version. Two copies of the rule can diverge and make the emitted version disagree withbondingAssetConfigurationVersion().Return the computed version and let the caller store exactly that value.
♻️ Proposed change
- ) external returns (bool assetChanged) { + ) external returns (bool assetChanged, uint64 newConfigurationVersion) { @@ + newConfigurationVersion = + configurationVersion + + (assetChanged ? 1 : 0); emit IBondingRegistry.BondingAssetConfigUpdated( InterfoldTicketToken(config.ticketToken), IERC20(config.licenseToken), config.ticketPrice, config.licenseRequiredBond, config.expectedTicketDecimals, config.expectedLicenseDecimals, - configurationVersion + (assetChanged ? 1 : 0) + newConfigurationVersion );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/lib/BondingAssetLib.sol` around lines 85 - 102, Update validateBondingAssetConfig to return the computed configuration version, configurationVersion + (assetChanged ? 1 : 0), alongside or instead of the current assetChanged result, and use that returned value in BondingRegistry when storing the configuration version. Remove the caller’s duplicate increment logic so the emitted event and bondingAssetConfigurationVersion() use the same value.packages/interfold-contracts/contracts/lib/BondingEligibilityLib.sol (1)
121-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo new storage-slot accessors omit the
memory-safeassembly annotation.BondingAssetLib._slashingLayoutandBondingSlashingLib._layoutuseassembly ("memory-safe"). These two accessors use plainassembly, which blocks the optimizer assumptions the other accessors keep.
packages/interfold-contracts/contracts/lib/BondingEligibilityLib.sol#L121-L131: changeassembly {toassembly ("memory-safe") {in_layout.packages/interfold-contracts/contracts/lib/InterfoldLifecycle.sol#L268-L277: changeassembly {toassembly ("memory-safe") {in_ciphertextVerifierLayout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/lib/BondingEligibilityLib.sol` around lines 121 - 131, Add the "memory-safe" annotation to the assembly blocks in BondingEligibilityLib._layout (packages/interfold-contracts/contracts/lib/BondingEligibilityLib.sol:121-131) and InterfoldLifecycle._ciphertextVerifierLayout (packages/interfold-contracts/contracts/lib/InterfoldLifecycle.sol:268-277), matching the existing storage-slot accessors; no other logic changes are needed.packages/interfold-contracts/contracts/Interfold.sol (1)
1009-1018: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_setFeeAssetConfigallow-lists the new token and keeps the previous one allowed.The function sets
_feeTokenAllowed[token] = truefor the new asset. It never clears the entry for the rotated-out token. The allow-list therefore accumulates stale entries after each rotation, and the guard inrequestat Line 260 can no longer fail for a freshly configured asset.If the allow-list is meant to be an explicit owner decision, keep
setFeeAssetConfigandsetFeeTokenAllowedindependent. If the automatic entry is intended, document it on the setter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/contracts/Interfold.sol` around lines 1009 - 1018, Update _setFeeAssetConfig so rotating the fee token does not automatically add the new token to _feeTokenAllowed or leave stale allow-list entries; keep setFeeAssetConfig and setFeeTokenAllowed independent, allowing only explicit owner actions to manage the allow-list.packages/interfold-contracts/test/BfvPkVerifier.spec.ts (1)
280-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a mismatch test for
expectedESmC2ChunkKeyHash.This test only verifies rejection at index
5 + H. Add the same test with index6 + Hchanged. This protects the ESM C2 chunk binding from removal or an offset error.Proposed test
+ it("reverts VkHashMismatch when the ESM C2 chunk key hash does not match", async function () { + const { bfvPkVerifier } = await loadFixture(deployWithMockCircuit); + const { e3Id, root, nodes } = ctx(); + const pkCommitment = ethers.keccak256("0xabcd"); + const publicInputs = minimalDkgPublicInputs(pkCommitment).map((v, i) => + i === 6 + H ? ethers.id("wrong-esm-c2-chunk") : v, + ); + + await expect( + bfvPkVerifier.verify.staticCall( + e3Id, + root, + nodes, + pkCommitment, + ethers.ZeroHash, + encodeProof("0x01", publicInputs), + ), + ).to.be.revertedWithCustomError(bfvPkVerifier, "VkHashMismatch"); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/test/BfvPkVerifier.spec.ts` around lines 280 - 300, Add a second test alongside the existing SK C2 mismatch case, changing the public input at index 6 + H to an incorrect ESM C2 chunk key hash while keeping the remaining fixture, proof, verification call, and VkHashMismatch assertion consistent.packages/interfold-contracts/test/Slashing/CommitteeExpulsion.spec.ts (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport the failure-reason value from the shared fixtures.
INSUFFICIENT_COMMITTEE_MEMBERS = 2encodes a contract enum value as a local literal. The file already importsCOMMITTEE_THRESHOLDS_ONCHAINfrom../fixtures. Move this constant intopackages/interfold-contracts/test/fixtures/constants.tsand import it. The enum value then has one definition for every spec that asserts a failure reason.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/test/Slashing/CommitteeExpulsion.spec.ts` at line 36, Move INSUFFICIENT_COMMITTEE_MEMBERS from CommitteeExpulsion.spec.ts into the shared constants fixture, then import and reuse it alongside COMMITTEE_THRESHOLDS_ONCHAIN. Remove the local literal so all specs asserting this failure reason use the single shared definition.packages/interfold-contracts/test/Slashing/SlashingManager.spec.ts (1)
285-299: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeploy this system through a fixture.
Every other test in this file obtains state from
loadFixture(setup). This test callsdeployInterfoldSysteminline, so the deployment runs on every invocation and is not covered by the snapshot cache. Extract a second fixture function, for examplesetupWiredSlashingManager, and load it withloadFixture. The file then keeps one state-setup mechanism.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/test/Slashing/SlashingManager.spec.ts` around lines 285 - 299, Extract the inline deployment in the “requires policies to be refreshed after an asset rotation” test into a dedicated fixture function such as setupWiredSlashingManager, preserving its deployInterfoldSystem options and returned system state. Update the test to obtain that state through loadFixture(setupWiredSlashingManager), while retaining the existing signer setup as appropriate.scripts/build-circuits.ts (1)
311-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the preset/committee derivation and honor
skipUtilsPatch.Two concerns in
writeActiveCryptoConfig:
- Lines 313-315 repeat the
paramSetandcommitteeSizederivation frompatchUtilsTs(lines 254-256). Extract one helper so the two generated artifacts cannot diverge.patchUtilsTsreturns early whenskipUtilsPatchis set and when the target file is absent ("optional in minimal checkouts").writeActiveCryptoConfighas neither guard, sowriteFileSyncat line 362 throwsENOENTin a checkout withoutpackages/interfold-contracts/contracts/lib/. Add the same guards.♻️ Proposed guard for parity with `patchUtilsTs`
private writeActiveCryptoConfig(preset: CircuitPreset, committee: CircuitCommittee): void { + if (this.options.skipUtilsPatch) return const { h, t, n } = COMMITTEE_PARAMS[committee] const paramSet = preset === CIRCUIT_PRESETS.INSECURE_512 ? 0 : 1 const committeeSize = ALL_COMMITTEES.indexOf(committee) const params = paramSet === 0 ? BFV_PARAMS.insecure512 : BFV_PARAMS.secure8192const path = join(this.rootDir, 'packages', 'interfold-contracts', 'contracts', 'lib', 'ActiveCryptoConfig.sol') + if (!existsSync(dirname(path))) return // optional in minimal checkoutsAlso applies to: 322-323
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-circuits.ts` around lines 311 - 313, Extract the shared preset/committee derivation used by patchUtilsTs and writeActiveCryptoConfig into one helper, then reuse it in both paths. Update writeActiveCryptoConfig to return early when skipUtilsPatch is enabled or its target file is absent, matching patchUtilsTs before calling writeFileSync.packages/interfold-contracts/test/Registry/BondingRegistry.spec.ts (1)
115-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImpersonation is acquired without a guaranteed release in both specs. Each helper calls
impersonateAccountand then relies on a laterstopImpersonatingAccountthat a failed assertion or a reverting call can skip. Impersonation is node-level state thatloadFixturesnapshot restore does not clear, so one failing test can leak signing authority for that address into the remaining tests.
packages/interfold-contracts/test/Registry/BondingRegistry.spec.ts#L115-L122: changeimpersonateSlashingManagerinto a scoped helper that runs the caller's callback and callsstopImpersonatingAccountin afinallyblock, then drop the manual cleanup at each call site.packages/interfold-contracts/test/Slashing/CommitteeExpulsion.spec.ts#L189-L206: wrap theexpelCommitteeMemberloop intry/finallysostopImpersonatingAccountruns when an expulsion reverts, or reuse the scoped helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/test/Registry/BondingRegistry.spec.ts` around lines 115 - 122, Ensure impersonated accounts are always released: in packages/interfold-contracts/test/Registry/BondingRegistry.spec.ts#L115-L122, make impersonateSlashingManager execute a caller callback and stop impersonating in a finally block, removing manual cleanup at its call sites; in packages/interfold-contracts/test/Slashing/CommitteeExpulsion.spec.ts#L189-L206, wrap the expelCommitteeMember loop in try/finally or reuse the scoped helper so cleanup occurs on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent/flow-trace/04_DKG_AND_COMPUTATION.md`:
- Around line 329-331: Update the paragraph describing BfvPkVerifier so it
states that publicInputs[0] anchors the nodesFold VK hash and publicInputs[1]
anchors the C5 VK hash; remove the claim that SK/ESM chunk VK hashes are direct
immutable anchors while preserving the surrounding terminal-proof flow.
In `@agent/flow-trace/05_FAILURE_REFUND_SLASHING.md`:
- Around line 216-222: Update the claimant terminology in the E3 reward-claim
flow to identify the frozen reward recipient rather than the bond owner. Revise
the caller requirement around msg.sender == recipient and the related transfer
description so they consistently state that the frozen recipient is the
claimant, including the corresponding section at the additional referenced
location.
In `@crates/events/src/interfold_event/compute_request/zk.rs`:
- Around line 193-195: Update the slot-index calculation in the surrounding
proof-request method to use checked multiplication for recipient_party_id and
n_moduli, checked addition with row_index, and a fallible conversion to u32.
Propagate the resulting failure so proof requests with unrepresentable slot
indices are rejected rather than producing a wrapped or saturated value.
In `@crates/multithread/src/multithread.rs`:
- Around line 1154-1155: In the proof-generation flow constructing the values
with party_idx and mod_idx, replace the truncating casts with checked
conversions. Validate req.recipient_party_id against the committee size and
req.row_index against the configured modulus count before generating the proof,
rejecting out-of-range identifiers so the circuit-bound values match the
response identifiers.
In `@crates/support/README.md`:
- Around line 183-186: Update the request example’s JSON key from
committee_public_key to committee_public_key_hash so it matches the required
ComputeRequest field and copied curl requests are accepted.
In `@crates/zk-prover/src/circuits/dkg/share_computation.rs`:
- Around line 98-146: Validate secret coefficient lengths once before the chunk
loop, using the same expected degree as the existing y validation: check the
SecretKey coefficient array and every ESM CRT limb, returning
ZkError::InvalidInput on mismatch. Add or reuse a slice_coefficients helper that
performs bounds-safe chunk extraction, then replace both direct coefficient
range slices in the secret_chunk construction with slice_coefficients(...)?
while preserving the existing JSON validation errors.
In `@crates/zk-prover/src/commitment_links/c2_to_c4/mod.rs`:
- Around line 213-223: Update the C2 commitment-link flow to receive the
public-signal layout explicitly, deriving it from the C2 proof circuit name
rather than inferring it from signal lengths or alternative positions. In
crates/zk-prover/src/commitment_links/c2_to_c4/mod.rs lines 213-223, replace the
hardcoded [3usize, 9, 19] length test with the layout-provided prefix length. In
crates/zk-prover/src/commitment_links/c1_to_c2.rs lines 70-72 and 109-112, make
the C2 and C2b checks each compare the commitment at the single position
specified by that layout, removing the field 0/field 1 alternatives.
In `@packages/interfold-contracts/contracts/E3RefundManager.sol`:
- Around line 1086-1091: Update the expulsion resolution flow around
openExpulsionProposal and _resolveExpulsionOutcome so an expelled operator’s
base reward is forfeited whenever _distributions[e3Id].calculated is true,
regardless of proposal.holdsBaseReward or when the proposal opened. Ensure
_takeForfeitedBaseReward receives the calculated-distribution case and
_consumeBaseReward is invoked as needed so the operator share and final dust are
accounted for.
In `@packages/interfold-contracts/contracts/interfaces/ICiphernodeRegistry.sol`:
- Around line 71-84: Update the NatSpec block for CommitteeRequested to document
its ticketPrice parameter, describing the ticket price associated with committee
formation while preserving the existing parameter documentation.
In `@packages/interfold-contracts/contracts/lib/ActiveCryptoConfig.sol`:
- Line 15: Update the generator that produces ActiveCryptoConfig so
COMMITTEE_SIZE uses the active generated committee size rather than zero,
preserving the supported preset values 3, 9, and 19. Add a request-creation test
covering the active preset and verifying it succeeds with the generated
committee size.
In `@packages/interfold-contracts/contracts/lib/BondingEligibilityLib.sol`:
- Around line 38-52: Update invalidateConfiguration to return a reset
active-operator count alongside newVersion, and set the checkpointed count to
that same reset value. Modify the caller of invalidateConfiguration to assign
the returned reset value to its local activeOperatorCount before any subsequent
updateOperator call, ensuring both storage representations are reset
consistently.
In `@packages/interfold-contracts/contracts/lib/InterfoldLifecycle.sol`:
- Around line 163-188: In publishCiphertext, call _verifyCiphertext before
updating e3.ciphertextOutput, e3.ciphertextCommitment, stages[e3Id], or
deadlines[e3Id].decryptionDeadline. Keep the existing state writes and
CiphertextOutputPublished/E3StageChanged emissions after successful verification
so reentrant calls cannot observe CiphertextReady before publication events.
In `@packages/interfold-contracts/contracts/lib/InterfoldPricing.sol`:
- Around line 142-160: Align _creditReward and rewardDisposition so excluded
operators are treated as non-held and are not credited, preventing
holdSuccessReward from reverting when pending expulsions coexist with exclusion.
Also inspect the active-node flow around getActiveCommitteeNodes,
expelCommitteeMember, and snapshotRewardRecipients to ensure every returned node
has a snapshotted recipient, preserving reward completion without
RewardRecipientNotSnapshotted failures.
In
`@packages/interfold-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Around line 294-296: Update requestCommittee and _validateNodeEligibility to
freeze the bonding asset context when an E3 is requested, not just
sortitionTicketPrices[e3Id]. Snapshot the ticket token address (or
bondingAssetConfigurationVersion()) alongside the price, then reject submissions
if the current context differs from the request-time snapshot before calculating
ticket weight.
In
`@packages/interfold-contracts/contracts/verifiers/bfv/Risc0BfvCiphertextVerifier.sol`:
- Around line 30-35: Update the Risc0BfvCiphertextVerifier constructor to reject
verifier addresses with no deployed code, in addition to the existing
zero-address check. Validate address(verifier).code.length is nonzero before
assigning risc0Verifier, while preserving the existing InvalidVerifier handling.
In `@packages/interfold-contracts/scripts/benchmarkGasFromRaw.ts`:
- Around line 328-330: Update the failure message for the dkgPublicInputs
validation to report all four validated VK hashes: retain nodes_fold and
pk_aggregation, and add sk_share_computation_chunk and
esm_share_computation_chunk for the two C2 chunk checks.
In `@packages/interfold-contracts/test/E3Lifecycle/E3Integration.spec.ts`:
- Around line 1700-1702: Make the final claimHonestNodeReward call in the E3
lifecycle test an explicit assertion, using the test’s established assertion
style to state the intended successful claim for operator1. Preserve the
existing requester connection and claim arguments.
In `@scripts/check-committee.sh`:
- Around line 87-93: Add a default branch to the EXPECTED_SIZE case mapping in
scripts/check-committee.sh that calls fail for unknown ACTIVE_COMMITTEE values,
before the subsequent SOL_SIZE comparison. Preserve the existing minimum, micro,
and small mappings so unexpected committees produce the intended drift failure
instead of leaving EXPECTED_SIZE unset.
In `@templates/default/contracts/MyProgram.sol`:
- Around line 79-89: Update publishInput to verify that ciphertext matches
ciphertextCommitment using the configured ciphertext verifier or an equivalent
proof before calling inputs[e3Id]._insert. Reject invalid bindings before
mutating the input tree, while preserving the existing empty-ciphertext check
and insertion flow for valid submissions.
---
Outside diff comments:
In `@circuits/bin/recursive_aggregation/c2ab_chunk_fold/src/main.nr`:
- Around line 18-29: Bind each recursive child proof to its canonical circuit
before using its public outputs: in
circuits/bin/recursive_aggregation/c2ab_chunk_fold/src/main.nr lines 18-29,
validate c2a_key_hash and c2b_key_hash against the canonical C2 finalizer
verification-key hashes before the verify_honk_proof calls or any output
consumption; in
circuits/bin/recursive_aggregation/esm_c2_chunk_finalize/src/main.nr lines
27-47, validate batch_key_hash against the canonical c2_chunk_batch
verification-key hash before consuming batch public outputs.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs`:
- Around line 239-259: Update the chunk-size validation in the chunked prover to
reject every size except the configured SHARE_COMPUTATION_CHUNK_SIZE, rather
than accepting any non-zero divisor. Ensure circuit selection, witness slicing,
and root commitment calls such as compute_sc_sk_secret_root_commitment and
compute_sc_esm_secret_root_commitment all use the same configured size.
---
Nitpick comments:
In `@circuits/bin/recursive_aggregation/node_fold/src/main.nr`:
- Around line 154-157: Define named globals for the root-commitment slots
currently represented by indices 3 and 4, alongside the existing c2ab layout
globals, then replace c2ab_public[3] and c2ab_public[4] in the emitted tuple
with those named globals.
In `@crates/keyshare/src/threshold_keyshare/derive_decryption_key.rs`:
- Line 339: In the decryption-key derivation flow, replace both
recipient_party_id assignments in the C4 proof construction with own_party_id
directly, at the locations currently using party_id as u64. Preserve the exact
caller-supplied identifier without converting through party_id.
In `@crates/program-server/src/lib.rs`:
- Around line 307-310: In the computation handler around runner(job).await,
replace the full job.inputs clone with a clone of only its BFV params field.
Update the later commitment call to use this params value while preserving the
existing runner and result flow.
In `@crates/zk-prover/src/circuits/aggregation/c2_chunk_batch.rs`:
- Around line 202-215: Extend the tests module around chunks_per_batch with
focused tests for validation branches in generate_c2_chunk_batches and
finalize_c2_chunk_batches, using lightweight inputs without generating
artifacts. Cover wrong and non-divisible chunk_count, mismatched chunk_circuit,
empty batches, invalid finalizer circuit, and unequal batch public-input
lengths, asserting each input is rejected.
In `@crates/zk-prover/src/circuits/aggregation/c3_accumulator.rs`:
- Around line 128-136: Update the field extraction logic near the fields array
to use field_keys constants for party_idx and mod_idx, defining those constants
alongside the existing field key definitions. Preserve share_encryption’s
public-input metadata contract by ensuring these internal indices are not
required as declared circuit inputs.
In `@crates/zk-prover/src/circuits/dkg/share_computation.rs`:
- Around line 91-97: Move the chunk-independent setup out of the chunk loop:
resolve `secret_key` and `secret` once, compute `circuit_path`, create the
`CompiledCircuit` via `CompiledCircuit::from_file`, and initialize
`WitnessGenerator::new()` before iterating chunks. Reuse these values for every
chunk while preserving the existing per-chunk computation and error behavior.
In `@examples/CRISP/server/src/cli/commands.rs`:
- Around line 191-201: Update the event query in the readiness-check flow using
CommitteePublished_filter to set a bounded starting block instead of
from_block(0), choosing the registry deployment block or the E3 request block,
while preserving the existing topic filter and readiness result.
In `@packages/interfold-contracts/contracts/E3RefundManager.sol`:
- Around line 895-899: Update the eligible == 0 treasury fallback in
_redistributeHeldSuccess to emit the same TreasurySlashedCredited event used by
_creditBaseTopUps after incrementing _pendingTreasury, preserving the existing
treasury address, token, and amount values.
In `@packages/interfold-contracts/contracts/interfaces/IBondingRegistry.sol`:
- Around line 542-552: Rename the IBondingRegistry function
getTicketBalanceAtBlock to getTicketBalanceAt so its exported name matches the
timestamp-based timepoint parameter and documentation. Update all corresponding
declarations, implementations, and call sites in the same change set, preserving
the existing behavior.
In `@packages/interfold-contracts/contracts/interfaces/ICiphertextVerifier.sol`:
- Around line 13-29: Update the NatSpec for ICiphertextVerifier.verify to
explicitly document its rejection behavior, matching the established wording and
contract used by IDecryptionVerifier.verify: specify whether invalid
verification reverts with the typed error or returns false. Keep the function
signature unchanged.
In `@packages/interfold-contracts/contracts/interfaces/IInterfold.sol`:
- Around line 646-655: Update the getCiphertextVerifier declaration in
IInterfold so its return type is ICiphertextVerifier, matching
setCiphertextVerifier and the interface-typed getter contract used by the
pk-verifier pair.
In `@packages/interfold-contracts/contracts/Interfold.sol`:
- Around line 1009-1018: Update _setFeeAssetConfig so rotating the fee token
does not automatically add the new token to _feeTokenAllowed or leave stale
allow-list entries; keep setFeeAssetConfig and setFeeTokenAllowed independent,
allowing only explicit owner actions to manage the allow-list.
In `@packages/interfold-contracts/contracts/lib/BondingAssetLib.sol`:
- Around line 85-102: Update validateBondingAssetConfig to return the computed
configuration version, configurationVersion + (assetChanged ? 1 : 0), alongside
or instead of the current assetChanged result, and use that returned value in
BondingRegistry when storing the configuration version. Remove the caller’s
duplicate increment logic so the emitted event and
bondingAssetConfigurationVersion() use the same value.
In `@packages/interfold-contracts/contracts/lib/BondingEligibilityLib.sol`:
- Around line 121-131: Add the "memory-safe" annotation to the assembly blocks
in BondingEligibilityLib._layout
(packages/interfold-contracts/contracts/lib/BondingEligibilityLib.sol:121-131)
and InterfoldLifecycle._ciphertextVerifierLayout
(packages/interfold-contracts/contracts/lib/InterfoldLifecycle.sol:268-277),
matching the existing storage-slot accessors; no other logic changes are needed.
In
`@packages/interfold-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Line 295: Replace InvalidTicketNumber in the ticketPrice validation with the
contract’s distinct configuration error for an unset or zero ticket price, while
preserving the existing ticketPrice > 0 condition.
In `@packages/interfold-contracts/contracts/slashing/SlashingManager.sol`:
- Around line 369-379: Make failureReason behavior consistent across
setSlashPolicy, both proposal paths, and _executeSlash: either propagate and use
p.failureReason through execution, or remove the derivations and explicitly mark
policy.failureReason and p.failureReason deprecated because they are
compatibility-only fields. Ensure the implementation does not imply that the
stored policy value affects the slashing reason when _executeSlash still
hardcodes InsufficientCommitteeMembers.
In `@packages/interfold-contracts/scripts/deployAndSave/bondingRegistry.ts`:
- Around line 50-51: Remove the implicit defaults for ticketTokenDecimals and
licenseTokenDecimals in the deployment argument configuration, making both
values required and validating them alongside the other contract-dependent
inputs in the existing guard. If preserving the defaults is necessary,
explicitly document and enforce that licenseTokenDecimals of 0 represents no
configured license token, while allowing real license tokens to supply their
actual decimals.
In `@packages/interfold-contracts/test/BfvPkVerifier.spec.ts`:
- Around line 280-300: Add a second test alongside the existing SK C2 mismatch
case, changing the public input at index 6 + H to an incorrect ESM C2 chunk key
hash while keeping the remaining fixture, proof, verification call, and
VkHashMismatch assertion consistent.
In `@packages/interfold-contracts/test/E3Lifecycle/E3Integration.spec.ts`:
- Around line 214-228: Update finalizeAndPublishCommittee so it leaves the
committee in the same fully published state as setupAndPublishCommittee: after
publishCommittee, also publish the corresponding public key using
publishCommitteePublicKey with e3Id 0 and the same publicKey, or delegate to the
shared helper while preserving the existing setup flow.
In `@packages/interfold-contracts/test/Registry/BondingRegistry.spec.ts`:
- Around line 115-122: Ensure impersonated accounts are always released: in
packages/interfold-contracts/test/Registry/BondingRegistry.spec.ts#L115-L122,
make impersonateSlashingManager execute a caller callback and stop impersonating
in a finally block, removing manual cleanup at its call sites; in
packages/interfold-contracts/test/Slashing/CommitteeExpulsion.spec.ts#L189-L206,
wrap the expelCommitteeMember loop in try/finally or reuse the scoped helper so
cleanup occurs on failures.
In `@packages/interfold-contracts/test/Slashing/CommitteeExpulsion.spec.ts`:
- Line 36: Move INSUFFICIENT_COMMITTEE_MEMBERS from CommitteeExpulsion.spec.ts
into the shared constants fixture, then import and reuse it alongside
COMMITTEE_THRESHOLDS_ONCHAIN. Remove the local literal so all specs asserting
this failure reason use the single shared definition.
In `@packages/interfold-contracts/test/Slashing/SlashingManager.spec.ts`:
- Around line 285-299: Extract the inline deployment in the “requires policies
to be refreshed after an asset rotation” test into a dedicated fixture function
such as setupWiredSlashingManager, preserving its deployInterfoldSystem options
and returned system state. Update the test to obtain that state through
loadFixture(setupWiredSlashingManager), while retaining the existing signer
setup as appropriate.
In `@scripts/build-circuits.ts`:
- Around line 311-313: Extract the shared preset/committee derivation used by
patchUtilsTs and writeActiveCryptoConfig into one helper, then reuse it in both
paths. Update writeActiveCryptoConfig to return early when skipUtilsPatch is
enabled or its target file is absent, matching patchUtilsTs before calling
writeFileSync.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/zk-prover/src/commitment_links/c2_to_c4/mod.rs`:
- Around line 230-242: Update the target-length bounds check near c4_row_end to
compute c4_row_end plus FIELD_BYTE_LEN with checked_add, returning false if that
addition overflows; use the checked result for the target_public_signals length
comparison.
In `@crates/zk-prover/src/node_proof_aggregation/effects.rs`:
- Around line 170-178: Update the failure handling around the E3Failed
publication to publish the terminal event before removing self.states[e3_id].
Handle the bus.publish result explicitly: only delete the aggregation state
after successful publication, and retain or queue it when publication fails so
the failure can be retried.
🪄 Autofix
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 Plus
Run ID: 8a13a13e-7677-4f4b-888f-6bcb2e5797d7
📒 Files selected for processing (20)
agent/flow-trace/04_DKG_AND_COMPUTATION.mdagent/flow-trace/05_FAILURE_REFUND_SLASHING.mdcrates/events/src/interfold_event/compute_request/zk.rscrates/multithread/src/multithread.rscrates/support/README.mdcrates/zk-prover/src/circuits/dkg/share_computation.rscrates/zk-prover/src/commitment_links/c1_to_c2.rscrates/zk-prover/src/commitment_links/c2_to_c4/mod.rscrates/zk-prover/src/commitment_links/c2_to_c4/tests.rscrates/zk-prover/src/commitment_links/mod.rscrates/zk-prover/src/node_proof_aggregation/effects.rscrates/zk-prover/src/node_proof_aggregation/workflow.rspackages/interfold-contracts/contracts/E3RefundManager.solpackages/interfold-contracts/contracts/interfaces/ICiphernodeRegistry.solpackages/interfold-contracts/contracts/lib/InterfoldLifecycle.solpackages/interfold-contracts/contracts/verifiers/bfv/Risc0BfvCiphertextVerifier.solpackages/interfold-contracts/scripts/benchmarkGasFromRaw.tspackages/interfold-contracts/test/E3Lifecycle/E3Integration.spec.tspackages/interfold-contracts/test/Risc0BfvCiphertextVerifier.spec.tsscripts/check-committee.sh
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/support/README.md
- scripts/check-committee.sh
- crates/zk-prover/src/commitment_links/c1_to_c2.rs
- crates/multithread/src/multithread.rs
- packages/interfold-contracts/test/Risc0BfvCiphertextVerifier.spec.ts
- packages/interfold-contracts/scripts/benchmarkGasFromRaw.ts
- packages/interfold-contracts/contracts/verifiers/bfv/Risc0BfvCiphertextVerifier.sol
- packages/interfold-contracts/contracts/interfaces/ICiphernodeRegistry.sol
- packages/interfold-contracts/contracts/lib/InterfoldLifecycle.sol
- agent/flow-trace/05_FAILURE_REFUND_SLASHING.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/zk-helpers/src/circuits/threshold/pk_aggregation/computation.rs (1)
161-181: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExtra shares beyond
hstay untransformed and still reach the witness.Line 161 accepts
pk0_shares.len() > canonical.h. Line 172 clones every share intopk0. Line 181 reverses and centers only the firsth.to_jsonat line 199 serializes all ofpk0, so any share past indexhis written un-reversed and un-centered. The circuit expects exactlyHshares. Either require an exact count or truncatepk0tohbefore the loop.🐛 Proposed fix
- let mut pk0: Vec<CrtPolynomial> = data.pk0_shares.clone(); + let mut pk0: Vec<CrtPolynomial> = data.pk0_shares[..h].to_vec();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/threshold/pk_aggregation/computation.rs` around lines 161 - 181, Ensure pk0_shares contributes exactly canonical.h shares to the witness: either reject lengths that differ from h at the existing validation in the computation flow, or truncate the cloned pk0 vector to h before the transformation loop. Preserve the reverse/center processing and ensure to_json serializes no extra untransformed shares.crates/tests/tests/integration.rs (1)
563-579: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd default-variant staging for the C2 finalizer JSON.
finalize_c2_chunk_batchesloads the finalizer compiled circuit fromCircuitVariant::Default, but the fixture stages it only underrecursive; witness generation then fails. Keep the finalizer VKs underrecursive, becauseC2abChunkFoldreceives them from that variant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tests/tests/integration.rs` around lines 563 - 579, Update the C2 finalizer fixture staging around sk_c2_chunk_finalize and esm_c2_chunk_finalize to also copy the compiled finalizer circuit JSON into the CircuitVariant::Default location used by finalize_c2_chunk_batches, while preserving the existing recursive VK staging required by C2abChunkFold.
🧹 Nitpick comments (23)
crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs (2)
160-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
generate_configsandgenerate_configs_with_chunk_sizenow have identical signatures.
generate_configsonly forwards its arguments. Two public functions with the same parameter list and behavior add surface area without value. Keep one, or makegenerate_configstake a default chunk size.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs` around lines 160 - 176, Remove the redundant public wrapper between generate_configs and generate_configs_with_chunk_size, or change generate_configs to provide a distinct default chunk-size API. Update all callers to use the retained function while preserving the existing configuration-generation behavior.
52-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider sharing the chunk-slicing logic with the prover.
The doc comment states that this mirrors the slice that
e3-zk-provercomputes at prove time.crates/zk-prover/src/circuits/dkg/share_computation.rscontains the samechunk_idx/secret_chunk/y_chunkconstruction with its own error type. Two copies of one witness contract can drift, and drift produces commitment mismatches that only appear during proving. Extract the JSON slicing into one helper ine3-zk-helpersand call it from both crates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs` around lines 52 - 60, Extract the shared chunk-slicing construction from generate_chunk_toml and the corresponding share-computation prover flow into a reusable helper in e3-zk-helpers. Have the helper produce the chunk_idx, secret_chunk, and y_chunk witness data while exposing an error result that each crate can map to its own error type, then update both callers to use it and preserve their existing error behavior.crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs (2)
220-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
enumerate()is redundant here.The index produced by
enumerate()is discarded by both the filter andcount().skipworks on the iterator directly.♻️ Proposed change
let dropped_non_zero = data .message_vec .iter() - .enumerate() .skip(max_msg_non_zero_coeffs) - .filter(|(_, v)| **v != 0) + .filter(|v| **v != 0) .count();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs` around lines 220 - 226, Remove the redundant enumerate() call from the iterator building dropped_non_zero in the decrypted shares aggregation computation, and update the filter closure to consume each value directly while preserving the existing skip(max_msg_non_zero_coeffs), non-zero check, and count behavior.
234-273: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRun the canonical committee check before the count checks.
Line 214 reads
thresholdfromdata.committee.thresholdwithout validation. Lines 234-248 then compare share counts against that unvalidated value. Line 272 validates the committee last. For a non-canonical committee, the caller receives a count error that names an expected count derived from an invalid committee. Move thecanonical_committee_for_circuitcall above line 234 and derivethresholdfrom the returned canonical committee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs` around lines 234 - 273, Move the canonical_committee_for_circuit validation before the d_share_polys and reconstructing_parties count checks, and use its returned canonical committee to derive threshold. Remove the later duplicate validation, ensuring count errors are based only on validated canonical committee parameters.packages/interfold-contracts/test/BfvVkBindingIntegration.spec.ts (1)
374-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the public-input offsets.
4 + BFV_DKG_H,21 + BFV_DKG_H, and22 + BFV_DKG_Hencode the folded DKG layout as literals. The binding block length also depends on the number of entries returned bygetBfvPkVkBindingHashPaths(). If a circuit is added to that list, these two literals must move, and nothing links them. Derive the chunk-hash offsets from the binding array length, and export the base offset next tobfvPkExpectedPublicInputsLeninpackages/interfold-contracts/scripts/utils.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/test/BfvVkBindingIntegration.spec.ts` around lines 374 - 382, The public-input offsets in the BFV binding integration test are hard-coded and disconnected from the binding layout. Export a named base offset alongside bfvPkExpectedPublicInputsLen in utils.ts, then derive the expectedVkBinding, expectedSkC2ChunkKeyHash, and expectedESmC2ChunkKeyHash indices from that offset plus getBfvPkVkBindingHashPaths()’s length, preserving the existing folded DKG ordering.crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs (1)
232-233: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse the pair already built at line 191.
Line 191 calls
build_pair_for_preset(preset)and discards the DKG params. Line 232 calls it again only to read the DKG params. Destructure both values once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs` around lines 232 - 233, Reuse the result of build_pair_for_preset(preset) already obtained near line 191 instead of calling it again. Destructure and retain both the pair and DKG parameters there, then use the retained DKG parameters at the later computation point while preserving the existing CircuitsErrors::Sample error mapping.circuits/bin/recursive_aggregation/nodes_fold_kernel/src/main.nr (1)
40-41: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueMark the expected hashes as intentionally unused.
nodes_fold_kernelonly mints the genesis accumulator. The firstnodes_foldstep binds its ownexpected_kernel_key_hashto the kernel verification-key hash. Add both fields to the existing discard tuple and document that they exist only for signature parity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@circuits/bin/recursive_aggregation/nodes_fold_kernel/src/main.nr` around lines 40 - 41, Add expected_kernel_key_hash and expected_fold_key_hash to the existing discard tuple in nodes_fold_kernel, and document there that both fields are retained solely for signature parity. Leave the genesis accumulator behavior unchanged.crates/zk-prover/src/circuits/aggregation/nodes_fold_accumulator.rs (2)
43-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
NODES_FOLD_PREFIX_LENinstead of the literal.Line 25 defines the prefix length. Line 44 repeats the value as a literal. The two values must change together.
♻️ Proposed change
fn nodes_fold_acc_public_len(node_fold_fields: usize, total_slots: usize) -> usize { - 6 + total_slots * node_fold_fields + NODES_FOLD_PREFIX_LEN + total_slots * node_fold_fields }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/nodes_fold_accumulator.rs` around lines 43 - 45, Update nodes_fold_acc_public_len to use the existing NODES_FOLD_PREFIX_LEN constant instead of the literal 6 when calculating the public length, keeping the total_slots * node_fold_fields term unchanged.
125-133: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the redundant
NodesFoldVK load.The genesis function reads the
NodesFoldVK artifact from disk again. The only caller,generate_nodes_fold_step_with_vks, already holdsvks.fold_vk. Pass the hash as a parameter to remove the extra file read and to guarantee both paths use the same artifact.♻️ Proposed change
fn generate_nodes_fold_kernel_genesis_proof( prover: &ZkProver, inner: &Proof, total_slots: usize, slot_index: u32, artifacts_dir: &str, job_id: &str, + expected_fold_key_hash: String, ) -> Result<Proof, ZkError> { @@ expected_kernel_key_hash: kernel_vk.key_hash, - expected_fold_key_hash: vk::load_vk_artifacts( - &prover.circuits_dir(CircuitVariant::Default, artifacts_dir), - CircuitName::NodesFold, - )? - .key_hash, + expected_fold_key_hash,Update the call site at line 189 to pass
vks.fold_vk.key_hash.clone().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/nodes_fold_accumulator.rs` around lines 125 - 133, Update the genesis function initializing the accumulator to accept the fold VK hash as a parameter instead of loading the NodesFold artifact via vk::load_vk_artifacts. In generate_nodes_fold_step_with_vks, pass vks.fold_vk.key_hash.clone() to the genesis call and use that parameter for expected_fold_key_hash.crates/zk-prover/src/circuits/aggregation/node_dkg_fold.rs (2)
178-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the single-variant circuit checks.
Each
matchaccepts exactly one variant and returns the same value. An equality check states the intent more directly.♻️ Optional simplification
- let c2a_circuit = match input.c2a_proof.circuit { - CircuitName::SkC2ChunkFinalize => input.c2a_proof.circuit, - other => { - return Err(ZkError::InvalidInput(format!( - "invalid C2a proof circuit {other}" - ))) - } - }; + let c2a_circuit = CircuitName::SkC2ChunkFinalize; + if input.c2a_proof.circuit != c2a_circuit { + return Err(ZkError::InvalidInput(format!( + "invalid C2a proof circuit {}", + input.c2a_proof.circuit + ))); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/node_dkg_fold.rs` around lines 178 - 204, Replace the single-variant match expressions assigning c2a_circuit and c2b_circuit with direct equality validation against CircuitName::SkC2ChunkFinalize and CircuitName::ESmC2ChunkFinalize, respectively. Preserve the existing ZkError::InvalidInput messages and continue using the validated circuit values for VK loading.
597-614: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
vk_bindingindex contract.This vector order must match the
VK_*globals incircuits/bin/recursive_aggregation/dkg_aggregator/src/main.nrand the path array inpackages/interfold-contracts/scripts/utils.ts(getBfvPkVkBindingHashPaths). I checked all three: they agree today. The ordering is replicated in three languages with no shared definition, so a future insertion in one place breaks proving silently at the circuit assertion. Add an index comment per entry that names the matchingVK_*global, or generate the list from one source.♻️ Suggested annotation
vk_binding: vec![ + // Order must match `VK_*` globals in dkg_aggregator/src/main.nr + // and getBfvPkVkBindingHashPaths() in scripts/utils.ts. - node_fold_vk.key_hash, + node_fold_vk.key_hash, // VK_NODE_FOLD = 0 - c0_vk.key_hash, + c0_vk.key_hash, // VK_C0 = 1 - c1_vk.key_hash, + c1_vk.key_hash, // VK_C1 = 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/node_dkg_fold.rs` around lines 597 - 614, Document the positional contract of the vk_binding vector by adding an index comment for every entry in the list, naming its corresponding VK_* global from the recursive aggregation circuit. Keep the existing order and values unchanged, and ensure the annotations cover all entries so they can be compared with getBfvPkVkBindingHashPaths.crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs (1)
299-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the expected chunk count from
DEFAULT_C2_CHUNK_SIZE.The literal
512repeats the prover's default chunk size.prove_chunked_share_computationusesDEFAULT_C2_CHUNK_SIZE. If that constant changes, this assertion asserts the wrong value instead of failing for the right reason. Import the constant, astests/common/node_fold_witness.rsalready does.♻️ Proposed change
- let expected_chunk_count = preset.metadata().degree / 512; + let expected_chunk_count = + preset.metadata().degree.div_ceil(e3_zk_prover::DEFAULT_C2_CHUNK_SIZE);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs` around lines 299 - 301, Update the expected_chunk_count calculation in the relevant test to divide preset.metadata().degree by DEFAULT_C2_CHUNK_SIZE instead of the hardcoded 512. Import and reuse DEFAULT_C2_CHUNK_SIZE from the existing prover constant location, matching the import pattern in tests/common/node_fold_witness.rs.packages/interfold-contracts/scripts/utils.ts (1)
336-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the mismatching binding index.
The error text states "recursive VK binding mismatch at one or more indices" even when every binding hash matches and only a direct hash is stale. Include the failing indices so the operator can identify the stale circuit.
♻️ Proposed change
+ const bindingMismatches = onChainVkBinding + .map((value, index) => (value === expectedVkBinding[index] ? -1 : index)) + .filter((index) => index >= 0); + throw new Error( `BfvPkVerifier at ${address} has stale sub-circuit VK immutables. ` + `On-chain nodes_fold=${onChainNodesFold} expected=${expectedNodesFold}; ` + `on-chain c5=${onChainC5} expected=${expectedC5}; ` + `on-chain sk_c2_chunk=${onChainSkC2Chunk} expected=${expectedSkC2Chunk}; ` + `on-chain esm_c2_chunk=${onChainESmC2Chunk} expected=${expectedESmC2Chunk}; ` + - `recursive VK binding mismatch at one or more indices. ` + + `recursive VK binding mismatches at indices [${bindingMismatches.join(", ")}]. ` + `Redeploy after pnpm compile:circuits or remove the stale entry from deployed_contracts.json.`, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/interfold-contracts/scripts/utils.ts` around lines 336 - 343, Update the BfvPkVerifier stale-immutables validation to collect the indices whose recursive VK bindings differ and include those indices in the thrown error. Only report the recursive binding mismatch when at least one index fails; preserve the existing direct hash diagnostics and redeployment guidance.crates/zk-prover/tests/local_e2e_tests.rs (3)
537-541: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the literal chunk size with
DEFAULT_C2_CHUNK_SIZE.The third argument of
compute_sc_sk_secret_root_commitmentis the chunk size. The literal512matches the current default only forBfvPreset::InsecureThreshold512. If the default chunk size changes, this assertion compares a commitment built with a different chunking than the proof, and the failure message points at the commitment rather than at the constant.♻️ Proposed refactor
let sk_commitment_expected = compute_sc_sk_secret_root_commitment( &computation_output.inputs.sk, computation_output.bits.sk_bit, - 512, + e3_zk_prover::DEFAULT_C2_CHUNK_SIZE, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/tests/local_e2e_tests.rs` around lines 537 - 541, Update the compute_sc_sk_secret_root_commitment call when assigning sk_commitment_expected to use DEFAULT_C2_CHUNK_SIZE instead of the literal 512, keeping the commitment calculation aligned with the proof’s configured chunk size.
755-783: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the expected chunk count from the configuration.
Both secure tests hard-code
16. This value equalsdegree / DEFAULT_C2_CHUNK_SIZEfor degree 8192. State the derivation so a change to the default chunk size produces a clear failure cause.Add a short comment, or compute the expectation from
preset.metadata().degreeand the exportedDEFAULT_C2_CHUNK_SIZE.Also applies to: 785-812
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/tests/local_e2e_tests.rs` around lines 755 - 783, Update the chunk-count assertions in test_secure_chunked_share_computation_proof and the other secure test to derive the expected value from preset.metadata().degree divided by the exported DEFAULT_C2_CHUNK_SIZE, instead of hard-coding 16; keep the existing result validation unchanged.
700-753: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrint a skip reason when
find_bbreturnsNone.Line 717 uses
find_bb().await?, which returnsNonewithout a message. The two secure tests then pass silently with no output. The other guards in this function print an explicit skip reason. A missingbbbinary produces no signal at all.♻️ Proposed refactor
- let bb = find_bb().await?; + let Some(bb) = find_bb().await else { + println!("skipping: bb not found"); + return None; + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/tests/local_e2e_tests.rs` around lines 700 - 753, Update setup_secure_chunked_share_computation_test so a None result from find_bb().await prints an explicit skip reason before returning None. Preserve the existing early-return behavior and match the diagnostic style of the other skip guards in this function.crates/zk-helpers/src/circuits/output_layout.rs (1)
49-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the catch-all match arms with explicit variants.
field_indexandextract_fieldkeep_ => return Nonearms.CircuitOutputLayoutnow has only two variants. Explicit arms make the compiler report every site that must handle a future variant.♻️ Proposed refactor
pub fn field_index(&self, name: &str) -> Option<usize> { match self { CircuitOutputLayout::Fixed { fields } => fields.iter().position(|f| f.name == name), - _ => None, + CircuitOutputLayout::None => None, } }let fields = match self { CircuitOutputLayout::Fixed { fields } => fields, - _ => return None, + CircuitOutputLayout::None => return None, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-helpers/src/circuits/output_layout.rs` around lines 49 - 64, Update field_index and extract_field to replace their catch-all match arms with explicit handling for every current CircuitOutputLayout variant, returning None for the non-Fixed variant. Keep the existing Fixed behavior unchanged so future variants produce compiler errors at both methods.crates/zk-prover/src/circuits/dkg/share_computation.rs (2)
210-213: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
slice_coefficientsfor theychunk.Line 212 slices
ydirectly while the secret coefficients on lines 165 and 195 use the bounds-safe helper. The current slice is in range, because line 132 validatesy.len() == degreeandchunk_countderives from the samedegree. The direct slice still panics inside a rayon worker if either derivation changes. Use the helper so all three slices share one failure mode.♻️ Proposed refactor
chunk_json.insert( "y_chunk".into(), - Value::Array(y[start..start + chunk_size].to_vec()), + Value::Array(slice_coefficients(y, start, chunk_size, "C2 y")?), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/dkg/share_computation.rs` around lines 210 - 213, Update the y chunk construction in the share computation flow to use slice_coefficients with the same start and end bounds as the existing direct slice. Keep the resulting Value::Array and chunking behavior unchanged, so the secret coefficient and y slices share the helper’s bounds handling.
141-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the secret lookup and shape validation out of the parallel loop.
secret_keyselection, thebase_json.get(secret_key)lookup, and the malformed-JSON checks repeat once per chunk. These values do not depend onchunk_idx. A malformed secret currently fails after the runtime spawns parallel work, and each worker reports the same error.Resolve the secret and its coefficient arrays once, before
into_par_iter, then let each chunk slice from the resolved arrays.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/dkg/share_computation.rs` around lines 141 - 161, In the chunk-building flow around the parallel `into_par_iter` closure, move `secret_key` selection, `base_json.get`, and secret shape validation before creating the iterator. Resolve the required coefficient arrays once, return the existing serialization errors during this sequential validation, and have each chunk use the prevalidated arrays for slicing without repeating lookups or checks inside the closure.crates/zk-prover/src/circuits/aggregation/c2_chunk_batch.rs (2)
215-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
chunks_per_batchtests next to their implementation.Both tests only exercise
chunks_per_batch, which lives inc2_chunk_config. This module contributes no assertions aboutgenerate_c2_chunk_batchesorfinalize_c2_chunk_batches. Tests that cover the validation branches of these two functions would add value here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/c2_chunk_batch.rs` around lines 215 - 228, Move the tests uses_one_batch_chunk_for_insecure_degree and uses_four_batch_chunks_for_secure_degree into the test module adjacent to chunks_per_batch in c2_chunk_config. Keep the aggregation tests focused on generate_c2_chunk_batches and finalize_c2_chunk_batches, adding coverage for their validation branches if appropriate.
149-185: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate the batch count in the public finalizer entry point.
finalize_c2_chunk_batchesispuband only rejects an empty slice. The finalizer circuit is compiled for a fixed batch count. A caller that passes a different number of batches fails late, during witness encoding, after every chunk proof and batch proof is already generated.
generate_c2_chunk_batchesalready performs this check withcompiled_batch_count. Apply the same check here so both public entry points fail early.♻️ Proposed refactor
Accept
degreeand comparebatch_proofs.len()withcompiled_batch_count(degree)before the verification-key load.if batch_proofs.is_empty() { return Err(ZkError::InvalidInput( "C2 chunk finalizer requires at least one batch".into(), )); } + let expected_batch_count = compiled_batch_count(degree); + if batch_proofs.len() != expected_batch_count { + return Err(ZkError::InvalidInput(format!( + "C2 finalizer received {} batches, but the selected artifacts require {expected_batch_count}", + batch_proofs.len() + ))); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/zk-prover/src/circuits/aggregation/c2_chunk_batch.rs` around lines 149 - 185, Update the public finalizer entry point finalize_c2_chunk_batches to accept degree and validate batch_proofs.len() against compiled_batch_count(degree) immediately after the empty-input check and before loading the verification key. Reuse the same mismatch validation and error behavior as generate_c2_chunk_batches so callers fail early for any batch count other than the compiled count.circuits/bin/recursive_aggregation/nodes_fold/src/main.nr (1)
47-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the accumulator parameter indices.
The recursive check reads
acc_public_inputs[2],[4], and[5]. These positions must stay aligned with thepubparameter order ofmain:inner_key_hash,acc_key_hash,is_first_step,slot_index,expected_kernel_key_hash,expected_fold_key_hash. A future parameter insertion breaks this binding silently, because the assertion still passes on the wrong field.Add globals for these offsets so the layout is explicit.
♻️ Proposed refactor
+/// Offsets of `main`'s `pub` parameters inside a `nodes_fold` public-input row. +pub global ACC_IS_FIRST_STEP_IDX: u32 = 2; +pub global ACC_EXPECTED_KERNEL_KEY_HASH_IDX: u32 = 4; +pub global ACC_EXPECTED_FOLD_KEY_HASH_IDX: u32 = 5;- if acc_public_inputs[2] == 1 { - assert(acc_public_inputs[4] == expected_kernel_key_hash); - } else { - assert(acc_public_inputs[5] == expected_fold_key_hash); - } + if acc_public_inputs[ACC_IS_FIRST_STEP_IDX] == 1 { + assert(acc_public_inputs[ACC_EXPECTED_KERNEL_KEY_HASH_IDX] == expected_kernel_key_hash); + } else { + assert(acc_public_inputs[ACC_EXPECTED_FOLD_KEY_HASH_IDX] == expected_fold_key_hash); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@circuits/bin/recursive_aggregation/nodes_fold/src/main.nr` around lines 47 - 56, Define named global constants for the accumulator public-input offsets corresponding to is_first_step, expected_kernel_key_hash, and expected_fold_key_hash, based on main’s declared pub parameter order. Replace the literal indices 2, 4, and 5 in the recursive checks with those constants, preserving the existing assertions.circuits/bin/recursive_aggregation/node_fold/src/main.nr (1)
169-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the C2AB prefix indices.
Line 170 returns
c2ab_public[3]andc2ab_public[4]as bare literals. Every other offset in this file uses a named global. Named globals keep the prefix layout auditable ifC2AB_PREFIX_LENchanges again.♻️ Proposed refactor
+/// Root commitments carried in the `c2ab_chunk_fold` prefix and re-exported by `node_fold`. +pub global C2AB_SK_ROOT_IDX: u32 = 3; +pub global C2AB_ESM_ROOT_IDX: u32 = 4;( - key_hash, c2ab_public[3], c2ab_public[4], c0_pk, c1_pk, c3_pk, c2a, c2b, c4a_exp, c4b_exp, - sk_agg, esm_agg, vk_manifest, + key_hash, c2ab_public[C2AB_SK_ROOT_IDX], c2ab_public[C2AB_ESM_ROOT_IDX], c0_pk, c1_pk, + c3_pk, c2a, c2b, c4a_exp, c4b_exp, sk_agg, esm_agg, vk_manifest, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@circuits/bin/recursive_aggregation/node_fold/src/main.nr` around lines 169 - 172, Update the return tuple in the surrounding aggregation function to replace the bare c2ab_public indices 3 and 4 with the corresponding named global offset constants used for C2AB prefix fields. Preserve the tuple order and all other values unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@circuits/benchmarks/results_insecure_minimum/report.md`:
- Around line 3-6: Regenerate both benchmark reports from one identical Git
checkout and commit so their measurements are directly comparable: update
circuits/benchmarks/results_insecure_minimum/report.md lines 3-6 and
circuits/benchmarks/results_secure_micro/report.md lines 3-6 with the matching
commit metadata. Re-run the respective benchmark generation rather than editing
only the displayed metadata.
In `@circuits/bin/recursive_aggregation/dkg_aggregator/src/main.nr`:
- Around line 167-173: Update validate_dkg_aggregation_shape to require
party_ids to be strictly increasing after validating each ID’s range, and return
ZkError::InvalidInput when any adjacent pair is unsorted or duplicated. Ensure
prove_dkg_aggregation rejects unsorted IDs through this validator while
preserving the existing length and range checks.
In `@circuits/bin/recursive_aggregation/node_fold/src/main.nr`:
- Around line 93-95: Update the layout comment above the C2AB index computations
to describe the current constants: use C2AB_PREFIX_LEN = 5 and
C2AB_FOLD_PUBLIC_LEN = 6 + 2 * C2_PUBLIC_LEN, preserving the explanation of the
two C2 public vectors and their root-commitment indexing.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs`:
- Around line 83-89: Update the secret-slice handling in the SK and ESM limb
branches to reject ranges where end exceeds coeffs.len() instead of clamping
with end.min(coeffs.len()). Keep the existing start-bound validation and return
the same out-of-bounds error style, matching the y-slice range validation so all
inputs reject partial chunks consistently.
- Around line 177-193: The C2 flow uses inconsistent polynomial degrees for
chunk generation, validation, and commitment. In
crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs:177-193,
document and select one authoritative degree, then derive degree and chunk_count
from it; in
crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs:231-244,
validate chunk_size against that same degree and the length of
secret_crt.limb(0) passed to compute_sc_sk_secret_root_commitment, preventing
mismatches and panics.
- Around line 129-134: Update the fallback error handling around the unexpected
chunk secret shape to avoid formatting secret witness content. Report only the
JSON value kind or equivalent non-sensitive type information in the
CircuitsErrors::Sample message, while preserving the existing error path and
context.
In `@crates/zk-helpers/src/circuits/dkg/share_decryption/computation.rs`:
- Around line 418-423: Update the direct commitment calculation in the
computation flow using compute_sc_party_share_root_commitment to pass
sample.chunk_size as usize instead of the hardcoded 512, matching
Inputs::compute behavior for all chunk sizes.
- Around line 208-213: Update the chunk-size validation in the DKG share
decryption computation to reject any nonzero value that does not evenly divide
the DKG polynomial degree before invoking
compute_sc_party_share_root_commitment. Return the existing
CircuitsErrors::Sample error path for invalid sizes, while preserving valid
divisible chunk sizes.
In `@crates/zk-helpers/src/circuits/output_layout.rs`:
- Around line 40-46: The removed Dynamic variant still leaves outdated
documentation and wildcard match arms in CircuitOutputLayout. In
crates/zk-helpers/src/circuits/output_layout.rs lines 40-46, update
field_count’s documentation to state that void layouts return Some(0); in lines
49-64, replace the wildcard arms in field_index and extract_field with explicit
CircuitOutputLayout::None arms so future variants require deliberate handling.
In `@crates/zk-prover/src/circuits/aggregation/c2_chunk_batch.rs`:
- Around line 88-89: Remove the unused mutable batches allocation before the
chunk_proofs assignment; initialize batches directly from the existing
chunk_proofs transformation and preserve the current capacity/collection
behavior without the discarded Vec.
In `@crates/zk-prover/src/commitment_links/c2_to_c3.rs`:
- Around line 124-130: The C2-to-C3 scanner currently skips two metadata fields
that are absent from the SK and ESM terminal-proof layout. Define and reuse the
shared C2 commitment-link prefix length in the C1-to-C2 and C2-to-C3 scanners,
then update the c2_signals tests to represent the existing terminal proof layout
while preserving its intentional omission of the child VK hash and secret-root
commitment.
In `@crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs`:
- Around line 357-368: Remove the second duplicate CircuitName::C2ChunkBatch
entry from the recursive_circuits array, preserving the single entry and the
ordering of all subsequent circuit names.
- Line 70: Add the same ABI-shape assertions used by the corresponding helper in
fold_accumulators_e2e_tests before the `(len - 6) / 3` calculation: assert that
len is at least 6 and that the post-prefix length is divisible by 3, then retain
the existing calculation.
In `@docs/pages/cryptography.mdx`:
- Line 354: Update the “Smudging noise contribution / share” glossary row so its
C2 link targets the esm_share_computation_chunk package instead of
sk_share_computation_chunk; leave the C3 and C4 links unchanged.
In `@docs/pages/internals/dkg.mdx`:
- Around line 50-51: Update the “Proof Count Per E3” table below the C2a and C2b
entries so each node’s count includes degree / chunk_size chunk proofs plus the
existing batch and finalize proofs, keeping both chunked C2 rows consistent with
their descriptions above.
In `@packages/interfold-contracts/contracts/verifiers/bfv/BfvPkVerifier.sol`:
- Around line 154-164: Bind the DKG return VK hash by adding a nonzero immutable
constructor argument, storing it in the verifier, and checking it against
publicInputs[20 + h] in verify between the manifest and existing chunk-key hash
checks. Update every deployment path to pass the generated DKG return VK hash.
In `@packages/interfold-contracts/scripts/utils.ts`:
- Around line 102-105: Update the comment above bfvPkExpectedPublicInputsLen to
describe the dkg_aggregator public-input layout that yields 3 * h + 24,
including the fixed fields and the h-dependent party and commitment arrays;
remove the incorrect reference to eight pairing-point slots. Leave the
calculation unchanged.
---
Outside diff comments:
In `@crates/tests/tests/integration.rs`:
- Around line 563-579: Update the C2 finalizer fixture staging around
sk_c2_chunk_finalize and esm_c2_chunk_finalize to also copy the compiled
finalizer circuit JSON into the CircuitVariant::Default location used by
finalize_c2_chunk_batches, while preserving the existing recursive VK staging
required by C2abChunkFold.
In `@crates/zk-helpers/src/circuits/threshold/pk_aggregation/computation.rs`:
- Around line 161-181: Ensure pk0_shares contributes exactly canonical.h shares
to the witness: either reject lengths that differ from h at the existing
validation in the computation flow, or truncate the cloned pk0 vector to h
before the transformation loop. Preserve the reverse/center processing and
ensure to_json serializes no extra untransformed shares.
---
Nitpick comments:
In `@circuits/bin/recursive_aggregation/node_fold/src/main.nr`:
- Around line 169-172: Update the return tuple in the surrounding aggregation
function to replace the bare c2ab_public indices 3 and 4 with the corresponding
named global offset constants used for C2AB prefix fields. Preserve the tuple
order and all other values unchanged.
In `@circuits/bin/recursive_aggregation/nodes_fold_kernel/src/main.nr`:
- Around line 40-41: Add expected_kernel_key_hash and expected_fold_key_hash to
the existing discard tuple in nodes_fold_kernel, and document there that both
fields are retained solely for signature parity. Leave the genesis accumulator
behavior unchanged.
In `@circuits/bin/recursive_aggregation/nodes_fold/src/main.nr`:
- Around line 47-56: Define named global constants for the accumulator
public-input offsets corresponding to is_first_step, expected_kernel_key_hash,
and expected_fold_key_hash, based on main’s declared pub parameter order.
Replace the literal indices 2, 4, and 5 in the recursive checks with those
constants, preserving the existing assertions.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/codegen.rs`:
- Around line 160-176: Remove the redundant public wrapper between
generate_configs and generate_configs_with_chunk_size, or change
generate_configs to provide a distinct default chunk-size API. Update all
callers to use the retained function while preserving the existing
configuration-generation behavior.
- Around line 52-60: Extract the shared chunk-slicing construction from
generate_chunk_toml and the corresponding share-computation prover flow into a
reusable helper in e3-zk-helpers. Have the helper produce the chunk_idx,
secret_chunk, and y_chunk witness data while exposing an error result that each
crate can map to its own error type, then update both callers to use it and
preserve their existing error behavior.
In `@crates/zk-helpers/src/circuits/dkg/share_computation/computation.rs`:
- Around line 232-233: Reuse the result of build_pair_for_preset(preset) already
obtained near line 191 instead of calling it again. Destructure and retain both
the pair and DKG parameters there, then use the retained DKG parameters at the
later computation point while preserving the existing CircuitsErrors::Sample
error mapping.
In `@crates/zk-helpers/src/circuits/output_layout.rs`:
- Around line 49-64: Update field_index and extract_field to replace their
catch-all match arms with explicit handling for every current
CircuitOutputLayout variant, returning None for the non-Fixed variant. Keep the
existing Fixed behavior unchanged so future variants produce compiler errors at
both methods.
In
`@crates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rs`:
- Around line 220-226: Remove the redundant enumerate() call from the iterator
building dropped_non_zero in the decrypted shares aggregation computation, and
update the filter closure to consume each value directly while preserving the
existing skip(max_msg_non_zero_coeffs), non-zero check, and count behavior.
- Around line 234-273: Move the canonical_committee_for_circuit validation
before the d_share_polys and reconstructing_parties count checks, and use its
returned canonical committee to derive threshold. Remove the later duplicate
validation, ensuring count errors are based only on validated canonical
committee parameters.
In `@crates/zk-prover/src/circuits/aggregation/c2_chunk_batch.rs`:
- Around line 215-228: Move the tests uses_one_batch_chunk_for_insecure_degree
and uses_four_batch_chunks_for_secure_degree into the test module adjacent to
chunks_per_batch in c2_chunk_config. Keep the aggregation tests focused on
generate_c2_chunk_batches and finalize_c2_chunk_batches, adding coverage for
their validation branches if appropriate.
- Around line 149-185: Update the public finalizer entry point
finalize_c2_chunk_batches to accept degree and validate batch_proofs.len()
against compiled_batch_count(degree) immediately after the empty-input check and
before loading the verification key. Reuse the same mismatch validation and
error behavior as generate_c2_chunk_batches so callers fail early for any batch
count other than the compiled count.
In `@crates/zk-prover/src/circuits/aggregation/node_dkg_fold.rs`:
- Around line 178-204: Replace the single-variant match expressions assigning
c2a_circuit and c2b_circuit with direct equality validation against
CircuitName::SkC2ChunkFinalize and CircuitName::ESmC2ChunkFinalize,
respectively. Preserve the existing ZkError::InvalidInput messages and continue
using the validated circuit values for VK loading.
- Around line 597-614: Document the positional contract of the vk_binding vector
by adding an index comment for every entry in the list, naming its corresponding
VK_* global from the recursive aggregation circuit. Keep the existing order and
values unchanged, and ensure the annotations cover all entries so they can be
compared with getBfvPkVkBindingHashPaths.
In `@crates/zk-prover/src/circuits/aggregation/nodes_fold_accumulator.rs`:
- Around line 43-45: Update nodes_fold_acc_public_len to use the existing
NODES_FOLD_PREFIX_LEN constant instead of the literal 6 when calculating the
public length, keeping the total_slots * node_fold_fields term unchanged.
- Around line 125-133: Update the genesis function initializing the accumulator
to accept the fold VK hash as a parameter instead of loading the NodesFold
artifact via vk::load_vk_artifacts. In generate_nodes_fold_step_with_vks, pass
vks.fold_vk.key_hash.clone() to the genesis call and use that parameter for
expected_fold_key_hash.
In `@crates/zk-prover/src/circuits/dkg/share_computation.rs`:
- Around line 210-213: Update the y chunk construction in the share computation
flow to use slice_coefficients with the same start and end bounds as the
existing direct slice. Keep the resulting Value::Array and chunking behavior
unchanged, so the secret coefficient and y slices share the helper’s bounds
handling.
- Around line 141-161: In the chunk-building flow around the parallel
`into_par_iter` closure, move `secret_key` selection, `base_json.get`, and
secret shape validation before creating the iterator. Resolve the required
coefficient arrays once, return the existing serialization errors during this
sequential validation, and have each chunk use the prevalidated arrays for
slicing without repeating lookups or checks inside the closure.
In `@crates/zk-prover/tests/local_e2e_tests.rs`:
- Around line 537-541: Update the compute_sc_sk_secret_root_commitment call when
assigning sk_commitment_expected to use DEFAULT_C2_CHUNK_SIZE instead of the
literal 512, keeping the commitment calculation aligned with the proof’s
configured chunk size.
- Around line 755-783: Update the chunk-count assertions in
test_secure_chunked_share_computation_proof and the other secure test to derive
the expected value from preset.metadata().degree divided by the exported
DEFAULT_C2_CHUNK_SIZE, instead of hard-coding 16; keep the existing result
validation unchanged.
- Around line 700-753: Update setup_secure_chunked_share_computation_test so a
None result from find_bb().await prints an explicit skip reason before returning
None. Preserve the existing early-return behavior and match the diagnostic style
of the other skip guards in this function.
In `@crates/zk-prover/tests/node_fold_correlated_e2e_tests.rs`:
- Around line 299-301: Update the expected_chunk_count calculation in the
relevant test to divide preset.metadata().degree by DEFAULT_C2_CHUNK_SIZE
instead of the hardcoded 512. Import and reuse DEFAULT_C2_CHUNK_SIZE from the
existing prover constant location, matching the import pattern in
tests/common/node_fold_witness.rs.
In `@packages/interfold-contracts/scripts/utils.ts`:
- Around line 336-343: Update the BfvPkVerifier stale-immutables validation to
collect the indices whose recursive VK bindings differ and include those indices
in the thrown error. Only report the recursive binding mismatch when at least
one index fails; preserve the existing direct hash diagnostics and redeployment
guidance.
In `@packages/interfold-contracts/test/BfvVkBindingIntegration.spec.ts`:
- Around line 374-382: The public-input offsets in the BFV binding integration
test are hard-coded and disconnected from the binding layout. Export a named
base offset alongside bfvPkExpectedPublicInputsLen in utils.ts, then derive the
expectedVkBinding, expectedSkC2ChunkKeyHash, and expectedESmC2ChunkKeyHash
indices from that offset plus getBfvPkVkBindingHashPaths()’s length, preserving
the existing folded DKG ordering.
🪄 Autofix
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 Plus
Run ID: 74e611f3-99bb-41f5-b20c-e5c8b10ce927
📒 Files selected for processing (111)
agent/CONTEXT.mdagent/INVARIANTS.mdagent/flow-trace/04_DKG_AND_COMPUTATION.mdcircuits/README.mdcircuits/benchmarks/config.jsoncircuits/benchmarks/results_insecure_agg_micro/benchmark_run_meta.jsoncircuits/benchmarks/results_insecure_agg_micro/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure_agg_micro/integration_summary.jsoncircuits/benchmarks/results_insecure_agg_micro/report.mdcircuits/benchmarks/results_insecure_minimum/crisp_verify_gas.jsoncircuits/benchmarks/results_insecure_minimum/integration_summary.jsoncircuits/benchmarks/results_insecure_minimum/report.mdcircuits/benchmarks/results_secure_agg_micro/benchmark_run_meta.jsoncircuits/benchmarks/results_secure_agg_micro/crisp_verify_gas.jsoncircuits/benchmarks/results_secure_agg_micro/integration_summary.jsoncircuits/benchmarks/results_secure_agg_micro/report.mdcircuits/benchmarks/results_secure_agg_small/benchmark_run_meta.jsoncircuits/benchmarks/results_secure_agg_small/integration_summary.jsoncircuits/benchmarks/results_secure_agg_small/report.mdcircuits/benchmarks/results_secure_micro/crisp_verify_gas.jsoncircuits/benchmarks/results_secure_micro/integration_summary.jsoncircuits/benchmarks/results_secure_micro/report.mdcircuits/benchmarks/results_secure_minimum/benchmark_run_meta.jsoncircuits/benchmarks/results_secure_minimum/crisp_verify_gas.jsoncircuits/benchmarks/results_secure_minimum/integration_summary.jsoncircuits/benchmarks/results_secure_minimum/report.mdcircuits/benchmarks/scripts/generate_prover_toml.shcircuits/benchmarks/scripts/generate_report.shcircuits/bin/dkg/Nargo.tomlcircuits/bin/dkg/e_sm_share_computation/README.mdcircuits/bin/dkg/e_sm_share_computation/src/main.nrcircuits/bin/dkg/sk_share_computation/README.mdcircuits/bin/dkg/sk_share_computation/src/main.nrcircuits/bin/recursive_aggregation/c2_chunk_batch/Nargo.tomlcircuits/bin/recursive_aggregation/c2_chunk_batch/src/main.nrcircuits/bin/recursive_aggregation/c2ab_chunk_fold/src/main.nrcircuits/bin/recursive_aggregation/c2ab_fold/src/main.nrcircuits/bin/recursive_aggregation/c3_fold/src/main.nrcircuits/bin/recursive_aggregation/c3_fold_kernel/src/main.nrcircuits/bin/recursive_aggregation/c3ab_fold/src/main.nrcircuits/bin/recursive_aggregation/dkg_aggregator/src/main.nrcircuits/bin/recursive_aggregation/esm_c2_chunk_finalize/src/main.nrcircuits/bin/recursive_aggregation/node_fold/src/main.nrcircuits/bin/recursive_aggregation/nodes_fold/src/main.nrcircuits/bin/recursive_aggregation/nodes_fold_kernel/src/main.nrcircuits/bin/recursive_aggregation/sk_c2_chunk_finalize/src/main.nrcircuits/lib/src/math/commitments.nrcrates/events/src/interfold_event/compute_request/zk.rscrates/events/src/interfold_event/proof.rscrates/events/src/interfold_event/signed_proof.rscrates/multithread/src/multithread.rscrates/tests/tests/integration.rscrates/zk-helpers/src/bin/zk_cli.rscrates/zk-helpers/src/ciphernodes_committee.rscrates/zk-helpers/src/circuits/commitments.rscrates/zk-helpers/src/circuits/dkg/pk/codegen.rscrates/zk-helpers/src/circuits/dkg/share_computation/circuit.rscrates/zk-helpers/src/circuits/dkg/share_computation/codegen.rscrates/zk-helpers/src/circuits/dkg/share_computation/computation.rscrates/zk-helpers/src/circuits/dkg/share_computation/mod.rscrates/zk-helpers/src/circuits/dkg/share_computation/sample.rscrates/zk-helpers/src/circuits/dkg/share_decryption/circuit.rscrates/zk-helpers/src/circuits/dkg/share_decryption/codegen.rscrates/zk-helpers/src/circuits/dkg/share_decryption/computation.rscrates/zk-helpers/src/circuits/dkg/share_decryption/sample.rscrates/zk-helpers/src/circuits/dkg/share_encryption/circuit.rscrates/zk-helpers/src/circuits/dkg/share_encryption/codegen.rscrates/zk-helpers/src/circuits/dkg/share_encryption/computation.rscrates/zk-helpers/src/circuits/dkg/share_encryption/sample.rscrates/zk-helpers/src/circuits/output_layout.rscrates/zk-helpers/src/circuits/threshold/decrypted_shares_aggregation/computation.rscrates/zk-helpers/src/circuits/threshold/pk_aggregation/computation.rscrates/zk-helpers/src/circuits/threshold/pk_generation/computation.rscrates/zk-helpers/src/circuits/threshold/share_decryption/computation.rscrates/zk-helpers/src/circuits/threshold/user_data_encryption/computation.rscrates/zk-helpers/src/utils.rscrates/zk-prover/src/circuits/aggregation/c2_chunk_batch.rscrates/zk-prover/src/circuits/aggregation/c3_accumulator.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/src/circuits/dkg/share_computation.rscrates/zk-prover/src/commitment_links/c1_to_c2.rscrates/zk-prover/src/commitment_links/c2_to_c3.rscrates/zk-prover/src/commitment_links/c2_to_c4/mod.rscrates/zk-prover/src/commitment_links/c2_to_c4/tests.rscrates/zk-prover/src/node_fold_public.rscrates/zk-prover/src/node_proof_aggregation/effects.rscrates/zk-prover/tests/common/node_fold_witness.rscrates/zk-prover/tests/fold_accumulators_e2e_tests.rscrates/zk-prover/tests/local_e2e_tests.rscrates/zk-prover/tests/node_fold_correlated_e2e_tests.rsdocs/pages/cryptography.mdxdocs/pages/internals/dkg.mdxpackages/interfold-contracts/contracts/interfaces/IPkVerifier.solpackages/interfold-contracts/contracts/verifiers/DkgFoldAttestationVerifier.solpackages/interfold-contracts/contracts/verifiers/bfv/BfvPkVerifier.solpackages/interfold-contracts/contracts/verifiers/bfv/honk/DkgAggregatorVerifier.solpackages/interfold-contracts/ignition/modules/bfvPkVerifier.tspackages/interfold-contracts/scripts/benchmarkGasFromRaw.tspackages/interfold-contracts/scripts/deployAndSave/bfvPkVerifier.tspackages/interfold-contracts/scripts/utils.tspackages/interfold-contracts/test/BfvPkVerifier.spec.tspackages/interfold-contracts/test/BfvVkBindingIntegration.spec.tspackages/interfold-contracts/test/Governance/AccessAndBounds.spec.tspackages/interfold-contracts/test/fixtures/bfv_vk_binding/folded_artifacts.jsonpackages/interfold-contracts/test/fixtures/dkgAttestation.tsscripts/build-circuits.tsscripts/generate-verifiers.tsscripts/invariant-baselines.envscripts/lint-circuits.sh
💤 Files with no reviewable changes (21)
- circuits/benchmarks/results_secure_agg_small/benchmark_run_meta.json
- circuits/benchmarks/results_insecure_agg_micro/benchmark_run_meta.json
- circuits/benchmarks/results_insecure_agg_micro/report.md
- circuits/benchmarks/results_secure_agg_micro/benchmark_run_meta.json
- circuits/benchmarks/results_secure_agg_small/integration_summary.json
- circuits/benchmarks/results_secure_minimum/report.md
- circuits/benchmarks/results_secure_agg_micro/integration_summary.json
- circuits/benchmarks/results_secure_minimum/integration_summary.json
- circuits/benchmarks/results_secure_minimum/crisp_verify_gas.json
- circuits/benchmarks/results_secure_agg_small/report.md
- circuits/bin/dkg/sk_share_computation/README.md
- circuits/benchmarks/results_secure_agg_micro/report.md
- circuits/bin/dkg/sk_share_computation/src/main.nr
- circuits/benchmarks/results_insecure_agg_micro/crisp_verify_gas.json
- circuits/bin/dkg/e_sm_share_computation/src/main.nr
- circuits/bin/dkg/e_sm_share_computation/README.md
- circuits/bin/recursive_aggregation/c2ab_fold/src/main.nr
- circuits/benchmarks/results_secure_minimum/benchmark_run_meta.json
- circuits/benchmarks/results_secure_agg_micro/crisp_verify_gas.json
- packages/interfold-contracts/contracts/verifiers/bfv/honk/DkgAggregatorVerifier.sol
- circuits/benchmarks/results_insecure_agg_micro/integration_summary.json
🚧 Files skipped from review as they are similar to previous changes (18)
- crates/zk-prover/src/node_proof_aggregation/effects.rs
- crates/zk-helpers/src/circuits/dkg/share_decryption/sample.rs
- scripts/generate-verifiers.ts
- circuits/bin/recursive_aggregation/c2_chunk_batch/src/main.nr
- crates/zk-prover/src/commitment_links/c2_to_c4/tests.rs
- packages/interfold-contracts/test/Governance/AccessAndBounds.spec.ts
- circuits/bin/recursive_aggregation/esm_c2_chunk_finalize/src/main.nr
- circuits/bin/recursive_aggregation/c2ab_chunk_fold/src/main.nr
- circuits/bin/recursive_aggregation/c2_chunk_batch/Nargo.toml
- circuits/bin/recursive_aggregation/sk_c2_chunk_finalize/src/main.nr
- packages/interfold-contracts/test/BfvPkVerifier.spec.ts
- crates/zk-prover/src/commitment_links/c2_to_c4/mod.rs
- crates/multithread/src/multithread.rs
- crates/zk-prover/src/circuits/aggregation/c3_accumulator.rs
- circuits/lib/src/math/commitments.nr
- agent/flow-trace/04_DKG_AND_COMPUTATION.md
- crates/zk-helpers/src/circuits/commitments.rs
- crates/zk-prover/src/commitment_links/c1_to_c2.rs
Artifacts had drifted from their sources: ISlashingManager and ICiphernodeRegistry ABI entries (SlashPolicyAssetConfigurationMismatch, E3NotTerminal, CommitteeObligationsAlreadyReleased, pricing struct) were committed in source but missing from the tracked JSON. A fresh hardhat compile regenerates them deterministically.
…ecks - validate chunk bounds with end > len and derive chunk size from the authoritative threshold degree in share_computation codegen/computation - reject zero/non-divisor chunk sizes in share_decryption with a unified message instead of a partial zero-only check - enforce strictly increasing dkg_aggregator party ids before circuit execution, matching the circuit's requirement - use the sample chunk size (not a hardcoded 512) in share_decryption tests - require acc_public_inputs length 6 + 3*slots in the correlated e2e test and drop the duplicate C2ChunkBatch entry - document C2 chunk/batch/finalize proof counts in dkg.mdx and the dkg_aggregator public-input formula in contracts utils.ts
1 participant
EXPERIMENTAL WIP DO NOT MERGE!
Summary by CodeRabbit
New Features
Bug Fixes
Documentation