Fix ecdsa_bls verify in BEEFY primitives

substrate/primitives/consensus/beefy/src/lib.rs

@@ -133,7 +133,7 @@ pub mod bls_crypto {
 		<MsgHash as Hash>::Output: Into<[u8; 32]>,
 	{
 		fn verify(&self, signature: &<Self as RuntimeAppPublic>::Signature, msg: &[u8]) -> bool {
-			// `w3f-bls` library uses IETF hashing standard and as such does not exposes
+			// `w3f-bls` library uses IETF hashing standard and as such does not expose
 			// a choice of hash to field function.
 			// We are directly calling into the library to avoid introducing new host call.
 			// and because BeefyAuthorityId::verify is being called in the runtime so we don't have
@@ -157,7 +157,7 @@ pub mod bls_crypto {
 pub mod ecdsa_bls_crypto {
 	use super::{BeefyAuthorityId, Hash, RuntimeAppPublic, KEY_TYPE};
 	use sp_application_crypto::{app_crypto, ecdsa_bls377};
-	use sp_core::{crypto::Wraps, ecdsa_bls377::Pair as EcdsaBlsPair, Pair as _};
+	use sp_core::{crypto::Wraps, ecdsa_bls377::Pair as EcdsaBlsPair};
 
 	app_crypto!(ecdsa_bls377, KEY_TYPE);
 
@@ -172,12 +172,22 @@ pub mod ecdsa_bls_crypto {
 		<MsgHash as Hash>::Output: Into<[u8; 32]>,
 	{
 		fn verify(&self, signature: &<Self as RuntimeAppPublic>::Signature, msg: &[u8]) -> bool {
-			// `w3f-bls` library uses IETF hashing standard and as such does not exposes
-			// a choice of hash to field function.
-			// We are directly calling into the library to avoid introducing new host call.
-			// and because BeefyAuthorityId::verify is being called in the runtime so we don't have
-
-			EcdsaBlsPair::verify(signature.as_inner_ref(), msg, self.as_inner_ref())
+			// We can not simply call
+			// `EcdsaBlsPair::verify(signature.as_inner_ref(), msg, self.as_inner_ref())`
+			// because that invokes ecdsa default verification which perfoms blake2 hash
+			// which we don't want. This is because ECDSA signatures are meant to be verified
+			// on Ethereum network where Keccak hasher is siginificantly cheaper than Blake2b.
+			// See Figure 3 of [OnSc21](https://www.scitepress.org/Papers/2021/106066/106066.pdf)
+			// for comparison.
+			//
+			// - [OnSc21] Onica, E., & Schifirneţ, C. (2021). Towards efficient hashing in
+			// ethereum smart contracts. Proceedings of the 16th International
+			// Conference on Software Technologies, ()
+			EcdsaBlsPair::verify_with_hasher::<MsgHash>(
+				signature.as_inner_ref(),
+				msg,
+				self.as_inner_ref(),
+			)
 		}
 	}
 }
@@ -257,6 +267,7 @@ pub enum ConsensusLog<AuthorityId: Codec> {
 ///
 /// A vote message is a direct vote created by a BEEFY node on every voting round
 /// and is gossiped to its peers.
+// TODO: Remove `Signature` generic type, instead get it from `Id::Signature`.
 #[derive(Clone, Debug, Decode, Encode, PartialEq, TypeInfo)]
 pub struct VoteMessage<Number, Id, Signature> {
 	/// Commit to information extracted from a finalized block
@@ -507,11 +518,15 @@ mod tests {
 		let msg = &b"test-message"[..];
 		let (pair, _) = ecdsa_bls_crypto::Pair::generate();
 
-		let signature: ecdsa_bls_crypto::Signature = pair.as_inner_ref().sign(&msg).into();
+		let signature: ecdsa_bls_crypto::Signature =
+			pair.as_inner_ref().sign_with_hasher::<Keccak256>(&msg).into();
 
 		// Verification works if same hashing function is used when signing and verifying.
 		assert!(BeefyAuthorityId::<Keccak256>::verify(&pair.public(), &signature, msg));
 
+		// Verification doesn't work if we verify function provided by pair_crypto implementation
+		assert!(!ecdsa_bls_crypto::Pair::verify(&signature, msg, &pair.public()));
+
 		// Other public key doesn't work
 		let (other_pair, _) = ecdsa_bls_crypto::Pair::generate();
 		assert!(!BeefyAuthorityId::<Keccak256>::verify(&other_pair.public(), &signature, msg,));

substrate/primitives/core/src/paired_crypto.rs

@@ -39,7 +39,11 @@ use sp_std::convert::TryFrom;
 /// ECDSA and BLS12-377 paired crypto scheme
 #[cfg(feature = "bls-experimental")]
 pub mod ecdsa_bls377 {
-	use crate::{bls377, crypto::CryptoTypeId, ecdsa};
+	use crate::{
+		bls377,
+		crypto::{CryptoTypeId, Pair as PairT, UncheckedFrom},
+		ecdsa,
+	};
 
 	/// An identifier used to match public keys against BLS12-377 keys
 	pub const CRYPTO_ID: CryptoTypeId = CryptoTypeId(*b"ecb7");
@@ -71,6 +75,60 @@ pub mod ecdsa_bls377 {
 	impl super::CryptoType for Pair {
 		type Pair = Pair;
 	}
+
+	#[cfg(feature = "full_crypto")]
+	impl Pair {
+		/// hashes the `message` with the specified `MsgHasher` and then signs it using ECDSA
+		/// algorithm. It does not affect the behavoir of BLS12-377 component. It generates
+		/// BLS12-377 Signature according to IETF standard and disregards the hasher for the
+		/// BLS12-377 component
+		pub fn sign_with_hasher<MsgHasher: crate::Hasher>(&self, message: &[u8]) -> Signature
+		where
+			<MsgHasher as crate::Hasher>::Out: Into<[u8; 32]>,
+		{
+			let msg_hash = <MsgHasher as crate::Hasher>::hash(message).into();
+
+			let mut raw: [u8; SIGNATURE_LEN] = [0u8; SIGNATURE_LEN];
+			raw[..ecdsa::SIGNATURE_SERIALIZED_SIZE]
+				.copy_from_slice(self.left.sign_prehashed(&msg_hash).as_ref());
+			raw[ecdsa::SIGNATURE_SERIALIZED_SIZE..]
+				.copy_from_slice(self.right.sign(message).as_ref());
+			<Self as PairT>::Signature::unchecked_from(raw)
+		}
+
+		/// Hashes the `message` with the specified `MsgHasher` and then verifies whether the
+		/// resulting hash was signed by the provided ECDSA public key. It does not affect the
+		/// behavior of the BLS12-377 component. It verifies whether the BLS12-377 signature was
+		/// hashed and signed according to IETF standard
+		pub fn verify_with_hasher<MsgHasher: crate::Hasher>(
+			sig: &Signature,
+			message: &[u8],
+			public: &Public,
+		) -> bool
+		where
+			<MsgHasher as crate::Hasher>::Out: Into<[u8; 32]>,
+		{
+			let msg_hash = <MsgHasher as crate::Hasher>::hash(message).into();
+
+			let Ok(left_pub) = public.0[..ecdsa::PUBLIC_KEY_SERIALIZED_SIZE].try_into() else {
+				return false
+			};
+			let Ok(left_sig) = sig.0[0..ecdsa::SIGNATURE_SERIALIZED_SIZE].try_into() else {
+				return false
+			};
+			if !ecdsa::Pair::verify_prehashed(&left_sig, &msg_hash, &left_pub) {
+				return false
+			}
+
+			let Ok(right_pub) = public.0[ecdsa::PUBLIC_KEY_SERIALIZED_SIZE..].try_into() else {
+				return false
+			};
+			let Ok(right_sig) = sig.0[ecdsa::SIGNATURE_SERIALIZED_SIZE..].try_into() else {
+				return false
+			};
+			bls377::Pair::verify(&right_sig, message.as_ref(), &right_pub)
+		}
+	}
 }
 
 /// Secure seed length.
@@ -455,12 +513,12 @@ where
 #[cfg(all(test, feature = "bls-experimental"))]
 mod test {
 	use super::*;
-	use crate::crypto::DEV_PHRASE;
+	use crate::{crypto::DEV_PHRASE, KeccakHasher};
 	use ecdsa_bls377::{Pair, Signature};
 
 	use crate::{bls377, ecdsa};
-	#[test]
 
+	#[test]
 	fn test_length_of_paired_ecdsa_and_bls377_public_key_and_signature_is_correct() {
 		assert_eq!(
 			<Pair as PairT>::Public::LEN,
@@ -617,6 +675,16 @@ mod test {
 		assert_eq!(cmp, public);
 	}
 
+	#[test]
+	fn sign_and_verify_with_hasher_works() {
+		let pair =
+			Pair::from_seed(&(b"12345678901234567890123456789012".as_slice().try_into().unwrap()));
+		let message = b"Something important";
+		let signature = pair.sign_with_hasher::<KeccakHasher>(&message[..]);
+
+		assert!(Pair::verify_with_hasher::<KeccakHasher>(&signature, &message[..], &pair.public()));
+	}
+
 	#[test]
 	fn signature_serialization_works() {
 		let pair =