From 04af09bf9e37a2d2884f24f18c57122539481d31 Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Tue, 26 Nov 2024 19:12:58 +0100 Subject: [PATCH 1/4] txpool: API update: remove_invalid -> report_invalid --- .../client/transaction-pool/api/src/lib.rs | 20 ++++++- .../fork_aware_txpool/fork_aware_txpool.rs | 55 +++++++++++++++---- .../src/fork_aware_txpool/metrics.rs | 11 +++- .../src/fork_aware_txpool/view.rs | 11 +++- .../src/fork_aware_txpool/view_store.rs | 49 ++++++++++++++++- .../single_state_txpool.rs | 9 ++- .../src/transaction_pool_wrapper.rs | 8 ++- 7 files changed, 140 insertions(+), 23 deletions(-) diff --git a/substrate/client/transaction-pool/api/src/lib.rs b/substrate/client/transaction-pool/api/src/lib.rs index 6f771e9479bd..f0edf136c2fc 100644 --- a/substrate/client/transaction-pool/api/src/lib.rs +++ b/substrate/client/transaction-pool/api/src/lib.rs @@ -290,8 +290,24 @@ pub trait TransactionPool: Send + Sync { fn ready(&self) -> Box> + Send>; // *** Block production - /// Remove transactions identified by given hashes (and dependent transactions) from the pool. - fn remove_invalid(&self, hashes: &[TxHash]) -> Vec>; + /// Reports invalid transactions to the transaction pool. + /// + /// This function accepts an array of tuples, each containing a transaction hash and an + /// optional error encountered during the transaction execution at a specific (also optional) + /// block. + /// + /// The transaction pool implementation decides which transactions to remove. Transactions + /// dependent on invalid ones will also be removed. + /// + /// If the tuple's error is None, the transaction will be forcibly removed from the pool. + /// + /// The optional `at` parameter provides additional context regarding the block where the error + /// occurred. + fn report_invalid( + &self, + at: Option<::Hash>, + invalid_tx_errors: &[(TxHash, Option)], + ) -> Vec>; // *** logging /// Get futures transaction list. 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 065d0cb3a274..9929ef524c6f 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 @@ -675,18 +675,49 @@ where .inspect_err(|_| mempool.remove(xt_hash)) } - /// Intended to remove transactions identified by the given hashes, and any dependent - /// transactions, from the pool. In current implementation this function only outputs the error. - /// Seems that API change is needed here to make this call reasonable. - // todo [#5491]: api change? we need block hash here (assuming we need it at all - could be - // useful for verification for debugging purposes). - fn remove_invalid(&self, hashes: &[TxHash]) -> Vec> { - if !hashes.is_empty() { - log::debug!(target: LOG_TARGET, "fatp::remove_invalid {}", hashes.len()); - log_xt_trace!(target:LOG_TARGET, hashes, "[{:?}] fatp::remove_invalid"); - self.metrics - .report(|metrics| metrics.removed_invalid_txs.inc_by(hashes.len() as _)); - } + // /// Intended to remove transactions identified by the given hashes, and any dependent + // /// transactions, from the pool. In current implementation this function only outputs the + // error. /// Seems that API change is needed here to make this call reasonable. + // // todo [#5491]: api change? we need block hash here (assuming we need it at all - could be + // // useful for verification for debugging purposes). + // fn remove_invalid(&self, hashes: &[TxHash]) -> Vec> { + // if !hashes.is_empty() { + // log::debug!(target: LOG_TARGET, "fatp::remove_invalid {}", hashes.len()); + // log_xt_trace!(target:LOG_TARGET, hashes, "[{:?}] fatp::remove_invalid"); + // self.metrics + // .report(|metrics| metrics.removed_invalid_txs.inc_by(hashes.len() as _)); + // } + // Default::default() + // } + + /// Reports invalid transactions to the transaction pool. + /// + /// This function takes an array of tuples, each consisting of a transaction hash and the + /// corresponding error that occurred during transaction execution at given block. + /// + /// The transaction pool implementation will determine which transactions should be + /// removed from the pool. Transactions that depend on invalid transactions will also + /// be removed. + fn report_invalid( + &self, + at: Option<::Hash>, + invalid_tx_errors: &[(TxHash, Option)], + ) -> Vec> { + self.metrics + .report(|metrics| metrics.reported_invalid_txs.inc_by(invalid_tx_errors.len() as _)); + + let removed = self.view_store.report_invalid(at, invalid_tx_errors); + + self.metrics + .report(|metrics| metrics.removed_invalid_txs.inc_by(removed.len() as _)); + + // todo (after merging / rebasing) + + // depending on error: + // - handle cloned view with view_store replacements + // - remove resulting hashes from mempool and collect Arc + // - send notification using listener (should this be done in view_store) + Default::default() } diff --git a/substrate/client/transaction-pool/src/fork_aware_txpool/metrics.rs b/substrate/client/transaction-pool/src/fork_aware_txpool/metrics.rs index 73d45ac43051..6b8d579c693d 100644 --- a/substrate/client/transaction-pool/src/fork_aware_txpool/metrics.rs +++ b/substrate/client/transaction-pool/src/fork_aware_txpool/metrics.rs @@ -40,6 +40,8 @@ pub struct Metrics { /// Total number of unwatched transactions in txpool. pub unwatched_txs: Gauge, /// Total number of transactions reported as invalid. + pub reported_invalid_txs: Counter, + /// Total number of transactions removed as invalid. pub removed_invalid_txs: Counter, /// Total number of finalized transactions. pub finalized_txs: Counter, @@ -99,10 +101,17 @@ impl MetricsRegistrant for Metrics { )?, registry, )?, + reported_invalid_txs: register( + Counter::new( + "substrate_sub_txpool_reported_invalid_txs_total", + "Total number of transactions reported as invalid.", + )?, + registry, + )?, removed_invalid_txs: register( Counter::new( "substrate_sub_txpool_removed_invalid_txs_total", - "Total number of transactions reported as invalid.", + "Total number of transactions removed as invalid.", )?, registry, )?, 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 99095d88cb0a..297a6ebb5319 100644 --- a/substrate/client/transaction-pool/src/fork_aware_txpool/view.rs +++ b/substrate/client/transaction-pool/src/fork_aware_txpool/view.rs @@ -27,8 +27,8 @@ use super::metrics::MetricsLink as PrometheusMetrics; use crate::{ common::log_xt::log_xt_trace, graph::{ - self, watcher::Watcher, ExtrinsicFor, ExtrinsicHash, IsValidator, ValidatedTransaction, - ValidatedTransactionFor, + self, watcher::Watcher, ExtrinsicFor, ExtrinsicHash, IsValidator, TransactionFor, + ValidatedTransaction, ValidatedTransactionFor, }, LOG_TARGET, }; @@ -455,4 +455,11 @@ where ); } } + + pub(crate) fn remove_invalid( + &self, + hashes: &[ExtrinsicHash], + ) -> Vec> { + self.pool.validated_pool().remove_invalid(hashes) + } } 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 f23dcedd5bfd..5257081dcbac 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 @@ -32,8 +32,12 @@ use futures::prelude::*; use itertools::Itertools; use parking_lot::RwLock; use sc_transaction_pool_api::{error::Error as PoolError, PoolStatus, TransactionSource}; -use sp_blockchain::TreeRoute; -use sp_runtime::{generic::BlockId, traits::Block as BlockT}; +use sp_blockchain::{ApplyExtrinsicFailed, Error as BlockchainError, TreeRoute}; +use sp_runtime::{ + generic::BlockId, + traits::Block as BlockT, + transaction_validity::{InvalidTransaction, TransactionValidityError}, +}; use std::{collections::HashMap, sync::Arc, time::Instant}; /// The helper structure encapsulates all the views. @@ -484,4 +488,45 @@ where futures::future::join_all(finish_revalidation_futures).await; log::trace!(target:LOG_TARGET,"finish_background_revalidations took {:?}", start.elapsed()); } + + pub(crate) fn report_invalid( + &self, + at: Option, + invalid_tx_errors: &[(ExtrinsicHash, Option)], + ) -> Vec> { + let mut remove_from_view = vec![]; + let mut remove_from_pool = vec![]; + + invalid_tx_errors.iter().for_each(|(hash, e)| match e { + Some(BlockchainError::ApplyExtrinsicFailed(ApplyExtrinsicFailed::Validity( + TransactionValidityError::Invalid( + InvalidTransaction::Future | InvalidTransaction::Stale, + ), + ))) => { + remove_from_view.push(*hash); + }, + _ => { + remove_from_pool.push(*hash); + }, + }); + + at.inspect(|at| { + self.get_view_at(*at, true) + .map(|(view, _)| view.remove_invalid(&remove_from_view[..])) + .unwrap_or_default(); + }); + + //todo: duplicated code - we need to remove subtree from every view + // let active_views = self.active_views.read(); + // let inactive_views = self.inactive_views.read(); + // active_views + // .iter() + // .chain(inactive_views.iter()) + // .filter(|(_, view)| view.is_imported(&xt_hash)) + // .for_each(|(_, view)| { + // view.remove_subtree(xt_hash, replaced_with); + // }); + + remove_from_pool + } } diff --git a/substrate/client/transaction-pool/src/single_state_txpool/single_state_txpool.rs b/substrate/client/transaction-pool/src/single_state_txpool/single_state_txpool.rs index b29630b563bb..35724a10a6c4 100644 --- a/substrate/client/transaction-pool/src/single_state_txpool/single_state_txpool.rs +++ b/substrate/client/transaction-pool/src/single_state_txpool/single_state_txpool.rs @@ -299,8 +299,13 @@ where Ok(watcher.into_stream().boxed()) } - fn remove_invalid(&self, hashes: &[TxHash]) -> Vec> { - let removed = self.pool.validated_pool().remove_invalid(hashes); + fn report_invalid( + &self, + _at: Option<::Hash>, + invalid_tx_errors: &[(TxHash, Option)], + ) -> Vec> { + let hashes = invalid_tx_errors.iter().map(|(hash, _)| *hash).collect::>(); + let removed = self.pool.validated_pool().remove_invalid(&hashes[..]); self.metrics .report(|metrics| metrics.validations_invalid.inc_by(removed.len() as u64)); removed diff --git a/substrate/client/transaction-pool/src/transaction_pool_wrapper.rs b/substrate/client/transaction-pool/src/transaction_pool_wrapper.rs index e373c0278d80..b10094f7a8b9 100644 --- a/substrate/client/transaction-pool/src/transaction_pool_wrapper.rs +++ b/substrate/client/transaction-pool/src/transaction_pool_wrapper.rs @@ -107,8 +107,12 @@ where self.0.ready() } - fn remove_invalid(&self, hashes: &[TxHash]) -> Vec> { - self.0.remove_invalid(hashes) + fn report_invalid( + &self, + at: Option<::Hash>, + invalid_tx_errors: &[(TxHash, Option)], + ) -> Vec> { + self.0.report_invalid(at, invalid_tx_errors) } fn futures(&self) -> Vec { From bf2dd1fe6f274e9b7892de59d6127ea6872cc3fb Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Tue, 26 Nov 2024 19:13:36 +0100 Subject: [PATCH 2/4] other modules updated --- substrate/bin/node/bench/Cargo.toml | 1 + substrate/bin/node/bench/src/construct.rs | 6 +++++- .../rpc-spec-v2/src/transaction/tests/middleware_pool.rs | 8 ++++++-- .../rpc-spec-v2/src/transaction/transaction_broadcast.rs | 2 +- substrate/client/rpc/src/author/mod.rs | 6 +++--- 5 files changed, 16 insertions(+), 7 deletions(-) diff --git a/substrate/bin/node/bench/Cargo.toml b/substrate/bin/node/bench/Cargo.toml index 447f947107c1..f5218397da03 100644 --- a/substrate/bin/node/bench/Cargo.toml +++ b/substrate/bin/node/bench/Cargo.toml @@ -25,6 +25,7 @@ kitchensink-runtime = { workspace = true } sc-client-api = { workspace = true, default-features = true } sp-runtime = { workspace = true, default-features = true } sp-state-machine = { workspace = true, default-features = true } +sp-blockchain = { workspace = true, default-features = true } serde = { workspace = true, default-features = true } serde_json = { workspace = true, default-features = true } derive_more = { features = ["display"], workspace = true } diff --git a/substrate/bin/node/bench/src/construct.rs b/substrate/bin/node/bench/src/construct.rs index 22129c6a1d69..6e34cb499a8a 100644 --- a/substrate/bin/node/bench/src/construct.rs +++ b/substrate/bin/node/bench/src/construct.rs @@ -271,7 +271,11 @@ impl sc_transaction_pool_api::TransactionPool for Transactions { unimplemented!() } - fn remove_invalid(&self, _hashes: &[TxHash]) -> Vec> { + fn report_invalid( + &self, + _at: Option, + _invalid_tx_errors: &[(TxHash, Option)], + ) -> Vec> { Default::default() } diff --git a/substrate/client/rpc-spec-v2/src/transaction/tests/middleware_pool.rs b/substrate/client/rpc-spec-v2/src/transaction/tests/middleware_pool.rs index a543969a89b8..124029121253 100644 --- a/substrate/client/rpc-spec-v2/src/transaction/tests/middleware_pool.rs +++ b/substrate/client/rpc-spec-v2/src/transaction/tests/middleware_pool.rs @@ -137,8 +137,12 @@ impl TransactionPool for MiddlewarePool { Ok(watcher.boxed()) } - fn remove_invalid(&self, hashes: &[TxHash]) -> Vec> { - self.inner_pool.remove_invalid(hashes) + fn report_invalid( + &self, + at: Option<::Hash>, + invalid_tx_errors: &[(TxHash, Option)], + ) -> Vec> { + self.inner_pool.report_invalid(at, invalid_tx_errors) } fn status(&self) -> PoolStatus { diff --git a/substrate/client/rpc-spec-v2/src/transaction/transaction_broadcast.rs b/substrate/client/rpc-spec-v2/src/transaction/transaction_broadcast.rs index 2fd4ce245456..2e077ef4bfac 100644 --- a/substrate/client/rpc-spec-v2/src/transaction/transaction_broadcast.rs +++ b/substrate/client/rpc-spec-v2/src/transaction/transaction_broadcast.rs @@ -228,7 +228,7 @@ where } // Best effort pool removal (tx can already be finalized). - pool.remove_invalid(&[broadcast_state.tx_hash]); + pool.report_invalid(None, &[(broadcast_state.tx_hash, None)]); }); // Keep track of this entry and the abortable handle. diff --git a/substrate/client/rpc/src/author/mod.rs b/substrate/client/rpc/src/author/mod.rs index 6afc871e565a..d6bb8863b8e9 100644 --- a/substrate/client/rpc/src/author/mod.rs +++ b/substrate/client/rpc/src/author/mod.rs @@ -164,17 +164,17 @@ where let hashes = bytes_or_hash .into_iter() .map(|x| match x { - hash::ExtrinsicOrHash::Hash(h) => Ok(h), + hash::ExtrinsicOrHash::Hash(h) => Ok((h, None)), hash::ExtrinsicOrHash::Extrinsic(bytes) => { let xt = Decode::decode(&mut &bytes[..])?; - Ok(self.pool.hash_of(&xt)) + Ok((self.pool.hash_of(&xt), None)) }, }) .collect::>>()?; Ok(self .pool - .remove_invalid(&hashes) + .report_invalid(None, &hashes) .into_iter() .map(|tx| tx.hash().clone()) .collect()) From 8c8e0336a2697c24d54cff8675c3de7e0ad5a733 Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Tue, 26 Nov 2024 19:14:02 +0100 Subject: [PATCH 3/4] basic-authorship: updated --- substrate/client/basic-authorship/src/basic_authorship.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/basic-authorship/src/basic_authorship.rs b/substrate/client/basic-authorship/src/basic_authorship.rs index 79e6fddae99f..8e0d21538959 100644 --- a/substrate/client/basic-authorship/src/basic_authorship.rs +++ b/substrate/client/basic-authorship/src/basic_authorship.rs @@ -512,7 +512,7 @@ where target: LOG_TARGET, "[{:?}] Invalid transaction: {} at: {}", pending_tx_hash, e, self.parent_hash ); - unqueue_invalid.push(pending_tx_hash); + unqueue_invalid.push((pending_tx_hash, Some(e))); }, } }; @@ -524,7 +524,7 @@ where ); } - self.transaction_pool.remove_invalid(&unqueue_invalid); + self.transaction_pool.report_invalid(Some(self.parent_hash), &unqueue_invalid); Ok(end_reason) } From 18ceac8129509505341711e8d672ecb15875fff5 Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Tue, 26 Nov 2024 19:28:46 +0100 Subject: [PATCH 4/4] Cargo.lock updated --- Cargo.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.lock b/Cargo.lock index c2d2eb3e9644..d20143ac837c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11107,6 +11107,7 @@ dependencies = [ "sc-transaction-pool-api", "serde", "serde_json", + "sp-blockchain", "sp-consensus", "sp-core 28.0.0", "sp-inherents 26.0.0",