feat: make script for compilation to take --all flag [skip-line-limit]#1578
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads committee size from threshold parameters through aggregators, ZK request structs, proof builders, multithread handlers, and test/build artifact paths; adds per-committee artifact directories and extends the Noir build script and benchmark CLI for committee variants. ChangesCommittee-size threading through aggregation and proof generation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/zk-prover/src/actors/proof_verification.rs (1)
107-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInsert into
pendingonly after the preset/committee lookup succeeds.If
self.presetsmisses thise3_id, this branch returns after inserting intoself.pending, and that entry is never removed because no ZK request was dispatched. After a restart or missedCiphernodeSelected, each rejected key leaks stale pending state for(e3_id, party_id).🤖 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/actors/proof_verification.rs` around lines 107 - 123, Currently the code inserts into self.pending (via self.pending.insert(...) creating a PendingVerification for (msg.e3_id.clone(), msg.key.party_id)) before checking self.presets.get(&msg.e3_id), which can cause leaked pending entries when the preset is missing; move the self.pending.insert call so it occurs only after the lookup succeeds (after let Some((preset, committee)) = self.presets.get(&msg.e3_id).copied() else { ... }), remove the pre-check insertion, and only construct PendingVerification and call preset.artifacts_dir_for_committee(committee.as_str()) once the preset/committee is present so rejected keys do not leave stale entries for (msg.e3_id, msg.key.party_id).scripts/build-circuits.ts (3)
109-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--preset all/--committee allbypasses the unsupported-pair guard.The constructor only calls
isPresetCommitteeSupported(...)when both options are concrete. In the new nested build loop, unsupported combinations can still slip through, even though this file already documents that such pairs emit stub parity matrices and invalid proofs. Re-check each(preset, committee)insidebuildAll()orbuildForPreset().Also applies to: 139-146
🤖 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 109 - 115, The guard for unsupported (preset, committee) pairs only runs for concrete options at construction, so when buildAll() or buildForPreset() iterates nested presets/committees it can still emit invalid combinations; call isPresetCommitteeSupported(preset, committee) inside the nested loop in buildAll() and/or at the start of buildForPreset() and skip (or throw) for unsupported pairs instead of proceeding to build, ensuring you reference the same options.preset/options.committee values when deciding to continue; update both places mentioned (the new nested build loop and the buildForPreset() entry) so every generated (preset, committee) is validated.
122-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChecksum manifests no longer identify which committee each artifact belongs to.
buildAll()now merges compiled outputs from multiple committees into oneresult.compiled, but the checksum manifest still emits paths shaped like<preset>/<variant>/...without the committee segment. Under--committee all, entries for different committees collide inchecksums.json/SHA256SUMS, and the recorded paths no longer match the actual files written under<preset>/<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 `@scripts/build-circuits.ts` around lines 122 - 150, The checksum manifest ignores the committee segment because buildAll merges outputs from multiple committees into result.compiled; update either buildForPreset to embed the committee into each compiled artifact's output path or modify generateChecksumFile to read a committee property from each compiled entry and include it in the emitted path (so manifest entries become <preset>/<committee>/<variant>/...). Locate buildForPreset, result.compiled, and generateChecksumFile in scripts/build-circuits.ts and ensure the produced path used by generateChecksumFile includes the committee segment (when this.options.committee === 'all' or committees contains multiple entries) so checksums reference the actual file locations.
457-539:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe active-bin stamp can record
"all"instead of the concrete committee incircuits/bin/.
writeActiveBinPresetStamp()serializesthis.options.committee, so a--committee allrun writes{ committee: "all" }on every iteration even thoughcircuits/binonly contains the last committee built. That breaks consumers that use.active-preset.jsonto determine the staged ABI. Pass the current loop'scommitteeinto the stamp writer.🤖 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 457 - 539, The active-bin stamp is being written using this.options.committee causing "--committee all" runs to record "all" instead of the specific committee just built; update the writeActiveBinPresetStamp call in buildForPreset (inside the block after copyArtifacts where currently this.writeActiveBinPresetStamp(preset, sourceHash) is invoked) to pass the current loop's committee (this.writeActiveBinPresetStamp(preset, sourceHash, committee)) and update the writeActiveBinPresetStamp function signature/implementation to accept and serialize that committee parameter instead of reading this.options.committee.crates/zk-prover/src/circuits/aggregation/node_dkg_fold.rs (1)
364-372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the new
committeeargument against the request payload before selecting artifacts.These builders now trust
committeeto choosepreset/committee/..., but they never check it against the other committee-shaped inputs they already have. A mismatched caller can therefore load the wrong circuit ABI/VKs and fail later with opaque witness/proof errors. At minimum, reject when the selected committee's registered-party count does not matchcommittee_addresses.len().Also applies to: 493-503
🤖 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 364 - 372, The code currently picks artifacts via preset.artifacts_dir_for_committee(committee.as_str()) without checking that the chosen CiphernodesCommitteeSize actually matches the payload; before calling artifacts_dir_for_committee, fetch the expected registered-party count from the CiphernodesCommitteeSize value (use its method that returns the registered party count) and compare it to input.committee_addresses.len(); if they differ, return an Err(ZkError::InvalidInput/validation error) instead of proceeding. Apply the same validation in the other identical block mentioned (the second prove_dkg_aggregation-like usage around the 493-503 region) so both locations reject mismatched committee sizes before loading presets/VKs.
🧹 Nitpick comments (2)
crates/zk-helpers/src/ciphernodes_committee.rs (1)
55-64: ⚡ Quick winAdd direct regression tests for
from_n_hmapping.This new constructor is part of the committee-size contract surface, but there’s no direct test proving canonical pairs map correctly and unknown pairs error.
✅ Suggested test addition
#[cfg(test)] mod tests { use super::*; @@ + #[test] + fn from_n_h_maps_canonical_rows_and_rejects_unknown() { + assert_eq!(CiphernodesCommitteeSize::from_n_h(3, 3).unwrap(), CiphernodesCommitteeSize::Micro); + assert_eq!(CiphernodesCommitteeSize::from_n_h(5, 5).unwrap(), CiphernodesCommitteeSize::Small); + assert_eq!(CiphernodesCommitteeSize::from_n_h(10, 8).unwrap(), CiphernodesCommitteeSize::Medium); + assert_eq!(CiphernodesCommitteeSize::from_n_h(20, 15).unwrap(), CiphernodesCommitteeSize::Large); + assert!(CiphernodesCommitteeSize::from_n_h(10, 7).is_err()); + }🤖 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/ciphernodes_committee.rs` around lines 55 - 64, Add unit tests that cover CommitteeSize::from_n_h: assert that from_n_h(3,3) -> Ok(CommitteeSize::Micro), (5,5) -> Ok(Small), (10,8) -> Ok(Medium), (20,15) -> Ok(Large), and that an unknown pair (e.g. from_n_h(4,2)) returns Err. Place tests in the module tests (or #[cfg(test)] mod tests) alongside ciphernodes_committee.rs, using the same enum/constructor names (CommitteeSize and from_n_h) so they fail if mappings change or error handling regresses.crates/fhe-params/src/presets.rs (1)
396-402: ⚡ Quick winAdd a unit test for committee-scoped artifact path formatting.
artifacts_dir_for_committeeis now part of the runtime routing contract but isn’t covered by tests in this file.✅ Suggested test addition
#[cfg(test)] mod tests { use super::*; @@ + #[test] + fn test_artifacts_dir_for_committee() { + assert_eq!( + BfvPreset::InsecureThreshold512.artifacts_dir_for_committee("micro"), + "insecure-512/micro" + ); + assert_eq!( + BfvPreset::SecureThreshold8192.artifacts_dir_for_committee("large"), + "secure-8192/large" + ); + }🤖 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/fhe-params/src/presets.rs` around lines 396 - 402, Add a unit test in this module that constructs a Preset instance and asserts artifacts_dir_for_committee returns the expected string by comparing it to format!("{}/{}", preset.artifacts_dir(), committee). In the test call preset.artifacts_dir_for_committee with a couple of representative committee names (e.g. "medium", "micro") and assert equality to the explicit format; use the existing Preset constructor present in this file to create the preset and place the test in the #[cfg(test)] tests module in this file.
🤖 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/aggregator/src/ext.rs`:
- Around line 87-88: The code currently calls
CiphernodesCommitteeSize::from_threshold(...).unwrap_or(CiphernodesCommitteeSize::Micro),
which silently masks invalid/unknown threshold configs; change these sites to
fail fast instead: replace unwrap_or(Micro) with explicit error handling that
returns a Result or panics with a clear message (e.g., using expect or mapping
the Err into a startup error) so misconfiguration surfaces during
hydrate/startup of both publickey and plaintext aggregator wiring; update the
occurrences where committee_size is assigned (the
CiphernodesCommitteeSize::from_threshold calls) to propagate an error upward or
terminate with a descriptive message rather than defaulting to Micro.
In `@crates/multithread/src/multithread.rs`:
- Around line 699-701: Several earlier ZK handler code paths still call
params_preset.artifacts_dir(), causing them to look in the legacy directory;
update those to use the committee-specific path. Replace uses of
req.params_preset.artifacts_dir() with
req.params_preset.artifacts_dir_for_committee(req.committee_size.as_str())
(e.g., where artifacts_dir is assigned in the C0–C7 handlers and the ranges
around the noted spots such as the blocks referenced around artifacts_dir
assignment, and the ranges 753-759 and 788-796). Ensure each handler (the
functions/blocks that currently call params_preset.artifacts_dir()) assigns
artifacts_dir using artifacts_dir_for_committee and that any downstream logic
still references that local artifacts_dir variable unchanged.
In `@crates/zk-prover/src/actors/proof_verification.rs`:
- Around line 169-173: The code currently defaults unknown (m,n) pairs to
CiphernodesCommitteeSize::Micro via unwrap_or, which masks invalid thresholds;
change the logic around
CiphernodesCommitteeSize::from_threshold(data.threshold_m, data.threshold_n) so
that unknown pairs are rejected instead of mapped to Micro: match the
Result/Option returned by from_threshold (or remove unwrap_or) and on None/Err
return or propagate an error (or skip insertion and log) rather than inserting a
Micro committee; update the call site where
self.presets.insert(data.e3_id.clone(), (data.params_preset, committee)) is
performed to only insert when a valid committee is returned.
In `@scripts/build-circuits.ts`:
- Around line 408-422: The check in isDistPresetUpToDate keys stamps by
"preset/committee" but requiredDistMarkers and hydrateBinFromDist still use the
old preset-only dist path; update those functions to construct and check dist
paths under dist/circuits/<preset>/<committee>/... (not
dist/circuits/<preset>/...) so requiredDistMarkers(), hydrateBinFromDist(), and
any other dist-readers look in the committee subdirectory consistent with
readPresetStamp/readActiveBinPreset; ensure path construction uses the same
preset+committee combination used by isDistPresetUpToDate and isPresetUpToDate.
---
Outside diff comments:
In `@crates/zk-prover/src/actors/proof_verification.rs`:
- Around line 107-123: Currently the code inserts into self.pending (via
self.pending.insert(...) creating a PendingVerification for (msg.e3_id.clone(),
msg.key.party_id)) before checking self.presets.get(&msg.e3_id), which can cause
leaked pending entries when the preset is missing; move the self.pending.insert
call so it occurs only after the lookup succeeds (after let Some((preset,
committee)) = self.presets.get(&msg.e3_id).copied() else { ... }), remove the
pre-check insertion, and only construct PendingVerification and call
preset.artifacts_dir_for_committee(committee.as_str()) once the preset/committee
is present so rejected keys do not leave stale entries for (msg.e3_id,
msg.key.party_id).
In `@crates/zk-prover/src/circuits/aggregation/node_dkg_fold.rs`:
- Around line 364-372: The code currently picks artifacts via
preset.artifacts_dir_for_committee(committee.as_str()) without checking that the
chosen CiphernodesCommitteeSize actually matches the payload; before calling
artifacts_dir_for_committee, fetch the expected registered-party count from the
CiphernodesCommitteeSize value (use its method that returns the registered party
count) and compare it to input.committee_addresses.len(); if they differ, return
an Err(ZkError::InvalidInput/validation error) instead of proceeding. Apply the
same validation in the other identical block mentioned (the second
prove_dkg_aggregation-like usage around the 493-503 region) so both locations
reject mismatched committee sizes before loading presets/VKs.
In `@scripts/build-circuits.ts`:
- Around line 109-115: The guard for unsupported (preset, committee) pairs only
runs for concrete options at construction, so when buildAll() or
buildForPreset() iterates nested presets/committees it can still emit invalid
combinations; call isPresetCommitteeSupported(preset, committee) inside the
nested loop in buildAll() and/or at the start of buildForPreset() and skip (or
throw) for unsupported pairs instead of proceeding to build, ensuring you
reference the same options.preset/options.committee values when deciding to
continue; update both places mentioned (the new nested build loop and the
buildForPreset() entry) so every generated (preset, committee) is validated.
- Around line 122-150: The checksum manifest ignores the committee segment
because buildAll merges outputs from multiple committees into result.compiled;
update either buildForPreset to embed the committee into each compiled
artifact's output path or modify generateChecksumFile to read a committee
property from each compiled entry and include it in the emitted path (so
manifest entries become <preset>/<committee>/<variant>/...). Locate
buildForPreset, result.compiled, and generateChecksumFile in
scripts/build-circuits.ts and ensure the produced path used by
generateChecksumFile includes the committee segment (when this.options.committee
=== 'all' or committees contains multiple entries) so checksums reference the
actual file locations.
- Around line 457-539: The active-bin stamp is being written using
this.options.committee causing "--committee all" runs to record "all" instead of
the specific committee just built; update the writeActiveBinPresetStamp call in
buildForPreset (inside the block after copyArtifacts where currently
this.writeActiveBinPresetStamp(preset, sourceHash) is invoked) to pass the
current loop's committee (this.writeActiveBinPresetStamp(preset, sourceHash,
committee)) and update the writeActiveBinPresetStamp function
signature/implementation to accept and serialize that committee parameter
instead of reading this.options.committee.
---
Nitpick comments:
In `@crates/fhe-params/src/presets.rs`:
- Around line 396-402: Add a unit test in this module that constructs a Preset
instance and asserts artifacts_dir_for_committee returns the expected string by
comparing it to format!("{}/{}", preset.artifacts_dir(), committee). In the test
call preset.artifacts_dir_for_committee with a couple of representative
committee names (e.g. "medium", "micro") and assert equality to the explicit
format; use the existing Preset constructor present in this file to create the
preset and place the test in the #[cfg(test)] tests module in this file.
In `@crates/zk-helpers/src/ciphernodes_committee.rs`:
- Around line 55-64: Add unit tests that cover CommitteeSize::from_n_h: assert
that from_n_h(3,3) -> Ok(CommitteeSize::Micro), (5,5) -> Ok(Small), (10,8) ->
Ok(Medium), (20,15) -> Ok(Large), and that an unknown pair (e.g. from_n_h(4,2))
returns Err. Place tests in the module tests (or #[cfg(test)] mod tests)
alongside ciphernodes_committee.rs, using the same enum/constructor names
(CommitteeSize and from_n_h) so they fail if mappings change or error handling
regresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b01b61a0-214b-4193-814d-02d70dac304f
📒 Files selected for processing (14)
circuits/benchmarks/scripts/run_benchmarks.shcrates/aggregator/src/actors/publickey_aggregator.rscrates/aggregator/src/actors/threshold_plaintext_aggregator.rscrates/aggregator/src/ext.rscrates/events/src/enclave_event/compute_request/zk.rscrates/fhe-params/src/presets.rscrates/multithread/src/multithread.rscrates/zk-helpers/src/ciphernodes_committee.rscrates/zk-prover/src/actors/node_proof_aggregator.rscrates/zk-prover/src/actors/proof_verification.rscrates/zk-prover/src/circuits/aggregation/node_dkg_fold.rscrates/zk-prover/src/domain/node_dkg_fold.rscrates/zk-prover/tests/common/helpers.rsscripts/build-circuits.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/zk-prover/src/actors/proof_verification.rs (1)
107-122:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove pending-cache insertion after preset lookup.
At Line 107, pending state is inserted before the preset guard at Line 115. If preset is missing, the function returns without dispatching verification, leaving stale
pendingentries.Suggested fix
- // Store the signed payload so we can reference it in the verification response - self.pending.insert( - (msg.e3_id.clone(), msg.key.party_id), - PendingVerification { - signed_payload: validated.signed_payload, - recovered_signer: validated.recovered_signer, - }, - ); - let Some((preset, committee)) = self.presets.get(&msg.e3_id).copied() else { error!( "No BfvPreset known for e3_id={} — cannot determine circuit artifacts directory. \ This can happen if CiphernodeSelected was missed (e.g. after restart). Rejecting key from party {}.", msg.e3_id, msg.key.party_id ); return; }; + // Store pending only when we are actually dispatching verification. + self.pending.insert( + (msg.e3_id.clone(), msg.key.party_id), + PendingVerification { + signed_payload: validated.signed_payload, + recovered_signer: validated.recovered_signer, + }, + ); let artifacts_dir = preset.artifacts_dir_for_committee(committee.as_str());🤖 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/actors/proof_verification.rs` around lines 107 - 122, The pending insertion currently happens before checking for a preset, which can leave stale entries if the guard returns early; move the self.pending.insert call and creation of PendingVerification (the tuple key using msg.e3_id and msg.key.party_id and values validated.signed_payload / validated.recovered_signer) to after the preset lookup that uses self.presets.get(&msg.e3_id).copied() (the block that logs the "No BfvPreset known for e3_id=..." error) so insertion only occurs once the preset and committee are present and verification will be dispatched.
🤖 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.
Outside diff comments:
In `@crates/zk-prover/src/actors/proof_verification.rs`:
- Around line 107-122: The pending insertion currently happens before checking
for a preset, which can leave stale entries if the guard returns early; move the
self.pending.insert call and creation of PendingVerification (the tuple key
using msg.e3_id and msg.key.party_id and values validated.signed_payload /
validated.recovered_signer) to after the preset lookup that uses
self.presets.get(&msg.e3_id).copied() (the block that logs the "No BfvPreset
known for e3_id=..." error) so insertion only occurs once the preset and
committee are present and verification will be dispatched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c77d62e6-2f5d-498e-ad67-bbc507bbb813
📒 Files selected for processing (4)
crates/aggregator/src/ext.rscrates/multithread/src/multithread.rscrates/zk-prover/src/actors/proof_verification.rsscripts/build-circuits.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/build-circuits.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/tests/integration.rs`:
- Around line 382-386: The current fallback reuses circuits/bin when
circuits_bin_marker exists but doesn't verify it matches the requested preset;
update the check around uses_dist_preset_artifacts/preset_build_stamp to
validate the active bin stamp contents against the requested preset (use
resolve_preset_stamp_path, preset_subdir and committee_str) before trusting
circuits_bin_marker. Concretely: read/parse the active preset stamp (the file
pointed to by circuits_bin_marker or the .active-preset.json in circuits/bin),
compare its stored preset identifier/committee to preset_subdir and
committee_str (or compare to preset_build_stamp identity), and only skip
rebuilding/copying if the stamp matches; otherwise treat it as missing and fall
through to copy/build as when !preset_build_stamp.exists().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6f65efc-c78e-4bcd-910c-570524d27f77
📒 Files selected for processing (2)
crates/tests/tests/integration.rscrates/zk-prover/tests/local_e2e_tests.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/zk-prover/src/actors/proof_verification.rs (1)
62-62: ⚡ Quick winRemove or use
circuits_baseinProofVerificationActor
crates/zk-prover/src/actors/proof_verification.rsstorescircuits_base: PathBufin the actor and threads it throughnew/setup, butself.circuits_baseis never read anywhere in the actor file. If it isn’t intended for artifact-path derivation/other logic yet, drop the field + constructor parameter (and thebackend.circuits_dir.clone()argument incrates/zk-prover/src/actors/mod.rs).🤖 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/actors/proof_verification.rs` at line 62, The ProofVerificationActor struct contains an unused field circuits_base (and its constructor parameter) that is only threaded through new/setup but never read; remove the circuits_base field from ProofVerificationActor, delete the corresponding parameter in ProofVerificationActor::new and any uses in setup, and remove the argument backend.circuits_dir.clone() passed when constructing the actor in actors::mod (or adjust the callsite to match the new constructor signature) so there are no unused fields or parameters left.examples/CRISP/scripts/lib/dev_config.sh (1)
83-97: ⚡ Quick winConsider whether silent fallback is the desired behavior when circuit artifacts are missing.
The function returns 0 (success) when the expected circuit artifacts at
${src}/recursive/dkg/pk/pk.jsondon't exist, with a message directing users to runpnpm dev:setupfirst. This silent fallback means dependent operations won't fail, which could hide configuration issues if a developer expects the sync to ensure artifacts are present.If the intent is to gracefully fall back to release artifacts downloaded by
enclave noir setup, the current behavior is appropriate. Otherwise, consider returning a non-zero exit code to surface missing artifacts as an error.🤖 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/scripts/lib/dev_config.sh` around lines 83 - 97, The sync_enclave_circuit_artifacts function currently returns success (0) when circuit artifacts are missing which silently masks failures; change it to either return a non-zero exit status to surface the missing-artifact error or make the behavior configurable (e.g. check an env flag like CRISP_ALLOW_RELEASE_FALLBACK) so that by default missing `${src}/recursive/dkg/pk/pk.json` causes the function to exit non‑zero (and print the same guidance message), while when the flag allows fallback it continues as today; update the existence-check branch inside sync_enclave_circuit_artifacts to implement the chosen behavior and ensure callers handle the non-zero return appropriately..github/workflows/ci.yml (1)
775-798: 💤 Low valueReassess the SHA-pinning request for the new Nargo/Barretenberg steps
The concern isn’t consistent with the existing workflow: in .github/workflows/ci.yml, only
akhilmhdh/contributors-readme-action@1ff4c56187458b34cd602aee93e897344ce34bfcis pinned to a commit SHA, while other actions (includingactions/cache@v4andactions/checkout@v6) use version tags. Keep these as-is unless the repo has an explicit documented policy requiring SHA pins.🤖 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 @.github/workflows/ci.yml around lines 775 - 798, Leave the new workflow steps as version-tagged actions rather than converting them to commit-SHA pins: keep the "Install Nargo" step using noir-lang/noirup@v0.1.4, keep the "Cache Barretenberg binary" step using actions/cache@v4 (id: cache-bb), and leave the "Install Barretenberg (bb)" and "Build enclave circuits" steps unchanged; only change to SHA pins is acceptable if the repository has an explicit documented policy requiring SHA-pinning—otherwise match existing convention where only akhilmhdh/contributors-readme-action is SHA-pinned.
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 775-798: Leave the new workflow steps as version-tagged actions
rather than converting them to commit-SHA pins: keep the "Install Nargo" step
using noir-lang/noirup@v0.1.4, keep the "Cache Barretenberg binary" step using
actions/cache@v4 (id: cache-bb), and leave the "Install Barretenberg (bb)" and
"Build enclave circuits" steps unchanged; only change to SHA pins is acceptable
if the repository has an explicit documented policy requiring
SHA-pinning—otherwise match existing convention where only
akhilmhdh/contributors-readme-action is SHA-pinned.
In `@crates/zk-prover/src/actors/proof_verification.rs`:
- Line 62: The ProofVerificationActor struct contains an unused field
circuits_base (and its constructor parameter) that is only threaded through
new/setup but never read; remove the circuits_base field from
ProofVerificationActor, delete the corresponding parameter in
ProofVerificationActor::new and any uses in setup, and remove the argument
backend.circuits_dir.clone() passed when constructing the actor in actors::mod
(or adjust the callsite to match the new constructor signature) so there are no
unused fields or parameters left.
In `@examples/CRISP/scripts/lib/dev_config.sh`:
- Around line 83-97: The sync_enclave_circuit_artifacts function currently
returns success (0) when circuit artifacts are missing which silently masks
failures; change it to either return a non-zero exit status to surface the
missing-artifact error or make the behavior configurable (e.g. check an env flag
like CRISP_ALLOW_RELEASE_FALLBACK) so that by default missing
`${src}/recursive/dkg/pk/pk.json` causes the function to exit non‑zero (and
print the same guidance message), while when the flag allows fallback it
continues as today; update the existence-check branch inside
sync_enclave_circuit_artifacts to implement the chosen behavior and ensure
callers handle the non-zero return appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af197f06-40e2-44e0-b590-92df188ce6fd
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.github/workflows/ci.ymlcircuits/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/scripts/check_circuit_preset_artifacts.shcircuits/benchmarks/scripts/extract_crisp_verify_gas.shcircuits/benchmarks/scripts/run_benchmarks.shcrates/aggregator/src/actors/publickey_aggregator.rscrates/aggregator/src/actors/threshold_plaintext_aggregator.rscrates/aggregator/src/ext.rscrates/events/src/enclave_event/compute_request/zk.rscrates/events/src/enclave_event/encryption_key_pending.rscrates/events/src/enclave_event/share_verification.rscrates/evm/src/contracts.rscrates/fhe-params/src/presets.rscrates/keyshare/src/actors/threshold_keyshare.rscrates/keyshare/src/domain/decryption_key_calculation.rscrates/multithread/src/multithread.rscrates/slashing/Cargo.tomlcrates/slashing/src/domain/accusation_voting.rscrates/tests/tests/integration.rscrates/zk-prover/src/actors/mod.rscrates/zk-prover/src/actors/proof_request.rscrates/zk-prover/src/actors/proof_verification.rscrates/zk-prover/src/actors/share_verification.rscrates/zk-prover/src/circuits/aggregation/node_dkg_fold.rscrates/zk-prover/src/domain/proof_request.rscrates/zk-prover/src/domain/share_verification.rscrates/zk-prover/src/prover.rsexamples/CRISP/scripts/dev_cipher.shexamples/CRISP/scripts/lib/dev_config.shexamples/CRISP/scripts/setup.shtemplates/default/package.jsontemplates/default/scripts/dev_ciphernodes.shtemplates/default/scripts/lib/dev_config.shtemplates/default/scripts/setup.sh
✅ Files skipped from review due to trivial changes (4)
- crates/slashing/Cargo.toml
- circuits/benchmarks/results_insecure_agg_micro/benchmark_run_meta.json
- crates/events/src/enclave_event/encryption_key_pending.rs
- circuits/benchmarks/results_insecure_agg_micro/report.md
🚧 Files skipped from review as they are similar to previous changes (17)
- crates/events/src/enclave_event/share_verification.rs
- crates/zk-prover/src/domain/proof_request.rs
- crates/zk-prover/src/actors/proof_request.rs
- circuits/benchmarks/scripts/run_benchmarks.sh
- crates/fhe-params/src/presets.rs
- crates/zk-prover/src/domain/share_verification.rs
- circuits/benchmarks/scripts/check_circuit_preset_artifacts.sh
- crates/zk-prover/src/circuits/aggregation/node_dkg_fold.rs
- circuits/benchmarks/scripts/extract_crisp_verify_gas.sh
- crates/aggregator/src/ext.rs
- crates/zk-prover/src/actors/share_verification.rs
- crates/multithread/src/multithread.rs
- crates/keyshare/src/actors/threshold_keyshare.rs
- crates/events/src/enclave_event/compute_request/zk.rs
- crates/aggregator/src/actors/publickey_aggregator.rs
- crates/aggregator/src/actors/threshold_plaintext_aggregator.rs
- crates/tests/tests/integration.rs
ctrlc03
approved these changes
Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests
Bug Fixes