Skip to content

Conversation

@noel2004
Copy link
Member

@noel2004 noel2004 commented Nov 25, 2025

This PR purpose a stable version for prover working along with GalileoV2 forking.

Current status:

  • depending on zkvm-prover v0.7.1
  • drop the compatibility for guest built from openvm-1.3, performance gain from ~0.2MHz -> 0.7MHz (proving speed)
  • exit process while proving process panic to avoid a not-working prover lingers

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling: worker panics now terminate the prover; non-panic failures return a unified failed status and message.
  • New Features

    • Added a debug mode for asset handling that can skip remote checks and emits debug logs when local assets are used.
  • Chores

    • Updated proving SDK and workspace pins to specific revisions.
    • Increased default processing segment capacity for better efficiency.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 25, 2025

Walkthrough

Updated dependency pins in workspace and prover-bin, added debug_mode to asset location handling (skipping remote checks when true), changed prover task error handling to resume unwinding on panics and return unified Failed responses for non-panic errors, and increased the universal handler's default segment length from 2^21-100 to 2^22-100.

Changes

Cohort / File(s) Change Summary
prover-bin dependency
crates/prover-bin/Cargo.toml
Updated scroll-proving-sdk git revision from 05648db to 22ad34e.
Workspace manifest
Cargo.toml
Switched three workspace dependency entries from tag pinning (tag = "v0.7.1") to commit pinning (rev = "9fb016f"), added explanatory comment.
Prover runtime & assets
crates/prover-bin/src/prover.rs
Added public field debug_mode: bool to AssetsLocationData; when a local asset exists and debug_mode is true, skip remote HEAD/download and log debug. Modified LocalProver::query_task error handling: on task handle Err(e), if e.is_panic() then resume_unwind (re-throw); otherwise return QueryTaskResponse with Failed status and message proving task failed: {e}.
Universal handler config
crates/prover-bin/src/zk_circuits_handler/universal.rs
Increased segment_len in UniversalHandler::new from (1 << 21) - 100 to (1 << 22) - 100.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay extra attention to crates/prover-bin/src/prover.rs for:
    • Correct visibility and usage of the new debug_mode field and its effects on asset fetching logic.
    • Correct detection of panics via is_panic() and safe use of std::panic::resume_unwind, ensuring no resource leaks or inconsistent worker state.
  • Verify workspace Cargo changes correctly resolve and that the pinned revisions are intentional.
  • Validate that the increased segment_len value is compatible with associated proving configuration and resource expectations.

Possibly related PRs

Suggested reviewers

  • georgehao
  • Thegaram

Poem

🐰 A hop, a skip, a revision cheer,
New commits snugged, old tags disappear.
If a task panics, I let it leap free,
If not, I hush errors politely.
Segments grow tall — I munch logs with glee. ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ⚠️ Warning The PR description is incomplete and lacks proper structure. It does not follow the required template with sections for purpose/design rationale, PR title format (conventional commits), deployment tag versioning, or breaking change label. Provide a complete PR description following the template: include explicit answers to what/why/how questions, ensure PR title follows conventional commits format (e.g., 'perf: ...'), and check the deployment tag versioning and breaking change label checkboxes.
Title check ❓ Inconclusive The title 'Optimization for prover' is vague and generic, lacking specificity about the actual changes (dependency updates, panic handling, segment length adjustment). Consider a more specific title following conventional commits format (e.g., 'perf: optimize prover with zkvm-prover v0.7.1 and panic recovery').
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/prover_4.7

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9dceae1 and 9839bf7.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is 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 22ad34e exists and is correctly referenced. Since no tag exists for this commit, using the commit hash is the appropriate approach for deterministic builds.

Copy link

@coderabbitai coderabbitai bot left a 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_len going from (1 << 21) - 100 to (1 << 22) - 100 roughly 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::ProverConfig or the universal circuits.

I’d recommend:

  1. Running this value through your usual Galileo scenarios (max concurrency, largest expected proofs) to ensure no OOM or unacceptable latency regressions.
  2. Adding a short comment or central constant explaining why (1 << 22) - 100 is the right value for this fork, so future tuning is less guesswork.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9839bf7 and af63bc0.

📒 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

@noel2004 noel2004 changed the title [DO NOT MERGE] For prover 4.7 Optimization for prover Dec 3, 2025
lispc
lispc previously approved these changes Dec 9, 2025
Copy link

@coderabbitai coderabbitai bot left a 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 JoinError cases. The intentional panic propagation via resume_unwind aligns 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 continue properly bypasses network verification when the file exists locally in debug mode.

Consider using tracing::debug! instead of println! for consistency with structured logging, though the existing code in this function also uses println!.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ae31bc and b833468.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is 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 default false value 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 9fb016f exists on the master branch and provides the v0.7.1 plus openvm performance features your workspace requires.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants