Document TransactionStatus and fix termination conditions. (#4446)

* Document TransactionStatus and fix termination conditions.

* Update client/rpc-api/src/author/mod.rs

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/client/rpc-api/src/author/mod.rs

@@ -60,6 +60,9 @@ pub trait AuthorApi<Hash, BlockHash> {
 	) -> Result<Vec<Hash>>;
 
 	/// Submit an extrinsic to watch.
+	///
+	/// See [`TransactionStatus`](sp_transaction_pool::TransactionStatus) for details on transaction
+	/// lifecycle.
 	#[pubsub(
 		subscription = "author_extrinsicUpdate",
 		subscribe,

substrate/client/rpc-api/src/system/mod.rs

@@ -22,9 +22,8 @@ pub mod helpers;
 use crate::helpers::Receiver;
 use jsonrpc_derive::rpc;
 use futures::{future::BoxFuture, compat::Compat};
-use std::pin::Pin;
 
-use self::error::{Error, Result};
+use self::error::Result as SystemResult;
 
 pub use self::helpers::{Properties, SystemInfo, Health, PeerInfo, NodeRole};
 pub use self::gen_client::Client as SystemClient;
@@ -34,19 +33,19 @@ pub use self::gen_client::Client as SystemClient;
 pub trait SystemApi<Hash, Number> {
 	/// Get the node's implementation name. Plain old string.
 	#[rpc(name = "system_name")]
-	fn system_name(&self) -> Result<String>;
+	fn system_name(&self) -> SystemResult<String>;
 
 	/// Get the node implementation's version. Should be a semver string.
 	#[rpc(name = "system_version")]
-	fn system_version(&self) -> Result<String>;
+	fn system_version(&self) -> SystemResult<String>;
 
 	/// Get the chain's type. Given as a string identifier.
 	#[rpc(name = "system_chain")]
-	fn system_chain(&self) -> Result<String>;
+	fn system_chain(&self) -> SystemResult<String>;
 
 	/// Get a custom set of properties as a JSON object, defined in the chain spec.
 	#[rpc(name = "system_properties")]
-	fn system_properties(&self) -> Result<Properties>;
+	fn system_properties(&self) -> SystemResult<Properties>;
 
 	/// Return health status of the node.
 	///
@@ -74,13 +73,13 @@ pub trait SystemApi<Hash, Number> {
 	/// is an example of a valid, passing multiaddr with PeerId attached.
 	#[rpc(name = "system_addReservedPeer", returns = "()")]
 	fn system_add_reserved_peer(&self, peer: String)
-		-> Compat<BoxFuture<'static, std::result::Result<(), jsonrpc_core::Error>>>;
+		-> Compat<BoxFuture<'static, Result<(), jsonrpc_core::Error>>>;
 
 	/// Remove a reserved peer. Returns the empty string or an error. The string
 	/// should encode only the PeerId e.g. `QmSk5HQbn6LhUwDiNMseVUjuRYhEtYj4aUZ6WfWoGURpdV`.
 	#[rpc(name = "system_removeReservedPeer", returns = "()")]
 	fn system_remove_reserved_peer(&self, peer_id: String)
-		-> Compat<BoxFuture<'static, std::result::Result<(), jsonrpc_core::Error>>>;
+		-> Compat<BoxFuture<'static, Result<(), jsonrpc_core::Error>>>;
 
 	/// Returns the roles the node is running as.
 	#[rpc(name = "system_nodeRoles", returns = "Vec<NodeRole>")]

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

@@ -103,6 +103,6 @@ impl<H: hash::Hash + traits::Member + Serialize, H2: Clone + fmt::Debug> Listene
 	/// Transaction was pruned from the pool.
 	pub fn pruned(&mut self, header_hash: H2, tx: &H) {
 		debug!(target: "txpool", "[{:?}] Pruned at {:?}", tx, header_hash);
-		self.fire(tx, |watcher| watcher.finalized(header_hash))
+		self.fire(tx, |watcher| watcher.in_block(header_hash))
 	}
 }

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

@@ -806,7 +806,7 @@ mod tests {
 			// then
 			let mut stream = futures::executor::block_on_stream(watcher.into_stream());
 			assert_eq!(stream.next(), Some(TransactionStatus::Ready));
-			assert_eq!(stream.next(), Some(TransactionStatus::Finalized(H256::from_low_u64_be(2).into())));
+			assert_eq!(stream.next(), Some(TransactionStatus::InBlock(H256::from_low_u64_be(2).into())));
 			assert_eq!(stream.next(), None);
 		}
 
@@ -831,7 +831,7 @@ mod tests {
 			// then
 			let mut stream = futures::executor::block_on_stream(watcher.into_stream());
 			assert_eq!(stream.next(), Some(TransactionStatus::Ready));
-			assert_eq!(stream.next(), Some(TransactionStatus::Finalized(H256::from_low_u64_be(2).into())));
+			assert_eq!(stream.next(), Some(TransactionStatus::InBlock(H256::from_low_u64_be(2).into())));
 			assert_eq!(stream.next(), None);
 		}
 

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

@@ -84,12 +84,13 @@ impl<H: Clone, H2: Clone> Sender<H, H2> {
 
 	/// Some state change (perhaps another extrinsic was included) rendered this extrinsic invalid.
 	pub fn usurped(&mut self, hash: H) {
-		self.send(TransactionStatus::Usurped(hash))
+		self.send(TransactionStatus::Usurped(hash));
+		self.finalized = true;
 	}
 
-	/// Extrinsic has been finalized in block with given hash.
-	pub fn finalized(&mut self, hash: H2) {
-		self.send(TransactionStatus::Finalized(hash));
+	/// Extrinsic has been included in block with given hash.
+	pub fn in_block(&mut self, hash: H2) {
+		self.send(TransactionStatus::InBlock(hash));
 		self.finalized = true;
 	}
 
@@ -103,6 +104,7 @@ impl<H: Clone, H2: Clone> Sender<H, H2> {
 	/// Transaction has been dropped from the pool because of the limit.
 	pub fn dropped(&mut self) {
 		self.send(TransactionStatus::Dropped);
+		self.finalized = true;
 	}
 
 	/// The extrinsic has been broadcast to the given peers.

substrate/primitives/transaction-pool/src/pool.rs

@@ -55,6 +55,38 @@ impl PoolStatus {
 }
 
 /// Possible transaction status events.
+///
+/// This events are being emitted by `TransactionPool` watchers,
+/// which are also exposed over RPC.
+///
+/// The status events can be grouped based on their kinds as:
+/// 1. Entering/Moving within the pool:
+///		- `Future`
+///		- `Ready`
+/// 2. Inside `Ready` queue:
+///		- `Broadcast`
+/// 3. Leaving the pool:
+///		- `InBlock`
+///		- `Invalid`
+///		- `Usurped`
+///		- `Dropped`
+///
+/// The events will always be received in the order described above, however
+/// there might be cases where transactions alternate between `Future` and `Ready`
+/// pool, and are `Broadcast` in the meantime.
+///
+/// There is also only single event causing the transaction to leave the pool.
+///
+/// Note that there are conditions that may cause transactions to reappear in the pool.
+/// 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.
+/// 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).
+///
+/// However the user needs to re-subscribe to receive such notifications.
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub enum TransactionStatus<Hash, BlockHash> {
@@ -62,15 +94,17 @@ pub enum TransactionStatus<Hash, BlockHash> {
 	Future,
 	/// Transaction is part of the ready queue.
 	Ready,
-	/// Transaction has been finalized in block with given hash.
-	Finalized(BlockHash),
-	/// Some state change (perhaps another transaction was included) rendered this transaction invalid.
-	Usurped(Hash),
 	/// The transaction has been broadcast to the given peers.
 	Broadcast(Vec<String>),
+	/// Transaction has been included in block with given hash.
+	#[serde(rename = "finalized")] // See #4438
+	InBlock(BlockHash),
+	/// Transaction has been replaced in the pool, by another transaction
+	/// that provides the same tags. (e.g. same (sender, nonce)).
+	Usurped(Hash),
 	/// Transaction has been dropped from the pool because of the limit.
 	Dropped,
-	/// Transaction was detected as invalid.
+	/// Transaction is no longer valid in the current state.
 	Invalid,
 }