Optimization for prover #1761
Optimization for prover #1761
Conversation
WalkthroughUpdated scroll-proving-sdk dependency revision, modified panic handling in the prover query task to re-throw panics instead of returning error responses, and increased the segment length configuration from 2^21 to 2^22 in the universal handler. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(1 hunks)crates/prover-bin/Cargo.toml(1 hunks)crates/prover-bin/src/prover.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
🔇 Additional comments (2)
Cargo.toml (1)
20-22: Dependency updates to pre-release v0.7.0-rc.4 are consistent and valid.The three zkvm workspace dependencies are updated consistently to the same tag, which has been verified to exist in the scroll-tech/zkvm-prover repository. All references are syntactically correct and properly formatted.
crates/prover-bin/Cargo.toml (1)
12-12: Commit hash verified and properly pinned.The commit
22ad34eexists and is correctly referenced. Since no tag exists for this commit, using the commit hash is the appropriate approach for deterministic builds.
| Err(e) => { | ||
| if e.is_panic() { | ||
| // simply re-throw panic for any panicking in proving prrocess, | ||
| // cause worker loop and the whole prover exit | ||
| std::panic::resume_unwind(e.into_panic()); | ||
| } | ||
|
|
||
| QueryTaskResponse { | ||
| task_id: req.task_id, | ||
| status: TaskStatus::Failed, | ||
| error: Some(format!("proving task panicked: {}", e)), | ||
| ..Default::default() | ||
| } | ||
| } |
There was a problem hiding this comment.
Incorrect error message for non-panic JoinError.
The else branch (lines 212-217) handles non-panic JoinErrors (e.g., task cancellation), but the error message incorrectly states "proving task panicked". This branch is only reached when e.is_panic() is false.
Also, minor typo on line 207: "prrocess" → "process".
Err(e) => {
if e.is_panic() {
- // simply re-throw panic for any panicking in proving prrocess,
+ // simply re-throw panic for any panicking in proving process,
// cause worker loop and the whole prover exit
std::panic::resume_unwind(e.into_panic());
}
QueryTaskResponse {
task_id: req.task_id,
status: TaskStatus::Failed,
- error: Some(format!("proving task panicked: {}", e)),
+ error: Some(format!("proving task failed: {}", e)),
..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.
| Err(e) => { | |
| if e.is_panic() { | |
| // simply re-throw panic for any panicking in proving prrocess, | |
| // cause worker loop and the whole prover exit | |
| std::panic::resume_unwind(e.into_panic()); | |
| } | |
| QueryTaskResponse { | |
| task_id: req.task_id, | |
| status: TaskStatus::Failed, | |
| error: Some(format!("proving task panicked: {}", e)), | |
| ..Default::default() | |
| } | |
| } | |
| Err(e) => { | |
| if e.is_panic() { | |
| // simply re-throw panic for any panicking in proving process, | |
| // cause worker loop and the whole prover exit | |
| std::panic::resume_unwind(e.into_panic()); | |
| } | |
| QueryTaskResponse { | |
| task_id: req.task_id, | |
| status: TaskStatus::Failed, | |
| error: Some(format!("proving task failed: {}", e)), | |
| ..Default::default() | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/prover-bin/src/prover.rs around lines 205 to 218, the non-panic
JoinError branch logs "proving task panicked" which is incorrect and there is a
typo "prrocess". Change the error message to reflect non-panic JoinError cases
(e.g., cancelled or join error) and include the JoinError details (e.g., via
e.to_string() or debug formatting) in the error field, and correct the comment
typo to "process". Keep the existing resume_unwind call for the is_panic() case
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/prover-bin/src/zk_circuits_handler/universal.rs (1)
20-28: Segment length doubled; please validate memory/perf impact and consider documenting the magic constant
segment_lengoing from(1 << 21) - 100to(1 << 22) - 100roughly doubles the segment size. Type and shape stay the same (Some(<int>)), so this should be API‑compatible, but it can materially change:
- Peak memory usage per prover instance.
- Per‑segment proving latency, and thus how well this runs on existing Galileo worker hardware.
- Any implicit limits/expectations inside
scroll_zkvm_prover::ProverConfigor the universal circuits.I’d recommend:
- Running this value through your usual Galileo scenarios (max concurrency, largest expected proofs) to ensure no OOM or unacceptable latency regressions.
- Adding a short comment or central constant explaining why
(1 << 22) - 100is the right value for this fork, so future tuning is less guesswork.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/prover-bin/src/zk_circuits_handler/universal.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
This PR purpose a stable version for prover working along with GalileoV2 forking.
Current status:
v0.7.1Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.