Trappist node refactor

[hbulgarini]

Trappist node refactor

As it was decided, we will refactor the node build setup, specifically over the service.rs + command.rs files. In a nutshell we will get rid of the current runtime build features with-stout-runtime and with-trappist-runtime and compile all native runtimes into one and only binary trappist-collator.

Why we are not convinced with the current runtime build features setup?

• The compilation time is the double since the two collators need to be built from scratch for each runtime. Naturally, the current usage of build features forces to build two binaries since build features are used to establish conditional exclusive build.
• The current functions at the service file do not contain the necessary generics duplicating the exact same code but with different build features. Here you have an example: the trappist start_node_impl and the stout start_node_impl. Another example is how we import the chain_specs : https://github.com/paritytech/trappist/blob/main/node/cli/src/command.rs#L65

Refactor proposal

The proposal is to basically remove runtime build features and merge all the runtimes into one single collator file (trappist-collator). Also remove the cli and service directories and comeback to the command.rs and service.rs files, matching with the parachain template and common good parachains.

There are some tweaks that need to be done on the service file like for example extending the types for the start_node_impl fn as:

#[sc_tracing::logging::prefix_logs_with("Parachain")]
async fn start_node_impl<RuntimeApi, RB, BIQ, BIC>(
	parachain_config: Configuration,
	polkadot_config: Configuration,
	collator_options: CollatorOptions,
	para_id: ParaId,
	_rpc_ext_builder: RB,
	build_import_queue: BIQ,
	build_consensus: BIC,
	hwbench: Option<sc_sysinfo::HwBench>,
) -> sc_service::error::Result<(TaskManager, Arc<ParachainClient<RuntimeApi>>)>
where
	RuntimeApi: ConstructRuntimeApi<Block, ParachainClient<RuntimeApi>> + Send + Sync + 'static,
	RuntimeApi::RuntimeApi: sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block>
		+ sp_api::Metadata<Block>
		+ sp_session::SessionKeys<Block>
		+ sp_api::ApiExt<
			Block,
			StateBackend = sc_client_api::StateBackendFor<ParachainBackend, Block>,
		> + sp_offchain::OffchainWorkerApi<Block>
		+ sp_block_builder::BlockBuilder<Block>
		+ cumulus_primitives_core::CollectCollationInfo<Block>
		+ pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi<Block, Balance>
		+ frame_rpc_system::AccountNonceApi<Block, AccountId, Nonce>,
	sc_client_api::StateBackendFor<ParachainBackend, Block>: sp_api::StateBackend<BlakeTwo256>,
	RB: Fn(Arc<ParachainClient<RuntimeApi>>) -> Result<jsonrpsee::RpcModule<()>, sc_service::Error>,
	BIQ: FnOnce(
		Arc<ParachainClient<RuntimeApi>>,
		ParachainBlockImport<RuntimeApi>,
		&Configuration,
		Option<TelemetryHandle>,
		&TaskManager,
	) -> Result<
		sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
		sc_service::Error,
	>,
	BIC: FnOnce(
		Arc<ParachainClient<RuntimeApi>>,
		ParachainBlockImport<RuntimeApi>,
		Option<&Registry>,
		Option<TelemetryHandle>,
		&TaskManager,
		Arc<dyn RelayChainInterface>,
		Arc<sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>>,
		Arc<SyncingService<Block>>,
		SyncCryptoStorePtr,
		bool,
	) -> Result<Box<dyn ParachainConsensus<Block>>, sc_service::Error>,
{ 
    ...

Also the command.rs need some additional code like

#[derive(Debug, PartialEq, Default)]
enum Runtime {
	/// This is the default runtime (actually based on rococo)
	#[default]
	Default,
	Shell,
	Seedling,
	Statemint,
    ...
}

macro_rules! construct_benchmark_partials {
	($config:expr, |$partials:ident| $code:expr) => {
		match $config.chain_spec.runtime() {
			Runtime::Statemine => {
				let $partials = new_partial::<statemine_runtime::RuntimeApi, _>(
					&$config,
					crate::service::aura_build_import_queue::<_, AuraId>,
				)?;
				$code
			},
			Runtime::Westmint => {
				let $partials = new_partial::<westmint_runtime::RuntimeApi, _>(
					&$config,
					crate::service::aura_build_import_queue::<_, AuraId>,
				)?;
				$code
			},
...

macro_rules! construct_async_run {
    	(|$components:ident, $cli:ident, $cmd:ident, $config:ident| $( $code:tt )* ) => {{
		let runner = $cli.create_runner($cmd)?;
		match runner.config().chain_spec.runtime() {
			Runtime::Westmint => {
				runner.async_run(|$config| {
					let $components = new_partial::<westmint_runtime::RuntimeApi, _>(
						&$config,
						crate::service::aura_build_import_queue::<_, AuraId>,
					)?;
					let task_manager = $components.task_manager;
					{ $( $code )* }.map(|v| (v, task_manager))
				})
			},
			Runtime::Statemine => {
				runner.async_run(|$config| {
					let $components = new_partial::<statemine_runtime::RuntimeApi, _>(
						&$config,
						crate::service::aura_build_import_queue::<_, AuraId>,
					)?;
					let task_manager = $components.task_manager;
					{ $( $code )* }.map(|v| (v, task_manager))
				})
			},

TODO: After we achieve this we can implement the build features with the right set up.

[hbulgarini]

I'm encountering an issue, which has led me to understand why there's a certain amount of duplication in the current code of the node. Let's delve into the composition of the service file: Essentially, there are three functions that come into play when the commands for starting the node are invoked (command.rs):

new_partial<RuntimeApi, BIQ>
start_node_impl<RuntimeApi, RB, BIQ, BIC>
start_generic_aura_node<RuntimeApi, AuraId: AppKey>

The challenge I'm currently encountering is that Stout and Trappist have different RuntimeApi configurations. This difference is due to the runtime APIs for the DEX that exist on Trappist but not on Stout.

If we examine how the RuntimeApi generics are set for the new_partial function, we can observe the following bounds:

pub fn new_partial<RuntimeApi, BIQ>(
	config: &Configuration,
	build_import_queue: BIQ,
) -> Result<
	PartialComponents<
		ParachainClient<RuntimeApi>,
		ParachainBackend,
		(),
		sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
		sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>,
		(ParachainBlockImport<RuntimeApi>, Option<Telemetry>, Option<TelemetryWorkerHandle>),
	>,
	sc_service::Error,
>
where
	RuntimeApi: ConstructRuntimeApi<Block, ParachainClient<RuntimeApi>> + Send + Sync + 'static,
	RuntimeApi::RuntimeApi: sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block>
		+ sp_api::Metadata<Block>
		+ sp_session::SessionKeys<Block>
		+ sp_api::ApiExt<
			Block,
			StateBackend = sc_client_api::StateBackendFor<ParachainBackend, Block>,
		> + sp_offchain::OffchainWorkerApi<Block>
		+ sp_block_builder::BlockBuilder<Block>,
	sc_client_api::StateBackendFor<ParachainBackend, Block>: sp_api::StateBackend<BlakeTwo256>,
	BIQ: FnOnce(
		Arc<ParachainClient<RuntimeApi>>,
		ParachainBlockImport<RuntimeApi>,
		&Configuration,
		Option<TelemetryHandle>,
		&TaskManager,
	) -> Result<
		sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
		sc_service::Error,
	>,

The specific part I want to highlight is RuntimeApi::RuntimeApi: ..... As in Trappist, the RPC requires the pallet_dex_rpc::DexRuntimeApi (link), this leads to the three functions new_partial, start_node_impl, and start_generic_aura_node needing to bind this DexRuntimeApi as well. This results in the following RuntimeApi::RuntimeApi:

	RuntimeApi::RuntimeApi: sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block>
		+ sp_api::Metadata<Block>
		+ sp_session::SessionKeys<Block>
		+ sp_api::ApiExt<
			Block,
			StateBackend = sc_client_api::StateBackendFor<ParachainBackend, Block>,
		> + sp_offchain::OffchainWorkerApi<Block>
		+ sp_block_builder::BlockBuilder<Block>
		+ cumulus_primitives_core::CollectCollationInfo<Block>
		+ pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi<Block, Balance>
		+ frame_rpc_system::AccountNonceApi<Block, AccountId, Nonce>
		+ pallet_dex_rpc::DexRuntimeApi<Block, AssetId, Balance, Balance>,

It's important to note the sequence of functions where the RPC is built, specifically at this location (link):

async fn start_node_impl<RuntimeApi, RB, BIQ, BIC>(
...
	let rpc_builder = {
		let client = client.clone();
		let transaction_pool = transaction_pool.clone();

		let backend_for_rpc = backend.clone();
		Box::new(move |deny_unsafe, _| {
			let deps = rpc::FullDeps {
				client: client.clone(),
				pool: transaction_pool.clone(),
				deny_unsafe,
			};

			rpc::create_full(deps, backend_for_rpc.clone()).map_err(Into::into)
		})
	};
...
}

Now that some background has been covered, the issue I'm facing right now is that if I add the trait bound + pallet_dex_rpc::DexRuntimeApi<Block, AssetId, Balance, Balance>, the Stout build will fail because it doesn't implement the DexRuntimeApi trait (which is expected, of course).

There are a couple of options I'm considering to resolve this issue:

1. Create one service file per runtime. The two service files will look very similar, with the only difference being the Dex trait bound.
   On the positive side, we can customize each service file per runtime. In a sense, Cumulus is doing this with different functions for each runtime target (system parachain runtimes, shell, and penpal). On the negative side, there's a lot of duplicated code, and it's not trivial (at least to me) to abstract all this logic into one single piece of code that meets the needs of both.

2. Implement the DEX pallet and their runtime API on Stout.
   On the positive side, we'll only need one set of service functions. On the negative side, every change in the RPC of each runtime will have to be replicated. If we consider different parachain teams, it's expected that every runtime will contain the same runtime API since in a way a runtime for a Kusama parachain is a predecessor of a Polkadot parachain. What I don't like about this option is that we would be missing the chance to offer a very flexible and customized setup if we go with this option.

My preferred option is the option 1) but at least investing some time in thinking the best way to abstract behaviors and do not duplicate code.

What do you think @stiiifff @kalaninja @valentinfernandez1 ?

[hbulgarini]

I forgot to mention that in case we go for the option 1) we could create one create_full function per runtime in the node/rpc.rs file.

[hbulgarini]

One potential approach is to rely on generics runtime api. Here is an example for the new_partial function:

pub trait BaseRuntimeApi<RuntimeApi>:
	ConstructRuntimeApi<Block, ParachainClient<RuntimeApi>> + Send + Sync + 'static
where
	RuntimeApi: sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block>
		+ sp_api::Metadata<Block>
		+ sp_session::SessionKeys<Block>
		+ sp_api::ApiExt<
			Block,
			StateBackend = sc_client_api::StateBackendFor<ParachainBackend, Block>,
		> + sp_offchain::OffchainWorkerApi<Block>
		+ sp_block_builder::BlockBuilder<Block>,
	sc_client_api::StateBackendFor<ParachainBackend, Block>: sp_api::StateBackend<BlakeTwo256>,
{
}

pub trait DexRuntimeApi<RuntimeApi>: BaseRuntimeApi<RuntimeApi>
where
	RuntimeApi: pallet_dex_rpc::DexRuntimeApi<Block, AssetId, Balance, Balance>,
{
}

pub fn new_generic_partial<RuntimeApi: BaseRuntimeApi, BIQ>(
	config: &Configuration,
	build_import_queue: BIQ,
) -> Result<
	PartialComponents<
		ParachainClient<RuntimeApi>,
		ParachainBackend,
		(),
		sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
		sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>,
		(ParachainBlockImport<RuntimeApi>, Option<Telemetry>, Option<TelemetryWorkerHandle>),
	>,
	sc_service::Error,
> {
	let telemetry = config
		.telemetry_endpoints
		.clone()
		.filter(|x| !x.is_empty())
		.map(|endpoints| -> Result<_, sc_telemetry::Error> {
			let worker = TelemetryWorker::new(16)?;
			let telemetry = worker.handle().new_telemetry(endpoints);
			Ok((worker, telemetry))
		})
		.transpose()?;

	let executor = sc_executor::WasmExecutor::<HostFunctions>::new(
		config.wasm_method,
		config.default_heap_pages,
		config.max_runtime_instances,
		None,
		config.runtime_cache_size,
	);

	let (client, backend, keystore_container, task_manager) =
		sc_service::new_full_parts::<Block, RuntimeApi, _>(
			config,
			telemetry.as_ref().map(|(_, telemetry)| telemetry.handle()),
			executor,
		)?;
	let client = Arc::new(client);

	let telemetry_worker_handle = telemetry.as_ref().map(|(worker, _)| worker.handle());

	let telemetry = telemetry.map(|(worker, telemetry)| {
		task_manager.spawn_handle().spawn("telemetry", None, worker.run());
		telemetry
	});

	let transaction_pool = sc_transaction_pool::BasicPool::new_full(
		config.transaction_pool.clone(),
		config.role.is_authority().into(),
		config.prometheus_registry(),
		task_manager.spawn_essential_handle(),
		client.clone(),
	);

	let block_import = ParachainBlockImport::new(client.clone(), backend.clone());

	let import_queue = build_import_queue(
		client.clone(),
		block_import.clone(),
		config,
		telemetry.as_ref().map(|telemetry| telemetry.handle()),
		&task_manager,
	)?;

	let params = PartialComponents {
		backend,
		client,
		import_queue,
		keystore_container,
		task_manager,
		transaction_pool,
		select_chain: (),
		other: (block_import, telemetry, telemetry_worker_handle),
	};

	Ok(params)
}


pub fn new_partial<RuntimeApi: DexRuntimeApi, BIQ>(
	config: &Configuration,
	build_import_queue: BIQ,
) -> Result<
	PartialComponents<
		ParachainClient<RuntimeApi>,
		ParachainBackend,
		(),
		sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
		sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>,
		(ParachainBlockImport<RuntimeApi>, Option<Telemetry>, Option<TelemetryWorkerHandle>),
	>,
	sc_service::Error,
> {
	new_generic_partial::<RuntimeApi, BIQ>(config, build_import_queue)
}

pub fn new_stout_partial<RuntimeApi: BaseRuntimeApi, BIQ>(
	config: &Configuration,
	build_import_queue: BIQ,
) -> Result<
	PartialComponents<
		ParachainClient<RuntimeApi>,
		ParachainBackend,
		(),
		sc_consensus::DefaultImportQueue<Block, ParachainClient<RuntimeApi>>,
		sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>,
		(ParachainBlockImport<RuntimeApi>, Option<Telemetry>, Option<TelemetryWorkerHandle>),
	>,
	sc_service::Error,
> {
	new_generic_partial::<RuntimeApi, BIQ>(config, build_import_queue)
}

[valentinfernandez1]

So maybe this is a dumb question but why do we need the trait bound to pallet_dex in the first place?

[hbulgarini]

So maybe this is a dumb question but why do we need the trait bound to pallet_dex in the first place?

Because it's needed to make to work the following line in the rpc create_full function:

pub fn create_full<C, P, B>(
	deps: FullDeps<C, P>,
	backend: Arc<B>,
) -> Result<RpcExtension, Box<dyn std::error::Error + Send + Sync>>
where
....
	C::Api: pallet_dex_rpc::DexRuntimeApi<
		trappist_runtime::opaque::Block,
		trappist_runtime::AssetId,
		trappist_runtime::Balance,
		trappist_runtime::AssetBalance,
	>,
...
{ 
   ...
   module.merge(Dex::new(client).into_rpc())?;
   ...
}

Then the types are inferred in the following line from the service file:

https://github.com/paritytech/trappist/pull/206/files#diff-594440db8adfc5d1b37711309312bfa06b49e488309186ad062a896778cf9fbfR326