Fix bug in extracting L2 msgs from batch and enable skipped system_tests

arbnode/mel/extraction/messages_in_batch.go

@@ -33,7 +33,11 @@ func messagesFromBatchSegments(
 	messages := make([]*arbostypes.MessageWithMetadata, 0)
 	timestamp := uint64(0)
 	blockNumber := uint64(0)
-	for idx, segment := range seqMsg.Segments {
+	segments := seqMsg.Segments
+	if len(segments) == 0 {
+		segments = [][]byte{{arbstate.BatchSegmentKindDelayedMessages}}
+	}
+	for idx, segment := range segments {
 		msg, newBlockNumber, newTimestamp, err := messageFromSegment(
 			ctx,
 			melState,
@@ -204,7 +208,7 @@ func extractDelayedMessageFromSegment(
 		return nil, err
 	}
 	if delayed == nil {
-		log.Error("No more delayed messages in queue", "delayedMessagesRead", melState.DelayedMessagesRead)
+		log.Error("No more delayed messages in queue", "delayedMessagesRead", melState.DelayedMessagesRead, "delayedMessagesSeen", melState.DelayedMessagesSeen)
 		return nil, fmt.Errorf("no more delayed messages in db")
 	}
 

changelog/ganeshvanahalli-nit-3883.md

@@ -0,0 +1,2 @@
+### Fixed
+ - Fix bug in L2 message extraction from batch segments and enable previously skipped system_tests

system_tests/anytrust_test.go

@@ -12,7 +12,6 @@ import (
 	"time"
 
 	"github.com/ethereum/go-ethereum/common"
-	"github.com/ethereum/go-ethereum/core/rawdb"
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/ethclient"
 
@@ -27,7 +26,6 @@ import (
 	"github.com/offchainlabs/nitro/util/headerreader"
 	"github.com/offchainlabs/nitro/util/rpcclient"
 	"github.com/offchainlabs/nitro/util/testhelpers"
-	"github.com/offchainlabs/nitro/util/testhelpers/flag"
 )
 
 func startLocalAnyTrustServer(
@@ -112,9 +110,6 @@ func TestAnyTrustRekey(t *testing.T) {
 
 	// Setup L1 chain and contracts
 	builder := NewNodeBuilder(ctx).DefaultConfig(t, true)
-	if *testflag.StateSchemeFlag == rawdb.PathScheme {
-		builder.nodeConfig.MessageExtraction.Enable = false
-	}
 	builder.BuildL1(t)
 
 	// Setup AnyTrust servers

system_tests/common_test.go

@@ -221,12 +221,18 @@ func (tc *TestClient) BalanceDifferenceAtBlock(address common.Address, blockNum
 	return arbmath.BigSub(newBalance, prevBalance), nil
 }
 
-func (tc *TestClient) AdvanceBlocks(t *testing.T, numBlocks int, lInfo info) {
+func (tc *TestClient) AdvanceBlocks(t *testing.T, account string, numBlocks int, lInfo info) {
 	for range numBlocks {
-		tc.TransferBalance(t, "Faucet", "Faucet", common.Big1, lInfo)
+		tc.TransferBalance(t, account, account, common.Big1, lInfo)
 	}
 }
 
+func (tc *TestClient) RecalibrateNonce(t *testing.T, account string, lInfo info) {
+	currNonce, err := tc.Client.PendingNonceAt(tc.ctx, lInfo.GetAddress(account))
+	Require(t, err)
+	lInfo.GetInfoWithPrivKey(account).Nonce.Store(currNonce)
+}
+
 var DefaultTestForwarderConfig = gethexec.ForwarderConfig{
 	ConnectionTimeout:     2 * time.Second,
 	IdleConnectionTimeout: 2 * time.Second,

system_tests/meaningless_reorg_test.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/ethereum/go-ethereum/common"
 
+	"github.com/offchainlabs/nitro/arbnode/mel"
 	"github.com/offchainlabs/nitro/solgen/go/bridgegen"
 )
 
@@ -19,7 +20,6 @@ func TestMeaninglessBatchReorg(t *testing.T) {
 	defer cancel()
 
 	builder := NewNodeBuilder(ctx).DefaultConfig(t, true).WithTakeOwnership(false)
-	builder.nodeConfig.MessageExtraction.Enable = false
 	builder.nodeConfig.BatchPoster.Enable = false
 	cleanup := builder.Build(t)
 	defer cleanup()
@@ -46,7 +46,12 @@ func TestMeaninglessBatchReorg(t *testing.T) {
 		}
 		time.Sleep(10 * time.Millisecond)
 	}
-	metadata, err := builder.L2.ConsensusNode.InboxTracker.GetBatchMetadata(1)
+	var metadata mel.BatchMetadata
+	if builder.L2.ConsensusNode.MessageExtractor != nil {
+		metadata, err = builder.L2.ConsensusNode.MessageExtractor.GetBatchMetadata(1)
+	} else {
+		metadata, err = builder.L2.ConsensusNode.InboxTracker.GetBatchMetadata(1)
+	}
 	Require(t, err)
 	originalBatchBlock := batchReceipt.BlockNumber.Uint64()
 	if metadata.ParentChainBlock != originalBatchBlock {
@@ -59,18 +64,32 @@ func TestMeaninglessBatchReorg(t *testing.T) {
 	// The miner usually collects transactions from deleted blocks and puts them in the mempool.
 	// However, this code doesn't run on reorgs larger than 64 blocks for performance reasons.
 	// Therefore, we make a bunch of small blocks to prevent the code from running.
-	for j := uint64(0); j < 70; j++ {
-		builder.L1.TransferBalance(t, "Faucet", "Faucet", common.Big1, builder.L1Info)
-	}
+	builder.L1.AdvanceBlocks(t, "Faucet", 70, builder.L1Info)
 
 	compareAllMsgResultsFromConsensusAndExecution(t, ctx, builder.L2, "before reorg")
 
+	currHead, err := builder.L1.Client.BlockNumber(ctx)
+	Require(t, err)
 	parentBlock := builder.L1.L1Backend.BlockChain().GetBlockByNumber(batchReceipt.BlockNumber.Uint64() - 1)
 	err = builder.L1.L1Backend.BlockChain().ReorgToOldBlock(parentBlock)
 	Require(t, err)
 
-	// Produce a new l1Block so that the batch ends up in a different l1Block than before
-	builder.L1.TransferBalance(t, "User", "User", common.Big1, builder.L1Info)
+	// Check that reorg was processed so that correct pending nonce is set
+	for i := 0; ; i++ {
+		if i >= 500 {
+			Fatal(t, "Failed to see reorg in L1")
+		}
+		newHead, err := builder.L1.Client.BlockNumber(ctx)
+		Require(t, err)
+		if newHead < currHead {
+			break
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+	builder.L1.RecalibrateNonce(t, "Faucet", builder.L1Info)
+	// Produce new l1Blocks so that the batch ends up in a different l1Block than before
+	// #nosec G115
+	builder.L1.AdvanceBlocks(t, "Faucet", int(currHead-parentBlock.NumberU64()+5), builder.L1Info)
 
 	tx, err = seqInbox.AddSequencerL2BatchFromOrigin8f111f3c(&seqOpts, big.NewInt(1), nil, big.NewInt(1), common.Address{}, common.Big0, common.Big0)
 	Require(t, err)
@@ -88,8 +107,19 @@ func TestMeaninglessBatchReorg(t *testing.T) {
 		if i >= 500 {
 			Fatal(t, "Failed to read batch reorg from L1")
 		}
-		metadata, err = builder.L2.ConsensusNode.InboxTracker.GetBatchMetadata(1)
-		Require(t, err)
+		if builder.L2.ConsensusNode.MessageExtractor != nil {
+			batchCount, err := builder.L2.ConsensusNode.MessageExtractor.GetBatchCount()
+			Require(t, err)
+			if batchCount <= 1 {
+				time.Sleep(10 * time.Millisecond)
+				continue
+			}
+			metadata, err = builder.L2.ConsensusNode.MessageExtractor.GetBatchMetadata(1)
+			Require(t, err)
+		} else {
+			metadata, err = builder.L2.ConsensusNode.InboxTracker.GetBatchMetadata(1)
+			Require(t, err)
+		}
 		if metadata.ParentChainBlock == newBatchBlock {
 			break
 		} else if metadata.ParentChainBlock != originalBatchBlock {
@@ -98,7 +128,11 @@ func TestMeaninglessBatchReorg(t *testing.T) {
 		time.Sleep(10 * time.Millisecond)
 	}
 
-	_, _, err = builder.L2.ConsensusNode.InboxReader.GetSequencerMessageBytes(ctx, 1)
+	if builder.L2.ConsensusNode.MessageExtractor != nil {
+		_, _, err = builder.L2.ConsensusNode.MessageExtractor.GetSequencerMessageBytes(ctx, 1)
+	} else {
+		_, _, err = builder.L2.ConsensusNode.InboxReader.GetSequencerMessageBytes(ctx, 1)
+	}
 	Require(t, err)
 
 	l2Header, err := builder.L2.Client.HeaderByNumber(ctx, l2Receipt.BlockNumber)

system_tests/multi_constraint_pricer_test.go

@@ -419,7 +419,7 @@ func TestMultiGasDoesntRefundRetryablesMultipleTimes(t *testing.T) {
 	})
 	require.NoError(t, err)
 	require.NotNil(t, tx)
-	builder.L2.AdvanceBlocks(t, 2, builder.L2Info) // Advance blocks so changes take effect
+	builder.L2.AdvanceBlocks(t, "Faucet", 2, builder.L2Info) // Advance blocks so changes take effect
 	baseFeeAfter := builder.L2.GetBaseFee(t)
 	t.Log("Base fee before: ", baseFeeBefore.Int64())
 	t.Log("Base fee after:  ", baseFeeAfter.Int64())

system_tests/recreatestate_rpc_test.go

@@ -490,7 +490,6 @@ func testGettingState(t *testing.T, execConfig *gethexec.Config) {
 	// This test is HashScheme specific, it shouldn't be run with Pathdb
 	builder.RequireScheme(t, rawdb.HashScheme)
 	builder.execConfig = execConfig
-	builder.nodeConfig.MessageExtraction.Enable = (execConfig.Caching.StateScheme != rawdb.PathScheme) // TODO: investigate why this test fails for MEL when run with pathdb
 	cancelNode := buildWithHistory(t, ctx, builder, 16)
 	execNode := builder.L2.ExecNode
 	defer cancelNode()

system_tests/seqinbox_test.go

@@ -10,15 +10,13 @@ import (
 	"fmt"
 	"math/big"
 	"math/rand"
-	"strings"
 	"testing"
 	"time"
 
 	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/accounts/abi"
 	"github.com/ethereum/go-ethereum/accounts/abi/bind"
 	"github.com/ethereum/go-ethereum/common"
-	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/ethclient/gethclient"
 	"github.com/ethereum/go-ethereum/params"
@@ -140,7 +138,6 @@ func testSequencerInboxReaderImpl(t *testing.T, validator bool) {
 	defer cancel()
 
 	builder := NewNodeBuilder(ctx).DefaultConfig(t, true).DontParalellise().WithTakeOwnership(false)
-	builder.nodeConfig.MessageExtraction.Enable = false // TODO: solve for => Error in message extractor               err="batch posting reports 0 do not match the number of batches 1"
 	if validator {
 		builder.nodeConfig.BlockValidator.Enable = true
 	}
@@ -219,20 +216,9 @@ func testSequencerInboxReaderImpl(t *testing.T, validator bool) {
 			Require(t, err)
 			blocksToPad := 65 - (currentHeader.Number.Uint64() - reorgTargetNumber)
 
-			currNonce, err := builder.L1.Client.PendingNonceAt(ctx, builder.L1Info.GetAddress("Faucet"))
-			Require(t, err)
-			builder.L1Info.GetInfoWithPrivKey("Faucet").Nonce.Store(currNonce)
-			for j := uint64(0); j < blocksToPad; j++ {
-				tx := builder.L1Info.PrepareTx("Faucet", "User", 30000, big.NewInt(1e12), nil)
-				err = builder.L1.Client.SendTransaction(ctx, tx)
-				if err != nil {
-					if !strings.Contains(err.Error(), "already known") && !strings.Contains(err.Error(), core.ErrNonceTooLow.Error()) {
-						t.Fatalf("error sending txs to create padding for reorg: %s", err.Error())
-					}
-				} else {
-					_, _ = builder.L1.EnsureTxSucceeded(tx)
-				}
-			}
+			builder.L1.RecalibrateNonce(t, "Faucet", builder.L1Info)
+			// #nosec G115
+			builder.L1.AdvanceBlocks(t, "Faucet", int(blocksToPad), builder.L1Info)
 			currentHeader, err = builder.L1.Client.HeaderByNumber(ctx, nil)
 			Require(t, err)
 			// #nosec G115
@@ -253,6 +239,11 @@ func testSequencerInboxReaderImpl(t *testing.T, validator bool) {
 			err = builder.L1.Client.SendTransaction(ctx, tx)
 			Require(t, err)
 			_, _ = WaitForTx(ctx, builder.L1.Client, tx.Hash(), time.Second)
+
+			// Advance L1 to currentHeader+1 block so that MEL can detect reorg
+			builder.L1.RecalibrateNonce(t, "Faucet", builder.L1Info)
+			// #nosec G115
+			builder.L1.AdvanceBlocks(t, "Faucet", int(currentHeader.Number.Uint64()-reorgTargetNumber+1), builder.L1Info)
 		} else {
 			state := blockStates[len(blockStates)-1]
 			newBalances := make(map[common.Address]*big.Int)