fatxpool: add fallback for ready at light

prdoc/pr_8533.prdoc

@@ -0,0 +1,8 @@
+title: '`fatxpool`:  add fallback for ready at light'
+doc:
+- audience: Node Dev 
+  description: |
+     Add fallback for `ready_at_light` for the case of not finding a best view that can be used to return a set of ready transactions. Optimised as well how the best view is searched.
+crates:
+- name: sc-transaction-pool
+  bump: major

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

@@ -474,7 +474,10 @@ where
 	///
 	/// The method attempts to build a temporary view and create an iterator of ready transactions
 	/// for a specific `at` hash. If a valid view is found, it collects and prunes
-	/// transactions already included in the blocks and returns the valid set.
+	/// transactions already included in the blocks and returns the valid set. Not finding a view
+	/// returns with the ready transaction set found in the most recent view processed by the
+	/// fork-aware txpool. Not being able to query for block number for the provided `at` block hash
+	/// results in returning an empty transaction set.
 	///
 	/// Pruning is just rebuilding the underlying transactions graph, no validations are executed,
 	/// so this process shall be fast.
@@ -487,35 +490,27 @@ where
 			"fatp::ready_at_light"
 		);
 
-		let Ok(block_number) = self.api.resolve_block_number(at) else {
-			return Box::new(std::iter::empty())
-		};
-
-		let best_result = {
-			api.tree_route(self.enactment_state.lock().recent_finalized_block(), at).map(
-				|tree_route| {
-					if let Some((index, view)) =
-						tree_route.enacted().iter().enumerate().rev().skip(1).find_map(|(i, b)| {
-							self.view_store.get_view_at(b.hash, true).map(|(view, _)| (i, view))
-						}) {
-						let e = tree_route.enacted()[index..].to_vec();
-						(TreeRoute::new(e, 0).ok(), Some(view))
-					} else {
-						(None, None)
-					}
-				},
-			)
-		};
-
-		if let Ok((Some(best_tree_route), Some(best_view))) = best_result {
-			let (tmp_view, _, _): (View<ChainApi>, _, _) =
-				View::new_from_other(&best_view, &HashAndNumber { hash: at, number: block_number });
-
+		let at_number = self.api.resolve_block_number(at).ok();
+		let finalized_number = self
+			.api
+			.resolve_block_number(self.enactment_state.lock().recent_finalized_block())
+			.ok();
+
+		// Prune all txs from the best view found, considering the extrinsics part of the blocks
+		// that are more recent than the view itself.
+		if let Some((view, enacted_blocks, at_hn)) = at_number.and_then(|at_number| {
+			let at_hn = HashAndNumber { hash: at, number: at_number };
+			finalized_number.and_then(|finalized_number| {
+				self.view_store
+					.find_view_descendent_up_to_number(&at_hn, finalized_number)
+					.map(|(view, enacted_blocks)| (view, enacted_blocks, at_hn))
+			})
+		}) {
+			let (tmp_view, _, _): (View<ChainApi>, _, _) = View::new_from_other(&view, &at_hn);
 			let mut all_extrinsics = vec![];
-
-			for h in best_tree_route.enacted() {
+			for h in enacted_blocks {
 				let extrinsics = api
-					.block_body(h.hash)
+					.block_body(h)
 					.await
 					.unwrap_or_else(|error| {
 						warn!(
@@ -546,14 +541,25 @@ where
 			debug!(
 				target: LOG_TARGET,
 				?at,
-				best_view_hash = ?best_view.at.hash,
+				best_view_hash = ?view.at.hash,
 				before_count,
 				to_be_removed = all_extrinsics.len(),
 				after_count,
 				duration = ?start.elapsed(),
 				"fatp::ready_at_light"
 			);
 			Box::new(tmp_view.pool.validated_pool().ready())
+		} else if let Some((most_recent_view, _)) = self
+			.view_store
+			.most_recent_view
+			.read()
+			.and_then(|at| self.view_store.get_view_at(at, true))
+		{
+			// Fallback for the case when `at` is not on the already known fork.
+			// Falls back to the most recent view, which may include txs which
+			// are invalid or already included in the blocks but can still yield a
+			// partially valid ready set, which is still better than including nothing.
+			Box::new(most_recent_view.pool.validated_pool().ready())
 		} else {
 			let empty: ReadyIteratorFor<ChainApi> = Box::new(std::iter::empty());
 			debug!(

substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs

@@ -209,14 +209,16 @@
 //! ### Light maintain
 //! The [maintain](#maintain) procedure can sometimes be quite heavy, and it may not be accomplished
 //! within the time window expected by the block builder. On top of that block builder may want to
-//! build few blocks in the raw, not giving the pool enough time to accomplish possible ongoing
+//! build few blocks in the row, not giving the pool enough time to accomplish possible ongoing
 //! maintain process.
 //!
 //! To address this, there is a [light version][`ready_at_light`] of the maintain procedure. It
-//! [finds the best view][find_best_view], clones it and prunes all the transactions that were
-//! included in enacted part of [tree route][`TreeRoute`] from the base view to the block at which a
-//! ready iterator was requested. No new [transaction validations][runtime_api::validate] are
-//! required to accomplish it.
+//! [finds the first descendent view][`find_view_descendent_up_to_number`] up to the recent
+//! finalized block, clones it and prunes all the transactions that were included in enacted part of
+//! the traversed route, from the base view to the block at which a ready iterator was requested. No
+//! new [transaction validations][runtime_api::validate] are required to accomplish it. If no view
+//! is found, it will return the ready transactions of the most recent view processed by the
+//! transaction pool.
 //!
 //! ### Providing ready transactions: `ready_at`
 //! The asynchronous [`ready_at`] function resolves to the [ready transactions
@@ -314,6 +316,7 @@
 //! [`ViewStore`]: crate::fork_aware_txpool::view_store::ViewStore
 //! [`finish_background_revalidations`]: crate::fork_aware_txpool::view_store::ViewStore::finish_background_revalidations
 //! [find_best_view]: crate::fork_aware_txpool::view_store::ViewStore::find_best_view
+//! [`find_view_descendent_up_to_number`]: crate::fork_aware_txpool::view_store::ViewStore::find_view_descendent_up_to_number
 //! [`active_views`]: crate::fork_aware_txpool::view_store::ViewStore::active_views
 //! [`inactive_views`]: crate::fork_aware_txpool::view_store::ViewStore::inactive_views
 //! [`TxMemPool`]: crate::fork_aware_txpool::tx_mem_pool::TxMemPool

substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs

@@ -40,7 +40,7 @@ use sc_transaction_pool_api::{
 use sp_blockchain::{HashAndNumber, TreeRoute};
 use sp_runtime::{
 	generic::BlockId,
-	traits::{Block as BlockT, Saturating},
+	traits::{Block as BlockT, Header, One, Saturating},
 	transaction_validity::{InvalidTransaction, TransactionValidityError},
 };
 use std::{
@@ -338,6 +338,39 @@ where
 		self.active_views.read().is_empty() && self.inactive_views.read().is_empty()
 	}
 
+	/// Searches in the view store for the first descendant view by iterating through the fork of
+	/// the `at` block, up to the provided `block_number`.
+	///
+	/// Returns with a maybe pair of a view and a set of enacted blocks when the first view is
+	/// found.
+	pub(super) fn find_view_descendent_up_to_number(
+		&self,
+		at: &HashAndNumber<Block>,
+		up_to: <<Block as BlockT>::Header as Header>::Number,
+	) -> Option<(Arc<View<ChainApi>>, Vec<Block::Hash>)> {
+		let mut enacted_blocks = Vec::new();
+		let mut at_hash = at.hash;
+		let mut at_number = at.number;
+
+		// Search for a view that can be used to get and return an approximate ready
+		// transaction set.
+		while at_number >= up_to {
+			// Found a view, stop searching.
+			if let Some((view, _)) = self.get_view_at(at_hash, true) {
+				return Some((view, enacted_blocks));
+			}
+
+			enacted_blocks.push(at_hash);
+
+			// Move up into the fork.
+			let header = self.api.block_header(at_hash).ok().flatten()?;
+			at_hash = *header.parent_hash();
+			at_number = at_number.saturating_sub(One::one());
+		}
+
+		None
+	}
+
 	/// Finds the best existing active view to clone from along the path.
 	///
 	/// ```text

substrate/client/transaction-pool/tests/fatp.rs

@@ -2328,12 +2328,80 @@ fn fatp_ready_light_long_fork_retracted_works() {
 	let header02b = api.push_block_with_parent(header01b.hash(), vec![xt1.clone()], true);
 	let header03b = api.push_block_with_parent(header02b.hash(), vec![xt2.clone()], true);
 
+	// Returns the most recent view (`header01a`) ready transactions set.
+	let ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap();
+	assert_eq!(ready_iterator.count(), 4);
+
+	let event = new_best_block_event(&pool, Some(header01a.hash()), header01b.hash());
+	block_on(pool.maintain(event));
+
 	let mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap();
+	let ready01 = ready_iterator.next();
+	assert_eq!(ready01.unwrap().hash, api.hash_and_length(&xt3).0);
+	let ready02 = ready_iterator.next();
+	assert_eq!(ready02.unwrap().hash, api.hash_and_length(&xt4).0);
 	assert!(ready_iterator.next().is_none());
+}
+
+#[test]
+fn fatp_ready_light_fallback_gets_triggered() {
+	sp_tracing::try_init_simple();
+
+	let (pool, api, _) = pool();
+	api.set_nonce(api.genesis_hash(), Bob.into(), 200);
+	api.set_nonce(api.genesis_hash(), Charlie.into(), 200);
+	api.set_nonce(api.genesis_hash(), Dave.into(), 200);
+	api.set_nonce(api.genesis_hash(), Eve.into(), 200);
+
+	let genesis = api.genesis_hash();
+
+	let xt0 = uxt(Alice, 200);
+	let xt1 = uxt(Bob, 200);
+	let xt2 = uxt(Charlie, 200);
+	let xt3 = uxt(Dave, 200);
+	let xt4 = uxt(Eve, 200);
+
+	let submissions = vec![pool.submit_at(genesis, SOURCE, vec![xt0.clone(), xt1.clone()])];
+	let results = block_on(futures::future::join_all(submissions));
+	assert!(results.iter().all(|r| { r.is_ok() }));
+
+	let header01a = api.push_block_with_parent(genesis, vec![xt4.clone()], true);
+	let event = new_best_block_event(&pool, Some(genesis), header01a.hash());
+	block_on(pool.maintain(event));
+
+	let header01b = api.push_block_with_parent(genesis, vec![xt0.clone()], true);
+	// Call `ready_at_light` at genesis direct descendent, even if not notified as best or
+	// finalized. Should still return ready txs based on the most recent view processed by the
+	// txpool.
+	let ready_iterator = pool.ready_at_light(header01b.hash()).now_or_never().unwrap();
+	assert_eq!(ready_iterator.count(), 2);
+
+	let header02b = api.push_block_with_parent(header01b.hash(), vec![xt1.clone()], true);
+	let header03b = api.push_block_with_parent(header02b.hash(), vec![xt2.clone()], true);
+
+	// Submit a few more txs to the pool.
+	let submissions = vec![pool.submit_at(
+		// `at` is ignored.
+		genesis,
+		SOURCE,
+		vec![xt2.clone(), xt3.clone()],
+	)];
+	let results = block_on(futures::future::join_all(submissions));
+	assert!(results.iter().all(|r| { r.is_ok() }));
+
+	// Calling `ready_at_light` now on the last block of a fork, with no block notified as best.
+	// We should still get the ready txs from the most recent view processed by the txpool,
+	// but now with a few more txs which were submitted previously.
+	let ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap();
+	assert_eq!(ready_iterator.count(), 4);
 
 	let event = new_best_block_event(&pool, Some(header01a.hash()), header01b.hash());
 	block_on(pool.maintain(event));
 
+	// Calling `ready_at_light` on the new best block (`header03b`) should consider its fork up to
+	// the finalized block for the search of the best view, and coincidentaly, that's the only view
+	// of the tree route, being the view created for NBB `header03b`. The returned ready txs are the
+	// ones left in the best view's pool after prunning the txs.
 	let mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap();
 	let ready01 = ready_iterator.next();
 	assert_eq!(ready01.unwrap().hash, api.hash_and_length(&xt3).0);