BEEFY: Define a BeefyVerify trait for signatures (#12299)

* Define CustomVerify trait

Signed-off-by: Serban Iorga <serban@parity.io>

* Use ECDSA CustomVerify for MultiSignature

Signed-off-by: Serban Iorga <serban@parity.io>

* beefy: small simplifications

Signed-off-by: Serban Iorga <serban@parity.io>

* Revert "Use ECDSA CustomVerify for MultiSignature"

This reverts commit 136cff8.

* Revert "Define CustomVerify trait"

This reverts commit adf91e9.

* Define BeefyAuthorityId and BeefyVerify traits

* Improve BeefyVerify unit tests

Co-authored-by: Robert Hambrock <roberthambrock@gmail.com>

* fmt & import sp_core::blake2_256

* Renamings

* remove SignerToAccountId

* fix

Signed-off-by: Serban Iorga <serban@parity.io>
Co-authored-by: Robert Hambrock <roberthambrock@gmail.com>

client/beefy/src/keystore.rs

@@ -19,12 +19,13 @@
 use sp_application_crypto::RuntimeAppPublic;
 use sp_core::keccak_256;
 use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
+use sp_runtime::traits::Keccak256;
 
 use log::warn;
 
 use beefy_primitives::{
 	crypto::{Public, Signature},
-	KEY_TYPE,
+	BeefyVerify, KEY_TYPE,
 };
 
 use crate::error;
@@ -98,11 +99,7 @@ impl BeefyKeystore {
 	///
 	/// Return `true` if the signature is authentic, `false` otherwise.
 	pub fn verify(public: &Public, sig: &Signature, message: &[u8]) -> bool {
-		let msg = keccak_256(message);
-		let sig = sig.as_ref();
-		let public = public.as_ref();
-
-		sp_core::ecdsa::Pair::verify_prehashed(sig, &msg, public)
+		BeefyVerify::<Keccak256>::verify(sig, message, public)
 	}
 }
 

frame/beefy-mmr/src/lib.rs

@@ -73,12 +73,8 @@ where
 /// Convert BEEFY secp256k1 public keys into Ethereum addresses
 pub struct BeefyEcdsaToEthereum;
 impl Convert<beefy_primitives::crypto::AuthorityId, Vec<u8>> for BeefyEcdsaToEthereum {
-	fn convert(a: beefy_primitives::crypto::AuthorityId) -> Vec<u8> {
-		sp_core::ecdsa::Public::try_from(a.as_ref())
-			.map_err(|_| {
-				log::error!(target: "runtime::beefy", "Invalid BEEFY PublicKey format!");
-			})
-			.unwrap_or(sp_core::ecdsa::Public::from_raw([0u8; 33]))
+	fn convert(beefy_id: beefy_primitives::crypto::AuthorityId) -> Vec<u8> {
+		sp_core::ecdsa::Public::from(beefy_id)
 			.to_eth_address()
 			.map(|v| v.to_vec())
 			.map_err(|_| {

primitives/beefy/Cargo.toml

@@ -18,12 +18,13 @@ scale-info = { version = "2.1.1", default-features = false, features = ["derive"
 sp-api = { version = "4.0.0-dev", default-features = false, path = "../api" }
 sp-application-crypto = { version = "6.0.0", default-features = false, path = "../application-crypto" }
 sp-core = { version = "6.0.0", default-features = false, path = "../core" }
+sp-io = { version = "6.0.0", default-features = false, path = "../io" }
+sp-mmr-primitives = { version = "4.0.0-dev", default-features = false, path = "../merkle-mountain-range" }
 sp-runtime = { version = "6.0.0", default-features = false, path = "../runtime" }
 sp-std = { version = "4.0.0", default-features = false, path = "../std" }
 
 [dev-dependencies]
-hex = "0.4.3"
-hex-literal = "0.3"
+array-bytes = "4.1"
 sp-keystore = { version = "0.12.0", path = "../keystore" }
 
 [features]
@@ -34,6 +35,8 @@ std = [
 	"sp-api/std",
 	"sp-application-crypto/std",
 	"sp-core/std",
+	"sp-io/std",
+	"sp-mmr-primitives/std",
 	"sp-runtime/std",
 	"sp-std/std",
 ]

primitives/beefy/src/lib.rs

@@ -42,12 +42,30 @@ pub use commitment::{
 
 use codec::{Codec, Decode, Encode};
 use scale_info::TypeInfo;
+use sp_application_crypto::RuntimeAppPublic;
 use sp_core::H256;
+use sp_runtime::traits::Hash;
 use sp_std::prelude::*;
 
 /// Key type for BEEFY module.
 pub const KEY_TYPE: sp_application_crypto::KeyTypeId = sp_application_crypto::KeyTypeId(*b"beef");
 
+/// Trait representing BEEFY authority id.
+pub trait BeefyAuthorityId: RuntimeAppPublic {}
+
+/// Means of verification for a BEEFY authority signature.
+///
+/// Accepts custom hashing fn for the message and custom convertor fn for the signer.
+pub trait BeefyVerify<MsgHash: Hash> {
+	/// Type of the signer.
+	type Signer: BeefyAuthorityId;
+
+	/// Verify a signature.
+	///
+	/// Return `true` if signature is valid for the value.
+	fn verify(&self, msg: &[u8], signer: &Self::Signer) -> bool;
+}
+
 /// BEEFY cryptographic types
 ///
 /// This module basically introduces three crypto types:
@@ -61,14 +79,36 @@ pub const KEY_TYPE: sp_application_crypto::KeyTypeId = sp_application_crypto::Ke
 /// The current underlying crypto scheme used is ECDSA. This can be changed,
 /// without affecting code restricted against the above listed crypto types.
 pub mod crypto {
+	use super::{BeefyAuthorityId, BeefyVerify, Hash};
 	use sp_application_crypto::{app_crypto, ecdsa};
+	use sp_core::crypto::Wraps;
 	app_crypto!(ecdsa, crate::KEY_TYPE);
 
 	/// Identity of a BEEFY authority using ECDSA as its crypto.
 	pub type AuthorityId = Public;
 
 	/// Signature for a BEEFY authority using ECDSA as its crypto.
 	pub type AuthoritySignature = Signature;
+
+	impl BeefyAuthorityId for AuthorityId {}
+
+	impl<MsgHash: Hash> BeefyVerify<MsgHash> for AuthoritySignature
+	where
+		<MsgHash as Hash>::Output: Into<[u8; 32]>,
+	{
+		type Signer = AuthorityId;
+
+		fn verify(&self, msg: &[u8], signer: &Self::Signer) -> bool {
+			let msg_hash = <MsgHash as Hash>::hash(msg).into();
+			match sp_io::crypto::secp256k1_ecdsa_recover_compressed(
+				self.as_inner_ref().as_ref(),
+				&msg_hash,
+			) {
+				Ok(raw_pubkey) => raw_pubkey.as_ref() == AsRef::<[u8]>::as_ref(signer),
+				_ => false,
+			}
+		}
+	}
 }
 
 /// The `ConsensusEngineId` of BEEFY.
@@ -181,7 +221,8 @@ sp_api::decl_runtime_apis! {
 mod tests {
 	use super::*;
 	use sp_application_crypto::ecdsa::{self, Public};
-	use sp_core::Pair;
+	use sp_core::{blake2_256, crypto::Wraps, keccak_256, Pair};
+	use sp_runtime::traits::{BlakeTwo256, Keccak256};
 
 	#[test]
 	fn validator_set() {
@@ -195,4 +236,36 @@ mod tests {
 		assert_eq!(validators.id(), set_id);
 		assert_eq!(validators.validators(), &vec![alice.public()]);
 	}
+
+	#[test]
+	fn beefy_verify_works() {
+		let msg = &b"test-message"[..];
+		let (pair, _) = crypto::Pair::generate();
+
+		let keccak_256_signature: crypto::Signature =
+			pair.as_inner_ref().sign_prehashed(&keccak_256(msg)).into();
+
+		let blake2_256_signature: crypto::Signature =
+			pair.as_inner_ref().sign_prehashed(&blake2_256(msg)).into();
+
+		// Verification works if same hashing function is used when signing and verifying.
+		assert!(BeefyVerify::<Keccak256>::verify(&keccak_256_signature, msg, &pair.public()));
+		assert!(BeefyVerify::<BlakeTwo256>::verify(&blake2_256_signature, msg, &pair.public()));
+		// Verification fails if distinct hashing functions are used when signing and verifying.
+		assert!(!BeefyVerify::<Keccak256>::verify(&blake2_256_signature, msg, &pair.public()));
+		assert!(!BeefyVerify::<BlakeTwo256>::verify(&keccak_256_signature, msg, &pair.public()));
+
+		// Other public key doesn't work
+		let (other_pair, _) = crypto::Pair::generate();
+		assert!(!BeefyVerify::<Keccak256>::verify(
+			&keccak_256_signature,
+			msg,
+			&other_pair.public()
+		));
+		assert!(!BeefyVerify::<BlakeTwo256>::verify(
+			&blake2_256_signature,
+			msg,
+			&other_pair.public()
+		));
+	}
 }

primitives/core/src/crypto.rs

@@ -987,6 +987,11 @@ pub trait IsWrappedBy<Outer>: From<Outer> + Into<Outer> {
 pub trait Wraps: Sized {
 	/// The inner type it is wrapping.
 	type Inner: IsWrappedBy<Self>;
+
+	/// Get a reference to the inner type that is wrapped.
+	fn as_inner_ref(&self) -> &Self::Inner {
+		Self::Inner::from_ref(self)
+	}
 }
 
 impl<T, Outer> IsWrappedBy<Outer> for T