fix(rollup-relayer): sanity checks #1720
Conversation
|
Warning Rate limit exceeded@colinlyguo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRefactors commitBatches calldata parsing to an ABI-driven top-level parser, introduces exported CalldataInfo, threads a map of L1 messages-by-block through validation and DABatch/block assembly, adds a database-consistency validation, adds an end-to-end DABatch assembly test and testdata, and bumps version v4.5.39 → v4.5.40. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ABI
participant Relayer
participant DB
participant Codec
Caller->>ABI: parseCommitBatchesCalldata(abi, calldata)
ABI-->>Relayer: CalldataInfo{Version, ParentBatchHash, LastBatchHash}
Relayer->>DB: getBatchesFromCalldata(info) -> fetch L2 blocks
DB-->>Relayer: blocks + L1 messages per block
Relayer-->>Relayer: l1MessagesWithBlockNumbers map
Relayer->>Relayer: validateDatabaseConsistency(batchesToValidate)
loop per blob
Relayer->>Codec: decode blob -> DABlobPayload
Relayer->>Relayer: validateSingleBlobAgainstBatch(..., l1MessagesWithBlockNumbers)
Relayer->>Relayer: assembleDABatchFromPayload(..., l1MessagesWithBlockNumbers)
Relayer-->>Caller: DABatch (blocks include L1 messages)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
colinlyguo
force-pushed
the
fix-rollup-relayer-sanity-checks
branch
from
76cd3b3 to
819f4ce
Compare
![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)
rollup/internal/controller/relayer/l2_relayer_sanity_test.go (2)
102-102: Use idiomatic Go increment operator.- index += 1 + index++
46-82: Consider extracting hardcoded test data to fixtures.The test contains significant hardcoded data (addresses, transaction data). Consider moving this to a test fixture file for better maintainability and reusability.
Would you like me to help refactor this test to use external test fixtures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
common/version/version.go(1 hunks)rollup/internal/controller/relayer/l2_relayer_sanity.go(11 hunks)rollup/internal/controller/relayer/l2_relayer_sanity_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-29T16:38:24.647Z
Learnt from: colinlyguo
PR: scroll-tech/scroll#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_sanity_test.gorollup/internal/controller/relayer/l2_relayer_sanity.go
📚 Learning: 2025-07-07T12:02:52.214Z
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.
Applied to files:
rollup/internal/controller/relayer/l2_relayer_sanity.go
🧬 Code Graph Analysis (2)
rollup/internal/controller/relayer/l2_relayer_sanity_test.go (3)
rollup/abi/bridge_abi.go (1)
ScrollChainABI(10-10)common/version/version.go (1)
Version(31-31)coordinator/internal/types/auth.go (1)
Hash(134-141)
rollup/internal/controller/relayer/l2_relayer_sanity.go (2)
rollup/internal/controller/relayer/l2_relayer.go (1)
Layer2Relayer(66-97)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). (3)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (4)
common/version/version.go (1)
8-8: Version bump looks good, but update the PR checklist.The version has been correctly incremented from v4.5.39 to v4.5.40. However, the PR checklist indicates the "tag in common/version.go has been updated" checkbox is unchecked - please check this box since you've updated the version.
rollup/internal/controller/relayer/l2_relayer_sanity.go (3)
49-82: Good refactor to top-level function.Moving
parseCommitBatchesCalldatato a top-level function improves reusability (as demonstrated in the test file) and follows good separation of concerns.
146-420: Excellent comprehensive validation logic.The new database consistency validation functions provide thorough checks with clear error messages that will greatly help with debugging. The validation covers:
- Batch and chunk continuity
- Codec version consistency
- L1 message queue hash consistency
- Essential field validation
This significantly improves the robustness of the sanity checks.
464-467: Critical: L1 messages assignment will be overwritten.The L1 messages are assigned to
blocks[i].Transactionson line 465, but this assignment is immediately overwritten by the append operation on line 467. The L1 messages should be included in the append operation.- if l1Messages, ok := l1MessagesWithBlockNumbers[daBlocks[i].Number()]; ok { - blocks[i].Transactions = l1Messages - } - blocks[i].Transactions = append(blocks[i].Transactions, encoding.TxsToTxsData(txns[i])...) + if l1Messages, ok := l1MessagesWithBlockNumbers[daBlocks[i].Number()]; ok { + blocks[i].Transactions = append(l1Messages, encoding.TxsToTxsData(txns[i])...) + } else { + blocks[i].Transactions = encoding.TxsToTxsData(txns[i]) + }⛔ Skipped due to learnings
Learnt from: colinlyguo PR: scroll-tech/scroll#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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1720 +/- ##
===========================================
- Coverage 37.51% 37.48% -0.03%
===========================================
Files 243 243
Lines 20466 20499 +33
===========================================
+ Hits 7678 7685 +7
- Misses 11981 12005 +24
- Partials 807 809 +2
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:
|
colinlyguo
force-pushed
the
fix-rollup-relayer-sanity-checks
branch
from
dca5306 to
d6e03e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
rollup/internal/controller/relayer/l2_relayer_sanity.go (2)
49-82: Harden calldata parsing: method existence, selector check, calldata length, and avoid package shadowing
- Avoid shadowing the imported
abipackage with a parameter namedabi.- Validate
rollupABI != nil.- Ensure the calldata is at least 4 bytes and the 4-byte selector matches
commitBatches.- Check that the ABI actually contains
commitBatchesbefore unpacking.These guardrails prevent panics and mis-decoding.
Apply this diff:
-// parseCommitBatchesCalldata parses the commitBatches calldata and extracts key information -func parseCommitBatchesCalldata(abi *abi.ABI, calldata []byte) (*CalldataInfo, error) { - method := abi.Methods["commitBatches"] - decoded, err := method.Inputs.Unpack(calldata[4:]) +// parseCommitBatchesCalldata parses the commitBatches calldata and extracts key information +func parseCommitBatchesCalldata(rollupABI *abi.ABI, calldata []byte) (*CalldataInfo, error) { + if rollupABI == nil { + return nil, fmt.Errorf("nil ABI: cannot parse commitBatches calldata") + } + method, ok := rollupABI.Methods["commitBatches"] + if !ok { + return nil, fmt.Errorf("ABI missing method commitBatches") + } + if len(calldata) < 4 { + return nil, fmt.Errorf("calldata too short: need at least 4 bytes for selector, got %d", len(calldata)) + } + if !bytes.Equal(calldata[:4], method.ID) { + return nil, fmt.Errorf("function selector mismatch: got 0x%x, want 0x%x (commitBatches)", calldata[:4], method.ID) + } + decoded, err := method.Inputs.Unpack(calldata[4:]) if err != nil { return nil, fmt.Errorf("failed to unpack commitBatches calldata: %w", err) } if len(decoded) != 3 { return nil, fmt.Errorf("unexpected number of decoded parameters: got %d, want 3", len(decoded)) } version, ok := decoded[0].(uint8) if !ok { return nil, fmt.Errorf("failed to type assert version to uint8") } - parentBatchHashB, ok := decoded[1].([32]uint8) + parentBatchHashB, ok := decoded[1].([32]uint8) if !ok { return nil, fmt.Errorf("failed to type assert parentBatchHash to [32]uint8") } parentBatchHash := common.BytesToHash(parentBatchHashB[:]) - lastBatchHashB, ok := decoded[2].([32]uint8) + lastBatchHashB, ok := decoded[2].([32]uint8) if !ok { return nil, fmt.Errorf("failed to type assert lastBatchHash to [32]uint8") } lastBatchHash := common.BytesToHash(lastBatchHashB[:]) return &CalldataInfo{ Version: version, ParentBatchHash: parentBatchHash, LastBatchHash: lastBatchHash, }, nil }Additionally, add the missing import:
// at top of file import ( "bytes" // existing imports... )
160-167: Avoid potential panic when accessing the first chunk
batchesToValidate[0].Chunks[0]is accessed before verifying the batch actually has chunks. Add a guard forlen(batchesToValidate[0].Chunks) == 0before indexing.Apply this diff:
// Get previous chunk for continuity check - firstChunk := batchesToValidate[0].Chunks[0] + if len(batchesToValidate[0].Chunks) == 0 { + return fmt.Errorf("first batch %d has no chunks", batchesToValidate[0].Batch.Index) + } + firstChunk := batchesToValidate[0].Chunks[0] if firstChunk.Index == 0 { return fmt.Errorf("genesis chunk should not be in normal batch submission flow, chunk index: %d", firstChunk.Index) } prevChunk, err := r.chunkOrm.GetChunkByIndex(r.ctx, firstChunk.Index-1)
♻️ Duplicate comments (1)
rollup/internal/controller/relayer/l2_relayer_sanity.go (1)
126-149: Validate L1 message counts per chunk and include block hash in invariant errorTwo improvements:
- Cross-check that the number of collected L1 messages across the chunk’s block range equals
chunk.TotalL1MessagesPoppedInChunk. This catches DB/code drift early.- Include the block hash in the invariant violation for easier debugging (if available on the returned block type).
Also see past review about early break after first L2 tx; if you still want that micro-optimization, it can be applied without changing behavior.
Apply this diff to add the count check:
for _, chunk := range dbChunks { if chunk.TotalL1MessagesPoppedInChunk > 0 { blockWithL1Messages, err := r.l2BlockOrm.GetL2BlocksInRange(r.ctx, chunk.StartBlockNumber, chunk.EndBlockNumber) if err != nil { return nil, nil, fmt.Errorf("failed to get L2 blocks for chunk %d: %w", chunk.Index, err) } + var collected uint64 for _, block := range blockWithL1Messages { bn := block.Header.Number.Uint64() seenL2 := false for _, tx := range block.Transactions { if tx.Type == types.L1MessageTxType { if seenL2 { - // Invariant violated: found an L1 after an L2 in the same block. - return nil, nil, fmt.Errorf("L1 message after L2 tx in block %d", bn) + // Invariant violated: found an L1 after an L2 in the same block. + return nil, nil, fmt.Errorf("L1 message after L2 tx in block %d (hash=%s)", bn, block.Header.Hash().Hex()) } l1MessagesWithBlockNumbers[bn] = append(l1MessagesWithBlockNumbers[bn], tx) + collected++ } else { seenL2 = true } } } + if collected != chunk.TotalL1MessagesPoppedInChunk { + return nil, nil, fmt.Errorf("chunk %d L1 message count mismatch: collected=%d, expected=%d (block_range=%d-%d)", + chunk.Index, collected, chunk.TotalL1MessagesPoppedInChunk, chunk.StartBlockNumber, chunk.EndBlockNumber) + } } }Optional micro-optimization (keeps the invariant check but reduces inner work): stop scanning the remainder of the block once the first L2 tx is seen since any later L1 would already be caught by the existing check.
- for _, tx := range block.Transactions { + for _, tx := range block.Transactions { if tx.Type == types.L1MessageTxType { if seenL2 { // Invariant violated: found an L1 after an L2 in the same block. return nil, nil, fmt.Errorf("L1 message after L2 tx in block %d (hash=%s)", bn, block.Header.Hash().Hex()) } l1MessagesWithBlockNumbers[bn] = append(l1MessagesWithBlockNumbers[bn], tx) collected++ } else { seenL2 = true + // We can break here; any subsequent L1 would be caught at the top of a new iteration + // if the invariant is ever broken in another context. + break } }
🧹 Nitpick comments (3)
rollup/internal/controller/relayer/l2_relayer_sanity.go (3)
349-356: Strict equality checks are good; consider logging actual vs. expected batch indices as contextThe mismatch errors are clear. If these errors surface in production, having both indexes and hashes is already helpful. No action required.
456-476: Transaction assembly: ensure no duplication and order correctness
- Prepending per-block L1 messages and appending on-chain transactions from the payload preserves the intended order.
- Consider optionally asserting that
encoding.TxsToTxsData(txns[i])contains no L1 messages (if the codec guarantees this invariant). A quick defensive assertion would tighten checks.If helpful, I can add a guard like:
for _, t := range encoding.TxsToTxsData(txns[i]) { if t.Type == types.L1MessageTxType { return nil, fmt.Errorf("unexpected L1 message inside payload txs for block %d", daBlocks[i].Number()) } }Would you like me to wire a unit test to cover this case and improve Codecov on this file?
328-371: Good cross-checks across calldata/db/blobs; consider adding tests for error pathsThe validations on codec version, parent/last batch hashes, and blob count are solid. Given Codecov feedback, adding unit tests for:
- mismatched selector/version,
- parent/last batch mismatch,
- blob count mismatch,
- and l1Messages invariant violation
would meaningfully improve patch coverage.I can draft focused tests in l2_relayer_sanity_test.go to exercise these error paths. Want me to push those?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/internal/controller/relayer/l2_relayer_sanity.go(11 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-07T12:02:52.214Z
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.
Applied to files:
rollup/internal/controller/relayer/l2_relayer_sanity.go
📚 Learning: 2025-07-29T16:38:24.647Z
Learnt from: colinlyguo
PR: scroll-tech/scroll#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_sanity.go
🧬 Code Graph Analysis (1)
rollup/internal/controller/relayer/l2_relayer_sanity.go (2)
rollup/internal/controller/relayer/l2_relayer.go (1)
Layer2Relayer(66-97)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). (3)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (3)
rollup/internal/controller/relayer/l2_relayer_sanity.go (3)
85-96: Error messages good; confirm intentional hard-fail on missing parent batchReturning an error when parent/last batches are not found aligns with prior decisions for batch validation flow (missing parents are error conditions).
239-246: LGTM: correct behavior when parent batch missingThis explicit fetch of the parent batch for the first batch and failing on not found is consistent with the team’s intended behavior (genesis handled separately; missing parents should halt processing).
430-454: Batch assembly looks correct; hashes derived from payload ensure consistency with blobConstructing the encoding.Batch from payload-derived queue hashes and blocks, then hashing via the codec, is the right way to ensure blob fidelity against DB metadata.
Purpose or design rationale of this PR
Considering fetching L1 messages from the database, which is not included in blobs.
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
Bug Fixes
Refactor
Tests
Chores