[Feat] Prover loading assets (circuits) dynamically #1717
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change refactors the prover and verifier infrastructure by replacing EuclidV2-specific handlers and dependencies with universal counterparts. Key workspace dependencies are pinned to a specific git commit. The prover now supports dynamic asset loading and caching per verification key, and legacy Euclid handler modules are removed in favor of newly introduced universal and asset handler modules. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LocalProver
participant AssetsLocationData
participant UniversalHandler
participant AssetsHandler
User->>LocalProver: prove(request with VK)
LocalProver->>LocalProver: decode VK, check handler cache
alt Handler not cached
LocalProver->>AssetsLocationData: get_asset("app.vmexe"), get_asset("openvm.toml")
AssetsLocationData-->>LocalProver: Downloaded assets
LocalProver->>UniversalHandler: instantiate with assets
LocalProver->>LocalProver: cache handler by VK
end
LocalProver->>UniversalHandler: generate proof
UniversalHandler-->>LocalProver: proof data
LocalProver-->>User: proof response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (3)
crates/libzkp/src/lib.rs (2)
132-136: Avoid unwrap() on verifier mutex to prevent panics.
Propagate poisoning as an error instead.- let ret = verifier.lock().unwrap().verify(task_type, &proof)?; + let ret = verifier + .lock() + .map_err(|e| eyre::eyre!("verifier mutex poisoned: {e}"))? + .verify(task_type, &proof)?;
140-145: Same here: replace unwrap() with error propagation.
Consistent error handling.- verifier.lock().unwrap().dump_vk(Path::new(file)); + verifier + .lock() + .map_err(|e| eyre::eyre!("verifier mutex poisoned: {e}"))? + .dump_vk(Path::new(file));crates/libzkp/src/verifier/universal.rs (1)
33-64: Avoid unwraps and panic_catch; prefer structured error propagationMultiple
unwrap()calls wrapped bypanic_catchobscures error causes and stacks. Use proper deserialization and verifier error propagation with?, returningResult<bool>directly.Example direction:
- fn verify(&self, task_type: super::TaskType, proof: &[u8]) -> Result<bool> { - panic_catch(|| match task_type { - TaskType::Chunk => { - let proof = serde_json::from_slice::<ChunkProof>(proof).unwrap(); - if !proof.pi_hash_check(self.fork) { return false; } - self.verifier.verify_proof(proof.as_root_proof(), &proof.vk).unwrap() - } - ... - }).map_err(|err_str: String| eyre::eyre!("{err_str}")) - } + fn verify(&self, task_type: super::TaskType, proof: &[u8]) -> Result<bool> { + match task_type { + TaskType::Chunk => { + let proof: ChunkProof = serde_json::from_slice(proof) + .map_err(|e| eyre::eyre!("chunk proof deserialization failed: {e}"))?; + if !proof.pi_hash_check(self.fork) { + return Ok(false); + } + Ok(self.verifier.verify_proof(proof.as_root_proof(), &proof.vk)?) + } + TaskType::Batch => { + let proof: BatchProof = serde_json::from_slice(proof) + .map_err(|e| eyre::eyre!("batch proof deserialization failed: {e}"))?; + if !proof.pi_hash_check(self.fork) { + return Ok(false); + } + Ok(self.verifier.verify_proof(proof.as_root_proof(), &proof.vk)?) + } + TaskType::Bundle => { + let proof: BundleProof = serde_json::from_slice(proof) + .map_err(|e| eyre::eyre!("bundle proof deserialization failed: {e}"))?; + if !proof.pi_hash_check(self.fork) { + return Ok(false); + } + let vk = proof.vk.clone(); + let evm_proof = proof.into_evm_proof(); + Ok(self.verifier.verify_proof_evm(&evm_proof, &vk)?) + } + } + }
🧹 Nitpick comments (14)
crates/libzkp/src/lib.rs (2)
52-58: Typos and clearer error messages (nit).
Polish comments and bail messages; improves diagnosability.- // normailze fork name field in task + // normalize fork name field in task ... - eyre::bail!("fork name in chunk task not match the calling arg, expected {fork_name_str}, get {}", task.fork_name); + eyre::bail!( + "fork name in chunk task does not match the argument; expected '{fork_name_str}', got {}", + task.fork_name + ); ... - eyre::bail!("fork name in batch task not match the calling arg, expected {fork_name_str}, get {}", task.fork_name); + eyre::bail!( + "fork name in batch task does not match the argument; expected '{fork_name_str}', got {}", + task.fork_name + ); ... - eyre::bail!("fork name in bundle task not match the calling arg, expected {fork_name_str}, get {}", task.fork_name); + eyre::bail!( + "fork name in bundle task does not match the argument; expected '{fork_name_str}', got {}", + task.fork_name + );Also applies to: 67-71, 78-82
71-75: Fix copy-paste in panic mapping messages (batch/bundle).
They currently say “chunk task” in non-chunk paths.- .map_err(|e| eyre::eyre!("caught panic in chunk task{e}"))??; + .map_err(|e| eyre::eyre!("caught panic in batch task: {e}"))??; ... - .map_err(|e| eyre::eyre!("caught panic in chunk task{e}"))??; + .map_err(|e| eyre::eyre!("caught panic in bundle task: {e}"))??;Also applies to: 82-86
crates/prover-bin/Cargo.toml (1)
21-21: Pin futures-util to exact version for consistency.
Keeps it aligned with futures = 0.3.30 and avoids duplicate minor versions.-futures-util = "0.3" +futures-util = "0.3.30"crates/libzkp/src/verifier.rs (3)
55-56: Unify trait-object bounds in type alias and return type for clarity
VerifierTypeincludes+ Send, whileget_verifierreturnsArc<Mutex<dyn ProofVerifier>>without+ Send. Although this likely coerces, returning the alias improves readability and avoids confusion.Apply:
-pub fn get_verifier(fork_name: &str) -> Result<Arc<Mutex<dyn ProofVerifier>>> { +pub fn get_verifier(fork_name: &str) -> Result<VerifierType> {Also applies to: 78-79
78-88: Case-insensitive lookup to match lowercased insert keysYou lowercase keys on insert but not on lookup. Normalize
fork_nameon retrieval to avoid surprising misses.- if let Some(verifier) = verifiers.get(fork_name) { + if let Some(verifier) = verifiers.get(&fork_name.to_lowercase()) {
74-76: Prefer expect with message over bare assert for OnceLock setProvide a clear error message if
initis called twice.- let ret = VERIFIERS.set(verifiers).is_ok(); - assert!(ret); + VERIFIERS + .set(verifiers) + .expect("VERIFIERS already initialized; init() should only be called once");crates/libzkp/src/verifier/universal.rs (2)
19-26: Tweak setup error message; consider propagating errors instead of panickingThe expect message mentions “chunk verifier”, which is misleading here. Also, panicking in constructor complicates recovery and testing.
- verifier: UniversalVerifier::setup(&config, &exe, &verifier_bin) - .expect("Setting up chunk verifier"), + verifier: UniversalVerifier::setup(&config, &exe, &verifier_bin) + .expect("Setting up universal verifier"),If feasible, change
newto returneyre::Result<Self>and propagate the setup error instead of panicking.
66-68: Don’t panic in deprecated method; no-op with a warningPanicking on
dump_vkcan take down the process if someone calls it by mistake. Prefer a no-op and log a warning.- fn dump_vk(&self, _file: &Path) { - panic!("dump vk has been deprecated"); - } + fn dump_vk(&self, _file: &Path) { + tracing::warn!("dump_vk is deprecated on universal verifier; no-op"); + }crates/prover-bin/src/zk_circuits_handler.rs (1)
22-26: Naming nit: Phase::EuclidV2 under a universal handler can misleadPhase name is legacy but the context is now universal/asset-driven. Consider renaming or documenting why EuclidV2 phase is kept for asset layout only.
crates/prover-bin/src/zk_circuits_handler/assets.rs (2)
20-32: Prover setup per proof type is clear; minor suggestion on error contextSetup for chunk/batch/bundle looks correct with EVM enabled only for bundle. Consider adding more specific error messages to ease troubleshooting.
- let chunk_prover = Prover::setup(p.phase_spec_chunk(workspace_path), false, None) - .expect("Failed to setup chunk prover"); + let chunk_prover = Prover::setup(p.phase_spec_chunk(workspace_path), false, None) + .expect("Failed to setup chunk prover (phase_spec_chunk)"); ... - let batch_prover = Prover::setup(p.phase_spec_batch(workspace_path), false, None) - .expect("Failed to setup batch prover"); + let batch_prover = Prover::setup(p.phase_spec_batch(workspace_path), false, None) + .expect("Failed to setup batch prover (phase_spec_batch)"); ... - let bundle_prover = Prover::setup(p.phase_spec_bundle(workspace_path), true, None) - .expect("Failed to setup bundle prover"); + let bundle_prover = Prover::setup(p.phase_spec_bundle(workspace_path), true, None) + .expect("Failed to setup bundle prover (phase_spec_bundle)");
33-41: VK cache init is sound; ensureProofTypekeys are exhaustiveLazy
OnceLock<String>per proof type is good. IfProofTypegains variants later, it would be safer to derive keys fromcfg.vksor a canonical list to avoid missing cache entries.Consider building
cached_vksfrom an iterator over supported types, or assert all required keys are present at init.Also applies to: 46-50
crates/prover-bin/src/zk_circuits_handler/universal.rs (2)
45-47: Validate task payload before deserialising
serde_json::from_strblindly trusts the input. Consider cheap, explicit checks (e.g. size limits, required fields) before parsing to avoid DoS via gigabyte-sized JSON.
69-72: Error message loses contextThe bail message when
evm_proveris missing only prints the VK. Add the requested proof type and maybe the fork name to help operators diagnose mis-configurations.crates/prover-bin/src/prover.rs (1)
73-90: HEAD size-check may fail silentlyMany servers disable
HEADor omitContent-Length. In that case you fall through tocontinue, skipping a potentially stale file. At minimum, fall back to re-downloading when the HEAD request is not200 OK/ lacks length.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.toml(1 hunks)crates/libzkp/Cargo.toml(1 hunks)crates/libzkp/src/lib.rs(1 hunks)crates/libzkp/src/proofs.rs(1 hunks)crates/libzkp/src/tasks/batch/utils.rs(1 hunks)crates/libzkp/src/verifier.rs(2 hunks)crates/libzkp/src/verifier/universal.rs(2 hunks)crates/prover-bin/Cargo.toml(3 hunks)crates/prover-bin/src/prover.rs(5 hunks)crates/prover-bin/src/zk_circuits_handler.rs(1 hunks)crates/prover-bin/src/zk_circuits_handler/assets.rs(1 hunks)crates/prover-bin/src/zk_circuits_handler/euclid.rs(0 hunks)crates/prover-bin/src/zk_circuits_handler/euclidV2.rs(0 hunks)crates/prover-bin/src/zk_circuits_handler/universal.rs(1 hunks)
💤 Files with no reviewable changes (2)
- crates/prover-bin/src/zk_circuits_handler/euclid.rs
- crates/prover-bin/src/zk_circuits_handler/euclidV2.rs
🔇 Additional comments (13)
crates/libzkp/src/proofs.rs (1)
13-14: Import path fix verified: no outdated references remain
- Ran
rg -n --hidden --glob '!target' 'scroll_zkvm_types::util::vec_as_base64'; no matches found- The new
utils::vec_as_base64import is correct and aligns with the types crate renameApprove code changes.
crates/libzkp/src/tasks/batch/utils.rs (1)
21-21: No stale imports; import path change is correct
Ran a repo-wide search for the old import path (scroll_zkvm_types::util::sha256_rv32) and found no occurrences. The update toscroll_zkvm_types::utils::sha256_rv32is consistent with the types crate refactor, andget_versioned_hashremains compliant with EIP-4844.crates/libzkp/Cargo.toml (1)
9-9: No stale Euclid-specific crate references found
- Ran
rg -n --hidden --glob '!target' 'scroll-zkvm-(verifier|prover)-euclid'and confirmed there are no remaining imports of separate Euclid-specific verifier/prover crates.- All remaining
EuclidV2symbols (e.g.Phase::EuclidV2,finalizeBundlePostEuclidV2) belong to universal verifier logic and are expected.crates/libzkp/src/lib.rs (1)
8-8: Import path fix to utils::vec_as_base64 is correct.
Matches updates elsewhere in the crate.crates/prover-bin/Cargo.toml (3)
10-10: Switch to scroll-zkvm-prover (workspace) aligns with universal flow.
Good step toward dynamic asset loading.
34-34: url with serde feature is a sensible addition.
Enables (de)serialization of URLs in config or tasks.
22-22: No reqwest version skew detectedWe’ve scanned the workspace and found only:
- crates/prover-bin/Cargo.toml
• reqwest = { version = "0.12.4", features = ["gzip", "stream"] }
• reqwest-middleware = "0.3"The
"0.3"spec will automatically pick the latest 0.3.x patch, which is compatible with reqwest 0.12.x. Noreqwest-retrydependency was found.
No further action required.crates/libzkp/src/verifier.rs (1)
3-4: Universal verifier switch looks correctModule import and usage of
universal::Verifieralign with the PR direction. No issues spotted here.crates/prover-bin/src/zk_circuits_handler.rs (3)
4-6: Module re-org (universal + assets) is consistent with PR goalsSeparation of universal proving and asset utilities is clear and aligns with dynamic asset loading keyed by VK.
10-12: Imports updated to universal types/configs look goodUsing
ProvingTaskandProverConfigfrom universal crates matches the architecture shift.
16-20: AllCircuitsHandlerimplementations and call sites updated
Verified that the onlyimpl CircuitsHandlerin
crates/prover-bin/src/zk_circuits_handler/universal.rs
matches the newasync fn get_vk(&self) -> String; async fn get_proof_data(&self, u_task: &ProvingTask, need_snark: bool) -> Result<String>;and that the lone usage in
crates/prover-bin/src/prover.rs
(handler.get_proof_data(&prover_task, is_evm)) has been adapted accordingly. No further changes required.Cargo.toml (1)
20-22: Commit 2ec1303 not found in scroll-tech/zkvm-prover – please verify crate sourcesThe checkout script failed with “couldn’t find remote ref 2ec1303”. Before proceeding, confirm that:
- The revision
2ec1303actually exists in https://github.com/scroll-tech/zkvm-prover.git.- All three crates (
scroll-zkvm-prover,scroll-zkvm-verifier,scroll-zkvm-types) are present at that commit, or update the repo URLs/revs if they live in separate repositories.Affected location:
- Cargo.toml, lines 20–22
crates/prover-bin/src/prover.rs (1)
57-68: Asset directory can collide across proof typesAssets for chunk/batch/bundle with identical VKs share the same
storage_path(base_path/vk). If the files differ per proof type the cache becomes corrupt. Consider includingproof_typein the directory name.let storage_path = base_path.as_ref() .join(format!("{vk}_{:?}", proof_type).to_lowercase());
crates/prover-bin/src/prover.rs
Outdated
| let storage_path = base_path.as_ref().join(vk); | ||
| std::fs::create_dir_all(&storage_path)?; | ||
|
|
||
| // Step 2 & 3: Download each file if needed | ||
| let url_base = self.gen_asset_url(vk, proof_type)?; | ||
| let client = reqwest::Client::new(); |
There was a problem hiding this comment.
vk is used as a filesystem path – sanitise it
storage_path = base_path.join(vk) lets a malicious coordinator supply vk = "../../evil" and escape the workspace directory. Reject strings containing /, \, or .. components before using them as paths.
if vk.contains('/') || vk.contains('\\') || vk.contains("..") {
eyre::bail!("invalid vk value");
}🤖 Prompt for AI Agents
In crates/prover-bin/src/prover.rs around lines 61 to 66, the variable `vk` is
used directly to create a filesystem path, which can lead to directory traversal
vulnerabilities if `vk` contains characters like '/', '\\', or '..'. To fix
this, add a validation step before using `vk` in the path: check if `vk`
contains any of these forbidden substrings and if so, return an error using
`eyre::bail!("invalid vk value")`. This ensures only safe, sanitized strings are
used as path components.
crates/prover-bin/src/prover.rs
Outdated
| let is_evm = req.proof_type == ProofType::Bundle; | ||
| let task_handle = | ||
| tokio::task::spawn_blocking(move || handle.block_on(handler.get_proof_data(req_clone))); | ||
| tokio::task::spawn_blocking(move || handle.block_on(handler.get_proof_data(&prover_task, is_evm))); | ||
| self.current_task = Some(task_handle); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Nested runtime execution risk
spawn_blocking + handle.block_on(...) runs a future on the same runtime from a blocking thread; if that future internally calls more blocking tasks it can deadlock. Prefer:
let handler_cloned = handler.clone();
let task_handle = tokio::task::spawn(async move {
handler_cloned.get_proof_data(&prover_task, is_evm).await
});and drop spawn_blocking altogether unless the proof generation is strictly synchronous.
🤖 Prompt for AI Agents
In crates/prover-bin/src/prover.rs around lines 276 to 279, the current code
uses tokio::task::spawn_blocking combined with handle.block_on to run an async
future on the same runtime from a blocking thread, which risks deadlocks if the
future itself uses blocking calls. To fix this, remove spawn_blocking and
instead clone the handler, then use tokio::task::spawn with an async block that
awaits handler.get_proof_data directly, ensuring the future runs asynchronously
on the runtime without blocking threads.
| prover: Prover, | ||
| } | ||
|
|
||
| unsafe impl Send for UniversalHandler {} |
There was a problem hiding this comment.
unsafe impl Send needs a justification or removal
Prover is forcibly marked Send via unsafe impl Send for UniversalHandler {}.
Unless you can prove scroll_zkvm_prover::Prover is genuinely thread-safe, this is undefined behaviour waiting to happen. Either:
-unsafe impl Send for UniversalHandler {}
+// SAFETY: Explain here why Prover is thread-safe (interior mutability, no raw
+// pointers, etc.) or remove this entirely and keep the type !Send.Please document the safety contract or drop the unsafe implementation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unsafe impl Send for UniversalHandler {} | |
| // SAFETY: Explain here why `UniversalHandler` (and its contained `Prover`) is | |
| // genuinely `Send` (e.g. only interior mutability with proper synchronization, | |
| // no raw pointers, etc.), or remove this entirely and keep the type `!Send`. |
🤖 Prompt for AI Agents
In crates/prover-bin/src/zk_circuits_handler/universal.rs at line 15, the unsafe
implementation of Send for UniversalHandler lacks a safety justification. You
need to either provide a detailed comment explaining why UniversalHandler is
safe to send across threads, specifically proving that
scroll_zkvm_prover::Prover is thread-safe, or remove the unsafe impl Send
entirely to avoid undefined behavior.
|
Has passed prover e2e test with dynamically downloading assets |
|
reviewed. Don't force push from now on |
Prover can load assets while it has received a task.
It load the corresponding task according to the vk specified in universal task.
Summary by CodeRabbit
New Features
UniversalHandler,AssetsHandler) for flexible and universal zero-knowledge proof operations.Improvements
Bug Fixes
Chores