chore(op-dispute-mon): Detector Type Refactor (#9443)

* chore(op-dispute-mon): refactor types and unit tests

* chore(op-dispute-mon): refactor output validation into a separate component

---------
Co-authored-by: Adrian Sutton <adrian@oplabs.co>

op-dispute-mon/mon/detector.go

@@ -5,45 +5,13 @@ import (
 	"fmt"
 
 	"github.com/ethereum-optimism/optimism/op-challenger/game/types"
-	"github.com/ethereum-optimism/optimism/op-service/eth"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/log"
 )
 
-type statusBatch struct {
-	inProgress, defenderWon, challengerWon int
-}
-
-func (s *statusBatch) Add(status types.GameStatus) {
-	switch status {
-	case types.GameStatusInProgress:
-		s.inProgress++
-	case types.GameStatusDefenderWon:
-		s.defenderWon++
-	case types.GameStatusChallengerWon:
-		s.challengerWon++
-	}
-}
-
-type detectionBatch struct {
-	inProgress             int
-	agreeDefenderWins      int
-	disagreeDefenderWins   int
-	agreeChallengerWins    int
-	disagreeChallengerWins int
-}
-
-func (d *detectionBatch) merge(other detectionBatch) {
-	d.inProgress += other.inProgress
-	d.agreeDefenderWins += other.agreeDefenderWins
-	d.disagreeDefenderWins += other.disagreeDefenderWins
-	d.agreeChallengerWins += other.agreeChallengerWins
-	d.disagreeChallengerWins += other.disagreeChallengerWins
-}
-
-type OutputRollupClient interface {
-	OutputAtBlock(ctx context.Context, blockNum uint64) (*eth.OutputResponse, error)
+type OutputValidator interface {
+	CheckRootAgreement(ctx context.Context, blockNum uint64, root common.Hash) (bool, common.Hash, error)
 }
 
 type MetadataCreator interface {
@@ -56,18 +24,18 @@ type DetectorMetrics interface {
 }
 
 type detector struct {
-	logger       log.Logger
-	metrics      DetectorMetrics
-	creator      MetadataCreator
-	outputClient OutputRollupClient
+	logger    log.Logger
+	metrics   DetectorMetrics
+	creator   MetadataCreator
+	validator OutputValidator
 }
 
-func newDetector(logger log.Logger, metrics DetectorMetrics, creator MetadataCreator, outputClient OutputRollupClient) *detector {
+func newDetector(logger log.Logger, metrics DetectorMetrics, creator MetadataCreator, validator OutputValidator) *detector {
 	return &detector{
-		logger:       logger,
-		metrics:      metrics,
-		creator:      creator,
-		outputClient: outputClient,
+		logger:    logger,
+		metrics:   metrics,
+		creator:   creator,
+		validator: validator,
 	}
 }
 
@@ -88,7 +56,7 @@ func (d *detector) Detect(ctx context.Context, games []types.GameMetadata) {
 			d.logger.Error("Failed to process game", "err", err)
 			continue
 		}
-		detectBatch.merge(processed)
+		detectBatch.Merge(processed)
 	}
 	d.metrics.RecordGamesStatus(statBatch.inProgress, statBatch.defenderWon, statBatch.challengerWon)
 	d.recordBatch(detectBatch)
@@ -116,36 +84,17 @@ func (d *detector) fetchGameMetadata(ctx context.Context, game types.GameMetadat
 }
 
 func (d *detector) checkAgreement(ctx context.Context, addr common.Address, blockNum uint64, rootClaim common.Hash, status types.GameStatus) (detectionBatch, error) {
-	agree, err := d.checkRootAgreement(ctx, blockNum, rootClaim)
+	agree, expected, err := d.validator.CheckRootAgreement(ctx, blockNum, rootClaim)
 	if err != nil {
 		return detectionBatch{}, err
 	}
 	batch := detectionBatch{}
-	switch status {
-	case types.GameStatusInProgress:
-		batch.inProgress++
-	case types.GameStatusDefenderWon:
-		if agree {
-			batch.agreeDefenderWins++
-		} else {
-			batch.disagreeDefenderWins++
-			d.logger.Error("Defender won but root claim does not match", "gameAddr", addr, "rootClaim", rootClaim)
-		}
-	case types.GameStatusChallengerWon:
-		if agree {
-			batch.agreeChallengerWins++
-		} else {
-			batch.disagreeChallengerWins++
-			d.logger.Error("Challenger won but root claim does not match", "gameAddr", addr, "rootClaim", rootClaim)
-		}
+	batch.Update(status, agree)
+	if !agree && status == types.GameStatusChallengerWon {
+		d.logger.Error("Challenger won but expected defender to win", "gameAddr", addr, "rootClaim", rootClaim, "expected", expected)
 	}
-	return batch, nil
-}
-
-func (d *detector) checkRootAgreement(ctx context.Context, blockNum uint64, rootClaim common.Hash) (bool, error) {
-	output, err := d.outputClient.OutputAtBlock(ctx, blockNum)
-	if err != nil {
-		return false, fmt.Errorf("failed to get output at block: %w", err)
+	if !agree && status == types.GameStatusDefenderWon {
+		d.logger.Error("Defender won but expected challenger to win", "gameAddr", addr, "rootClaim", rootClaim, "expected", expected)
 	}
-	return rootClaim == common.Hash(output.OutputRoot), nil
+	return batch, nil
 }

op-dispute-mon/mon/detector_test.go

@@ -6,17 +6,12 @@ import (
 	"testing"
 
 	"github.com/ethereum-optimism/optimism/op-challenger/game/types"
-	"github.com/ethereum-optimism/optimism/op-service/eth"
 	"github.com/ethereum-optimism/optimism/op-service/testlog"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/stretchr/testify/require"
 )
 
-var (
-	mockRootClaim = common.HexToHash("0x10")
-)
-
 func TestDetector_Detect(t *testing.T) {
 	t.Parallel()
 
@@ -213,50 +208,25 @@ func TestDetector_CheckAgreement_Succeeds(t *testing.T) {
 	}
 }
 
-func TestDetector_CheckRootAgreement(t *testing.T) {
-	t.Parallel()
-
-	t.Run("OutputFetchFails", func(t *testing.T) {
-		detector, _, _, rollup := setupDetectorTest(t)
-		rollup.err = errors.New("boom")
-		agree, err := detector.checkRootAgreement(context.Background(), 0, mockRootClaim)
-		require.ErrorIs(t, err, rollup.err)
-		require.False(t, agree)
-	})
-
-	t.Run("OutputMismatch", func(t *testing.T) {
-		detector, _, _, _ := setupDetectorTest(t)
-		agree, err := detector.checkRootAgreement(context.Background(), 0, common.Hash{})
-		require.NoError(t, err)
-		require.False(t, agree)
-	})
-
-	t.Run("OutputMatches", func(t *testing.T) {
-		detector, _, _, _ := setupDetectorTest(t)
-		agree, err := detector.checkRootAgreement(context.Background(), 0, mockRootClaim)
-		require.NoError(t, err)
-		require.True(t, agree)
-	})
-}
-
-func setupDetectorTest(t *testing.T) (*detector, *mockDetectorMetricer, *mockMetadataCreator, *stubRollupClient) {
+func setupDetectorTest(t *testing.T) (*detector, *mockDetectorMetricer, *mockMetadataCreator, *stubOutputValidator) {
 	logger := testlog.Logger(t, log.LvlDebug)
 	metrics := &mockDetectorMetricer{}
 	loader := &mockMetadataLoader{}
 	creator := &mockMetadataCreator{loader: loader}
-	rollupClient := &stubRollupClient{}
-	detector := newDetector(logger, metrics, creator, rollupClient)
-	return detector, metrics, creator, rollupClient
+	validator := &stubOutputValidator{}
+	detector := newDetector(logger, metrics, creator, validator)
+	return detector, metrics, creator, validator
 }
 
-type stubRollupClient struct {
-	blockNum uint64
-	err      error
+type stubOutputValidator struct {
+	err error
 }
 
-func (s *stubRollupClient) OutputAtBlock(ctx context.Context, blockNum uint64) (*eth.OutputResponse, error) {
-	s.blockNum = blockNum
-	return &eth.OutputResponse{OutputRoot: eth.Bytes32(mockRootClaim)}, s.err
+func (s *stubOutputValidator) CheckRootAgreement(ctx context.Context, blockNum uint64, rootClaim common.Hash) (bool, common.Hash, error) {
+	if s.err != nil {
+		return false, common.Hash{}, s.err
+	}
+	return rootClaim == mockRootClaim, mockRootClaim, nil
 }
 
 type mockMetadataCreator struct {

op-dispute-mon/mon/service.go

@@ -37,6 +37,7 @@ type Service struct {
 	metadata     *metadataCreator
 	rollupClient *sources.RollupClient
 	detector     *detector
+	validator    *outputValidator
 
 	l1Client *ethclient.Client
 
@@ -78,6 +79,8 @@ func (s *Service) initFromConfig(ctx context.Context, cfg *config.Config) error
 		return fmt.Errorf("failed to init rollup client: %w", err)
 	}
 	s.initMetadataCreator()
+	s.initOutputValidator()
+	s.initDetector()
 	s.initDetector()
 	s.initMonitor(ctx, cfg)
 
@@ -87,8 +90,12 @@ func (s *Service) initFromConfig(ctx context.Context, cfg *config.Config) error
 	return nil
 }
 
+func (s *Service) initOutputValidator() {
+	s.validator = newOutputValidator(s.rollupClient)
+}
+
 func (s *Service) initDetector() {
-	s.detector = newDetector(s.logger, s.metrics, s.metadata, s.rollupClient)
+	s.detector = newDetector(s.logger, s.metrics, s.metadata, s.validator)
 }
 
 func (s *Service) initOutputRollupClient(ctx context.Context, cfg *config.Config) error {

op-dispute-mon/mon/types.go

+package mon
+
+import (
+	"github.com/ethereum-optimism/optimism/op-challenger/game/types"
+)
+
+type statusBatch struct {
+	inProgress, defenderWon, challengerWon int
+}
+
+func (s *statusBatch) Add(status types.GameStatus) {
+	switch status {
+	case types.GameStatusInProgress:
+		s.inProgress++
+	case types.GameStatusDefenderWon:
+		s.defenderWon++
+	case types.GameStatusChallengerWon:
+		s.challengerWon++
+	}
+}
+
+type detectionBatch struct {
+	inProgress             int
+	agreeDefenderWins      int
+	disagreeDefenderWins   int
+	agreeChallengerWins    int
+	disagreeChallengerWins int
+}
+
+func (d *detectionBatch) Update(status types.GameStatus, agree bool) {
+	switch status {
+	case types.GameStatusInProgress:
+		d.inProgress++
+	case types.GameStatusDefenderWon:
+		if agree {
+			d.agreeDefenderWins++
+		} else {
+			d.disagreeDefenderWins++
+		}
+	case types.GameStatusChallengerWon:
+		if agree {
+			d.agreeChallengerWins++
+		} else {
+			d.disagreeChallengerWins++
+		}
+	}
+}
+
+func (d *detectionBatch) Merge(other detectionBatch) {
+	d.inProgress += other.inProgress
+	d.agreeDefenderWins += other.agreeDefenderWins
+	d.disagreeDefenderWins += other.disagreeDefenderWins
+	d.agreeChallengerWins += other.agreeChallengerWins
+	d.disagreeChallengerWins += other.disagreeChallengerWins
+}

op-dispute-mon/mon/types_test.go

+package mon
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/ethereum-optimism/optimism/op-challenger/game/types"
+	"github.com/stretchr/testify/require"
+)
+
+func TestStatusBatch_Add(t *testing.T) {
+	statusExpectations := []struct {
+		status types.GameStatus
+		create func(int) statusBatch
+	}{
+		{
+			status: types.GameStatusInProgress,
+			create: func(inProgress int) statusBatch {
+				return statusBatch{inProgress, 0, 0}
+			},
+		},
+		{
+			status: types.GameStatusDefenderWon,
+			create: func(defenderWon int) statusBatch {
+				return statusBatch{0, defenderWon, 0}
+			},
+		},
+		{
+			status: types.GameStatusChallengerWon,
+			create: func(challengerWon int) statusBatch {
+				return statusBatch{0, 0, challengerWon}
+			},
+		},
+	}
+
+	type test struct {
+		name        string
+		status      types.GameStatus
+		invocations int
+		expected    statusBatch
+	}
+
+	var tests []test
+	for i := 0; i < 100; i++ {
+		for _, exp := range statusExpectations {
+			tests = append(tests, test{
+				name:        fmt.Sprintf("Invocation-%d", i),
+				status:      exp.status,
+				invocations: i,
+				expected:    exp.create(i),
+			})
+		}
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			s := statusBatch{}
+			for i := 0; i < test.invocations; i++ {
+				s.Add(test.status)
+			}
+			require.Equal(t, test.expected, s)
+		})
+	}
+}
+
+func TestDetectionBatch_Update(t *testing.T) {
+	statusExpectations := []struct {
+		status types.GameStatus
+		create func(int, bool) detectionBatch
+	}{
+		{
+			status: types.GameStatusInProgress,
+			create: func(inProgress int, _ bool) detectionBatch {
+				return detectionBatch{inProgress, 0, 0, 0, 0}
+			},
+		},
+		{
+			status: types.GameStatusDefenderWon,
+			create: func(defenderWon int, agree bool) detectionBatch {
+				if agree {
+					return detectionBatch{0, defenderWon, 0, 0, 0}
+				}
+				return detectionBatch{0, 0, defenderWon, 0, 0}
+			},
+		},
+		{
+			status: types.GameStatusChallengerWon,
+			create: func(challengerWon int, agree bool) detectionBatch {
+				if agree {
+					return detectionBatch{0, 0, 0, challengerWon, 0}
+				}
+				return detectionBatch{0, 0, 0, 0, challengerWon}
+			},
+		},
+	}
+
+	type test struct {
+		name        string
+		status      types.GameStatus
+		agree       bool
+		invocations int
+		expected    detectionBatch
+	}
+
+	var tests []test
+	for i := 0; i < 100; i++ {
+		for _, exp := range statusExpectations {
+			agree := i%2 == 0
+			tests = append(tests, test{
+				name:        fmt.Sprintf("Invocation-%d", i),
+				status:      exp.status,
+				agree:       agree,
+				invocations: i,
+				expected:    exp.create(i, agree),
+			})
+		}
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			d := detectionBatch{}
+			for i := 0; i < test.invocations; i++ {
+				d.Update(test.status, test.agree)
+			}
+			require.Equal(t, test.expected, d)
+		})
+	}
+}
+
+func TestDetectionBatch_Merge(t *testing.T) {
+	type test struct {
+		name     string
+		merge    detectionBatch
+		expected detectionBatch
+	}
+
+	tests := []test{
+		{
+			name:     "Empty",
+			merge:    detectionBatch{},
+			expected: detectionBatch{},
+		},
+		{
+			name:     "InProgress",
+			merge:    detectionBatch{1, 0, 0, 0, 0},
+			expected: detectionBatch{1, 0, 0, 0, 0},
+		},
+		{
+			name:     "AgreeDefenderWins",
+			merge:    detectionBatch{0, 1, 0, 0, 0},
+			expected: detectionBatch{0, 1, 0, 0, 0},
+		},
+		{
+			name:     "DisagreeDefenderWins",
+			merge:    detectionBatch{0, 0, 1, 0, 0},
+			expected: detectionBatch{0, 0, 1, 0, 0},
+		},
+		{
+			name:     "AgreeChallengerWins",
+			merge:    detectionBatch{0, 0, 0, 1, 0},
+			expected: detectionBatch{0, 0, 0, 1, 0},
+		},
+		{
+			name:     "DisagreeChallengerWins",
+			merge:    detectionBatch{0, 0, 0, 0, 1},
+			expected: detectionBatch{0, 0, 0, 0, 1},
+		},
+		{
+			name:     "All",
+			merge:    detectionBatch{1, 1, 1, 1, 1},
+			expected: detectionBatch{1, 1, 1, 1, 1},
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			d := detectionBatch{}
+			d.Merge(test.merge)
+			require.Equal(t, test.expected, d)
+		})
+	}
+}

op-dispute-mon/mon/validator.go

+package mon
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/ethereum/go-ethereum/common"
+
+	"github.com/ethereum-optimism/optimism/op-service/eth"
+)
+
+type OutputRollupClient interface {
+	OutputAtBlock(ctx context.Context, blockNum uint64) (*eth.OutputResponse, error)
+}
+
+type outputValidator struct {
+	client OutputRollupClient
+}
+
+func newOutputValidator(client OutputRollupClient) *outputValidator {
+	return &outputValidator{
+		client: client,
+	}
+}
+
+// CheckRootAgreement validates the specified root claim against the output at the given block number.
+func (o *outputValidator) CheckRootAgreement(ctx context.Context, blockNum uint64, rootClaim common.Hash) (bool, common.Hash, error) {
+	output, err := o.client.OutputAtBlock(ctx, blockNum)
+	if err != nil {
+		return false, common.Hash{}, fmt.Errorf("failed to get output at block: %w", err)
+	}
+	expected := common.Hash(output.OutputRoot)
+	return rootClaim == expected, expected, nil
+}

op-dispute-mon/mon/validator_test.go

+package mon
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/ethereum-optimism/optimism/op-service/eth"
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/stretchr/testify/require"
+)
+
+var (
+	mockRootClaim = common.HexToHash("0x10")
+)
+
+func TestDetector_CheckRootAgreement(t *testing.T) {
+	t.Parallel()
+
+	t.Run("OutputFetchFails", func(t *testing.T) {
+		validator, rollup := setupOutputValidatorTest(t)
+		rollup.err = errors.New("boom")
+		agree, fetched, err := validator.CheckRootAgreement(context.Background(), 0, mockRootClaim)
+		require.ErrorIs(t, err, rollup.err)
+		require.Equal(t, common.Hash{}, fetched)
+		require.False(t, agree)
+	})
+
+	t.Run("OutputMismatch", func(t *testing.T) {
+		validator, _ := setupOutputValidatorTest(t)
+		agree, fetched, err := validator.CheckRootAgreement(context.Background(), 0, common.Hash{})
+		require.NoError(t, err)
+		require.Equal(t, mockRootClaim, fetched)
+		require.False(t, agree)
+	})
+
+	t.Run("OutputMatches", func(t *testing.T) {
+		validator, _ := setupOutputValidatorTest(t)
+		agree, fetched, err := validator.CheckRootAgreement(context.Background(), 0, mockRootClaim)
+		require.NoError(t, err)
+		require.Equal(t, mockRootClaim, fetched)
+		require.True(t, agree)
+	})
+}
+
+func setupOutputValidatorTest(t *testing.T) (*outputValidator, *stubRollupClient) {
+	client := &stubRollupClient{}
+	validator := newOutputValidator(client)
+	return validator, client
+}
+
+type stubRollupClient struct {
+	blockNum uint64
+	err      error
+}
+
+func (s *stubRollupClient) OutputAtBlock(ctx context.Context, blockNum uint64) (*eth.OutputResponse, error) {
+	s.blockNum = blockNum
+	return &eth.OutputResponse{OutputRoot: eth.Bytes32(mockRootClaim)}, s.err
+}