Keystore overhaul (#13615)

* Remove 'supported_keys' 'sign_with_any' and 'sign_with_all' from keystore trait

* Remove the aync keystore

* Renaming:
- SyncCryptoStore -> Keystore
- SyncCryptoStorePtr -> KeystorePtr
- KeyStore -> MemoryKeystore

* Fix authority discovery worker and tests

* Rename 'insert_unknown' to 'insert'

* Remove leftover

substrate/client/authority-discovery/src/tests.rs

@@ -33,7 +33,7 @@ use std::{collections::HashSet, sync::Arc};
 
 use sp_authority_discovery::AuthorityId;
 use sp_core::crypto::key_types;
-use sp_keystore::{testing::KeyStore, CryptoStore};
+use sp_keystore::{testing::MemoryKeystore, Keystore};
 
 #[test]
 fn get_addresses_and_authority_id() {
@@ -42,12 +42,11 @@ fn get_addresses_and_authority_id() {
 
 	let mut pool = LocalPool::new();
 
-	let key_store = KeyStore::new();
+	let key_store = MemoryKeystore::new();
 
 	let remote_authority_id: AuthorityId = pool.run_until(async {
 		key_store
 			.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None)
-			.await
 			.unwrap()
 			.into()
 	});

substrate/client/authority-discovery/src/worker.rs

@@ -53,7 +53,7 @@ use sp_authority_discovery::{
 use sp_blockchain::HeaderBackend;
 
 use sp_core::crypto::{key_types, CryptoTypePublicPair, Pair};
-use sp_keystore::CryptoStore;
+use sp_keystore::{Keystore, KeystorePtr};
 use sp_runtime::traits::Block as BlockT;
 
 mod addr_cache;
@@ -78,7 +78,7 @@ const MAX_IN_FLIGHT_LOOKUPS: usize = 8;
 /// Role an authority discovery [`Worker`] can run as.
 pub enum Role {
 	/// Publish own addresses and discover addresses of others.
-	PublishAndDiscover(Arc<dyn CryptoStore>),
+	PublishAndDiscover(KeystorePtr),
 	/// Discover addresses of others.
 	Discover,
 }
@@ -364,8 +364,7 @@ where
 			Some(peer_signature),
 			key_store.as_ref(),
 			keys_vec,
-		)
-		.await?;
+		)?;
 
 		for (key, value) in kv_pairs.into_iter() {
 			self.network.put_value(key, value);
@@ -382,7 +381,6 @@ where
 		let local_keys = match &self.role {
 			Role::PublishAndDiscover(key_store) => key_store
 				.sr25519_public_keys(key_types::AUTHORITY_DISCOVERY)
-				.await
 				.into_iter()
 				.collect::<HashSet<_>>(),
 			Role::Discover => HashSet::new(),
@@ -588,12 +586,11 @@ where
 	// next authority set with two keys. The function does not return all of the local authority
 	// discovery public keys, but only the ones intersecting with the current or next authority set.
 	async fn get_own_public_keys_within_authority_set(
-		key_store: Arc<dyn CryptoStore>,
+		key_store: KeystorePtr,
 		client: &Client,
 	) -> Result<HashSet<AuthorityId>> {
 		let local_pub_keys = key_store
 			.sr25519_public_keys(key_types::AUTHORITY_DISCOVERY)
-			.await
 			.into_iter()
 			.collect::<HashSet<_>>();
 
@@ -663,33 +660,28 @@ fn sign_record_with_peer_id(
 	Ok(schema::PeerSignature { signature, public_key })
 }
 
-async fn sign_record_with_authority_ids(
+fn sign_record_with_authority_ids(
 	serialized_record: Vec<u8>,
 	peer_signature: Option<schema::PeerSignature>,
-	key_store: &dyn CryptoStore,
+	key_store: &dyn Keystore,
 	keys: Vec<CryptoTypePublicPair>,
 ) -> Result<Vec<(KademliaKey, Vec<u8>)>> {
-	let signatures = key_store
-		.sign_with_all(key_types::AUTHORITY_DISCOVERY, keys.clone(), &serialized_record)
-		.await
-		.map_err(|_| Error::Signing)?;
+	let mut result = Vec::with_capacity(keys.len());
 
-	let mut result = vec![];
-	for (sign_result, key) in signatures.into_iter().zip(keys.iter()) {
-		let mut signed_record = vec![];
+	for key in keys.iter() {
+		let auth_signature = key_store
+			.sign_with(key_types::AUTHORITY_DISCOVERY, key, &serialized_record)
+			.map_err(|_| Error::Signing)?
+			.ok_or_else(|| Error::MissingSignature(key.clone()))?;
 
-		// Verify that all signatures exist for all provided keys.
-		let auth_signature =
-			sign_result.ok().flatten().ok_or_else(|| Error::MissingSignature(key.clone()))?;
-		schema::SignedAuthorityRecord {
+		let signed_record = schema::SignedAuthorityRecord {
 			record: serialized_record.clone(),
 			auth_signature,
 			peer_signature: peer_signature.clone(),
 		}
-		.encode(&mut signed_record)
-		.map_err(Error::EncodingProto)?;
+		.encode_to_vec();
 
-		result.push((hash_authority_id(key.1.as_ref()), signed_record));
+		result.push((hash_authority_id(&key.1), signed_record));
 	}
 
 	Ok(result)

substrate/client/authority-discovery/src/worker/tests.rs

@@ -40,7 +40,7 @@ use prometheus_endpoint::prometheus::default_registry;
 use sc_client_api::HeaderBackend;
 use sc_network::Signature;
 use sp_api::{ApiRef, ProvideRuntimeApi};
-use sp_keystore::{testing::KeyStore, CryptoStore};
+use sp_keystore::{testing::MemoryKeystore, Keystore};
 use sp_runtime::traits::{Block as BlockT, NumberFor, Zero};
 use substrate_test_runtime_client::runtime::Block;
 
@@ -208,10 +208,10 @@ impl<'a> NetworkSigner for TestSigner<'a> {
 	}
 }
 
-async fn build_dht_event<Signer: NetworkSigner>(
+fn build_dht_event<Signer: NetworkSigner>(
 	addresses: Vec<Multiaddr>,
 	public_key: AuthorityId,
-	key_store: &dyn CryptoStore,
+	key_store: &MemoryKeystore,
 	network: Option<&Signer>,
 ) -> Vec<(KademliaKey, Vec<u8>)> {
 	let serialized_record =
@@ -224,7 +224,6 @@ async fn build_dht_event<Signer: NetworkSigner>(
 		key_store,
 		vec![public_key.into()],
 	)
-	.await
 	.unwrap();
 	// There is always a single item in it, because we signed it with a single key
 	kv_pairs
@@ -234,7 +233,7 @@ async fn build_dht_event<Signer: NetworkSigner>(
 fn new_registers_metrics() {
 	let (_dht_event_tx, dht_event_rx) = mpsc::channel(1000);
 	let network: Arc<TestNetwork> = Arc::new(Default::default());
-	let key_store = KeyStore::new();
+	let key_store = MemoryKeystore::new();
 	let test_api = Arc::new(TestApi { authorities: vec![] });
 
 	let registry = prometheus_endpoint::Registry::new();
@@ -266,7 +265,7 @@ fn triggers_dht_get_query() {
 	let test_api = Arc::new(TestApi { authorities: authorities.clone() });
 
 	let network = Arc::new(TestNetwork::default());
-	let key_store = KeyStore::new();
+	let key_store = MemoryKeystore::new();
 
 	let (_to_worker, from_service) = mpsc::channel(0);
 	let mut worker = Worker::new(
@@ -298,14 +297,12 @@ fn publish_discover_cycle() {
 
 	let network: Arc<TestNetwork> = Arc::new(Default::default());
 
-	let key_store = KeyStore::new();
+	let key_store = MemoryKeystore::new();
 
 	let _ = pool.spawner().spawn_local_obj(
 		async move {
-			let node_a_public = key_store
-				.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None)
-				.await
-				.unwrap();
+			let node_a_public =
+				key_store.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None).unwrap();
 			let test_api = Arc::new(TestApi { authorities: vec![node_a_public.into()] });
 
 			let (_to_worker, from_service) = mpsc::channel(0);
@@ -337,7 +334,7 @@ fn publish_discover_cycle() {
 				authorities: vec![node_a_public.into()],
 			});
 			let network: Arc<TestNetwork> = Arc::new(Default::default());
-			let key_store = KeyStore::new();
+			let key_store = MemoryKeystore::new();
 
 			let (_to_worker, from_service) = mpsc::channel(0);
 			let mut worker = Worker::new(
@@ -371,7 +368,7 @@ fn publish_discover_cycle() {
 fn terminate_when_event_stream_terminates() {
 	let (dht_event_tx, dht_event_rx) = channel(1000);
 	let network: Arc<TestNetwork> = Arc::new(Default::default());
-	let key_store = KeyStore::new();
+	let key_store = MemoryKeystore::new();
 	let test_api = Arc::new(TestApi { authorities: vec![] });
 
 	let (to_worker, from_service) = mpsc::channel(0);
@@ -420,11 +417,11 @@ fn dont_stop_polling_dht_event_stream_after_bogus_event() {
 
 		address.with(multiaddr::Protocol::P2p(peer_id.into()))
 	};
-	let remote_key_store = KeyStore::new();
-	let remote_public_key: AuthorityId =
-		block_on(remote_key_store.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None))
-			.unwrap()
-			.into();
+	let remote_key_store = MemoryKeystore::new();
+	let remote_public_key: AuthorityId = remote_key_store
+		.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None)
+		.unwrap()
+		.into();
 
 	let (mut dht_event_tx, dht_event_rx) = channel(1);
 	let (network, mut network_events) = {
@@ -433,7 +430,7 @@ fn dont_stop_polling_dht_event_stream_after_bogus_event() {
 		(Arc::new(n), r)
 	};
 
-	let key_store = KeyStore::new();
+	let key_store = MemoryKeystore::new();
 	let test_api = Arc::new(TestApi { authorities: vec![remote_public_key.clone()] });
 	let mut pool = LocalPool::new();
 
@@ -480,8 +477,7 @@ fn dont_stop_polling_dht_event_stream_after_bogus_event() {
 				remote_public_key.clone(),
 				&remote_key_store,
 				None,
-			)
-			.await;
+			);
 			DhtEvent::ValueFound(kv_pairs)
 		};
 		dht_event_tx.send(dht_event).await.expect("Channel has capacity of 1.");
@@ -498,7 +494,7 @@ fn dont_stop_polling_dht_event_stream_after_bogus_event() {
 }
 
 struct DhtValueFoundTester {
-	pub remote_key_store: KeyStore,
+	pub remote_key_store: MemoryKeystore,
 	pub remote_authority_public: sp_core::sr25519::Public,
 	pub remote_node_key: Keypair,
 	pub local_worker: Option<
@@ -516,10 +512,10 @@ struct DhtValueFoundTester {
 
 impl DhtValueFoundTester {
 	fn new() -> Self {
-		let remote_key_store = KeyStore::new();
-		let remote_authority_public =
-			block_on(remote_key_store.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None))
-				.unwrap();
+		let remote_key_store = MemoryKeystore::new();
+		let remote_authority_public = remote_key_store
+			.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None)
+			.unwrap();
 
 		let remote_node_key = Keypair::generate_ed25519();
 		Self { remote_key_store, remote_authority_public, remote_node_key, local_worker: None }
@@ -542,7 +538,7 @@ impl DhtValueFoundTester {
 		let local_test_api =
 			Arc::new(TestApi { authorities: vec![self.remote_authority_public.into()] });
 		let local_network: Arc<TestNetwork> = Arc::new(Default::default());
-		let local_key_store = KeyStore::new();
+		let local_key_store = MemoryKeystore::new();
 
 		let (_to_worker, from_service) = mpsc::channel(0);
 		let mut local_worker = Worker::new(
@@ -576,12 +572,12 @@ fn limit_number_of_addresses_added_to_cache_per_authority() {
 	let mut tester = DhtValueFoundTester::new();
 	assert!(MAX_ADDRESSES_PER_AUTHORITY < 100);
 	let addresses = (1..100).map(|i| tester.multiaddr_with_peer_id(i)).collect();
-	let kv_pairs = block_on(build_dht_event::<TestNetwork>(
+	let kv_pairs = build_dht_event::<TestNetwork>(
 		addresses,
 		tester.remote_authority_public.into(),
 		&tester.remote_key_store,
 		None,
-	));
+	);
 
 	let cached_remote_addresses = tester.process_value_found(false, kv_pairs);
 	assert_eq!(MAX_ADDRESSES_PER_AUTHORITY, cached_remote_addresses.unwrap().len());
@@ -591,12 +587,12 @@ fn limit_number_of_addresses_added_to_cache_per_authority() {
 fn strict_accept_address_with_peer_signature() {
 	let mut tester = DhtValueFoundTester::new();
 	let addr = tester.multiaddr_with_peer_id(1);
-	let kv_pairs = block_on(build_dht_event(
+	let kv_pairs = build_dht_event(
 		vec![addr.clone()],
 		tester.remote_authority_public.into(),
 		&tester.remote_key_store,
 		Some(&TestSigner { keypair: &tester.remote_node_key }),
-	));
+	);
 
 	let cached_remote_addresses = tester.process_value_found(true, kv_pairs);
 
@@ -611,12 +607,12 @@ fn strict_accept_address_with_peer_signature() {
 fn reject_address_with_rogue_peer_signature() {
 	let mut tester = DhtValueFoundTester::new();
 	let rogue_remote_node_key = Keypair::generate_ed25519();
-	let kv_pairs = block_on(build_dht_event(
+	let kv_pairs = build_dht_event(
 		vec![tester.multiaddr_with_peer_id(1)],
 		tester.remote_authority_public.into(),
 		&tester.remote_key_store,
 		Some(&TestSigner { keypair: &rogue_remote_node_key }),
-	));
+	);
 
 	let cached_remote_addresses = tester.process_value_found(false, kv_pairs);
 
@@ -629,12 +625,12 @@ fn reject_address_with_rogue_peer_signature() {
 #[test]
 fn reject_address_with_invalid_peer_signature() {
 	let mut tester = DhtValueFoundTester::new();
-	let mut kv_pairs = block_on(build_dht_event(
+	let mut kv_pairs = build_dht_event(
 		vec![tester.multiaddr_with_peer_id(1)],
 		tester.remote_authority_public.into(),
 		&tester.remote_key_store,
 		Some(&TestSigner { keypair: &tester.remote_node_key }),
-	));
+	);
 	// tamper with the signature
 	let mut record = schema::SignedAuthorityRecord::decode(kv_pairs[0].1.as_slice()).unwrap();
 	record.peer_signature.as_mut().map(|p| p.signature[1] = !p.signature[1]);
@@ -651,12 +647,12 @@ fn reject_address_with_invalid_peer_signature() {
 #[test]
 fn reject_address_without_peer_signature() {
 	let mut tester = DhtValueFoundTester::new();
-	let kv_pairs = block_on(build_dht_event::<TestNetwork>(
+	let kv_pairs = build_dht_event::<TestNetwork>(
 		vec![tester.multiaddr_with_peer_id(1)],
 		tester.remote_authority_public.into(),
 		&tester.remote_key_store,
 		None,
-	));
+	);
 
 	let cached_remote_addresses = tester.process_value_found(true, kv_pairs);
 
@@ -669,12 +665,12 @@ fn do_not_cache_addresses_without_peer_id() {
 	let multiaddr_with_peer_id = tester.multiaddr_with_peer_id(1);
 	let multiaddr_without_peer_id: Multiaddr =
 		"/ip6/2001:db8:0:0:0:0:0:2/tcp/30333".parse().unwrap();
-	let kv_pairs = block_on(build_dht_event::<TestNetwork>(
+	let kv_pairs = build_dht_event::<TestNetwork>(
 		vec![multiaddr_with_peer_id.clone(), multiaddr_without_peer_id],
 		tester.remote_authority_public.into(),
 		&tester.remote_key_store,
 		None,
-	));
+	);
 
 	let cached_remote_addresses = tester.process_value_found(false, kv_pairs);
 
@@ -701,7 +697,7 @@ fn addresses_to_publish_adds_p2p() {
 		Arc::new(TestApi { authorities: vec![] }),
 		network.clone(),
 		Box::pin(dht_event_rx),
-		Role::PublishAndDiscover(Arc::new(KeyStore::new())),
+		Role::PublishAndDiscover(Arc::new(MemoryKeystore::new())),
 		Some(prometheus_endpoint::Registry::new()),
 		Default::default(),
 	);
@@ -735,7 +731,7 @@ fn addresses_to_publish_respects_existing_p2p_protocol() {
 		Arc::new(TestApi { authorities: vec![] }),
 		network.clone(),
 		Box::pin(dht_event_rx),
-		Role::PublishAndDiscover(Arc::new(KeyStore::new())),
+		Role::PublishAndDiscover(Arc::new(MemoryKeystore::new())),
 		Some(prometheus_endpoint::Registry::new()),
 		Default::default(),
 	);
@@ -755,10 +751,11 @@ fn lookup_throttling() {
 
 		address.with(multiaddr::Protocol::P2p(peer_id.into()))
 	};
-	let remote_key_store = KeyStore::new();
+	let remote_key_store = MemoryKeystore::new();
 	let remote_public_keys: Vec<AuthorityId> = (0..20)
 		.map(|_| {
-			block_on(remote_key_store.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None))
+			remote_key_store
+				.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None)
 				.unwrap()
 				.into()
 		})
@@ -818,8 +815,7 @@ fn lookup_throttling() {
 					remote_key,
 					&remote_key_store,
 					None,
-				)
-				.await;
+				);
 				DhtEvent::ValueFound(kv_pairs)
 			};
 			dht_event_tx.send(dht_event).await.expect("Channel has capacity of 1.");