Transaction pool wastes CPU

[arkpar]

I've noticed that when doing full sync, all transaction from all imported blocks are re-validated by the transaction pool after the block has been imported. This happens here:

substrate/client/transaction-pool/src/graph/pool.rs

Line 277 in 9772209

.validate_transaction(parent, TransactionSource::InBlock, extrinsic.clone())

This wastes a lot of resources and hurts sync performance. Transaction pool should only re-validate transactions that are actually in the pool, shouldn't it?. Or at least skip the whole thing when the pool is empty.

[bkchr]

Transaction pool should not be called while doing a full/major sync as we don't send block import notifications?

[arkpar]

It looks like finality events are still being sent when GRANDPA imports a justification during major sync. Transaction pool then computes a tree route to the block from the previous notification and processes all blocks.

[bkchr]

Ahh! Ty for the report.

@michalkucharczyk could you please fix this? :D

[michalkucharczyk]

The other solution could be skipping notifications for txpool while the major sync is ongoing:

substrate/client/transaction-pool/src/lib.rs

Lines 770 to 782 in 1f0865c

	pub async fn notification_future<Client, Pool, Block>(client: Arc<Client>, txpool: Arc<Pool>)
	where
	Block: BlockT,
	Client: sc_client_api::BlockchainEvents<Block>,
	Pool: MaintainedTransactionPool<Block = Block>,
	{
	let import_stream = client
	.import_notification_stream()
	.filter_map(|n| ready(n.try_into().ok()))
	.fuse();
	let finality_stream = client.finality_notification_stream().map(Into::into).fuse();
	futures::stream::select(import_stream, finality_stream)

This would require passing the sync_oracle to the spawn_task:

substrate/client/service/src/builder.rs

Line 412 in 1f0865c

pub fn spawn_tasks<TBl, TBackend, TExPool, TRpc, TCl>(

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

Lines 385 to 397 in 1f0865c

	let rpc_handlers = sc_service::spawn_tasks(sc_service::SpawnTasksParams {
	config,
	backend,
	client: client.clone(),
	keystore: keystore_container.sync_keystore(),
	network: network.clone(),
	rpc_builder: Box::new(rpc_builder),
	transaction_pool: transaction_pool.clone(),
	task_manager: &mut task_manager,
	system_rpc_tx,
	tx_handler_controller,
	telemetry: telemetry.as_mut(),
	})?;

[michalkucharczyk]

@arkpar looking at your #12973 (comment), does the above proposal (#12903 (comment)) make sense to you?

[arkpar]

@arkpar looking at your #12973 (comment), does the above proposal (#12903 (comment)) make sense to you?

Without fixing the tree route issue, it will only make things worse. Now, at least, you get at most few hundred blocks between notifications because of how grandpa sends them when finalizing. If you stop processing them at all when syncing, as I've said in the mentioned comment, you can potentially receive notification for block that's a million blocks later than the the previous notification.