op-supervisor: handle out-of-sync on known local-safe tip with alread…

…y-known source blocks (#14252)

* op-supervisor: handle out-of-sync on known local-safe tip with already-known source blocks

* op-supervisor: improve logging

* op-supervisor: fix fromda test, add test cases

* op-supervisor: fix lint

op-node/rollup/interop/managed/system.go

@@ -257,7 +257,7 @@ const (
 
 func (m *ManagedMode) Reset(ctx context.Context, unsafe, safe, finalized eth.BlockID) error {
 	logger := m.log.New("unsafe", unsafe, "safe", safe, "finalized", finalized)
-	logger.Debug("Received reset request", "unsafe", unsafe, "safe", safe, "finalized", finalized)
+	logger.Info("Received reset request", "unsafe", unsafe, "safe", safe, "finalized", finalized)
 
 	verify := func(ref eth.BlockID, name string) (eth.L2BlockRef, error) {
 		result, err := m.l2.L2BlockRefByNumber(ctx, ref.Number)

op-supervisor/supervisor/backend/db/anchor.go

@@ -12,7 +12,7 @@ import (
 func (db *ChainsDB) maybeInitSafeDB(id eth.ChainID, anchor types.DerivedBlockRefPair) {
 	_, err := db.LocalSafe(id)
 	if errors.Is(err, types.ErrFuture) {
-		db.logger.Debug("initializing chain database", "chain", id)
+		db.logger.Info("initializing chain database", "chain", id, "anchor", anchor)
 		if err := db.UpdateCrossSafe(id, anchor.Source, anchor.Derived); err != nil {
 			db.logger.Warn("failed to initialize cross safe", "chain", id, "error", err)
 		}

op-supervisor/supervisor/backend/db/fromda/update.go

@@ -264,7 +264,24 @@ func (db *DB) addLink(derivedFrom eth.BlockRef, derived eth.BlockRef, invalidate
 			lastDerived, lastSource,
 			types.ErrOutOfOrder)
 	} else {
-		// adding block that is derived from something too old
+		if lastDerived.Hash == derived.Hash {
+			// we might see L1 blocks repeat,
+			// if the deriver has reset to the latest local-safe block,
+			// since we don't reset it to any particular source block.
+			// So check if it's canonical, and if it is, we can gracefully accept it, to allow forwards progress.
+			_, got, err := db.lookup(derivedFrom.Number, derived.Number)
+			if err != nil {
+				return fmt.Errorf("failed to check if block %s with old source %s was derived from canonical source chain: %w",
+					derived, derivedFrom, err)
+			}
+			if got.source.Hash != derivedFrom.Hash {
+				return fmt.Errorf("cannot add block %s that matches latest derived since it is derived from non-canonical source %s, expected %s: %w",
+					derived, derivedFrom, got.source, types.ErrConflict)
+			}
+			return fmt.Errorf("received latest block %s, derived from known old source %s, latest source is %s: %w",
+				derived, derivedFrom, lastSource, types.ErrIneffective)
+		}
+		// Adding a newer block that is derived from an older source, that cannot be right
 		return fmt.Errorf("cannot add block %s as derived from %s, deriving already at %s: %w",
 			derived, derivedFrom, lastSource, types.ErrOutOfOrder)
 	}

op-supervisor/supervisor/backend/db/fromda/update_test.go

@@ -6,6 +6,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/crypto"
 
 	"github.com/ethereum-optimism/optimism/op-supervisor/supervisor/types"
 )
@@ -24,6 +25,8 @@ func TestBadUpdates(t *testing.T) {
 	cSource := mockL1(3)
 	cDerived := mockL2(203)
 	dSource := mockL1(4)
+	dAltSource := mockL1(4)
+	dAltSource.Hash = crypto.Keccak256Hash([]byte("bad alternative block d"))
 	dDerived := mockL2(204)
 	eSource := mockL1(5)
 	eDerived := mockL2(205)
@@ -39,34 +42,110 @@ func TestBadUpdates(t *testing.T) {
 
 	testCases := []testCase{
 		{
-			name: "add on old derivedFrom",
+			name: "add on old derivedFrom before DB start",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(bSource, aSource.Hash), toRef(dDerived, cDerived.Hash)), types.ErrOutOfOrder)
+				require.ErrorIs(t, db.AddDerived(
+					toRef(bSource, aSource.Hash), // b is before c
+					toRef(dDerived, cDerived.Hash)), types.ErrSkipped)
 			},
 			assertFn: noChange,
 		},
 		{
-			name: "repeat parent derivedFrom",
+			name: "repeat second latest",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(cSource, bSource.Hash), toRef(dDerived, cDerived.Hash)), types.ErrOutOfOrder)
+				require.ErrorIs(t, db.AddDerived(
+					toRef(cSource, bSource.Hash),
+					toRef(cDerived, bDerived.Hash),
+				), types.ErrOutOfOrder)
 			},
 			assertFn: noChange,
 		},
+		{
+			name: "repeat latest",
+			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
+				require.NoError(t, db.AddDerived(
+					toRef(dSource, cSource.Hash),
+					toRef(dDerived, cDerived.Hash),
+				))
+			},
+			assertFn: noChange,
+		},
+		{
+			name: "repeat latest derived, old derivedFrom",
+			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
+				require.NoError(t, db.AddDerived(
+					toRef(eSource, dSource.Hash),   // new L1 block
+					toRef(dDerived, cDerived.Hash), // same L2 block
+				))
+				require.ErrorIs(t, db.AddDerived(
+					toRef(dSource, cSource.Hash),   // d is old, but was canonically linked like this before
+					toRef(dDerived, cDerived.Hash), // same L2 block
+				), types.ErrIneffective)
+			},
+			assertFn: func(t *testing.T, db *DB, m *stubMetrics) {
+				pair, err := db.Last()
+				require.NoError(t, err)
+				require.Equal(t, eSource, pair.Source)
+				require.Equal(t, dDerived, pair.Derived)
+			},
+		},
+		{
+			name: "repeat latest derived, conflicting old derivedFrom",
+			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
+				require.NoError(t, db.AddDerived(
+					toRef(eSource, dSource.Hash),   // new L1 block
+					toRef(dDerived, cDerived.Hash), // same L2 block
+				))
+				require.ErrorIs(t, db.AddDerived(
+					toRef(dAltSource, cSource.Hash), // conflicting old block
+					toRef(dDerived, cDerived.Hash),  // same L2 block
+				), types.ErrConflict)
+			},
+			assertFn: func(t *testing.T, db *DB, m *stubMetrics) {
+				pair, err := db.Last()
+				require.NoError(t, err)
+				require.Equal(t, eSource, pair.Source)
+				require.Equal(t, dDerived, pair.Derived)
+			},
+		},
+		{
+			name: "new derived, old derivedFrom",
+			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
+				require.NoError(t, db.AddDerived(
+					toRef(eSource, dSource.Hash),   // new L1 block
+					toRef(dDerived, cDerived.Hash), // same L2 block
+				))
+				require.ErrorIs(t, db.AddDerived(
+					toRef(dSource, cSource.Hash),   // old L1 block
+					toRef(eDerived, dDerived.Hash), // new L2 block
+				), types.ErrOutOfOrder)
+			},
+			assertFn: func(t *testing.T, db *DB, m *stubMetrics) {
+				pair, err := db.Last()
+				require.NoError(t, err)
+				require.Equal(t, eSource, pair.Source)
+				require.Equal(t, dDerived, pair.Derived)
+			},
+		},
 		{
 			name: "add on conflicting derivedFrom, same height. And new derived value",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(types.BlockSeal{
-					Hash:      common.Hash{0xba, 0xd},
-					Number:    dSource.Number,
-					Timestamp: dSource.Timestamp,
-				}, cSource.Hash), toRef(eDerived, dDerived.Hash)), types.ErrConflict)
+				require.ErrorIs(t, db.AddDerived(
+					toRef(types.BlockSeal{
+						Hash:      common.Hash{0xba, 0xd},
+						Number:    dSource.Number,
+						Timestamp: dSource.Timestamp,
+					}, cSource.Hash),
+					toRef(eDerived, dDerived.Hash)), types.ErrConflict)
 			},
 			assertFn: noChange,
 		},
 		{
 			name: "CrossSource with conflicting parent root, same L1 height, new L2: accepted, L1 parent-hash is used only on L1 increments.",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.NoError(t, db.AddDerived(toRef(dSource, common.Hash{0x42}), toRef(eDerived, dDerived.Hash)), types.ErrConflict)
+				require.NoError(t, db.AddDerived(
+					toRef(dSource, common.Hash{0x42}),
+					toRef(eDerived, dDerived.Hash)), types.ErrConflict)
 			},
 			assertFn: func(t *testing.T, db *DB, m *stubMetrics) {
 				pair, err := db.Last()
@@ -78,46 +157,59 @@ func TestBadUpdates(t *testing.T) {
 		{
 			name: "Conflicting derivedFrom parent root, new L1 height, same L2",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(eSource, common.Hash{0x42}), toRef(dDerived, cDerived.Hash)), types.ErrConflict)
+				require.ErrorIs(t,
+					db.AddDerived(
+						toRef(eSource, common.Hash{0x42}),
+						toRef(dDerived, cDerived.Hash)), types.ErrConflict)
 			},
 			assertFn: noChange,
 		},
 		{
 			name: "add on too new derivedFrom (even if parent-hash looks correct)",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(fSource, dSource.Hash), toRef(eDerived, dDerived.Hash)), types.ErrOutOfOrder)
+				require.ErrorIs(t,
+					db.AddDerived(toRef(fSource, dSource.Hash),
+						toRef(eDerived, dDerived.Hash)), types.ErrOutOfOrder)
 			},
 			assertFn: noChange,
 		},
 		{
 			name: "add on old derivedFrom (even if parent-hash looks correct)",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(cSource, bSource.Hash), toRef(cDerived, dDerived.Hash)), types.ErrOutOfOrder)
+				require.ErrorIs(t, db.AddDerived(
+					toRef(cSource, bSource.Hash),
+					toRef(cDerived, dDerived.Hash)), types.ErrOutOfOrder)
 			},
 			assertFn: noChange,
 		},
 		{
 			name: "add on even older derivedFrom",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(bSource, aSource.Hash), toRef(dDerived, cDerived.Hash)), types.ErrOutOfOrder)
+				require.ErrorIs(t, db.AddDerived(
+					toRef(bSource, aSource.Hash),
+					toRef(dDerived, cDerived.Hash)), types.ErrSkipped)
 			},
 			assertFn: noChange,
 		},
 		{
 			name: "add on conflicting derived, same L2 height, new L1 block",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(eSource, dSource.Hash), toRef(types.BlockSeal{
-					Hash:      common.Hash{0x42},
-					Number:    dDerived.Number,
-					Timestamp: dDerived.Timestamp,
-				}, cDerived.Hash)), types.ErrConflict)
+				require.ErrorIs(t, db.AddDerived(
+					toRef(eSource, dSource.Hash),
+					toRef(types.BlockSeal{
+						Hash:      common.Hash{0x42},
+						Number:    dDerived.Number,
+						Timestamp: dDerived.Timestamp,
+					}, cDerived.Hash)), types.ErrConflict)
 			},
 			assertFn: noChange,
 		},
 		{
 			name: "add derived with conflicting parent hash, new L1 height, same L2 height: accepted, L2 parent-hash is only checked on L2 increments.",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.NoError(t, db.AddDerived(toRef(eSource, dSource.Hash), toRef(dDerived, common.Hash{0x42})), types.ErrConflict)
+				require.NoError(t, db.AddDerived(
+					toRef(eSource, dSource.Hash),
+					toRef(dDerived, common.Hash{0x42})), types.ErrConflict)
 			},
 			assertFn: func(t *testing.T, db *DB, m *stubMetrics) {
 				pair, err := db.Last()
@@ -129,36 +221,46 @@ func TestBadUpdates(t *testing.T) {
 		{
 			name: "add derived with conflicting parent hash, same L1 height, new L2 height",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(dSource, cSource.Hash), toRef(eDerived, common.Hash{0x42})), types.ErrConflict)
+				require.ErrorIs(t, db.AddDerived(
+					toRef(dSource, cSource.Hash),
+					toRef(eDerived, common.Hash{0x42})), types.ErrConflict)
 			},
 			assertFn: noChange,
 		},
 		{
 			name: "add on too new derived (even if parent-hash looks correct)",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(dSource, cSource.Hash), toRef(fDerived, dDerived.Hash)), types.ErrOutOfOrder)
+				require.ErrorIs(t, db.AddDerived(
+					toRef(dSource, cSource.Hash),
+					toRef(fDerived, dDerived.Hash)), types.ErrOutOfOrder)
 			},
 			assertFn: noChange,
 		},
 		{
 			name: "add on old derived (even if parent-hash looks correct)",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(dSource, cSource.Hash), toRef(cDerived, bDerived.Hash)), types.ErrOutOfOrder)
+				require.ErrorIs(t, db.AddDerived(
+					toRef(dSource, cSource.Hash),
+					toRef(cDerived, bDerived.Hash)), types.ErrOutOfOrder)
 			},
 			assertFn: noChange,
 		},
 		{
 			name: "add on even older derived",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
-				require.ErrorIs(t, db.AddDerived(toRef(dSource, cSource.Hash), toRef(bDerived, aDerived.Hash)), types.ErrOutOfOrder)
+				require.ErrorIs(t, db.AddDerived(
+					toRef(dSource, cSource.Hash),
+					toRef(bDerived, aDerived.Hash)), types.ErrOutOfOrder)
 			},
 			assertFn: noChange,
 		},
 		{
 			name: "repeat self, silent no-op",
 			setupFn: func(t *testing.T, db *DB, m *stubMetrics) {
 				pre := m.DBDerivedEntryCount
-				require.NoError(t, db.AddDerived(toRef(dSource, cSource.Hash), toRef(dDerived, cDerived.Hash)), types.ErrOutOfOrder)
+				require.NoError(t, db.AddDerived(
+					toRef(dSource, cSource.Hash),
+					toRef(dDerived, cDerived.Hash)), types.ErrOutOfOrder)
 				require.Equal(t, pre, m.DBDerivedEntryCount)
 			},
 			assertFn: noChange,
@@ -170,7 +272,9 @@ func TestBadUpdates(t *testing.T) {
 			runDBTest(t,
 				func(t *testing.T, db *DB, m *stubMetrics) {
 					// Good first entry
-					require.NoError(t, db.AddDerived(toRef(dSource, cSource.Hash), toRef(dDerived, cDerived.Hash)))
+					require.NoError(t, db.AddDerived(
+						toRef(dSource, cSource.Hash),
+						toRef(dDerived, cDerived.Hash)))
 					// apply the test-case setup
 					tc.setupFn(t, db, m)
 				},

op-supervisor/supervisor/backend/db/update.go

@@ -1,6 +1,7 @@
 package db
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/ethereum/go-ethereum/common"
@@ -79,15 +80,19 @@ func (db *ChainsDB) UpdateLocalSafe(chain eth.ChainID, derivedFrom eth.BlockRef,
 	}
 	logger.Debug("Updating local safe DB")
 	if err := localDB.AddDerived(derivedFrom, lastDerived); err != nil {
-		db.logger.Warn("Failed to update local safe", "err", err)
+		if errors.Is(err, types.ErrIneffective) {
+			logger.Info("Node is syncing known source blocks on known latest local-safe block", "err", err)
+			return
+		}
+		logger.Warn("Failed to update local safe", "err", err)
 		db.emitter.Emit(superevents.LocalSafeOutOfSyncEvent{
 			ChainID: chain,
 			L1Ref:   derivedFrom,
 			Err:     err,
 		})
 		return
 	}
-	db.logger.Info("Updated local safe DB")
+	logger.Info("Updated local safe DB")
 	db.emitter.Emit(superevents.LocalSafeUpdateEvent{
 		ChainID: chain,
 		NewLocalSafe: types.DerivedBlockSealPair{

op-supervisor/supervisor/types/error.go

@@ -13,6 +13,10 @@ var (
 	ErrSkipped = errors.New("skipped data")
 	// ErrFuture happens when data is just not yet available
 	ErrFuture = errors.New("future data")
+	// ErrIneffective happens when data is accepted as compatible, but did not change anything.
+	// This happens when a node is deriving an L2 block we already know of being derived from the given source,
+	// but without path to skip forward to newer source blocks without doing the known derivation work first.
+	ErrIneffective = errors.New("ineffective data")
 	// ErrConflict happens when we know for sure that there is different canonical data
 	ErrConflict = errors.New("conflicting data")
 	// ErrAwaitReplacementBlock happens when we know for sure that a replacement block is needed before progress can be made.