fix(op-node): temporary batch-builder reorg fix - don't omit empty batches (#2705)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

op-node/l2/util.go

@@ -82,7 +82,6 @@ func (res *AccountResult) Verify(stateRoot common.Hash) error {
 }
 
 // BlockToBatch converts a L2 block to batch-data.
-// Empty L2 blocks (i.e. only a L1 info deposit tx) return a nil batch with nil error.
 // Invalid L2 blocks may return an error.
 func BlockToBatch(config *rollup.Config, block *types.Block) (*derive.BatchData, error) {
 	txs := block.Transactions()
@@ -92,9 +91,6 @@ func BlockToBatch(config *rollup.Config, block *types.Block) (*derive.BatchData,
 	if typ := txs[0].Type(); typ != types.DepositTxType {
 		return nil, fmt.Errorf("expected first tx to be a deposit of L1 info, but got type: %d", typ)
 	}
-	if len(txs) == 1 { // the L1 info deposit tx, but empty otherwise, no batch data to submit
-		return nil, nil
-	}
 
 	// encode non-deposit transactions
 	var opaqueTxs []hexutil.Bytes

op-node/node/api.go

@@ -219,7 +219,7 @@ func (n *nodeAPI) GetBatchBundle(ctx context.Context, req *BatchBundleRequest) (
 
 	var pruneCount int
 	for {
-		if !bundleBuilder.HasNonEmptyCandidate() {
+		if !bundleBuilder.HasCandidate() {
 			return bundleBuilder.Response(nil), nil
 		}
 
@@ -236,7 +236,7 @@ func (n *nodeAPI) GetBatchBundle(ctx context.Context, req *BatchBundleRequest) (
 		// occur since our initial greedy estimate has a very small, bounded
 		// error tolerance, so simply remove the last block and try again.
 		if bundleSize > uint64(req.MaxSize) {
-			bundleBuilder.PruneLastNonEmpty()
+			bundleBuilder.PruneLast()
 			pruneCount++
 			continue
 		}

op-node/node/bundle_builder.go

@@ -12,9 +12,7 @@ type BundleCandidate struct {
 	// ID is the block ID of an L2 block.
 	ID eth.BlockID
 
-	// Batch is batch data drived from the L2 Block. If Batch is nil, the block
-	// is considered to be empty. Empty blocks do not contribute to the size of
-	// a bundle.
+	// Batch is batch data drived from the L2 Block.
 	Batch *derive.BatchData
 }
 
@@ -24,7 +22,6 @@ type BundleCandidate struct {
 type BundleBuilder struct {
 	prevBlockID eth.BlockID
 	candidates  []BundleCandidate
-	numNonEmpty int
 }
 
 // NewBundleBuilder creates a new instance of a BundleBuilder, where prevBlockID
@@ -33,52 +30,37 @@ func NewBundleBuilder(prevBlockID eth.BlockID) *BundleBuilder {
 	return &BundleBuilder{
 		prevBlockID: prevBlockID,
 		candidates:  nil,
-		numNonEmpty: 0,
 	}
 }
 
 // AddCandidate appends a candidate block to the BundleBuilder.
 func (b *BundleBuilder) AddCandidate(candidate BundleCandidate) {
 	b.candidates = append(b.candidates, candidate)
-	if candidate.Batch != nil {
-		b.numNonEmpty++
-	}
 }
 
-// HasNonEmptyCandidate returns true if there are a non-zero number of
+// HasCandidate returns true if there are a non-zero number of
 // non-empty bundle candidates.
-func (b *BundleBuilder) HasNonEmptyCandidate() bool {
-	return b.numNonEmpty > 0
+func (b *BundleBuilder) HasCandidate() bool {
+	return len(b.candidates) > 0
 }
 
-// PruneLastNonEmpty removes the latest non-empty candidate block and all empty
-// blocks follow it. This method is used to reduce the size of the encoded
+// PruneLast removes the last candidate block.
+// This method is used to reduce the size of the encoded
 // bundle in order to satisfy the desired size constraints.
-func (b *BundleBuilder) PruneLastNonEmpty() {
-	if b.numNonEmpty == 0 {
+func (b *BundleBuilder) PruneLast() {
+	if len(b.candidates) == 0 {
 		return
 	}
-
-	for i := len(b.candidates) - 1; i >= 0; i-- {
-		candidate := b.candidates[i]
-		if candidate.Batch != nil {
-			b.candidates = b.candidates[:i]
-			b.numNonEmpty--
-			return
-		}
-	}
+	b.candidates = b.candidates[:len(b.candidates)-1]
 }
 
 // Batches returns a slice of all non-nil batches contained within the candidate
 // blocks.
 func (b *BundleBuilder) Batches() []*derive.BatchData {
-	var batches = make([]*derive.BatchData, 0, b.numNonEmpty)
+	var batches = make([]*derive.BatchData, 0, len(b.candidates))
 	for _, candidate := range b.candidates {
-		if candidate.Batch != nil {
-			batches = append(batches, candidate.Batch)
-		}
+		batches = append(batches, candidate.Batch)
 	}
-
 	return batches
 }
 

op-node/node/bundle_builder_test.go

@@ -38,7 +38,7 @@ func createResponse(
 func TestNewBundleBuilder(t *testing.T) {
 	builder := node.NewBundleBuilder(testPrevBlockID)
 
-	require.False(t, builder.HasNonEmptyCandidate())
+	require.False(t, builder.HasCandidate())
 	require.Equal(t, builder.Batches(), []*derive.BatchData{})
 	expResponse := createResponse(testPrevBlockID, testPrevBlockID, nil)
 	require.Equal(t, expResponse, builder.Response(nil))
@@ -49,31 +49,14 @@ func TestNewBundleBuilder(t *testing.T) {
 func TestBundleBuilderAddCandidate(t *testing.T) {
 	builder := node.NewBundleBuilder(testPrevBlockID)
 
-	// Add an empty candidate.
-	blockID6 := eth.BlockID{
-		Number: 6,
-		Hash:   common.HexToHash("0x66"),
-	}
-	builder.AddCandidate(node.BundleCandidate{
-		ID:    blockID6,
-		Batch: nil,
-	})
-
-	// Should behave the same as completely empty builder except for updated
-	// last block ID fields.
-	require.False(t, builder.HasNonEmptyCandidate())
-	require.Equal(t, builder.Batches(), []*derive.BatchData{})
-	expResponse := createResponse(testPrevBlockID, blockID6, nil)
-	require.Equal(t, expResponse, builder.Response(nil))
-
-	// Add non-empty candidate.
+	// Add candidate.
 	blockID7 := eth.BlockID{
 		Number: 7,
 		Hash:   common.HexToHash("0x77"),
 	}
 	batchData7 := &derive.BatchData{
 		BatchV1: derive.BatchV1{
-			Epoch:     7,
+			Epoch:     3,
 			Timestamp: 42,
 			Transactions: []hexutil.Bytes{
 				hexutil.Bytes([]byte{0x42, 0x07}),
@@ -85,144 +68,35 @@ func TestBundleBuilderAddCandidate(t *testing.T) {
 		Batch: batchData7,
 	})
 
-	// HasNonEmptyCandidate should register that we have data to submit to L1,
+	// HasCandidate should register that we have data to submit to L1,
 	// last block ID fields should also be updated.
-	require.True(t, builder.HasNonEmptyCandidate())
+	require.True(t, builder.HasCandidate())
 	require.Equal(t, builder.Batches(), []*derive.BatchData{batchData7})
-	expResponse = createResponse(testPrevBlockID, blockID7, testBundleData)
+	expResponse := createResponse(testPrevBlockID, blockID7, testBundleData)
 	require.Equal(t, expResponse, builder.Response(testBundleData))
 
-	// Add another empty block.
+	// Add another block.
 	blockID8 := eth.BlockID{
 		Number: 8,
 		Hash:   common.HexToHash("0x88"),
 	}
+	batchData8 := &derive.BatchData{
+		BatchV1: derive.BatchV1{
+			Epoch:     5,
+			Timestamp: 44,
+			Transactions: []hexutil.Bytes{
+				hexutil.Bytes([]byte{0x13, 0x37}),
+			},
+		},
+	}
 	builder.AddCandidate(node.BundleCandidate{
 		ID:    blockID8,
-		Batch: nil,
+		Batch: batchData8,
 	})
 
 	// Last block ID fields should be updated.
-	require.True(t, builder.HasNonEmptyCandidate())
-	require.Equal(t, builder.Batches(), []*derive.BatchData{batchData7})
+	require.True(t, builder.HasCandidate())
+	require.Equal(t, builder.Batches(), []*derive.BatchData{batchData7, batchData8})
 	expResponse = createResponse(testPrevBlockID, blockID8, testBundleData)
 	require.Equal(t, expResponse, builder.Response(testBundleData))
 }
-
-var pruneLastNonEmptyTests = []pruneLastNonEmptyTestCase{
-	{
-		name:        "no candidates",
-		candidates:  nil,
-		expResponse: createResponse(testPrevBlockID, testPrevBlockID, nil),
-	},
-	{
-		name: "only empty blocks",
-		candidates: []node.BundleCandidate{
-			{
-				ID: eth.BlockID{
-					Number: 6,
-					Hash:   common.HexToHash("0x66"),
-				},
-				Batch: nil,
-			},
-			{
-				ID: eth.BlockID{
-					Number: 7,
-					Hash:   common.HexToHash("0x77"),
-				},
-				Batch: nil,
-			},
-		},
-		expResponse: createResponse(
-			testPrevBlockID,
-			eth.BlockID{
-				Number: 7,
-				Hash:   common.HexToHash("0x77"),
-			}, nil,
-		),
-	},
-	{
-		name: "last block is non empty",
-		candidates: []node.BundleCandidate{
-			{
-				ID: eth.BlockID{
-					Number: 6,
-					Hash:   common.HexToHash("0x66"),
-				},
-				Batch: nil,
-			},
-			{
-				ID: eth.BlockID{
-					Number: 7,
-					Hash:   common.HexToHash("0x77"),
-				},
-				Batch: &derive.BatchData{},
-			},
-		},
-		expResponse: createResponse(
-			testPrevBlockID,
-			eth.BlockID{
-				Number: 6,
-				Hash:   common.HexToHash("0x66"),
-			}, nil,
-		),
-	},
-	{
-		name: "non empty block followed by empty block",
-		candidates: []node.BundleCandidate{
-			{
-				ID: eth.BlockID{
-					Number: 6,
-					Hash:   common.HexToHash("0x66"),
-				},
-				Batch: nil,
-			},
-			{
-				ID: eth.BlockID{
-					Number: 7,
-					Hash:   common.HexToHash("0x77"),
-				},
-				Batch: &derive.BatchData{},
-			},
-			{
-				ID: eth.BlockID{
-					Number: 8,
-					Hash:   common.HexToHash("0x88"),
-				},
-				Batch: nil,
-			},
-		},
-		expResponse: createResponse(
-			testPrevBlockID,
-			eth.BlockID{
-				Number: 6,
-				Hash:   common.HexToHash("0x66"),
-			}, nil,
-		),
-	},
-}
-
-// TestBundleBuilderPruneLastNonEmpty asserts that pruning the BundleBuilder
-// always removes the last non-empty block, if one exists, and any subsequent
-// empty blocks.
-func TestBundleBuilderPruneLastNonEmpty(t *testing.T) {
-	for _, test := range pruneLastNonEmptyTests {
-		t.Run(test.name, test.run)
-	}
-}
-
-type pruneLastNonEmptyTestCase struct {
-	name        string
-	candidates  []node.BundleCandidate
-	expResponse *node.BatchBundleResponse
-}
-
-func (tc *pruneLastNonEmptyTestCase) run(t *testing.T) {
-	builder := node.NewBundleBuilder(testPrevBlockID)
-	for _, candidate := range tc.candidates {
-		builder.AddCandidate(candidate)
-	}
-
-	builder.PruneLastNonEmpty()
-	require.Equal(t, tc.expResponse, builder.Response(nil))
-}