Skip to content

Commit 6efe9f5

Browse files
michalkucharczykiulianbarbugithub-actions[bot]
authored
fatxpool: limits handling optimizations and fixes (#8596)
This PR adds some optimization and fixes in handling limits in fork-aware transaction pool. #### Notes for reviewers Changes made (random order): - debug levels adjusted in numerous places places ( `debug -> trace` for tx, `trace -> debug` for general flow) for better readablity, - internal TxMemPool [storage](https://github.com/paritytech/polkadot-sdk/blob/74dafaee5c600fd2c8a59a280f647f94ccf0a755/substrate/client/transaction-pool/src/fork_aware_txpool/tx_mem_pool.rs#L268-L272) is now [sorted](https://github.com/paritytech/polkadot-sdk/blob/74dafaee5c600fd2c8a59a280f647f94ccf0a755/substrate/client/transaction-pool/src/fork_aware_txpool/tx_mem_pool/tx_mem_pool_map.rs#L123-L132). A new helper exposes methods to reduce number of transaction clones (e.g. [here](https://github.com/paritytech/polkadot-sdk/blob/74dafaee5c600fd2c8a59a280f647f94ccf0a755/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs#L1404-L1410), see [previous verions](https://github.com/paritytech/polkadot-sdk/blob/2863b7a9a879935ff16987d0e95065d088dad9f8/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs#L1352-L1359)). This new structure eliminates the necessity of sorting transactions on the fly which was a not efficient, naive, [first](https://github.com/paritytech/polkadot-sdk/blob/e44b89fb7ca3385f314803c733ad97b26cd14e9f/substrate/client/transaction-pool/src/fork_aware_txpool/tx_mem_pool.rs#L384-L400) implementation. - some _mutexes_ were migrated to `tokio::sync::Mutex` to avoid tokio threads locking, - `sync` to `async` _[message](https://github.com/paritytech/polkadot-sdk/blob/74dafaee5c600fd2c8a59a280f647f94ccf0a755/substrate/client/transaction-pool/src/fork_aware_txpool/tx_mem_pool.rs#L743) based_ bridge was implemented. It's purpose is mainly to support `LocalTransactionPool / OffchainTransactionPool` infrastucture. `sync` methods can be called from both non-tokio and tokio context. This requires one additional [blocking](https://github.com/paritytech/polkadot-sdk/blob/74dafaee5c600fd2c8a59a280f647f94ccf0a755/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs#L398-L402) task for transaction pool. - `ViewStore::most_recent_view` is now a [reference](https://github.com/paritytech/polkadot-sdk/blob/74dafaee5c600fd2c8a59a280f647f94ccf0a755/substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs#L171) to `View`. - `TXMEMPOOL_TRANSACTION_LIMIT_MULTIPLIER` removed, there is no point for buffering more. Initially intended to work as buffer accommodating transactions from two different full views (which could have different set of transactions), turned out to be a bottleneck in maintain function (it still is but aligning sizes reduced the impact), - [bug](https://github.com/paritytech/polkadot-sdk/blob/74dafaee5c600fd2c8a59a280f647f94ccf0a755/substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs#L769) :see_no_evil: fixed in pre-insert actions removal - [`ValidateTransactionPriority`](https://github.com/paritytech/polkadot-sdk/blob/045bc6d342620a02ee9b28d8de51f72ae680f06f/substrate/client/transaction-pool/src/graph/pool.rs#L64-L71) was added. The goal is to allow faster processing of validation requests that were made from the `maintain` context. Otherwise all requests were landing in the same queue and maintain requests could be delayed. Now the processing power is evenly 50/50 split between _maintain_ and _submit+revalidate_ context. Related work: - michalkucharczyk/tx-test-tool#42, - #8152, Todo: - [x] some run should be done also for parachain, --------- Co-authored-by: Iulian Barbu <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 parent 8339a0f commit 6efe9f5

File tree

27 files changed

+2637
-595
lines changed

27 files changed

+2637
-595
lines changed

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

prdoc/pr_8596.prdoc

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
title: '`fatxpool`: limits handling optimizations and fixes'
2+
doc:
3+
- audience: Node Dev
4+
description: |-
5+
This PR adds some optimization and fixes in handling limits in fork-aware transaction pool.
6+
crates:
7+
- name: sc-transaction-pool
8+
bump: minor

substrate/client/transaction-pool/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ tracing = { workspace = true, default-features = true }
4949
anyhow = { workspace = true }
5050
assert_matches = { workspace = true }
5151
criterion = { workspace = true, default-features = true }
52+
rstest = { workspace = true }
5253
sc-block-builder = { workspace = true, default-features = true }
5354
sp-consensus = { workspace = true, default-features = true }
5455
substrate-test-runtime = { workspace = true }

substrate/client/transaction-pool/benches/basics.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,9 @@ impl ChainApi for TestApi {
6464
fn validate_transaction(
6565
&self,
6666
at: <Self::Block as BlockT>::Hash,
67-
_source: TransactionSource,
67+
_: TransactionSource,
6868
uxt: Arc<<Self::Block as BlockT>::Extrinsic>,
69+
_: ValidateTransactionPriority,
6970
) -> Self::ValidationFuture {
7071
let uxt = (*uxt).clone();
7172
let transfer = TransferData::try_from(&uxt)

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

Lines changed: 125 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,13 @@
1818

1919
//! Chain api required for the transaction pool.
2020
21-
use crate::LOG_TARGET;
22-
use codec::Encode;
23-
use futures::{
24-
channel::{mpsc, oneshot},
25-
future::{ready, Future, FutureExt, Ready},
26-
lock::Mutex,
27-
SinkExt, StreamExt,
21+
use crate::{
22+
common::{sliding_stat::DurationSlidingStats, STAT_SLIDING_WINDOW},
23+
graph::ValidateTransactionPriority,
24+
insert_and_log_throttled, LOG_TARGET, LOG_TARGET_STAT,
2825
};
29-
use std::{marker::PhantomData, pin::Pin, sync::Arc};
30-
26+
use codec::Encode;
27+
use futures::future::{ready, Future, FutureExt, Ready};
3128
use prometheus_endpoint::Registry as PrometheusRegistry;
3229
use sc_client_api::{blockchain::HeaderBackend, BlockBackend};
3330
use sp_api::{ApiExt, ProvideRuntimeApi};
@@ -39,38 +36,85 @@ use sp_runtime::{
3936
transaction_validity::{TransactionSource, TransactionValidity},
4037
};
4138
use sp_transaction_pool::runtime_api::TaggedTransactionQueue;
39+
use std::{
40+
marker::PhantomData,
41+
pin::Pin,
42+
sync::Arc,
43+
time::{Duration, Instant},
44+
};
45+
use tokio::sync::{mpsc, oneshot, Mutex};
4246

4347
use super::{
4448
error::{self, Error},
4549
metrics::{ApiMetrics, ApiMetricsExt},
4650
};
4751
use crate::graph;
48-
use tracing::{trace, warn};
52+
use tracing::{trace, warn, Level};
4953

5054
/// The transaction pool logic for full client.
5155
pub struct FullChainApi<Client, Block> {
5256
client: Arc<Client>,
5357
_marker: PhantomData<Block>,
5458
metrics: Option<Arc<ApiMetrics>>,
55-
validation_pool: mpsc::Sender<Pin<Box<dyn Future<Output = ()> + Send>>>,
59+
validation_pool_normal: mpsc::Sender<Pin<Box<dyn Future<Output = ()> + Send>>>,
60+
validation_pool_maintained: mpsc::Sender<Pin<Box<dyn Future<Output = ()> + Send>>>,
61+
validate_transaction_normal_stats: DurationSlidingStats,
62+
validate_transaction_maintained_stats: DurationSlidingStats,
5663
}
5764

5865
/// Spawn a validation task that will be used by the transaction pool to validate transactions.
5966
fn spawn_validation_pool_task(
6067
name: &'static str,
61-
receiver: Arc<Mutex<mpsc::Receiver<Pin<Box<dyn Future<Output = ()> + Send>>>>>,
68+
receiver_normal: Arc<Mutex<mpsc::Receiver<Pin<Box<dyn Future<Output = ()> + Send>>>>>,
69+
receiver_maintained: Arc<Mutex<mpsc::Receiver<Pin<Box<dyn Future<Output = ()> + Send>>>>>,
6270
spawner: &impl SpawnEssentialNamed,
71+
stats: DurationSlidingStats,
72+
blocking_stats: DurationSlidingStats,
6373
) {
6474
spawner.spawn_essential_blocking(
6575
name,
6676
Some("transaction-pool"),
6777
async move {
6878
loop {
69-
let task = receiver.lock().await.next().await;
70-
match task {
71-
None => return,
72-
Some(task) => task.await,
73-
}
79+
let start = Instant::now();
80+
81+
let task = {
82+
let receiver_maintained = receiver_maintained.clone();
83+
let receiver_normal = receiver_normal.clone();
84+
tokio::select! {
85+
Some(task) = async {
86+
receiver_maintained.lock().await.recv().await
87+
} => { task }
88+
Some(task) = async {
89+
receiver_normal.lock().await.recv().await
90+
} => { task }
91+
else => {
92+
return
93+
}
94+
}
95+
};
96+
97+
let blocking_duration = {
98+
let start = Instant::now();
99+
task.await;
100+
start.elapsed()
101+
};
102+
103+
insert_and_log_throttled!(
104+
Level::DEBUG,
105+
target:LOG_TARGET_STAT,
106+
prefix:format!("validate_transaction_inner_stats"),
107+
stats,
108+
start.elapsed().into()
109+
);
110+
insert_and_log_throttled!(
111+
Level::DEBUG,
112+
target:LOG_TARGET_STAT,
113+
prefix:format!("validate_transaction_blocking_stats"),
114+
blocking_stats,
115+
blocking_duration.into()
116+
);
117+
trace!(target:LOG_TARGET, duration=?start.elapsed(), "spawn_validation_pool_task");
74118
}
75119
}
76120
.boxed(),
@@ -84,6 +128,9 @@ impl<Client, Block> FullChainApi<Client, Block> {
84128
prometheus: Option<&PrometheusRegistry>,
85129
spawner: &impl SpawnEssentialNamed,
86130
) -> Self {
131+
let stats = DurationSlidingStats::new(Duration::from_secs(STAT_SLIDING_WINDOW));
132+
let blocking_stats = DurationSlidingStats::new(Duration::from_secs(STAT_SLIDING_WINDOW));
133+
87134
let metrics = prometheus.map(ApiMetrics::register).and_then(|r| match r {
88135
Err(error) => {
89136
warn!(
@@ -96,13 +143,41 @@ impl<Client, Block> FullChainApi<Client, Block> {
96143
Ok(api) => Some(Arc::new(api)),
97144
});
98145

99-
let (sender, receiver) = mpsc::channel(0);
146+
let (sender, receiver) = mpsc::channel(1);
147+
let (sender_maintained, receiver_maintained) = mpsc::channel(1);
100148

101149
let receiver = Arc::new(Mutex::new(receiver));
102-
spawn_validation_pool_task("transaction-pool-task-0", receiver.clone(), spawner);
103-
spawn_validation_pool_task("transaction-pool-task-1", receiver, spawner);
104-
105-
FullChainApi { client, validation_pool: sender, _marker: Default::default(), metrics }
150+
let receiver_maintained = Arc::new(Mutex::new(receiver_maintained));
151+
spawn_validation_pool_task(
152+
"transaction-pool-task-0",
153+
receiver.clone(),
154+
receiver_maintained.clone(),
155+
spawner,
156+
stats.clone(),
157+
blocking_stats.clone(),
158+
);
159+
spawn_validation_pool_task(
160+
"transaction-pool-task-1",
161+
receiver,
162+
receiver_maintained,
163+
spawner,
164+
stats.clone(),
165+
blocking_stats.clone(),
166+
);
167+
168+
FullChainApi {
169+
client,
170+
validation_pool_normal: sender,
171+
validation_pool_maintained: sender_maintained,
172+
_marker: Default::default(),
173+
metrics,
174+
validate_transaction_normal_stats: DurationSlidingStats::new(Duration::from_secs(
175+
STAT_SLIDING_WINDOW,
176+
)),
177+
validate_transaction_maintained_stats: DurationSlidingStats::new(Duration::from_secs(
178+
STAT_SLIDING_WINDOW,
179+
)),
180+
}
106181
}
107182
}
108183

@@ -132,10 +207,25 @@ where
132207
at: <Self::Block as BlockT>::Hash,
133208
source: TransactionSource,
134209
uxt: graph::ExtrinsicFor<Self>,
210+
validation_priority: ValidateTransactionPriority,
135211
) -> Self::ValidationFuture {
212+
let start = Instant::now();
136213
let (tx, rx) = oneshot::channel();
137214
let client = self.client.clone();
138-
let mut validation_pool = self.validation_pool.clone();
215+
let (stats, validation_pool, prefix) =
216+
if validation_priority == ValidateTransactionPriority::Maintained {
217+
(
218+
self.validate_transaction_maintained_stats.clone(),
219+
self.validation_pool_maintained.clone(),
220+
"validate_transaction_maintained_stats",
221+
)
222+
} else {
223+
(
224+
self.validate_transaction_normal_stats.clone(),
225+
self.validation_pool_normal.clone(),
226+
"validate_transaction_stats",
227+
)
228+
};
139229
let metrics = self.metrics.clone();
140230

141231
async move {
@@ -155,10 +245,20 @@ where
155245
.map_err(|e| Error::RuntimeApi(format!("Validation pool down: {:?}", e)))?;
156246
}
157247

158-
match rx.await {
248+
let validity = match rx.await {
159249
Ok(r) => r,
160250
Err(_) => Err(Error::RuntimeApi("Validation was canceled".into())),
161-
}
251+
};
252+
253+
insert_and_log_throttled!(
254+
Level::DEBUG,
255+
target:LOG_TARGET_STAT,
256+
prefix:prefix,
257+
stats,
258+
start.elapsed().into()
259+
);
260+
261+
validity
162262
}
163263
.boxed()
164264
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,17 @@ pub(crate) mod api;
2222
pub(crate) mod enactment_state;
2323
pub(crate) mod error;
2424
pub(crate) mod metrics;
25+
pub(crate) mod sliding_stat;
2526
#[cfg(test)]
2627
pub(crate) mod tests;
2728
pub(crate) mod tracing_log_xt;
2829

2930
use futures::StreamExt;
3031
use std::sync::Arc;
3132

33+
/// Stat sliding window, in seconds for per-transaction activities.
34+
pub(crate) const STAT_SLIDING_WINDOW: u64 = 3;
35+
3236
/// Inform the transaction pool about imported and finalized blocks.
3337
pub async fn notification_future<Client, Pool, Block>(client: Arc<Client>, txpool: Arc<Pool>)
3438
where

0 commit comments

Comments
 (0)