Refactor/zkvm 3 #1684
Refactor/zkvm 3 #1684
Conversation
|
""" WalkthroughThis update introduces significant refactoring and feature additions across the prover, verifier, and coordinator components. Key changes include enhanced verification key (VK) management with per-proof-type and per-fork caching, new CLI commands for proof verification and VK dumping, revised prover task assignment logic to handle already-assigned tasks, and improved configuration and build processes. Rust and Go interfaces are aligned for universal task handling, and new configuration files, scripts, and ignore patterns are added. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Tool
participant Config as Config Loader
participant Verifier as Verifier
participant ProofFile as Proof File
participant VKStore as VK Store
CLI->>Config: Load config (Before hook)
CLI->>ProofFile: Read proof file
CLI->>Verifier: Initialize with config
Verifier->>VKStore: Retrieve VK for fork/type
CLI->>Verifier: Verify proof with VK
Verifier-->>CLI: Verification result
CLI->>CLI: Print result
sequenceDiagram
participant Prover as Prover
participant HandlerMap as Handler Map
participant VKCache as VK Cache
participant ProofReq as Proof Request
Prover->>HandlerMap: Get or init handler for fork
HandlerMap->>VKCache: Get VK for proof type
ProofReq->>HandlerMap: Request proof generation
HandlerMap->>VKCache: Compare VK in request vs expected
VKCache-->>HandlerMap: OK/Error
HandlerMap-->>ProofReq: Proof result or error
Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
enalbe block height check inside coordinator
noel2004
force-pushed
the
refactor/zkvm_3
branch
from
748b2e8 to
5417c9c
Compare
|
reviewed till 6c76a9b . don't force push later. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1684 +/- ##
===========================================
- Coverage 40.09% 39.88% -0.22%
===========================================
Files 234 236 +2
Lines 18618 18829 +211
===========================================
+ Hits 7465 7510 +45
- Misses 10425 10577 +152
- Partials 728 742 +14
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
coordinator/internal/config/config.go (1)
58-58: Fix typo in comment.The comment contains a typo that should be corrected for clarity.
-// AssetConfig contain assets configurated for each fork, the defaul vkfile name is "OpenVmVk.json". +// AssetConfig contains assets configured for each fork, the default vkfile name is "OpenVmVk.json".coordinator/internal/logic/verifier/verifier.go (2)
120-131: Remove commented code or document retention reason.The commented-out
ReadVKmethod should either be removed if no longer needed or documented if kept for future reference.-// func (v *Verifier) ReadVK(filePat string) (string, error) { - -// f, err := os.Open(filepath.Clean(filePat)) -// if err != nil { -// return "", err -// } -// byt, err := io.ReadAll(f) -// if err != nil { -// return "", err -// } -// return base64.StdEncoding.EncodeToString(byt), nil -// }
159-174: Consider refactoring repetitive base64 decoding logic.The base64 decoding and caching logic is repeated three times. Consider extracting it into a helper function for better maintainability.
+func (v *Verifier) cacheVk(encoded string, forkName string, vkMap map[string][]byte) error { + decodedBytes, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + return err + } + vkMap[forkName] = decodedBytes + return nil +} func (v *Verifier) loadOpenVMVks(cfg config.AssetConfig) error { // ... existing code ... - decodedBytes, err := base64.StdEncoding.DecodeString(dump.Chunk) - if err != nil { - return err - } - v.ChunkVk[cfg.ForkName] = decodedBytes - decodedBytes, err = base64.StdEncoding.DecodeString(dump.Batch) - if err != nil { - return err - } - v.BatchVk[cfg.ForkName] = decodedBytes - decodedBytes, err = base64.StdEncoding.DecodeString(dump.Bundle) - if err != nil { - return err - } - v.BundleVk[cfg.ForkName] = decodedBytes + if err := v.cacheVk(dump.Chunk, cfg.ForkName, v.ChunkVk); err != nil { + return err + } + if err := v.cacheVk(dump.Batch, cfg.ForkName, v.BatchVk); err != nil { + return err + } + if err := v.cacheVk(dump.Bundle, cfg.ForkName, v.BundleVk); err != nil { + return err + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
coordinator/cmd/api/app/mock_app.go(1 hunks)coordinator/cmd/tool/verify.go(1 hunks)coordinator/conf/config.json(2 hunks)coordinator/internal/config/config.go(1 hunks)coordinator/internal/logic/auth/login.go(2 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(4 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(4 hunks)coordinator/internal/logic/provertask/chunk_prover_task.go(4 hunks)coordinator/internal/logic/verifier/verifier.go(4 hunks)coordinator/test/api_test.go(1 hunks)crates/libzkp/src/verifier.rs(2 hunks)zkvm-prover/config.json.template(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- zkvm-prover/config.json.template
- coordinator/test/api_test.go
- coordinator/conf/config.json
- coordinator/cmd/tool/verify.go
- coordinator/internal/logic/provertask/chunk_prover_task.go
- coordinator/internal/logic/provertask/batch_prover_task.go
- coordinator/internal/logic/provertask/bundle_prover_task.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
coordinator/internal/logic/auth/login.go (3)
coordinator/internal/config/config.go (1)
ProverManager(13-29)common/version/prover_version.go (1)
CheckScrollRepoVersion(37-55)coordinator/internal/types/auth.go (2)
Message(42-49)ProverVersion(20-20)
coordinator/cmd/api/app/mock_app.go (1)
coordinator/internal/config/config.go (1)
AssetConfig(59-63)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (10)
coordinator/internal/logic/auth/login.go (2)
34-38: LGTM! Correctly implements multi-verifier fork aggregation.The changes properly collect fork names from all configured verifiers instead of relying on a single high version circuit, which aligns with the new multi-verifier architecture.
61-62: LGTM! Updated to use new configuration structure.The version check correctly uses the new
MinProverVersionfield from the verifier configuration, maintaining backward compatibility while supporting the new multi-verifier setup.coordinator/cmd/api/app/mock_app.go (1)
93-98: LGTM! Mock configuration correctly updated for new structure.The mock configuration properly reflects the new verifier config schema with
MinProverVersionas a top-level field andVerifiersas a slice ofAssetConfigstructs, maintaining the same test values in the new format.coordinator/internal/config/config.go (2)
58-63: LGTM! Well-designed configuration structure for multi-verifier support.The
AssetConfigstruct properly supports per-fork asset configuration with an optional verification key file, enabling flexible verifier setups.
67-68: LGTM! Correctly restructured for multi-verifier architecture.The new
VerifierConfigstructure with explicitMinProverVersionand a slice ofVerifiersproperly supports multiple verifier configurations while maintaining clear version requirements.crates/libzkp/src/verifier.rs (3)
55-56: LGTM! Excellent thread-safe multi-verifier implementation.The use of
OnceLock<HashMap<>>withArc<Mutex<>>provides proper thread safety for concurrent verifier access. The initialization logic correctly handles multiple circuits with good error handling and prevents duplicate fork names.Also applies to: 58-74
76-93: LGTM! Well-structured verifier retrieval with comprehensive error handling.The
get_verifierfunction properly handles both uninitialized state and missing fork names, providing clear error messages for debugging.
63-63: Verify case sensitivity requirements for fork names.The code converts fork names to lowercase when storing in the HashMap. Ensure this doesn't cause issues if other parts of the system expect case-sensitive fork name matching.
#!/bin/bash # Description: Check if fork names are used case-sensitively elsewhere in the codebase # Expected: Find usages of fork names to verify case sensitivity requirements echo "Searching for fork name usage patterns..." rg -A 2 -B 2 "fork_name|ForkName" --type go echo -e "\n--- Rust fork name usage ---" rg -A 2 -B 2 "fork_name" --type rustcoordinator/internal/logic/verifier/verifier.go (2)
40-51: LGTM! Correctly implements multi-circuit configuration mapping.The
rustVerifierConfigproperly maps the new configuration structure with multiple circuits, maintaining clean separation between Go config and Rust FFI types.
72-75: LGTM! Excellent addition of VK caching for performance.The new per-fork verification key maps (
ChunkVk,BatchVk,BundleVk) provide efficient in-memory caching, and the initialization loop correctly processes all configured verifiers.Also applies to: 77-81
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
coordinator/internal/controller/api/controller.go(2 hunks)coordinator/internal/controller/api/get_task.go(4 hunks)coordinator/internal/logic/libzkp/lib.go(6 hunks)coordinator/internal/logic/libzkp/libzkp.h(1 hunks)coordinator/internal/logic/libzkp/mock_universal_task.go(1 hunks)coordinator/internal/logic/libzkp/universal_task.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(5 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(5 hunks)coordinator/internal/logic/provertask/chunk_prover_task.go(5 hunks)coordinator/internal/logic/provertask/prover_task.go(3 hunks)crates/libzkp/src/lib.rs(5 hunks)crates/libzkp/src/proofs.rs(2 hunks)crates/libzkp/src/tasks.rs(1 hunks)crates/libzkp/src/tasks/batch.rs(4 hunks)crates/libzkp/src/tasks/bundle.rs(2 hunks)crates/libzkp/src/verifier.rs(3 hunks)crates/libzkp/src/verifier/euclidv2.rs(1 hunks)crates/libzkp_c/src/lib.rs(2 hunks)crates/prover-bin/src/zk_circuits_handler.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- coordinator/internal/controller/api/controller.go
- coordinator/internal/logic/libzkp/mock_universal_task.go
- crates/prover-bin/src/zk_circuits_handler.rs
- crates/libzkp/src/tasks/batch.rs
- crates/libzkp/src/tasks/bundle.rs
- coordinator/internal/logic/libzkp/universal_task.go
- coordinator/internal/logic/provertask/prover_task.go
- crates/libzkp_c/src/lib.rs
- crates/libzkp/src/tasks.rs
- coordinator/internal/logic/libzkp/lib.go
- crates/libzkp/src/lib.rs
- coordinator/internal/logic/provertask/batch_prover_task.go
- coordinator/internal/logic/provertask/chunk_prover_task.go
- coordinator/internal/controller/api/get_task.go
- coordinator/internal/logic/provertask/bundle_prover_task.go
- crates/libzkp/src/verifier/euclidv2.rs
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (7)
crates/libzkp/src/verifier.rs (5)
7-11: LGTM: Thread-safe concurrency primitives properly imported.The imports correctly include
OnceLock,Arc,Mutex, andHashMapfor implementing thread-safe multi-verifier management. The choice ofOnceLockfor one-time initialization andArc<Mutex<>>for shared mutable access is appropriate for this use case.
38-38: Good optimization: Changed from ownedVec<u8>to borrowed&[u8].This change improves memory efficiency by avoiding unnecessary allocations and copies when verifying proofs. The verifier doesn't need to own the proof data, just read it.
55-56: LGTM: Appropriate global state management for multi-verifier system.Using
OnceLock<HashMap<...>>is the correct pattern for thread-safe, one-time initialization of global state. The type aliasVerifierTypeimproves readability.
58-76: Good implementation with proper validation.The initialization logic correctly:
- Iterates through all circuit configurations
- Canonicalizes fork names to lowercase for consistent lookup
- Validates against duplicate fork registrations
- Provides clear error messages and logging
The assertion for duplicate prevention is appropriate since this represents a configuration error that should fail fast.
78-95: Excellent error handling and helpful debugging information.The
get_verifierfunction provides comprehensive error handling with informative messages:
- Lists available keys when a fork is not found
- Distinguishes between uninitialized state and missing keys
- Returns proper
Resulttypes for error propagationcoordinator/internal/logic/libzkp/libzkp.h (1)
35-41: All callers updated with new parametersVerified that every invocation of
gen_universal_task—in the Rust crate (crates/libzkp/src/lib.rs), the C-ABI wrapper (crates/libzkp_c/src/lib.rs), and the Go binding (coordinator/internal/logic/libzkp/universal_task.go)—now includes bothexpected_vkandexpected_vk_len. No outdated 3-argument calls remain.crates/libzkp/src/proofs.rs (1)
182-182: Function renamed with improved error handling approach.Good refactoring! Changing from
sanity_checktopi_hash_checkwith a boolean return type allows callers to handle mismatches gracefully instead of panicking. This aligns well with the broader verification workflow improvements.
|
test passed with chunk from sepolia testnet |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/prover-bin/src/zk_circuits_handler/euclidV2.rs (2)
60-71: Address the unreachable! macro usage.The
get_vk_and_cachemethod implements proper lazy VK initialization, but still usesunreachable!for unsupported proof types.Replace
unreachable!with proper error handling:- _ => unreachable!("Unsupported proof type {:?}", task_type), + _ => return String::new(), // Or handle this case appropriately in the caller
76-80: Avoid using try_lock().expect() to prevent potential panics.The
try_lock().expect()pattern could still panic if the lock is already held, similar to the previoustry_lock().unwrap()issue.Consider using proper error handling:
- fn get_vk(&self, task_type: ProofType) -> String { - self.try_lock() - .expect("get vk is on called before other entry is used") - .get_vk_and_cache(task_type) + fn get_vk(&self, task_type: ProofType) -> String { + match self.try_lock() { + Ok(handler) => handler.get_vk_and_cache(task_type), + Err(_) => { + tracing::error!("Failed to acquire lock for get_vk"); + String::new() + } + }
🧹 Nitpick comments (1)
coordinator/internal/logic/verifier/verifier_test.go (1)
32-36: Consider setting a valid MinProverVersion and testing multi-verifier scenarios.The configuration structure correctly reflects the new multi-verifier design. However, consider:
- The empty
MinProverVersionmight not adequately test version validation logic- Using only one verifier in the slice doesn't fully exercise the new multi-verifier capability
Consider this enhancement to better test the new functionality:
cfg := &config.VerifierConfig{ - MinProverVersion: "", + MinProverVersion: "v4.4.45", Verifiers: []config.AssetConfig{{ AssetsPath: *assetsPathHi, ForkName: "euclidV2", }}, }Additionally, consider adding a test case that exercises multiple verifiers to ensure the new architecture works as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
coordinator/cmd/tool/verify.go(1 hunks)coordinator/conf/config.json(2 hunks)coordinator/internal/config/config_test.go(1 hunks)coordinator/internal/logic/verifier/verifier_test.go(1 hunks)crates/libzkp/src/proofs.rs(2 hunks)crates/prover-bin/src/zk_circuits_handler/euclidV2.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- coordinator/conf/config.json
- crates/libzkp/src/proofs.rs
- coordinator/cmd/tool/verify.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
coordinator/internal/logic/verifier/verifier_test.go (1)
coordinator/internal/config/config.go (1)
AssetConfig(59-63)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (6)
coordinator/internal/config/config_test.go (1)
23-27: Configuration structure correctly reflects the new multi-verifier architecture.The JSON configuration properly demonstrates the structural changes:
min_prover_versionmoved to top level ✓verifiersarray replaces single circuit config ✓- Asset configuration format matches
AssetConfigstruct ✓Consider adding a test case with multiple verifiers in the array to fully validate the new multi-verifier functionality:
"verifiers": [{ "assets_path": "assets", "fork_name": "euclidV2" -}] +}, { + "assets_path": "assets_v2", + "fork_name": "feynmanV1" +}]crates/prover-bin/src/zk_circuits_handler/euclidV2.rs (5)
1-5: LGTM: Appropriate imports for VK caching functionality.The new imports support the VK caching mechanism with HashMap for storage and OnceLock for thread-safe lazy initialization.
20-20: LGTM: Well-designed VK caching field.The
cached_vksfield usesOnceLock<String>for thread-safe lazy initialization of base64-encoded verification keys, which is appropriate for this caching pattern.
26-58: LGTM: Constructor properly initializes VK cache.The constructor correctly:
- Accepts a
CircuitConfigreference for better configuration access- Initializes the VK cache using either pre-existing config values or empty
OnceLockinstances- Uses a clean closure pattern for cache initialization
The approach allows for flexible VK initialization while maintaining thread safety.
82-94: LGTM: Improved VK validation with proper async locking.The VK validation logic is well-implemented:
- Uses proper async locking with
self.lock().await- Compares base64-encoded VKs for consistency
- Provides clear error messages with context
- Prevents proof generation with mismatched VKs
This significantly improves security by ensuring proof consistency.
95-115: LGTM: Improved error handling for unsupported proof types.The proof generation logic correctly:
- Uses the locked handler instance consistently
- Replaces panics with proper
Resulterror returns for unsupported proof types- Maintains the same proof generation flow
The TODO comment for checking expected public inputs is noted and appropriate for future enhancement.
The last refactoring phase before Feynman
Some breaking changes on deployment has been induced:
circuits, with separated workspace path for each. (There is no change for the layout inside workspace), for example:{ "circuits": { "euclidV2": { "hard_fork_name": "euclidV2", "workspace_path": "/workspace/euclid" }, "feynmanV1": { "hard_fork_name": "feynmanV1", "workspace_path": "/workspace/feynman1" }, "feynmanV2": { "hard_fork_name": "feynmanV2", "workspace_path": "/workspace/feynman2" } } }Currently only one set of stuffs for verifier would be loaded by coordinator, and it is expected to be compatible with all proofs generated from different circuits loaded by prover. The vk is no longer read from verifier stuff directly. Instead, we have to specify them under the asset path
The format of dumped vk file is used for vk specify, there can be multiple of them with names being specified under
verifierfield in the config file. Currently there can be only one vk set (for chunk/batch/bundle) for each hardfork phaseA utility has been added to prover to generate the corresponding vk file, for example, we can generate the vk file for euclidv2 phase being set by the config file above by following command:
The generated
euclid.jsonfile would be like:{ "chunk_vk": "bq14DegWJXatu142361dX/qzxwZxk9lEMXh8AceWwDhmGOolp5kYRbvF/VcWcO5HN5ujGs6S00W8pZcCoNQRLQ==", "batch_vk": "ShuQOk3N3x84Qe1iiN/vAgnBegBM/ys6Ib5yWXRgLUTyvi9EaNGsSnDnaCurscYEF+IcdjPUtVtY9EcD7IKwWg==", "bundle_vk": "AhYAAAAABAAAAD2PumVP6pqldS0PKWW8Q4IvnE/rvtm5/2fXvG196sYhwoWi5H4McKnjcoiwkxtSOLUNBvs1q0ogwWe4x+9lUyENu8s47Xabxy0EViND1dzsu5HicdAWl0xG5C+VpO2faJdK4nGwtD4WHtbdqWY72nSY5aKSDxAYO85vLy+9cJZlCigGKHeuwjPQrj3o8XjzGDHYqCyOjMJggo2ZN+X7nh5uSWyZPNuozaLpcGG7WwjLVKxXlfrRFeKO86ZJ3m61Lcw8T9j5UXpHz3K/2yuVSXK6OvMG4/058TXG09qKgXYP" }{ "prover_manager":{ "verifier": { "min_prover_version": "v4.5.25", "verifiers": [{ "assets_path": "assets", "fork_name": "euclidV2" }] } }}The vk file (with default name as
openVmVk.json) is put underassets_path, in additional with other stuffs being needed by a verifierSummary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores
.gitignorefiles and removed obsolete configuration files.Documentation