diff --git a/op-supervisor/supervisor/backend/cross/cycle.go b/op-supervisor/supervisor/backend/cross/cycle.go index a872bc75a860..9884856a34a5 100644 --- a/op-supervisor/supervisor/backend/cross/cycle.go +++ b/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 diff --git a/op-supervisor/supervisor/backend/cross/cycle_test.go b/op-supervisor/supervisor/backend/cross/cycle_test.go index a90258b88a26..e160023caf81 100644 --- a/op-supervisor/supervisor/backend/cross/cycle_test.go +++ b/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", diff --git a/op-supervisor/supervisor/backend/cross/safe_update.go b/op-supervisor/supervisor/backend/cross/safe_update.go index 172277db52de..6d74ca8ad2ba 100644 --- a/op-supervisor/supervisor/backend/cross/safe_update.go +++ b/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 { diff --git a/op-supervisor/supervisor/backend/cross/safe_update_test.go b/op-supervisor/supervisor/backend/cross/safe_update_test.go index fec5f69b8ca4..b3361c711bd0 100644 --- a/op-supervisor/supervisor/backend/cross/safe_update_test.go +++ b/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 } diff --git a/op-supervisor/supervisor/backend/cross/unsafe_update.go b/op-supervisor/supervisor/backend/cross/unsafe_update.go index bc13146d38ff..56d396127158 100644 --- a/op-supervisor/supervisor/backend/cross/unsafe_update.go +++ b/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 { diff --git a/op-supervisor/supervisor/backend/cross/unsafe_update_test.go b/op-supervisor/supervisor/backend/cross/unsafe_update_test.go index c0e264c97206..ada7d0756cf4 100644 --- a/op-supervisor/supervisor/backend/cross/unsafe_update_test.go +++ b/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 }