EnactmentAction: all logic moved to EnactmentState

Tests to be done.

client/transaction-pool/Cargo.toml

@@ -19,6 +19,7 @@ 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.136", features = ["derive"] }
 thiserror = "1.0.30"

client/transaction-pool/src/enactment_state.rs

@@ -18,9 +18,12 @@
 
 //! Substrate transaction pool implementation.
 
+use num_traits::CheckedSub;
 use sc_transaction_pool_api::ChainEvent;
 use sp_blockchain::TreeRoute;
-use sp_runtime::traits::Block as BlockT;
+use sp_runtime::traits::{Block as BlockT, NumberFor};
+
+const SKIP_MAINTAINANCE_THRESHOLD: u16 = 20;
 
 /// Helper struct for keeping track of the current state of processed new best
 /// block and finalized events. The main purpose of keeping track of this state
@@ -54,6 +57,12 @@ where
 	recent_finalized_block: Block::Hash,
 }
 
+pub enum EnactmentAction<Block: BlockT> {
+	Skip,
+	HandleEnactment(TreeRoute<Block>),
+	HandleFinalization,
+}
+
 impl<Block> EnactmentState<Block>
 where
 	Block: BlockT,
@@ -68,32 +77,66 @@ where
 		self.recent_finalized_block
 	}
 
+	/// Returns the recently finalized block.
+	pub fn recent_best_block(&self) -> Block::Hash {
+		self.recent_best_block
+	}
+
 	/// Updates the state according to the given `ChainEvent`, returning
 	/// `Some(tree_route)` with a tree route including the blocks that need to
 	/// be enacted/retracted. If no enactment is needed then `None` is returned.
-	pub fn update<F>(
+	pub fn update<TreeRouteF, IsMajorSyncF, BlockNumberF>(
 		&mut self,
 		event: &ChainEvent<Block>,
-		tree_route: &F,
-	) -> Result<Option<TreeRoute<Block>>, String>
+		tree_route: &TreeRouteF,
+		is_major_syncing: &IsMajorSyncF,
+		hash_to_number: &BlockNumberF,
+	) -> Result<EnactmentAction<Block>, String>
 	where
-		F: Fn(Block::Hash, Block::Hash) -> Result<TreeRoute<Block>, String>,
+		TreeRouteF: Fn(Block::Hash, Block::Hash) -> Result<TreeRoute<Block>, String>,
+		IsMajorSyncF: Fn() -> bool,
+		BlockNumberF: Fn(Block::Hash) -> Result<Option<NumberFor<Block>>, String>,
 	{
+		if is_major_syncing() {
+			log::info!(target: "txpool", "skip: major");
+			self.force_update(event);
+			return Ok(EnactmentAction::Skip)
+		}
+
 		let (new_hash, finalized) = match event {
 			ChainEvent::NewBestBlock { hash, .. } => (*hash, false),
 			ChainEvent::Finalized { hash, .. } => (*hash, true),
 		};
 
+		let distance = match (hash_to_number(new_hash), hash_to_number(self.recent_best_block())) {
+			(Ok(Some(notified)), Ok(Some(current))) => notified.checked_sub(&current),
+			_ => None,
+		};
+
+		// do not process maintain txpool if node is out of sync
+		if Some(SKIP_MAINTAINANCE_THRESHOLD.into()) < distance {
+			log::info!(target: "txpool", "skip: long");
+			self.force_update(event);
+			return Ok(EnactmentAction::Skip)
+		}
+
 		// block was already finalized
 		if self.recent_finalized_block == new_hash {
 			log::debug!(target: "txpool", "handle_enactment: block already finalized");
-			return Ok(None)
+			return Ok(EnactmentAction::Skip) //Ok(None)
 		}
 
 		// compute actual tree route from best_block to notified block, and use
 		// it instead of tree_route provided with event
 		let tree_route = tree_route(self.recent_best_block, new_hash)?;
 
+		// do not process maintain txpool if node is out of sync
+		if tree_route.enacted().len() > SKIP_MAINTAINANCE_THRESHOLD.into() {
+			log::info!(target: "txpool", "skip: enacted too long");
+			self.force_update(event);
+			return Ok(EnactmentAction::Skip)
+		}
+
 		log::debug!(
 			target: "txpool",
 			"resolve hash:{:?} finalized:{:?} tree_route:{:?} best_block:{:?} finalized_block:{:?}",
@@ -109,7 +152,7 @@ where
 				"Recently finalized block {} would be retracted by ChainEvent {}, skipping",
 				self.recent_finalized_block, new_hash
 			);
-			return Ok(None)
+			return Ok(EnactmentAction::Skip) //Ok(None)
 		}
 
 		if finalized {
@@ -124,15 +167,15 @@ where
 					target: "txpool",
 					"handle_enactment: no newly enacted blocks since recent best block"
 				);
-				return Ok(None)
+				return Ok(EnactmentAction::HandleFinalization)
 			}
 
 			// otherwise enacted finalized block becomes best block...
 		}
 
 		self.recent_best_block = new_hash;
 
-		Ok(Some(tree_route))
+		Ok(EnactmentAction::HandleEnactment(tree_route))
 	}
 
 	/// Forces update of the state according to the given `ChainEvent`. Intended to be used as a
@@ -151,7 +194,7 @@ where
 
 #[cfg(test)]
 mod enactment_state_tests {
-	use super::EnactmentState;
+	use super::{EnactmentAction, EnactmentState};
 	use sc_transaction_pool_api::ChainEvent;
 	use sp_blockchain::{HashAndNumber, TreeRoute};
 	use std::sync::Arc;
@@ -395,8 +438,11 @@ mod enactment_state_tests {
 				},
 				&tree_route,
 			)
+			.map(|s| match s {
+				EnactmentAction::HandleEnactment(_) => true,
+				_ => false,
+			})
 			.unwrap()
-			.is_some()
 	}
 
 	fn trigger_finalized(
@@ -415,8 +461,11 @@ mod enactment_state_tests {
 
 		state
 			.update(&ChainEvent::Finalized { hash: acted_on, tree_route: v.into() }, &tree_route)
+			.map(|s| match s {
+				EnactmentAction::HandleEnactment(_) => true,
+				_ => false,
+			})
 			.unwrap()
-			.is_some()
 	}
 
 	fn assert_es_eq(

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

@@ -272,7 +272,7 @@ impl<B: ChainApi> Pool<B> {
 				// to get validity info and tags that the extrinsic provides.
 				None => {
 					// Avoid validating block txs if the pool is empty
-					if !self.validated_pool.status().is_empty() {
+					if true || !self.validated_pool.status().is_empty() {
 						let validity = self
 							.validated_pool
 							.api()
@@ -283,6 +283,8 @@ impl<B: ChainApi> Pool<B> {
 							)
 							.await;
 
+						log::trace!(target: "txpool", "validating {:?}", validity);
+
 						if let Ok(Ok(validity)) = validity {
 							future_tags.extend(validity.provides);
 						}

client/transaction-pool/src/lib.rs

@@ -33,7 +33,7 @@ mod tests;
 
 pub use crate::api::FullChainApi;
 use async_trait::async_trait;
-use enactment_state::EnactmentState;
+use enactment_state::{EnactmentAction, EnactmentState};
 use futures::{
 	channel::oneshot,
 	future::{self, ready},
@@ -591,11 +591,6 @@ where
 		let pool = self.pool.clone();
 		let api = self.api.clone();
 
-		// do not process maintain txpool if node is out of sync
-		if tree_route.enacted().len() > 20 {
-			return
-		}
-
 		let (hash, block_number) = match tree_route.last() {
 			Some(HashAndNumber { hash, number }) => (hash, number),
 			None => {
@@ -730,11 +725,6 @@ where
 	where
 		SO: sp_consensus::SyncOracle + std::marker::Send + std::marker::Sync + ?Sized,
 	{
-		if sync_oracle.is_major_syncing() {
-			self.enactment_state.lock().force_update(&event);
-			return
-		}
-
 		let prev_finalized_block = self.enactment_state.lock().recent_finalized_block();
 		let compute_tree_route = |from, to| -> Result<TreeRoute<Block>, String> {
 			match self.api.tree_route(from, to) {
@@ -746,15 +736,25 @@ where
 			}
 		};
 
-		let result = self.enactment_state.lock().update(&event, &compute_tree_route);
+		let is_major_syncing = || sync_oracle.is_major_syncing();
+		let block_id_to_number =
+			|hash| self.api.block_id_to_number(&BlockId::Hash(hash)).map_err(|e| format!("{}", e));
+
+		let result = self.enactment_state.lock().update(
+			&event,
+			&compute_tree_route,
+			&is_major_syncing,
+			&block_id_to_number,
+		);
 
 		match result {
 			Err(msg) => {
 				log::debug!(target: "txpool", "{msg}");
 				self.enactment_state.lock().force_update(&event);
 			},
-			Ok(None) => {},
-			Ok(Some(tree_route)) => {
+			Ok(EnactmentAction::Skip) => return,
+			Ok(EnactmentAction::HandleFinalization) => {},
+			Ok(EnactmentAction::HandleEnactment(tree_route)) => {
 				self.handle_enactment(tree_route).await;
 			},
 		};