Improve sc-service API (#5364)

This improves `sc-service` API by not requiring the whole
`&Configuration`, using specific configuration options instead.
`RpcConfiguration` was also extracted from `Configuration` to group all
RPC options together.

We don't use Substrate's CLI and would rather not use `Configuration`
either, but some key public functions require it even though they
ignored most of the fields anyway.

`RpcConfiguration` is very helpful not just for consolidation of the
fields, but also to finally make RPC optional for our use case, while
Substrate still runs RPC server on localhost even if listening address
is explicitly set to `None`, which is annoying (and I suspect there is a
reason for it, so didn't want to change the default just yet).

While this is a breaking change, most developers will not notice it if
they use higher-level APIs.

Fixes #2897

---------

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

cumulus/client/relay-chain-minimal-node/src/lib.rs

@@ -175,7 +175,7 @@ async fn new_minimal_relay_chain<Block: BlockT, Network: NetworkBackend<RelayBlo
 	collator_pair: CollatorPair,
 	relay_chain_rpc_client: Arc<BlockChainRpcClient>,
 ) -> Result<NewMinimalNode, RelayChainError> {
-	let role = config.role.clone();
+	let role = config.role;
 	let mut net_config = sc_network::config::FullNetworkConfiguration::<_, _, Network>::new(
 		&config.network,
 		config.prometheus_config.as_ref().map(|cfg| cfg.registry.clone()),

cumulus/client/relay-chain-minimal-node/src/network.rs

@@ -65,7 +65,7 @@ pub(crate) fn build_collator_network<Network: NetworkBackend<Block, Hash>>(
 	spawn_handle.spawn("peer-store", Some("networking"), peer_store.run());
 
 	let network_params = sc_network::config::Params::<Block, Hash, Network> {
-		role: config.role.clone(),
+		role: config.role,
 		executor: {
 			let spawn_handle = Clone::clone(&spawn_handle);
 			Box::new(move |fut| {

cumulus/polkadot-parachain/polkadot-parachain-lib/src/cli.rs

@@ -317,10 +317,9 @@ impl<Config: CliConfig> CliConfiguration<Self> for RelayChainCli<Config> {
 		_support_url: &String,
 		_impl_version: &String,
 		_logger_hook: F,
-		_config: &sc_service::Configuration,
 	) -> sc_cli::Result<()>
 	where
-		F: FnOnce(&mut sc_cli::LoggerBuilder, &sc_service::Configuration),
+		F: FnOnce(&mut sc_cli::LoggerBuilder),
 	{
 		unreachable!("PolkadotCli is never initialized; qed");
 	}

cumulus/test/service/src/cli.rs

@@ -139,10 +139,9 @@ impl CliConfiguration<Self> for RelayChainCli {
 		_support_url: &String,
 		_impl_version: &String,
 		_logger_hook: F,
-		_config: &sc_service::Configuration,
 	) -> CliResult<()>
 	where
-		F: FnOnce(&mut sc_cli::LoggerBuilder, &sc_service::Configuration),
+		F: FnOnce(&mut sc_cli::LoggerBuilder),
 	{
 		unreachable!("PolkadotCli is never initialized; qed");
 	}

polkadot/node/service/src/lib.rs

@@ -437,15 +437,16 @@ fn new_partial_basics(
 		.transpose()?;
 
 	let heap_pages = config
+		.executor
 		.default_heap_pages
 		.map_or(DEFAULT_HEAP_ALLOC_STRATEGY, |h| HeapAllocStrategy::Static { extra_pages: h as _ });
 
 	let executor = WasmExecutor::builder()
-		.with_execution_method(config.wasm_method)
+		.with_execution_method(config.executor.wasm_method)
 		.with_onchain_heap_alloc_strategy(heap_pages)
 		.with_offchain_heap_alloc_strategy(heap_pages)
-		.with_max_runtime_instances(config.max_runtime_instances)
-		.with_runtime_cache_size(config.runtime_cache_size)
+		.with_max_runtime_instances(config.executor.max_runtime_instances)
+		.with_runtime_cache_size(config.executor.runtime_cache_size)
 		.build();
 
 	let (client, backend, keystore_container, task_manager) =
@@ -767,7 +768,7 @@ pub fn new_full<
 	use sc_network_sync::WarpSyncConfig;
 
 	let is_offchain_indexing_enabled = config.offchain_worker.indexing_enabled;
-	let role = config.role.clone();
+	let role = config.role;
 	let force_authoring = config.force_authoring;
 	let backoff_authoring_blocks = if !force_authoring_backoff &&
 		(config.chain_spec.is_polkadot() || config.chain_spec.is_kusama())

polkadot/node/test/service/src/lib.rs

@@ -68,6 +68,7 @@ use substrate_test_client::{
 pub type Client = FullClient;
 
 pub use polkadot_service::{FullBackend, GetLastTimestamp};
+use sc_service::config::{ExecutorConfiguration, RpcConfiguration};
 
 /// Create a new full node.
 #[sc_tracing::logging::prefix_logs_with(config.network.node_name.as_str())]
@@ -200,35 +201,37 @@ pub fn node_config(
 		state_pruning: Default::default(),
 		blocks_pruning: BlocksPruning::KeepFinalized,
 		chain_spec: Box::new(spec),
-		wasm_method: WasmExecutionMethod::Compiled {
-			instantiation_strategy: WasmtimeInstantiationStrategy::PoolingCopyOnWrite,
+		executor: ExecutorConfiguration {
+			wasm_method: WasmExecutionMethod::Compiled {
+				instantiation_strategy: WasmtimeInstantiationStrategy::PoolingCopyOnWrite,
+			},
+			..ExecutorConfiguration::default()
 		},
 		wasm_runtime_overrides: Default::default(),
-		rpc_addr: Default::default(),
-		rpc_max_request_size: Default::default(),
-		rpc_max_response_size: Default::default(),
-		rpc_max_connections: Default::default(),
-		rpc_cors: None,
-		rpc_methods: Default::default(),
-		rpc_id_provider: None,
-		rpc_max_subs_per_conn: Default::default(),
-		rpc_port: 9944,
-		rpc_message_buffer_capacity: Default::default(),
-		rpc_batch_config: RpcBatchRequestConfig::Unlimited,
-		rpc_rate_limit: None,
-		rpc_rate_limit_whitelisted_ips: Default::default(),
-		rpc_rate_limit_trust_proxy_headers: Default::default(),
+		rpc: RpcConfiguration {
+			addr: Default::default(),
+			max_request_size: Default::default(),
+			max_response_size: Default::default(),
+			max_connections: Default::default(),
+			cors: None,
+			methods: Default::default(),
+			id_provider: None,
+			max_subs_per_conn: Default::default(),
+			port: 9944,
+			message_buffer_capacity: Default::default(),
+			batch_config: RpcBatchRequestConfig::Unlimited,
+			rate_limit: None,
+			rate_limit_whitelisted_ips: Default::default(),
+			rate_limit_trust_proxy_headers: Default::default(),
+		},
 		prometheus_config: None,
 		telemetry_endpoints: None,
-		default_heap_pages: None,
 		offchain_worker: Default::default(),
 		force_authoring: false,
 		disable_grandpa: false,
 		dev_key_seed: Some(key_seed),
 		tracing_targets: None,
 		tracing_receiver: Default::default(),
-		max_runtime_instances: 8,
-		runtime_cache_size: 2,
 		announce_block: true,
 		data_path: root,
 		base_path,

prdoc/pr_5364.prdoc

@@ -0,0 +1,39 @@
+title: Improve `sc-service` API
+
+doc:
+  - audience: Node Dev
+    description: |
+      This improves `sc-service` API by not requiring the whole `&Configuration`, using specific configuration options
+      instead. `RpcConfiguration` and `ExecutorConfiguration` were also extracted from `Configuration` to group all RPC
+      and executor options together.
+      If `sc-service` is used as a library with lower-level APIs, `Configuration` can now be avoided in most cases.
+
+      This mainly impacts you on your node implementation. There you need to change this:
+      ```
+      with_execution_method(config.wasm_method)
+      ```
+
+      to this:
+      ```
+      with_execution_method(config.executor.wasm_method)
+      ```
+
+      There are similar changes required as well, but all are around the initialization of the wasm executor.
+
+crates:
+  - name: sc-service
+    bump: major
+  - name: sc-network-common
+    bump: patch
+  - name: sc-cli
+    bump: major
+  - name: polkadot-service
+    bump: patch
+  - name: cumulus-relay-chain-minimal-node
+    bump: none
+  - name: polkadot-parachain-bin
+    bump: major
+  - name: polkadot-parachain-lib
+    bump: major
+  - name: staging-node-inspect
+    bump: major

substrate/bin/node/cli/benches/block_production.rs

@@ -22,6 +22,7 @@ use criterion::{criterion_group, criterion_main, BatchSize, Criterion, Throughpu
 
 use kitchensink_runtime::{constants::currency::*, BalancesCall};
 use node_cli::service::{create_extrinsic, FullClient};
+use polkadot_sdk::sc_service::config::{ExecutorConfiguration, RpcConfiguration};
 use sc_block_builder::{BlockBuilderBuilder, BuiltBlock};
 use sc_consensus::{
 	block_import::{BlockImportParams, ForkChoiceStrategy},
@@ -73,34 +74,36 @@ fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
 		state_pruning: Some(PruningMode::ArchiveAll),
 		blocks_pruning: BlocksPruning::KeepAll,
 		chain_spec: spec,
-		wasm_method: WasmExecutionMethod::Compiled {
-			instantiation_strategy: WasmtimeInstantiationStrategy::PoolingCopyOnWrite,
+		executor: ExecutorConfiguration {
+			wasm_method: WasmExecutionMethod::Compiled {
+				instantiation_strategy: WasmtimeInstantiationStrategy::PoolingCopyOnWrite,
+			},
+			..ExecutorConfiguration::default()
+		},
+		rpc: RpcConfiguration {
+			addr: None,
+			max_connections: Default::default(),
+			cors: None,
+			methods: Default::default(),
+			max_request_size: Default::default(),
+			max_response_size: Default::default(),
+			id_provider: Default::default(),
+			max_subs_per_conn: Default::default(),
+			port: 9944,
+			message_buffer_capacity: Default::default(),
+			batch_config: RpcBatchRequestConfig::Unlimited,
+			rate_limit: None,
+			rate_limit_whitelisted_ips: Default::default(),
+			rate_limit_trust_proxy_headers: Default::default(),
 		},
-		rpc_addr: None,
-		rpc_max_connections: Default::default(),
-		rpc_cors: None,
-		rpc_methods: Default::default(),
-		rpc_max_request_size: Default::default(),
-		rpc_max_response_size: Default::default(),
-		rpc_id_provider: Default::default(),
-		rpc_max_subs_per_conn: Default::default(),
-		rpc_port: 9944,
-		rpc_message_buffer_capacity: Default::default(),
-		rpc_batch_config: RpcBatchRequestConfig::Unlimited,
-		rpc_rate_limit: None,
-		rpc_rate_limit_whitelisted_ips: Default::default(),
-		rpc_rate_limit_trust_proxy_headers: Default::default(),
 		prometheus_config: None,
 		telemetry_endpoints: None,
-		default_heap_pages: None,
 		offchain_worker: OffchainWorkerConfig { enabled: true, indexing_enabled: false },
 		force_authoring: false,
 		disable_grandpa: false,
 		dev_key_seed: Some(Sr25519Keyring::Alice.to_seed()),
 		tracing_targets: None,
 		tracing_receiver: Default::default(),
-		max_runtime_instances: 8,
-		runtime_cache_size: 2,
 		announce_block: true,
 		data_path: base_path.path().into(),
 		base_path,

substrate/bin/node/cli/benches/transaction_pool.rs

@@ -24,6 +24,7 @@ use futures::{future, StreamExt};
 use kitchensink_runtime::{constants::currency::*, BalancesCall, SudoCall};
 use node_cli::service::{create_extrinsic, fetch_nonce, FullClient, TransactionPool};
 use node_primitives::AccountId;
+use polkadot_sdk::sc_service::config::{ExecutorConfiguration, RpcConfiguration};
 use sc_service::{
 	config::{
 		BlocksPruning, DatabaseSource, KeystoreConfig, NetworkConfiguration, OffchainWorkerConfig,
@@ -70,32 +71,31 @@ fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
 		state_pruning: Some(PruningMode::ArchiveAll),
 		blocks_pruning: BlocksPruning::KeepAll,
 		chain_spec: spec,
-		wasm_method: Default::default(),
-		rpc_addr: None,
-		rpc_max_connections: Default::default(),
-		rpc_cors: None,
-		rpc_methods: Default::default(),
-		rpc_max_request_size: Default::default(),
-		rpc_max_response_size: Default::default(),
-		rpc_id_provider: Default::default(),
-		rpc_max_subs_per_conn: Default::default(),
-		rpc_port: 9944,
-		rpc_message_buffer_capacity: Default::default(),
-		rpc_batch_config: RpcBatchRequestConfig::Unlimited,
-		rpc_rate_limit: None,
-		rpc_rate_limit_whitelisted_ips: Default::default(),
-		rpc_rate_limit_trust_proxy_headers: Default::default(),
+		executor: ExecutorConfiguration::default(),
+		rpc: RpcConfiguration {
+			addr: None,
+			max_connections: Default::default(),
+			cors: None,
+			methods: Default::default(),
+			max_request_size: Default::default(),
+			max_response_size: Default::default(),
+			id_provider: Default::default(),
+			max_subs_per_conn: Default::default(),
+			port: 9944,
+			message_buffer_capacity: Default::default(),
+			batch_config: RpcBatchRequestConfig::Unlimited,
+			rate_limit: None,
+			rate_limit_whitelisted_ips: Default::default(),
+			rate_limit_trust_proxy_headers: Default::default(),
+		},
 		prometheus_config: None,
 		telemetry_endpoints: None,
-		default_heap_pages: None,
 		offchain_worker: OffchainWorkerConfig { enabled: true, indexing_enabled: false },
 		force_authoring: false,
 		disable_grandpa: false,
 		dev_key_seed: Some(Sr25519Keyring::Alice.to_seed()),
 		tracing_targets: None,
 		tracing_receiver: Default::default(),
-		max_runtime_instances: 8,
-		runtime_cache_size: 2,
 		announce_block: true,
 		data_path: base_path.path().into(),
 		base_path,

substrate/bin/node/cli/src/service.rs

@@ -209,7 +209,7 @@ pub fn new_partial(
 		})
 		.transpose()?;
 
-	let executor = sc_service::new_wasm_executor(&config);
+	let executor = sc_service::new_wasm_executor(&config.executor);
 
 	let (client, backend, keystore_container, task_manager) =
 		sc_service::new_full_parts::<Block, RuntimeApi, _>(
@@ -406,7 +406,7 @@ pub fn new_full_base<N: NetworkBackend<Block, <Block as BlockT>::Hash>>(
 	),
 ) -> Result<NewFullBase, ServiceError> {
 	let is_offchain_indexing_enabled = config.offchain_worker.indexing_enabled;
-	let role = config.role.clone();
+	let role = config.role;
 	let force_authoring = config.force_authoring;
 	let backoff_authoring_blocks =
 		Some(sc_consensus_slots::BackoffAuthoringOnFinalizedHeadLagging::default());
@@ -719,7 +719,7 @@ pub fn new_full_base<N: NetworkBackend<Block, <Block as BlockT>::Hash>>(
 		name: Some(name),
 		observer_enabled: false,
 		keystore,
-		local_role: role.clone(),
+		local_role: role,
 		telemetry: telemetry.as_ref().map(|x| x.handle()),
 		protocol_name: grandpa_protocol_name,
 	};

substrate/bin/node/inspect/src/command.rs

@@ -36,7 +36,7 @@ impl InspectCmd {
 		B: Block,
 		RA: Send + Sync + 'static,
 	{
-		let executor = sc_service::new_wasm_executor::<HostFunctions>(&config);
+		let executor = sc_service::new_wasm_executor::<HostFunctions>(&config.executor);
 		let client = sc_service::new_full_client::<B, RA, _>(&config, None, executor)?;
 		let inspect = Inspector::<B>::new(client);
 

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

@@ -304,7 +304,17 @@ impl CliConfiguration for RunCmd {
 	}
 
 	fn network_params(&self) -> Option<&NetworkParams> {
-		Some(&self.network_params)
+		let network_params = &self.network_params;
+		let is_authority = self.role(self.is_dev().ok()?).ok()?.is_authority();
+		if is_authority && network_params.public_addr.is_empty() {
+			eprintln!(
+				"WARNING: No public address specified, validator node may not be reachable.
+				Consider setting `--public-addr` to the public IP address of this node.
+				This will become a hard requirement in future versions."
+			);
+		}
+
+		Some(network_params)
 	}
 
 	fn keystore_params(&self) -> Option<&KeystoreParams> {

substrate/client/cli/src/lib.rs

@@ -25,6 +25,7 @@
 #![warn(unused_imports)]
 
 use clap::{CommandFactory, FromArgMatches, Parser};
+use log::warn;
 use sc_service::Configuration;
 
 pub mod arg_enums;
@@ -242,7 +243,10 @@ pub trait SubstrateCli: Sized {
 
 		let config = command.create_configuration(self, tokio_runtime.handle().clone())?;
 
-		command.init(&Self::support_url(), &Self::impl_version(), logger_hook, &config)?;
+		command.init(&Self::support_url(), &Self::impl_version(), |logger_builder| {
+			logger_hook(logger_builder, &config)
+		})?;
+
 		Runner::new(config, tokio_runtime, signals)
 	}
 }

substrate/client/cli/src/params/node_key_params.rs

@@ -237,7 +237,6 @@ mod tests {
 				|params| {
 					let dir = PathBuf::from(net_config_dir.clone());
 					let typ = params.node_key_type;
-					let role = role.clone();
 					params.node_key(net_config_dir, role, is_dev).and_then(move |c| match c {
 						NodeKeyConfig::Ed25519(sc_network::config::Secret::File(ref f))
 							if typ == NodeKeyType::Ed25519 &&