Optimization for prover (override #1761) #1774
Conversation
…d in current code base)
WalkthroughThis PR removes legacy witness encoding infrastructure from the zkprover, discontinues OpenVM 13 support, pins zkvm dependencies to specific commits, introduces test infrastructure for the Galileo fork, and deprecates Feynman test configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ 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: 2
🧹 Nitpick comments (1)
crates/prover-bin/src/prover.rs (1)
85-91: Consider usingtracingfor consistency.The
println!here is inconsistent with the rest of the codebase which uses thetracingcrate. Consider usingtracing::debug!for observability consistency.if self.debug_mode { - println!( + tracing::debug!( "File {} already exists, skipping download under debugmode", filename ); continue; }
📜 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 (15)
Cargo.toml(1 hunks)crates/libzkp/src/lib.rs(1 hunks)crates/libzkp/src/tasks/batch.rs(2 hunks)crates/libzkp/src/tasks/bundle.rs(2 hunks)crates/libzkp/src/tasks/chunk.rs(2 hunks)crates/prover-bin/Cargo.toml(1 hunks)crates/prover-bin/src/prover.rs(5 hunks)crates/prover-bin/src/zk_circuits_handler/universal.rs(1 hunks)tests/prover-e2e/mainnet-galileo/.make.env(1 hunks)tests/prover-e2e/mainnet-galileo/config.json(1 hunks)tests/prover-e2e/mainnet-galileo/config.template.json(2 hunks)tests/prover-e2e/mainnet-galileo/genesis.json(1 hunks)tests/prover-e2e/sepolia-feynman/.make.env(0 hunks)tests/prover-e2e/sepolia-feynman/00100_import_blocks.sql(0 hunks)tests/prover-e2e/sepolia-feynman/genesis.json(0 hunks)
💤 Files with no reviewable changes (3)
- tests/prover-e2e/sepolia-feynman/genesis.json
- tests/prover-e2e/sepolia-feynman/.make.env
- tests/prover-e2e/sepolia-feynman/00100_import_blocks.sql
🧰 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/tasks/bundle.rs
🧬 Code graph analysis (4)
crates/libzkp/src/tasks/batch.rs (2)
coordinator/internal/types/block.go (1)
BatchInfo(4-8)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/zk_circuits_handler/universal.rs (2)
crates/libzkp/src/lib.rs (1)
new(24-40)crates/libzkp/src/tasks.rs (1)
new(53-58)
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/prover-bin/src/zk_circuits_handler/universal.rs (2)
19-19: Removal of OpenVM 1.3 compatibility parameter looks correct.This aligns with the PR objective to remove OpenVM 1.3 support, simplifying the constructor signature.
22-22: Verify the segment length increase is intentional.The
segment_lenis doubled from(1 << 21) - 100(~2M) to(1 << 22) - 100(~4M). This likely impacts memory usage and proving performance. Confirm this change is tested and aligns with the reported proving speed improvement (0.2 MHz → 0.7 MHz).crates/prover-bin/src/prover.rs (4)
33-35: LGTM - debug_mode field addition.The
debug_modefield with#[serde(default)]provides backward-compatible configuration for skipping network verification during local development.
214-227: Critical fix for panic propagation - LGTM.Using
resume_unwindcorrectly ensures the process exits when the proving task panics, preventing a non-working prover from lingering. This aligns with the PR objective.
294-296: Proper guard for OpenVM 1.3 deprecation.This correctly rejects tasks requesting OpenVM 1.3 snark params. Note that
libzkp::DynamicFeature::newstill parses and warns about theopenvm_13feature flag (seecrates/libzkp/src/lib.rslines 29-32), which provides a migration path with clear warnings.
323-323: LGTM - Updated to match simplified constructor.The call correctly matches the updated
UniversalHandler::newsignature that no longer requires theis_openvm_v13parameter.crates/prover-bin/Cargo.toml (1)
12-12: Commit22ad34eis valid and exists in scroll-proving-sdk.The dependency update to commit
22ad34ebdeb3d9a0ccb2993bb46d8d2522c1e667is confirmed. Review the actual changes to ensure they include the expected GalileoV2 fork compatibility updates.Cargo.toml (1)
20-23: Dependency pinning to specific commit.The switch from tag-based (
v0.7.1) to rev-based (2e8e29f) pinning ensures reproducible builds. The comment documents the OpenVM version relationship.However, verify that commit
2e8e29fexists in the scroll-tech/zkvm-prover repository and that the OpenVM 1.4.2 reference is accurate.tests/prover-e2e/mainnet-galileo/config.json (1)
8-13: LGTM!The
fetch_configaddition with the mainnet RPC endpoint and thecodec_versionupdate to 9 are consistent with the Galileo fork migration.tests/prover-e2e/mainnet-galileo/.make.env (1)
1-3: LGTM!The block range and fork name configuration look appropriate for the Galileo e2e tests. Note that
SCROLL_FORK_NAMEuses=(non-overridable) while block variables use?=(overridable) - this appears intentional to enforce the galileo fork context.tests/prover-e2e/mainnet-galileo/config.template.json (1)
13-14: LGTM!The verifier configuration properly references the Galileo assets and fork name.
tests/prover-e2e/mainnet-galileo/genesis.json (1)
1-111: LGTM overall!The genesis configuration is comprehensive and includes:
- Proper chainId (534352) matching other config files
- Complete fork activation schedule
- L1/L2 configuration with message queue addresses
- Pre-deployed system contracts with appropriate storage initialization
crates/libzkp/src/tasks/chunk.rs (2)
1-8: LGTM - Import cleanup aligns with legacy encoding removal.The removal of
LegacyChunkWitnessandRancorErrorfrom imports is consistent with eliminating the legacy witness encoding path.
117-130: LGTM - Simplified witness serialization.The removal of the conditional legacy witness encoding path simplifies the code. The function now always uses
encode_task_to_witness, which is consistent with the PR's goal of removing OpenVM 1.3 compatibility.crates/libzkp/src/tasks/batch.rs (2)
1-13: LGTM - Consistent import cleanup.The removal of
LegacyBatchWitnessfrom imports mirrors the changes in chunk.rs, maintaining consistency across task types.
117-134: LGTM - Witness encoding simplified.The batch task now consistently uses
encode_task_to_witnessfor serialization, matching the approach in chunk and bundle tasks.crates/libzkp/src/tasks/bundle.rs (2)
1-8: LGTM - Import cleanup completes the trilogy.The removal of
LegacyBundleWitnesscompletes the consistent cleanup across all three task types (chunk, batch, bundle).
27-44: LGTM - Consistent implementation across all task types.The bundle task follows the same pattern as chunk and batch tasks, using
encode_task_to_witnessexclusively and properly propagating fork context.crates/libzkp/src/lib.rs (1)
23-40: OpenVM 1.3 flag usage is consistent with PR objectives.The flag is actively used: when
use_openvm_13is set to true, the prover explicitly rejects the task with the error "prover do not support snark params base on openvm 13" (crates/prover-bin/src/prover.rs:294-295). This implementation correctly removes OpenVM 1.3 support by preventing any guest built from openvm-1.3 from being processed, which aligns with the stated PR goal.
| "chain_id": 534351, | ||
| "chain_id": 534352, | ||
| "l2geth": { | ||
| "endpoint": "<serach a public rpc endpoint like alchemy>" |
There was a problem hiding this comment.
Typo in placeholder comment.
Minor typo: "serach" should be "search".
"l2geth": {
- "endpoint": "<serach a public rpc endpoint like alchemy>"
+ "endpoint": "<search a public rpc endpoint like alchemy>"
}📝 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.
| "endpoint": "<serach a public rpc endpoint like alchemy>" | |
| "l2geth": { | |
| "endpoint": "<search a public rpc endpoint like alchemy>" | |
| } |
🤖 Prompt for AI Agents
In tests/prover-e2e/mainnet-galileo/config.template.json around line 29, the
placeholder comment contains a typo ("serach"); update the placeholder to read
"search a public rpc endpoint like alchemy" so the comment is spelled correctly
and clearer to users.
| "feynmanTime": 1755576000, | ||
| "GalileoTime": 1765868400, | ||
| "GalileoV2Time": 1766041200, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all genesis files and check for fork time naming patterns
echo "=== Searching for fork time patterns in genesis files ==="
rg "feynmanTime|GalileoTime|galileoTime|GalileoV2Time" --type json -B2 -A2Repository: scroll-tech/scroll
Length of output: 2143
Change fork time field names to lowercase for consistency.
Lines 23-25 use inconsistent capitalization: feynmanTime (lowercase) vs GalileoTime and GalileoV2Time (PascalCase). All other genesis files in the repo (sepolia-galileo, sepolia-galileoV2) use lowercase naming (galileoTime, galileoV2Time), and this matches upstream Scroll protocol naming. Change to: feynmanTime, galileoTime, galileoV2Time.
🤖 Prompt for AI Agents
In tests/prover-e2e/mainnet-galileo/genesis.json around lines 23 to 25, the fork
time field names use inconsistent capitalization; change "GalileoTime" to
"galileoTime" and "GalileoV2Time" to "galileoV2Time" so all three keys follow
lowercase naming ("feynmanTime", "galileoTime", "galileoV2Time") consistent with
other genesis files and upstream Scroll naming; update the JSON keys accordingly
without altering their numeric values.
This PR purpose a stable version for prover after GalileoV2 forking.
This PR would replace #1761
Current status:
zkvm-proverwith openvm 1.4.2Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.