Merge pull request #4464 from OffchainLabs/mel-batchpostingreport-bugfix

MEL fix number of batch posting reports check bug

Author: eljobe

arbnode/mel/extraction/message_extraction_function.go

@@ -5,7 +5,6 @@ package melextraction
 import (
 	"bytes"
 	"context"
-	"errors"
 	"fmt"
 
 	"github.com/ethereum/go-ethereum/common"
@@ -148,16 +147,16 @@ func extractMessagesImpl(
 		// If this message is a batch posting report, we save it for later
 		// use in this function once we extract messages from batches and
 		// need to fill in their batch posting report.
-		if delayed.Message.Header.Kind == arbostypes.L1MessageType_BatchPostingReport || delayed.Message.Header.Kind == arbostypes.L1MessageType_Initialize { // Let's consider the init message as a batch posting report, since it is seen as a batch as well, we can later ignore filling its batchGasCost anyway
+		if delayed.Message.Header.Kind == arbostypes.L1MessageType_BatchPostingReport {
 			batchPostingReports = append(batchPostingReports, delayed)
 		}
 	}
 
 	// Batch posting reports are included in the same transaction as a batch, so there should
-	// always be the same number of reports as there are batches.
-	if len(batchPostingReports) != len(batches) {
+	// be the same or lesser number of reports as there are batches.
+	if len(batchPostingReports) > len(batches) {
 		return nil, nil, nil, nil, fmt.Errorf(
-			"batch posting reports %d do not match the number of batches %d",
+			"number of batch posting reports %d higher than the number of batches %d",
 			len(batchPostingReports),
 			len(batches),
 		)
@@ -166,6 +165,8 @@ func extractMessagesImpl(
 	var batchMetas []*mel.BatchMetadata
 	var messages []*arbostypes.MessageWithMetadata
 	var serializedBatches [][]byte
+	var batchPostReportIndex int
+	var batchPostReportBatchHash common.Hash
 	for i, batch := range batches {
 		batchTx := batchTxs[i]
 		serialized, err := serialize(
@@ -178,31 +179,38 @@ func extractMessagesImpl(
 			return nil, nil, nil, nil, err
 		}
 
-		batchPostReport := batchPostingReports[i]
-		if batchPostReport.Message.Header.Kind != arbostypes.L1MessageType_Initialize {
-			_, _, batchHash, _, _, _, err := parseBatchPostingReport(bytes.NewReader(batchPostReport.Message.L2msg))
-			if err != nil {
-				return nil, nil, nil, nil, fmt.Errorf("failed to parse batch posting report: %w", err)
+		if batchPostReportIndex < len(batchPostingReports) {
+			batchPostReport := batchPostingReports[batchPostReportIndex]
+			if (batchPostReportBatchHash == common.Hash{}) {
+				_, _, batchPostReportBatchHash, _, _, _, err = parseBatchPostingReport(bytes.NewReader(batchPostReport.Message.L2msg))
+				if err != nil {
+					return nil, nil, nil, nil, fmt.Errorf("failed to parse batch posting report: %w", err)
+				}
 			}
 			gotHash := crypto.Keccak256Hash(serialized)
-			if gotHash != batchHash {
-				return nil, nil, nil, nil, fmt.Errorf(
-					"batch data hash incorrect %v (wanted %v for batch %v)",
-					gotHash,
-					batchHash,
-					batch.SequenceNumber,
-				)
+			if gotHash == batchPostReportBatchHash {
+				// Fill in the batch gas stats into the batch posting report.
+				batchPostReport.Message.BatchDataStats = arbostypes.GetDataStats(serialized)
+				legacyCost := arbostypes.LegacyCostForStats(batchPostReport.Message.BatchDataStats)
+				batchPostReport.Message.LegacyBatchGasCost = &legacyCost
+				// Process next report
+				batchPostReportIndex++
+				batchPostReportBatchHash = common.Hash{}
 			}
-			// Fill in the batch gas stats into the batch posting report.
-			batchPostReport.Message.BatchDataStats = arbostypes.GetDataStats(serialized)
-			legacyCost := arbostypes.LegacyCostForStats(batchPostReport.Message.BatchDataStats)
-			batchPostReport.Message.LegacyBatchGasCost = &legacyCost
-		} else if !(inputState.DelayedMessagesSeen == 0 && i == 0 && delayedMessages[i] == batchPostReport) {
-			return nil, nil, nil, nil, errors.New("encountered initialize message that is not the first delayed message and the first batch ")
 		}
 		serializedBatches = append(serializedBatches, serialized)
 	}
 
+	// Batch posting reports are included in the same transaction as a batch, so all the
+	// reports should have been filled in with the batch gas stats
+	if len(batchPostingReports) != batchPostReportIndex {
+		return nil, nil, nil, nil, fmt.Errorf(
+			"not all batch posting reports processed. Have: %d, Processed: %d",
+			len(batchPostingReports),
+			batchPostReportIndex,
+		)
+	}
+
 	// Update the delayed message accumulator in the MEL state.
 	for _, delayed := range delayedMessages {
 		if err = state.AccumulateDelayedMessage(delayed); err != nil {

arbnode/mel/extraction/message_extraction_function_test.go

@@ -5,6 +5,7 @@ package melextraction
 import (
 	"context"
 	"errors"
+	"fmt"
 	"io"
 	"math/big"
 	"testing"
@@ -64,11 +65,11 @@ func TestExtractMessages(t *testing.T) {
 			expectedError:      "failed to lookup delayed messages",
 		},
 		{
-			name:               "mismatched number of batch posting reports vs batches",
+			name:               "number of batch posting reports higher than batches",
 			melStateParentHash: prevParentBlockHash,
 			lookupBatches:      emptyLookupBatches,          // 0 batches
 			lookupDelayedMsgs:  successfulLookupDelayedMsgs, // 1 batch posting report
-			expectedError:      "batch posting reports 1 do not match the number of batches 0",
+			expectedError:      "number of batch posting reports 1 higher than the number of batches 0",
 		},
 		{
 			name:               "batch serialization fails",
@@ -94,7 +95,7 @@ func TestExtractMessages(t *testing.T) {
 			lookupDelayedMsgs:  successfulLookupDelayedMsgs,
 			serializer:         emptySerializer,  // Returns nil, hash will be different from parseReport
 			parseReport:        emptyParseReport, // Returns empty hash
-			expectedError:      "batch data hash incorrect",
+			expectedError:      "not all batch posting reports processed.",
 		},
 		{
 			name:               "parse sequencer message fails",
@@ -131,6 +132,32 @@ func TestExtractMessages(t *testing.T) {
 			expectedMessages:     2,
 			expectedDelayedMsgs:  1,
 		},
+		{
+			// 3 batches, 1 batch posting report whose hash matches only the 2nd batch.
+			// Exercises the continue path where batch 0's hash doesn't match the report,
+			// batch 1's hash does match, and batch 2 has no report to check.
+			name:               "OK - multiple batches with subset of reports",
+			melStateParentHash: prevParentBlockHash,
+			lookupBatches:      threeBatchLookup,
+			lookupDelayedMsgs:  delayedMsgsWithOneReportForSecondBatch,
+			serializer:         indexedSerializer(),
+			parseReport:        parseReportForSecondBatch,
+			parseSequencerMsg:  successfulParseSequencerMsg,
+			extractBatchMessages: func(ctx context.Context, melState *mel.State, seqMsg *arbstate.SequencerMessage, delayedMsgDB DelayedMessageDatabase) ([]*arbostypes.MessageWithMetadata, error) {
+				return []*arbostypes.MessageWithMetadata{
+					{
+						Message: &arbostypes.L1IncomingMessage{
+							L2msg:  []byte("msg"),
+							Header: &arbostypes.L1IncomingMessageHeader{Kind: arbostypes.L1MessageType_L2Message},
+						},
+					},
+				}, nil
+			},
+			expectedMsgCount:    3, // 1 message per batch * 3 batches
+			expectedDelayedSeen: 1,
+			expectedMessages:    3,
+			expectedDelayedMsgs: 1,
+		},
 	}
 
 	for _, tt := range tests {
@@ -374,3 +401,60 @@ func failingExtractBatchMessages(
 ) ([]*arbostypes.MessageWithMetadata, error) {
 	return nil, errors.New("failed to extract batch messages")
 }
+
+// threeBatchLookup returns 3 batches and 3 transactions.
+func threeBatchLookup(
+	ctx context.Context,
+	parentChainBlock *types.Header,
+	txFetcher TransactionFetcher,
+	logsFetcher LogsFetcher,
+	eventUnpacker EventUnpacker,
+) ([]*mel.SequencerInboxBatch, []*types.Transaction, error) {
+	batches := []*mel.SequencerInboxBatch{{}, {}, {}}
+	txs := []*types.Transaction{{}, {}, {}}
+	return batches, txs, nil
+}
+
+// delayedMsgsWithOneReportForSecondBatch returns 1 delayed message that is a
+// batch posting report whose hash will match the 2nd batch (index 1) produced
+// by indexedSerializer.
+func delayedMsgsWithOneReportForSecondBatch(
+	ctx context.Context,
+	melState *mel.State,
+	parentChainBlock *types.Header,
+	txFetcher TransactionFetcher,
+	logsFetcher LogsFetcher,
+) ([]*mel.DelayedInboxMessage, error) {
+	hash := common.MaxHash
+	return []*mel.DelayedInboxMessage{
+		{
+			Message: &arbostypes.L1IncomingMessage{
+				L2msg: []byte("report-for-batch1"),
+				Header: &arbostypes.L1IncomingMessageHeader{
+					Kind:      arbostypes.L1MessageType_BatchPostingReport,
+					RequestId: &hash,
+					L1BaseFee: common.Big0,
+				},
+			},
+		},
+	}, nil
+}
+
+// indexedSerializer returns a serializer that produces different bytes for each
+// successive call: "batch0", "batch1", "batch2", …
+func indexedSerializer() func(context.Context, *mel.SequencerInboxBatch, *types.Transaction, LogsFetcher) ([]byte, error) {
+	callIndex := 0
+	return func(ctx context.Context, batch *mel.SequencerInboxBatch, tx *types.Transaction, logsFetcher LogsFetcher) ([]byte, error) {
+		data := []byte(fmt.Sprintf("batch%d", callIndex))
+		callIndex++
+		return data, nil
+	}
+}
+
+// parseReportForSecondBatch returns a hash that matches the serialized bytes
+// of the 2nd batch ("batch1") from indexedSerializer.
+func parseReportForSecondBatch(
+	rd io.Reader,
+) (*big.Int, common.Address, common.Hash, uint64, *big.Int, uint64, error) {
+	return nil, common.Address{}, crypto.Keccak256Hash([]byte("batch1")), 0, nil, 0, nil
+}

changelog/ganeshvanahalli-nit-4540.md

@@ -0,0 +1,2 @@
+### Added
+ - Fix Message Extraction function to handle cases when number of batch posting reports are not equal to the number of batches