feat: euclid prover and coordinator #1597
Conversation
WalkthroughThis extensive update refactors proof-related types across modules by transitioning from concrete structures to an interface‐based design. New factory functions (e.g. NewChunkProof, NewBatchProof, NewBundleProof) are introduced for flexible instantiation based on hard fork names, with method signatures adjusted from pointer to value types. Additionally, task assignment, verifier logic, and SQL queries for rollup and coordinator components have been revised to include hard fork compatibility checks and codec filtering. Upgraded dependency versions, updated Go and Rust toolchains, enhanced Dockerfile/Makefile configurations, and a new Rust-based prover project further consolidate these sweeping changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Receiver as ProofReceiverLogic
participant Factory as NewProofFactory
participant Verifier as Verifier
Client->>Receiver: Submit proof request
Receiver->>Factory: Instantiate proof (NewChunkProof/NewBatchProof/NewBundleProof)
Factory-->>Receiver: Return proof instance
Receiver->>Verifier: Call verify(proof)
Verifier-->>Receiver: Return verification result
Receiver-->>Client: Send verification status
sequenceDiagram
participant Proposer as ChunkProposer
participant BlockSrc as BlockSource
Proposer->>BlockSrc: Retrieve block list
Proposer->>Proposer: tryProposeEuclidTransitionChunk(blocks)
alt Euclid transition detected
Proposer-->>Proposer: Propose transition chunk and exit early
else No transition
Proposer->>Proposer: Continue with standard proposal logic
end
Possibly related PRs
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"" 🪧 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.05% -9.68%
===========================================
Files 157 222 +65
Lines 12555 17739 +5184
===========================================
+ Hits 6494 7460 +966
- Misses 5497 9573 +4076
- Partials 564 706 +142
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
coordinator/internal/logic/verifier/mock.go (1)
36-40: Check for nil pointer dereference.
Again, guard against a nil pointer. Consistently applying checks prevents panics in all verification methods.
🧹 Nitpick comments (7)
prover/src/main.rs (2)
28-30: Consider graceful shutdown for asynchronous main.Switching to
#[tokio::main]is a solid approach for running async code, though you might consider capturing signals (e.g., SIGINT) or providing a graceful shutdown mechanism to ensure system resources are released properly.
35-35: Unify logging for version output.Using
println!here works, but for consistency with other logs, considerlog::info!or an equivalent logging macro to keep log levels and outputs unified.- println!("version is {}", get_version()); + log::info!("version is {}", get_version());prover/src/zk_circuits_handler/euclid.rs (2)
5-11: Overall concurrency approach.Asynchronous trait usage (
#[async_trait]) withtokio::sync::Mutexshows a robust concurrency approach. Just ensure that the lock is not held for a prolonged time if these tasks involve heavy computation.
48-101: Comprehensive trait implementation for asynchronous proof generation.
- Lock usage: Repeated calls to
self.try_lock().unwrap()signals potential panic if resources are locked. Consider usinglock().awaitif concurrency might block.- Dynamic input parsing: Rely on
serde_json::from_streffectively; ensure robust error messaging to highlight malformed requests.- Future expansions: If new
ProofTypevariants appear, an exhaustive pattern match keeps the code clean but re-check for theunreachable!blocks in production scenarios.prover/src/prover.rs (1)
49-55: Consider multi-task concurrency support.
Currently, the design stores only oneJoinHandleincurrent_task, so new tasks could overwrite or conflict with an existing one. If supporting multiple simultaneous proofs is desired, implement a queue or pool of tasks and track them accordingly.prover/src/zk_circuits_handler.rs (1)
9-11: Consider using a Result instead of Option for get_vk.
At the moment, havingNonehides the reason for an unavailable verification key. If there’s any chance of an error, returningResult<Vec<u8>>can convey more context.prover/Cargo.toml (1)
28-34: New Proving-Related Dependencies AddedThe additions of
scroll-proving-sdk,scroll-zkvm-prover, andsbv-primitivesclearly support the new Euclid prover functionality. Ensure that the pinned revision (rev = "3331117") and the tag forscroll-zkvm-prover("v0.1.0-rc.1") are aligned with the expected feature set. Also, consider adding brief inline documentation or commit notes regarding the use of the custom patch branch (omerfirmak-patch-1) forsbv-primitivesfor future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
common/types/message/message.go(2 hunks)common/version/version.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(0 hunks)coordinator/internal/logic/verifier/mock.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(3 hunks)coordinator/test/mock_prover.go(1 hunks)prover/Cargo.toml(3 hunks)prover/config.json(1 hunks)prover/rust-toolchain(1 hunks)prover/src/config.rs(0 hunks)prover/src/coordinator_client.rs(0 hunks)prover/src/coordinator_client/api.rs(0 hunks)prover/src/coordinator_client/errors.rs(0 hunks)prover/src/coordinator_client/listener.rs(0 hunks)prover/src/coordinator_client/types.rs(0 hunks)prover/src/geth_client.rs(0 hunks)prover/src/key_signer.rs(0 hunks)prover/src/main.rs(2 hunks)prover/src/prover.rs(1 hunks)prover/src/task_cache.rs(0 hunks)prover/src/task_processor.rs(0 hunks)prover/src/types.rs(1 hunks)prover/src/utils.rs(0 hunks)prover/src/version.rs(0 hunks)prover/src/zk_circuits_handler.rs(1 hunks)prover/src/zk_circuits_handler/common.rs(0 hunks)prover/src/zk_circuits_handler/darwin.rs(0 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(0 hunks)prover/src/zk_circuits_handler/euclid.rs(1 hunks)
💤 Files with no reviewable changes (16)
- prover/src/coordinator_client/listener.rs
- coordinator/internal/logic/provertask/batch_prover_task.go
- prover/src/zk_circuits_handler/common.rs
- prover/src/coordinator_client.rs
- prover/src/task_processor.rs
- prover/src/utils.rs
- prover/src/coordinator_client/errors.rs
- prover/src/geth_client.rs
- prover/src/key_signer.rs
- prover/src/coordinator_client/types.rs
- prover/src/task_cache.rs
- prover/src/coordinator_client/api.rs
- prover/src/version.rs
- prover/src/config.rs
- prover/src/zk_circuits_handler/darwin.rs
- prover/src/zk_circuits_handler/darwin_v2.rs
✅ Files skipped from review due to trivial changes (2)
- prover/rust-toolchain
- common/version/version.go
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: fmt
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (18)
prover/src/main.rs (3)
6-10: Imports look appropriate.The newly introduced imports for
LocalProver,LocalProverConfig, and thescroll_proving_sdkalign with the updated proving architecture.
39-42: Initialization looks good.Configuration loading and prover creation are handled cleanly, leveraging the
?operator to propagate errors. This should be reliable as long asfrom_fileand builder logic are well-tested.
44-44: Verify error handling fromprover.run().
prover.run().awaitdoes not appear to return or propagate any potential errors. If it can fail, consider capturing its result. Otherwise, confirm that no error scenario is being silently ignored.common/types/message/message.go (1)
4-4: Import ofencoding/jsonapproved.This change is necessary to handle raw JSON data for proofs. No concerns here.
prover/src/zk_circuits_handler/euclid.rs (5)
1-2: Imports for concurrency and path usage.The use of
Arc,Mutex, and filesystemPathindicates concurrency readiness and filesystem-based workspace handling. Ensure that concurrent access to file paths remains safe across multiple tasks.
3-4: Proper layering and error handling.Bringing in
anyhowaligns with the rest of the system’s error-handling pattern. The usage ofCircuitsHandlerfrom the super module is consistent with an extensible design.
12-16:EuclidHandlerstruct for specialized proving tasks.Bundling
ChunkProver,BatchProver, andBundleProverin a single handler is clear, but be mindful of future expansions if more provers are introduced.
18-19:unsafe impl Sendusage.Declaring
Sendmakes sense if the underlying fields are truly thread-safe. Carefully verify no external synchronization is missing.
20-46: Constructor logic for multiple provers.Approach to set up each prover with
Path::joinis straightforward. Confirm that each.vmexeand.tomlfile path is correct, especially in production or containerized environments.prover/src/prover.rs (1)
1-22: Imports and dependencies look good.
No immediate concerns. They are consistent with the shift to the new asynchronous architecture and use ofanyhowfor error handling.prover/src/types.rs (1)
3-3: LGTM! Good use of SDK type.Using
CircuitTypefromscroll_proving_sdkinstead of custom enums reduces code duplication and improves maintainability.coordinator/internal/logic/verifier/verifier.go (1)
116-116: LGTM! Consistent proof validation approach.The change to use
string(*proof)across all verification methods maintains consistency with the new JSON-based proof representation.Also applies to: 143-143, 170-170
coordinator/test/mock_prover.go (1)
226-227: LGTM! Simplified proof construction in tests.The change to use byte slices directly for invalid test proofs aligns well with the new JSON-based proof representation while maintaining test coverage.
Also applies to: 232-233
prover/config.json (3)
2-3: Verify key management security.The transition from
keystore_pathandkeystore_passwordtokeys_dirrepresents a significant change in key management. Please ensure proper access controls and security measures are in place for the new key storage approach.
13-28: LGTM! Well-structured circuit configuration.The new
proversection with circuit types and versioning improves configuration organization and maintainability. The separation of low and high version circuits provides good flexibility for version management.
4-9: Verify timeout and retry configurations.The coordinator and l2geth connection settings should be reviewed to ensure they are appropriate for production environments:
- 30-second connection timeout might be too long
- 10-second retry wait time might need adjustment based on production load
Also applies to: 10-12
✅ Verification successful
🌐 Web query:
What are the recommended connection timeout and retry settings for production Go services?Length of output: 5436
Adjust Production Timeout and Retry Configurations
The current coordinator settings in
prover/config.jsonuse a 30‑second connection timeout and a fixed 10‑second retry wait. Based on production best practices for Go services, these values should be reviewed:
- Connection Timeout: Rather than a fixed 30‑second timeout, consider configuring the timeout relative to the network’s RTT (e.g., RTT × 3) or using lower values (often in the 5‑10 second range for HTTP server timeouts) to help detect issues faster.
- Retry Strategy: Rather than a fixed 10‑second delay between retries, production setups benefit from an exponential backoff with jitter; this adjustment can better accommodate varying load and transient network issues.
- Scope: These recommendations apply to both the coordinator settings shown (lines 4–9) and the similar settings on lines 10–12 for l2geth.
Review and adjust these configurations so they align with your production environment’s performance requirements and error recovery strategies.
prover/Cargo.toml (2)
10-17: Patch Dependencies: Confirm Branch Usage and MSRV SupportThe patches for
ruint,alloy-primitives, and the variousrevmcrates are now referenced from specific git branches. This is a good way to ensure you’re using the patched versions with the new MSRV (1.77) support. Please verify that these branch references remain stable and document the rationale (e.g., addressing rkyv support) if not already captured elsewhere.
46-52: Utility and Asynchronous Dependency EnhancementsThe addition of
async-trait,url, andserde_bytesis appropriate given the move towards an asynchronous prover model and enhanced data handling. Please confirm that these dependency versions are fully compatible with the rest of the codebase. Additionally, it might be beneficial to re-check if there are newer patch versions available forasync-traitthat provide any important improvements or fixes.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
from
0c6e954 to
c591328
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
common/types/message/message.go (3)
143-149: Improve proof length validation error messages.The error messages for proof length validation could be more descriptive. Consider adding the expected length multiple to the error message.
Apply this diff:
- return fmt.Errorf("proof buffer length must be a multiple of 32, got: %d", len(ap.RawProof)) + return fmt.Errorf("proof buffer length must be a multiple of 32 bytes (got: %d bytes)", len(ap.RawProof))Also applies to: 202-204
101-103: Consider validating proofs before returning.The
Proof()methods simply return raw bytes without any validation. Consider validating the proof before returning to catch issues early.Apply this diff to add validation:
func (ap *Halo2ChunkProof) Proof() []byte { + if len(ap.RawProof) == 0 { + return nil + } + if len(ap.RawProof)%32 != 0 { + return nil + } return ap.RawProof }Apply similar changes to
Halo2BatchProof.Proof()andHalo2BundleProof.Proof().Also applies to: 133-135, 188-190
148-148: Address TODO comment.The TODO comment indicates that
NewBundleProofshould be used. This should be implemented to ensure consistent proof creation.Would you like me to help implement this change?
rollup/tests/rollup_test.go (1)
147-151: Document test data structure.The test uses hardcoded byte arrays for proof data. Consider documenting the structure of these test proofs or using helper functions to generate meaningful test data.
Apply this diff:
+// mockProofBytes returns a 32-byte array for testing +func mockProofBytes() []byte { + return bytes.Repeat([]byte{0, 1, 2, 3}, 8) // 32 bytes +} batchProof := &message.Halo2BatchProof{ - RawProof: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, - Instances: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, - Vk: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, + RawProof: mockProofBytes(), + Instances: mockProofBytes(), + Vk: mockProofBytes(), }Also applies to: 162-166
rollup/internal/orm/bundle.go (1)
137-153: Improve error handling in GetVerifiedProofByHash.The error handling could be more specific and include context about the failure.
Apply this diff:
func (o *Bundle) GetVerifiedProofByHash(ctx context.Context, hash string) (message.BundleProof, error) { + if hash == "" { + return nil, fmt.Errorf("GetVerifiedProofByHash: empty hash provided") + } + db := o.db.WithContext(ctx) db = db.Model(&Bundle{}) db = db.Select("proof") db = db.Where("hash = ? AND proving_status = ?", hash, types.ProvingTaskVerified) var bundle Bundle if err := db.Find(&bundle).Error; err != nil { return nil, fmt.Errorf("Bundle.GetVerifiedProofByHash error: %w, bundle hash: %v", err, hash) } + if bundle.Proof == nil { + return nil, fmt.Errorf("Bundle.GetVerifiedProofByHash: no proof found for hash: %v", hash) + } var proof message.BundleProof if err := json.Unmarshal(bundle.Proof, &proof); err != nil { - return nil, fmt.Errorf("Bundle.GetVerifiedProofByHash error: %w, bundle hash: %v", err, hash) + return nil, fmt.Errorf("Bundle.GetVerifiedProofByHash: failed to unmarshal proof: %w, bundle hash: %v", err, hash) } return proof, nil }coordinator/internal/logic/provertask/bundle_prover_task.go (1)
224-231: Improve error handling in proof unmarshaling.The error handling during proof unmarshaling could be more robust and include validation of the unmarshaled proof.
Apply this diff:
var batchProofs []message.BatchProof for _, batch := range batches { - proof := message.NewBatchProof(hardForkName) + proof, err := message.NewBatchProof(hardForkName) + if err != nil { + return nil, fmt.Errorf("failed to create batch proof: %w, bundle hash: %v, batch hash: %v", err, task.TaskID, batch.Hash) + } + if encodeErr := json.Unmarshal(batch.Proof, &proof); encodeErr != nil { return nil, fmt.Errorf("failed to unmarshal proof: %w, bundle hash: %v, batch hash: %v", encodeErr, task.TaskID, batch.Hash) } + + if err := proof.SanityCheck(); err != nil { + return nil, fmt.Errorf("proof validation failed: %w, bundle hash: %v, batch hash: %v", err, task.TaskID, batch.Hash) + } + batchProofs = append(batchProofs, proof) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
common/types/message/message.go(4 hunks)coordinator/cmd/tool/tool.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(3 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(1 hunks)coordinator/internal/logic/submitproof/proof_receiver.go(2 hunks)coordinator/internal/logic/verifier/mock.go(2 hunks)coordinator/internal/logic/verifier/verifier.go(3 hunks)coordinator/internal/logic/verifier/verifier_test.go(2 hunks)coordinator/internal/orm/chunk.go(0 hunks)coordinator/internal/types/prover.go(1 hunks)coordinator/test/api_test.go(1 hunks)coordinator/test/mock_prover.go(2 hunks)rollup/internal/controller/relayer/l2_relayer.go(2 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(1 hunks)rollup/internal/orm/batch.go(3 hunks)rollup/internal/orm/bundle.go(3 hunks)rollup/internal/orm/orm_test.go(4 hunks)rollup/tests/rollup_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- coordinator/internal/orm/chunk.go
🚧 Files skipped from review as they are similar to previous changes (3)
- coordinator/test/mock_prover.go
- coordinator/internal/logic/verifier/verifier.go
- coordinator/internal/logic/provertask/batch_prover_task.go
🧰 Additional context used
🪛 GitHub Check: tests
coordinator/internal/logic/verifier/mock.go
[failure] 27-27:
syntax error: unexpected ., expected name
[failure] 35-35:
syntax error: unexpected ., expected name
🪛 GitHub Actions: Integration
coordinator/internal/logic/verifier/mock.go
[error] 27-27: syntax error: unexpected ., expected name
🪛 GitHub Actions: Coordinator
coordinator/internal/logic/verifier/mock.go
[error] 27-27: expected 'IDENT', found '.'
🪛 golangci-lint (1.62.2)
coordinator/internal/logic/submitproof/proof_receiver.go
178-178: m.verifier.VerifyChunkProof undefined (type *verifier.Verifier has no field or method VerifyChunkProof)
(typecheck)
184-184: m.verifier.VerifyBatchProof undefined (type *verifier.Verifier has no field or method VerifyBatchProof)
(typecheck)
190-190: m.verifier.VerifyBundleProof undefined (type *verifier.Verifier has no field or method VerifyBundleProof)
(typecheck)
🪛 GitHub Actions: Common
common/types/message/message.go
[error] 80-80: string darwinV2 has 3 occurrences, make it a constant (goconst)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: clippy
- GitHub Check: tests
🔇 Additional comments (14)
coordinator/internal/logic/verifier/mock.go (1)
19-24: LGTM! Interface-based proof verification.The transition from concrete struct pointers to interfaces improves flexibility and polymorphism in proof handling. The implementation correctly uses the
Proof()method from the interface.Also applies to: 27-32, 35-40
coordinator/internal/types/prover.go (1)
9-17: LGTM! Well-structured status code implementation.The implementation follows Go best practices:
- Uses iota for enum-like constants
- Clear documentation for type and constants
- Appropriate use of uint32 for status codes
coordinator/internal/logic/verifier/verifier_test.go (1)
68-78: LGTM! Consistent interface implementation in tests.The changes correctly:
- Return interface types (types.BatchProof, types.ChunkProof)
- Use concrete implementations (Halo2BatchProof, Halo2ChunkProof)
- Maintain proper error handling during unmarshaling
Also applies to: 80-90
coordinator/cmd/tool/tool.go (2)
67-67: Address the TODO comment about NewBatchProof.The TODO comment suggests that a factory method for creating BatchProof instances might be needed. Consider implementing this to ensure consistent object creation.
Would you like me to help implement the
NewBatchProoffactory method?
65-73: LGTM! Consistent proof handling.The changes correctly transition from pointer slices to value slices, aligning with the broader codebase changes for proof handling.
coordinator/internal/logic/submitproof/proof_receiver.go (2)
174-174: LGTM! Using constructor functions improves initialization consistency.The use of constructor functions (
NewChunkProof,NewBatchProof,NewBundleProof) ensures proper initialization of proof objects with the correct hard fork name.Also applies to: 180-180, 186-186
268-268: LGTM! Using typed status improves type safety.The change to use
coordinatorType.StatusOkinstead of a raw status value improves type safety and maintainability.rollup/internal/orm/batch.go (1)
125-125: LGTM! Consistent use of value types for proofs.The changes to use value types instead of pointers for
BatchProofin bothGetVerifiedProofByHashandUpdateProofByHashimprove consistency with the new proof interface design.Also applies to: 437-437
rollup/internal/controller/relayer/l2_relayer_test.go (1)
148-150: LGTM! Updated proof structure in tests.The test has been correctly updated to use
Halo2BundleProofwith theRawProoffield, aligning with the new proof structure.rollup/internal/orm/orm_test.go (1)
463-465: LGTM! Tests updated to use new proof interface.The tests have been correctly updated to:
- Use
Halo2BundleProofwithRawProoffield- Use the
Proof()method instead of direct field access- Handle proof unmarshaling consistently with the new structure
Also applies to: 474-474, 486-488, 499-502
coordinator/test/api_test.go (1)
597-597: LGTM! Aligns with the interface-based proof type design.The change from marshaling a
ChunkProoftoHalo2ChunkProofaligns with the broader refactor to use interface-based proof types.rollup/internal/controller/relayer/l2_relayer.go (3)
612-612: LGTM! Improved type safety with interface-based design.Changed
aggProoffrom pointer to value type as part of the transition to interface-based proof types.
886-886: LGTM! Updated function signature for interface-based design.The function signature now accepts a value type
message.BundleProofinterface instead of a pointer.
893-893: LGTM! Using interface method instead of direct field access.Changed from dereferencing the
Prooffield to calling theProof()method, aligning with the interface-based design.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
6 times, most recently
from
f1bd75c to
08b89b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
common/types/message/message.go (2)
109-115:⚠️ Potential issueHandle unknown hard fork names in NewBatchProof.
Same issue as
NewChunkProof: returns a default implementation for any hard fork name.Apply similar error handling pattern as suggested for
NewChunkProof.
164-170:⚠️ Potential issueHandle unknown hard fork names in NewBundleProof.
Same issue as
NewChunkProofandNewBatchProof: returns a default implementation for any hard fork name.Apply similar error handling pattern as suggested for
NewChunkProof.coordinator/internal/logic/submitproof/proof_receiver.go (1)
174-178:⚠️ Potential issueMissing verifier method implementation.
The
VerifyChunkProofmethod is undefined in the*verifier.Verifiertype. This will cause runtime errors.🧰 Tools
🪛 golangci-lint (1.62.2)
178-178: m.verifier.VerifyChunkProof undefined (type *verifier.Verifier has no field or method VerifyChunkProof)
(typecheck)
🧹 Nitpick comments (6)
common/types/message/message.go (1)
133-156: Improve error messages in SanityCheck.The error messages could be more descriptive to help with debugging.
Apply this diff to improve error messages:
func (ap *Halo2BatchProof) SanityCheck() error { if ap == nil { - return errors.New("agg_proof is nil") + return errors.New("batch proof is nil") } if len(ap.RawProof) == 0 { - return errors.New("proof not ready") + return errors.New("batch proof data is empty") } if len(ap.RawProof)%32 != 0 { return fmt.Errorf("proof buffer length must be a multiple of 32, got: %d", len(ap.RawProof)) } if len(ap.Instances) == 0 { - return errors.New("instance not ready") + return errors.New("batch proof instances are empty") } if len(ap.Vk) == 0 { - return errors.New("vk not ready") + return errors.New("batch proof verification key is empty") } return nil }rollup/tests/rollup_test.go (1)
147-151: Add test cases for proof validation.The test initializes proofs with fixed byte arrays but doesn't validate the proof format or content.
Add test cases to verify:
- Proof length is multiple of 32
- SanityCheck passes
- Invalid proof handling
coordinator/test/mock_prover.go (3)
204-207: Consider using constants for status values.The status values are hardcoded. Consider using constants from the types package for better maintainability.
212-221: Add test cases for different proof types.The mock implementation creates empty proofs. Consider adding test cases with different proof contents to test error handling.
226-239: Improve test coverage for invalid proofs.The test only checks one invalid proof case. Consider adding more test cases for different types of invalid proofs.
prover/config.json (1)
2-29: Configuration improvements enhance security and flexibility.The changes:
- Remove sensitive keystore-related fields
- Add circuit configuration for better flexibility
- Improve organization of settings
Consider documenting the available circuit types and versions in a README file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
common/types/message/message.go(4 hunks)common/version/version.go(1 hunks)coordinator/cmd/tool/tool.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(3 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(1 hunks)coordinator/internal/logic/submitproof/proof_receiver.go(2 hunks)coordinator/internal/logic/verifier/mock.go(1 hunks)coordinator/internal/logic/verifier/verifier.go(3 hunks)coordinator/internal/logic/verifier/verifier_test.go(2 hunks)coordinator/internal/orm/chunk.go(0 hunks)coordinator/internal/types/prover.go(1 hunks)coordinator/test/api_test.go(1 hunks)coordinator/test/mock_prover.go(2 hunks)prover/Cargo.toml(3 hunks)prover/config.json(1 hunks)prover/rust-toolchain(1 hunks)prover/src/config.rs(0 hunks)prover/src/coordinator_client.rs(0 hunks)prover/src/coordinator_client/api.rs(0 hunks)prover/src/coordinator_client/errors.rs(0 hunks)prover/src/coordinator_client/listener.rs(0 hunks)prover/src/coordinator_client/types.rs(0 hunks)prover/src/geth_client.rs(0 hunks)prover/src/key_signer.rs(0 hunks)prover/src/main.rs(2 hunks)prover/src/prover.rs(1 hunks)prover/src/task_cache.rs(0 hunks)prover/src/task_processor.rs(0 hunks)prover/src/types.rs(1 hunks)prover/src/utils.rs(0 hunks)prover/src/version.rs(0 hunks)prover/src/zk_circuits_handler.rs(1 hunks)prover/src/zk_circuits_handler/common.rs(0 hunks)prover/src/zk_circuits_handler/darwin.rs(0 hunks)prover/src/zk_circuits_handler/darwin_v2.rs(0 hunks)prover/src/zk_circuits_handler/euclid.rs(1 hunks)rollup/internal/controller/relayer/l2_relayer.go(2 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(2 hunks)rollup/internal/orm/batch.go(3 hunks)rollup/internal/orm/bundle.go(3 hunks)rollup/internal/orm/orm_test.go(4 hunks)rollup/tests/rollup_test.go(2 hunks)
💤 Files with no reviewable changes (16)
- prover/src/coordinator_client/listener.rs
- prover/src/utils.rs
- coordinator/internal/orm/chunk.go
- prover/src/coordinator_client/errors.rs
- prover/src/coordinator_client.rs
- prover/src/version.rs
- prover/src/zk_circuits_handler/darwin.rs
- prover/src/zk_circuits_handler/common.rs
- prover/src/geth_client.rs
- prover/src/task_processor.rs
- prover/src/key_signer.rs
- prover/src/task_cache.rs
- prover/src/coordinator_client/api.rs
- prover/src/config.rs
- prover/src/coordinator_client/types.rs
- prover/src/zk_circuits_handler/darwin_v2.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- prover/rust-toolchain
- coordinator/internal/types/prover.go
- common/version/version.go
- coordinator/internal/logic/provertask/bundle_prover_task.go
- coordinator/cmd/tool/tool.go
- rollup/internal/orm/orm_test.go
- coordinator/internal/logic/verifier/verifier.go
- rollup/internal/controller/relayer/l2_relayer.go
- coordinator/internal/logic/provertask/batch_prover_task.go
- coordinator/internal/logic/verifier/mock.go
- coordinator/test/api_test.go
- coordinator/internal/logic/verifier/verifier_test.go
- rollup/internal/orm/bundle.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
coordinator/internal/logic/submitproof/proof_receiver.go
178-178: m.verifier.VerifyChunkProof undefined (type *verifier.Verifier has no field or method VerifyChunkProof)
(typecheck)
184-184: m.verifier.VerifyBatchProof undefined (type *verifier.Verifier has no field or method VerifyBatchProof)
(typecheck)
190-190: m.verifier.VerifyBundleProof undefined (type *verifier.Verifier has no field or method VerifyBundleProof)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: clippy
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (16)
prover/src/zk_circuits_handler.rs (1)
1-12: LGTM! Well-structured trait definition.The trait is well-defined with appropriate async methods and thread safety bounds.
prover/src/main.rs (1)
28-46: LGTM! Clean async implementation.The transition to async/await with Tokio is well-implemented, and the builder pattern usage improves code readability.
prover/src/types.rs (1)
8-8: Verify CircuitType compatibility with existing code.The transition from TaskType to CircuitType needs verification to ensure compatibility with existing code.
#!/bin/bash # Search for any direct usage of CircuitType to verify the transition rg -A 3 "CircuitType::"Also applies to: 18-18
common/types/message/message.go (1)
77-83:⚠️ Potential issueHandle unknown hard fork names in NewChunkProof.
The factory method returns a default
Halo2ChunkProoffor any hard fork name, which could lead to runtime errors if an unsupported hard fork is provided.Apply this diff to improve error handling:
-func NewChunkProof(hardForkName string) ChunkProof { +func NewChunkProof(hardForkName string) (ChunkProof, error) { switch hardForkName { + case "darwinV2": + return &Halo2ChunkProof{}, nil default: - return &Halo2ChunkProof{} + return nil, fmt.Errorf("unsupported hard fork name: %s", hardForkName) } }Likely invalid or redundant comment.
prover/src/prover.rs (1)
182-183:⚠️ Potential issueHandle unwrap safely in set_active_handler.
Using
unwrap()oncircuits.get()could cause panics.Apply this diff to handle errors safely:
- let config = self.config.circuits.get(hard_fork_name).unwrap(); + let config = match self.config.circuits.get(hard_fork_name) { + Some(cfg) => cfg, + None => panic!("Unknown hard fork name: {}", hard_fork_name), + };Likely invalid or redundant comment.
rollup/tests/rollup_test.go (1)
162-166: Add test cases for bundle proof validation.Similar to BatchProof, the test lacks validation of the bundle proof format and content.
Add similar validation test cases as suggested for BatchProof.
coordinator/internal/logic/submitproof/proof_receiver.go (1)
268-268: LGTM!The status check has been correctly updated to use
coordinatorType.StatusOk.rollup/internal/orm/batch.go (1)
125-141: LGTM!The changes improve the method by:
- Adding
hardForkNameparameter for better flexibility- Using value types instead of pointers for safer memory management
- Using the factory function for proof initialization
rollup/internal/controller/relayer/l2_relayer_test.go (2)
126-130: LGTM!The test setup has been improved by adding chunk insertion before batch insertion, ensuring proper data dependencies.
154-158: LGTM!The proof type and field names have been correctly updated to align with the new proof type system.
prover/Cargo.toml (6)
10-16: Enhanced Patch Dependencies Update.
The[patch.crates-io]block now introduces updated dependencies (e.g.,ruint,alloy-primitives, and severalrevm-*crates) along with a clarifying comment about adding rkyv support and MSRV 1.77. Consider verifying that the git-branch references remain stable and, if possible, pin to specific commits or tags to ensure reproducible builds.
28-30: Addition of scroll-proving-sdk Dependency.
The new dependencyscroll-proving-sdkis added with a fixed revision (3331117) and includes the"openvm"feature. Please confirm that this specific revision is stable and that all relevant code is updated to adopt the SDK's API.
31-34: New ZKVM and SBV-Primitives Dependencies.
scroll-zkvm-proveris now included (tagged asv0.1.0-rc.1), andsbv-primitivesis added from branchomerfirmak-patch-1with the"scroll"feature. Ensure these additions align with the overall architecture and that the chosen branch and tag meet your release requirements.
46-46: Introduction of async-trait Dependency.
The addition ofasync-trait = "0.1"supports the new asynchronous architecture in the prover module. Confirm that this version is compatible with your async workflows and that there are no conflicts with other async components.
51-52: Inclusion of URL and serde_bytes Dependencies.
The new dependenciesurl = "2.5.4"andserde_bytes = "0.11.15"appear to support enhanced URL handling and binary serialization. Verify that these additions meet your security, performance, and compatibility requirements throughout the codebase.
1-53: Overall Dependency Consistency Review.
TheCargo.tomlmodifications are well aligned with the restructured prover architecture and the move toward improved modularity. As these dependencies are now sourced directly from git repositories and specific branches or revisions, please ensure that all changes are thoroughly documented in your release notes/changelog and that versioning remains consistent for reproducibility and security.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
rollup/tests/rollup_test.go (1)
63-67: Update validation logic to handle all codec versions.The current validation logic only handles CodecV4 and fails for other versions, which contradicts the addition of CodecV5 and CodecV6 to the test.
Apply this diff to handle all codec versions:
-if codecVersion == encoding.CodecV4 { +switch codecVersion { +case encoding.CodecV4: chainConfig = ¶ms.ChainConfig{LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64)} -} else { +case encoding.CodecV5: + chainConfig = ¶ms.ChainConfig{LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64)} +case encoding.CodecV6: + chainConfig = ¶ms.ChainConfig{LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64)} +default: assert.Fail(t, "unsupported codec version, expected CodecV4") }
🧹 Nitpick comments (10)
rollup/internal/controller/watcher/batch_proposer.go (3)
247-250: LGTM! Consider enhancing the error message.The change to use
CodecFromVersionis a good improvement. The error handling is now more comprehensive by checking bothnilcodec and error cases.Consider making the error message more specific to version:
- return fmt.Errorf("failed to retrieve codec for block number %v and time %v: %w", firstUnbatchedChunk.StartBlockNumber, firstUnbatchedChunk.StartBlockTime, err) + return fmt.Errorf("failed to retrieve codec version %v for block number %v and time %v: %w", firstUnbatchedChunk.CodecVersion, firstUnbatchedChunk.StartBlockNumber, firstUnbatchedChunk.StartBlockTime, err)
280-333: Consider refactoring batch processing logic for better readability.The batch processing logic is complex and handles multiple constraints. Consider extracting the limit checking logic into a separate method for better maintainability.
+func (p *BatchProposer) exceedsBatchLimits(metrics *utils.BatchMetrics) (bool, error) { + totalOverEstimateL1CommitGas := uint64(p.gasCostIncreaseMultiplier * float64(metrics.L1CommitGas)) + return metrics.L1CommitCalldataSize > p.maxL1CommitCalldataSizePerBatch || + totalOverEstimateL1CommitGas > p.maxL1CommitGasPerBatch || + metrics.L1CommitBlobSize > maxBlobSize || + metrics.L1CommitUncompressedBatchBytesSize > p.maxUncompressedBatchBytesSize, + nil +}Then use it in the main loop:
- if metrics.L1CommitCalldataSize > p.maxL1CommitCalldataSizePerBatch || totalOverEstimateL1CommitGas > p.maxL1CommitGasPerBatch || - metrics.L1CommitBlobSize > maxBlobSize || metrics.L1CommitUncompressedBatchBytesSize > p.maxUncompressedBatchBytesSize { + exceedsLimits, err := p.exceedsBatchLimits(metrics) + if err != nil { + return fmt.Errorf("failed to check batch limits: %w", err) + } + if exceedsLimits {
374-388: Consider consolidating metric recording functions to reduce duplication.The
recordTimerBatchMetricsfunction's logic is completely contained withinrecordAllBatchMetrics, which suggests unnecessary duplication.Consider refactoring to eliminate duplication:
func (p *BatchProposer) recordAllBatchMetrics(metrics *utils.BatchMetrics) { p.totalL1CommitGas.Set(float64(metrics.L1CommitGas)) p.totalL1CommitCalldataSize.Set(float64(metrics.L1CommitCalldataSize)) p.batchChunksNum.Set(float64(metrics.NumChunks)) p.totalL1CommitBlobSize.Set(float64(metrics.L1CommitBlobSize)) - p.batchEstimateGasTime.Set(float64(metrics.EstimateGasTime)) - p.batchEstimateCalldataSizeTime.Set(float64(metrics.EstimateCalldataSizeTime)) - p.batchEstimateBlobSizeTime.Set(float64(metrics.EstimateBlobSizeTime)) + p.recordTimerBatchMetrics(metrics) }rollup/internal/controller/watcher/bundle_proposer.go (2)
182-187: Consider updating the log message for CodecV5.The current log message doesn't distinguish between reaching the maximum due to
CodecV5versus the regularmaxBatchesThisBundlelimit. This could make debugging more difficult.- log.Info("reached maximum number of batches per bundle", "batch count", len(batches), "start batch index", batches[0].Index, "end batch index", batches[len(batches)-1].Index) + log.Info("reached maximum number of batches per bundle", "batch count", len(batches), "start batch index", batches[0].Index, "end batch index", batches[len(batches)-1].Index, "codec_version", codecVersion)
197-199: Consider updating the debug message for clarity.The debug message could be more informative by including the current batch count and the maximum allowed.
- log.Debug("pending batches are not enough and do not contain a timeout batch") + log.Debug("pending batches are not enough and do not contain a timeout batch", "current_batch_count", len(batches), "max_allowed", maxBatchesThisBundle)rollup/internal/controller/watcher/chunk_proposer.go (3)
297-299: Ensure proper error handling in 'proposeChunk' methodIn the
proposeChunkmethod, the conditionif proposed, err := p.tryProposeEuclidTransitionChunk(blocks); proposed || err != nil { return err }may lead to returningnilwhenproposedistrueanderrisnil. For better clarity and maintainability, consider handlingproposedanderrseparately to make the code more readable.Suggested change:
+ proposed, err := p.tryProposeEuclidTransitionChunk(blocks) + if err != nil { + return err + } + if proposed { + return nil + }
401-403: Improve error message when parent block retrieval failsWhen
prevBlocks[0].Header.Hash()does not matchblocks[0].Header.ParentHash, the error message"failed to get parent block"may not accurately describe the issue. Consider updating the error message to reflect a potential parent hash mismatch.Suggested change:
- return false, fmt.Errorf("failed to get parent block: %w", err) + return false, fmt.Errorf("parent hash mismatch or failed to get parent block: %w", err)
412-413: Consider using dynamic codec versionCurrently, the
codecVersionis hard-coded toencoding.CodecV5. For future compatibility and maintainability, consider determining the codec version dynamically based on the block's properties, similar to other parts of the code.Suggested change:
- codecVersion := encoding.CodecV5 + codecVersion := encoding.GetCodecVersion(p.chainCfg, blocks[0].Header.Number.Uint64(), blocks[0].Header.Time)database/go.mod (2)
26-26: Addition of Indirect Dependency:github.com/kr/pretty.
A new indirect dependency has been added. Please confirm that this package is truly needed (or will be used transitively) and does not bloat the dependency graph.
31-31: Inclusion ofgithub.com/rogpeppe/go-internal.
This indirect dependency appears to support internal tooling or additional functionality. Verify if this was introduced intentionally and whether its usage is consistent across the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcoordinator/go.sumis excluded by!**/*.sumdatabase/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
Makefile(1 hunks)bridge-history-api/go.mod(3 hunks)common/go.mod(10 hunks)coordinator/go.mod(4 hunks)database/go.mod(2 hunks)rollup/go.mod(4 hunks)rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/sender/sender.go(2 hunks)rollup/internal/controller/watcher/batch_proposer.go(1 hunks)rollup/internal/controller/watcher/bundle_proposer.go(1 hunks)rollup/internal/controller/watcher/chunk_proposer.go(2 hunks)rollup/internal/controller/watcher/l1_watcher.go(2 hunks)rollup/internal/controller/watcher/l2_watcher.go(1 hunks)rollup/tests/rollup_test.go(3 hunks)tests/integration-test/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/internal/controller/relayer/l2_relayer.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: compile
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (46)
Makefile (1)
3-3: Update L2GETH_TAG Version TagThe updated value (
L2GETH_TAG=scroll-v5.8.4) is consistent with the changes made in the corresponding module files (go.mod) and should ensure that all components pull the correct version ofgo-ethereum. Please ensure that this update is also reflected in any related documentation or release notes if needed.rollup/internal/controller/watcher/batch_proposer.go (1)
252-254: LGTM! Version validation is clear and effective.The version validation ensures compatibility by checking against the minimum required version.
rollup/internal/controller/watcher/l2_watcher.go (1)
133-136: Document the Euclid time exception and verify downstream handling.The change to allow nil
RowConsumptionfor Euclid blocks aligns with the PR objectives. However, please:
- Add a comment explaining why Euclid blocks are allowed to have nil
RowConsumption- Verify that downstream code properly handles nil
RowConsumptionfor Euclid blocksLet's verify the downstream handling of nil
RowConsumption:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check how nil RowConsumption is handled downstream # Search for RowConsumption usage rg -A 5 'RowConsumption' # Search for IsEuclid usage to understand the special case rg -A 5 'IsEuclid'Length of output: 30995
Document and Verify Downstream Handling of Nil RowConsumption in Euclid Blocks
The change allowing nil
RowConsumptionfor blocks with a Euclid timestamp appears to be working as intended. The downstream code—particularly in the ORM layer—handles nil values correctly (e.g., via JSON marshaling, which converts a nil to"null"), and the special check inl2_watcher.goprevents non‐Euclid blocks from proceeding without aRowConsumptionvalue.Please add an inline comment describing why Euclid blocks are allowed to have nil
RowConsumptionso that future maintainers understand this exception. Additionally, ensure that any downstream expectations of a non-nilRowConsumptionare adequately documented or adapted, though the current usage (as seen in the ORM and chunk proposer tests) suggests that handling nil values is safe.rollup/internal/controller/watcher/bundle_proposer.go (1)
164-167: Verify the impact of limiting CodecV5 bundles to a single batch.The code enforces a strict limit of one batch per bundle when using
CodecV5. While this change aligns with the architectural changes mentioned in the PR objectives, we should verify:
- The performance impact of processing single-batch bundles
- Whether this limitation is temporary or permanent
- If there are any downstream systems that assume bundles can contain multiple batches
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any dependencies on multi-batch bundles rg -A 5 "len\(.*bundle.*batch.*\)" --type goLength of output: 46
🏁 Script executed:
#!/bin/bash # Let's gather more context on the usage of 'CodecV5' and 'maxBatchesThisBundle' to verify any dependencies on multiple batches. echo "Searching for references to CodecV5 in .go files:" rg -n "CodecV5" --type go echo -e "\nSearching for references to maxBatchesThisBundle in .go files:" rg -n "maxBatchesThisBundle" --type goLength of output: 1546
CodecV5 Single Batch Handling Confirmed
The search results show that the
maxBatchesThisBundlevariable is only used within theBundleProposercode and there are no external dependencies assuming multiple batches for CodecV5. Limiting CodecV5 bundles to a single batch appears intentional, with no evidence of downstream usage issues.
- The only reference to adjusting batch count for CodecV5 is localized within
rollup/internal/controller/watcher/bundle_proposer.go.- Other files and tests (e.g., in the rollup test suite) simply select among codec versions without imposing multi-batch semantics for CodecV5.
rollup/internal/controller/watcher/l1_watcher.go (2)
83-83: LGTM!The
CalcBlobFeecall has been correctly updated to use the new package location.
9-9: Verify the package reorganization in go-ethereum.The import path has been updated from
eip4844tomisc, reflecting a package reorganization in go-ethereum.✅ Verification successful
🌐 Web query:
Has the EIP-4844 functionality in go-ethereum been moved from eip4844 to misc package?💡 Result:
Based on the provided search results, there is no direct mention of EIP-4844 functionality being moved from an `eip4844` package to a `misc` package in go-ethereum. However, some relevant technical details were found: 1. **op-geth fork changes** (Search Result 6) show modifications to Ethereum's execution layer client implementation, including: - Engine API extensions for rollup-specific block building - Transaction pool adjustments for L1 cost accounting - Dynamic EIP-1559 parameter support 2. **Core implementation details** (Search Result 4) specify that EIP-4844 modifications span both: - Consensus layer (beacon node implementations) - Execution layer (geth client modifications) The search results don't explicitly confirm package restructuring between `eip4844` and `misc`, but they demonstrate that EIP-4844 implementation affects multiple components across Ethereum's stack. For definitive code structure confirmation, you'd need to consult: 1. The latest go-ethereum GitHub repository 2. Official Ethereum client documentation 3. Core developer changelogs No matching evidence was found in the provided sources about this specific package migration[6][4]. Citations: - 1: https://magazine.mindplex.ai/understanding-eip-4844-ethereums-proto-danksharding-upgrade-for-layer-2s/ - 2: https://www.quicknode.com/guides/ethereum-development/transactions/eip4844-explained - 3: https://blog.coinshares.com/understanding-eip-4844-how-it-greatly-reduces-transaction-fees-for-ethereum-layer-2s-9cc106ffc077?gi=cc5ae08d0c83 - 4: https://www.eip4844.com - 5: https://nownodes.io/blog/ethereum-dencun-upgrade-eip-4844-proto-danksharding/ - 6: https://op-geth.optimism.io - 7: https://www.galaxy.com/insights/research/ethereum-150-days-after-dencun/ - 8: https://pkg.go.dev/github.com/jumbochain/jumbochain-parlia-go - 9: https://metlabs.io/en/eip-4844-ethereum-scalability/ - 10: https://blockworks.co/news/ethereum-improvement-proposal-4844
🏁 Script executed:
#!/bin/bash echo "Searching for references to 'go-ethereum/consensus/eip4844'..." rg -n "go-ethereum/consensus/eip4844" || echo "No references found for eip4844." echo "Searching for references to 'go-ethereum/consensus/misc'..." rg -n "go-ethereum/consensus/misc" || echo "No references found for misc."Length of output: 627
EIP-4844 Package Reorganization Verified
- The repository no longer contains any references to the
eip4844package.- Both
rollup/internal/controller/watcher/l1_watcher.goandrollup/internal/controller/sender/sender.gonow import from themiscpackage, confirming the apparent reorganization.rollup/internal/controller/sender/sender.go (2)
15-15: LGTM!The import path update is consistent with the package reorganization.
678-678: LGTM!The
CalcBlobFeecall has been correctly updated to use the new package location.rollup/internal/controller/watcher/chunk_proposer.go (1)
395-422: Add unit tests for 'tryProposeEuclidTransitionChunk'The new method
tryProposeEuclidTransitionChunkis crucial for handling the Euclid transition. Ensure that it is adequately covered by unit tests, including edge cases such as the genesis block handling and transitions between different phases.Would you like assistance in creating unit tests for this function or opening a new issue to track this task?
database/go.mod (3)
3-5: Update Go Version and Toolchain Specification.
The Go version is now upgraded to 1.22 and a toolchain (go1.22.2) has been specified. Please make sure that your build/CI environment is compatible with Go 1.22 and the selected toolchain.
11-12: Upgrade Core Dependencies.
The versions forgithub.com/scroll-tech/go-ethereumandgithub.com/stretchr/testifyhave been updated. Verify that these newer versions do not introduce breaking changes or affect any runtime assumptions in your code.
37-41: Upgrade of Golang.org/x Dependencies.
The versions forgolang.org/x/crypto,golang.org/x/sync,golang.org/x/sys, andgolang.org/x/texthave been updated. Ensure these upgrades are backward compatible with your codebase and that no subtle API changes affect your application.tests/integration-test/go.mod (3)
3-5: Update Go Version and Toolchain Specification.
The module now targets Go 1.22 and specifies the toolchaingo1.22.2. This update should be validated against your integration tests to ensure no compatibility issues arise.
8-10: Upgrade Main Dependencies.
The dependencies forgithub.com/scroll-tech/da-codec,github.com/scroll-tech/go-ethereum, andgithub.com/stretchr/testifyhave been updated. Double-check that these updates work seamlessly within the integration-test suite.
15-47: Review Indirect Dependencies Upgrade.
A number of indirect packages—including updates for libraries likebitset,bavard,gnark-crypto,crate-crypto, and others—have been refreshed. Ensure that these changes do not lead to transitive issues with your integration tests.coordinator/go.mod (4)
3-5: Update Go Version and Toolchain in Coordinator Module.
The Go version has been bumped to 1.22 with the toolchaingo1.22.2. Verify that this change is consistently adopted in CI and that no dependent tooling is disrupted.
14-17: Upgrade Direct Dependencies.
Key dependencies likegithub.com/scroll-tech/da-codec,github.com/scroll-tech/go-ethereum, andgithub.com/stretchr/testifyhave their versions updated. Please double-check integration points in the coordinator logic to confirm these updates are fully compatible.
45-46: Update Golang.org/x Dependencies.
The updates togolang.org/x/net(v0.23.0) andgolang.org/x/text(v0.21.0) should be validated for any API or behavioral changes.
51-84: Review Indirect Dependencies for Consistency.
This block updates many indirect dependencies (e.g.bits-and-blooms/bitset,bavard,gnark-crypto,crate-crypto,holiman/uint256, and others). Ensure that these library versions are aligned with your overall dependency management strategy.bridge-history-api/go.mod (5)
3-6: Update Go Version and Toolchain.
The module now uses Go 1.22 with the specified toolchaingo1.22.2. Please ensure that any associated scripts or CI configurations reflect this change.
13-14: Upgrade Core Dependencies.
The updated versions forgithub.com/scroll-tech/go-ethereumandgithub.com/stretchr/testifyshould be verified against the API contracts used in the bridge-history-api module.
16-16: Updategolang.org/x/sync.
The dependency togolang.org/x/syncis upgraded to v0.11.0. Validate that this improvement does not alter expected synchronization behavior.
21-47: Review Indirect Dependencies (Block 1).
This section upgrades various indirect dependencies (for example,fastcache,bitset,gnark-crypto, etc.). Confirm that these new versions are compatible and solve any known issues without introducing new problems.
82-124: Review Indirect Dependencies (Block 2).
A broad array of indirect dependencies have been updated, including changes to Prometheus libraries, several golang.org/x libraries, and packages likenpipeandurfave/cli. Ensure these versions are in-sync with other modules across the project.rollup/go.mod (3)
3-5: Update Go Version and Toolchain in Rollup Module.
The file now specifies Go 1.22 with the toolchaingo1.22.2. Confirm that all build and deployment scripts reflect this change uniformly across modules.
7-23: Upgrade Direct Dependencies in Rollup Module.
Several direct dependencies have been updated:
github.com/consensys/gnark-crypto→ v0.16.0github.com/crate-crypto/go-kzg-4844→ v1.1.0github.com/holiman/uint256→ v1.3.2da-codecandgo-ethereumnow match the updated versions from other modules.testifyis now at v1.10.0.Please verify that these version bumps integrate well with the rollup logic and do not cause compatibility issues.
25-131: Review Comprehensive Indirect Dependencies.
This extensive list of indirect dependencies has been updated with many version bumps. Ensure that every updated package (including libraries likeprometheus/tsdb, variousgolang.org/x/*packages, and helper packages such asnpipeandurfave/cli.v1) is consistent across the codebase and that no conflicts arise.common/go.mod (18)
3-5: Go Version and Toolchain UpgradeThe module now requires Go 1.22 with an explicit toolchain set to go1.22.2. Ensure that all downstream dependencies and CI/CD tooling support this updated version.
9-9: Dependency Update: BitsetUpgraded
github.com/bits-and-blooms/bitsetto v1.20.0. Please verify that no breaking API changes affect its usage downstream.
18-19: Dependency Updates: go-ethereum and TestifyUpdated:
github.com/scroll-tech/go-ethereumto v1.10.14-0.20250205135740-4bdf6d096c38github.com/stretchr/testifyto v1.10.0Please confirm these updates are compatible with existing code and that no testing regressions occur.
35-35: Dependency Update: FastcacheUpgraded
github.com/VictoriaMetrics/fastcacheto v1.12.2. It is recommended to review the release notes to ensure that caching behavior remains consistent.
58-58: Dependency Update: Consensys BavardBumped
github.com/consensys/bavardto v0.1.29. Please check that the updated version integrates smoothly with the proof system components.
59-59: Dependency Update: Gnark-CryptoUpgraded
github.com/consensys/gnark-cryptoto v0.16.0. Given the changes in cryptographic functionality, ensure that all proof verification and construction flows are thoroughly tested.
66-67: Dependency Updates: go-md2man and go-kzg-4844Updated:
github.com/cpuguy83/go-md2man/v2to v2.0.4github.com/crate-crypto/go-kzg-4844to v1.1.0Verify that documentation generation (if using go-md2man) and any cryptographic operations relying on go-kzg-4844 remain unbroken.
69-69: Dependency Update: Golang-SetUpgraded
github.com/deckarep/golang-setto v0.0.0-20180603214616-504e848d77ea. Review its usage to ensure that no deprecated or breaking APIs are introduced.
80-80: Dependency Update: mmap-goBumped
github.com/edsrzf/mmap-goto v1.0.0. Please ensure that any memory-mapped file operations in the code continue to behave as expected.
182-182: New Indirect Dependency: Prometheus TSDBAdded
github.com/prometheus/tsdbv0.7.1 as an indirect dependency. Confirm that its inclusion does not pull unwanted transitive dependencies.
184-184: Dependency Update: NotifyUpgraded
github.com/rjeczalik/notifyto v0.9.1. Please verify that file-system event handling remains stable.
187-187: Dependency Update: da-codecBumped
github.com/scroll-tech/da-codecto v0.1.3-0.20241218102542-9852fa4e1be5. Ensure that any encoding/decoding logic utilizing this package is compatible with the new version.
197-197: Dependency Update: CobraUpdated
github.com/spf13/cobrato v1.8.1. It’s important to confirm that command-line interface behaviors and flag parsing remain consistent post-update.
201-201: Dependency Update: BLSTBumped
github.com/supranational/blstto v0.3.13. Given its role in cryptography, ensure that all related operations are validated after the update.
206-206: Dependency Update: numcpusUpgraded
github.com/tklauser/numcpusto v0.9.0. Please double-check its behavior in concurrency-related parts of the code.
238-238: Dependency Update: golang.org/x/cryptoUpdated
golang.org/x/cryptoto v0.32.0. Verify that all cryptographic primitives and functions perform as expected under this new version.
243-246: Dependency Updates: x/sync, x/sys, x/term, and x/textUpgraded the following packages:
golang.org/x/syncto v0.11.0golang.org/x/systo v0.30.0golang.org/x/termto v0.28.0golang.org/x/textto v0.21.0Ensure that these updates do not affect low-level system calls, terminal interactions, or text processing functionalities.
256-257: Dependency Updates: npipe and urfave/cliReplaced
gopkg.in/natefinch/lumberjack.v2withgopkg.in/natefinch/npipe.v2(v2.0.0-20160621034901-c1b8fa8bdcce) and updatedgopkg.in/urfave/cli.v1to v1.20.0. Please confirm that the new npipe implementation satisfies the expected pipe utilities and that CLI functionalities remain robust.rollup/tests/rollup_test.go (1)
162-166: Verify the Halo2BundleProof structure matches the interface contract.The code has been updated to use the new
Halo2BundleProoftype. Ensure that this type correctly implements theBundleProofinterface as mentioned in the AI summary.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that Halo2BundleProof implements the BundleProof interface ast-grep --pattern $'type Halo2BundleProof struct { $$$ } func (p *Halo2BundleProof) Proof() []byte { $$$ }'Length of output: 114
🏁 Script executed:
#!/bin/bash # This script gathers information on the BundleProof interface and the Halo2BundleProof implementation echo "Searching for the BundleProof interface definition:" rg "type BundleProof interface" -n echo "--------------" echo "Searching for the Halo2BundleProof struct definition:" rg "type Halo2BundleProof struct" -n echo "--------------" echo "Searching for the Proof() method on Halo2BundleProof:" rg "func (p \\*Halo2BundleProof) Proof() \\[\\]byte" -nLength of output: 683
🏁 Script executed:
#!/bin/bash # Let's search for any method with (p *Halo2BundleProof) in the codebase to check for any potential Proof method implementation. rg "func (p \\*Halo2BundleProof)" -n echo "--------------" # Additionally, let's inspect the context in common/types/message/message.go around the Halo2BundleProof struct definition. sed -n '160,220p' common/types/message/message.goLength of output: 1347
Halo2BundleProof implements the BundleProof interface correctly.
Both required methods,Proof() []byteandSanityCheck() error, are implemented in theHalo2BundleProoftype as defined incommon/types/message/message.go.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
2 times, most recently
from
ec6ebcd to
1a879fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
common/types/message/message_test.go (2)
13-13: Improve error message formatting.The error messages could be more descriptive. Include the file path in the error message for better debugging.
- t.Fatalf("failed to read batch proof sample.json: %v", err) + t.Fatalf("failed to read batch proof sample file 'batch-proof-sample.json': %v", err) - t.Fatalf("failed to read bundle proof sample.json: %v", err) + t.Fatalf("failed to read bundle proof file 'bundle-proof-sample.json': %v", err)Also applies to: 28-28
17-23: Add test cases for error scenarios.The test only covers the happy path. Consider adding test cases for:
- Invalid JSON data
- Missing required fields
- Invalid proof data format
Also applies to: 32-38
rollup/tests/rollup_test.go (1)
142-146: Add test cases for proof verification.The test initializes proofs with static data but doesn't verify the proof content. Consider adding test cases to verify:
- Proof data integrity
- Verification of proof signatures
- Handling of invalid proofs
Also applies to: 157-161
common/types/message/message.go (1)
81-81: Extract repeated string literal into a constant.The string literal "euclid" is used multiple times. Extract it into a constant to improve maintainability.
+// HardForkEuclid represents the Euclid hard fork name +const HardForkEuclid = "euclid" - case "euclid": + case HardForkEuclid:Also applies to: 115-115, 172-172
🧰 Tools
🪛 GitHub Actions: Common
[error] 81-81: string
euclidhas 3 occurrences, make it a constant (goconst)rollup/internal/controller/watcher/chunk_proposer.go (2)
412-412: Use constant for codec version.The codec version is hardcoded to
CodecV5. Consider using a constant or configuration value.+const EuclidTransitionCodecVersion = encoding.CodecV5 - codecVersion := encoding.CodecV5 + codecVersion := EuclidTransitionCodecVersion
395-422: Add metrics for Euclid transition chunks.The new Euclid transition functionality lacks metric tracking. Consider adding metrics to monitor:
- Number of transition chunks proposed
- Success/failure rates
- Transition timing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcoordinator/go.sumis excluded by!**/*.sumdatabase/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
Makefile(1 hunks)bridge-history-api/go.mod(3 hunks)common/go.mod(10 hunks)common/types/message/bundle-proof-sample.json(1 hunks)common/types/message/message.go(6 hunks)common/types/message/message_test.go(1 hunks)coordinator/go.mod(4 hunks)database/go.mod(2 hunks)rollup/go.mod(4 hunks)rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/sender/sender.go(2 hunks)rollup/internal/controller/watcher/batch_proposer.go(1 hunks)rollup/internal/controller/watcher/bundle_proposer.go(1 hunks)rollup/internal/controller/watcher/chunk_proposer.go(2 hunks)rollup/internal/controller/watcher/l1_watcher.go(2 hunks)rollup/internal/controller/watcher/l2_watcher.go(1 hunks)rollup/tests/rollup_test.go(4 hunks)tests/integration-test/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/types/message/bundle-proof-sample.json
🚧 Files skipped from review as they are similar to previous changes (11)
- rollup/internal/controller/watcher/bundle_proposer.go
- rollup/internal/controller/watcher/l1_watcher.go
- rollup/internal/controller/watcher/l2_watcher.go
- Makefile
- rollup/internal/controller/watcher/batch_proposer.go
- rollup/internal/controller/relayer/l2_relayer.go
- database/go.mod
- rollup/internal/controller/sender/sender.go
- coordinator/go.mod
- bridge-history-api/go.mod
- common/go.mod
🧰 Additional context used
🪛 GitHub Actions: Common
common/types/message/message.go
[error] 81-81: string euclid has 3 occurrences, make it a constant (goconst)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: compile
- GitHub Check: tests
🔇 Additional comments (9)
common/types/message/message_test.go (1)
11-14: Verify test data file availability.The test depends on external JSON files (
batch-proof-sample.jsonandbundle-proof-sample.json). Ensure these files are committed to the repository and document their location.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the required JSON files exist in the repository fd -t f "batch-proof-sample.json" "bundle-proof-sample.json"Length of output: 170
🏁 Script executed:
#!/bin/bash # Verify that the required JSON test data files exist in the repository. # This script individually searches for each file starting from the repository root. for file in "batch-proof-sample.json" "bundle-proof-sample.json"; do echo "Checking for: $file" result=$(fd -t f "$file" .) if [ -n "$result" ]; then echo "Found file '$file' at:" echo "$result" else echo "File '$file' not found in the repository." fi doneLength of output: 1014
Test data files located in
common/types/messageare available and committed.
- Both
batch-proof-sample.jsonandbundle-proof-sample.jsonexist in the repository at./common/types/message.- Update the test documentation if necessary to indicate these file locations.
rollup/tests/rollup_test.go (1)
56-56: Inconsistency between added codec versions and validation logic.The test adds support for CodecV5 and CodecV6 in the
codecVersionsslice, but the validation logic only handles CodecV4.Either:
- Update the validation logic to handle CodecV5 and CodecV6, or
- Remove CodecV5 and CodecV6 from the
codecVersionsslice.-codecVersions := []encoding.CodecVersion{encoding.CodecV4, encoding.CodecV5, encoding.CodecV6} +codecVersions := []encoding.CodecVersion{encoding.CodecV4}rollup/internal/controller/watcher/chunk_proposer.go (1)
400-403:⚠️ Potential issuePrevent potential underflow when accessing previous block.
Subtracting 1 from
blocks[0].Header.Number.Uint64()may cause an underflow ifblocks[0]is the genesis block.+ if blocks[0].Header.Number.Uint64() == 0 { + // Genesis block, no previous block + return false, fmt.Errorf("cannot get parent block for genesis block") + } prevBlocks, err := p.l2BlockOrm.GetL2BlocksGEHeight(p.ctx, blocks[0].Header.Number.Uint64()-1, 1)Likely invalid or redundant comment.
tests/integration-test/go.mod (3)
3-5: Go Version and Toolchain UpdateThe Go version has been bumped to 1.22 and the toolchain is explicitly set to go1.22.2. This ensures consistency with your dependency requirements and aligns with the overall upgrade strategy.
8-12: Core Dependency Updates VerificationThe primary require block reflects updates for key dependencies:
•github.com/scroll-tech/da-codecis now on v0.1.3-0.20241218102542-9852fa4e1be5.
•github.com/scroll-tech/go-ethereumis updated as expected.
•github.com/stretchr/testifyhas been bumped to v1.10.0.These changes are in line with the broader project updates. Please verify that any local tests using these dependencies are passing.
15-47: Indirect Dependencies AlignmentThe indirect dependency block has been updated across numerous packages (e.g.,
bitset,bavard,gnark-crypto,go-kzg-4844,uint256,go-iden3-crypto, etc.). These updates help maintain compatibility and security across the project.
It is recommended to double-check that no version conflicts arise with these updates across other modules.rollup/go.mod (3)
3-5: Go Version and Toolchain UpdateSimilar to the integration-test module, the rollup module now targets Go 1.22 with a specified toolchain (go1.22.2). This update promotes consistency across the repository.
7-23: Direct Dependency Version BumpsThe first require block in rollup/go.mod shows several dependencies updated as follows:
•github.com/consensys/gnark-cryptois updated to v0.16.0.
•github.com/crate-crypto/go-kzg-4844has been bumped to v1.1.0.
•github.com/holiman/uint256now uses v1.3.2.
•github.com/scroll-tech/da-codecandgithub.com/scroll-tech/go-ethereummatch the intended update.
•github.com/stretchr/testifynow appears in its v1.10.0 release.These updates are consistent with changes in other modules. Ensure that any module-level integration tests involving these dependencies run smoothly.
25-131: Indirect Dependencies Comprehensive UpdateThe second require block updates a large number of indirect dependencies. Notable updates include:
•github.com/bits-and-blooms/bitsetto v1.20.0.
•github.com/deckarep/golang-setand several other packages now reflect the updated versions.
• Updates forgolang.org/x/crypto,golang.org/x/sync,golang.org/x/sys, andgolang.org/x/textalso seem aligned with the overall upgrade to Go 1.22.This thorough refresh should improve compatibility, performance, and security. It is advisable to run the full test suite to ensure that the dependency updates do not introduce any breaking changes.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
4 times, most recently
from
b3b0fea to
b3d62dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
rollup/tests/rollup_test.go (2)
55-55: Function name should reflect its purpose.The function name
testCommitBatchAndFinalizeBundleCodecV4V5V6suggests support for CodecV4, V5, and V6, but the test only uses CodecV4 and CodecV5. Consider renaming it to accurately reflect the codecs being tested.-func testCommitBatchAndFinalizeBundleCodecV4V5V6(t *testing.T) { +func testCommitBatchAndFinalizeBundle(t *testing.T) {
142-146: Use constants for mock proof data.The hardcoded byte arrays for proof data should be extracted into constants for better maintainability and reusability.
+const ( + mockProofBytes = []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31} +) batchProof := &message.Halo2BatchProof{ - RawProof: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, - Instances: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, - Vk: []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31}, + RawProof: mockProofBytes, + Instances: mockProofBytes, + Vk: mockProofBytes,rollup/internal/controller/watcher/chunk_proposer.go (1)
395-422: Add metrics for Euclid transition chunks.The
tryProposeEuclidTransitionChunkfunction handles an important transition but lacks metrics tracking. Consider adding metrics to monitor transition chunk proposals.+ euclidTransitionChunkTotal prometheus.Counter + euclidTransitionChunkFailureTotal prometheus.Counter func NewChunkProposer(ctx context.Context, cfg *config.ChunkProposerConfig, minCodecVersion encoding.CodecVersion, chainCfg *params.ChainConfig, db *gorm.DB, reg prometheus.Registerer) *ChunkProposer { + p.euclidTransitionChunkTotal = promauto.With(reg).NewCounter(prometheus.CounterOpts{ + Name: "rollup_propose_euclid_transition_chunk_total", + Help: "Total number of Euclid transition chunk proposals.", + }) + p.euclidTransitionChunkFailureTotal = promauto.With(reg).NewCounter(prometheus.CounterOpts{ + Name: "rollup_propose_euclid_transition_chunk_failure_total", + Help: "Total number of failed Euclid transition chunk proposals.", + }) func (p *ChunkProposer) tryProposeEuclidTransitionChunk(blocks []*encoding.Block) (bool, error) { + p.euclidTransitionChunkTotal.Inc() if !p.chainCfg.IsEuclid(blocks[0].Header.Time) { return false, nil } // ... rest of the function if err := p.updateDBChunkInfo(&chunk, codecVersion, metrics); err != nil { + p.euclidTransitionChunkFailureTotal.Inc() return false, err } return true, nil }rollup/internal/controller/sender/sender_test.go (1)
291-292: Improve transaction hash tracking.The code now tracks multiple transaction hashes but could benefit from a more structured approach. Consider using a map to track transaction status and avoid duplicate checks.
+ type txStatus struct { + hash common.Hash + checked bool + pending bool + } + txStatuses := make(map[common.Hash]*txStatus) + for _, hash := range txHashes { + if status, exists := txStatuses[hash]; !exists || !status.checked { + _, isPending, err := s.client.TransactionByHash(context.Background(), hash) + txStatuses[hash] = &txStatus{ + hash: hash, + checked: true, + pending: err == nil && isPending, + } + if err == nil && !isPending { + return true + } + } + }Also applies to: 299-300
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcoordinator/go.sumis excluded by!**/*.sumdatabase/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
Makefile(1 hunks)bridge-history-api/go.mod(3 hunks)common/go.mod(10 hunks)common/types/message/bundle-proof-sample.json(1 hunks)common/types/message/message.go(7 hunks)common/types/message/message_test.go(1 hunks)coordinator/go.mod(4 hunks)database/go.mod(2 hunks)rollup/go.mod(4 hunks)rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/sender/sender.go(2 hunks)rollup/internal/controller/sender/sender_test.go(1 hunks)rollup/internal/controller/watcher/batch_proposer.go(1 hunks)rollup/internal/controller/watcher/bundle_proposer.go(1 hunks)rollup/internal/controller/watcher/chunk_proposer.go(2 hunks)rollup/internal/controller/watcher/l1_watcher.go(2 hunks)rollup/internal/controller/watcher/l2_watcher.go(1 hunks)rollup/tests/bridge_test.go(1 hunks)rollup/tests/rollup_test.go(1 hunks)tests/integration-test/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- rollup/tests/bridge_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- Makefile
- rollup/internal/controller/watcher/bundle_proposer.go
- common/types/message/message_test.go
- common/types/message/bundle-proof-sample.json
- rollup/internal/controller/watcher/batch_proposer.go
- rollup/internal/controller/watcher/l1_watcher.go
- rollup/internal/controller/watcher/l2_watcher.go
- database/go.mod
- rollup/internal/controller/sender/sender.go
- rollup/internal/controller/relayer/l2_relayer.go
- rollup/go.mod
- bridge-history-api/go.mod
- coordinator/go.mod
- common/go.mod
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (16)
rollup/internal/controller/watcher/chunk_proposer.go (1)
400-403:⚠️ Potential issuePrevent potential underflow when accessing previous block.
Subtracting 1 from
blocks[0].Header.Number.Uint64()may cause an underflow ifblocks[0]is the genesis block (block number 0), resulting in an invalid block height when retrieving the previous block.Apply this diff to handle the genesis block case:
+ if blocks[0].Header.Number.Uint64() == 0 { + // Genesis block, no previous block + return false, nil + } prevBlocks, err := p.l2BlockOrm.GetL2BlocksGEHeight(p.ctx, blocks[0].Header.Number.Uint64()-1, 1)Likely invalid or redundant comment.
tests/integration-test/go.mod (15)
3-5: Go Version and Toolchain UpdateThe go.mod now specifies Go 1.22 with an explicit toolchain (go1.22.2). This ensures that the module builds with the expected language features and behaviors in Go 1.22. Please confirm that all integration tests run correctly under this version.
8-10: First Require Block Dependency UpdatesThe dependency versions for "github.com/scroll-tech/da-codec", "github.com/scroll-tech/go-ethereum", and "github.com/stretchr/testify" have been updated. Ensure that any changes in these libraries’ APIs (especially in test helpers for testify) have been accounted for in the integration tests.
15-15: Updated Bitset Dependency"Github.com/bits-and-blooms/bitset" is now updated to v1.20.0. Review its changelog for any performance improvements or API modifications that might affect parts of the code relying on bitset operations.
17-17: Updated Bavard DependencyUpgrading "github.com/consensys/bavard" to v0.1.29 should bring minor improvements or fixes. Please verify that modules using this library continue to function as expected.
18-18: Updated Gnark-Crypto DependencyThe update of "github.com/consensys/gnark-crypto" to v0.16.0 may include important cryptographic enhancements and security fixes. It is advisable to run the relevant cryptographic tests to ensure no regressions occur.
19-19: Updated Go-KZG-4844 Dependency"Github.com/crate-crypto/go-kzg-4844" is updated to v1.1.0. Check if the update delivers bug fixes or performance improvements and that its usage in proof or verification routines isn’t adversely affected.
27-27: Updated Uint256 Dependency"Github.com/holiman/uint256" has been upgraded to v1.3.2. This update likely brings improved functionality for handling large integers. Please verify any arithmetic-dependent code paths.
28-28: Updated Iden3-Crypto DependencyThe dependency "github.com/iden3/go-iden3-crypto" is updated to v0.0.17. It is recommended to review the changelog for any API changes that could affect cryptographic or identity verification functions.
34-34: Updated Notify DependencyUpgrading "github.com/rjeczalik/notify" to v0.9.1 should enhance file system event monitoring. Please verify that any file-watching or event-related functionality still performs as expected.
37-37: Updated Supranational BLST DependencyThe dependency "github.com/supranational/blst" has been updated to v0.3.13. As this library is critical for cryptographic operations, ensure that its integration (e.g., in signature aggregation) is fully covered by tests.
39-39: Updated Numcpus Dependency"Github.com/tklauser/numcpus" is now updated to v0.9.0. This could affect concurrency behavior by providing a more accurate count of CPUs. It is recommended to ascertain that the changes do not adversely impact performance-sensitive code.
41-41: Updated golang.org/x/crypto DependencyThe update to "golang.org/x/crypto" v0.32.0 is important given the sensitivity around cryptographic functionality. Please ensure that all cryptography-related tests are re-run to confirm compatibility.
42-42: Updated golang.org/x/sync DependencyUpdating "golang.org/x/sync" to v0.11.0 should provide improved synchronization primitives. Confirm that concurrent operations within the integration suite are functioning correctly.
43-43: Updated golang.org/x/sys DependencyThe upgrade of "golang.org/x/sys" to v0.30.0 brings updates to system call interactions. It is advisable to verify that system-level behaviors, especially in cross-platform testing, continue to operate as expected.
44-44: Added Natefinch Npipe DependencyThe new indirect dependency "gopkg.in/natefinch/npipe.v2" at v2.0.0-20160621034901-c1b8fa8bdcce indicates a requirement for named pipe support, likely in Windows environments. Verify its necessity in the integration tests and add documentation if its purpose is not immediately obvious.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
common/libzkp/impl/src/verifier.rs (4)
51-54: Be mindful of global environment variable side effects.
Setting SCROLL_PROVER_ASSETS_DIR to config.halo2_circuit.assets_path will affect the entire process. Ensure that multiple inits or different configurations won't conflict and that external consumers are aware of this side effect.
64-70: Improve error handling in load_params usage.
Lines 64 and 70 panic on failure to load parameters. Consider user-friendly error handling or graceful fallback to avoid abrupt termination if the params file is missing or invalid.
79-82: Confirm that DarwinV2Verifier is strictly a Halo2 verifier.
You're instantiating DarwinV2Verifier and storing it in VERIFIER_HALO2. If Darwin V2 is indeed specific to Halo2, the naming is consistent. Otherwise, consider clarifying the name to better convey its role.
108-114: Consider supporting multiple forks more dynamically.
Lines 108–114 match the requested fork_name against the stored verifiers. If additional forks are introduced, you might want a more extensible mechanism. For now, this logic is straightforward and functional.common/libzkp/impl/src/verifier/euclid.rs (2)
16-18: Clarify or resolve the TODO regarding EVM parameter management.
This TODO suggests a potential refactor or consolidation of parameter management solely within the bundle verifier. Please confirm if you plan to revise this section to avoid duplicate or unnecessary parameter usage.Would you like assistance drafting a proposal for consolidating these parameters so only the bundle verifier needs them?
19-19: Consider removing or utilizing _params_map.
Currently, this parameter isn’t used, which may lead to confusion. Either remove it if truly unnecessary or utilize it for configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
common/libzkp/impl/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
common/libzkp/impl/Cargo.toml(1 hunks)common/libzkp/impl/rust-toolchain(1 hunks)common/libzkp/impl/src/verifier.rs(4 hunks)common/libzkp/impl/src/verifier/darwin.rs(0 hunks)common/libzkp/impl/src/verifier/euclid.rs(1 hunks)
💤 Files with no reviewable changes (1)
- common/libzkp/impl/src/verifier/darwin.rs
✅ Files skipped from review due to trivial changes (1)
- common/libzkp/impl/rust-toolchain
🔇 Additional comments (15)
common/libzkp/impl/Cargo.toml (6)
15-15: Added ethers-signers Dependency
The addition of the ethers-signers dependency nicely complements the ethers-core and ethers-providers already referenced. Please verify that the branch "v2.0.7" remains compatible with the rest of the ethers libraries in the project.
17-22: New Zero-Knowledge Proof & EVM Dependencies
The inclusion of new dependencies (ruint, alloy-primitives, revm, revm-interpreter, revm-precompile, and revm-primitives) appears essential for supporting the new Euclid features. Ensure that pinning to specific branches (e.g., "v1.12.3" for ruint and "scroll-evm-executor/v50" for the revm-related crates) does not introduce compatibility issues later.
25-25: Updated halo2_proofs Patch Dependency
The patch dependency for halo2_proofs now points to branch "v1.1". Confirm that this update addresses any previous version mismatches and integrates well with other halo2-related dependencies across the project.
33-37: Reformatted snark-verifier-sdk Declaration
The dependency declaration for snark-verifier-sdk has been reformatted for better readability with features listed on separate lines. Please double-check that all enabled features ("loader_halo2", "loader_evm", "halo2-pse") are indeed required for the intended verifier functionality.
40-43: Updated prover_v5 Dependency Declaration
The reformatted prover_v5 declaration clearly lists its features ("parallel_syn", "scroll") under the "darwin_v2" context. Verify that this configuration remains compatible with the existing circuits and that any adjustments serve the overall build expectation.
45-46: New Euclid Prover & Verifier Dependencies Added
The introduction of prover_v7 and verifier_v7 (both tagged as "v0.1.0-rc.1") is a significant change to support the new Euclid feature. Ensure thorough integration testing, document any potential breaking changes, and confirm that these new dependencies integrate smoothly with the existing architecture.common/libzkp/impl/src/verifier.rs (9)
2-6: Consider documenting and testing the new Euclid module.
You've introduced "mod euclid" (line 2) and are now using the "EuclidVerifier" (line 6). Please ensure that the new module has sufficient documentation, examples, and test coverage to verify its functionality and maintainability.
30-34: Validate the necessity of OpenVMConfig fields.
The newly added OpenVMConfig struct (lines 30–34) only contains fork_name and workspace_path. Ensure no additional configuration fields (e.g., params_path or assets_path) are needed for the zkVM circuit.
38-39: New fields in VerifierConfig appear consistent.
You've added halo2_circuit and zkvm_circuit to VerifierConfig. The naming convention is straightforward, and each field references a dedicated config struct. This approach looks coherent.
46-47: Revisit the safety of static mutable usage with OnceCell.
Declaring VERIFIER_ZKVM and VERIFIER_HALO2 as mutable statics can introduce concurrency pitfalls if not carefully managed. Although OnceCell reduces risks, consider verifying that no race conditions can occur when initializing or re-initializing these verifiers.
56-57: Check memory constraints for large polynomial degrees.
You're using LAYER2_DEGREE and LAYER4_DEGREE from prover_v5. Large degree polynomials can result in significant memory usage. Verify that your environment can handle expansions at these degrees without performance regressions.
84-86: Use of fork_name.clone() is acceptable.
Storing config.halo2_circuit.fork_name in the verifier is standard practice. This usage is straightforward and should not introduce issues.
92-95: EuclidVerifier instantiation appears sound.
Your EuclidVerifier::new call references the shared PARAMS_MAP and workspace_path. The design is consistent with your refactored config.
97-99: Placing EuclidVerifier in VERIFIER_ZKVM matches naming.
Since EuclidVerifier handles the zkVM circuit, storing it in VERIFIER_ZKVM is appropriately named. This aligns well with the new config structure.
8-8: Confirm compatibility of the load_params usage.
You're now importing load_params on line 8. Check that the function signature aligns with your usage, particularly if upgrades have occurred in the prover_v5 library.Run the following script to list all references to load_params and compare them with the current signature:
✅ Verification successful
🏁 Script executed:
#!/bin/bash rg "load_params" -A 5Length of output: 911
Verified: The load_params usage is compatible with its current signature.
The grep results confirm that load_params is invoked with three arguments—namely, a reference to the parameters path, a value for degree, and None—which aligns with its expected signature in prover_v5 as currently used in the verifier. No discrepancies were identified.
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
from
94d8f74 to
9d019a0
Compare
omerfirmak
force-pushed
the
omerfirmak/euclid-prover
branch
2 times, most recently
from
bab780e to
38cc6ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
common/libzkp/impl/src/verifier.rs (2)
46-100: Add proper error handling and remove unsafe code.The initialization function has several issues:
- Uses
unwrap_uncheckedwhich can lead to undefined behavior- No error handling for environment variable setting
- Extensive use of unsafe blocks
Consider this safer approach:
pub fn init(config: VerifierConfig) -> Result<()> { std::env::set_var( "SCROLL_PROVER_ASSETS_DIR", &config.low_version_circuit.assets_path, ); let params_map = { let mut map = BTreeMap::new(); for degree in [ *prover_v5::config::LAYER2_DEGREE, *prover_v5::config::LAYER4_DEGREE, ] { if !map.contains_key(°ree) { let params = load_params( &config.low_version_circuit.params_path, degree, None, ).map_err(|e| anyhow::anyhow!( "failed to load params, degree {}, dir {}, err {}", degree, config.low_version_circuit.params_path, e ))?; map.insert(degree, params); } } map }; PARAMS_MAP.get_or_init(|| RwLock::new(params_map)); // Initialize verifiers let halo2_verifier = DarwinV2Verifier::new( &PARAMS_MAP.get().unwrap().read().unwrap(), &config.low_version_circuit.assets_path, ); VERIFIER_HALO2.get_or_init(|| RwLock::new(Some(VerifierPair( config.low_version_circuit.fork_name.clone(), Rc::new(Box::new(halo2_verifier)), )))); let zkvm_verifier = EuclidVerifier::new( &PARAMS_MAP.get().unwrap().read().unwrap(), &config.high_version_circuit.assets_path, ); VERIFIER_ZKVM.get_or_init(|| RwLock::new(Some(VerifierPair( config.high_version_circuit.fork_name, Rc::new(Box::new(zkvm_verifier)), )))); Ok(()) }
102-117: Remove unsafe blocks from getter function.The function correctly uses Result for error handling but still relies on unsafe blocks. Consider this safer implementation:
pub fn get_verifier(fork_name: &str) -> Result<Rc<Box<dyn ProofVerifier>>> { if let Some(verifier) = VERIFIER_HALO2 .get() .and_then(|lock| lock.read().ok()) .and_then(|guard| guard.as_ref()) { if verifier.0 == fork_name { return Ok(verifier.1.clone()); } } if let Some(verifier) = VERIFIER_ZKVM .get() .and_then(|lock| lock.read().ok()) .and_then(|guard| guard.as_ref()) { if verifier.0 == fork_name { return Ok(verifier.1.clone()); } } bail!("failed to get verifier, key not found, {}", fork_name) }
🧹 Nitpick comments (3)
common/libzkp/impl/src/verifier/darwin_v2.rs (1)
31-47: Consider improving error handling.The current implementation has a few areas where error handling could be more robust:
- The
unwrap()calls in proof deserialization could fail at runtime.- The
panic_catchwraps all panics into generic errors, losing specific error context.Consider this more robust error handling approach:
fn verify(&self, task_type: super::TaskType, proof: Vec<u8>) -> Result<bool> { - let result = panic_catch(|| match task_type { + let result = panic_catch(|| -> Result<bool> { + let verification_result = match task_type { TaskType::Chunk => { - let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()).unwrap(); - self.verifier.verify_chunk_proof(proof) + let proof = serde_json::from_slice::<ChunkProof>(proof.as_slice()) + .map_err(|e| anyhow::anyhow!("Failed to deserialize ChunkProof: {}", e))?; + self.verifier.verify_chunk_proof(proof) } TaskType::Batch => { - let proof = serde_json::from_slice::<BatchProof>(proof.as_slice()).unwrap(); - self.agg_verifier.verify_batch_proof(&proof) + let proof = serde_json::from_slice::<BatchProof>(proof.as_slice()) + .map_err(|e| anyhow::anyhow!("Failed to deserialize BatchProof: {}", e))?; + self.agg_verifier.verify_batch_proof(&proof) } TaskType::Bundle => { - let proof = serde_json::from_slice::<BundleProof>(proof.as_slice()).unwrap(); - self.agg_verifier.verify_bundle_proof(proof) + let proof = serde_json::from_slice::<BundleProof>(proof.as_slice()) + .map_err(|e| anyhow::anyhow!("Failed to deserialize BundleProof: {}", e))?; + self.agg_verifier.verify_bundle_proof(proof) } - }); - result.map_err(|e| anyhow::anyhow!(e)) + }; + Ok(verification_result) + }); + result.map_err(|e| anyhow::anyhow!("Proof verification failed: {}", e)) }common/libzkp/impl/src/verifier.rs (2)
14-36: Add documentation for public types and traits.Consider adding documentation comments (
///) for public entities to improve code maintainability:
TaskTypeenum and its variantsProofVerifiertrait and its methodsCircuitConfigandVerifierConfigstructs and their fieldsExample:
/// Represents different types of proof verification tasks #[derive(Debug, Clone, Copy, PartialEq)] pub enum TaskType { /// Chunk-level proof verification Chunk, // ... etc }
42-44: Good naming change, but consider thread-safe alternatives.The renaming from HIGH/LOW to ZKVM/HALO2 better reflects the purpose of these verifiers. However, consider using thread-safe alternatives:
use once_cell::sync::Lazy; static VERIFIER_ZKVM: Lazy<RwLock<Option<VerifierPair>>> = Lazy::new(|| RwLock::new(None)); static VERIFIER_HALO2: Lazy<RwLock<Option<VerifierPair>>> = Lazy::new(|| RwLock::new(None)); static PARAMS_MAP: Lazy<RwLock<BTreeMap<u32, ParamsKZG<Bn256>>>> = Lazy::new(|| RwLock::new(BTreeMap::new()));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
common/libzkp/impl/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
common/libzkp/impl/Cargo.toml(1 hunks)common/libzkp/impl/rust-toolchain(1 hunks)common/libzkp/impl/src/verifier.rs(4 hunks)common/libzkp/impl/src/verifier/darwin.rs(0 hunks)common/libzkp/impl/src/verifier/darwin_v2.rs(1 hunks)common/libzkp/impl/src/verifier/euclid.rs(1 hunks)
💤 Files with no reviewable changes (1)
- common/libzkp/impl/src/verifier/darwin.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- common/libzkp/impl/rust-toolchain
- common/libzkp/impl/src/verifier/euclid.rs
- common/libzkp/impl/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: compile
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (1)
common/libzkp/impl/src/verifier/darwin_v2.rs (1)
30-30: LGTM! Lifetime parameter simplification.The change to use
'_instead of an explicit lifetime parameter simplifies the code while maintaining the same functionality. This is a good practice when the lifetime relationships are straightforward.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/internal/controller/watcher/batch_proposer_test.go(4 hunks)rollup/tests/rollup_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (9)
rollup/internal/controller/watcher/batch_proposer_test.go (4)
143-143: Added new required configuration parameter.The
MaxChunksPerBatchparameter is now required inBatchProposerConfig. This parameter has been set tomath.MaxInt32, which effectively means no practical limit on the number of chunks per batch for this test, ensuring it doesn't interfere with the focus of this specific test.
232-232: Added new required configuration parameter.The
MaxChunksPerBatchparameter has been consistently added here to match the updatedBatchProposerConfigstructure. Setting it tomath.MaxInt32ensures it doesn't affect the gas and calldata size estimations being tested.
322-322: Added new required configuration parameter.The
MaxChunksPerBatchparameter has been properly added to theBatchProposerConfigstructure. Usingmath.MaxInt32as the value ensures the blob size limit test can proceed without chunk count restrictions.
412-412: Added new required configuration parameter.The
MaxChunksPerBatchparameter has been correctly added to theBatchProposerConfig. This addition is consistent with other tests in this file and ensures proper configuration for the batch proposer.rollup/tests/rollup_test.go (5)
60-61: Added support for Euclid hard fork.The chainConfig now includes the
EuclidTimeparameter set touint64(3), properly enabling the Euclid hard fork for testing.
77-77: Added timestamp to block headers.Setting the
Timefield in the block header is crucial for hard fork testing, as it determines which protocol rules apply. This value will be compared with theEuclidTimeparameter to determine if Euclid rules are active.
102-102: Added MaxChunksPerBatch configuration parameter.The new required parameter
MaxChunksPerBatchhas been properly added to theBatchProposerConfigwith a value ofmath.MaxInt32, consistent with the changes in the batch proposer test file.
143-147:✅ Verification successful
Switched to Halo2BatchProof implementation.
The test now uses
message.Halo2BatchProofwith the appropriate fields (RawProof,Instances, andVk) instead of the previous proof structure.
🏁 Script executed:
#!/bin/bash # Check if Halo2BatchProof properly implements the required BatchProof interface echo "Checking Halo2BatchProof interface implementation:" rg -A 3 "func \(p \*Halo2BatchProof\)" --type go echo "Looking for BatchProof interface definition:" rg -A 3 "type BatchProof interface" --type goLength of output: 503
Halo2BatchProof implementation verified
The new test code correctly instantiates
message.Halo2BatchProofwith the expected fields (RawProof,Instances, andVk). Our verification confirms that theBatchProofinterface (which requiresSanityCheck()andProof() []byte) is properly implemented and aligns with the updated test code.
- Confirmed the interface definition in
common/types/message/message.go.- Verified that the
Halo2BatchProofstruct is in use withinrollup/tests/rollup_test.go.
158-162:✅ Verification successful
Switched to Halo2BundleProof implementation.
The test now uses
message.Halo2BundleProofwith the appropriate fields (RawProof,Instances, andVk) instead of the previous bundle proof structure.
🏁 Script executed:
#!/bin/bash # Check if Halo2BundleProof properly implements the required BundleProof interface echo "Checking Halo2BundleProof interface implementation:" rg -A 3 "func \(p \*Halo2BundleProof\)" --type go echo "Looking for BundleProof interface definition:" rg -A 3 "type BundleProof interface" --type goLength of output: 510
Verification OK: Updated Halo2BundleProof Usage Confirmed
The test in
rollup/tests/rollup_test.gonow correctly instantiatesmessage.Halo2BundleProofwith the expected fields (RawProof,Instances, andVk). We also confirmed that theBundleProofinterface—declared incommon/types/message/message.gowith methodsSanityCheck() errorandProof() []byte—is present. Please ensure thatHalo2BundleProofexplicitly implements these methods (or satisfies the interface through embedding) so that all interface contracts are met.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
zkvm-prover/print_high_zkvm_version.sh (3)
1-2: Improve Shell Options for Better Reliability
The shebang and the use ofset -ueare good practices, but consider adding thepipefailoption to catch errors in pipelines. This change can prevent silent failures if any command in the pipeline fails.- set -ue + set -euo pipefail
4-4: Modernize Command Substitution
The current script uses backticks for command substitution. Replacing them with the$()syntax will improve readability and facilitate nesting if needed.- higher_zkvm_item=`grep "zkvm-prover" ./Cargo.lock | sort | uniq | awk -F "[#=]" '{print $3" "$4}' | sort -k 1 | tail -n 1` + higher_zkvm_item=$(grep "zkvm-prover" ./Cargo.lock | sort | uniq | awk -F "[#=]" '{print $3" "$4}' | sort -k 1 | tail -n 1)
6-8: Enhance Robustness with Quoting and Modern Substitution
For the extraction of version and commit hash, use the modern$()syntax and enclose variable expansions in double quotes to avoid potential word-splitting issues.- higher_version=`echo $higher_zkvm_item | awk '{print $1}'` + higher_version=$(echo "$higher_zkvm_item" | awk '{print $1}') - higher_commit=`echo $higher_zkvm_item | cut -d ' ' -f2 | cut -c-7` + higher_commit=$(echo "$higher_zkvm_item" | cut -d ' ' -f2 | cut -c-7)zkvm-prover/print_plonky3gpu_version.sh (3)
3-5: Enhance Robustness: Verify Configuration File ExistenceConsider checking for the existence of the Cargo configuration file before running the
grepcommand. This will avoid errors if the file does not exist.config_file=~/.cargo/config.toml +if [ ! -f "$config_file" ]; then + echo "Config file $config_file not found." + exit 1 +fi plonky3_gpu_path=$(grep 'path.*plonky3-gpu' "$config_file" | cut -d'"' -f2 | head -n 1)
7-9: Ensure Proper Variable Quoting in the If ConditionTo avoid potential word-splitting issues when the variable is empty or contains spaces, quote the variable in the test condition.
-if [ -z $plonky3_gpu_path ]; then +if [ -z "$plonky3_gpu_path" ]; then
11-12: Review Commit Hash Extraction LogicThe command
git log --pretty=format:%h -n 1already returns a short commit hash. The subsequent substring extraction (${commit_hash:0:7}) may be redundant, but if you choose to enforce a fixed length, it is acceptable.zkvm-prover/Makefile (4)
3-7: Conditional Extraction of PLONKY3_VERSION
This block handles the extraction ofPLONKY3_VERSIONbased on the detected Make version. The two differentgreppatterns (using#vs.\#) indicate handling differences in shell behavior or make version output. Consider adding an inline comment to document why the distinction is necessary for future maintainers.
16-18: Extracting ZKVM_COMMIT from ZKVM_VERSION
The extraction of the commit hash fromZKVM_VERSIONusingcutis straightforward. Ensure that the value ofZKVM_VERSIONalways includes a space-separated field so that the commit extraction works as intended. Adding an inline note could help clarify this assumption.
19-19: Fetching PLONKY3_GPU_VERSION
Usingsed -n '2p'to retrieve the GPU version fromprint_plonky3gpu_version.shis clever but could be brittle if the script’s output format changes. Consider documenting the expected output format or adding error handling if the desired line is not present.
41-44: Tests Binary Target Setup
Thetests_binarytarget cleans the build, runs tests in release mode (without execution), and creates symbolic links for test binaries. Although effective, consider enhancing robustness by handling the case where no matching files are found from thelscommand or by using a more reliable file selection mechanism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
build/dockerfiles/prover.Dockerfile(1 hunks)zkvm-prover/Makefile(1 hunks)zkvm-prover/print_high_zkvm_version.sh(1 hunks)zkvm-prover/print_plonky3gpu_version.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build/dockerfiles/prover.Dockerfile
🧰 Additional context used
🪛 Shellcheck (0.10.0)
zkvm-prover/print_plonky3gpu_version.sh
[warning] 10-10: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.
(SC2164)
[warning] 14-14: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: tests
- GitHub Check: test
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (8)
zkvm-prover/print_high_zkvm_version.sh (1)
10-10: Clear Output of Version Information
The final echo command correctly outputs the extracted version and commit hash, making it easy to verify the script’s output.zkvm-prover/Makefile (7)
1-1: PHONY Targets Declaration is Clear
The declaration for PHONY targets on line 1 properly defines theprover,lint, andtests_binarytargets.
9-14: Robust Check for ZKVM_VERSION
The Makefile obtainsZKVM_VERSIONfrom an external script and immediately checks if it is set, using a well-placed error directive if not. It would be useful to include a brief comment on the expected format of the output fromprint_high_zkvm_version.sh.
21-23: Acquiring GIT_REV and GO_TAG
The retrieval ofGIT_REVviagit rev-parse --short HEADandGO_TAGvia a grep onversion.gois implemented efficiently. Be mindful that the relative path../common/version/version.goremains valid as the repository structure evolves.
24-29: GO_TAG Verification Block
This conditional block verifiesGO_TAGand halts the Make process if it isn’t set, ensuring mandatory versioning information is enforced. The logic is sound and contributes to a fail-fast build environment.
30-36: Determining ZK_VERSION Based on GPU Support
This segment correctly selects between the CPU and GPU versions by checking ifPLONKY3_GPU_VERSIONis set. The inline comments clarify each branch’s purpose, making the decision logic transparent.
38-40: Prover Build Target Configuration
Theprovertarget passes the necessary environment variables and triggers a Cargo release build. The approach is clear; if further build flags or options become necessary, consider abstracting common settings into variables.
45-48: Lint Target for Code Quality
Thelinttarget sequentially runscargo check,cargo clippy(with flags to treat all warnings as errors), and formats the code withcargo fmt. This is a comprehensive approach to maintain code quality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
zkvm-prover/Cargo.toml (3)
8-12: Patch Section Review.
The[patch.crates-io]section is clearly set up to override dependencies from crates.io, with each dependency sourced from the appropriate Git repository and branch. It is advisable to periodically verify that these branch references remain stable, especially as they may affect reproducibility and integration in production.
13-47: Comprehensive Dependencies Listing.
The[dependencies]section is thorough and correctly lists required libraries with their versions and any necessary feature flags. One recommendation is to consider pinning Git-sourced dependencies (such asethers-core,ethers-providers, andsbv-primitives) to a specific commit or tag rather than a branch. This change can help ensure consistent builds and prevent unexpected upstream changes. For example:-ethers-core = { git = "https://github.com/scroll-tech/ethers-rs.git", branch = "v2.0.7" } +ethers-core = { git = "https://github.com/scroll-tech/ethers-rs.git", rev = "replace_with_specific_commit_hash" }Applying a similar change for other Git dependencies can be beneficial.
47-48: Nitpick: Cleanup of Trailing Line.
The appearance of the trailing line marked as "48" seems extraneous. Consider removing this stray line to maintain clean formatting in the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
common/libzkp/impl/Cargo.lockis excluded by!**/*.lockzkvm-prover/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
zkvm-prover/Cargo.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: tests
- GitHub Check: test
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (1)
zkvm-prover/Cargo.toml (1)
1-4: Package metadata is correctly defined.
The package name, version, and edition are all specified in accordance with Rust conventions. Please ensure that the version here aligns with any related version updates (e.g., incommon/version.goor through thebump-versionprocess) as prescribed in the PR objectives.
This reverts commit a868bc1.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/version/prover_version.go (1)
60-67: Consider more descriptive function naming and documentation.The implementation correctly extracts the semantic version prefix, but consider renaming this to something more descriptive like
extractSemverPrefixand adding documentation that explains how it handles pre-release identifiers and build metadata.-func extractVersionPrefix(version string) string { +// extractSemverPrefix extracts the semantic version component (vX.Y.Z) from a version string, +// ignoring any pre-release identifiers (-alpha, -beta) or build metadata (+build123). +func extractSemverPrefix(version string) string {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/version/prover_version.go(1 hunks)common/version/prover_version_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (3)
common/version/prover_version_test.go (1)
52-52: Good addition of test case for complex version format.This test case verifies that the version checking logic correctly handles version strings with non-standard formats like double hyphens and commit hashes, ensuring compatibility with your version extraction logic.
common/version/prover_version.go (2)
42-46: Good improvement to version comparison logic.Extracting the version prefix before comparison makes the version check more robust when dealing with complex version strings that include commit hashes and other metadata.
51-54: Consistent application of prefix extraction.Good update to error logging, making sure to use the extracted prefix consistently throughout the function.
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
Improvements