op-node: disallow empty span batches, implement review suggestions

op-node/rollup/derive/batch_queue.go

@@ -70,7 +70,12 @@ func (bq *BatchQueue) Origin() eth.L1BlockRef {
 	return bq.prev.Origin()
 }
 
+// popNextBatch pops the next batch from the current queued up span-batch nextSpan.
+// The queue must be non-empty, or the function will panic.
 func (bq *BatchQueue) popNextBatch(safeL2Head eth.L2BlockRef) *SingularBatch {
+	if len(bq.nextSpan) == 0 {
+		panic("popping non-existent span-batch, invalid state")
+	}
 	nextBatch := bq.nextSpan[0]
 	bq.nextSpan = bq.nextSpan[1:]
 	// Must set ParentHash before return. we can use safeL2Head because the parentCheck is verified in CheckBatch().
@@ -78,7 +83,7 @@ func (bq *BatchQueue) popNextBatch(safeL2Head eth.L2BlockRef) *SingularBatch {
 	return nextBatch
 }
 
-func (bq *BatchQueue) advanceEpochMaybe(nextBatch *SingularBatch) {
+func (bq *BatchQueue) maybeAdvanceEpoch(nextBatch *SingularBatch) {
 	if len(bq.l1Blocks) == 0 {
 		return
 	}
@@ -92,7 +97,7 @@ func (bq *BatchQueue) NextBatch(ctx context.Context, safeL2Head eth.L2BlockRef)
 	if len(bq.nextSpan) > 0 {
 		// If there are cached singular batches, pop first one and return.
 		nextBatch := bq.popNextBatch(safeL2Head)
-		bq.advanceEpochMaybe(nextBatch)
+		bq.maybeAdvanceEpoch(nextBatch)
 		return nextBatch, nil
 	}
 
@@ -167,12 +172,13 @@ func (bq *BatchQueue) NextBatch(ctx context.Context, safeL2Head eth.L2BlockRef)
 			return nil, NewCriticalError(err)
 		}
 		bq.nextSpan = singularBatches
+		// span-batches are non-empty, so the below pop is safe.
 		nextBatch = bq.popNextBatch(safeL2Head)
 	default:
 		return nil, NewCriticalError(fmt.Errorf("unrecognized batch type: %d", batch.GetBatchType()))
 	}
 
-	bq.advanceEpochMaybe(nextBatch)
+	bq.maybeAdvanceEpoch(nextBatch)
 	return nextBatch, nil
 }
 

op-node/rollup/derive/span_batch.go

@@ -27,6 +27,8 @@ import (
 
 var ErrTooBigSpanBatchFieldSize = errors.New("batch would cause field bytes to go over limit")
 
+var ErrEmptySpanBatch = errors.New("span-batch must not be empty")
+
 type spanBatchPrefix struct {
 	relTimestamp  uint64 // Relative timestamp of the first block
 	l1OriginNum   uint64 // L1 origin number
@@ -139,10 +141,13 @@ func (bp *spanBatchPrefix) decodePrefix(r *bytes.Reader) error {
 // decodeBlockCount parses data into bp.blockCount
 func (bp *spanBatchPayload) decodeBlockCount(r *bytes.Reader) error {
 	blockCount, err := binary.ReadUvarint(r)
-	bp.blockCount = blockCount
 	if err != nil {
 		return fmt.Errorf("failed to read block count: %w", err)
 	}
+	bp.blockCount = blockCount
+	if blockCount == 0 {
+		return ErrEmptySpanBatch
+	}
 	return nil
 }
 
@@ -362,6 +367,9 @@ func (b *RawSpanBatch) encodeBytes() ([]byte, error) {
 // derive converts RawSpanBatch into SpanBatch, which has a list of spanBatchElement.
 // We need chain config constants to derive values for making payload attributes.
 func (b *RawSpanBatch) derive(blockTime, genesisTimestamp uint64, chainID *big.Int) (*SpanBatch, error) {
+	if b.blockCount == 0 {
+		return nil, ErrEmptySpanBatch
+	}
 	blockOriginNums := make([]uint64, b.blockCount)
 	l1OriginBlockNumber := b.l1OriginNum
 	for i := int(b.blockCount) - 1; i >= 0; i-- {

op-node/rollup/derive/span_batch_test.go

@@ -6,12 +6,15 @@ import (
 	"math/rand"
 	"testing"
 
-	"github.com/ethereum-optimism/optimism/op-node/rollup"
-	"github.com/ethereum-optimism/optimism/op-service/testutils"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/rlp"
-	"github.com/stretchr/testify/assert"
+
+	"github.com/ethereum-optimism/optimism/op-node/rollup"
+	"github.com/ethereum-optimism/optimism/op-service/testutils"
 )
 
 func TestSpanBatchForBatchInterface(t *testing.T) {
@@ -33,6 +36,39 @@ func TestSpanBatchForBatchInterface(t *testing.T) {
 	assert.True(t, spanBatch.CheckParentHash(singularBatches[0].ParentHash))
 }
 
+func TestEmptySpanBatch(t *testing.T) {
+	rng := rand.New(rand.NewSource(0x77556691))
+	chainID := big.NewInt(rng.Int63n(1000))
+	spanTxs, err := newSpanBatchTxs(nil, chainID)
+	require.NoError(t, err)
+
+	rawSpanBatch := RawSpanBatch{
+		spanBatchPrefix: spanBatchPrefix{
+			relTimestamp:  uint64(rng.Uint32()),
+			l1OriginNum:   rng.Uint64(),
+			parentCheck:   testutils.RandomData(rng, 20),
+			l1OriginCheck: testutils.RandomData(rng, 20),
+		},
+		spanBatchPayload: spanBatchPayload{
+			blockCount:    0,
+			originBits:    big.NewInt(0),
+			blockTxCounts: []uint64{},
+			txs:           spanTxs,
+		},
+	}
+
+	var buf bytes.Buffer
+	err = rawSpanBatch.encodeBlockCount(&buf)
+	assert.NoError(t, err)
+
+	result := buf.Bytes()
+	r := bytes.NewReader(result)
+	var sb RawSpanBatch
+
+	err = sb.decodeBlockCount(r)
+	require.ErrorIs(t, err, ErrEmptySpanBatch)
+}
+
 func TestSpanBatchOriginBits(t *testing.T) {
 	rng := rand.New(rand.NewSource(0x77665544))
 	chainID := big.NewInt(rng.Int63n(1000))