Skip to content

Commit 57fa161

Browse files
joshuacolvin0claude
andcommitted
Add sentinel errors, edge-case tests, warning logs, and init.go fix
- Add ErrNilBatchFetcher, ErrBatchHashMismatch, ErrParseBatchPostingReport sentinel errors with %w wrapping for programmatic error checking - Remove redundant nil guard in delayed.go since FillInBatchGasFieldsWithParentBlock handles non-BatchPostingReport - Add edge-case tests: nil fetcher data, empty batch, idempotent calls, WithParentBlock nil data, and all non-BatchPostingReport message kinds - Add warning logs for swallowed FillInBatchGasFields errors in replay binary and GetMessage; fix misleading reorg log message - Use log.Error instead of log.Warn in replay binary (log level filtering) - Provide stub batchFetcher in init.go to satisfy non-nil contract Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent c2b1d6a commit 57fa161

6 files changed

Lines changed: 201 additions & 23 deletions

File tree

arbnode/delayed.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -239,11 +239,9 @@ func (b *DelayedBridge) logsToDeliveredMessages(ctx context.Context, logs []type
239239
},
240240
ParentChainBlockNumber: parsedLog.Raw.BlockNumber,
241241
}
242-
if batchFetcher != nil {
243-
err := msg.Message.FillInBatchGasFieldsWithParentBlock(batchFetcher, msg.ParentChainBlockNumber)
244-
if err != nil {
245-
return nil, err
246-
}
242+
err := msg.Message.FillInBatchGasFieldsWithParentBlock(batchFetcher, msg.ParentChainBlockNumber)
243+
if err != nil {
244+
return nil, err
247245
}
248246
messages = append(messages, msg)
249247
}

arbnode/transaction_streamer.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,7 @@ func (s *TransactionStreamer) addMessagesAndReorg(batch ethdb.Batch, msgIdxOfFir
388388
}
389389
delayedInBlock, err := s.delayedBridge.LookupMessagesInRange(s.GetContext(), msgBlockNum, msgBlockNum, batchFetcher)
390390
if err != nil {
391-
log.Error("reorg-resequence: failed to serialize old delayed message from database", "err", err)
391+
log.Error("reorg-resequence: failed to look up old delayed message from L1", "err", err)
392392
continue
393393
}
394394
messageFound := false
@@ -542,6 +542,8 @@ func (s *TransactionStreamer) GetMessage(msgIdx arbutil.MessageIndex) (*arbostyp
542542
if err != nil {
543543
return nil, err
544544
}
545+
} else if message.Message.Header.Kind == arbostypes.L1MessageType_BatchPostingReport {
546+
log.Warn("GetMessage: cannot fill in batch gas fields for BatchPostingReport without inboxReader", "msgIdx", msgIdx)
545547
}
546548
return &message, nil
547549
}

arbos/arbostypes/incomingmessage.go

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,12 @@ const (
3636

3737
const MaxL2MessageSize = 256 * 1024
3838

39+
var (
40+
ErrNilBatchFetcher = errors.New("batch fetcher is nil")
41+
ErrBatchHashMismatch = errors.New("batch data hash mismatch")
42+
ErrParseBatchPostingReport = errors.New("failed to parse batch posting report")
43+
)
44+
3945
func ValidateMaxTxDataSize(maxTxDataSize uint64) error {
4046
// tighter limit https://github.com/OffchainLabs/nitro/commit/ed015e752d7d24e59ec9e6f894fe1a26ffa19036
4147
// The default block gas limit can fit 1523 txs
@@ -201,15 +207,15 @@ func (msg *L1IncomingMessage) FillInBatchGasFieldsWithParentBlock(batchFetcher F
201207
return nil
202208
}
203209
if batchFetcher == nil {
204-
return fmt.Errorf("batch fetcher is nil, cannot fill in batch gas fields for batch posting report (parentChainBlockNumber %d)", parentChainBlockNumber)
210+
return fmt.Errorf("%w: cannot fill in batch gas fields for batch posting report (parentChainBlockNumber %d)", ErrNilBatchFetcher, parentChainBlockNumber)
205211
}
206212
if msg.BatchDataStats != nil && msg.LegacyBatchGasCost != nil {
207213
return nil
208214
}
209215
if msg.BatchDataStats == nil {
210216
_, _, batchHash, batchNum, _, _, err := ParseBatchPostingReportMessageFields(bytes.NewReader(msg.L2msg))
211217
if err != nil {
212-
return fmt.Errorf("failed to parse batch posting report: %w", err)
218+
return fmt.Errorf("%w: %w", ErrParseBatchPostingReport, err)
213219
}
214220
batchData, err := batchFetcher(batchNum, parentChainBlockNumber)
215221
if err != nil {
@@ -232,7 +238,7 @@ func (msg *L1IncomingMessage) FillInBatchGasFieldsWithParentBlock(batchFetcher F
232238
} else {
233239
gotHash := crypto.Keccak256Hash(batchData)
234240
if gotHash != batchHash {
235-
return fmt.Errorf("batch fetcher returned incorrect data hash %v (wanted %v for batch %v)", gotHash, batchHash, batchNum)
241+
return fmt.Errorf("%w: got %v, wanted %v for batch %v", ErrBatchHashMismatch, gotHash, batchHash, batchNum)
236242
}
237243
msg.BatchDataStats = GetDataStats(batchData)
238244
}

arbos/arbostypes/incomingmessage_test.go

Lines changed: 177 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"bytes"
88
"errors"
99
"math/big"
10-
"strings"
1110
"testing"
1211

1312
"github.com/ethereum/go-ethereum/common"
@@ -24,9 +23,13 @@ func TestFillInBatchGasFieldsNilFetcherBatchPostingReport(t *testing.T) {
2423
},
2524
L2msg: make([]byte, 148),
2625
}
27-
if err := msg.FillInBatchGasFields(nil); err == nil {
26+
err := msg.FillInBatchGasFields(nil)
27+
if err == nil {
2828
t.Fatal("expected error when batchFetcher is nil for BatchPostingReport")
2929
}
30+
if !errors.Is(err, ErrNilBatchFetcher) {
31+
t.Fatalf("expected ErrNilBatchFetcher, got: %v", err)
32+
}
3033
}
3134

3235
func TestFillInBatchGasFieldsNilFetcherNonBatchPostingReport(t *testing.T) {
@@ -56,8 +59,8 @@ func TestFillInBatchGasFieldsWithParentBlockNilFetcherBatchPostingReport(t *test
5659
if err == nil {
5760
t.Fatal("expected error when batchFetcher is nil for BatchPostingReport")
5861
}
59-
if !strings.Contains(err.Error(), "parentChainBlockNumber 42") {
60-
t.Fatalf("error should contain parentChainBlockNumber context, got: %v", err)
62+
if !errors.Is(err, ErrNilBatchFetcher) {
63+
t.Fatalf("expected ErrNilBatchFetcher, got: %v", err)
6164
}
6265
}
6366

@@ -104,7 +107,9 @@ func TestParseIncomingL1MessageNilFetcherNonBatchPostingReport(t *testing.T) {
104107
}
105108

106109
// buildBatchPostingReportL2msg constructs an L2msg payload for a
107-
// BatchPostingReport that references batchData with the given batchNum.
110+
// BatchPostingReport that embeds the Keccak256 hash of batchData along with
111+
// the given batchNum, matching the format expected by
112+
// ParseBatchPostingReportMessageFields.
108113
func buildBatchPostingReportL2msg(t *testing.T, batchData []byte, batchNum uint64) []byte {
109114
t.Helper()
110115
dataHash := crypto.Keccak256Hash(batchData)
@@ -119,7 +124,7 @@ func buildBatchPostingReportL2msg(t *testing.T, batchData []byte, batchNum uint6
119124
}
120125

121126
// buildBatchPostingReportMsg constructs a serialized BatchPostingReport message
122-
// whose L2msg references batchData with the given batchNum.
127+
// whose L2msg embeds the Keccak256 hash of batchData along with the given batchNum.
123128
func buildBatchPostingReportMsg(t *testing.T, batchData []byte, batchNum uint64) []byte {
124129
t.Helper()
125130
msg := &L1IncomingMessage{
@@ -149,6 +154,9 @@ func TestParseIncomingL1MessageNilFetcherBatchPostingReport(t *testing.T) {
149154
if err == nil {
150155
t.Fatal("expected error when batchFetcher is nil for BatchPostingReport")
151156
}
157+
if !errors.Is(err, ErrNilBatchFetcher) {
158+
t.Fatalf("expected ErrNilBatchFetcher, got: %v", err)
159+
}
152160
}
153161

154162
func TestParseAndFillGasFieldsWithFetcher(t *testing.T) {
@@ -217,9 +225,10 @@ func TestFillInBatchGasFieldsSkipsWhenAlreadyPopulated(t *testing.T) {
217225
}
218226
}
219227

220-
func TestFillInBatchGasFieldsHashMismatch(t *testing.T) {
221-
// If the fetcher returns data whose hash doesn't match the batch
222-
// posting report, an error must be returned.
228+
func TestParseIncomingL1MessageHashMismatch(t *testing.T) {
229+
// ParseIncomingL1Message must return ErrBatchHashMismatch when the
230+
// fetcher returns data whose Keccak256 hash doesn't match the hash
231+
// embedded in the BatchPostingReport's L2msg.
223232
batchData := []byte("correct batch data")
224233
var batchNum uint64 = 3
225234
serialized := buildBatchPostingReportMsg(t, batchData, batchNum)
@@ -230,6 +239,9 @@ func TestFillInBatchGasFieldsHashMismatch(t *testing.T) {
230239
if err == nil {
231240
t.Fatal("expected error for hash mismatch")
232241
}
242+
if !errors.Is(err, ErrBatchHashMismatch) {
243+
t.Fatalf("expected ErrBatchHashMismatch, got: %v", err)
244+
}
233245
if msg != nil {
234246
t.Fatal("expected nil message on error")
235247
}
@@ -388,10 +400,8 @@ func TestFillInBatchGasFieldsOnlyBatchDataStatsSet(t *testing.T) {
388400
}
389401

390402
func TestFillInBatchGasFieldsPopulatesFields(t *testing.T) {
391-
// FillInBatchGasFields with a real fetcher must populate both
392-
// BatchDataStats and LegacyBatchGasCost, and must pass
393-
// msg.Header.BlockNumber as the parentChainBlockNumber to the
394-
// underlying FallibleBatchFetcherWithParentBlock.
403+
// FillInBatchGasFields with a working fetcher must populate both
404+
// BatchDataStats and LegacyBatchGasCost.
395405
batchData := []byte("test batch data direct")
396406
var batchNum uint64 = 3
397407
var blockNumber uint64 = 42
@@ -472,4 +482,158 @@ func TestFillInBatchGasFieldsTruncatedL2msg(t *testing.T) {
472482
if err == nil {
473483
t.Fatal("expected error for truncated L2msg")
474484
}
485+
if !errors.Is(err, ErrParseBatchPostingReport) {
486+
t.Fatalf("expected ErrParseBatchPostingReport, got: %v", err)
487+
}
488+
}
489+
490+
func TestFillInBatchGasFieldsFetcherReturnsNilData(t *testing.T) {
491+
// When the fetcher returns (nil, nil) — no error but nil data — the
492+
// Keccak256 hash of an empty input won't match the expected hash, so
493+
// the function must return ErrBatchHashMismatch.
494+
batchData := []byte("real batch data")
495+
var batchNum uint64 = 4
496+
msg := &L1IncomingMessage{
497+
Header: &L1IncomingMessageHeader{
498+
Kind: L1MessageType_BatchPostingReport,
499+
},
500+
L2msg: buildBatchPostingReportL2msg(t, batchData, batchNum),
501+
}
502+
fetcher := func(num uint64) ([]byte, error) {
503+
return nil, nil
504+
}
505+
err := msg.FillInBatchGasFields(fetcher)
506+
if err == nil {
507+
t.Fatal("expected error when fetcher returns nil data")
508+
}
509+
if !errors.Is(err, ErrBatchHashMismatch) {
510+
t.Fatalf("expected ErrBatchHashMismatch, got: %v", err)
511+
}
512+
}
513+
514+
func TestFillInBatchGasFieldsEmptyBatchData(t *testing.T) {
515+
// When the fetcher returns empty (non-nil) batch data whose hash
516+
// matches the L2msg, both fields should be populated correctly.
517+
batchData := []byte{}
518+
var batchNum uint64 = 8
519+
msg := &L1IncomingMessage{
520+
Header: &L1IncomingMessageHeader{
521+
Kind: L1MessageType_BatchPostingReport,
522+
BlockNumber: 50,
523+
},
524+
L2msg: buildBatchPostingReportL2msg(t, batchData, batchNum),
525+
}
526+
fetcher := func(num uint64) ([]byte, error) {
527+
if num != batchNum {
528+
t.Fatalf("fetcher called with unexpected batch number %d, want %d", num, batchNum)
529+
}
530+
return batchData, nil
531+
}
532+
if err := msg.FillInBatchGasFields(fetcher); err != nil {
533+
t.Fatalf("unexpected error: %v", err)
534+
}
535+
if msg.BatchDataStats == nil {
536+
t.Fatal("expected BatchDataStats to be populated")
537+
}
538+
expectedStats := GetDataStats(batchData)
539+
if msg.BatchDataStats.Length != expectedStats.Length {
540+
t.Fatalf("BatchDataStats.Length = %d, want %d", msg.BatchDataStats.Length, expectedStats.Length)
541+
}
542+
if msg.BatchDataStats.NonZeros != expectedStats.NonZeros {
543+
t.Fatalf("BatchDataStats.NonZeros = %d, want %d", msg.BatchDataStats.NonZeros, expectedStats.NonZeros)
544+
}
545+
if msg.LegacyBatchGasCost == nil {
546+
t.Fatal("expected LegacyBatchGasCost to be populated")
547+
}
548+
}
549+
550+
func TestFillInBatchGasFieldsWithParentBlockFetcherReturnsNilData(t *testing.T) {
551+
// Same as TestFillInBatchGasFieldsFetcherReturnsNilData but exercises
552+
// the WithParentBlock variant directly.
553+
batchData := []byte("real batch data")
554+
var batchNum uint64 = 4
555+
msg := &L1IncomingMessage{
556+
Header: &L1IncomingMessageHeader{
557+
Kind: L1MessageType_BatchPostingReport,
558+
BlockNumber: 10,
559+
},
560+
L2msg: buildBatchPostingReportL2msg(t, batchData, batchNum),
561+
}
562+
fetcher := func(num uint64, parentBlock uint64) ([]byte, error) {
563+
return nil, nil
564+
}
565+
err := msg.FillInBatchGasFieldsWithParentBlock(fetcher, 10)
566+
if err == nil {
567+
t.Fatal("expected error when fetcher returns nil data")
568+
}
569+
if !errors.Is(err, ErrBatchHashMismatch) {
570+
t.Fatalf("expected ErrBatchHashMismatch, got: %v", err)
571+
}
572+
}
573+
574+
func TestFillInBatchGasFieldsIdempotent(t *testing.T) {
575+
// Calling FillInBatchGasFields twice must be idempotent — the second
576+
// call should be a no-op since both fields are already populated.
577+
batchData := []byte("idempotent batch data")
578+
var batchNum uint64 = 6
579+
msg := &L1IncomingMessage{
580+
Header: &L1IncomingMessageHeader{
581+
Kind: L1MessageType_BatchPostingReport,
582+
BlockNumber: 20,
583+
},
584+
L2msg: buildBatchPostingReportL2msg(t, batchData, batchNum),
585+
}
586+
callCount := 0
587+
fetcher := func(num uint64) ([]byte, error) {
588+
callCount++
589+
return batchData, nil
590+
}
591+
if err := msg.FillInBatchGasFields(fetcher); err != nil {
592+
t.Fatalf("first call: unexpected error: %v", err)
593+
}
594+
if callCount != 1 {
595+
t.Fatalf("expected fetcher to be called once, got %d", callCount)
596+
}
597+
firstStats := *msg.BatchDataStats
598+
firstCost := *msg.LegacyBatchGasCost
599+
600+
if err := msg.FillInBatchGasFields(fetcher); err != nil {
601+
t.Fatalf("second call: unexpected error: %v", err)
602+
}
603+
if callCount != 1 {
604+
t.Fatalf("expected fetcher not to be called again, got %d calls", callCount)
605+
}
606+
if msg.BatchDataStats.Length != firstStats.Length || msg.BatchDataStats.NonZeros != firstStats.NonZeros {
607+
t.Fatal("BatchDataStats changed on second call")
608+
}
609+
if *msg.LegacyBatchGasCost != firstCost {
610+
t.Fatalf("LegacyBatchGasCost changed on second call: got %d, want %d", *msg.LegacyBatchGasCost, firstCost)
611+
}
612+
}
613+
614+
func TestFillInBatchGasFieldsAllMessageKindsWithNilFetcher(t *testing.T) {
615+
// All non-BatchPostingReport message kinds must succeed with a nil
616+
// fetcher, since there are no batch gas fields to fill.
617+
kinds := []uint8{
618+
L1MessageType_L2Message,
619+
L1MessageType_EndOfBlock,
620+
L1MessageType_L2FundedByL1,
621+
L1MessageType_RollupEvent,
622+
L1MessageType_SubmitRetryable,
623+
L1MessageType_BatchForGasEstimation,
624+
L1MessageType_Initialize,
625+
L1MessageType_EthDeposit,
626+
L1MessageType_Invalid,
627+
}
628+
for _, kind := range kinds {
629+
msg := &L1IncomingMessage{
630+
Header: &L1IncomingMessageHeader{
631+
Kind: kind,
632+
},
633+
L2msg: make([]byte, 32),
634+
}
635+
if err := msg.FillInBatchGasFields(nil); err != nil {
636+
t.Fatalf("kind %d: unexpected error with nil fetcher: %v", kind, err)
637+
}
638+
}
475639
}

cmd/nitro/init/init.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1040,7 +1040,14 @@ func GetConsensusParsedInitMsg(ctx context.Context, parentChainReaderEnabled boo
10401040
return nil, fmt.Errorf("failed creating delayed bridge while attempting to get serialized chain config from init message: %w", err)
10411041
}
10421042
deployedAt := new(big.Int).SetUint64(rollupAddrs.DeployedAt)
1043-
delayedMessages, err := delayedBridge.LookupMessagesInRange(ctx, deployedAt, deployedAt, nil)
1043+
// This fetcher is only needed for BatchPostingReport messages, which
1044+
// should not appear at the deployment block. We only need Initialize
1045+
// messages here, but LookupMessagesInRange processes all messages in
1046+
// the block and requires a non-nil fetcher for BatchPostingReport.
1047+
batchFetcher := func(batchNum uint64, parentChainBlockNumber uint64) ([]byte, error) {
1048+
return nil, fmt.Errorf("batch data not available during init (batch %d at L1 block %d)", batchNum, parentChainBlockNumber)
1049+
}
1050+
delayedMessages, err := delayedBridge.LookupMessagesInRange(ctx, deployedAt, deployedAt, batchFetcher)
10441051
if err != nil {
10451052
return nil, fmt.Errorf("failed getting delayed messages while attempting to get serialized chain config from init message: %w", err)
10461053
}

cmd/replay/main.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,7 @@ func main() {
356356

357357
err = message.Message.FillInBatchGasFields(batchFetcher)
358358
if err != nil {
359+
log.Error("Failed to fill in batch gas fields, treating as invalid message", "err", err)
359360
message.Message = arbostypes.InvalidL1Message
360361
}
361362
return message

0 commit comments

Comments
 (0)