Merge pull request #7542 from ethereum-optimism/refcell/todo-touchups

fix(op-challenger): Remove Temporary Implementations

op-challenger/game/fault/loader.go

@@ -92,7 +92,7 @@ func (l *loader) fetchClaim(ctx context.Context, arrIndex uint64) (types.Claim,
 	claim := types.Claim{
 		ClaimData: types.ClaimData{
 			Value:    fetchedClaim.Claim,
-			Position: types.NewPositionFromGIndex(fetchedClaim.Position.Uint64()),
+			Position: types.NewPositionFromGIndex(fetchedClaim.Position),
 		},
 		Countered:           fetchedClaim.Countered,
 		Clock:               fetchedClaim.Clock.Uint64(),
@@ -108,7 +108,7 @@ func (l *loader) fetchClaim(ctx context.Context, arrIndex uint64) (types.Claim,
 		}
 		claim.Parent = types.ClaimData{
 			Value:    parentClaim.Claim,
-			Position: types.NewPositionFromGIndex(parentClaim.Position.Uint64()),
+			Position: types.NewPositionFromGIndex(parentClaim.Position),
 		}
 	}
 

op-challenger/game/fault/loader_test.go

@@ -117,7 +117,7 @@ func TestLoader_FetchClaims(t *testing.T) {
 			{
 				ClaimData: types.ClaimData{
 					Value:    expectedClaims[0].Claim,
-					Position: types.NewPositionFromGIndex(expectedClaims[0].Position.Uint64()),
+					Position: types.NewPositionFromGIndex(expectedClaims[0].Position),
 				},
 				Countered:     false,
 				Clock:         uint64(0),
@@ -126,11 +126,11 @@ func TestLoader_FetchClaims(t *testing.T) {
 			{
 				ClaimData: types.ClaimData{
 					Value:    expectedClaims[1].Claim,
-					Position: types.NewPositionFromGIndex(expectedClaims[1].Position.Uint64()),
+					Position: types.NewPositionFromGIndex(expectedClaims[1].Position),
 				},
 				Parent: types.ClaimData{
 					Value:    expectedClaims[0].Claim,
-					Position: types.NewPositionFromGIndex(expectedClaims[0].Position.Uint64()),
+					Position: types.NewPositionFromGIndex(expectedClaims[0].Position),
 				},
 				Countered:           false,
 				Clock:               uint64(0),
@@ -140,11 +140,11 @@ func TestLoader_FetchClaims(t *testing.T) {
 			{
 				ClaimData: types.ClaimData{
 					Value:    expectedClaims[2].Claim,
-					Position: types.NewPositionFromGIndex(expectedClaims[2].Position.Uint64()),
+					Position: types.NewPositionFromGIndex(expectedClaims[2].Position),
 				},
 				Parent: types.ClaimData{
 					Value:    expectedClaims[1].Claim,
-					Position: types.NewPositionFromGIndex(expectedClaims[1].Position.Uint64()),
+					Position: types.NewPositionFromGIndex(expectedClaims[1].Position),
 				},
 				Countered:           false,
 				Clock:               uint64(0),

op-challenger/game/fault/test/claim_builder.go

@@ -74,7 +74,7 @@ func (c *ClaimBuilder) claim(pos types.Position, correct bool) common.Hash {
 }
 
 func (c *ClaimBuilder) CreateRootClaim(correct bool) types.Claim {
-	value := c.claim(types.NewPositionFromGIndex(1), correct)
+	value := c.claim(types.NewPositionFromGIndex(big.NewInt(1)), correct)
 	claim := types.Claim{
 		ClaimData: types.ClaimData{
 			Value:    value,

op-challenger/game/fault/trace/cannon/provider.go

@@ -88,7 +88,11 @@ func (p *CannonTraceProvider) SetMaxDepth(gameDepth uint64) {
 }
 
 func (p *CannonTraceProvider) Get(ctx context.Context, pos types.Position) (common.Hash, error) {
-	proof, err := p.loadProof(ctx, pos.UnsafeTraceIndex(int(p.gameDepth)))
+	traceIndex := pos.TraceIndex(int(p.gameDepth))
+	if !traceIndex.IsUint64() {
+		return common.Hash{}, errors.New("trace index out of bounds")
+	}
+	proof, err := p.loadProof(ctx, traceIndex.Uint64())
 	if err != nil {
 		return common.Hash{}, err
 	}
@@ -101,7 +105,11 @@ func (p *CannonTraceProvider) Get(ctx context.Context, pos types.Position) (comm
 }
 
 func (p *CannonTraceProvider) GetStepData(ctx context.Context, pos types.Position) ([]byte, []byte, *types.PreimageOracleData, error) {
-	proof, err := p.loadProof(ctx, pos.UnsafeTraceIndex(int(p.gameDepth)))
+	traceIndex := pos.TraceIndex(int(p.gameDepth))
+	if !traceIndex.IsUint64() {
+		return nil, nil, nil, errors.New("trace index out of bounds")
+	}
+	proof, err := p.loadProof(ctx, traceIndex.Uint64())
 	if err != nil {
 		return nil, nil, nil, err
 	}

op-challenger/game/fault/trace/cannon/provider_test.go

@@ -6,6 +6,7 @@ import (
 	_ "embed"
 	"encoding/json"
 	"fmt"
+	"math"
 	"math/big"
 	"os"
 	"path/filepath"
@@ -37,6 +38,14 @@ func TestGet(t *testing.T) {
 		require.Empty(t, generator.generated)
 	})
 
+	t.Run("ErrorsTraceIndexOutOfBounds", func(t *testing.T) {
+		provider, generator := setupWithTestData(t, dataDir, prestate)
+		largePosition := PositionFromTraceIndex(provider, new(big.Int).Mul(new(big.Int).SetUint64(math.MaxUint64), big.NewInt(2)))
+		_, err := provider.Get(context.Background(), largePosition)
+		require.ErrorContains(t, err, "trace index out of bounds")
+		require.Empty(t, generator.generated)
+	})
+
 	t.Run("ProofAfterEndOfTrace", func(t *testing.T) {
 		provider, generator := setupWithTestData(t, dataDir, prestate)
 		generator.finalState = &mipsevm.State{
@@ -84,6 +93,15 @@ func TestGetStepData(t *testing.T) {
 		require.Empty(t, generator.generated)
 	})
 
+	t.Run("ErrorsTraceIndexOutOfBounds", func(t *testing.T) {
+		dataDir, prestate := setupTestData(t)
+		provider, generator := setupWithTestData(t, dataDir, prestate)
+		largePosition := PositionFromTraceIndex(provider, new(big.Int).Mul(new(big.Int).SetUint64(math.MaxUint64), big.NewInt(2)))
+		_, _, _, err := provider.GetStepData(context.Background(), largePosition)
+		require.ErrorContains(t, err, "trace index out of bounds")
+		require.Empty(t, generator.generated)
+	})
+
 	t.Run("GenerateProof", func(t *testing.T) {
 		dataDir, prestate := setupTestData(t)
 		provider, generator := setupWithTestData(t, dataDir, prestate)

op-challenger/game/fault/trace/outputs/provider.go

@@ -3,7 +3,6 @@ package outputs
 import (
 	"context"
 	"fmt"
-	"math"
 
 	"github.com/ethereum-optimism/optimism/op-challenger/game/fault/types"
 	"github.com/ethereum-optimism/optimism/op-service/client"
@@ -54,7 +53,7 @@ func NewTraceProviderFromInputs(logger log.Logger, rollupClient OutputRollupClie
 
 func (o *OutputTraceProvider) Get(ctx context.Context, pos types.Position) (common.Hash, error) {
 	traceIndex := pos.TraceIndex(int(o.gameDepth))
-	if traceIndex.Cmp(common.Big0.SetUint64(math.MaxUint64)) > 0 {
+	if !traceIndex.IsUint64() {
 		return common.Hash{}, fmt.Errorf("trace index %v is greater than max uint64", traceIndex)
 	}
 	outputBlock := traceIndex.Uint64() + o.prestateBlock + 1

op-challenger/game/fault/trace/outputs/provider_test.go

@@ -47,27 +47,27 @@ func TestGet(t *testing.T) {
 
 	t.Run("FirstBlockAfterPrestate", func(t *testing.T) {
 		provider, _ := setupWithTestData(t, prestateBlock, poststateBlock)
-		value, err := provider.Get(context.Background(), types.NewPositionFromGIndex(128))
+		value, err := provider.Get(context.Background(), types.NewPositionFromGIndex(big.NewInt(128)))
 		require.NoError(t, err)
 		require.Equal(t, firstOutputRoot, value)
 	})
 
 	t.Run("MissingOutputAtBlock", func(t *testing.T) {
 		provider, _ := setupWithTestData(t, prestateBlock, poststateBlock)
-		_, err := provider.Get(context.Background(), types.NewPositionFromGIndex(129))
+		_, err := provider.Get(context.Background(), types.NewPositionFromGIndex(big.NewInt(129)))
 		require.ErrorAs(t, fmt.Errorf("no output at block %d", prestateBlock+2), &err)
 	})
 
 	t.Run("PostStateBlock", func(t *testing.T) {
 		provider, _ := setupWithTestData(t, prestateBlock, poststateBlock)
-		value, err := provider.Get(context.Background(), types.NewPositionFromGIndex(228))
+		value, err := provider.Get(context.Background(), types.NewPositionFromGIndex(big.NewInt(228)))
 		require.NoError(t, err)
 		require.Equal(t, value, poststateOutputRoot)
 	})
 
 	t.Run("AfterPostStateBlock", func(t *testing.T) {
 		provider, _ := setupWithTestData(t, prestateBlock, poststateBlock)
-		value, err := provider.Get(context.Background(), types.NewPositionFromGIndex(229))
+		value, err := provider.Get(context.Background(), types.NewPositionFromGIndex(big.NewInt(229)))
 		require.NoError(t, err)
 		require.Equal(t, value, poststateOutputRoot)
 	})

op-challenger/game/fault/types/position.go

@@ -25,19 +25,13 @@ func NewPosition(depth int, indexAtDepth *big.Int) Position {
 	}
 }
 
-func NewLargePositionFromGIndex(x *big.Int) Position {
+func NewPositionFromGIndex(x *big.Int) Position {
 	depth := bigMSB(x)
-	indexAtDepth := new(big.Int).Sub(x, new(big.Int).Lsh(big.NewInt(1), uint(depth)))
+	withoutMSB := new(big.Int).Not(new(big.Int).Lsh(big.NewInt(1), uint(depth)))
+	indexAtDepth := new(big.Int).And(x, withoutMSB)
 	return NewPosition(depth, indexAtDepth)
 }
 
-// todo(client-pod#80): remove this to use the NewLargePositionFromGIndex.
-func NewPositionFromGIndex(x uint64) Position {
-	depth := MSBIndex(x)
-	indexAtDepth := ^(1 << depth) & x
-	return NewPosition(depth, big.NewInt(int64(indexAtDepth)))
-}
-
 func (p Position) MoveRight() Position {
 	return Position{
 		depth:        p.depth,
@@ -87,13 +81,6 @@ func (p Position) TraceIndex(maxDepth int) *big.Int {
 	return ti
 }
 
-// UnsafeTraceIndex returns a uint64 representation of the trace index.
-// todo(refcell): This should be removed in a follow-on pr and any invocations
-// should be updated to use TraceIndex.
-func (p Position) UnsafeTraceIndex(maxDepth int) uint64 {
-	return p.TraceIndex(maxDepth).Uint64()
-}
-
 // move returns a new position at the left or right child.
 func (p Position) move(right bool) Position {
 	return Position{
@@ -146,24 +133,12 @@ func (p Position) ToGIndex() *big.Int {
 
 // bigMSB returns the index of the most significant bit
 func bigMSB(x *big.Int) int {
-	if x.Cmp(common.Big0) == 0 {
+	if x.Cmp(big.NewInt(0)) == 0 {
 		return 0
 	}
 	out := 0
-	for ; x.Cmp(common.Big0) != 0; out++ {
+	for ; x.Cmp(big.NewInt(0)) != 0; out++ {
 		x = new(big.Int).Rsh(x, 1)
 	}
 	return out - 1
 }
-
-// MSBIndex returns the index of the most significant bit
-func MSBIndex(x uint64) int {
-	if x == 0 {
-		return 0
-	}
-	out := 0
-	for ; x != 0; out++ {
-		x = x >> 1
-	}
-	return out - 1
-}

op-challenger/game/fault/types/position_test.go

@@ -8,34 +8,36 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestMSBIndex(t *testing.T) {
+func bi(i int) *big.Int {
+	return big.NewInt(int64(i))
+}
+
+func TestBigMSB(t *testing.T) {
+	large, ok := new(big.Int).SetString("18446744073709551615", 10)
+	require.True(t, ok)
 	tests := []struct {
-		input    uint64
+		input    *big.Int
 		expected int
 	}{
-		{0, 0},
-		{1, 0},
-		{2, 1},
-		{4, 2},
-		{8, 3},
-		{16, 4},
-		{255, 7},
-		{1024, 10},
-		{18446744073709551615, 63},
+		{bi(0), 0},
+		{bi(1), 0},
+		{bi(2), 1},
+		{bi(4), 2},
+		{bi(8), 3},
+		{bi(16), 4},
+		{bi(255), 7},
+		{bi(1024), 10},
+		{large, 63},
 	}
 
 	for _, test := range tests {
-		result := MSBIndex(test.input)
+		result := bigMSB(test.input)
 		if result != test.expected {
 			t.Errorf("MSBIndex(%d) expected %d, but got %d", test.input, test.expected, result)
 		}
 	}
 }
 
-func bi(i int) *big.Int {
-	return big.NewInt(int64(i))
-}
-
 type testNodeInfo struct {
 	GIndex       *big.Int
 	Depth        int
@@ -89,7 +91,7 @@ var treeNodes = []testNodeInfo{
 // TestGINConversions does To & From the generalized index on the treeNodesMaxDepth4 data
 func TestGINConversions(t *testing.T) {
 	for _, test := range treeNodes {
-		from := NewLargePositionFromGIndex(test.GIndex)
+		from := NewPositionFromGIndex(test.GIndex)
 		pos := NewPosition(test.Depth, test.IndexAtDepth)
 		require.EqualValuesf(t, pos.Depth(), from.Depth(), "From GIndex %v vs pos %v", from.Depth(), pos.Depth())
 		require.Zerof(t, pos.IndexAtDepth().Cmp(from.IndexAtDepth()), "From GIndex %v vs pos %v", from.IndexAtDepth(), pos.IndexAtDepth())
@@ -100,7 +102,7 @@ func TestGINConversions(t *testing.T) {
 
 func TestTraceIndexOfRootWithLargeDepth(t *testing.T) {
 	traceIdx := new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 100), big.NewInt(1))
-	pos := NewLargePositionFromGIndex(big.NewInt(1))
+	pos := NewPositionFromGIndex(big.NewInt(1))
 	actual := pos.TraceIndex(100)
 	require.Equal(t, traceIdx, actual)
 }

op-challenger/game/fault/types/types_test.go

 package types
 
 import (
+	"math/big"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -32,17 +33,17 @@ func TestIsRootPosition(t *testing.T) {
 	}{
 		{
 			name:     "ZeroRoot",
-			position: NewPositionFromGIndex(0),
+			position: NewPositionFromGIndex(big.NewInt(0)),
 			expected: true,
 		},
 		{
 			name:     "ValidRoot",
-			position: NewPositionFromGIndex(1),
+			position: NewPositionFromGIndex(big.NewInt(1)),
 			expected: true,
 		},
 		{
 			name:     "NotRoot",
-			position: NewPositionFromGIndex(2),
+			position: NewPositionFromGIndex(big.NewInt(2)),
 			expected: false,
 		},
 	}
@@ -54,7 +55,7 @@ func TestIsRootPosition(t *testing.T) {
 	}
 }
 
-func buildClaim(gindex uint64, parentGIndex uint64) Claim {
+func buildClaim(gindex *big.Int, parentGIndex *big.Int) Claim {
 	return Claim{
 		ClaimData: ClaimData{
 			Position: NewPositionFromGIndex(gindex),
@@ -73,32 +74,32 @@ func TestDefendsParent(t *testing.T) {
 	}{
 		{
 			name:     "LeftChildAttacks",
-			claim:    buildClaim(2, 1),
+			claim:    buildClaim(big.NewInt(2), big.NewInt(1)),
 			expected: false,
 		},
 		{
 			name:     "RightChildDoesntDefend",
-			claim:    buildClaim(3, 1),
+			claim:    buildClaim(big.NewInt(3), big.NewInt(1)),
 			expected: false,
 		},
 		{
 			name:     "SubChildDoesntDefend",
-			claim:    buildClaim(4, 1),
+			claim:    buildClaim(big.NewInt(4), big.NewInt(1)),
 			expected: false,
 		},
 		{
 			name:     "SubSecondChildDoesntDefend",
-			claim:    buildClaim(5, 1),
+			claim:    buildClaim(big.NewInt(5), big.NewInt(1)),
 			expected: false,
 		},
 		{
 			name:     "RightLeftChildDefendsParent",
-			claim:    buildClaim(6, 1),
+			claim:    buildClaim(big.NewInt(6), big.NewInt(1)),
 			expected: true,
 		},
 		{
 			name:     "SubThirdChildDefends",
-			claim:    buildClaim(7, 1),
+			claim:    buildClaim(big.NewInt(7), big.NewInt(1)),
 			expected: true,
 		},
 	}

op-e2e/e2eutils/disputegame/dishonest_helper.go

@@ -74,7 +74,7 @@ func (d *DishonestHelper) ExhaustDishonestClaims(ctx context.Context) {
 		// honest level, invalid attack
 		// honest level, invalid defense
 
-		pos := types.NewPositionFromGIndex(claimData.Position.Uint64())
+		pos := types.NewPositionFromGIndex(claimData.Position)
 		if int64(pos.Depth()) == depth {
 			return
 		}

op-e2e/e2eutils/disputegame/game_helper.go

@@ -128,7 +128,7 @@ func (g *FaultGameHelper) GetClaimValue(ctx context.Context, claimIdx int64) com
 func (g *FaultGameHelper) GetClaimPosition(ctx context.Context, claimIdx int64) types.Position {
 	g.WaitForClaimCount(ctx, claimIdx+1)
 	claim := g.getClaim(ctx, claimIdx)
-	return types.NewPositionFromGIndex(claim.Position.Uint64())
+	return types.NewPositionFromGIndex(claim.Position)
 }
 
 // getClaim retrieves the claim data for a specific index.
@@ -143,7 +143,7 @@ func (g *FaultGameHelper) getClaim(ctx context.Context, claimIdx int64) Contract
 
 // getClaimPosition retrieves the [types.Position] of a claim at a specific index.
 func (g *FaultGameHelper) getClaimPosition(ctx context.Context, claimIdx int64) types.Position {
-	return types.NewPositionFromGIndex(g.getClaim(ctx, claimIdx).Position.Uint64())
+	return types.NewPositionFromGIndex(g.getClaim(ctx, claimIdx).Position)
 }
 
 func (g *FaultGameHelper) WaitForClaimAtDepth(ctx context.Context, depth int) {
@@ -151,7 +151,7 @@ func (g *FaultGameHelper) WaitForClaimAtDepth(ctx context.Context, depth int) {
 		ctx,
 		fmt.Sprintf("Could not find claim depth %v", depth),
 		func(claim ContractClaim) bool {
-			pos := types.NewPositionFromGIndex(claim.Position.Uint64())
+			pos := types.NewPositionFromGIndex(claim.Position)
 			return pos.Depth() == depth
 		})
 }
@@ -162,7 +162,7 @@ func (g *FaultGameHelper) WaitForClaimAtMaxDepth(ctx context.Context, countered
 		ctx,
 		fmt.Sprintf("Could not find claim depth %v with countered=%v", maxDepth, countered),
 		func(claim ContractClaim) bool {
-			pos := types.NewPositionFromGIndex(claim.Position.Uint64())
+			pos := types.NewPositionFromGIndex(claim.Position)
 			return int64(pos.Depth()) == maxDepth && claim.Countered == countered
 		})
 }
@@ -380,7 +380,7 @@ func (g *FaultGameHelper) gameData(ctx context.Context) string {
 		claim, err := g.game.ClaimData(opts, big.NewInt(i))
 		g.require.NoErrorf(err, "Fetch claim %v", i)
 
-		pos := types.NewPositionFromGIndex(claim.Position.Uint64())
+		pos := types.NewPositionFromGIndex(claim.Position)
 		info = info + fmt.Sprintf("%v - Position: %v, Depth: %v, IndexAtDepth: %v Trace Index: %v, Value: %v, Countered: %v, ParentIndex: %v\n",
 			i, claim.Position.Int64(), pos.Depth(), pos.IndexAtDepth(), pos.TraceIndex(maxDepth), common.Hash(claim.Claim).Hex(), claim.Countered, claim.ParentIndex)
 	}

op-e2e/e2eutils/disputegame/helper.go

@@ -38,7 +38,7 @@ const alphabetGameDepth = 4
 var lastAlphabetTraceIndex = big.NewInt(1<<alphabetGameDepth - 1)
 
 // rootPosition is the position of the root claim.
-var rootPosition = faultTypes.NewPositionFromGIndex(1)
+var rootPosition = faultTypes.NewPositionFromGIndex(big.NewInt(1))
 
 type Status uint8
 

op-e2e/e2eutils/disputegame/honest_helper.go

@@ -20,7 +20,7 @@ func (h *HonestHelper) Attack(ctx context.Context, claimIdx int64) {
 	ctx, cancel := context.WithTimeout(ctx, 2*time.Minute)
 	defer cancel()
 	claim := h.game.getClaim(ctx, claimIdx)
-	pos := types.NewPositionFromGIndex(claim.Position.Uint64())
+	pos := types.NewPositionFromGIndex(claim.Position)
 	attackPos := pos.Attack()
 	h.t.Logf("Attacking at position %v with g index %v", attackPos, attackPos.ToGIndex())
 	value, err := h.correctTrace.Get(ctx, attackPos)
@@ -34,7 +34,7 @@ func (h *HonestHelper) Defend(ctx context.Context, claimIdx int64) {
 	ctx, cancel := context.WithTimeout(ctx, 2*time.Minute)
 	defer cancel()
 	claim := h.game.getClaim(ctx, claimIdx)
-	pos := types.NewPositionFromGIndex(claim.Position.Uint64())
+	pos := types.NewPositionFromGIndex(claim.Position)
 	defendPos := pos.Defend()
 	value, err := h.correctTrace.Get(ctx, defendPos)
 	h.game.require.NoErrorf(err, "Get correct claim at position %v with g index %v", defendPos, defendPos.ToGIndex())