Keystore overhaul (iter 2) (#13634)

* Remove bloat about remote keystore

* Update docs and remove unused 'KeystoreRef' trait

* Use wherever possible, MemoryKeystore for testing

* Remove unrequired fully qualified method syntax for Keystore

substrate/client/consensus/babe/rpc/Cargo.toml

@@ -31,7 +31,6 @@ sp-runtime = { version = "7.0.0", path = "../../../../primitives/runtime" }
 
 [dev-dependencies]
 serde_json = "1.0.85"
-tempfile = "3.1.0"
 tokio = "1.22.0"
 sc-consensus = { version = "0.10.0-dev", path = "../../../consensus/common" }
 sc-keystore = { version = "4.0.0-dev", path = "../../../keystore" }

substrate/client/consensus/babe/rpc/src/lib.rs

@@ -37,7 +37,7 @@ use sp_consensus_babe::{
 	digests::PreDigest, AuthorityId, BabeApi as BabeRuntimeApi, BabeConfiguration,
 };
 use sp_core::crypto::ByteArray;
-use sp_keystore::{Keystore, KeystorePtr};
+use sp_keystore::KeystorePtr;
 use sp_runtime::traits::{Block as BlockT, Header as _};
 use std::{collections::HashMap, sync::Arc};
 
@@ -117,7 +117,7 @@ where
 				.iter()
 				.enumerate()
 				.filter_map(|(i, a)| {
-					if Keystore::has_keys(&*self.keystore, &[(a.0.to_raw_vec(), AuthorityId::ID)]) {
+					if self.keystore.has_keys(&[(a.0.to_raw_vec(), AuthorityId::ID)]) {
 						Some((a.0.clone(), i))
 					} else {
 						None
@@ -210,30 +210,21 @@ where
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use sc_keystore::LocalKeystore;
-	use sp_application_crypto::AppPair;
+	use sc_consensus_babe::block_import;
 	use sp_core::crypto::key_types::BABE;
 	use sp_keyring::Sr25519Keyring;
-	use sp_keystore::{Keystore, KeystorePtr};
+	use sp_keystore::{testing::MemoryKeystore, Keystore};
 	use substrate_test_runtime_client::{
 		runtime::Block, Backend, DefaultTestClientBuilderExt, TestClient, TestClientBuilder,
 		TestClientBuilderExt,
 	};
 
-	use sc_consensus_babe::{block_import, AuthorityPair};
-	use std::sync::Arc;
-
-	/// creates keystore backed by a temp file
-	fn create_temp_keystore<P: AppPair>(
-		authority: Sr25519Keyring,
-	) -> (KeystorePtr, tempfile::TempDir) {
-		let keystore_path = tempfile::tempdir().expect("Creates keystore path");
-		let keystore =
-			Arc::new(LocalKeystore::open(keystore_path.path(), None).expect("Creates keystore"));
-		Keystore::sr25519_generate_new(&*keystore, BABE, Some(&authority.to_seed()))
+	fn create_keystore(authority: Sr25519Keyring) -> KeystorePtr {
+		let keystore = MemoryKeystore::new();
+		keystore
+			.sr25519_generate_new(BABE, Some(&authority.to_seed()))
 			.expect("Creates authority key");
-
-		(keystore, keystore_path)
+		keystore.into()
 	}
 
 	fn test_babe_rpc_module(
@@ -247,7 +238,7 @@ mod tests {
 			.expect("can initialize block-import");
 
 		let epoch_changes = link.epoch_changes().clone();
-		let keystore = create_temp_keystore::<AuthorityPair>(Sr25519Keyring::Alice).0;
+		let keystore = create_keystore(Sr25519Keyring::Alice);
 
 		Babe::new(client.clone(), epoch_changes, keystore, config, longest_chain, deny_unsafe)
 	}

substrate/client/consensus/babe/src/authorship.rs

@@ -29,7 +29,7 @@ use sp_consensus_babe::{
 };
 use sp_consensus_vrf::schnorrkel::{VRFOutput, VRFProof};
 use sp_core::{blake2_256, crypto::ByteArray, U256};
-use sp_keystore::{Keystore, KeystorePtr};
+use sp_keystore::KeystorePtr;
 
 /// Calculates the primary selection threshold for a given authority, taking
 /// into account `c` (`1 - c` represents the probability of a slot being empty).
@@ -153,8 +153,7 @@ fn claim_secondary_slot(
 		if authority_id == expected_author {
 			let pre_digest = if author_secondary_vrf {
 				let transcript_data = make_transcript_data(randomness, slot, epoch_index);
-				let result = Keystore::sr25519_vrf_sign(
-					&**keystore,
+				let result = keystore.sr25519_vrf_sign(
 					AuthorityId::ID,
 					authority_id.as_ref(),
 					transcript_data,
@@ -169,10 +168,7 @@ fn claim_secondary_slot(
 				} else {
 					None
 				}
-			} else if Keystore::has_keys(
-				&**keystore,
-				&[(authority_id.to_raw_vec(), AuthorityId::ID)],
-			) {
+			} else if keystore.has_keys(&[(authority_id.to_raw_vec(), AuthorityId::ID)]) {
 				Some(PreDigest::SecondaryPlain(SecondaryPlainPreDigest {
 					slot,
 					authority_index: *authority_index as u32,
@@ -254,12 +250,8 @@ fn claim_primary_slot(
 	for (authority_id, authority_index) in keys {
 		let transcript = make_transcript(randomness, slot, epoch_index);
 		let transcript_data = make_transcript_data(randomness, slot, epoch_index);
-		let result = Keystore::sr25519_vrf_sign(
-			&**keystore,
-			AuthorityId::ID,
-			authority_id.as_ref(),
-			transcript_data,
-		);
+		let result =
+			keystore.sr25519_vrf_sign(AuthorityId::ID, authority_id.as_ref(), transcript_data);
 		if let Ok(Some(signature)) = result {
 			let public = PublicKey::from_bytes(&authority_id.to_raw_vec()).ok()?;
 			let inout = match signature.output.attach_input_hash(&public, transcript) {
@@ -287,20 +279,16 @@ fn claim_primary_slot(
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use sc_keystore::LocalKeystore;
 	use sp_consensus_babe::{AllowedSlots, AuthorityId, BabeEpochConfiguration};
 	use sp_core::{crypto::Pair as _, sr25519::Pair};
-	use std::sync::Arc;
+	use sp_keystore::testing::MemoryKeystore;
 
 	#[test]
 	fn claim_secondary_plain_slot_works() {
-		let keystore: KeystorePtr = Arc::new(LocalKeystore::in_memory());
-		let valid_public_key = Keystore::sr25519_generate_new(
-			&*keystore,
-			AuthorityId::ID,
-			Some(sp_core::crypto::DEV_PHRASE),
-		)
-		.unwrap();
+		let keystore: KeystorePtr = MemoryKeystore::new().into();
+		let valid_public_key = keystore
+			.sr25519_generate_new(AuthorityId::ID, Some(sp_core::crypto::DEV_PHRASE))
+			.unwrap();
 
 		let authorities = vec![
 			(AuthorityId::from(Pair::generate().0.public()), 5),

substrate/client/consensus/babe/src/lib.rs

@@ -119,7 +119,7 @@ use sp_consensus_babe::inherents::BabeInherentData;
 use sp_consensus_slots::Slot;
 use sp_core::{crypto::ByteArray, ExecutionContext};
 use sp_inherents::{CreateInherentDataProviders, InherentData, InherentDataProvider};
-use sp_keystore::{Keystore, KeystorePtr};
+use sp_keystore::KeystorePtr;
 use sp_runtime::{
 	generic::OpaqueDigestItemId,
 	traits::{Block as BlockT, Header, NumberFor, SaturatedConversion, Zero},
@@ -834,19 +834,16 @@ where
 		// add it to a digest item.
 		let public_type_pair = public.clone().into();
 		let public = public.to_raw_vec();
-		let signature = Keystore::sign_with(
-			&*self.keystore,
-			<AuthorityId as AppKey>::ID,
-			&public_type_pair,
-			header_hash.as_ref(),
-		)
-		.map_err(|e| sp_consensus::Error::CannotSign(public.clone(), e.to_string()))?
-		.ok_or_else(|| {
-			sp_consensus::Error::CannotSign(
-				public.clone(),
-				"Could not find key in keystore.".into(),
-			)
-		})?;
+		let signature = self
+			.keystore
+			.sign_with(<AuthorityId as AppKey>::ID, &public_type_pair, header_hash.as_ref())
+			.map_err(|e| sp_consensus::Error::CannotSign(public.clone(), e.to_string()))?
+			.ok_or_else(|| {
+				sp_consensus::Error::CannotSign(
+					public.clone(),
+					"Could not find key in keystore.".into(),
+				)
+			})?;
 		let signature: AuthoritySignature = signature
 			.clone()
 			.try_into()

substrate/client/consensus/babe/src/tests.rs

@@ -369,10 +369,11 @@ async fn rejects_empty_block() {
 }
 
 fn create_keystore(authority: Sr25519Keyring) -> KeystorePtr {
-	let keystore = Arc::new(MemoryKeystore::new());
-	Keystore::sr25519_generate_new(&*keystore, BABE, Some(&authority.to_seed()))
-		.expect("Generates authority key");
+	let keystore = MemoryKeystore::new();
 	keystore
+		.sr25519_generate_new(BABE, Some(&authority.to_seed()))
+		.expect("Generates authority key");
+	keystore.into()
 }
 
 async fn run_one_test(mutator: impl Fn(&mut TestHeader, Stage) + Send + Sync + 'static) {
@@ -637,7 +638,8 @@ fn claim_vrf_check() {
 		v => panic!("Unexpected pre-digest variant {:?}", v),
 	};
 	let transcript = make_transcript_data(&epoch.randomness.clone(), 0.into(), epoch.epoch_index);
-	let sign = Keystore::sr25519_vrf_sign(&*keystore, AuthorityId::ID, &public, transcript)
+	let sign = keystore
+		.sr25519_vrf_sign(AuthorityId::ID, &public, transcript)
 		.unwrap()
 		.unwrap();
 	assert_eq!(pre_digest.vrf_output, VRFOutput(sign.output));
@@ -648,7 +650,8 @@ fn claim_vrf_check() {
 		v => panic!("Unexpected pre-digest variant {:?}", v),
 	};
 	let transcript = make_transcript_data(&epoch.randomness.clone(), 1.into(), epoch.epoch_index);
-	let sign = Keystore::sr25519_vrf_sign(&*keystore, AuthorityId::ID, &public, transcript)
+	let sign = keystore
+		.sr25519_vrf_sign(AuthorityId::ID, &public, transcript)
 		.unwrap()
 		.unwrap();
 	assert_eq!(pre_digest.vrf_output, VRFOutput(sign.output));
@@ -661,7 +664,8 @@ fn claim_vrf_check() {
 	};
 	let fixed_epoch = epoch.clone_for_slot(slot);
 	let transcript = make_transcript_data(&epoch.randomness.clone(), slot, fixed_epoch.epoch_index);
-	let sign = Keystore::sr25519_vrf_sign(&*keystore, AuthorityId::ID, &public, transcript)
+	let sign = keystore
+		.sr25519_vrf_sign(AuthorityId::ID, &public, transcript)
 		.unwrap()
 		.unwrap();
 	assert_eq!(fixed_epoch.epoch_index, 11);
@@ -675,7 +679,8 @@ fn claim_vrf_check() {
 	};
 	let fixed_epoch = epoch.clone_for_slot(slot);
 	let transcript = make_transcript_data(&epoch.randomness.clone(), slot, fixed_epoch.epoch_index);
-	let sign = Keystore::sr25519_vrf_sign(&*keystore, AuthorityId::ID, &public, transcript)
+	let sign = keystore
+		.sr25519_vrf_sign(AuthorityId::ID, &public, transcript)
 		.unwrap()
 		.unwrap();
 	assert_eq!(fixed_epoch.epoch_index, 11);