txpool: don't validate block transactions if the pool is empty

[michalkucharczyk]

Fix shall prevent from wasting the CPU during the major sync. Block
transaction don't need to be re-validated when the txpool is empty.

Part of: #12903

[lexnv]

LGTM! 👍

[arkpar]

I'd prefer this check to be earlier in the event handling flow. This skips validation but there's still unnecessary work done to read potentially thousands of extrinsics from all blocks since the last notification into memory.

I think the root of the issue is the way the tree route is queried here:

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

Line 727 in cad470b

match self.api.tree_route(from, to) {

This code seems to calculate the route from a previous notification, expecting it to be short. In reality notifications are not guaranteed to be sent for all blocks. I.e. if a node goes offline for a few days, then reconnects and catches up the route may be huge. The fact that we send a finality notification on each session end even when doing a full sync is not something the tx pool should rely on. There should be a reasonable maximum of blocks that are queried here.

[michalkucharczyk]

I agree this solution is not a perfect solution of original problem. Please refer to: #12903 (comment) for my other proposal.

I.e. if a node goes offline for a few days, then reconnects and catches up the route may be huge.

• Just a thought - the local node being offline for long time, could have non-propagate transactions in its txpool. It could build a local fork. To handle this case correctly and notify txs' listeners, the complete tree route must be computed, and all transaction from enacted/retracted blocks should be processed. Not sure if this can be avoided.
• question: is there any indication of this state (catching up after being offline) in the client (sth like is_major_syncing)

[arkpar]

Just a thought - the local node being offline for long time, could have non-propagate transactions in its txpool. It could build a local fork. To handle this case correctly and notify txs' listeners, the complete tree route must be computed, and all transaction from enacted/retracted blocks should be processed. Not sure if this can be avoided.

The simplest solution I see is to simply clear the pool if the period between notifications is too large.

question: is there any indication of this state (catching up after being offline) in the client (sth like is_major_syncing)

IIRC major syncing state indicates not just the initial sync, but also covers this case.

[bkchr]

While this doesn't solve the issue properly as @arkpar mentioned, this is still a good optimization.

[bkchr]

bot merge

[paritytech-processbot]

Waiting for commit status.