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

Author: davxy

Cargo.lock

@@ -5,7 +5,6 @@ use sc_client_api::BlockBackend;
 use sc_consensus_aura::{ImportQueueParams, SlotProportion, StartAuraParams};
 use sc_consensus_grandpa::SharedVoterState;
 pub use sc_executor::NativeElseWasmExecutor;
-use sc_keystore::LocalKeystore;
 use sc_service::{error::Error as ServiceError, Configuration, TaskManager, WarpSyncParams};
 use sc_telemetry::{Telemetry, TelemetryWorker};
 use sp_consensus_aura::sr25519::AuthorityPair as AuraPair;
@@ -58,10 +57,6 @@ pub fn new_partial(
 	>,
 	ServiceError,
 > {
-	if config.keystore_remote.is_some() {
-		return Err(ServiceError::Other("Remote Keystores are not supported.".into()))
-	}
-
 	let telemetry = config
 		.telemetry_endpoints
 		.clone()
@@ -147,36 +142,19 @@ 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 `Keystore`
-	Err("Remote Keystore not supported.")
-}
-
 /// Builds a new service for a full client.
 pub fn new_full(mut config: Configuration) -> Result<TaskManager, ServiceError> {
 	let sc_service::PartialComponents {
 		client,
 		backend,
 		mut task_manager,
 		import_queue,
-		mut keystore_container,
+		keystore_container,
 		select_chain,
 		transaction_pool,
 		other: (block_import, grandpa_link, mut telemetry),
 	} = new_partial(&config)?;
 
-	if let Some(url) = &config.keystore_remote {
-		match remote_keystore(url) {
-			Ok(k) => keystore_container.set_remote_keystore(k),
-			Err(e) =>
-				return Err(ServiceError::Other(format!(
-					"Error hooking up remote keystore for {}: {}",
-					url, e
-				))),
-		};
-	}
 	let grandpa_protocol_name = sc_consensus_grandpa::protocol_standard_name(
 		&client.block_hash(0).ok().flatten().expect("Genesis block exists; qed"),
 		&config.chain_spec,

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

@@ -69,7 +69,6 @@ fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
 		transaction_pool: Default::default(),
 		network: network_config,
 		keystore: KeystoreConfig::InMemory,
-		keystore_remote: Default::default(),
 		database: DatabaseSource::RocksDb { path: root.join("db"), cache_size: 128 },
 		trie_cache_maximum_size: Some(64 * 1024 * 1024),
 		state_pruning: Some(PruningMode::ArchiveAll),

bin/node/cli/benches/block_production.rs

@@ -64,7 +64,6 @@ fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
 		},
 		network: network_config,
 		keystore: KeystoreConfig::InMemory,
-		keystore_remote: Default::default(),
 		database: DatabaseSource::RocksDb { path: root.join("db"), cache_size: 128 },
 		trie_cache_maximum_size: Some(64 * 1024 * 1024),
 		state_pruning: Some(PruningMode::ArchiveAll),

bin/node/cli/benches/transaction_pool.rs

@@ -595,7 +595,7 @@ mod tests {
 	use sp_core::{crypto::Pair as CryptoPair, Public};
 	use sp_inherents::InherentDataProvider;
 	use sp_keyring::AccountKeyring;
-	use sp_keystore::{Keystore, KeystorePtr};
+	use sp_keystore::KeystorePtr;
 	use sp_runtime::{
 		generic::{Digest, Era, SignedPayload},
 		key_types::BABE,
@@ -615,12 +615,13 @@ mod tests {
 		sp_tracing::try_init_simple();
 
 		let keystore_path = tempfile::tempdir().expect("Creates keystore path");
-		let keystore: KeystorePtr =
-			Arc::new(LocalKeystore::open(keystore_path.path(), None).expect("Creates keystore"));
-		let alice: sp_consensus_babe::AuthorityId =
-			Keystore::sr25519_generate_new(&*keystore, BABE, Some("//Alice"))
-				.expect("Creates authority pair")
-				.into();
+		let keystore: KeystorePtr = LocalKeystore::open(keystore_path.path(), None)
+			.expect("Creates keystore")
+			.into();
+		let alice: sp_consensus_babe::AuthorityId = keystore
+			.sr25519_generate_new(BABE, Some("//Alice"))
+			.expect("Creates authority pair")
+			.into();
 
 		let chain_spec = crate::chain_spec::tests::integration_test_config_with_single_authority();
 
@@ -735,16 +736,16 @@ 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 = Keystore::sign_with(
-					&*keystore,
-					sp_consensus_babe::AuthorityId::ID,
-					&alice.to_public_crypto_pair(),
-					&to_sign,
-				)
-				.unwrap()
-				.unwrap()
-				.try_into()
-				.unwrap();
+				let signature = keystore
+					.sign_with(
+						sp_consensus_babe::AuthorityId::ID,
+						&alice.to_public_crypto_pair(),
+						&to_sign,
+					)
+					.unwrap()
+					.unwrap()
+					.try_into()
+					.unwrap();
 				let item = <DigestItem as CompatibleDigestItem>::babe_seal(signature);
 				slot += 1;
 

bin/node/cli/src/service.rs

@@ -22,7 +22,6 @@ use sp_application_crypto::AppKey;
 use sp_core::offchain::{testing::TestTransactionPoolExt, TransactionPoolExt};
 use sp_keyring::sr25519::Keyring::Alice;
 use sp_keystore::{testing::MemoryKeystore, Keystore, KeystoreExt};
-use std::sync::Arc;
 
 pub mod common;
 use self::common::*;
@@ -63,25 +62,16 @@ fn should_submit_signed_transaction() {
 	t.register_extension(TransactionPoolExt::new(pool));
 
 	let keystore = MemoryKeystore::new();
-	Keystore::sr25519_generate_new(
-		&keystore,
-		sr25519::AuthorityId::ID,
-		Some(&format!("{}/hunter1", PHRASE)),
-	)
-	.unwrap();
-	Keystore::sr25519_generate_new(
-		&keystore,
-		sr25519::AuthorityId::ID,
-		Some(&format!("{}/hunter2", PHRASE)),
-	)
-	.unwrap();
-	Keystore::sr25519_generate_new(
-		&keystore,
-		sr25519::AuthorityId::ID,
-		Some(&format!("{}/hunter3", PHRASE)),
-	)
-	.unwrap();
-	t.register_extension(KeystoreExt(Arc::new(keystore)));
+	keystore
+		.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter1", PHRASE)))
+		.unwrap();
+	keystore
+		.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter2", PHRASE)))
+		.unwrap();
+	keystore
+		.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter3", PHRASE)))
+		.unwrap();
+	t.register_extension(KeystoreExt::new(keystore));
 
 	t.execute_with(|| {
 		let results =
@@ -106,19 +96,13 @@ fn should_submit_signed_twice_from_the_same_account() {
 	t.register_extension(TransactionPoolExt::new(pool));
 
 	let keystore = MemoryKeystore::new();
-	Keystore::sr25519_generate_new(
-		&keystore,
-		sr25519::AuthorityId::ID,
-		Some(&format!("{}/hunter1", PHRASE)),
-	)
-	.unwrap();
-	Keystore::sr25519_generate_new(
-		&keystore,
-		sr25519::AuthorityId::ID,
-		Some(&format!("{}/hunter2", PHRASE)),
-	)
-	.unwrap();
-	t.register_extension(KeystoreExt(Arc::new(keystore)));
+	keystore
+		.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter1", PHRASE)))
+		.unwrap();
+	keystore
+		.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter2", PHRASE)))
+		.unwrap();
+	t.register_extension(KeystoreExt::new(keystore));
 
 	t.execute_with(|| {
 		let result =
@@ -169,7 +153,7 @@ fn should_submit_signed_twice_from_all_accounts() {
 	keystore
 		.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter2", PHRASE)))
 		.unwrap();
-	t.register_extension(KeystoreExt(Arc::new(keystore)));
+	t.register_extension(KeystoreExt::new(keystore));
 
 	t.execute_with(|| {
 		let results = Signer::<Runtime, TestAuthorityId>::all_accounts()
@@ -227,13 +211,10 @@ fn submitted_transaction_should_be_valid() {
 	t.register_extension(TransactionPoolExt::new(pool));
 
 	let keystore = MemoryKeystore::new();
-	Keystore::sr25519_generate_new(
-		&keystore,
-		sr25519::AuthorityId::ID,
-		Some(&format!("{}/hunter1", PHRASE)),
-	)
-	.unwrap();
-	t.register_extension(KeystoreExt(Arc::new(keystore)));
+	keystore
+		.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter1", PHRASE)))
+		.unwrap();
+	t.register_extension(KeystoreExt::new(keystore));
 
 	t.execute_with(|| {
 		let results =

bin/node/executor/tests/submit_transaction.rs

@@ -19,7 +19,6 @@
 use std::{
 	fs,
 	path::{Path, PathBuf},
-	sync::Arc,
 };
 
 use ansi_term::Style;
@@ -32,7 +31,7 @@ use sp_core::{
 	crypto::{ByteArray, Ss58Codec},
 	sr25519,
 };
-use sp_keystore::{Keystore, KeystorePtr};
+use sp_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,16 +163,17 @@ 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: KeystorePtr = Arc::new(
+		let keystore: KeystorePtr =
 			LocalKeystore::open(keystore_path.join(format!("auth-{}", n)), None)
-				.map_err(|err| err.to_string())?,
-		);
+				.map_err(|err| err.to_string())?
+				.into();
 
 		let (_, _, grandpa, babe, im_online, authority_discovery) =
 			chain_spec::authority_keys_from_seed(seed);
 
 		let insert_key = |key_type, public| {
-			Keystore::insert(&*keystore, key_type, &format!("//{}", seed), public)
+			keystore
+				.insert(key_type, &format!("//{}", seed), public)
 				.map_err(|_| format!("Failed to insert key: {}", grandpa))
 		};
 

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

@@ -697,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(MemoryKeystore::new())),
+		Role::PublishAndDiscover(MemoryKeystore::new().into()),
 		Some(prometheus_endpoint::Registry::new()),
 		Default::default(),
 	);
@@ -731,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(MemoryKeystore::new())),
+		Role::PublishAndDiscover(MemoryKeystore::new().into()),
 		Some(prometheus_endpoint::Registry::new()),
 		Default::default(),
 	);

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

@@ -24,8 +24,7 @@ use clap::Parser;
 use sc_keystore::LocalKeystore;
 use sc_service::config::{BasePath, KeystoreConfig};
 use sp_core::crypto::{KeyTypeId, SecretString};
-use sp_keystore::{Keystore, KeystorePtr};
-use std::sync::Arc;
+use sp_keystore::KeystorePtr;
 
 /// The `insert` command
 #[derive(Debug, Clone, Parser)]
@@ -67,9 +66,9 @@ impl InsertKeyCmd {
 		let config_dir = base_path.config_dir(chain_spec.id());
 
 		let (keystore, public) = match self.keystore_params.keystore_config(&config_dir)? {
-			(_, KeystoreConfig::Path { path, password }) => {
+			KeystoreConfig::Path { path, password } => {
 				let public = with_crypto_scheme!(self.scheme, to_vec(&suri, password.clone()))?;
-				let keystore: KeystorePtr = Arc::new(LocalKeystore::open(path, password)?);
+				let keystore: KeystorePtr = LocalKeystore::open(path, password)?.into();
 				(keystore, public)
 			},
 			_ => unreachable!("keystore_config always returns path and password; qed"),
@@ -78,7 +77,8 @@ impl InsertKeyCmd {
 		let key_type =
 			KeyTypeId::try_from(self.key_type.as_str()).map_err(|_| Error::KeyTypeInvalid)?;
 
-		Keystore::insert(&*keystore, key_type, &suri, &public[..])
+		keystore
+			.insert(key_type, &suri, &public[..])
 			.map_err(|_| Error::KeystoreOperation)?;
 
 		Ok(())
@@ -95,6 +95,7 @@ mod tests {
 	use super::*;
 	use sc_service::{ChainSpec, ChainType, GenericChainSpec, NoExtension};
 	use sp_core::{sr25519::Pair, ByteArray, Pair as _};
+	use sp_keystore::Keystore;
 	use tempfile::TempDir;
 
 	struct Cli;

client/cli/src/commands/insert_key.rs

@@ -185,10 +185,10 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
 	///
 	/// By default this is retrieved from `KeystoreParams` if it is available. Otherwise it uses
 	/// `KeystoreConfig::InMemory`.
-	fn keystore_config(&self, config_dir: &PathBuf) -> Result<(Option<String>, KeystoreConfig)> {
+	fn keystore_config(&self, config_dir: &PathBuf) -> Result<KeystoreConfig> {
 		self.keystore_params()
 			.map(|x| x.keystore_config(config_dir))
-			.unwrap_or_else(|| Ok((None, KeystoreConfig::InMemory)))
+			.unwrap_or_else(|| Ok(KeystoreConfig::InMemory))
 	}
 
 	/// Get the database cache size.
@@ -505,7 +505,7 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
 		let role = self.role(is_dev)?;
 		let max_runtime_instances = self.max_runtime_instances()?.unwrap_or(8);
 		let is_validator = role.is_authority();
-		let (keystore_remote, keystore) = self.keystore_config(&config_dir)?;
+		let keystore = self.keystore_config(&config_dir)?;
 		let telemetry_endpoints = self.telemetry_endpoints(&chain_spec)?;
 		let runtime_cache_size = self.runtime_cache_size()?;
 
@@ -524,7 +524,6 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
 				node_key,
 				DCV::p2p_listen_port(),
 			)?,
-			keystore_remote,
 			keystore,
 			database: self.database_config(&config_dir, database_cache_size, database)?,
 			trie_cache_maximum_size: self.trie_cache_maximum_size()?,