From 4f13d5b790d847d7fca62a88a024080d515aba6e Mon Sep 17 00:00:00 2001
From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Date: Mon, 12 Feb 2024 11:53:00 +0200
Subject: [PATCH] transaction-pool: Improve transaction status documentation
 and add helpers (#3215)

This PR improves the transaction status documentation.
- Added doc references for describing the main states
- Extra comment wrt pool ready / future queues
- `FinalityTimeout` no longer describes a lagging finality gadget, it
signals that the maximum number of finality gadgets has been reached

A few helper methods are added to indicate when:
- a final event is generated by the transaction pool for a given event
- a final event is provided, although the transaction might become valid
at a later time and could be re-submitted

The helper methods are used and taken from
https://github.com/paritytech/polkadot-sdk/pull/3079 to help us better
keep it in sync.


cc @paritytech/subxt-team

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
---
 .../client/transaction-pool/api/src/error.rs  | 24 +++++
 .../client/transaction-pool/api/src/lib.rs    | 89 +++++++++++++++----
 2 files changed, 97 insertions(+), 16 deletions(-)

diff --git a/substrate/client/transaction-pool/api/src/error.rs b/substrate/client/transaction-pool/api/src/error.rs
index e521502f66f..d0744bfa3e1 100644
--- a/substrate/client/transaction-pool/api/src/error.rs
+++ b/substrate/client/transaction-pool/api/src/error.rs
@@ -71,6 +71,30 @@ pub enum Error {
 	RejectedFutureTransaction,
 }
 
+impl Error {
+	/// Returns true if the transaction could be re-submitted to the pool in the future.
+	///
+	/// For example, `Error::ImmediatelyDropped` is retriable, because the transaction
+	/// may enter the pool if there is space for it in the future.
+	pub fn is_retriable(&self) -> bool {
+		match self {
+			// An invalid transaction is temporarily banned, however it can
+			// become valid at a later time.
+			Error::TemporarilyBanned |
+			// The pool is full at the moment.
+			Error::ImmediatelyDropped |
+			// The block id is not known to the pool.
+			// The node might be lagging behind, or during a warp sync.
+			Error::InvalidBlockId(_) |
+			// The pool is configured to not accept future transactions.
+			Error::RejectedFutureTransaction => {
+				true
+			}
+			_ => false
+		}
+	}
+}
+
 /// Transaction pool error conversion.
 pub trait IntoPoolError: std::error::Error + Send + Sized + Sync {
 	/// Try to extract original `Error`
diff --git a/substrate/client/transaction-pool/api/src/lib.rs b/substrate/client/transaction-pool/api/src/lib.rs
index a795917528f..0a313c5b782 100644
--- a/substrate/client/transaction-pool/api/src/lib.rs
+++ b/substrate/client/transaction-pool/api/src/lib.rs
@@ -62,20 +62,27 @@ impl PoolStatus {
 ///
 /// The status events can be grouped based on their kinds as:
 /// 1. Entering/Moving within the pool:
-/// 		- `Future`
-/// 		- `Ready`
+/// 		- [Future](TransactionStatus::Future)
+/// 		- [Ready](TransactionStatus::Ready)
 /// 2. Inside `Ready` queue:
-/// 		- `Broadcast`
+/// 		- [Broadcast](TransactionStatus::Broadcast)
 /// 3. Leaving the pool:
-/// 		- `InBlock`
-/// 		- `Invalid`
-/// 		- `Usurped`
-/// 		- `Dropped`
+/// 		- [InBlock](TransactionStatus::InBlock)
+/// 		- [Invalid](TransactionStatus::Invalid)
+/// 		- [Usurped](TransactionStatus::Usurped)
+/// 		- [Dropped](TransactionStatus::Dropped)
 /// 	4. Re-entering the pool:
-/// 		- `Retracted`
+/// 		- [Retracted](TransactionStatus::Retracted)
 /// 	5. Block finalized:
-/// 		- `Finalized`
-/// 		- `FinalityTimeout`
+/// 		- [Finalized](TransactionStatus::Finalized)
+/// 		- [FinalityTimeout](TransactionStatus::FinalityTimeout)
+///
+/// Transactions are first placed in either the `Ready` or `Future` queues of the transaction pool.
+/// Substrate validates the transaction before it enters the pool.
+///
+/// A transaction is placed in the `Future` queue if it will become valid at a future time.
+/// For example, submitting a transaction with a higher account nonce than the current
+/// expected nonce will place the transaction in the `Future` queue.
 ///
 /// The events will always be received in the order described above, however
 /// there might be cases where transactions alternate between `Future` and `Ready`
@@ -88,19 +95,37 @@ impl PoolStatus {
 /// 1. Due to possible forks, the transaction that ends up being in included
 /// in one block, may later re-enter the pool or be marked as invalid.
 /// 2. Transaction `Dropped` at one point, may later re-enter the pool if some other
-/// transactions are removed.
+/// transactions are removed. A `Dropped` transaction may re-enter the pool only if it is
+/// resubmitted.
 /// 3. `Invalid` transaction may become valid at some point in the future.
 /// (Note that runtimes are encouraged to use `UnknownValidity` to inform the pool about
-/// such case).
+/// such case). An `Invalid` transaction may re-enter the pool only if it is resubmitted.
 /// 4. `Retracted` transactions might be included in some next block.
 ///
-/// The stream is considered finished only when either `Finalized` or `FinalityTimeout`
-/// event is triggered. You are however free to unsubscribe from notifications at any point.
-/// The first one will be emitted when the block, in which transaction was included gets
-/// finalized. The `FinalityTimeout` event will be emitted when the block did not reach finality
+/// The `FinalityTimeout` event will be emitted when the block did not reach finality
 /// within 512 blocks. This either indicates that finality is not available for your chain,
 /// or that finality gadget is lagging behind. If you choose to wait for finality longer, you can
 /// re-subscribe for a particular transaction hash manually again.
+///
+/// ### Last Event
+///
+/// The stream is considered finished when one of the following events happen:
+/// - [Finalized](TransactionStatus::Finalized)
+/// - [FinalityTimeout](TransactionStatus::FinalityTimeout)
+/// - [Usurped](TransactionStatus::Usurped)
+/// - [Invalid](TransactionStatus::Invalid)
+/// - [Dropped](TransactionStatus::Dropped)
+///
+/// See [`TransactionStatus::is_final`] for more details.
+///
+/// ### Resubmit Transactions
+///
+/// Users might resubmit the transaction at a later time for the following events:
+/// - [FinalityTimeout](TransactionStatus::FinalityTimeout)
+/// - [Invalid](TransactionStatus::Invalid)
+/// - [Dropped](TransactionStatus::Dropped)
+///
+/// See [`TransactionStatus::is_retriable`] for more details.
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub enum TransactionStatus<Hash, BlockHash> {
@@ -131,6 +156,38 @@ pub enum TransactionStatus<Hash, BlockHash> {
 	Invalid,
 }
 
+impl<Hash, BlockHash> TransactionStatus<Hash, BlockHash> {
+	/// Returns true if this is the last event emitted by [`TransactionStatusStream`].
+	pub fn is_final(&self) -> bool {
+		// The state must be kept in sync with `crate::graph::Sender`.
+		match self {
+			Self::Usurped(_) |
+			Self::Finalized(_) |
+			Self::FinalityTimeout(_) |
+			Self::Invalid |
+			Self::Dropped => true,
+			_ => false,
+		}
+	}
+
+	/// Returns true if the transaction could be re-submitted to the pool in the future.
+	///
+	/// For example, `TransactionStatus::Dropped` is retriable, because the transaction
+	/// may enter the pool if there is space for it in the future.
+	pub fn is_retriable(&self) -> bool {
+		match self {
+			// The number of finality watchers has been reached.
+			Self::FinalityTimeout(_) |
+			// An invalid transaction might be valid at a later time.
+			Self::Invalid |
+			// The transaction was dropped because of the limits of the pool.
+			// It can reenter the pool when other transactions are removed / finalized.
+			Self::Dropped => true,
+			_ => false,
+		}
+	}
+}
+
 /// The stream of transaction events.
 pub type TransactionStatusStream<Hash, BlockHash> =
 	dyn Stream<Item = TransactionStatus<Hash, BlockHash>> + Send;
-- 
GitLab