[Feat] Update zkvm to 0.6 (use openvm 1.4)#1736
Conversation
WalkthroughAdds runtime-configurable libzkp features (FFI + Go), removes the interpreter parameter from universal task generation, introduces a dual-mode legacy witness encoding path, refactors l2geth RPC client to be provider-generic and use sbv-core BlockWitness, updates verifier init, adjusts GPU override/build artifacts, and bumps workspace deps and toolchain. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Coordinator
participant Verifier
participant libzkp_c as libzkp_c FFI
participant libzkp as libzkp (Rust)
User->>Coordinator: NewVerifier(cfg)
Coordinator->>Verifier: New(cfg)
alt cfg.Features non-empty
Verifier->>libzkp_c: set_dynamic_feature(feats)
libzkp_c->>libzkp: set_dynamic_feature(feats:&str)
note right of libzkp #D0F0C0: Parse features\nEnable legacy_witness if present
end
Verifier->>libzkp: Init/Setup verifier
Verifier-->>Coordinator: Verifier ready
sequenceDiagram
autonumber
actor Caller
participant libzkp as libzkp
participant Tasks as tasks::{chunk,batch,bundle}
participant Encoder as Encoder
Caller->>libzkp: gen_universal_task(type, task_json, fork, expected_vk)
libzkp->>Tasks: build ProvingTask (no interpreter)
alt legacy_witness enabled
Tasks->>Encoder: to_rkyv_bytes(Legacy*Witness)
else modern mode
Tasks->>Encoder: bincode::serde::encode_to_vec(task)
end
libzkp-->>Caller: (pi_hash, metadata_json, universal_task_json)
sequenceDiagram
autonumber
participant L2Core as l2geth::RpcClientCore
participant Rpc as alloy::rpc::client::RpcClient
participant Provider as impl Provider<Network>
participant SBV as sbv_core
L2Core->>Rpc: hold raw Rpc client
L2Core->>Provider: construct RpcClient(provider from Rpc)
Provider->>SBV: fetch_witness_async(prev: Option<&SBV::BlockWitness>)
SBV-->>Provider: SBV::BlockWitness
Provider-->>L2Core: sbv_core::BlockWitness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
|
looks good so far. (i think you need to add feature=cuda somewhere finally in prover) |
|
done except for gpu building |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
coordinator/internal/logic/libzkp/lib.go (1)
5-5: CUDA runtime path likely missing from rpath for GPU builds.Without an rpath to /usr/local/cuda/lib64, libcudart may fail to load at runtime unless LD_LIBRARY_PATH is set.
Apply:
-#cgo gpu LDFLAGS: -lzkp -lm -ldl -lgmp -lstdc++ -lprocps -L/usr/local/cuda/lib64/ -lcudart -L${SRCDIR}/lib/ -Wl,-rpath=${SRCDIR}/lib +#cgo gpu LDFLAGS: -lzkp -lm -ldl -lgmp -lstdc++ -lprocps -L/usr/local/cuda/lib64/ -lcudart -Wl,-rpath,/usr/local/cuda/lib64 -L${SRCDIR}/lib/ -Wl,-rpath=${SRCDIR}/libAlso confirm the build tag name (“gpu”) matches your docs and CI flags.
crates/libzkp/src/tasks/bundle.rs (1)
79-89: Avoid panics when extracting root proofs; return a proper error instead.
expect("expect root proof")can crash the process on malformed input. Make the conversion fallible and bubble the error up.Apply this diff:
- Ok(ProvingTask { - identifier: value.identifier(), - fork_name: value.fork_name, - aggregated_proofs: value - .batch_proofs - .into_iter() - .map(|w_proof| w_proof.proof.into_stark_proof().expect("expect root proof")) - .collect(), - serialized_witness: vec![serialized_witness], - vk: Vec::new(), - }) + let fork_name = value.fork_name.to_lowercase(); + let aggregated_proofs = value + .batch_proofs + .into_iter() + .map(|w_proof| { + w_proof + .proof + .into_stark_proof() + .ok_or_else(|| eyre::eyre!("missing root proof")) + }) + .collect::<eyre::Result<Vec<_>>>()?; + Ok(ProvingTask { + identifier: value.identifier(), + fork_name, + aggregated_proofs, + serialized_witness: vec![serialized_witness], + vk: Vec::new(), + })crates/libzkp/src/tasks/batch.rs (1)
94-104: Avoid panic on missing root proof; return an error.Same issue as bundle:
expectwill crash on bad inputs. Make it fallible.Apply this diff:
- Ok(ProvingTask { - identifier: value.batch_header.batch_hash().to_string(), - fork_name: value.fork_name, - aggregated_proofs: value - .chunk_proofs - .into_iter() - .map(|w_proof| w_proof.proof.into_stark_proof().expect("expect root proof")) - .collect(), - serialized_witness: vec![serialized_witness], - vk: Vec::new(), - }) + let fork_name = value.fork_name; + let aggregated_proofs = value + .chunk_proofs + .into_iter() + .map(|w_proof| { + w_proof + .proof + .into_stark_proof() + .ok_or_else(|| eyre::eyre!("missing root proof")) + }) + .collect::<eyre::Result<Vec<_>>>()?; + Ok(ProvingTask { + identifier: value.batch_header.batch_hash().to_string(), + fork_name, + aggregated_proofs, + serialized_witness: vec![serialized_witness], + vk: Vec::new(), + })
🧹 Nitpick comments (19)
crates/gpu_override/Makefile (1)
17-17: Add CUDA toolchain preflight for clearer failures.Fail early if CUDA/NVCC is missing to avoid long Rust compiles before erroring.
build: - GO_TAG=${GO_TAG} GIT_REV=${GIT_REV} ZK_VERSION=${ZK_VERSION} cargo build -Z unstable-options --release -p prover --features cuda --lockfile-path ./Cargo.lock + @command -v nvcc >/dev/null 2>&1 || { echo "nvcc not found. Please install CUDA Toolkit."; exit 1; } + GO_TAG=${GO_TAG} GIT_REV=${GIT_REV} ZK_VERSION=${ZK_VERSION} cargo build -Z unstable-options --profile maxperf -p prover --features cuda --lockfile-path ./Cargo.lockNote: switched to
--profile maxperfso the new profile is actually used (see workspace Cargo.toml).crates/libzkp/src/proofs.rs (1)
171-176: Guard against invalid/empty VK during deserialize to avoid panics.Consider a safe constructor or fallback.
If supported, prefer an option-returning API:
- commitment: serialize_vk::deserialize(&value.vk), + commitment: serialize_vk::deserialize_opt(&value.vk).unwrap_or_default(),If not available, explicitly handle empty:
- commitment: serialize_vk::deserialize(&value.vk), + commitment: if value.vk.is_empty() { + Default::default() + } else { + serialize_vk::deserialize(&value.vk) + },Please confirm
serialize_vk::deserialize's behavior on zero-length input.Cargo.toml (1)
65-69: maxperf profile likely unused; wire it into builds or move options to release.The GPU Makefile uses
--release, solto = "fat"andcodegen-units = 1under[profile.maxperf]won’t apply.Two options:
- Use
--profile maxperfin your Makefile (suggested in Makefile comment above), or- Move these options under
[profile.release]if you want them globally.Example if you want global release:
-[profile.maxperf] -inherits = "release" -lto = "fat" -codegen-units = 1 +[profile.release] +lto = "fat" +codegen-units = 1coordinator/internal/logic/libzkp/libzkp.h (1)
57-58: Add ownership/thread-safety docs and C++ ABI guards for set_dynamic_feature — coordinator already calls it before InitVerifierConfirmed: coordinator/internal/logic/verifier/verifier.go invokes libzkp.SetDynamicFeature(cfg.Features) before libzkp.InitVerifier(...).
#ifndef LIBZKP_H #define LIBZKP_H +#ifdef __cplusplus +extern "C" { +#endif @@ -void set_dynamic_feature(const char* feats); +// Set runtime features for the verifier. +// - feats: colon-separated list (e.g., "legacy_witness"), UTF-8, null-terminated. +// - Ownership: caller retains ownership; callee reads only. +// - Thread-safety: must be called prior to init_verifier/init_l2geth and before any worker threads start. +void set_dynamic_feature(const char* feats); @@ -#endif /* LIBZKP_H */ +#ifdef __cplusplus +} // extern "C" +#endif +#endif /* LIBZKP_H */coordinator/internal/logic/libzkp/lib.go (1)
144-149: FFI wrapper: OK; add guardrails and brief doc.Function works; add a brief comment about expected colon-separated features and that it must be invoked before InitVerifier.
I can add a GoDoc comment if you want.
crates/libzkp/src/verifier/universal.rs (1)
20-26: Path now treated as a directory — clarify variable name.verifier_bin now points to a directory. Rename to assets_dir_path for clarity.
- let verifier_bin = Path::new(assets_dir); + let assets_dir_path = Path::new(assets_dir); Self { - verifier: UniversalVerifier::setup(verifier_bin).expect("Setting up chunk verifier"), + verifier: UniversalVerifier::setup(assets_dir_path).expect("Setting up chunk verifier"), fork, }crates/libzkp/src/tasks.rs (1)
19-22: Unused helper encode_task_to_witness — remove or use.Currently unused; will trigger dead_code warnings.
-fn encode_task_to_witness<T: serde::Serialize>(task: &T) -> eyre::Result<Vec<u8>> { - let config = bincode::config::standard(); - Ok(bincode::serde::encode_to_vec(task, config)?) -} +// Re-add when needed or move to the module that uses it.crates/libzkp_c/src/lib.rs (2)
160-166: Minor naming nit: avoidstras an identifier.
strshadows the primitive type name; prefersorout.- Ok(str) => str, + Ok(s) => s,
249-254: set_dynamic_feature FFI: guard against null and call ordering.Add a null check to avoid UB if called from non-Go callers, and document it must be invoked before init_verifier.
#[no_mangle] pub unsafe extern "C" fn set_dynamic_feature(feats: *const c_char) { - let feats_str = c_char_to_str(feats); - libzkp::set_dynamic_feature(feats_str); + if feats.is_null() { + return; + } + libzkp::set_dynamic_feature(c_char_to_str(feats)); }crates/libzkp/src/lib.rs (2)
76-79: Polish error text (nit).Improve clarity/grammar of mismatch messages.
Apply this diff:
- // normailze fork name field in task + // normalize fork name field in task @@ - // if the fork_name wrapped in task is not match, consider it a malformed task - if fork_name_str != task.fork_name.as_str() { - eyre::bail!("fork name in chunk task not match the calling arg, expected {fork_name_str}, get {}", task.fork_name); + // if the fork_name wrapped in task does not match, consider it a malformed task + if fork_name_str != task.fork_name.as_str() { + eyre::bail!("fork name in chunk task does not match the calling arg, expected {fork_name_str}, got {}", task.fork_name); } @@ - if fork_name_str != task.fork_name.as_str() { - eyre::bail!("fork name in batch task not match the calling arg, expected {fork_name_str}, get {}", task.fork_name); + if fork_name_str != task.fork_name.as_str() { + eyre::bail!("fork name in batch task does not match the calling arg, expected {fork_name_str}, got {}", task.fork_name); } @@ - if fork_name_str != task.fork_name.as_str() { - eyre::bail!("fork name in bundle task not match the calling arg, expected {fork_name_str}, get {}", task.fork_name); + if fork_name_str != task.fork_name.as_str() { + eyre::bail!("fork name in bundle task does not match the calling arg, expected {fork_name_str}, got {}", task.fork_name); }Also applies to: 88-89, 99-100
79-82: Fix copy-paste in panic wrappers; add colon.The batch/bundle branches say “chunk task{e}”.
Apply this diff:
- utils::panic_catch(move || gen_universal_chunk_task(task, fork_name_str.into())) - .map_err(|e| eyre::eyre!("caught panic in chunk task{e}"))??; + utils::panic_catch(move || gen_universal_chunk_task(task, fork_name_str.into())) + .map_err(|e| eyre::eyre!("caught panic in chunk task: {e}"))??; @@ - utils::panic_catch(move || gen_universal_batch_task(task, fork_name_str.into())) - .map_err(|e| eyre::eyre!("caught panic in chunk task{e}"))??; + utils::panic_catch(move || gen_universal_batch_task(task, fork_name_str.into())) + .map_err(|e| eyre::eyre!("caught panic in batch task: {e}"))??; @@ - utils::panic_catch(move || gen_universal_bundle_task(task, fork_name_str.into())) - .map_err(|e| eyre::eyre!("caught panic in chunk task{e}"))??; + utils::panic_catch(move || gen_universal_bundle_task(task, fork_name_str.into())) + .map_err(|e| eyre::eyre!("caught panic in bundle task: {e}"))??;Also applies to: 91-93, 102-104
crates/libzkp/src/tasks/batch.rs (1)
158-169: Prefer returningErroverassert_eq!in library code.The KZG sanity checks will abort the process on mismatch. Return a descriptive error instead to keep the process alive.
If you’re open to it, I can draft a minimal refactor that validates and returns
eyre::Result<BatchWitness>frombuild_guest_input()and plumbs the error up.crates/libzkp/src/tasks/chunk.rs (1)
149-199: Optional: avoid compiling the regex on every call.Cache the regex (e.g.,
once_cell::sync::Lazy) to reduce overhead in retry loops.crates/l2geth/src/rpc_client.rs (2)
139-140: Typo in error message (nit).“avaliable” → “available”.
Apply this diff:
- .ok_or_else(|| eyre::eyre!("Block witness {block_num} not avaliable"))??; + .ok_or_else(|| eyre::eyre!("Block witness {block_num} not available"))??;
195-207: Test comment contradicts expectation.Comment says “expected to fail” but the test uses
.expect("should success").Either remove the comment or change it to reflect success. Example:
- // This is expected to fail since we're using a dummy hash, but it tests the code path + // Fetch a known block by hash; this should succeed when L2GETH_ENDPOINT is setcrates/prover-bin/src/zk_circuits_handler/universal.rs (4)
20-25: Name the magic segment length.
(1 << 22) - 100is opaque. Use a named constant.+const SEGMENT_LEN: usize = (1 << 22) - 100; @@ - let segment_len = Some((1 << 22) - 100); + let segment_len = Some(SEGMENT_LEN);
33-35: Avoid exposing &mut Prover; call through the field to reduce API surface.Returning a mutable reference expands the public surface. Since the call site is in the same module, use the private field directly and drop
get_prover.- /// universal prover, before that, use bundle_prover as the represent one - pub fn get_prover(&mut self) -> &mut Prover { - &mut self.prover - } + /// universal prover, before that, use bundle_prover as the represent one + /// Consider deleting this method and using `self.prover` directly within the module.And in
get_proof_data:- let proof = handler_self - .get_prover() - .gen_proof_universal(u_task, need_snark)?; + let proof = handler_self.prover.gen_proof_universal(u_task, need_snark)?;
45-53: Delete commented-out runtime gating or replace with an explicit feature check.Commented code adds noise. If SNARK/EVM prerequisites must be validated at runtime with dynamic features, add a clear check and bail early with a precise error; otherwise, remove these lines.
Minimal cleanup:
- // if need_snark && handler_self.prover.evm_prover.is_none() { - // use base64::{prelude::BASE64_STANDARD, Engine}; - // eyre::bail!( - // "do not init prover for evm (vk: {})", - // BASE64_STANDARD.encode(handler_self.get_prover().get_app_vk()) - // ) - // }
55-57: CPU-bound proving under tokio::Mutex: consider a dedicated worker.
gen_proof_universalis likely CPU-bound and runs while holding the Tokio mutex, serializing all requests on this handler. Consider funneling requests to a dedicated worker thread or queue to avoid stalling the async scheduler and to enable better concurrency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/gpu_override/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Cargo.toml(2 hunks)coordinator/internal/config/config.go(1 hunks)coordinator/internal/logic/libzkp/lib.go(1 hunks)coordinator/internal/logic/libzkp/libzkp.h(1 hunks)coordinator/internal/logic/verifier/verifier.go(1 hunks)crates/gpu_override/.cargo/config.toml(0 hunks)crates/gpu_override/Makefile(1 hunks)crates/l2geth/Cargo.toml(1 hunks)crates/l2geth/src/lib.rs(1 hunks)crates/l2geth/src/rpc_client.rs(6 hunks)crates/libzkp/Cargo.toml(2 hunks)crates/libzkp/src/lib.rs(2 hunks)crates/libzkp/src/proofs.rs(2 hunks)crates/libzkp/src/tasks.rs(2 hunks)crates/libzkp/src/tasks/batch.rs(5 hunks)crates/libzkp/src/tasks/bundle.rs(4 hunks)crates/libzkp/src/tasks/chunk.rs(5 hunks)crates/libzkp/src/tasks/chunk_interpreter.rs(1 hunks)crates/libzkp/src/verifier/universal.rs(2 hunks)crates/libzkp_c/src/lib.rs(3 hunks)crates/prover-bin/Cargo.toml(1 hunks)crates/prover-bin/src/types.rs(1 hunks)crates/prover-bin/src/zk_circuits_handler/universal.rs(3 hunks)rust-toolchain(1 hunks)
💤 Files with no reviewable changes (1)
- crates/gpu_override/.cargo/config.toml
🧰 Additional context used
🧬 Code graph analysis (11)
coordinator/internal/logic/verifier/verifier.go (1)
coordinator/internal/logic/libzkp/lib.go (1)
SetDynamicFeature(145-149)
crates/l2geth/src/lib.rs (1)
crates/l2geth/src/rpc_client.rs (1)
get_client(81-87)
coordinator/internal/logic/libzkp/libzkp.h (2)
crates/libzkp/src/lib.rs (1)
set_dynamic_feature(20-33)crates/libzkp_c/src/lib.rs (1)
set_dynamic_feature(251-254)
crates/libzkp_c/src/lib.rs (2)
crates/libzkp/src/lib.rs (2)
gen_universal_task(51-116)set_dynamic_feature(20-33)crates/libzkp_c/src/utils.rs (1)
c_char_to_str(3-6)
crates/prover-bin/src/zk_circuits_handler/universal.rs (1)
common/types/message/message.go (1)
ProofType(14-14)
crates/libzkp/src/verifier/universal.rs (1)
crates/libzkp/src/proofs.rs (1)
pi_hash_check(184-202)
crates/libzkp/src/lib.rs (3)
crates/libzkp_c/src/lib.rs (1)
set_dynamic_feature(251-254)crates/libzkp/src/utils.rs (1)
panic_catch(43-53)crates/libzkp/src/tasks.rs (1)
gen_universal_chunk_task(44-60)
crates/libzkp/src/tasks/chunk.rs (4)
crates/libzkp/src/tasks/batch.rs (2)
to_rkyv_bytes(89-89)try_from(85-105)crates/libzkp/src/tasks/bundle.rs (2)
to_rkyv_bytes(74-74)try_from(71-90)crates/libzkp/src/lib.rs (1)
witness_use_legacy_mode(16-18)crates/libzkp/src/tasks.rs (1)
encode_task_to_witness(19-22)
crates/libzkp/src/tasks/bundle.rs (2)
crates/libzkp/src/lib.rs (1)
witness_use_legacy_mode(16-18)crates/libzkp/src/tasks.rs (1)
encode_task_to_witness(19-22)
crates/libzkp/src/tasks/batch.rs (3)
coordinator/internal/types/block.go (1)
BatchInfo(4-8)crates/libzkp/src/lib.rs (1)
witness_use_legacy_mode(16-18)crates/libzkp/src/tasks.rs (1)
encode_task_to_witness(19-22)
crates/l2geth/src/rpc_client.rs (2)
crates/l2geth/src/lib.rs (1)
get_client(14-19)crates/libzkp/src/tasks/chunk_interpreter.rs (2)
try_fetch_block_witness(9-15)try_fetch_storage_node(16-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (17)
crates/prover-bin/src/types.rs (1)
1-1: Scope down dead_code suppression in crates/prover-bin/src/types.rsModule-level
#![allow(dead_code)]can mask real unused items during the zkvm/openvm migration — narrow the suppression.Options:
- Conditional on non-GPU builds:
-#![allow(dead_code)] +#![cfg_attr(not(any(feature = "cuda", feature = "gpu")), allow(dead_code))]
- Or remove the module-wide allow and add
#[allow(dead_code)]to the specific items that are temporarily unused with a TODO and removal date.Verify (run locally from repo root):
#!/bin/bash # Find Cargo.toml files that define features fd --type f Cargo.toml | while read -r f; do echo "== $f ==" rg -n -C2 '^\s*\[features\]' "$f" || true rg -n -C2 -i 'cuda|gpu' "$f" || true done # Build checks for prover-bin under different feature sets cargo check -p prover-bin --all-targets --no-default-features || true cargo check -p prover-bin --all-targets --features cuda || true cargo check -p prover-bin --all-targets --features gpu || truecrates/prover-bin/Cargo.toml (1)
37-39: CUDA feature gate added correctly.Matches the reviewer note; pairs with Makefile switch. No issues.
crates/libzkp/src/proofs.rs (1)
12-14: Switch to serialize_vk module: check empty-vk behavior.If
vkis absent (default empty), ensureserialize_vk::deserializewon’t panic or produce an invalid commitment.Add a unit test to cover empty
vk:#[test] fn aggregation_input_handles_empty_vk() { let dummy = WrappedProof { metadata: (), proof: ProofEnum::default_for_tests(), // or minimal valid variant vk: Vec::new(), git_version: "test".into(), }; let _ = AggregationInput::from(&dummy); // should not panic }Cargo.toml (2)
17-17: Version bump noted.No concerns with the workspace version update.
20-27: Good: zkvm crates pinned to a commit rev.This improves reproducibility. Keep this pattern for other git deps too.
crates/l2geth/Cargo.toml (1)
16-16: LGTM — sbv-core is used in l2geth (not a dead dependency).
Referenced in crates/l2geth/src/rpc_client.rs (uses sbv_core::BlockWitness; matches at lines 94, 95, 99, 100).coordinator/internal/config/config.go (1)
69-69: Validate VerifierConfig.Features early; fail-fast on unknown values.Add a whitelist (only "legacy_witness" today) and a Validate method on VerifierConfig, then call it from NewConfig after unmarshalling and applying env overrides.
type VerifierConfig struct { MinProverVersion string `json:"min_prover_version"` Features string `json:"features,omitempty"` Verifiers []AssetConfig `json:"verifiers"` } +func (v *VerifierConfig) Validate() error { + if v == nil || v.Features == "" { + return nil + } + allowed := map[string]struct{}{"legacy_witness": {}} + for _, f := range strings.Split(v.Features, ":") { + f = strings.TrimSpace(strings.ToLower(f)) + if f == "" { + continue + } + if _, ok := allowed[f]; !ok { + return fmt.Errorf("unknown verifier feature: %q", f) + } + } + return nil +}Call site: invoke cfg.ProverManager.Verifier.Validate() in NewConfig after unmarshal/env override (ops env example: SCROLL_COORDINATOR_PROVER_MANAGER__VERIFIER__FEATURES=legacy_witness).
rust-toolchain (1)
2-4: Drop rustc-dev unless required; verify build on nightly-2025-08-18
- Repo search found no uses of rustc internals (rustc_private / #![feature] / extern crate rustc_) and no references to llvm coverage tooling (cargo-llvm-cov / llvm-cov). The only occurrence of rustc-dev is in rust-toolchain (repo root) — components = ["llvm-tools", "rustc-dev"].
- Action: remove rustc-dev if not needed (suggested diff):
[toolchain] channel = "nightly-2025-08-18" targets = ["riscv32im-unknown-none-elf", "x86_64-unknown-linux-gnu"] -components = ["llvm-tools", "rustc-dev"] +components = ["llvm-tools"]
- Run a full workspace build and test suite on nightly-2025-08-18 before merging to confirm the nightly bump doesn't break anything.
coordinator/internal/logic/verifier/verifier.go (1)
70-72: Feature toggle prior to InitVerifier: LGTM; document format and scope.Good placement before InitVerifier. Please document expected format (e.g., "legacy_witness", colon-separated list) and that this must be called only during process init to avoid races with the underlying static toggle.
Would you like me to add a short comment and validation that trims/ignores empty/whitespace-only values?
crates/libzkp/src/tasks/chunk_interpreter.rs (1)
2-3: Switch to sbv_core::BlockWitness: LGTM; check downstream imports.Looks consistent with the workspace move to sbv_core. Ensure all remaining imports and serde derives align.
crates/libzkp_c/src/lib.rs (1)
176-178: FFI call path simplified (no interpreter): LGTM.Matches the Rust API change.
crates/libzkp/src/tasks.rs (1)
45-60: Interpreter parameter removal: confirm all call sites updatedVerification couldn't run — ripgrep returned "No files were searched". Re-run the search from the repository root (e.g. rg -nP '\bgen_universal_chunk_task\s*(') or manually confirm there are no remaining callers passing an interpreter.
crates/l2geth/src/lib.rs (1)
14-19: Public API change — returning impl ChunkInterpreter hides RpcClient; confirm intended break and update callers.
Internal usage found: crates/libzkp_c/src/lib.rs:158 (l2geth::get_client()). No other RpcClient references located in this repo; external/downstream consumers unknown — verify downstream crates/repos. Provide either a helper that returns the concrete RpcClient, restore the concrete return type, or add migration notes for downstream users.crates/libzkp/src/tasks/chunk.rs (2)
69-77: Legacy/non‑legacy witness gating — LGTM.The dual-path serialization is clear and consistent with batch/bundle.
91-96: Tx counting fix — LGTM.Switch to
transactions.len()looks correct with the new witness type.crates/l2geth/src/rpc_client.rs (1)
75-79: Runtime/provider wiring — LGTM.Provider-agnostic
RpcClientand layered retry look good.Also applies to: 81-87
crates/prover-bin/src/zk_circuits_handler/universal.rs (1)
27-27: Confirm Prover::setup(config, None) semantics for zkvm 0.6.API changed from the older triplet. Verify that
Nonecorrectly disables the optional component (EVM/GPU/etc.) and that required features are set elsewhere beforesetup.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1736 +/- ##
========================================
Coverage 36.98% 36.99%
========================================
Files 245 245
Lines 20804 20808 +4
========================================
+ Hits 7695 7697 +2
- Misses 12299 12300 +1
- Partials 810 811 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
.github/workflows/coordinator.yml (3)
34-45: Add cargo caching to speed up checks.Mirror common.yml by caching target to cut minutes from builds.
Apply this diff:
steps: - uses: actions-rs/toolchain@v1 with: toolchain: nightly-2025-08-18 override: true components: rustfmt, clippy + - name: Cache cargo + uses: Swatinem/rust-cache@v2 + with: + workspaces: ". -> target"
40-42: Update actions/setup-go to v5.v2 is old; v5 has security and caching improvements.
- uses: actions/setup-go@v2 + uses: actions/setup-go@v5Repeat in the tests job below.
115-116: Drop ineffective linker flag.
-ldflags="-s=false"is a no-op;-shas no=false. Remove for clarity.- go test -v -race -gcflags="-l" -ldflags="-s=false" -coverprofile=coverage.txt -covermode=atomic -tags mock_verifier ./... + go test -v -race -gcflags="-l" -coverprofile=coverage.txt -covermode=atomic -tags mock_verifier ./....github/workflows/common.yml (2)
36-39: Upgrade setup-go to v5.Align with current runner features and fixes.
- - name: Install Go - uses: actions/setup-go@v2 + - name: Install Go + uses: actions/setup-go@v5Repeat in tests job.
99-99: Remove no-op-ldflags="-s=false"in tests.Same rationale as coordinator workflow.
- go test -v -race -gcflags="-l" -ldflags="-s=false" -coverprofile=coverage.txt -covermode=atomic ./... + go test -v -race -gcflags="-l" -coverprofile=coverage.txt -covermode=atomic ./....github/workflows/intermediate-docker.yml (2)
25-27: Quote version literals to avoid YAML parsing surprises.
1.86.0unquoted can be misparsed; quote for consistency with other options.- - 1.86.0 + - "1.86.0" - nightly-2025-08-18 - default: "nightly-2025-08-18" + default: "nightly-2025-08-18"
77-84: Bump Docker actions to current majors.Use latest majors for fixes and performance.
- - name: Set up QEMU - uses: docker/setup-qemu-action@v2 + - name: Set up QEMU + uses: docker/setup-qemu-action@v3 - - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v2 + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@v3 - - name: Login to Docker Hub - uses: docker/login-action@v2 + - name: Login to Docker Hub + uses: docker/login-action@v3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/common.yml(1 hunks).github/workflows/coordinator.yml(1 hunks).github/workflows/intermediate-docker.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (3)
.github/workflows/coordinator.yml (1)
36-38: Nightly bump: verify rustfmt/clippy availability for 2025-08-18.Nightly components can disappear; ensure this date has both rustfmt and clippy to avoid CI flakes.
.github/workflows/common.yml (1)
32-34: Nightly bump: confirm component availability.Ensure rustfmt/clippy exist for nightly-2025-08-18 to avoid intermittent failures.
.github/workflows/intermediate-docker.yml (1)
85-103: ARG RUST_VERSION usage verified — no action requiredIntermediate Dockerfiles that should consume RUST_VERSION do declare and use it: build/dockerfiles/intermediate/{cuda-go-rust-builder.Dockerfile, go-rust-builder.Dockerfile, go-rust-alpine-builder.Dockerfile, rust-builder.Dockerfile, rust-alpine-builder.Dockerfile}. build/dockerfiles/intermediate/go-alpine-builder.Dockerfile and build/dockerfiles/intermediate/py-runner.Dockerfile neither declare nor reference RUST_VERSION (their workflow TAGs don't include Rust).
|
if the gpu testing works well, i think there will be many files that can be cleaned. They can be cleaned in later PRs. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
crates/l2geth/src/rpc_client.rs (1)
113-121: Good: panic replaced with error. Include parent hash for easier triage.Optional tweak:
- .ok_or_else(|| { - eyre::eyre!( - "parent block for block {} should exist", - block.header.number - ) - })?; + .ok_or_else(|| { + eyre::eyre!( + "parent block {} (for block {}) not found", + block.header.parent_hash, + block.header.number + ) + })?;crates/prover-bin/src/zk_circuits_handler/universal.rs (2)
19-19: Drop unused ProofType argument from new() (and its import)
_proof_typeis ignored. Remove it to avoid confusion and dead deps; update call sites accordingly.Suggested diff (also removes the import at top; adjust callers):
-use scroll_proving_sdk::prover::ProofType; @@ - pub fn new(workspace_path: impl AsRef<Path>, _proof_type: ProofType) -> Result<Self> { + pub fn new(workspace_path: impl AsRef<Path>) -> Result<Self> {Verify call sites:
#!/bin/bash rg -nP 'UniversalHandler::new\s*\(' -C2 rg -nP '\bProofType\b' -g '!**/target/**'
14-16: Remove unsafe Send impl; current safety note is not a proofDeclaring
unsafe impl Sendfor a type wrapping a non‑obviouslySendProveris unsound. If upstream madeProver: Sendin zkvm 0.6, this impl is unnecessary; if not, this masks a real issue. Drop it or justify with concrete upstream guarantees.Apply this diff:
-/// Safe for current usage as `CircuitsHandler` trait (protected inside of Mutex and NEVER extract -/// the instance out by `into_inner`) -unsafe impl Send for UniversalHandler {}
🧹 Nitpick comments (5)
crates/l2geth/src/rpc_client.rs (4)
81-87: Avoid nestedimpl Traitin return type; returnimpl ChunkInterpreterinstead.
RpcClient<'_, impl Provider<Network>>in a return type may require nightly/unstable features depending on your toolchain. Safer: hide the concrete type behind the trait you already implement.- pub fn get_client(&self) -> RpcClient<'_, impl Provider<Network>> { + pub fn get_client(&self) -> impl ChunkInterpreter { RpcClient { provider: ProviderBuilder::<_, _, Network>::default() .connect_client(self.client.clone()), handle: self.rt.handle(), } }If you prefer to keep the concrete type, consider a private TAIT alias or a concrete provider type instead.
97-101: Pass provider by reference in async helper.Make the helper take
&impl Provider<Network>to avoid relying on reference blanket impls implicitly and to prevent accidental moves. The call sites already pass&self.provider.- async fn fetch_witness_async( - provider: impl Provider<Network>, + async fn fetch_witness_async( + provider: &impl Provider<Network>, block_hash: sbv_primitives::B256, prev_witness: Option<&sbv_core::BlockWitness>, ) -> Result<sbv_core::BlockWitness> {Apply the same pattern to
fetch_storage_node_async.
157-159: Mirror the provider reference for storage-node helper.- async fn fetch_storage_node_async( - provider: impl Provider<Network>, + async fn fetch_storage_node_async( + provider: &impl Provider<Network>, node_hash: sbv_primitives::B256, ) -> Result<sbv_primitives::Bytes> {Call site already passes
&self.provider.
195-217: Fix test logic: comment contradicts assertion; also avoid relying on a failing dummy hash.If the first hash is meant to fail, assert error and don’t bind it to
wit1. Then either:
- fetch a valid witness with the second hash using
None, or- first get a valid
wit1, then testprev_witnesspath with the next block.Here’s a minimal fix keeping one success path:
- // This is expected to fail since we're using a dummy hash, but it tests the code path - let wit1 = client - .try_fetch_block_witness(block_hash, None) - .expect("should success"); + // This is expected to fail since we're using a dummy hash + assert!( + client + .try_fetch_block_witness(block_hash, None) + .is_err(), + "expected failure for dummy block hash" + ); @@ - let wit2 = client - .try_fetch_block_witness(block_hash, Some(&wit1)) + let wit2 = client + .try_fetch_block_witness(block_hash, None) .expect("should success");If you want to exercise the
prev_witnesspath, fetch a realwit1first, then call withSome(&wit1). Want me to provide that variant?crates/prover-bin/src/zk_circuits_handler/universal.rs (1)
35-36: Narrow API surface: avoid exposing &mut Prover publiclyReturning
&mut Proverbroadens the mutable surface. Preferpub(crate)or a thin method that performs the needed calls, keepingProverencapsulated.Apply this small change:
- pub fn get_prover(&mut self) -> &mut Prover { + pub(crate) fn get_prover(&mut self) -> &mut Prover { &mut self.prover }LGTM on making the lock mutable (Line 47) to align with this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/l2geth/src/rpc_client.rs(6 hunks)crates/prover-bin/src/zk_circuits_handler/universal.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/l2geth/src/rpc_client.rs (1)
crates/l2geth/src/lib.rs (1)
get_client(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (1)
crates/prover-bin/src/zk_circuits_handler/universal.rs (1)
29-29: Enable SNARK/EVM backend when required — pass true to Prover::setupUpstream (scroll_zkvm_prover v0.6 / openvm 1.4) shows the second argument is a boolean that enables the halo2/EVM SNARK backend when true. At crates/prover-bin/src/zk_circuits_handler/universal.rs:29 change the call from Prover::setup(config, None) to Prover::setup(config, true) (or Prover::setup(config, Some(true)) if the API is Option<bool)); ensure halo2 params are available (ProverConfig.dir_halo2_params or $HOME/.openvm) and host GPU/CUDA prerequisites are met. There is no dedicated runtime capability query — pre-check params/files or handle setup errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
zkvm-prover/Makefile (2)
1-1: Add missing PHONYs and convenience targets (align with checkmake).Include clean/version and add all/test convenience targets to satisfy tooling and make UX clearer.
-.PHONY: prover prover_cpu lint tests_binary test_e2e_run test_run +.PHONY: all clean test version prover prover_cpu lint tests_binary test_e2e_run test_runAdd these targets outside the changed hunk:
# convenience targets all: prover test: cargo test --release
39-39: Parameterize features for GPU/CPU builds.Expose cargo features via a make var to toggle cuda (or others) without editing the file.
- GO_TAG=${GO_TAG} GIT_REV=${GIT_REV} ZK_VERSION=${ZK_VERSION} cargo build --locked --release -Z unstable-options --lockfile-path ../crates/gpu_override/Cargo.lock -p prover + GO_TAG=${GO_TAG} GIT_REV=${GIT_REV} ZK_VERSION=${ZK_VERSION} cargo build --locked --release -Z unstable-options --lockfile-path ../crates/gpu_override/Cargo.lock -p prover $(if $(FEATURES),--features $(FEATURES),)Declare the opt-in var outside the hunk:
FEATURES ?= # e.g., make prover FEATURES=cuda
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/gpu_override/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/gpu_override/Makefile(0 hunks)crates/gpu_override/print_plonky3gpu_version.sh(0 hunks)zkvm-prover/Makefile(2 hunks)
💤 Files with no reviewable changes (2)
- crates/gpu_override/print_plonky3gpu_version.sh
- crates/gpu_override/Makefile
🧰 Additional context used
🪛 checkmake (0.2.2)
zkvm-prover/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (2)
zkvm-prover/Makefile (2)
42-42: LGTM: simple, deterministic version string.Echoing GO_TAG-GIT_REV-ZK_VERSION is clear and reproducible.
47-48: Fix invalid cargo clean flags (zkvm-prover/Makefile — lines 47–48)cargo clean does not support --release or --lockfile-path; -Z unstable-options is unnecessary. Verified
cargo clean --helpcontains no --release. Sandbox couldn't runmake clean(make: not found) — runmake cleanlocally/CI to confirm the target succeeds.-clean: - cargo clean -Z unstable-options --release -p prover --lockfile-path ../crates/gpu_override/Cargo.lock +clean: + cargo clean -p prover
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/l2geth/src/rpc_client.rs (1)
135-139: Fix Result/Option handling: removetranspose()and stray??(compile blocker).
.send().awaitlikely returnsResult<Option<_>, _>. Calling.transpose()here and then??won’t compile. Propagate the RPC error, then turnNoneinto an error.Apply this diff:
- let witness = req - .send() - .await - .transpose() - .ok_or_else(|| eyre::eyre!("Block witness {block_num} not available"))??; + let witness = req + .send() + .await? + .ok_or_else(|| eyre::eyre!("Block witness {block_num} not available"))?;
🧹 Nitpick comments (3)
crates/l2geth/src/rpc_client.rs (3)
113-121: Improve error context for missing parent block.Include the parent hash and use a clear “not found” wording.
Apply this diff:
- .ok_or_else(|| { - eyre::eyre!( - "parent block for block {} should exist", - block.header.number - ) - })?; + .ok_or_else(|| { + eyre::eyre!( + "parent block {} for block {} not found", + block.header.parent_hash, + block.header.number + ) + })?;
48-60: Name shadowing withRpcClient(ours) vsalloy::rpc::client::RpcClient.Not a bug, but easy to confuse during maintenance. Consider renaming our struct (e.g.,
ZkpRpcClientorL2GethClient) to avoid ambiguity.
195-199: Stale, date-specific comments and contradictory note.Comments reference “2025.9.11” and say “expected to fail” while the assertion expects success. Clean up to avoid confusion.
Apply this diff:
- // latest - 1 block in 2025.9.11 + // Example block (chain-specific; test is ignored by default) ... - // This is expected to fail since we're using a dummy hash, but it tests the code path + // Fetch witness for the example block; expects success when pointed at the right chain ... - // block selected in 2025.9.11 + // Another example block (chain-specific)Optionally, read block hashes from env vars (e.g., L2GETH_BLOCK_HASH1/2) to make this test portable.
Also applies to: 208-212
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/l2geth/src/rpc_client.rs(6 hunks)crates/prover-bin/src/zk_circuits_handler/universal.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/prover-bin/src/zk_circuits_handler/universal.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/l2geth/src/rpc_client.rs (2)
crates/l2geth/src/lib.rs (1)
get_client(14-19)crates/libzkp/src/tasks/chunk_interpreter.rs (2)
try_fetch_block_witness(9-15)try_fetch_storage_node(16-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (3)
crates/l2geth/src/rpc_client.rs (3)
57-60: GenericRpcClientoverProvider<Network>looks good.This unlocks provider-agnostic usage without leaking concrete types.
81-87: Provider-agnosticget_client()is clean.Returning
RpcClient<'_, impl Provider<Network>>keeps the public API flexible while preserving type safety.
169-169: LGTM.Blocking on the per-instance runtime for this one-shot is appropriate here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
zkvm-prover/Makefile (2)
1-1: Add missing PHONYs and basic aliases ("all", "test", "clean", "version").Align with checkmake and avoid accidental file/dir name collisions.
Apply:
-.PHONY: prover prover_cpu lint tests_binary test_e2e_run test_run +.PHONY: all prover prover_cpu clean version test tests_binary test_e2e_run test_run lint + +# Default aliases +all: prover +test: tests_binary
39-39: Gate CUDA via configurable features to avoid failures on non‑GPU builders.Make GPU optional without changing current default.
Apply:
+PROVER_FEATURES ?= cuda prover: - GO_TAG=${GO_TAG} GIT_REV=${GIT_REV} ZK_VERSION=${ZK_VERSION} cargo build --locked --release --features cuda -p prover + GO_TAG=${GO_TAG} GIT_REV=${GIT_REV} ZK_VERSION=${ZK_VERSION} cargo build --locked --release -p prover $(if $(strip $(PROVER_FEATURES)),--features $(PROVER_FEATURES),)Add CI matrix entries: CPU (empty features) and GPU (PROVER_FEATURES=cuda).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/gpu_override/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
zkvm-prover/Makefile(2 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
zkvm-prover/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (1)
zkvm-prover/Makefile (1)
42-42: LGTM: version string is concise and reproducible.
4 participants
Summary by CodeRabbit
New Features
Improvements
Performance
Chores