Keystore overhaul (paritytech#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

bin/node-template/node/src/service.rs

@@ -150,7 +150,7 @@ pub fn new_partial(
 fn remote_keystore(_url: &String) -> Result<Arc<LocalKeystore>, &'static str> {
 	// FIXME: here would the concrete keystore be built,
 	//        must return a concrete type (NOT `LocalKeystore`) that
-	//        implements `CryptoStore` and `SyncCryptoStore`
+	//        implements `Keystore`
 	Err("Remote Keystore not supported.")
 }
 
@@ -233,7 +233,7 @@ pub fn new_full(mut config: Configuration) -> Result<TaskManager, ServiceError>
 	let _rpc_handlers = sc_service::spawn_tasks(sc_service::SpawnTasksParams {
 		network: network.clone(),
 		client: client.clone(),
-		keystore: keystore_container.sync_keystore(),
+		keystore: keystore_container.keystore(),
 		task_manager: &mut task_manager,
 		transaction_pool: transaction_pool.clone(),
 		rpc_builder: rpc_extensions_builder,
@@ -276,7 +276,7 @@ pub fn new_full(mut config: Configuration) -> Result<TaskManager, ServiceError>
 				},
 				force_authoring,
 				backoff_authoring_blocks,
-				keystore: keystore_container.sync_keystore(),
+				keystore: keystore_container.keystore(),
 				sync_oracle: sync_service.clone(),
 				justification_sync_link: sync_service.clone(),
 				block_proposal_slot_portion: SlotProportion::new(2f32 / 3f32),
@@ -296,8 +296,7 @@ pub fn new_full(mut config: Configuration) -> Result<TaskManager, ServiceError>
 	if enable_grandpa {
 		// if the node isn't actively participating in consensus then it doesn't
 		// need a keystore, regardless of which protocol we use below.
-		let keystore =
-			if role.is_authority() { Some(keystore_container.sync_keystore()) } else { None };
+		let keystore = if role.is_authority() { Some(keystore_container.keystore()) } else { None };
 
 		let grandpa_config = sc_consensus_grandpa::Config {
 			// FIXME #1578 make this available through chainspec

bin/node/cli/src/service.rs

@@ -251,7 +251,7 @@ pub fn new_partial(
 		let client = client.clone();
 		let pool = transaction_pool.clone();
 		let select_chain = select_chain.clone();
-		let keystore = keystore_container.sync_keystore();
+		let keystore = keystore_container.keystore();
 		let chain_spec = config.chain_spec.cloned_box();
 
 		let rpc_backend = backend.clone();
@@ -386,7 +386,7 @@ pub fn new_full_base(
 		config,
 		backend,
 		client: client.clone(),
-		keystore: keystore_container.sync_keystore(),
+		keystore: keystore_container.keystore(),
 		network: network.clone(),
 		rpc_builder: Box::new(rpc_builder),
 		transaction_pool: transaction_pool.clone(),
@@ -431,7 +431,7 @@ pub fn new_full_base(
 		let client_clone = client.clone();
 		let slot_duration = babe_link.config().slot_duration();
 		let babe_config = sc_consensus_babe::BabeParams {
-			keystore: keystore_container.sync_keystore(),
+			keystore: keystore_container.keystore(),
 			client: client.clone(),
 			select_chain,
 			env: proposer,
@@ -507,8 +507,7 @@ pub fn new_full_base(
 
 	// if the node isn't actively participating in consensus then it doesn't
 	// need a keystore, regardless of which protocol we use below.
-	let keystore =
-		if role.is_authority() { Some(keystore_container.sync_keystore()) } else { None };
+	let keystore = if role.is_authority() { Some(keystore_container.keystore()) } else { None };
 
 	let config = grandpa::Config {
 		// FIXME #1578 make this available through chainspec
@@ -596,7 +595,7 @@ mod tests {
 	use sp_core::{crypto::Pair as CryptoPair, Public};
 	use sp_inherents::InherentDataProvider;
 	use sp_keyring::AccountKeyring;
-	use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
+	use sp_keystore::{Keystore, KeystorePtr};
 	use sp_runtime::{
 		generic::{Digest, Era, SignedPayload},
 		key_types::BABE,
@@ -616,10 +615,10 @@ mod tests {
 		sp_tracing::try_init_simple();
 
 		let keystore_path = tempfile::tempdir().expect("Creates keystore path");
-		let keystore: SyncCryptoStorePtr =
+		let keystore: KeystorePtr =
 			Arc::new(LocalKeystore::open(keystore_path.path(), None).expect("Creates keystore"));
 		let alice: sp_consensus_babe::AuthorityId =
-			SyncCryptoStore::sr25519_generate_new(&*keystore, BABE, Some("//Alice"))
+			Keystore::sr25519_generate_new(&*keystore, BABE, Some("//Alice"))
 				.expect("Creates authority pair")
 				.into();
 
@@ -736,7 +735,7 @@ mod tests {
 				// sign the pre-sealed hash of the block and then
 				// add it to a digest item.
 				let to_sign = pre_hash.encode();
-				let signature = SyncCryptoStore::sign_with(
+				let signature = Keystore::sign_with(
 					&*keystore,
 					sp_consensus_babe::AuthorityId::ID,
 					&alice.to_public_crypto_pair(),

bin/node/executor/tests/submit_transaction.rs

@@ -21,7 +21,7 @@ use kitchensink_runtime::{Executive, Indices, Runtime, UncheckedExtrinsic};
 use sp_application_crypto::AppKey;
 use sp_core::offchain::{testing::TestTransactionPoolExt, TransactionPoolExt};
 use sp_keyring::sr25519::Keyring::Alice;
-use sp_keystore::{testing::KeyStore, KeystoreExt, SyncCryptoStore};
+use sp_keystore::{testing::MemoryKeystore, Keystore, KeystoreExt};
 use std::sync::Arc;
 
 pub mod common;
@@ -62,20 +62,20 @@ fn should_submit_signed_transaction() {
 	let (pool, state) = TestTransactionPoolExt::new();
 	t.register_extension(TransactionPoolExt::new(pool));
 
-	let keystore = KeyStore::new();
-	SyncCryptoStore::sr25519_generate_new(
+	let keystore = MemoryKeystore::new();
+	Keystore::sr25519_generate_new(
 		&keystore,
 		sr25519::AuthorityId::ID,
 		Some(&format!("{}/hunter1", PHRASE)),
 	)
 	.unwrap();
-	SyncCryptoStore::sr25519_generate_new(
+	Keystore::sr25519_generate_new(
 		&keystore,
 		sr25519::AuthorityId::ID,
 		Some(&format!("{}/hunter2", PHRASE)),
 	)
 	.unwrap();
-	SyncCryptoStore::sr25519_generate_new(
+	Keystore::sr25519_generate_new(
 		&keystore,
 		sr25519::AuthorityId::ID,
 		Some(&format!("{}/hunter3", PHRASE)),
@@ -105,14 +105,14 @@ fn should_submit_signed_twice_from_the_same_account() {
 	let (pool, state) = TestTransactionPoolExt::new();
 	t.register_extension(TransactionPoolExt::new(pool));
 
-	let keystore = KeyStore::new();
-	SyncCryptoStore::sr25519_generate_new(
+	let keystore = MemoryKeystore::new();
+	Keystore::sr25519_generate_new(
 		&keystore,
 		sr25519::AuthorityId::ID,
 		Some(&format!("{}/hunter1", PHRASE)),
 	)
 	.unwrap();
-	SyncCryptoStore::sr25519_generate_new(
+	Keystore::sr25519_generate_new(
 		&keystore,
 		sr25519::AuthorityId::ID,
 		Some(&format!("{}/hunter2", PHRASE)),
@@ -162,7 +162,7 @@ fn should_submit_signed_twice_from_all_accounts() {
 	let (pool, state) = TestTransactionPoolExt::new();
 	t.register_extension(TransactionPoolExt::new(pool));
 
-	let keystore = KeyStore::new();
+	let keystore = MemoryKeystore::new();
 	keystore
 		.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter1", PHRASE)))
 		.unwrap();
@@ -226,8 +226,8 @@ fn submitted_transaction_should_be_valid() {
 	let (pool, state) = TestTransactionPoolExt::new();
 	t.register_extension(TransactionPoolExt::new(pool));
 
-	let keystore = KeyStore::new();
-	SyncCryptoStore::sr25519_generate_new(
+	let keystore = MemoryKeystore::new();
+	Keystore::sr25519_generate_new(
 		&keystore,
 		sr25519::AuthorityId::ID,
 		Some(&format!("{}/hunter1", PHRASE)),

bin/node/rpc/src/lib.rs

@@ -49,7 +49,7 @@ use sp_block_builder::BlockBuilder;
 use sp_blockchain::{Error as BlockChainError, HeaderBackend, HeaderMetadata};
 use sp_consensus::SelectChain;
 use sp_consensus_babe::BabeApi;
-use sp_keystore::SyncCryptoStorePtr;
+use sp_keystore::KeystorePtr;
 
 /// Extra dependencies for BABE.
 pub struct BabeDeps {
@@ -58,7 +58,7 @@ pub struct BabeDeps {
 	/// BABE pending epoch changes.
 	pub shared_epoch_changes: SharedEpochChanges<Block, Epoch>,
 	/// The keystore that manages the keys of the node.
-	pub keystore: SyncCryptoStorePtr,
+	pub keystore: KeystorePtr,
 }
 
 /// Extra dependencies for GRANDPA

bin/utils/chain-spec-builder/src/main.rs

@@ -32,7 +32,7 @@ use sp_core::{
 	crypto::{ByteArray, Ss58Codec},
 	sr25519,
 };
-use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
+use sp_keystore::{Keystore, KeystorePtr};
 
 /// A utility to easily create a testnet chain spec definition with a given set
 /// of authorities and endowed accounts and/or generate random accounts.
@@ -164,7 +164,7 @@ fn generate_chain_spec(
 
 fn generate_authority_keys_and_store(seeds: &[String], keystore_path: &Path) -> Result<(), String> {
 	for (n, seed) in seeds.iter().enumerate() {
-		let keystore: SyncCryptoStorePtr = Arc::new(
+		let keystore: KeystorePtr = Arc::new(
 			LocalKeystore::open(keystore_path.join(format!("auth-{}", n)), None)
 				.map_err(|err| err.to_string())?,
 		);
@@ -173,7 +173,7 @@ fn generate_authority_keys_and_store(seeds: &[String], keystore_path: &Path) ->
 			chain_spec::authority_keys_from_seed(seed);
 
 		let insert_key = |key_type, public| {
-			SyncCryptoStore::insert_unknown(&*keystore, key_type, &format!("//{}", seed), public)
+			Keystore::insert(&*keystore, key_type, &format!("//{}", seed), public)
 				.map_err(|_| format!("Failed to insert key: {}", grandpa))
 		};
 

client/api/src/execution_extensions.rs

@@ -30,7 +30,7 @@ use sp_core::{
 	ExecutionContext,
 };
 use sp_externalities::{Extension, Extensions};
-use sp_keystore::{KeystoreExt, SyncCryptoStorePtr};
+use sp_keystore::{KeystoreExt, KeystorePtr};
 use sp_runtime::{
 	generic::BlockId,
 	traits::{Block as BlockT, NumberFor},
@@ -163,7 +163,7 @@ impl<T: offchain::DbExternalities + Clone + Sync + Send + 'static> DbExternaliti
 /// for each call, based on required `Capabilities`.
 pub struct ExecutionExtensions<Block: BlockT> {
 	strategies: ExecutionStrategies,
-	keystore: Option<SyncCryptoStorePtr>,
+	keystore: Option<KeystorePtr>,
 	offchain_db: Option<Box<dyn DbExternalitiesFactory>>,
 	// FIXME: these two are only RwLock because of https://github.com/paritytech/substrate/issues/4587
 	//        remove when fixed.
@@ -191,7 +191,7 @@ impl<Block: BlockT> ExecutionExtensions<Block> {
 	/// Create new `ExecutionExtensions` given a `keystore` and `ExecutionStrategies`.
 	pub fn new(
 		strategies: ExecutionStrategies,
-		keystore: Option<SyncCryptoStorePtr>,
+		keystore: Option<KeystorePtr>,
 		offchain_db: Option<Box<dyn DbExternalitiesFactory>>,
 	) -> Self {
 		let transaction_pool = RwLock::new(None);

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()
 	});

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,
 }
@@ -371,8 +371,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);
@@ -389,7 +388,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(),
@@ -595,12 +593,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<_>>();
 
@@ -670,33 +667,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)