BFT-457: Deal with the shifted recovery ID in decoding signatures

node/libs/crypto/src/secp256k1/mod.rs

@@ -136,7 +136,8 @@ impl ByteFmt for Signature {
             "unexpected signature length: {}",
             bytes.len()
         );
-        let Some(recid) = k256::ecdsa::RecoveryId::from_byte(bytes[64]) else {
+        let recid = normalize_recovery_id(bytes[64]);
+        let Some(recid) = k256::ecdsa::RecoveryId::from_byte(recid) else {
             bail!("unexpected recovery ID: {}", bytes[64]);
         };
         let sig = k256::ecdsa::Signature::from_slice(&bytes[..64])?;
@@ -258,3 +259,17 @@ impl ByteFmt for AggregateSignature {
         self.0.iter().flat_map(|s| s.encode()).collect()
     }
 }
+
+/// Normalize the V in signatures from Ethereum tooling.
+///
+/// Based on <https://github.com/gakonst/ethers-rs/blob/51fe937f6515689b17a3a83b74a05984ad3a7f11/ethers-core/src/types/signature.rs#L202>
+fn normalize_recovery_id(v: u8) -> u8 {
+    match v {
+        // Case 0: raw/bare
+        v @ 0..=26 => (v % 4) as u8,
+        // Case 2: non-eip155 v value
+        v @ 27..=34 => ((v - 27) % 4) as u8,
+        // Case 3: eip155 V value
+        v @ 35.. => ((v - 1) % 2) as u8,
+    }
+}

node/libs/crypto/src/secp256k1/tests.rs

@@ -152,3 +152,40 @@ fn prop_sign_verify_agg_fail() {
             .unwrap_err();
     }
 }
+
+/// Test vectors from <https://web3js.readthedocs.io/en/v1.2.0/web3-eth-accounts.html#eth-accounts-signtransaction>
+///
+/// The same example has been used in <https://github.com/gakonst/ethers-rs/blob/ba00f549/ethers-signers/src/wallet/private_key.rs#L197>
+/// and transitively in <https://github.com/RustCrypto/elliptic-curves/blob/82e1b11fba2b01a6e60add989ab50af5cad14d5f/k256/src/ecdsa.rs#L234C14-L234C110>
+///
+/// This isn't strictly a use case for us, it was just a test to see what's happening with the recovery ID.
+#[test]
+fn test_ethereum_example() {
+    let unhex = |h| hex::decode(h).unwrap();
+    // Hexadecimal values from the web3js example.
+    // The rawTransaction isn't used becuase it itself contains the signature and isn't what has been hashed.
+    let mh = "88cfbd7e51c7a40540b233cf68b62ad1df3e92462f1c6018d6d67eae0f3b08f5";
+    let sk = "4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318";
+    let r = "c9cf86333bcb065d140032ecaab5d9281bde80f21b9687b3e94161de42d51895";
+    let s = "727a108a0b8d101465414033c3f705a9c7b826e596766046ee1183dbc8aeaa68";
+    let v = "25"; // Includes the chain ID of 1
+
+    let mh = unhex(mh);
+    let sk = {
+        let decoded = SecretKey::decode(&unhex(sk)).unwrap();
+        assert_eq!(hex::encode(decoded.encode()), sk, "sk encodes");
+        decoded
+    };
+
+    // The signature in the example should verify the hash.
+    let sig = format!("{r}{s}{v}");
+    let sig = Signature::decode(&unhex(&sig)).expect("sig decodes");
+    sig.verify_hash(&mh, &sk.public())
+        .expect("decoded sig verifies message hash");
+
+    // Check if we calculate the same R and V.
+    let sig = sk.sign_hash(&mh).unwrap();
+    assert_eq!(hex::encode(sig.sig.r().to_bytes()), r, "r matches");
+    assert_eq!(hex::encode(sig.sig.s().to_bytes()), s, "s matches");
+    assert_eq!(sig.recid.to_byte(), 0x0, "v is not shifted");
+}