Merge pull request #39 from EspressoSystems/test/backwards-hotshot-bl…

…ocks

Handle the case where the L1 block number from HotShot decreases

op-node/rollup/driver/sequencer.go

@@ -170,6 +170,13 @@ func (d *Sequencer) startBuildingEspressoBatch(ctx context.Context, l2Head eth.L
 
 	// Fetch the L1 origin determined by the first Espresso block.
 	l1OriginNumber := batch.jst.FirstBlock.L1Block.Number
+	// While Espresso _should_ guarantee that L1 origin numbers are monotonically increasing, a
+	// limitation in the current design means that on rare occasions the L1 origin number can
+	// decrease. As a temporary work-around, we detect this case and handle it as if Espresso had
+	// simply chosen the same L1 origin as the previous block.
+	if l1OriginNumber < l2Head.L1Origin.Number {
+		l1OriginNumber = l2Head.L1Origin.Number
+	}
 	batch.l1Origin, err = d.l1OriginSelector.FindL1OriginByNumber(ctx, l1OriginNumber)
 	if err != nil {
 		d.log.Error("Error finding L1 origin with number", l1OriginNumber, "err", err)
@@ -220,10 +227,14 @@ func (d *Sequencer) updateEspressoBatch(ctx context.Context, newHeaders []espres
 			return derive.NewCriticalError(fmt.Errorf("inconsistent data from Espresso query service: header %v in window has timestamp after window end %d", header, batch.windowEnd))
 		}
 		if header.Timestamp < batch.windowStart {
-			return derive.NewCriticalError(fmt.Errorf("inconsistent data from Espresso query service: header %v is before window start %d", header, batch.windowStart))
+			// Eventually, we should return an error here. However due to a limitation in the
+			// current implementation of HotShot/Espresso, block timestamps will sometimes decrease.
+			d.log.Error("inconsistent data from Espresso query service: header", header, "is before window start", batch.windowStart)
 		}
 		if header.Timestamp < batch.jst.Payload.LastBlock.Timestamp {
-			return derive.NewCriticalError(fmt.Errorf("inconsistent data from Espresso query service: header %v is before its predecessor %v", header, batch.jst.Payload.LastBlock))
+			// Similarly, this should eventually be an error, but can happen with the current
+			// version of Espresso.
+			d.log.Error("inconsistent data from Espresso query service: header", header, "is before its predecessor", batch.jst.Payload.LastBlock)
 		}
 
 		txs, err := d.espresso.FetchTransactionsInBlock(ctx, batch.jst.FirstBlockNumber+uint64(len(batch.blocks)), header, d.config.L2ChainID.Uint64())

op-node/rollup/driver/sequencer_test.go

@@ -416,11 +416,18 @@ func (s *TestSequencer) nextEspressoBlock(ctx context.Context) *espresso.Header
 	binary.LittleEndian.PutUint64(root.Root, uint64(len(s.espresso.Blocks)))
 
 	l1OriginNumber := prev.L1Block.Number
-	switch s.rng.Intn(10) {
+	switch s.rng.Intn(20) {
 	case 0:
+		// 5%: move the L1 origin _backwards_. Espresso is supposed to enforce that the L1 origin is
+		// monotonically increasing, but due to limitations in the current version of the HotShot
+		// interfaces, the current version does not, and the L1 block number will, rarely, decrease.
+		if l1OriginNumber > 0 {
+			l1OriginNumber -= 1
+		}
+	case 1, 2:
 		// 10%: advance L1 origin
 		l1OriginNumber += 1
-	case 1:
+	case 3, 4:
 		// 10%: skip ahead to the latest possible L1 origin
 		for {
 			l1Origin := s.l1BlockByNumber(l1OriginNumber + 1)
@@ -441,6 +448,12 @@ func (s *TestSequencer) nextEspressoBlock(ctx context.Context) *espresso.Header
 		l1Origin = s.l1BlockByNumber(prev.L1Block.Number)
 	}
 
+	// 5% of the time, mess with the timestamp. Again, Espresso should ensure that the timestamps
+	// are monotonically increasing, but for now, it doesn't.
+	if prev.Timestamp > 0 && s.rng.Intn(20) == 0 {
+		timestamp = prev.Timestamp - 1
+	}
+
 	header := espresso.Header{
 		Timestamp: timestamp,
 		L1Block: espresso.L1BlockInfo{
@@ -663,7 +676,7 @@ func TestSequencerChaosMonkeyEspresso(t *testing.T) {
 	// batch. This problem is exacerbated with the chaos monkey, since it may take even more wall
 	// clock time to fetch that last Espresso block due to injected errors.
 	l2Head := s.engControl.UnsafeL2Head()
-	require.Less(t, s.clockTime.Sub(time.Unix(int64(l2Head.Time), 0)).Abs(), 4*time.Second, "L2 time is accurate, within 4 seconds of wallclock")
+	require.Less(t, s.clockTime.Sub(time.Unix(int64(l2Head.Time), 0)).Abs(), 12*time.Second, "L2 time is accurate, within 12 seconds of wallclock")
 	// Here, the legacy test checks `avgBuildingTime()`. This stat is meaningless for the Espresso
 	// mode sequencer, since it builds blocks locally rather than in the engine.
 
@@ -674,6 +687,7 @@ func TestSequencerChaosMonkeyEspresso(t *testing.T) {
 	noEspressoBlocks := 0
 	oldL1Origin := 0
 	skippedL1Origin := 0
+	decreasingL1Origin := 0
 	for i := range s.engControl.l2Batches {
 		payload := s.engControl.l2Batches[i]
 
@@ -696,6 +710,10 @@ func TestSequencerChaosMonkeyEspresso(t *testing.T) {
 		jst := info.Justification
 		l1Origin := info.Number
 
+		// Check the basic L1 origin invariants.
+		require.GreaterOrEqual(t, l1Origin, prevL1Origin)
+		require.LessOrEqual(t, l1Origin, prevL1Origin+1)
+
 		// Check that the headers defining the start of the included block range match the output of
 		// the Espresso Sequencer.
 		require.Equal(t, jst.FirstBlock.Commit(), s.espresso.Blocks[jst.FirstBlockNumber].Header.Commit())
@@ -727,8 +745,14 @@ func TestSequencerChaosMonkeyEspresso(t *testing.T) {
 				}
 				offset += len(s.espresso.Blocks[block].Transactions)
 			}
-			// Check that the L1 origin was chosen by Espresso.
-			require.Equal(t, l1Origin, s.espresso.Blocks[jst.FirstBlockNumber].Header.L1Block.Number)
+			// Check that the L1 origin was chosen by Espresso, unless Espresso chose a bad
+			// (decreasing) L1 origin, in which case the sequencer just uses the previous L1 origin.
+			if s.espresso.Blocks[jst.FirstBlockNumber].Header.L1Block.Number < prevL1Origin {
+				decreasingL1Origin += 1
+				require.Equal(t, l1Origin, prevL1Origin)
+			} else {
+				require.Equal(t, l1Origin, s.espresso.Blocks[jst.FirstBlockNumber].Header.L1Block.Number)
+			}
 			prevL1Origin = l1Origin
 			happyPath += 1
 			continue
@@ -770,8 +794,9 @@ func TestSequencerChaosMonkeyEspresso(t *testing.T) {
 	}
 
 	t.Logf("Espresso sequencing case coverage:")
-	t.Logf("Happy path:         %d", happyPath)
-	t.Logf("No Espresso blocks: %d", noEspressoBlocks)
-	t.Logf("Old L1 origin:      %d", oldL1Origin)
-	t.Logf("Skipped L1 origin:  %d", skippedL1Origin)
+	t.Logf("Happy path:           %d", happyPath)
+	t.Logf("No Espresso blocks:   %d", noEspressoBlocks)
+	t.Logf("Old L1 origin:        %d", oldL1Origin)
+	t.Logf("Skipped L1 origin:    %d", skippedL1Origin)
+	t.Logf("Decreasing L1 origin: %d", decreasingL1Origin)
 }