Keystore overhaul (iter 2)

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

@@ -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/cli/benches/block_production.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/transaction_pool.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),

client/cli/src/commands/insert_key.rs

@@ -67,7 +67,7 @@ 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)?);
 				(keystore, public)

client/cli/src/config.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()?,

client/cli/src/params/keystore_params.rs

@@ -68,9 +68,7 @@ pub fn secret_string_from_str(s: &str) -> std::result::Result<SecretString, Stri
 
 impl KeystoreParams {
 	/// Get the keystore configuration for the parameters
-	///
-	/// Returns a vector of remote-urls and the local Keystore configuration
-	pub fn keystore_config(&self, config_dir: &Path) -> Result<(Option<String>, KeystoreConfig)> {
+	pub fn keystore_config(&self, config_dir: &Path) -> Result<KeystoreConfig> {
 		let password = if self.password_interactive {
 			Some(SecretString::new(input_keystore_password()?))
 		} else if let Some(ref file) = self.password_filename {
@@ -85,7 +83,7 @@ impl KeystoreParams {
 			.clone()
 			.unwrap_or_else(|| config_dir.join(DEFAULT_KEYSTORE_CONFIG_PATH));
 
-		Ok((self.keystore_uri.clone(), KeystoreConfig::Path { path, password }))
+		Ok(KeystoreConfig::Path { path, password })
 	}
 
 	/// helper method to fetch password from `KeyParams` or read from stdin

client/cli/src/runner.rs

@@ -345,7 +345,6 @@ mod tests {
 				transaction_pool: Default::default(),
 				network: NetworkConfiguration::new_memory(),
 				keystore: sc_service::config::KeystoreConfig::InMemory,
-				keystore_remote: None,
 				database: sc_client_db::DatabaseSource::ParityDb { path: PathBuf::from("db") },
 				trie_cache_maximum_size: None,
 				state_pruning: None,

client/consensus/babe/src/authorship.rs

@@ -287,14 +287,14 @@ 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 sp_keystore::testing::MemoryKeystore;
 	use std::sync::Arc;
 
 	#[test]
 	fn claim_secondary_plain_slot_works() {
-		let keystore: KeystorePtr = Arc::new(LocalKeystore::in_memory());
+		let keystore: KeystorePtr = Arc::new(MemoryKeystore::new());
 		let valid_public_key = Keystore::sr25519_generate_new(
 			&*keystore,
 			AuthorityId::ID,

client/consensus/beefy/src/communication/gossip.rs

@@ -243,17 +243,14 @@ where
 
 #[cfg(test)]
 mod tests {
-	use sc_keystore::LocalKeystore;
-	use sc_network_test::Block;
-	use sp_keystore::{Keystore, KeystorePtr};
-
+	use super::*;
 	use crate::keystore::BeefyKeystore;
+	use sc_network_test::Block;
 	use sp_consensus_beefy::{
 		crypto::Signature, known_payloads, Commitment, Keyring, MmrRootHash, Payload, VoteMessage,
 		KEY_TYPE,
 	};
-
-	use super::*;
+	use sp_keystore::{testing::MemoryKeystore, Keystore, KeystorePtr};
 
 	#[test]
 	fn known_votes_insert_remove() {
@@ -306,7 +303,7 @@ mod tests {
 	}
 
 	fn sign_commitment<BN: Encode>(who: &Keyring, commitment: &Commitment<BN>) -> Signature {
-		let store: KeystorePtr = std::sync::Arc::new(LocalKeystore::in_memory());
+		let store: KeystorePtr = std::sync::Arc::new(MemoryKeystore::new());
 		Keystore::ecdsa_generate_new(&*store, KEY_TYPE, Some(&who.to_seed())).unwrap();
 		let beefy_keystore: BeefyKeystore = Some(store).into();
 

client/consensus/beefy/src/keystore.rs

@@ -120,16 +120,15 @@ impl From<Option<KeystorePtr>> for BeefyKeystore {
 pub mod tests {
 	use std::sync::Arc;
 
-	use sc_keystore::LocalKeystore;
-	use sp_core::{ecdsa, Pair};
-
 	use sp_consensus_beefy::{crypto, Keyring};
+	use sp_core::{ecdsa, Pair};
+	use sp_keystore::testing::MemoryKeystore;
 
 	use super::*;
 	use crate::error::Error;
 
 	fn keystore() -> KeystorePtr {
-		Arc::new(LocalKeystore::in_memory())
+		Arc::new(MemoryKeystore::new())
 	}
 
 	#[test]

client/keystore/src/local.rs

@@ -68,6 +68,9 @@ impl LocalKeystore {
 	}
 }
 
+/// Implementation details:
+/// - Key generation functions (e.g. `ed25519_generate_new`): if the `seed` parameter is `Some(_)`
+///   then the key will be ephemeral and stored in memory.
 impl Keystore for LocalKeystore {
 	fn keys(&self, id: KeyTypeId) -> std::result::Result<Vec<CryptoTypePublicPair>, TraitError> {
 		let raw_keys = self.0.read().raw_public_keys(id)?;

client/service/src/builder.rs

@@ -67,7 +67,7 @@ use sp_consensus::block_validation::{
 	BlockAnnounceValidator, Chain, DefaultBlockAnnounceValidator,
 };
 use sp_core::traits::{CodeExecutor, SpawnNamed};
-use sp_keystore::{Keystore, KeystorePtr};
+use sp_keystore::KeystorePtr;
 use sp_runtime::traits::{Block as BlockT, BlockIdTo, NumberFor, Zero};
 use std::{str::FromStr, sync::Arc, time::SystemTime};
 
@@ -85,24 +85,8 @@ pub type TFullCallExecutor<TBl, TExec> =
 type TFullParts<TBl, TRtApi, TExec> =
 	(TFullClient<TBl, TRtApi, TExec>, Arc<TFullBackend<TBl>>, KeystoreContainer, TaskManager);
 
-trait AsKeystoreRef {
-	fn keystore_ref(&self) -> Arc<dyn Keystore>;
-}
-
-impl<T> AsKeystoreRef for Arc<T>
-where
-	T: Keystore + 'static,
-{
-	fn keystore_ref(&self) -> Arc<dyn Keystore> {
-		self.clone()
-	}
-}
-
-/// Construct and hold different layers of Keystore wrappers
-pub struct KeystoreContainer {
-	remote: Option<Box<dyn AsKeystoreRef>>,
-	local: Arc<LocalKeystore>,
-}
+/// Construct a local keystore shareable container
+pub struct KeystoreContainer(Arc<LocalKeystore>);
 
 impl KeystoreContainer {
 	/// Construct KeystoreContainer
@@ -113,41 +97,17 @@ impl KeystoreContainer {
 			KeystoreConfig::InMemory => LocalKeystore::in_memory(),
 		});
 
-		Ok(Self { remote: Default::default(), local: keystore })
+		Ok(Self(keystore))
 	}
 
-	/// Set the remote keystore.
-	/// Should be called right away at startup and not at runtime:
-	/// even though this overrides any previously set remote store, it
-	/// does not reset any references previously handed out - they will
-	/// stick around.
-	pub fn set_remote_keystore<T>(&mut self, remote: Arc<T>)
-	where
-		T: Keystore + 'static,
-	{
-		self.remote = Some(Box::new(remote))
+	/// Returns a shared reference to a dynamic `Keystore` trait implementation.
+	pub fn keystore(&self) -> KeystorePtr {
+		self.0.clone()
 	}
 
-	/// Returns an adapter to a `Keystore` implementation.
-	pub fn keystore(&self) -> Arc<dyn Keystore> {
-		if let Some(c) = self.remote.as_ref() {
-			c.keystore_ref()
-		} else {
-			self.local.clone()
-		}
-	}
-
-	/// Returns the local keystore if available
-	///
-	/// The function will return None if the available keystore is not a local keystore.
-	///
-	/// # Note
-	///
-	/// Using the [`LocalKeystore`] will result in loosing the ability to use any other keystore
-	/// implementation, like a remote keystore for example. Only use this if you a certain that you
-	/// require it!
-	pub fn local_keystore(&self) -> Option<Arc<LocalKeystore>> {
-		Some(self.local.clone())
+	/// Returns a shared reference to the local keystore .
+	pub fn local_keystore(&self) -> Arc<LocalKeystore> {
+		self.0.clone()
 	}
 }
 

client/service/src/config.rs

@@ -61,8 +61,6 @@ pub struct Configuration {
 	pub network: NetworkConfiguration,
 	/// Configuration for the keystore.
 	pub keystore: KeystoreConfig,
-	/// Remote URI to connect to for async keystore support
-	pub keystore_remote: Option<String>,
 	/// Configuration for the database.
 	pub database: DatabaseSource,
 	/// Maximum size of internal trie cache in bytes.

client/service/test/src/lib.rs

@@ -242,7 +242,6 @@ fn node_config<
 		tokio_handle,
 		transaction_pool: Default::default(),
 		network: network_config,
-		keystore_remote: Default::default(),
 		keystore: KeystoreConfig::Path { path: root.join("key"), password: None },
 		database: DatabaseSource::RocksDb { path: root.join("db"), cache_size: 128 },
 		trie_cache_maximum_size: Some(16 * 1024 * 1024),