Change Polkadot run_collator to start_collator

[bkchr]

Currently to start a Collator with Polkadot, we use run_collator. I think we should add a function start_collator, that takes almost the same arguments, but returns a future. I think it would make the setup stuff easier, when we are driving the Polkadot future/service and not the other way around (like it is done now). This also replicates more of the "default" model of starting a service in Substrate.

[cecton]

So to be clear. You would like to remove and replace the SetupParachain code here:

cumulus/test/parachain/src/service.rs

Lines 80 to 182 in 0efd15c

	pub fn run_collator<E: sc_service::ChainSpecExtension>(
	config: Configuration<GenesisConfig, E>,
	key: Arc<polkadot_primitives::parachain::CollatorPair>,
	polkadot_config: polkadot_collator::Configuration,
	) -> Result<(), sc_cli::error::Error> {
	let (builder, inherent_data_providers) = new_full_start!(config);
	inherent_data_providers
	.register_provider(sp_timestamp::InherentDataProvider)
	.unwrap();
	let service = builder
	.with_network_protocol(|_| Ok(NodeProtocol::new()))?
	.build()?;
	let proposer_factory = sc_basic_authorship::ProposerFactory {
	client: service.client(),
	transaction_pool: service.transaction_pool(),
	};
	let block_import = service.client();
	let setup_parachain = SetupParachain {
	service,
	inherent_data_providers,
	proposer_factory,
	block_import,
	};
	cumulus_collator::run_collator(
	setup_parachain,
	crate::PARA_ID,
	key,
	polkadot_config,
	)
	}
	struct SetupParachain<S, PF, BI> {
	service: S,
	proposer_factory: PF,
	inherent_data_providers: InherentDataProviders,
	block_import: BI,
	}
	type TransactionFor<E, Block> =
	<<E as Environment<Block>>::Proposer as Proposer<Block>>::Transaction;
	impl<S, PF, BI> cumulus_collator::SetupParachain<Block> for SetupParachain<S, PF, BI>
	where
	S: AbstractService,
	PF: Environment<Block> + Send + 'static,
	BI: BlockImport<Block, Error = sp_consensus::Error, Transaction = TransactionFor<PF, Block>>
	+ Send
	+ Sync
	+ 'static,
	{
	type ProposerFactory = PF;
	type BlockImport = BI;
	fn setup_parachain<P: cumulus_consensus::PolkadotClient, Spawner>(
	self,
	polkadot_client: P,
	spawner: Spawner,
	) -> Result<
	(
	Self::ProposerFactory,
	Self::BlockImport,
	InherentDataProviders,
	),
	String,
	>
	where
	Spawner: Spawn + Clone + Send + Sync + 'static,
	{
	let client = self.service.client();
	let follow =
	match cumulus_consensus::follow_polkadot(crate::PARA_ID, client, polkadot_client) {
	Ok(follow) => follow,
	Err(e) => {
	return Err(format!("Could not start following polkadot: {:?}", e));
	}
	};
	spawner
	.spawn_obj(
	Box::new(
	future::select(
	self.service
	.map_err(|e| error!("Parachain service error: {:?}", e)),
	follow,
	)
	.map(|_| ()),
	)
	.into(),
	)
	.map_err(|_| "Could not spawn parachain server!")?;
	Ok((
	self.proposer_factory,
	self.block_import,
	self.inherent_data_providers,
	))
	}
	}

But also the CollatorBuilder then?

cumulus/collator/src/lib.rs

Lines 259 to 346 in 0efd15c

	struct CollatorBuilder<Block, SP> {
	setup_parachain: SP,
	_marker: PhantomData<Block>,
	}
	impl<Block, SP> CollatorBuilder<Block, SP> {
	/// Create a new instance of self.
	fn new(setup_parachain: SP) -> Self {
	Self {
	setup_parachain,
	_marker: PhantomData,
	}
	}
	}
	impl<Block: BlockT, SP: SetupParachain<Block>> BuildParachainContext for CollatorBuilder<Block, SP>
	where
	<SP::ProposerFactory as Environment<Block>>::Proposer: Send,
	{
	type ParachainContext = Collator<Block, SP::ProposerFactory, SP::BlockImport>;
	fn build<B, E, R, Spawner, Extrinsic>(
	self,
	client: Arc<PolkadotClient<B, E, R>>,
	spawner: Spawner,
	network: Arc<dyn CollatorNetwork>,
	) -> Result<Self::ParachainContext, ()>
	where
	PolkadotClient<B, E, R>: sp_api::ProvideRuntimeApi<PBlock>,
	<PolkadotClient<B, E, R> as sp_api::ProvideRuntimeApi<PBlock>>::Api:
	polkadot_service::RuntimeApiCollection<Extrinsic>,
	E: sc_client::CallExecutor<PBlock> + Clone + Send + Sync + 'static,
	Spawner: Spawn + Clone + Send + Sync + 'static,
	Extrinsic: codec::Codec + Send + Sync + 'static,
	<<PolkadotClient<B, E, R> as sp_api::ProvideRuntimeApi<PBlock>>::Api as sp_api::ApiExt<
	PBlock,
	>>::StateBackend: sp_api::StateBackend<sp_core::Blake2Hasher>,
	R: Send + Sync + 'static,
	B: sc_client_api::Backend<PBlock> + 'static,
	// Rust bug: https://github.com/rust-lang/rust/issues/24159
	B::State: sp_api::StateBackend<sp_core::Blake2Hasher>,
	{
	let (proposer_factory, block_import, inherent_data_providers) = self
	.setup_parachain
	.setup_parachain(client, spawner)
	.map_err(|e| error!("Error setting up the parachain: {}", e))?;
	Ok(Collator::new(
	proposer_factory,
	inherent_data_providers,
	network,
	block_import,
	))
	}
	}
	/// Something that can setup a parachain.
	pub trait SetupParachain<Block: BlockT>: Send {
	/// The proposer factory of the parachain to build blocks.
	type ProposerFactory: Environment<Block> + Send + 'static;
	/// The block import for importing the blocks build by the collator.
	type BlockImport: BlockImport<
	Block,
	Error = ConsensusError,
	Transaction = <<Self::ProposerFactory as Environment<Block>>::Proposer as Proposer<
	Block,
	>>::Transaction,
	> + Send
	+ Sync
	+ 'static;
	/// Setup the parachain.
	fn setup_parachain<P, SP>(
	self,
	polkadot_client: P,
	spawner: SP,
	) -> Result<
	(
	Self::ProposerFactory,
	Self::BlockImport,
	InherentDataProviders,
	),
	String,
	>
	where
	P: cumulus_consensus::PolkadotClient,
	SP: Spawn + Clone + Send + Sync + 'static;
	}

And in exchange, having in polkadot a start_collator that will return a Future (or something that impl Future).

This is more or less what I did with the build_collator_service in polkadot (cecton-cumulus-branch):

https://github.com/paritytech/polkadot/blob/4f158e95c1a053f0fc8185cdbc8acded028613c1/collator/src/lib.rs#L276-L448

This returns a service which impl Future but it doesn't get rid of the whole BuildParachainContext part.

So all I need to do is to build the ParachainContext in cumulus instead of polkadot?

[bkchr]

I don't care how the Cumulus side will look like, as long as it easier to use and better reusable.

And I think we should have the following requirements:

1. We (the collator) drive the runtime, so not that Polkadot creates some runtime. For this I want to have the Future returned, as you already explained above.
2. ParachainContext will still need to be provided to the start_collator (maybe better name build_collator) function.

[bkchr]

I think this is done? @cecton

[cecton]

Yes! Looks like it has been solved by paritytech/polkadot#851