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, len(seqMsg.Segments))
 	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

@@ -228,6 +228,16 @@ func (tc *TestClient) AdvanceBlocks(t *testing.T, numBlocks int, lInfo info) {
 	}
 }
 
+func (tc *TestClient) RecalibrateNonce(t *testing.T, lInfo info) {
+	for account, accInfo := range lInfo.Accounts {
+		if accInfo.PrivateKey != nil {
+			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

@@ -19,7 +19,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()
@@ -59,18 +58,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, 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, builder.L1Info)
+	// Produce new l1Blocks so that the batch ends up in a different l1Block than before
+	// #nosec G115
+	builder.L1.AdvanceBlocks(t, 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,6 +101,14 @@ func TestMeaninglessBatchReorg(t *testing.T) {
 		if i >= 500 {
 			Fatal(t, "Failed to read batch reorg from L1")
 		}
+		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.GetParentChainDataSource().GetBatchMetadata(1)
 		Require(t, err)
 		if metadata.ParentChainBlock == newBatchBlock {

system_tests/pruning_test.go

@@ -4,7 +4,6 @@ package arbtest
 
 import (
 	"context"
-	"fmt"
 	"math/big"
 	"strings"
 	"testing"
@@ -37,12 +36,20 @@ func countStateEntries(db ethdb.Iteratee) int {
 	return entries
 }
 
-func TestPruningDBSizeReduction(t *testing.T) {
-	// TODO test "validator" pruning mode - requires latest confirmed
-	for _, mode := range []string{"full", "minimal"} {
-		t.Run(fmt.Sprintf("-%s-mode-without-parallel-storage-traversal", mode), func(t *testing.T) { runPruningDBSizeReductionTest(t, mode, false) })
-		t.Run(fmt.Sprintf("-%s-mode-with-parallel-storage-traversal", mode), func(t *testing.T) { runPruningDBSizeReductionTest(t, mode, true) })
-	}
+func TestPruningDBSizeReductionFullModeWithoutParallelStorageTraversal(t *testing.T) {
+	runPruningDBSizeReductionTest(t, "full", false)
+}
+
+func TestPruningDBSizeReductionFullModeWithParallelStorageTraversal(t *testing.T) {
+	runPruningDBSizeReductionTest(t, "full", true)
+}
+
+func TestPruningDBSizeReductionMinimalModeWithoutParallelStorageTraversal(t *testing.T) {
+	runPruningDBSizeReductionTest(t, "minimal", false)
+}
+
+func TestPruningDBSizeReductionMinimalModeWithParallelStorageTraversal(t *testing.T) {
+	runPruningDBSizeReductionTest(t, "minimal", true)
 }
 
 func runPruningDBSizeReductionTest(t *testing.T, mode string, pruneParallelStorageTraversal bool) {
@@ -146,6 +153,9 @@ func runPruningDBSizeReductionTest(t *testing.T, mode string, pruneParallelStora
 	}
 	for i := start; i <= currentBlock; i++ {
 		header := bc.GetHeaderByNumber(i)
+		if header == nil {
+			t.Fatalf("missing header for block %d", i)
+		}
 		_, err := bc.StateAt(header.Root)
 		Require(t, err)
 		tr, err := trie.New(trie.TrieID(header.Root), triedb)
@@ -356,10 +366,20 @@ func runPruningStateAvailabilityTest(t *testing.T, mode string) {
 }
 
 func waitForChainToCatchUp(t *testing.T, ctx context.Context, testClient *TestClient, lastBlock uint64) uint64 {
+	t.Helper()
+	deadline := time.Now().Add(2 * time.Minute)
 	currentBlock := uint64(0)
 	var err error
 	// wait for the chain to catch up
 	for currentBlock < lastBlock {
+		if time.Now().After(deadline) {
+			t.Fatalf("timed out waiting for chain to catch up to block %d (currently at %d)", lastBlock, currentBlock)
+		}
+		select {
+		case <-ctx.Done():
+			t.Fatalf("context cancelled waiting for chain to catch up: %v", ctx.Err())
+		default:
+		}
 		currentBlock, err = testClient.Client.BlockNumber(ctx)
 		Require(t, err)
 		time.Sleep(20 * time.Millisecond)

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, builder.L1Info)
+			// #nosec G115
+			builder.L1.AdvanceBlocks(t, 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, builder.L1Info)
+			// #nosec G115
+			builder.L1.AdvanceBlocks(t, int(currentHeader.Number.Uint64()-reorgTargetNumber+1), builder.L1Info)
 		} else {
 			state := blockStates[len(blockStates)-1]
 			newBalances := make(map[common.Address]*big.Int)