op-node: fix sync heads starting point [bedrock]

op-node/rollup/derive/engine_queue.go

@@ -10,7 +10,6 @@ import (
 	"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"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/core/types"
@@ -72,6 +71,8 @@ type EngineQueue struct {
 	// Tracks which L2 blocks where last derived from which L1 block. At most finalityLookback large.
 	finalityData []FinalityData
 
+	syncStart *sync.FindSyncStart
+
 	engine Engine
 
 	metrics Metrics
@@ -175,6 +176,10 @@ func (eq *EngineQueue) postProcessSafeL2() {
 	if len(eq.finalityData) >= finalityLookback {
 		eq.finalityData = append(eq.finalityData[:0], eq.finalityData[1:finalityLookback]...)
 	}
+	// after resets we have a safe head that is equal or later than the L1 chain, we can't count that as derived.
+	if eq.safeHead.L1Origin.Number >= eq.progress.Origin.Number {
+		return
+	}
 	// remember the last L2 block that we fully derived from the given finality data
 	if len(eq.finalityData) == 0 || eq.finalityData[len(eq.finalityData)-1].L1Block.Number < eq.progress.Origin.Number {
 		// append entry for new L1 block
@@ -369,38 +374,37 @@ 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 {
-	finalized, err := eq.engine.L2BlockRefByLabel(ctx, eth.Finalized)
-	if errors.Is(err, ethereum.NotFound) {
-		// default to genesis if we have not finalized anything before.
-		finalized, err = eq.engine.L2BlockRefByHash(ctx, eq.cfg.Genesis.L2.Hash)
-	}
-	if err != nil {
-		return NewTemporaryError(fmt.Errorf("failed to find the finalized L2 block: %w", err))
-	}
-	// TODO: this should be resetting using the safe head instead. Out of scope for L2 client bindings PR.
-	prevUnsafe, err := eq.engine.L2BlockRefByLabel(ctx, eth.Unsafe)
-	if err != nil {
-		return NewTemporaryError(fmt.Errorf("failed to find the L2 Head block: %w", err))
-	}
-	unsafe, safe, err := sync.FindL2Heads(ctx, prevUnsafe, eq.cfg.SeqWindowSize, l1Fetcher, eq.engine, &eq.cfg.Genesis)
-	if err != nil {
-		return NewTemporaryError(fmt.Errorf("failed to find the L2 Heads to start from: %w", err))
+	if eq.syncStart == nil {
+		eq.syncStart = sync.NewFindSyncStart(eq.cfg, l1Fetcher, eq.engine)
+		return nil
 	}
-	l1Origin, err := l1Fetcher.L1BlockRefByHash(ctx, safe.L1Origin.Hash)
-	if err != nil {
-		return NewTemporaryError(fmt.Errorf("failed to fetch the new L1 progress: origin: %v; err: %v", safe.L1Origin, err))
+
+	err := eq.syncStart.Step(ctx)
+	if err == nil {
+		return nil // more sync start steps left to do
 	}
-	if safe.Time < l1Origin.Time {
-		return NewResetError(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))
+	if err != io.EOF {
+		if errors.Is(err, sync.TooDeepReorgErr) {
+			return NewCriticalError(err)
+		}
+		if errors.Is(err, sync.WrongChainErr) {
+			return NewCriticalError(err)
+		}
+		if errors.Is(err, sync.ReorgFinalizedErr) {
+			return NewResetError(err)
+		}
+		return NewTemporaryError(fmt.Errorf("sync start step failed: %w", err))
 	}
-	eq.log.Debug("Reset engine queue", "safeHead", safe, "unsafe", unsafe, "safe_timestamp", safe.Time, "unsafe_timestamp", unsafe.Time, "l1Origin", l1Origin)
+
+	finalized, safe, unsafe, startL1 := eq.syncStart.Result()
+
+	eq.log.Debug("Reset engine queue", "l2_finalized", finalized, "l2_safe", safe, "l2_unsafe", unsafe, "l1_start", startL1)
 	eq.unsafeHead = unsafe
 	eq.safeHead = safe
 	eq.finalized = finalized
 	eq.finalityData = eq.finalityData[:0]
 	eq.progress = Progress{
-		Origin: l1Origin,
+		Origin: startL1,
 		Closed: false,
 	}
 	eq.metrics.RecordL2Ref("l2_finalized", finalized)

op-node/rollup/derive/engine_queue_test.go

@@ -1,6 +1,7 @@
 package derive
 
 import (
+	"fmt"
 	"math/rand"
 	"testing"
 
@@ -33,8 +34,12 @@ func TestEngineQueue_Finalize(t *testing.T) {
 	//  D: unsafe, not yet referenced by L2
 
 	l1Time := uint64(2)
-	refA := testutils.RandomBlockRef(rng)
-
+	refA := eth.L1BlockRef{
+		Hash:       testutils.RandomHash(rng),
+		Number:     10000,
+		ParentHash: testutils.RandomHash(rng),
+		Time:       2000000,
+	}
 	refB := eth.L1BlockRef{
 		Hash:       testutils.RandomHash(rng),
 		Number:     refA.Number + 1,
@@ -103,42 +108,85 @@ func TestEngineQueue_Finalize(t *testing.T) {
 		L1Origin:       refC.ID(),
 		SequenceNumber: 0,
 	}
+	refC1 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         refC0.Number + 1,
+		ParentHash:     refC0.Hash,
+		Time:           refC0.Time + cfg.BlockTime,
+		L1Origin:       refC.ID(),
+		SequenceNumber: 1,
+	}
+	refD0 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         refC1.Number + 1,
+		ParentHash:     refC1.Hash,
+		Time:           refC1.Time + cfg.BlockTime,
+		L1Origin:       refD.ID(),
+		SequenceNumber: 0,
+	}
+	refD1 := eth.L2BlockRef{
+		Hash:           testutils.RandomHash(rng),
+		Number:         refD0.Number + 1,
+		ParentHash:     refD0.Hash,
+		Time:           refD0.Time + cfg.BlockTime,
+		L1Origin:       refD.ID(),
+		SequenceNumber: 1,
+	}
+	fmt.Println("refA", refA.Hash)
+	fmt.Println("refB", refB.Hash)
+	fmt.Println("refC", refC.Hash)
+	fmt.Println("refD", refD.Hash)
+
+	fmt.Println("refA0", refA0.Hash)
+	fmt.Println("refA1", refA1.Hash)
+	fmt.Println("refB0", refB0.Hash)
+	fmt.Println("refB1", refB1.Hash)
+	fmt.Println("refC0", refC0.Hash)
+	fmt.Println("refC1", refC1.Hash)
+	fmt.Println("refD0", refD0.Hash)
 
 	metrics := &TestMetrics{}
 	eng := &testutils.MockEngine{}
-	eng.ExpectL2BlockRefByLabel(eth.Finalized, refA1, nil)
-	// TODO(Proto): update expectation once we're using safe block label properly for sync starting point
-	eng.ExpectL2BlockRefByLabel(eth.Unsafe, refC0, nil)
-
 	// we find the common point to initialize to by comparing the L1 origins in the L2 chain with the L1 chain
 	l1F := &testutils.MockL1Source{}
-	l1F.ExpectL1BlockRefByLabel(eth.Unsafe, refD, nil)
-	l1F.ExpectL1BlockRefByNumber(refC0.L1Origin.Number, refC, nil)
-	eng.ExpectL2BlockRefByHash(refC0.ParentHash, refB1, nil)   // good L1 origin
-	eng.ExpectL2BlockRefByHash(refB1.ParentHash, refB0, nil)   // need a block with seqnr == 0, don't stop at above
-	l1F.ExpectL1BlockRefByHash(refB0.L1Origin.Hash, refB, nil) // the origin of the safe L2 head will be the L1 starting point for derivation.
 
-	eq := NewEngineQueue(logger, cfg, eng, metrics)
-	require.NoError(t, RepeatResetStep(t, eq.ResetStep, l1F, 3))
+	eng.ExpectL2BlockRefByLabel(eth.Finalized, refA1, nil)
+	eng.ExpectL2BlockRefByLabel(eth.Safe, refD0, nil)
+	eng.ExpectL2BlockRefByLabel(eth.Unsafe, refD1, nil)
 
-	// TODO(proto): this is changing, needs to be a sequence window ago, but starting traversal back from safe block,
-	// safe blocks with canon origin are good, but we go back a full window to ensure they are all included in L1,
-	// by forcing them to be consolidated with L1 again.
-	require.Equal(t, eq.SafeL2Head(), refB0, "L2 reset should go back to sequence window ago")
+	l1F.ExpectL1BlockRefByNumber(refD.Number, refD, nil)     // fetch L1 origin of head, it's canon
+	eng.ExpectL2BlockRefByHash(refD1.ParentHash, refD0, nil) // traverse L2 chain, find safe head D0
+	eng.ExpectL2BlockRefByHash(refD0.ParentHash, refC1, nil) // traverse back full seq window
+	l1F.ExpectL1BlockRefByNumber(refC.Number, refC, nil)
+	eng.ExpectL2BlockRefByHash(refC1.ParentHash, refC0, nil)
+	eng.ExpectL2BlockRefByHash(refC0.ParentHash, refB1, nil)
+	l1F.ExpectL1BlockRefByNumber(refB.Number, refB, nil)
+	l1F.ExpectL1BlockRefByHash(refB.Hash, refB, nil)
 
+	eq := NewEngineQueue(logger, cfg, eng, metrics)
+	require.NoError(t, RepeatResetStep(t, eq.ResetStep, l1F, 20))
+
+	require.Equal(t, refB1, eq.SafeL2Head(), "L2 reset should go back to sequence window ago: blocks with origin D and C are not safe until we reconcile")
+	require.Equal(t, refB, eq.Progress().Origin, "Expecting to be set back derivation L1 progress to B")
 	require.Equal(t, refA1, eq.Finalized(), "A1 is recognized as finalized before we run any steps")
 
-	// we are not adding blocks in this test,
-	// but we can still trigger post-processing for the already existing safe head,
-	// so the engine can prepare to finalize that.
+	// now say B1 was included in C and became the new safe head
+	eq.progress.Origin = refC
+	eq.safeHead = refB1
 	eq.postProcessSafeL2()
-	// let's finalize C, which included B0, but not B1
+
+	// now say C0 was included in D and became the new safe head
+	eq.progress.Origin = refD
+	eq.safeHead = refC0
+	eq.postProcessSafeL2()
+
+	// let's finalize C (current L1), from which we fully derived B1, but not C0
 	eq.Finalize(refC.ID())
 
 	// Now a few steps later, without consuming any additional L1 inputs,
-	// we should be able to resolve that B0 is now finalized
+	// we should be able to resolve that B1 is now finalized
 	require.NoError(t, RepeatStep(t, eq.Step, eq.progress, 10))
-	require.Equal(t, refB0, eq.Finalized(), "B0 was included in finalized C, and should now be finalized")
+	require.Equal(t, refB1, eq.Finalized(), "B1 was included in finalized C, and should now be finalized")
 
 	l1F.AssertExpectations(t)
 	eng.AssertExpectations(t)