feat: openvm euclid v1 #1633
feat: openvm euclid v1 #1633
Conversation
WalkthroughThe changes update dependency version tags in two Changes
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 (9)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1633 +/- ##
===========================================
- Coverage 41.49% 41.31% -0.18%
===========================================
Files 222 222
Lines 18220 18275 +55
===========================================
- Hits 7561 7551 -10
- Misses 9926 9995 +69
+ Partials 733 729 -4
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)
zkvm-prover/src/zk_circuits_handler/euclid.rs (3)
27-35: Consider extracting the segment length constant.The specific value
(1 << 22) - 100appears multiple times in the code. Consider extracting this to a constant at the module level to improve maintainability and ensure consistency.use tokio::sync::Mutex; + +// Maximum segment length for Euclid provers (4M - 100) +const EUCLID_SEGMENT_LEN: usize = (1 << 22) - 100; + pub struct EuclidHandler {Then use this constant in all three ProverConfig instances:
ProverConfig { - segment_len: Some((1 << 22) - 100), + segment_len: Some(EUCLID_SEGMENT_LEN), },
31-33: Add documentation for the segment_len configuration.Consider adding a comment explaining what
segment_lenconfigures and why this specific value is chosen. Documentation will help other developers understand the purpose and impact of this configuration parameter.
27-59: Consider more graceful error handling during setup.All three prover setup calls use
.expect()which will panic if setup fails. For a production system, consider implementing more graceful error handling that allows for recovery or proper reporting, especially since these are being initialized during the application startup.Example approach:
-let chunk_prover = ChunkProver::setup( +let chunk_prover = match ChunkProver::setup( chunk_exe, chunk_app_config, Some(cache_dir.clone()), ProverConfig { segment_len: Some((1 << 22) - 100), }, -) -.expect("Failed to setup chunk prover"); +) { + Ok(prover) => prover, + Err(e) => { + log::error!("Failed to setup chunk prover: {}", e); + return Err(anyhow!("Failed to initialize EuclidHandler: {}", e)); + } +};This would require modifying the return type of the
newfunction toResult<Self, anyhow::Error>.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
common/libzkp/impl/Cargo.lockis excluded by!**/*.lockzkvm-prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
common/libzkp/impl/Cargo.toml(1 hunks)zkvm-prover/Cargo.toml(1 hunks)zkvm-prover/src/zk_circuits_handler/euclid.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- zkvm-prover/Cargo.toml
- common/libzkp/impl/Cargo.toml
⏰ 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
🔇 Additional comments (3)
zkvm-prover/src/zk_circuits_handler/euclid.rs (3)
9-9: Import of new ProverConfig struct.This addition imports the new
ProverConfigstruct from thescroll_zkvm_provermodule, which is necessary for the updated prover setup methods. This aligns with the version bump tov0.1.0-rc.8mentioned in the PR.
39-47: Configuration for BatchProver setup.The BatchProver setup now correctly includes the ProverConfig parameter with a consistent segment length. This aligns with the updated API from the dependency upgrade.
51-59: Configuration for BundleProver setup.The BundleProver setup now correctly includes the ProverConfig parameter with a consistent segment length. This aligns with the updated API from the dependency upgrade.
Signed-off-by: noelwei <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
zkvm-prover/src/zk_circuits_handler/euclid.rs (2)
27-35: Consider making segment_len configurableThe segment length is hardcoded to
(1 << 22) - 100. While this specific value (4,194,204 bytes) may be optimal for current use cases, consider making this configurable via environment variables or configuration files for better flexibility across different deployment environments.- ProverConfig { - segment_len: Some((1 << 22) - 100), - }, + ProverConfig { + segment_len: Some(config.get_segment_len().unwrap_or((1 << 22) - 100)), + },
39-47: Consider extracting duplicate ProverConfigThe same ProverConfig with identical values is repeated for all three provers. Consider extracting this to a single configuration instance to improve maintainability.
pub fn new(workspace_path: &str) -> Self { let workspace_path = Path::new(workspace_path); let cache_dir = workspace_path.join("cache"); + let prover_config = ProverConfig { + segment_len: Some((1 << 22) - 100), + }; 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), - }, + prover_config.clone(), ) .expect("Failed to setup chunk prover");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zkvm-prover/src/zk_circuits_handler/euclid.rs(3 hunks)
⏰ 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: tests
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (2)
zkvm-prover/src/zk_circuits_handler/euclid.rs (2)
9-9: Import of ProverConfig added correctlyThe ProverConfig import is appropriately added to support the new configuration parameter in the prover setup methods.
92-92:❓ Verification inconclusive
Verify correct default for prev_msg_queue_hash
The
prev_msg_queue_hashfield is being added with a default value. Verify that using a default value is appropriate for this field and wouldn't lead to security issues or unexpected behavior in the proving system.
🏁 Script executed:
#!/bin/bash # Search for other references to prev_msg_queue_hash to understand its usage rg -A 2 -B 2 "prev_msg_queue_hash" --type rustLength of output: 470
Action Required: Validate the Default Value for
prev_msg_queue_hashI verified that the only instance of setting
prev_msg_queue_hashis viaDefault::default()inzkvm-prover/src/zk_circuits_handler/euclid.rs(line 92). Please confirm that initializing this security-critical field with its default value is indeed the intended behavior for the proving task. If the default is correct (for example, if this field should be unset or zeroed in the initial proving state), consider adding a clarifying inline comment explaining whyDefault::default()is appropriate so that future maintainers understand its security implications.
Co-authored-by: Morty <[email protected]> Co-authored-by: colin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
zkvm-prover/Cargo.toml (1)
49-52: New Patch for OpenVM Stark Dependencies
This section adds patches foropenvm-stark-backendandopenvm-stark-sdkpointing to an SSH URL and themainbranch with thegpufeature enabled. Ensure that using SSH URLs is intentional and supported in all build and CI environments—if HTTPS access might be preferable for easier integration in some contexts, consider revisiting this configuration.
📜 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 (9)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (3)
zkvm-prover/Cargo.toml (3)
21-21: Version Update forscroll-zkvm-proverDependency
The dependency version is updated from the previous release candidate (rc.6) tov0.1.0-rc.8, which aligns with the PR’s objective for the openvm euclid feature upgrade. Ensure that downstream integrations or any dependent packages are tested with this new version.
27-29: Branch Update forsbv-primitivesDependency
The branch has been updated fromzkvm/euclid-v2tozkvm/euclid-upgradeas part of the coordinated version bump across related components. While this update should be compatible, please verify that no unexpected API changes or breaking modifications have been introduced by the branch change.
53-79:❓ Verification inconclusive
Extensive Addition of Plonky3 Dependencies
A significant number of new dependencies related toplonky3-gpu(e.g.,p3-air,p3-field,p3-commit, etc.) have been added under a new patch section, with the branch set toopenvm-v2. This is a major change. Please verify that these dependencies are compatible with the rest of the codebase and that any potential API changes are well documented in the release notes or migration guide, as they could have far-reaching impacts.
Action Required: Confirm Compatibility and Documentation for New Plonky3 Dependencies
The patch in
zkvm-prover/Cargo.tomladds multiple dependencies from theplonky3-gpurepository (all pinned to theopenvm-v2branch). Given the scope of these changes, please ensure that:
- All the newly referenced dependencies (e.g.,
p3-air,p3-field, etc.) are fully compatible with the rest of the codebase.- Any API changes or behavioral differences introduced by these updates are clearly documented in the release notes or a migration guide.
Please review and update the documentation as needed.
🏁 Script executed:
#!/bin/bash echo "Searching for usage of 'openvm-v2' branch in the repository..." rg -n "openvm-v2" . echo -e "\nSearching for migration documentation or release notes referencing 'plonky3' changes..." rg -n -i "plonky3" README.md CHANGELOG.md MIGRATION.md || echo "No matching documentation files found"Length of output: 8006
Action Required: Verify Compatibility and Documentation for New
plonky3-gpuDependenciesThe patch in
zkvm-prover/Cargo.tomladds many dependencies from theplonky3-gpurepository, all pinned to theopenvm-v2branch. Our repository search confirmed that these dependencies (e.g.,p3-air,p3-field, etc.) are indeed included, and references to theopenvm-v2branch are consistent across bothCargo.tomlandCargo.lock.However, no migration documentation or release notes were found that reference changes related to
plonky3. Given the major nature of these updates, please ensure that:
- Compatibility: These new dependencies are fully compatible with the rest of the codebase.
- Documentation: Any API changes or modifications introduced by these updates are clearly documented. If such documentation or a migration guide is missing, please add or reference it to help future developers understand the implications of the changes.
Please review these points carefully to ensure the stability and maintainability of the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
zkvm-prover/Cargo.toml (2)
48-51: New Patch: Addedopenvm-stark-backendandopenvm-stark-sdk
A new patch section has been added with the dependencies foropenvm-stark-backendandopenvm-stark-sdk, both using the"main"branch and GPU features. Consider whether pinning to a specific commit (or using a tag) might improve stability if these dependencies are critical for production.
53-79: New Patch: Addition of Multipleplonky3-gpuDependencies
This patch introduces a broad set of dependencies (e.g.,p3-air,p3-field, etc.) all targeting the"openvm-v2"branch. This represents a significant update to the dependency graph. Please ensure that these libraries are fully compatible with your system and that they undergo proper testing given the scope of changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
common/libzkp/impl/Cargo.lockis excluded by!**/*.lockzkvm-prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
common/libzkp/impl/Cargo.toml(1 hunks)zkvm-prover/Cargo.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/libzkp/impl/Cargo.toml
🔇 Additional comments (2)
zkvm-prover/Cargo.toml (2)
21-21: Dependency Version Bump forscroll-zkvm-prover
The version tag has been updated tov0.1.0-rc.8.1, aligning the dependency with the new release candidate. Please verify that all dependent modules work with this updated version.
27-29: Branch Update forsbv-primitivesDependency
The dependency branch has been updated to"zkvm/euclid-upgrade". Confirm that this branch includes the necessary updates and that it remains compatible with the rest of the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
common/types/message/message.go (3)
46-48: Add tests for new fields inChunkTaskDetail.
The newBlockHashesandPrevMsgQueueHashfields are integral to chunk details. Please ensure thorough test coverage to validate their behavior.
50-68:Byte48type and JSON methods appear robust.
The length checks, positivity checks, and error conditions inMarshalText/UnmarshalJSONare well-structured. Consider adding explicit unit tests for edge cases (e.g., negative numbers, incorrect byte lengths).Also applies to: 70-91
126-132:BlockContextV2addition.
Simple structure for block context. Double-check references throughout the codebase to confirm correct usage.coordinator/internal/logic/provertask/chunk_prover_task.go (1)
191-192:PrevMsgQueueHashassignment.
Creates a more complete task detail. Remember to handle cases wherechunk.PrevL1MessageQueueHashmight be empty or zero.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
common/types/message/message.go(9 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(3 hunks)coordinator/internal/logic/provertask/chunk_prover_task.go(3 hunks)
🔇 Additional comments (15)
common/types/message/message.go (7)
7-10: Imports look correct.
No issues found with the updated import statements bringing in"math/big"and"github.com/scroll-tech/go-ethereum/common/hexutil".
14-14: ExportingEuclidForkis beneficial.
MakingEuclidForkpublic increases flexibility across packages.
95-101: Confirm usage of newBatchTaskDetailfields.
These added fields (KzgProof,KzgCommitment,ChallengeDigest) are critical for cryptographic verifications. Verify any dependency areas that need to handle potential errors or missing data.
111-124: ExpandedChunkInfostruct.
Including message queues, data lengths, and block contexts aligns with the Euclid fork’s requirements. Ensure each field is consumed properly in the proof pipeline.
318-327: AugmentedOpenVMBatchInfofields.
Fields likeParentBatchHashandPrevMsgQueueHashare crucial for continuity checks. Please verify that all creations and checks for these fields are consistent across the codebase.
387-387: NewMsgQueueHashwithinOpenVMBundleInfo.
This additional field ensures consistency across bundles, batches, and chunks.
414-414: Updated proof documentation.
The transition from length 384 to 1472 reflects the expanded proof size. Good clarity in the comment.coordinator/internal/logic/provertask/chunk_prover_task.go (3)
12-12: New import forcommon.
Used forcommon.Hashconversions.
166-166: ExtendedformatProverTaskcall signature.
PassingchunkTaskaligns with the need to include message queue details.
183-183: UpdatedformatProverTaskmethod signature.
Acceptingchunkprovides additional context to populateChunkTaskDetail.coordinator/internal/logic/provertask/batch_prover_task.go (5)
7-7: Addedmath/bigimport.
Necessary for dealing with BLS curve points.
15-15: Addedhexutilimport.
Essential forByte48conversions.
216-216: PopulatingPostMsgQueueHashinchunkInfo.
Helps maintain continuity between consecutive message queue states in the batch.
219-228: Conditional logic forHalo2ChunkProofvsOpenVMChunkProof.
Extracting custom fields for each proof type makes the aggregator more flexible. Ensure thorough testing of both code paths.
291-295: Safe extraction of KZG fields fromBlobDataProof.
Well-safeguarded by the prior length check. The slice indices match the known byte offsets forKzgCommitmentandKzgProof.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
common/types/message/message.go (2)
407-415:⚠️ Potential issueUpdate documentation to match implementation.
The comment describes 12 accumulators totaling 384 bytes, but the implementation now uses 1472 bytes. The documentation should be updated to match the new implementation.
// The structure of these bytes is: // | byte index start | byte length | value | description | // |------------------|----------------|----------|---------------------| // | 0 | 32 | accs[0] | accumulator 1 | // | 32 | 32 | accs[1] | accumulator 2 | // | 32*i ... | 32 | accs[i] | accumulator i ... | -// | 352 | 32 | accs[11] | accumulator 12 | -// | 384 | dynamic | proof | proof bytes | +// | ... | ... | ... | ... | +// | 1440 | 32 | accs[45] | accumulator 46 | +// | 1472 | dynamic | proof | proof bytes |
416-416:⚠️ Potential issueUpdate buffer capacity to match new size requirements.
The buffer capacity needs to be updated to match the new size requirements.
- proofBytes := make([]byte, 0, 384+len(p.EvmProof.Proof)) + proofBytes := make([]byte, 0, 1472+len(p.EvmProof.Proof))
🧹 Nitpick comments (3)
common/types/message/message.go (3)
57-58: Fix typo in comment.There's a spelling error in the comment - "overrite" should be "override".
- // overrite encode big + // override encode big
63-65: Improve error message clarity.The error message mentions "384bit" which is technically correct (48 bytes = 384 bits), but it would be clearer to maintain consistency with the type name.
- return nil, fmt.Errorf("integer Exceed 384bit") + return nil, fmt.Errorf("integer exceeds 48 bytes (384 bits)")
50-53: Consider a more descriptive name for the Byte48 type.The name
Byte48is descriptive of its size, but it might be confusing as it's not actually a byte slice of length 48, but rather a structure that wraps ahexutil.Bigwith specific behaviors.Consider renaming to something like
Fixed48BytesBigto make its purpose clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/types/message/message.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (2)
common/types/message/message.go (2)
415-419:⚠️ Potential issueGuard against potential slice boundary errors.
Before slicing
p.EvmProof.Instances[:1472], consider validatinglen(p.EvmProof.Instances) >= 1472to avoid panic in malformed proofs.func (p *OpenVMBundleProof) Proof() []byte { + if len(p.EvmProof.Instances) < 1472 { + panic("instances array is too short to safely slice 1472 bytes") + } proofBytes := make([]byte, 0, 1472+len(p.EvmProof.Proof)) proofBytes = append(proofBytes, p.EvmProof.Instances[:1472]...) return append(proofBytes, p.EvmProof.Proof...) }
14-14: Check if exposing this constant publicly is intentional.The constant
euclidForkhas been renamed toEuclidForkwith a capital first letter, making it publicly accessible. Ensure this change is intentional and that external packages should be able to access this constant.
Signed-off-by: noelwei <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
common/types/message/message.go (2)
50-53: Consider refining doc comments forByte48
TheByte48struct is straightforward and necessary for 48-byte cryptographic values. You might clarify typical usage scenarios with a more descriptive comment.-// it is a hex encoded big with fixed length on 48 bytes +// Byte48 represents a 48-byte big-endian value for cryptographic usage, encoded as a hex string.
55-68: Check zero-padding edge cases inMarshalText
The function ensures the integer is not negative and does not exceed 384 bits. It also left-pads the output with zeros if needed. This is correct, but consider adding test coverage for edge cases (like zero or very small values) to prevent regressions.coordinator/internal/logic/provertask/batch_prover_task.go (2)
225-228: Avoid duplication in chunk field assignments
These assignments fromopenvmProof.MetaData.ChunkInfooverlap with those fromhalo2Proof.ChunkInfo. Factorizing them could simplify the code and reduce duplication.
293-297: Address the FIXME forChallengeDigest
Currently, theChallengeDigestfield is commented out to bypass the prover's sanity checks. This may cause confusion or inconsistencies. Consider implementing the missing logic or clarifying the rationale in code documentation.Would you like assistance creating an issue or proposing a fix?
zkvm-prover/Cargo.toml (1)
49-52: Review new openvm-stark patches
Adding these patches (openvm-stark-backend,openvm-stark-sdk) may bring additional dependencies or performance overhead. Confirm that they do not affect existing features unexpectedly.
📜 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 (4)
common/libzkp/impl/Cargo.toml(1 hunks)common/types/message/message.go(8 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(3 hunks)zkvm-prover/Cargo.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/libzkp/impl/Cargo.toml
🧰 Additional context used
🧬 Code Definitions (1)
coordinator/internal/logic/provertask/batch_prover_task.go (1)
common/types/message/message.go (4) (4)
Halo2ChunkProof(157-167)ChunkInfo(110-124)OpenVMChunkProof(299-307)Byte48(51-53)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (12)
common/types/message/message.go (7)
7-10: No issues found with new imports
The newly introduced imports"math/big"and"github.com/scroll-tech/go-ethereum/common/hexutil"appear to be correctly used for handling large numeric values and hexadecimal encoding in the newByte48type.
14-14: Public constant renaming
Renaming the constant toEuclidForkis a clear and consistent way to reference this hard fork.
70-91: Validate 48-byte requirement inUnmarshalJSON
Rejecting any input that does not precisely decode to 48 bytes is appropriate for your cryptographic use case. If you anticipate any leading-zero encodings or alternative formats, ensure they are addressed. Otherwise, this looks good as is.
111-123: Confirm memory impact of new fields inChunkInfo
PostMsgQueueHash,TxDataLength,InitialBlockNumber, and the potentially largeBlockCtxsfield can significantly affect memory usage for large chunks. Verify that any new overhead is acceptable and that boundary scenarios (e.g., zero or high values) are handled.
126-132: Cover the newBlockContextV2in tests
TheBlockContextV2struct introduces additional block context fields (Timestamp,BaseFee, etc.). Confirm that these values are thoroughly tested, especially if they differ from existing context structures.
387-387: Check references toMsgQueueHash
Introducing theMsgQueueHashfield inOpenVMBundleInfomight require updates in proof verification or other logic. Verify that this new field is consistently handled across the codebase.
95-101:❓ Verification inconclusive
Ensure cryptographic verification for new
BatchTaskDetailfields
The fieldsKzgProof,KzgCommitment, andChallengeDigestare critical for proof integrity. Make sure you have proper validations or sanity checks in any downstream logic that consumes these fields.
🏁 Script executed:
#!/bin/bash # Searching for references to ensure these fields are validated ast-grep --pattern $'KzgProof($_)' ast-grep --pattern $'KzgCommitment($_)' ast-grep --pattern $'ChallengeDigest($_)'Length of output: 119
Action: Verify Cryptographic Validations for BatchTaskDetail Fields
Automated searches using
ast-grepproduced no evidence of cryptographic validations for the new fields (KzgProof,KzgCommitment, andChallengeDigest) added incommon/types/message/message.go(lines 95-101). Since these fields are critical to maintaining the integrity of proof data, please manually verify that any downstream logic processing these fields (e.g., within BatchTaskDetail handling) includes rigorous checks such as format validation, consistency verification, and any other appropriate sanity checks.
- File:
common/types/message/message.go(lines 95-101)- Fields to verify:
KzgProofKzgCommitmentChallengeDigestIf validations are implemented in parts of the code not easily detected by our automated search, please provide further details to ensure robust cryptographic integrity.
coordinator/internal/logic/provertask/batch_prover_task.go (2)
7-15: Validate new imports
The import of"math/big"and"github.com/scroll-tech/go-ethereum/common/hexutil"indicates additional big-integer and hex handling. Ensure they are needed and used consistently.
224-224: Consider checking for nil metadata inOpenVMChunkProof
When casting toOpenVMChunkProof, ensureMetaDataandMetaData.ChunkInfoare non-nil to avoid runtime errors.zkvm-prover/Cargo.toml (3)
21-21: Upgrade library version
Bumpingscroll-zkvm-proverfromv0.1.0-rc.6tov0.1.0-rc.8aligns with the new fork logic. This looks consistent with other changes.
53-78: Verify GPU branch dependencies
Multiple new dependencies on Plonky3 GPU branches (p3-air,p3-field,p3-commit, etc.) could be critical for performance. Ensure they are compatible and consistently versioned to avoid integration issues.
27-27:❓ Verification inconclusive
Confirm branch stability for
sbv-primitives
Switching to branch"zkvm/euclid-upgrade"may introduce untested changes. Verify the branch is stable or pinned to a known commit if reliability is crucial.
🏁 Script executed:
#!/bin/bash # Confirm the new branch exists in the remote repository git ls-remote https://github.com/scroll-tech/stateless-block-verifier.git | grep "zkvm/euclid-upgrade"Length of output: 174
Action Required: Confirm Dependency Stability for
sbv-primitivesThe remote branch
zkvm/euclid-upgradehas been verified to exist (commit:c63ff8b7904692df3243f7a6ed1c7ee56b8c4f5d). However, since tracking a branch can introduce untested changes over time, please ensure that this branch is stable for our use case. If stability is crucial, consider pinning the dependency to the specific commit.
- File:
zkvm-prover/Cargo.toml(line 27)- Dependency:
sbv-primitivesusing branchzkvm/euclid-upgrade
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
zkvm-prover/Cargo.toml (1)
53-79: New Patch for Plonky3 GPU Dependencies
A comprehensive patch block under[patch."https://github.com/Plonky3/Plonky3.git"]has been introduced, adding multiple dependencies (e.g.,p3-air,p3-field,p3-commit, etc.) all targeting the"openvm-v2"branch. This addition substantially expands the dependency graph to support the OpenVM upgrade. It is recommended to:
- Double-check consistency in branch targeting for all these dependencies.
- Verify that these dependencies integrate smoothly without causing compile-time or runtime conflicts.
- Consider adding CI checks or integration tests if not already in place, as this is a significant expansion of external code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
common/libzkp/impl/Cargo.lockis excluded by!**/*.lockzkvm-prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
common/libzkp/impl/Cargo.toml(1 hunks)zkvm-prover/Cargo.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/libzkp/impl/Cargo.toml
⏰ 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: tests
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (2)
zkvm-prover/Cargo.toml (2)
21-21: Dependency Version Bump forscroll-zkvm-prover
The version tag forscroll-zkvm-proverhas been updated to"v0.1.0-rc.9"as required by the PR objectives. This change is consistent with the coordinated upgrade across dependencies. Please verify that downstream consumers and integrations are compatible with this release candidate.
27-27: Branch Update forsbv-primitivesDependency
The branch forsbv-primitiveshas been updated to"zkvm/euclid-upgrade"in line with the overall upgrade efforts referenced in the PR. Confirm that this change does not lead to unexpected breaking changes in dependent modules.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
common/types/message/message.go (1)
50-72: Refine edge-case testing for the 48-byte integer logic.The
Byte48type properly rejects negative integers and checks for values that exceed 384 bits. The code is well-structured. However, consider adding unit tests to confirm behavior at exact boundary sizes (e.g., exactly 48 bytes, or zero).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/types/message/message.go(7 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (6)
common/types/message/message.go (5)
46-48: Add unit tests for these new chunk fields.The newly introduced
BlockHashesandPrevMsgQueueHashfields inChunkTaskDetailmirror the enhancements seen in other task structures. This addition is similar to past expansions where dedicated unit tests were requested to ensure correct marshaling/unmarshaling, validation, and usage.Would you like me to generate a script to locate or create test coverage for these fields in the codebase?
95-101: Confirm coverage for newly added cryptographic fields.
KzgProof,KzgCommitment, andChallengeDigestare fundamental parts of the batch proof workflow. Ensure they are validated and covered in unit or integration tests to catch potential mismatch or serialization issues.
112-125: Request coverage and usage validation for newChunkInfofields.The fields
PrevMsgQueueHash,PostMsgQueueHash,TxDataLength,InitialBlockNumber, andBlockCtxsare critical to proof correctness. Confirm these fields are rigorously tested, especially with edge cases (e.g., empty message queue, zero block numbers).
127-134: Straightforward extension for block context.The
BlockContextV2structure extends existing patterns for block time, base fee, gas limits, and message counts. The field definitions look good; just remember to ensure robust test coverage, especially for boundary values (e.g., very large gas limits).
416-419:⚠️ Potential issueGuard against potential slice out-of-bounds.
When slicing
p.EvmProof.Instances[:384], a malformed or emptyInstancesarray could trigger a runtime panic. Match the prior boundary checks recommended for chunk and batch proofs to ensure safe slicing.Below is a possible fix:
func (p *OpenVMBundleProof) Proof() []byte { + if len(p.EvmProof.Instances) < 384 { + panic("instances array is too short to safely slice 384 bytes") + } proofBytes := make([]byte, 0, 384+len(p.EvmProof.Proof)) proofBytes = append(proofBytes, p.EvmProof.Instances[:384]...) return append(proofBytes, p.EvmProof.Proof...) }coordinator/internal/logic/provertask/bundle_prover_task.go (1)
216-225: Public interface aligns with new data structures.Populating
BundleInfowithChainID,PrevStateRoot, andNumBatchesfollows the new schema introduced inmessage.OpenVMBundleInfo. This approach seems consistent and logical.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
common/types/message/message.go (1)
417-419:⚠️ Potential issueUpdate capacity and slice values to match updated accumulator count
The capacity in the
Proof()method is hardcoded to 384 bytes, but the comments suggest the structure has changed to include more accumulators (1472 bytes).func (p *OpenVMBundleProof) Proof() []byte { - proofBytes := make([]byte, 0, 384+len(p.EvmProof.Proof)) - proofBytes = append(proofBytes, p.EvmProof.Instances[:384]...) + if len(p.EvmProof.Instances) < 1472 { + panic("instances array is too short to safely slice 1472 bytes") + } + proofBytes := make([]byte, 0, 1472+len(p.EvmProof.Proof)) + proofBytes = append(proofBytes, p.EvmProof.Instances[:1472]...) return append(proofBytes, p.EvmProof.Proof...) }The implementation is still using 384 bytes for accumulators, but comments mention 1472 bytes (12 accumulators × 32 bytes = 384, but the logic seems to have changed). Update the capacity and slice values to match the expected size, and add a boundary check for safety.
🧹 Nitpick comments (2)
common/types/message/message.go (2)
50-68: Fix typo and simplify conditional logic in MarshalText methodThere's a typo in the comment and unnecessary conditional structure.
// 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 } + + 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 }
388-388: Add validation for MsgQueueHash fieldThe new
MsgQueueHashfield inOpenVMBundleInfois important for tracking message queue state but lacks validation.Consider adding validation for this field in the
SanityCheckmethod ofOpenVMBundleProofto ensure it's properly initialized.
📜 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)
common/types/message/message.go(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (5)
common/types/message/message.go (5)
14-14: Public export of fork constant is good for consistencyThe renaming of
euclidForktoEuclidForkmakes it publicly accessible, which is appropriate for a constant that may be referenced by other packages.
46-48: Add unit tests for new chunk fieldsThese new fields are important for tracking block hashes and message queue state, but need test coverage to ensure correctness.
As highlighted in a previous review, the newly added fields in
ChunkTaskDetaillack dedicated unit tests. Please add tests that verify these fields are correctly initialized, marshaled/unmarshaled, and consumed by downstream components.
95-101:✅ Verification successful
Ensure proper initialization for new Byte48 fields in BatchTaskDetail
The
KzgProofandKzgCommitmentfields now use the newByte48type, and a newChallengeDigestfield has been added.Verify that all code that initializes or accesses these fields correctly handles the type change:
🏁 Script executed:
#!/bin/bash # Find all locations accessing KzgProof or KzgCommitment rg -A 2 -B 2 "KzgProof|KzgCommitment" --type="go"Length of output: 1383
Byte48 Fields Initialization Verified
The newByte48fields inBatchTaskDetailare correctly updated and initialized. Our verification shows:
In
common/types/message/message.go(Lines 95-101):
TheKzgProofandKzgCommitmentfields now use the updatedByte48type, and the newChallengeDigestfield is defined as intended.In
coordinator/internal/logic/provertask/batch_prover_task.go:
The initialization ofKzgProofandKzgCommitmentis properly handled using the conversion tomessage.Byte48(viahexutil.Big(*new(big.Int).SetBytes(...))). Note that theChallengeDigestfield is currently omitted based on the existing FIXME comments, which appear to be intentional pending further handling.No additional issues were found with respect to the new type changes.
321-328:✅ Verification successful
Ensure message queue hash updates are properly propagated
The
OpenVMBatchInfostruct has been updated to track message queue state changes.Verify that the message queue hashes are properly computed and propagated throughout the system:
🏁 Script executed:
#!/bin/bash # Find where message queue hashes are computed or used rg -A 3 -B 3 "MsgQueueHash|PrevMsgQueueHash|PostMsgQueueHash" --type="go"Length of output: 4689
Message queue hash propagation verified and approved
The search confirms that the new message queue hash fields (
PrevMsgQueueHashandPostMsgQueueHash) are correctly computed in both the chunk and batch prover tasks and are consistently propagated through the code—as seen in the conversion from input values usingcommon.HexToHash. The note in the bundle prover task regarding the optionalMsgQueueHash(introduced in phase-2) aligns with the intended design, and the updated struct incommon/types/message/message.goproperly reflects these changes.
74-91:❓ Verification inconclusive
Add test coverage for Byte48 custom marshaling/unmarshaling
The
Byte48type provides custom JSON marshaling/unmarshaling to handle 384-bit values (48 bytes), which exceeds the standard hexutil.Big limit of 256 bits.Please ensure there are dedicated tests for the JSON serialization/deserialization of this new type to catch potential edge cases:
🏁 Script executed:
#!/bin/bash # Check if there are tests for the new Byte48 type rg -A 5 -B 5 "Byte48" --type="go" --type="test"Length of output: 72
Ensure Dedicated Tests for Byte48 JSON Marshaling/Unmarshaling Are Present
The custom JSON un/marshaling logic for the
Byte48type—which handles 384-bit values—is critical for correct behavior. However, our initial search for tests using the command:resulted in an error (
unrecognized file type: test), which suggests that the search approach did not properly locate related test files. To re-verify, please run the following updated command to search for tests in Go test files:Once executed, manually confirm that dedicated tests are implemented for:
- Valid JSON input containing a proper hex string representing exactly 48 bytes.
- Edge cases such as inputs with incorrect lengths or non-hex characters.
- Boundary conditions to catch any unexpected values.
If tests are missing or incomplete, please add them (for example, in a new file like
common/types/message/byte48_test.go). This test coverage is essential to ensure robustness of the custom JSON handling for theByte48type.
🏁 Script executed:
rg -A 5 -B 5 "Byte48" --type="go" --type="test"Length of output: 72
🏁 Script executed:
#!/bin/bash rg -A 5 -B 5 "Byte48" --glob "*_test.go"Length of output: 162
Action: Verify Test Coverage for
Byte48JSON Marshaling/UnmarshalingOur automated search using
rgdid not return any evidence of dedicated tests for theByte48type. This could mean that tests are missing or that our search parameters need manual confirmation. Please verify manually that:
- There is a test file (e.g.,
common/types/message/byte48_test.goor similar) covering the JSON serialization/deserialization forByte48.- Tests include scenarios for successful decoding of valid 48-byte hex strings.
- Tests cover error cases such as incorrect string lengths or invalid hex formats.
If such tests are not present, please add the necessary test cases to ensure robust coverage for these edge cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
zkvm-prover/Cargo.toml (1)
53-79: Addition of New Plonky3-GPU Dependencies
A comprehensive set of dependencies for Plonky3 (e.g.,p3-air,p3-field,p3-commit, etc.) have been added with uniform tagging ("v0.1.0") via SSH URLs. Verify that these new dependencies meet your compatibility requirements and that SSH usage aligns with your build/deployment pipelines. In particular, confirm that the deliberate exclusion of the "parallel" feature forp3-maybe-rayon(as noted in the inline comment) is the intended configuration for benchmarking purposes.
📜 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 (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
🔇 Additional comments (4)
zkvm-prover/Cargo.toml (4)
21-21: Dependency Version Upgrade forscroll-zkvm-prover
The dependency version forscroll-zkvm-proverhas been updated to"v0.1.0-rc.9", aligning with the overall release upgrade. Please ensure that any downstream references or integration points expecting the previous version are updated accordingly.
24-29: Updated Dependency Specifications for Proof Components
The updates forscroll-proving-sdk(using revision"02c2cac") andsbv-primitives(switched branch from the old euclid naming to"zkvm/euclid-upgrade") indicate a coordinated dependency upgrade. Please verify that these new versions are fully compatible with the existing codebase and that any breaking changes have been accounted for.
47-48: Minor Formatting Adjustment
The slight modification leading to an annotated change (line 48) does not affect functionality but appears to be a formatting or structural adjustment.
49-52: OpenVM Stark Dependencies Patch Confirmation
The new patch section for dependencies sourced fromhttps://github.com/openvm-org/stark-backend.gituses SSH URLs and specifies a tag of"v1.0.0-rc.2"with the GPU feature enabled. This is consistent with previous feedback on similar patches. Please ensure that using SSH URLs is intentional and that the build environment is prepared for the necessary authentication.
Purpose or design rationale of this PR
Upgrade to
v0.1.0-rc.10.PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit