Op-Node: Fix divergence due to bad reset logic

op-node/rollup/derive/batch_queue.go

@@ -89,29 +89,14 @@ func (bq *BatchQueue) Step(ctx context.Context, outer Progress) error {
 }
 
 func (bq *BatchQueue) ResetStep(ctx context.Context, l1Fetcher L1Fetcher) error {
-	// Reset such that the highestL1InclusionBlock is the same as the l2SafeHeadOrigin - the sequence window size
+	// Copy over the Origin the from the next stage
+	// It is set in the engine queue (two stages away) such that the L2 Safe Head origin is the progress
+	bq.progress = bq.next.Progress()
 	bq.batchesByTimestamp = make(map[uint64][]*BatchWithL1InclusionBlock)
+	// Include the new origin as an origin to build off of.
 	bq.l1Blocks = bq.l1Blocks[:0]
+	bq.l1Blocks = append(bq.l1Blocks, bq.progress.Origin)
 
-	startNumber := bq.next.Progress().Origin.Number
-	if startNumber < bq.config.SeqWindowSize {
-		startNumber = 0
-	} else {
-		startNumber -= bq.config.SeqWindowSize
-	}
-	// clip to genesis
-	if startNumber < bq.config.Genesis.L1.Number {
-		startNumber = bq.config.Genesis.L1.Number
-	}
-	l1BlockStart, err := l1Fetcher.L1BlockRefByNumber(ctx, startNumber)
-	if err != nil {
-		return err
-	}
-
-	bq.log.Info("found reset origin for batch queue", "origin", l1BlockStart)
-	bq.l1Blocks = append(bq.l1Blocks, l1BlockStart)
-	bq.progress.Origin = l1BlockStart
-	bq.progress.Closed = false
 	return io.EOF
 }
 

op-node/rollup/derive/engine_queue.go

@@ -2,15 +2,13 @@ package derive
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"time"
 
-	"github.com/ethereum/go-ethereum"
-
 	"github.com/ethereum-optimism/optimism/op-node/eth"
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
+	"github.com/ethereum-optimism/optimism/op-node/rollup/sync"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/log"
 )
@@ -37,8 +35,6 @@ type EngineQueue struct {
 	safeHead   eth.L2BlockRef
 	unsafeHead eth.L2BlockRef
 
-	resetting bool
-
 	toFinalize eth.BlockID
 
 	progress Progress
@@ -262,64 +258,32 @@ func (eq *EngineQueue) forceNextSafeAttributes(ctx context.Context) error {
 // ResetStep Walks the L2 chain backwards until it finds an L2 block whose L1 origin is canonical.
 // The unsafe head is set to the head of the L2 chain, unless the existing safe head is not canonical.
 func (eq *EngineQueue) ResetStep(ctx context.Context, l1Fetcher L1Fetcher) error {
-	if !eq.resetting {
-		eq.resetting = true
-
-		head, err := eq.engine.L2BlockRefHead(ctx)
-		if err != nil {
-			eq.log.Error("failed to get L2 engine head to start finding reset point from", "err", err)
-			return nil
-		}
-		eq.unsafeHead = head
-
-		// TODO: this should be different for safe head.
-		// We can't trust the origin data of the unsafe chain.
-		// We should query the engine for its current safe-head.
-		eq.safeHead = head
-		return nil
-	}
 
-	// check if the block origin is canonical
-	if canonicalRef, err := l1Fetcher.L1BlockRefByNumber(ctx, eq.safeHead.L1Origin.Number); errors.Is(err, ethereum.NotFound) {
-		// if our view of the l1 chain is lagging behind, we may get this error
-		eq.log.Warn("engine safe head is ahead of L1 view", "block", eq.safeHead, "origin", eq.safeHead.L1Origin)
-	} else if err != nil {
-		eq.log.Warn("failed to get L1 block ref to check if origin of l2 block is canonical", "err", err, "num", eq.safeHead.L1Origin.Number)
-	} else {
-		// if we find the safe head, then we found the canon chain
-		if canonicalRef.Hash == eq.safeHead.L1Origin.Hash {
-			eq.resetting = false
-			// if the unsafe head was broken, then restore it to start from the safe head
-			if eq.unsafeHead == (eth.L2BlockRef{}) {
-				eq.unsafeHead = eq.safeHead
-			}
-			eq.progress = Progress{
-				Origin: canonicalRef,
-				Closed: false,
-			}
-			if eq.safeHead.Time < canonicalRef.Time {
-				return fmt.Errorf("cannot reset block derivation to start at L2 block %s with time %d older than its L1 origin %s with time %d, time invariant is broken",
-					eq.safeHead, eq.safeHead.Time, canonicalRef, canonicalRef.Time)
-			}
-			return io.EOF
-		} else {
-			// if the safe head is not canonical, then the unsafe head will not be either
-			eq.unsafeHead = eth.L2BlockRef{}
-		}
+	l2Head, err := eq.engine.L2BlockRefHead(ctx)
+	if err != nil {
+		eq.log.Error("failed to find the L2 Head block", "err", err)
+		return nil
 	}
-
-	// Don't walk past genesis. If we were at the L2 genesis, but could not find its L1 origin,
-	// the L2 chain is building on the wrong L1 branch.
-	if eq.safeHead.Hash == eq.cfg.Genesis.L2.Hash || eq.safeHead.Number == eq.cfg.Genesis.L2.Number {
-		return fmt.Errorf("the L2 engine is coupled to unrecognized L1 chain: %v", eq.cfg.Genesis)
+	unsafe, safe, err := sync.FindL2Heads(ctx, l2Head, eq.cfg.SeqWindowSize, l1Fetcher, eq.engine, &eq.cfg.Genesis)
+	if err != nil {
+		eq.log.Error("failed to find the L2 Heads to start from", "err", err)
+		return nil
 	}
-
-	// Pull L2 parent for next iteration
-	block, err := eq.engine.L2BlockRefByHash(ctx, eq.safeHead.ParentHash)
+	l1Origin, err := l1Fetcher.L1BlockRefByHash(ctx, safe.L1Origin.Hash)
 	if err != nil {
-		eq.log.Error("failed to fetch L2 block by hash during reset", "parent", eq.safeHead.ParentHash, "err", err)
+		eq.log.Error("failed to fetch the new L1 progress", "err", err, "origin", safe.L1Origin)
 		return nil
 	}
-	eq.safeHead = block
-	return nil
+	if safe.Time < l1Origin.Time {
+		return fmt.Errorf("cannot reset block derivation to start at L2 block %s with time %d older than its L1 origin %s with time %d, time invariant is broken",
+			safe, safe.Time, l1Origin, l1Origin.Time)
+	}
+	eq.unsafeHead = unsafe
+	eq.safeHead = safe
+	eq.progress = Progress{
+		Origin: l1Origin,
+		Closed: false,
+	}
+	return io.EOF
+
 }

op-node/rollup/derive/pipeline.go

@@ -11,6 +11,7 @@ import (
 )
 
 type L1Fetcher interface {
+	L1HeadBlockRef(ctx context.Context) (eth.L1BlockRef, error)
 	L1BlockRefByNumberFetcher
 	L1BlockRefByHashFetcher
 	L1ReceiptsFetcher

op-node/rollup/sync/start.go

@@ -0,0 +1,201 @@
+// The sync package is responsible for reconciling L1 and L2.
+//
+// The Ethereum chain is a DAG of blocks with the root block being the genesis block. At any given
+// time, the head (or tip) of the chain can change if an offshoot/branch of the chain has a higher
+// total difficulty. This is known as a re-organization of the canonical chain. Each block points to
+// a parent block and the node is responsible for deciding which block is the head and thus the
+// mapping from block number to canonical block.
+//
+// The Optimism (L2) chain has similar properties, but also retains references to the Ethereum (L1)
+// chain. Each L2 block retains a reference to an L1 block (its "L1 origin", i.e. L1 block
+// associated with the epoch that the L2 block belongs to) and to its parent L2 block. The L2 chain
+// node must satisfy the following validity rules:
+//
+//     1. l2block.number == l2block.l2parent.block.number + 1
+//     2. l2block.l1Origin.number >= l2block.l2parent.l1Origin.number
+//     3. l2block.l1Origin is in the canonical chain on L1
+//     4. l1_rollup_genesis is an ancestor of l2block.l1Origin
+//
+// During normal operation, both the L1 and L2 canonical chains can change, due to a re-organisation
+// or due to an extension (new L1 or L2 block).
+//
+// When one of these changes occurs, the rollup node needs to determine what the new L2 head blocks
+// should be. We track two L2 head blocks:
+//
+//     - The *unsafe L2 block*: This is the highest L2 block whose L1 origin is a plausible (1)
+//       extension of the canonical L1 chain (as known to the op-node).
+//     - The *safe L2 block*: This is the highest L2 block whose epoch's sequencing window is
+//       complete within the canonical L1 chain (as known to the op-node).
+//
+// (1) Plausible meaning that the blockhash of the L2 block's L1 origin (as reported in the L1
+//     Attributes deposit within the L2 block) is not canonical at another height in the L1 chain,
+//     and the same holds for all its ancestors.
+//
+// In particular, in the case of L1 extension, the L2 unsafe head will generally remain the same,
+// but in the case of an L1 re-org, we need to search for the new safe and unsafe L2 block.
+package sync
+
+import (
+	"context"
+	"errors"
+	"fmt"
+
+	"github.com/ethereum/go-ethereum/common"
+
+	"github.com/ethereum-optimism/optimism/op-node/eth"
+	"github.com/ethereum-optimism/optimism/op-node/rollup"
+)
+
+type L1Chain interface {
+	L1HeadBlockRef(ctx context.Context) (eth.L1BlockRef, error)
+	L1BlockRefByNumber(ctx context.Context, number uint64) (eth.L1BlockRef, error)
+}
+
+type L2Chain interface {
+	L2BlockRefByHash(ctx context.Context, l2Hash common.Hash) (eth.L2BlockRef, error)
+}
+
+var WrongChainErr = errors.New("wrong chain")
+var TooDeepReorgErr = errors.New("reorg is too deep")
+
+const MaxReorgDepth = 500
+
+// isCanonical returns the following values:
+// - `aheadOrCanonical: true if the supplied block is ahead of the known head of the L1 chain,
+//    or canonical in the L1 chain.
+// - `canonical`: true if the block is canonical in the L1 chain.
+func isAheadOrCanonical(ctx context.Context, l1 L1Chain, block eth.BlockID) (aheadOrCanonical bool, canonical bool, err error) {
+	if l1Head, err := l1.L1HeadBlockRef(ctx); err != nil {
+		return false, false, err
+	} else if block.Number > l1Head.Number {
+		return true, false, nil
+	} else if canonical, err := l1.L1BlockRefByNumber(ctx, block.Number); err != nil {
+		return false, false, err
+	} else {
+		canonical := canonical.Hash == block.Hash
+		return canonical, canonical, nil
+	}
+}
+
+// FindL2Heads walks back from `start` (the previous unsafe L2 block) and finds the unsafe and safe
+// L2 blocks.
+//
+//     - The *unsafe L2 block*: This is the highest L2 block whose L1 origin is a plausible (1)
+//       extension of the canonical L1 chain (as known to the op-node).
+//     - The *safe L2 block*: This is the highest L2 block whose epoch's sequencing window is
+//       complete within the canonical L1 chain (as known to the op-node).
+//
+// (1) Plausible meaning that the blockhash of the L2 block's L1 origin (as reported in the L1
+//     Attributes deposit within the L2 block) is not canonical at another height in the L1 chain,
+//     and the same holds for all its ancestors.
+func FindL2Heads(ctx context.Context, start eth.L2BlockRef, seqWindowSize uint64,
+	l1 L1Chain, l2 L2Chain, genesis *rollup.Genesis) (unsafe eth.L2BlockRef, safe eth.L2BlockRef, err error) {
+
+	// Loop 1. Walk the L2 chain backwards until we find an L2 block whose L1 origin is canonical.
+
+	// Current L2 block.
+	n := start
+
+	// Number of blocks between n and start.
+	reorgDepth := 0
+
+	// Blockhash of L1 origin hash for the L2 block during the previous iteration, 0 for first
+	// iteration. When this changes as we walk the L2 chain backwards, it means we're seeing a different
+	// (earlier) epoch.
+	var prevL1OriginHash common.Hash
+
+	// The highest L2 ancestor of `start` (or `start` itself) whose ancestors are not (yet) known
+	// to have a non-canonical L1 origin. Empty if no such candidate is known yet. Guaranteed to be
+	// set after exiting from Loop 1.
+	var highestPlausibleCanonicalOrigin eth.L2BlockRef
+
+	for {
+		// Check if l1Origin is canonical when we get to a new epoch.
+		if prevL1OriginHash != n.L1Origin.Hash {
+			prevL1OriginHash = n.L1Origin.Hash
+
+			if plausible, canonical, err := isAheadOrCanonical(ctx, l1, n.L1Origin); err != nil {
+				return eth.L2BlockRef{}, eth.L2BlockRef{}, err
+			} else if !plausible {
+				// L1 origin nor ahead of L1 head nor canonical, discard previous candidate and
+				// keep looking.
+				highestPlausibleCanonicalOrigin = eth.L2BlockRef{}
+			} else {
+				if highestPlausibleCanonicalOrigin == (eth.L2BlockRef{}) {
+					// No highest plausible candidate, make L2 block new candidate.
+					highestPlausibleCanonicalOrigin = n
+				}
+				if canonical {
+					break
+				}
+			}
+		}
+
+		// Don't walk past genesis. If we were at the L2 genesis, but could not find its L1 origin,
+		// the L2 chain is building on the wrong L1 branch.
+		if n.Hash == genesis.L2.Hash || n.Number == genesis.L2.Number {
+			return eth.L2BlockRef{}, eth.L2BlockRef{}, WrongChainErr
+		}
+
+		// Pull L2 parent for next iteration
+		n, err = l2.L2BlockRefByHash(ctx, n.ParentHash)
+		if err != nil {
+			return eth.L2BlockRef{}, eth.L2BlockRef{},
+				fmt.Errorf("failed to fetch L2 block by hash %v: %w", n.ParentHash, err)
+		}
+
+		reorgDepth++
+		if reorgDepth >= MaxReorgDepth {
+			// If the reorg depth is too large, something is fishy.
+			// This can legitimately happen if L1 goes down for a while. But in that case,
+			// restarting the L2 node with a bigger configured MaxReorgDepth is an acceptable
+			// stopgap solution.
+			// Currently this can also happen if the L2 node is down for a while, but in the future
+			// state sync should prevent this issue.
+			return eth.L2BlockRef{}, eth.L2BlockRef{}, TooDeepReorgErr
+		}
+	}
+
+	// Loop 2. Walk from the L1 origin of the `n` block (*) back to the L1 block that starts the
+	// sequencing window ending at that block. Instead of iterating on L1 blocks, we actually
+	// iterate on L2 blocks, because we want to find the safe L2 head, i.e. the highest L2 block
+	// whose L1 origin is the start of the sequencing window.
+
+	// (*) `n` being at this stage the highest L2 block whose L1 origin is canonical.
+
+	// Depth counter: we need to walk back `seqWindowSize` L1 blocks in order to find the start
+	// of the sequencing window.
+	depth := uint64(1)
+
+	// Before entering the loop: `prevL1OriginHash == n.L1Origin.Hash`
+	// The original definitions of `n` and `prevL1OriginHash` still hold.
+	for {
+		// Advance depth if we change to a different (earlier) epoch.
+		if n.L1Origin.Hash != prevL1OriginHash {
+			depth++
+			prevL1OriginHash = n.L1Origin.Hash
+		}
+
+		// Found an L2 block whose L1 origin is the start of the sequencing window.
+		// Note: We also ensure that we are on the block number with the 0 seq number.
+		// This is a little hacky, but kinda works. The issue is about where the
+		// batch queue should start building.
+		if depth == seqWindowSize && n.SequenceNumber == 0 {
+			return highestPlausibleCanonicalOrigin, n, nil
+		}
+
+		// Genesis is always safe.
+		if n.Hash == genesis.L2.Hash || n.Number == genesis.L2.Number {
+			safe = eth.L2BlockRef{Hash: genesis.L2.Hash, Number: genesis.L2.Number,
+				Time: genesis.L2Time, L1Origin: genesis.L1, SequenceNumber: 0}
+			return highestPlausibleCanonicalOrigin, safe, nil
+		}
+
+		// Pull L2 parent for next iteration.
+		n, err = l2.L2BlockRefByHash(ctx, n.ParentHash)
+		if err != nil {
+			return eth.L2BlockRef{}, eth.L2BlockRef{},
+				fmt.Errorf("failed to fetch L2 block by hash %v: %w", n.ParentHash, err)
+		}
+	}
+}