Merge pull request #4809 from ethereum-optimism/fix-derivation-origin-selection

op-node: handle when L1 time gap is larger than sequencer time drift

op-node/rollup/derive/batches.go

@@ -100,11 +100,31 @@ func CheckBatch(cfg *rollup.Config, log log.Logger, l1Blocks []eth.L1BlockRef, l
 		return BatchDrop
 	}
 
-	// If we ran out of sequencer time drift, then we drop the batch and produce an empty batch instead,
-	// as the sequencer is not allowed to include anything past this point without moving to the next epoch.
+	// Check if we ran out of sequencer time drift
 	if max := batchOrigin.Time + cfg.MaxSequencerDrift; batch.Batch.Timestamp > max {
-		log.Warn("batch exceeded sequencer time drift, sequencer must adopt new L1 origin to include transactions again", "max_time", max)
-		return BatchDrop
+		if len(batch.Batch.Transactions) == 0 {
+			// If the sequencer is co-operating by producing an empty batch,
+			// then allow the batch if it was the right thing to do to maintain the L2 time >= L1 time invariant.
+			// We only check batches that do not advance the epoch, to ensure epoch advancement regardless of time drift is allowed.
+			if epoch.Number == batchOrigin.Number {
+				if len(l1Blocks) < 2 {
+					log.Info("without the next L1 origin we cannot determine yet if this empty batch that exceeds the time drift is still valid")
+					return BatchUndecided
+				}
+				nextOrigin := l1Blocks[1]
+				if batch.Batch.Timestamp >= nextOrigin.Time { // check if the next L1 origin could have been adopted
+					log.Info("batch exceeded sequencer time drift without adopting next origin, and next L1 origin would have been valid")
+					return BatchDrop
+				} else {
+					log.Info("continuing with empty batch before late L1 block to preserve L2 time invariant")
+				}
+			}
+		} else {
+			// If the sequencer is ignoring the time drift rule, then drop the batch and force an empty batch instead,
+			// as the sequencer is not allowed to include anything past this point without moving to the next epoch.
+			log.Warn("batch exceeded sequencer time drift, sequencer must adopt new L1 origin to include transactions again", "max_time", max)
+			return BatchDrop
+		}
 	}
 
 	// We can do this check earlier, but it's a more intensive one, so we do this last.

op-node/rollup/derive/batches_test.go

@@ -151,6 +151,22 @@ func TestValidBatch(t *testing.T) {
 		SequenceNumber: 0,
 	}
 
+	l2A4 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         l2A3.Number + 1,
+		ParentHash:     l2A3.Hash,
+		Time:           l2A3.Time + conf.BlockTime, // 4*2 = 8, higher than seq time drift
+		L1Origin:       l1A.ID(),
+		SequenceNumber: 4,
+	}
+
+	l1BLate := eth.L1BlockRef{
+		Hash:       testutils.RandomHash(rng),
+		Number:     l1A.Number + 1,
+		ParentHash: l1A.Hash,
+		Time:       l2A4.Time + 1, // too late for l2A4 to adopt yet
+	}
+
 	testCases := []ValidBatchTestCase{
 		{
 			Name:       "missing L1 info",
@@ -249,16 +265,16 @@ func TestValidBatch(t *testing.T) {
 			Expected: BatchDrop,
 		},
 		{
-			Name:       "epoch too old", // repeat of now outdated l2A3 data
+			Name:       "epoch too old, but good parent hash and timestamp", // repeat of now outdated l2A3 data
 			L1Blocks:   []eth.L1BlockRef{l1B, l1C, l1D},
 			L2SafeHead: l2B0, // we already moved on to B
 			Batch: BatchWithL1InclusionBlock{
 				L1InclusionBlock: l1C,
 				Batch: &BatchData{BatchV1{
-					ParentHash:   l2A3.ParentHash,
+					ParentHash:   l2B0.Hash,                          // build on top of safe head to continue
 					EpochNum:     rollup.Epoch(l2A3.L1Origin.Number), // epoch A is no longer valid
 					EpochHash:    l2A3.L1Origin.Hash,
-					Timestamp:    l2A3.Time,
+					Timestamp:    l2B0.Time + conf.BlockTime, // pass the timestamp check to get too epoch check
 					Transactions: nil,
 				}},
 			},
@@ -313,23 +329,23 @@ func TestValidBatch(t *testing.T) {
 			Expected: BatchDrop,
 		},
 		{
-			Name:       "sequencer time drift on same epoch",
+			Name:       "sequencer time drift on same epoch with non-empty txs",
 			L1Blocks:   []eth.L1BlockRef{l1A, l1B},
 			L2SafeHead: l2A3,
 			Batch: BatchWithL1InclusionBlock{
 				L1InclusionBlock: l1B,
 				Batch: &BatchData{BatchV1{ // we build l2A4, which has a timestamp of 2*4 = 8 higher than l2A0
-					ParentHash:   l2A3.Hash,
-					EpochNum:     rollup.Epoch(l2A3.L1Origin.Number),
-					EpochHash:    l2A3.L1Origin.Hash,
-					Timestamp:    l2A3.Time + conf.BlockTime,
-					Transactions: nil,
+					ParentHash:   l2A4.ParentHash,
+					EpochNum:     rollup.Epoch(l2A4.L1Origin.Number),
+					EpochHash:    l2A4.L1Origin.Hash,
+					Timestamp:    l2A4.Time,
+					Transactions: []hexutil.Bytes{[]byte("sequencer should not include this tx")},
 				}},
 			},
 			Expected: BatchDrop,
 		},
 		{
-			Name:       "sequencer time drift on changing epoch",
+			Name:       "sequencer time drift on changing epoch with non-empty txs",
 			L1Blocks:   []eth.L1BlockRef{l1X, l1Y, l1Z},
 			L2SafeHead: l2X0,
 			Batch: BatchWithL1InclusionBlock{
@@ -339,11 +355,75 @@ func TestValidBatch(t *testing.T) {
 					EpochNum:     rollup.Epoch(l2Y0.L1Origin.Number),
 					EpochHash:    l2Y0.L1Origin.Hash,
 					Timestamp:    l2Y0.Time, // valid, but more than 6 ahead of l1Y.Time
-					Transactions: nil,
+					Transactions: []hexutil.Bytes{[]byte("sequencer should not include this tx")},
 				}},
 			},
 			Expected: BatchDrop,
 		},
+		{
+			Name:       "sequencer time drift on same epoch with empty txs and late next epoch",
+			L1Blocks:   []eth.L1BlockRef{l1A, l1BLate},
+			L2SafeHead: l2A3,
+			Batch: BatchWithL1InclusionBlock{
+				L1InclusionBlock: l1BLate,
+				Batch: &BatchData{BatchV1{ // l2A4 time < l1BLate time, so we cannot adopt origin B yet
+					ParentHash:   l2A4.ParentHash,
+					EpochNum:     rollup.Epoch(l2A4.L1Origin.Number),
+					EpochHash:    l2A4.L1Origin.Hash,
+					Timestamp:    l2A4.Time,
+					Transactions: nil,
+				}},
+			},
+			Expected: BatchAccept, // accepted because empty & preserving L2 time invariant
+		},
+		{
+			Name:       "sequencer time drift on changing epoch with empty txs",
+			L1Blocks:   []eth.L1BlockRef{l1X, l1Y, l1Z},
+			L2SafeHead: l2X0,
+			Batch: BatchWithL1InclusionBlock{
+				L1InclusionBlock: l1Z,
+				Batch: &BatchData{BatchV1{
+					ParentHash:   l2Y0.ParentHash,
+					EpochNum:     rollup.Epoch(l2Y0.L1Origin.Number),
+					EpochHash:    l2Y0.L1Origin.Hash,
+					Timestamp:    l2Y0.Time, // valid, but more than 6 ahead of l1Y.Time
+					Transactions: nil,
+				}},
+			},
+			Expected: BatchAccept, // accepted because empty & still advancing epoch
+		},
+		{
+			Name:       "sequencer time drift on same epoch with empty txs and no next epoch in sight yet",
+			L1Blocks:   []eth.L1BlockRef{l1A},
+			L2SafeHead: l2A3,
+			Batch: BatchWithL1InclusionBlock{
+				L1InclusionBlock: l1B,
+				Batch: &BatchData{BatchV1{ // we build l2A4, which has a timestamp of 2*4 = 8 higher than l2A0
+					ParentHash:   l2A4.ParentHash,
+					EpochNum:     rollup.Epoch(l2A4.L1Origin.Number),
+					EpochHash:    l2A4.L1Origin.Hash,
+					Timestamp:    l2A4.Time,
+					Transactions: nil,
+				}},
+			},
+			Expected: BatchUndecided, // we have to wait till the next epoch is in sight to check the time
+		},
+		{
+			Name:       "sequencer time drift on same epoch with empty txs and but in-sight epoch that invalidates it",
+			L1Blocks:   []eth.L1BlockRef{l1A, l1B, l1C},
+			L2SafeHead: l2A3,
+			Batch: BatchWithL1InclusionBlock{
+				L1InclusionBlock: l1C,
+				Batch: &BatchData{BatchV1{ // we build l2A4, which has a timestamp of 2*4 = 8 higher than l2A0
+					ParentHash:   l2A4.ParentHash,
+					EpochNum:     rollup.Epoch(l2A4.L1Origin.Number),
+					EpochHash:    l2A4.L1Origin.Hash,
+					Timestamp:    l2A4.Time,
+					Transactions: nil,
+				}},
+			},
+			Expected: BatchDrop, // dropped because it could have advanced the epoch to B
+		},
 		{
 			Name:       "empty tx included",
 			L1Blocks:   []eth.L1BlockRef{l1A, l1B},

specs/derivation.md

@@ -146,6 +146,12 @@ The rationale is to maintain liveness in case of either a skipped slot on L1, or
 which requires longer epochs.
 Shorter epochs are then required to avoid L2 timestamps drifting further and further ahead of L1.
 
+Note that `min_l2_timestamp + l2_block_time` ensures that a new L2 batch can always be processed, even if the
+`max_sequencer_drift` is exceeded. However, when exceeding the `max_sequencer_drift`, progression to the next L1 origin
+is enforced, with an exception to ensure the minimum timestamp bound (based on this next L1 origin) can be met in the
+next L2 batch, and `len(batch.transactions) == 0` continues to be enforced while the `max_sequencer_drift` is exceeded.
+See [Batch Queue] for more details.
+
 ## Eager Block Derivation
 
 In practice, it is often not necessary to wait for a full sequencing window of L1 blocks in order to start deriving the
@@ -598,10 +604,18 @@ Rules, in validation order:
 - `batch.epoch_num > epoch.number+1` -> `drop`: i.e. the L1 origin cannot change by more than one L1 block per L2 block.
 - `batch.epoch_hash != batch_origin.hash` -> `drop`: i.e. a batch must reference a canonical L1 origin,
   to prevent batches from being replayed onto unexpected L1 chains.
-- `batch.timestamp > batch_origin.time + max_sequencer_drift` -> `drop`: i.e. a batch that does not adopt the next L1
-  within time will be dropped, in favor of an empty batch that can advance the L1 origin. This enforces the max L2
-  timestamp rule.
 - `batch.timestamp < batch_origin.time` -> `drop`: enforce the min L2 timestamp rule.
+- `batch.timestamp > batch_origin.time + max_sequencer_drift`: enforce the L2 timestamp drift rule,
+  but with exceptions to preserve above min L2 timestamp invariant:
+  - `len(batch.transactions) == 0`:
+    - `epoch.number == batch.epoch_num`:
+      this implies the batch does not already advance the L1 origin, and must thus be checked against `next_epoch`.
+      - If `next_epoch` is not known -> `undecided`:
+        without the next L1 origin we cannot yet determine if time invariant could have been kept.
+      - If `batch.timestamp >= next_epoch.time` -> `drop`:
+        the batch could have adopted the next L1 origin without breaking the `L2 time >= L1 time` invariant.
+  - `len(batch.transactions) > 0`: -> `drop`:
+    when exceeding the sequencer time drift, never allow the sequencer to include transactions.
 - `batch.transactions`: `drop` if the `batch.transactions` list contains a transaction
   that is invalid or derived by other means exclusively:
   - any transaction that is empty (zero length byte string)