op-node: Fix invalid epoch advance

This includes an op-e2e action test which can be used as a regression test
against the invalid epoch advancement. I also had to fix several other tests.

This bug is caused by the batch queue advancing to the next epoch prior to
having the full sequence window for that epoch. This occurs when auto generating
batches. Once it has generated all of the batches for the current epoch, it
creates the first batch of the next epoch before it should.

Note: I had to remove the assumption that the epoch of the safe head is
the same as the epoch inside the batch queue. This is not true when generating
the last batch of an epoch. We can advance the batch queue, but we don't
generate a safehead with the new origin.

op-e2e/actions/blocktime_test.go

@@ -12,6 +12,108 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// TestBatchInLastPossibleBlocks tests that the derivation pipeline
+// accepts a batch that is included in the last possible L1 block
+// where there are also no other batches included in the sequence
+// window.
+// This is a regression test against the bug fixed in PR #4566
+func TestBatchInLastPossibleBlocks(gt *testing.T) {
+	t := NewDefaultTesting(gt)
+	dp := e2eutils.MakeDeployParams(t, defaultRollupTestParams)
+	dp.DeployConfig.SequencerWindowSize = 4
+	sd := e2eutils.Setup(t, dp, defaultAlloc)
+	log := testlog.Logger(t, log.LvlDebug)
+
+	sd, miner, sequencer, sequencerEngine, _, _, batcher := setupReorgTestActors(t, dp, sd, log)
+
+	signer := types.LatestSigner(sd.L2Cfg.Config)
+	cl := sequencerEngine.EthClient()
+	aliceTx := func() {
+		n, err := cl.PendingNonceAt(t.Ctx(), dp.Addresses.Alice)
+		require.NoError(t, err)
+		tx := types.MustSignNewTx(dp.Secrets.Alice, signer, &types.DynamicFeeTx{
+			ChainID:   sd.L2Cfg.Config.ChainID,
+			Nonce:     n,
+			GasTipCap: big.NewInt(2 * params.GWei),
+			GasFeeCap: new(big.Int).Add(miner.l1Chain.CurrentBlock().BaseFee(), big.NewInt(2*params.GWei)),
+			Gas:       params.TxGas,
+			To:        &dp.Addresses.Bob,
+			Value:     e2eutils.Ether(2),
+		})
+		require.NoError(gt, cl.SendTransaction(t.Ctx(), tx))
+	}
+	makeL2BlockWithAliceTx := func() {
+		aliceTx()
+		sequencer.ActL2StartBlock(t)
+		sequencerEngine.ActL2IncludeTx(dp.Addresses.Alice)(t) // include a test tx from alice
+		sequencer.ActL2EndBlock(t)
+	}
+	verifyChainStateOnSequencer := func(l1Number, unsafeHead, unsafeHeadOrigin, safeHead, safeHeadOrigin uint64) {
+		require.Equal(t, l1Number, miner.l1Chain.CurrentHeader().Number.Uint64())
+		require.Equal(t, unsafeHead, sequencer.L2Unsafe().Number)
+		require.Equal(t, unsafeHeadOrigin, sequencer.L2Unsafe().L1Origin.Number)
+		require.Equal(t, safeHead, sequencer.L2Safe().Number)
+		require.Equal(t, safeHeadOrigin, sequencer.L2Safe().L1Origin.Number)
+	}
+
+	// Make 8 L1 blocks & 17 L2 blocks.
+	miner.ActL1StartBlock(4)(t)
+	miner.ActL1EndBlock(t)
+	sequencer.ActL1HeadSignal(t)
+	sequencer.ActL2PipelineFull(t)
+	makeL2BlockWithAliceTx()
+	makeL2BlockWithAliceTx()
+	makeL2BlockWithAliceTx()
+
+	for i := 0; i < 7; i++ {
+		batcher.ActSubmitAll(t)
+		miner.ActL1StartBlock(4)(t)
+		miner.ActL1IncludeTx(sd.RollupCfg.Genesis.SystemConfig.BatcherAddr)(t)
+		miner.ActL1EndBlock(t)
+		sequencer.ActL1HeadSignal(t)
+		sequencer.ActL2PipelineFull(t)
+		makeL2BlockWithAliceTx()
+		makeL2BlockWithAliceTx()
+	}
+
+	// 8 L1 blocks with 17 L2 blocks is the unsafe state.
+	// Because wew consistently batch submitted we are one epoch behind the unsafe head with the safe head
+	verifyChainStateOnSequencer(8, 17, 8, 15, 7)
+
+	// Create the batch for L2 blocks 16 & 17
+	batcher.ActSubmitAll(t)
+
+	// L1 Block 8 contains the batch for L2 blocks 14 & 15
+	// Then we create L1 blocks 9, 10, 11
+	// The L1 origin of L2 block 16 is L1 block 8
+	// At a seq window of 4, should be possible to include the batch for L2 block 16 & 17 at L1 block 12
+
+	// Make 3 more L1 + 6 L2 blocks
+	for i := 0; i < 3; i++ {
+		miner.ActL1StartBlock(4)(t)
+		miner.ActL1EndBlock(t)
+		sequencer.ActL1HeadSignal(t)
+		sequencer.ActL2PipelineFull(t)
+		makeL2BlockWithAliceTx()
+		makeL2BlockWithAliceTx()
+	}
+
+	// At this point verify that we have not started auto generating blocks
+	// by checking that L1 & the unsafe head have advanced as expected, but the safe head is the same.
+	verifyChainStateOnSequencer(11, 23, 11, 15, 7)
+
+	// Check that the batch can go in on the last block of the sequence window
+	miner.ActL1StartBlock(4)(t)
+	miner.ActL1IncludeTx(sd.RollupCfg.Genesis.SystemConfig.BatcherAddr)(t)
+	miner.ActL1EndBlock(t)
+	sequencer.ActL1HeadSignal(t)
+	sequencer.ActL2PipelineFull(t)
+
+	// We have one more L1 block, no more unsafe blocks, but advance one
+	// epoch on the safe head with the submitted batches
+	verifyChainStateOnSequencer(12, 23, 11, 17, 8)
+}
+
 // TestLargeL1Gaps tests the case that there is a gap between two L1 blocks which
 // is larger than the sequencer drift.
 // This test has the following parameters:

op-e2e/actions/l2_batcher_test.go

@@ -72,7 +72,8 @@ func TestBatcher(gt *testing.T) {
 	log.Info("bl", "txs", len(bl.Transactions()))
 
 	// Now make enough L1 blocks that the verifier will have to derive a L2 block
-	for i := uint64(1); i < sd.RollupCfg.SeqWindowSize; i++ {
+	// It will also eagerly derive the block from the batcher
+	for i := uint64(0); i < sd.RollupCfg.SeqWindowSize; i++ {
 		miner.ActL1StartBlock(12)(t)
 		miner.ActL1EndBlock(t)
 	}

op-e2e/actions/l2_verifier_test.go

@@ -51,7 +51,7 @@ func TestL2Verifier_SequenceWindow(gt *testing.T) {
 		expectedL1Origin := uint64(0)
 		// as soon as we complete the sequence window, we force-adopt the L1 origin
 		if l1Head >= sd.RollupCfg.SeqWindowSize {
-			expectedL1Origin = l1Head - sd.RollupCfg.SeqWindowSize + 1
+			expectedL1Origin = l1Head - sd.RollupCfg.SeqWindowSize
 		}
 		require.Equal(t, expectedL1Origin, verifier.SyncStatus().SafeL2.L1Origin.Number, "L1 origin is forced in, given enough L1 blocks pass by")
 		require.LessOrEqual(t, miner.l1Chain.GetBlockByNumber(expectedL1Origin).Time(), engine.l2Chain.CurrentBlock().Time(), "L2 time higher than L1 origin time")
@@ -87,7 +87,6 @@ func TestL2Verifier_SequenceWindow(gt *testing.T) {
 	// Now it will reorg
 	verifier.ActL2PipelineFull(t)
 
-	// due to workaround we synced one more L1 block, so we need to compare against the parent of that
-	got := miner.l1Chain.GetBlockByHash(miner.l1Chain.GetBlockByHash(verifier.SyncStatus().SafeL2.L1Origin.Hash).ParentHash())
+	got := miner.l1Chain.GetBlockByHash(miner.l1Chain.GetBlockByHash(verifier.SyncStatus().SafeL2.L1Origin.Hash).Hash())
 	require.Equal(t, reorgL1Block.Hash(), got.Hash(), "must have reorged L2 chain to the new L1 chain")
 }

op-node/rollup/derive/batch_queue.go

@@ -79,7 +79,7 @@ func (bq *BatchQueue) NextBatch(ctx context.Context, safeL2Head eth.L2BlockRef)
 			// originBehind is false.
 			bq.l1Blocks = bq.l1Blocks[:0]
 		}
-		bq.log.Info("Advancing bq origin", "origin", bq.origin)
+		bq.log.Info("Advancing bq origin", "origin", bq.origin, "originBehind", originBehind)
 	}
 
 	// Load more data into the batch queue
@@ -151,8 +151,12 @@ func (bq *BatchQueue) deriveNextBatch(ctx context.Context, outOfData bool, l2Saf
 		return nil, NewCriticalError(errors.New("cannot derive next batch, no origin was prepared"))
 	}
 	epoch := bq.l1Blocks[0]
+	bq.log.Trace("Deriving the next batch", "epoch", epoch, "l2SafeHead", l2SafeHead, "outOfData", outOfData)
 
-	if l2SafeHead.L1Origin != epoch.ID() {
+	// Note: epoch origin can now be one block ahead of the L2 Safe Head
+	// This is in the case where we auto generate all batches in an epoch & advance the epoch
+	// but don't advance the L2 Safe Head's epoch
+	if l2SafeHead.L1Origin != epoch.ID() && l2SafeHead.L1Origin.Number != epoch.Number-1 {
 		return nil, NewResetError(fmt.Errorf("buffered L1 chain epoch %s in batch queue does not match safe head origin %s", epoch, l2SafeHead.L1Origin))
 	}
 
@@ -208,17 +212,16 @@ batchLoop:
 		if nextBatch.Batch.EpochNum == rollup.Epoch(epoch.Number)+1 {
 			bq.l1Blocks = bq.l1Blocks[1:]
 		}
+		bq.log.Trace("Returning found batch", "epoch", epoch, "batch_epoch", nextBatch.Batch.EpochNum, "batch_timestamp", nextBatch.Batch.Timestamp)
 		return nextBatch.Batch, nil
 	}
 
 	// If the current epoch is too old compared to the L1 block we are at,
-	// i.e. if the sequence window expired, we create empty batches
+	// i.e. if the sequence window expired, we create empty batches for the current epoch
 	expiryEpoch := epoch.Number + bq.config.SeqWindowSize
-	forceNextEpoch :=
-		(expiryEpoch == bq.origin.Number && outOfData) ||
-			expiryEpoch < bq.origin.Number
+	forceEmptyBatches := (expiryEpoch == bq.origin.Number && outOfData) || expiryEpoch < bq.origin.Number
 
-	if !forceNextEpoch {
+	if !forceEmptyBatches {
 		// sequence window did not expire yet, still room to receive batches for the current epoch,
 		// no need to force-create empty batch(es) towards the next epoch yet.
 		return nil, io.EOF
@@ -242,15 +245,9 @@ batchLoop:
 			},
 		}, nil
 	}
-	// As we move the safe head origin forward, we also drop the old L1 block reference
+
+	// At this point we have auto generated every batch for the current epoch
+	// that we can, so we can advance to the next epoch.
 	bq.l1Blocks = bq.l1Blocks[1:]
-	return &BatchData{
-		BatchV1{
-			ParentHash:   l2SafeHead.Hash,
-			EpochNum:     rollup.Epoch(nextEpoch.Number),
-			EpochHash:    nextEpoch.Hash,
-			Timestamp:    nextTimestamp,
-			Transactions: nil,
-		},
-	}, nil
+	return nil, io.EOF
 }

op-node/rollup/derive/batch_queue_test.go

@@ -185,7 +185,7 @@ func TestBatchQueueEager(t *testing.T) {
 
 func TestBatchQueueMissing(t *testing.T) {
 	log := testlog.Logger(t, log.LvlCrit)
-	l1 := L1Chain([]uint64{10, 15, 20})
+	l1 := L1Chain([]uint64{10, 15, 20, 25})
 	safeHead := eth.L2BlockRef{
 		Hash:           mockHash(10, 2),
 		Number:         0,
@@ -239,6 +239,7 @@ func TestBatchQueueMissing(t *testing.T) {
 	require.Nil(t, e)
 	require.Equal(t, b.Timestamp, uint64(12))
 	require.Empty(t, b.BatchV1.Transactions)
+	require.Equal(t, rollup.Epoch(0), b.EpochNum)
 	safeHead.Number += 1
 	safeHead.Time += 2
 	safeHead.Hash = mockHash(b.Timestamp, 2)
@@ -248,6 +249,7 @@ func TestBatchQueueMissing(t *testing.T) {
 	require.Nil(t, e)
 	require.Equal(t, b.Timestamp, uint64(14))
 	require.Empty(t, b.BatchV1.Transactions)
+	require.Equal(t, rollup.Epoch(0), b.EpochNum)
 	safeHead.Number += 1
 	safeHead.Time += 2
 	safeHead.Hash = mockHash(b.Timestamp, 2)
@@ -256,11 +258,19 @@ func TestBatchQueueMissing(t *testing.T) {
 	b, e = bq.NextBatch(context.Background(), safeHead)
 	require.Nil(t, e)
 	require.Equal(t, b, batches[0])
+	require.Equal(t, rollup.Epoch(0), b.EpochNum)
 	safeHead.Number += 1
 	safeHead.Time += 2
 	safeHead.Hash = mockHash(b.Timestamp, 2)
 
+	// Advance the origin. At this point the batch with timestamp 18 will be created
+	input.origin = l1[3]
+
 	// Check for the generated batch at t = 18. This batch advances the epoch
+	// Note: We need one io.EOF returned from the bq that advances the internal L1 Blocks view
+	// before the batch will be auto generated
+	_, e = bq.NextBatch(context.Background(), safeHead)
+	require.Equal(t, e, io.EOF)
 	b, e = bq.NextBatch(context.Background(), safeHead)
 	require.Nil(t, e)
 	require.Equal(t, b.Timestamp, uint64(18))

op-node/rollup/derive/batches.go

@@ -43,11 +43,6 @@ func CheckBatch(cfg *rollup.Config, log log.Logger, l1Blocks []eth.L1BlockRef, l
 		return BatchUndecided
 	}
 	epoch := l1Blocks[0]
-	if epoch.Hash != l2SafeHead.L1Origin.Hash {
-		log.Warn("safe L2 head L1 origin does not match batch first l1 block (current epoch)",
-			"safe_l2", l2SafeHead, "safe_origin", l2SafeHead.L1Origin, "epoch", epoch)
-		return BatchUndecided
-	}
 
 	nextTimestamp := l2SafeHead.Time + cfg.BlockTime
 	if batch.Batch.Timestamp > nextTimestamp {

op-node/rollup/derive/batches_test.go

@@ -168,22 +168,6 @@ func TestValidBatch(t *testing.T) {
 			},
 			Expected: BatchUndecided,
 		},
-		{
-			Name:       "inconsistent L1 info",
-			L1Blocks:   []eth.L1BlockRef{l1B},
-			L2SafeHead: l2A0,
-			Batch: BatchWithL1InclusionBlock{
-				L1InclusionBlock: l1B,
-				Batch: &BatchData{BatchV1{
-					ParentHash:   l2A1.ParentHash,
-					EpochNum:     rollup.Epoch(l2A1.L1Origin.Number),
-					EpochHash:    l2A1.L1Origin.Hash,
-					Timestamp:    l2A1.Time,
-					Transactions: nil,
-				}},
-			},
-			Expected: BatchUndecided,
-		},
 		{
 			Name:       "future timestamp",
 			L1Blocks:   []eth.L1BlockRef{l1A, l1B, l1C},