Cross safe updates use cycle checks (#12726)

* tweak: Make cycle checks work with an OpenBlock that returns BlockRef.

* tests,fix: Make tests generate correct hazard maps.

* tests: Add assertion on returned blockRef.

* tests,fix: Fix TestCrossUnsafeUpdate to use correct mock log count.

* tweak: Call HazardCycleChecks from scopedCrossSafeUpdate and CrossUnsafeUpdate.

* tests,cleanup: Fix test assertion comment.

* tests,fix: Fix TestCrossSafeUpdate.

op-supervisor/supervisor/backend/cross/cycle.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/ethereum-optimism/optimism/op-service/eth"
 	"github.com/ethereum-optimism/optimism/op-supervisor/supervisor/types"
 )
 
@@ -17,7 +18,7 @@ var (
 // CycleCheckDeps is an interface for checking cyclical dependencies between logs.
 type CycleCheckDeps interface {
 	// OpenBlock returns log data for the requested block, to be used for cycle checking.
-	OpenBlock(chainID types.ChainID, blockNum uint64) (seal types.BlockSeal, logCount uint32, execMsgs map[uint32]*types.ExecutingMessage, err error)
+	OpenBlock(chainID types.ChainID, blockNum uint64) (block eth.BlockRef, logCount uint32, execMsgs map[uint32]*types.ExecutingMessage, err error)
 }
 
 // node represents a log entry in the dependency graph.
@@ -95,7 +96,8 @@ func gatherLogs(d CycleCheckDeps, inTimestamp uint64, hazards map[types.ChainInd
 		if err != nil {
 			return nil, nil, fmt.Errorf("failed to open block: %w", err)
 		}
-		if bl != hazardBlock {
+
+		if !blockSealMatchesRef(hazardBlock, bl) {
 			return nil, nil, fmt.Errorf("tried to open block %s of chain %s, but got different block %s than expected, use a reorg lock for consistency", hazardBlock, hazardChainID, bl)
 		}
 
@@ -246,6 +248,10 @@ func checkGraph(g *graph) error {
 	}
 }
 
+func blockSealMatchesRef(seal types.BlockSeal, ref eth.BlockRef) bool {
+	return seal.Number == ref.Number && seal.Hash == ref.Hash
+}
+
 // GenerateMermaidDiagram creates a Mermaid flowchart diagram from the graph data for debugging.
 func GenerateMermaidDiagram(g *graph) string {
 	var sb strings.Builder

op-supervisor/supervisor/backend/cross/cycle_test.go

@@ -8,14 +8,15 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	"github.com/ethereum-optimism/optimism/op-service/eth"
 	"github.com/ethereum-optimism/optimism/op-supervisor/supervisor/types"
 )
 
 type mockCycleCheckDeps struct {
-	openBlockFn func(chainID types.ChainID, blockNum uint64) (types.BlockSeal, uint32, map[uint32]*types.ExecutingMessage, error)
+	openBlockFn func(chainID types.ChainID, blockNum uint64) (eth.BlockRef, uint32, map[uint32]*types.ExecutingMessage, error)
 }
 
-func (m *mockCycleCheckDeps) OpenBlock(chainID types.ChainID, blockNum uint64) (types.BlockSeal, uint32, map[uint32]*types.ExecutingMessage, error) {
+func (m *mockCycleCheckDeps) OpenBlock(chainID types.ChainID, blockNum uint64) (eth.BlockRef, uint32, map[uint32]*types.ExecutingMessage, error) {
 	return m.openBlockFn(chainID, blockNum)
 }
 
@@ -33,7 +34,7 @@ type hazardCycleChecksTestCase struct {
 
 	// Optional overrides
 	hazards     map[types.ChainIndex]types.BlockSeal
-	openBlockFn func(chainID types.ChainID, blockNum uint64) (types.BlockSeal, uint32, map[uint32]*types.ExecutingMessage, error)
+	openBlockFn func(chainID types.ChainID, blockNum uint64) (eth.BlockRef, uint32, map[uint32]*types.ExecutingMessage, error)
 }
 
 func runHazardCycleChecksTestCaseGroup(t *testing.T, group string, tests []hazardCycleChecksTestCase) {
@@ -47,7 +48,7 @@ func runHazardCycleChecksTestCaseGroup(t *testing.T, group string, tests []hazar
 func runHazardCycleChecksTestCase(t *testing.T, tc hazardCycleChecksTestCase) {
 	// Create mocked dependencies
 	deps := &mockCycleCheckDeps{
-		openBlockFn: func(chainID types.ChainID, blockNum uint64) (types.BlockSeal, uint32, map[uint32]*types.ExecutingMessage, error) {
+		openBlockFn: func(chainID types.ChainID, blockNum uint64) (eth.BlockRef, uint32, map[uint32]*types.ExecutingMessage, error) {
 			// Use override if provided
 			if tc.openBlockFn != nil {
 				return tc.openBlockFn(chainID, blockNum)
@@ -57,12 +58,12 @@ func runHazardCycleChecksTestCase(t *testing.T, tc hazardCycleChecksTestCase) {
 			chainStr := chainID.String()
 			def, ok := tc.chainBlocks[chainStr]
 			if !ok {
-				return types.BlockSeal{}, 0, nil, errors.New("unexpected chain")
+				return eth.BlockRef{}, 0, nil, errors.New("unexpected chain")
 			}
 			if def.error != nil {
-				return types.BlockSeal{}, 0, nil, def.error
+				return eth.BlockRef{}, 0, nil, def.error
 			}
-			return types.BlockSeal{Number: blockNum}, def.logCount, def.messages, nil
+			return eth.BlockRef{Number: blockNum}, def.logCount, def.messages, nil
 		},
 	}
 
@@ -149,8 +150,8 @@ func TestHazardCycleChecksFailures(t *testing.T) {
 		{
 			name:        "failed to open block error",
 			chainBlocks: emptyChainBlocks,
-			openBlockFn: func(chainID types.ChainID, blockNum uint64) (types.BlockSeal, uint32, map[uint32]*types.ExecutingMessage, error) {
-				return types.BlockSeal{}, 0, nil, testOpenBlockErr
+			openBlockFn: func(chainID types.ChainID, blockNum uint64) (eth.BlockRef, uint32, map[uint32]*types.ExecutingMessage, error) {
+				return eth.BlockRef{}, 0, nil, testOpenBlockErr
 			},
 			expectErr: errors.New("failed to open block"),
 			msg:       "expected error when OpenBlock fails",
@@ -159,8 +160,8 @@ func TestHazardCycleChecksFailures(t *testing.T) {
 			name:        "block mismatch error",
 			chainBlocks: emptyChainBlocks,
 			// openBlockFn returns a block number that doesn't match the expected block number.
-			openBlockFn: func(chainID types.ChainID, blockNum uint64) (types.BlockSeal, uint32, map[uint32]*types.ExecutingMessage, error) {
-				return types.BlockSeal{Number: blockNum + 1}, 0, make(map[uint32]*types.ExecutingMessage), nil
+			openBlockFn: func(chainID types.ChainID, blockNum uint64) (eth.BlockRef, uint32, map[uint32]*types.ExecutingMessage, error) {
+				return eth.BlockRef{Number: blockNum + 1}, 0, make(map[uint32]*types.ExecutingMessage), nil
 			},
 			expectErr: errors.New("tried to open block"),
 			msg:       "expected error due to block mismatch",

op-supervisor/supervisor/backend/cross/safe_update.go

@@ -89,9 +89,9 @@ func scopedCrossSafeUpdate(logger log.Logger, chainID types.ChainID, d CrossSafe
 	if err := HazardSafeFrontierChecks(d, candidateScope.ID(), hazards); err != nil {
 		return candidateScope, fmt.Errorf("failed to verify block %s in cross-safe frontier: %w", candidate, err)
 	}
-	//if err := HazardCycleChecks(d, candidate.Timestamp, hazards); err != nil {
-	// TODO
-	//}
+	if err := HazardCycleChecks(d, candidate.Time, hazards); err != nil {
+		return candidateScope, fmt.Errorf("failed to verify block %s in cross-safe check for cycle hazards: %w", candidate, err)
+	}
 
 	// promote the candidate block to cross-safe
 	if err := d.UpdateCrossSafe(chainID, candidateScope, candidate); err != nil {

op-supervisor/supervisor/backend/cross/safe_update_test.go

@@ -30,6 +30,9 @@ func TestCrossSafeUpdate(t *testing.T) {
 		csd.openBlockFn = func(chainID types.ChainID, blockNum uint64) (ref eth.BlockRef, logCount uint32, execMsgs map[uint32]*types.ExecutingMessage, err error) {
 			return opened, 10, execs, nil
 		}
+		csd.checkFn = func(chainID types.ChainID, blockNum uint64, logIdx uint32, logHash common.Hash) (types.BlockSeal, error) {
+			return types.BlockSeal{Number: 1, Timestamp: 1}, nil
+		}
 		csd.deps = mockDependencySet{}
 		// when scopedCrossSafeUpdate returns no error,
 		// no error is returned
@@ -256,6 +259,9 @@ func TestScopedCrossSafeUpdate(t *testing.T) {
 		csd.openBlockFn = func(chainID types.ChainID, blockNum uint64) (ref eth.BlockRef, logCount uint32, execMsgs map[uint32]*types.ExecutingMessage, err error) {
 			return opened, 10, execs, nil
 		}
+		csd.checkFn = func(chainID types.ChainID, blockNum uint64, logIdx uint32, logHash common.Hash) (types.BlockSeal, error) {
+			return types.BlockSeal{Number: 1, Timestamp: 1}, nil
+		}
 		count := 0
 		csd.deps = mockDependencySet{}
 		// cause CrossSafeHazards to return an error by making ChainIDFromIndex return an error
@@ -274,6 +280,32 @@ func TestScopedCrossSafeUpdate(t *testing.T) {
 		require.ErrorContains(t, err, "frontier")
 		require.Equal(t, eth.BlockRef{}, blockRef)
 	})
+	t.Run("HazardCycleChecks returns error", func(t *testing.T) {
+		logger := testlog.Logger(t, log.LevelDebug)
+		chainID := types.ChainIDFromUInt64(0)
+		csd := &mockCrossSafeDeps{}
+		candidate := eth.BlockRef{Number: 1, Time: 1}
+		candidateScope := eth.BlockRef{Number: 2}
+		csd.candidateCrossSafeFn = func() (derivedFromScope, crossSafe eth.BlockRef, err error) {
+			return candidateScope, candidate, nil
+		}
+		opened := eth.BlockRef{Number: 1, Time: 1}
+		em1 := &types.ExecutingMessage{Chain: types.ChainIndex(0), Timestamp: 1, LogIdx: 2}
+		em2 := &types.ExecutingMessage{Chain: types.ChainIndex(0), Timestamp: 1, LogIdx: 1}
+		csd.openBlockFn = func(chainID types.ChainID, blockNum uint64) (ref eth.BlockRef, logCount uint32, execMsgs map[uint32]*types.ExecutingMessage, err error) {
+			return opened, 3, map[uint32]*types.ExecutingMessage{1: em1, 2: em2}, nil
+		}
+		csd.checkFn = func(chainID types.ChainID, blockNum uint64, logIdx uint32, logHash common.Hash) (types.BlockSeal, error) {
+			return types.BlockSeal{Number: 1, Timestamp: 1}, nil
+		}
+		csd.deps = mockDependencySet{}
+
+		// HazardCycleChecks returns an error with appropriate wrapping
+		blockRef, err := scopedCrossSafeUpdate(logger, chainID, csd)
+		require.ErrorContains(t, err, "cycle detected")
+		require.ErrorContains(t, err, "failed to verify block")
+		require.Equal(t, eth.BlockRef{Number: 2}, blockRef)
+	})
 	t.Run("UpdateCrossSafe returns error", func(t *testing.T) {
 		logger := testlog.Logger(t, log.LevelDebug)
 		chainID := types.ChainIDFromUInt64(0)
@@ -288,15 +320,19 @@ func TestScopedCrossSafeUpdate(t *testing.T) {
 		csd.openBlockFn = func(chainID types.ChainID, blockNum uint64) (ref eth.BlockRef, logCount uint32, execMsgs map[uint32]*types.ExecutingMessage, err error) {
 			return opened, 10, execs, nil
 		}
+		csd.checkFn = func(chainID types.ChainID, blockNum uint64, logIdx uint32, logHash common.Hash) (types.BlockSeal, error) {
+			return types.BlockSeal{Number: 1, Timestamp: 1}, nil
+		}
 		csd.deps = mockDependencySet{}
 		csd.updateCrossSafeFn = func(chain types.ChainID, l1View eth.BlockRef, lastCrossDerived eth.BlockRef) error {
 			return errors.New("some error")
 		}
 		// when UpdateCrossSafe returns an error,
 		// the error is returned
-		_, err := scopedCrossSafeUpdate(logger, chainID, csd)
+		blockRef, err := scopedCrossSafeUpdate(logger, chainID, csd)
 		require.ErrorContains(t, err, "some error")
 		require.ErrorContains(t, err, "failed to update")
+		require.Equal(t, eth.BlockRef{Number: 2}, blockRef)
 	})
 	t.Run("successful update", func(t *testing.T) {
 		logger := testlog.Logger(t, log.LevelDebug)
@@ -325,6 +361,9 @@ func TestScopedCrossSafeUpdate(t *testing.T) {
 		// when no errors occur, the update is carried out
 		// the used candidate and scope are from CandidateCrossSafe
 		// the candidateScope is returned
+		csd.checkFn = func(chainID types.ChainID, blockNum uint64, logIdx uint32, logHash common.Hash) (types.BlockSeal, error) {
+			return types.BlockSeal{Number: 1, Timestamp: 1}, nil
+		}
 		blockRef, err := scopedCrossSafeUpdate(logger, chainID, csd)
 		require.Equal(t, chainID, updatingChain)
 		require.Equal(t, candidateScope, updatingCandidateScope)
@@ -342,6 +381,7 @@ type mockCrossSafeDeps struct {
 	updateCrossSafeFn    func(chain types.ChainID, l1View eth.BlockRef, lastCrossDerived eth.BlockRef) error
 	nextDerivedFromFn    func(chain types.ChainID, derivedFrom eth.BlockID) (after eth.BlockRef, err error)
 	previousDerivedFn    func(chain types.ChainID, derived eth.BlockID) (prevDerived types.BlockSeal, err error)
+	checkFn              func(chainID types.ChainID, blockNum uint64, logIdx uint32, logHash common.Hash) (types.BlockSeal, error)
 }
 
 func (m *mockCrossSafeDeps) CrossSafe(chainID types.ChainID) (derivedFrom types.BlockSeal, derived types.BlockSeal, err error) {
@@ -367,6 +407,9 @@ func (m *mockCrossSafeDeps) CrossDerivedFrom(chainID types.ChainID, derived eth.
 }
 
 func (m *mockCrossSafeDeps) Check(chainID types.ChainID, blockNum uint64, logIdx uint32, logHash common.Hash) (types.BlockSeal, error) {
+	if m.checkFn != nil {
+		return m.checkFn(chainID, blockNum, logIdx, logHash)
+	}
 	return types.BlockSeal{}, nil
 }
 

op-supervisor/supervisor/backend/cross/unsafe_update.go

@@ -60,9 +60,9 @@ func CrossUnsafeUpdate(ctx context.Context, logger log.Logger, chainID types.Cha
 	if err := HazardUnsafeFrontierChecks(d, hazards); err != nil {
 		return fmt.Errorf("failed to verify block %s in cross-unsafe frontier: %w", candidate, err)
 	}
-	//if err := HazardCycleChecks(d, candidate.Timestamp, hazards); err != nil {
-	//// TODO
-	//}
+	if err := HazardCycleChecks(d, candidate.Timestamp, hazards); err != nil {
+		return fmt.Errorf("failed to verify block %s in cross-unsafe check for cycle hazards: %w", candidate, err)
+	}
 
 	// promote the candidate block to cross-unsafe
 	if err := d.UpdateCrossUnsafe(chainID, candidate); err != nil {

op-supervisor/supervisor/backend/cross/unsafe_update_test.go

@@ -131,6 +131,31 @@ func TestCrossUnsafeUpdate(t *testing.T) {
 		err := CrossUnsafeUpdate(ctx, logger, chainID, usd)
 		require.ErrorContains(t, err, "some error")
 	})
+	t.Run("HazardCycleChecks returns error", func(t *testing.T) {
+		ctx := context.Background()
+		logger := testlog.Logger(t, log.LevelDebug)
+		chainID := types.ChainIDFromUInt64(0)
+		usd := &mockCrossUnsafeDeps{}
+		crossUnsafe := types.BlockSeal{Hash: common.Hash{0x01}}
+		usd.crossUnsafeFn = func(chainID types.ChainID) (types.BlockSeal, error) {
+			return crossUnsafe, nil
+		}
+		bl := eth.BlockRef{ParentHash: common.Hash{0x01}, Number: 1, Time: 1}
+		em1 := &types.ExecutingMessage{Chain: types.ChainIndex(0), Timestamp: 1, LogIdx: 2}
+		em2 := &types.ExecutingMessage{Chain: types.ChainIndex(0), Timestamp: 1, LogIdx: 1}
+		usd.openBlockFn = func(chainID types.ChainID, blockNum uint64) (ref eth.BlockRef, logCount uint32, execMsgs map[uint32]*types.ExecutingMessage, err error) {
+			return bl, 3, map[uint32]*types.ExecutingMessage{1: em1, 2: em2}, nil
+		}
+		usd.checkFn = func(chainID types.ChainID, blockNum uint64, logIdx uint32, logHash common.Hash) (types.BlockSeal, error) {
+			return types.BlockSeal{Number: 1, Timestamp: 1}, nil
+		}
+		usd.deps = mockDependencySet{}
+
+		// HazardCycleChecks returns an error with appropriate wrapping
+		err := CrossUnsafeUpdate(ctx, logger, chainID, usd)
+		require.ErrorContains(t, err, "cycle detected")
+		require.ErrorContains(t, err, "failed to verify block")
+	})
 	t.Run("successful update", func(t *testing.T) {
 		ctx := context.Background()
 		logger := testlog.Logger(t, log.LevelDebug)
@@ -144,7 +169,7 @@ func TestCrossUnsafeUpdate(t *testing.T) {
 		em1 := &types.ExecutingMessage{Timestamp: 1}
 		usd.openBlockFn = func(chainID types.ChainID, blockNum uint64) (ref eth.BlockRef, logCount uint32, execMsgs map[uint32]*types.ExecutingMessage, err error) {
 			// include one executing message to ensure one hazard is returned
-			return bl, 0, map[uint32]*types.ExecutingMessage{1: em1}, nil
+			return bl, 2, map[uint32]*types.ExecutingMessage{1: em1}, nil
 		}
 		usd.deps = mockDependencySet{}
 		var updatingChainID types.ChainID
@@ -168,6 +193,7 @@ type mockCrossUnsafeDeps struct {
 	crossUnsafeFn       func(chainID types.ChainID) (types.BlockSeal, error)
 	openBlockFn         func(chainID types.ChainID, blockNum uint64) (ref eth.BlockRef, logCount uint32, execMsgs map[uint32]*types.ExecutingMessage, err error)
 	updateCrossUnsafeFn func(chain types.ChainID, crossUnsafe types.BlockSeal) error
+	checkFn             func(chainID types.ChainID, blockNum uint64, logIdx uint32, logHash common.Hash) (types.BlockSeal, error)
 }
 
 func (m *mockCrossUnsafeDeps) CrossUnsafe(chainID types.ChainID) (derived types.BlockSeal, err error) {
@@ -182,6 +208,9 @@ func (m *mockCrossUnsafeDeps) DependencySet() depset.DependencySet {
 }
 
 func (m *mockCrossUnsafeDeps) Check(chainID types.ChainID, blockNum uint64, logIdx uint32, logHash common.Hash) (types.BlockSeal, error) {
+	if m.checkFn != nil {
+		return m.checkFn(chainID, blockNum, logIdx, logHash)
+	}
 	return types.BlockSeal{}, nil
 }