Merge pull request #6081 from ethereum-optimism/clabby/dispute/libpos-updates

chore(ctb): `LibPosition` documentation, tests, perf

packages/contracts-bedrock/.gas-snapshot

@@ -32,30 +32,30 @@ 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: 498304)
-FaultDisputeGame_ResolvesCorrectly_CorrectRoot4:test_resolvesCorrectly_succeeds() (gas: 500038)
-FaultDisputeGame_ResolvesCorrectly_CorrectRoot:test_resolvesCorrectly_succeeds() (gas: 490057)
-FaultDisputeGame_ResolvesCorrectly_IncorrectRoot2:test_resolvesCorrectly_succeeds() (gas: 495173)
-FaultDisputeGame_ResolvesCorrectly_IncorrectRoot3:test_resolvesCorrectly_succeeds() (gas: 496907)
-FaultDisputeGame_ResolvesCorrectly_IncorrectRoot:test_resolvesCorrectly_succeeds() (gas: 486926)
-FaultDisputeGame_Test:test_clockTimeExceeded_reverts() (gas: 26444)
-FaultDisputeGame_Test:test_defendRoot_reverts() (gas: 13281)
-FaultDisputeGame_Test:test_duplicateClaim_reverts() (gas: 103343)
-FaultDisputeGame_Test:test_extraData_succeeds() (gas: 17431)
+FaultDisputeGame_ResolvesCorrectly_CorrectRoot2:test_resolvesCorrectly_succeeds() (gas: 497198)
+FaultDisputeGame_ResolvesCorrectly_CorrectRoot4:test_resolvesCorrectly_succeeds() (gas: 499064)
+FaultDisputeGame_ResolvesCorrectly_CorrectRoot:test_resolvesCorrectly_succeeds() (gas: 489092)
+FaultDisputeGame_ResolvesCorrectly_IncorrectRoot2:test_resolvesCorrectly_succeeds() (gas: 494067)
+FaultDisputeGame_ResolvesCorrectly_IncorrectRoot3:test_resolvesCorrectly_succeeds() (gas: 495933)
+FaultDisputeGame_ResolvesCorrectly_IncorrectRoot:test_resolvesCorrectly_succeeds() (gas: 485961)
+FaultDisputeGame_Test:test_defendRoot_invalidMove_reverts() (gas: 13250)
+FaultDisputeGame_Test:test_extraData_succeeds() (gas: 17409)
 FaultDisputeGame_Test:test_gameData_succeeds() (gas: 17834)
-FaultDisputeGame_Test:test_gameDepthExceeded_reverts() (gas: 408128)
-FaultDisputeGame_Test:test_gameStart_succeeds() (gas: 10359)
-FaultDisputeGame_Test:test_gameType_succeeds() (gas: 8281)
-FaultDisputeGame_Test:test_initialRootClaimData_succeeds() (gas: 17669)
-FaultDisputeGame_Test:test_moveAgainstNonexistentParent_reverts() (gas: 24611)
-FaultDisputeGame_Test:test_move_gameNotInProgress_reverts() (gas: 10901)
-FaultDisputeGame_Test:test_resolve_challengeContested() (gas: 221222)
+FaultDisputeGame_Test:test_gameStart_succeeds() (gas: 10337)
+FaultDisputeGame_Test:test_gameType_succeeds() (gas: 8216)
+FaultDisputeGame_Test:test_initialRootClaimData_succeeds() (gas: 17691)
+FaultDisputeGame_Test:test_move_clockTimeExceeded_reverts() (gas: 26410)
+FaultDisputeGame_Test:test_move_duplicateClaim_reverts() (gas: 103231)
+FaultDisputeGame_Test:test_move_gameDepthExceeded_reverts() (gas: 407967)
+FaultDisputeGame_Test:test_move_gameNotInProgress_reverts() (gas: 10923)
+FaultDisputeGame_Test:test_move_nonExistentParent_reverts() (gas: 24632)
+FaultDisputeGame_Test:test_resolve_challengeContested() (gas: 221074)
 FaultDisputeGame_Test:test_resolve_notInProgress_reverts() (gas: 9657)
-FaultDisputeGame_Test:test_resolve_rootContested() (gas: 106144)
-FaultDisputeGame_Test:test_resolve_rootUncontested() (gas: 23697)
-FaultDisputeGame_Test:test_resolve_teamDeathmatch() (gas: 391960)
-FaultDisputeGame_Test:test_rootClaim_succeeds() (gas: 8225)
-FaultDisputeGame_Test:test_simpleAttack_succeeds() (gas: 107438)
+FaultDisputeGame_Test:test_resolve_rootContested() (gas: 106120)
+FaultDisputeGame_Test:test_resolve_rootUncontested() (gas: 23630)
+FaultDisputeGame_Test:test_resolve_teamDeathmatch() (gas: 391731)
+FaultDisputeGame_Test:test_rootClaim_succeeds() (gas: 8203)
+FaultDisputeGame_Test:test_simpleAttack_succeeds() (gas: 107322)
 FeeVault_Test:test_constructor_succeeds() (gas: 18185)
 GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_0() (gas: 352135)
 GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_1() (gas: 2950342)
@@ -230,6 +230,7 @@ LegacyERC20ETH_Test:test_mint_doesNotExist_reverts() (gas: 10627)
 LegacyERC20ETH_Test:test_transferFrom_doesNotExist_reverts() (gas: 12957)
 LegacyERC20ETH_Test:test_transfer_doesNotExist_reverts() (gas: 10755)
 LegacyMessagePasser_Test:test_passMessageToL1_succeeds() (gas: 34524)
+LibPosition_Test:test_pos_correctness_succeeds() (gas: 38711)
 MerkleTrie_get_Test:test_get_corruptedProof_reverts() (gas: 5736)
 MerkleTrie_get_Test:test_get_extraProofElements_reverts() (gas: 58975)
 MerkleTrie_get_Test:test_get_invalidDataRemainder_reverts() (gas: 35852)

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

@@ -123,7 +123,7 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone {
         // Pull the parent position out of storage.
         Position parentPos = parent.position;
         // Determine the position of the step.
-        Position stepPos = _isAttack ? parentPos.attack() : parentPos.defend();
+        Position stepPos = parentPos.move(_isAttack);
 
         // Ensure that the step position is 1 deeper than the maximum game depth.
         if (stepPos.depth() != MAX_GAME_DEPTH + 1) {
@@ -214,7 +214,7 @@ contract FaultDisputeGame is IFaultDisputeGame, Clone {
         // Compute the position that the claim commits to. Because the parent's position is already
         // known, we can compute the next position by moving left or right depending on whether
         // or not the move is an attack or defense.
-        Position nextPosition = _isAttack ? parent.position.attack() : parent.position.defend();
+        Position nextPosition = parent.position.move(_isAttack);
 
         // At the leaf nodes of the game, the only option is to run a step to prove or disprove
         // the above claim. At this depth, the parent claim commits to the state after a single

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

@@ -8,20 +8,27 @@ import "../../libraries/DisputeTypes.sol";
  * @notice This library contains helper functions for working with the `Position` type.
  */
 library LibPosition {
+    /**
+     * @notice Computes a generalized index (2^{depth} + indexAtDepth).
+     * @param _depth The depth of the position.
+     * @param _indexAtDepth The index at the depth of the position.
+     * @return position_ The computed generalized index.
+     */
     function wrap(uint64 _depth, uint64 _indexAtDepth) internal pure returns (Position position_) {
         assembly {
+            // gindex = 2^{_depth} + _indexAtDepth
             position_ := add(shl(_depth, 1), _indexAtDepth)
         }
     }
 
     /**
-     * @notice Pulls the `depth` out of a packed `Position` type.
-     * @param _position The position to get the `depth` of.
-     * @return depth_ The `depth` of the `position`.
+     * @notice Pulls the `depth` out of a `Position` type.
+     * @param _position The generalized index to get the `depth` of.
+     * @return depth_ The `depth` of the `position` gindex.
      * @custom:attribution Solady <https://github.com/Vectorized/Solady>
      */
     function depth(Position _position) internal pure returns (uint64 depth_) {
-        // Return the most significant bit position
+        // Return the most significant bit offset, which signifies the depth of the gindex.
         assembly {
             depth_ := or(depth_, shl(6, lt(0xffffffffffffffff, shr(depth_, _position))))
             depth_ := or(depth_, shl(5, lt(0xffffffff, shr(depth_, _position))))
@@ -45,12 +52,16 @@ library LibPosition {
     }
 
     /**
-     * @notice Pulls the `indexAtDepth` out of a packed `Position` type.
-     * @param _position The position to get the `indexAtDepth` of.
-     * @return indexAtDepth_ The `indexAtDepth` of the `position`.
+     * @notice Pulls the `indexAtDepth` out of a `Position` type.
+     *         The `indexAtDepth` is the left/right index of a position at a specific depth within
+     *         the binary tree, starting from index 0. For example, at gindex 2, the `depth` = 1
+     *         and the `indexAtDepth` = 0.
+     * @param _position The generalized index to get the `indexAtDepth` of.
+     * @return indexAtDepth_ The `indexAtDepth` of the `position` gindex.
      */
     function indexAtDepth(Position _position) internal pure returns (uint64 indexAtDepth_) {
-        // Return bits p_{msb-1}...p_{0}
+        // Return bits p_{msb-1}...p_{0}. This effectively pulls the 2^{depth} out of the gindex,
+        // leaving only the `indexAtDepth`.
         uint256 msb = depth(_position);
         assembly {
             indexAtDepth_ := sub(_position, shl(msb, 1))
@@ -58,7 +69,7 @@ library LibPosition {
     }
 
     /**
-     * @notice Get the position to the left of `position`.
+     * @notice Get the left child of `_position`.
      * @param _position The position to get the left position of.
      * @return left_ The position to the left of `position`.
      */
@@ -69,18 +80,18 @@ library LibPosition {
     }
 
     /**
-     * @notice Get the position to the right of `position`.
+     * @notice Get the right child of `_position`
      * @param _position The position to get the right position of.
      * @return right_ The position to the right of `position`.
      */
     function right(Position _position) internal pure returns (Position right_) {
         assembly {
-            right_ := add(1, shl(1, _position))
+            right_ := or(1, shl(1, _position))
         }
     }
 
     /**
-     * @notice Get the parent position of `position`.
+     * @notice Get the parent position of `_position`.
      * @param _position The position to get the parent position of.
      * @return parent_ The parent position of `position`.
      */
@@ -91,10 +102,11 @@ library LibPosition {
     }
 
     /**
-     * @notice Get the deepest, right most index relative to the `position`.
-     * @param _position The position to get the relative deepest, right most index of.
+     * @notice Get the deepest, right most gindex relative to the `position`. This is equivalent to
+     *         calling `right` on a position until the maximum depth is reached.
+     * @param _position The position to get the relative deepest, right most gindex of.
      * @param _maxDepth The maximum depth of the game.
-     * @return rightIndex_ The deepest, right most index relative to the `position`.
+     * @return rightIndex_ The deepest, right most gindex relative to the `position`.
      */
     function rightIndex(
         Position _position,
@@ -102,34 +114,22 @@ library LibPosition {
     ) internal pure returns (Position rightIndex_) {
         uint256 msb = depth(_position);
         assembly {
-            switch eq(msb, _maxDepth)
-            case true {
-                rightIndex_ := _position
-            }
-            default {
             let remaining := sub(_maxDepth, msb)
             rightIndex_ := or(shl(remaining, _position), sub(shl(remaining, 1), 1))
         }
     }
-    }
-
-    /**
-     * @notice Get the attack position relative to `position`.
-     * @param _position The position to get the relative attack position of.
-     * @return attack_ The attack position relative to `position`.
-     */
-    function attack(Position _position) internal pure returns (Position attack_) {
-        return left(_position);
-    }
 
     /**
-     * @notice Get the defense position relative to `position`.
-     * @param _position The position to get the relative defense position of.
-     * @return defense_ The defense position relative to `position`.
+     * @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.
+     * @param _position The position to get the relative attack/defense position of.
+     * @param _isAttack Whether or not the move is an attack move.
+     * @return move_ The move position relative to `position`.
      */
-    function defend(Position _position) internal pure returns (Position defense_) {
+    function move(Position _position, bool _isAttack) internal pure returns (Position move_) {
         assembly {
-            defense_ := shl(1, add(1, shl(1, shr(1, _position))))
+            move_ := shl(1, or(iszero(_isAttack), _position))
         }
     }
 }

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

@@ -157,7 +157,7 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
     /**
      * @dev Tests that an attempt to defend the root claim reverts with the `CannotDefendRootClaim` error.
      */
-    function test_defendRoot_reverts() public {
+    function test_defendRoot_invalidMove_reverts() public {
         vm.expectRevert(CannotDefendRootClaim.selector);
         gameProxy.defend(0, Claim.wrap(bytes32(uint256(5))));
     }
@@ -166,7 +166,7 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
      * @dev Tests that an attempt to move against a claim that does not exist reverts with the
      *      `ParentDoesNotExist` error.
      */
-    function test_moveAgainstNonexistentParent_reverts() public {
+    function test_move_nonExistentParent_reverts() public {
         Claim claim = Claim.wrap(bytes32(uint256(5)));
 
         // Expect an out of bounds revert for an attack
@@ -182,7 +182,7 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
      * @dev Tests that an attempt to move at the maximum game depth reverts with the
      *      `GameDepthExceeded` error.
      */
-    function test_gameDepthExceeded_reverts() public {
+    function test_move_gameDepthExceeded_reverts() public {
         Claim claim = Claim.wrap(bytes32(uint256(5)));
 
         uint256 maxDepth = gameProxy.MAX_GAME_DEPTH();
@@ -201,7 +201,7 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
      * @dev Tests that a move made after the clock time has exceeded reverts with the
      *      `ClockTimeExceeded` error.
      */
-    function test_clockTimeExceeded_reverts() public {
+    function test_move_clockTimeExceeded_reverts() public {
         // Warp ahead past the clock time for the first move (3 1/2 days)
         vm.warp(block.timestamp + 3 days + 12 hours + 1);
         vm.expectRevert(ClockTimeExceeded.selector);
@@ -212,7 +212,7 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
      * @dev Tests that an identical claim cannot be made twice. The duplicate claim attempt should
      *      revert with the `ClaimAlreadyExists` error.
      */
-    function test_duplicateClaim_reverts() public {
+    function test_move_duplicateClaim_reverts() public {
         Claim claim = Claim.wrap(bytes32(uint256(5)));
 
         // Make the first move. This should succeed.
@@ -250,7 +250,7 @@ contract FaultDisputeGame_Test is FaultDisputeGame_Init {
         assertEq(parentIndex, 0);
         assertEq(countered, false);
         assertEq(Claim.unwrap(claim), Claim.unwrap(counter));
-        assertEq(Position.unwrap(position), Position.unwrap(Position.wrap(1).attack()));
+        assertEq(Position.unwrap(position), Position.unwrap(Position.wrap(1).move(true)));
         assertEq(
             Clock.unwrap(clock),
             Clock.unwrap(LibClock.wrap(Duration.wrap(5), Timestamp.wrap(uint64(block.timestamp))))
@@ -384,7 +384,7 @@ contract GamePlayer {
 
         if (grandparentIndex == type(uint32).max) {
             // If the parent claim is the root claim, begin by attacking.
-            movePos = parentPos.attack();
+            movePos = parentPos.move(true);
             // Flag the move as an attack.
             isAttack = true;
         } else {
@@ -402,10 +402,10 @@ contract GamePlayer {
 
             if (Claim.unwrap(ourParentClaim) != Claim.unwrap(parentClaim)) {
                 // Attack parent.
-                movePos = parentPos.attack();
+                movePos = parentPos.move(true);
                 // If we also disagree with the grandparent, attack it as well.
                 if (Claim.unwrap(ourGrandparentClaim) != Claim.unwrap(grandparentClaim)) {
-                    movePos2 = grandparentPos.attack();
+                    movePos2 = grandparentPos.move(true);
                 }
 
                 // Flag the move as an attack.
@@ -414,7 +414,7 @@ contract GamePlayer {
                 Claim.unwrap(ourParentClaim) == Claim.unwrap(parentClaim) &&
                 Claim.unwrap(ourGrandparentClaim) == Claim.unwrap(grandparentClaim)
             ) {
-                movePos = parentPos.defend();
+                movePos = parentPos.move(false);
             }
         }
 
@@ -474,7 +474,7 @@ contract GamePlayer {
                 // If we have a second move position, attack the grandparent.
                 if (Position.unwrap(movePos2) != 0) {
                     (, , , Position grandparentPos, ) = gameProxy.claimData(grandparentIndex);
-                    Claim ourGrandparentClaim = claimAt(grandparentPos.attack());
+                    Claim ourGrandparentClaim = claimAt(grandparentPos.move(true));
 
                     gameProxy.attack(grandparentIndex, ourGrandparentClaim);
                     counterParty.play(claimDataLen() - 1);