Can now disable the keystore (#3004)

* Can now disable the keystore

* Fix service test

* Apply suggestions from code review

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

* Fix cli

substrate/core/cli/src/lib.rs

@@ -399,13 +399,9 @@ where
 
 	let base_path = base_path(&cli.shared_params, version);
 
-	config.keystore_path = cli.keystore_path
+	config.keystore_path = cli.keystore_path.or_else(|| Some(keystore_path(&base_path, config.chain_spec.id())));
-		.unwrap_or_else(|| keystore_path(&base_path, config.chain_spec.id()))
-		.to_string_lossy()
-		.into();
 
-	config.database_path =
+	config.database_path = db_path(&base_path, config.chain_spec.id());
-		db_path(&base_path, config.chain_spec.id()).to_string_lossy().into();
 	config.database_cache_size = cli.database_cache_size;
 	config.state_cache_size = cli.state_cache_size;
 	config.pruning = match cli.pruning {
@@ -594,7 +590,7 @@ where
 	let base_path = base_path(cli, version);
 
 	let mut config = service::Configuration::default_with_spec(spec.clone());
-	config.database_path = db_path(&base_path, spec.id()).to_string_lossy().into();
+	config.database_path = db_path(&base_path, spec.id());
 
 	Ok(config)
 }
@@ -612,7 +608,7 @@ where
 {
 	let config = create_config_with_db_path::<F, _>(spec_factory, &cli.shared_params, version)?;
 
-	info!("DB path: {}", config.database_path);
+	info!("DB path: {}", config.database_path.display());
 	let from = cli.from.unwrap_or(1);
 	let to = cli.to;
 	let json = cli.json;

substrate/core/service/src/components.rs

@@ -525,7 +525,7 @@ impl<Factory: ServiceFactory> Components for FullComponents<Factory> {
 			state_cache_size: config.state_cache_size,
 			state_cache_child_ratio:
 				config.state_cache_child_ratio.map(|v| (v, 100)),
-			path: config.database_path.as_str().into(),
+			path: config.database_path.clone(),
 			pruning: config.pruning.clone(),
 		};
 		Ok((Arc::new(client_db::new_client(
@@ -627,7 +627,7 @@ impl<Factory: ServiceFactory> Components for LightComponents<Factory> {
 			state_cache_size: config.state_cache_size,
 			state_cache_child_ratio:
 				config.state_cache_child_ratio.map(|v| (v, 100)),
-			path: config.database_path.as_str().into(),
+			path: config.database_path.clone(),
 			pruning: config.pruning.clone(),
 		};
 		let db_storage = client_db::light::LightStorage::new(db_settings)?;

substrate/core/service/src/config.rs

@@ -16,7 +16,7 @@
 
 //! Service configuration.
 
-use std::net::SocketAddr;
+use std::{path::PathBuf, net::SocketAddr};
 use transaction_pool;
 use crate::chain_spec::ChainSpec;
 pub use client::ExecutionStrategies;
@@ -44,9 +44,9 @@ pub struct Configuration<C, G: Serialize + DeserializeOwned + BuildStorage> {
 	/// Network configuration.
 	pub network: NetworkConfiguration,
 	/// Path to key files.
-	pub keystore_path: String,
+	pub keystore_path: Option<PathBuf>,
 	/// Path to the database.
-	pub database_path: String,
+	pub database_path: PathBuf,
 	/// Cache Size for internal database in MiB
 	pub database_cache_size: Option<u32>,
 	/// Size of internal state cache in Bytes

substrate/core/service/src/lib.rs

@@ -78,7 +78,7 @@ pub struct Service<Components: components::Components> {
 	/// Sinks to propagate network status updates.
 	network_status_sinks: Arc<Mutex<Vec<mpsc::UnboundedSender<NetworkStatus<ComponentBlock<Components>>>>>>,
 	transaction_pool: Arc<TransactionPool<Components::TransactionPoolApi>>,
-	keystore: Keystore,
+	keystore: Option<Keystore>,
 	exit: ::exit_future::Exit,
 	signal: Option<Signal>,
 	/// Sender for futures that must be spawned as background tasks.
@@ -163,24 +163,40 @@ impl<Components: components::Components> Service<Components> {
 		// Create client
 		let executor = NativeExecutor::new(config.default_heap_pages);
 
-		let mut keystore = Keystore::open(config.keystore_path.as_str().into())?;
+		let mut keystore = if let Some(keystore_path) = config.keystore_path.as_ref() {
+			match Keystore::open(keystore_path.clone()) {
+				Ok(ks) => Some(ks),
+				Err(err) => {
+					error!("Failed to initialize keystore: {}", err);
+					None
+				}
+			}
+		} else {
+			None
+		};
+
+		// Keep the public key for telemetry
+		let public_key: String;
 
 		// This is meant to be for testing only
 		// FIXME #1063 remove this
-		for seed in &config.keys {
+		if let Some(keystore) = keystore.as_mut() {
-			keystore.generate_from_seed(seed)?;
+			for seed in &config.keys {
-		}
+				keystore.generate_from_seed(seed)?;
-		// Keep the public key for telemetry
-		let public_key = match keystore.contents()?.get(0) {
-			Some(public_key) => public_key.clone(),
-			None => {
-				let key = keystore.generate(&config.password)?;
-				let public_key = key.public();
-				info!("Generated a new keypair: {:?}", public_key);
-
-				public_key
 			}
-		};
+
+			public_key = match keystore.contents()?.get(0) {
+				Some(public_key) => public_key.to_string(),
+				None => {
+					let key = keystore.generate(&config.password)?;
+					let public_key = key.public();
+					info!("Generated a new keypair: {:?}", public_key);
+					public_key.to_string()
+				}
+			}
+		} else {
+			public_key = format!("<disabled-keystore>");
+		}
 
 		let (client, on_demand) = Components::build_client(&config, executor)?;
 		let select_chain = Components::build_select_chain(&mut config, client.clone())?;
@@ -469,7 +485,6 @@ impl<Components: components::Components> Service<Components> {
 		let telemetry = config.telemetry_endpoints.clone().map(|endpoints| {
 			let is_authority = config.roles == Roles::AUTHORITY;
 			let network_id = network.local_peer_id().to_base58();
-			let pubkey = format!("{}", public_key);
 			let name = config.name.clone();
 			let impl_name = config.impl_name.to_owned();
 			let version = version.clone();
@@ -490,7 +505,7 @@ impl<Components: components::Components> Service<Components> {
 						"version" => version.clone(),
 						"config" => "",
 						"chain" => chain_name.clone(),
-						"pubkey" => &pubkey,
+						"pubkey" => &public_key,
 						"authority" => is_authority,
 						"network_id" => network_id.clone()
 					);
@@ -529,11 +544,14 @@ impl<Components: components::Components> Service<Components> {
 	/// give the authority key, if we are an authority and have a key
 	pub fn authority_key(&self) -> Option<primitives::ed25519::Pair> {
 		if self.config.roles != Roles::AUTHORITY { return None }
-		let keystore = &self.keystore;
+		if let Some(keystore) = &self.keystore {
-		if let Ok(Some(Ok(key))) =  keystore.contents().map(|keys| keys.get(0)
+			if let Ok(Some(Ok(key))) =  keystore.contents().map(|keys| keys.get(0)
-				.map(|k| keystore.load(k, &self.config.password)))
+					.map(|k| keystore.load(k, &self.config.password)))
-		{
+			{
-			Some(key)
+				Some(key)
+			} else {
+				None
+			}
 		} else {
 			None
 		}
@@ -583,11 +601,6 @@ impl<Components: components::Components> Service<Components> {
 		self.transaction_pool.clone()
 	}
 
-	/// Get shared keystore.
-	pub fn keystore(&self) -> &Keystore {
-		&self.keystore
-	}
-
 	/// Get a handle to a future that will resolve on exit.
 	pub fn on_exit(&self) -> ::exit_future::Exit {
 		self.exit.clone()

substrate/core/service/test/src/lib.rs

@@ -171,8 +171,8 @@ fn node_config<F: ServiceFactory> (
 		roles: role,
 		transaction_pool: Default::default(),
 		network: network_config,
-		keystore_path: root.join("key").to_str().unwrap().into(),
+		keystore_path: Some(root.join("key")),
-		database_path: root.join("db").to_str().unwrap().into(),
+		database_path: root.join("db"),
 		database_cache_size: None,
 		state_cache_size: 16777216,
 		state_cache_child_ratio: None,