feat: Euclid release on Scroll Sepolia #1122
Conversation
WalkthroughThis pull request enhances blockchain configuration settings. In Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
Thegaram
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
params/config.go (2)
652-653: Consider adding documentation for Euclid fork featuresWhile the timing parameters are properly added, it would be helpful to include documentation comments describing what the Euclid fork introduces in terms of features, changes, or improvements to the protocol.
+// EuclidTime is the timestamp when the Euclid fork goes into effect. This upgrade introduces... EuclidTime *uint64 `json:"euclidTime,omitempty"` // Euclid switch time (nil = no fork, 0 = already on euclid) +// EuclidV2Time is the timestamp when the EuclidV2 fork goes into effect. This upgrade enhances... EuclidV2Time *uint64 `json:"euclidv2Time,omitempty"` // EuclidV2 switch time (nil = no fork, 0 = already on euclidv2)
1037-1039: Consider updating CheckConfigForkOrder for time-based forksThe
CheckConfigForkOrdermethod currently only checks block-based forks, but does not validate the ordering of time-based forks like Darwin, DarwinV2, Euclid, and EuclidV2. Consider extending this method to ensure time-based forks are also properly ordered.return nil } + +// CheckConfigTimeForksOrder checks that time-based forks are properly ordered +func (c *ChainConfig) CheckConfigTimeForksOrder() error { + var lastForkTime uint64 + + for _, cur := range []struct { + name string + time *uint64 + }{ + {name: "darwinTime", time: c.DarwinTime}, + {name: "darwinV2Time", time: c.DarwinV2Time}, + {name: "euclidTime", time: c.EuclidTime}, + {name: "euclidV2Time", time: c.EuclidV2Time}, + } { + if cur.time != nil { + if lastForkTime != 0 && *cur.time < lastForkTime { + return fmt.Errorf("unsupported fork ordering: %v time (%v) cannot be less than previous fork time (%v)", + cur.name, *cur.time, lastForkTime) + } + lastForkTime = *cur.time + } + } + return nil +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
params/config.go(3 hunks)params/version.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- params/version.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (8)
params/config.go (8)
331-332: Well-structured fork timing configuration for EuclidThe new fork timing parameters are properly added to the
ScrollSepoliaChainConfig. The timestamp values indicate this upgrade is scheduled for mid-July 2025:
EuclidTime: July 18, 2025, 8:00:00 PM GMTEuclidV2Time: July 20, 2025, 8:00:00 PM GMT (2 days after Euclid)This follows the established pattern of previous forks like Darwin/DarwinV2.
337-341: New SystemContract configuration added appropriatelyThe addition of the
SystemContractconfiguration block provides important parameters for the consensus mechanism:
Period: 3 seconds between blocks (matching Clique configuration)SystemContractAddress: Contract address on L1 for system operationsSystemContractSlot: Storage slot for the signer address in the system contractThis implements what appears to be a new consensus mechanism or an extension to the existing one, providing better integration with the L1 chain.
714-717: String method correctly updated to include new L1Config fieldsThe
String()method forL1Confighas been properly updated to include the new V2 message queue fields in its string representation. This is important for debugging and logging purposes.
822-829: ChainConfig String method updated to include Euclid time parametersThe
String()method ofChainConfighas been properly updated to include the new Euclid time parameters in its output. This ensures consistent logging and debugging capabilities for the new fork times.Also applies to: 852-853
956-964: IsEuclid and IsEuclidV2 helper methods correctly implementedThese helper methods follow the established pattern for time-based fork activation checks, allowing the codebase to easily determine if the Euclid forks are active at a given timestamp.
1178-1178: Rules struct and method properly updated to include Euclid flagsThe
Rulesstruct and theRules()method have been correctly updated to include the Euclid fork activation flags. This ensures that the EVM and other components can check for Euclid activation through the Rules interface.Also applies to: 1204-1205
511-512: TestChainConfig updated to include Euclid parametersThe test configuration has been properly updated to include the new Euclid parameters, ensuring test cases can properly exercise the new fork conditions.
348-354:✅ Verification successful
L1 Message Queue V2 configuration properly defined
The addition of V2 message queue parameters indicates an upgrade to the cross-layer messaging system:
L1MessageQueueV2Addresspoints to the new message queue contractL1MessageQueueV2DeploymentBlockspecifies when the V2 contract was deployed (block 7773746)This ensures that nodes know about both messaging versions and can handle the transition appropriately.
🏁 Script executed:
#!/bin/bash # Verify if the L1MessageQueueV2 fields are properly used in the codebase # Check for references to the new L1MessageQueueV2Address echo "Checking for references to L1MessageQueueV2Address..." rg -A 3 "L1MessageQueueV2Address" --type goLength of output: 1754
L1 Message Queue V2 configuration validated and integrated correctly
The new parameters in
params/config.goare correctly defined and referenced:
L1MessageQueueV2Address: Points to the new message queue contract.L1MessageQueueV2DeploymentBlock: Specifies the deployment block (7773746) for the new contract.Additionally, the fields are properly used in the sync service (see
rollup/sync_service/sync_service.go), allowing nodes to select between V1 and V2 as intended. No further changes are required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
consensus/system_contract/system_contract.go (1)
87-109: Thread-safety improvement with better error handling strategy.The updated implementation correctly adds explicit locking with
s.lock.Lock()and provides more nuanced error handling for different scenarios. This improves both thread safety and error visibility.However, the error handling approach for empty addresses could be more consistent:
- Line 100 logs at Debug level
- Line 103 logs at Warn level
- Line 107 logs at Error level
These different severity levels for what appears to be the same condition might be confusing for troubleshooting.
Consider standardizing the log levels or adding more context to clarify why empty addresses are treated with different severity levels:
// Validate the address is not empty if bAddress == (common.Address{}) { - log.Debug("retrieved empty signer address from L1 System Contract", "contract", s.config.SystemContractAddress.Hex(), "slot", s.config.SystemContractSlot.Hex()) + log.Info("retrieved empty signer address from L1 System Contract", "contract", s.config.SystemContractAddress.Hex(), "slot", s.config.SystemContractSlot.Hex()) if s.signerAddressL1 == (common.Address{}) { - log.Warn("System Contract signer address not initialized") + log.Warn("System Contract signer address not initialized yet, will retry") return } - log.Error("retrieved empty signer address from L1 System Contract") + log.Warn("Unexpected empty address from L1 System Contract after previous successful initialization") return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
consensus/system_contract/system_contract.go(2 hunks)params/version.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- params/version.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
consensus/system_contract/system_contract.go (3)
49-51: Improved error handling approach.The code now properly attempts to initialize the signer address without blocking the constructor if the fetch fails. This is a better approach for initialization that might fail in a non-critical way, allowing the system to retry later.
81-81: Error handling simplified in background process.The removal of error checking simplifies the periodic fetch operation. Since this runs in a background goroutine, logging errors directly in the fetch method is more appropriate than returning errors that would be ignored anyway.
95-97: Thread safety improvement with explicit locking.Adding explicit locking here is an important improvement that ensures thread safety when modifying the shared state (
s.signerAddressL1). This prevents potential race conditions when reading the address in other methods.
1. Purpose or design rationale of this PR
Enable Euclid upgrade on Scroll Sepolia.
2. 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:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Chores
Bug Fixes