-
Notifications
You must be signed in to change notification settings - Fork 624
[Upgrade] feynman 0.5.0rc1 #1699
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
Conversation
WalkthroughThis update introduces new dependency versions and tags, adds stricter task assignment logic in the coordinator, enhances batch header and fork handling in the prover, and implements new Makefiles and scripts for build and release processes. Debugging and error reporting are improved, and new CLI functionality is added for handling multiple tasks. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (prover-bin)
participant Prover as Prover
participant TaskSet as HandleSet
participant Chunk as ChunkTask
participant Batch as BatchTask
participant Bundle as BundleTask
CLI->>TaskSet: Read and parse JSON task file
CLI->>Prover: Build prover instance
CLI->>Prover: For each chunk in TaskSet.chunks
Prover->>Chunk: Process chunk task (one_shot)
CLI->>Prover: For each batch in TaskSet.batches
Prover->>Batch: Process batch task (one_shot)
CLI->>Prover: For each bundle in TaskSet.bundles
Prover->>Bundle: Process bundle task (one_shot)
CLI-->>CLI: Print completion messages
sequenceDiagram
participant Coordinator as Coordinator
participant Prover as Prover
participant TaskDB as Task Database
Prover->>Coordinator: Assign task request (with optional TaskID)
Coordinator->>TaskDB: Check assigned task for prover
alt Prover has assigned task
Coordinator->>Prover: Error if task type mismatch
else Prover provides TaskID
Coordinator->>TaskDB: Fetch task by TaskID
alt Task not found
Coordinator->>Prover: Error (task dropped)
else
Coordinator->>Prover: Assign fetched task
end
else
Coordinator->>TaskDB: Assign new task
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 8
🧹 Nitpick comments (9)
crates/libzkp_c/src/lib.rs (2)
74-89
: Consider security and portability implications of debug dumping.The debug dumping feature is useful for diagnostics, but has some concerns:
- Security: Failed proofs are dumped to
/tmp
which may be accessible to other users on the system- Portability:
/tmp
path is Unix-specific and may not work on all platforms- Disk usage: No cleanup mechanism for old debug files
Consider using a configurable debug directory with proper permissions or a more secure temporary location.
- let filename = format!("/tmp/proof_{}.json", timestamp); + let debug_dir = std::env::var("ZKVM_DEBUG_DIR").unwrap_or_else(|_| "/tmp".to_string()); + let filename = format!("{}/proof_{}.json", debug_dir, timestamp);
198-212
: Apply the same security and portability improvements here.The same concerns about debug file location apply to the task dumping logic. Consider using a configurable debug directory as suggested in the proof dumping section.
- let filename = format!("/tmp/task_{}_{}.json", c_str.to_str().unwrap(), timestamp); + let debug_dir = std::env::var("ZKVM_DEBUG_DIR").unwrap_or_else(|_| "/tmp".to_string()); + let filename = format!("{}/task_{}_{}.json", debug_dir, c_str.to_str().unwrap(), timestamp);coordinator/internal/logic/provertask/chunk_prover_task.go (1)
237-237
: Minor: Error wrapping removed.The error formatting was changed from
%w
(error wrapping) to%v
(simple formatting). While this is consistent with the AI summary mentioning similar changes in related files, consider if error wrapping might be beneficial for error chain analysis.crates/gpu_override/Makefile (2)
1-1
: Add missing standard phony targets.The Makefile is missing common phony targets that are typically expected in build systems.
-.PHONY: build update clean +.PHONY: all test build update clean + +all: build + +test: + @echo "No tests defined for GPU override crate"
4-10
: Consider adding validation for extracted version values.The version extraction logic appears robust, but it would be helpful to validate that the extracted values are non-empty to catch configuration issues early.
PLONKY3_GPU_VERSION=$(shell ./print_plonky3gpu_version.sh | sed -n '2p') +ifeq ($(PLONKY3_GPU_VERSION),) +$(error PLONKY3_GPU_VERSION is empty - check print_plonky3gpu_version.sh) +endif $(info PLONKY3_GPU_VERSION is ${PLONKY3_GPU_VERSION})crates/prover-bin/src/main.rs (1)
42-46
: Consider improving the command description.The comment "path to save the verifier's asset" appears to be copied from the
Dump
command and doesn't accurately describe theHandle
command's purpose.Handle { - /// path to save the verifier's asset + /// path to JSON file containing task sets to process task_path: String, },coordinator/internal/logic/provertask/bundle_prover_task.go (1)
100-108
: Consider enhancing error messages for better debugging.The implementation correctly handles explicit task ID requests. However, the error messages could be more descriptive.
- return nil, fmt.Errorf("Expected task (%s) is already dropped", getTaskParameter.TaskID) + return nil, fmt.Errorf("Expected task (ID: %s) has already been dropped and is no longer available", getTaskParameter.TaskID)zkvm-prover/Makefile (1)
56-58
: Consider providing more helpful error message for circuit artifacts.The error message could be more informative about where to get the artifacts or how to run the download script.
- @echo "Download stuff with download-release.sh, and put them into correct directory"; + @echo "Circuit artifacts not found. Please run './download-release.sh' to download required artifacts and place them in the correct directory (.work/)";crates/libzkp/src/tasks/batch.rs (1)
25-28
: Consider documenting the V7_8 variant design decision.The combined
V7_8
variant suggests that V7 and V8 headers share the same structure. Add a comment explaining why these versions are combined to help future maintainers.#[serde(untagged)] pub enum BatchHeaderV { V6(BatchHeaderV6), + /// V7 and V8 headers share the same structure but differ in processing logic V7_8(BatchHeaderV7), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lock
is excluded by!**/*.lock
crates/gpu_override/Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.toml
(2 hunks)coordinator/Makefile
(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go
(2 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go
(4 hunks)coordinator/internal/logic/provertask/chunk_prover_task.go
(3 hunks)coordinator/internal/logic/verifier/verifier.go
(3 hunks)crates/gpu_override/.cargo/config.toml
(1 hunks)crates/gpu_override/Makefile
(1 hunks)crates/gpu_override/print_plonky3gpu_version.sh
(1 hunks)crates/l2geth/src/rpc_client.rs
(1 hunks)crates/libzkp/Cargo.toml
(1 hunks)crates/libzkp/src/lib.rs
(2 hunks)crates/libzkp/src/tasks.rs
(2 hunks)crates/libzkp/src/tasks/batch.rs
(4 hunks)crates/libzkp_c/src/lib.rs
(3 hunks)crates/prover-bin/src/main.rs
(3 hunks)crates/prover-bin/src/prover.rs
(1 hunks)zkvm-prover/Makefile
(4 hunks)zkvm-prover/download-release.sh
(1 hunks)zkvm-prover/release-verifier-stuff.sh
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
coordinator/internal/logic/provertask/bundle_prover_task.go (1)
Learnt from: colinlyguo
PR: scroll-tech/scroll#1530
File: rollup/internal/controller/watcher/batch_proposer.go:291-294
Timestamp: 2024-10-20T16:13:20.397Z
Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
🧬 Code Graph Analysis (4)
coordinator/internal/logic/provertask/chunk_prover_task.go (3)
common/types/message/message.go (1)
ProofTypeChunk
(33-33)coordinator/internal/types/auth.go (3)
PublicKey
(16-16)ProverName
(18-18)ProverVersion
(20-20)coordinator/internal/logic/provertask/prover_task.go (1)
ErrCoordinatorInternalFailure
(26-26)
crates/libzkp_c/src/lib.rs (2)
crates/libzkp_c/src/utils.rs (1)
c_char_to_vec
(8-11)crates/libzkp/src/lib.rs (1)
verify_proof
(131-136)
crates/libzkp/src/tasks.rs (1)
crates/libzkp/src/utils.rs (1)
panic_catch
(43-53)
crates/libzkp/src/lib.rs (2)
crates/libzkp/src/utils.rs (1)
panic_catch
(43-53)crates/libzkp/src/tasks.rs (3)
gen_universal_chunk_task
(39-55)gen_universal_batch_task
(58-74)gen_universal_bundle_task
(77-93)
🪛 checkmake (0.2.2)
coordinator/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
crates/gpu_override/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 Shellcheck (0.10.0)
zkvm-prover/release-verifier-stuff.sh
[warning] 4-4: Quote this to prevent word splitting.
(SC2046)
⏰ 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). (3)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (27)
crates/libzkp/Cargo.toml (1)
12-12
: LGTM! Feature enablement aligns with version upgrade.The addition of
scroll-compress-ratio
andscroll
features tosbv-primitives
is appropriate for the feynman 0.5.0rc1 upgrade and follows proper Cargo.toml feature specification syntax.crates/prover-bin/src/prover.rs (1)
206-208
: Excellent safety check for verifier asset integrity.This validation prevents dumping potentially stale or incorrect verification keys by ensuring the VK cache is clean before proceeding. This defensive approach aligns well with the enhanced verifier asset management workflow.
crates/gpu_override/.cargo/config.toml (2)
16-17
: Good move to stable branch references.Switching from the sync branch to
main
for stark backend dependencies indicates a move toward more stable dependency management.
20-45
: Excellent transition to stable tagged releases.Replacing commit hash references with the stable tag
v0.2.1
for all Plonky3 dependencies improves build reproducibility and dependency management. This indicates these dependencies have reached a stable release state.zkvm-prover/download-release.sh (1)
6-6
: Correct version bump for feynman 0.5.0rc1.The version update from "0.5.0rc0" to "0.5.0rc1" aligns perfectly with the PR objective of upgrading to feynman 0.5.0rc1.
coordinator/Makefile (1)
1-1
: Correct addition of libzkp to phony targets.Adding
libzkp
to the.PHONY
declaration ensures the make target is always executed regardless of file existence, which is appropriate for build targets.crates/gpu_override/print_plonky3gpu_version.sh (1)
3-3
: Relative path invocation validated
- Confirmed that
crates/gpu_override/.cargo/config.toml
exists.- The
Makefile
incrates/gpu_override
invokes the script with./print_plonky3gpu_version.sh
, ensuring it runs from the correct directory.No further action required.
crates/l2geth/src/rpc_client.rs (1)
111-111
: LGTM! Enhanced error messaging for better debugging.The change from generic "Block not found" to "Block {block_hash} not found" improves error specificity by including the actual block hash that wasn't found. This enhancement aligns well with the PR's focus on improving debugging capabilities.
Cargo.toml (3)
20-22
: LGTM! Migration to stable tag for zkvm-prover dependencies.The change from specific git revisions to the stable tag
v0.5.0rc1
is a good practice that aligns with the PR objectives. Using tags provides better stability and version tracking compared to commit hashes.
24-25
: Compatibility check for openvm-1.3 branchConfirmed that the
chore/openvm-1.3
branch exists at commitc7575f1
and that allsbv-primitives
/sbv-utils
dependencies have been updated accordingly in:
crates/libzkp/Cargo.toml
crates/l2geth/Cargo.toml
crates/gpu_override/Cargo.lock
Please run a full project build and execute the zkvm-prover v0.5.0rc1 test suite to ensure there are no breaking changes introduced by this branch.
49-60
: No direct revm crate references found – please verify compatibilityI ran a repo-wide search for any direct use of the updated revm crates (
revm::
,use revm
,use revm_
) and found no occurrences. While this suggests the branch bump fromfeat/reth-v74
tofeat/reth-v78
may not impact existing code, please still:
- Run
cargo check
andcargo test
to ensure there are no build or test failures.- Confirm any indirect or dynamic revm usage (e.g., via trait objects or plugins) continues to work as expected.
crates/libzkp_c/src/lib.rs (1)
12-20
: LGTM! Well-implemented debug flag initialization.The
enable_dump()
function is well-designed with proper use ofOnceLock
for thread-safe initialization and appropriate fallback betweenZKVM_DEBUG
andZKVM_DEBUG_PROOF
environment variables.coordinator/internal/logic/provertask/chunk_prover_task.go (2)
84-88
: LGTM! Excellent validation to prevent conflicting task assignments.The new validation logic properly checks if a prover already has an assigned task of a different type, preventing conflicting assignments. This is a good defensive programming practice that will help maintain system integrity.
99-106
: LGTM! Useful feature for debugging and explicit task handling.The new logic to handle explicit
TaskID
requests is a valuable addition that aligns with the PR's debugging enhancements. The error handling appropriately distinguishes between retrieval failures and missing tasks.crates/libzkp/src/tasks.rs (2)
12-15
: Good refactor to centralize panic handling.The addition of the
panic_catch
utility import and its usage in thecheck_aggregation_proofs
function standardizes panic handling across the codebase. This improves maintainability and provides consistent error message formatting.
26-33
: Excellent use of the panic_catch utility.The refactor from direct
std::panic::catch_unwind
usage to thepanic_catch
utility function is a good improvement. The utility handles different panic payload types gracefully and provides consistent error formatting, while the validation logic remains intact.crates/gpu_override/Makefile (1)
16-17
: Good use of --locked flag for reproducible builds.The
build
target correctly uses the--locked
flag to prevent lockfile modifications, ensuring reproducible builds. The environment variable setup is also appropriate.crates/prover-bin/src/main.rs (1)
48-53
: Good structure for task set deserialization.The
HandleSet
struct provides a clean way to deserialize task sets from JSON, with appropriate field names for the different proof types.coordinator/internal/logic/provertask/bundle_prover_task.go (2)
87-89
: LGTM! Good defensive programming.The validation ensures that a prover with an already assigned task can only be reassigned tasks of the same type (bundle proof). This prevents task type conflicts and maintains consistency.
249-257
: Confirm initial batch handling forprevStateRoot
The current check (if batches[0].Index > 1
) only setsprevStateRoot
when the batch index is 2 or higher, leaving it as the zero hash for indices 0 and 1. Given that elsewhere in the codebase:
batchOrm.GetBatchByIndex(ctx, i-1)
is invoked for anyIndex > 0
(seerollup/internal/orm/batch.go
)- Index
0
is treated as the genesis batch in relayer logic (seel2_relayer.go
)Please verify that:
- An index of
1
(the first real batch after genesis) should indeed use a zero hash rather than fetching the genesis state root viaParentBatchHash
.- If instead every non-genesis batch (i.e.
Index > 0
) should retrieve its parent state root, change the condition toif batches[0].Index > 0
.- If leaving it as zero is intentional, add a code comment explaining that batches with
Index ≤ 1
default to the empty root.· File: coordinator/internal/logic/provertask/bundle_prover_task.go
· Lines: 249–257coordinator/internal/logic/provertask/batch_prover_task.go (1)
89-91
: Consistent implementation with bundle prover task.The changes maintain consistency with the bundle prover task implementation, ensuring uniform behavior across different prover task types. This is good for maintainability.
Also applies to: 102-109
crates/libzkp/src/lib.rs (3)
33-33
: Good parameter naming improvement.Renaming to
fork_name_str
clearly indicates this is a string parameter, improving code readability.
51-62
: Robust fork name validation and error handling.The implementation correctly:
- Normalizes the fork name to lowercase for consistent comparison
- Validates that the task's fork name matches the expected value
- Wraps the task generation in panic catching for better error handling
131-136
: Clean removal of debug logic.The simplified
verify_proof
function removes side effects (file dumping), making it more predictable and suitable for production use.zkvm-prover/Makefile (1)
40-41
: New GPU build integration looks good.The integration with the gpu_override build system properly passes required environment variables.
crates/libzkp/src/tasks/batch.rs (2)
129-143
: Well-structured fork-specific envelope handling.The match statement correctly handles different envelope types based on the fork name, with appropriate error handling for unexpected forks.
169-173
: Consistent fork-based header conversion.The reference header conversion properly maps each fork to its corresponding header version, maintaining consistency with the envelope handling logic.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1699 +/- ##
===========================================
- Coverage 39.68% 39.61% -0.08%
===========================================
Files 237 237
Lines 18937 18975 +38
===========================================
+ Hits 7516 7517 +1
- Misses 10682 10717 +35
- Partials 739 741 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
LGTM
This PR squash commits from #1694 with all updates towards latest feynman zkvm-prover, include:
0.5.0rc1
task_id
argument inget_task
API, enable prover handle specified task for debugging purposeSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores