feat(rollup-relayer): add Euclid support #1590
Conversation
|
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 : could not load export data: no export data for "scroll-tech/common/bytecode/erc20"" WalkthroughThis pull request introduces comprehensive updates across multiple Go modules in the Scroll Tech repository. The primary changes include upgrading the Go version from 1.21 to 1.22.2, updating numerous dependencies, and making specific modifications to the rollup and controller components. The updates span various files, with a focus on dependency management, codec version handling, and introducing new methods for handling specific blockchain transitions like the Euclid hardfork. Changes
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1590 +/- ##
===========================================
- Coverage 52.01% 42.70% -9.32%
===========================================
Files 157 221 +64
Lines 12476 17441 +4965
===========================================
+ Hits 6490 7449 +959
- Misses 5424 9302 +3878
- Partials 562 690 +128
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
rollup/internal/controller/watcher/l2_watcher.go (2)
133-136: Consider adding debug logging for nil RowConsumption cases.When skipping the error for Euclid blocks, it would be helpful to add debug logging to track these occurrences. This will aid in monitoring and debugging during the transition period.
if block.RowConsumption == nil && !w.chainCfg.IsEuclid(block.Time()) { w.metrics.fetchNilRowConsumptionBlockTotal.Inc() return fmt.Errorf("fetched block does not contain RowConsumption. number: %v", number) + } else if block.RowConsumption == nil { + log.Debug("Skipping nil RowConsumption check for Euclid block", "number", number, "time", block.Time()) }
133-136: Consider updating metric tracking for Euclid blocks.The current implementation only increments
fetchNilRowConsumptionBlockTotalfor non-Euclid blocks. Consider adding a separate metric to track Euclid blocks with nilRowConsumptionfor better observability.+ if block.RowConsumption == nil { + if w.chainCfg.IsEuclid(block.Time()) { + w.metrics.fetchNilRowConsumptionEuclidBlockTotal.Inc() + } else { + w.metrics.fetchNilRowConsumptionBlockTotal.Inc() + return fmt.Errorf("fetched block does not contain RowConsumption. number: %v", number) + } + } - if block.RowConsumption == nil && !w.chainCfg.IsEuclid(block.Time()) { - w.metrics.fetchNilRowConsumptionBlockTotal.Inc() - return fmt.Errorf("fetched block does not contain RowConsumption. number: %v", number) - }rollup/internal/controller/watcher/chunk_proposer.go (1)
297-299: Simplify condition and improve control flow inproposeChunkThe combined condition
proposed || err != nilcan lead to confusion and may not handle errors and successful proposals distinctly. Consider separating the error handling from the success path to enhance readability and maintainability.Refactored code:
if proposed { return nil } +if err != nil { return err }This change ensures that the function returns immediately after a successful proposal and handles errors appropriately.
rollup/internal/controller/watcher/bundle_proposer.go (1)
164-168: Add explanation for limiting batches when usingCodecV5The code limits the number of batches per bundle to 1 when
codecVersionisencoding.CodecV5. To improve clarity, include a comment explaining the reason for this constraint.Updated code with explanatory comment:
if codecVersion == encoding.CodecV5 { // CodecV5 requires each bundle to contain exactly one batch due to protocol specifications maxBatchesThisBundle = 1 batches = batches[:maxBatchesThisBundle] }This helps maintainers understand the rationale behind the special handling of
CodecV5.Makefile (1)
3-3: Consider using released versions instead of commit hashes.The change from
scroll-v5.6.3to a specific commit hash suggests reliance on unreleased features. While this might be necessary for Euclid support, consider:
- Creating a new release tag once the changes are stable
- Documenting why this specific commit is required
📜 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 (14)
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(5 hunks)rollup/internal/controller/relayer/l2_relayer.go(2 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)tests/integration-test/go.mod(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (19)
rollup/internal/controller/watcher/l2_watcher.go (1)
133-133: Verify the Euclid transition handling.The condition now allows blocks with nil
RowConsumptionif they are from the Euclid era. While this change aligns with adding Euclid support, we should verify:
- The behavior during the transition period
- Whether this change is backward compatible
- If there are any implications for block processing
Let's verify the Euclid transition handling:
✅ Verification successful
✓ Euclid transition handling is properly implemented
The implementation correctly handles the Euclid transition by:
- Maintaining backward compatibility for pre-Euclid blocks
- Using consistent timestamp-based era detection
- Having explicit transition handling in both watcher and proposer components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent Euclid transition handling across the codebase # Look for other IsEuclid checks to ensure consistent usage rg "IsEuclid\(" -A 3 # Look for RowConsumption handling to ensure consistent nil checks ast-grep --pattern 'if $x.RowConsumption == nil { $$$ }'Length of output: 1133
rollup/internal/controller/watcher/chunk_proposer.go (2)
395-422:⚠️ Potential issuePrevent nil pointer dereference when accessing
prevBlocks[0]In the condition at line 401, accessing
prevBlocks[0]without ensuring thatlen(prevBlocks) > 0can lead to a nil pointer dereference ifprevBlocksis empty.Modify the condition to safely access
prevBlocks[0]:-if err != nil || len(prevBlocks) == 0 || prevBlocks[0].Header.Hash() != blocks[0].Header.ParentHash { +if err != nil || len(prevBlocks) == 0 { + return false, fmt.Errorf("failed to get parent block: %w", err) +} +if prevBlocks[0].Header.Hash() != blocks[0].Header.ParentHash { + return false, fmt.Errorf("parent block hash does not match") +}This ensures that
prevBlocks[0]is only accessed when it is safe to do so, preventing potential runtime errors.Likely invalid or redundant comment.
395-422: Check for underflow when calculating previous block numberSubtracting 1 from
blocks[0].Header.Number.Uint64()can cause an underflow if the block number is 0 (genesis block). This would result in an incorrect block height due to wraparound in unsigned integers.Please add a check to ensure the block number is greater than 0 before subtracting:
if blocks[0].Header.Number.Uint64() == 0 { return false, fmt.Errorf("cannot retrieve parent of genesis block") } prevBlocks, err := p.l2BlockOrm.GetL2BlocksGEHeight(p.ctx, blocks[0].Header.Number.Uint64()-1, 1)This prevents underflow and ensures correct behavior when processing the genesis block.
rollup/internal/controller/watcher/l1_watcher.go (2)
9-9: Confirm correctness ofCalcBlobFeeafter import changeThe import has changed from
eip4844tomisc, and theCalcBlobFeefunction is now used from themiscpackage. Verify that this function behaves as expected.Ensure that
misc.CalcBlobFeeprovides the same calculation as the previous implementation. If there are differences, update the code accordingly or adjust any dependent calculations.
83-83: Validate parameters passed toCalcBlobFeeConfirm that the parameter
*excessis appropriate formisc.CalcBlobFeeand that it returns the correctblobBaseFee.Review the function signature and implementation of
misc.CalcBlobFeeto ensure compatibility:
- Check that
*excessis of the expected type.- Verify that the return value is correctly converted using
Uint64().rollup/internal/controller/relayer/l2_relayer.go (2)
208-211: LGTM! Genesis block persistence added.The addition of L2 block insertion for genesis ensures proper chain history tracking.
433-433: LGTM! Codec version support expanded for Euclid.The addition of
CodecV5andCodecV6support maintains backward compatibility while enabling Euclid functionality.Let's verify the codec version handling across the codebase:
✅ Verification successful
Codec version handling is consistent across the codebase
The addition of
CodecV5andCodecV6in the switch case is properly integrated with existing codec handling patterns. The codebase shows consistent implementation across relayer, proposer, and test components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent codec version handling # Look for other switch statements handling codec versions ast-grep --pattern 'switch $codecVersion { $$$ case $v4, $v5, $v6: $$$ }'Length of output: 83
Script:
#!/bin/bash # Find codec version declarations and usages rg "CodecV[456]" -A 2 -B 2 # Find all codec-related files fd "codec|encoding" # Look for switch statements with codec versions using a more flexible pattern ast-grep --pattern 'switch $_ { case $$$: $$$ }'Length of output: 31492
rollup/internal/controller/sender/sender.go (2)
15-15: LGTM! Import path updated following upstream changes.The import path has been updated to use the consolidated
miscpackage instead ofeip4844.
678-678: LGTM! Blob fee calculation updated.The blob fee calculation now uses
misc.CalcBlobFeefollowing the upstream package reorganization.rollup/internal/controller/watcher/batch_proposer.go (1)
247-249: LGTM! Enhanced error handling for codec retrieval.The error message now includes block number and timestamp context, and the nil check adds robustness.
database/go.mod (2)
11-11: Verify go-ethereum dependency compatibility.The update to go-ethereum
v1.10.14-0.20250103082839-ea3ec93d8c1eis required for Euclid support.Check for consistent dependency versions:
✅ Verification successful
✓ go-ethereum dependency is consistent across all modules
All modules in the codebase use the same version:
v1.10.14-0.20250103082839-ea3ec93d8c1e🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check go-ethereum version consistency find . -name "go.mod" -exec grep -H "github.com/scroll-tech/go-ethereum" {} \;Length of output: 660
3-5: Verify Go version compatibility across the project.The upgrade to Go 1.22.2 is significant. Ensure all developers and CI systems are prepared for this change.
Run this check to verify Go version consistency:
✅ Verification successful
Go version consistency verified across all modules ✅
All 6 modules in the project are consistently configured with Go 1.22 and toolchain go1.22.2, indicating proper project-wide alignment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Go version consistency across all modules find . -name "go.mod" -exec grep -H "^go 1\." {} \;Length of output: 233
Script:
#!/bin/bash # Check toolchain directives across all modules find . -name "go.mod" -exec grep -H "^toolchain" {} \;Length of output: 302
coordinator/go.mod (1)
79-81: Review security implications of dependency updates.Several security-related packages have been updated:
golang.org/x/cryptoto v0.32.0golang.org/x/systo v0.29.0Please verify there are no breaking changes in these security packages that could affect the system's security model.
Run this script to check for security advisories:
✅ Verification successful
Security implications verified - update is safe
The update to
golang.org/x/crypto v0.32.0is safe and recommended:
- Includes critical security fixes from v0.31.0 (authorization bypass patch)
- No breaking changes between v0.31.0 and v0.32.0
- Only contains test documentation updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories in updated packages # Check for security advisories in golang.org/x/crypto gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "golang.org/x/crypto") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1705
Script:
#!/bin/bash # Check for breaking changes in recent releases curl -s https://raw.githubusercontent.com/golang/crypto/master/CHANGELOG.md 2>/dev/null || echo "Changelog not found" # Alternative: check commit messages between versions git clone --quiet https://github.com/golang/crypto /tmp/crypto 2>/dev/null cd /tmp/crypto && git log --oneline v0.31.0..v0.32.0 2>/dev/null || echo "Version tags not found"Length of output: 156
Script:
#!/bin/bash # Try different tag format and look for breaking changes cd /tmp/crypto git tag | grep "v0.3" git log --oneline --no-merges v0.31.0..v0.32.0 2>/dev/null # Check for API changes in ssh package git diff v0.31.0..v0.32.0 -- ssh/ 2>/dev/null # Backup: search main repo for any breaking changes cd - rg -l "BREAKING CHANGE" || echo "No breaking changes found"Length of output: 931
bridge-history-api/go.mod (1)
118-118:⚠️ Potential issueVerify the impact of replacing lumberjack with npipe.
The change from
gopkg.in/natefinch/lumberjack.v2togopkg.in/natefinch/npipe.v2represents a significant shift:
- lumberjack is a log rotation package
- npipe is for Windows named pipes
This change could affect logging capabilities. Please confirm if this was intentional and how logging will be handled going forward.
rollup/go.mod (1)
9-10: Review cryptography package updates for Euclid compatibility.Critical cryptography packages have been updated:
consensys/gnark-cryptoto v0.14.0crate-crypto/go-kzg-4844to v1.1.0These updates appear to be related to Euclid support. Please ensure:
- The versions are compatible with Euclid's cryptographic requirements
- Any necessary migration steps are documented
Run this script to check for breaking changes in the crypto packages:
✅ Verification successful
Crypto package updates only affect test code
The updated crypto packages (
gnark-cryptoandgo-kzg-4844) are only imported in test files:
rollup/internal/controller/sender/sender_test.goSince these dependencies are not used in production code, the version updates pose minimal risk and don't require extensive migration steps.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes in crypto packages # Search for API changes in gnark-crypto rg -A 5 "type\s+|func\s+|interface\s+" | grep -B 5 "BREAKING" # Search for migration guides fd -t f "MIGRATION|UPGRADING|CHANGELOG" -X cat {}Length of output: 115
Script:
#!/bin/bash # Find imports and usages of the crypto packages echo "=== Searching for gnark-crypto imports ===" rg -l "github.com/consensys/gnark-crypto" --type go echo -e "\n=== Searching for go-kzg-4844 imports ===" rg -l "github.com/crate-crypto/go-kzg-4844" --type go echo -e "\n=== Examining usage patterns ===" rg "gnark-crypto|go-kzg-4844" --type go -A 3 -B 3 # Look for any go.mod history to see previous versions echo -e "\n=== Checking go.mod history ===" git log -p rollup/go.mod | grep -A 5 -B 5 "gnark-crypto\|go-kzg-4844"Length of output: 1282
common/go.mod (4)
58-59: Verify compatibility of updated crypto packages.The updates to crypto-related packages are significant:
consensys/bavardv0.1.13 → v0.1.25consensys/gnark-cryptov0.12.1 → v0.14.0crate-crypto/go-kzg-4844→ v1.1.0These updates are crucial for Euclid support. Please ensure they've been thoroughly tested with your use cases.
Also applies to: 67-67
238-238: Review security implications of crypto package update.The update to
golang.org/x/crypto v0.32.0might include important security fixes. Ensure all security-critical code paths using this package are reviewed and tested.✅ Verification successful
Crypto package update appears safe to proceed
The update is safe as the codebase uses the crypto package primarily through go-ethereum's abstractions for standard Ethereum operations (key handling, transaction signing, hashing). No direct usage of low-level cryptographic primitives that might be affected by security fixes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all uses of crypto package to review rg --type go "crypto" -l | while read -r file; do echo "=== $file ===" rg "crypto" "$file" -A 2 -B 2 doneLength of output: 14526
3-5: Verify Go 1.22.2 compatibility across the codebase.The upgrade to Go 1.22.2 is significant. While it brings performance improvements and new features, it's important to ensure compatibility with all dependencies and existing code.
✅ Verification successful
Go 1.22.2 upgrade appears safe
The codebase shows no usage of deprecated features or patterns affected by Go 1.22 breaking changes. All modules are consistently set to Go 1.22, and no experimental features are in use. The upgrade should proceed smoothly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential compatibility issues with Go 1.22 # Look for deprecated features or syntax that might break # Search for common patterns that changed in Go 1.22 rg -l "for\s+range\s+.*\s*{\s*}\s*$" # Empty for-range loops changed behavior rg "(?:^|\s)go\s+func\(\)\s*{\s*select\s*{\s*}\s*}\(\)" # Empty select behavior changedLength of output: 385
Script:
#!/bin/bash # Check for empty for-range loops rg "for\s+range.*\{\s*\}" # Check for empty select statements in goroutines rg "go func.*select\s*\{" # Check version constraints in go.mod files fd -e mod -x cat {} \; | rg "^require|go\s+1\." # Look for range-related patterns that might be affected rg "for\s+range" -A 2Length of output: 310
Script:
#!/bin/bash # Check for usage of new Go 1.22 features or preview features rg "//\s*go:build" -A 1 rg "range over integer|range over func|clear\(" # New Go 1.22 featuresLength of output: 502
18-18: Review the future-dated go-ethereum dependency.The go-ethereum dependency version
v1.10.14-0.20250103082839-ea3ec93d8c1eappears to be future-dated (2025). This could indicate a temporary development version. Consider using a stable, current version or document why this specific version is required.
| go 1.22 | ||
|
|
||
| toolchain go1.22.2 |
There was a problem hiding this comment.
💡 Codebase verification
Synchronize da-codec versions across modules
The Go version upgrade and go-ethereum dependency are properly coordinated, but there's a version mismatch in da-codec:
- rollup: v0.1.3-0.20250110130755-bc9cd3c73290
- other modules: v0.1.3-0.20241218102542-9852fa4e1be5
Please ensure all modules use the same version of da-codec to prevent potential compatibility issues.
🔗 Analysis chain
Verify compatibility with the updated dependencies.
The Go version upgrade and dependency updates appear to be coordinated across modules. However, please ensure:
- The pre-release version of
da-codec(v0.1.3-0.20241218102542-9852fa4e1be5) is stable enough for production use. - The specific commit of
go-ethereumcontains the necessary Euclid support changes.
Run this script to check the changes in the specified go-ethereum commit:
Also applies to: 8-9
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Euclid-related changes in the go-ethereum dependency
# Check commit message and changes
gh api repos/scroll-tech/go-ethereum/commits/ea3ec93d8c1e379bfd7957d4866f204d8d71237c \
--jq '.commit.message, .files[].filename'
Length of output: 173
Script:
#!/bin/bash
# Find all go.mod files and their content
fd -t f "go.mod$" --exec sh -c 'echo "=== {} ==="; cat {}'
# Search for da-codec usage
rg --type go "scroll-tech/da-codec" -B 1 -A 1
Length of output: 41640
| go 1.21 | ||
| go 1.22 | ||
|
|
||
| toolchain go1.22.2 |
There was a problem hiding this comment.
this can possibly be removed.
| @@ -161,6 +161,11 @@ func (p *BundleProposer) proposeBundle() error { | |||
| return fmt.Errorf("unsupported codec version: %v, expected at least %v", codecVersion, p.minCodecVersion) | |||
| } | |||
|
|
|||
| if codecVersion == encoding.CodecV5 { | |||
There was a problem hiding this comment.
Please add a comment here why this special transition is necessary
| @@ -294,6 +294,10 @@ func (p *ChunkProposer) proposeChunk() error { | |||
| return p.updateDBChunkInfo(&chunk, codecVersion, metrics) | |||
| } | |||
|
|
|||
| if proposed, err := p.tryProposeEuclidTransitionChunk(blocks); proposed || err != nil { | |||
| return err | |||
There was a problem hiding this comment.
wrap the error for better understanding where it comes from
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
Dependency Updates
github.com/scroll-tech/go-ethereumto a new commit hashCode Changes
Maintenance