fatxpool: proper handling of priorities when mempool is full (parit…

…ytech#6647)

Higher-priority transactions can now replace lower-priority transactions
even when the internal _tx_mem_pool_ is full.

**Notes for reviewers:**
- The _tx_mem_pool_ now maintains information about transaction
priority. Although _tx_mem_pool_ itself is stateless, transaction
priority is updated after submission to the view. An alternative
approach could involve validating transactions at the `at` block, but
this is computationally expensive. To avoid additional validation
overhead, I opted to use the priority obtained from runtime during
submission to the view. This is the rationale behind introducing the
`SubmitOutcome` struct, which synchronously communicates transaction
priority from the view to the pool. This results in a very brief window
during which the transaction priority remains unknown - those
transaction are not taken into consideration while dropping takes place.
In the future, if needed, we could update transaction priority using
view revalidation results to keep this information fully up-to-date (as
priority of transaction may change with chain-state evolution).
- When _tx_mem_pool_ becomes full (an event anticipated to be rare),
transaction priority must be known to perform priority-based removal. In
such cases, the most recent block known is utilized for validation. I
think that speculative submission to the view and re-using the priority
from this submission would be an unnecessary complication.
- Once the priority is determined, lower-priority transactions whose
cumulative size meets or exceeds the size of the new transaction are
collected to ensure the pool size limit is not exceeded.
- Transaction removed from _tx_mem_pool_ , also needs to be removed from
all the views with appropriate event (which is done by
`remove_transaction_subtree`). To ensure complete removal, the
`PendingTxReplacement` struct was re-factored to more generic
`PendingPreInsertTask` (introduced in paritytech#6405) which covers removal and
submssion of transaction in the view which may be potentially created in
the background. This is to ensure that removed transaction will not
re-enter to the newly created view.
- `submit_local` implementation was also improved to properly handle
priorities in case when mempool is full. Some missing tests for this
method were also added.

Closes: paritytech#5809

---------

Co-authored-by: command-bot <>
Co-authored-by: Iulian Barbu <[email protected]>

prdoc/pr_6647.prdoc

@@ -0,0 +1,8 @@
+title: '`fatxpool`: proper handling of priorities when mempool is full'
+doc:
+- audience: Node Dev
+  description: |-
+    Higher-priority transactions can now replace lower-priority transactions even when the internal _tx_mem_pool_ is full.
+crates:
+- name: sc-transaction-pool
+  bump: minor

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

@@ -53,11 +53,13 @@ pub struct DroppedTransaction<Hash> {
 }
 
 impl<Hash> DroppedTransaction<Hash> {
-	fn new_usurped(tx_hash: Hash, by: Hash) -> Self {
+	/// Creates a new instance with reason set to `DroppedReason::Usurped(by)`.
+	pub fn new_usurped(tx_hash: Hash, by: Hash) -> Self {
 		Self { reason: DroppedReason::Usurped(by), tx_hash }
 	}
 
-	fn new_enforced_by_limts(tx_hash: Hash) -> Self {
+	/// Creates a new instance with reason set to `DroppedReason::LimitsEnforced`.
+	pub fn new_enforced_by_limts(tx_hash: Hash) -> Self {
 		Self { reason: DroppedReason::LimitsEnforced, tx_hash }
 	}
 }
@@ -256,11 +258,13 @@ where
 				self.future_transaction_views.entry(tx_hash).or_default().insert(block_hash);
 			},
 			TransactionStatus::Ready | TransactionStatus::InBlock(..) => {
-				// note: if future transaction was once seens as the ready we may want to treat it
-				// as ready transactions. Unreferenced future transactions are more likely to be
-				// removed when the last referencing view is removed then ready transactions.
-				// Transcaction seen as ready is likely quite close to be included in some
-				// future fork.
+				// note: if future transaction was once seen as the ready we may want to treat it
+				// as ready transaction. The rationale behind this is as follows: we want to remove
+				// unreferenced future transactions when the last referencing view is removed (to
+				// avoid clogging mempool). For ready transactions we prefer to keep them in mempool
+				// even if no view is currently referencing them. Future transcaction once seen as
+				// ready is likely quite close to be included in some future fork (it is close to be
+				// ready, so we make exception and treat such transaction as ready).
 				if let Some(mut views) = self.future_transaction_views.remove(&tx_hash) {
 					views.insert(block_hash);
 					self.ready_transaction_views.insert(tx_hash, views);