Update schnorrkel with versioning (#5358)

* use versioning for deprecated api, remove deprecated api from regular verification

* Update primitives/core/src/sr25519.rs

* add test to transaction pool

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/Cargo.lock

@@ -6650,10 +6650,12 @@ dependencies = [
 name = "sc-transaction-pool"
 version = "2.0.0-alpha.5"
 dependencies = [
+ "assert_matches",
  "derive_more",
  "futures 0.3.4",
  "futures-diagnose",
  "futures-timer 2.0.2",
+ "hex",
  "log 0.4.8",
  "parity-scale-codec",
  "parity-util-mem",

substrate/client/transaction-pool/Cargo.toml

@@ -27,6 +27,8 @@ futures-timer = "2.0"
 parity-util-mem = { version = "0.6.0", default-features = false, features = ["primitive-types"] }
 
 [dev-dependencies]
+assert_matches = "1.3.0"
+hex = "0.4"
 sp-keyring = { version = "2.0.0-alpha.5", path = "../../primitives/keyring" }
 substrate-test-runtime-transaction-pool = { version = "2.0.0-dev", path = "../../test-utils/runtime/transaction-pool" }
 substrate-test-runtime-client = { version = "2.0.0-dev", path = "../../test-utils/runtime/client" }

substrate/client/transaction-pool/src/testing/pool.rs

@@ -20,15 +20,16 @@ use futures::executor::block_on;
 use txpool::{self, Pool};
 use sp_runtime::{
 	generic::BlockId,
-	transaction_validity::ValidTransaction,
+	transaction_validity::{ValidTransaction, InvalidTransaction},
 };
 use substrate_test_runtime_client::{
-	runtime::{Block, Hash, Index, Header, Extrinsic},
+	runtime::{Block, Hash, Index, Header, Extrinsic, Transfer},
 	AccountKeyring::*,
 };
 use substrate_test_runtime_transaction_pool::{TestApi, uxt};
 use crate::revalidation::BACKGROUND_REVALIDATION_INTERVAL;
 use futures::task::Poll;
+use codec::Encode;
 
 fn pool() -> Pool<TestApi> {
 	Pool::new(Default::default(), TestApi::with_alice_nonce(209).into())
@@ -653,4 +654,37 @@ fn ready_set_should_eventually_resolve_when_block_update_arrives() {
 			assert_eq!(data.len(), 1);
 		}
 	}
-}
+}
+
+#[test]
+fn should_not_accept_old_signatures() {
+	use std::convert::TryFrom;
+
+	let client = Arc::new(substrate_test_runtime_client::new());
+	let pool = Arc::new(
+		BasicPool::new(Default::default(), Arc::new(FullChainApi::new(client))).0
+	);
+
+	let transfer = Transfer {
+		from: Alice.into(),
+		to: Bob.into(),
+		nonce: 0,
+		amount: 1,
+	};
+	let _bytes: sp_core::sr25519::Signature = transfer.using_encoded(|e| Alice.sign(e)).into();
+
+	// generated with schnorrkel 0.1.1 from `_bytes`
+	let old_singature = sp_core::sr25519::Signature::try_from(&hex::decode(
+		"c427eb672e8c441c86d31f1a81b22b43102058e9ce237cabe9897ea5099ffd426cd1c6a1f4f2869c3df57901d36bedcb295657adb3a4355add86ed234eb83108"
+	).expect("hex invalid")[..]).expect("signature construction failed");
+
+	let xt = Extrinsic::Transfer { transfer, signature: old_singature, exhaust_resources_when_not_first: false };
+
+	assert_matches::assert_matches!(
+		block_on(pool.submit_one(&BlockId::number(0), xt.clone())),
+		Err(error::Error::Pool(
+			sp_transaction_pool::error::Error::InvalidTransaction(InvalidTransaction::BadProof)
+		)),
+		"Should be invalid transactiono with bad proof",
+	);
+}

substrate/primitives/core/src/sr25519.rs

@@ -529,25 +529,24 @@ impl TraitPair for Pair {
 		self.0.sign(context.bytes(message)).into()
 	}
 
-	/// Verify a signature on a message. Returns true if the signature is good.
 	fn verify<M: AsRef<[u8]>>(sig: &Self::Signature, message: M, pubkey: &Self::Public) -> bool {
 		Self::verify_weak(&sig.0[..], message, pubkey)
 	}
 
-	/// Verify a signature on a message. Returns true if the signature is good.
 	fn verify_weak<P: AsRef<[u8]>, M: AsRef<[u8]>>(sig: &[u8], message: M, pubkey: P) -> bool {
-		// Match both schnorrkel 0.1.1 and 0.8.0+ signatures, supporting both wallets
-		// that have not been upgraded and those that have. To swap to 0.8.0 only,
-		// create `schnorrkel::Signature` and pass that into `verify_simple`
-		match PublicKey::from_bytes(pubkey.as_ref()) {
-			Ok(pk) => pk.verify_simple_preaudit_deprecated(
-				SIGNING_CTX, message.as_ref(), &sig,
-			).is_ok(),
-			Err(_) => false,
-		}
+		let signature = match schnorrkel::Signature::from_bytes(sig) {
+			Ok(signature) => signature,
+			Err(_) => return false,
+		};
+
+		let pub_key = match PublicKey::from_bytes(pubkey.as_ref()) {
+			Ok(pub_key) => pub_key,
+			Err(_) => return false,
+		};
+
+		pub_key.verify_simple(SIGNING_CTX, message.as_ref(), &signature).is_ok()
 	}
 
-	/// Return a vec filled with raw data.
 	fn to_raw_vec(&self) -> Vec<u8> {
 		self.0.secret.to_bytes().to_vec()
 	}
@@ -566,6 +565,20 @@ impl Pair {
 		let kp = mini_key.expand_to_keypair(ExpansionMode::Ed25519);
 		(Pair(kp), mini_key.to_bytes())
 	}
+
+	/// Verify a signature on a message. Returns `true` if the signature is good.
+	/// Supports old 0.1.1 deprecated signatures and should be used only for backward
+	/// compatibility.
+	pub fn verify_deprecated<M: AsRef<[u8]>>(sig: &Signature, message: M, pubkey: &Public) -> bool {
+		// Match both schnorrkel 0.1.1 and 0.8.0+ signatures, supporting both wallets
+		// that have not been upgraded and those that have.
+		match PublicKey::from_bytes(pubkey.as_ref()) {
+			Ok(pk) => pk.verify_simple_preaudit_deprecated(
+				SIGNING_CTX, message.as_ref(), &sig.0[..],
+			).is_ok(),
+			Err(_) => false,
+		}
+	}
 }
 
 impl CryptoType for Public {
@@ -609,14 +622,15 @@ mod compatibility_test {
 	}
 
 	#[test]
-	fn verify_known_message_should_work() {
+	fn verify_known_old_message_should_work() {
 		let public = Public::from_raw(hex!("b4bfa1f7a5166695eb75299fd1c4c03ea212871c342f2c5dfea0902b2c246918"));
 		// signature generated by the 1.1 version with the same ^^ public key.
 		let signature = Signature::from_raw(hex!(
 			"5a9755f069939f45d96aaf125cf5ce7ba1db998686f87f2fb3cbdea922078741a73891ba265f70c31436e18a9acd14d189d73c12317ab6c313285cd938453202"
 		));
 		let message = b"Verifying that I am the owner of 5G9hQLdsKQswNPgB499DeA5PkFBbgkLPJWkkS6FAM6xGQ8xD. Hash: 221455a3\n";
-		assert!(Pair::verify(&signature, &message[..], &public));
+		assert!(Pair::verify_deprecated(&signature, &message[..], &public));
+		assert!(!Pair::verify(&signature, &message[..], &public));
 	}
 }
 
@@ -776,7 +790,7 @@ mod test {
 	}
 
 	#[test]
-	fn verify_from_wasm_works() {
+	fn verify_from_old_wasm_works() {
 		// The values in this test case are compared to the output of `node-test.js` in schnorrkel-js.
 		//
 		// This is to make sure that the wasm library is compatible.
@@ -787,7 +801,8 @@ mod test {
 		let js_signature = Signature::from_raw(hex!(
 			"28a854d54903e056f89581c691c1f7d2ff39f8f896c9e9c22475e60902cc2b3547199e0e91fa32902028f2ca2355e8cdd16cfe19ba5e8b658c94aa80f3b81a00"
 		));
-		assert!(Pair::verify(&js_signature, b"SUBSTRATE", &public));
+		assert!(Pair::verify_deprecated(&js_signature, b"SUBSTRATE", &public));
+		assert!(!Pair::verify(&js_signature, b"SUBSTRATE", &public));
 	}
 
 	#[test]

substrate/primitives/io/src/lib.rs

@@ -466,9 +466,18 @@ pub trait Crypto {
 			.map(|k| k.sign(msg))
 	}
 
+	/// Verify an `sr25519` signature.
+	///
+	/// Returns `true` when the verification in successful regardless of
+	/// signature version.
+	fn sr25519_verify(sig: &sr25519::Signature, msg: &[u8], pubkey: &sr25519::Public) -> bool {
+		sr25519::Pair::verify_deprecated(sig, msg, pubkey)
+	}
+
 	/// Verify an `sr25519` signature.
 	///
 	/// Returns `true` when the verification in successful.
+	#[version(2)]
 	fn sr25519_verify(sig: &sr25519::Signature, msg: &[u8], pubkey: &sr25519::Public) -> bool {
 		sr25519::Pair::verify(sig, msg, pubkey)
 	}