feat(rollup-relayer): support Validium - #1693
Conversation
Sorry, something went wrong.
|
Caution Review failedThe pull request is closed. """ WalkthroughThe changes introduce a "validium mode" to the rollup system, affecting batch proposal, relayer logic, configuration, and ABI handling. This includes new ABI definitions for Validium, configuration flags, mode-aware logic in relayer and batch proposer components, and updated utility functions for batch encoding and metadata extraction in validium mode. Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant App
participant BatchProposer
participant Relayer
participant Utils
participant ValidiumABI
Config->>App: Provide ValidiumMode flag
App->>BatchProposer: Initialize with ValidiumMode
BatchProposer->>Utils: CalculateBatchMetrics(..., validiumMode)
Relayer->>Utils: GetBatchMetadata(..., validiumMode)
Relayer->>Relayer: If ValidiumMode, use validium ABI for calldata packing
Relayer->>ValidiumABI: Pack calldata for commitBatch/finalizeBundle (validium mode)
Possibly related PRs
Suggested labelsbump-version Suggested reviewers
Poem
📜 Recent review details Configuration used: CodeRabbit UI Reviewing files that changed from the base of the PR and between 72aa00e and 9e4e9b4. 📒 Files selected for processing (2)
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. ❤️ Share 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (.coderabbit.yaml)
Documentation and Community
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review detailsConfiguration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 3050ccb and 9297244.
📒 Files selected for processing (9)- common/version/version.go (1 hunks)
- rollup/abi/validium_abi.go (1 hunks)
- rollup/cmd/rollup_relayer/app/app.go (1 hunks)
- rollup/conf/config.json (2 hunks)
- rollup/internal/config/relayer.go (1 hunks)
- rollup/internal/controller/relayer/l2_relayer.go (9 hunks)
- rollup/internal/controller/watcher/batch_proposer.go (7 hunks)
- rollup/internal/orm/batch.go (1 hunks)
- rollup/internal/utils/utils.go (4 hunks)
Learnt from: colinlyguo PR: scroll-tech/scroll#1645 File: rollup/proposer-tool-config.json:34-40 Timestamp: 2025-04-15T08:52:44.176Z Learning: In configuration files like `rollup/proposer-tool-config.json`, placeholders such as `<mainnet read db config>` are intentionally left as-is to be replaced by users with their own configuration values when deploying the tool.
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
rollup/internal/config/l2.go (1)rollup/internal/orm/batch.go (1)rollup/internal/config/relayer.go (1)
- L2Config (10-29)
- RelayerConfig (55-82)
rollup/internal/utils/utils.go (1)rollup/internal/utils/utils.go (2)
- GetBatchMetadata (148-207)
rollup/internal/orm/batch.go (2)⏰ 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). (3)coordinator/internal/types/auth.go (1)
- Batch (22-72)
- Batch (80-82)
- Hash (134-141)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
common/version/version.go (1)8-8: LGTM: Version bump is correctly implemented.
The version increment from v4.5.27 to v4.5.28 properly reflects the addition of new Validium mode functionality.
rollup/conf/config.json (1)39-39: LGTM: Well-structured configuration addition.
The validium_mode field is appropriately placed within the relayer configuration and uses a safe default value of false for backward compatibility.
rollup/internal/config/relayer.go (1)56-57: LGTM: Proper struct field implementation.
The ValidiumMode field is correctly added with appropriate JSON tagging and a clear descriptive comment. The placement at the beginning of the struct is logical for a fundamental operational mode setting.
rollup/cmd/rollup_relayer/app/app.go (1)111-111: Confirmed: SetValidiumMode Exists on BatchProposer
The SetValidiumMode(validiumMode bool) method is defined in
rollup/internal/orm/batch.go (1)
rollup/internal/controller/watcher/batch_proposer.go:134.
The integration in rollup/cmd/rollup_relayer/app/app.go:111 is correct and can be approved.288-288: LGTM: Correct function signature alignment.
The addition of metrics.ValidiumMode parameter correctly aligns with the updated GetBatchMetadata function signature that now accepts a validiumMode boolean parameter.
rollup/internal/controller/watcher/batch_proposer.go (1)35-35: LGTM! Validium mode integration is clean and consistent.
The addition of the validiumMode field with proper initialization and the consistent propagation to all CalculateBatchMetrics calls is well implemented.
Also applies to: 67-68, 133-137, 181-181, 297-297, 322-322, 332-332
rollup/internal/controller/relayer/l2_relayer.go (1)1017-1019: Confirm EndBlockHash usage as validium commitment
We weren’t able to locate any in-repo documentation or examples that define how the commitment should be derived for validium mode. The code currently does:
lastChunk := lastBatch.Chunks[len(lastBatch.Chunks)-1] commitment := common.HexToHash(lastChunk.EndBlockHash)Please verify against the official validium protocol specification that using the last chunk’s EndBlockHash here is the correct source for the commitment.
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is 20.35928% with 133 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## develop #1693 +/- ##
===========================================
+ Coverage 39.86% 39.90% +0.03%
===========================================
Files 236 233 -3
Lines 18808 18373 -435
===========================================
- Hits 7498 7331 -167
+ Misses 10575 10333 -242
+ Partials 735 709 -26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
Co-authored-by: Péter Garamvölgyi <peter@scroll.io>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)rollup/internal/controller/relayer/l2_relayer.go (1)📜 Review details984-1027: Consider the validium commitment strategy and batch processing differences.
The function correctly implements validium-specific logic:
- Uses last block hash as commitment (acknowledged as temporary in TODO)
- Calculates metrics without processing individual L2 blocks (appropriate for validium)
- Validates batch consistency and codec versions
However, consider if the temporary commitment strategy meets security requirements for your use case.
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between bd00cb4 and f4d6e67.
📒 Files selected for processing (9)- rollup/abi/validium_abi.go (1 hunks)
- rollup/cmd/rollup_relayer/app/app.go (1 hunks)
- rollup/internal/controller/relayer/l2_relayer.go (9 hunks)
- rollup/internal/controller/watcher/batch_proposer.go (7 hunks)
- rollup/internal/controller/watcher/batch_proposer_test.go (4 hunks)
- rollup/internal/controller/watcher/bundle_proposer_test.go (1 hunks)
- rollup/internal/controller/watcher/proposer_tool.go (1 hunks)
- rollup/internal/utils/utils.go (4 hunks)
- rollup/tests/rollup_test.go (1 hunks)
- rollup/cmd/rollup_relayer/app/app.go
- rollup/internal/controller/watcher/batch_proposer.go
- rollup/internal/utils/utils.go
- rollup/abi/validium_abi.go
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
Learnt from: colinlyguo PR: scroll-tech/scroll#1693 File: rollup/abi/validium_abi.go:13-15 Timestamp: 2025-07-07T12:02:52.191Z Learning: In the Scroll codebase, ABI initialization in files like bridge_abi.go and validium_abi.go follows a consistent pattern of ignoring errors from GetAbi() in init functions to maintain code style consistency across the project.
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
rollup/internal/controller/watcher/proposer_tool.go (1)128-128: LGTM: Constructor update is consistent with validium mode support.
The addition of the false /* rollup mode */ parameter aligns with the BatchProposer constructor signature update to support both rollup and validium modes.
rollup/internal/controller/watcher/bundle_proposer_test.go (1)106-106: LGTM: Test constructor update is correct.
The test correctly specifies rollup mode with the false /* rollup mode */ parameter, which is appropriate for general batch proposer testing.
rollup/tests/rollup_test.go (1)131-131: LGTM: Test constructor update maintains existing behavior.
The explicit false /* rollup mode */ parameter ensures the test continues to use rollup mode as expected.
rollup/internal/controller/watcher/batch_proposer_test.go (1)103-103: LGTM: Consistent test constructor updates across all test functions.
All test functions correctly add the false /* rollup mode */ parameter to maintain existing rollup mode behavior in tests.
Also applies to: 181-181, 249-249, 338-338
rollup/internal/controller/relayer/l2_relayer.go (6)82-82: LGTM: ValidiumABI field addition and initialization.
The validiumABI field is properly added and initialized from the bridgeAbi package, following the same pattern as the existing l1RollupABI.
Also applies to: 176-176
279-296: LGTM: Genesis batch commitment logic branches correctly for validium mode.
The conditional logic properly handles both rollup and validium modes:
- Validium mode: Only passes batchHeader to importGenesisBatch
- Rollup mode: Passes both batchHeader and stateRoot
- Logging appropriately identifies the mode being used
Also applies to: 303-303, 328-328
484-496: LGTM: Batch processing correctly branches based on validium mode.
The ProcessPendingBatches function properly calls different payload construction methods based on the validium mode flag, with appropriate error handling for each path.
716-726: LGTM: Bundle finalization correctly branches based on validium mode.
The finalizeBundle function appropriately calls different payload construction methods for validium vs rollup modes with proper error handling.
1045-1045: LGTM: Added logging for rollup mode finalization.
The logging addition provides useful visibility into the rollup mode finalization process, mirroring the logging added for validium mode.
1061-1078: Validium finalizeBundle always requires aggProof
I compared both implementations and confirmed:
- In rollup mode (constructFinalizeBundlePayloadCodecV7), a nil aggProof falls back to finalizeBundlePostEuclidV2NoProof.
- In validium mode (constructFinalizeBundlePayloadValidium), a nil aggProof returns an error (lines 1064–1066).
- There are no test-bypass flags or special branches for validium mode.
This behavior is intentional: validium finalization cannot proceed without a proof. If you need proof-less finalization in test environments, add a nil-proof branch similar to the rollup version.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)rollup/internal/controller/relayer/l2_relayer.go (1)📜 Review details1000-1027: LGTM: Correct validium batch payload construction with follow-up needed.
The method correctly constructs the validium batch payload with proper metrics calculation and ABI packing. However, the TODO comment on lines 1013-1013 indicates the commitment calculation is temporary and may need future enhancement.
Would you like me to help track this TODO item by opening a new issue to address the commitment calculation enhancement?
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 90282fc and 72aa00e.
📒 Files selected for processing (2)- rollup/cmd/rollup_relayer/app/app.go (1 hunks)
- rollup/internal/controller/relayer/l2_relayer.go (12 hunks)
- rollup/cmd/rollup_relayer/app/app.go
Learnt from: colinlyguo PR: scroll-tech/scroll#1693 File: rollup/abi/validium_abi.go:13-15 Timestamp: 2025-07-07T12:02:52.214Z Learning: In the Scroll codebase, ABI initialization in files like bridge_abi.go and validium_abi.go follows a consistent pattern of ignoring errors from GetAbi() in init functions to maintain code style consistency across the project.
Learnt from: colinlyguo PR: scroll-tech/scroll#1530 File: rollup/internal/controller/watcher/batch_proposer.go:291-294 Timestamp: 2024-10-20T16:13:20.397Z Learning: In `batch_proposer.go`, it's acceptable to call `utils.CalculateBatchMetrics` multiple times within the loop because the batch's chunks are increasing in the loop, and each calculation reflects the updated batch state.
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
rollup/internal/controller/relayer/l2_relayer.go (9)82-82: LGTM: Clean addition of validium ABI field.
The new validiumABI field follows the same pattern as the existing l1RollupABI field and properly supports the validium mode functionality.
176-176: LGTM: Proper ABI initialization.
The initialization follows the established pattern and correctly assigns the ValidiumABI from the bridge ABI package.
244-248: LGTM: Proper batch creation updates for validium support.
The addition of the Blocks field and the inclusion of ValidiumMode in BatchMetrics correctly propagates the validium mode information through the batch creation process.
280-297: LGTM: Well-structured mode-aware genesis batch commitment.
The branching logic correctly handles the different requirements for validium and rollup modes, with appropriate error handling and logging for both cases.
557-562: LGTM: Correct batch limit enforcement for validium mode.
The method properly enforces single batch submission for validium mode while preserving existing behavior for rollup mode, addressing the architectural requirements.
341-342: LGTM: Proper batch processing logic for validium mode.
The integration of getEffectiveBatchLimits and the mode-aware payload construction correctly implements the validium requirements while maintaining existing rollup functionality.
Also applies to: 488-504
732-742: LGTM: Consistent finalize bundle payload construction.
The mode-aware branching for finalize bundle payload construction follows the established pattern and correctly handles both validium and rollup modes.
1061-1079: LGTM: Robust validium finalize bundle implementation.
The method correctly handles the nil proof case and properly constructs the validium finalize bundle payload. The previous nil pointer issue has been addressed.
304-304: LGTM: Enhanced logging for better observability.
The logging updates provide valuable debugging information and help distinguish between validium and rollup mode operations, improving system observability.
Also applies to: 329-329, 1045-1046, 1062-1062
Sorry, something went wrong.
Purpose or design rationale of this PR
Support Validium in rollup-relayer.
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 tag in common/version.go been updated or have you added bump-version label to this PR?
Breaking change label
Does this PR have the breaking-change label?
Summary by CodeRabbit
New Features
Chores