feat: add e3-noir-prover crate for proof generation [skip-line-limit]#1211
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant KS as ThresholdKeyshare
participant PRA as ProofRequestActor
participant MT as Multithread
participant ZK as ZkProver
participant BB as Barretenberg
KS->>PRA: EncryptionKeyPending(e3_id, key, preset)
activate PRA
PRA->>PRA: create CorrelationId & store pending
PRA->>MT: publish ComputeRequest::zk(PkBfvProofRequest, corr_id)
activate MT
MT->>ZK: handle_zk_request(PkBfvProofRequest)
activate ZK
ZK->>ZK: build witness from PublicKey
ZK->>BB: run Barretenberg to generate proof
BB-->>ZK: proof + public_signals
ZK-->>MT: ComputeResponse::zk(PkBfvProofResponse, corr_id)
MT-->>PRA: ComputeResponse with proof
PRA->>PRA: match correlation_id, attach proof to key
PRA->>KS: publish EncryptionKeyCreated(e3_id, key+proof)
deactivate PRA
sequenceDiagram
participant Net as DocumentPublisher
participant PVA as ProofVerificationActor
participant ZKA as ZkActor
participant ZK as ZkProver
participant BB as Barretenberg
Net->>PVA: EncryptionKeyReceived(e3_id, key+proof)
activate PVA
alt verifier available
PVA->>ZKA: ZkVerificationRequest(proof, key, e3_id)
activate ZKA
ZKA->>ZK: verify_proof(circuit, proof_data, public_signals)
activate ZK
ZK->>BB: verify proof
BB-->>ZK: verification result
ZK-->>ZKA: boolean result
ZKA-->>PVA: ZkVerificationResponse(verified, e3_id, key)
alt verified
PVA->>Net: publish EncryptionKeyCreated(external: true)
else failed
PVA->>PVA: log error, do not publish
end
deactivate ZKA
else no verifier
PVA->>Net: publish EncryptionKeyCreated(external: true)
end
deactivate PVA
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
hmzakhalid
changed the title
hmzakhalid
changed the title
ctrlc03
force-pushed
the
feat/noir-prover
branch
from
33b73af to
3082d9e
Compare
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@crates/ciphernode-builder/src/ciphernode_builder.rs`:
- Around line 423-435: The code currently hardcodes BfvPreset::InsecureDkg512
when creating ThresholdKeyshareExtension (see BfvPreset::InsecureDkg512 and
ThresholdKeyshareExtension::create in ciphernode_builder.rs), which is unsafe
for production; either add a clear TODO comment next to that line documenting
the planned migration to secure parameters (e.g., BfvPreset::SecureDkg8192) or,
preferably, make the preset configurable by adding a builder method (e.g.,
with_share_enc_preset(BfvPreset)) on the same builder that provides
with_zkproof(), store the chosen preset in self, and replace the hardcoded
BfvPreset::InsecureDkg512 with that field when calling
ThresholdKeyshareExtension::create.
In `@crates/cli/src/noir.rs`:
- Around line 111-134: The force flag is ignored in execute_setup: when force is
true you print "Force reinstalling..." but then call ZkBackend::ensure_installed
which currently bails out if SetupStatus::Ready; either modify ensure_installed
to accept a force: bool parameter and pass force from execute_setup (update its
signature and internal logic to skip the readiness short-circuit when
force==true), or bypass ensure_installed in the force branch and call
ZkBackend::download_bb() and ZkBackend::download_circuits() (and any other
install steps) directly from execute_setup; locate these by the function names
execute_setup, ensure_installed, download_bb, download_circuits and update the
caller and/or callee accordingly so a true force actually reinstalls.
In `@crates/multithread/src/multithread.rs`:
- Around line 342-343: The comment is misleading and the code uses
BfvParamSet::from(req.params_preset.clone()).build_arc() without ensuring the
preset is a DKG preset; either update the comment to state that
req.params_preset MUST be a DKG preset or add runtime validation that rejects
non-DKG presets before building params (e.g., check the preset kind or that its
threshold_counterpart() exists/returns the expected DKG variant) and return an
error if validation fails so subsequent use of params (and any
threshold_counterpart() calls) is correct; locate the logic around
BfvParamSet::from and req.params_preset to add the validation or change the
comment accordingly.
In `@crates/zk-prover/src/actors/proof_request.rs`:
- Around line 112-120: The handler handle_compute_request_error currently
removes the pending entry for a failed ComputeRequestError (matching
ComputeRequestErrorKind::Zk) and only logs a warning, which leaves downstream
actors waiting; change it to publish an error event or a fallback
EncryptionKeyCreated so the pipeline continues: after detecting the ZK error and
removing self.pending (using msg.correlation_id()), call the event-bus publish
method (e.g., bus.err(...) with the error and correlation id) or publish an
EncryptionKeyCreated for that node without a proof (populate the same
correlation id and node id from the pending entry) so consumers receive a
terminal event; ensure you reference ComputeRequestError,
ComputeRequestErrorKind::Zk, handle_compute_request_error, self.pending, and
EncryptionKeyCreated when making the change.
In `@crates/zk-prover/src/backend/mod.rs`:
- Around line 65-67: The function work_dir_for currently joins an unsanitized
e3_id into the path (unlike cleanup_work_dir which validates first); move the
sanitization/validation into work_dir_for (or extract a shared helper like
validate_e3_id) and have work_dir_for call that helper to reject or normalize
unsafe inputs (e.g., disallow path separators, empty strings, .., or invalid
characters) before doing self.work_dir.join(sanitized_e3_id); update callers
(including cleanup_work_dir) to use the new helper/work_dir_for so all call
sites get the same protection.
🧹 Nitpick comments (28)
crates/Dockerfile (2)
68-68: Nit:zk-proverCOPY is out of alphabetical order.All other
COPYlines in this block follow roughly alphabetical order.zk-provershould be placed afterzk-helpers(Line 83) to stay consistent.COPY crates/parity-matrix/Cargo.toml ./parity-matrix/Cargo.toml -COPY crates/zk-prover/Cargo.toml ./zk-prover/Cargo.toml COPY crates/polynomial/Cargo.toml ./polynomial/Cargo.toml ... COPY crates/zk-helpers/Cargo.toml ./zk-helpers/Cargo.toml +COPY crates/zk-prover/Cargo.toml ./zk-prover/Cargo.toml
110-139: Runtime stage does not include thebb(Barretenberg) binary.The PR objectives note "bundle bb binary in Docker for production" as a remaining task. Currently the runtime image has no
bbbinary, no circuit artifacts under~/.enclave/noir/, and no download step — so ZK proof generation will fail at runtime in Docker. Just flagging this as a reminder since it's already tracked.crates/cli/Cargo.toml (1)
27-27: Nit:e3-zk-provershould come aftere3-support-scriptsalphabetically.e3-init = { workspace = true } -e3-zk-prover = { workspace = true } e3-support-scripts = { workspace = true } +e3-zk-prover = { workspace = true }crates/fhe-params/src/presets.rs (1)
314-326: Clean inverse ofdkg_counterpart— consider adding a unit test.The mapping is correct and symmetric with
dkg_counterpart. A small round-trip test would lock in the invariant thatpreset.dkg_counterpart().and_then(|d| d.threshold_counterpart()) == Some(preset)for threshold presets (and vice-versa).💡 Optional test for symmetry
#[test] fn threshold_dkg_counterparts_are_symmetric() { for preset in BfvPreset::PAIR_PRESETS { let dkg = preset.dkg_counterpart().expect("pair preset has DKG counterpart"); assert_eq!(dkg.threshold_counterpart(), Some(preset)); } }crates/zk-prover/src/error.rs (2)
63-64: Nit: inconsistent error message capitalization.
"checksum missing for {0}"starts lowercase while all other variants use sentence-case (e.g.,"Checksum mismatch for {file}").Proposed fix
- #[error("checksum missing for {0}")] + #[error("Checksum missing for {0}")]
51-52:TomlErroronly covers serialization errors; consider adding deserialization support if TOML reading is added later.The
#[from]conversion is fortoml::ser::Erroronly. Currently the crate only serializes TOML (inputs.toml), so this works. If the crate evolves to read/parse TOML files, you'll need to handletoml::de::Errorseparately or add another variant.crates/zk-prover/src/prover.rs (2)
31-37: Consider returning&Pathinstead of&PathBuf.Idiomatic Rust prefers returning
&Pathfrom accessors sincePathBufauto-derefs toPath, and&Pathis the more general borrowed form.♻️ Suggested change
- pub fn circuits_dir(&self) -> &PathBuf { + pub fn circuits_dir(&self) -> &std::path::Path { &self.circuits_dir } - pub fn work_dir(&self) -> &PathBuf { + pub fn work_dir(&self) -> &std::path::Path { &self.work_dir }
205-219: Unit test coverage is minimal.There is only one test (
test_prover_requires_bb), which validates a single error path. Consider adding tests for other validation paths (e.g., missing circuit file, missing VK file) that don't require a realbbbinary.crates/zk-prover/tests/fixtures/pk.json (1)
1-65: Large compiled circuit fixture checked into the repository.This file is a large compiled Noir circuit artifact. The ABI correctly marks
pk0isandpk1isas private inputs with a publicFieldreturn type, consistent with the commitment scheme's hiding property. Based on learnings, pk0is and pk1is in the pk_bfv circuit should remain private inputs because the commitment scheme's hiding property requires these values to stay private.Consider whether this fixture could be downloaded from the circuit releases (as done for production) or generated at test time to reduce repository size, though acknowledging it's acceptable for test stability.
crates/zk-prover/src/circuits/pkbfv.rs (1)
54-75: Coefficient conversion goes through string round-trip.Each polynomial coefficient is converted via
b.to_string()→FieldElement::try_from_str(&s). While this works correctly, it allocates a string per coefficient (up to 2048+ allocations per proof). IfFieldElementhas afrom_be_bytesor similar API, a direct conversion would be more efficient — but this is acceptable for the current use case where proof generation dominates runtime.crates/zk-prover/tests/local_e2e_tests.rs (2)
24-45:find_bbrelies onwhich, which is not portable.
whichis a Unix utility. On non-Unix platforms (or minimal containers), it may not exist. Since the test is already Unix-only (#[cfg(unix)]symlink on Line 61), consider either:
- Gating the entire test module with
#[cfg(unix)], or- Using
std::process::Command::new("bb").arg("--version")to probe directly instead ofwhich.This is low-priority since the tests effectively skip on non-Unix platforms anyway.
66-106: Consider extracting the repeated fixture setup into a shared helper.All three tests duplicate the same pattern: find bb → setup prover → create circuit dir → copy pk.json + pk.vk. A helper like
setup_pk_bfv_test() -> Option<(ZkBackend, ZkProver, TempDir)>would reduce ~20 lines of boilerplate per test.Also applies to: 108-149, 151-213
crates/zk-prover/tests/integration_tests.rs (1)
97-133: Consider adding a#[tokio::test]timeout to prevent indefinite hangs on network failures.These tests depend on network access. If the remote server is slow or unreachable, the test will hang indefinitely. Tokio's test macro doesn't have a built-in timeout, but you could wrap the body with
tokio::time::timeoutor use a CI-level timeout..github/workflows/ci.yml (1)
392-394: Consider caching the ZK prover artifacts across CI jobs to avoid repeated downloads.Each job independently runs
enclave noir setup, which downloads the BB binary and circuits from GitHub Releases. With multiple jobs running this step, you're making redundant network calls on every CI run. You could cache~/.enclave/noir/usingactions/cachekeyed on the versions.json hash.Also applies to: 595-597, 802-804
crates/zk-prover/src/actors/proof_request.rs (1)
30-31: Pending map entries have no TTL — orphaned entries will leak if no response or error arrives.If neither
ComputeResponsenorComputeRequestErroris ever received for a givencorrelation_id(e.g., the multithread actor crashes or the message is lost), thependingHashMap entry persists indefinitely. Over time with repeated failures, this could cause unbounded memory growth.Consider adding a periodic cleanup or timeout mechanism. This isn't critical for initial release but worth tracking.
Also applies to: 66-73
crates/zk-prover/src/backend/download.rs (2)
104-116: Placeholder version "0.0.0-placeholder" is a good improvement over recording the real version.This ensures
circuits_match()returnsfalseso a retry is triggered on the next setup. However, the save on line 113 still persists version info — consider whether the version file should be left untouched on failure socheck_statusreportsFullSetupNeededrather thanCircuitsNeedUpdate.
164-207:download_with_progressbuffers the entire download in memory.For large circuit archives, this could consume significant memory. Consider streaming to a temporary file instead of accumulating in a
Vec<u8>. This is acceptable for now given the likely sizes involved, but worth noting for future scalability.crates/zk-prover/src/backend/mod.rs (2)
31-38: Consider reducing field visibility.All fields of
ZkBackendarepub. If only specific fields need external access (e.g.,bb_binaryandcircuits_dirfor display in CLI), consider exposing them via getters instead. This would prevent external code from mutating paths inconsistently.
69-83: Path traversal guard is good but incomplete.The sanitization rejects
..,/, and\ine3_id, which covers the most common traversal vectors. However, null bytes (\0) could also be problematic on some systems. Consider also rejecting empty strings.🛡️ Suggested enhancement
pub async fn cleanup_work_dir(&self, e3_id: &str) -> Result<(), ZkError> { // Sanitize e3_id to prevent path traversal - if e3_id.contains("..") || e3_id.contains('/') || e3_id.contains('\\') { + if e3_id.is_empty() + || e3_id.contains("..") + || e3_id.contains('/') + || e3_id.contains('\\') + || e3_id.contains('\0') + { return Err(ZkError::IoError(std::io::Error::new( std::io::ErrorKind::InvalidInput, "e3_id contains invalid characters", ))); }crates/cli/src/noir.rs (1)
12-19: Consider addingProveandVerifysubcommands.The PR description mentions the CLI should support
prove,verify, andhelpcommands, but onlyStatusandSetupare implemented. If those are planned for a future PR, consider adding a comment or TODO here.crates/events/src/enclave_event/compute_request/mod.rs (2)
71-89: Prefer implementingDisplayoverToString.
ToStringis deprecated in favor of implementingDisplay(which auto-derivesToString). This is a pre-existing pattern in the file, so not blocking.
121-133: Error messages intry_into_zk/try_into_trbfvhardcode the "other" variant name.If a third
ComputeResponseKindvariant is added, these error messages become inaccurate. Consider using{:?}formatting on the variant or a generic "unexpected variant" message.Minor suggestion
pub fn try_into_zk(self) -> anyhow::Result<ZkResponse> { match self.response { ComputeResponseKind::Zk(zk) => Ok(zk), - _ => bail!("Expected ZkResponse but got TrBFV"), + other => bail!("Expected ZkResponse but got {:?}", std::mem::discriminant(&other)), } }crates/zk-prover/src/config.rs (2)
200-220:verify_checksumsilently passes when no expected checksum is provided.This is correct behavior for optional checksums, but combined with the fail-open
fetch_or_default, it creates a chain where network failure → empty checksums → all verification skipped. Ensure at least thedownload_bbpath always has checksums available (e.g., embedded defaults).
96-119:fetch_latestcreates a newreqwest::Clientper call.If
fetch_latestorfetch_or_defaultis called frequently, consider reusing a client. For a one-shot config fetch this is fine, but worth noting if usage patterns change.crates/keyshare/src/threshold_keyshare.rs (4)
404-418: Catch-all arm silently swallows unknownTrBFVResponsevariants.Line 415's
_ => Ok(())means any newTrBFVResponsevariant added in the future will be silently ignored here without a compiler warning. Consider logging a warning for unrecognised variants to aid debugging.💡 Suggested improvement
- _ => Ok(()), + other => { + warn!("Unhandled TrBFVResponse variant: {:?}", other); + Ok(()) + }
428-430: Silently discardingResultfrom collector setup.Both
ensure_collectorandensure_encryption_key_collectorreturnResult, and the errors are discarded withlet _ =. Ifself.state.get()returnsNoneat this point, the collectors won't be created and no diagnostic will be emitted. Since these are retried lazily later, this isn't a functional bug, but at minimum awarn!on failure would help diagnose startup ordering issues.💡 Suggested improvement
- let _ = self.ensure_collector(address.clone()); - let _ = self.ensure_encryption_key_collector(address.clone()); + if let Err(e) = self.ensure_collector(address.clone()) { + warn!("Early collector setup failed (will retry): {e}"); + } + if let Err(e) = self.ensure_encryption_key_collector(address.clone()) { + warn!("Early encryption key collector setup failed (will retry): {e}"); + }
432-432: RepeatedBfvParamSet::from(self.share_enc_preset…).build_arc()in three places.Lines 432, 684, and 760 all perform the same preset-to-params conversion. A small helper on
ThresholdKeyshare(e.g.,fn share_enc_params(&self) -> Arc<BfvParameters>) would eliminate the duplication and make future changes to the conversion a single-point edit.Also applies to: 684-684, 760-760
441-458: State snapshot used after mutation — works correctly but is subtle.
stateis captured at line 441 (before the mutation at line 444), thenstate.party_idis read at line 456 to construct the published event. This is safe becauseparty_idlives on the outerThresholdKeyshareStateand is invariant across transitions, but it's easy to misread as a stale-state bug. A brief inline comment would help future readers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/config/src/paths_engine.rs`:
- Around line 173-182: The fallback in get_enclave_dir currently builds the path
from default_data_dir.parent().join(".enclave"), which yields
~/.local/share/.enclave instead of the intended ~/.enclave; update
get_enclave_dir to use the user home directory (e.g. dirs::home_dir() or
dirs_next::home_dir()) and join(".enclave") when get_root_dir() is None,
ensuring you still handle None from home_dir() (fall back to default_data_dir as
last resort), or alternatively update the doc comment to explicitly state that
default_data_dir.parent().join(".enclave") is intended; refer to
get_enclave_dir, default_data_dir and OsDirs::data_dir() when making the change.
🧹 Nitpick comments (3)
crates/zk-prover/src/backend/mod.rs (2)
61-77:with_default_dirduplicates path layout already defined inPathsEngine.
PathsEngine(incrates/config/src/paths_engine.rs, lines 184-201) already centralizes the~/.enclave/noir/directory structure (noir_dir,bb_binary,circuits_dir,work_dir). Hardcoding the same".enclave"/"noir"/"bin"/"bb"etc. here means two places must be kept in sync.The entrypoint in
start.rsalready usesAppConfigpaths (e.g.,config.bb_binary()), sowith_default_dirappears to be a convenience for CLI/test paths that don't have anAppConfig. Consider delegating toPathsEngineto avoid divergence.
47-49:expect()in a public constructor can panic on pathological input.If
circuits_dirhas no parent (e.g., a bare filename or root path), this panics. SinceZkBackend::newis a public API called with external config, returningResult(or defaulting) would be safer.Proposed handling
- let base_dir = circuits_dir - .parent() - .expect("circuits_dir should have a parent") - .to_path_buf(); + let base_dir = circuits_dir + .parent() + .unwrap_or(&circuits_dir) + .to_path_buf();Or change the signature to return
Result<Self, ZkError>.crates/config/src/paths_engine.rs (1)
184-202: No test coverage for the new path accessors.The existing test infrastructure (
TestCase/PathsExpected) validatesconfig_file,key_file,db_file, andlog_file, but the newnoir_dir,bb_binary,circuits_dir, andwork_dirmethods have no test coverage. Given the nuanced fallback logic inget_enclave_dir, at least two cases should be tested: (1) with afound_config_file(project-local) and (2) without (global fallback).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/cli/src/noir.rs`:
- Around line 74-106: Status output currently displays only the single
SetupStatus returned by check_status(), so if both Barretenberg and circuits
need updates the user only sees one; update the logic to report both checks by
either changing check_status() to return a composite result (e.g., a struct or
bitflags) or by calling the individual check functions for Barretenberg and
circuits from the status command and printing both results; ensure you reference
and adjust the SetupStatus enum handling in the status display code (the match
on SetupStatus in noir.rs) so it can emit combined messages like "Barretenberg
needs update" and "Circuits need update" together, and keep the existing prompts
to run `enclave noir setup`.
- Around line 21-28: The execute function currently ignores AppConfig and always
initializes ZkBackend via ZkBackend::with_default_dir("test_node"), causing
hardcoded work paths; change execute to build the ZkBackend using values
resolved from the provided &AppConfig (use its PathsEngine-resolved node name
and paths such as bb_binary, circuits_dir and work_dir) and pass that backend
into the same logic instead of calling execute_without_config; keep
execute_without_config as a true no-config fallback (or at least replace the
literal "test_node" with a documented dev-only default) so callers with a config
get a backend constructed from AppConfig rather than the hardcoded path.
In `@crates/zk-prover/src/backend/mod.rs`:
- Around line 79-88: sanitize_e3_id currently allows empty strings and null
bytes which can lead to joining the root work_dir (deleting everything) or
truncation; update sanitize_e3_id to reject empty e3_id and any embedded NUL
characters: return Err(ZkError::IoError(...InvalidInput...)) if e3_id.is_empty()
or e3_id.contains('\0'), in addition to the existing checks for "..", '/' and
'\\' so cleanup_work_dir / work_dir.join(e3_id) cannot be passed an empty or
NUL-containing identifier.
🧹 Nitpick comments (7)
crates/zk-prover/src/backend/mod.rs (1)
46-50:expect()in a public constructor can panic on edge-case input.
circuits_dir.parent()returnsNonefor root paths (e.g.,/) or relative paths with a single component. Sincenewis public, a misconfigured caller could trigger this panic at runtime.Proposed fix — return `Result` or default gracefully
If changing
newto returnResultis too disruptive, at minimum guard against the edge case:- let base_dir = circuits_dir - .parent() - .expect("circuits_dir should have a parent") - .to_path_buf(); + let base_dir = circuits_dir + .parent() + .unwrap_or(&circuits_dir) + .to_path_buf();crates/zk-prover/src/prover.rs (1)
39-123: All I/O and subprocess calls ingenerate_proof/verify_proofare blocking.
std::fs::*andstd::process::Commandblock the calling thread. If these methods are invoked on a Tokio runtime (the tests use#[tokio::test]), they'll block the async executor. Consider usingtokio::task::spawn_blockingat the call sites, or switching totokio::fsandtokio::process::Commandinternally.Also applies to: 129-194
crates/zk-prover/tests/local_e2e_tests.rs (2)
86-96: Fixture copy block is duplicated three times — consider a helper.The
circuit_dircreation +pk.json/pk.vkcopy sequence is identical across all three tests. Extracting it would reduce maintenance and make adding new test fixtures simpler.♻️ Sketch
async fn copy_pk_fixtures(backend: &ZkBackend, fixtures: &std::path::Path) { let circuit_dir = backend.circuits_dir.join("dkg").join("pk"); fs::create_dir_all(&circuit_dir).await.unwrap(); fs::copy(fixtures.join("pk.json"), circuit_dir.join("pk.json")) .await .unwrap(); fs::copy(fixtures.join("pk.vk"), circuit_dir.join("pk.vk")) .await .unwrap(); }Also applies to: 128-138, 171-181
47-47: Nit: prefer&Pathover&PathBufin function signatures.Idiomatic Rust:
&Pathis the borrowed form ofPathBuf, analogous to&strvs&String.&PathBufauto-derefs but needlessly restricts the caller.-async fn setup_test_prover(bb: &PathBuf) -> (ZkBackend, tempfile::TempDir) { +async fn setup_test_prover(bb: &std::path::Path) -> (ZkBackend, tempfile::TempDir) {crates/config/src/paths_engine.rs (1)
180-198: No test coverage for the new noir path methods.The existing test suite validates
config_file,key_file,db_file, andlog_file, but none of the newnoir_dir,bb_binary,circuits_dir, orwork_dirmethods are tested. Consider extending theTestCase/PathsExpectedstruct to cover these paths, especially given the fallback logic inget_noir_base.crates/multithread/src/multithread.rs (1)
337-353: No validation thatreq.params_presetis a DKG preset.The comment on line 342 states the expectation but doesn't enforce it. If a threshold preset (e.g.,
InsecureThreshold512) is passed,PublicKey::from_byteswould attempt deserialization with threshold parameters (which may succeed or produce garbage), andthreshold_counterpart()would returnNone, triggering the silent fallback. Consider adding a guard.Proposed guard
+ if req.params_preset.threshold_counterpart().is_none() { + return Err(ComputeRequestError::new( + ComputeRequestErrorKind::Zk(ZkEventError::InvalidParams(format!( + "Expected a DKG preset, got {:?}", + req.params_preset + ))), + request.clone(), + )); + } + // NOTE: req.params_preset is expected to contain a DKG preset (e.g., InsecureDkg512)crates/cli/src/noir.rs (1)
7-7: Prefer explicit imports over glob import.
use anyhow::*pulls in everything from anyhow. The file only needsResultandanyhow!.Proposed fix
-use anyhow::*; +use anyhow::{anyhow, Result};
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/cli/src/noir.rs`:
- Around line 128-156: The force branch in execute_setup calls
backend.download_bb() and backend.download_circuits() without creating the
required directories that ensure_installed() would create, causing unpack
failures; fix by either invoking backend.ensure_installed().await at the start
of the force path (bypassing the prior status check) or explicitly calling
fs::create_dir_all() for the same directories ensure_installed() creates (e.g.,
base_dir, bin, circuits_dir, work_dir) before calling download_bb() and
download_circuits(); update map_err messages as needed and keep the rest of
execute_setup logic unchanged.
🧹 Nitpick comments (2)
crates/cli/src/noir.rs (2)
42-57: Consider adding a doc comment clarifying this is a dev/test fallback.The
"default"string is an improvement over the previous"test_node", but callers have no visibility into when this path is used or what directory it resolves to. A brief///doc comment explaining this is the no-config fallback (and what work directory it maps to) would help maintainability.
134-155: Prefer.context()over.map_err(|e| anyhow!(…))for idiomatic error chaining.The current
.map_err(|e| anyhow!("Failed to download bb: {}", e))pattern stringifies the underlying error, losing the causal chain. Using.context("Failed to download bb")(fromanyhow::Context) preserves the original error as a cause, which aids debugging.Proposed diff
+use anyhow::Context; + // In execute_setup: backend .download_bb() .await - .map_err(|e| anyhow!("Failed to download bb: {}", e))?; + .context("Failed to download bb")?; backend .download_circuits() .await - .map_err(|e| anyhow!("Failed to download circuits: {}", e))?; + .context("Failed to download circuits")?; } else { // ... backend .ensure_installed() .await - .map_err(|e| anyhow!("Setup failed: {}", e))?; + .context("Setup failed")?;The same applies to line 45 in
execute_without_config.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/zk-prover/src/config.rs`:
- Line 20: BB_VERSION in config.rs is set to a nightly pre-release
("3.0.0-nightly..."); change the constant BB_VERSION to a tagged stable
Barretenberg release string (e.g., "3.0.0" or the current official stable tag)
to avoid using nightly builds; update the BB_VERSION constant in config.rs (and
any build/packaging metadata that references it) so the project pins to that
stable tag.
- Around line 194-197: The current verify_circuit_checksum in
verify_circuit_checksum silently skips verification when the circuit name is
missing because expected is None; change it to explicitly check
self.circuits.get(circuit_name) and return an Err(ZkError::...) (e.g., a new or
existing CircuitNotFound/ManifestError variant) if the circuit entry is absent,
otherwise pass the found checksum string to verify_checksum as before
(reference: verify_circuit_checksum, self.circuits, verify_checksum).
- Around line 401-534: Mark the two network-heavy integration tests
test_download_and_verify_bb and test_download_checksum_mismatch_on_corruption so
they don't run by default: either add the #[ignore] attribute above each test
function (so they run only with cargo test -- --ignored) or gate them behind a
feature (e.g., #[cfg(feature = "download_tests")] plus enabling that feature in
CI when needed). Update the test annotations accordingly and, if using a feature
gate, document the feature in Cargo.toml so callers can enable it.
🧹 Nitpick comments (1)
crates/zk-prover/src/config.rs (1)
490-534: Corruption test downloads the full binary just to flip one byte — consider using synthetic data.
test_download_checksum_mismatch_on_corruptionre-downloads the entire tarball only to corrupt it and verify the mismatch path. This test doesn't validate anything about the real binary; it only exercisesverify_bb_checksumwith non-matching data. A unit test with a small synthetic byte array would be equivalent, faster, and deterministic.Suggested replacement
- #[tokio::test] - async fn test_download_checksum_mismatch_on_corruption() { - let Some(target) = BbTarget::current() else { - println!("skipping test: unsupported platform"); - return; - }; - - let version = BB_VERSION; - let (arch, os) = target.url_parts(); - let url = format!( - "https://github.com/AztecProtocol/aztec-packages/releases/download/v{version}/barretenberg-{arch}-{os}.tar.gz" - ); - - let client = reqwest::Client::new(); - let response = client - .get(&url) - .timeout(Duration::from_secs(120)) - .send() - .await - .expect("failed to send request"); - - let mut bytes = response - .bytes() - .await - .expect("failed to read body") - .to_vec(); - - // Corrupt the data - if !bytes.is_empty() { - bytes[0] ^= 0xFF; - } - - // Use a valid checksum that won't match corrupted data - let info = VersionInfo { - bb_checksum: Some( - "a56257d8edc226180f5a7093393e4adc99447368a65099bb34292bd261408b99".to_string(), - ), - ..Default::default() - }; - - let result = info.verify_bb_checksum(&bytes); - assert!(matches!(result, Err(ZkError::ChecksumMismatch { .. }))); - println!("correctly detected corrupted download"); - } + #[test] + fn test_verify_bb_checksum_mismatch_on_corruption() { + let data = b"some arbitrary binary payload"; + // Checksum that does NOT match the data above + let info = VersionInfo { + bb_checksum: Some( + "0000000000000000000000000000000000000000000000000000000000000000".to_string(), + ), + ..Default::default() + }; + + let result = info.verify_bb_checksum(data); + assert!(matches!(result, Err(ZkError::ChecksumMismatch { .. }))); + }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/zk-prover/src/config.rs`:
- Around line 409-466: The test uses BB_VERSION to download barretenberg but the
hard-coded checksums map (called checksums) is for an older v0.82.2, causing
verify_checksum (and the test_download_and_verify_bb) to always fail; fix by
either updating the checksums map entries to the correct SHA256 values for the
current BB_VERSION ("3.0.0-nightly.20251104") for each target key (amd64-linux,
amd64-darwin, arm64-linux, arm64-darwin), or change the test to derive the
download version from the checksums source (or a checksum file) instead of
BB_VERSION so the url/version and the checksums stay in sync — update the
checksums variable or the version assignment (version / url) and re-run
verify_checksum to ensure they match.
In `@crates/zk-prover/src/prover.rs`:
- Around line 69-75: The output directory (output_dir) is not created before
being passed to the bb prove -o flag; update the code that builds
job_dir/output_dir (the block creating job_dir, witness_path, and output_dir) to
call fs::create_dir_all(&output_dir) (or replace the existing
fs::create_dir_all(&job_dir) with fs::create_dir_all(&output_dir)) so the out
directory exists prior to running bb prove, matching the verify_proof pattern
and ensuring subsequent reads of proof and public_inputs succeed.
🧹 Nitpick comments (3)
crates/zk-prover/src/config.rs (1)
176-179: Non-atomic file write risks partial/corrupt state on crash.
save()writes directly to the target path. If the process is interrupted mid-write,version.jsoncould end up truncated or empty. Consider writing to a temporary file in the same directory, then renaming atomically.♻️ Suggested approach
pub async fn save(&self, path: &Path) -> std::io::Result<()> { let contents = serde_json::to_string_pretty(self)?; - fs::write(path, contents).await + let tmp = path.with_extension("tmp"); + fs::write(&tmp, contents).await?; + fs::rename(&tmp, path).await }crates/zk-prover/src/prover.rs (2)
152-170: Verification artifacts are left on disk afterverify_proofcompletes.The
verify_{verification_id}subdirectory (proof file, public_inputs) persists after verification finishes. Sincecleanup(e3_id)removes the entiree3_iddirectory, these files will only be cleaned up ifcleanupis explicitly called later. Consider removing the per-verification subdirectory at the end ofverify_proof(in afinally-style block or explicit cleanup), especially since the written data is already held in memory.
224-237: Test coverage is minimal — only theBbNotInstalledpath is exercised.The PR description already notes E2E tests are a remaining task. Consider adding unit tests for the error paths that don't require a real
bbbinary — e.g.,CircuitNotFoundwhen circuit/VK files are missing (create thebb_binaryfile but not the circuit files).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/zk-prover/src/actors/zk_actor.rs`:
- Around line 84-85: The current send call msg.sender.do_send(response) can
silently drop the message; replace it with msg.sender.try_send(response) and
handle the Result: on Err, log the failure (include the response details and the
error) and take appropriate recovery (retry, escalate, or mark the verification
as failed) so the verification path cannot silently stall; alternatively, if you
move this code to async, use msg.sender.send(response).await and similarly
handle/log the error case.
- Around line 32-34: The Actor impl for ZkActor currently uses the default
Context and runs blocking work (verify_proof invoking bb and filesystem I/O) on
the arbiter thread; change the actor to use a blocking-friendly context or
offload blocking I/O: either switch the actor context to actix::SyncContext by
updating impl Actor for ZkActor { type Context = actix::SyncContext<Self>; } and
start instances with SyncArbiter::start(...) so verify_proof runs on worker
threads, or keep the current Context and modify the handler that calls
verify_proof to run the subprocess/filesystem operations inside a
tokio::task::spawn_blocking (or equivalent) so the blocking work does not run on
the arbiter thread. Ensure references to ZkActor and the verify_proof callsite
are updated accordingly.
In `@crates/zk-prover/src/prover.rs`:
- Around line 83-97: The subprocess calls using StdCommand::output() in the
prove routine (invoking self.bb_binary with "prove") — and the similar call in
verify_proof (the "bb verify" invocation) — lack a timeout and can block
indefinitely; change both to spawn the child (StdCommand::spawn()), monitor with
a timeout (e.g., wait-timeout crate or std/tokio timeout), and if the timeout
elapses kill the child, collect/return its stderr/stdout as part of an Err
variant so callers see why it timed out; ensure you use the same bb_binary,
circuit_path/witness_path/vk_path/output_dir arguments as before and surface a
descriptive error from the prove and verify_proof functions when the process is
killed for exceeding the timeout.
🧹 Nitpick comments (2)
crates/zk-prover/src/traits.rs (1)
29-49: Cacheself.circuit()in a local variable to avoid repeated calls.
self.circuit()is invoked three times (lines 38, 41, 48). Storing it once improves readability and avoids any subtle inconsistency risk.♻️ Proposed refactor
fn prove( &self, prover: &ZkProver, params: &Self::Params, input: &Self::Input, e3_id: &str, ) -> Result<Proof, ZkError> { let inputs = self.build_witness(params, input)?; - let circuit_name = self.circuit().as_str(); + let circuit = self.circuit(); + let circuit_name = circuit.as_str(); let circuit_path = prover .circuits_dir() - .join(self.circuit().dir_path()) + .join(circuit.dir_path()) .join(format!("{}.json", circuit_name)); - let circuit = CompiledCircuit::from_file(&circuit_path)?; + let compiled = CompiledCircuit::from_file(&circuit_path)?; let witness_gen = WitnessGenerator::new(); - let witness = witness_gen.generate_witness(&circuit, inputs)?; + let witness = witness_gen.generate_witness(&compiled, inputs)?; - prover.generate_proof(self.circuit(), &witness, e3_id) + prover.generate_proof(circuit, &witness, e3_id) }crates/zk-prover/src/prover.rs (1)
31-37: Nit: return&Pathinstead of&PathBuffrom accessors.Idiomatic Rust accessors for
PathBuffields return&Path, since callers rarely need the owned-buffer methods.&PathBufcoerces to&Pathat call sites, so this is source-compatible if changed now but gets harder to change later once there are more consumers.♻️ Suggested diff
+use std::path::{Path, PathBuf}; -use std::path::PathBuf; - pub fn circuits_dir(&self) -> &PathBuf { + pub fn circuits_dir(&self) -> &Path { &self.circuits_dir } - pub fn work_dir(&self) -> &PathBuf { + pub fn work_dir(&self) -> &Path { &self.work_dir }
3 participants
Enable ciphernodes to generate proofs for cryptographic operations (key generation, decryption shares, etc.) using Noir circuits and the Barretenberg prover.
High-Level Flow
Enclave Noir CLI
Directory Structure
Remaining Tasks:
~/.enclave/noir/work/<e3_id>/Summary by CodeRabbit