Only support ECDSA compressed public keys (#4667)

Some fixes after: https://github.com/paritytech/substrate/pull/4502

This removes the unwanted `expect`s from `MultiSigner`. Instead we
convert from full to compressed in `TryFrom` and can return an error on
invalid input.

substrate/primitives/core/src/ecdsa.rs

@@ -46,14 +46,9 @@ use secp256k1::{PublicKey, SecretKey};
 #[cfg(feature = "full_crypto")]
 type Seed = [u8; 32];
 
-/// The ECDSA public key.
+/// The ECDSA compressed public key.
 #[derive(Clone, Encode, Decode)]
-pub enum Public {
-	/// A full raw ECDSA public key.
-	Full([u8; 64]),
-	/// A compressed ECDSA public key.
-	Compressed([u8; 33]),
-}
+pub struct Public([u8; 33]);
 
 impl PartialOrd for Public {
 	fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
@@ -90,42 +85,12 @@ pub enum PublicError {
 }
 
 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`.
+	/// A new instance from the given 33-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)
+	pub fn from_raw(data: [u8; 33]) -> Self {
+		Self(data)
 	}
 }
 
@@ -135,15 +100,9 @@ impl TraitPublic for Public {
 	/// 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)
-		}
+		let mut r = [0u8; 33];
+		r.copy_from_slice(data);
+		Self(r)
 	}
 }
 
@@ -151,25 +110,19 @@ impl Derive for Public {}
 
 impl Default for Public {
 	fn default() -> Self {
-		Public::Full([0u8; 64])
+		Public([0u8; 33])
 	}
 }
 
 impl AsRef<[u8]> for Public {
 	fn as_ref(&self) -> &[u8] {
-		match self {
-			Self::Full(d) => &d[..],
-			Self::Compressed(d) => &d[..],
-		}
+		&self.0[..]
 	}
 }
 
 impl AsMut<[u8]> for Public {
 	fn as_mut(&mut self) -> &mut [u8] {
-		match self {
-			Self::Full(d) => &mut d[..],
-			Self::Compressed(d) => &mut d[..],
-		}
+		&mut self.0[..]
 	}
 }
 
@@ -177,10 +130,13 @@ impl sp_std::convert::TryFrom<&[u8]> for Public {
 	type Error = ();
 
 	fn try_from(data: &[u8]) -> Result<Self, Self::Error> {
-		if data.len() == 33 || data.len() == 64 {
+		if data.len() == 33 {
 			Ok(Self::from_slice(data))
 		} else {
-			Err(())
+			secp256k1::PublicKey::parse_slice(data, None)
+				.map(|k| k.serialize_compressed())
+				.map(Self)
+				.map_err(|_| ())
 		}
 	}
 }
@@ -192,9 +148,9 @@ impl From<Pair> for Public {
 	}
 }
 
-impl UncheckedFrom<[u8; 64]> for Public {
-	fn unchecked_from(x: [u8; 64]) -> Self {
-		Public::Full(x)
+impl UncheckedFrom<[u8; 33]> for Public {
+	fn unchecked_from(x: [u8; 33]) -> Self {
+		Public(x)
 	}
 }
 
@@ -354,8 +310,9 @@ impl Signature {
 	pub fn recover<M: AsRef<[u8]>>(&self, message: M) -> Option<Public> {
 		let message = secp256k1::Message::parse(&blake2_256(message.as_ref()));
 		let sig: (_, _) = self.try_into().ok()?;
-		secp256k1::recover(&message, &sig.0, &sig.1).ok()
-			.map(|recovered| Public::from_full(recovered.serialize()))
+		secp256k1::recover(&message, &sig.0, &sig.1)
+			.ok()
+			.map(|recovered| Public(recovered.serialize_compressed()))
 	}
 }
 
@@ -476,7 +433,7 @@ impl TraitPair for Pair {
 
 	/// Get the public key.
 	fn public(&self) -> Public {
-		Public::from_full(self.public.serialize())
+		Public(self.public.serialize_compressed())
 	}
 
 	/// Sign a message.
@@ -490,9 +447,7 @@ 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.as_compressed()
-								.map(|p| &p[..] == &actual.serialize_compressed()[..])
-								.unwrap_or(false),
+			Ok(actual) => &pubkey.0[..] == &actual.serialize_compressed()[..],
 			_ => false,
 		}
 	}
@@ -587,9 +542,12 @@ mod test {
 			&hex!("9d61b19deffd5a60ba844af492ec2cc44449c5697b326919703bac031cae7f60")
 		);
 		let public = pair.public();
-		assert_eq!(public, Public::from_raw(
-			hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4")
-		));
+		assert_eq!(
+			public,
+			Public::try_from(
+				&hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4")[..],
+			).unwrap(),
+		);
 		let message = b"";
 		let signature = hex!("3dde91174bd9359027be59a428b8146513df80a2a3c7eda2194f64de04a69ab97b753169e94db6ffd50921a2668a48b94ca11e3d32c1ff19cfe88890aa7e8f3c00");
 		let signature = Signature::from_raw(signature);
@@ -604,9 +562,12 @@ mod test {
 			None
 		).unwrap();
 		let public = pair.public();
-		assert_eq!(public, Public::from_raw(
-			hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4")
-		));
+		assert_eq!(
+			public,
+			Public::try_from(
+				&hex!("8db55b05db86c0b1786ca49f095d76344c9e6056b2f02701a7e7f3c20aabfd913ebbe148dd17c56551a52952371071a6c604b3f3abe8f2c8fa742158ea6dd7d4")[..],
+			).unwrap(),
+		);
 		let message = b"";
 		let signature = hex!("3dde91174bd9359027be59a428b8146513df80a2a3c7eda2194f64de04a69ab97b753169e94db6ffd50921a2668a48b94ca11e3d32c1ff19cfe88890aa7e8f3c00");
 		let signature = Signature::from_raw(signature);
@@ -628,9 +589,12 @@ mod test {
 	fn seeded_pair_should_work() {
 		let pair = Pair::from_seed(b"12345678901234567890123456789012");
 		let public = pair.public();
-		assert_eq!(public, Public::from_raw(
-			hex!("5676109c54b9a16d271abeb4954316a40a32bcce023ac14c8e26e958aa68fba995840f3de562156558efbfdac3f16af0065e5f66795f4dd8262a228ef8c6d813")
-		));
+		assert_eq!(
+			public,
+			Public::try_from(
+				&hex!("5676109c54b9a16d271abeb4954316a40a32bcce023ac14c8e26e958aa68fba995840f3de562156558efbfdac3f16af0065e5f66795f4dd8262a228ef8c6d813")[..],
+			).unwrap(),
+		);
 		let message = hex!("2f8c6129d816cf51c374bc7f08c3e63ed156cf78aefb4a6550d97b87997977ee00000000000000000200d75a980182b10ab7d54bfed3c964073a0ee172f3daa62325af021a68f707511a4500000000000000");
 		let signature = pair.sign(&message[..]);
 		println!("Correct signature: {:?}", signature);

substrate/primitives/runtime/src/lib.rs

@@ -238,9 +238,7 @@ 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_compressed().expect("`who` is a valid `ECDSA` public key; qed")[..],
-			).into(),
+			MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256(&who.as_ref()[..]).into(),
 		}
 	}
 }
@@ -724,7 +722,7 @@ mod tests {
 		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());
+		let multi_signer = MultiSigner::from(pair.public());
 		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) => signer.as_compressed().map(|s| &s[..] == &pubkey[..]).unwrap_or(false),
+			Ok(pubkey) => &signer.as_ref()[..] == &pubkey[..],
 			_ => false,
 		}
 	}
@@ -1357,6 +1357,6 @@ mod tests {
 		assert!(ecdsa::Pair::verify(&signature, msg, &pair.public()));
 
 		assert!(signature.verify(msg, &pair.public()));
-		assert!(signature.verify(msg, &pair.public().into_compressed().unwrap()));
+		assert!(signature.verify(msg, &pair.public()));
 	}
 }