-
Notifications
You must be signed in to change notification settings - Fork 719
Fix nil batchFetcher dereference in FillInBatchGasFields #4540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
joshuacolvin0
wants to merge
15
commits into
master
Choose a base branch
from
fix-nil-batch-fetcher
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
3e288af
Fix nil pointer dereference in FillInBatchGasFields when batchFetcher…
joshuacolvin0 43c8bf4
Fix gofmt comment alignment in incomingmessage_test.go
joshuacolvin0 435ff68
Return error on nil batchFetcher for BatchPostingReport messages
joshuacolvin0 05e1c97
Move nil batchFetcher check to FillInBatchGasFieldsWithParentBlock
joshuacolvin0 78f1390
Merge branch 'master' into fix-nil-batch-fetcher
joshuacolvin0 1699ad8
Fix linter issue
joshuacolvin0 db98c3d
Improve code clarity
joshuacolvin0 dd2bfcc
Add direct unit tests for FillInBatchGasFields happy path
joshuacolvin0 f1f0477
Merge branch 'master' into fix-nil-batch-fetcher
joshuacolvin0 61b172e
Merge branch 'master' into fix-nil-batch-fetcher
joshuacolvin0 9479a30
Merge branch 'master' into fix-nil-batch-fetcher
joshuacolvin0 c2b1d6a
Add context to error and log messages in FillInBatchGasFieldsWithPare…
joshuacolvin0 57fa161
Add sentinel errors, edge-case tests, warning logs, and init.go fix
joshuacolvin0 cf50f61
Fix nil batchFetcher in callers, add replay log context, trim comments
joshuacolvin0 e734095
Fix CI failures: restore FillInBatchGasFieldsWithParentBlock nil→skip…
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| // Copyright 2026, Offchain Labs, Inc. | ||
| // For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE.md | ||
|
|
||
| package arbostypes | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "math/big" | ||
| "testing" | ||
|
|
||
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/crypto" | ||
| ) | ||
|
|
||
| func TestFillInBatchGasFieldsNilFetcher(t *testing.T) { | ||
| // Must not panic when batchFetcher is nil, even for BatchPostingReport messages. | ||
| msg := &L1IncomingMessage{ | ||
| Header: &L1IncomingMessageHeader{ | ||
| Kind: L1MessageType_BatchPostingReport, | ||
| }, | ||
| L2msg: make([]byte, 148), | ||
| } | ||
| if err := msg.FillInBatchGasFields(nil); err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if msg.BatchDataStats != nil { | ||
| t.Error("expected BatchDataStats to remain nil with nil fetcher") | ||
| } | ||
| if msg.LegacyBatchGasCost != nil { | ||
| t.Error("expected LegacyBatchGasCost to remain nil with nil fetcher") | ||
| } | ||
| } | ||
|
|
||
| // buildBatchPostingReportMsg constructs a serialized BatchPostingReport message | ||
| // whose L2msg references batchData with the given batchNum. | ||
| func buildBatchPostingReportMsg(t *testing.T, batchData []byte, batchNum uint64) []byte { | ||
| t.Helper() | ||
| dataHash := crypto.Keccak256Hash(batchData) | ||
| // L2msg: 32 (timestamp) + 20 (address) + 32 (dataHash) + 32 (batchNum) + 32 (baseFee) = 148 bytes | ||
| var l2msg []byte | ||
| l2msg = append(l2msg, common.BigToHash(big.NewInt(1000)).Bytes()...) // timestamp | ||
| l2msg = append(l2msg, common.Address{}.Bytes()...) // poster address | ||
| l2msg = append(l2msg, dataHash.Bytes()...) // data hash | ||
| l2msg = append(l2msg, common.BigToHash(big.NewInt(0).SetUint64(batchNum)).Bytes()...) // batch number | ||
| l2msg = append(l2msg, common.BigToHash(big.NewInt(1)).Bytes()...) // base fee | ||
|
|
||
| msg := &L1IncomingMessage{ | ||
| Header: &L1IncomingMessageHeader{ | ||
| Kind: L1MessageType_BatchPostingReport, | ||
| Poster: common.Address{}, | ||
| BlockNumber: 1, | ||
| Timestamp: 1000, | ||
| RequestId: &common.Hash{}, | ||
| L1BaseFee: big.NewInt(1), | ||
| }, | ||
| L2msg: l2msg, | ||
| } | ||
| serialized, err := msg.Serialize() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| return serialized | ||
| } | ||
|
|
||
| func TestParseIncomingL1MessageNilFetcherBatchPostingReport(t *testing.T) { | ||
| // ParseIncomingL1Message must not panic when batchFetcher is nil and the | ||
| // message kind is BatchPostingReport. This is the end-to-end path that | ||
| // triggered the original nil pointer dereference. | ||
| serialized := buildBatchPostingReportMsg(t, []byte("batch data"), 1) | ||
| msg, err := ParseIncomingL1Message(bytes.NewReader(serialized), nil) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if msg.Header.Kind != L1MessageType_BatchPostingReport { | ||
| t.Fatalf("expected BatchPostingReport kind, got %d", msg.Header.Kind) | ||
| } | ||
| if msg.BatchDataStats != nil { | ||
| t.Error("expected BatchDataStats to remain nil with nil fetcher") | ||
| } | ||
| if msg.LegacyBatchGasCost != nil { | ||
| t.Error("expected LegacyBatchGasCost to remain nil with nil fetcher") | ||
| } | ||
| } | ||
|
|
||
| func TestTwoStepParseAndFillGasFields(t *testing.T) { | ||
| // Exercises the inbox_tracker.go pattern: parse with nil fetcher first, | ||
| // then fill gas fields with a real fetcher in a separate call. | ||
| batchData := []byte("test batch data") | ||
| var batchNum uint64 = 7 | ||
| serialized := buildBatchPostingReportMsg(t, batchData, batchNum) | ||
|
|
||
| // Step 1: Parse with nil fetcher (should not panic or error). | ||
| msg, err := ParseIncomingL1Message(bytes.NewReader(serialized), nil) | ||
| if err != nil { | ||
| t.Fatalf("parse with nil fetcher: %v", err) | ||
| } | ||
| if msg.BatchDataStats != nil || msg.LegacyBatchGasCost != nil { | ||
| t.Fatal("gas fields should be nil after parsing with nil fetcher") | ||
| } | ||
|
|
||
| // Step 2: Fill gas fields with a real fetcher. | ||
| fetcher := func(num uint64) ([]byte, error) { | ||
| if num != batchNum { | ||
| t.Fatalf("fetcher called with unexpected batch number %d, want %d", num, batchNum) | ||
| } | ||
| return batchData, nil | ||
| } | ||
| if err := msg.FillInBatchGasFields(fetcher); err != nil { | ||
| t.Fatalf("FillInBatchGasFields: %v", err) | ||
| } | ||
| if msg.BatchDataStats == nil { | ||
| t.Fatal("expected BatchDataStats to be populated after FillInBatchGasFields") | ||
| } | ||
| if msg.LegacyBatchGasCost == nil { | ||
| t.Fatal("expected LegacyBatchGasCost to be populated after FillInBatchGasFields") | ||
| } | ||
|
bragaigor marked this conversation as resolved.
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ### Fixed | ||
| - Fix nil pointer dereference in `FillInBatchGasFields` when `batchFetcher` is nil and message kind is `BatchPostingReport`. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nil check should be in FillInBatchGasFields, so that if the fields need to be filled the function will error out and not silently create wrong messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done