Make MultiSigner use compressed ECDSA public key (#4502)

* Don't use compressed ecdsa public key in verify

* Make `ECDSA` public support compressed

* Make it a proper `expect` message

substrate/primitives/core/Cargo.toml

@@ -28,13 +28,13 @@ parking_lot = { version = "0.9.0", optional = true }
 sp-debug-derive = { version = "2.0.0", path = "../debug-derive" }
 sp-externalities = { version = "2.0.0", optional = true, path = "../externalities" }
 sp-storage = { version = "2.0.0", default-features = false, path = "../storage" }
+libsecp256k1 = { version = "0.3.2", default-features = false }
 
 # full crypto
 ed25519-dalek = { version = "1.0.0-pre.3", default-features = false, features = ["u64_backend", "alloc"], optional = true }
 blake2-rfc = { version = "0.2.18", default-features = false, optional = true }
 tiny-keccak = { version = "2.0.1", features = ["keccak"], optional = true }
 schnorrkel = { version = "0.8.5", features = ["preaudit_deprecated", "u64_backend"], default-features = false, optional = true }
-libsecp256k1 = { version = "0.3.2", default-features = false, optional = true }
 sha2 = { version = "0.8.0", default-features = false, optional = true }
 hex = { version = "0.4", default-features = false, optional = true }
 twox-hash = { version = "1.5.0", default-features = false, optional = true }
@@ -90,7 +90,7 @@ std = [
 	"schnorrkel/std",
 	"regex",
 	"num-traits/std",
-	"libsecp256k1",
+	"libsecp256k1/std",
 	"tiny-keccak",
 	"sp-debug-derive/std",
 	"sp-externalities",
@@ -107,7 +107,6 @@ full_crypto = [
 	"blake2-rfc",
 	"tiny-keccak",
 	"schnorrkel",
-	"libsecp256k1",
 	"hex",
 	"sha2",
 	"twox-hash",

substrate/primitives/core/src/ecdsa.rs

@@ -46,9 +46,14 @@ use secp256k1::{PublicKey, SecretKey};
 #[cfg(feature = "full_crypto")]
 type Seed = [u8; 32];
 
-/// The ECDSA 64-byte raw public key.
+/// The ECDSA public key.
 #[derive(Clone, Encode, Decode)]
-pub struct Public(pub [u8; 64]);
+pub enum Public {
+	/// A full raw ECDSA public key.
+	Full([u8; 64]),
+	/// A compressed ECDSA public key.
+	Compressed([u8; 33]),
+}
 
 impl PartialOrd for Public {
 	fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
@@ -58,47 +63,113 @@ impl PartialOrd for Public {
 
 impl Ord for Public {
 	fn cmp(&self, other: &Self) -> Ordering {
-		self.0[..].cmp(&other.0[..])
+		self.as_ref().cmp(&other.as_ref())
 	}
 }
 
 impl PartialEq for Public {
 	fn eq(&self, other: &Self) -> bool {
-		&self.0[..] == &other.0[..]
+		self.as_ref() == other.as_ref()
 	}
 }
 
 impl Eq for Public {}
 
-impl Default for Public {
-	fn default() -> Self {
-		Public([0u8; 64])
+/// An error type for SS58 decoding.
+#[cfg(feature = "std")]
+#[derive(Clone, Copy, Eq, PartialEq, Debug)]
+pub enum PublicError {
+	/// Bad alphabet.
+	BadBase58,
+	/// Bad length.
+	BadLength,
+	/// Unknown version.
+	UnknownVersion,
+	/// Invalid checksum.
+	InvalidChecksum,
+}
+
+impl Public {
+	/// A new instance from the given 64-byte `data`.
+	///
+	/// NOTE: No checking goes on to ensure this is a real public key. Only use it if
+	/// you are certain that the array actually is a pubkey. GIGO!
+	pub fn from_raw(data: [u8; 64]) -> Self {
+		Self::Full(data)
+	}
+
+	/// A new instance from the given 65-byte `data`.
+	///
+	/// NOTE: No checking goes on to ensure this is a real public key. Only use it if
+	/// you are certain that the array actually is a pubkey. GIGO!
+	pub fn from_full(data: [u8; 65]) -> Self {
+		let raw_key = &data[1..];
+		let mut key = [0u8; 64];
+		key.copy_from_slice(raw_key);
+		Self::Full(key)
+	}
+
+	/// Return in compressed format.
+	///
+	/// Returns an error if `self` is an invalid full public key.
+	pub fn as_compressed(&self) -> Result<[u8; 33], ()> {
+		match self {
+			Self::Full(d) => secp256k1::PublicKey::parse_slice(d, None)
+				.map(|k| k.serialize_compressed())
+				.map_err(|_| ()),
+			Self::Compressed(d) => Ok(*d),
+		}
+	}
+
+	/// Convert `Self` into a compressed public key.
+	///
+	/// Returns an error if `self` is an invalid full public key.
+	pub fn into_compressed(self) -> Result<Self, ()> {
+		self.as_compressed().map(Self::Compressed)
 	}
 }
 
-/// A key pair.
-#[cfg(feature = "full_crypto")]
-#[derive(Clone)]
-pub struct Pair {
-	public: PublicKey,
-	secret: SecretKey,
+impl TraitPublic for Public {
+	/// A new instance from the given slice that should be 33 bytes long.
+	///
+	/// NOTE: No checking goes on to ensure this is a real public key. Only use it if
+	/// you are certain that the array actually is a pubkey. GIGO!
+	fn from_slice(data: &[u8]) -> Self {
+		if data.len() == 33 {
+			let mut r = [0u8; 33];
+			r.copy_from_slice(data);
+			Self::Compressed(r)
+		} else {
+			let mut r = [0u8; 64];
+			r.copy_from_slice(data);
+			Self::Full(r)
+		}
+	}
 }
 
-impl AsRef<[u8; 64]> for Public {
-	fn as_ref(&self) -> &[u8; 64] {
-		&self.0
+impl Derive for Public {}
+
+impl Default for Public {
+	fn default() -> Self {
+		Public::Full([0u8; 64])
 	}
 }
 
 impl AsRef<[u8]> for Public {
 	fn as_ref(&self) -> &[u8] {
-		&self.0[..]
+		match self {
+			Self::Full(d) => &d[..],
+			Self::Compressed(d) => &d[..],
+		}
 	}
 }
 
 impl AsMut<[u8]> for Public {
 	fn as_mut(&mut self) -> &mut [u8] {
-		&mut self.0[..]
+		match self {
+			Self::Full(d) => &mut d[..],
+			Self::Compressed(d) => &mut d[..],
+		}
 	}
 }
 
@@ -106,22 +177,14 @@ impl sp_std::convert::TryFrom<&[u8]> for Public {
 	type Error = ();
 
 	fn try_from(data: &[u8]) -> Result<Self, Self::Error> {
-		if data.len() == 64 {
-			let mut inner = [0u8; 64];
-			inner.copy_from_slice(data);
-			Ok(Public(inner))
+		if data.len() == 33 || data.len() == 64 {
+			Ok(Self::from_slice(data))
 		} else {
 			Err(())
 		}
 	}
 }
 
-impl From<Public> for [u8; 64] {
-	fn from(x: Public) -> Self {
-		x.0
-	}
-}
-
 #[cfg(feature = "full_crypto")]
 impl From<Pair> for Public {
 	fn from(x: Pair) -> Self {
@@ -131,7 +194,7 @@ impl From<Pair> for Public {
 
 impl UncheckedFrom<[u8; 64]> for Public {
 	fn unchecked_from(x: [u8; 64]) -> Self {
-		Public(x)
+		Public::Full(x)
 	}
 }
 
@@ -146,7 +209,7 @@ impl std::fmt::Display for Public {
 impl std::fmt::Debug for Public {
 	fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
 		let s = self.to_ss58check();
-		write!(f, "{} ({}...)", crate::hexdisplay::HexDisplay::from(&&self.0[..]), &s[0..8])
+		write!(f, "{} ({}...)", crate::hexdisplay::HexDisplay::from(&self.as_ref()), &s[0..8])
 	}
 }
 
@@ -168,7 +231,7 @@ impl<'de> Deserialize<'de> for Public {
 #[cfg(feature = "full_crypto")]
 impl sp_std::hash::Hash for Public {
 	fn hash<H: sp_std::hash::Hasher>(&self, state: &mut H) {
-		self.0.hash(state);
+		self.as_ref().hash(state);
 	}
 }
 
@@ -317,60 +380,6 @@ impl<'a> TryFrom<&'a Signature> for (secp256k1::Signature, secp256k1::RecoveryId
 	}
 }
 
-/// An error type for SS58 decoding.
-#[cfg(feature = "std")]
-#[derive(Clone, Copy, Eq, PartialEq, Debug)]
-pub enum PublicError {
-	/// Bad alphabet.
-	BadBase58,
-	/// Bad length.
-	BadLength,
-	/// Unknown version.
-	UnknownVersion,
-	/// Invalid checksum.
-	InvalidChecksum,
-}
-
-impl Public {
-	/// A new instance from the given 64-byte `data`.
-	///
-	/// NOTE: No checking goes on to ensure this is a real public key. Only use it if
-	/// you are certain that the array actually is a pubkey. GIGO!
-	pub fn from_raw(data: [u8; 64]) -> Self {
-		Public(data)
-	}
-
-	/// A new instance from the given 65-byte `data`.
-	///
-	/// NOTE: No checking goes on to ensure this is a real public key. Only use it if
-	/// you are certain that the array actually is a pubkey. GIGO!
-	pub fn from_full(data: [u8; 65]) -> Self {
-		let raw_key = &data[1..];
-		let mut key = [0u8; 64];
-		key.copy_from_slice(raw_key);
-		Public(key)
-	}
-
-	/// Return a slice filled with raw data.
-	pub fn as_array_ref(&self) -> &[u8; 64] {
-		self.as_ref()
-	}
-}
-
-impl TraitPublic for Public {
-	/// A new instance from the given slice that should be 33 bytes long.
-	///
-	/// NOTE: No checking goes on to ensure this is a real public key. Only use it if
-	/// you are certain that the array actually is a pubkey. GIGO!
-	fn from_slice(data: &[u8]) -> Self {
-		let mut r = [0u8; 64];
-		r.copy_from_slice(data);
-		Public(r)
-	}
-}
-
-impl Derive for Public {}
-
 /// Derive a single hard junction.
 #[cfg(feature = "full_crypto")]
 fn derive_hard_junction(secret_seed: &Seed, cc: &[u8; 32]) -> Seed {
@@ -388,6 +397,14 @@ pub enum DeriveError {
 	SoftKeyInPath,
 }
 
+/// A key pair.
+#[cfg(feature = "full_crypto")]
+#[derive(Clone)]
+pub struct Pair {
+	public: PublicKey,
+	secret: SecretKey,
+}
+
 #[cfg(feature = "full_crypto")]
 impl TraitPair for Pair {
 	type Public = Public;
@@ -473,7 +490,9 @@ impl TraitPair for Pair {
 		let message = secp256k1::Message::parse(&blake2_256(message.as_ref()));
 		let sig: (_, _) = match sig.try_into() { Ok(x) => x, _ => return false };
 		match secp256k1::recover(&message, &sig.0, &sig.1) {
-			Ok(actual) => &pubkey.0[..] == &actual.serialize()[1..],
+			Ok(actual) => pubkey.as_compressed()
+								.map(|p| &p[..] == &actual.serialize_compressed()[..])
+								.unwrap_or(false),
 			_ => false,
 		}
 	}

substrate/primitives/core/src/hexdisplay.rs

@@ -58,7 +58,7 @@ pub trait AsBytesRef {
 	fn as_bytes_ref(&self) -> &[u8];
 }
 
-impl<'a> AsBytesRef for &'a [u8] {
+impl AsBytesRef for &[u8] {
 	fn as_bytes_ref(&self) -> &[u8] { self }
 }
 

substrate/primitives/runtime/src/lib.rs

@@ -212,7 +212,7 @@ pub enum MultiSigner {
 	Ed25519(ed25519::Public),
 	/// An Sr25519 identity.
 	Sr25519(sr25519::Public),
-	/// An SECP256k1/ECDSA identity (actually, the Blake2 hash of the pub key).
+	/// An SECP256k1/ECDSA identity (actually, the Blake2 hash of the compressed pub key).
 	Ecdsa(ecdsa::Public),
 }
 
@@ -246,7 +246,9 @@ impl traits::IdentifyAccount for MultiSigner {
 		match self {
 			MultiSigner::Ed25519(who) => <[u8; 32]>::from(who).into(),
 			MultiSigner::Sr25519(who) => <[u8; 32]>::from(who).into(),
-			MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256(who.as_ref()).into(),
+			MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256(
+				&who.as_compressed().expect("`who` is a valid `ECDSA` public key; qed")[..],
+			).into(),
 		}
 	}
 }
@@ -688,8 +690,9 @@ pub fn print(print: impl traits::Printable) {
 
 #[cfg(test)]
 mod tests {
-	use crate::DispatchError;
+	use super::*;
 	use codec::{Encode, Decode};
+	use sp_core::crypto::Pair;
 
 	#[test]
 	fn opaque_extrinsic_serialization() {
@@ -716,4 +719,20 @@ mod tests {
 			},
 		);
 	}
+
+	#[test]
+	fn multi_signature_ecdsa_verify_works() {
+		let msg = &b"test-message"[..];
+		let (pair, _) = ecdsa::Pair::generate();
+
+		let signature = pair.sign(&msg);
+		assert!(ecdsa::Pair::verify(&signature, msg, &pair.public()));
+
+		let multi_sig = MultiSignature::from(signature);
+		let multi_signer = MultiSigner::from(pair.public());
+		assert!(multi_sig.verify(msg, &multi_signer.into_account()));
+
+		let multi_signer = MultiSigner::from(pair.public().into_compressed().unwrap());
+		assert!(multi_sig.verify(msg, &multi_signer.into_account()));
+	}
 }

substrate/primitives/runtime/src/traits.rs

@@ -102,7 +102,7 @@ impl Verify for sp_core::ecdsa::Signature {
 			self.as_ref(),
 			&sp_io::hashing::blake2_256(msg.get()),
 		) {
-			Ok(pubkey) => <dyn AsRef<[u8]>>::as_ref(signer) == &pubkey[..],
+			Ok(pubkey) => signer.as_compressed().map(|s| &s[..] == &pubkey[..]).unwrap_or(false),
 			_ => false,
 		}
 	}
@@ -1307,8 +1307,9 @@ pub trait BlockIdTo<Block: self::Block> {
 
 #[cfg(test)]
 mod tests {
-	use super::AccountIdConversion;
+	use super::*;
 	use crate::codec::{Encode, Decode, Input};
+	use sp_core::{crypto::Pair, ecdsa};
 
 	mod t {
 		use sp_core::crypto::KeyTypeId;
@@ -1388,4 +1389,16 @@ mod tests {
 		assert_eq!(t.remaining_len(), Ok(None));
 		assert_eq!(buffer, [0, 0]);
 	}
+
+	#[test]
+	fn ecdsa_verify_works() {
+		let msg = &b"test-message"[..];
+		let (pair, _) = ecdsa::Pair::generate();
+
+		let signature = pair.sign(&msg);
+		assert!(ecdsa::Pair::verify(&signature, msg, &pair.public()));
+
+		assert!(signature.verify(msg, &pair.public()));
+		assert!(signature.verify(msg, &pair.public().into_compressed().unwrap()));
+	}
 }