Merge pull request #7082 from ethereum-optimism/aj/no-move-on-lost-games

op-challenger: Don't try to perform moves on resolvable games.

op-challenger/game/fault/agent.go

@@ -74,7 +74,7 @@ func (a *Agent) Act(ctx context.Context) error {
 
 // shouldResolve returns true if the agent should resolve the game.
 // This method will return false if the game is still in progress.
-func (a *Agent) shouldResolve(ctx context.Context, status types.GameStatus) bool {
+func (a *Agent) shouldResolve(status types.GameStatus) bool {
 	expected := types.GameStatusDefenderWon
 	if a.agreeWithProposedOutput {
 		expected = types.GameStatusChallengerWon
@@ -85,20 +85,19 @@ func (a *Agent) shouldResolve(ctx context.Context, status types.GameStatus) bool
 	return expected == status
 }
 
-// tryResolve resolves the game if it is in a terminal state
-// and returns true if the game resolves successfully.
+// tryResolve resolves the game if it is in a winning state
+// Returns true if the game is resolvable (regardless of whether it was actually resolved)
 func (a *Agent) tryResolve(ctx context.Context) bool {
 	status, err := a.responder.CallResolve(ctx)
-	if err != nil {
+	if err != nil || status == types.GameStatusInProgress {
 		return false
 	}
-	if !a.shouldResolve(ctx, status) {
-		return false
+	if !a.shouldResolve(status) {
+		return true
 	}
 	a.log.Info("Resolving game")
 	if err := a.responder.Resolve(ctx); err != nil {
 		a.log.Error("Failed to resolve the game", "err", err)
-		return false
 	}
 	return true
 }

op-challenger/game/fault/agent_test.go

@@ -2,8 +2,11 @@ package fault
 
 import (
 	"context"
+	"errors"
 	"testing"
 
+	"github.com/ethereum-optimism/optimism/op-challenger/game/fault/test"
+	"github.com/ethereum-optimism/optimism/op-challenger/game/fault/trace/alphabet"
 	"github.com/ethereum-optimism/optimism/op-challenger/game/fault/types"
 	"github.com/ethereum-optimism/optimism/op-challenger/metrics"
 	"github.com/ethereum/go-ethereum/log"
@@ -14,19 +17,143 @@ import (
 
 // TestShouldResolve tests the resolution logic.
 func TestShouldResolve(t *testing.T) {
-	log := testlog.Logger(t, log.LvlCrit)
-
 	t.Run("AgreeWithProposedOutput", func(t *testing.T) {
-		agent := NewAgent(metrics.NoopMetrics, nil, 0, nil, nil, nil, true, log)
-		require.False(t, agent.shouldResolve(context.Background(), types.GameStatusDefenderWon))
-		require.True(t, agent.shouldResolve(context.Background(), types.GameStatusChallengerWon))
-		require.False(t, agent.shouldResolve(context.Background(), types.GameStatusInProgress))
+		agent, _, _ := setupTestAgent(t, true)
+		require.False(t, agent.shouldResolve(types.GameStatusDefenderWon))
+		require.True(t, agent.shouldResolve(types.GameStatusChallengerWon))
+		require.False(t, agent.shouldResolve(types.GameStatusInProgress))
 	})
 
 	t.Run("DisagreeWithProposedOutput", func(t *testing.T) {
-		agent := NewAgent(metrics.NoopMetrics, nil, 0, nil, nil, nil, false, log)
-		require.True(t, agent.shouldResolve(context.Background(), types.GameStatusDefenderWon))
-		require.False(t, agent.shouldResolve(context.Background(), types.GameStatusChallengerWon))
-		require.False(t, agent.shouldResolve(context.Background(), types.GameStatusInProgress))
+		agent, _, _ := setupTestAgent(t, false)
+		require.True(t, agent.shouldResolve(types.GameStatusDefenderWon))
+		require.False(t, agent.shouldResolve(types.GameStatusChallengerWon))
+		require.False(t, agent.shouldResolve(types.GameStatusInProgress))
 	})
 }
+
+func TestDoNotMakeMovesWhenGameIsResolvable(t *testing.T) {
+	ctx := context.Background()
+
+	tests := []struct {
+		name                    string
+		agreeWithProposedOutput bool
+		callResolveStatus       types.GameStatus
+		shouldResolve           bool
+	}{
+		{
+			name:                    "Agree_Losing",
+			agreeWithProposedOutput: true,
+			callResolveStatus:       types.GameStatusDefenderWon,
+			shouldResolve:           false,
+		},
+		{
+			name:                    "Agree_Winning",
+			agreeWithProposedOutput: true,
+			callResolveStatus:       types.GameStatusChallengerWon,
+			shouldResolve:           true,
+		},
+		{
+			name:                    "Disagree_Losing",
+			agreeWithProposedOutput: false,
+			callResolveStatus:       types.GameStatusChallengerWon,
+			shouldResolve:           false,
+		},
+		{
+			name:                    "Disagree_Winning",
+			agreeWithProposedOutput: false,
+			callResolveStatus:       types.GameStatusDefenderWon,
+			shouldResolve:           true,
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			agent, claimLoader, responder := setupTestAgent(t, test.agreeWithProposedOutput)
+			responder.callResolveStatus = test.callResolveStatus
+
+			require.NoError(t, agent.Act(ctx))
+
+			require.Equal(t, 1, responder.callResolveCount, "should check if game is resolvable")
+			require.Zero(t, claimLoader.callCount, "should not fetch claims for resolvable game")
+
+			if test.shouldResolve {
+				require.EqualValues(t, 1, responder.resolveCount, "should resolve winning game")
+			} else {
+				require.Zero(t, responder.resolveCount, "should not resolve losing game")
+			}
+		})
+	}
+}
+
+func TestLoadClaimsWhenGameNotResolvable(t *testing.T) {
+	// Checks that if the game isn't resolvable, that the agent continues on to start checking claims
+	agent, claimLoader, responder := setupTestAgent(t, false)
+	responder.callResolveErr = errors.New("game is not resolvable")
+	depth := 4
+	claimBuilder := test.NewClaimBuilder(t, depth, alphabet.NewTraceProvider("abcdefg", uint64(depth)))
+
+	claimLoader.claims = []types.Claim{
+		claimBuilder.CreateRootClaim(true),
+	}
+
+	require.NoError(t, agent.Act(context.Background()))
+
+	require.EqualValues(t, 1, claimLoader.callCount, "should load claims for unresolvable game")
+}
+
+func setupTestAgent(t *testing.T, agreeWithProposedOutput bool) (*Agent, *stubClaimLoader, *stubResponder) {
+	logger := testlog.Logger(t, log.LvlInfo)
+	claimLoader := &stubClaimLoader{}
+	depth := 4
+	trace := alphabet.NewTraceProvider("abcd", uint64(depth))
+	responder := &stubResponder{}
+	updater := &stubUpdater{}
+	agent := NewAgent(metrics.NoopMetrics, claimLoader, depth, trace, responder, updater, agreeWithProposedOutput, logger)
+	return agent, claimLoader, responder
+}
+
+type stubClaimLoader struct {
+	callCount int
+	claims    []types.Claim
+}
+
+func (s *stubClaimLoader) FetchClaims(ctx context.Context) ([]types.Claim, error) {
+	s.callCount++
+	return s.claims, nil
+}
+
+type stubResponder struct {
+	callResolveCount  int
+	callResolveStatus types.GameStatus
+	callResolveErr    error
+
+	resolveCount int
+	resolveErr   error
+}
+
+func (s *stubResponder) CallResolve(ctx context.Context) (types.GameStatus, error) {
+	s.callResolveCount++
+	return s.callResolveStatus, s.callResolveErr
+}
+
+func (s *stubResponder) Resolve(ctx context.Context) error {
+	s.resolveCount++
+	return s.resolveErr
+}
+
+func (s *stubResponder) Respond(ctx context.Context, response types.Claim) error {
+	panic("Not implemented")
+}
+
+func (s *stubResponder) Step(ctx context.Context, stepData types.StepCallData) error {
+	panic("Not implemented")
+}
+
+type stubUpdater struct {
+}
+
+func (s *stubUpdater) UpdateOracle(ctx context.Context, data *types.PreimageOracleData) error {
+	panic("Not implemented")
+}