fix(derivation): advance L1 message cursor for skipped blocks#942
fix(derivation): advance L1 message cursor for skipped blocks#942
Conversation
When a node starts from a snapshot, blocks in the current batch whose number is <= the local L2 height are skipped to avoid re-injecting L1 messages. However, the L1 message cursor (totalL1MessagePopped) was not advanced for those skipped blocks, causing subsequent blocks in the same batch to read L1 messages from the wrong offset. This could fail the length check, or worse, prepend incorrect L1 transactions to a block's SafeL2Data. Always advance the cursor by block.l1MsgNum, even when the block is skipped, so that the per-batch L1 message offsets stay aligned with the batch encoding. Also fix the error message to print the number of L1 messages received instead of the entire slice. Made-with: Cursor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
node/derivation/derivation.go (1)
566-596: Consider adding the unit test called out in the PR test plan.The PR description explicitly plans a unit/integration test for a batch where initial block(s) are
<= l2Heightwith non-zerol1MsgNumfollowed by a later block withl1MsgNum > 0. I don't see a corresponding test file in this diff. Given this is a subtle cursor-alignment bug that only manifests on snapshot-started nodes, regression coverage would be valuable so a future refactor doesn't silently reintroduce the off-by-N on the skip path.Want me to sketch a table-driven unit test for
handleL1Messagethat fakesd.syncer.ReadL1MessagesInRangeand asserts both the skip-advance and mixed skip/non-skip cases?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/derivation/derivation.go` around lines 566 - 596, Add a unit test for Derivation.handleL1Message that covers the snapshot-skip path: create a BatchInfo with initial block(s) having Number <= l2Height but l1MsgNum > 0 and a later block with Number > l2Height and l1MsgNum > 0, mock d.syncer.ReadL1MessagesInRange (or d.getL1Message) to return predictable L1 messages, call handleL1Message and assert the L1 cursor advancement (totalL1MessagePopped behavior) and that rollupData.blockContexts for the non-skipped block has the encoded L1 transactions prepended to SafeL2Data.Transactions; include an additional table-driven case where no skipping occurs to verify normal behavior. Ensure the test references handleL1Message and the mocked getL1Message/ReadL1MessagesInRange so future refactors catch the cursor-alignment regression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@node/derivation/derivation.go`:
- Around line 566-596: Add a unit test for Derivation.handleL1Message that
covers the snapshot-skip path: create a BatchInfo with initial block(s) having
Number <= l2Height but l1MsgNum > 0 and a later block with Number > l2Height and
l1MsgNum > 0, mock d.syncer.ReadL1MessagesInRange (or d.getL1Message) to return
predictable L1 messages, call handleL1Message and assert the L1 cursor
advancement (totalL1MessagePopped behavior) and that rollupData.blockContexts
for the non-skipped block has the encoded L1 transactions prepended to
SafeL2Data.Transactions; include an additional table-driven case where no
skipping occurs to verify normal behavior. Ensure the test references
handleL1Message and the mocked getL1Message/ReadL1MessagesInRange so future
refactors catch the cursor-alignment regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9db0b0da-8232-4e70-8132-bc8b5311a576
📒 Files selected for processing (1)
node/derivation/derivation.go
1 participant
Summary
handleL1Message, when a block is skipped becauseblock.Number <= l2Height(snapshot-started nodes), the L1 message cursortotalL1MessagePoppedwas not advanced. Subsequent blocks in the same batch then read L1 messages from the wrong offset, which could fail the length check or prepend incorrect L1 txs toSafeL2Data.totalL1MessagePoppedbyblock.l1MsgNum, even on the skip path, so per-batch offsets stay aligned with the batch encoding (consistent withBatchInfo.ParseBatch's accumulation ofl1MsgNum).len(l1Messages)instead of the whole slice.Why this matters
parentTotalL1MessagePoppedreflects the queue position at the end of the parent batch. Within the current batch, each block — in order — consumesl1MsgNummessages regardless of whether this node re-executes it. Skipping the cursor bump breaks that invariant and silently misaligns L1 reads for later blocks.Test plan
<= l2Heightand carry non-zerol1MsgNum, and a later block hasl1MsgNum > 0; assert correct L1 txs are prepended and no length-mismatch error is returned.Made with Cursor
Summary by CodeRabbit