feat: dual circuits architecture poseidon & keccak#1382
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:
📝 WalkthroughWalkthroughAdds a CircuitFlavor enum (Default, Recursive, Evm); makes circuit artifact directories and VK filenames flavor-aware across prover, build scripts, and tests; validates VK hash contents; removes placeholder circuit creation on download failure; and expands CI artifact checks to include additional VK-related patterns. Changes
Sequence DiagramsequenceDiagram
participant User
participant Prover
participant CircuitsDir as "CircuitsDir Resolver (flavor)"
participant ArtifactLoader as "Artifact Loader (reads .vk/.vk_hash)"
participant Verifier as "Prover/Verifier CLI"
User->>Prover: generate_proof_with_flavor(circuit, witness, flavor)
Prover->>CircuitsDir: circuits_dir(flavor)
CircuitsDir-->>Prover: path (.../<flavor>/...)
Prover->>ArtifactLoader: load circuit JSON + VkArtifacts (path)
ArtifactLoader->>ArtifactLoader: read vk and vk_hash (validate 32 bytes)
ArtifactLoader-->>Prover: circuit + VkArtifacts (key_hash)
Prover->>Verifier: prove/verify using flavor.verifier_target()
Verifier-->>Prover: proof/result
Prover-->>User: return Proof/result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/zk-prover/src/circuits/recursive_aggregation/vk.rs (1)
31-41:⚠️ Potential issue | 🟡 MinorDocument
.vk_hashfile format or add format validation.Lines 31 and 40 assume
.vk_hashis raw 32-byte binary. Without test fixtures or documentation confirming this format, future changes to howbb write_vkemits artifacts could break silently. Either add a test fixture to validate the format, document the expected format explicitly, or add defensive logic to handle both raw binary and hex-encoded text formats (checkingvk_hash_bytes.len()for 32 vs. 64+ and decoding hex if needed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/circuits/recursive_aggregation/vk.rs` around lines 31 - 41, The code assumes vk_hash_bytes is raw 32-byte binary which can break if .vk_hash is hex text; update the parsing in the block that checks vk_hash_bytes.len() (and the surrounding logic that produces key_hash) to accept both formats: if length == 32 treat as raw bytes, else if length >= 64 or the bytes are ASCII hex (possibly with "0x" prefix or newline) attempt to parse hex (trim whitespace/newline, strip 0x, then hex::decode) and verify it yields 32 bytes, otherwise return ZkError::InvalidInput mentioning vk_hash_path and the detected format; ensure key_hash is still created as "0x" + hex::encode(&vk_hash_bytes) after decoding and keep bytes_to_field_strings(&vk_bytes)? unchanged. This adds defensive handling for hex-encoded files while preserving existing raw-binary behavior.crates/zk-prover/src/prover.rs (1)
230-257:⚠️ Potential issue | 🟠 MajorVerification API is now flavor-ambiguous and easy to misuse.
Line 230–257 keeps
verify_proof()as default-only, while EVM proofs share the sameProoftype and can be passed accidentally. That silently routes to the wrong VK/target path.Suggested hardening
pub fn verify_proof(&self, proof: &Proof, e3_id: &str, party_id: u64) -> Result<bool, ZkError> { - self.verify_proof_impl( - proof.circuit, - &proof.data, - &proof.public_signals, - proof.circuit.dir_path(), - e3_id, - party_id, - CircuitFlavor::Default, - ) + self.verify_proof_with_flavor(proof, e3_id, party_id, CircuitFlavor::Default) } +pub fn verify_proof_with_flavor( + &self, + proof: &Proof, + e3_id: &str, + party_id: u64, + flavor: CircuitFlavor, +) -> Result<bool, ZkError> { + self.verify_proof_impl( + proof.circuit, + &proof.data, + &proof.public_signals, + proof.circuit.dir_path(), + e3_id, + party_id, + flavor, + ) +} + pub fn verify_evm_proof( &self, proof: &Proof, e3_id: &str, party_id: u64, ) -> Result<bool, ZkError> { - self.verify_proof_impl( - proof.circuit, - &proof.data, - &proof.public_signals, - proof.circuit.dir_path(), - e3_id, - party_id, - CircuitFlavor::Evm, - ) + self.verify_proof_with_flavor(proof, e3_id, party_id, CircuitFlavor::Evm) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/prover.rs` around lines 230 - 257, The public APIs verify_proof and verify_evm_proof are ambiguous because they accept the same Proof type and can silently verify with the wrong flavor; add an explicit runtime flavor check before delegating to verify_proof_impl: in verify_proof assert/provide an error if proof.circuit.flavor != CircuitFlavor::Default, and in verify_evm_proof error if proof.circuit.flavor != CircuitFlavor::Evm, returning a descriptive ZkError rather than proceeding; reference the existing functions verify_proof, verify_evm_proof, verify_proof_impl, the Proof.circuit (and its dir_path()) and the CircuitFlavor enum to locate where to add the checks.
🧹 Nitpick comments (4)
crates/zk-prover/src/backend/download.rs (1)
109-114: Redundant error wrapping produces nested messages.
download_with_progressalready returnsZkError::DownloadFailedfor all failure paths. Wrappingein anotherZkError::DownloadFailedproduces messages like:
"Failed to download <url>: could not download circuits: Failed to download <url>: HTTP 404"Consider propagating the error directly for consistency with
download_bb()(line 45):♻️ Suggested simplification
Err(e) => { - return Err(ZkError::DownloadFailed( - url, - format!("could not download circuits: {}", e), - )); + return Err(e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/src/backend/download.rs` around lines 109 - 114, The current Err branch in download_with_progress redundantly wraps an existing ZkError::DownloadFailed into a new DownloadFailed, causing nested messages; instead propagate the existing error directly (return Err(e)) to match download_bb's behavior — locate the error handling in download_with_progress and replace the wrapping return Err(ZkError::DownloadFailed(...)) with returning the original error e when it is already a ZkError::DownloadFailed (or simply return Err(e) if the function signature allows)..github/workflows/ci.yml (1)
660-662: Also validate VK hash artifacts in this CI gate.On Line 660 and Line 662, the check now asserts
*.vkand*.vk_recursive, but it still skips*.vk_hash/*.vk_recursive_hash. Since runtime VK loading now depends on hash files, this step should fail early if hashes are missing.🔧 Suggested update
- ls -la circuits/bin/dkg/target/*.json circuits/bin/dkg/target/*.vk circuits/bin/dkg/target/*.vk_recursive + ls -la circuits/bin/dkg/target/*.json circuits/bin/dkg/target/*.vk circuits/bin/dkg/target/*.vk_hash circuits/bin/dkg/target/*.vk_recursive circuits/bin/dkg/target/*.vk_recursive_hash ... - ls -la circuits/bin/threshold/target/*.json circuits/bin/threshold/target/*.vk circuits/bin/threshold/target/*.vk_recursive + ls -la circuits/bin/threshold/target/*.json circuits/bin/threshold/target/*.vk circuits/bin/threshold/target/*.vk_hash circuits/bin/threshold/target/*.vk_recursive circuits/bin/threshold/target/*.vk_recursive_hash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 660 - 662, The CI step that lists artifacts currently checks for "*.vk" and "*.vk_recursive" but omits the derived hash artifacts; update the artifact validation to also include "*.vk_hash" and "*.vk_recursive_hash" so the ls commands (the lines that list circuits/bin/dkg/target/*.vk and circuits/bin/dkg/target/*.vk_recursive and the threshold equivalents) will fail the job when hash files are missing, ensuring runtime VK loading prerequisites are validated early.scripts/build-circuits.ts (1)
389-425: Consider iteratingALL_FLAVORSinstead of hardcoding two flavor branches.Line 389-425 and Line 443-461 duplicate flavor-specific checksum/copy logic. This works now, but adding another flavor later can silently produce incomplete artifacts/checksums unless both blocks are manually updated.
Also applies to: 443-461
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-circuits.ts` around lines 389 - 425, Replace the duplicated hardcoded "evm" and "default" branches by iterating over ALL_FLAVORS and applying flavor-specific mapping rules: for each flavor in ALL_FLAVORS compute prefix = `${CIRCUIT_FLAVORS[flavor]}/${c.group}/${c.name}`, then for each possible artifact/checksum pair use the existing c.artifacts and c.checksums to add entries to checksums and lines (using basename for file names where appropriate); for the "default" flavor apply the special filename mappings (map c.checksums.vkRecursive -> `${packageName}.vk` and c.checksums.vkRecursiveHash -> `${packageName}.vk_hash`) while other flavors (e.g., "evm") keep the basename of the artifact files, ensuring you reference symbols c, checksums, lines, basename, packageName and CIRCUIT_FLAVORS and remove the duplicated blocks.crates/events/src/enclave_event/proof.rs (1)
40-47: Add serde rename to normalizeCircuitFlavorserialization.
CircuitFlavoris markedSerialize/Deserializebut serde will serialize variants as"Default"/"Evm"(PascalCase), whileas_str()and flavor directories use lowercase"default"/"evm". AlthoughCircuitFlavoris not currently serialized to wire/config formats, adding#[serde(rename_all = "lowercase")]aligns the serialized representation with the canonical string form and prevents inconsistencies if this enum is exposed in future APIs.#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize, Default)] +#[serde(rename_all = "lowercase")] pub enum CircuitFlavor {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/enclave_event/proof.rs` around lines 40 - 47, Add serde rename to the CircuitFlavor enum so its serialized variant names match the lowercase canonical strings used by as_str() and flavor directories; specifically, annotate the enum CircuitFlavor with #[serde(rename_all = "lowercase")] (keeps existing Serialize/Deserialize) so "Default"/"Evm" serialize as "default"/"evm" and avoids future mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/zk-prover/src/circuits/recursive_aggregation/mod.rs`:
- Around line 414-447: The code checks artifact paths using dist.join("default")
instead of the configured backend.circuits_dir, so tests can miss valid
artifacts located under backend.circuits_dir/default; change the path base to
use backend.circuits_dir (e.g., set default_dist =
backend.circuits_dir.join("default") or otherwise derive pk_wrapper,
share_enc_wrapper, and fold_dir from backend.circuits_dir) so the initial
existence checks and the later panics use the same directory the backend was
configured with (symbols to update: default_dist, pk_wrapper, share_enc_wrapper,
fold_dir, where backend.circuits_dir is set before ZkProver::new).
---
Outside diff comments:
In `@crates/zk-prover/src/circuits/recursive_aggregation/vk.rs`:
- Around line 31-41: The code assumes vk_hash_bytes is raw 32-byte binary which
can break if .vk_hash is hex text; update the parsing in the block that checks
vk_hash_bytes.len() (and the surrounding logic that produces key_hash) to accept
both formats: if length == 32 treat as raw bytes, else if length >= 64 or the
bytes are ASCII hex (possibly with "0x" prefix or newline) attempt to parse hex
(trim whitespace/newline, strip 0x, then hex::decode) and verify it yields 32
bytes, otherwise return ZkError::InvalidInput mentioning vk_hash_path and the
detected format; ensure key_hash is still created as "0x" +
hex::encode(&vk_hash_bytes) after decoding and keep
bytes_to_field_strings(&vk_bytes)? unchanged. This adds defensive handling for
hex-encoded files while preserving existing raw-binary behavior.
In `@crates/zk-prover/src/prover.rs`:
- Around line 230-257: The public APIs verify_proof and verify_evm_proof are
ambiguous because they accept the same Proof type and can silently verify with
the wrong flavor; add an explicit runtime flavor check before delegating to
verify_proof_impl: in verify_proof assert/provide an error if
proof.circuit.flavor != CircuitFlavor::Default, and in verify_evm_proof error if
proof.circuit.flavor != CircuitFlavor::Evm, returning a descriptive ZkError
rather than proceeding; reference the existing functions verify_proof,
verify_evm_proof, verify_proof_impl, the Proof.circuit (and its dir_path()) and
the CircuitFlavor enum to locate where to add the checks.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 660-662: The CI step that lists artifacts currently checks for
"*.vk" and "*.vk_recursive" but omits the derived hash artifacts; update the
artifact validation to also include "*.vk_hash" and "*.vk_recursive_hash" so the
ls commands (the lines that list circuits/bin/dkg/target/*.vk and
circuits/bin/dkg/target/*.vk_recursive and the threshold equivalents) will fail
the job when hash files are missing, ensuring runtime VK loading prerequisites
are validated early.
In `@crates/events/src/enclave_event/proof.rs`:
- Around line 40-47: Add serde rename to the CircuitFlavor enum so its
serialized variant names match the lowercase canonical strings used by as_str()
and flavor directories; specifically, annotate the enum CircuitFlavor with
#[serde(rename_all = "lowercase")] (keeps existing Serialize/Deserialize) so
"Default"/"Evm" serialize as "default"/"evm" and avoids future mismatches.
In `@crates/zk-prover/src/backend/download.rs`:
- Around line 109-114: The current Err branch in download_with_progress
redundantly wraps an existing ZkError::DownloadFailed into a new DownloadFailed,
causing nested messages; instead propagate the existing error directly (return
Err(e)) to match download_bb's behavior — locate the error handling in
download_with_progress and replace the wrapping return
Err(ZkError::DownloadFailed(...)) with returning the original error e when it is
already a ZkError::DownloadFailed (or simply return Err(e) if the function
signature allows).
In `@scripts/build-circuits.ts`:
- Around line 389-425: Replace the duplicated hardcoded "evm" and "default"
branches by iterating over ALL_FLAVORS and applying flavor-specific mapping
rules: for each flavor in ALL_FLAVORS compute prefix =
`${CIRCUIT_FLAVORS[flavor]}/${c.group}/${c.name}`, then for each possible
artifact/checksum pair use the existing c.artifacts and c.checksums to add
entries to checksums and lines (using basename for file names where
appropriate); for the "default" flavor apply the special filename mappings (map
c.checksums.vkRecursive -> `${packageName}.vk` and c.checksums.vkRecursiveHash
-> `${packageName}.vk_hash`) while other flavors (e.g., "evm") keep the basename
of the artifact files, ensuring you reference symbols c, checksums, lines,
basename, packageName and CIRCUIT_FLAVORS and remove the duplicated blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14f202f4-f2e5-4217-b1fb-0aaee4a358ad
📒 Files selected for processing (12)
.github/workflows/ci.ymlcrates/events/src/enclave_event/proof.rscrates/zk-prover/src/backend/download.rscrates/zk-prover/src/circuits/recursive_aggregation/mod.rscrates/zk-prover/src/circuits/recursive_aggregation/vk.rscrates/zk-prover/src/lib.rscrates/zk-prover/src/prover.rscrates/zk-prover/src/traits.rscrates/zk-prover/tests/common/helpers.rscrates/zk-prover/tests/integration_tests.rsscripts/build-circuits.tsscripts/circuit-constants.ts
cedoor
reviewed
Mar 5, 2026
cedoor
left a comment
cedoor
left a comment
Contributor
There was a problem hiding this comment.
Ideally, inner proofs should be noir_recursive, whereas wrapper & fold proofs could be noir_recursive_non_zk, which should allow more efficient verification.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/build-circuits.ts (1)
292-294: Make fold handling explicit in VK-generation branching.Current predicate only treats
wrapper/*as wrapper-like, but comments and intended flow treat fold similarly. Consider explicitly classifying fold to avoid unintended recursive/noir artifact generation for fold.Suggested refactor
@@ - private isWrapper(circuit: CircuitInfo): boolean { + private isWrapper(circuit: CircuitInfo): boolean { return circuit.name.startsWith('wrapper/') } + + private isWrapperOrFold(circuit: CircuitInfo): boolean { + return this.isWrapper(circuit) || (circuit.group === CIRCUIT_GROUPS.AGGREGATION && circuit.name === 'fold') + } @@ - const isWrapper = this.isWrapper(circuit) + const isWrapperLike = this.isWrapperOrFold(circuit) @@ - if (!isWrapper) { + if (!isWrapperLike) { @@ - } else { - // Wrapper/fold circuits: noir-recursive-no-zk only (Default flavor) + } else { + // Wrapper/fold circuits: noir-recursive-no-zk only (Default flavor)Also applies to: 350-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-circuits.ts` around lines 292 - 294, The isWrapper(CircuitInfo) predicate currently only checks circuit.name.startsWith('wrapper/') which misses circuits named 'fold/*' and causes fold circuits to be treated like regular circuits; update isWrapper to explicitly detect fold (e.g., circuit.name.startsWith('fold/') or circuit.kind === 'fold' if such metadata exists) and adjust the VK-generation branching logic (the same pattern used later around the VK-generation code paths) to treat fold circuits the same as wrappers to prevent recursive/noir artefact generation; ensure all places referencing isWrapper (including the VK-generation branch) use the updated predicate so fold is handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/zk-prover/tests/common/helpers.rs`:
- Around line 49-51: In setup_compiled_circuit in helpers.rs the test helper
only copies the {circuit_name}.vk files into the flavor directories; update the
routine to also copy the corresponding {circuit_name}.vk_hash files wherever
{circuit_name}.vk is copied (specifically for vk_evm_path, vk_recursive_path,
vk_noir_path and the other similar copy blocks for lines ~64-127), ensuring both
.vk and .vk_hash are present in each flavor directory so recursive aggregation
VK loaders can find the hash artifacts.
---
Nitpick comments:
In `@scripts/build-circuits.ts`:
- Around line 292-294: The isWrapper(CircuitInfo) predicate currently only
checks circuit.name.startsWith('wrapper/') which misses circuits named 'fold/*'
and causes fold circuits to be treated like regular circuits; update isWrapper
to explicitly detect fold (e.g., circuit.name.startsWith('fold/') or
circuit.kind === 'fold' if such metadata exists) and adjust the VK-generation
branching logic (the same pattern used later around the VK-generation code
paths) to treat fold circuits the same as wrappers to prevent recursive/noir
artefact generation; ensure all places referencing isWrapper (including the
VK-generation branch) use the updated predicate so fold is handled consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6325ccdd-56ed-4328-af8b-7e291e90eba9
📒 Files selected for processing (8)
crates/events/src/enclave_event/proof.rscrates/zk-prover/src/circuits/recursive_aggregation/mod.rscrates/zk-prover/src/circuits/recursive_aggregation/vk.rscrates/zk-prover/src/prover.rscrates/zk-prover/src/traits.rscrates/zk-prover/tests/common/helpers.rsscripts/build-circuits.tsscripts/circuit-constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/zk-prover/src/traits.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/zk-prover/tests/common/helpers.rs (1)
62-64:⚠️ Potential issue | 🟠 MajorSelect fallback artifacts only when both VK and VK-hash exist.
Line 100 and Line 134 choose sources based on
.vkonly, then Line 111 and Line 147 treat.vk_hashas optional. This can produce partial artifacts (.vkwithout.vk_hash), but runtime loaders require both with no fallback.Suggested patch
@@ assert!( vk_evm_path.exists(), "evm verification key not found: {} (run `pnpm build:circuits` to compile)", vk_evm_path.display() ); + assert!( + vk_evm_hash_path.exists(), + "evm verification key hash not found: {} (run `pnpm build:circuits` to compile)", + vk_evm_hash_path.display() + ); @@ - if vk_evm_hash_path.exists() { - fs::copy( - &vk_evm_hash_path, - evm_dir.join(format!("{circuit_name}.vk_hash")), - ) - .await - .unwrap(); - } + fs::copy( + &vk_evm_hash_path, + evm_dir.join(format!("{circuit_name}.vk_hash")), + ) + .await + .unwrap(); @@ - let (default_vk_src, default_hash_src) = if vk_recursive_path.exists() { + let (default_vk_src, default_hash_src) = if vk_recursive_path.exists() + && vk_recursive_hash_path.exists() + { (&vk_recursive_path, &vk_recursive_hash_path) - } else { + } else if vk_evm_path.exists() && vk_evm_hash_path.exists() { (&vk_evm_path, &vk_evm_hash_path) + } else { + panic!( + "missing default VK/hash pair for {circuit_name} in {}", + target_dir.display() + ); }; @@ - if default_hash_src.exists() { - fs::copy( - default_hash_src, - default_dir.join(format!("{circuit_name}.vk_hash")), - ) - .await - .unwrap(); - } + fs::copy( + default_hash_src, + default_dir.join(format!("{circuit_name}.vk_hash")), + ) + .await + .unwrap(); @@ - let (recursive_vk_src, recursive_hash_src) = if vk_noir_path.exists() { + let (recursive_vk_src, recursive_hash_src) = if vk_noir_path.exists() + && vk_noir_hash_path.exists() + { (&vk_noir_path, &vk_noir_hash_path) - } else if vk_recursive_path.exists() { + } else if vk_recursive_path.exists() && vk_recursive_hash_path.exists() { (&vk_recursive_path, &vk_recursive_hash_path) - } else { + } else if vk_evm_path.exists() && vk_evm_hash_path.exists() { (&vk_evm_path, &vk_evm_hash_path) + } else { + panic!( + "missing recursive VK/hash pair for {circuit_name} in {}", + target_dir.display() + ); }; @@ - if recursive_hash_src.exists() { - fs::copy( - recursive_hash_src, - recursive_dir.join(format!("{circuit_name}.vk_hash")), - ) - .await - .unwrap(); - } + fs::copy( + recursive_hash_src, + recursive_dir.join(format!("{circuit_name}.vk_hash")), + ) + .await + .unwrap();Also applies to: 100-104, 111-118, 134-140, 147-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/tests/common/helpers.rs` around lines 62 - 64, The selection logic currently treats the presence of a .vk file as sufficient to choose an artifact source while treating the corresponding .vk_hash as optional; update the checks so that artifact fallback is only chosen when both vk_evm_path and vk_evm_hash_path (and their prover equivalents) exist. Locate the variables vk_evm_path and vk_evm_hash_path (and the prover counterparts) in crates/zk-prover/tests/common/helpers.rs and change the boolean conditions that decide the source to require both files (e.g., replace checks like vk_evm_path.exists() with vk_evm_path.exists() && vk_evm_hash_path.exists()); also adjust any error/assert messages to reflect that both files are required and remove code paths that proceed when only the .vk exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/zk-prover/tests/common/helpers.rs`:
- Around line 62-64: The selection logic currently treats the presence of a .vk
file as sufficient to choose an artifact source while treating the corresponding
.vk_hash as optional; update the checks so that artifact fallback is only chosen
when both vk_evm_path and vk_evm_hash_path (and their prover equivalents) exist.
Locate the variables vk_evm_path and vk_evm_hash_path (and the prover
counterparts) in crates/zk-prover/tests/common/helpers.rs and change the boolean
conditions that decide the source to require both files (e.g., replace checks
like vk_evm_path.exists() with vk_evm_path.exists() &&
vk_evm_hash_path.exists()); also adjust any error/assert messages to reflect
that both files are required and remove code paths that proceed when only the
.vk exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06269021-d967-40b9-9795-8859795ed500
📒 Files selected for processing (2)
.github/workflows/ci.ymlcrates/zk-prover/tests/common/helpers.rs
cedoor
reviewed
Mar 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
3 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.
closes #1381
Adds
CircuitFlavorenum (Default/Evm) splitting compiled artifacts intocircuits/default/(poseidon) andcircuits/evm/(keccak, for on-chain verification). ZkProver::circuits_dir(flavor) replaces the old flat layout. Node now fails to start if circuits can't be downloaded.prove()/generate_proof()recursive/noir-recursive(with ZK blinding)aggregate_proof()generate_wrapper_proof()default/(wrapper dir)noir-recursive-no-zkgenerate_fold_proof()default/noir-recursive-no-zkgenerate_final_fold_proof()evm/evm(keccak, on-chain)verify_proof()recursive/noir-recursive(matchesprove())verify_wrapper_proof()default/(wrapper dir)noir-recursive-no-zkverify_fold_proof()default/noir-recursive-no-zkverify_evm_proof()evm/evmSummary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
CI