Remove ServiceBuilderCommand and implement the chain ops as standalone functions instead. (#6543)

* :)

* Slight tidy

* Remove ServiceBuilderCommand

* Remove whitespace

* Keep task manager alive for check_block/import_blocks

* Pass task_manager to run_until_exit

* Make task_manager in run_until_exit and make subcommands async

* Change the async_run fn to return a future and task manager

* async_run should take a result fn

* Apply suggestions from code review

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

* Fix spaces in export_raw_state

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

substrate/client/cli/src/commands/build_spec_cmd.rs

@@ -22,7 +22,7 @@ use crate::params::SharedParams;
 use crate::CliConfiguration;
 use log::info;
 use sc_network::config::build_multiaddr;
-use sc_service::{config::MultiaddrWithPeerId, Configuration};
+use sc_service::{config::{MultiaddrWithPeerId, NetworkConfiguration}, ChainSpec};
 use structopt::StructOpt;
 use std::io::Write;
 
@@ -51,13 +51,16 @@ pub struct BuildSpecCmd {
 
 impl BuildSpecCmd {
 	/// Run the build-spec command
-	pub fn run(&self, config: Configuration) -> error::Result<()> {
+	pub fn run(
+		&self,
+		mut spec: Box<dyn ChainSpec>,
+		network_config: NetworkConfiguration,
+	) -> error::Result<()> {
 		info!("Building chain spec");
-		let mut spec = config.chain_spec;
 		let raw_output = self.raw;
 
 		if spec.boot_nodes().is_empty() && !self.disable_default_bootnode {
-			let keys = config.network.node_key.into_keypair()?;
+			let keys = network_config.node_key.into_keypair()?;
 			let peer_id = keys.public().into_peer_id();
 			let addr = MultiaddrWithPeerId {
 				multiaddr: build_multiaddr![Ip4([127, 0, 0, 1]), Tcp(30333u16)],

substrate/client/cli/src/commands/check_block_cmd.rs

@@ -19,9 +19,9 @@
 use crate::{
 	CliConfiguration, error, params::{ImportParams, SharedParams, BlockNumberOrHash},
 };
-use sc_service::{Configuration, ServiceBuilderCommand};
-use sp_runtime::traits::{Block as BlockT, NumberFor};
-use std::{fmt::Debug, str::FromStr};
+use sc_client_api::{BlockBackend, UsageProvider};
+use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
+use std::{fmt::Debug, str::FromStr, sync::Arc};
 use structopt::StructOpt;
 
 /// The `check-block` command used to validate blocks.
@@ -48,21 +48,21 @@ pub struct CheckBlockCmd {
 
 impl CheckBlockCmd {
 	/// Run the check-block command
-	pub async fn run<B, BC, BB>(
+	pub async fn run<B, C, IQ>(
 		&self,
-		config: Configuration,
-		builder: B,
+		client: Arc<C>,
+		import_queue: IQ,
 	) -> error::Result<()>
 	where
-		B: FnOnce(Configuration) -> Result<BC, sc_service::error::Error>,
-		BC: ServiceBuilderCommand<Block = BB> + Unpin,
-		BB: BlockT + Debug,
-		<NumberFor<BB> as FromStr>::Err: std::fmt::Debug,
-		BB::Hash: FromStr,
-		<BB::Hash as FromStr>::Err: std::fmt::Debug,
+		B: BlockT + for<'de> serde::Deserialize<'de>,
+		C: BlockBackend<B> + UsageProvider<B> + Send + Sync + 'static,
+		IQ: sc_service::ImportQueue<B> + 'static,
+		B::Hash: FromStr,
+		<B::Hash as FromStr>::Err: Debug,
+		<<B::Header as HeaderT>::Number as FromStr>::Err: Debug,
 	{
 		let start = std::time::Instant::now();
-		builder(config)?.check_block(self.input.parse()?).await?;
+		sc_service::chain_ops::check_block(client, import_queue, self.input.parse()?).await?;
 		println!("Completed in {} ms.", start.elapsed().as_millis());
 
 		Ok(())

substrate/client/cli/src/commands/export_blocks_cmd.rs

@@ -21,13 +21,16 @@ use crate::params::{BlockNumber, DatabaseParams, PruningParams, SharedParams};
 use crate::CliConfiguration;
 use log::info;
 use sc_service::{
-	config::DatabaseConfig, Configuration, ServiceBuilderCommand,
+	config::DatabaseConfig, chain_ops::export_blocks,
 };
+use sc_client_api::{BlockBackend, UsageProvider};
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 use std::fmt::Debug;
 use std::fs;
 use std::io;
 use std::path::PathBuf;
+use std::str::FromStr;
+use std::sync::Arc;
 use structopt::StructOpt;
 
 /// The `export-blocks` command used to export blocks.
@@ -68,19 +71,17 @@ pub struct ExportBlocksCmd {
 
 impl ExportBlocksCmd {
 	/// Run the export-blocks command
-	pub async fn run<B, BC, BB>(
+	pub async fn run<B, C>(
 		&self,
-		config: Configuration,
-		builder: B,
+		client: Arc<C>,
+		database_config: DatabaseConfig,
 	) -> error::Result<()>
 	where
-		B: FnOnce(Configuration) -> Result<BC, sc_service::error::Error>,
-		BC: ServiceBuilderCommand<Block = BB> + Unpin,
-		BB: sp_runtime::traits::Block + Debug,
-		<<<BB as BlockT>::Header as HeaderT>::Number as std::str::FromStr>::Err: std::fmt::Debug,
-		<BB as BlockT>::Hash: std::str::FromStr,
+		B: BlockT,
+		C: BlockBackend<B> + UsageProvider<B> + 'static,
+		<<B::Header as HeaderT>::Number as FromStr>::Err: Debug,
 	{
-		if let DatabaseConfig::RocksDb { ref path, .. } = &config.database {
+		if let DatabaseConfig::RocksDb { ref path, .. } = database_config {
 			info!("DB path: {}", path.display());
 		}
 
@@ -94,8 +95,7 @@ impl ExportBlocksCmd {
 			None => Box::new(io::stdout()),
 		};
 
-		builder(config)?
-			.export_blocks(file, from.into(), to, binary)
+		export_blocks(client, file, from.into(), to, binary)
 			.await
 			.map_err(Into::into)
 	}

substrate/client/cli/src/commands/export_state_cmd.rs

@@ -20,10 +20,10 @@ use crate::{
 	CliConfiguration, error, params::{PruningParams, SharedParams, BlockNumberOrHash},
 };
 use log::info;
-use sc_service::{Configuration, ServiceBuilderCommand};
-use sp_runtime::traits::{Block as BlockT, NumberFor};
-use std::{fmt::Debug, str::FromStr, io::Write};
+use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
+use std::{fmt::Debug, str::FromStr, io::Write, sync::Arc};
 use structopt::StructOpt;
+use sc_client_api::{StorageProvider, UsageProvider};
 
 /// The `export-state` command used to export the state of a given block into
 /// a chain spec.
@@ -44,23 +44,22 @@ pub struct ExportStateCmd {
 
 impl ExportStateCmd {
 	/// Run the `export-state` command
-	pub fn run<B, BC, BB>(
+	pub async fn run<B, BA, C>(
 		&self,
-		config: Configuration,
-		builder: B,
+		client: Arc<C>,
+		mut input_spec: Box<dyn sc_service::ChainSpec>,
 	) -> error::Result<()>
 	where
-		B: FnOnce(Configuration) -> Result<BC, sc_service::error::Error>,
-		BC: ServiceBuilderCommand<Block = BB> + Unpin,
-		BB: BlockT + Debug,
-		<NumberFor<BB> as FromStr>::Err: std::fmt::Debug,
-		BB::Hash: FromStr,
-		<BB::Hash as FromStr>::Err: std::fmt::Debug,
+		B: BlockT,
+		C: UsageProvider<B> + StorageProvider<B, BA>,
+		BA: sc_client_api::backend::Backend<B>,
+		B::Hash: FromStr,
+		<B::Hash as FromStr>::Err: Debug,
+		<<B::Header as HeaderT>::Number as FromStr>::Err: Debug,
 	{
 		info!("Exporting raw state...");
-		let mut input_spec = config.chain_spec.cloned_box();
 		let block_id = self.input.as_ref().map(|b| b.parse()).transpose()?;
-		let raw_state = builder(config)?.export_raw_state(block_id)?;
+		let raw_state = sc_service::chain_ops::export_raw_state(client, block_id)?;
 		input_spec.set_storage(raw_state);
 
 		info!("Generating new chain spec...");

substrate/client/cli/src/commands/import_blocks_cmd.rs

@@ -20,13 +20,15 @@ use crate::error;
 use crate::params::ImportParams;
 use crate::params::SharedParams;
 use crate::CliConfiguration;
-use sc_service::{Configuration, ServiceBuilderCommand};
-use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
+use sc_service::chain_ops::import_blocks;
+use sp_runtime::traits::Block as BlockT;
 use std::fmt::Debug;
 use std::fs;
 use std::io::{self, Read, Seek};
 use std::path::PathBuf;
+use std::sync::Arc;
 use structopt::StructOpt;
+use sc_client_api::UsageProvider;
 
 /// The `import-blocks` command used to import blocks.
 #[derive(Debug, StructOpt)]
@@ -61,17 +63,15 @@ impl<T: Read + Seek> ReadPlusSeek for T {}
 
 impl ImportBlocksCmd {
 	/// Run the import-blocks command
-	pub async fn run<B, BC, BB>(
+	pub async fn run<B, C, IQ>(
 		&self,
-		config: Configuration,
-		builder: B,
+		client: Arc<C>,
+		import_queue: IQ,
 	) -> error::Result<()>
 	where
-		B: FnOnce(Configuration) -> Result<BC, sc_service::error::Error>,
-		BC: ServiceBuilderCommand<Block = BB> + Unpin,
-		BB: sp_runtime::traits::Block + Debug,
-		<<<BB as BlockT>::Header as HeaderT>::Number as std::str::FromStr>::Err: std::fmt::Debug,
-		<BB as BlockT>::Hash: std::str::FromStr,
+		C: UsageProvider<B> + Send + Sync + 'static,
+		B: BlockT + for<'de> serde::Deserialize<'de>,
+		IQ: sc_service::ImportQueue<B> + 'static,
 	{
 		let file: Box<dyn ReadPlusSeek + Send> = match &self.input {
 			Some(filename) => Box::new(fs::File::open(filename)?),
@@ -82,8 +82,7 @@ impl ImportBlocksCmd {
 			}
 		};
 
-		builder(config)?
-			.import_blocks(file, false, self.binary)
+		import_blocks(client, import_queue, file, false, self.binary)
 			.await
 			.map_err(Into::into)
 	}

substrate/client/cli/src/commands/purge_chain_cmd.rs

@@ -19,7 +19,7 @@
 use crate::error;
 use crate::params::{DatabaseParams, SharedParams};
 use crate::CliConfiguration;
-use sc_service::Configuration;
+use sc_service::DatabaseConfig;
 use std::fmt::Debug;
 use std::fs;
 use std::io::{self, Write};
@@ -43,8 +43,8 @@ pub struct PurgeChainCmd {
 
 impl PurgeChainCmd {
 	/// Run the purge command
-	pub fn run(&self, config: Configuration) -> error::Result<()> {
-		let db_path = config.database.path()
+	pub fn run(&self, database_config: DatabaseConfig) -> error::Result<()> {
+		let db_path = database_config.path()
 			.ok_or_else(||
 				error::Error::Input("Cannot purge custom database implementation".into())
 		)?;

substrate/client/cli/src/commands/revert_cmd.rs

@@ -19,10 +19,13 @@
 use crate::error;
 use crate::params::{BlockNumber, PruningParams, SharedParams};
 use crate::CliConfiguration;
-use sc_service::{Configuration, ServiceBuilderCommand};
+use sc_service::chain_ops::revert_chain;
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 use std::fmt::Debug;
+use std::str::FromStr;
+use std::sync::Arc;
 use structopt::StructOpt;
+use sc_client_api::{Backend, UsageProvider};
 
 /// The `revert` command used revert the chain to a previous state.
 #[derive(Debug, StructOpt)]
@@ -42,16 +45,19 @@ pub struct RevertCmd {
 
 impl RevertCmd {
 	/// Run the revert command
-	pub fn run<B, BC, BB>(&self, config: Configuration, builder: B) -> error::Result<()>
+	pub async fn run<B, BA, C>(
+		&self,
+		client: Arc<C>,
+		backend: Arc<BA>,
+	) -> error::Result<()>
 	where
-		B: FnOnce(Configuration) -> Result<BC, sc_service::error::Error>,
-		BC: ServiceBuilderCommand<Block = BB> + Unpin,
-		BB: sp_runtime::traits::Block + Debug,
-		<<<BB as BlockT>::Header as HeaderT>::Number as std::str::FromStr>::Err: std::fmt::Debug,
-		<BB as BlockT>::Hash: std::str::FromStr,
+		B: BlockT,
+		BA: Backend<B>,
+		C: UsageProvider<B>,
+		<<<B as BlockT>::Header as HeaderT>::Number as FromStr>::Err: Debug,
 	{
 		let blocks = self.num.parse()?;
-		builder(config)?.revert_chain(blocks)?;
+		revert_chain(client, backend, blocks)?;
 
 		Ok(())
 	}

substrate/client/cli/src/runner.rs

@@ -25,10 +25,11 @@ use futures::pin_mut;
 use futures::select;
 use futures::{future, future::FutureExt, Future};
 use log::info;
-use sc_service::{Configuration, ServiceBuilderCommand, TaskType, TaskManager};
+use sc_service::{Configuration, TaskType, TaskManager};
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 use sp_utils::metrics::{TOKIO_THREADS_ALIVE, TOKIO_THREADS_TOTAL};
-use std::{fmt::Debug, marker::PhantomData, str::FromStr};
+use std::{fmt::Debug, marker::PhantomData, str::FromStr, sync::Arc};
+use sc_client_api::{UsageProvider, BlockBackend, StorageProvider};
 
 #[cfg(target_family = "unix")]
 async fn main<F, E>(func: F) -> std::result::Result<(), Box<dyn std::error::Error>>
@@ -92,7 +93,11 @@ pub fn build_runtime() -> std::result::Result<tokio::runtime::Runtime, std::io::
 		.build()
 }
 
-fn run_until_exit<FUT, ERR>(mut tokio_runtime: tokio::runtime::Runtime, future: FUT) -> Result<()>
+fn run_until_exit<FUT, ERR>(
+	mut tokio_runtime: tokio::runtime::Runtime, 
+	future: FUT, 
+	mut task_manager: TaskManager,
+) -> Result<()>
 where
 	FUT: Future<Output = std::result::Result<(), ERR>> + future::Future,
 	ERR: 'static + std::error::Error,
@@ -102,6 +107,9 @@ where
 
 	tokio_runtime.block_on(main(f)).map_err(|e| e.to_string())?;
 
+	task_manager.terminate();
+	drop(tokio_runtime);
+
 	Ok(())
 }
 
@@ -173,29 +181,47 @@ impl<C: SubstrateCli> Runner<C> {
 
 	/// A helper function that runs a future with tokio and stops if the process receives the signal
 	/// `SIGTERM` or `SIGINT`.
-	pub fn run_subcommand<B, BC, BB>(self, subcommand: &Subcommand, builder: B) -> Result<()>
+	pub fn run_subcommand<BU, B, BA, IQ, CL>(self, subcommand: &Subcommand, builder: BU)
+		-> Result<()>
 	where
-		B: FnOnce(Configuration) -> sc_service::error::Result<BC>,
-		BC: ServiceBuilderCommand<Block = BB> + Unpin,
-		BB: sp_runtime::traits::Block + Debug,
-		<<<BB as BlockT>::Header as HeaderT>::Number as FromStr>::Err: Debug,
-		<BB as BlockT>::Hash: FromStr,
-		<<BB as BlockT>::Hash as FromStr>::Err: Debug,
+		BU: FnOnce(Configuration)
+			-> sc_service::error::Result<(Arc<CL>, Arc<BA>, IQ, TaskManager)>,
+		B: BlockT + for<'de> serde::Deserialize<'de>,
+		BA: sc_client_api::backend::Backend<B> + 'static,
+		IQ: sc_service::ImportQueue<B> + 'static,
+		<B as BlockT>::Hash: FromStr,
+		<<B as BlockT>::Hash as FromStr>::Err: Debug,
+		<<<B as BlockT>::Header as HeaderT>::Number as FromStr>::Err: Debug,
+		CL: UsageProvider<B> + BlockBackend<B> + StorageProvider<B, BA> + Send + Sync +
+		'static,
 	{
+		let chain_spec = self.config.chain_spec.cloned_box();
+		let network_config = self.config.network.clone();
+		let db_config = self.config.database.clone();
+
 		match subcommand {
-			Subcommand::BuildSpec(cmd) => cmd.run(self.config),
+			Subcommand::BuildSpec(cmd) => cmd.run(chain_spec, network_config),
 			Subcommand::ExportBlocks(cmd) => {
-				run_until_exit(self.tokio_runtime, cmd.run(self.config, builder))
+				let (client, _, _, task_manager) = builder(self.config)?;
+				run_until_exit(self.tokio_runtime, cmd.run(client, db_config), task_manager)
 			}
 			Subcommand::ImportBlocks(cmd) => {
-				run_until_exit(self.tokio_runtime, cmd.run(self.config, builder))
+				let (client, _, import_queue, task_manager) = builder(self.config)?;
+				run_until_exit(self.tokio_runtime, cmd.run(client, import_queue), task_manager)
 			}
 			Subcommand::CheckBlock(cmd) => {
-				run_until_exit(self.tokio_runtime, cmd.run(self.config, builder))
+				let (client, _, import_queue, task_manager) = builder(self.config)?;
+				run_until_exit(self.tokio_runtime, cmd.run(client, import_queue), task_manager)
 			}
-			Subcommand::Revert(cmd) => cmd.run(self.config, builder),
-			Subcommand::PurgeChain(cmd) => cmd.run(self.config),
-			Subcommand::ExportState(cmd) => cmd.run(self.config, builder),
+			Subcommand::Revert(cmd) => {
+				let (client, backend, _, task_manager) = builder(self.config)?;
+				run_until_exit(self.tokio_runtime, cmd.run(client, backend), task_manager)
+			},
+			Subcommand::PurgeChain(cmd) => cmd.run(db_config),
+			Subcommand::ExportState(cmd) => {
+				let (client, _, _, task_manager) = builder(self.config)?;
+				run_until_exit(self.tokio_runtime, cmd.run(client, chain_spec), task_manager)
+			},
 		}
 	}
 
@@ -221,11 +247,14 @@ impl<C: SubstrateCli> Runner<C> {
 
 	/// A helper function that runs a future with tokio and stops if the process receives
 	/// the signal SIGTERM or SIGINT
-	pub fn async_run<FUT>(self, runner: impl FnOnce(Configuration) -> FUT) -> Result<()>
+	pub fn async_run<FUT>(
+		self, runner: impl FnOnce(Configuration) -> Result<(FUT, TaskManager)>,
+	) -> Result<()>
 	where
 		FUT: Future<Output = Result<()>>,
 	{
-		run_until_exit(self.tokio_runtime, runner(self.config))
+		let (future, task_manager) = runner(self.config)?;
+		run_until_exit(self.tokio_runtime, future, task_manager)
 	}
 
 	/// Get an immutable reference to the node Configuration