feat: euclid prover and coordinator #1597
Conversation
WalkthroughThe pull request introduces significant changes to proof-related types, transitioning from concrete struct definitions to an interface-based design. It updates the 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
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.19% -9.54%
===========================================
Files 157 222 +65
Lines 12555 17703 +5148
===========================================
+ Hits 6494 7469 +975
- Misses 5497 9518 +4021
- Partials 564 716 +152
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 ofjackfan.us.kg/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 forjackfan.us.kg/scroll-tech/go-ethereumandjackfan.us.kg/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 forjackfan.us.kg/scroll-tech/da-codec,github.com/scroll-tech/go-ethereum, andjackfan.us.kg/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 likejackfan.us.kg/scroll-tech/da-codec,github.com/scroll-tech/go-ethereum, andjackfan.us.kg/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 forjackfan.us.kg/scroll-tech/go-ethereumandjackfan.us.kg/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.0jackfan.us.kg/crate-crypto/go-kzg-4844→ v1.1.0jackfan.us.kg/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-4bdf6d096c38jackfan.us.kg/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.4jackfan.us.kg/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-codecandjackfan.us.kg/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
38cc6ac to
7bd05c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
coordinator/internal/types/prover.go (1)
22-31: Update String() method to include ProverTypeOpenVM.The String() method needs to be updated to handle the new ProverTypeOpenVM case.
func (r ProverType) String() string { switch r { case ProverTypeChunk: return "prover type chunk" case ProverTypeBatch: return "prover type batch" + case ProverTypeOpenVM: + return "prover type openvm" default: return fmt.Sprintf("illegal prover type: %d", r) } }
♻️ Duplicate comments (4)
common/libzkp/impl/src/verifier/euclid.rs (3)
23-31:⚠️ Potential issueHandle setup errors gracefully.
Using
unwrap()for setup calls can cause panics if the asset directories are missing or invalid.
44-45:⚠️ Potential issueHandle deserialization errors without panicking.
Using
unwrap()onserde_json::from_slicecan crash the verifier if proof data is invalid.Also applies to: 48-49, 52-53
56-57:⚠️ Potential issuePreserve and return the actual verification result.
Currently, the code always returns
Ok(true), ignoring the real verification outcome stored inresult.common/libzkp/impl/src/verifier.rs (1)
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.
🧹 Nitpick comments (4)
common/libzkp/interface/libzkp.h (1)
11-12: Add documentation for thedump_vkfunction.Please add documentation comments explaining:
- The purpose of the function
- Parameters description and their expected format
- Any error conditions or side effects
coordinator/internal/logic/verifier/types.go (1)
16-16: Add documentation for theOpenVMVkMapfield.Please add a comment explaining the purpose and usage of this field, similar to other VK maps in the struct.
common/libzkp/impl/src/verifier/darwin_v2.rs (1)
49-51: Improve error handling fordump_vk.Instead of using
unreachable!, consider returning a proper error type indicating that this operation is not supported for DarwinV2 verifier. This would provide better error handling and debugging information.- 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<()> { + Err(anyhow::anyhow!("DarwinV2 verifier does not support dumping verification keys - coordinator already has access to DarwinV2 vks")) + }coordinator/internal/types/prover.go (1)
40-41: Add documentation forProverTypeOpenVM.Please add a comment explaining the purpose and usage of this prover type, similar to other prover type constants.
📜 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 (12)
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/logic/auth/login.go(3 hunks)coordinator/internal/logic/verifier/types.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(5 hunks)coordinator/internal/types/prover.go(2 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 (2)
- common/libzkp/impl/rust-toolchain
- common/libzkp/impl/Cargo.toml
🧰 Additional context used
🪛 GitHub Actions: Coordinator
coordinator/internal/logic/verifier/verifier.go
[error] 248-248: cannot use _cgo0 (variable of type *_Ctype_char) as unsafe.Pointer value in argument to _Cfunc_free
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: compile
- GitHub Check: tests
🔇 Additional comments (5)
coordinator/internal/types/prover.go (1)
45-54: Update MakeProverType to handle ProverTypeOpenVM.The MakeProverType function needs to be updated to handle the new ProverTypeOpenVM case.
coordinator/internal/logic/auth/login.go (1)
28-28: LGTM!The changes correctly handle the new
ProverTypeOpenVMcase, following the existing pattern and maintaining consistent error handling.Also applies to: 55-55, 94-98
coordinator/internal/logic/verifier/verifier.go (3)
60-64: LGTM! Well-structured type definition.The
rustVkDumpstruct is well-organized with clear field names and appropriate JSON tags.
72-79: LGTM! Consistent initialization.The OpenVMVkMap is consistently initialized with the same pattern as other VK maps.
126-126: LGTM! Type change from pointer to value.The change from pointer types to value types for proof parameters is consistent across all verification methods. This change aligns with the interface-based design mentioned in the AI summary.
Also applies to: 153-153, 180-180
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
3 times, most recently
from
f0c6ccd to
cf59035
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
rollup/internal/orm/batch.go (1)
125-141: Add error handling for missing proof.The function should return an error if no proof is found in the database.
Apply this diff:
var batch Batch if err := db.Find(&batch).Error; err != nil { return nil, fmt.Errorf("Batch.GetVerifiedProofByHash error: %w, batch hash: %v", err, hash) } + if batch.Proof == nil { + return nil, fmt.Errorf("no proof found for batch hash: %v", hash) + } proof := message.NewBatchProof(hardForkName) if err := json.Unmarshal(batch.Proof, &proof); err != nil { return nil, fmt.Errorf("Batch.GetVerifiedProofByHash error: %w, batch hash: %v", err, hash) } return proof, nil
♻️ Duplicate comments (1)
common/types/message/message.go (1)
277-284:⚠️ Potential issueFix error handling in
OpenVMBatchProof.Proof().Similar to
OpenVMChunkProof.Proof(), this function also panics on JSON marshaling error.Apply this diff to handle errors gracefully:
-func (p *OpenVMBatchProof) Proof() []byte { +func (p *OpenVMBatchProof) Proof() ([]byte, error) { proofJson, err := json.Marshal(p.VmProof) if err != nil { - panic(fmt.Sprint("marshaling error", err)) + return nil, fmt.Errorf("marshaling error: %w", err) } - return []byte(proofJson) + return proofJson, nil }
🧹 Nitpick comments (5)
rollup/tests/rollup_test.go (1)
142-146: Consider using test data constants for better maintainability.The test uses hardcoded byte arrays for proof data. Consider extracting these into test constants for better maintainability.
Apply this diff to improve the test data setup:
+const ( + testProofBytes = []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: testProofBytes, + Instances: testProofBytes, + Vk: testProofBytes, }Also applies to: 157-161
common/types/message/message.go (1)
253-253: Remove unnecessary type conversion.The type conversion
[]byte(proofJson)is unnecessary asproofJsonis already of type[]byte.Apply this diff:
- return []byte(proofJson) + return proofJson🧰 Tools
🪛 GitHub Actions: Common
[error] 253-253: unnecessary conversion (unconvert)
rollup/internal/controller/watcher/chunk_proposer.go (1)
411-421: Add logging for Euclid transition events.The transition from non-Euclid to Euclid blocks is a significant event that should be logged for monitoring and debugging.
Apply this diff to add logging:
// blocks[0] is Euclid, but parent is not, propose a chunk with only blocks[0] + log.Info("Proposing Euclid transition chunk", + "block_number", blocks[0].Header.Number, + "block_time", blocks[0].Header.Time, + "parent_block_number", prevBlocks[0].Header.Number, + "parent_block_time", prevBlocks[0].Header.Time) chunk := encoding.Chunk{Blocks: blocks[:1]}rollup/internal/controller/relayer/l2_relayer_test.go (1)
154-158: Add test cases for different proof types.The test only covers
Halo2BundleProof. Add test cases forOpenVMBundleProofto ensure compatibility with both proof systems.Add test cases for OpenVM proofs:
+ // Test OpenVM proof + openVMProof := &message.OpenVMBundleProof{ + MetaData: struct { + BundleInfo *message.OpenVMBundleInfo `json:"bundle_info"` + BunndlePIHash common.Hash `json:"bundle_pi_hash"` + }{ + BundleInfo: &message.OpenVMBundleInfo{}, + }, + EvmProof: &message.OpenVMEvmProof{ + Proof: []byte{0, 1, 2, 3}, + Instances: [][]string{{"0", "1"}}, + Vk: []byte{0, 1, 2, 3}, + }, + }rollup/internal/orm/orm_test.go (1)
463-464: LGTM! Consider adding documentation for the version parameter.The transition to
Halo2BundleProofand the use of theProof()method aligns with the interface-based design. The addition of the version parameter "darwinV2" suggests multi-version support.Consider adding a comment explaining the supported version values and their implications.
Also applies to: 472-472, 474-474
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcommon/libzkp/impl/Cargo.lockis excluded by!**/*.lockcoordinator/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 (46)
Makefile(1 hunks)bridge-history-api/go.mod(3 hunks)common/go.mod(10 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)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/cmd/tool/tool.go(1 hunks)coordinator/go.mod(4 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/submitproof/proof_receiver.go(2 hunks)coordinator/internal/logic/verifier/mock.go(1 hunks)coordinator/internal/logic/verifier/types.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(5 hunks)coordinator/internal/logic/verifier/verifier_test.go(2 hunks)coordinator/internal/orm/chunk.go(0 hunks)coordinator/internal/types/prover.go(2 hunks)coordinator/test/api_test.go(1 hunks)coordinator/test/mock_prover.go(2 hunks)database/go.mod(2 hunks)rollup/go.mod(4 hunks)rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(2 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/internal/orm/batch.go(3 hunks)rollup/internal/orm/bundle.go(3 hunks)rollup/internal/orm/orm_test.go(4 hunks)rollup/tests/bridge_test.go(1 hunks)rollup/tests/rollup_test.go(1 hunks)tests/integration-test/go.mod(1 hunks)
💤 Files with no reviewable changes (2)
- coordinator/internal/orm/chunk.go
- common/libzkp/impl/src/verifier/darwin.rs
🚧 Files skipped from review as they are similar to previous changes (31)
- common/libzkp/interface/libzkp.h
- coordinator/test/api_test.go
- coordinator/internal/logic/verifier/types.go
- rollup/internal/controller/watcher/bundle_proposer.go
- common/libzkp/impl/rust-toolchain
- common/libzkp/impl/src/lib.rs
- Makefile
- coordinator/internal/types/prover.go
- rollup/internal/controller/watcher/l1_watcher.go
- coordinator/cmd/tool/tool.go
- rollup/internal/controller/watcher/l2_watcher.go
- coordinator/internal/logic/auth/login.go
- coordinator/test/mock_prover.go
- coordinator/internal/logic/verifier/mock.go
- common/types/message/message_test.go
- rollup/internal/controller/sender/sender.go
- rollup/internal/controller/watcher/batch_proposer.go
- rollup/internal/orm/bundle.go
- rollup/internal/controller/sender/sender_test.go
- common/types/message/bundle-proof-sample.json
- coordinator/internal/logic/submitproof/proof_receiver.go
- coordinator/internal/logic/verifier/verifier_test.go
- rollup/tests/bridge_test.go
- common/libzkp/impl/src/verifier/euclid.rs
- coordinator/internal/logic/verifier/verifier.go
- coordinator/internal/logic/provertask/bundle_prover_task.go
- rollup/internal/controller/relayer/l2_relayer.go
- database/go.mod
- common/libzkp/impl/Cargo.toml
- coordinator/internal/logic/provertask/batch_prover_task.go
- common/go.mod
🧰 Additional context used
🪛 GitHub Actions: Common
common/types/message/message.go
[error] 253-253: unnecessary conversion (unconvert)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (25)
common/libzkp/impl/src/verifier/darwin_v2.rs (1)
30-30: LGTM!The change from
'paramsto'_is a safe simplification that allows the compiler to infer the lifetime parameter.rollup/tests/rollup_test.go (1)
55-55: Function name suggests support for multiple codec versions but only CodecV4 is used.The function name implies support for CodecV4, V5, and V6, but the
ChunkProposeris initialized with onlyCodecV4. Consider either:
- Adding test cases for CodecV5 and CodecV6, or
- Renaming the function to reflect the actual codec version being tested.
Also applies to: 87-95
coordinator/internal/logic/provertask/chunk_prover_task.go (1)
145-152: LGTM!The added check for incompatible prover versions is well-implemented with appropriate error handling and logging.
common/types/message/message.go (1)
303-307: Implement missing validation for VK.The TODO comments indicate that VK validation is temporarily disabled. This could lead to security issues if not properly validated.
Also applies to: 360-364
rollup/internal/controller/watcher/chunk_proposer.go (1)
395-403:⚠️ Potential issueAdd genesis block check in
tryProposeEuclidTransitionChunk.The function could panic when accessing
blocks[0].Header.Number.Uint64()-1if the block number is 0 (genesis block).Apply this diff to handle the genesis block case:
+ if blocks[0].Header.Number.Uint64() == 0 { + 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) if err != nil || len(prevBlocks) == 0 || prevBlocks[0].Header.Hash() != blocks[0].Header.ParentHash { return false, fmt.Errorf("failed to get parent block: %w", err) }Likely invalid or redundant comment.
rollup/internal/orm/orm_test.go (1)
486-487: LGTM! Test coverage looks good.The test case properly validates both the proof data and metadata after the transition to
Halo2BundleProof.Also applies to: 499-502
tests/integration-test/go.mod (1)
21-21: Pinned dependency requires explanation.The dependency "github.com/deckarep/golang-set" is pinned to a specific commit. This was previously flagged in an earlier review.
coordinator/go.mod (1)
3-5: LGTM! Version updates are consistent.The Go version update and dependency changes align with the project-wide updates and the new proof type implementation.
Also applies to: 14-15
bridge-history-api/go.mod (1)
118-118: Verify logging implementation after dependency change.The replacement of
lumberjack.v2withnpipe.v2suggests a change in the logging implementation.Please confirm that the logging functionality is properly maintained after this dependency change.
rollup/go.mod (16)
3-5: Update Go Version and Toolchain
The module now targets Go 1.22 with an explicit toolchain version (go1.22.2). This update ensures the project can leverage new language features and optimizations. Please verify downstream dependencies work as expected with Go 1.22.
9-10: Upgrade Critical Cryptography Dependencies
The upgrades for "github.com/consensys/gnark-crypto" to v0.16.0 and "github.com/crate-crypto/go-kzg-4844" to v1.1.0 improve compatibility and security. Ensure that any code consuming these libraries is tested against the new versions.
13-13: Upgrade uint256 Dependency
"www.github.com/holiman/uint256" has been updated to v1.3.2. This change should bring improvements and fixes from the previous v1.2.4 release.
16-17: Upgrade Blockchain-Related Dependencies
Both "github.com/scroll-tech/da-codec" and "github.com/scroll-tech/go-ethereum" have been updated to newer versions. Confirm that all interactions with these packages in your codebase are compatible with the updated APIs.
20-20: Upgrade Testing Library
The dependency "github.com/stretchr/testify" now points to v1.10.0. This upgrade helps keep testing utilities up-to-date with bug fixes and new features.
28-28: Upgrade Bitset Library
"Github.com/bits-and-blooms/bitset" has been updated to v1.20.0. This version increment likely includes performance improvements and bug fixes.
34-34: Upgrade Consensus-Related Utility
The update to "github.com/consensys/bavard" v0.1.29 ensures you benefit from the latest improvements in this package. Confirm that any dependent modules adjust to any API changes.
37-38: Update Utility Dependencies
The "github.com/deckarep/golang-set" dependency has been updated to commit v0.0.0-20180603214616-504e848d77ea, and "github.com/edsrzf/mmap-go" is now at v1.0.0. For "golang-set", please verify that the chosen commit meets the project’s long-term maintenance and compatibility requirements.
53-53: Upgrade go-cmp Library
"Github.com/google/go-cmp" is now at v0.6.0. This update should improve diffing functionalities in tests or comparisons.
58-58: Upgrade Hashicorp LRU Cache
The package "github.com/hashicorp/golang-lru" has been updated to v0.5.5-0.20210104140557-80c98217689d. Validate that cache-related behaviors continue to perform as expected.
62-62: Upgrade iden3 Crypto Library
The upgrade to "github.com/iden3/go-iden3-crypto" v0.0.17 suggests improvements and bug fixes from the previous version. Ensure that any cryptography-related flows integrate smoothly after this change.
105-105: Upgrade BLST Library
"Github.com/supranational/blst" has been incremented to v0.3.13. This minor upgrade likely includes important optimizations or fixes.
108-108: Upgrade NumCPUs Package
The dependency "github.com/tklauser/numcpus" is updated to v0.9.0, which should help in better CPU utilization detection.
117-117: Upgrade Cryptography Package
The update to "golang.org/x/crypto" v0.32.0 reflects significant changes from the previous v0.24.0 version. This revision might affect cryptographic primitives—please re-run relevant tests.
120-122: Upgrade Synchronization Libraries
"golang.org/x/sync", "golang.org/x/sys", and "golang.org/x/text" have been updated to v0.11.0, v0.30.0, and v0.21.0 respectively. These updates are important for ensuring better performance and stability in concurrent operations.
126-127: Dependency Replacement and CLI Version Verification
The file now uses "gopkg.in/natefinch/npipe.v2" (v2.0.0-20160621034901-c1b8fa8bdcce) to replace a previously used lumberjack package. Additionally, "gopkg.in/urfave/cli.v1" is pinned to v1.20.0 even though "github.com/urfave/cli/v2" is declared in the main require block. Please verify that having both CLI v1 and v2 does not lead to version conflicts or unexpected behavior in your application.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
from
cf59035 to
d7812d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
prover/src/main.rs (1)
44-44:⚠️ Potential issueAdd error handling for prover.run().
The
run()call should handle potential errors instead of ignoring them.- prover.run().await; + prover.run().await?;common/libzkp/impl/src/verifier.rs (1)
1-2:⚠️ 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::OnceCellcommon/types/message/message.go (2)
78-81: 🛠️ Refactor suggestionAdd SanityCheck to ChunkProof interface.
The
ChunkProofinterface only requiresProof()method whileBatchProofandBundleProofinterfaces require bothProof()andSanityCheck(). For consistency and validation, consider addingSanityCheck()to theChunkProofinterface.type ChunkProof interface { Proof() []byte + SanityCheck() error }
247-254:⚠️ Potential issueFix error handling in OpenVMChunkProof.Proof().
The function panics on JSON marshaling error, which could crash the application. Error handling should be consistent with other methods that return errors.
Apply this diff to handle errors gracefully:
-func (p *OpenVMChunkProof) Proof() []byte { +func (p *OpenVMChunkProof) Proof() ([]byte, error) { proofJson, err := json.Marshal(p.VmProof) if err != nil { - panic(fmt.Sprint("marshaling error", err)) + return nil, fmt.Errorf("marshaling error: %w", err) } - return []byte(proofJson) + return proofJson, nil }🧰 Tools
🪛 GitHub Actions: Common
[error] 253-253: unnecessary conversion (unconvert)
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)coordinator/internal/logic/submitproof/proof_receiver.go (1)
178-178:⚠️ Potential issueVerify verifier interface implementation.
The verifier methods
VerifyChunkProof,VerifyBatchProof, andVerifyBundleProofare undefined in the*verifier.Verifiertype. Ensure these methods are properly implemented in the verifier package.Run the following script to check the verifier interface implementation:
#!/bin/bash # Description: Check if verifier methods are implemented. # Search for verifier interface definition ast-grep --pattern $'interface { $$$ VerifyChunkProof($_, $_) (bool, error) VerifyBatchProof($_, $_) (bool, error) VerifyBundleProof($_, $_) (bool, error) $$$ }' # Search for verifier implementation ast-grep --pattern $'type Verifier struct { $$$ } func (v *Verifier) VerifyChunkProof($_, $_) (bool, error) { $$$ } func (v *Verifier) VerifyBatchProof($_, $_) (bool, error) { $$$ } func (v *Verifier) VerifyBundleProof($_, $_) (bool, error) { $$$ }'Also applies to: 184-184, 190-190
🧰 Tools
🪛 golangci-lint (1.62.2)
178-178: m.verifier.VerifyChunkProof undefined (type *verifier.Verifier has no field or method VerifyChunkProof)
(typecheck)
🧹 Nitpick comments (5)
prover/src/zk_circuits_handler.rs (2)
1-1: Add documentation for the neweuclidmodule.Consider adding module-level documentation to explain the purpose and functionality of the new
euclidmodule.+/// Module implementing Euclid-specific circuit handling functionality. pub mod euclid;
8-11: Add documentation for theCircuitsHandlertrait and its methods.The trait and its methods would benefit from documentation explaining their purpose, parameters, and return values, especially since they're public and async.
+/// Trait for handling circuit-related operations asynchronously. #[async_trait] pub trait CircuitsHandler: Sync + Send { + /// Retrieves the verification key for a given proof type. + /// Returns the key as a byte vector if available. async fn get_vk(&self, task_type: ProofType) -> Option<Vec<u8>>; + /// Generates proof data for a given prove request. + /// Returns the proof data as a string or an error if generation fails. async fn get_proof_data(&self, prove_request: ProveRequest) -> Result<String>; }common/libzkp/impl/src/verifier.rs (1)
22-26: Add documentation for theVKDumpstruct.The new
VKDumpstruct would benefit from documentation explaining its purpose and field meanings.+/// Represents a collection of verification keys for different proof types. #[derive(Debug, Serialize, Deserialize)] pub struct VKDump { + /// Verification key for chunk proofs pub chunk_vk: String, + /// Verification key for batch proofs pub batch_vk: String, + /// Verification key for bundle proofs pub bundle_vk: String, }rollup/tests/rollup_test.go (1)
142-146: Use meaningful test data instead of hardcoded bytes.The hardcoded proof data might not be representative of real proofs and could miss edge cases.
Consider using test data that represents different proof scenarios or generating proofs dynamically.
common/types/message/message.go (1)
303-307: Implement missing functionality marked with TODO.Several TODO comments indicate missing functionality:
- OpenVMBundleProof has commented-out validation for bundle info and VK readiness.
Would you like me to help implement these missing functions?
Also applies to: 352-364
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcommon/libzkp/impl/Cargo.lockis excluded by!**/*.lockcoordinator/go.sumis excluded by!**/*.sumdatabase/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumprover/Cargo.lockis excluded by!**/*.lockrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (70)
Makefile(1 hunks)bridge-history-api/go.mod(3 hunks)common/go.mod(10 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)common/types/message/bundle-proof-sample.json(1 hunks)common/types/message/message.go(7 hunks)common/types/message/message_test.go(1 hunks)common/version/version.go(1 hunks)coordinator/cmd/tool/tool.go(1 hunks)coordinator/go.mod(4 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/submitproof/proof_receiver.go(2 hunks)coordinator/internal/logic/verifier/mock.go(1 hunks)coordinator/internal/logic/verifier/types.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(5 hunks)coordinator/internal/logic/verifier/verifier_test.go(2 hunks)coordinator/internal/orm/chunk.go(0 hunks)coordinator/internal/types/prover.go(2 hunks)coordinator/test/api_test.go(1 hunks)coordinator/test/mock_prover.go(2 hunks)database/go.mod(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/go.mod(4 hunks)rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(2 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/internal/orm/batch.go(3 hunks)rollup/internal/orm/bundle.go(3 hunks)rollup/internal/orm/orm_test.go(4 hunks)rollup/tests/bridge_test.go(1 hunks)rollup/tests/rollup_test.go(1 hunks)tests/integration-test/go.mod(1 hunks)
💤 Files with no reviewable changes (17)
- prover/src/coordinator_client/listener.rs
- prover/src/coordinator_client.rs
- prover/src/zk_circuits_handler/common.rs
- prover/src/utils.rs
- coordinator/internal/orm/chunk.go
- common/libzkp/impl/src/verifier/darwin.rs
- prover/src/coordinator_client/api.rs
- prover/src/task_processor.rs
- prover/src/config.rs
- prover/src/geth_client.rs
- prover/src/zk_circuits_handler/darwin.rs
- prover/src/key_signer.rs
- prover/src/task_cache.rs
- prover/src/version.rs
- prover/src/zk_circuits_handler/darwin_v2.rs
- prover/src/coordinator_client/errors.rs
- prover/src/coordinator_client/types.rs
🚧 Files skipped from review as they are similar to previous changes (33)
- prover/rust-toolchain
- common/libzkp/interface/libzkp.h
- coordinator/internal/logic/verifier/types.go
- common/libzkp/impl/rust-toolchain
- rollup/internal/controller/watcher/bundle_proposer.go
- Makefile
- rollup/internal/controller/watcher/l1_watcher.go
- common/types/message/bundle-proof-sample.json
- coordinator/internal/types/prover.go
- common/libzkp/impl/src/lib.rs
- coordinator/internal/logic/provertask/chunk_prover_task.go
- common/types/message/message_test.go
- common/libzkp/impl/src/verifier/darwin_v2.rs
- rollup/internal/controller/sender/sender_test.go
- common/version/version.go
- coordinator/cmd/tool/tool.go
- coordinator/internal/logic/verifier/mock.go
- rollup/tests/bridge_test.go
- rollup/internal/controller/watcher/batch_proposer.go
- rollup/internal/controller/sender/sender.go
- rollup/internal/controller/watcher/l2_watcher.go
- coordinator/internal/logic/verifier/verifier_test.go
- common/libzkp/impl/src/verifier/euclid.rs
- prover/config.json
- coordinator/internal/logic/provertask/batch_prover_task.go
- rollup/internal/orm/batch.go
- coordinator/test/api_test.go
- database/go.mod
- rollup/internal/orm/bundle.go
- common/libzkp/impl/Cargo.toml
- coordinator/internal/logic/verifier/verifier.go
- rollup/internal/controller/relayer/l2_relayer.go
- common/go.mod
🧰 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)
🪛 GitHub Actions: Common
common/types/message/message.go
[error] 253-253: unnecessary conversion (unconvert)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: compile
- GitHub Check: tests
🔇 Additional comments (45)
prover/src/main.rs (1)
39-42: 🛠️ Refactor suggestionAdd validation for the prover configuration.
The configuration loading and prover initialization lack validation of critical settings.
let cfg = LocalProverConfig::from_file(args.config_file)?; + cfg.validate()?; let sdk_config = cfg.sdk_config.clone(); let local_prover = LocalProver::new(cfg);prover/src/types.rs (1)
8-8: Verify task_type and proof_type field compatibility.The transition to
CircuitTypefor bothtask_typeandproof_typefields may affect existing code that relies on the previous enum values.#!/bin/bash # Search for any direct usage of the old TaskType or ProverType enums rg -A 3 "TaskType::|ProverType::"Also applies to: 18-18
common/libzkp/impl/src/verifier.rs (1)
80-107: Replaceunwrap_unchecked()with safe alternatives.Using
unwrap_unchecked()is unsafe and can lead to undefined behavior if the value is not initialized.- PARAMS_MAP.set(params_map).unwrap_unchecked(); + if let Err(_) = PARAMS_MAP.set(params_map) { + panic!("Failed to set PARAMS_MAP: already initialized"); + } - VERIFIER_HALO2 - .set(VerifierPair( - config.low_version_circuit.fork_name.clone(), - Rc::new(Box::new(verifier)), - )) - .unwrap_unchecked(); + if let Err(_) = VERIFIER_HALO2.set(VerifierPair( + config.low_version_circuit.fork_name.clone(), + Rc::new(Box::new(verifier)), + )) { + panic!("Failed to set VERIFIER_HALO2: already initialized"); + } - VERIFIER_ZKVM - .set(VerifierPair( - config.high_version_circuit.fork_name, - Rc::new(Box::new(verifier)), - )) - .unwrap_unchecked(); + if let Err(_) = VERIFIER_ZKVM.set(VerifierPair( + config.high_version_circuit.fork_name, + Rc::new(Box::new(verifier)), + )) { + panic!("Failed to set VERIFIER_ZKVM: already initialized"); + }prover/src/zk_circuits_handler/euclid.rs (4)
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.
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("chunk/app.vmexe"); let config = workspace_path.join("chunk/openvm.toml"); T::setup(exe, config, Some(cache_dir.to_path_buf())) .map_err(|e| anyhow!("Failed to setup {} prover: {}", prover_type, e)) } }
62-79: Replace unwrap() with proper error handling.Using unwrap() can lead to panics. Consider using proper error handling with Result types.
Apply this diff to improve error handling:
- let vk = self - .active_handler - .as_ref() - .unwrap() - .1 - .get_vk(proof_type) - .await; + let vk = match &self.active_handler { + Some((_, handler)) => handler.get_vk(proof_type).await, + None => { + return GetVkResponse { + vks: vec![], + error: Some("No active handler".to_string()), + } + } + }; if let Some(vk) = vk { - vks.push(String::from_utf8(vk).unwrap()); + match String::from_utf8(vk) { + Ok(s) => vks.push(s), + Err(e) => { + return GetVkResponse { + vks, + error: Some(format!("Invalid UTF-8 in vk: {}", e)), + } + } + } }
173-189: Handle missing circuit config instead of unwrapping.Using unwrap() on config.circuits.get() will panic if hard_fork_name is not present.
Consider returning a descriptive error:
- let config = self.config.circuits.get(hard_fork_name).unwrap(); + let config = match self.config.circuits.get(hard_fork_name) { + Some(cfg) => cfg, + None => { + // Handle gracefully, e.g. logging or returning an error + panic!("Unknown hard fork name: {}", hard_fork_name); + } + };coordinator/internal/logic/auth/login.go (2)
28-28: LGTM!The new field
openVmVksfollows the established pattern for verification key maps and is properly initialized.Also applies to: 55-55
94-97: LGTM!The implementation for
ProverTypeOpenVMfollows the established pattern and is consistent with other prover type handlers.prover/src/prover.rs (5)
23-41: LGTM!The
LocalProverConfigimplementation provides good error handling and follows Rust best practices for configuration management.
95-133: LGTM!The implementation properly handles all error cases and follows good practices for task status management.
80-93:⚠️ Potential issueReplace unwrap() with proper error handling in prove method.
Using unwrap() on active_handler can lead to panics.
Apply this diff to improve error handling:
- match self - .do_prove(req, self.active_handler.as_ref().unwrap().1.clone()) - .await + let handler = match &self.active_handler { + Some((_, h)) => h.clone(), + None => { + return ProveResponse { + status: TaskStatus::Failed, + error: Some("No active handler".to_string()), + ..Default::default() + } + } + }; + match self.do_prove(req, handler).awaitLikely invalid or redundant comment.
62-79:⚠️ Potential issueReplace unwrap() with proper error handling.
Using unwrap() can lead to panics. Consider using proper error handling with Result types.
Apply this diff to improve error handling:
- let vk = self - .active_handler - .as_ref() - .unwrap() - .1 - .get_vk(proof_type) - .await; + let vk = match &self.active_handler { + Some((_, handler)) => handler.get_vk(proof_type).await, + None => { + return GetVkResponse { + vks: vec![], + error: Some("No active handler".to_string()), + } + } + }; if let Some(vk) = vk { - vks.push(String::from_utf8(vk).unwrap()); + match String::from_utf8(vk) { + Ok(s) => vks.push(s), + Err(e) => { + return GetVkResponse { + vks, + error: Some(format!("Invalid UTF-8 in vk: {}", e)), + } + } + } }Likely invalid or redundant comment.
173-189:⚠️ Potential issueReplace unwrap() and unreachable!() with proper error handling.
Using unwrap() and unreachable!() can lead to panics.
Apply this diff to improve error handling:
- let config = self.config.circuits.get(hard_fork_name).unwrap(); + let config = match self.config.circuits.get(hard_fork_name) { + Some(cfg) => cfg, + None => { + log::error!("Unknown hard fork name: {}", hard_fork_name); + return; + } + }; let handler = Arc::new(match hard_fork_name { "euclid" => Arc::new(Mutex::new(EuclidHandler::new(&config.workspace_path))), - _ => unreachable!(), + _ => { + log::error!("Unsupported hard fork name: {}", hard_fork_name); + return; + } }) as Arc<dyn CircuitsHandler>;Likely invalid or redundant comment.
coordinator/test/mock_prover.go (3)
204-207: LGTM: Status type transition.The change from
messagetotypespackage for status constants is consistent with the codebase's architectural changes.
212-213: LGTM: Proof type updates.The transition to
Halo2ChunkProofandHalo2BatchProofaligns with the new interface-based design inmessage.go.Also applies to: 217-218
226-228: LGTM: Field name updates.The field name change from
ProoftoRawProofis consistent with the new proof structure design.Also applies to: 233-235
coordinator/internal/logic/provertask/bundle_prover_task.go (2)
147-154: LGTM: Hard fork compatibility check.The addition of hard fork name compatibility check enhances error handling and prevents incompatible prover versions from processing tasks.
224-231: LGTM: Proof type updates.The transition from pointer slice to value slice for
batchProofsaligns with the new proof handling design and may improve memory management.rollup/internal/controller/watcher/chunk_proposer.go (1)
395-422: LGTM: Euclid transition handling.The new method
tryProposeEuclidTransitionChunkcorrectly handles the transition phase by ensuring that blocks from different hard forks are not mixed in the same chunk.coordinator/internal/logic/submitproof/proof_receiver.go (2)
174-174: LGTM: Improved proof initialization using constructor functions.The use of constructor functions (
NewChunkProof,NewBatchProof,NewBundleProof) for proof initialization is a good practice as it ensures proper initialization and encapsulation.Also applies to: 180-180, 186-186
268-268: LGTM: Updated status check to use coordinatorType.StatusOk.The status check has been correctly updated to use
coordinatorType.StatusOkinstead ofmessage.StatusOk, improving type consistency.rollup/internal/controller/relayer/l2_relayer_test.go (2)
126-130: LGTM: Added chunk insertion before batch processing.The addition of chunk insertion before batch processing ensures proper data setup and maintains referential integrity.
154-156: LGTM: Updated proof type to use Halo2BundleProof.The proof type has been correctly updated to use
message.Halo2BundleProofwith theRawProoffield, aligning with the new proof type system.rollup/internal/orm/orm_test.go (3)
463-465: LGTM: Updated proof type to use Halo2BundleProof.The proof type has been correctly updated to use
message.Halo2BundleProofwith theRawProoffield, aligning with the new proof type system.Also applies to: 486-488
472-474: LGTM: Added version parameter to GetVerifiedProofByHash.The addition of the version parameter ("darwinV2") to
GetVerifiedProofByHashimproves version control and compatibility.
474-474: LGTM: Updated proof access to use Proof() method.The proof access has been correctly updated to use the
Proof()method instead of direct field access, following the interface-based design.Also applies to: 502-502
tests/integration-test/go.mod (3)
21-21: Consider updating golang-set dependency.The dependency "github.com/deckarep/golang-set" is pinned to an old commit (v0.0.0-20180603214616-504e848d77ea). Consider updating to a newer version or adding a comment explaining the pinning.
3-5: Verify Go version compatibility.The upgrade to Go 1.22 with toolchain go1.22.2 may introduce breaking changes. Please ensure all dependencies are compatible with this version.
8-10: Verify dependency version compatibility.The updates to major dependencies need verification:
- da-codec: v0.1.2 → v0.1.3-0.20241218102542-9852fa4e1be5
- go-ethereum: v1.10.14-0.20241011150208-4742882675d8 → v1.10.14-0.20250205135740-4bdf6d096c38
- testify: v1.9.0 → v1.10.0
✅ Verification successful
🌐 Web query:
What are the latest stable versions of: 1. github.com/scroll-tech/da-codec 2. github.com/scroll-tech/go-ethereum 3. github.com/stretchr/testify💡 Result:
Here are the latest stable versions based on available information: ## 1. github.com/scroll-tech/da-codec - **Latest Stable Version**: No specific version information found in provided sources[10]. The repository appears active but doesn't show formal releases in search results. ## 2. github.com/scroll-tech/go-ethereum - **Latest Stable Version**: No explicit version data found for Scroll's fork[3][9]. The original Ethereum go-ethereum shows v1.15.0 as its latest release[3], but Scroll's fork might have different versioning. ## 3. github.com/stretchr/testify - **Latest Stable Version**: **v1.10.0** - Added Go 1.18 support[2] - Dependency updates and CI improvements[2] - Fedora packages list it as stable in Rawhide[5] Key differences between components: | Component | Release Status | Source | |-----------|----------------|--------| | da-codec | Active development | [GitHub][10] | | testify | Actively maintained | [GitHub][2][5] | Citations: - 1: https://shark007.net/x64components1.html - 2: https://github.com/stretchr/testify/releases - 3: https://github.com/ethereum/go-ethereum/releases - 4: https://docs.nvidia.com/cuda/cuda-toolkit-release-notes/index.html - 5: https://packages.fedoraproject.org/pkgs/golang-github-stretchr-testify/golang-github-stretchr-testify-devel/ - 6: https://github.com/scroll-tech - 7: https://www.videohelp.com/software/K-Lite-Codec-Pack - 8: https://pkg.go.dev/github.com/stretchr/testify/suite - 9: https://github.com/scroll-tech/scroll - 10: https://github.com/scroll-tech/da-codec
Dependency Versions Verified
- da-codec: The update to v0.1.3-0.20241218102542-9852fa4e1be5 reflects an active development strategy with commit-based versioning. Since a formal “stable” version isn’t defined, this is acceptable as long as integration tests pass.
- go-ethereum: The change to v1.10.14-0.20250205135740-4bdf6d096c38 also follows a commit-based versioning approach from the Scroll fork. Although upstream go-ethereum has a different release cadence, this update appears intentional for the fork.
- testify: Upgrading from v1.9.0 to v1.10.0 aligns with the latest stable version available and comes with the expected improvements.
Based on the available information, these dependency updates are in line with current repository practices. Please ensure the integration tests fully validate that no breaking changes occurred due to the upgrades.
prover/Cargo.toml (3)
10-16: Dependency Patch Updates: Ensure Reproducibility
The updated [patch.crates-io] section now pulls in new dependencies (ruint, alloy‑primitives, revm and its related crates) from Git using branch names (e.g. "scroll-evm-executor/v50"). Consider whether pinning to a specific commit or tag would improve reproducibility and reduce surprises from upstream changes.
28-33: New Proving Dependencies: Confirm Integration and Documentation
New dependencies—scroll‑proving‑sdk, scroll‑zkvm‑prover, and sbv‑primitives—have been added to the [dependencies] section. Make sure that their APIs are fully integrated into the prover’s logic and that any related changes (for example, new proof construction or verification code) are well documented.
46-52: Auxiliary Dependency Additions: Validate Necessity and Versioning
The additions of async‑trait, url, and serde_bytes support asynchronous operations and serialization needs. Please verify that these dependencies are in line with the module’s requirements and that their versions are compatible with other parts of the codebase.coordinator/go.mod (3)
3-5: Go Version and Toolchain Upgrade
Upgrading the module to Go 1.22 with the specified toolchain (go1.22.2) is a positive step toward leveraging the latest language features and performance improvements. Ensure that all parts of the coordinator and its dependencies are fully compatible with this version.
14-15: Updated Dependency Versions: Watch for API Changes
The versions for github.com/scroll‑tech/da‑codec and github.com/scroll‑tech/go‑ethereum have been updated. It is important to double‑check that these versions do not introduce breaking changes and that any API differences are addressed in the coordinator’s implementation.
17-17: Testify Version Update: Confirm Test Stability
The upgrade to github.com/stretchr/testify v1.10.0 appears standard. Please run the full test suite to ensure that custom assertions or test helper behaviors have not been affected.bridge-history-api/go.mod (3)
3-5: Go Version and Toolchain Alignment
Changing the module’s Go version to 1.22 and specifying toolchain go1.22.2 is in line with the broader project updates. Verify that your CI and local environments are updated accordingly.
13-16: Core Dependency Updates: Monitor Breaking Changes
The update for github.com/scroll‑tech/go‑ethereum and the upgrade of github.com/stretchr/testify (as well as the adjustment for golang.org/x/sync to v0.11.0) should be checked against the module’s test and integration suites to catch any incompatibilities.
22-36: Indirect Dependencies Update: Ensure Compatibility
Several indirect dependency versions have been bumped (including fastcache, bits‑and‑blooms/bitset, consen‑sys/bavard, gnark‑crypto, crate‑crypto/go‑kzg‑4844, deckarep/golang‑set, and mmap‑go). It’s recommended to run a full dependency audit and test suite to detect any transitive conflicts or unexpected behaviors stemming from these updates.rollup/go.mod (6)
3-5: Go Version and Toolchain Upgrade
The rollup module now targets Go 1.22 with toolchain go1.22.2. This upgrade should position the module to benefit from the latest language improvements. Ensure all build systems and integration tests are adjusted accordingly.
9-10: Cryptography Library Upgrades: Check API Adjustments
Upgrading github.com/consensys/gnark‑crypto to v0.16.0 and github.com/crate‑crypto/go‑kzg‑4844 to v1.1.0 may introduce API changes or improvements in performance/security. Please review the library changelogs and verify that integration points in the rollup module are updated.
13-13: Uint256 Update: Validate Behavior
The update of github.com/holiman/uint256 to v1.3.2 should be reviewed for any changes in arithmetic behavior or API adjustments.
16-17: Ethereum & Proving Dependency Updates: Confirm Integration
The new versions for github.com/scroll‑tech/da‑codec and github.com/scroll‑tech/go‑ethereum have been incorporated. Confirm that these changes are reflected in the rollup’s handling of Ethereum interactions and proof processing.
20-20: Testify Version Update: Execute Complete Test Suite
Upgrading github.com/stretchr/testify to v1.10.0 is standard practice; ensure that your tests run without issues under the new version.
26-131: Indirect Dependencies Overhaul in Rollup Module
The secondrequireblock exhibits extensive updates across numerous indirect dependencies—including updates to bitset, bavard, golang‑set, mmap‑go, google/go‑cmp, hashicorp/golang‑lru, iden3/go‑iden3‑crypto, prometheus/tsdb, rjeczalik/notify, supranational/blst, tklauser/numcpus, and various golang.org/x packages, as well as replacements (e.g. switching from lumberjack to natefinch/npipe and adding urfave/cli.v1). This comprehensive overhaul is aimed at enhancing security, performance, and compatibility. A full dependency audit and regression testing cycle is strongly recommended to catch any unforeseen issues.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
from
d7812d1 to
f3470f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
common/libzkp/impl/src/verifier/darwin_v2.rs (1)
49-51:⚠️ Potential issueReplace
unreachable!with proper error handling.Using
unreachable!macro is unsafe as it will cause a panic if the code is executed.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") + }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
80-81:⚠️ 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:- PARAMS_MAP.set(params_map).unwrap_unchecked(); + if let Err(_) = PARAMS_MAP.set(params_map) { + panic!("Failed to set PARAMS_MAP: already initialized"); + } - VERIFIER_HALO2 - .set(VerifierPair( - config.low_version_circuit.fork_name.clone(), - Rc::new(Box::new(verifier)), - )) - .unwrap_unchecked(); + if let Err(_) = VERIFIER_HALO2.set(VerifierPair( + config.low_version_circuit.fork_name.clone(), + Rc::new(Box::new(verifier)), + )) { + panic!("Failed to set VERIFIER_HALO2: already initialized"); + } - VERIFIER_ZKVM - .set(VerifierPair( - config.high_version_circuit.fork_name, - Rc::new(Box::new(verifier)), - )) - .unwrap_unchecked(); + if let Err(_) = VERIFIER_ZKVM.set(VerifierPair( + config.high_version_circuit.fork_name, + Rc::new(Box::new(verifier)), + )) { + panic!("Failed to set VERIFIER_ZKVM: already initialized"); + }Also applies to: 88-94, 101-107
common/types/message/message.go (2)
247-254:⚠️ Potential issueFix error handling in
OpenVMChunkProof.Proof().The function panics on JSON marshaling error, which could crash the application. Apply this diff to handle errors gracefully:
-func (p *OpenVMChunkProof) Proof() []byte { +func (p *OpenVMChunkProof) Proof() ([]byte, error) { proofJson, err := json.Marshal(p.VmProof) if err != nil { - panic(fmt.Sprint("marshaling error", err)) + return nil, fmt.Errorf("marshaling error: %w", err) } - return []byte(proofJson) + return proofJson, nil }
49-52: 🛠️ Refactor suggestionImprove type safety in BatchTaskDetail.
The
interface{}type forBatchHeaderis too generic and could lead to runtime type errors. Consider using a concrete type or defining an interface with required methods.
🧹 Nitpick comments (2)
coordinator/internal/logic/provertask/batch_prover_task.go (2)
147-154: LGTM! Version compatibility check is well-implemented.The version compatibility check is crucial for preventing task assignment to incompatible provers. The error handling is comprehensive, including proper cleanup of active attempts.
Consider structuring the error log as a single line for better log aggregation:
- log.Error("incompatible prover version", - "requisite hard fork name", hardForkName, - "prover hard fork name", taskCtx.HardForkNames, - "task_id", batchTask.Hash) + log.Error(fmt.Sprintf("incompatible prover version: required=%s available=%v task_id=%s", + hardForkName, taskCtx.HardForkNames, batchTask.Hash))
218-239: LGTM! Proof handling changes improve type safety.The changes align well with the interface-based design and improve memory management by reducing pointer indirection. The type assertion is properly handled.
Consider adding an error check for cases where the type assertion fails but
ChunkInfois required:if haloProot, ok := proof.(*message.Halo2ChunkProof); ok { if haloProot.ChunkInfo != nil { chunkInfo.TxBytes = haloProot.ChunkInfo.TxBytes } + } else if len(chunkInfo.TxBytes) == 0 { + log.Warn("TxBytes not available: non-Halo2 proof type", + "proof_type", fmt.Sprintf("%T", proof)) }
📜 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 (19)
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)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/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(5 hunks)coordinator/internal/types/prover.go(2 hunks)rollup/internal/controller/sender/sender_test.go(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 (13)
- coordinator/internal/logic/provertask/chunk_prover_task.go
- coordinator/internal/logic/verifier/types.go
- common/libzkp/impl/src/lib.rs
- coordinator/internal/types/prover.go
- common/types/message/bundle-proof-sample.json
- common/libzkp/interface/libzkp.h
- common/types/message/message_test.go
- coordinator/internal/logic/auth/login.go
- rollup/internal/controller/sender/sender_test.go
- common/libzkp/impl/rust-toolchain
- coordinator/internal/logic/verifier/verifier.go
- common/libzkp/impl/src/verifier/euclid.rs
- common/libzkp/impl/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: compile
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (6)
common/libzkp/impl/src/verifier/darwin_v2.rs (1)
30-30: LGTM: Simplified lifetime parameter.The change from
impl<'params> ProofVerifier for DarwinV2Verifier<'params>toimpl ProofVerifier for DarwinV2Verifier<'_>is a good simplification that maintains the same functionality.coordinator/internal/logic/provertask/bundle_prover_task.go (2)
147-154: LGTM: Added version compatibility check.Good addition of version compatibility check to prevent assigning tasks to incompatible provers.
224-231: LGTM: Updated BatchProof type to use value semantics.The change from pointer slice to value slice for BatchProof aligns with the new interface-based design.
common/types/message/message.go (2)
343-346: Implement missing functionality marked with TODO.Several TODO comments indicate missing functionality:
- Bundle info metadata validation is commented out
- VK validation is commented out with "resume after rc2"
Would you like me to help implement these missing validations?
Also applies to: 360-364
79-81: Add SanityCheck to ChunkProof interface.The
ChunkProofinterface only requiresProof()method whileBatchProofandBundleProofinterfaces require bothProof()andSanityCheck(). For consistency and validation, consider addingSanityCheck()to theChunkProofinterface.type ChunkProof interface { Proof() []byte + SanityCheck() error }coordinator/internal/logic/provertask/batch_prover_task.go (1)
269-269: LGTM! Method signature update is consistent with the new design.The parameter type change from
[]*message.ChunkProofto[]message.ChunkProofaligns with the interface-based design and improves memory efficiency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
common/types/message/message.go (3)
79-81: 🛠️ Refactor suggestionAdd SanityCheck to ChunkProof interface.
The
ChunkProofinterface only requiresProof()method whileBatchProofandBundleProofinterfaces require bothProof()andSanityCheck(). For consistency and validation, consider addingSanityCheck()to theChunkProofinterface.
84-91: 🛠️ Refactor suggestionHandle unknown hard fork names more gracefully.
The factory method returns nil for unknown hard fork names, which could lead to nil pointer dereferences. Consider returning an error along with the proof instance.
246-253:⚠️ Potential issueFix error handling in OpenVMChunkProof.Proof().
The function panics on JSON marshaling error, which could crash the application. Error handling should be consistent with other methods that return errors.
🧹 Nitpick comments (3)
common/types/message/message_test.go (1)
11-54: Improve test coverage and error handling.The test function could be enhanced in the following areas:
- Add test cases for invalid JSON data
- Add test cases for invalid proof data
- Add test coverage for chunk proof deserialization
- Add error handling for file not found scenarios
Here's a suggested enhancement:
func TestDeserializeOpenVMProof(t *testing.T) { + t.Run("batch proof - success", func(t *testing.T) { // Read the batch.json file located in the same directory. data, err := os.ReadFile("batch-proof-sample.json") if err != nil { t.Fatalf("failed to read batch proof sample.json: %v", err) } // Decode the JSON data into an BatchTask instance. batchProof := NewBatchProof("euclid") if err = json.Unmarshal(data, &batchProof); err != nil { t.Fatalf("failed to unmarshal JSON into Batch Proof: %v", err) } if err = batchProof.SanityCheck(); err != nil { t.Fatalf("failed to sanity check for Batch Proof: %v", err) } ovmbatchProof := batchProof.(*OpenVMBatchProof) if ovmbatchProof.MetaData.BatchInfo.ParentStateRoot != common.HexToHash("0xe3440bcf882852bb1a9d6ba941e53a645220fee2c531ed79fa60481be8078c12") { t.Fatalf("get unexpected bundle info, parent state root is %v", ovmbatchProof.MetaData.BatchInfo.ParentStateRoot) } + }) + + t.Run("batch proof - invalid json", func(t *testing.T) { + invalidJSON := []byte(`{"invalid": json}`) + batchProof := NewBatchProof("euclid") + err := json.Unmarshal(invalidJSON, &batchProof) + if err == nil { + t.Fatal("expected error for invalid JSON") + } + }) + + t.Run("bundle proof - success", func(t *testing.T) { // Read the batch.json file located in the same directory. data, err = os.ReadFile("bundle-proof-sample.json") if err != nil { t.Fatalf("failed to read bundle proof sample.json: %v", err) } // Decode the JSON data into an BatchTask instance. bundleProof := NewBundleProof("euclid") if err = json.Unmarshal(data, &bundleProof); err != nil { t.Fatalf("failed to unmarshal JSON into Bundle Proof: %v", err) } if err = bundleProof.SanityCheck(); err != nil { t.Fatalf("failed to sanity check for Bundle Proof: %v", err) } ovmbundleProof := bundleProof.(*OpenVMBundleProof) if ovmbundleProof.MetaData.BundleInfo.PostStateRoot != common.HexToHash("0x9e8b9928c55ccbc933911283175842fa515e49dd3f2fe0192c4346095695d741") { t.Fatalf("get unexpected bundle info, post state root is %v", ovmbundleProof.MetaData.BundleInfo.PostStateRoot) } + }) + + t.Run("bundle proof - invalid json", func(t *testing.T) { + invalidJSON := []byte(`{"invalid": json}`) + bundleProof := NewBundleProof("euclid") + err := json.Unmarshal(invalidJSON, &bundleProof) + if err == nil { + t.Fatal("expected error for invalid JSON") + } + }) + + t.Run("chunk proof - success", func(t *testing.T) { + data, err := os.ReadFile("chunk-proof-sample.json") + if os.IsNotExist(err) { + t.Skip("chunk proof sample file not found") + } + if err != nil { + t.Fatalf("failed to read chunk proof sample.json: %v", err) + } + + chunkProof := NewChunkProof("euclid") + if err = json.Unmarshal(data, &chunkProof); err != nil { + t.Fatalf("failed to unmarshal JSON into Chunk Proof: %v", err) + } + }) }common/types/message/bundle-proof-sample.json (2)
1-1: Add schema validation and documentation.The JSON structure lacks schema validation in tests and documentation of expected field formats. Consider:
- Adding JSON schema validation in tests
- Documenting expected formats for each field
- Adding examples of valid/invalid values
Here's a suggested JSON schema to validate the structure:
{ "$schema": "http://json-schema.org/draft-07/schema#", "type": "object", "required": ["metadata", "proof", "vk", "git_version"], "properties": { "metadata": { "type": "object", "required": ["bundle_info", "bundle_pi_hash"], "properties": { "bundle_info": { "type": "object", "required": ["chain_id", "num_batches", "prev_state_root", "prev_batch_hash", "post_state_root", "batch_hash", "withdraw_root"], "properties": { "chain_id": { "type": "integer" }, "num_batches": { "type": "integer" }, "prev_state_root": { "type": "string", "pattern": "^0x[a-fA-F0-9]{64}$" }, "prev_batch_hash": { "type": "string", "pattern": "^0x[a-fA-F0-9]{64}$" }, "post_state_root": { "type": "string", "pattern": "^0x[a-fA-F0-9]{64}$" }, "batch_hash": { "type": "string", "pattern": "^0x[a-fA-F0-9]{64}$" }, "withdraw_root": { "type": "string", "pattern": "^0x[a-fA-F0-9]{64}$" } } }, "bundle_pi_hash": { "type": "string", "pattern": "^0x[a-fA-F0-9]{64}$" } } }, "proof": { "type": "object", "required": ["instances", "proof"], "properties": { "instances": { "type": "array", "items": { "type": "array", "items": { "type": "string", "pattern": "^[a-fA-F0-9]{64}$" } } }, "proof": { "type": "array", "items": { "type": "integer" } } } }, "vk": { "type": "string" }, "git_version": { "type": "string" } } }
1-100: Data Format Consistency & Documentation Suggestions
Ensure that all hexadecimal strings (e.g., for state roots, hashes, and the verification key "vk") adhere to the expected format (including the "0x" prefix and proper length) required by downstream consumers. Additionally, since this file will likely serve as a sample for deserialization tests, consider adding inline comments or separate documentation specifying the role and expected format of each field (for instance, clarifying the purpose of "bundle_pi_hash" versus the contents of "bundle_info"). This will ease future maintenance and integration testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
common/types/message/bundle-proof-sample.json(1 hunks)common/types/message/message.go(7 hunks)common/types/message/message_test.go(1 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: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (1)
common/types/message/bundle-proof-sample.json (1)
1-100: JSON Structure and Schema Validation
The new JSON file clearly defines a complex proof bundle object with well-organized sections for metadata and cryptographic proof data. The metadata block (including bundle_info and bundle_pi_hash) and the proof block (with separate arrays for instances and proof values) appear correctly structured. Please double-check that the key names (for example, "bundle_pi_hash") match the expected field names used elsewhere in the system’s schema so that deserialization in your Go types (such as the newly introduced polymorphic proof interfaces) works seamlessly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
common/types/message/message.go (3)
79-81:⚠️ Potential issueAdd SanityCheck to ChunkProof interface.
The
ChunkProofinterface is missing theSanityCheck()method while bothBatchProofandBundleProofinterfaces include it.type ChunkProof interface { Proof() []byte + SanityCheck() error }
246-253:⚠️ Potential issueFix error handling in OpenVMChunkProof.Proof().
The function panics on JSON marshaling error, which could crash the application.
-func (p *OpenVMChunkProof) Proof() []byte { +func (p *OpenVMChunkProof) Proof() ([]byte, error) { proofJson, err := json.Marshal(p.VmProof) if err != nil { - panic(fmt.Sprint("marshaling error", err)) + return nil, fmt.Errorf("marshaling error: %w", err) } - return proofJson + return proofJson, nil }
84-91:⚠️ Potential issueAdd error handling to proof factory methods.
The factory methods return nil for unknown hard fork names, which could lead to nil pointer dereferences.
Example fix for NewChunkProof (apply similar changes to NewBatchProof and NewBundleProof):
-func NewChunkProof(hardForkName string) ChunkProof { +func NewChunkProof(hardForkName string) (ChunkProof, error) { switch hardForkName { case euclidFork: - return &OpenVMChunkProof{} + return &OpenVMChunkProof{}, nil default: - return &Halo2ChunkProof{} + return &Halo2ChunkProof{}, nil } }Also applies to: 118-125, 175-182
🧹 Nitpick comments (3)
common/types/message/message.go (3)
11-13: Extract hard fork name into a package-level constant with documentation.The
euclidForkconstant should be documented and follow Go naming conventions for constants.const ( + // EuclidFork represents the Euclid hard fork name + EuclidFork = "euclid" - euclidFork = "euclid" )
328-328: Fix typo in field name.The field name
BunndlePIHashcontains a typo.- BunndlePIHash common.Hash `json:"bundle_pi_hash"` + BundlePIHash common.Hash `json:"bundle_pi_hash"`
350-364: Simplify nested if-else structure in OpenVMBundleProof.SanityCheck().The nested if-else structure can be flattened for better readability.
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") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/types/message/message.go(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (2)
common/types/message/message.go (2)
49-52: Consider using a concrete type for BatchHeader.The
interface{}type forBatchHeaderis too generic and could lead to runtime type errors.Consider defining a concrete type or interface with required methods to improve type safety.
224-227: 🏁 Script executed:#!/bin/bash # Search for usage of OpenVMProof.Proof to understand the expected type rg -A 5 "OpenVMProof.*Proof\s*interface{}"Length of output: 190
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
from
cfc3f14 to
6602a38
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
common/libzkp/impl/src/verifier.rs (1)
54-107: Replace unsafe code blocks with safe alternatives.The initialization code uses multiple unsafe blocks and
unwrap_unchecked()which can lead to undefined behavior.Apply this diff to use safe alternatives:
- unsafe { - PARAMS_MAP.set(params_map).unwrap_unchecked(); - } + if let Err(_) = PARAMS_MAP.set(params_map) { + panic!("Failed to set PARAMS_MAP: already initialized"); + } - unsafe { - VERIFIER_HALO2 - .set(VerifierPair( - config.low_version_circuit.fork_name.clone(), - Rc::new(Box::new(verifier)), - )) - .unwrap_unchecked(); - } + if let Err(_) = VERIFIER_HALO2.set(VerifierPair( + config.low_version_circuit.fork_name.clone(), + Rc::new(Box::new(verifier)), + )) { + panic!("Failed to set VERIFIER_HALO2: already initialized"); + } - unsafe { - VERIFIER_ZKVM - .set(VerifierPair( - config.high_version_circuit.fork_name, - Rc::new(Box::new(verifier)), - )) - .unwrap_unchecked(); - } + if let Err(_) = VERIFIER_ZKVM.set(VerifierPair( + config.high_version_circuit.fork_name, + Rc::new(Box::new(verifier)), + )) { + panic!("Failed to set VERIFIER_ZKVM: already initialized"); + }
🧹 Nitpick comments (2)
common/types/message/message.go (2)
78-81: Add SanityCheck method to ChunkProof interface.For consistency with
BatchProofandBundleProofinterfaces, consider adding theSanityCheck()method to validate chunk proofs.type ChunkProof interface { Proof() []byte + SanityCheck() error }
340-367: Remove redundant else block.The else block is redundant as the function returns early on error.
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 ignored due to path filters (1)
common/libzkp/impl/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
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)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/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(5 hunks)coordinator/internal/types/prover.go(2 hunks)rollup/internal/controller/sender/sender_test.go(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 (13)
- common/libzkp/interface/libzkp.h
- coordinator/internal/logic/verifier/types.go
- common/libzkp/impl/rust-toolchain
- coordinator/internal/logic/provertask/chunk_prover_task.go
- coordinator/internal/types/prover.go
- common/libzkp/impl/src/lib.rs
- common/types/message/bundle-proof-sample.json
- common/types/message/message_test.go
- common/libzkp/impl/src/verifier/darwin_v2.rs
- coordinator/internal/logic/auth/login.go
- common/libzkp/impl/src/verifier/euclid.rs
- coordinator/internal/logic/verifier/verifier.go
- rollup/internal/controller/sender/sender_test.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (13)
common/libzkp/impl/src/verifier.rs (1)
1-2: 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.
coordinator/internal/logic/provertask/bundle_prover_task.go (1)
147-154: LGTM! Good addition of version compatibility check.The added version compatibility check helps prevent mismatched prover versions from processing tasks.
common/libzkp/impl/Cargo.toml (5)
15-16: Correct dependency formatting for ethers-signers.
The update ensures that the dependency for ethers-signers is formatted consistently (i.e. without the unnecessary space) and correctly references the intended Git repository and branch.
17-22: Addition of new ZKP dependencies.
New dependencies (ruint, alloy-primitives, revm, revm-interpreter, revm-precompile, revm-primitives) have been introduced to support enhanced zero-knowledge proof functionality. Please verify that the specified branches (e.g., "v1.12.3", "v0.8.18", "scroll-evm-executor/v50") match the versions intended for production stability.
25-25: Update halo2_proofs patch dependency.
The patch for halo2_proofs now correctly points to the repository at branch "v1.1". This change improves clarity and consistency across the project's dependency management.
33-37: Clean formatting for snark-verifier-sdk dependency.
The multi-line configuration for snark-verifier-sdk, including its feature list, enhances readability and maintainability. Confirm that the enabled features ("loader_halo2", "loader_evm", "halo2-pse") are all required for the intended functionality.
40-46: Introduce Euclid-related dependencies and update prover_v5 formatting.
The dependency section now includes an updated configuration for prover_v5 along with new dependencies prover_v7 and verifier_v7 (tagged "v0.1.0-rc.1") to support the Euclid prover feature. Please ensure that these version tags and repository configurations are compatible with the new feature’s requirements.common/types/message/message.go (4)
11-13: LGTM!The constant name is descriptive and follows Go naming conventions.
277-284:⚠️ Potential issueFix error handling.
The function panics on JSON marshaling error, which could crash the application.
-func (p *OpenVMBatchProof) Proof() []byte { +func (p *OpenVMBatchProof) Proof() ([]byte, error) { proofJson, err := json.Marshal(p.VmProof) if err != nil { - panic(fmt.Sprint("marshaling error", err)) + return nil, fmt.Errorf("marshaling error: %w", err) } - return proofJson + return proofJson, nil }Likely invalid or redundant comment.
246-253:⚠️ Potential issueFix error handling and remove unnecessary conversion.
The function has two issues:
- It panics on JSON marshaling error, which could crash the application.
- It performs an unnecessary conversion of
proofJsonto[]byte.-func (p *OpenVMChunkProof) Proof() []byte { +func (p *OpenVMChunkProof) Proof() ([]byte, error) { proofJson, err := json.Marshal(p.VmProof) if err != nil { - panic(fmt.Sprint("marshaling error", err)) + return nil, fmt.Errorf("marshaling error: %w", err) } - return []byte(proofJson) + return proofJson, nil }Likely invalid or redundant comment.
83-91: 🛠️ Refactor suggestionReturn error for unknown hard fork names.
The function should return an error for unknown hard fork names to avoid potential nil pointer dereferences.
-func NewChunkProof(hardForkName string) ChunkProof { +func NewChunkProof(hardForkName string) (ChunkProof, error) { switch hardForkName { case euclidFork: - return &OpenVMChunkProof{} + return &OpenVMChunkProof{}, nil default: - return &Halo2ChunkProof{} + return &Halo2ChunkProof{}, nil } }Likely invalid or redundant comment.
coordinator/internal/logic/provertask/batch_prover_task.go (2)
269-292: LGTM!The function has proper error handling and validation of codec version.
235-239:⚠️ Potential issueUse safer type assertion.
The type assertion could panic if it fails. Use the comma-ok idiom to handle the case where the assertion fails.
- if haloProot, ok := proof.(*message.Halo2ChunkProof); ok { + if proof, 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)) }Likely invalid or redundant comment.
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.Refactor
Halo2types, enhancing consistency and performance.BatchProposerandBundleProposermethods.Chores