feat: openvm euclid v2 #1613
feat: openvm euclid v2 #1613
Conversation
…submit multiple batches in a single transaction
…ntextID logic to support multiple batches by concatenating batch hashes
Co-authored-by: noelwei <fan@scroll.io>
This reverts commit 7d5b77a.
![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
🔭 Outside diff range comments (1)
common/libzkp/impl/src/verifier.rs (1)
62-70:⚠️ Potential issueCritical bug: second verifier is erroneously assigned to
VERIFIER_LOW.
This overrides the first assignment and never setsVERIFIER_HIGH. UseVERIFIER_HIGHfor the high version circuit instead.- VERIFIER_LOW + VERIFIER_HIGH
🧹 Nitpick comments (4)
common/libzkp/impl/src/verifier.rs (1)
52-60: Prefer safer initialization overunwrap_unchecked().
Usingunwrap_unchecked()may cause undefined behavior ifinitis invoked multiple times or concurrently. Consider handling potential errors gracefully with a guard or conditional check.common/libzkp/impl/src/verifier/euclidv2.rs (3)
10-30: Consider returning aResult<Self>innew()for better error propagation.
Currently, the.expect(...)calls will panic if setup fails. Handling errors at this layer helps avoid unexpected panics in production.
33-53: Improve JSON parsing error handling.
Relying on.unwrap()withinpanic_catchmight obscure proper error messages or lead to abrupt panics. Using.map_err(...)would offer more robust feedback in case of malformed proofs.
55-65: Consider returning aResultfromdump_vk.
Panicking on file I/O errors makes the application less resilient. Exposing errors to callers can help recover gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
common/libzkp/impl/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
common/libzkp/impl/Cargo.toml(1 hunks)common/libzkp/impl/src/verifier.rs(2 hunks)common/libzkp/impl/src/verifier/euclidv2.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: clippy
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (4)
common/libzkp/impl/Cargo.toml (1)
19-20: New EuclidV2 Dependencies AddedThe new dependency entries for
euclidv2_proverandeuclidv2_verifierare correctly configured with the updated tag"v0.1.1-rc.2". Please verify that keeping the legacy dependencies (euclid_proverandeuclid_verifieron"v0.1.0-rc.6") is intentional. If the goal is to fully transition to EuclidV2, consider removing or deprecating the legacy entries to prevent any ambiguity in dependency resolution.common/libzkp/impl/src/verifier.rs (2)
4-4: No issues found with the new module import.
8-8: Good use of the newly introducedEuclidV2Verifier.
No further concerns here.common/libzkp/impl/src/verifier/euclidv2.rs (1)
1-8: Import statements look correct and well-organized.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
zkvm-prover/src/zk_circuits_handler/euclidV2.rs (2)
75-82: 🛠️ Refactor suggestionHandle potential lock unavailability more gracefully.
Using
try_lock().unwrap()will panic if the lock is already held. In asynchronous environments, unexpected panics can break the system. Consider usinglock().awaitor returning a meaningful error instead.async fn get_vk(&self, task_type: ProofType) -> Option<Vec<u8>> { - Some(match task_type { - ProofType::Chunk => self.try_lock().unwrap().chunk_prover.get_app_vk(), - ProofType::Batch => self.try_lock().unwrap().batch_prover.get_app_vk(), - ProofType::Bundle => self.try_lock().unwrap().bundle_prover.get_app_vk(), - _ => unreachable!("Unsupported proof type"), - }) + let handler = match self.lock().await { + Ok(handler) => handler, + Err(e) => { + log::error!("Failed to acquire lock: {}", e); + return None; + } + }; + + Some(match task_type { + ProofType::Chunk => handler.chunk_prover.get_app_vk(), + ProofType::Batch => handler.batch_prover.get_app_vk(), + ProofType::Bundle => handler.bundle_prover.get_app_vk(), + _ => { + log::error!("Unsupported proof type: {:?}", task_type); + return None; + } + }) }
84-110: 🛠️ Refactor suggestionReconsider
.try_lock().unwrap()usage inget_proof_data.Similar to
get_vk, this pattern will panic if the lock is unavailable. Validate if a panic is intended. Otherwise, adopt a safer async lock strategy or return an error if concurrency conflicts arise.async fn get_proof_data(&self, prove_request: ProveRequest) -> Result<String> { + let handler = self.lock().await.map_err(|e| anyhow!("Failed to acquire lock: {}", e))?; + match prove_request.proof_type { ProofType::Chunk => { let task: ChunkProvingTask = serde_json::from_str(&prove_request.input)?; - let proof = self.try_lock().unwrap().chunk_prover.gen_proof(&task)?; + let proof = handler.chunk_prover.gen_proof(&task)?; Ok(serde_json::to_string(&proof)?) } ProofType::Batch => { let task: BatchProvingTask = serde_json::from_str(&prove_request.input)?; - let proof = self.try_lock().unwrap().batch_prover.gen_proof(&task)?; + let proof = handler.batch_prover.gen_proof(&task)?; Ok(serde_json::to_string(&proof)?) } ProofType::Bundle => { let batch_proofs: BundleProvingTask = serde_json::from_str(&prove_request.input)?; - let proof = self - .try_lock() - .unwrap() - .bundle_prover - .gen_proof_evm(&batch_proofs)?; + let proof = handler.bundle_prover.gen_proof_evm(&batch_proofs)?; Ok(serde_json::to_string(&proof)?) } _ => Err(anyhow!("Unsupported proof type")), } }
🧹 Nitpick comments (3)
zkvm-prover/src/zk_circuits_handler/euclidV2.rs (3)
32-34: Extract repeated configuration into constants.The segment length value
(1 << 22) - 100is repeated multiple times. Extract this to a constant to improve maintainability and reduce the risk of inconsistencies.+ const DEFAULT_SEGMENT_LENGTH: usize = (1 << 22) - 100; + pub fn new(workspace_path: &str) -> Self { // ... existing code ... let chunk_prover = ChunkProver::setup( chunk_exe, chunk_app_config, Some(cache_dir.clone()), ProverConfig { - segment_len: Some((1 << 22) - 100), + segment_len: Some(Self::DEFAULT_SEGMENT_LENGTH), }, )Also applies to: 45-47, 58-60
87-91: Refactor repeated JSON serialization/deserialization pattern.The same pattern of deserializing input, generating proof, and serializing output is repeated three times with minor variations. This could be refactored to reduce duplication.
async fn get_proof_data(&self, prove_request: ProveRequest) -> Result<String> { let handler = self.lock().await.map_err(|e| anyhow!("Failed to acquire lock: {}", e))?; // Generic function to handle deserialization, proof generation, and serialization async fn process_and_serialize<T, P, F>(input: &str, generator: F) -> Result<String> where T: serde::de::DeserializeOwned, P: serde::Serialize, F: FnOnce(T) -> Result<P>, { let task = serde_json::from_str(input)?; let proof = generator(task)?; Ok(serde_json::to_string(&proof)?) } match prove_request.proof_type { ProofType::Chunk => { process_and_serialize(&prove_request.input, |task: ChunkProvingTask| { handler.chunk_prover.gen_proof(&task) }).await } ProofType::Batch => { // Similar implementation for batch } ProofType::Bundle => { // Similar implementation for bundle } _ => Err(anyhow!("Unsupported proof type")), } }Also applies to: 93-97, 99-107
80-81: Use consistent error handling for unsupported proof types.In
get_vk()you useunreachable!()for unsupported proof types, while inget_proof_data()you return an error. Choose one consistent approach across all methods.async fn get_vk(&self, task_type: ProofType) -> Option<Vec<u8>> { + let handler = self.lock().await.map_err(|e| { + log::error!("Failed to acquire lock: {}", e); + return None; + })?; + Some(match task_type { - ProofType::Chunk => self.try_lock().unwrap().chunk_prover.get_app_vk(), - ProofType::Batch => self.try_lock().unwrap().batch_prover.get_app_vk(), - ProofType::Bundle => self.try_lock().unwrap().bundle_prover.get_app_vk(), - _ => unreachable!("Unsupported proof type"), + ProofType::Chunk => handler.chunk_prover.get_app_vk(), + ProofType::Batch => handler.batch_prover.get_app_vk(), + ProofType::Bundle => handler.bundle_prover.get_app_vk(), + _ => { + log::error!("Unsupported proof type: {:?}", task_type); + return None; + } }) }Also applies to: 108-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
zkvm-prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
zkvm-prover/src/prover.rs(3 hunks)zkvm-prover/src/zk_circuits_handler/euclidV2.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zkvm-prover/src/prover.rs
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
zkvm-prover/src/zk_circuits_handler/euclidV2.rs (5)
32-32: Consider making segment length configurable or document its purpose.The segment length is hard-coded as
(1 << 22) - 100in multiple places. Consider making this a configurable parameter or adding a comment explaining why this specific value is chosen.+ // The segment length 2^22 - 100 is chosen to optimize memory usage while maintaining performance. + // This value could be made configurable in the future if needed. + const DEFAULT_SEGMENT_LEN: usize = (1 << 22) - 100; pub fn new(workspace_path: &str) -> Self { // ... let chunk_prover = ChunkProver::setup( chunk_exe, chunk_app_config, Some(cache_dir.clone()), ProverConfig { - segment_len: Some((1 << 22) - 100), + segment_len: Some(Self::DEFAULT_SEGMENT_LEN), }, )Also applies to: 44-44, 56-56
76-76: Improve error handling for unsupported proof types.Using
unreachable!()assumes all possibleProofTypevalues are covered, which might not be true if new types are added. Consider a more graceful approach.- _ => unreachable!("Unsupported proof type"), + _ => return None, // Return None for unsupported types instead of panicking
12-16: Add documentation for theEuclidV2Handlerstruct.Adding documentation comments will improve code readability and maintainability, especially for new contributors.
+/// Handler for EuclidV2 zk circuits that manages the proving process for different proof types. +/// +/// This struct encapsulates three provers: +/// - `chunk_prover`: For generating chunk-level proofs +/// - `batch_prover`: For generating batch-level proofs +/// - `bundle_prover`: For generating bundle-level proofs pub struct EuclidV2Handler { chunk_prover: ChunkProver, batch_prover: BatchProver, bundle_prover: BundleProver, }
24-34: Create directory if it doesn't exist.The code assumes the cache directory already exists or is created by the
ChunkProver::setupmethod. Consider explicitly creating the directory to improve robustness.let cache_dir = workspace_path.join("cache"); +std::fs::create_dir_all(&cache_dir).map_err(|e| anyhow!("Failed to create cache directory: {}", e))?; let chunk_exe = workspace_path.join("chunk/app.vmexe");
1-11: Organize imports and add a module-level documentation comment.Adding a module-level documentation comment and organizing imports can improve readability.
+//! EuclidV2 implementation of the CircuitsHandler trait. +//! +//! This module provides functionality for setting up and using the EuclidV2 proving system. +//! It handles proving operations for chunk, batch, and bundle proofs. + use std::{path::Path, sync::Arc}; use super::CircuitsHandler; use anyhow::{anyhow, Result}; use async_trait::async_trait; use scroll_proving_sdk::prover::{proving_service::ProveRequest, ProofType}; use scroll_zkvm_prover_euclidv2::{ task::{batch::BatchProvingTask, bundle::BundleProvingTask, chunk::ChunkProvingTask}, BatchProver, BundleProver, ChunkProver, ProverConfig, }; use tokio::sync::Mutex;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
common/types/message/message.go(8 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(1 hunks)coordinator/internal/logic/provertask/chunk_prover_task.go(3 hunks)zkvm-prover/src/prover.rs(2 hunks)zkvm-prover/src/zk_circuits_handler/euclidV2.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- zkvm-prover/src/prover.rs
- coordinator/internal/logic/auth/login.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (21)
common/types/message/message.go (9)
12-14: Exported constants make hardforking logic more accessibleThe rename from lowercase
euclidForkto capitalizedEuclidForkexports these constants, making them accessible to external packages, which is appropriate for these critical system-wide values.
44-53: Good approach for backward compatibility with the new EuclidV2 forkRenaming the original
ChunkTaskDetailtoLegacyChunkTaskDetailand creating a newEuclidV2ChunkTaskDetailwith the additionalPrevMsgQueueHashfield is a clean approach to maintain backward compatibility while supporting the new fork features.
71-86: Enhanced ChunkInfo struct with additional fields for EuclidV2 forkThe
ChunkInfostruct has been extended with several new fields to support EuclidV2 functionality. This change is well-structured and maintains backward compatibility.
88-95: New BlockContextV2 struct for EuclidV2The
BlockContextV2struct adds necessary context for blocks in the EuclidV2 fork. The fields and types look appropriate for capturing the block metadata.
110-115: Support for multiple forks in NewChunkProof factory methodThe
NewChunkProoffactory method now handles bothEuclidForkandEuclidV2Forkidentically, creating anOpenVMChunkProof. This ensures consistent proof generation across forks.
144-149: Support for multiple forks in NewBatchProof factory methodSimilar to
NewChunkProof, theNewBatchProoffactory method now handles both forks consistently.
201-206: Support for multiple forks in NewBundleProof factory methodThe
NewBundleProoffactory method has been updated to handle both forks consistently as well, completing the pattern across all proof types.
280-290: Updated OpenVMBatchInfo with new message queue hash fieldsThe addition of
PostMsgQueueHashfield toOpenVMBatchInfoproperly aligns with the EuclidV2 fork requirements for tracking message queue state changes.
55-64:✅ Verification successful
Type change from []byte to hexutil.Big for KZG-related fields
The change from
[]bytetohexutil.Bigfor KZG-related fields provides better semantic meaning and type safety. Make sure all code using these fields has been updated to handle the new types.
🏁 Script executed:
#!/bin/bash # Verify all consumers of BatchTaskDetail properly handle the new hexutil.Big types rg -A 3 -B 3 "BatchTaskDetail" --type goLength of output: 2008
KZG Fields Type Change Verified
The type change for the KZG-related fields from
[]bytetohexutil.BiginBatchTaskDetailimproves semantic clarity and type safety. A search of the primary consumer incoordinator/internal/logic/provertask/batch_prover_task.goshows that the struct is instantiated without any explicit processing of these new fields, which suggests that they are currently managed using their default zero values with no downstream issues. Please ensure that any future usage (or conversion logic) for these KZG fields is updated accordingly if additional manipulation is required.coordinator/internal/logic/provertask/batch_prover_task.go (4)
216-216: Added PostMsgQueueHash to support EuclidV2 forkThe addition of
PostMsgQueueHashfield correctly handles the new message queue hash tracking requirement in the EuclidV2 fork.
219-223: Improved variable naming from haloProot to halo2ProofRenaming
haloProottohalo2Proofimproves code clarity and reduces potential confusion.
224-228: Support for OpenVMChunkProof in addition to Halo2ChunkProofThe addition of the OpenVMChunkProof case properly handles the new proof format for EuclidV2 fork, extracting the necessary fields from the proof metadata.
294-296: Updated type handling for KZG fieldsThe fields are now correctly handled as
hexutil.Bigtypes, aligning with the updatedBatchTaskDetailstruct definition. The comment aboutChallengeDigestclarifies the relationship between the challenge and its digest.coordinator/internal/logic/provertask/chunk_prover_task.go (4)
166-166: Added chunk parameter to formatProverTask methodThe method now takes a chunk parameter, allowing access to chunk-specific data needed for the EuclidV2 fork.
183-183: Updated method signature to include chunk parameterThe signature change properly reflects the addition of the chunk parameter needed for EuclidV2 functionality.
190-210: Fork-specific task detail handlingThis conditional logic elegantly handles both EuclidV2 and legacy forks, using the appropriate task detail struct based on the fork name. For EuclidV2, it includes the
PrevMsgQueueHashfield from the chunk data.
220-220: Enhanced debug loggingThe debug log provides useful context about the task, including ID, type, fork name, and data, which will assist in troubleshooting and monitoring.
zkvm-prover/src/zk_circuits_handler/euclidV2.rs (4)
18-18: Add safety documentation for the Send implementation.Using
unsafe impl Sendindicates that you're overriding the compiler's thread safety checks. Please add a comment explaining why this implementation is safe and why it's necessary.-unsafe impl Send for EuclidV2Handler {} +// This is safe because all internal provers (ChunkProver, BatchProver, BundleProver) implement Send +// and the struct contains no other shared state that would make it unsafe to transfer between threads. +unsafe impl Send for EuclidV2Handler {}
20-67: Improve error handling in thenewmethod.The current implementation uses
.expect()for error handling, which will cause the program to panic if setup fails. Consider propagating errors instead for better error handling.- pub fn new(workspace_path: &str) -> Self { + pub fn new(workspace_path: &str) -> Result<Self> { let workspace_path = Path::new(workspace_path); let cache_dir = workspace_path.join("cache"); let chunk_exe = workspace_path.join("chunk/app.vmexe"); let chunk_app_config = workspace_path.join("chunk/openvm.toml"); let chunk_prover = ChunkProver::setup( chunk_exe, chunk_app_config, Some(cache_dir.clone()), ProverConfig { segment_len: Some((1 << 22) - 100), }, ) - .expect("Failed to setup chunk prover"); + .map_err(|e| anyhow!("Failed to setup chunk prover: {}", e))?; // Similar changes for batch_prover and bundle_prover - Self { + Ok(Self { chunk_prover, batch_prover, bundle_prover, - } + }) }
71-78: Handle potential lock unavailability more gracefully.Using
try_lock().unwrap()will panic if the lock is already held. In asynchronous environments, unexpected panics can break the system. Consider usinglock().awaitor returning a meaningful error instead.async fn get_vk(&self, task_type: ProofType) -> Option<Vec<u8>> { - Some(match task_type { - ProofType::Chunk => self.try_lock().unwrap().chunk_prover.get_app_vk(), - ProofType::Batch => self.try_lock().unwrap().batch_prover.get_app_vk(), - ProofType::Bundle => self.try_lock().unwrap().bundle_prover.get_app_vk(), - _ => unreachable!("Unsupported proof type"), - }) + let handler = self.lock().await; + Some(match task_type { + ProofType::Chunk => handler.chunk_prover.get_app_vk(), + ProofType::Batch => handler.batch_prover.get_app_vk(), + ProofType::Bundle => handler.bundle_prover.get_app_vk(), + _ => return None, // Return None instead of panicking + }) }
80-106: Reconsider.try_lock().unwrap()usage inget_proof_data.Similar to
get_vk, this pattern will panic if the lock is unavailable. Consider adopting a safer async lock strategy or return an error if concurrency conflicts arise.async fn get_proof_data(&self, prove_request: ProveRequest) -> Result<String> { + let mut handler = self.lock().await; match prove_request.proof_type { ProofType::Chunk => { let task: ChunkProvingTask = serde_json::from_str(&prove_request.input)?; - let proof = self.try_lock().unwrap().chunk_prover.gen_proof(&task)?; + let proof = handler.chunk_prover.gen_proof(&task)?; Ok(serde_json::to_string(&proof)?) } // Similar changes for Batch and Bundle cases
colinlyguo
changed the title
Signed-off-by: noelwei <fan@scroll.io> Co-authored-by: colin <102356659+colinlyguo@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
common/types/message/message_test.go (1)
8-22: Enhance test using proper testing practices.The test uses
panicinstead of the standard Go testing functions liket.Fatalfort.Errorf. Additionally, it doesn't check for errors fromUnmarshalTextoperations and contains duplicate code.Consider refactoring as follows:
func TestBytes48(t *testing.T) { - ti := &Byte48{} - ti.UnmarshalText([]byte("0x1")) - if s, err := ti.MarshalText(); err == nil { - if len(s) != 98 { - panic(fmt.Sprintf("wrong str: %s", s)) - } - } - ti.UnmarshalText([]byte("0x0")) - if s, err := ti.MarshalText(); err == nil { - if len(s) != 98 { - panic(fmt.Sprintf("wrong str: %s", s)) - } - } + testCases := []struct { + name string + input string + }{ + {"Small Value", "0x1"}, + {"Zero Value", "0x0"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ti := &Byte48{} + err := ti.UnmarshalText([]byte(tc.input)) + if err != nil { + t.Fatalf("failed to unmarshal text: %v", err) + } + + s, err := ti.MarshalText() + if err != nil { + t.Fatalf("failed to marshal text: %v", err) + } + + if len(s) != 98 { + t.Errorf("expected marshaled string length of 98, got %d: %s", len(s), s) + } + }) + } }common/types/message/message.go (2)
57-97: Fix typos and improve error handling in Byte48 implementation.The
Byte48implementation has a few minor issues:
- There's a typo in the comment on line 63: "overrite" should be "override"
- Error handling in
MarshalTextcould be more consistent (mixing panic and error returns)- The
isStringfunction might not handle all edge cases// it is a hex encoded big with fixed length on 48 bytes type Byte48 struct { hexutil.Big } func (e Byte48) MarshalText() ([]byte, error) { i := e.ToInt() - // overrite encode big + // override encode big if sign := i.Sign(); sign < 0 { // sanity check return nil, fmt.Errorf("Byte48 must be positive integer") } else { s := i.Text(16) if len(s) > 96 { return nil, fmt.Errorf("Integer Exceed 384bit") } return []byte(fmt.Sprintf("0x%0*s", 96, s)), nil } }Also, consider adding a comment explaining why 48 bytes is required for this specific implementation.
80-97: Consider adding validation for UnmarshalText method.The
Byte48type has an implementation forUnmarshalJSONbut lacks anUnmarshalTextmethod to match the implementedMarshalTextmethod. This could lead to unexpected behavior when using encoding/decoding packages that rely on TextMarshaler/TextUnmarshaler interfaces.Consider adding:
func (e *Byte48) UnmarshalText(input []byte) error { b, err := hexutil.Decode(string(input)) if err != nil { return err } if len(b) != 48 { return fmt.Errorf("not a 48 bytes hex string: %d", len(b)) } var dec big.Int dec.SetBytes(b) *e = Byte48{(hexutil.Big)(dec)} return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
common/types/message/message.go(8 hunks)common/types/message/message_test.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- coordinator/internal/logic/provertask/batch_prover_task.go
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: compile
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (9)
common/types/message/message.go (9)
14-15: Exported constants follow Go naming conventions.The constants have been properly renamed from
euclidForkandeuclidV2ForktoEuclidForkandEuclidV2Forkto make them public, following Go's capitalization convention for exported identifiers.
45-54: LGTM: Task detail types properly reflect fork versioning.The renaming of
ChunkTaskDetailtoLegacyChunkTaskDetailand addition ofEuclidV2ChunkTaskDetailwith the newPrevMsgQueueHashfield is consistent with supporting the EuclidV2 hard fork while maintaining backward compatibility.
101-108: Fields appropriately updated for EuclidV2 support.Changing the types of
KzgProofandKzgCommitmentfrom[]bytetoByte48and renamingChallengetoChallengeDigestare appropriate updates to support the EuclidV2 hard fork.
117-130: Enhanced ChunkInfo with additional metadata fields.The additional fields in
ChunkInfo(PostMsgQueueHash,TxDataLength,InitialBlockNumber, andBlockCtxs) provide necessary metadata for the EuclidV2 fork implementation. These fields appear to support improved message queue handling and block context tracking.
133-139: New BlockContextV2 struct added for EuclidV2 fork.The
BlockContextV2struct provides necessary block metadata that appears to be required for the EuclidV2 implementation, including transaction counts and message counts per block.
155-156: Fork handling in proof creation is consistent.The
NewChunkProoffunction appropriately handles bothEuclidForkandEuclidV2Forkto return anOpenVMChunkProofin both cases. This ensures backward compatibility while supporting the new fork.
189-190: Consistent fork handling across proof types.Similar to the chunk proof handling, the
NewBatchProoffunction correctly handles bothEuclidForkandEuclidV2Forkvalues to maintain consistency across the codebase.
246-247: Consistent fork handling for bundle proofs.The
NewBundleProoffunction's handling of bothEuclidForkandEuclidV2Forkensures a consistent approach to fork support across all proof types in the system.
104-104: Verify blob bytes encoding.In a previous review comment, there was a question about whether blob bytes can be marshalled into base64 format. The field definition remains unchanged, but it's important to verify that the coordinator and prover components handle this field correctly.
Can you confirm that blob bytes are properly encoded and decoded between the coordinator and prover components? A previous reviewer noted this might need base64 encoding.
Co-authored-by: colin <102356659+colinlyguo@users.noreply.github.com> Co-authored-by: Morty <70688412+yiweichi@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
zkvm-prover/Cargo.toml (1)
54-80: Introduction of p3- Dependencies on the openvm-v2 Branch*
Multiplep3-*dependencies have been added, all pointing to theopenvm-v2branch. This aligns with the required updates for EuclidV2 support. Please verify that these dependencies are fully compatible with the rest of the codebase, and note that the commentary onp3-maybe-rayon(disabling parallel features by default) is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
zkvm-prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
zkvm-prover/Cargo.toml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: clippy
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (3)
zkvm-prover/Cargo.toml (3)
21-22: Dependency Renaming for Euclid Support
The new dependency entries forscroll-zkvm-prover-euclidandscroll-zkvm-prover-euclidv2correctly align with the introduction of EuclidV2 support. Please verify that aliasing both dependencies with the package name"scroll-zkvm-prover"does not lead to version conflicts and behaves as expected with Cargo’s aliasing mechanism.
25-27: Updated scroll-proving-sdk Revision
The dependency forscroll-proving-sdkis updated to revision36f1808. Confirm that this new revision is compatible with the EuclidV2 changes and does not introduce any breaking changes to the proof handling logic.
49-53: New Patch Overrides for OpenVM Stark Dependencies
A patch section has been introduced to overrideopenvm-stark-backendandopenvm-stark-sdk, sourcing them from thescroll-tech/openvm-stark-gpurepository with thegpufeature enabled. Ensure that these patched dependencies work seamlessly in your GPU-enabled environment and that any potential integration issues are addressed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
common/types/message/message.go (2)
75-92: Ensure special cases are handled in UnmarshalJSON.While the implementation is generally robust, consider adding handling for edge cases such as null values or empty strings.
func (e *Byte48) UnmarshalJSON(input []byte) error { + // Handle null or empty value + if len(input) == 0 || string(input) == "null" { + return nil + } if !isString(input) { return errors.New("not hex string") }
110-125: Enhanced ChunkInfo structure supports EuclidV2Fork requirements.The additional fields provide necessary context for EuclidV2Fork processing. Consider adding more detailed documentation for each new field to explain their purpose and usage.
type ChunkInfo struct { ChainID uint64 `json:"chain_id"` PrevStateRoot common.Hash `json:"prev_state_root"` PostStateRoot common.Hash `json:"post_state_root"` WithdrawRoot common.Hash `json:"withdraw_root"` DataHash common.Hash `json:"data_hash"` IsPadding bool `json:"is_padding"` TxBytes []byte `json:"tx_bytes"` TxBytesHash common.Hash `json:"tx_data_digest"` PrevMsgQueueHash common.Hash `json:"prev_msg_queue_hash"` + // Hash of the message queue after processing this chunk PostMsgQueueHash common.Hash `json:"post_msg_queue_hash"` + // Total length of transaction data in this chunk TxDataLength uint64 `json:"tx_data_length"` + // Initial block number in this chunk InitialBlockNumber uint64 `json:"initial_block_number"` + // Block contexts for each block in this chunk BlockCtxs []BlockContextV2 `json:"block_ctxs"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/types/message/message.go(8 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- coordinator/internal/logic/provertask/batch_prover_task.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: fmt
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (9)
common/types/message/message.go (9)
14-15: Exporting the fork constants is a good practice.Making
EuclidForkpublic (renamed fromeuclidFork) and adding the newEuclidV2Forkconstant with consistent naming enhances API clarity and accessibility.
47-48: New message queue hash field aligns with EuclidV2Fork requirements.The addition of
PrevMsgQueueHashto theChunkTaskDetailstruct properly supports the EuclidV2Fork functionality by tracking the previous message queue state.
51-69: Well-implemented custom type for large integers.The
Byte48type is well-designed with proper validation for handling 384-bit integers with fixed-length encoding. The implementation includes appropriate error handling for negative values and integers exceeding the bit limit.
96-102: Field type changes improve type safety.Changing
KzgProofandKzgCommitmentfrom[]bytetoByte48and renamingChallengetoChallengeDigestprovides better type safety and naming clarity. Theomitemptytags are also appropriate for these optional fields.
127-134: New BlockContextV2 structure is properly defined.The
BlockContextV2struct provides all necessary fields for the EuclidV2Fork block context with appropriate types and JSON tags.
150-150: Function now handles both fork versions correctly.The update to
NewChunkProofproperly handles bothEuclidForkandEuclidV2Forkwhen creating a new chunk proof instance.
184-184: Function now handles both fork versions correctly.The update to
NewBatchProofproperly handles bothEuclidForkandEuclidV2Forkwhen creating a new batch proof instance.
241-241: Function now handles both fork versions correctly.The update to
NewBundleProofproperly handles bothEuclidForkandEuclidV2Forkwhen creating a new bundle proof instance.
319-329: OpenVMBatchInfo struct properly updated for EuclidV2Fork.The struct includes all necessary fields for the EuclidV2Fork, with proper types and JSON tags. The message queue hash fields align with the other changes in the codebase.
common/types/message/message.go
Outdated
| BlobBytes []byte `json:"blob_bytes"` | ||
| KzgProof Byte48 `json:"kzg_proof,omitempty"` | ||
| KzgCommitment Byte48 `json:"kzg_commitment,omitempty"` | ||
| ChallengeDigest common.Hash `json:"challenge_digest,omitempty"` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if BlobBytes should be encoded as base64.
Based on previous comments, there was a concern about BlobBytes being marshaled into base64. While it seems to be working in the current implementation, consider adding a comment to clarify the encoding expectations.
🌐 Web query:
Is it common practice to encode blob bytes as base64 in JSON for EVM/ZK proof systems?
💡 Result:
Encoding blob bytes as Base64 in JSON is a common practice in systems involving EVM and ZK proofs when interoperability with text-based formats is required, though there are tradeoffs between compatibility and efficiency. Below is the breakdown:
General Practice
-
JSON Limitations:
JSON doesn’t natively support binary data, requiring Base64 encoding for binary/bytes storage (Source 3, Source 9).
Examples from APIs (e.g., Ethereum JSON-RPC, Vertex AI) mandate Base64 for binary payloads like images or hashes[3][9]. -
Base64 Adoption:
- Widely used for transmitting binary (e.g., BLOBs) over text-based protocols (HTTP/JSON).
- Simplifies embedding binary data in human-readable formats[3][4][7].
- Tools like Google Cloud and Ethereum clients standardize this approach[3][9].
-
Performance Tradeoffs:
- Adds ~33% overhead compared to raw binary[7][10].
- Encoding/decoding costs can be non-trivial in high-throughput systems.
EVM/ZK-Specific Considerations
EVM Ecosystems
- EIP-4844 (Proto-Danksharding): Uses binary blobs for rollup data with KZG commitments rather than Base64 for efficiency[8][14].
- JSON-RPC Interfaces: May use Base64 for API fields (e.g.,
logsBloom,inputin transaction receipts)[9]. - Smart Contracts: Avoid Base64; rely on raw bytes or hashes (e.g., keccak256 for onchain verification)[5][8].
Zero-Knowledge Proofs
- Circuit Constraints: Base64 increases computational cost (decoding adds gates). Noir and ZK-EVMs prefer raw bytes or field elements[6][11].
- Data Serialization:
- Noir Libraries: Provide Base64 utilities for JSON compatibility but favor binary formats in proofs[6].
- ZK-EVM Circuits: Use custom encodings (e.g., protobuf for PFB transactions[2]) or direct field element packing[5][12].
When Base64 is Avoided
- High-Performance Layers:
- Blob data in EIP-4844 transactions avoids Base64 for direct KZG proof integration[8][14].
- ZK proof systems like Plonky2 or STARKs optimize for raw binary to minimize proof size[5][11].
- Onchain Verification:
- Precompiles (e.g.,
point_evaluation_precompile) use raw bytes for cryptographic operations[8]. - Merkle proofs and hash functions (e.g., Keccak256) operate on unencoded bytes[5][6].
- Precompiles (e.g.,
Summary
- Common in APIs/Interfaces: Base64 is standard for JSON interoperability (e.g., REST APIs[3], Ethereum’s JSON-RPC[9]).
- Avoided in Core Layers: EVM execution and ZK circuits prioritize efficiency with raw bytes/commitments[2][5][8].
Key Sources:
- Base64 in JSON-RPC and APIs (3, 9)
- Efficiency-driven alternatives in ZK/EVM (5, 8, 14)
Citations:
- 1: https://jb64.org/specification/
- 2: https://docs.celestia.org/how-to-guides/blobstream-rollups
- 3: https://cloud.google.com/vertex-ai/generative-ai/docs/image/base64-encode
- 4: https://ionic.io/blog/converting-a-base64-string-to-a-blob-in-javascript
- 5: https://toposware.com/blog/pushing-the-limits-of-zero-knowledge/
- 6: https://github.com/noir-lang/awesome-noir/blob/main/README.md
- 7: https://base64.guru/converter/encode/file
- 8: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-4844.md
- 9: https://entethalliance.github.io/client-spec/spec.html
- 10: https://www.base64encode.org/enc/blob/
- 11: https://dev.to/spalladino/a-beginners-intro-to-coding-zero-knowledge-proofs-c56
- 12: https://www2.eecs.berkeley.edu/Pubs/TechRpts/2019/EECS-2019-113.pdf
- 13: Do not convert BLOB value to base64 rqlite/rqlite#1691
- 14: https://www.eip4844.com
- 15: https://github.com/dotnet/corefx/issues/36974
- 16: https://developer.electricimp.com/examples/cookbook/device-base-64
- 17: https://github.com/delendum-xyz/zk-knowledge/blob/main/zk-knowledge.md
Add Clarifying Comment on BlobBytes Encoding
The field BlobBytes in common/types/message/message.go is currently marshaled into JSON without any documentation on its encoding. Based on common practices in EVM and ZK proof systems, binary data meant for JSON interoperability is generally encoded as Base64. To ensure clarity and prevent any ambiguity for future maintainers, please add an inline comment specifying that BlobBytes is expected to be Base64 encoded.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
common/types/message/message.go (1)
51-92: New Byte48 type implementationGood implementation of a specialized type for handling 48-byte fixed-length big integers with proper validation. The type properly handles marshaling and unmarshaling with appropriate error checking.
However, there's a potential issue with the error message format in line 86.
Consider improving the error message format:
- return fmt.Errorf("not a 48 bytes hex string: %d", len(b)) + return fmt.Errorf("expected 48 bytes hex string, got: %d bytes", len(b))coordinator/internal/logic/provertask/batch_prover_task.go (1)
219-223: Variable rename for clarityRenamed variable from
haloProottohalo2Prooffor better readability and clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/types/message/message.go(8 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
coordinator/internal/logic/provertask/batch_prover_task.go (1)
common/types/message/message.go (3) (3)
Halo2ChunkProof(158-168)ChunkInfo(111-125)OpenVMChunkProof(300-308)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (12)
common/types/message/message.go (8)
14-15: Rename and addition of fork constantsThe constants have been updated to support the EuclidV2 fork. Good practice to use capitalized names for exported constants.
45-49: Added PrevMsgQueueHash to ChunkTaskDetailThe addition of this field ensures we can track the previous message queue hash state, which is necessary for the EuclidV2 fork.
110-125: Extended ChunkInfo with additional fields for EuclidV2New fields added to the
ChunkInfostruct for the EuclidV2 fork includingPostMsgQueueHash,TxDataLength,InitialBlockNumber, andBlockCtxs. These fields provide the necessary context data for the EuclidV2 implementation.
127-134: Added BlockContextV2 struct for EuclidV2The new
BlockContextV2struct contains necessary block context information for the EuclidV2 fork, with all the required fields for block processing.
147-155: Updated NewChunkProof to support EuclidV2ForkModified to handle both
EuclidForkandEuclidV2Forkin the same switch case, which is clean and ensures consistent handling of both forks.
181-189: Updated NewBatchProof to support EuclidV2ForkSimilar to the
NewChunkProofupdate, this method now handles bothEuclidForkandEuclidV2Forkin the same case, maintaining consistent fork handling.
238-246: Updated NewBundleProof to support EuclidV2ForkThe method has been updated to handle both
EuclidForkandEuclidV2Forkin the same case, consistent with the updates toNewChunkProofandNewBatchProof.
319-329: Added message queue hash fields to OpenVMBatchInfoAdded both
PrevMsgQueueHashandPostMsgQueueHashfields to theOpenVMBatchInfostruct to properly track message queue state changes in the EuclidV2 fork.coordinator/internal/logic/provertask/batch_prover_task.go (4)
215-217: Added PostMsgQueueHash to chunkInfoNow including the post-state of the message queue hash, which complements the existing
PrevMsgQueueHashfield and provides complete state transition information for EuclidV2.
224-228: Added support for OpenVMChunkProofAdded a new condition to check for
OpenVMChunkProoftype and populate additional fields required for EuclidV2 includingInitialBlockNumber,BlockCtxs, andTxDataLength.
249-251: Added debug loggingAdded useful debug logging to display task information. This correctly uses
message.ProofTypeBatch.String()as noted in a previous review.
294-296: Fixed type handling for KzgProof and KzgCommitmentUpdated to use the new
Byte48type forKzgProofandKzgCommitmentfields, resolving the type mismatch issue mentioned in previous reviews.
Purpose or design rationale of this PR
This PR:
euclidv2dependencies of provers and coordinator's verifier.euclidv2in proof handling.euclidv2verifier in coordinator.euclidv2handler in prover.Corresponding
scroll-proving-sdkpr: scroll-tech/scroll-proving-sdk#64Summary by CodeRabbit
New Features
EuclidV2Handler, for improved management of proof tasks.euclidv2support in prover and verifier components.Bug Fixes
Documentation
Byte48type to validate its functionality.Chores