fix: correct implementation of srav (#245)

Existing implementation of SRAV had a bug where it would perform a
shift with all bytes of the rs register when the spec says it
should only be using the lower 5 bits of the register. Updates the
implementation to reflect this, updates the existing test to use
the same test vector as provided in the open mips tests, and adds
fuzz tests that shows srav works as expected with rs values that
have more than the lower 5 bits set.

cannon/mipsevm/exec/mips_instructions.go

@@ -153,7 +153,8 @@ func ExecuteMipsInstruction(insn, opcode, fun, rs, rt, mem uint32) uint32 {
 		case 0x06: // srlv
 			return rt >> (rs & 0x1F)
 		case 0x07: // srav
-			return SignExtend(rt>>rs, 32-rs)
+			shamt := rs & 0x1F
+			return SignExtend(rt>>shamt, 32-shamt)
 		// functs in range [0x8, 0x1b] are handled specially by other functions
 		case 0x08: // jr
 			return rs

cannon/mipsevm/tests/open_mips_tests/test/srav_large.asm

+###############################################################################
+# Description:
+#   Tests that the 'srav' instruction properly only utilizes the lower 5 bits
+#   of the rs register rather than using the entire 32 bits.
+#
+###############################################################################
+
+
+    .section .test, "x"
+    .balign 4
+    .set    noreorder
+    .global test
+    .ent    test
+test:
+    lui     $s0, 0xbfff         # Load the base address 0xbffffff0
+    ori     $s0, 0xfff0
+    ori     $s1, $0, 1          # Prepare the 'done' status
+
+    #### Test code start ####
+
+    lui     $t0, 0xdeaf         # A = 0xdeafbeef
+    ori     $t0, 0xbeef
+    ori     $t1, $0, 0x2c
+    srav    $t2, $t0, $t1       # B = 0xdeafbeef >> (0x2c & 0x1f) = 0xdeadbeef >> 12 = 0xfffdeafb
+    lui     $t3, 0xfffd
+    ori     $t3, 0xeafb
+    subu    $t4, $t2, $t3
+    sltiu   $v0, $t4, 1
+
+    #### Test code end ####
+
+    sw      $v0, 8($s0)         # Set the test result
+    sw      $s1, 4($s0)         # Set 'done'
+
+$done:
+    jr      $ra
+    nop
+
+    .end test

packages/contracts-bedrock/semver-lock.json

@@ -140,11 +140,11 @@
     "sourceCodeHash": "0x3ff4a3f21202478935412d47fd5ef7f94a170402ddc50e5c062013ce5544c83f"
   },
   "src/cannon/MIPS.sol": {
-    "initCodeHash": "0x67e75c05afbb472210294f922631a3f5762b05216ba2351c493aa846e5b45dfc",
+    "initCodeHash": "0x0f7ca5db42a3829a6d20d8fe81e3462922c1d0e335be1394fbf79287c289c3f3",
     "sourceCodeHash": "0x5f5d13be282305b2947079cbf8f01e750e5c88a2b7a7a7267e79cba3ee4616fa"
   },
   "src/cannon/MIPS2.sol": {
-    "initCodeHash": "0x926af65d34d68a15333d127f5354ff2dcb6307fa51b799f597fd3c648b5c901e",
+    "initCodeHash": "0xf3ff16b352e256534761844d49e159c267dd09e79bc9ddec78d0c90f8746ea20",
     "sourceCodeHash": "0x115bd6a4c4d77ed210dfd468675b409fdae9f79b932063c138f0765ba9063462"
   },
   "src/cannon/PreimageOracle.sol": {

packages/contracts-bedrock/src/cannon/libraries/MIPSInstructions.sol

@@ -227,7 +227,10 @@ library MIPSInstructions {
                 }
                 // srav
                 else if (_fun == 0x07) {
-                    return signExtend(_rt >> _rs, 32 - _rs);
+                    // shamt here is different than the typical shamt which comes from the
+                    // instruction itself, here it comes from the rs register
+                    uint32 shamt = _rs & 0x1F;
+                    return signExtend(_rt >> shamt, 32 - shamt);
                 }
                 // functs in range [0x8, 0x1b] are handled specially by other functions
                 // Explicitly enumerate each funct in range to reduce code diff against Go Vm

packages/contracts-bedrock/test/cannon/MIPS.t.sol

@@ -4,6 +4,7 @@ pragma solidity 0.8.15;
 import { CommonTest } from "test/setup/CommonTest.sol";
 import { MIPS } from "src/cannon/MIPS.sol";
 import { PreimageOracle } from "src/cannon/PreimageOracle.sol";
+import { MIPSInstructions } from "src/cannon/libraries/MIPSInstructions.sol";
 import "src/dispute/lib/Types.sol";
 
 contract MIPS_Test is CommonTest {
@@ -1437,15 +1438,46 @@ contract MIPS_Test is CommonTest {
     function test_srav_succeeds() external {
         uint32 insn = encodespec(0xa, 0x9, 0x8, 7); // srav t0, t1, t2
         (MIPS.State memory state, bytes memory proof) = constructMIPSState(0, insn, 0x4, 0);
-        state.registers[9] = 0x20_00; // t1
-        state.registers[10] = 4; // t2
+        state.registers[9] = 0xdeafbeef; // t1
+        state.registers[10] = 12; // t2
 
         MIPS.State memory expect;
         expect.memRoot = state.memRoot;
         expect.pc = state.nextPC;
         expect.nextPC = state.nextPC + 4;
         expect.step = state.step + 1;
-        expect.registers[8] = state.registers[9] >> state.registers[10]; // t0
+        expect.registers[8] = 0xfffdeafb; // t0
+        expect.registers[9] = state.registers[9];
+        expect.registers[10] = state.registers[10];
+
+        bytes memory enc = encodeState(state);
+        bytes32 postState = mips.step(enc, proof, 0);
+        assertEq(postState, outputState(expect), "unexpected post state");
+    }
+
+    /// @notice Tests that the SRAV instruction succeeds when it includes extra bits in the shift
+    ///         amount beyond the lower 5 bits that are actually used for the shift. Extra bits
+    ///         need to be ignored but the instruction should still succeed.
+    /// @param _rs Value to set in the shift register $rs.
+    function testFuzz_srav_withExtraBits_succeeds(uint32 _rs) external {
+        // Assume
+        // Force _rs to have more than 5 bits set.
+        _rs = uint32(bound(uint256(_rs), 0x20, type(uint32).max));
+
+        uint32 insn = encodespec(0xa, 0x9, 0x8, 7); // srav t0, t1, t2
+        (MIPS.State memory state, bytes memory proof) = constructMIPSState(0, insn, 0x4, 0);
+        state.registers[9] = 0xdeadbeef; // t1
+        state.registers[10] = _rs; // t2
+
+        // Calculate shamt
+        uint32 shamt = state.registers[10] & 0x1F;
+
+        MIPS.State memory expect;
+        expect.memRoot = state.memRoot;
+        expect.pc = state.nextPC;
+        expect.nextPC = state.nextPC + 4;
+        expect.step = state.step + 1;
+        expect.registers[8] = MIPSInstructions.signExtend(state.registers[9] >> shamt, 32 - shamt); // t0
         expect.registers[9] = state.registers[9];
         expect.registers[10] = state.registers[10];
 

packages/contracts-bedrock/test/kontrol/proofs/utils/DeploymentSummary.sol

@@ -27,7 +27,7 @@ contract DeploymentSummary is DeploymentSummaryCode {
     address internal constant l1StandardBridgeProxyAddress = 0x20A42a5a785622c6Ba2576B2D6e924aA82BFA11D;
     address internal constant l2OutputOracleAddress = 0x19652082F846171168Daf378C4fD3ee85a0D4A60;
     address internal constant l2OutputOracleProxyAddress = 0x39Af23E00F1e662025aA01b0cEdA19542B78DF99;
-    address internal constant mipsAddress = 0xecD4f82974C78282709e1C318ef108AA5c4631D3;
+    address internal constant mipsAddress = 0xaB87a32314674174f62160a7Ce648b24e6D9D36e;
     address internal constant optimismMintableERC20FactoryAddress = 0x39Aea2Dd53f2d01c15877aCc2791af6BDD7aD567;
     address internal constant optimismMintableERC20FactoryProxyAddress = 0xc7B87b2b892EA5C3CfF47168881FE168C00377FB;
     address internal constant optimismPortalAddress = 0xbdD90485FCbcac869D5b5752179815a3103d8131;

packages/contracts-bedrock/test/kontrol/proofs/utils/DeploymentSummaryFaultProofs.sol

@@ -27,7 +27,7 @@ contract DeploymentSummaryFaultProofs is DeploymentSummaryFaultProofsCode {
     address internal constant l1StandardBridgeProxyAddress = 0x20A42a5a785622c6Ba2576B2D6e924aA82BFA11D;
     address internal constant l2OutputOracleAddress = 0x19652082F846171168Daf378C4fD3ee85a0D4A60;
     address internal constant l2OutputOracleProxyAddress = 0x39Af23E00F1e662025aA01b0cEdA19542B78DF99;
-    address internal constant mipsAddress = 0xecD4f82974C78282709e1C318ef108AA5c4631D3;
+    address internal constant mipsAddress = 0xaB87a32314674174f62160a7Ce648b24e6D9D36e;
     address internal constant optimismMintableERC20FactoryAddress = 0x39Aea2Dd53f2d01c15877aCc2791af6BDD7aD567;
     address internal constant optimismMintableERC20FactoryProxyAddress = 0xc7B87b2b892EA5C3CfF47168881FE168C00377FB;
     address internal constant optimismPortalAddress = 0xbdD90485FCbcac869D5b5752179815a3103d8131;