`BlockImport`-Interface

[mustermeiszer]

This issue is more a temperature check, whether there is a will to change the BlockImport interface.

Currently, the traits needed for a "client" are all - I did not check every trait, but most - taking &self as a parameter, with the sole conception of BlockImport that is taking &mut self introduced in the following PR: #3058.

As the default Substrate client is usually used as an Arc<Client>, taking just &self this is very convenient. But due to the API of BlockImport many generic usages of client need an HRTB of the kind of for<'r> &'r Client: BlockImport in order to use it behind an Arc<Client>, while some still hide it under an Arc<Mutex<Client>> in order to satisfy the traits API.

While developing FUDGE I tried to be generic at all points, in order to be flexible with possible alternative implementations of the core structs that satisfy backend, client and pool needs. But the API of BlockImport forces me to either use the restrictive HRTB or hide the generic client behind a Mutex, which is unnecessary as I know the client takes care of interior mutability itself.

So, my main point is:

• Is there a chance to change the API of BlockImport and basically enforce every implementation to take care of interior mutability

OR

• live with the fact that in order to use a generic client there is a need for a "restrictive" HRTB or an Mutex

[bkchr]

The client should have been written holding an internal state. Then we would not require this complicated trait implementation.

So to your question, use an internal Mutex.

[mustermeiszer]

That was not my question. The client is the Substrate

substrate/client/service/src/client/client.rs

Lines 98 to 120 in 940a458

	pub struct Client<B, E, Block, RA>
	where
	Block: BlockT,
	{
	backend: Arc<B>,
	executor: E,
	storage_notifications: StorageNotifications<Block>,
	import_notification_sinks: NotificationSinks<BlockImportNotification<Block>>,
	finality_notification_sinks: NotificationSinks<FinalityNotification<Block>>,
	// Collects auxiliary operations to be performed atomically together with
	// block import operations.
	import_actions: Mutex<Vec<OnImportAction<Block>>>,
	// Collects auxiliary operations to be performed atomically together with
	// block finalization operations.
	finality_actions: Mutex<Vec<OnFinalityAction<Block>>>,
	// Holds the block hash currently being imported. TODO: replace this with block queue.
	importing_block: RwLock<Option<Block::Hash>>,
	block_rules: BlockRules<Block>,
	execution_extensions: ExecutionExtensions<Block>,
	config: ClientConfig<Block>,
	telemetry: Option<TelemetryHandle>,
	_phantom: PhantomData<RA>,
	}

and the problem is the current definition of the BlockImport parameters taking &mut self, making it impossible to use it as an Arc<Client> unless using the annoying HRTB I mention above.

Sources:

• BlockImport:

  substrate/client/consensus/common/src/block_import.rs

  Lines 338 to 358 in 940a458

  	pub trait BlockImport<B: BlockT> {
  	/// The error type.
  	type Error: std::error::Error + Send + 'static;
  	/// The transaction type used by the backend.
  	type Transaction: Send + 'static;
  	/// Check block preconditions.
  	async fn check_block(
  	&mut self,
  	block: BlockCheckParams<B>,
  	) -> Result<ImportResult, Self::Error>;
  	/// Import a block.
  	///
  	/// Cached data can be accessed through the blockchain cache.
  	async fn import_block(
  	&mut self,
  	block: BlockImportParams<B, Self::Transaction>,
  	cache: HashMap<CacheKeyId, Vec<u8>>,
  	) -> Result<ImportResult, Self::Error>;
  	}

• HRTB:

  substrate/client/consensus/common/src/block_import.rs

  Line 391 in 940a458

  for<'r> &'r T: BlockImport<B, Error = E, Transaction = Transaction>,

[bkchr]

I know what you were speaking about.

I meant, that using Arc<Client> was wrong from my perspective. It should have been:

struct ClientInner {}

struct Client { inner: Arc<Mutex<ClientInner>> }

In the end the entire client object is a failure, one central component that combines all the things. We want to get rid off this anyway.

[mustermeiszer]

Totally agree on the above! Then I will close this and adapt once the refactor goes through. Thanks @bkchr!