Update Galileo Dependency #1752
Conversation
WalkthroughAdds CodecV9 support across coordinator and rollup codepaths, updates da-codec and go-ethereum dependency versions and replace directives in multiple go.mod files, and rotates verifier fork names in mock config, config.json, and test fixture. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant CLI as Integration Tool
participant Relayer as rollup/relayer
participant Blob as blob_uploader
participant Utils as rollup/utils
participant Coordinator as coordinator/provertask
participant DACodec as da-codec
Dev->>CLI: select codec = 9
CLI->>Relayer: submit batch(es) marked CodecV9
Relayer->>Utils: derive validium/version for CodecV9
Relayer->>Blob: construct blob payload (CodecV9)
Blob->>DACodec: encode batch (V9 treated like V7/V8)
Relayer->>Coordinator: request batch details decode (CodecV9)
Coordinator->>DACodec: decode batch (supports V9)
note right of DACodec `#D0F0C0`: da-codec v0.9.0 provides V9 support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rollup/internal/controller/relayer/l2_relayer.go (1)
489-517: Fix log field in ProcessPendingBatches default case; CodecV9 integration verifiedThe CodecV9 plumbing through commit/finalize paths and validium mapping is correct: CodecV9 is consistently grouped with CodecV7/CodecV8 in both
ProcessPendingBatchesandfinalizeBundle, and the validium flow correctly maps both CodecV8 and CodecV9 toversion = 1.However, there is one confirmed minor issue: line 515 has a logging bug that should be fixed:
default: - log.Error("unsupported codec version in ProcessPendingBatches", "codecVersion", codecVersion, "start index", firstBatch, "end index", lastBatch.Index) + log.Error("unsupported codec version in ProcessPendingBatches", "codecVersion", codecVersion, "start index", firstBatch.Index, "end index", lastBatch.Index) return }Note: The original review's "Also applies to: 748-764, 1052-1059" is inaccurate. Line 763 is a different default case in
finalizeBundle(returns a formatted error, not a log), and line 1053 is the validium version mapping (not affected by this bug).L1 contract acceptance of CodecV9 cannot be verified from this codebase alone and would require reviewing the L1 contracts or external documentation.
🧹 Nitpick comments (1)
rollup/internal/controller/blob_uploader/blob_uploader.go (1)
170-178: CodecV9 blob construction aligned with V7/V8Adding
encoding.CodecV9to this branch reuses the V7/V8 batch layout (parent hash, queue hashes, chunks, and aggregatedBlocks), which is consistent with how the relayer constructs blobs for V7+ batches. Only nit: theallBlockscomment above still mentions “CodecV7” specifically and could be updated to “CodecV7+” or “CodecV7–V9” for clarity.Please double‑check that
github.com/scroll-tech/da-codec/encodingv0.9.0 expects the sameencoding.Batchshape forCodecV9as for V7/V8 (parent hash + queue hashes + blocks) end‑to‑end.
📜 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 (10)
coordinator/cmd/api/app/mock_app.go(1 hunks)coordinator/conf/config.json(1 hunks)coordinator/go.mod(1 hunks)coordinator/internal/config/config_test.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(1 hunks)rollup/go.mod(1 hunks)rollup/internal/controller/blob_uploader/blob_uploader.go(1 hunks)rollup/internal/controller/relayer/l2_relayer.go(3 hunks)rollup/internal/utils/utils.go(1 hunks)rollup/tests/integration_tool/main.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-18T06:49:24.796Z
Learnt from: Thegaram
Repo: scroll-tech/scroll PR: 1746
File: rollup/internal/controller/sender/sender.go:637-640
Timestamp: 2025-10-18T06:49:24.796Z
Learning: In the file `rollup/internal/controller/sender/sender.go`, the resubmission logic in `createReplacingTransaction` does not convert V0 blob sidecars to V1 when resubmitting transactions after the Fusaka upgrade. This is an accepted edge case because it's unlikely to occur, geth is expected to handle it gracefully, and manual recovery is available if needed.
Applied to files:
rollup/internal/controller/blob_uploader/blob_uploader.gorollup/internal/controller/relayer/l2_relayer.go
📚 Learning: 2025-07-29T16:38:24.647Z
Learnt from: colinlyguo
Repo: scroll-tech/scroll PR: 1714
File: rollup/internal/controller/relayer/l2_relayer.go:1548-1555
Timestamp: 2025-07-29T16:38:24.647Z
Learning: In rollup/internal/controller/relayer/l2_relayer.go, the validateBatchFields function should error out when GetBatchByHash fails to find a parent batch. This is intentional behavior - missing parent batches represent genuine error conditions that should halt batch submission processing. Genesis batch handling occurs separately from normal batch validation flow.
Applied to files:
rollup/internal/controller/relayer/l2_relayer.go
🧬 Code graph analysis (3)
coordinator/cmd/api/app/mock_app.go (1)
coordinator/internal/config/config.go (1)
AssetConfig(66-72)
rollup/tests/integration_tool/main.go (1)
coordinator/internal/utils/codec_validium.go (1)
CodecVersion(10-10)
rollup/internal/controller/relayer/l2_relayer.go (2)
coordinator/internal/utils/codec_validium.go (1)
CodecVersion(10-10)rollup/internal/orm/batch.go (2)
Batch(25-75)Batch(83-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (9)
coordinator/conf/config.json (1)
14-18: LGTM! Fork name updates align with verifier configuration progression.The updated fork names ("feynman" and "galileo") are consistent with the broader changes throughout the codebase, including test fixtures and mock configurations.
coordinator/internal/logic/provertask/batch_prover_task.go (1)
314-314: LGTM! CodecV9 support properly integrated.The addition of
encoding.CodecV9to the switch case is consistent with the existing pattern for codec versions V6 through V8, ensuring proper batch header decoding for the new codec version.rollup/tests/integration_tool/main.go (2)
23-23: LGTM! Default codec updated to V9.Updating the default codec to
CodecV9aligns with the broader changes in the PR to support the latest codec version.
147-148: LGTM! Explicit CodecV9 handling added.The case for codec version 9 is properly integrated alongside existing codec versions (6, 7, 8), maintaining consistency in the codec selection logic.
coordinator/go.mod (1)
12-12: Verify the major version jump of da-codec dependency.The dependency update to
v0.9.0mirrors the change inrollup/go.mod. Ensure that:
- Version
v0.9.0exists and is stable- All breaking changes are properly handled in the coordinator module
- The CodecV9 APIs are fully compatible
This verification can be done once for both modules using the script provided in the
rollup/go.modreview.coordinator/internal/config/config_test.go (1)
26-26: LGTM! Test fixture updated to match new fork name.The fork name change to "galileo" in the test fixture is consistent with the broader configuration updates across the codebase.
rollup/internal/utils/utils.go (1)
189-189: LGTM! CodecV9 properly handled for validium mode.The addition of
codecVersion == encoding.CodecV9to the condition ensures that CodecV9 follows the same validium encoding behavior as CodecV8, mapping to version 1. This is consistent with the existing comment explaining the versioning strategy.coordinator/cmd/api/app/mock_app.go (1)
94-100: Simplification is intentional and appropriate for test scope.The reduction to a single verifier in the mock configuration is clearly intentional. The
MockConfigmethod contains an explicit comment stating: "Reset prover manager config for manager test cases" (line 85), and it deliberately overrides the production config with a simplified test fixture containing only the "galileo" verifier.Analysis confirms:
- The mock is actively used during test initialization in
NewCoordinatorAppconstructor (line 58)- The only existing test (
TestRunCoordinator) performs a basic version check and doesn't validate verifier configuration- No tests in the codebase validate multi-verifier scenarios
- Using minimal mock configurations for unit test isolation is standard practice
The mock configuration appropriately reflects the current test scope, which doesn't require multi-verifier validation. The discrepancy with production (which has two verifiers) is intentional and expected for a simplified test fixture.
rollup/go.mod (1)
18-18: No critical issues found with da-codec v0.9.0 update.The version jump is verified as legitimate:
v0.9.0 exists and is tagged – Confirmed in da-codec repository (commit 7850bc63ac773e9089d1584b184d9dc5873df121)
CodecV9 APIs are integrated – The latest da-codec commit "add galileo CodecV9" (Nov 17, 2025) introduced CodecV9 support, which is already handled throughout the rollup codebase in multiple files
Breaking changes are managed – Existing code in rollup uses version-aware switch statements that handle CodecV7, CodecV8, and CodecV9 together (e.g.,
rollup/internal/controller/relayer/l2_relayer.go:491,rollup/internal/controller/blob_uploader/blob_uploader.go:170), indicating forward compatibility is designed inThe version jump from v0.1.3-pseudo to v0.9.0 is intentional and reflects the feature addition rather than arbitrary versioning.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1752 +/- ##
===========================================
+ Coverage 36.48% 36.54% +0.05%
===========================================
Files 247 247
Lines 21187 21191 +4
===========================================
+ Hits 7731 7745 +14
+ Misses 12634 12617 -17
- Partials 822 829 +7
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (6)
bridge-history-api/go.mod(1 hunks)common/go.mod(1 hunks)coordinator/go.mod(1 hunks)database/go.mod(1 hunks)rollup/go.mod(1 hunks)tests/integration-test/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- coordinator/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: test
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (4)
tests/integration-test/go.mod (1)
8-9: Verify da-codec version consistency across modules.Line 8 retains
da-codec v0.1.3-0.20250826112206-b4cce5c5d178, butrollup/go.mod(line 18) updates toda-codec v0.9.0. This inconsistency may indicate a missing update or intentional divergence. Verify whether the integration tests should also bump da-codec to v0.9.0 for consistency, especially if CodecV9 support is being added system-wide.common/go.mod (1)
18-18: Verify go-ethereum version b5c3dd1aa975 compatibility.The go-ethereum dependency was updated with a newer commit hash (b5c3dd1aa975). While the timestamp (20251117071111) is consistent across modules, ensure there are no breaking changes in this specific commit compared to the previous version (9aa8b3f38f63). Consider verifying against the go-ethereum repository release notes or commit history for this build.
bridge-history-api/go.mod (1)
21-24: Clarify go-ethereum version divergence in bridge-history-api.Lines 21–24 add a replace directive using a different go-ethereum commit (47c22325665e) than other modules (b5c3dd1aa975), citing a hotfix for a "header hash incompatibility issue" (PR #1133). While the documentation is helpful, this creates a version divergence:
- Most modules:
v1.10.14-0.20251117071111-b5c3dd1aa975- bridge-history-api:
v1.10.14-0.20251117071111-47c22325665e(via replace)Verify:
- Whether PR #1133's fix is included in the standard commit hash (b5c3dd1aa975) used by other modules
- If not, whether other modules also need this hotfix or if it's bridge-history-api-specific
- Whether this replace directive should be temporary pending an upstream merge
Consider adding a comment with a link to PR #1133 for future maintainers to track when this hotfix can be removed.
database/go.mod (1)
11-11: Clarify intentionality of go-ethereum version mismatch in database module.The version inconsistency across modules is confirmed:
- database/go.mod:
v1.10.14-0.20251017054300-b5c3dd1aa975(Oct 17)- common/go.mod, coordinator/go.mod, rollup/go.mod, tests/integration-test/go.mod:
v1.10.14-0.20251117071111-b5c3dd1aa975(Nov 17)Only database/go.mod deviates from the Nov 17 version used by all other core modules. Verify whether this version skew is intentional or should be aligned with the other modules to ensure consistency across the multi-module system.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
bridge-history-api/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcoordinator/go.sumis excluded by!**/*.sumdatabase/go.sumis excluded by!**/*.sumrollup/go.sumis excluded by!**/*.sumtests/integration-test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
bridge-history-api/go.mod(1 hunks)common/go.mod(2 hunks)coordinator/go.mod(1 hunks)database/go.mod(1 hunks)rollup/go.mod(1 hunks)tests/integration-test/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- database/go.mod
- common/go.mod
- rollup/go.mod
- coordinator/go.mod
- tests/integration-test/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: test
🔇 Additional comments (2)
bridge-history-api/go.mod (2)
21-24: Hotfix directive is well-documented.The added comments clearly explain the header hash incompatibility issue (PR reference included) and warn maintainers to verify the fix during future upgrades. This is good practice for non-standard pin directives.
13-13: da-codec v0.9.0 upgrade is compatible; add integration test for batch parsing.The upgrade from v0.1.3-0.20250826112206-b4cce5c5d178 to v0.9.0 is safe for bridge-history-api. The code uses
encoding.CodecFromVersion()for dynamic version loading, and the required functions (NewDABatchFromParams,DecodeBlob) are confirmed compatible with v0.9.0 across the rollup and coordinator modules. CodecV9 is fully supported.However,
ParseL1BatchEventLogslacks dedicated test coverage (5 lines missing per the PR report). Consider adding an integration test that exercises this function with v7+ codec versions to ensure the major dependency upgrade is validated end-to-end.
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 (1)
coordinator/internal/config/config_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: test
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
Thegaram
left a comment
Thegaram
left a comment
There was a problem hiding this comment.
Lgtm. Do you want to bump version?
hmm, good catch, we can bump on @ho pr |
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
Chores
Tests