fatxpool: add labels for mempool_revalidation_invalid_txs metric (#9003)

# Description

This PR considers errors propagated from mempool revalidation in the
context of fork-aware transaction pool and groups them under unique
labels to count them under `mempool_revalidation_invalid_txs` prometheus
metric.
 
Closes #7973 

## Integration

Node developers/operators can use labels like `category` & `reason` to
differentiate between invalid transactions, as reported by mempool
revalidation.

## Review Notes

Labeling mempool revalidation invalid txs is done based on `category` &
`reason`:
- the transaction validity invalid and unknown are mapped to category =
"invalid" or "unknown", while the underlying variant name is the reason,
converted to snake case.
- `sc_transaction_pool::common::error::Error` returned from
`sc_transaction_pool_api::api::validate_transaction_blocking` function
is mapped to category = "validation_failed" and reason is variant name
in snake_case.
- txs part of subtrees of invalid txs are considered invalid, and they
are mapped to category = "subtree" and reason =
"invalid"|"unknown"|"validation_failed", based on the category of the
invalid root tx that started the invalidation of the subtree the tx is
part of.

---------

Signed-off-by: Iulian Barbu <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Bastian Köcher <[email protected]>

Author: 3 people

Cargo.lock

@@ -0,0 +1,12 @@
+title: '`fatxpool`: add labels for mempool revalidation invalid txs counting'
+doc:
+- audience: [ Node Dev, Node Operator ]
+  description: |-
+    This PR considers errors propagated from mempool revalidation in the context of fork-aware transaction pool and groups them under unique labels to count them under `mempool_revalidation_invalid_txs` prometheus metric. This results in a breakdown of the type of root causes for why a transaction is considered invalid, helping with debugging.
+crates:
+- name: sc-transaction-pool
+  bump: minor 
+- name: sc-transaction-pool-api
+  bump: minor
+- name: sp-runtime
+  bump: minor 

prdoc/pr_9003.prdoc

@@ -40,6 +40,7 @@ sp-crypto-hashing = { workspace = true, default-features = true }
 sp-runtime = { workspace = true, default-features = true }
 sp-tracing = { workspace = true, default-features = true }
 sp-transaction-pool = { workspace = true, default-features = true }
+strum = { workspace = true, features = ["derive"] }
 thiserror = { workspace = true }
 tokio = { workspace = true, default-features = true, features = ["macros", "time"] }
 tokio-stream = { workspace = true }

substrate/client/transaction-pool/Cargo.toml

@@ -21,6 +21,7 @@ serde = { features = ["derive"], workspace = true, default-features = true }
 sp-blockchain = { workspace = true, default-features = true }
 sp-core = { workspace = true }
 sp-runtime = { workspace = true }
+strum = { workspace = true, features = ["derive"] }
 thiserror = { workspace = true }
 
 [dev-dependencies]

substrate/client/transaction-pool/api/Cargo.toml

@@ -26,7 +26,7 @@ use sp_runtime::transaction_validity::{
 pub type Result<T> = std::result::Result<T, Error>;
 
 /// Transaction pool error type.
-#[derive(Debug, thiserror::Error)]
+#[derive(Debug, thiserror::Error, strum::AsRefStr)]
 #[allow(missing_docs)]
 pub enum Error {
 	#[error("Unknown transaction validity: {0:?}")]
@@ -112,3 +112,19 @@ impl IntoPoolError for Error {
 		Ok(self)
 	}
 }
+
+/// Provide a representation of the underlying instance as a prometheus metric label.
+pub trait IntoMetricsLabel {
+	/// Short string representation of the underlying instance.
+	///
+	/// This is intended to be used for a prometheus metric that tracks this instance
+	/// with the goal of distributing the observations over multiple groups, where an unique
+	/// label represents a group of same instance, observed many times.
+	fn label(&self) -> String;
+}
+
+impl IntoMetricsLabel for Error {
+	fn label(&self) -> String {
+		self.as_ref().to_string()
+	}
+}

substrate/client/transaction-pool/api/src/error.rs

@@ -24,7 +24,8 @@ use sc_transaction_pool_api::error::Error as TxPoolError;
 pub type Result<T> = std::result::Result<T, Error>;
 
 /// Transaction pool error type.
-#[derive(Debug, thiserror::Error)]
+#[derive(Debug, thiserror::Error, strum::AsRefStr)]
+#[strum(serialize_all = "snake_case")]
 #[allow(missing_docs)]
 pub enum Error {
 	#[error("Transaction pool error: {0}")]
@@ -48,3 +49,9 @@ impl sc_transaction_pool_api::error::IntoPoolError for Error {
 		}
 	}
 }
+
+impl sc_transaction_pool_api::error::IntoMetricsLabel for Error {
+	fn label(&self) -> String {
+		self.as_ref().to_string()
+	}
+}

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

@@ -18,16 +18,16 @@
 
 //! Prometheus's metrics for a fork-aware transaction pool.
 
-use super::tx_mem_pool::InsertionInfo;
+use super::tx_mem_pool::{InsertionInfo, InvalidTxReason};
 use crate::{
 	common::metrics::{GenericMetricsLink, MetricsRegistrant},
 	graph::{self, BlockHash, ExtrinsicHash},
 	LOG_TARGET,
 };
 use futures::{FutureExt, StreamExt};
 use prometheus_endpoint::{
-	exponential_buckets, histogram_opts, linear_buckets, register, Counter, Gauge, Histogram,
-	PrometheusError, Registry, U64,
+	exponential_buckets, histogram_opts, linear_buckets, register, Counter, CounterVec, Gauge,
+	Histogram, Opts, PrometheusError, Registry, U64,
 };
 #[cfg(doc)]
 use sc_transaction_pool_api::TransactionPool;
@@ -74,7 +74,7 @@ pub struct Metrics {
 	/// Total number of transactions submitted from mempool to views.
 	pub submitted_from_mempool_txs: Counter<U64>,
 	/// Total number of transactions found as invalid during mempool revalidation.
-	pub mempool_revalidation_invalid_txs: Counter<U64>,
+	pub mempool_revalidation_invalid_txs: MempoolInvalidTxReasonCounter,
 	/// Total number of transactions found as invalid during view revalidation.
 	pub view_revalidation_invalid_txs: Counter<U64>,
 	/// Total number of valid transactions processed during view revalidation.
@@ -292,6 +292,39 @@ impl EventsHistograms {
 	}
 }
 
+/// Represents a labeled counter of invalid tx reasons.
+pub struct MempoolInvalidTxReasonCounter {
+	inner: CounterVec<U64>,
+}
+
+impl MempoolInvalidTxReasonCounter {
+	fn register(registry: &Registry) -> Result<Self, PrometheusError> {
+		Ok(Self {
+			inner: register(
+				CounterVec::new(
+					Opts::new(
+						"substrate_sub_txpool_mempool_revalidation_invalid_txs_total",
+						r#"Total number of transactions found as invalid during mempool revalidation.
+						They are broken down into `category` and `reason` labels.
+						- `category` can be `invalid`, `unknown`, `subtree` or `validation_failed`.
+						- `reason` is more nuanced, but is worth mentioning that for `subtree` category,
+						   the underlying reason can be one of the other categories."#,
+					),
+					&["category", "reason"],
+				)?,
+				registry,
+			)?,
+		})
+	}
+
+	// Increments the mempool invalid txs metrics based on a custom category & reason.
+	pub fn observe(&self, label: &InvalidTxReason, count: u64) {
+		self.inner
+			.with_label_values(&[label.category().as_str(), label.reason().as_str()])
+			.inc_by(count)
+	}
+}
+
 impl MetricsRegistrant for Metrics {
 	fn register(registry: &Registry) -> Result<Box<Self>, PrometheusError> {
 		Ok(Box::from(Self {
@@ -380,13 +413,7 @@ impl MetricsRegistrant for Metrics {
 				)?,
 				registry,
 			)?,
-			mempool_revalidation_invalid_txs: register(
-				Counter::new(
-					"substrate_sub_txpool_mempool_revalidation_invalid_txs_total",
-					"Total number of transactions found as invalid during mempool revalidation.",
-				)?,
-				registry,
-			)?,
+			mempool_revalidation_invalid_txs: MempoolInvalidTxReasonCounter::register(registry)?,
 			view_revalidation_invalid_txs: register(
 				Counter::new(
 					"substrate_sub_txpool_view_revalidation_invalid_txs_total",

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

@@ -34,7 +34,7 @@
 use futures::{future::join_all, FutureExt};
 use itertools::Itertools;
 use parking_lot::RwLock;
-use sc_transaction_pool_api::{TransactionPriority, TransactionSource};
+use sc_transaction_pool_api::{error::IntoMetricsLabel, TransactionPriority, TransactionSource};
 use sp_blockchain::HashAndNumber;
 use sp_runtime::{
 	traits::Block as BlockT,
@@ -58,8 +58,7 @@ use tracing::{debug, trace};
 
 use crate::{
 	common::tracing_log_xt::log_xt_trace,
-	graph,
-	graph::{base_pool::TimedTransactionSource, ExtrinsicFor, ExtrinsicHash},
+	graph::{self, base_pool::TimedTransactionSource, ExtrinsicFor, ExtrinsicHash},
 	ValidateTransactionPriority, LOG_TARGET,
 };
 
@@ -78,6 +77,39 @@ pub(crate) const TXMEMPOOL_MAX_REVALIDATION_BATCH_SIZE: usize = 1000;
 
 const SYNC_BRIDGE_EXPECT: &str = "The mempool blocking task shall not be terminated. qed.";
 
+#[derive(strum::Display)]
+#[strum(serialize_all = "snake_case")]
+/// Provides a type safe way of determining the category and reason of why
+/// a transaction is marked as invalid, useful to extract labels in the context of
+/// `mempool_revalidation_invalid_txs` metric.
+pub(super) enum InvalidTxReason {
+	/// Coresponds to invalid validity.
+	Invalid(String),
+	/// Corresponds to unknown validity.
+	Unknown(String),
+	/// Corresponds to a transaction which is marked as invalid because it is part of a subtree of
+	/// a transaction with invalid or unknown validities.
+	Subtree(String),
+	/// Corresponds to a transaction for which a validity couldn't be determined due to a failure
+	/// during the validation, or because of a missing runtime prerequisite.
+	ValidationFailed(String),
+}
+
+impl InvalidTxReason {
+	pub(super) fn reason(&self) -> &String {
+		match self {
+			InvalidTxReason::Invalid(inner) |
+			InvalidTxReason::Unknown(inner) |
+			InvalidTxReason::Subtree(inner) |
+			InvalidTxReason::ValidationFailed(inner) => inner,
+		}
+	}
+
+	pub(super) fn category(&self) -> String {
+		self.to_string()
+	}
+}
+
 /// Represents the transaction in the intermediary buffer.
 pub(crate) struct TxInMemPool<ChainApi, Block>
 where
@@ -584,7 +616,11 @@ where
 	/// Revalidates a batch of transactions against the provided finalized block.
 	///
 	/// Returns a vector of invalid transaction hashes.
-	async fn revalidate_inner(&self, finalized_block: HashAndNumber<Block>) -> Vec<Block::Hash> {
+	async fn revalidate_inner(
+		&self,
+		view_store: Arc<ViewStore<ChainApi, Block>>,
+		finalized_block: HashAndNumber<Block>,
+	) -> HashSet<ExtrinsicHash<ChainApi>> {
 		trace!(
 			target: LOG_TARGET,
 			?finalized_block,
@@ -629,32 +665,69 @@ where
 		let validated_count = validation_results.len();
 
 		let duration = start.elapsed();
-
 		let invalid_hashes = validation_results
 			.into_iter()
 			.filter_map(|(tx_hash, validation_result)| match validation_result {
-				Ok(Ok(_)) |
-				Ok(Err(TransactionValidityError::Invalid(InvalidTransaction::Future))) => None,
-				Err(_) |
-				Ok(Err(TransactionValidityError::Unknown(_))) |
-				Ok(Err(TransactionValidityError::Invalid(_))) => {
+				Ok(Ok(_) | Err(TransactionValidityError::Invalid(InvalidTransaction::Future))) =>
+					None,
+				Err(ref error) => {
+					trace!(
+						target: LOG_TARGET,
+						?tx_hash,
+						?validation_result,
+						"mempool::revalidate_inner error during revalidation"
+					);
+					Some((tx_hash, InvalidTxReason::ValidationFailed(error.label())))
+				},
+				Ok(Err(TransactionValidityError::Unknown(error))) => {
 					trace!(
 						target: LOG_TARGET,
 						?tx_hash,
 						?validation_result,
-						"mempool::revalidate_inner invalid"
+						"mempool::revalidate_inner cannot determine transaction validity"
 					);
-					Some(tx_hash)
+					Some((tx_hash, InvalidTxReason::Unknown(error.as_ref().to_string())))
+				},
+				Ok(Err(TransactionValidityError::Invalid(error))) => {
+					trace!(
+						target: LOG_TARGET,
+						?tx_hash,
+						?validation_result,
+						"mempool::revalidate_inner transaction is invalid"
+					);
+					Some((tx_hash, InvalidTxReason::Invalid(error.as_ref().to_string())))
 				},
 			})
 			.collect::<Vec<_>>();
 
+		let mut invalid_hashes_subtrees = Vec::new();
+		// Include also subtree txs.
+		for (tx, reason) in &invalid_hashes {
+			let txs_in_subtree = view_store
+				.remove_transaction_subtree(*tx, |_, _| {})
+				.into_iter()
+				.map(|tx| (tx.hash, InvalidTxReason::Subtree(reason.to_string())));
+			invalid_hashes_subtrees.extend(txs_in_subtree);
+		}
+
+		let revalidated_invalid_hashes_len = invalid_hashes.len();
+		let invalid_hashes = invalid_hashes
+			.into_iter()
+			.chain(invalid_hashes_subtrees)
+			.map(|(tx, reason)| {
+				self.metrics
+					.report(|metrics| metrics.mempool_revalidation_invalid_txs.observe(&reason, 1));
+				tx
+			})
+			.collect::<HashSet<_>>();
+
 		debug!(
 			target: LOG_TARGET,
 			?finalized_block,
 			validated_count,
 			total_count,
-			invalid_hashes = invalid_hashes.len(),
+			invalid_hashes_subtrees_len = invalid_hashes.len(),
+			revalidated_invalid_hashes_len,
 			?duration,
 			"mempool::revalidate_inner"
 		);
@@ -686,53 +759,30 @@ where
 		view_store: Arc<ViewStore<ChainApi, Block>>,
 		finalized_block: HashAndNumber<Block>,
 	) {
-		let revalidated_invalid_hashes = self.revalidate_inner(finalized_block.clone()).await;
-
-		let mut invalid_hashes_subtrees =
-			revalidated_invalid_hashes.clone().into_iter().collect::<HashSet<_>>();
-		for tx in &revalidated_invalid_hashes {
-			invalid_hashes_subtrees.extend(
-				view_store
-					.remove_transaction_subtree(*tx, |_, _| {})
-					.into_iter()
-					.map(|tx| tx.hash),
-			);
-		}
-
+		let invalid_hashes_subtrees =
+			self.revalidate_inner(view_store.clone(), finalized_block.clone()).await;
 		{
 			let mut transactions = self.transactions.write().await;
 			invalid_hashes_subtrees.iter().for_each(|tx_hash| {
 				transactions.remove(&tx_hash);
 			});
 		};
 
-		self.metrics.report(|metrics| {
-			metrics
-				.mempool_revalidation_invalid_txs
-				.inc_by(invalid_hashes_subtrees.len() as _)
-		});
-
-		let revalidated_invalid_hashes_len = revalidated_invalid_hashes.len();
-		let invalid_hashes_subtrees_len = invalid_hashes_subtrees.len();
-
-		let invalid_hashes_subtrees = invalid_hashes_subtrees.into_iter().collect::<Vec<_>>();
-
 		//note: here the consistency is assumed: it is expected that transaction will be
 		// actually removed from the listener with Invalid event. This means assumption that no view
 		// is referencing tx as ready.
-		self.listener.transactions_invalidated(&invalid_hashes_subtrees);
+		let invalid_hashes_subtrees = invalid_hashes_subtrees.into_iter().collect::<Vec<_>>();
+		self.listener.transactions_invalidated(invalid_hashes_subtrees.as_slice());
 		view_store
 			.import_notification_sink
-			.clean_notified_items(&invalid_hashes_subtrees);
+			.clean_notified_items(invalid_hashes_subtrees.as_slice());
 		view_store
 			.dropped_stream_controller
 			.remove_transactions(invalid_hashes_subtrees);
 
 		trace!(
 			target: LOG_TARGET,
 			?finalized_block,
-			revalidated_invalid_hashes_len,
-			invalid_hashes_subtrees_len,
 			"mempool::revalidate"
 		);
 	}