Optimization for prover (override #1761) #1774
Conversation
…d in current code base)
WalkthroughPins zkvm workspace dependencies to specific commits, removes runtime legacy witness-encoding branches and Legacy* witness types, adds a Dumper one-shot CLI flow that writes binary outputs, updates prover asset/handler behavior and OpenVM handling, and replaces Feynman e2e fixtures with Galileo ones. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI
participant Dumper
participant FS as Filesystem
participant ProvingSvc as ProvingService
CLI->>Dumper: Dump(command) with input JSON
Dumper->>Dumper: parse JSON → ProvingTaskExt → ProvingTask
Dumper->>FS: write `input_task.bin` (serialized_witness) via bincode
Dumper->>FS: write `agg_proofs.bin` (aggregated_proofs) via bincode
Dumper->>ProvingSvc: return ProveResponse (status/error)
ProvingSvc-->>CLI: deliver result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/prover-bin/src/dumper.rs (1)
71-73: Consider returning an error response instead ofunreachable!()for robustness.While the one-shot pattern makes
query_task()unexpected, usingunreachable!()will panic if this method is accidentally called. A more defensive approach would return an error response indicating this operation is unsupported.🔎 Suggested refactor for more graceful error handling:
async fn query_task(&mut self, _: QueryTaskRequest) -> QueryTaskResponse { - unreachable!("for one_shot routine, we should be returned in prove call"); + QueryTaskResponse { + status: TaskStatus::Failed, + error: Some("query_task is not supported for one-shot dumper".to_string()), + ..Default::default() + } }
📜 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 (3)
crates/prover-bin/Cargo.toml(2 hunks)crates/prover-bin/src/dumper.rs(1 hunks)crates/prover-bin/src/main.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/prover-bin/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/prover-bin/Cargo.toml
⏰ 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
| #[derive(Default)] | ||
| pub struct Dumper { | ||
| #[allow(dead_code)] | ||
| target_path: String, | ||
| } |
There was a problem hiding this comment.
Unused target_path field while output paths are hardcoded.
The target_path field is marked as dead code and never used. However, the dump() method writes to hardcoded file paths "input_task.bin" and "agg_proofs.bin" in the current working directory (lines 24 and 33). This appears to be an incomplete implementation.
Consider using target_path to control the output directory:
🔎 Suggested fix to use `target_path` for output files:
impl Dumper {
fn dump(&self, input_string: &str) -> eyre::Result<()> {
let task: ProvingTaskExt = serde_json::from_str(input_string)?;
let task = ProvingTask::from(task);
// stream-encode serialized_witness to input_task.bin using bincode 2.0
- let input_file = std::fs::File::create("input_task.bin")?;
+ let input_path = std::path::Path::new(&self.target_path).join("input_task.bin");
+ let input_file = std::fs::File::create(input_path)?;
let mut input_writer = std::io::BufWriter::new(input_file);
bincode::serde::encode_into_std_write(
&task.serialized_witness,
&mut input_writer,
bincode::config::standard(),
)?;
// stream-encode aggregated_proofs to agg_proofs.bin using bincode 2.0
- let agg_file = std::fs::File::create("agg_proofs.bin")?;
+ let agg_path = std::path::Path::new(&self.target_path).join("agg_proofs.bin");
+ let agg_file = std::fs::File::create(agg_path)?;
let mut agg_writer = std::io::BufWriter::new(agg_file);
bincode::serde::encode_into_std_write(
&task.aggregated_proofs,
&mut agg_writer,
bincode::config::standard(),
)?;
Ok(())
}
}And remove the #[allow(dead_code)] attribute:
#[derive(Default)]
pub struct Dumper {
- #[allow(dead_code)]
target_path: String,
}📝 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.
| #[derive(Default)] | |
| pub struct Dumper { | |
| #[allow(dead_code)] | |
| target_path: String, | |
| } | |
| #[derive(Default)] | |
| pub struct Dumper { | |
| target_path: String, | |
| } |
| async fn prove(&mut self, req: ProveRequest) -> ProveResponse { | ||
| let error = if let Err(e) = self.dump(&req.input) { | ||
| Some(format!("failed to dump: {}", e)) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| ProveResponse { | ||
| status: TaskStatus::Failed, | ||
| error, | ||
| ..Default::default() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full dumper.rs file to understand the context
cat crates/prover-bin/src/dumper.rsRepository: scroll-tech/scroll
Length of output: 2301
🏁 Script executed:
# Search for TaskStatus enum definition
rg "enum TaskStatus" -A 10Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
# Search for how prove() is used
rg "\.prove\(" -B 2 -A 2Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
# Look for any comments or documentation about Dumper's purpose
rg -i "dumper" -B 2 -A 2 | head -100Repository: scroll-tech/scroll
Length of output: 1372
🏁 Script executed:
# Search for TaskStatus definition in different ways
rg "TaskStatus" -B 2 -A 5 | head -150Repository: scroll-tech/scroll
Length of output: 7859
🏁 Script executed:
# Look for tests related to dumper
fd "test" -path "*dumper*" -o -name "*test*" | grep -i dumpRepository: scroll-tech/scroll
Length of output: 230
🏁 Script executed:
# Search for TaskStatus in proving_sdk or prover modules
fd "proving_service" -o -name "*.rs" | xargs rg "enum TaskStatus\|pub enum TaskStatus" -A 10Repository: scroll-tech/scroll
Length of output: 294
🏁 Script executed:
# Search for TaskStatus enum definition in scroll_proving_sdk
find . -name "*.rs" -type f | xargs rg "pub enum TaskStatus" -A 10Repository: scroll-tech/scroll
Length of output: 46
🏁 Script executed:
# Also check if there's documentation about Dumper's purpose
find . -name "*.md" -o -name "README*" | xargs grep -l -i "dump" 2>/dev/null | head -5Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
# Search for TaskStatus definition more broadly
rg "enum TaskStatus|struct TaskStatus" -A 15Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
# Check if scroll_proving_sdk is in the repo or external
find . -name "scroll_proving_sdk" -type dRepository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
# Look at Cargo.toml to understand dependencies
cat Cargo.toml 2>/dev/null || find . -name "Cargo.toml" -path "*/prover-bin/*" -exec cat {} \;Repository: scroll-tech/scroll
Length of output: 2786
🏁 Script executed:
# Check prover-bin's Cargo.toml for dependencies
cat crates/prover-bin/Cargo.tomlRepository: scroll-tech/scroll
Length of output: 1129
🏁 Script executed:
# Search for any local definition of TaskStatus or proving_service
find . -name "*.rs" -type f -exec grep -l "proving_service\|ProvingService" {} \; | head -20Repository: scroll-tech/scroll
Length of output: 129
Status should reflect dump success or failure, not always return Failed.
The prove() method always sets status: TaskStatus::Failed regardless of whether the dump succeeds. When error is None (successful dump), the status should be TaskStatus::Success. This causes all dump operations to be incorrectly reported as failed.
Update the implementation to conditionally set the status:
async fn prove(&mut self, req: ProveRequest) -> ProveResponse {
- let error = if let Err(e) = self.dump(&req.input) {
- Some(format!("failed to dump: {}", e))
- } else {
- None
- };
+ let (status, error) = if let Err(e) = self.dump(&req.input) {
+ (TaskStatus::Failed, Some(format!("failed to dump: {}", e)))
+ } else {
+ (TaskStatus::Success, None)
+ };
ProveResponse {
- status: TaskStatus::Failed,
+ status,
error,
..Default::default()
}
}📝 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.
| async fn prove(&mut self, req: ProveRequest) -> ProveResponse { | |
| let error = if let Err(e) = self.dump(&req.input) { | |
| Some(format!("failed to dump: {}", e)) | |
| } else { | |
| None | |
| }; | |
| ProveResponse { | |
| status: TaskStatus::Failed, | |
| error, | |
| ..Default::default() | |
| } | |
| } | |
| async fn prove(&mut self, req: ProveRequest) -> ProveResponse { | |
| let (status, error) = if let Err(e) = self.dump(&req.input) { | |
| (TaskStatus::Failed, Some(format!("failed to dump: {}", e))) | |
| } else { | |
| (TaskStatus::Success, None) | |
| }; | |
| ProveResponse { | |
| status, | |
| error, | |
| ..Default::default() | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/prover-bin/src/dumper.rs around lines 57 to 69, the prove() method
always returns TaskStatus::Failed even when dump succeeds; change the status to
be conditional on the dump result: call self.dump(&req.input) and if it returns
Err set error to Some(...) and status to TaskStatus::Failed, otherwise set error
to None and status to TaskStatus::Success, then construct ProveResponse using
that conditional status and error (keep the rest of the default fields).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/prover-bin/src/dumper.rs (2)
12-16: Unusedtarget_pathfield while output paths are hardcoded.This issue was already flagged in a previous review. The
target_pathfield is marked as dead code and never used, while thedump()method writes to hardcoded paths"input_task.bin"and"agg_proofs.bin"in the current working directory.
57-69: Status should reflect dump success or failure, not always return Failed.This issue was already flagged in a previous review. The
prove()method always setsstatus: TaskStatus::Failedregardless of whether the dump succeeds. WhenerrorisNone(successful dump), the status should beTaskStatus::Success.
🧹 Nitpick comments (1)
crates/prover-bin/src/dumper.rs (1)
23-39: Consider atomic file writes to prevent partial corruption.The method writes directly to the final file paths without using temporary files. If the process crashes or is interrupted between writing
input_task.bin(line 24) andagg_proofs.bin(line 33), it could leave the output in an inconsistent state with only one file present.Consider writing to temporary files first and then renaming them atomically:
🔎 View atomic write pattern:
use std::fs; // Write input_task.bin atomically let input_temp = "input_task.bin.tmp"; let input_file = std::fs::File::create(input_temp)?; let mut input_writer = std::io::BufWriter::new(input_file); bincode::serde::encode_into_std_write( &task.serialized_witness, &mut input_writer, bincode::config::standard(), )?; drop(input_writer); // Ensure flush fs::rename(input_temp, "input_task.bin")?; // Write agg_proofs.bin atomically let agg_temp = "agg_proofs.bin.tmp"; let agg_file = std::fs::File::create(agg_temp)?; let mut agg_writer = std::io::BufWriter::new(agg_file); bincode::serde::encode_into_std_write( &task.aggregated_proofs, &mut agg_writer, bincode::config::standard(), )?; drop(agg_writer); // Ensure flush fs::rename(agg_temp, "agg_proofs.bin")?;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/prover-bin/src/dumper.rs(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). (1)
- GitHub Check: tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
crates/prover-bin/src/dumper.rs (2)
12-16: Unusedtarget_pathfield while output paths are hardcoded.The
target_pathfield remains marked as dead code and is never used. Thedump()method still writes to hardcoded paths"input_task.bin"and"agg_proofs.bin"in the current directory. This issue was flagged in a previous review but has not been addressed.
59-71: Status should reflect dump success or failure, not always return Failed.The
prove()method still always setsstatus: TaskStatus::Failedon line 67, regardless of whether the dump succeeds (whenerrorisNone). This issue was flagged in a previous review but remains unresolved.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/prover-bin/Cargo.toml(2 hunks)crates/prover-bin/src/dumper.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/prover-bin/Cargo.toml
⏰ 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
crates/prover-bin/src/dumper.rs
Outdated
| // stream-encode serialized_witness to input_task.bin using bincode 2.0 | ||
| let input_file = std::fs::File::create("input_task.bin")?; | ||
| let mut input_writer = std::io::BufWriter::new(input_file); | ||
| bincode::encode_into_std_write( |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "dumper.rs" -type f 2>/dev/null | head -20Repository: scroll-tech/scroll
Length of output: 95
🏁 Script executed:
cat -n ./crates/prover-bin/src/dumper.rs | head -60Repository: scroll-tech/scroll
Length of output: 2381
🏁 Script executed:
# Search for ProvingTask struct definition to understand field types
rg -A 20 "struct ProvingTask" --type rustRepository: scroll-tech/scroll
Length of output: 1025
🏁 Script executed:
# Find where ProvingTask is defined (it's imported from scroll_zkvm_types)
rg "pub struct ProvingTask" --type rust -A 15Repository: scroll-tech/scroll
Length of output: 820
🏁 Script executed:
# Find ProvingTask definition in scroll_zkvm_types crate
find . -name "*.rs" -type f -exec grep -l "pub struct ProvingTask" {} \; | head -5Repository: scroll-tech/scroll
Length of output: 90
🏁 Script executed:
# Check the import statement and look for where ProvingTask comes from
rg "use.*ProvingTask|from scroll_zkvm_types" crates/prover-bin/src/dumper.rsRepository: scroll-tech/scroll
Length of output: 125
🏁 Script executed:
# Find scroll_zkvm_types crate and its ProvingTask definition
find . -path "*/scroll_zkvm_types/*" -name "*.rs" -type f | head -10Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
# Check Cargo.toml for prover-bin to understand bincode configuration
cat crates/prover-bin/Cargo.tomlRepository: scroll-tech/scroll
Length of output: 1152
🏁 Script executed:
# Check if scroll_zkvm_types is a workspace member
rg "scroll.zkvm.types|scroll-zkvm-types" workspace.members -A 2 -B 2 2>/dev/null || cat Cargo.toml | grep -A 50 "\[workspace\]"Repository: scroll-tech/scroll
Length of output: 1695
🏁 Script executed:
# Search for deserialization or loading code for these binary files
rg "aggregated_proofs|serialized_witness" crates/ -B 2 -A 2 --type rust | head -60Repository: scroll-tech/scroll
Length of output: 4156
🏁 Script executed:
# Try to get struct definitions from the external git dependency documentation
# Let's check what types serialized_witness and proof.proofs are
rg "struct.*Proof|serialized_witness.*:|proofs.*:" crates/libzkp/src/ --type rust -B 1 -A 3 | head -80Repository: scroll-tech/scroll
Length of output: 4761
🏁 Script executed:
# Look for deserialization code that reads these files back
rg "decode_from_std_read|decode_from" crates/ --type rust -B 2 -A 2 | head -60Repository: scroll-tech/scroll
Length of output: 44
🏁 Script executed:
# Search for any code that reads input_task.bin or agg_proofs.bin files
rg "input_task\.bin|agg_proofs\.bin" crates/ --type rust -B 3 -A 3Repository: scroll-tech/scroll
Length of output: 1346
Clarify the bincode API usage and fix the loop encoding pattern.
Line 26 uses bincode::encode_into_std_write for serialized_witness (a native Rust Vec type), while line 36 uses bincode::serde::encode_into_std_write for proof.proofs (a serde type). This API split is intentional and correct based on trait implementations.
However, lines 35-41 encode each proof individually in a loop, concatenating multiple bincode values to agg_proofs.bin. Without a length prefix or container wrapper, deserialization cannot reliably determine where one proof ends and the next begins. Either encode the entire aggregated_proofs collection at once, or add explicit length prefixes for each proof.
🤖 Prompt for AI Agents
In crates/prover-bin/src/dumper.rs around lines 26 and 35-41, the current code
encodes each proof in a loop (lines ~35-41) which writes concatenated bincode
blobs without boundaries, making deserialization ambiguous; instead encode the
entire aggregated_proofs collection in one shot so bincode records lengths
internally. Replace the per-item loop with a single call to the serde bincode
encoder (e.g., bincode::serde::encode_into_std_write) that writes the whole
Vec/collection of proofs to agg_proofs.bin; keep the existing
bincode::encode_into_std_write usage for serialized_witness at line 26 (since it
uses the native encode API), but switch the proofs writing to a single serde
encode of the collection to ensure correct round-trip deserialization.
crates/prover-bin/src/dumper.rs
Outdated
| for proof in &task.aggregated_proofs { | ||
| bincode::serde::encode_into_std_write( | ||
| &proof.proofs, | ||
| &mut agg_writer, | ||
| bincode::config::standard(), | ||
| )?; | ||
| } |
There was a problem hiding this comment.
Loop encodes individual proofs, not the aggregated collection.
The code encodes each proof.proofs separately in a loop, concatenating multiple encoded values into agg_proofs.bin. This pattern makes deserialization ambiguous—a reader won't know how many proofs to decode without additional metadata.
The comment "stream-encode aggregated_proofs" suggests encoding the entire collection once. Encoding individual items without length prefixes or delimiters will likely cause deserialization failures.
🔎 Consider encoding the entire collection at once:
// stream-encode aggregated_proofs to agg_proofs.bin using bincode 2.0
let agg_file = std::fs::File::create("agg_proofs.bin")?;
let mut agg_writer = std::io::BufWriter::new(agg_file);
-for proof in &task.aggregated_proofs {
- bincode::serde::encode_into_std_write(
- &proof.proofs,
- &mut agg_writer,
- bincode::config::standard(),
- )?;
-}
+bincode::serde::encode_into_std_write(
+ &task.aggregated_proofs,
+ &mut agg_writer,
+ bincode::config::standard(),
+)?;Alternatively, if individual encoding is required, prefix each entry with its length or use a proper container format.
📝 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.
| for proof in &task.aggregated_proofs { | |
| bincode::serde::encode_into_std_write( | |
| &proof.proofs, | |
| &mut agg_writer, | |
| bincode::config::standard(), | |
| )?; | |
| } | |
| for proof in &task.aggregated_proofs { | |
| bincode::serde::encode_into_std_write( | |
| &task.aggregated_proofs, | |
| &mut agg_writer, | |
| bincode::config::standard(), | |
| )?; | |
| } |
🤖 Prompt for AI Agents
In crates/prover-bin/src/dumper.rs around lines 35 to 41, the code currently
encodes each proof.proofs in a loop which concatenates multiple bincode blobs
into agg_proofs.bin and makes deserialization ambiguous; replace the loop by
encoding the entire aggregated_proofs collection once (i.e., serialize
task.aggregated_proofs as a single container) so a reader can decode the whole
collection in one call, or if you must stream individual entries, write a length
prefix before each encoded item (serialize the length then the item) so
deserializers can know where each entry ends.
| async fn query_task(&mut self, req: QueryTaskRequest) -> QueryTaskResponse { | ||
| QueryTaskResponse { | ||
| task_id: req.task_id, | ||
| status: TaskStatus::Failed, | ||
| error: Some("dump file finished but need a fail return to exit".to_string()), | ||
| ..Default::default() | ||
| } | ||
| } |
There was a problem hiding this comment.
Misleading task status despite intentional exit behavior.
The query_task() method returns TaskStatus::Failed with message "dump file finished but need a fail return to exit" for all queries. While this appears intentional to force process termination (per PR objectives about ensuring exit), it violates expected API semantics—the status should reflect the actual task outcome, not be used as a process control mechanism.
Consider separating concerns: track actual task status (Success/Failed based on dump result) and use a different mechanism (e.g., explicit process exit in the caller) to ensure termination.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
crates/prover-bin/src/dumper.rs (4)
14-15: Unusedtarget_pathfield while output paths are hardcoded.This issue was already flagged in a previous review. The
target_pathfield remains unused while thedump()method continues to write files to the current working directory with hardcoded names.
40-47: Loop encodes individual proofs, causing deserialization ambiguity.This issue was already flagged in a previous review. The loop concatenates multiple bincode blobs without length metadata, making it impossible to deserialize correctly without knowing the count in advance.
66-78: Status should reflect dump success or failure, not always return Failed.This issue was already flagged in a previous review. The
prove()method returnsTaskStatus::Failedregardless of whether the dump succeeds, causing successful operations to be incorrectly reported as failed.
80-87: Misleading task status despite intentional exit behavior.This issue was already flagged in a previous review. The
query_task()method returnsTaskStatus::Failedwith a message indicating this is intentional for process control, but it violates expected API semantics.
🧹 Nitpick comments (2)
crates/prover-bin/src/dumper.rs (1)
46-46: Consider using tracing instead of println! for consistency.The code uses
println!for logging proof sizes. Since the codebase initializes tracing (viainit_tracing()in main.rs), consider usingtracing::info!ortracing::debug!for consistency with the rest of the logging infrastructure.🔎 Apply this diff to use tracing:
- println!("written {sz} bytes for proof"); + tracing::info!("written {sz} bytes for proof");crates/prover-bin/src/main.rs (1)
106-108: Unnecessary Arc wrapper since prover is not cloned.Unlike the
Handlebranch which callsprover.clone()multiple times, theDumpbranch uses the prover only once. TheArc::newwrapper adds unnecessary overhead.🔎 Simplify by removing Arc wrapper:
- std::sync::Arc::new(prover) - .one_shot(&[task_id], task_type.into()) + prover + .one_shot(&[task_id.clone()], task_type.into()) .await;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/prover-bin/src/dumper.rs(1 hunks)crates/prover-bin/src/main.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/prover-bin/src/main.rs (2)
common/types/message/message.go (1)
ProofType(14-14)crates/prover-bin/src/prover.rs (1)
new(254-286)
⏰ 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 (3)
crates/prover-bin/src/main.rs (3)
36-41: LGTM!The
TaskTypeenum is well-defined with appropriate derives for CLI usage via clap'sValueEnum.
59-65: LGTM!The
Dumpsubcommand definition is clear and has appropriate field types for the dumping workflow.
43-51: TheFrom<TaskType>forProofTypeconversion is correctly implemented with all required variants present in the external dependency.
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
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.