feat: add chunked C2 proving [skip-line-limit] - #1772
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. 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:
📝 WalkthroughWalkthroughThis PR adds chunked C2 proving with recursive aggregation across DKG circuits and the Rust proving pipeline, and reworks committee publication into a proof-backed commitment plus a separate repeatable public-key candidate. It also introduces request-time sortition snapshots, bonding-asset/pricing configuration overhaul, proposal-scoped slashing, a ComputeDomain-bound compute proof journal, and matching contract, script, and documentation updates. ChangesChunked C2 Proving
Estimated code review effort: 5 (Critical) | ~150 minutes Committee, Sortition, and Bonding Protocol
Estimated code review effort: 5 (Critical) | ~180 minutes Compute Domain and Support Services
Estimated code review effort: 4 (Complex) | ~60 minutes CRISP Examples and Default Templates
Estimated code review effort: 3 (Moderate) | ~30 minutes Protocol Documentation
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Node as Ciphernode
participant Registry as CiphernodeRegistryOwnable
participant BFVProver as BFV Pk Verifier
participant Interfold
Node->>Registry: publishCommittee(e3Id, pkCommitment, proof, attestationBundle)
Registry->>BFVProver: verify(proof, pkCommitment)
BFVProver-->>Registry: verification result
Registry->>Registry: store commitment, mark proof published
Registry->>Interfold: notify committee proof published
Registry-->>Node: CommitteeProofPublished event
Node->>Registry: publishCommitteePublicKey(e3Id, publicKey)
Registry->>Registry: validate length, match against pkCommitment
Registry-->>Node: CommitteePublished event (compatibility, empty proof)
sequenceDiagram
participant Prover as ZkProver
participant ChunkCircuit as Chunk Circuit
participant BatchCircuit as Chunk Batch Circuit
participant FoldCircuit as Chunk Fold/Finalize Circuit
Prover->>Prover: split secret into N chunks
loop each chunk
Prover->>ChunkCircuit: generate witness and proof
end
Prover->>BatchCircuit: verify batch of chunk proofs
BatchCircuit-->>Prover: aggregate commitment
Prover->>FoldCircuit: fold base + chunk + accumulator proofs
FoldCircuit-->>Prover: updated accumulator state
Prover->>FoldCircuit: finalize accumulator into legacy C2 layout
FoldCircuit-->>Prover: terminal C2 proof
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
| let Some(c4_row_start) = src_idx | ||
| .checked_mul(l) | ||
| .and_then(|offset| offset.checked_mul(FIELD_BYTE_LEN)) | ||
| else { | ||
| return false; | ||
| }; | ||
| let Some(c4_row_len) = l.checked_mul(FIELD_BYTE_LEN) else { | ||
| return false; | ||
| }; | ||
| let Some(c4_row_end) = c4_row_start.checked_add(c4_row_len) else { | ||
| return false; | ||
| }; | ||
| if target_public_signals.len() < c4_row_end + FIELD_BYTE_LEN { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Complete the checked target-length calculation.
Line 242 uses c4_row_end + FIELD_BYTE_LEN without checked addition. For a large src_party_id, this can overflow before the bounds check. Debug builds panic. Release builds can wrap and later slice at an invalid offset. Use checked_add and return false when it fails.
Proposed fix
let Some(c4_row_end) = c4_row_start.checked_add(c4_row_len) else {
return false;
};
- if target_public_signals.len() < c4_row_end + FIELD_BYTE_LEN {
+ let Some(c4_required_len) = c4_row_end.checked_add(FIELD_BYTE_LEN) else {
+ return false;
+ };
+ if target_public_signals.len() < c4_required_len {
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let Some(c4_row_start) = src_idx | |
| .checked_mul(l) | |
| .and_then(|offset| offset.checked_mul(FIELD_BYTE_LEN)) | |
| else { | |
| return false; | |
| }; | |
| let Some(c4_row_len) = l.checked_mul(FIELD_BYTE_LEN) else { | |
| return false; | |
| }; | |
| let Some(c4_row_end) = c4_row_start.checked_add(c4_row_len) else { | |
| return false; | |
| }; | |
| if target_public_signals.len() < c4_row_end + FIELD_BYTE_LEN { | |
| let Some(c4_row_start) = src_idx | |
| .checked_mul(l) | |
| .and_then(|offset| offset.checked_mul(FIELD_BYTE_LEN)) | |
| else { | |
| return false; | |
| }; | |
| let Some(c4_row_len) = l.checked_mul(FIELD_BYTE_LEN) else { | |
| return false; | |
| }; | |
| let Some(c4_row_end) = c4_row_start.checked_add(c4_row_len) else { | |
| return false; | |
| }; | |
| let Some(c4_required_len) = c4_row_end.checked_add(FIELD_BYTE_LEN) else { | |
| return false; | |
| }; | |
| if target_public_signals.len() < c4_required_len { | |
| return false; | |
| } |
🤖 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/commitment_links/c2_to_c4/mod.rs` around lines 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.
| self.states.remove(e3_id); | ||
| let _ = self.bus.publish( | ||
| E3Failed { | ||
| e3_id: e3_id.clone(), | ||
| failed_at_stage: E3Stage::CommitteeFinalized, | ||
| reason: FailureReason::DKGInvalidShares, | ||
| }, | ||
| ec, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not delete aggregation state before terminal failure publication succeeds.
The code removes self.states[e3_id] and discards the E3Failed publication result. If bus.publish fails, the E3 has no terminal event and no retained state for retry. Publish the failure first. If publication fails, retain or queue the state for a retry.
🤖 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/node_proof_aggregation/effects.rs` around lines 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.
…ommittee/canonical shapes Audit follow-up across C2/C3/C4/C5/C7 and downstream construction sites: - F1/F2/F3: thread chunk_size (DEFAULT_C2_CHUNK_SIZE) through C2/C3/C4 circuit data and sample/computation; C3/C4 also emit SHARE_COMPUTATION_CHUNK_SIZE / SHARE_COMPUTATION_N_CHUNKS from their configs, requiring a recompile for non-512 chunk sizes. Ranges verified against canonical committee (CiphernodesCommittee). - F7: validate C4 recipient_party_id against canonical.n in DKG, matching the C3 row/coeff fixes from the prior commit. - F8: drop DKG n/l duplication from preset metadata; pk / share_encryption / share_decryption generate_configs now take &Configs holding n/l/chunk_size; apply threshold_l correction to C3. - C7 (threshold decrypted_shares_aggregation): enforce exact T+1 shares, reconstructing_parties length/bounds/unique (BTreeSet), and non-zero-coeff drop check vs max_msg_non_zero_coeffs. - C5 pk_aggregation: assert canonical.h instead of raw h. - C3 mod_idx/row_index now bound by threshold moduli len, not DKG moduli (insecure-512 DKG L=1, threshold L=2). - Wire new chunk_size/committee fields into multithread (3 sites) and node_fold_witness test data; update e3-events input layout test to the 4 C3 public inputs. - Add secure-preset codegen test; delete obsolete CHUNKED_C2_PLAN. - Flow-trace: document chunk-size loading across C2/C3/C4, N/L sourcing, 512 default, and recompile-on-non-512 warning.
Add per-step wall-time tracking to the chunked DKG share-computation prove path (chunks, batches, finalize), mirroring the existing NodeDkgFold timing pattern, and emit them in the multithread report as ZkShareComputation/*. Update the benchmark report generator to put fold and share-computation sub-steps under a single "Operation sub-steps" section and relabel the isolated C2 rows to make clear they measure a single chunk circuit. Regenerate results_insecure_minimum report; drop obsolete results and the old monolithic C2 circuits in favour of the chunked build.
… benches Parallelize the independent chunk proofs (16 for secure degree) with rayon par_iter and the per-batch aggregation proofs (4) with par_chunks; both are IndexedParallelIterators so collect preserves the ordering the finalizer needs. Emit per-step prove timings (chunks, batches, finalize) so the benchmark report can attribute the C2 regression to the aggregation tree. Also switch the active committee to micro (benchmark run config) and add the regenerated secure-micro results.
1 participant
EXPERIMENTAL WIP DO NOT MERGE!