Merge pull request #6227 from ethereum-optimism/clabby/ctb/fix-step-bug

fixup(ctb): `step` bug

packages/contracts-bedrock/.gas-snapshot

@@ -32,12 +32,12 @@ DisputeGameFactory_SetImplementation_Test:test_setImplementation_notOwner_revert
 DisputeGameFactory_SetImplementation_Test:test_setImplementation_succeeds() (gas: 44243)
 DisputeGameFactory_TransferOwnership_Test:test_transferOwnership_notOwner_reverts() (gas: 15950)
 DisputeGameFactory_TransferOwnership_Test:test_transferOwnership_succeeds() (gas: 18642)
-FaultDisputeGame_ResolvesCorrectly_CorrectRoot2:test_resolvesCorrectly_succeeds() (gas: 502169)
-FaultDisputeGame_ResolvesCorrectly_CorrectRoot3:test_resolvesCorrectly_succeeds() (gas: 504048)
-FaultDisputeGame_ResolvesCorrectly_CorrectRoot:test_resolvesCorrectly_succeeds() (gas: 491512)
-FaultDisputeGame_ResolvesCorrectly_IncorrectRoot2:test_resolvesCorrectly_succeeds() (gas: 500932)
-FaultDisputeGame_ResolvesCorrectly_IncorrectRoot3:test_resolvesCorrectly_succeeds() (gas: 502811)
-FaultDisputeGame_ResolvesCorrectly_IncorrectRoot:test_resolvesCorrectly_succeeds() (gas: 490275)
+FaultDisputeGame_ResolvesCorrectly_CorrectRoot1:test_resolvesCorrectly_succeeds() (gas: 491839)
+FaultDisputeGame_ResolvesCorrectly_CorrectRoot2:test_resolvesCorrectly_succeeds() (gas: 495751)
+FaultDisputeGame_ResolvesCorrectly_CorrectRoot3:test_resolvesCorrectly_succeeds() (gas: 495049)
+FaultDisputeGame_ResolvesCorrectly_IncorrectRoot1:test_resolvesCorrectly_succeeds() (gas: 490600)
+FaultDisputeGame_ResolvesCorrectly_IncorrectRoot2:test_resolvesCorrectly_succeeds() (gas: 494512)
+FaultDisputeGame_ResolvesCorrectly_IncorrectRoot3:test_resolvesCorrectly_succeeds() (gas: 493810)
 FaultDisputeGame_Test:test_extraData_succeeds() (gas: 17426)
 FaultDisputeGame_Test:test_gameData_succeeds() (gas: 17917)
 FaultDisputeGame_Test:test_gameStart_succeeds() (gas: 10315)

packages/contracts-bedrock/contracts/dispute/FaultDisputeGame.sol

@@ -71,7 +71,6 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, Semver {
 
     /// @inheritdoc IFaultDisputeGame
     function step(
-        uint256 _stateIndex,
         uint256 _claimIndex,
         bool _isAttack,
         bytes calldata _stateData,
@@ -94,43 +93,36 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, Semver {
         // Determine the expected pre & post states of the step.
         Claim preStateClaim;
         Claim postStateClaim;
-        if (stepPos.indexAtDepth() == 0) {
-            // If the step position's index at depth is 0, the prestate is the absolute prestate
-            // and the post state is the parent claim.
-            preStateClaim = ABSOLUTE_PRESTATE;
-            postStateClaim = claimData[_claimIndex].claim;
-        } else {
-            Position preStatePos;
-            Position postStatePos;
-            if (_isAttack) {
-                // If the step is an attack, the prestate exists elsewhere in the game state,
-                // and the parent claim is the expected post-state.
-                preStatePos = claimData[_stateIndex].position;
-                preStateClaim = claimData[_stateIndex].claim;
-                postStatePos = parentPos;
-                postStateClaim = parent.claim;
+        if (_isAttack) {
+            if (stepPos.indexAtDepth() == 0) {
+                // If the step position's index at depth is 0, the prestate is the absolute
+                // prestate.
+                preStateClaim = ABSOLUTE_PRESTATE;
             } else {
-                // If the step is a defense, the poststate exists elsewhere in the game state,
-                // and the parent claim is the expected pre-state.
-                preStatePos = parent.position;
-                preStateClaim = parent.claim;
-                postStatePos = claimData[_stateIndex].position;
-                postStateClaim = claimData[_stateIndex].claim;
+                // If the step is an attack at a trace index > 0, the prestate exists elsewhere in
+                // the game state.
+                preStateClaim = findTraceAncestor(
+                    Position.wrap(Position.unwrap(parentPos) - 1),
+                    parent.parentIndex
+                );
             }
 
-            // INVARIANT: The prestate is always invalid if its gindex is not one less than that
-            //            of the post state.
-            // INVARIANT: The prestate is always invalid if the passed `_stateData` is not the
-            //            preimage of the prestate claim hash.
-            if (
-                Position.unwrap(preStatePos.rightIndex(MAX_GAME_DEPTH)) !=
-                Position.unwrap(postStatePos.rightIndex(MAX_GAME_DEPTH)) - 1 ||
-                keccak256(_stateData) != Claim.unwrap(preStateClaim)
-            ) {
-                revert InvalidPrestate();
-            }
+            // For all attacks, the poststate is the parent claim.
+            postStateClaim = parent.claim;
+        } else {
+            // If the step is a defense, the poststate exists elsewhere in the game state,
+            // and the parent claim is the expected pre-state.
+            preStateClaim = parent.claim;
+            postStateClaim = findTraceAncestor(
+                Position.wrap(Position.unwrap(parentPos) + 1),
+                parent.parentIndex
+            );
         }
 
+        // INVARIANT: The prestate is always invalid if the passed `_stateData` is not the
+        //            preimage of the prestate claim hash.
+        if (keccak256(_stateData) != Claim.unwrap(preStateClaim)) revert InvalidPrestate();
+
         // INVARIANT: A VM step can never counter a parent claim unless it produces a poststate
         //            that is not equal to the claim at `_parentIndex` if the step is an attack,
         //            or the claim at `_stateIndex` if the step is a defense.
@@ -379,4 +371,30 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone, Semver {
     function claimDataLen() external view returns (uint256 len_) {
         len_ = claimData.length;
     }
+
+    ////////////////////////////////////////////////////////////////
+    //                          HELPERS                           //
+    ////////////////////////////////////////////////////////////////
+
+    /// @notice Finds the trace ancestor of a given position within the DAG.
+    /// @param _pos The position to find the trace ancestor claim of.
+    /// @param _start The index to start searching from.
+    /// @return ancestor_ The ancestor claim that commits to the same trace index as `_pos`.
+    // TODO: Can we form a relationship between the trace path and the position to avoid looping?
+    function findTraceAncestor(Position _pos, uint256 _start)
+        internal
+        view
+        returns (Claim ancestor_)
+    {
+        // Grab the trace ancestor's expected position.
+        Position preStateTraceAncestor = _pos.traceAncestor();
+
+        // Walk up the DAG to find a claim that commits to the same trace index as `_pos`. It is
+        // guaranteed that such a claim exists.
+        ClaimData storage ancestor = claimData[_start];
+        while (Position.unwrap(ancestor.position) != Position.unwrap(preStateTraceAncestor)) {
+            ancestor = claimData[ancestor.parentIndex];
+        }
+        ancestor_ = ancestor.claim;
+    }
 }

packages/contracts-bedrock/contracts/dispute/interfaces/IFaultDisputeGame.sol

@@ -39,7 +39,6 @@ interface IFaultDisputeGame is IDisputeGame {
     ///      a step in the fault proof program on-chain. The interface of the fault proof
     ///      processor contract should be generic enough such that we can use different
     ///      fault proof VMs (MIPS, RiscV5, etc.)
-    /// @param _stateIndex The index of the pre/post state of the step within `claimData`.
     /// @param _claimIndex The index of the challenged claim within `claimData`.
     /// @param _isAttack Whether or not the step is an attack or a defense.
     /// @param _stateData The stateData of the step is the preimage of the claim at the given
@@ -48,7 +47,6 @@ interface IFaultDisputeGame is IDisputeGame {
     ///        the absolute prestate of the fault proof VM.
     /// @param _proof Proof to access memory leaf nodes in the VM.
     function step(
-        uint256 _stateIndex,
         uint256 _claimIndex,
         bool _isAttack,
         bytes calldata _stateData,

packages/contracts-bedrock/contracts/dispute/lib/LibPosition.sol

@@ -124,6 +124,27 @@ library LibPosition {
         }
     }
 
+    /// @notice Gets the position of the highest ancestor of `_position` that commits to the same
+    ///         trace index.
+    /// @param _position The position to get the highest ancestor of.
+    /// @return ancestor_ The highest ancestor of `position` that commits to the same trace index.
+    function traceAncestor(Position _position) internal pure returns (Position ancestor_) {
+        // Create a field with only the lowest unset bit of `_position` set.
+        Position lsb;
+        assembly {
+            lsb := and(not(_position), add(_position, 1))
+        }
+        // Find the index of the lowest unset bit within the field.
+        uint256 msb = depth(lsb);
+        // The highest ancestor that commits to the same trace index is the original position
+        // shifted right by the index of the lowest unset bit.
+        assembly {
+            let a := shr(msb, _position)
+            // Bound the ancestor to the minimum gindex, 1.
+            ancestor_ := or(a, iszero(a))
+        }
+    }
+
     /// @notice Get the move position of `_position`, which is the left child of:
     ///         1. `_position + 1` if `_isAttack` is true.
     ///         1. `_position` if `_isAttack` is false.

packages/contracts-bedrock/contracts/test/FaultDisputeGame.t.sol

@@ -424,7 +424,6 @@ contract GamePlayer {
 
         // If we are past the maximum depth, break the recursion and step.
         if (movePos.depth() > maxDepth) {
-            uint256 stateIndex;
             bytes memory preStateTrace;
 
             // First, we need to find the pre/post state index depending on whether we
@@ -435,27 +434,7 @@ contract GamePlayer {
                 Position leafPos = isAttack
                     ? Position.wrap(Position.unwrap(parentPos) - 1)
                     : Position.wrap(Position.unwrap(parentPos) + 1);
-                Position statePos = leafPos;
-
-                // Walk up until the valid position that commits to the prestate's
-                // trace index is found.
-                while (
-                    Position.unwrap(statePos.parent().rightIndex(maxDepth)) ==
-                    Position.unwrap(leafPos)
-                ) {
-                    statePos = statePos.parent();
-                }
-
-                // Now, search for the index of the claim that commits to the prestate's trace
-                // index.
-                uint256 len = gameProxy.claimDataLen();
-                for (uint256 i = 0; i < len; i++) {
-                    (, , , Position pos, ) = gameProxy.claimData(i);
-                    if (Position.unwrap(pos) == Position.unwrap(statePos)) {
-                        stateIndex = i;
-                        break;
-                    }
-                }
+                Position statePos = leafPos.traceAncestor();
 
                 // Grab the trace up to the prestate's trace index.
                 if (isAttack) {
@@ -463,10 +442,12 @@ contract GamePlayer {
                 } else {
                     preStateTrace = abi.encode(parentPos.traceIndex(maxDepth), traceAt(parentPos));
                 }
+            } else {
+                preStateTrace = abi.encode(15);
             }
 
             // Perform the step and halt recursion.
-            try gameProxy.step(stateIndex, _parentIndex, isAttack, preStateTrace, hex"") {
+            try gameProxy.step(_parentIndex, isAttack, preStateTrace, hex"") {
                 // Do nothing, step succeeded.
             } catch {
                 failedToStep = true;
@@ -523,7 +504,9 @@ contract GamePlayer {
 
 contract OneVsOne_Arena is FaultDisputeGame_Init {
     /// @dev The absolute prestate of the trace.
-    Claim internal constant ABSOLUTE_PRESTATE = Claim.wrap(bytes32(uint256(15)));
+    bytes ABSOLUTE_PRESTATE = abi.encode(15);
+    /// @dev The absolute prestate claim.
+    Claim internal constant ABSOLUTE_PRESTATE_CLAIM = Claim.wrap(keccak256(abi.encode(15)));
     /// @dev The defender.
     GamePlayer internal defender;
     /// @dev The challenger.
@@ -531,7 +514,7 @@ contract OneVsOne_Arena is FaultDisputeGame_Init {
 
     function init(GamePlayer _defender, GamePlayer _challenger) public {
         Claim rootClaim = Claim.wrap(keccak256(abi.encode(15, _defender.traceAt(15))));
-        super.init(rootClaim, ABSOLUTE_PRESTATE);
+        super.init(rootClaim, ABSOLUTE_PRESTATE_CLAIM);
         defender = _defender;
         challenger = _challenger;
 
@@ -545,7 +528,7 @@ contract OneVsOne_Arena is FaultDisputeGame_Init {
     }
 }
 
-contract FaultDisputeGame_ResolvesCorrectly_IncorrectRoot is OneVsOne_Arena {
+contract FaultDisputeGame_ResolvesCorrectly_IncorrectRoot1 is OneVsOne_Arena {
     function setUp() public override {
         GamePlayer honest = new HonestPlayer(ABSOLUTE_PRESTATE);
         GamePlayer dishonest = new FullyDivergentPlayer(ABSOLUTE_PRESTATE);
@@ -566,7 +549,7 @@ contract FaultDisputeGame_ResolvesCorrectly_IncorrectRoot is OneVsOne_Arena {
     }
 }
 
-contract FaultDisputeGame_ResolvesCorrectly_CorrectRoot is OneVsOne_Arena {
+contract FaultDisputeGame_ResolvesCorrectly_CorrectRoot1 is OneVsOne_Arena {
     function setUp() public override {
         GamePlayer honest = new HonestPlayer(ABSOLUTE_PRESTATE);
         GamePlayer dishonest = new FullyDivergentPlayer(ABSOLUTE_PRESTATE);
@@ -676,8 +659,8 @@ contract FaultDisputeGame_ResolvesCorrectly_CorrectRoot3 is OneVsOne_Arena {
 ////////////////////////////////////////////////////////////////
 
 contract HonestPlayer is GamePlayer {
-    constructor(Claim _absolutePrestate) {
-        uint8 absolutePrestate = uint8(uint256(Claim.unwrap(_absolutePrestate)));
+    constructor(bytes memory _absolutePrestate) {
+        uint8 absolutePrestate = uint8(_absolutePrestate[31]);
         bytes memory honestTrace = new bytes(16);
         for (uint8 i = 0; i < honestTrace.length; i++) {
             honestTrace[i] = bytes1(absolutePrestate + i + 1);
@@ -687,8 +670,8 @@ contract HonestPlayer is GamePlayer {
 }
 
 contract FullyDivergentPlayer is GamePlayer {
-    constructor(Claim _absolutePrestate) {
-        uint8 absolutePrestate = uint8(uint256(Claim.unwrap(_absolutePrestate)));
+    constructor(bytes memory _absolutePrestate) {
+        uint8 absolutePrestate = uint8(_absolutePrestate[31]);
         bytes memory dishonestTrace = new bytes(16);
         for (uint8 i = 0; i < dishonestTrace.length; i++) {
             // Offset the honest trace by 1.
@@ -699,8 +682,8 @@ contract FullyDivergentPlayer is GamePlayer {
 }
 
 contract HalfDivergentPlayer is GamePlayer {
-    constructor(Claim _absolutePrestate) {
-        uint8 absolutePrestate = uint8(uint256(Claim.unwrap(_absolutePrestate)));
+    constructor(bytes memory _absolutePrestate) {
+        uint8 absolutePrestate = uint8(_absolutePrestate[31]);
         bytes memory dishonestTrace = new bytes(16);
         for (uint8 i = 0; i < dishonestTrace.length; i++) {
             // Offset the trace after the first half.
@@ -711,11 +694,11 @@ contract HalfDivergentPlayer is GamePlayer {
 }
 
 contract EarlyDivergentPlayer is GamePlayer {
-    constructor(Claim _absolutePrestate) {
-        uint8 absolutePrestate = uint8(uint256(Claim.unwrap(_absolutePrestate)));
+    constructor(bytes memory _absolutePrestate) {
+        uint8 absolutePrestate = uint8(_absolutePrestate[31]);
         bytes memory dishonestTrace = new bytes(16);
         for (uint8 i = 0; i < dishonestTrace.length; i++) {
-            // Offset the trace after the first half.
+            // Offset the trace after the first 3 instructions.
             dishonestTrace[i] = i > 2 ? bytes1(i) : bytes1(absolutePrestate + i + 1);
         }
         trace = dishonestTrace;
@@ -741,10 +724,10 @@ contract AlphabetVM is IBigStepper {
     {
         uint256 traceIndex;
         uint256 claim;
-        if (_stateData.length == 0) {
+        if (keccak256(_stateData) == Claim.unwrap(ABSOLUTE_PRESTATE)) {
             // If the state data is empty, then the absolute prestate is the claim.
             traceIndex = 0;
-            claim = uint256(Claim.unwrap(ABSOLUTE_PRESTATE));
+            (claim) = abi.decode(_stateData, (uint256));
         } else {
             // Otherwise, decode the state data.
             (traceIndex, claim) = abi.decode(_stateData, (uint256, uint256));

packages/contracts-bedrock/contracts/test/LibPosition.t.sol

@@ -74,6 +74,24 @@ contract LibPosition_Test is Test {
         assertEq(parent.indexAtDepth(), _indexAtDepth / 2);
     }
 
+    /// @notice Tests that the `traceAncestor` function correctly computes the position of the
+    ///         highest ancestor that commits to the same trace index.
+    function testFuzz_traceAncestor_correctness_succeeds(uint8 _depth, uint64 _indexAtDepth)
+        public
+    {
+        _depth = uint8(bound(_depth, 1, MAX_DEPTH));
+        _indexAtDepth = boundIndexAtDepth(_depth, _indexAtDepth);
+
+        Position position = LibPosition.wrap(_depth, _indexAtDepth);
+        Position ancestor = position.traceAncestor();
+        Position loopAncestor = position;
+        while (loopAncestor.parent().traceIndex(MAX_DEPTH) == position.traceIndex(MAX_DEPTH)) {
+            loopAncestor = loopAncestor.parent();
+        }
+
+        assertEq(Position.unwrap(ancestor), Position.unwrap(loopAncestor));
+    }
+
     /// @notice Tests that the `rightIndex` function correctly computes the deepest, right most index relative
     ///         to a given position.
     function testFuzz_rightIndex_correctness_succeeds(