Merge pull request #6948 from ethereum-optimism/aj/skip-complete-games

op-challenger: Don't progress completed games

op-challenger/fault/caller.go

@@ -8,7 +8,6 @@ import (
 	"github.com/ethereum-optimism/optimism/op-challenger/fault/types"
 	"github.com/ethereum/go-ethereum/accounts/abi/bind"
 	"github.com/ethereum/go-ethereum/common"
-	"github.com/ethereum/go-ethereum/ethclient"
 )
 
 type FaultDisputeGameCaller interface {
@@ -26,7 +25,7 @@ func NewFaultCaller(caller FaultDisputeGameCaller) *FaultCaller {
 	}
 }
 
-func NewFaultCallerFromBindings(fdgAddr common.Address, client *ethclient.Client) (*FaultCaller, error) {
+func NewFaultCallerFromBindings(fdgAddr common.Address, client bind.ContractCaller) (*FaultCaller, error) {
 	caller, err := bindings.NewFaultDisputeGameCaller(fdgAddr, client)
 	if err != nil {
 		return nil, err

op-challenger/fault/monitor.go

@@ -80,11 +80,13 @@ func (m *gameMonitor) progressGames(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("failed to load games: %w", err)
 	}
+	requiredGames := make(map[common.Address]bool)
 	for _, game := range games {
 		if !m.allowedGame(game.Proxy) {
 			m.logger.Debug("Skipping game not on allow list", "game", game.Proxy)
 			continue
 		}
+		requiredGames[game.Proxy] = true
 		player, err := m.fetchOrCreateGamePlayer(game)
 		if err != nil {
 			m.logger.Error("Error while progressing game", "game", game.Proxy, "err", err)
@@ -92,6 +94,14 @@ func (m *gameMonitor) progressGames(ctx context.Context) error {
 		}
 		player.ProgressGame(ctx)
 	}
+	// Remove the player for any game that's no longer being returned from the list of active games
+	for addr := range m.players {
+		if _, ok := requiredGames[addr]; ok {
+			// Game still required
+			continue
+		}
+		delete(m.players, addr)
+	}
 	return nil
 }
 

op-challenger/fault/monitor_test.go

@@ -104,6 +104,48 @@ func TestMonitorOnlyCreateSpecifiedGame(t *testing.T) {
 	require.Equal(t, 1, games.created[addr2].progressCount)
 }
 
+func TestDeletePlayersWhenNoLongerInListOfGames(t *testing.T) {
+	addr1 := common.Address{0xaa}
+	addr2 := common.Address{0xbb}
+	monitor, source, games := setupMonitorTest(t, nil)
+
+	allGames := []FaultDisputeGame{
+		{
+			Proxy:     addr1,
+			Timestamp: 9999,
+		},
+		{
+			Proxy:     addr2,
+			Timestamp: 9999,
+		},
+	}
+	source.games = allGames
+
+	require.NoError(t, monitor.progressGames(context.Background()))
+	require.Len(t, games.created, 2)
+	require.Contains(t, games.created, addr1)
+	require.Contains(t, games.created, addr2)
+
+	// First game is now old enough it's not returned in the list of active games
+	source.games = source.games[1:]
+	require.NoError(t, monitor.progressGames(context.Background()))
+	require.Len(t, games.created, 2)
+	require.Contains(t, games.created, addr1)
+	require.Contains(t, games.created, addr2)
+
+	// Forget that we created the first game so it can be recreated if needed
+	delete(games.created, addr1)
+
+	// First game now reappears (inexplicably but usefully for our testing)
+	source.games = allGames
+	require.NoError(t, monitor.progressGames(context.Background()))
+	// A new player is created for it because the original was deleted
+	require.Len(t, games.created, 2)
+	require.Contains(t, games.created, addr1)
+	require.Contains(t, games.created, addr2)
+	require.Equal(t, 1, games.created[addr1].progressCount)
+}
+
 func setupMonitorTest(t *testing.T, allowedGames []common.Address) (*gameMonitor, *stubGameSource, *createdGames) {
 	logger := testlog.Logger(t, log.LvlDebug)
 	source := &stubGameSource{}

op-challenger/fault/player.go

@@ -10,8 +10,8 @@ import (
 	"github.com/ethereum-optimism/optimism/op-challenger/fault/cannon"
 	"github.com/ethereum-optimism/optimism/op-challenger/fault/types"
 	"github.com/ethereum-optimism/optimism/op-service/txmgr"
+	"github.com/ethereum/go-ethereum/accounts/abi/bind"
 	"github.com/ethereum/go-ethereum/common"
-	"github.com/ethereum/go-ethereum/ethclient"
 	"github.com/ethereum/go-ethereum/log"
 )
 
@@ -30,6 +30,8 @@ type GamePlayer struct {
 	caller                  GameInfo
 	logger                  log.Logger
 	cleanup                 func() error
+
+	completed bool
 }
 
 func NewGamePlayer(
@@ -38,7 +40,7 @@ func NewGamePlayer(
 	cfg *config.Config,
 	addr common.Address,
 	txMgr txmgr.TxManager,
-	client *ethclient.Client,
+	client bind.ContractCaller,
 ) (*GamePlayer, error) {
 	logger = logger.New("game", addr)
 	contract, err := bindings.NewFaultDisputeGameCaller(addr, client)
@@ -100,6 +102,11 @@ func NewGamePlayer(
 }
 
 func (g *GamePlayer) ProgressGame(ctx context.Context) bool {
+	if g.completed {
+		// Game is already complete so don't try to perform further actions.
+		g.logger.Trace("Skipping completed game")
+		return true
+	}
 	g.logger.Trace("Checking if actions are required")
 	if err := g.agent.Act(ctx); err != nil {
 		g.logger.Error("Error when acting on game", "err", err)
@@ -108,7 +115,8 @@ func (g *GamePlayer) ProgressGame(ctx context.Context) bool {
 		g.logger.Warn("Unable to retrieve game status", "err", err)
 	} else {
 		g.logGameStatus(ctx, status)
-		return status != types.GameStatusInProgress
+		g.completed = status != types.GameStatusInProgress
+		return g.completed
 	}
 	return false
 }

op-challenger/fault/player_test.go

@@ -12,14 +12,14 @@ import (
 )
 
 func TestProgressGame_LogErrorFromAct(t *testing.T) {
-	handler, game, actor, _ := setupProgressGameTest(t, true)
-	actor.err = errors.New("boom")
+	handler, game, actor := setupProgressGameTest(t, true)
+	actor.actErr = errors.New("boom")
 	done := game.ProgressGame(context.Background())
 	require.False(t, done, "should not be done")
 	require.Equal(t, 1, actor.callCount, "should perform next actions")
 	errLog := handler.FindLog(log.LvlError, "Error when acting on game")
 	require.NotNil(t, errLog, "should log error")
-	require.Equal(t, actor.err, errLog.GetContextValue("err"))
+	require.Equal(t, actor.actErr, errLog.GetContextValue("err"))
 
 	// Should still log game status
 	msg := handler.FindLog(log.LvlInfo, "Game info")
@@ -92,11 +92,11 @@ func TestProgressGame_LogGameStatus(t *testing.T) {
 	for _, test := range tests {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
-			handler, game, actor, gameInfo := setupProgressGameTest(t, test.agreeWithOutput)
-			gameInfo.status = test.status
+			handler, game, gameState := setupProgressGameTest(t, test.agreeWithOutput)
+			gameState.status = test.status
 
 			done := game.ProgressGame(context.Background())
-			require.Equal(t, 1, actor.callCount, "should perform next actions")
+			require.Equal(t, 1, gameState.callCount, "should perform next actions")
 			require.Equal(t, test.status != types.GameStatusInProgress, done, "should be done when not in progress")
 			errLog := handler.FindLog(test.logLevel, test.logMsg)
 			require.NotNil(t, errLog, "should log game result")
@@ -105,43 +105,57 @@ func TestProgressGame_LogGameStatus(t *testing.T) {
 	}
 }
 
-func setupProgressGameTest(t *testing.T, agreeWithProposedRoot bool) (*testlog.CapturingHandler, *GamePlayer, *stubActor, *stubGameInfo) {
+func TestDoNotActOnCompleteGame(t *testing.T) {
+	for _, status := range []types.GameStatus{types.GameStatusChallengerWon, types.GameStatusDefenderWon} {
+		t.Run(status.String(), func(t *testing.T) {
+			_, game, gameState := setupProgressGameTest(t, true)
+			gameState.status = status
+
+			done := game.ProgressGame(context.Background())
+			require.Equal(t, 1, gameState.callCount, "acts the first time")
+			require.True(t, done, "should be done")
+
+			// Should not act when it knows the game is already complete
+			done = game.ProgressGame(context.Background())
+			require.Equal(t, 1, gameState.callCount, "does not act after game is complete")
+			require.True(t, done, "should still be done")
+		})
+	}
+}
+
+func setupProgressGameTest(t *testing.T, agreeWithProposedRoot bool) (*testlog.CapturingHandler, *GamePlayer, *stubGameState) {
 	logger := testlog.Logger(t, log.LvlDebug)
 	handler := &testlog.CapturingHandler{
 		Delegate: logger.GetHandler(),
 	}
 	logger.SetHandler(handler)
-	actor := &stubActor{}
-	gameInfo := &stubGameInfo{claimCount: 1}
+	gameState := &stubGameState{claimCount: 1}
 	game := &GamePlayer{
-		agent:                   actor,
+		agent:                   gameState,
 		agreeWithProposedOutput: agreeWithProposedRoot,
-		caller:                  gameInfo,
+		caller:                  gameState,
 		logger:                  logger,
 	}
-	return handler, game, actor, gameInfo
+	return handler, game, gameState
 }
 
-type stubActor struct {
-	callCount int
-	err       error
-}
-
-func (a *stubActor) Act(ctx context.Context) error {
-	a.callCount++
-	return a.err
-}
-
-type stubGameInfo struct {
+type stubGameState struct {
 	status     types.GameStatus
 	claimCount uint64
-	err        error
+	callCount  int
+	actErr     error
+	Err        error
+}
+
+func (s *stubGameState) Act(ctx context.Context) error {
+	s.callCount++
+	return s.actErr
 }
 
-func (s *stubGameInfo) GetGameStatus(ctx context.Context) (types.GameStatus, error) {
-	return s.status, s.err
+func (s *stubGameState) GetGameStatus(ctx context.Context) (types.GameStatus, error) {
+	return s.status, nil
 }
 
-func (s *stubGameInfo) GetClaimCount(ctx context.Context) (uint64, error) {
-	return s.claimCount, s.err
+func (s *stubGameState) GetClaimCount(ctx context.Context) (uint64, error) {
+	return s.claimCount, nil
 }