fatxpool: report_invalid: do not ban Future/Stale txs from re-entering the view

prdoc/pr_7777.prdoc

@@ -0,0 +1,20 @@
+title: '`fatxpool`: report_invalid: do not ban Future/Stale txs from re-entering the
+  view'
+doc:
+- audience: Node Dev
+  description: |-
+    #### Description
+
+    Avoid banning future/stale transactions reported as invalid by the authorship module.
+
+    #### Note for reviewers
+    When re-org is handled by transaction pool, the view for new fork (`Bn'`) is cloned from the tip of the other existing fork (`Bn`). The new view is not entirely re-validated during the maintain process (it will be revalidated in the background), so it may happen that it contains transactions that are ready on (`Bn`) but actually are not ready on (`Bn'`). All required (which are expected to be in retracted set) transactions are submitted to the new view, but order of txs in ready iterator is not updated.
+
+    The proper fix would require to re-build the the iterator - which is not trivial as we do not have tags for transactions for block `Bn'` yet. We could force retracted txs to be before ready transactions but it also does not feel to be a good solution - it still would be best effort trial.
+
+    For now allowing future transactions to re-enter the view immediately is in my opinion a good compromise. This PR is a quick fix and actually brings back behavior of txpool from before merging #6008. The bad thing is that incorrect transactions are detected during block authorship, but this situation to happen requires some specific pre-conditions which should be rare.
+
+    If this PR is not merged, some transaction will get included into blocks, only after [`DEFAULT_BAN_TIME_SECS`](https://github.com/paritytech/polkadot-sdk/blob/4b39ff00b887039bc3e02a763a29deb09df56833/substrate/client/transaction-pool/src/graph/rotator.rs#L37), which is pretty bad.
+crates:
+- name: sc-transaction-pool
+  bump: minor

substrate/client/transaction-pool/src/common/tracing_log_xt.rs

@@ -53,6 +53,18 @@ macro_rules! log_xt {
         }
     };
 }
+macro_rules! log_xt_debug {
+    (data: $datatype:ident, target: $target:expr, $($arg:tt)+) => {
+        $crate::common::tracing_log_xt::log_xt!(data: $datatype, target: $target, tracing::Level::DEBUG, $($arg)+);
+    };
+    (target: $target:expr, $tx_collection:expr, $text_with_format:expr) => {
+        $crate::common::tracing_log_xt::log_xt!(data: hash, target: $target, tracing::Level::DEBUG, $tx_collection, $text_with_format);
+    };
+    (target: $target:expr, $tx_collection:expr, $text_with_format:expr, $($arg:expr)*) => {
+        $crate::common::tracing_log_xt::log_xt!(data: hash, target: $target, tracing::Level::DEBUG, $tx_collection, $text_with_format, $($arg)*);
+    };
+}
+
 macro_rules! log_xt_trace {
     (data: $datatype:ident, target: $target:expr, $($arg:tt)+) => {
         $crate::common::tracing_log_xt::log_xt!(data: $datatype, target: $target, tracing::Level::TRACE, $($arg)+);
@@ -66,4 +78,5 @@ macro_rules! log_xt_trace {
 }
 
 pub(crate) use log_xt;
+pub(crate) use log_xt_debug;
 pub(crate) use log_xt_trace;

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

@@ -29,7 +29,7 @@ use super::{
 };
 use crate::{
 	api::FullChainApi,
-	common::tracing_log_xt::log_xt_trace,
+	common::tracing_log_xt::{log_xt_debug, log_xt_trace},
 	enactment_state::{EnactmentAction, EnactmentState},
 	fork_aware_txpool::{
 		dropped_watcher::{DroppedReason, DroppedTransaction},
@@ -891,7 +891,7 @@ where
 		invalid_tx_errors: TxInvalidityReportMap<TxHash<Self>>,
 	) -> Vec<Arc<Self::InPoolTransaction>> {
 		debug!(target: LOG_TARGET, len = ?invalid_tx_errors.len(), "fatp::report_invalid");
-		log_xt_trace!(data: tuple, target:LOG_TARGET, invalid_tx_errors.iter(), "fatp::report_invalid {:?}");
+		log_xt_debug!(data: tuple, target:LOG_TARGET, invalid_tx_errors.iter(), "fatp::report_invalid {:?}");
 		self.metrics
 			.report(|metrics| metrics.reported_invalid_txs.inc_by(invalid_tx_errors.len() as _));
 

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

@@ -656,6 +656,7 @@ where
 	pub fn remove_subtree<F>(
 		&self,
 		hashes: &[ExtrinsicHash<ChainApi>],
+		ban_transactions: bool,
 		listener_action: F,
 	) -> Vec<TransactionFor<ChainApi>>
 	where
@@ -664,6 +665,8 @@ where
 			ExtrinsicHash<ChainApi>,
 		),
 	{
-		self.pool.validated_pool().remove_subtree(hashes, listener_action)
+		self.pool
+			.validated_pool()
+			.remove_subtree(hashes, ban_transactions, listener_action)
 	}
 }

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

@@ -647,8 +647,7 @@ where
 	///
 	/// Invalid future and stale transaction will be removed only from given `at` view, and will be
 	/// kept in the view_store. Such transaction will not be reported in returned vector. They
-	/// also will not be banned from re-entering the pool (however can be rejected from re-entring
-	/// the view). No event will be triggered.
+	/// also will not be banned from re-entering the pool. No event will be triggered.
 	///
 	/// For other errors, the transaction will be removed from the view_store, and it will be
 	/// included in the returned vector. Additionally, transactions provided as input will be banned
@@ -685,7 +684,7 @@ where
 		// be in the pool.
 		at.map(|at| {
 			self.get_view_at(at, true)
-				.map(|(view, _)| view.remove_subtree(&remove_from_view, |_, _| {}))
+				.map(|(view, _)| view.remove_subtree(&remove_from_view, false, |_, _| {}))
 		});
 
 		let mut removed = vec![];
@@ -757,7 +756,7 @@ where
 					));
 				},
 				PreInsertAction::RemoveSubtree(ref removal) => {
-					view.remove_subtree(&[removal.xt_hash], &*removal.listener_action);
+					view.remove_subtree(&[removal.xt_hash], true, &*removal.listener_action);
 				},
 			}
 		}
@@ -862,7 +861,7 @@ where
 			.iter()
 			.chain(self.inactive_views.read().iter())
 			.filter(|(_, view)| view.is_imported(&xt_hash))
-			.flat_map(|(_, view)| view.remove_subtree(&[xt_hash], &listener_action))
+			.flat_map(|(_, view)| view.remove_subtree(&[xt_hash], true, &listener_action))
 			.filter_map(|xt| seen.insert(xt.hash).then(|| xt.clone()))
 			.collect();
 

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

@@ -734,7 +734,7 @@ impl<B: ChainApi, L: EventHandler<B>> ValidatedPool<B, L> {
 			return vec![]
 		}
 
-		let invalid = self.remove_subtree(hashes, |listener, removed_tx_hash| {
+		let invalid = self.remove_subtree(hashes, true, |listener, removed_tx_hash| {
 			listener.invalid(&removed_tx_hash);
 		});
 
@@ -795,8 +795,8 @@ impl<B: ChainApi, L: EventHandler<B>> ValidatedPool<B, L> {
 	/// This function traverses the dependency graph of transactions and removes the specified
 	/// transaction along with all its descendant transactions from the pool.
 	///
-	/// The root transaction will be banned from re-entrering the pool. Descendant transactions may
-	/// be re-submitted to the pool if required.
+	/// The root transactions will be banned from re-entrering the pool if `ban_transactions` is
+	/// true. Descendant transactions may be re-submitted to the pool if required.
 	///
 	/// A `event_disaptcher_action` callback function is invoked for every transaction that is
 	/// removed, providing a reference to the pool's event dispatcher and the hash of the removed
@@ -807,13 +807,16 @@ impl<B: ChainApi, L: EventHandler<B>> ValidatedPool<B, L> {
 	pub fn remove_subtree<F>(
 		&self,
 		hashes: &[ExtrinsicHash<B>],
+		ban_transactions: bool,
 		event_dispatcher_action: F,
 	) -> Vec<TransactionFor<B>>
 	where
 		F: Fn(&mut EventDispatcher<B, L>, ExtrinsicHash<B>),
 	{
-		// temporarily ban removed transactions
-		self.rotator.ban(&Instant::now(), hashes.iter().cloned());
+		// temporarily ban removed transactions if requested
+		if ban_transactions {
+			self.rotator.ban(&Instant::now(), hashes.iter().cloned());
+		};
 		let removed = self.pool.write().remove_subtree(hashes);
 
 		removed