sc-transaction-pool: Always use best block to check if we should skip enactment (#14285)

We will calculate the tree route always against the best block and thus, we also should use this one
to check if we should skip the checks.

Author: bkchr

Cargo.lock

@@ -19,7 +19,6 @@ futures = "0.3.21"
 futures-timer = "3.0.2"
 linked-hash-map = "0.5.4"
 log = "0.4.17"
-num-traits = "0.2.8"
 parking_lot = "0.12.1"
 serde = { version = "1.0.163", features = ["derive"] }
 thiserror = "1.0.30"

client/transaction-pool/Cargo.toml

@@ -311,6 +311,20 @@ pub enum ChainEvent<B: BlockT> {
 	},
 }
 
+impl<B: BlockT> ChainEvent<B> {
+	/// Returns the block hash associated to the event.
+	pub fn hash(&self) -> B::Hash {
+		match self {
+			Self::NewBestBlock { hash, .. } | Self::Finalized { hash, .. } => *hash,
+		}
+	}
+
+	/// Is `self == Self::Finalized`?
+	pub fn is_finalized(&self) -> bool {
+		matches!(self, Self::Finalized { .. })
+	}
+}
+
 /// Trait for transaction pool maintenance.
 #[async_trait]
 pub trait MaintainedTransactionPool: TransactionPool {

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

@@ -19,10 +19,9 @@
 //! Substrate transaction pool implementation.
 
 use crate::LOG_TARGET;
-use num_traits::CheckedSub;
 use sc_transaction_pool_api::ChainEvent;
 use sp_blockchain::TreeRoute;
-use sp_runtime::traits::{Block as BlockT, NumberFor};
+use sp_runtime::traits::{Block as BlockT, NumberFor, Saturating};
 
 /// The threshold since the last update where we will skip any maintenance for blocks.
 ///
@@ -101,17 +100,16 @@ where
 		TreeRouteF: Fn(Block::Hash, Block::Hash) -> Result<TreeRoute<Block>, String>,
 		BlockNumberF: Fn(Block::Hash) -> Result<Option<NumberFor<Block>>, String>,
 	{
-		let (new_hash, current_hash, finalized) = match event {
-			ChainEvent::NewBestBlock { hash, .. } => (*hash, self.recent_best_block, false),
-			ChainEvent::Finalized { hash, .. } => (*hash, self.recent_finalized_block, true),
-		};
+		let new_hash = event.hash();
+		let finalized = event.is_finalized();
 
 		// do not proceed with txpool maintain if block distance is to high
-		let skip_maintenance = match (hash_to_number(new_hash), hash_to_number(current_hash)) {
-			(Ok(Some(new)), Ok(Some(current))) =>
-				new.checked_sub(&current) > Some(SKIP_MAINTENANCE_THRESHOLD.into()),
-			_ => true,
-		};
+		let skip_maintenance =
+			match (hash_to_number(new_hash), hash_to_number(self.recent_best_block)) {
+				(Ok(Some(new)), Ok(Some(current))) =>
+					new.saturating_sub(current) > SKIP_MAINTENANCE_THRESHOLD.into(),
+				_ => true,
+			};
 
 		if skip_maintenance {
 			log::debug!(target: LOG_TARGET, "skip maintain: tree_route would be too long");
@@ -131,10 +129,10 @@ where
 
 		log::debug!(
 			target: LOG_TARGET,
-			"resolve hash:{:?} finalized:{:?} tree_route:{:?} best_block:{:?} finalized_block:{:?}",
-			new_hash,
-			finalized,
-			tree_route,
+			"resolve hash: {new_hash:?} finalized: {finalized:?} \
+			 tree_route: (common {:?}, last {:?}) best_block: {:?} finalized_block:{:?}",
+			tree_route.common_block(),
+			tree_route.last(),
 			self.recent_best_block,
 			self.recent_finalized_block
 		);