Enable importing new blocks as best in dev mode

[ahmadkaouk]

What does it do?

The recent update (#2470) to the Cumulus block import feature was intended to resolve the sibling issue. However, this update has had an impact on Block Import when used in dev mode. Specifically, the use of ParachainBlockImport to determine the best block based on the relay chain is not applicable in dev mode. In dev mode, where no relay chain is utilized, manually created blocks will not be automatically imported as the best blocks by default, impacting certain test use cases.

To address this, the proposed change is to utilize FrontierBlockImport in dev mode, enabling the import of newly created unfinalized blocks as the best blocks.

Also, we are disabling code coverage for TypeScript-based tests due to the slow launch of the node, which has caused it to be broken.

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

[github-actions]

Coverage generated "Wed Sep 13 09:16:46 UTC 2023":
https://d3ifz9vhxc2wtb.cloudfront.net/pulls/2483/html/index.html

Master coverage: 87.39%
Pull coverage:

[JoshOrndorff]

So what is the motivation for

1. Adding the frontier block import to dev service to begin with. (I see it is harmless, but is there some advantage to adding it?)
2. Having a separate new_partial_dev function?

If you like having the services completely separate like this, It may be useful to have the dev service in a separate crate (or at least separate file) as it was before they were unifed in #260

[ahmadkaouk]

So what is the motivation for

1. Adding the frontier block import to dev service to begin with. (I see it is harmless, but is there some advantage to adding it?)
2. Having a separate new_partial_dev function?

If you like having the services completely separate like this, It may be useful to have the dev service in a separate crate (or at least separate file) as it was before they were unifed in #260

So what is the motivation for

1. Adding the frontier block import to dev service to begin with. (I see it is harmless, but is there some advantage to adding it?)
2. Having a separate new_partial_dev function?

If you like having the services completely separate like this, It may be useful to have the dev service in a separate crate (or at least separate file) as it was before they were unifed in #260

@JoshOrndorff, thanks for the comments.

1. I've updated the PR description to provide the motivation for this modification. In development mode, the current usage of ParachainBlockImport, which selects the best block based on the relay chain, is not functioning correctly due to the absence of a relay chain. The suggested resolution is to primarily use Cumulus ParachainBlockImport, but switch to FrontierBlockImport when in development mode.

2. To facilitate the above change, it's necessary to construct the PartialComponents with FrontierBlockImport in development mode and use ParachainBlockImport in all other instances. We're using new_partial to create PartialComponents with ParachainBlockImport, while new_partial_dev (which is called from new_dev) is used to build PartialComponents with the FrontierBlockImport.

[notlesh]

Dumb question:

#2470 put the FrontierBlockImport inside the ParachainBlockImport. Could it be done the other way, and would that avoid the issues it introduced?

In other words, change:

ParachainBlockImport where I: FrontierBlockImport

to:

FrontierBlockImport where I: ParachainBlockImport

[JoshOrndorff]

Okay, thanks for the context, I've commented on #2470 (review).

You are correct to want to fix this problem, but you are taking a much too heavy-handed approach in my opinion. Rather than creating an entire separate function, which reverses the effects of #260, I recommend you use the existing new_partial function and its existing dev_service parameter to determine whether the ParachainBlockImport should be included in the block import pipeline or not. If it makes the types easier, you can absolutely do what @notlesh suggested and change the ordering of the import pipeline.

Look at MaybeSelectChain as an example to copy if things really get complex, but hopefully it won't be quite so complex with this one.

All just my 2c. Nice to meet you @ahmadkaouk 👋

[JoshOrndorff]

Also, the title of this PR should be more like: Remove Parachain Block import from dev service. That is what will fix the problem, removing the parachain one, not anything to do with Frontier.

To paint a picture here, you are constructing a "block import pipeline" dynamically when the node starts based on stuff like chain spec and cli flags. You will need to include some pieces in the pipeline in some cases but not others.

Service	FrontierBlockImport	ParachainBlockImport
Dev Service	Could use it, or not, probably doesn't matter much.	MUST NOT use
Full Service	Should use it according to findings in #2470.	MUST use

Order does not matter in this case and in most cases in the Polkadot ecosystem. But in theory it could in some cases.

You guys should have a learning session about block import pipeline. It is a little understood topic. I wrote docs about it in c 2019 and they still kind of exist https://develop--substrate-docs.netlify.app/v3/advanced/block-import/#block-import-pipeline but got edited by someone without proper knowledge in the meantime.

[JoshOrndorff]

Okay, I can't stop thinking about this, so I'll present you with one more design option.

/// A Block import that applies the Parachain logic (or not) depending on
/// the state of an internal bool set at the time when the node runs
pub struct MaybeParachainBlockImport<I: inner>(bool);

// pseudo code
impl BlockImport for MaybeParachainBlockImport<I> {
  fn import_block() {
    if self.0 {
      // copy paste logic from parachain block import; it is only a few lines.
    }
    I::import_block()
  }
}

[ahmadkaouk]

Okay, I can't stop thinking about this, so I'll present you with one more design option.

/// A Block import that applies the Parachain logic (or not) depending on
/// the state of an internal bool set at the time when the node runs
pub struct MaybeParachainBlockImport<I: inner>(bool);

// pseudo code
impl BlockImport for MaybeParachainBlockImport<I> {
  fn import_block() {
    if self.0 {
      // copy paste logic from parachain block import; it is only a few lines.
    }
    I::import_block()
  }
}

I attempted the mentioned approach, but unfortunately, it doesn't work. The logic of ParachainBlockImport cannot be replicated due to the unexposed type of the monitor attribute. Moreover, a drawback is that any changes made to the logic in ParachainBlockImport would require manual importing of those changes.

However, this gave me an idea for an alternative design option. The main issue with using ParachainBlockImport is that it sets block_import_params.fork_choice to false when the block origin is not NetworkInitialSync (meaning the best block is determined by the relay chain).

But as we can have multiple levels of block imports, we can introduce a custom block import between ParachainBlockImport and FrontierBlockImport. This custom block import would solely change the fork_choice to custom (true) in dev_mode, ensuring that new blocks are imported as the best ones.

/// A Block import that is used to set the fork choice strategy to custom::true in dev mode. This
/// is used to allow to import blocks as new best blocks.
pub struct MaybeNewBestBlockImport<BI> {
	dev_service: bool,
	inner: BI,
}

#[async_trait::async_trait]
impl<BI> BlockImport<Block> for MaybeNewBestBlockImport<BI>
where
	BI: BlockImport<Block> + Send + Sync,
{
	type Error = BI::Error;
	type Transaction = BI::Transaction;

	async fn check_block(
		&mut self,
		block: sc_consensus::BlockCheckParams<Block>,
	) -> Result<sc_consensus::ImportResult, Self::Error> {
		self.inner.check_block(block).await
	}

	async fn import_block(
		&mut self,
		mut params: sc_consensus::BlockImportParams<Block, Self::Transaction>,
	) -> Result<sc_consensus::ImportResult, Self::Error> {
		if self.dev_service {
			params.fork_choice = Some(sc_consensus::ForkChoiceStrategy::Custom(true));
		}
		self.inner.import_block(params).await
	}
}

The new block import pipeline would be as follows:

ParachainBlockImport -> MaybeNewBestBlockImport -> FrontierBlockImport

It is important to note that the order of the first two levels matters, as they both modify the same parameter, fork_choice. However, the last change is the one that will be considered.

[JoshOrndorff]

This latest draft is certainly much better. It seems like you thought through a lot of important concepts related to block importing.

I think there is still room for improvement though. Consider the current pipeline in the dev service. Import Queue -> Your New Thing -> Parachain -> Frontier -> Client. Having the parachain piece there makes no sense and is incorrect as you observed and are trying to fix. Adding your custom thing overrides the incorrect parachain logic making the new behavior "good enough" for most dev service uses. However it would be better to just not have the parachain one in there in the first place.

Also I'm pretty sure (but lmk if I'm wrong) that your new implementation always sets the newest block to the best. It doesn't even use the longest chain rule or anything. So if I have ten blocks then a short fork at height 3, that would become the new best just because it is most recent.

[noandrea]

if the state is good enough, let's merge it as it is and we open a new one for further improvements.
The reason is that the CI is currently stuck because of test failures that this PR should fix

[ahmadkaouk]

The simplest solution I found is to have two pipelines:

• dev pipeline: frontier block import -> client
• parachain pipeline: parachain block import -> frontier block import -> client

To handle both cases using a single function new_partial, an enum is used to encapsulate the two types:

/// Block Import Pipeline used.
pub enum BlockImportPipeline<T, E> {
	/// Used in dev mode to import new blocks as best blocks.
	Dev(T),
	/// Used in parachain mode.
	Parachain(E),
}

In the new_partial function, the following logic is implemented:

	let (import_queue, block_import) = if dev_service {
		(
			nimbus_consensus::import_queue(
				client.clone(),
				frontier_block_import.clone(),
				create_inherent_data_providers,
				&task_manager.spawn_essential_handle(),
				config.prometheus_registry(),
				!dev_service,
			)?,
			BlockImportPipeline::Dev(frontier_block_import),
		)
	} else {
		let parachain_block_import =
			ParachainBlockImport::new(frontier_block_import, backend.clone());
		(
			nimbus_consensus::import_queue(
				client.clone(),
				parachain_block_import.clone(),
				create_inherent_data_providers,
				&task_manager.spawn_essential_handle(),
				config.prometheus_registry(),
				!dev_service,
			)?,
			BlockImportPipeline::Parachain(parachain_block_import),
		)
	};