crypto: use recoverable signature for secp256k1 (#564)

narwhal/.gitignore

@@ -26,3 +26,4 @@ workspace.xml
 **/*.rs.bk
 scripts/build_and_fab
 ./justfile
+crypto/proptest-regressions/tests/secp256k1_tests.txt

narwhal/crypto/Cargo.toml

@@ -20,7 +20,9 @@ tokio = { version = "1.20.0", features = ["sync", "rt", "macros"] }
 ark-bls12-377 = { version = "0.3.0", features = ["std"], optional = true }
 hkdf = { version = "0.12.3", features = ["std"] }
 serde_with = "1.14.0"
-rust_secp256k1 = { version = "0.24.0", package = "secp256k1", features = ["rand", "bitcoin_hashes"] }
+serde_json = "1.0.80"
+schemars ="0.8.10"
+rust_secp256k1 = { version = "0.24.0", package = "secp256k1", features = ["recovery", "rand-std", "bitcoin_hashes", "global-context"] }
 
 # TODO: switch to https://github.com/celo-org/celo-bls-snark-rs 
 # when https://github.com/celo-org/celo-bls-snark-rs/issues/228 is solved
@@ -57,5 +59,4 @@ sha3 = "0.10.1"
 hex-literal = "0.3.4"
 proptest = "1.0.0"
 proptest-derive = "0.3.0"
-serde_json = "1.0.80"
-k256 = { version = "0.11.3", features = ["ecdsa", "sha256"] }
+k256 = { version = "0.11.3", features = ["ecdsa", "sha256", "keccak256"] }

narwhal/crypto/src/secp256k1.rs

@@ -30,11 +30,14 @@ pub struct Secp256k1PrivateKey {
     pub bytes: OnceCell<[u8; constants::SECRET_KEY_SIZE]>,
 }
 
+// Compact signature followed by one extra byte for recover id, used to recover public key from signature.
+pub const RECOVERABLE_SIGNATURE_SIZE: usize = constants::COMPACT_SIGNATURE_SIZE + 1;
+
 #[readonly::make]
 #[derive(Debug, Clone)]
 pub struct Secp256k1Signature {
-    pub sig: rust_secp256k1::ecdsa::Signature,
-    pub bytes: OnceCell<[u8; constants::COMPACT_SIGNATURE_SIZE]>,
+    pub sig: rust_secp256k1::ecdsa::RecoverableSignature,
+    pub bytes: OnceCell<[u8; RECOVERABLE_SIGNATURE_SIZE]>,
 }
 
 impl std::hash::Hash for Secp256k1PublicKey {
@@ -71,9 +74,17 @@ impl VerifyingKey for Secp256k1PublicKey {
 
 impl Verifier<Secp256k1Signature> for Secp256k1PublicKey {
     fn verify(&self, msg: &[u8], signature: &Secp256k1Signature) -> Result<(), signature::Error> {
+        // k256 defaults to keccak256 as digest to hash message for sign/verify, thus use this hash function to match in proptest.
+        #[cfg(test)]
+        let message =
+            Message::from_slice(<sha3::Keccak256 as sha3::digest::Digest>::digest(msg).as_slice())
+                .unwrap();
+
+        #[cfg(not(test))]
         let message = Message::from_hashed_data::<rust_secp256k1::hashes::sha256::Hash>(msg);
+
         let vrfy = Secp256k1::verification_only();
-        vrfy.verify_ecdsa(&message, &signature.sig, &self.pubkey)
+        vrfy.verify_ecdsa(&message, &signature.sig.to_standard(), &self.pubkey)
             .map_err(|_e| signature::Error::new())
     }
 }
@@ -218,7 +229,8 @@ impl<'de> Deserialize<'de> for Secp256k1Signature {
 
 impl Signature for Secp256k1Signature {
     fn from_bytes(bytes: &[u8]) -> Result<Self, signature::Error> {
-        match rust_secp256k1::ecdsa::Signature::from_compact(bytes) {
+        let recovery_id = rust_secp256k1::ecdsa::RecoveryId::from_i32(bytes[64] as i32).unwrap();
+        match rust_secp256k1::ecdsa::RecoverableSignature::from_compact(&bytes[..64], recovery_id) {
             Ok(sig) => Ok(Secp256k1Signature {
                 sig,
                 bytes: OnceCell::new(),
@@ -231,13 +243,17 @@ impl Signature for Secp256k1Signature {
 impl Authenticator for Secp256k1Signature {
     type PubKey = Secp256k1PublicKey;
     type PrivKey = Secp256k1PrivateKey;
-    const LENGTH: usize = constants::COMPACT_SIGNATURE_SIZE;
+    const LENGTH: usize = RECOVERABLE_SIGNATURE_SIZE;
 }
 
 impl AsRef<[u8]> for Secp256k1Signature {
     fn as_ref(&self) -> &[u8] {
+        let mut bytes = [0u8; RECOVERABLE_SIGNATURE_SIZE];
+        let (recovery_id, sig) = self.sig.serialize_compact();
+        bytes[..64].copy_from_slice(&sig);
+        bytes[64] = recovery_id.to_i32() as u8;
         self.bytes
-            .get_or_try_init::<_, eyre::Report>(|| Ok(self.sig.serialize_compact()))
+            .get_or_try_init::<_, eyre::Report>(|| Ok(bytes))
             .expect("OnceCell invariant violated")
     }
 }
@@ -264,8 +280,7 @@ impl Display for Secp256k1Signature {
 
 impl Default for Secp256k1Signature {
     fn default() -> Self {
-        <Secp256k1Signature as Signature>::from_bytes(&[0u8; constants::COMPACT_SIGNATURE_SIZE])
-            .unwrap()
+        <Secp256k1Signature as Signature>::from_bytes(&[1u8; RECOVERABLE_SIGNATURE_SIZE]).unwrap()
     }
 }
 
@@ -340,10 +355,17 @@ impl FromStr for Secp256k1KeyPair {
 
 impl Signer<Secp256k1Signature> for Secp256k1KeyPair {
     fn try_sign(&self, msg: &[u8]) -> Result<Secp256k1Signature, signature::Error> {
-        let secp = Secp256k1::new();
+        let secp = Secp256k1::signing_only();
+        #[cfg(test)]
+        let message =
+            Message::from_slice(<sha3::Keccak256 as sha3::digest::Digest>::digest(msg).as_slice())
+                .unwrap();
+
+        #[cfg(not(test))]
         let message = Message::from_hashed_data::<rust_secp256k1::hashes::sha256::Hash>(msg);
+
         Ok(Secp256k1Signature {
-            sig: secp.sign_ecdsa(&message, &self.secret.privkey),
+            sig: secp.sign_ecdsa_recoverable(&message, &self.secret.privkey),
             bytes: OnceCell::new(),
         })
     }

narwhal/crypto/src/tests/secp256k1_tests.rs

@@ -150,31 +150,50 @@ use proptest::arbitrary::Arbitrary;
 
 proptest::proptest! {
     #[test]
-    fn test_k256_against_secp256k1_lib(
+    fn test_k256_against_secp256k1_lib_with_recovery(
         r in <[u8; 32]>::arbitrary()
-    ) {
-        let message: &[u8] = b"Hello, world!";
+) {
+        let message: &[u8] = b"hello world!";
+        let hashed_msg = rust_secp256k1::Message::from_slice(<sha3::Keccak256 as sha3::digest::Digest>::digest(message).as_slice()).unwrap();
 
         // contruct private key with bytes and signs message
         let priv_key = <Secp256k1PrivateKey as ToFromBytes>::from_bytes(&r).unwrap();
         let key_pair = Secp256k1KeyPair::from(priv_key);
         let key_pair_copied = key_pair.copy();
-        let signature = key_pair.sign(message);
+        let key_pair_copied_2 = key_pair.copy();
+        let signature: Secp256k1Signature = key_pair.sign(message);
         assert!(key_pair.public().verify(message, &signature).is_ok());
 
         // use k256 to construct private key with the same bytes and signs the same message
-        let priv_key_1 = k256::ecdsa::SigningKey::from_bytes(&r).map_err(|_e| signature::Error::new())?;
+        let priv_key_1 = k256::ecdsa::SigningKey::from_bytes(&r).unwrap();
         let pub_key_1 = priv_key_1.verifying_key();
-        let signature_1: k256::ecdsa::Signature = priv_key_1.sign(message);
+        let signature_1: k256::ecdsa::recoverable::Signature = priv_key_1.sign(message);
         assert!(pub_key_1.verify(message, &signature_1).is_ok());
 
-        // same byte string produces same private key
-        assert_eq!(key_pair.private().as_bytes(), priv_key_1.to_bytes().as_slice());
+        // two private keys are serialized the same
+        assert_eq!(key_pair_copied.private().as_bytes(), priv_key_1.to_bytes().as_slice());
 
-        // same public key is created
-        assert_eq!(key_pair_copied.public().as_bytes(), pub_key_1.to_bytes().as_slice());
+        // two pubkeys are the same
+        assert_eq!(
+            key_pair.public().as_bytes(),
+            pub_key_1.to_bytes().as_slice()
+        );
+
+        // same recovered pubkey are recovered
+        let recovered_key = signature.sig.recover(&hashed_msg).unwrap();
+        let recovered_key_1 = signature_1.recover_verifying_key(message).expect("couldn't recover pubkey");
+        assert_eq!(recovered_key.serialize(),recovered_key_1.to_bytes().as_slice());
 
         // same signatures produced from both implementations
         assert_eq!(signature.as_ref(), ToFromBytes::as_bytes(&signature_1));
-   }
+
+        // use ffi-implemented keypair to verify sig constructed by k256
+        let sig_bytes_1 = bincode::serialize(&signature_1.as_ref()).unwrap();
+        let secp_sig1 = bincode::deserialize::<Secp256k1Signature>(&sig_bytes_1).unwrap();
+        assert!(key_pair_copied_2.public().verify(message, &secp_sig1).is_ok());
+
+        // use k256 keypair to verify sig constructed by ffi-implementation
+        let typed_sig = k256::ecdsa::recoverable::Signature::try_from(signature.as_ref()).unwrap();
+        assert!(pub_key_1.verify(message, &typed_sig).is_ok());
+    }
 }