-
Notifications
You must be signed in to change notification settings - Fork 627
Optimization for prover #1761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Optimization for prover #1761
Conversation
WalkthroughUpdated dependency pins in workspace and prover-bin, added Changes
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/prover-bin/src/prover.rs (1)
214-227: Error handling improvements look good.The previous review feedback has been addressed: the typo is fixed ("process") and the error message correctly says "proving task failed" for non-panic
JoinErrorcases. The intentional panic propagation viaresume_unwindaligns with the PR objective to exit the prover when the proving process panics.
🧹 Nitpick comments (1)
crates/prover-bin/src/prover.rs (1)
85-91: Debug mode skip logic is correct.The early
continueproperly bypasses network verification when the file exists locally in debug mode.Consider using
tracing::debug!instead ofprintln!for consistency with structured logging, though the existing code in this function also usesprintln!.
📜 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 (2)
Cargo.toml(1 hunks)crates/prover-bin/src/prover.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/prover-bin/src/prover.rs (1)
crates/prover-bin/src/types.rs (2)
default(62-64)default(105-107)
⏰ 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)
crates/prover-bin/src/prover.rs (1)
33-35: Debug mode field properly implemented.The
#[serde(default)]attribute ensures backward compatibility with existing configurations, and the defaultfalsevalue maintains production behavior.Cargo.toml (1)
20-23: Revision pinning is appropriate here.Using a specific commit when you need changes beyond the tagged release is the right approach. The commit
9fb016fexists on the master branch and provides the v0.7.1 plus openvm performance features your workspace requires.
This PR purpose a stable version for prover working along with GalileoV2 forking.
Current status:
v0.7.1Summary by CodeRabbit
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.