feat: euclid prover and coordinator #1597
Conversation
WalkthroughThe pull request introduces substantial modifications to proof-related types, transitioning from concrete struct definitions to an interface-based design. It updates versioning, modifies proof verification by dereferencing pointers, and simplifies proof construction in tests. The changes also revise dependencies, configuration files, and the Rust toolchain. Several legacy modules and files related to configuration, API interactions, error handling, and task management have been removed, while the prover’s core flow has been restructured into an asynchronous model using a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package zstd: could not load export data: no export data for "github.com/scroll-tech/da-codec/encoding/zstd"" ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 #1597 +/- ##
===========================================
- Coverage 51.72% 42.23% -9.49%
===========================================
Files 157 222 +65
Lines 12555 17728 +5173
===========================================
+ Hits 6494 7488 +994
- Misses 5497 9518 +4021
- Partials 564 722 +158
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
coordinator/internal/logic/verifier/mock.go (1)
36-40: Check for nil pointer dereference.
Again, guard against a nil pointer. Consistently applying checks prevents panics in all verification methods.
🧹 Nitpick comments (7)
prover/src/main.rs (2)
28-30: Consider graceful shutdown for asynchronous main.Switching to
#[tokio::main]is a solid approach for running async code, though you might consider capturing signals (e.g., SIGINT) or providing a graceful shutdown mechanism to ensure system resources are released properly.
35-35: Unify logging for version output.Using
println!here works, but for consistency with other logs, considerlog::info!or an equivalent logging macro to keep log levels and outputs unified.- println!("version is {}", get_version()); + log::info!("version is {}", get_version());prover/src/zk_circuits_handler/euclid.rs (2)
5-11: Overall concurrency approach.Asynchronous trait usage (
#[async_trait]) withtokio::sync::Mutexshows a robust concurrency approach. Just ensure that the lock is not held for a prolonged time if these tasks involve heavy computation.
48-101: Comprehensive trait implementation for asynchronous proof generation.
- Lock usage: Repeated calls to
self.try_lock().unwrap()signals potential panic if resources are locked. Consider usinglock().awaitif concurrency might block.- Dynamic input parsing: Rely on
serde_json::from_streffectively; ensure robust error messaging to highlight malformed requests.- Future expansions: If new
ProofTypevariants appear, an exhaustive pattern match keeps the code clean but re-check for theunreachable!blocks in production scenarios.prover/src/prover.rs (1)
49-55: Consider multi-task concurrency support.
Currently, the design stores only oneJoinHandleincurrent_task, so new tasks could overwrite or conflict with an existing one. If supporting multiple simultaneous proofs is desired, implement a queue or pool of tasks and track them accordingly.prover/src/zk_circuits_handler.rs (1)
9-11: Consider using a Result instead of Option for get_vk.
At the moment, havingNonehides the reason for an unavailable verification key. If there’s any chance of an error, returningResult<Vec<u8>>can convey more context.prover/Cargo.toml (1)
28-34: New Proving-Related Dependencies AddedThe additions of
scroll-proving-sdk,scroll-zkvm-prover, andsbv-primitivesclearly support the new Euclid prover functionality. Ensure that the pinned revision (rev = "3331117") and the tag forscroll-zkvm-prover("v0.1.0-rc.1") are aligned with the expected feature set. Also, consider adding brief inline documentation or commit notes regarding the use of the custom patch branch (omerfirmak-patch-1) forsbv-primitivesfor future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
common/types/message/message.go(2 hunks)common/version/version.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(0 hunks)coordinator/internal/logic/verifier/mock.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(3 hunks)coordinator/test/mock_prover.go(1 hunks)prover/Cargo.toml(3 hunks)prover/config.json(1 hunks)prover/rust-toolchain(1 hunks)prover/src/config.rs(0 hunks)prover/src/coordinator_client.rs(0 hunks)prover/src/coordinator_client/api.rs(0 hunks)prover/src/coordinator_client/errors.rs(0 hunks)prover/src/coordinator_client/listener.rs(0 hunks)prover/src/coordinator_client/types.rs(0 hunks)prover/src/geth_client.rs(0 hunks)prover/src/key_signer.rs(0 hunks)prover/src/main.rs(2 hunks)prover/src/prover.rs(1 hunks)prover/src/task_cache.rs(0 hunks)prover/src/task_processor.rs(0 hunks)prover/src/types.rs(1 hunks)prover/src/utils.rs(0 hunks)prover/src/version.rs(0 hunks)prover/src/zk_circuits_handler.rs(1 hunks)prover/src/zk_circuits_handler/common.rs(0 hunks)prover/src/zk_circuits_handler/darwin.rs(0 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(0 hunks)prover/src/zk_circuits_handler/euclid.rs(1 hunks)
💤 Files with no reviewable changes (16)
- prover/src/coordinator_client/listener.rs
- coordinator/internal/logic/provertask/batch_prover_task.go
- prover/src/zk_circuits_handler/common.rs
- prover/src/coordinator_client.rs
- prover/src/task_processor.rs
- prover/src/utils.rs
- prover/src/coordinator_client/errors.rs
- prover/src/geth_client.rs
- prover/src/key_signer.rs
- prover/src/coordinator_client/types.rs
- prover/src/task_cache.rs
- prover/src/coordinator_client/api.rs
- prover/src/version.rs
- prover/src/config.rs
- prover/src/zk_circuits_handler/darwin.rs
- prover/src/zk_circuits_handler/darwin_v2.rs
✅ Files skipped from review due to trivial changes (2)
- prover/rust-toolchain
- common/version/version.go
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: fmt
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (18)
prover/src/main.rs (3)
6-10: Imports look appropriate.The newly introduced imports for
LocalProver,LocalProverConfig, and thescroll_proving_sdkalign with the updated proving architecture.
39-42: Initialization looks good.Configuration loading and prover creation are handled cleanly, leveraging the
?operator to propagate errors. This should be reliable as long asfrom_fileand builder logic are well-tested.
44-44: Verify error handling fromprover.run().
prover.run().awaitdoes not appear to return or propagate any potential errors. If it can fail, consider capturing its result. Otherwise, confirm that no error scenario is being silently ignored.common/types/message/message.go (1)
4-4: Import ofencoding/jsonapproved.This change is necessary to handle raw JSON data for proofs. No concerns here.
prover/src/zk_circuits_handler/euclid.rs (5)
1-2: Imports for concurrency and path usage.The use of
Arc,Mutex, and filesystemPathindicates concurrency readiness and filesystem-based workspace handling. Ensure that concurrent access to file paths remains safe across multiple tasks.
3-4: Proper layering and error handling.Bringing in
anyhowaligns with the rest of the system’s error-handling pattern. The usage ofCircuitsHandlerfrom the super module is consistent with an extensible design.
12-16:EuclidHandlerstruct for specialized proving tasks.Bundling
ChunkProver,BatchProver, andBundleProverin a single handler is clear, but be mindful of future expansions if more provers are introduced.
18-19:unsafe impl Sendusage.Declaring
Sendmakes sense if the underlying fields are truly thread-safe. Carefully verify no external synchronization is missing.
20-46: Constructor logic for multiple provers.Approach to set up each prover with
Path::joinis straightforward. Confirm that each.vmexeand.tomlfile path is correct, especially in production or containerized environments.prover/src/prover.rs (1)
1-22: Imports and dependencies look good.
No immediate concerns. They are consistent with the shift to the new asynchronous architecture and use ofanyhowfor error handling.prover/src/types.rs (1)
3-3: LGTM! Good use of SDK type.Using
CircuitTypefromscroll_proving_sdkinstead of custom enums reduces code duplication and improves maintainability.coordinator/internal/logic/verifier/verifier.go (1)
116-116: LGTM! Consistent proof validation approach.The change to use
string(*proof)across all verification methods maintains consistency with the new JSON-based proof representation.Also applies to: 143-143, 170-170
coordinator/test/mock_prover.go (1)
226-227: LGTM! Simplified proof construction in tests.The change to use byte slices directly for invalid test proofs aligns well with the new JSON-based proof representation while maintaining test coverage.
Also applies to: 232-233
prover/config.json (3)
2-3: Verify key management security.The transition from
keystore_pathandkeystore_passwordtokeys_dirrepresents a significant change in key management. Please ensure proper access controls and security measures are in place for the new key storage approach.
13-28: LGTM! Well-structured circuit configuration.The new
proversection with circuit types and versioning improves configuration organization and maintainability. The separation of low and high version circuits provides good flexibility for version management.
4-9: Verify timeout and retry configurations.The coordinator and l2geth connection settings should be reviewed to ensure they are appropriate for production environments:
- 30-second connection timeout might be too long
- 10-second retry wait time might need adjustment based on production load
Also applies to: 10-12
✅ Verification successful
🌐 Web query:
What are the recommended connection timeout and retry settings for production Go services?Length of output: 5436
Adjust Production Timeout and Retry Configurations
The current coordinator settings in
prover/config.jsonuse a 30‑second connection timeout and a fixed 10‑second retry wait. Based on production best practices for Go services, these values should be reviewed:
- Connection Timeout: Rather than a fixed 30‑second timeout, consider configuring the timeout relative to the network’s RTT (e.g., RTT × 3) or using lower values (often in the 5‑10 second range for HTTP server timeouts) to help detect issues faster.
- Retry Strategy: Rather than a fixed 10‑second delay between retries, production setups benefit from an exponential backoff with jitter; this adjustment can better accommodate varying load and transient network issues.
- Scope: These recommendations apply to both the coordinator settings shown (lines 4–9) and the similar settings on lines 10–12 for l2geth.
Review and adjust these configurations so they align with your production environment’s performance requirements and error recovery strategies.
prover/Cargo.toml (2)
10-17: Patch Dependencies: Confirm Branch Usage and MSRV SupportThe patches for
ruint,alloy-primitives, and the variousrevmcrates are now referenced from specific git branches. This is a good way to ensure you’re using the patched versions with the new MSRV (1.77) support. Please verify that these branch references remain stable and document the rationale (e.g., addressing rkyv support) if not already captured elsewhere.
46-52: Utility and Asynchronous Dependency EnhancementsThe addition of
async-trait,url, andserde_bytesis appropriate given the move towards an asynchronous prover model and enhanced data handling. Please confirm that these dependency versions are fully compatible with the rest of the codebase. Additionally, it might be beneficial to re-check if there are newer patch versions available forasync-traitthat provide any important improvements or fixes.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
from
0c6e954 to
c591328
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
common/types/message/message.go (3)
143-149: Improve proof length validation error messages.The error messages for proof length validation could be more descriptive. Consider adding the expected length multiple to the error message.
Apply this diff:
- return fmt.Errorf("proof buffer length must be a multiple of 32, got: %d", len(ap.RawProof)) + return fmt.Errorf("proof buffer length must be a multiple of 32 bytes (got: %d bytes)", len(ap.RawProof))Also applies to: 202-204
101-103: Consider validating proofs before returning.The
Proof()methods simply return raw bytes without any validation. Consider validating the proof before returning to catch issues early.Apply this diff to add validation:
func (ap *Halo2ChunkProof) Proof() []byte { + if len(ap.RawProof) == 0 { + return nil + } + if len(ap.RawProof)%32 != 0 { + return nil + } return ap.RawProof }Apply similar changes to
Halo2BatchProof.Proof()andHalo2BundleProof.Proof().Also applies to: 133-135, 188-190
148-148: Address TODO comment.The TODO comment indicates that
NewBundleProofshould be used. This should be implemented to ensure consistent proof creation.Would you like me to help implement this change?
rollup/tests/rollup_test.go (1)
147-151: Document test data structure.The test uses hardcoded byte arrays for proof data. Consider documenting the structure of these test proofs or using helper functions to generate meaningful test data.
Apply this diff:
+// mockProofBytes returns a 32-byte array for testing +func mockProofBytes() []byte { + return bytes.Repeat([]byte{0, 1, 2, 3}, 8) // 32 bytes +} batchProof := &message.Halo2BatchProof{ - RawProof: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, - Instances: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, - Vk: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, + RawProof: mockProofBytes(), + Instances: mockProofBytes(), + Vk: mockProofBytes(), }Also applies to: 162-166
rollup/internal/orm/bundle.go (1)
137-153: Improve error handling in GetVerifiedProofByHash.The error handling could be more specific and include context about the failure.
Apply this diff:
func (o *Bundle) GetVerifiedProofByHash(ctx context.Context, hash string) (message.BundleProof, error) { + if hash == "" { + return nil, fmt.Errorf("GetVerifiedProofByHash: empty hash provided") + } + db := o.db.WithContext(ctx) db = db.Model(&Bundle{}) db = db.Select("proof") db = db.Where("hash = ? AND proving_status = ?", hash, types.ProvingTaskVerified) var bundle Bundle if err := db.Find(&bundle).Error; err != nil { return nil, fmt.Errorf("Bundle.GetVerifiedProofByHash error: %w, bundle hash: %v", err, hash) } + if bundle.Proof == nil { + return nil, fmt.Errorf("Bundle.GetVerifiedProofByHash: no proof found for hash: %v", hash) + } var proof message.BundleProof if err := json.Unmarshal(bundle.Proof, &proof); err != nil { - return nil, fmt.Errorf("Bundle.GetVerifiedProofByHash error: %w, bundle hash: %v", err, hash) + return nil, fmt.Errorf("Bundle.GetVerifiedProofByHash: failed to unmarshal proof: %w, bundle hash: %v", err, hash) } return proof, nil }coordinator/internal/logic/provertask/bundle_prover_task.go (1)
224-231: Improve error handling in proof unmarshaling.The error handling during proof unmarshaling could be more robust and include validation of the unmarshaled proof.
Apply this diff:
var batchProofs []message.BatchProof for _, batch := range batches { - proof := message.NewBatchProof(hardForkName) + proof, err := message.NewBatchProof(hardForkName) + if err != nil { + return nil, fmt.Errorf("failed to create batch proof: %w, bundle hash: %v, batch hash: %v", err, task.TaskID, batch.Hash) + } + if encodeErr := json.Unmarshal(batch.Proof, &proof); encodeErr != nil { return nil, fmt.Errorf("failed to unmarshal proof: %w, bundle hash: %v, batch hash: %v", encodeErr, task.TaskID, batch.Hash) } + + if err := proof.SanityCheck(); err != nil { + return nil, fmt.Errorf("proof validation failed: %w, bundle hash: %v, batch hash: %v", err, task.TaskID, batch.Hash) + } + batchProofs = append(batchProofs, proof) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
common/types/message/message.go(4 hunks)coordinator/cmd/tool/tool.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(3 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(1 hunks)coordinator/internal/logic/submitproof/proof_receiver.go(2 hunks)coordinator/internal/logic/verifier/mock.go(2 hunks)coordinator/internal/logic/verifier/verifier.go(3 hunks)coordinator/internal/logic/verifier/verifier_test.go(2 hunks)coordinator/internal/orm/chunk.go(0 hunks)coordinator/internal/types/prover.go(1 hunks)coordinator/test/api_test.go(1 hunks)coordinator/test/mock_prover.go(2 hunks)rollup/internal/controller/relayer/l2_relayer.go(2 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(1 hunks)rollup/internal/orm/batch.go(3 hunks)rollup/internal/orm/bundle.go(3 hunks)rollup/internal/orm/orm_test.go(4 hunks)rollup/tests/rollup_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- coordinator/internal/orm/chunk.go
🚧 Files skipped from review as they are similar to previous changes (3)
- coordinator/test/mock_prover.go
- coordinator/internal/logic/verifier/verifier.go
- coordinator/internal/logic/provertask/batch_prover_task.go
🧰 Additional context used
🪛 GitHub Check: tests
coordinator/internal/logic/verifier/mock.go
[failure] 27-27:
syntax error: unexpected ., expected name
[failure] 35-35:
syntax error: unexpected ., expected name
🪛 GitHub Actions: Integration
coordinator/internal/logic/verifier/mock.go
[error] 27-27: syntax error: unexpected ., expected name
🪛 GitHub Actions: Coordinator
coordinator/internal/logic/verifier/mock.go
[error] 27-27: expected 'IDENT', found '.'
🪛 golangci-lint (1.62.2)
coordinator/internal/logic/submitproof/proof_receiver.go
178-178: m.verifier.VerifyChunkProof undefined (type *verifier.Verifier has no field or method VerifyChunkProof)
(typecheck)
184-184: m.verifier.VerifyBatchProof undefined (type *verifier.Verifier has no field or method VerifyBatchProof)
(typecheck)
190-190: m.verifier.VerifyBundleProof undefined (type *verifier.Verifier has no field or method VerifyBundleProof)
(typecheck)
🪛 GitHub Actions: Common
common/types/message/message.go
[error] 80-80: string darwinV2 has 3 occurrences, make it a constant (goconst)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: clippy
- GitHub Check: tests
🔇 Additional comments (14)
coordinator/internal/logic/verifier/mock.go (1)
19-24: LGTM! Interface-based proof verification.The transition from concrete struct pointers to interfaces improves flexibility and polymorphism in proof handling. The implementation correctly uses the
Proof()method from the interface.Also applies to: 27-32, 35-40
coordinator/internal/types/prover.go (1)
9-17: LGTM! Well-structured status code implementation.The implementation follows Go best practices:
- Uses iota for enum-like constants
- Clear documentation for type and constants
- Appropriate use of uint32 for status codes
coordinator/internal/logic/verifier/verifier_test.go (1)
68-78: LGTM! Consistent interface implementation in tests.The changes correctly:
- Return interface types (types.BatchProof, types.ChunkProof)
- Use concrete implementations (Halo2BatchProof, Halo2ChunkProof)
- Maintain proper error handling during unmarshaling
Also applies to: 80-90
coordinator/cmd/tool/tool.go (2)
67-67: Address the TODO comment about NewBatchProof.The TODO comment suggests that a factory method for creating BatchProof instances might be needed. Consider implementing this to ensure consistent object creation.
Would you like me to help implement the
NewBatchProoffactory method?
65-73: LGTM! Consistent proof handling.The changes correctly transition from pointer slices to value slices, aligning with the broader codebase changes for proof handling.
coordinator/internal/logic/submitproof/proof_receiver.go (2)
174-174: LGTM! Using constructor functions improves initialization consistency.The use of constructor functions (
NewChunkProof,NewBatchProof,NewBundleProof) ensures proper initialization of proof objects with the correct hard fork name.Also applies to: 180-180, 186-186
268-268: LGTM! Using typed status improves type safety.The change to use
coordinatorType.StatusOkinstead of a raw status value improves type safety and maintainability.rollup/internal/orm/batch.go (1)
125-125: LGTM! Consistent use of value types for proofs.The changes to use value types instead of pointers for
BatchProofin bothGetVerifiedProofByHashandUpdateProofByHashimprove consistency with the new proof interface design.Also applies to: 437-437
rollup/internal/controller/relayer/l2_relayer_test.go (1)
148-150: LGTM! Updated proof structure in tests.The test has been correctly updated to use
Halo2BundleProofwith theRawProoffield, aligning with the new proof structure.rollup/internal/orm/orm_test.go (1)
463-465: LGTM! Tests updated to use new proof interface.The tests have been correctly updated to:
- Use
Halo2BundleProofwithRawProoffield- Use the
Proof()method instead of direct field access- Handle proof unmarshaling consistently with the new structure
Also applies to: 474-474, 486-488, 499-502
coordinator/test/api_test.go (1)
597-597: LGTM! Aligns with the interface-based proof type design.The change from marshaling a
ChunkProoftoHalo2ChunkProofaligns with the broader refactor to use interface-based proof types.rollup/internal/controller/relayer/l2_relayer.go (3)
612-612: LGTM! Improved type safety with interface-based design.Changed
aggProoffrom pointer to value type as part of the transition to interface-based proof types.
886-886: LGTM! Updated function signature for interface-based design.The function signature now accepts a value type
message.BundleProofinterface instead of a pointer.
893-893: LGTM! Using interface method instead of direct field access.Changed from dereferencing the
Prooffield to calling theProof()method, aligning with the interface-based design.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
6 times, most recently
from
f1bd75c to
08b89b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
common/types/message/message.go (2)
109-115:⚠️ Potential issueHandle unknown hard fork names in NewBatchProof.
Same issue as
NewChunkProof: returns a default implementation for any hard fork name.Apply similar error handling pattern as suggested for
NewChunkProof.
164-170:⚠️ Potential issueHandle unknown hard fork names in NewBundleProof.
Same issue as
NewChunkProofandNewBatchProof: returns a default implementation for any hard fork name.Apply similar error handling pattern as suggested for
NewChunkProof.coordinator/internal/logic/submitproof/proof_receiver.go (1)
174-178:⚠️ Potential issueMissing verifier method implementation.
The
VerifyChunkProofmethod is undefined in the*verifier.Verifiertype. This will cause runtime errors.🧰 Tools
🪛 golangci-lint (1.62.2)
178-178: m.verifier.VerifyChunkProof undefined (type *verifier.Verifier has no field or method VerifyChunkProof)
(typecheck)
🧹 Nitpick comments (6)
common/types/message/message.go (1)
133-156: Improve error messages in SanityCheck.The error messages could be more descriptive to help with debugging.
Apply this diff to improve error messages:
func (ap *Halo2BatchProof) SanityCheck() error { if ap == nil { - return errors.New("agg_proof is nil") + return errors.New("batch proof is nil") } if len(ap.RawProof) == 0 { - return errors.New("proof not ready") + return errors.New("batch proof data is empty") } if len(ap.RawProof)%32 != 0 { return fmt.Errorf("proof buffer length must be a multiple of 32, got: %d", len(ap.RawProof)) } if len(ap.Instances) == 0 { - return errors.New("instance not ready") + return errors.New("batch proof instances are empty") } if len(ap.Vk) == 0 { - return errors.New("vk not ready") + return errors.New("batch proof verification key is empty") } return nil }rollup/tests/rollup_test.go (1)
147-151: Add test cases for proof validation.The test initializes proofs with fixed byte arrays but doesn't validate the proof format or content.
Add test cases to verify:
- Proof length is multiple of 32
- SanityCheck passes
- Invalid proof handling
coordinator/test/mock_prover.go (3)
204-207: Consider using constants for status values.The status values are hardcoded. Consider using constants from the types package for better maintainability.
212-221: Add test cases for different proof types.The mock implementation creates empty proofs. Consider adding test cases with different proof contents to test error handling.
226-239: Improve test coverage for invalid proofs.The test only checks one invalid proof case. Consider adding more test cases for different types of invalid proofs.
prover/config.json (1)
2-29: Configuration improvements enhance security and flexibility.The changes:
- Remove sensitive keystore-related fields
- Add circuit configuration for better flexibility
- Improve organization of settings
Consider documenting the available circuit types and versions in a README file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
common/types/message/message.go(4 hunks)common/version/version.go(1 hunks)coordinator/cmd/tool/tool.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(3 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(1 hunks)coordinator/internal/logic/submitproof/proof_receiver.go(2 hunks)coordinator/internal/logic/verifier/mock.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(3 hunks)coordinator/internal/logic/verifier/verifier_test.go(2 hunks)coordinator/internal/orm/chunk.go(0 hunks)coordinator/internal/types/prover.go(1 hunks)coordinator/test/api_test.go(1 hunks)coordinator/test/mock_prover.go(2 hunks)prover/Cargo.toml(3 hunks)prover/config.json(1 hunks)prover/rust-toolchain(1 hunks)prover/src/config.rs(0 hunks)prover/src/coordinator_client.rs(0 hunks)prover/src/coordinator_client/api.rs(0 hunks)prover/src/coordinator_client/errors.rs(0 hunks)prover/src/coordinator_client/listener.rs(0 hunks)prover/src/coordinator_client/types.rs(0 hunks)prover/src/geth_client.rs(0 hunks)prover/src/key_signer.rs(0 hunks)prover/src/main.rs(2 hunks)prover/src/prover.rs(1 hunks)prover/src/task_cache.rs(0 hunks)prover/src/task_processor.rs(0 hunks)prover/src/types.rs(1 hunks)prover/src/utils.rs(0 hunks)prover/src/version.rs(0 hunks)prover/src/zk_circuits_handler.rs(1 hunks)prover/src/zk_circuits_handler/common.rs(0 hunks)prover/src/zk_circuits_handler/darwin.rs(0 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(0 hunks)prover/src/zk_circuits_handler/euclid.rs(1 hunks)rollup/internal/controller/relayer/l2_relayer.go(2 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(2 hunks)rollup/internal/orm/batch.go(3 hunks)rollup/internal/orm/bundle.go(3 hunks)rollup/internal/orm/orm_test.go(4 hunks)rollup/tests/rollup_test.go(2 hunks)
💤 Files with no reviewable changes (16)
- prover/src/coordinator_client/listener.rs
- prover/src/utils.rs
- coordinator/internal/orm/chunk.go
- prover/src/coordinator_client/errors.rs
- prover/src/coordinator_client.rs
- prover/src/version.rs
- prover/src/zk_circuits_handler/darwin.rs
- prover/src/zk_circuits_handler/common.rs
- prover/src/geth_client.rs
- prover/src/task_processor.rs
- prover/src/key_signer.rs
- prover/src/task_cache.rs
- prover/src/coordinator_client/api.rs
- prover/src/config.rs
- prover/src/coordinator_client/types.rs
- prover/src/zk_circuits_handler/darwin_v2.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- prover/rust-toolchain
- coordinator/internal/types/prover.go
- common/version/version.go
- coordinator/internal/logic/provertask/bundle_prover_task.go
- coordinator/cmd/tool/tool.go
- rollup/internal/orm/orm_test.go
- coordinator/internal/logic/verifier/verifier.go
- rollup/internal/controller/relayer/l2_relayer.go
- coordinator/internal/logic/provertask/batch_prover_task.go
- coordinator/internal/logic/verifier/mock.go
- coordinator/test/api_test.go
- coordinator/internal/logic/verifier/verifier_test.go
- rollup/internal/orm/bundle.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
coordinator/internal/logic/submitproof/proof_receiver.go
178-178: m.verifier.VerifyChunkProof undefined (type *verifier.Verifier has no field or method VerifyChunkProof)
(typecheck)
184-184: m.verifier.VerifyBatchProof undefined (type *verifier.Verifier has no field or method VerifyBatchProof)
(typecheck)
190-190: m.verifier.VerifyBundleProof undefined (type *verifier.Verifier has no field or method VerifyBundleProof)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: clippy
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (16)
prover/src/zk_circuits_handler.rs (1)
1-12: LGTM! Well-structured trait definition.The trait is well-defined with appropriate async methods and thread safety bounds.
prover/src/main.rs (1)
28-46: LGTM! Clean async implementation.The transition to async/await with Tokio is well-implemented, and the builder pattern usage improves code readability.
prover/src/types.rs (1)
8-8: Verify CircuitType compatibility with existing code.The transition from TaskType to CircuitType needs verification to ensure compatibility with existing code.
#!/bin/bash # Search for any direct usage of CircuitType to verify the transition rg -A 3 "CircuitType::"Also applies to: 18-18
common/types/message/message.go (1)
77-83:⚠️ Potential issueHandle unknown hard fork names in NewChunkProof.
The factory method returns a default
Halo2ChunkProoffor any hard fork name, which could lead to runtime errors if an unsupported hard fork is provided.Apply this diff to improve error handling:
-func NewChunkProof(hardForkName string) ChunkProof { +func NewChunkProof(hardForkName string) (ChunkProof, error) { switch hardForkName { + case "darwinV2": + return &Halo2ChunkProof{}, nil default: - return &Halo2ChunkProof{} + return nil, fmt.Errorf("unsupported hard fork name: %s", hardForkName) } }Likely invalid or redundant comment.
prover/src/prover.rs (1)
182-183:⚠️ Potential issueHandle unwrap safely in set_active_handler.
Using
unwrap()oncircuits.get()could cause panics.Apply this diff to handle errors safely:
- let config = self.config.circuits.get(hard_fork_name).unwrap(); + let config = match self.config.circuits.get(hard_fork_name) { + Some(cfg) => cfg, + None => panic!("Unknown hard fork name: {}", hard_fork_name), + };Likely invalid or redundant comment.
rollup/tests/rollup_test.go (1)
162-166: Add test cases for bundle proof validation.Similar to BatchProof, the test lacks validation of the bundle proof format and content.
Add similar validation test cases as suggested for BatchProof.
coordinator/internal/logic/submitproof/proof_receiver.go (1)
268-268: LGTM!The status check has been correctly updated to use
coordinatorType.StatusOk.rollup/internal/orm/batch.go (1)
125-141: LGTM!The changes improve the method by:
- Adding
hardForkNameparameter for better flexibility- Using value types instead of pointers for safer memory management
- Using the factory function for proof initialization
rollup/internal/controller/relayer/l2_relayer_test.go (2)
126-130: LGTM!The test setup has been improved by adding chunk insertion before batch insertion, ensuring proper data dependencies.
154-158: LGTM!The proof type and field names have been correctly updated to align with the new proof type system.
prover/Cargo.toml (6)
10-16: Enhanced Patch Dependencies Update.
The[patch.crates-io]block now introduces updated dependencies (e.g.,ruint,alloy-primitives, and severalrevm-*crates) along with a clarifying comment about adding rkyv support and MSRV 1.77. Consider verifying that the git-branch references remain stable and, if possible, pin to specific commits or tags to ensure reproducible builds.
28-30: Addition of scroll-proving-sdk Dependency.
The new dependencyscroll-proving-sdkis added with a fixed revision (3331117) and includes the"openvm"feature. Please confirm that this specific revision is stable and that all relevant code is updated to adopt the SDK's API.
31-34: New ZKVM and SBV-Primitives Dependencies.
scroll-zkvm-proveris now included (tagged asv0.1.0-rc.1), andsbv-primitivesis added from branchomerfirmak-patch-1with the"scroll"feature. Ensure these additions align with the overall architecture and that the chosen branch and tag meet your release requirements.
46-46: Introduction of async-trait Dependency.
The addition ofasync-trait = "0.1"supports the new asynchronous architecture in the prover module. Confirm that this version is compatible with your async workflows and that there are no conflicts with other async components.
51-52: Inclusion of URL and serde_bytes Dependencies.
The new dependenciesurl = "2.5.4"andserde_bytes = "0.11.15"appear to support enhanced URL handling and binary serialization. Verify that these additions meet your security, performance, and compatibility requirements throughout the codebase.
1-53: Overall Dependency Consistency Review.
TheCargo.tomlmodifications are well aligned with the restructured prover architecture and the move toward improved modularity. As these dependencies are now sourced directly from git repositories and specific branches or revisions, please ensure that all changes are thoroughly documented in your release notes/changelog and that versioning remains consistent for reproducibility and security.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
rollup/tests/rollup_test.go (1)
63-67: Update validation logic to handle all codec versions.The current validation logic only handles CodecV4 and fails for other versions, which contradicts the addition of CodecV5 and CodecV6 to the test.
Apply this diff to handle all codec versions:
-if codecVersion == encoding.CodecV4 { +switch codecVersion { +case encoding.CodecV4: chainConfig = ¶ms.ChainConfig{LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64)} -} else { +case encoding.CodecV5: + chainConfig = ¶ms.ChainConfig{LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64)} +case encoding.CodecV6: + chainConfig = ¶ms.ChainConfig{LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64)} +default: assert.Fail(t, "unsupported codec version, expected CodecV4") }
🧹 Nitpick comments (10)
rollup/internal/controller/watcher/batch_proposer.go (3)
247-250: LGTM! Consider enhancing the error message.The change to use
CodecFromVersionis a good improvement. The error handling is now more comprehensive by checking bothnilcodec and error cases.Consider making the error message more specific to version:
- return fmt.Errorf("failed to retrieve codec for block number %v and time %v: %w", firstUnbatchedChunk.StartBlockNumber, firstUnbatchedChunk.StartBlockTime, err) + return fmt.Errorf("failed to retrieve codec version %v for block number %v and time %v: %w", firstUnbatchedChunk.CodecVersion, firstUnbatchedChunk.StartBlockNumber, firstUnbatchedChunk.StartBlockTime, err)
280-333: Consider refactoring batch processing logic for better readability.The batch processing logic is complex and handles multiple constraints. Consider extracting the limit checking logic into a separate method for better maintainability.
+func (p *BatchProposer) exceedsBatchLimits(metrics *utils.BatchMetrics) (bool, error) { + totalOverEstimateL1CommitGas := uint64(p.gasCostIncreaseMultiplier * float64(metrics.L1CommitGas)) + return metrics.L1CommitCalldataSize > p.maxL1CommitCalldataSizePerBatch || + totalOverEstimateL1CommitGas > p.maxL1CommitGasPerBatch || + metrics.L1CommitBlobSize > maxBlobSize || + metrics.L1CommitUncompressedBatchBytesSize > p.maxUncompressedBatchBytesSize, + nil +}Then use it in the main loop:
- if metrics.L1CommitCalldataSize > p.maxL1CommitCalldataSizePerBatch || totalOverEstimateL1CommitGas > p.maxL1CommitGasPerBatch || - metrics.L1CommitBlobSize > maxBlobSize || metrics.L1CommitUncompressedBatchBytesSize > p.maxUncompressedBatchBytesSize { + exceedsLimits, err := p.exceedsBatchLimits(metrics) + if err != nil { + return fmt.Errorf("failed to check batch limits: %w", err) + } + if exceedsLimits {
374-388: Consider consolidating metric recording functions to reduce duplication.The
recordTimerBatchMetricsfunction's logic is completely contained withinrecordAllBatchMetrics, which suggests unnecessary duplication.Consider refactoring to eliminate duplication:
func (p *BatchProposer) recordAllBatchMetrics(metrics *utils.BatchMetrics) { p.totalL1CommitGas.Set(float64(metrics.L1CommitGas)) p.totalL1CommitCalldataSize.Set(float64(metrics.L1CommitCalldataSize)) p.batchChunksNum.Set(float64(metrics.NumChunks)) p.totalL1CommitBlobSize.Set(float64(metrics.L1CommitBlobSize)) - p.batchEstimateGasTime.Set(float64(metrics.EstimateGasTime)) - p.batchEstimateCalldataSizeTime.Set(float64(metrics.EstimateCalldataSizeTime)) - p.batchEstimateBlobSizeTime.Set(float64(metrics.EstimateBlobSizeTime)) + p.recordTimerBatchMetrics(metrics) }rollup/internal/controller/watcher/bundle_proposer.go (2)
182-187: Consider updating the log message for CodecV5.The current log message doesn't distinguish between reaching the maximum due to
CodecV5versus the regularmaxBatchesThisBundlelimit. This could make debugging more difficult.- log.Info("reached maximum number of batches per bundle", "batch count", len(batches), "start batch index", batches[0].Index, "end batch index", batches[len(batches)-1].Index) + log.Info("reached maximum number of batches per bundle", "batch count", len(batches), "start batch index", batches[0].Index, "end batch index", batches[len(batches)-1].Index, "codec_version", codecVersion)
197-199: Consider updating the debug message for clarity.The debug message could be more informative by including the current batch count and the maximum allowed.
- log.Debug("pending batches are not enough and do not contain a timeout batch") + log.Debug("pending batches are not enough and do not contain a timeout batch", "current_batch_count", len(batches), "max_allowed", maxBatchesThisBundle)rollup/internal/controller/watcher/chunk_proposer.go (3)
297-299: Ensure proper error handling in 'proposeChunk' methodIn the
proposeChunkmethod, the conditionif proposed, err := p.tryProposeEuclidTransitionChunk(blocks); proposed || err != nil { return err }may lead to returningnilwhenproposedistrueanderrisnil. For better clarity and maintainability, consider handlingproposedanderrseparately to make the code more readable.Suggested change:
+ proposed, err := p.tryProposeEuclidTransitionChunk(blocks) + if err != nil { + return err + } + if proposed { + return nil + }
401-403: Improve error message when parent block retrieval failsWhen
prevBlocks[0].Header.Hash()does not matchblocks[0].Header.ParentHash, the error message"failed to get parent block"may not accurately describe the issue. Consider updating the error message to reflect a potential parent hash mismatch.Suggested change:
- return false, fmt.Errorf("failed to get parent block: %w", err) + return false, fmt.Errorf("parent hash mismatch or failed to get parent block: %w", err)
412-413: Consider using dynamic codec versionCurrently, the
codecVersionis hard-coded toencoding.CodecV5. For future compatibility and maintainability, consider determining the codec version dynamically based on the block's properties, similar to other parts of the code.Suggested change:
- codecVersion := encoding.CodecV5 + codecVersion := encoding.GetCodecVersion(p.chainCfg, blocks[0].Header.Number.Uint64(), blocks[0].Header.Time)database/go.mod (2)
26-26: Addition of Indirect Dependency:github.com/kr/pretty.
A new indirect dependency has been added. Please confirm that this package is truly needed (or will be used transitively) and does not bloat the dependency graph.
31-31: Inclusion ofgithub.com/rogpeppe/go-internal.
This indirect dependency appears to support internal tooling or additional functionality. Verify if this was introduced intentionally and whether its usage is consistent across the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcoordinator/go.sumis excluded by!**/*.sumdatabase/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
Makefile(1 hunks)bridge-history-api/go.mod(3 hunks)common/go.mod(10 hunks)coordinator/go.mod(4 hunks)database/go.mod(2 hunks)rollup/go.mod(4 hunks)rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/sender/sender.go(2 hunks)rollup/internal/controller/watcher/batch_proposer.go(1 hunks)rollup/internal/controller/watcher/bundle_proposer.go(1 hunks)rollup/internal/controller/watcher/chunk_proposer.go(2 hunks)rollup/internal/controller/watcher/l1_watcher.go(2 hunks)rollup/internal/controller/watcher/l2_watcher.go(1 hunks)rollup/tests/rollup_test.go(3 hunks)tests/integration-test/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/internal/controller/relayer/l2_relayer.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: compile
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (46)
Makefile (1)
3-3: Update L2GETH_TAG Version TagThe updated value (
L2GETH_TAG=scroll-v5.8.4) is consistent with the changes made in the corresponding module files (go.mod) and should ensure that all components pull the correct version ofgo-ethereum. Please ensure that this update is also reflected in any related documentation or release notes if needed.rollup/internal/controller/watcher/batch_proposer.go (1)
252-254: LGTM! Version validation is clear and effective.The version validation ensures compatibility by checking against the minimum required version.
rollup/internal/controller/watcher/l2_watcher.go (1)
133-136: Document the Euclid time exception and verify downstream handling.The change to allow nil
RowConsumptionfor Euclid blocks aligns with the PR objectives. However, please:
- Add a comment explaining why Euclid blocks are allowed to have nil
RowConsumption- Verify that downstream code properly handles nil
RowConsumptionfor Euclid blocksLet's verify the downstream handling of nil
RowConsumption:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check how nil RowConsumption is handled downstream # Search for RowConsumption usage rg -A 5 'RowConsumption' # Search for IsEuclid usage to understand the special case rg -A 5 'IsEuclid'Length of output: 30995
Document and Verify Downstream Handling of Nil RowConsumption in Euclid Blocks
The change allowing nil
RowConsumptionfor blocks with a Euclid timestamp appears to be working as intended. The downstream code—particularly in the ORM layer—handles nil values correctly (e.g., via JSON marshaling, which converts a nil to"null"), and the special check inl2_watcher.goprevents non‐Euclid blocks from proceeding without aRowConsumptionvalue.Please add an inline comment describing why Euclid blocks are allowed to have nil
RowConsumptionso that future maintainers understand this exception. Additionally, ensure that any downstream expectations of a non-nilRowConsumptionare adequately documented or adapted, though the current usage (as seen in the ORM and chunk proposer tests) suggests that handling nil values is safe.rollup/internal/controller/watcher/bundle_proposer.go (1)
164-167: Verify the impact of limiting CodecV5 bundles to a single batch.The code enforces a strict limit of one batch per bundle when using
CodecV5. While this change aligns with the architectural changes mentioned in the PR objectives, we should verify:
- The performance impact of processing single-batch bundles
- Whether this limitation is temporary or permanent
- If there are any downstream systems that assume bundles can contain multiple batches
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any dependencies on multi-batch bundles rg -A 5 "len\(.*bundle.*batch.*\)" --type goLength of output: 46
🏁 Script executed:
#!/bin/bash # Let's gather more context on the usage of 'CodecV5' and 'maxBatchesThisBundle' to verify any dependencies on multiple batches. echo "Searching for references to CodecV5 in .go files:" rg -n "CodecV5" --type go echo -e "\nSearching for references to maxBatchesThisBundle in .go files:" rg -n "maxBatchesThisBundle" --type goLength of output: 1546
CodecV5 Single Batch Handling Confirmed
The search results show that the
maxBatchesThisBundlevariable is only used within theBundleProposercode and there are no external dependencies assuming multiple batches for CodecV5. Limiting CodecV5 bundles to a single batch appears intentional, with no evidence of downstream usage issues.
- The only reference to adjusting batch count for CodecV5 is localized within
rollup/internal/controller/watcher/bundle_proposer.go.- Other files and tests (e.g., in the rollup test suite) simply select among codec versions without imposing multi-batch semantics for CodecV5.
rollup/internal/controller/watcher/l1_watcher.go (2)
83-83: LGTM!The
CalcBlobFeecall has been correctly updated to use the new package location.
9-9: Verify the package reorganization in go-ethereum.The import path has been updated from
eip4844tomisc, reflecting a package reorganization in go-ethereum.✅ Verification successful
🌐 Web query:
Has the EIP-4844 functionality in go-ethereum been moved from eip4844 to misc package?💡 Result:
Based on the provided search results, there is no direct mention of EIP-4844 functionality being moved from an `eip4844` package to a `misc` package in go-ethereum. However, some relevant technical details were found: 1. **op-geth fork changes** (Search Result 6) show modifications to Ethereum's execution layer client implementation, including: - Engine API extensions for rollup-specific block building - Transaction pool adjustments for L1 cost accounting - Dynamic EIP-1559 parameter support 2. **Core implementation details** (Search Result 4) specify that EIP-4844 modifications span both: - Consensus layer (beacon node implementations) - Execution layer (geth client modifications) The search results don't explicitly confirm package restructuring between `eip4844` and `misc`, but they demonstrate that EIP-4844 implementation affects multiple components across Ethereum's stack. For definitive code structure confirmation, you'd need to consult: 1. The latest go-ethereum GitHub repository 2. Official Ethereum client documentation 3. Core developer changelogs No matching evidence was found in the provided sources about this specific package migration[6][4]. Citations: - 1: https://magazine.mindplex.ai/understanding-eip-4844-ethereums-proto-danksharding-upgrade-for-layer-2s/ - 2: https://www.quicknode.com/guides/ethereum-development/transactions/eip4844-explained - 3: https://blog.coinshares.com/understanding-eip-4844-how-it-greatly-reduces-transaction-fees-for-ethereum-layer-2s-9cc106ffc077?gi=cc5ae08d0c83 - 4: https://www.eip4844.com - 5: https://nownodes.io/blog/ethereum-dencun-upgrade-eip-4844-proto-danksharding/ - 6: https://op-geth.optimism.io - 7: https://www.galaxy.com/insights/research/ethereum-150-days-after-dencun/ - 8: https://pkg.go.dev/github.com/jumbochain/jumbochain-parlia-go - 9: https://metlabs.io/en/eip-4844-ethereum-scalability/ - 10: https://blockworks.co/news/ethereum-improvement-proposal-4844
🏁 Script executed:
#!/bin/bash echo "Searching for references to 'go-ethereum/consensus/eip4844'..." rg -n "go-ethereum/consensus/eip4844" || echo "No references found for eip4844." echo "Searching for references to 'go-ethereum/consensus/misc'..." rg -n "go-ethereum/consensus/misc" || echo "No references found for misc."Length of output: 627
EIP-4844 Package Reorganization Verified
- The repository no longer contains any references to the
eip4844package.- Both
rollup/internal/controller/watcher/l1_watcher.goandrollup/internal/controller/sender/sender.gonow import from themiscpackage, confirming the apparent reorganization.rollup/internal/controller/sender/sender.go (2)
15-15: LGTM!The import path update is consistent with the package reorganization.
678-678: LGTM!The
CalcBlobFeecall has been correctly updated to use the new package location.rollup/internal/controller/watcher/chunk_proposer.go (1)
395-422: Add unit tests for 'tryProposeEuclidTransitionChunk'The new method
tryProposeEuclidTransitionChunkis crucial for handling the Euclid transition. Ensure that it is adequately covered by unit tests, including edge cases such as the genesis block handling and transitions between different phases.Would you like assistance in creating unit tests for this function or opening a new issue to track this task?
database/go.mod (3)
3-5: Update Go Version and Toolchain Specification.
The Go version is now upgraded to 1.22 and a toolchain (go1.22.2) has been specified. Please make sure that your build/CI environment is compatible with Go 1.22 and the selected toolchain.
11-12: Upgrade Core Dependencies.
The versions forgithub.com/scroll-tech/go-ethereumandgithub.com/stretchr/testifyhave been updated. Verify that these newer versions do not introduce breaking changes or affect any runtime assumptions in your code.
37-41: Upgrade of Golang.org/x Dependencies.
The versions forgolang.org/x/crypto,golang.org/x/sync,golang.org/x/sys, andgolang.org/x/texthave been updated. Ensure these upgrades are backward compatible with your codebase and that no subtle API changes affect your application.tests/integration-test/go.mod (3)
3-5: Update Go Version and Toolchain Specification.
The module now targets Go 1.22 and specifies the toolchaingo1.22.2. This update should be validated against your integration tests to ensure no compatibility issues arise.
8-10: Upgrade Main Dependencies.
The dependencies forgithub.com/scroll-tech/da-codec,github.com/scroll-tech/go-ethereum, andgithub.com/stretchr/testifyhave been updated. Double-check that these updates work seamlessly within the integration-test suite.
15-47: Review Indirect Dependencies Upgrade.
A number of indirect packages—including updates for libraries likebitset,bavard,gnark-crypto,crate-crypto, and others—have been refreshed. Ensure that these changes do not lead to transitive issues with your integration tests.coordinator/go.mod (4)
3-5: Update Go Version and Toolchain in Coordinator Module.
The Go version has been bumped to 1.22 with the toolchaingo1.22.2. Verify that this change is consistently adopted in CI and that no dependent tooling is disrupted.
14-17: Upgrade Direct Dependencies.
Key dependencies likegithub.com/scroll-tech/da-codec,github.com/scroll-tech/go-ethereum, andgithub.com/stretchr/testifyhave their versions updated. Please double-check integration points in the coordinator logic to confirm these updates are fully compatible.
45-46: Update Golang.org/x Dependencies.
The updates togolang.org/x/net(v0.23.0) andgolang.org/x/text(v0.21.0) should be validated for any API or behavioral changes.
51-84: Review Indirect Dependencies for Consistency.
This block updates many indirect dependencies (e.g.bits-and-blooms/bitset,bavard,gnark-crypto,crate-crypto,holiman/uint256, and others). Ensure that these library versions are aligned with your overall dependency management strategy.bridge-history-api/go.mod (5)
3-6: Update Go Version and Toolchain.
The module now uses Go 1.22 with the specified toolchaingo1.22.2. Please ensure that any associated scripts or CI configurations reflect this change.
13-14: Upgrade Core Dependencies.
The updated versions forgithub.com/scroll-tech/go-ethereumandgithub.com/stretchr/testifyshould be verified against the API contracts used in the bridge-history-api module.
16-16: Updategolang.org/x/sync.
The dependency togolang.org/x/syncis upgraded to v0.11.0. Validate that this improvement does not alter expected synchronization behavior.
21-47: Review Indirect Dependencies (Block 1).
This section upgrades various indirect dependencies (for example,fastcache,bitset,gnark-crypto, etc.). Confirm that these new versions are compatible and solve any known issues without introducing new problems.
82-124: Review Indirect Dependencies (Block 2).
A broad array of indirect dependencies have been updated, including changes to Prometheus libraries, several golang.org/x libraries, and packages likenpipeandurfave/cli. Ensure these versions are in-sync with other modules across the project.rollup/go.mod (3)
3-5: Update Go Version and Toolchain in Rollup Module.
The file now specifies Go 1.22 with the toolchaingo1.22.2. Confirm that all build and deployment scripts reflect this change uniformly across modules.
7-23: Upgrade Direct Dependencies in Rollup Module.
Several direct dependencies have been updated:
github.com/consensys/gnark-crypto→ v0.16.0github.com/crate-crypto/go-kzg-4844→ v1.1.0github.com/holiman/uint256→ v1.3.2da-codecandgo-ethereumnow match the updated versions from other modules.testifyis now at v1.10.0.Please verify that these version bumps integrate well with the rollup logic and do not cause compatibility issues.
25-131: Review Comprehensive Indirect Dependencies.
This extensive list of indirect dependencies has been updated with many version bumps. Ensure that every updated package (including libraries likeprometheus/tsdb, variousgolang.org/x/*packages, and helper packages such asnpipeandurfave/cli.v1) is consistent across the codebase and that no conflicts arise.common/go.mod (18)
3-5: Go Version and Toolchain UpgradeThe module now requires Go 1.22 with an explicit toolchain set to go1.22.2. Ensure that all downstream dependencies and CI/CD tooling support this updated version.
9-9: Dependency Update: BitsetUpgraded
github.com/bits-and-blooms/bitsetto v1.20.0. Please verify that no breaking API changes affect its usage downstream.
18-19: Dependency Updates: go-ethereum and TestifyUpdated:
github.com/scroll-tech/go-ethereumto v1.10.14-0.20250205135740-4bdf6d096c38github.com/stretchr/testifyto v1.10.0Please confirm these updates are compatible with existing code and that no testing regressions occur.
35-35: Dependency Update: FastcacheUpgraded
github.com/VictoriaMetrics/fastcacheto v1.12.2. It is recommended to review the release notes to ensure that caching behavior remains consistent.
58-58: Dependency Update: Consensys BavardBumped
github.com/consensys/bavardto v0.1.29. Please check that the updated version integrates smoothly with the proof system components.
59-59: Dependency Update: Gnark-CryptoUpgraded
github.com/consensys/gnark-cryptoto v0.16.0. Given the changes in cryptographic functionality, ensure that all proof verification and construction flows are thoroughly tested.
66-67: Dependency Updates: go-md2man and go-kzg-4844Updated:
github.com/cpuguy83/go-md2man/v2to v2.0.4github.com/crate-crypto/go-kzg-4844to v1.1.0Verify that documentation generation (if using go-md2man) and any cryptographic operations relying on go-kzg-4844 remain unbroken.
69-69: Dependency Update: Golang-SetUpgraded
github.com/deckarep/golang-setto v0.0.0-20180603214616-504e848d77ea. Review its usage to ensure that no deprecated or breaking APIs are introduced.
80-80: Dependency Update: mmap-goBumped
github.com/edsrzf/mmap-goto v1.0.0. Please ensure that any memory-mapped file operations in the code continue to behave as expected.
182-182: New Indirect Dependency: Prometheus TSDBAdded
github.com/prometheus/tsdbv0.7.1 as an indirect dependency. Confirm that its inclusion does not pull unwanted transitive dependencies.
184-184: Dependency Update: NotifyUpgraded
github.com/rjeczalik/notifyto v0.9.1. Please verify that file-system event handling remains stable.
187-187: Dependency Update: da-codecBumped
github.com/scroll-tech/da-codecto v0.1.3-0.20241218102542-9852fa4e1be5. Ensure that any encoding/decoding logic utilizing this package is compatible with the new version.
197-197: Dependency Update: CobraUpdated
github.com/spf13/cobrato v1.8.1. It’s important to confirm that command-line interface behaviors and flag parsing remain consistent post-update.
201-201: Dependency Update: BLSTBumped
github.com/supranational/blstto v0.3.13. Given its role in cryptography, ensure that all related operations are validated after the update.
206-206: Dependency Update: numcpusUpgraded
github.com/tklauser/numcpusto v0.9.0. Please double-check its behavior in concurrency-related parts of the code.
238-238: Dependency Update: golang.org/x/cryptoUpdated
golang.org/x/cryptoto v0.32.0. Verify that all cryptographic primitives and functions perform as expected under this new version.
243-246: Dependency Updates: x/sync, x/sys, x/term, and x/textUpgraded the following packages:
golang.org/x/syncto v0.11.0golang.org/x/systo v0.30.0golang.org/x/termto v0.28.0golang.org/x/textto v0.21.0Ensure that these updates do not affect low-level system calls, terminal interactions, or text processing functionalities.
256-257: Dependency Updates: npipe and urfave/cliReplaced
gopkg.in/natefinch/lumberjack.v2withgopkg.in/natefinch/npipe.v2(v2.0.0-20160621034901-c1b8fa8bdcce) and updatedgopkg.in/urfave/cli.v1to v1.20.0. Please confirm that the new npipe implementation satisfies the expected pipe utilities and that CLI functionalities remain robust.rollup/tests/rollup_test.go (1)
162-166: Verify the Halo2BundleProof structure matches the interface contract.The code has been updated to use the new
Halo2BundleProoftype. Ensure that this type correctly implements theBundleProofinterface as mentioned in the AI summary.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that Halo2BundleProof implements the BundleProof interface ast-grep --pattern $'type Halo2BundleProof struct { $$$ } func (p *Halo2BundleProof) Proof() []byte { $$$ }'Length of output: 114
🏁 Script executed:
#!/bin/bash # This script gathers information on the BundleProof interface and the Halo2BundleProof implementation echo "Searching for the BundleProof interface definition:" rg "type BundleProof interface" -n echo "--------------" echo "Searching for the Halo2BundleProof struct definition:" rg "type Halo2BundleProof struct" -n echo "--------------" echo "Searching for the Proof() method on Halo2BundleProof:" rg "func (p \\*Halo2BundleProof) Proof() \\[\\]byte" -nLength of output: 683
🏁 Script executed:
#!/bin/bash # Let's search for any method with (p *Halo2BundleProof) in the codebase to check for any potential Proof method implementation. rg "func (p \\*Halo2BundleProof)" -n echo "--------------" # Additionally, let's inspect the context in common/types/message/message.go around the Halo2BundleProof struct definition. sed -n '160,220p' common/types/message/message.goLength of output: 1347
Halo2BundleProof implements the BundleProof interface correctly.
Both required methods,Proof() []byteandSanityCheck() error, are implemented in theHalo2BundleProoftype as defined incommon/types/message/message.go.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
2 times, most recently
from
ec6ebcd to
1a879fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
common/types/message/message_test.go (2)
13-13: Improve error message formatting.The error messages could be more descriptive. Include the file path in the error message for better debugging.
- t.Fatalf("failed to read batch proof sample.json: %v", err) + t.Fatalf("failed to read batch proof sample file 'batch-proof-sample.json': %v", err) - t.Fatalf("failed to read bundle proof sample.json: %v", err) + t.Fatalf("failed to read bundle proof file 'bundle-proof-sample.json': %v", err)Also applies to: 28-28
17-23: Add test cases for error scenarios.The test only covers the happy path. Consider adding test cases for:
- Invalid JSON data
- Missing required fields
- Invalid proof data format
Also applies to: 32-38
rollup/tests/rollup_test.go (1)
142-146: Add test cases for proof verification.The test initializes proofs with static data but doesn't verify the proof content. Consider adding test cases to verify:
- Proof data integrity
- Verification of proof signatures
- Handling of invalid proofs
Also applies to: 157-161
common/types/message/message.go (1)
81-81: Extract repeated string literal into a constant.The string literal "euclid" is used multiple times. Extract it into a constant to improve maintainability.
+// HardForkEuclid represents the Euclid hard fork name +const HardForkEuclid = "euclid" - case "euclid": + case HardForkEuclid:Also applies to: 115-115, 172-172
🧰 Tools
🪛 GitHub Actions: Common
[error] 81-81: string
euclidhas 3 occurrences, make it a constant (goconst)rollup/internal/controller/watcher/chunk_proposer.go (2)
412-412: Use constant for codec version.The codec version is hardcoded to
CodecV5. Consider using a constant or configuration value.+const EuclidTransitionCodecVersion = encoding.CodecV5 - codecVersion := encoding.CodecV5 + codecVersion := EuclidTransitionCodecVersion
395-422: Add metrics for Euclid transition chunks.The new Euclid transition functionality lacks metric tracking. Consider adding metrics to monitor:
- Number of transition chunks proposed
- Success/failure rates
- Transition timing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcoordinator/go.sumis excluded by!**/*.sumdatabase/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
Makefile(1 hunks)bridge-history-api/go.mod(3 hunks)common/go.mod(10 hunks)common/types/message/bundle-proof-sample.json(1 hunks)common/types/message/message.go(6 hunks)common/types/message/message_test.go(1 hunks)coordinator/go.mod(4 hunks)database/go.mod(2 hunks)rollup/go.mod(4 hunks)rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/sender/sender.go(2 hunks)rollup/internal/controller/watcher/batch_proposer.go(1 hunks)rollup/internal/controller/watcher/bundle_proposer.go(1 hunks)rollup/internal/controller/watcher/chunk_proposer.go(2 hunks)rollup/internal/controller/watcher/l1_watcher.go(2 hunks)rollup/internal/controller/watcher/l2_watcher.go(1 hunks)rollup/tests/rollup_test.go(4 hunks)tests/integration-test/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/types/message/bundle-proof-sample.json
🚧 Files skipped from review as they are similar to previous changes (11)
- rollup/internal/controller/watcher/bundle_proposer.go
- rollup/internal/controller/watcher/l1_watcher.go
- rollup/internal/controller/watcher/l2_watcher.go
- Makefile
- rollup/internal/controller/watcher/batch_proposer.go
- rollup/internal/controller/relayer/l2_relayer.go
- database/go.mod
- rollup/internal/controller/sender/sender.go
- coordinator/go.mod
- bridge-history-api/go.mod
- common/go.mod
🧰 Additional context used
🪛 GitHub Actions: Common
common/types/message/message.go
[error] 81-81: string euclid has 3 occurrences, make it a constant (goconst)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: compile
- GitHub Check: tests
🔇 Additional comments (9)
common/types/message/message_test.go (1)
11-14: Verify test data file availability.The test depends on external JSON files (
batch-proof-sample.jsonandbundle-proof-sample.json). Ensure these files are committed to the repository and document their location.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the required JSON files exist in the repository fd -t f "batch-proof-sample.json" "bundle-proof-sample.json"Length of output: 170
🏁 Script executed:
#!/bin/bash # Verify that the required JSON test data files exist in the repository. # This script individually searches for each file starting from the repository root. for file in "batch-proof-sample.json" "bundle-proof-sample.json"; do echo "Checking for: $file" result=$(fd -t f "$file" .) if [ -n "$result" ]; then echo "Found file '$file' at:" echo "$result" else echo "File '$file' not found in the repository." fi doneLength of output: 1014
Test data files located in
common/types/messageare available and committed.
- Both
batch-proof-sample.jsonandbundle-proof-sample.jsonexist in the repository at./common/types/message.- Update the test documentation if necessary to indicate these file locations.
rollup/tests/rollup_test.go (1)
56-56: Inconsistency between added codec versions and validation logic.The test adds support for CodecV5 and CodecV6 in the
codecVersionsslice, but the validation logic only handles CodecV4.Either:
- Update the validation logic to handle CodecV5 and CodecV6, or
- Remove CodecV5 and CodecV6 from the
codecVersionsslice.-codecVersions := []encoding.CodecVersion{encoding.CodecV4, encoding.CodecV5, encoding.CodecV6} +codecVersions := []encoding.CodecVersion{encoding.CodecV4}rollup/internal/controller/watcher/chunk_proposer.go (1)
400-403:⚠️ Potential issuePrevent potential underflow when accessing previous block.
Subtracting 1 from
blocks[0].Header.Number.Uint64()may cause an underflow ifblocks[0]is the genesis block.+ if blocks[0].Header.Number.Uint64() == 0 { + // Genesis block, no previous block + return false, fmt.Errorf("cannot get parent block for genesis block") + } prevBlocks, err := p.l2BlockOrm.GetL2BlocksGEHeight(p.ctx, blocks[0].Header.Number.Uint64()-1, 1)Likely invalid or redundant comment.
tests/integration-test/go.mod (3)
3-5: Go Version and Toolchain UpdateThe Go version has been bumped to 1.22 and the toolchain is explicitly set to go1.22.2. This ensures consistency with your dependency requirements and aligns with the overall upgrade strategy.
8-12: Core Dependency Updates VerificationThe primary require block reflects updates for key dependencies:
•github.com/scroll-tech/da-codecis now on v0.1.3-0.20241218102542-9852fa4e1be5.
•github.com/scroll-tech/go-ethereumis updated as expected.
•github.com/stretchr/testifyhas been bumped to v1.10.0.These changes are in line with the broader project updates. Please verify that any local tests using these dependencies are passing.
15-47: Indirect Dependencies AlignmentThe indirect dependency block has been updated across numerous packages (e.g.,
bitset,bavard,gnark-crypto,go-kzg-4844,uint256,go-iden3-crypto, etc.). These updates help maintain compatibility and security across the project.
It is recommended to double-check that no version conflicts arise with these updates across other modules.rollup/go.mod (3)
3-5: Go Version and Toolchain UpdateSimilar to the integration-test module, the rollup module now targets Go 1.22 with a specified toolchain (go1.22.2). This update promotes consistency across the repository.
7-23: Direct Dependency Version BumpsThe first require block in rollup/go.mod shows several dependencies updated as follows:
•github.com/consensys/gnark-cryptois updated to v0.16.0.
•github.com/crate-crypto/go-kzg-4844has been bumped to v1.1.0.
•github.com/holiman/uint256now uses v1.3.2.
•github.com/scroll-tech/da-codecandgithub.com/scroll-tech/go-ethereummatch the intended update.
•github.com/stretchr/testifynow appears in its v1.10.0 release.These updates are consistent with changes in other modules. Ensure that any module-level integration tests involving these dependencies run smoothly.
25-131: Indirect Dependencies Comprehensive UpdateThe second require block updates a large number of indirect dependencies. Notable updates include:
•github.com/bits-and-blooms/bitsetto v1.20.0.
•github.com/deckarep/golang-setand several other packages now reflect the updated versions.
• Updates forgolang.org/x/crypto,golang.org/x/sync,golang.org/x/sys, andgolang.org/x/textalso seem aligned with the overall upgrade to Go 1.22.This thorough refresh should improve compatibility, performance, and security. It is advisable to run the full test suite to ensure that the dependency updates do not introduce any breaking changes.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
4 times, most recently
from
b3b0fea to
b3d62dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
rollup/tests/rollup_test.go (2)
55-55: Function name should reflect its purpose.The function name
testCommitBatchAndFinalizeBundleCodecV4V5V6suggests support for CodecV4, V5, and V6, but the test only uses CodecV4 and CodecV5. Consider renaming it to accurately reflect the codecs being tested.-func testCommitBatchAndFinalizeBundleCodecV4V5V6(t *testing.T) { +func testCommitBatchAndFinalizeBundle(t *testing.T) {
142-146: Use constants for mock proof data.The hardcoded byte arrays for proof data should be extracted into constants for better maintainability and reusability.
+const ( + mockProofBytes = []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31} +) batchProof := &message.Halo2BatchProof{ - RawProof: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, - Instances: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, - Vk: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, + RawProof: mockProofBytes, + Instances: mockProofBytes, + Vk: mockProofBytes,rollup/internal/controller/watcher/chunk_proposer.go (1)
395-422: Add metrics for Euclid transition chunks.The
tryProposeEuclidTransitionChunkfunction handles an important transition but lacks metrics tracking. Consider adding metrics to monitor transition chunk proposals.+ euclidTransitionChunkTotal prometheus.Counter + euclidTransitionChunkFailureTotal prometheus.Counter func NewChunkProposer(ctx context.Context, cfg *config.ChunkProposerConfig, minCodecVersion encoding.CodecVersion, chainCfg *params.ChainConfig, db *gorm.DB, reg prometheus.Registerer) *ChunkProposer { + p.euclidTransitionChunkTotal = promauto.With(reg).NewCounter(prometheus.CounterOpts{ + Name: "rollup_propose_euclid_transition_chunk_total", + Help: "Total number of Euclid transition chunk proposals.", + }) + p.euclidTransitionChunkFailureTotal = promauto.With(reg).NewCounter(prometheus.CounterOpts{ + Name: "rollup_propose_euclid_transition_chunk_failure_total", + Help: "Total number of failed Euclid transition chunk proposals.", + }) func (p *ChunkProposer) tryProposeEuclidTransitionChunk(blocks []*encoding.Block) (bool, error) { + p.euclidTransitionChunkTotal.Inc() if !p.chainCfg.IsEuclid(blocks[0].Header.Time) { return false, nil } // ... rest of the function if err := p.updateDBChunkInfo(&chunk, codecVersion, metrics); err != nil { + p.euclidTransitionChunkFailureTotal.Inc() return false, err } return true, nil }rollup/internal/controller/sender/sender_test.go (1)
291-292: Improve transaction hash tracking.The code now tracks multiple transaction hashes but could benefit from a more structured approach. Consider using a map to track transaction status and avoid duplicate checks.
+ type txStatus struct { + hash common.Hash + checked bool + pending bool + } + txStatuses := make(map[common.Hash]*txStatus) + for _, hash := range txHashes { + if status, exists := txStatuses[hash]; !exists || !status.checked { + _, isPending, err := s.client.TransactionByHash(context.Background(), hash) + txStatuses[hash] = &txStatus{ + hash: hash, + checked: true, + pending: err == nil && isPending, + } + if err == nil && !isPending { + return true + } + } + }Also applies to: 299-300
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcoordinator/go.sumis excluded by!**/*.sumdatabase/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
Makefile(1 hunks)bridge-history-api/go.mod(3 hunks)common/go.mod(10 hunks)common/types/message/bundle-proof-sample.json(1 hunks)common/types/message/message.go(7 hunks)common/types/message/message_test.go(1 hunks)coordinator/go.mod(4 hunks)database/go.mod(2 hunks)rollup/go.mod(4 hunks)rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/sender/sender.go(2 hunks)rollup/internal/controller/sender/sender_test.go(1 hunks)rollup/internal/controller/watcher/batch_proposer.go(1 hunks)rollup/internal/controller/watcher/bundle_proposer.go(1 hunks)rollup/internal/controller/watcher/chunk_proposer.go(2 hunks)rollup/internal/controller/watcher/l1_watcher.go(2 hunks)rollup/internal/controller/watcher/l2_watcher.go(1 hunks)rollup/tests/bridge_test.go(1 hunks)rollup/tests/rollup_test.go(1 hunks)tests/integration-test/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- rollup/tests/bridge_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- Makefile
- rollup/internal/controller/watcher/bundle_proposer.go
- common/types/message/message_test.go
- common/types/message/bundle-proof-sample.json
- rollup/internal/controller/watcher/batch_proposer.go
- rollup/internal/controller/watcher/l1_watcher.go
- rollup/internal/controller/watcher/l2_watcher.go
- database/go.mod
- rollup/internal/controller/sender/sender.go
- rollup/internal/controller/relayer/l2_relayer.go
- rollup/go.mod
- bridge-history-api/go.mod
- coordinator/go.mod
- common/go.mod
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (16)
rollup/internal/controller/watcher/chunk_proposer.go (1)
400-403:⚠️ Potential issuePrevent potential underflow when accessing previous block.
Subtracting 1 from
blocks[0].Header.Number.Uint64()may cause an underflow ifblocks[0]is the genesis block (block number 0), resulting in an invalid block height when retrieving the previous block.Apply this diff to handle the genesis block case:
+ if blocks[0].Header.Number.Uint64() == 0 { + // Genesis block, no previous block + return false, nil + } prevBlocks, err := p.l2BlockOrm.GetL2BlocksGEHeight(p.ctx, blocks[0].Header.Number.Uint64()-1, 1)Likely invalid or redundant comment.
tests/integration-test/go.mod (15)
3-5: Go Version and Toolchain UpdateThe go.mod now specifies Go 1.22 with an explicit toolchain (go1.22.2). This ensures that the module builds with the expected language features and behaviors in Go 1.22. Please confirm that all integration tests run correctly under this version.
8-10: First Require Block Dependency UpdatesThe dependency versions for "github.com/scroll-tech/da-codec", "github.com/scroll-tech/go-ethereum", and "github.com/stretchr/testify" have been updated. Ensure that any changes in these libraries’ APIs (especially in test helpers for testify) have been accounted for in the integration tests.
15-15: Updated Bitset Dependency"Github.com/bits-and-blooms/bitset" is now updated to v1.20.0. Review its changelog for any performance improvements or API modifications that might affect parts of the code relying on bitset operations.
17-17: Updated Bavard DependencyUpgrading "github.com/consensys/bavard" to v0.1.29 should bring minor improvements or fixes. Please verify that modules using this library continue to function as expected.
18-18: Updated Gnark-Crypto DependencyThe update of "github.com/consensys/gnark-crypto" to v0.16.0 may include important cryptographic enhancements and security fixes. It is advisable to run the relevant cryptographic tests to ensure no regressions occur.
19-19: Updated Go-KZG-4844 Dependency"Github.com/crate-crypto/go-kzg-4844" is updated to v1.1.0. Check if the update delivers bug fixes or performance improvements and that its usage in proof or verification routines isn’t adversely affected.
27-27: Updated Uint256 Dependency"Github.com/holiman/uint256" has been upgraded to v1.3.2. This update likely brings improved functionality for handling large integers. Please verify any arithmetic-dependent code paths.
28-28: Updated Iden3-Crypto DependencyThe dependency "github.com/iden3/go-iden3-crypto" is updated to v0.0.17. It is recommended to review the changelog for any API changes that could affect cryptographic or identity verification functions.
34-34: Updated Notify DependencyUpgrading "github.com/rjeczalik/notify" to v0.9.1 should enhance file system event monitoring. Please verify that any file-watching or event-related functionality still performs as expected.
37-37: Updated Supranational BLST DependencyThe dependency "github.com/supranational/blst" has been updated to v0.3.13. As this library is critical for cryptographic operations, ensure that its integration (e.g., in signature aggregation) is fully covered by tests.
39-39: Updated Numcpus Dependency"Github.com/tklauser/numcpus" is now updated to v0.9.0. This could affect concurrency behavior by providing a more accurate count of CPUs. It is recommended to ascertain that the changes do not adversely impact performance-sensitive code.
41-41: Updated golang.org/x/crypto DependencyThe update to "golang.org/x/crypto" v0.32.0 is important given the sensitivity around cryptographic functionality. Please ensure that all cryptography-related tests are re-run to confirm compatibility.
42-42: Updated golang.org/x/sync DependencyUpdating "golang.org/x/sync" to v0.11.0 should provide improved synchronization primitives. Confirm that concurrent operations within the integration suite are functioning correctly.
43-43: Updated golang.org/x/sys DependencyThe upgrade of "golang.org/x/sys" to v0.30.0 brings updates to system call interactions. It is advisable to verify that system-level behaviors, especially in cross-platform testing, continue to operate as expected.
44-44: Added Natefinch Npipe DependencyThe new indirect dependency "gopkg.in/natefinch/npipe.v2" at v2.0.0-20160621034901-c1b8fa8bdcce indicates a requirement for named pipe support, likely in Windows environments. Verify its necessity in the integration tests and add documentation if its purpose is not immediately obvious.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
common/libzkp/impl/src/verifier.rs (4)
51-54: Be mindful of global environment variable side effects.
Setting SCROLL_PROVER_ASSETS_DIR to config.halo2_circuit.assets_path will affect the entire process. Ensure that multiple inits or different configurations won't conflict and that external consumers are aware of this side effect.
64-70: Improve error handling in load_params usage.
Lines 64 and 70 panic on failure to load parameters. Consider user-friendly error handling or graceful fallback to avoid abrupt termination if the params file is missing or invalid.
79-82: Confirm that DarwinV2Verifier is strictly a Halo2 verifier.
You're instantiating DarwinV2Verifier and storing it in VERIFIER_HALO2. If Darwin V2 is indeed specific to Halo2, the naming is consistent. Otherwise, consider clarifying the name to better convey its role.
108-114: Consider supporting multiple forks more dynamically.
Lines 108–114 match the requested fork_name against the stored verifiers. If additional forks are introduced, you might want a more extensible mechanism. For now, this logic is straightforward and functional.common/libzkp/impl/src/verifier/euclid.rs (2)
16-18: Clarify or resolve the TODO regarding EVM parameter management.
This TODO suggests a potential refactor or consolidation of parameter management solely within the bundle verifier. Please confirm if you plan to revise this section to avoid duplicate or unnecessary parameter usage.Would you like assistance drafting a proposal for consolidating these parameters so only the bundle verifier needs them?
19-19: Consider removing or utilizing _params_map.
Currently, this parameter isn’t used, which may lead to confusion. Either remove it if truly unnecessary or utilize it for configuration.
📜 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 (5)
common/libzkp/impl/Cargo.toml(1 hunks)common/libzkp/impl/rust-toolchain(1 hunks)common/libzkp/impl/src/verifier.rs(4 hunks)common/libzkp/impl/src/verifier/darwin.rs(0 hunks)common/libzkp/impl/src/verifier/euclid.rs(1 hunks)
💤 Files with no reviewable changes (1)
- common/libzkp/impl/src/verifier/darwin.rs
✅ Files skipped from review due to trivial changes (1)
- common/libzkp/impl/rust-toolchain
🔇 Additional comments (15)
common/libzkp/impl/Cargo.toml (6)
15-15: Added ethers-signers Dependency
The addition of the ethers-signers dependency nicely complements the ethers-core and ethers-providers already referenced. Please verify that the branch "v2.0.7" remains compatible with the rest of the ethers libraries in the project.
17-22: New Zero-Knowledge Proof & EVM Dependencies
The inclusion of new dependencies (ruint, alloy-primitives, revm, revm-interpreter, revm-precompile, and revm-primitives) appears essential for supporting the new Euclid features. Ensure that pinning to specific branches (e.g., "v1.12.3" for ruint and "scroll-evm-executor/v50" for the revm-related crates) does not introduce compatibility issues later.
25-25: Updated halo2_proofs Patch Dependency
The patch dependency for halo2_proofs now points to branch "v1.1". Confirm that this update addresses any previous version mismatches and integrates well with other halo2-related dependencies across the project.
33-37: Reformatted snark-verifier-sdk Declaration
The dependency declaration for snark-verifier-sdk has been reformatted for better readability with features listed on separate lines. Please double-check that all enabled features ("loader_halo2", "loader_evm", "halo2-pse") are indeed required for the intended verifier functionality.
40-43: Updated prover_v5 Dependency Declaration
The reformatted prover_v5 declaration clearly lists its features ("parallel_syn", "scroll") under the "darwin_v2" context. Verify that this configuration remains compatible with the existing circuits and that any adjustments serve the overall build expectation.
45-46: New Euclid Prover & Verifier Dependencies Added
The introduction of prover_v7 and verifier_v7 (both tagged as "v0.1.0-rc.1") is a significant change to support the new Euclid feature. Ensure thorough integration testing, document any potential breaking changes, and confirm that these new dependencies integrate smoothly with the existing architecture.common/libzkp/impl/src/verifier.rs (9)
2-6: Consider documenting and testing the new Euclid module.
You've introduced "mod euclid" (line 2) and are now using the "EuclidVerifier" (line 6). Please ensure that the new module has sufficient documentation, examples, and test coverage to verify its functionality and maintainability.
30-34: Validate the necessity of OpenVMConfig fields.
The newly added OpenVMConfig struct (lines 30–34) only contains fork_name and workspace_path. Ensure no additional configuration fields (e.g., params_path or assets_path) are needed for the zkVM circuit.
38-39: New fields in VerifierConfig appear consistent.
You've added halo2_circuit and zkvm_circuit to VerifierConfig. The naming convention is straightforward, and each field references a dedicated config struct. This approach looks coherent.
46-47: Revisit the safety of static mutable usage with OnceCell.
Declaring VERIFIER_ZKVM and VERIFIER_HALO2 as mutable statics can introduce concurrency pitfalls if not carefully managed. Although OnceCell reduces risks, consider verifying that no race conditions can occur when initializing or re-initializing these verifiers.
56-57: Check memory constraints for large polynomial degrees.
You're using LAYER2_DEGREE and LAYER4_DEGREE from prover_v5. Large degree polynomials can result in significant memory usage. Verify that your environment can handle expansions at these degrees without performance regressions.
84-86: Use of fork_name.clone() is acceptable.
Storing config.halo2_circuit.fork_name in the verifier is standard practice. This usage is straightforward and should not introduce issues.
92-95: EuclidVerifier instantiation appears sound.
Your EuclidVerifier::new call references the shared PARAMS_MAP and workspace_path. The design is consistent with your refactored config.
97-99: Placing EuclidVerifier in VERIFIER_ZKVM matches naming.
Since EuclidVerifier handles the zkVM circuit, storing it in VERIFIER_ZKVM is appropriately named. This aligns well with the new config structure.
8-8: Confirm compatibility of the load_params usage.
You're now importing load_params on line 8. Check that the function signature aligns with your usage, particularly if upgrades have occurred in the prover_v5 library.Run the following script to list all references to load_params and compare them with the current signature:
✅ Verification successful
🏁 Script executed:
#!/bin/bash rg "load_params" -A 5Length of output: 911
Verified: The load_params usage is compatible with its current signature.
The grep results confirm that load_params is invoked with three arguments—namely, a reference to the parameters path, a value for degree, and None—which aligns with its expected signature in prover_v5 as currently used in the verifier. No discrepancies were identified.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
from
94d8f74 to
9d019a0
Compare
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
2 times, most recently
from
bab780e to
38cc6ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
common/libzkp/impl/src/verifier.rs (2)
46-100: Add proper error handling and remove unsafe code.The initialization function has several issues:
- Uses
unwrap_uncheckedwhich can lead to undefined behavior- No error handling for environment variable setting
- Extensive use of unsafe blocks
Consider this safer approach:
pub fn init(config: VerifierConfig) -> Result<()> { std::env::set_var( "SCROLL_PROVER_ASSETS_DIR", &config.low_version_circuit.assets_path, ); let params_map = { let mut map = BTreeMap::new(); for degree in [ *prover_v5::config::LAYER2_DEGREE, *prover_v5::config::LAYER4_DEGREE, ] { if !map.contains_key(°ree) { let params = load_params( &config.low_version_circuit.params_path, degree, None, ).map_err(|e| anyhow::anyhow!( "failed to load params, degree {}, dir {}, err {}", degree, config.low_version_circuit.params_path, e ))?; map.insert(degree, params); } } map }; PARAMS_MAP.get_or_init(|| RwLock::new(params_map)); // Initialize verifiers let halo2_verifier = DarwinV2Verifier::new( &PARAMS_MAP.get().unwrap().read().unwrap(), &config.low_version_circuit.assets_path, ); VERIFIER_HALO2.get_or_init(|| RwLock::new(Some(VerifierPair( config.low_version_circuit.fork_name.clone(), Rc::new(Box::new(halo2_verifier)), )))); let zkvm_verifier = EuclidVerifier::new( &PARAMS_MAP.get().unwrap().read().unwrap(), &config.high_version_circuit.assets_path, ); VERIFIER_ZKVM.get_or_init(|| RwLock::new(Some(VerifierPair( config.high_version_circuit.fork_name, Rc::new(Box::new(zkvm_verifier)), )))); Ok(()) }
102-117: Remove unsafe blocks from getter function.The function correctly uses Result for error handling but still relies on unsafe blocks. Consider this safer implementation:
pub fn get_verifier(fork_name: &str) -> Result<Rc<Box<dyn ProofVerifier>>> { if let Some(verifier) = VERIFIER_HALO2 .get() .and_then(|lock| lock.read().ok()) .and_then(|guard| guard.as_ref()) { if verifier.0 == fork_name { return Ok(verifier.1.clone()); } } if let Some(verifier) = VERIFIER_ZKVM .get() .and_then(|lock| lock.read().ok()) .and_then(|guard| guard.as_ref()) { if verifier.0 == fork_name { return Ok(verifier.1.clone()); } } bail!("failed to get verifier, key not found, {}", fork_name) }
🧹 Nitpick comments (3)
common/libzkp/impl/src/verifier/darwin_v2.rs (1)
31-47: Consider improving error handling.The current implementation has a few areas where error handling could be more robust:
- The
unwrap()calls in proof deserialization could fail at runtime.- The
panic_catchwraps all panics into generic errors, losing specific error context.Consider this more robust error handling approach:
fn verify(&self, task_type: super::TaskType, proof: Vec<u8>) -> Result<bool> { - let result = panic_catch(|| match task_type { + let result = panic_catch(|| -> Result<bool> { + let verification_result = match task_type { TaskType::Chunk => { - let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()).unwrap(); - self.verifier.verify_chunk_proof(proof) + let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()) + .map_err(|e| anyhow::anyhow!("Failed to deserialize ChunkProof: {}", e))?; + self.verifier.verify_chunk_proof(proof) } TaskType::Batch => { - let proof = serde_json::from_slice::<BatchProof>(proof.as_slice()).unwrap(); - self.agg_verifier.verify_batch_proof(&proof) + let proof = serde_json::from_slice::<BatchProof>(proof.as_slice()) + .map_err(|e| anyhow::anyhow!("Failed to deserialize BatchProof: {}", e))?; + self.agg_verifier.verify_batch_proof(&proof) } TaskType::Bundle => { - let proof = serde_json::from_slice::<BundleProof>(proof.as_slice()).unwrap(); - self.agg_verifier.verify_bundle_proof(proof) + let proof = serde_json::from_slice::<BundleProof>(proof.as_slice()) + .map_err(|e| anyhow::anyhow!("Failed to deserialize BundleProof: {}", e))?; + self.agg_verifier.verify_bundle_proof(proof) } - }); - result.map_err(|e| anyhow::anyhow!(e)) + }; + Ok(verification_result) + }); + result.map_err(|e| anyhow::anyhow!("Proof verification failed: {}", e)) }common/libzkp/impl/src/verifier.rs (2)
14-36: Add documentation for public types and traits.Consider adding documentation comments (
///) for public entities to improve code maintainability:
TaskTypeenum and its variantsProofVerifiertrait and its methodsCircuitConfigandVerifierConfigstructs and their fieldsExample:
/// Represents different types of proof verification tasks #[derive(Debug, Clone, Copy, PartialEq)] pub enum TaskType { /// Chunk-level proof verification Chunk, // ... etc }
42-44: Good naming change, but consider thread-safe alternatives.The renaming from HIGH/LOW to ZKVM/HALO2 better reflects the purpose of these verifiers. However, consider using thread-safe alternatives:
use once_cell::sync::Lazy; static VERIFIER_ZKVM: Lazy<RwLock<Option<VerifierPair>>> = Lazy::new(|| RwLock::new(None)); static VERIFIER_HALO2: Lazy<RwLock<Option<VerifierPair>>> = Lazy::new(|| RwLock::new(None)); static PARAMS_MAP: Lazy<RwLock<BTreeMap<u32, ParamsKZG<Bn256>>>> = Lazy::new(|| RwLock::new(BTreeMap::new()));
📜 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 (6)
common/libzkp/impl/Cargo.toml(1 hunks)common/libzkp/impl/rust-toolchain(1 hunks)common/libzkp/impl/src/verifier.rs(4 hunks)common/libzkp/impl/src/verifier/darwin.rs(0 hunks)common/libzkp/impl/src/verifier/darwin_v2.rs(1 hunks)common/libzkp/impl/src/verifier/euclid.rs(1 hunks)
💤 Files with no reviewable changes (1)
- common/libzkp/impl/src/verifier/darwin.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- common/libzkp/impl/rust-toolchain
- common/libzkp/impl/src/verifier/euclid.rs
- common/libzkp/impl/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: compile
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (1)
common/libzkp/impl/src/verifier/darwin_v2.rs (1)
30-30: LGTM! Lifetime parameter simplification.The change to use
'_instead of an explicit lifetime parameter simplifies the code while maintaining the same functionality. This is a good practice when the lifetime relationships are straightforward.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
from
15ac152 to
ac17696
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
common/libzkp/impl/src/verifier/darwin_v2.rs (1)
49-51:⚠️ Potential issueReplace
unreachable!with a proper error return.Using
unreachable!for business logic is not recommended as it will cause a panic if the code is executed. Consider returning a proper error instead.Apply this diff to handle the case gracefully:
- fn dump_vk(&self, _file: &std::path::Path) { - unreachable!("coordinator already has access to DarwinV2 vks") + fn dump_vk(&self, _file: &std::path::Path) -> Result<()> { + bail!("coordinator already has access to DarwinV2 vks") + }coordinator/internal/logic/provertask/bundle_prover_task.go (1)
224-231:⚠️ Potential issueAdd nil check for NewBatchProof.
Similar to the nil check recommended for
NewChunkProof, add a nil check forNewBatchProofto ensure proper error handling.Apply this diff:
var batchProofs []message.BatchProof for _, batch := range batches { proof := message.NewBatchProof(hardForkName) + if proof == nil { + return nil, fmt.Errorf("failed to create batch proof for hard fork: %s", hardForkName) + } if encodeErr := json.Unmarshal(batch.Proof, &proof); encodeErr != nil { return nil, fmt.Errorf("failed to unmarshal proof: %w, bundle hash: %v, batch hash: %v", encodeErr, task.TaskID, batch.Hash) } batchProofs = append(batchProofs, proof)coordinator/internal/logic/provertask/batch_prover_task.go (1)
218-226:⚠️ Potential issueAdd nil check for NewChunkProof.
Add a nil check for
NewChunkProofto ensure proper error handling.Apply this diff:
var chunkProofs []message.ChunkProof var chunkInfos []*message.ChunkInfo for _, chunk := range chunks { proof := message.NewChunkProof(hardForkName) + if proof == nil { + return nil, fmt.Errorf("failed to create chunk proof for hard fork: %s", hardForkName) + } if encodeErr := json.Unmarshal(chunk.Proof, &proof); encodeErr != nil { return nil, fmt.Errorf("Chunk.GetProofsByBatchHash unmarshal proof error: %w, batch hash: %v, chunk hash: %v", encodeErr, task.TaskID, chunk.Hash) } chunkProofs = append(chunkProofs, proof)
🧹 Nitpick comments (2)
common/libzkp/impl/src/verifier.rs (2)
55-58: Improve environment variable handling.The environment variable setup could be more robust by:
- Checking if the variable is already set
- Validating the path exists
- Using a builder pattern for configuration
+ if !std::path::Path::new(&config.low_version_circuit.assets_path).exists() { + return Err(anyhow::anyhow!("Assets path does not exist")); + } std::env::set_var( "SCROLL_PROVER_ASSETS_DIR", &config.low_version_circuit.assets_path, );
110-125: Improve verifier retrieval with more idiomatic Rust.The current implementation could be improved by:
- Using more descriptive error messages
- Implementing a safer abstraction around verifier access
pub fn get_verifier(fork_name: &str) -> Result<Rc<Box<dyn ProofVerifier>>> { + // Create a type-safe wrapper for verifier access + struct VerifierRegistry { + halo2: &'static OnceCell<VerifierPair>, + zkvm: &'static OnceCell<VerifierPair>, + } + + impl VerifierRegistry { + fn get_verifier(&self, fork_name: &str) -> Option<Rc<Box<dyn ProofVerifier>>> { + unsafe { + self.halo2 + .get() + .filter(|v| v.0 == fork_name) + .or_else(|| self.zkvm.get().filter(|v| v.0 == fork_name)) + .map(|v| v.1.clone()) + } + } + } + + let registry = VerifierRegistry { + halo2: unsafe { &VERIFIER_HALO2 }, + zkvm: unsafe { &VERIFIER_ZKVM }, + }; + + registry.get_verifier(fork_name).ok_or_else(|| { + anyhow::anyhow!( + "No verifier found for fork '{}'. Available forks: {:?}", + fork_name, + registry + .get_available_forks() + .unwrap_or_else(|| vec!["none".to_string()]) + ) + }) }
📜 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 (22)
build/dockerfiles/prover.Dockerfile(1 hunks)build/dockerfiles/prover.dockerignore(1 hunks)common/libzkp/impl/Cargo.toml(1 hunks)common/libzkp/impl/rust-toolchain(1 hunks)common/libzkp/impl/src/lib.rs(2 hunks)common/libzkp/impl/src/verifier.rs(5 hunks)common/libzkp/impl/src/verifier/darwin.rs(0 hunks)common/libzkp/impl/src/verifier/darwin_v2.rs(2 hunks)common/libzkp/impl/src/verifier/euclid.rs(1 hunks)common/libzkp/interface/libzkp.h(1 hunks)coordinator/internal/controller/api/controller.go(1 hunks)coordinator/internal/logic/auth/login.go(3 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(4 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(2 hunks)coordinator/internal/logic/provertask/chunk_prover_task.go(1 hunks)coordinator/internal/logic/verifier/types.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(6 hunks)coordinator/internal/types/prover.go(2 hunks)rollup/conf/config.json(2 hunks)rollup/internal/config/relayer.go(1 hunks)rollup/internal/controller/relayer/l2_relayer.go(5 hunks)scroll-contracts(1 hunks)
💤 Files with no reviewable changes (1)
- common/libzkp/impl/src/verifier/darwin.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- coordinator/internal/logic/verifier/types.go
- rollup/conf/config.json
- coordinator/internal/controller/api/controller.go
- common/libzkp/impl/rust-toolchain
- build/dockerfiles/prover.dockerignore
- coordinator/internal/logic/provertask/chunk_prover_task.go
- scroll-contracts
- common/libzkp/impl/src/lib.rs
- coordinator/internal/types/prover.go
- coordinator/internal/logic/auth/login.go
- rollup/internal/config/relayer.go
- build/dockerfiles/prover.Dockerfile
- coordinator/internal/logic/verifier/verifier.go
- common/libzkp/interface/libzkp.h
- rollup/internal/controller/relayer/l2_relayer.go
- common/libzkp/impl/src/verifier/euclid.rs
- common/libzkp/impl/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: compile
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (9)
common/libzkp/impl/src/verifier/darwin_v2.rs (1)
30-30: LGTM! Good use of modern Rust lifetime syntax.The change to use the anonymous lifetime
'_simplifies the code without affecting its semantics.common/libzkp/impl/src/verifier.rs (4)
1-1: Consider removing#![allow(static_mut_refs)]and using thread-safe alternatives.The use of static mutable references is unsafe and can lead to data races in concurrent scenarios. Consider using thread-safe alternatives like:
Arc<RwLock<T>>for shared mutable statelazy_static!with interior mutabilityonce_cell::sync::OnceCellinstead ofstd::cell::OnceCell
30-30: Add return type todump_vkfunction.The
dump_vkfunction should return aResult<()>to handle potential errors during verification key dumping.
21-26: LGTM! Well-structured serialization type.The
VKDumpstruct provides a clean interface for serializing verification keys with appropriate field visibility.
80-81: Replaceunwrap_unchecked()with safe alternatives.Using
unwrap_unchecked()is unsafe and can lead to undefined behavior if the value is not initialized.Also applies to: 88-94, 101-107
coordinator/internal/logic/provertask/bundle_prover_task.go (1)
147-154: LGTM! Prover version compatibility check.The added check ensures that the prover supports the required hard fork version, with proper error handling and recovery.
coordinator/internal/logic/provertask/batch_prover_task.go (3)
147-154: LGTM! Prover version compatibility check.The added check ensures that the prover supports the required hard fork version, with proper error handling and recovery.
235-239: LGTM! Safe type assertion for Halo2ChunkProof.The added type assertion safely handles the access to
ChunkInfofield, improving type safety.
269-269: LGTM! Updated method signature.The parameter type change from
[]*message.ChunkProofto[]message.ChunkProofaligns with the transition from pointer slices to value slices.
| ChunkVKMap: chunkVKMap, | ||
| BatchVKMap: batchVKMap, | ||
| BundleVkMap: bundleVKMap, | ||
| OpenVMVkMap: openVMVkMap, |
There was a problem hiding this comment.
Why don't we add openVm vks to ChunkVKMap, BatchVKMap and BundleVkMap? Seems openVm still has chunk, batch and bundle vks.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
common/libzkp/impl/src/verifier.rs (1)
54-108:⚠️ Potential issueReplace unsafe initialization with thread-safe alternatives.
The initialization code uses unsafe operations that could lead to undefined behavior:
- Uses
unwrap_unchecked()which is unsafe- Uses static mutable state without proper synchronization
Consider using thread-safe alternatives:
- Replace
OnceCellwithstd::sync::OnceLockfor thread-safe initialization- Use
Arc<RwLock<T>>for shared mutable state- Replace
unwrap_unchecked()with safe error handlingApply this diff:
-static mut VERIFIER_ZKVM: OnceCell<VerifierPair> = OnceCell::new(); -static mut VERIFIER_HALO2: OnceCell<VerifierPair> = OnceCell::new(); -static mut PARAMS_MAP: OnceCell<BTreeMap<u32, ParamsKZG<Bn256>>> = OnceCell::new(); +use std::sync::OnceLock; +static VERIFIER_ZKVM: OnceLock<VerifierPair> = OnceLock::new(); +static VERIFIER_HALO2: OnceLock<VerifierPair> = OnceLock::new(); +static PARAMS_MAP: OnceLock<BTreeMap<u32, ParamsKZG<Bn256>>> = OnceLock::new(); // In init function: - PARAMS_MAP.set(params_map).unwrap_unchecked(); + if let Err(_) = PARAMS_MAP.set(params_map) { + panic!("Failed to set PARAMS_MAP: already initialized"); + }
♻️ Duplicate comments (1)
common/libzkp/impl/src/verifier.rs (1)
21-31:⚠️ Potential issueAdd error handling to
dump_vkfunction.The
dump_vkfunction should return aResult<()>to handle potential file I/O errors during verification key dumping.Apply this diff:
- fn dump_vk(&self, file: &Path); + fn dump_vk(&self, file: &Path) -> Result<()>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
common/libzkp/impl/src/verifier.rs(5 hunks)common/libzkp/impl/src/verifier/euclid.rs(1 hunks)coordinator/internal/logic/verifier/verifier.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/libzkp/impl/src/verifier/euclid.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: compile
- GitHub Check: tests
🔇 Additional comments (4)
common/libzkp/impl/src/verifier.rs (2)
1-1: Consider removing#![allow(static_mut_refs)]and using thread-safe alternatives.The use of static mutable references is unsafe and can lead to data races in concurrent scenarios.
3-12: LGTM! Clean module organization and imports.The module organization and imports are well-structured, with clear separation between the Darwin v2 and Euclid implementations.
coordinator/internal/logic/verifier/verifier.go (2)
60-64: LGTM! Well-structured verification key dump type.The
rustVkDumpstruct is well-defined with appropriate JSON tags for serialization.
67-112: Consider adding OpenVM VKs to existing VK maps.The OpenVM verification keys are stored in a separate map, but they still represent chunk, batch, and bundle VKs. Consider adding them to the existing
ChunkVKMap,BatchVKMap, andBundleVkMapfor consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
prover/src/zk_circuits_handler/euclid.rs (4)
18-18:⚠️ Potential issueJustify or remove unsafe Send implementation.
The unsafe implementation of Send needs justification or should be replaced with a safe alternative.
Consider implementing proper Send bounds for the prover types instead of using unsafe.
27-28:⚠️ Potential issueAdd proper cleanup in error paths.
The prover setup could leak resources if initialization fails. Consider implementing Drop or using RAII patterns.
Also applies to: 32-33, 37-38
21-45: 🛠️ Refactor suggestionRefactor duplicate prover configurations.
The setup code for each prover uses duplicate paths and configurations.
52-54:⚠️ Potential issueReplace try_lock() with proper error handling.
Using try_lock() can lead to deadlocks. Consider using a proper locking strategy with timeout or error propagation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prover/src/zk_circuits_handler/euclid.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: clippy
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (1)
prover/src/zk_circuits_handler/euclid.rs (1)
1-16: LGTM! Clean imports and struct definition.The imports are appropriate and the struct definition is well-organized with clear member types.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
coordinator/internal/orm/batch.go (3)
115-130: 🛠️ Refactor suggestionVerify codec version filter in GetAssignedBatch.
The
GetAssignedBatchmethod should also include the codec version filter for consistency withGetUnassignedBatch.- sql := fmt.Sprintf("SELECT * FROM batch WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND chunk_proofs_status = %d AND batch.deleted_at IS NULL ORDER BY batch.index LIMIT 1;", + sql := fmt.Sprintf("SELECT * FROM batch WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND chunk_proofs_status = %d AND codec_version != %d AND batch.deleted_at IS NULL ORDER BY batch.index LIMIT 1;", - int(types.ProvingTaskAssigned), maxTotalAttempts, maxActiveAttempts, int(types.ChunkProofsStatusReady)) + int(types.ProvingTaskAssigned), maxTotalAttempts, maxActiveAttempts, int(types.ChunkProofsStatusReady), excludedCodecVersion)
132-150: 🛠️ Refactor suggestionVerify codec version filter in GetUnassignedAndChunksUnreadyBatches.
The
GetUnassignedAndChunksUnreadyBatchesmethod should also include the codec version filter for consistency.db := o.db.WithContext(ctx) db = db.Where("proving_status = ?", types.ProvingTaskUnassigned) db = db.Where("chunk_proofs_status = ?", types.ChunkProofsStatusPending) + db = db.Where("codec_version != ?", excludedCodecVersion) db = db.Order("index ASC")
152-163: 🛠️ Refactor suggestionVerify codec version filter in GetAssignedBatches.
The
GetAssignedBatchesmethod should also include the codec version filter for consistency.db := o.db.WithContext(ctx) db = db.Model(&Batch{}) db = db.Where("proving_status = ?", int(types.ProvingTaskAssigned)) + db = db.Where("codec_version != ?", excludedCodecVersion)
🧹 Nitpick comments (3)
coordinator/internal/orm/chunk.go (1)
72-87: Inconsistent query styles between similar methods.
GetUnassignedChunkuses raw SQL whileGetUnassignedChunkCountuses GORM's query builder for similar conditions. Consider using the same query style for consistency and maintainability.Also applies to: 89-104
coordinator/internal/orm/batch.go (1)
86-87: Consider using a constant for the codec version filter.The hardcoded value
5in the SQL query should be defined as a constant to improve maintainability and prevent magic numbers.+// Define at package level +const excludedCodecVersion = 5 - sql := fmt.Sprintf("SELECT * FROM batch WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND chunk_proofs_status = %d AND codec_version != 5 AND batch.deleted_at IS NULL ORDER BY batch.index LIMIT 1;", + sql := fmt.Sprintf("SELECT * FROM batch WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND chunk_proofs_status = %d AND codec_version != %d AND batch.deleted_at IS NULL ORDER BY batch.index LIMIT 1;", - int(types.ProvingTaskUnassigned), maxTotalAttempts, maxActiveAttempts, int(types.ChunkProofsStatusReady)) + int(types.ProvingTaskUnassigned), maxTotalAttempts, maxActiveAttempts, int(types.ChunkProofsStatusReady), excludedCodecVersion)coordinator/internal/orm/bundle.go (1)
96-97: Consider adding codec version filter to GetAssignedBundle for consistency.The
GetAssignedBundlemethod should potentially include the same codec version filter asGetUnassignedBundleto maintain consistent behavior.- sql := fmt.Sprintf("SELECT * FROM bundle WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND batch_proofs_status = %d AND bundle.deleted_at IS NULL ORDER BY bundle.index LIMIT 1;", + sql := fmt.Sprintf("SELECT * FROM bundle WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND batch_proofs_status = %d AND codec_version != %d AND bundle.deleted_at IS NULL ORDER BY bundle.index LIMIT 1;", - int(types.ProvingTaskAssigned), maxTotalAttempts, maxActiveAttempts, int(types.BatchProofsStatusReady)) + int(types.ProvingTaskAssigned), maxTotalAttempts, maxActiveAttempts, int(types.BatchProofsStatusReady), DeprecatedCodecVersion)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
coordinator/internal/orm/batch.go(2 hunks)coordinator/internal/orm/bundle.go(2 hunks)coordinator/internal/orm/chunk.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: compile
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (2)
coordinator/internal/orm/batch.go (1)
271-274:❓ Verification inconclusive
Consider using a more specific codec version.
The code uses
CodecV0by default. Should this be updated to handle the new codec version that's being filtered out?
🏁 Script executed:
#!/bin/bash # Description: Check for codec version constants and their usage rg -A 5 "CodecV[0-9]"Length of output: 54481
Refactor Codec Version Usage in Batch Insertion
The current implementation hardcodes
CodecV0when creating the codec:codec, err := encoding.CodecFromVersion(encoding.CodecV0) if err != nil { return nil, fmt.Errorf("Batch.InsertBatch error: %w", err) }However, our broader codebase already supports newer codec versions (e.g., in the prover task and relayer modules, codec versions are determined dynamically based on input). Please verify whether the ORM insertion should also be codec version–aware. If batches are always meant to use
CodecV0at this layer, consider adding a comment that clarifies its intent. Otherwise, update this function to accept (or derive) a more specific codec version—perhaps by using a field from the batch or a global configuration—to ensure consistency with how other parts of the system select their codec version.coordinator/internal/orm/bundle.go (1)
62-63:❓ Verification inconclusive
Verify the impact of codec version filtering.
The addition of
codec_version != 5filter inGetUnassignedBundleandGetUnassignedBundleCountbut not in other similar methods likeGetAssignedBundlecreates inconsistent behavior. This could lead to:
- Skipped bundles with codec_version=5
- Potential data processing gaps
- Inconsistent bundle processing across different methods
Run this script to assess the impact:
Also applies to: 83-83
🏁 Script executed:
#!/bin/bash # Description: Check for bundles that would be affected by the codec version filter # Check total bundles with codec_version=5 echo "Checking bundles with codec_version=5..." rg -A 5 "codec_version.*5" coordinator/internal/orm/ # Check if codec_version is used consistently across methods echo "Checking codec_version usage across methods..." ast-grep --pattern 'codec_version != $_'Length of output: 3576
Subject: Verify Consistency in Codec Version Filtering Across Bundle Methods
- The recent changes have introduced the condition
codec_version != 5in both the unassigned bundle and count queries. Our search confirms that this filter is applied consistently in the unassigned methods inbundle.go, as well as in analogous queries inbatch.goandchunk.go.- However, it appears that similar methods—such as any existing assigned bundle queries (e.g.,
GetAssignedBundle)—have not been updated with this filter. This discrepancy may cause:
- Skipped bundles with
codec_version=5in unassigned operations while processing assigned bundles differently.- Potential data processing gaps or inconsistent behavior between unassigned and assigned bundle handling.
Please double-check whether the omission of the codec version filter in the assigned bundle queries is intentional. If not, consider aligning the filtering logic across these methods to avoid unintended behavior.
common/libzkp/impl/src/verifier.rs
Outdated
| static mut VERIFIER_HIGH: OnceCell<VerifierPair> = OnceCell::new(); | ||
| static mut VERIFIER_LOW: OnceCell<VerifierPair> = OnceCell::new(); | ||
| static mut VERIFIER_ZKVM: OnceCell<VerifierPair> = OnceCell::new(); | ||
| static mut VERIFIER_HALO2: OnceCell<VerifierPair> = OnceCell::new(); |
There was a problem hiding this comment.
The next upgrade is all ZKVM, here name will be changed again, I suggest still use low and high
| // layer so `bundle_verifier` can manage the params by itself | ||
| pub fn new(_params_map: &BTreeMap<u32, ParamsKZG<Bn256>>, assets_dir: &str) -> Self { | ||
| env::set_var("SCROLL_PROVER_ASSETS_DIR", assets_dir); | ||
| let zkvm_release_path = Path::new(assets_dir).join("scroll-zkvm").join("current"); |
There was a problem hiding this comment.
the assets will have a current layer, it's weird
| // "task_id", batchTask.Hash) | ||
| // return nil, ErrCoordinatorInternalFailure | ||
| //} | ||
| if _, ok := taskCtx.HardForkNames[hardForkName]; !ok { |
There was a problem hiding this comment.
I suggest removing this part code, if don't have this check, the task distribution logic is still correct. I commented this code since mainnet have a bug after enable this. (unlucky, I forgot the bug detail)
There was a problem hiding this comment.
We need this now, because provers won't support darwinv2 and euclid at the same time.
| @@ -83,7 +83,7 @@ func (*Batch) TableName() string { | |||
| func (o *Batch) GetUnassignedBatch(ctx context.Context, maxActiveAttempts, maxTotalAttempts uint8) (*Batch, error) { | |||
| var batch Batch | |||
| db := o.db.WithContext(ctx) | |||
| sql := fmt.Sprintf("SELECT * FROM batch WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND chunk_proofs_status = %d AND batch.deleted_at IS NULL ORDER BY batch.index LIMIT 1;", | |||
| sql := fmt.Sprintf("SELECT * FROM batch WHERE proving_status = %d AND total_attempts < %d AND active_attempts < %d AND chunk_proofs_status = %d AND codec_version != 5 AND batch.deleted_at IS NULL ORDER BY batch.index LIMIT 1;", | |||
There was a problem hiding this comment.
Why add codec_version != 5 AND? coordinator don't need to care about this, it believes rollup_relayer's check, because they use the same db/table.
There was a problem hiding this comment.
codec_version = 5is the special euclid transition batch which doesn't need any proving. So coordinator should just ignore it.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
common/libzkp/impl/src/verifier.rs (2)
1-1:⚠️ Potential issueConsider removing
#![allow(static_mut_refs)]and using thread-safe alternatives.The use of static mutable references is unsafe and can lead to data races in concurrent scenarios.
Consider using thread-safe alternatives like:
Arc<RwLock<T>>for shared mutable statelazy_static!with interior mutabilityonce_cell::sync::OnceCellinstead ofstd::cell::OnceCell
26-26:⚠️ Potential issueAdd return type to
dump_vkfunction.The
dump_vkfunction should return aResult<()>to handle potential errors during verification key dumping.Apply this diff to add the return type:
- fn dump_vk(&self, file: &Path); + fn dump_vk(&self, file: &Path) -> Result<()>;common/libzkp/impl/src/lib.rs (2)
71-73:⚠️ Potential issueHandle errors in
dump_vkfunction.The function silently ignores errors from
_dump_vk. Consider propagating errors to the caller.Apply this diff to handle errors:
-pub unsafe extern "C" fn dump_vk(fork_name: *const c_char, file: *const c_char) { - _dump_vk(fork_name, file); +pub unsafe extern "C" fn dump_vk(fork_name: *const c_char, file: *const c_char) -> c_char { + match _dump_vk(fork_name, file) { + Ok(_) => 1 as c_char, + Err(e) => { + log::error!("Failed to dump VK: {:#}", e); + 0 as c_char + } + }
75-82:⚠️ Potential issueImprove error handling in
_dump_vkfunction.The function silently ignores errors from
get_verifieranddump_vk. Consider propagating errors to the caller.Apply this diff to handle errors:
-fn _dump_vk(fork_name: *const c_char, file: *const c_char) { +fn _dump_vk(fork_name: *const c_char, file: *const c_char) -> Result<(), anyhow::Error> { let fork_name_str = c_char_to_str(fork_name); - let verifier = verifier::get_verifier(fork_name_str); + let verifier = verifier::get_verifier(fork_name_str)?; + verifier.as_ref().dump_vk(Path::new(c_char_to_str(file)))?; + Ok(()) - if let Ok(verifier) = verifier { - verifier.as_ref().dump_vk(Path::new(c_char_to_str(file))); - } }common/libzkp/impl/src/verifier/euclid.rs (3)
43-44:⚠️ Potential issueHandle deserialization errors without panicking.
Using
unwrap()onserde_json::from_slicecan crash the verifier if proof data is invalid.Apply this diff to handle deserialization errors gracefully:
- let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()).unwrap(); + let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()) + .map_err(|e| anyhow::anyhow!("Invalid ChunkProof input: {}", e))?; - let proof = serde_json::from_slice::<BatchProof>(proof.as_slice()).unwrap(); + let proof = serde_json::from_slice::<BatchProof>(proof.as_slice()) + .map_err(|e| anyhow::anyhow!("Invalid BatchProof input: {}", e))?; - let proof = serde_json::from_slice::<BundleProof>(proof.as_slice()).unwrap(); + let proof = serde_json::from_slice::<BundleProof>(proof.as_slice()) + .map_err(|e| anyhow::anyhow!("Invalid BundleProof input: {}", e))?;Also applies to: 47-48, 51-52
55-58:⚠️ Potential issueFix incorrect result handling and remove commented code.
The code always returns
Ok(true), ignoring the actual verification result. Additionally, there's commented-out error handling code.Apply this diff to fix the result handling:
- let _ = result.is_ok(); - Ok(true) - //result.map_err(|e| anyhow::anyhow!(e)) + result.map_err(|e| anyhow::anyhow!("Verification failed: {}", e))
60-69:⚠️ Potential issueImprove error handling in
dump_vkmethod.The method uses
expect()which can panic if file operations fail.Apply this diff to handle errors properly:
- fn dump_vk(&self, file: &Path) { - let f = File::create(file).expect("Failed to open file to dump VK"); + fn dump_vk(&self, file: &Path) -> Result<()> { + let f = File::create(file) + .map_err(|e| anyhow::anyhow!("Failed to create file to dump VK: {}", e))?; let dump = VKDump { chunk_vk: base64::encode(self.chunk_verifier.get_app_vk()), batch_vk: base64::encode(self.batch_verifier.get_app_vk()), bundle_vk: base64::encode(self.bundle_verifier.get_app_vk()), }; - serde_json::to_writer(f, &dump).expect("Failed to dump VK"); + serde_json::to_writer(f, &dump) + .map_err(|e| anyhow::anyhow!("Failed to write VK to file: {}", e))?; + Ok(()) }
🧹 Nitpick comments (7)
prover/Cargo.toml (3)
10-17: Review patched dependencies and URL formatting in[patch.crates-io].
New dependencies (such asalloy-primitives,revm, etc.) have been added with custom git references. However, the URLs forrevmand related entries include a double slash (//), which may be unintentional and could cause repository fetching issues. Please verify the correctness of these URLs.For example, consider updating:
- revm = { git = "https://github.com/scroll-tech//revm", branch = "scroll-evm-executor/v55" } + revm = { git = "https://github.com/scroll-tech/revm", branch = "scroll-evm-executor/v55" }
18-23: Confirm URL correctness in[patch."https://github.com/scroll-tech/revm.git"].
The patch for therevmcrate repeats the double-slash pattern in its git URLs. Double-check that the URL"https://github.com/scroll-tech//revm"is correct; if not, please update it to the conventional format.A suggested diff:
- revm = { git = "https://github.com/scroll-tech//revm", branch = "scroll-evm-executor/v55" } + revm = { git = "https://github.com/scroll-tech/revm", branch = "scroll-evm-executor/v55" }
24-39: Ensure consistency of git URLs in the reth patch section.
In the[patch."https://github.com/scroll-tech/reth.git"]block, all dependencies have URLs containing a double slash (e.g.,"https://github.com/scroll-tech//reth"). Verify if this is intentional; if not, update them to a single slash to avoid potential issues with dependency resolution.For instance, update:
- reth-chainspec = { git = "https://github.com/scroll-tech//reth", branch = "fix/scroll-zkvm" } + reth-chainspec = { git = "https://github.com/scroll-tech/reth", branch = "fix/scroll-zkvm" }common/libzkp/impl/Cargo.toml (4)
11-18: Check Git URL Formatting in Patched Dependencies.
The patched dependencies in this[patch.crates-io]block include URLs such as"https://github.com/scroll-tech//revm"which use a double slash after the organization name. While this may still resolve correctly, it is best practice to use a single slash for clarity and to avoid potential resolution issues. Please verify whether this formatting is intentional or if it should be corrected.
20-24: Verify Consistency in therevmPatch Block.
This patch block for"https://github.com/scroll-tech/revm.git"appears to reiterate the configuration provided earlier. Ensure that this duplication is intentional and that both blocks use consistent dependency settings. Also, note the same URL formatting issue (double slash) here as well.
26-39: ReviewrethDependency Patches.
The updated patch block forhttps://github.com/scroll-tech/reth.gitintroduces multiple dependencies (e.g.,reth-chainspec,reth-evm, etc.) all pointing to the branch"fix/scroll-zkvm". Please ensure that:
- The branch
"fix/scroll-zkvm"exists and is the intended source for these dependencies.- The URL formatting (again using a double slash) is correct or should be normalized.
This consistency is critical as these dependencies likely interact with the proof types and verification processes in the project.
41-41: Checkscroll-alloy-consensusDependency URL.
The declaration forscroll-alloy-consensususes a URL with a double slash:"https://github.com/scroll-tech//reth". Confirm whether this is deliberate and that the branch"fix/scroll-zkvm"is the correct source for this dependency.
📜 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!**/*.lockprover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
common/libzkp/impl/Cargo.toml(1 hunks)common/libzkp/impl/src/lib.rs(3 hunks)common/libzkp/impl/src/verifier.rs(3 hunks)common/libzkp/impl/src/verifier/darwin_v2.rs(0 hunks)common/libzkp/impl/src/verifier/euclid.rs(1 hunks)prover/Cargo.toml(3 hunks)
💤 Files with no reviewable changes (1)
- common/libzkp/impl/src/verifier/darwin_v2.rs
🔇 Additional comments (7)
prover/Cargo.toml (2)
51-57: Verify versions and feature configurations for new dependencies.
The new dependencies—scroll-proving-sdk,scroll-zkvm-prover, andsbv-primitives—are added with specific revisions/tags and feature flags. Please ensure that the revision"34a4f8f"forscroll-proving-sdkand the tag"v0.1.0-rc.4"forscroll-zkvm-proverare as intended. Also, confirm that the feature list (e.g., the"openvm"feature) correctly reflects the required functionality in this PR.
69-69: Double-check updated dependency versions.
The dependency updates—specifically settingasync-traitto"0.1",urlto"2.5.4", andserde_bytesto"0.11.15"—should be reviewed for consistency with the rest of the codebase. Ensure that these versions are compatible with other crates and that no conflicts are introduced.Also applies to: 74-75
common/libzkp/impl/Cargo.toml (1)
43-45: New Prover and Verifier Dependencies.
The addition ofprover_v7andverifier_v7via git (tag"v0.1.0-rc.4") is noted. Ensure that these dependencies meet the stability and compatibility requirements for production. Also, double-check that any usage within the codebase is updated accordingly to reflect these new implementations.common/libzkp/impl/src/verifier.rs (2)
17-22: LGTM! Well-structured VKDump struct.The struct is properly defined with appropriate derive macros and clear field names.
49-59:⚠️ Potential issueReplace
unwrap_unchecked()with safe alternatives.Using
unwrap_unchecked()is unsafe and can lead to undefined behavior if the value is not initialized.Apply this diff to use safe alternatives:
- .unwrap_unchecked(); + .map_err(|_| panic!("Failed to set VERIFIER_LOW: already initialized"))?;common/libzkp/impl/src/verifier/euclid.rs (2)
1-13: LGTM! Clean struct definition with appropriate imports.The struct is well-organized with clear field definitions and necessary imports.
17-18: Avoid hardcoding the 'current' layer path.The hardcoded 'current' path is problematic as noted in past reviews. Consider making this configurable or using a more robust versioning mechanism.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
from
8d97872 to
37924b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
common/libzkp/impl/src/verifier.rs (1)
49-59:⚠️ Potential issueFix incorrect verifier initialization.
There are several issues in the initialization:
VERIFIER_LOWis incorrectly initialized with high version configVERIFIER_HIGHis declared but never initialized- Unsafe
unwrap_unchecked()is usedApply this diff to fix the initialization:
pub fn init(config: VerifierConfig) { - let verifier = EuclidVerifier::new(&config.high_version_circuit.assets_path); + let low_verifier = EuclidVerifier::new(&config.low_version_circuit.assets_path); + let high_verifier = EuclidVerifier::new(&config.high_version_circuit.assets_path); unsafe { - VERIFIER_LOW - .set(VerifierPair( - config.high_version_circuit.fork_name, - Rc::new(Box::new(verifier)), - )) - .unwrap_unchecked(); + if let Err(_) = VERIFIER_LOW.set(VerifierPair( + config.low_version_circuit.fork_name, + Rc::new(Box::new(low_verifier)), + )) { + panic!("Failed to set VERIFIER_LOW: already initialized"); + } + + if let Err(_) = VERIFIER_HIGH.set(VerifierPair( + config.high_version_circuit.fork_name, + Rc::new(Box::new(high_verifier)), + )) { + panic!("Failed to set VERIFIER_HIGH: already initialized"); + } } }
♻️ Duplicate comments (5)
common/libzkp/impl/src/verifier.rs (2)
1-1:⚠️ Potential issueReplace unsafe static mutable references with thread-safe alternatives.
The use of
#![allow(static_mut_refs)]andstd::cell::OnceCellcan lead to data races in concurrent scenarios.Consider using thread-safe alternatives:
-#![allow(static_mut_refs)] -use std::{cell::OnceCell, path::Path, rc::Rc}; +use std::{path::Path, rc::Rc, sync::Arc}; +use parking_lot::RwLock; +use once_cell::sync::OnceCell;Also applies to: 8-8
26-26:⚠️ Potential issueAdd return type to
dump_vkfunction.The
dump_vkmethod should handle potential I/O errors during verification key dumping.- fn dump_vk(&self, file: &Path); + fn dump_vk(&self, file: &Path) -> Result<()>;common/libzkp/impl/src/lib.rs (3)
29-33:⚠️ Potential issueCritical Security Warning: Bypassing Verification for "darwinV2"
The current implementation always returns true for the "darwinV2" branch without performing any actual verification. This workaround could expose the system to security vulnerabilities if the bypass remains in production.
- Impact: Unconditional approval without proper verification may lead to exploitable security issues.
- Suggestions:
- Consider deploying separate binaries for each verifier.
- Implement a proper version or condition check that only bypasses verification under controlled circumstances.
- Introduce monitoring or alerting for scenarios where verifications are skipped.
71-73:⚠️ Potential issueHandle errors in
dump_vkfunction.The function silently ignores errors from
_dump_vk. Consider propagating errors to the caller.Apply this diff to handle errors:
-pub unsafe extern "C" fn dump_vk(fork_name: *const c_char, file: *const c_char) { - _dump_vk(fork_name, file); +pub unsafe extern "C" fn dump_vk(fork_name: *const c_char, file: *const c_char) -> c_char { + match _dump_vk(fork_name, file) { + Ok(_) => 1 as c_char, + Err(e) => { + log::error!("Failed to dump VK: {:#}", e); + 0 as c_char + } + }
75-82:⚠️ Potential issueImprove error handling in
_dump_vkfunction.The function silently ignores errors from
get_verifieranddump_vk. Consider propagating errors to the caller.Apply this diff to handle errors:
-fn _dump_vk(fork_name: *const c_char, file: *const c_char) { +fn _dump_vk(fork_name: *const c_char, file: *const c_char) -> Result<(), anyhow::Error> { let fork_name_str = c_char_to_str(fork_name); - let verifier = verifier::get_verifier(fork_name_str); + let verifier = verifier::get_verifier(fork_name_str)?; + verifier.as_ref().dump_vk(Path::new(c_char_to_str(file)))?; + Ok(()) - if let Ok(verifier) = verifier { - verifier.as_ref().dump_vk(Path::new(c_char_to_str(file))); - } }
🧹 Nitpick comments (6)
common/libzkp/impl/src/verifier.rs (1)
61-76: Add error context to verifier lookup failures.The error message could be more descriptive to help with debugging.
- bail!("failed to get verifier, key not found, {}", fork_name) + bail!( + "failed to get verifier for fork '{}'. Available forks: low='{}', high='{}'", + fork_name, + unsafe { VERIFIER_LOW.get().map(|v| v.0.as_str()).unwrap_or("not_set") }, + unsafe { VERIFIER_HIGH.get().map(|v| v.0.as_str()).unwrap_or("not_set") } + )prover/Cargo.toml (5)
10-16: Review: Repository URL Formatting in Patch Section
The dependency entries added in this section foralloy-primitives,revm,revm-interpreter,revm-precompile, andrevm-primitivesuse URLs with a double slash (e.g.,"https://github.com/scroll-tech//revm"). Although this may work, it is best to standardize the URL to"https://github.com/scroll-tech/revm"for clarity and to avoid any potential tooling issues.
19-22: Review: Duplicate Patch for revm Dependencies
The entries in this[patch."https://github.com/scroll-tech/revm.git"]section duplicate those in the previous patch section and similarly contain the double slash in the Git URLs. Please verify if both patch blocks are necessary, or if they can be consolidated to reduce redundancy and potential maintenance overhead.
25-39: Review: Updating reth Dependencies and URL Consistency
In the reth patch section, multiple new dependencies are introduced with the branch"fix/scroll-zkvm". Please double-check the repository URLs (which also show a double slash, e.g.,"https://github.com/scroll-tech//reth") to ensure accuracy. Also, confirm that the branch naming and the consolidated reth dependency management align with your overall versioning strategy.
51-57: Review: New Proving SDK Dependencies
The additions ofscroll-proving-sdk,scroll-zkvm-prover, andsbv-primitivesare central to the new feature implementation. Please verify that:
- The pinned commit for
scroll-proving-sdk(revision"eed375d") and- The tag for
scroll-zkvm-prover("v0.1.0-rc.4")
are intentional for production stability. Additionally, consider whether using fixed versions (or more explicit version pinning) might add extra stability for production deployments.
69-69: Review: New Dependency Version Compatibility
The explicit additions ofasync-trait("0.1"),url("2.5.4"), andserde_bytes("0.11.15") look deliberate. Please confirm that these version choices are compatible with your codebase and that they align with the project's versioning and compatibility policies.Also applies to: 74-75
📜 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!**/*.lockprover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
common/libzkp/impl/Cargo.toml(1 hunks)common/libzkp/impl/src/lib.rs(3 hunks)common/libzkp/impl/src/verifier.rs(3 hunks)common/libzkp/impl/src/verifier/darwin_v2.rs(0 hunks)common/libzkp/impl/src/verifier/euclid.rs(1 hunks)prover/Cargo.toml(3 hunks)
💤 Files with no reviewable changes (1)
- common/libzkp/impl/src/verifier/darwin_v2.rs
🔇 Additional comments (14)
common/libzkp/impl/src/verifier/euclid.rs (5)
1-13: LGTM!The imports are well-organized, and the struct definition is clean and clear.
17-18: Clarify or refactor the "current" directory usage.The use of a hardcoded "current" directory in the assets path is unclear and potentially problematic.
16-36: Improve constructor's error handling and input validation.The constructor needs several improvements:
- Input validation for
assets_dir- Safe error handling for setup calls
- Environment variable safety check
55-56: Fix verification result handling.The code ignores the actual verification result and always returns
Ok(true), which could mask verification failures.
60-69: Improve error handling in dump_vk method.The method uses
expect()which can panic if file operations fail. Consider propagating errors to the caller.common/libzkp/impl/Cargo.toml (6)
11-18: Dependency Patches & URL VerificationThe patch section now introduces multiple dependencies (e.g.,
alloy-primitives,revm,revm-interpreter,revm-precompile,revm-primitives,ruint, andtiny-keccak). Please verify that the specified branches (such as"scroll-evm-executor/v55"and"scroll-patch-v2.0.2-openvm-v1.0.0-rc.1") correctly correspond to the intended versions and that the extra slash in the URL (e.g."https://github.com/scroll-tech//revm") is intentional and does not cause issues.
20-24: Consistency of RevM Patch DependenciesThis patch block for the
revmdependencies (hosted at"https://github.com/scroll-tech/revm.git") is consistent with the earlier section, using the same branch"scroll-evm-executor/v55". Please double-check that the repository URL (again noticing the extra slash in some instances) is correct and that this change aligns with the overall dependency strategy.
26-35: Reth Dependency Patches AlignmentThe reth-related dependencies (e.g.
reth-chainspec,reth-evm,reth-evm-ethereum,reth-execution-types,reth-primitives,reth-primitives-traits,reth-storage-errors,reth-trie, andreth-trie-sparse) are patched from the branch"fix/scroll-zkvm". This aligns with updating the proof verification strategy. Ensure that these patched versions are compatible with the rest of the codebase as intended.
37-39: Additional Reth-Scroll PatchesThe dependencies
reth-scroll-chainspec,reth-scroll-evm, andreth-scroll-primitivesare being sourced from the"fix/scroll-zkvm"branch. These additions seem consistent with the overall changes in proof handling. Please make sure that the corresponding modules that depend on these patches are updated accordingly.
41-41: Repository URL Check for scroll-alloy-consensusThe dependency
scroll-alloy-consensusreferences the repository"https://github.com/scroll-tech//reth". The double slash (//) in the URL might be an oversight. Please verify if this is intentional or if it should be corrected to"https://github.com/scroll-tech/reth".
43-45: Integration of New Prover & Verifier DependenciesThe new dependencies
prover_v7andverifier_v7are added from the repositoryhttps://github.com/scroll-tech/zkvm-prover.gitwith the tag"v0.1.0-rc.4". This aligns with the PR’s objectives of updating proof verification methods. Verify that all downstream components using these dependencies are updated to reflect these new versions.common/libzkp/impl/src/verifier.rs (2)
17-22: LGTM! Well-structured verification key dump.The
VKDumpstruct provides a clean interface for managing verification keys with appropriate serialization support.
29-40: LGTM! Well-structured configuration types.The configuration structures maintain the suggested "low" and "high" version naming convention while providing clear separation of concerns.
common/libzkp/impl/src/lib.rs (1)
12-16:❓ Verification inconclusive
Verify the impact of removing logging initialization.
The removal of
init_env_and_logcall might affect the logging functionality. Ensure that logging is properly initialized elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash # Search for other logging initialization calls rg -A 5 'init_env_and_log|env_logger::init'Length of output: 45
Action Required: Confirm Logging Initialization
After searching for any alternative logging initialization calls (e.g.,
init_env_and_logorenv_logger::init), no matching occurrences were found in the codebase. This raises concerns that the logging initialization might now be missing, which could affect logging behavior and diagnostic output.
- The removal of the
init_env_and_logcall in theinitfunction (incommon/libzkp/impl/src/lib.rs, lines 12-16) might leave logging uninitialized.- The initial search did not reveal alternative logging initialization calls elsewhere.
Please verify manually that logging is sufficiently initialized elsewhere or consider reinstating appropriate logging setup to ensure proper debug and error logging.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
coordinator/internal/logic/provertask/batch_prover_task.go (1)
218-226:⚠️ Potential issueAdd nil check for NewChunkProof.
The code should handle the case where
NewChunkProofreturns nil to prevent potential panic.Apply this diff:
var chunkProofs []message.ChunkProof var chunkInfos []*message.ChunkInfo for _, chunk := range chunks { proof := message.NewChunkProof(hardForkName) + if proof == nil { + return nil, fmt.Errorf("failed to create chunk proof for hard fork: %s", hardForkName) + } if encodeErr := json.Unmarshal(chunk.Proof, &proof); encodeErr != nil { return nil, fmt.Errorf("Chunk.GetProofsByBatchHash unmarshal proof error: %w, batch hash: %v, chunk hash: %v", encodeErr, task.TaskID, chunk.Hash) }
🧹 Nitpick comments (1)
coordinator/internal/logic/provertask/batch_prover_task.go (1)
275-280: Improve switch statement readability and maintainability.The switch statement could be more explicit about its purpose and handling of different codec versions.
Apply this diff:
- dbBatchCodecVersion := encoding.CodecVersion(dbBatch.CodecVersion) - switch dbBatchCodecVersion { - case encoding.CodecV3, encoding.CodecV4, encoding.CodecV6: - default: - return taskDetail, nil - } + dbBatchCodecVersion := encoding.CodecVersion(dbBatch.CodecVersion) + // Only process batch header for specific codec versions + switch dbBatchCodecVersion { + case encoding.CodecV3, encoding.CodecV4, encoding.CodecV6: + // Continue processing + default: + log.Debug("skipping batch header processing for unsupported codec version", + "version", dbBatchCodecVersion) + return taskDetail, nil + }
| if _, ok := taskCtx.HardForkNames[hardForkName]; !ok { | ||
| bp.recoverActiveAttempts(ctx, batchTask) | ||
| log.Error("incompatible prover version", | ||
| "requisite hard fork name", hardForkName, | ||
| "prover hard fork name", taskCtx.HardForkNames, | ||
| "task_id", batchTask.Hash) | ||
| return nil, ErrCoordinatorInternalFailure | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Re-enabling version compatibility check requires careful testing.
Based on the past review comments, this code was previously commented out due to a mainnet bug. While it's now needed for prover version compatibility, ensure thorough testing in staging environment before deployment.
Run the following script to check for any related issues or incidents:
🏁 Script executed:
#!/bin/bash
# Search for related issues or incidents
gh issue list --search "mainnet bug prover version compatibility"
gh issue list --search "prover version hard fork"Length of output: 119
Verify re-enabled version compatibility check in staging before deployment
The version compatibility check in coordinator/internal/logic/provertask/batch_prover_task.go (lines 147-154) has been re-enabled after previously being disabled due to a mainnet bug. Although the automated GitHub issue query produced no output, this absence of results isn't conclusive. Please manually verify that:
- The check against
taskCtx.HardForkNamesworks as intended. - The recovery via
bp.recoverActiveAttemptsand error logging (including the task details) occur correctly. - No unexpected behavior or side effects emerge from reactivating this check in a staging environment.
| if haloProot, ok := proof.(*message.Halo2ChunkProof); ok { | ||
| if haloProot.ChunkInfo != nil { | ||
| chunkInfo.TxBytes = haloProot.ChunkInfo.TxBytes | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type assertion error handling.
The type assertion could fail silently if the proof is not a Halo2ChunkProof. Consider handling the case where the type assertion fails.
Apply this diff:
- if haloProot, ok := proof.(*message.Halo2ChunkProof); ok {
- if haloProot.ChunkInfo != nil {
- chunkInfo.TxBytes = haloProot.ChunkInfo.TxBytes
- }
+ if haloProot, ok := proof.(*message.Halo2ChunkProof); ok {
+ if haloProot.ChunkInfo != nil {
+ chunkInfo.TxBytes = haloProot.ChunkInfo.TxBytes
+ }
+ } else {
+ log.Debug("proof is not a Halo2ChunkProof", "proof_type", fmt.Sprintf("%T", proof))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if haloProot, ok := proof.(*message.Halo2ChunkProof); ok { | |
| if haloProot.ChunkInfo != nil { | |
| chunkInfo.TxBytes = haloProot.ChunkInfo.TxBytes | |
| } | |
| } | |
| if haloProot, ok := proof.(*message.Halo2ChunkProof); ok { | |
| if haloProot.ChunkInfo != nil { | |
| chunkInfo.TxBytes = haloProot.ChunkInfo.TxBytes | |
| } | |
| } else { | |
| log.Debug("proof is not a Halo2ChunkProof", "proof_type", fmt.Sprintf("%T", proof)) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
common/libzkp/impl/src/verifier/euclid.rs (1)
18-18: 🛠️ Refactor suggestionAvoid hardcoding the "current" path segment.
The hardcoded "current" path segment makes assumptions about the directory structure and could break if the assets are organized differently. Consider making this configurable or using a more robust versioning scheme.
- let zkvm_release_path = Path::new(assets_dir).join("scroll-zkvm").join("current"); + let zkvm_version = env::var("SCROLL_ZKVM_VERSION").unwrap_or("current".to_string()); + let zkvm_release_path = Path::new(assets_dir).join("scroll-zkvm").join(zkvm_version);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/libzkp/impl/src/verifier/euclid.rs(1 hunks)
🔇 Additional comments (1)
common/libzkp/impl/src/verifier/euclid.rs (1)
9-13: LGTM! Clean struct definition.The
EuclidVerifierstruct is well-organized with clear field names and appropriate types.
| panic_catch(|| match task_type { | ||
| TaskType::Chunk => { | ||
| let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()).unwrap(); | ||
| self.chunk_verifier.verify_proof(&proof).is_ok() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve error information from verification calls.
Using is_ok() discards valuable error information that could help diagnose verification failures. Consider propagating the actual error messages.
- self.chunk_verifier.verify_proof(&proof).is_ok()
+ self.chunk_verifier.verify_proof(&proof)
+ .map_err(|e| anyhow::anyhow!("Chunk verification failed: {}", e))?;
+ true
- self.batch_verifier.verify_proof(&proof).is_ok()
+ self.batch_verifier.verify_proof(&proof)
+ .map_err(|e| anyhow::anyhow!("Batch verification failed: {}", e))?;
+ true
- self.bundle_verifier.verify_proof_evm(&proof).is_ok()
+ self.bundle_verifier.verify_proof_evm(&proof)
+ .map_err(|e| anyhow::anyhow!("Bundle verification failed: {}", e))?;
+ trueAlso applies to: 48-48, 52-52
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
common/libzkp/impl/src/verifier/euclid.rs (2)
17-30:⚠️ Potential issueImprove error handling and input validation in the constructor.
The constructor has several issues that could lead to runtime panics:
- No validation of the
assets_dirparameter- Unsafe usage of
expectin setup calls- No proper error propagation
Apply this diff to improve error handling:
- pub fn new(assets_dir: &str) -> Self { + pub fn new(assets_dir: &str) -> Result<Self> { + let assets_path = Path::new(assets_dir); + if !assets_path.is_dir() { + return Err(anyhow::anyhow!("Invalid assets directory: {}", assets_dir)); + } + let verifier_bin = Path::new(assets_dir).join("verifier.bin"); let config = Path::new(assets_dir).join("root-verifier-vm-config"); let exe = Path::new(assets_dir).join("root-verifier-committed-exe"); + + for path in [&verifier_bin, &config, &exe] { + if !path.exists() { + return Err(anyhow::anyhow!("Required file not found: {:?}", path)); + } + } - Self { + Ok(Self { chunk_verifier: ChunkVerifier::setup(&config, &exe, &verifier_bin) - .expect("Setting up chunk verifier"), + .map_err(|e| anyhow::anyhow!("Failed to setup chunk verifier: {}", e))?, batch_verifier: BatchVerifier::setup(&config, &exe, &verifier_bin) - .expect("Setting up batch verifier"), + .map_err(|e| anyhow::anyhow!("Failed to setup batch verifier: {}", e))?, bundle_verifier: BundleVerifier::setup(&config, &exe, &verifier_bin) - .expect("Setting up bundle verifier"), + .map_err(|e| anyhow::anyhow!("Failed to setup bundle verifier: {}", e))?, - } + }) }
52-61:⚠️ Potential issueImprove error handling in the dump_vk method.
The method uses
expectwhich can panic if file operations fail.Apply this diff to handle errors:
- fn dump_vk(&self, file: &Path) { - let f = File::create(file).expect("Failed to open file to dump VK"); + fn dump_vk(&self, file: &Path) -> Result<()> { + let f = File::create(file) + .map_err(|e| anyhow::anyhow!("Failed to create file to dump VK: {}", e))?; let dump = VKDump { chunk_vk: base64::encode(self.chunk_verifier.get_app_vk()), batch_vk: base64::encode(self.batch_verifier.get_app_vk()), bundle_vk: base64::encode(self.bundle_verifier.get_app_vk()), }; - serde_json::to_writer(f, &dump).expect("Failed to dump VK"); + serde_json::to_writer(f, &dump) + .map_err(|e| anyhow::anyhow!("Failed to write VK to file: {}", e))?; + Ok(()) }
🧹 Nitpick comments (1)
common/libzkp/impl/Cargo.toml (1)
20-25: Assess the Explicit RevM Patch Block
This patch block re-specifiesrevm,revm-interpreter,revm-precompile, andrevm-primitivesfrom"https://github.com/scroll-tech/revm.git". It appears similar to the definitions in the previous patch block. Consider consolidating these definitions if possible or document the reasoning behind having separate patch instances. Also, verify the URL formatting as the same double-slash pattern is observed.
📜 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!**/*.lockprover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
common/libzkp/impl/Cargo.toml(1 hunks)common/libzkp/impl/src/verifier/euclid.rs(1 hunks)prover/src/zk_circuits_handler/euclid.rs(1 hunks)
🔇 Additional comments (11)
prover/src/zk_circuits_handler/euclid.rs (6)
1-16: LGTM!The imports are well-organized and the struct definition is clean with appropriate field types.
18-18: Justify or remove unsafe Send implementation.The unsafe implementation of Send needs justification or should be replaced with a safe alternative.
Consider implementing proper Send bounds for the prover types instead of using unsafe.
27-38: Replace expect() with proper error handling.Using expect() can lead to panics in production. Consider propagating errors using Result.
Apply this diff to improve error handling:
- .expect("Failed to setup chunk prover"); + .map_err(|e| anyhow!("Failed to setup chunk prover: {}", e))?;Similar changes should be applied to batch and bundle prover setup.
24-38: Refactor duplicate prover configurations.The setup code for each prover uses duplicate paths and configurations.
Consider extracting the common configuration into a shared setup function:
impl EuclidHandler { fn setup_prover<T>( workspace_path: &Path, cache_dir: &Path, prover_type: &str, ) -> Result<T> where T: ProverSetup, { let exe = workspace_path.join(format!("{}/app.vmexe", prover_type)); let config = workspace_path.join(format!("{}/openvm.toml", prover_type)); T::setup(exe, config, Some(cache_dir.to_path_buf())) .map_err(|e| anyhow!("Failed to setup {} prover: {}", prover_type, e)) } }
52-54: Replace try_lock() with proper error handling.Using try_lock() can lead to deadlocks. Consider using a proper locking strategy with timeout or error propagation.
Apply this diff to improve error handling:
- ProofType::Chunk => self.try_lock().unwrap().chunk_prover.get_app_vk(), + ProofType::Chunk => self.lock().await.chunk_prover.get_app_vk(),Similar changes should be applied to other try_lock() calls in the file.
55-55: Replace unreachable! with proper error handling.Using unreachable! can lead to panics if new proof types are added. Consider returning an error instead.
Apply this diff to improve error handling:
- _ => unreachable!("Unsupported proof type"), + _ => return None,common/libzkp/impl/Cargo.toml (4)
11-18: Review Patch Block for Crates-IO Dependencies
The added patched dependencies foralloy-primitives,revm,revm-interpreter,revm-precompile,revm-primitives,ruint, andtiny-keccakare clear and well-organized. Please verify the git URL formatting forrevm(note the double slash in"https://github.com/scroll-tech//revm") to ensure it is intentional and correct.
26-39: Comprehensive Reth Patch Block
A wide array ofreth-related dependencies are patched from the"https://github.com/scroll-tech/reth.git"URL on the"fix/scroll-zkvm"branch. This policy aligns with the new interface-based design for proof types. Please ensure that the branch"fix/scroll-zkvm"is stable and the correct target for all these dependencies. Additionally, confirm the git URL formatting (observe the double slash in"https://github.com/scroll-tech//reth") for consistency.
41-42: Review for scroll-alloy-consensus Dependency
The patched dependency forscroll-alloy-consensusis added with a specific branch from the reth repository. Verify that the git URL (again, note the double slash) and branch are correct and that this dependency plays well with the overall dependency graph.
43-46: Euclid Prover and Verifier Dependencies
The neweuclid_proverandeuclid_verifierdependencies are added with explicit git tags (v0.1.0-rc.4) and package names. Since these are critical to the new Euclid prover and coordinator feature, please ensure that these release-candidate versions are thoroughly tested and meet all stability and functionality requirements before merging.common/libzkp/impl/src/verifier/euclid.rs (1)
1-14: LGTM!The imports are well-organized, and the struct definition follows Rust conventions.
| fn verify(&self, task_type: super::TaskType, proof: Vec<u8>) -> Result<bool> { | ||
| panic_catch(|| match task_type { | ||
| TaskType::Chunk => { | ||
| let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()).unwrap(); | ||
| self.chunk_verifier.verify_proof(&proof.proof) | ||
| } | ||
| TaskType::Batch => { | ||
| let proof = serde_json::from_slice::<BatchProof>(proof.as_slice()).unwrap(); | ||
| self.batch_verifier.verify_proof(&proof.proof) | ||
| } | ||
| TaskType::Bundle => { | ||
| let proof = serde_json::from_slice::<BundleProof>(proof.as_slice()).unwrap(); | ||
| self.bundle_verifier.verify_proof_evm(&proof.proof) | ||
| } | ||
| }) | ||
| .map_err(|err_str: String| anyhow::anyhow!(err_str)) | ||
| } |
There was a problem hiding this comment.
Improve error handling in the verify method.
The verify method has several issues that could lead to runtime panics:
- Unsafe deserialization with
unwrap - No proper error handling for verification results
- No validation of input parameters
Apply this diff to improve error handling:
fn verify(&self, task_type: super::TaskType, proof: Vec<u8>) -> Result<bool> {
+ if proof.is_empty() {
+ return Err(anyhow::anyhow!("Empty proof provided"));
+ }
+
panic_catch(|| match task_type {
TaskType::Chunk => {
- let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()).unwrap();
- self.chunk_verifier.verify_proof(&proof.proof)
+ let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice())
+ .map_err(|e| anyhow::anyhow!("Failed to deserialize chunk proof: {}", e))?;
+ self.chunk_verifier.verify_proof(&proof.proof)
+ .map_err(|e| anyhow::anyhow!("Chunk verification failed: {}", e))
}
TaskType::Batch => {
- let proof = serde_json::from_slice::<BatchProof>(proof.as_slice()).unwrap();
- self.batch_verifier.verify_proof(&proof.proof)
+ let proof = serde_json::from_slice::<BatchProof>(proof.as_slice())
+ .map_err(|e| anyhow::anyhow!("Failed to deserialize batch proof: {}", e))?;
+ self.batch_verifier.verify_proof(&proof.proof)
+ .map_err(|e| anyhow::anyhow!("Batch verification failed: {}", e))
}
TaskType::Bundle => {
- let proof = serde_json::from_slice::<BundleProof>(proof.as_slice()).unwrap();
- self.bundle_verifier.verify_proof_evm(&proof.proof)
+ let proof = serde_json::from_slice::<BundleProof>(proof.as_slice())
+ .map_err(|e| anyhow::anyhow!("Failed to deserialize bundle proof: {}", e))?;
+ self.bundle_verifier.verify_proof_evm(&proof.proof)
+ .map_err(|e| anyhow::anyhow!("Bundle verification failed: {}", e))
}
})
.map_err(|err_str: String| anyhow::anyhow!(err_str))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn verify(&self, task_type: super::TaskType, proof: Vec<u8>) -> Result<bool> { | |
| panic_catch(|| match task_type { | |
| TaskType::Chunk => { | |
| let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()).unwrap(); | |
| self.chunk_verifier.verify_proof(&proof.proof) | |
| } | |
| TaskType::Batch => { | |
| let proof = serde_json::from_slice::<BatchProof>(proof.as_slice()).unwrap(); | |
| self.batch_verifier.verify_proof(&proof.proof) | |
| } | |
| TaskType::Bundle => { | |
| let proof = serde_json::from_slice::<BundleProof>(proof.as_slice()).unwrap(); | |
| self.bundle_verifier.verify_proof_evm(&proof.proof) | |
| } | |
| }) | |
| .map_err(|err_str: String| anyhow::anyhow!(err_str)) | |
| } | |
| fn verify(&self, task_type: super::TaskType, proof: Vec<u8>) -> Result<bool> { | |
| if proof.is_empty() { | |
| return Err(anyhow::anyhow!("Empty proof provided")); | |
| } | |
| panic_catch(|| match task_type { | |
| TaskType::Chunk => { | |
| let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()) | |
| .map_err(|e| anyhow::anyhow!("Failed to deserialize chunk proof: {}", e))?; | |
| self.chunk_verifier.verify_proof(&proof.proof) | |
| .map_err(|e| anyhow::anyhow!("Chunk verification failed: {}", e)) | |
| } | |
| TaskType::Batch => { | |
| let proof = serde_json::from_slice::<BatchProof>(proof.as_slice()) | |
| .map_err(|e| anyhow::anyhow!("Failed to deserialize batch proof: {}", e))?; | |
| self.batch_verifier.verify_proof(&proof.proof) | |
| .map_err(|e| anyhow::anyhow!("Batch verification failed: {}", e)) | |
| } | |
| TaskType::Bundle => { | |
| let proof = serde_json::from_slice::<BundleProof>(proof.as_slice()) | |
| .map_err(|e| anyhow::anyhow!("Failed to deserialize bundle proof: {}", e))?; | |
| self.bundle_verifier.verify_proof_evm(&proof.proof) | |
| .map_err(|e| anyhow::anyhow!("Bundle verification failed: {}", e)) | |
| } | |
| }) | |
| .map_err(|err_str: String| anyhow::anyhow!(err_str)) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
prover/src/zk_circuits_handler/euclid.rs (1)
65-71: Improve error propagation in proof generation.The proof generation code could benefit from better error handling and logging:
- Add context to errors using anyhow::Context
- Log proof generation attempts and failures
- Consider adding timeouts for proof generation
Apply this diff to improve error handling:
let proof = self .try_lock() - .unwrap() + .map_err(|e| anyhow::anyhow!("Failed to acquire lock: {}", e))? .chunk_prover .gen_proof(&ChunkProvingTask { block_witnesses: witnesses, - })?; + }) + .with_context(|| "Failed to generate chunk proof")?;Similar changes should be applied to batch and bundle proof generation.
Also applies to: 76-78, 83-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prover/src/zk_circuits_handler/euclid.rs(1 hunks)
🔇 Additional comments (6)
prover/src/zk_circuits_handler/euclid.rs (6)
1-16: LGTM! Well-organized imports and struct definition.The imports are appropriate for the functionality, and the struct definition is clean and well-organized.
18-18: Justify or remove unsafe Send implementation.The unsafe implementation of Send needs justification or should be replaced with a safe alternative.
Consider implementing proper Send bounds for the prover types instead of using unsafe.
27-38: Replace expect() with proper error handling.Using expect() can lead to panics in production. Consider propagating errors using Result.
24-38: Refactor duplicate prover configurations.The setup code for each prover uses duplicate paths and configurations.
52-54: Replace try_lock() with proper error handling.Using try_lock() can lead to deadlocks. Consider using a proper locking strategy with timeout or error propagation.
55-55: Replace unreachable! with proper error handling.Using unreachable! can lead to panics if new proof types are added. Consider returning an error instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
common/types/message/message_test.go (2)
11-54: Add comprehensive test coverage for proof deserialization.The test has several areas for improvement:
- Consider embedding test data in the test file instead of relying on external JSON files.
- Add negative test cases (malformed JSON, missing fields, etc.).
- Verify all fields of the deserialized objects, not just state roots.
Here's a suggested improvement:
func TestDeserializeOpenVMProof(t *testing.T) { + // Test data + const batchProofJSON = `{ + "metadata": { + "batch_info": { + "parent_state_root": "0xe3440bcf882852bb1a9d6ba941e53a645220fee2c531ed79fa60481be8078c12" + } + } + }` + const bundleProofJSON = `{ + "metadata": { + "bundle_info": { + "post_state_root": "0x9e8b9928c55ccbc933911283175842fa515e49dd3f2fe0192c4346095695d741" + } + } + }` + + t.Run("valid batch proof", func(t *testing.T) { batchProof := NewBatchProof("euclid") - data, err := os.ReadFile("batch-proof-sample.json") - if err != nil { - t.Fatalf("failed to read batch proof sample.json: %v", err) - } - if err = json.Unmarshal(data, &batchProof); err != nil { + if err := json.Unmarshal([]byte(batchProofJSON), &batchProof); err != nil { t.Fatalf("failed to unmarshal JSON into Batch Proof: %v", err) } // ... existing checks ... }) + + t.Run("invalid batch proof", func(t *testing.T) { + batchProof := NewBatchProof("euclid") + err := json.Unmarshal([]byte(`{invalid json}`), &batchProof) + if err == nil { + t.Fatal("expected error for invalid JSON") + } + }) // ... similar changes for bundle proof tests ... }
56-131: Enhance ByteArray marshal/unmarshal test coverage.The test is well-structured using table-driven tests, but could benefit from additional test cases:
- Large arrays to test performance
- Arrays with special byte values (0x00, 0xFF)
- Error cases for unmarshal (malformed arrays)
Add these test cases:
marshalTests := []struct { name string data ByteArray expected string }{ // ... existing cases ... + { + name: "special bytes", + data: ByteArray{0x00, 0xFF}, + expected: "[0,255]", + }, + { + name: "large array", + data: bytes.Repeat([]byte{1}, 1000), + expected: "[" + strings.Repeat("1,", 999) + "1]", + }, } unmarshalTests := []struct { name string data string expected ByteArray + wantErr bool }{ // ... existing cases ... + { + name: "malformed array", + data: "[1,2,", + wantErr: true, + }, + { + name: "invalid values", + data: "[1,256]", + wantErr: true, + }, }common/types/message/message.go (3)
13-13: Extract hard fork name into a well-named constant.The constant name
euclidForkis not descriptive enough. Consider using a more descriptive name likeHardForkEuclidto match Go naming conventions.-const ( - euclidFork = "euclid" -) +const ( + // HardForkEuclid represents the Euclid hard fork name + HardForkEuclid = "euclid" +)
232-244: Optimize ByteArray.MarshalJSON performance.The current implementation using strings.Builder for array serialization could be optimized.
func (b ByteArray) MarshalJSON() ([]byte, error) { + if len(b) == 0 { + return []byte("[]"), nil + } + // Pre-calculate the exact size needed + size := 2 // [] + size += len(b) * 3 // each byte needs up to 3 chars (255) + size += len(b) - 1 // commas builder := strings.Builder{} + builder.Grow(size) builder.WriteString("[") for i, v := range b { builder.WriteString(fmt.Sprintf("%d", v)) if i != len(b)-1 { builder.WriteString(",") } } builder.WriteString("]") return []byte(builder.String()), nil }
357-384: Simplify OpenVMBundleProof.SanityCheck error handling.The nested if-else structure makes the code harder to read. Consider flattening the error checks.
func (ap *OpenVMBundleProof) SanityCheck() error { if ap == nil { return errors.New("agg_proof is nil") } if ap.MetaData.BundleInfo == nil { return errors.New("bundle info not ready") } if ap.EvmProof == nil { return errors.New("proof not ready") } - else { - if len(ap.Vk) == 0 { - return errors.New("vk not ready") - } - pf := ap.EvmProof - if len(pf.Proof)%32 != 0 { - return fmt.Errorf("proof buffer length must be a multiple of 32, got: %d", len(pf.Proof)) - } - if len(pf.Instances) == 0 { - return errors.New("instance not ready") - } + if len(ap.Vk) == 0 { + return errors.New("vk not ready") } + pf := ap.EvmProof + if len(pf.Proof)%32 != 0 { + return fmt.Errorf("proof buffer length must be a multiple of 32, got: %d", len(pf.Proof)) + } + if len(pf.Instances) == 0 { + return errors.New("instance not ready") + } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/types/message/message.go(7 hunks)common/types/message/message_test.go(1 hunks)
🔇 Additional comments (4)
common/types/message/message.go (4)
50-53: Improve type safety in BatchTaskDetail.The use of
interface{}forBatchHeaderis too generic and could lead to runtime type errors.
79-92: Improve error handling in proof factory methods.The factory methods should return errors for unknown hard fork names to prevent nil pointer dereferences.
263-270: Fix error handling in OpenVMChunkProof.Proof().The function panics on JSON marshaling error, which could crash the application.
294-301: Fix error handling in OpenVMBatchProof.Proof().Similar to OpenVMChunkProof.Proof(), this function also panics on JSON marshaling error.
| func (p *OpenVMBundleProof) Proof() []byte { | ||
| return p.EvmProof.Proof | ||
| } |
There was a problem hiding this comment.
Add error handling to OpenVMBundleProof.Proof().
Unlike other Proof() methods, this one doesn't handle JSON marshaling, but it should still handle potential errors consistently with other methods.
-func (p *OpenVMBundleProof) Proof() []byte {
- return p.EvmProof.Proof
+func (p *OpenVMBundleProof) Proof() ([]byte, error) {
+ if p.EvmProof == nil {
+ return nil, errors.New("evm proof is nil")
+ }
+ return p.EvmProof.Proof, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *OpenVMBundleProof) Proof() []byte { | |
| return p.EvmProof.Proof | |
| } | |
| func (p *OpenVMBundleProof) Proof() ([]byte, error) { | |
| if p.EvmProof == nil { | |
| return nil, errors.New("evm proof is nil") | |
| } | |
| return p.EvmProof.Proof, nil | |
| } |
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
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
New Features
EuclidHandlerfor asynchronous proof generation with enhanced handling of proof types.LoginLogicstruct to manage OpenVM-related verification keys.VKDumpstruct for handling verification key dumping.InitControllerfunction to include OpenVM verification key information.ChunkProverTaskandBundleProverTaskwith improved error handling for prover version compatibility.Refactor
Halo2types, enhancing consistency and performance.BatchProposerandBundleProposermethods.proposeBatchmethod in theBatchProposerto improve codec retrieval and error handling.Chores