chore: openvm 1.6#1783
Conversation
…d in current code base)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR bumps the workspace version and pinned zkvm dependencies, removes legacy witness encoding paths, reorganizes imports to nested Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant ProverBuilder
participant Dumper
participant FS
CLI->>ProverBuilder: invoke Dump (task_id, task_type, json_mode)
ProverBuilder->>Dumper: construct Dumper(json_mode, target_path)
Dumper->>FS: write input_task.json (if json_mode)
Dumper->>FS: write input_task.bin / agg_proofs.bin (if binary)
Dumper-->>ProverBuilder: return ProveResponse (failed with message)
ProverBuilder-->>CLI: exit (dump completed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Around line 17-23: Update the inline comment that currently reads "with openvm
1.4.2" to "with openvm 1.4.3" in the Cargo.toml workspace.dependencies block so
the comment matches the actual OpenVM version recorded in Cargo.lock; locate the
comment near the scroll-zkvm-prover/scroll-zkvm-verifier/scroll-zkvm-types
entries and change only the version number in that comment.
In `@crates/prover-bin/src/dumper.rs`:
- Around line 13-16: The Dumper struct's target_path is currently ignored by
dump(); update the Dumper::dump (and any related methods around lines 24-39) to
use the Dumper.target_path when creating/writing the output file: convert
target_path to a Path/PathBuf, ensure the parent directory exists
(create_dir_all if needed), join the intended filename to that path instead of
using the current working directory, and open/create the file at that joined
path for writing; also preserve existing json_mode logic and error handling when
writing to the computed target path.
In `@crates/prover-bin/src/main.rs`:
- Around line 60-61: Change the clap argument for the json flag to use
flag-style parsing by replacing the current default_value usage with action =
ArgAction::SetTrue for the json_mode field; locate the json_mode field
declaration in main.rs (the #[arg(long = "json", default_value = "false")]
json_mode: bool,) and update it to use #[arg(long = "json", action =
ArgAction::SetTrue)] json_mode: bool, and ensure ArgAction is in scope (same
pattern used by the version flag in this file).
- Around line 95-108: The Dump command currently calls
ProverBuilder::new(...).build().await and then ignores the boolean/result of
Arc::new(prover).one_shot(&[task_id], task_type.into()).await; modify the Dump
command path to check the returned Result/bool from one_shot (same as the Handle
path does), and if it indicates failure return or propagate an error to the CLI
(e.g., map_err/eyre::eyre! or bail). Locate the call site for one_shot in
main.rs and ensure you mirror the Handle flow: inspect the awaited value,
convert it to a useful error on failure, and propagate it up instead of
discarding it.
In `@tests/prover-e2e/mainnet-galileo/genesis.json`:
- Around line 23-25: The fork-time keys use inconsistent casing: change the
object keys "GalileoTime" and "GalileoV2Time" to lowerCamelCase "galileoTime"
and "galileoV2Time" so they match the rest of the fork-time fields (e.g.,
"feynmanTime", "darwinTime") and the JSON unmarshal expectation; update the keys
in the genesis JSON where "GalileoTime" and "GalileoV2Time" appear (replace with
"galileoTime" and "galileoV2Time") to restore correct fork activation parsing.
🧹 Nitpick comments (2)
tests/prover-e2e/mainnet-galileo/config.json (1)
8-11: Make the RPC endpoint configurable to improve E2E stability.The hardcoded
https://mainnet-rpc.scroll.ioendpoint lacks override mechanisms (no env vars, no CLI flags, no template substitution). E2E tests become brittle against rate limits and service outages. Consider adding environment variable support or command-line override forfetch_config.endpointso tests can point to controlled or local RPC nodes during CI and local development.crates/prover-bin/src/prover.rs (1)
33-35: Debug-mode skip is OK; consider clearer operator signal.Default
falsekeeps compatibility, but in debug mode stale/corrupt local files may be used. Consider logging the full local path (or a warning) to make this obvious in logs.Also applies to: 85-91
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.tomlcrates/libzkp/src/lib.rscrates/libzkp/src/proofs.rscrates/libzkp/src/tasks/batch.rscrates/libzkp/src/tasks/bundle.rscrates/libzkp/src/tasks/chunk.rscrates/prover-bin/Cargo.tomlcrates/prover-bin/src/dumper.rscrates/prover-bin/src/main.rscrates/prover-bin/src/prover.rscrates/prover-bin/src/zk_circuits_handler/universal.rstests/prover-e2e/mainnet-galileo/.make.envtests/prover-e2e/mainnet-galileo/config.jsontests/prover-e2e/mainnet-galileo/config.template.jsontests/prover-e2e/mainnet-galileo/genesis.jsontests/prover-e2e/sepolia-feynman/.make.envtests/prover-e2e/sepolia-feynman/00100_import_blocks.sqltests/prover-e2e/sepolia-feynman/genesis.json
💤 Files with no reviewable changes (3)
- tests/prover-e2e/sepolia-feynman/.make.env
- tests/prover-e2e/sepolia-feynman/00100_import_blocks.sql
- tests/prover-e2e/sepolia-feynman/genesis.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T02:09:51.657Z
Learnt from: noel2004
Repo: scroll-tech/scroll PR: 1736
File: crates/libzkp/src/verifier/universal.rs:35-45
Timestamp: 2025-09-16T02:09:51.657Z
Learning: The verify_stark_proof method in scroll-zkvm-verifier returns Result<(), Error> indicating success/failure, not Result<bool, Error>. When it succeeds, verification passed; when it fails, it returns an error.
Applied to files:
crates/libzkp/src/tasks/batch.rscrates/libzkp/src/proofs.rscrates/libzkp/src/tasks/bundle.rs
🧬 Code graph analysis (6)
crates/libzkp/src/tasks/batch.rs (1)
crates/libzkp/src/tasks.rs (1)
encode_task_to_witness(19-22)
crates/libzkp/src/tasks/chunk.rs (1)
crates/libzkp/src/tasks.rs (1)
encode_task_to_witness(19-22)
crates/prover-bin/src/prover.rs (4)
crates/prover-bin/src/types.rs (2)
default(62-64)default(105-107)crates/libzkp/src/lib.rs (1)
new(24-40)crates/prover-bin/src/zk_circuits_handler/universal.rs (1)
new(19-31)crates/libzkp/src/tasks.rs (1)
new(53-58)
crates/prover-bin/src/main.rs (1)
common/types/message/message.go (1)
ProofType(14-14)
crates/libzkp/src/proofs.rs (4)
coordinator/internal/types/block.go (1)
BatchInfo(4-8)common/types/message/message.go (1)
ChunkInfo(131-148)common/version/version.go (1)
Version(31-31)coordinator/internal/utils/version.go (1)
Version(16-42)
crates/libzkp/src/tasks/bundle.rs (1)
crates/libzkp/src/tasks.rs (1)
encode_task_to_witness(19-22)
⏰ 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). (1)
- GitHub Check: tests
🔇 Additional comments (19)
crates/libzkp/src/tasks/chunk.rs (2)
4-8: LGTM: updated chunk import path.Aligns with the new
scroll::chunkmodule layout and keeps types centralized.
117-126: LGTM: unified witness serialization.Using
encode_task_to_witnesshere keeps serialization consistent across tasks.crates/libzkp/src/tasks/batch.rs (2)
4-13: LGTM: batch/chunk imports reorganized.Keeps batch and chunk types under the unified
scrollmodule structure.
119-133: LGTM: consolidated witness encoding path.Consistent use of
encode_task_to_witnessimproves maintainability.crates/libzkp/src/tasks/bundle.rs (2)
3-7: LGTM: bundle imports aligned with new module layout.
27-40: LGTM: standardized witness serialization.crates/libzkp/src/proofs.rs (3)
6-13: Import reorganization looks good.The migration from flat paths to nested
scroll::{...}module paths aligns with the broader codebase refactoring. The types (BatchInfo,BundleInfo,ChunkInfo) are used consistently throughout the file.
216-216: Test import update is consistent.The test import aligns with the new
scroll::bundle::BundleInfopath used in the main imports.
254-254: Versioned API approach is more flexible.Using
pi_hash_by_version(version::Version::euclid_v1())instead of a specificpi_hash_euclidv1()method is a cleaner pattern that allows the same API to handle multiple protocol versions. This is consistent with thepi_hash_checkmethod on line 181 that takesversion::Versionas a parameter, and the pattern is used consistently across the codebase (chunk.rs, bundle.rs, batch.rs).tests/prover-e2e/mainnet-galileo/config.template.json (1)
13-15: LGTM — template now aligned with Galileo fork settings.Also applies to: 27-27
tests/prover-e2e/mainnet-galileo/.make.env (1)
1-3: Confirm blocks 26653680–26653686 fall within the Galileo fork activation window.The block range is consumed inclusively (7 blocks total) from the official Scroll mainnet RPC. Verify that these block numbers correspond to the Galileo fork epoch by checking against Scroll's fork activation documentation or querying actual block timestamps; otherwise, the E2E test coverage may be misleading or flaky if blocks predate the fork activation.
crates/prover-bin/Cargo.toml (1)
12-12: Verify dependency pins for scroll-proving-sdk and bincode 2.0.Please confirm the git rev
22ad34eis the intended commit and that bincode 2.0 withserdeis compatible with the new dumper serialization path.Also applies to: 37-37
crates/prover-bin/src/zk_circuits_handler/universal.rs (1)
19-27: Confirm the segment_len bump aligns with resource budgets.
segment_lenincreased to(1 << 22) - 100, which can affect memory and throughput. Please confirm this is the intended target for the OpenVM 1.4.x prover configuration and fits downstream limits.crates/prover-bin/src/prover.rs (2)
214-227: Panic propagation matches the “fail-fast” requirement.Resuming unwind on task panic will terminate the worker as intended.
294-296: Verify OpenVM 1.3 tasks are intentionally hard-rejected.
use_openvm_13now causes an immediate bail, andUniversalHandler::newno longer accepts the flag. Please confirm upstream won’t send OpenVM 1.3 tasks and that the error message is sufficient for operators.Also applies to: 323-323
crates/prover-bin/src/dumper.rs (1)
54-87: Confirm Failed status is the intended dump-mode signal.
prove()andquery_task()always returnFailedto force exit. Please verify this matches the coordinator’s expectations (no retries/alerts for dump mode).crates/libzkp/src/lib.rs (1)
24-37: Deprecation handling looks good.The explicit warnings for
legacy_witnessandopenvm_13make misconfiguration visible while keeping behavior deterministic.crates/prover-bin/src/main.rs (2)
1-6: New dumper module wiring looks fine.
36-51: TaskType → ProofType mapping is clear and complete.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/prover-e2e/cloak-galileoV2/config.json (1)
1-15: Minor: Inconsistent JSON indentation.The
db_configblock uses 4-space indentation while other blocks use 2-space indentation. Also, there's an extra blank line at line 14.🧹 Suggested formatting fix
{ - "db_config": { - "driver_name": "postgres", - "dsn": "postgres://dev:dev@localhost:5432/scroll?sslmode=disable", - "maxOpenNum": 5, - "maxIdleNum": 1 + "db_config": { + "driver_name": "postgres", + "dsn": "postgres://dev:dev@localhost:5432/scroll?sslmode=disable", + "maxOpenNum": 5, + "maxIdleNum": 1 }, "fetch_config": { "endpoint": "http://cloak-usx-sequencer.mainnet.scroll.tech:8545", "l2_message_queue_address": "0x5300000000000000000000000000000000000000" }, "validium_mode": true, "codec_version": 10 - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/prover-e2e/cloak-galileoV2/config.json` around lines 1 - 15, The JSON file has inconsistent indentation in the "db_config" block (4 spaces) compared to other blocks (2 spaces) and an extra blank line before the closing brace; normalize indentation across the file (e.g., convert "db_config" to 2-space indentation to match "fetch_config", "validium_mode", and "codec_version") and remove the trailing blank line so the JSON is consistently formatted and the object ends immediately with the closing brace.tests/prover-e2e/sepolia-galileoV2/config.template.json (1)
29-29: Keep the template customizable unless this file is intentionally a pinned fixture.Line 29 now bakes in a public RPC endpoint, which makes consumers of the template depend on Scroll's shared Sepolia node by default. If this file is still meant to be copied and edited, keep the placeholder here and leave the concrete value in
config.json.Based on learnings: In configuration files like
rollup/proposer-tool-config.json, placeholders such as<mainnet read db config>are intentionally left as-is to be replaced by users with their own configuration values when deploying the tool.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/prover-e2e/sepolia-galileoV2/config.template.json` at line 29, The template currently hardcodes the public RPC URL in the JSON key "endpoint" (value "http://l2geth-rpc-0.sepolia.scroll.tech:8545/"); replace that concrete URL with a clear placeholder (e.g. "<sepolia_rpc_endpoint>") so the template remains customizable and leave the real RPC URL only in the concrete config.json used by CI/deployment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/prover-e2e/cloak-galileoV2/config.json`:
- Around line 1-15: The JSON file has inconsistent indentation in the
"db_config" block (4 spaces) compared to other blocks (2 spaces) and an extra
blank line before the closing brace; normalize indentation across the file
(e.g., convert "db_config" to 2-space indentation to match "fetch_config",
"validium_mode", and "codec_version") and remove the trailing blank line so the
JSON is consistently formatted and the object ends immediately with the closing
brace.
In `@tests/prover-e2e/sepolia-galileoV2/config.template.json`:
- Line 29: The template currently hardcodes the public RPC URL in the JSON key
"endpoint" (value "http://l2geth-rpc-0.sepolia.scroll.tech:8545/"); replace that
concrete URL with a clear placeholder (e.g. "<sepolia_rpc_endpoint>") so the
template remains customizable and leave the real RPC URL only in the concrete
config.json used by CI/deployment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8473a821-8fd2-4525-b120-102069e7167d
📒 Files selected for processing (8)
.gitignoretests/prover-e2e/cloak-galileoV2/.make.envtests/prover-e2e/cloak-galileoV2/config.jsontests/prover-e2e/cloak-galileoV2/config.template.jsontests/prover-e2e/cloak-galileoV2/genesis.jsontests/prover-e2e/sepolia-galileoV2/.make.envtests/prover-e2e/sepolia-galileoV2/config.jsontests/prover-e2e/sepolia-galileoV2/config.template.json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/prover-e2e/cloak-galileoV2/.make.env
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/prover-bin/src/main.rs (1)
106-108:⚠️ Potential issue | 🟠 MajorReturn a non-zero status when
dumpfails.Line 106 awaits
one_shotand then falls through toOk(()). Since theHandlepath on Lines 122-141 treats the same awaited value as a success/failure signal, afalseresult from dump currently exits 0 and looks successful to the caller.#!/bin/bash set -euo pipefail # Show how `main.rs` handles the awaited `one_shot` result in Dump vs Handle. sed -n '104,141p' crates/prover-bin/src/main.rs💡 Suggested change
- std::sync::Arc::new(prover) - .one_shot(&[task_id], task_type.into()) - .await; + let prover = std::sync::Arc::new(prover); + let ok = prover + .one_shot(std::slice::from_ref(&task_id), task_type.into()) + .await; + if !ok { + eyre::bail!("dump failed for task_id={task_id}"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/prover-bin/src/main.rs` around lines 106 - 108, The Dump branch currently awaits std::sync::Arc::new(prover).one_shot(&[task_id], task_type.into()).await and then unconditionally returns Ok(()); modify the Dump handling in main (the match arm that calls one_shot) to inspect the boolean result from one_shot and return a non-zero exit on failure — e.g., if result is false call std::process::exit(1) or return Err(...) from main — so failure mirrors the Handle path's success/failure semantics; locate the call to one_shot and the surrounding Dump match arm to implement this check and proper non-zero exit.
🧹 Nitpick comments (1)
crates/prover-bin/src/main.rs (1)
90-109: BuildLocalProverlazily for the new dump path.The branch starting at Line 90 never uses
local_prover, butmainstill constructs it before thematch. Moving that initialization intoHandleandNonekeeps the dump path from doing unrelated startup work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/prover-bin/src/main.rs` around lines 90 - 109, The main creates LocalProver eagerly which the Dump branch doesn't need; instead make LocalProver built lazily only in the branches that require it (the Handle variant and the None/default startup path). Move the existing ProverBuilder::new(...) / .build().await block (and any creation of local_prover) out of the pre-match setup and into the Handle match arm and into the None/startup arm, leaving the Dump arm to only build the dumper prover as shown (so Dump never constructs LocalProver).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/prover-bin/src/main.rs`:
- Around line 106-108: The Dump branch currently awaits
std::sync::Arc::new(prover).one_shot(&[task_id], task_type.into()).await and
then unconditionally returns Ok(()); modify the Dump handling in main (the match
arm that calls one_shot) to inspect the boolean result from one_shot and return
a non-zero exit on failure — e.g., if result is false call std::process::exit(1)
or return Err(...) from main — so failure mirrors the Handle path's
success/failure semantics; locate the call to one_shot and the surrounding Dump
match arm to implement this check and proper non-zero exit.
---
Nitpick comments:
In `@crates/prover-bin/src/main.rs`:
- Around line 90-109: The main creates LocalProver eagerly which the Dump branch
doesn't need; instead make LocalProver built lazily only in the branches that
require it (the Handle variant and the None/default startup path). Move the
existing ProverBuilder::new(...) / .build().await block (and any creation of
local_prover) out of the pre-match setup and into the Handle match arm and into
the None/startup arm, leaving the Dump arm to only build the dumper prover as
shown (so Dump never constructs LocalProver).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ff2c80c-23b4-407a-969e-80b38e6ad74c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlcrates/prover-bin/Cargo.tomlcrates/prover-bin/src/main.rszkvm-prover/.work/.gitignorezkvm-prover/Makefile
✅ Files skipped from review due to trivial changes (3)
- zkvm-prover/.work/.gitignore
- Cargo.toml
- zkvm-prover/Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/prover-bin/Cargo.toml
Update stuffs Update intros purge files purge files
+ more defensive instruction + prune file being added by mistake
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 participants
This PR purpose a stable version for prover after GalileoV2 forking.
This PR would replace #1774
Current status:
scroll-proving-sdkfor better supporting of local storage, retry policy and axiom proverSummary by CodeRabbit
New Features
Bug Fixes
Chores