From 69c87f4da2fdc9d4dedf8e16866e11535db0678f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <info@kchr.de>
Date: Thu, 1 Jun 2023 18:42:33 +0100
Subject: [PATCH] sc-transaction-pool: Always use best block to check if we
 should skip enactment

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.
---
 Cargo.lock                                    |  1 -
 client/transaction-pool/Cargo.toml            |  1 -
 client/transaction-pool/api/src/lib.rs        | 14 ++++++++++
 .../transaction-pool/src/enactment_state.rs   | 28 +++++++++----------
 4 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index e37cc1cb52c90..8f62985d977f5 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -10163,7 +10163,6 @@ dependencies = [
  "futures-timer",
  "linked-hash-map",
  "log",
- "num-traits",
  "parity-scale-codec",
  "parking_lot 0.12.1",
  "sc-block-builder",
diff --git a/client/transaction-pool/Cargo.toml b/client/transaction-pool/Cargo.toml
index d6af6a9eabd20..83bb8bdbd5040 100644
--- a/client/transaction-pool/Cargo.toml
+++ b/client/transaction-pool/Cargo.toml
@@ -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"
diff --git a/client/transaction-pool/api/src/lib.rs b/client/transaction-pool/api/src/lib.rs
index eab9614cb8521..e7b3a9c5e16f8 100644
--- a/client/transaction-pool/api/src/lib.rs
+++ b/client/transaction-pool/api/src/lib.rs
@@ -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 {
diff --git a/client/transaction-pool/src/enactment_state.rs b/client/transaction-pool/src/enactment_state.rs
index ed6b3d186694f..85c572c127e84 100644
--- a/client/transaction-pool/src/enactment_state.rs
+++ b/client/transaction-pool/src/enactment_state.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
 		);