From 954d8571df26b5cc0d342c6c05d414bd6912f1cd Mon Sep 17 00:00:00 2001
From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
Date: Wed, 19 Mar 2025 17:20:25 +0100
Subject: [PATCH] `fatxpool`: report_invalid: do not ban Future/Stale txs from
re-entering the view (#7777)
#### 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.
---------
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
---
prdoc/pr_7777.prdoc | 10 ++++++++++
.../transaction-pool/src/common/tracing_log_xt.rs | 13 +++++++++++++
.../src/fork_aware_txpool/fork_aware_txpool.rs | 4 ++--
.../transaction-pool/src/fork_aware_txpool/view.rs | 5 ++++-
.../src/fork_aware_txpool/view_store.rs | 9 ++++-----
.../transaction-pool/src/graph/validated_pool.rs | 13 ++++++++-----
6 files changed, 41 insertions(+), 13 deletions(-)
create mode 100644 prdoc/pr_7777.prdoc
diff --git a/prdoc/pr_7777.prdoc b/prdoc/pr_7777.prdoc
new file mode 100644
index 00000000000..e62abf1bf6f
--- /dev/null
+++ b/prdoc/pr_7777.prdoc
@@ -0,0 +1,10 @@
+title: '`fatxpool`: report_invalid: do not ban Future/Stale txs from re-entering the
+ view'
+doc:
+- audience: Node Dev
+ description: |-
+ Avoid banning future/stale transactions reported as invalid by the authorship module.
+
+crates:
+- name: sc-transaction-pool
+ bump: minor
diff --git a/substrate/client/transaction-pool/src/common/tracing_log_xt.rs b/substrate/client/transaction-pool/src/common/tracing_log_xt.rs
index 9be9cddd978..ef13642bbb9 100644
--- a/substrate/client/transaction-pool/src/common/tracing_log_xt.rs
+++ b/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;
diff --git a/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs b/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs
index 337da1dd61d..855e9b55e85 100644
--- a/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs
+++ b/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 _));
diff --git a/substrate/client/transaction-pool/src/fork_aware_txpool/view.rs b/substrate/client/transaction-pool/src/fork_aware_txpool/view.rs
index d496b24fb4e..5c025b22573 100644
--- a/substrate/client/transaction-pool/src/fork_aware_txpool/view.rs
+++ b/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)
}
}
diff --git a/substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs b/substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs
index f61e300f364..3ee81d88b9c 100644
--- a/substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs
+++ b/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();
diff --git a/substrate/client/transaction-pool/src/graph/validated_pool.rs b/substrate/client/transaction-pool/src/graph/validated_pool.rs
index 8eb967421bd..281dc32e57c 100644
--- a/substrate/client/transaction-pool/src/graph/validated_pool.rs
+++ b/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
--
GitLab