-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix transaction pool & network issues #6288
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,8 +34,8 @@ pub mod testing; | |
pub use sc_transaction_graph as txpool; | ||
pub use crate::api::{FullChainApi, LightChainApi}; | ||
|
||
use std::{collections::HashMap, sync::Arc, pin::Pin}; | ||
use futures::{prelude::*, future::{ready, self}, channel::oneshot}; | ||
use std::{collections::{HashMap, HashSet}, sync::Arc, pin::Pin}; | ||
use futures::{prelude::*, future::{self, ready}, channel::oneshot}; | ||
use parking_lot::Mutex; | ||
|
||
use sp_runtime::{ | ||
|
@@ -47,7 +47,7 @@ use sp_transaction_pool::{ | |
TransactionStatusStreamFor, MaintainedTransactionPool, PoolFuture, ChainEvent, | ||
TransactionSource, | ||
}; | ||
use sc_transaction_graph::ChainApi; | ||
use sc_transaction_graph::{ChainApi, ExtrinsicHash}; | ||
use wasm_timer::Instant; | ||
|
||
use prometheus_endpoint::Registry as PrometheusRegistry; | ||
|
@@ -440,6 +440,7 @@ async fn prune_known_txs_for_block<Block: BlockT, Api: ChainApi<Block = Block>>( | |
block_id: BlockId<Block>, | ||
api: &Api, | ||
pool: &sc_transaction_graph::Pool<Api>, | ||
pruned_log: &mut HashSet<ExtrinsicHash<Api>>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You remove this and return a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should at least try to avoid allocations? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already allocate this vector in the function and just use the slice. So, we don't do any double allocations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right 🤦♀️ |
||
) { | ||
// We don't query block if we won't prune anything | ||
if pool.validated_pool().status().is_empty() { | ||
|
@@ -456,6 +457,8 @@ async fn prune_known_txs_for_block<Block: BlockT, Api: ChainApi<Block = Block>>( | |
.map(|tx| pool.hash_of(&tx)) | ||
.collect::<Vec<_>>(); | ||
|
||
pruned_log.extend(&hashes); | ||
|
||
if let Err(e) = pool.prune_known(&block_id, &hashes) { | ||
log::error!("Cannot prune known in the pool {:?}!", e); | ||
} | ||
|
@@ -495,26 +498,40 @@ impl<PoolApi, Block> MaintainedTransactionPool for BasicPool<PoolApi, Block> | |
let ready_poll = self.ready_poll.clone(); | ||
|
||
async move { | ||
// We keep track of everything we prune so that later we won't add | ||
// tranactions with those hashes from the retracted blocks. | ||
let mut pruned_log = HashSet::<ExtrinsicHash<PoolApi>>::new(); | ||
|
||
// If there is a tree route, we use this to prune known tx based on the enacted | ||
// blocks. | ||
if let Some(ref tree_route) = tree_route { | ||
future::join_all( | ||
for enacted_log in future::join_all( | ||
tree_route | ||
.enacted() | ||
.iter() | ||
.map(|h| | ||
NikVolf marked this conversation as resolved.
Show resolved
Hide resolved
|
||
prune_known_txs_for_block( | ||
BlockId::Hash(h.hash.clone()), | ||
&*api, | ||
&*pool, | ||
), | ||
), | ||
).await; | ||
.map(|enacted| { | ||
let api = api.clone(); | ||
let pool = pool.clone(); | ||
async move { | ||
let mut log = HashSet::<ExtrinsicHash<PoolApi>>::new(); | ||
prune_known_txs_for_block( | ||
BlockId::Hash(enacted.hash.clone()), | ||
&*api, | ||
&*pool, | ||
&mut log, | ||
).await; | ||
log | ||
} | ||
} | ||
) | ||
).await { | ||
pruned_log.extend(enacted_log); | ||
} | ||
} | ||
|
||
// If this is a new best block, we need to prune its transactions from the pool. | ||
if is_new_best { | ||
prune_known_txs_for_block(id.clone(), &*api, &*pool).await; | ||
prune_known_txs_for_block(id.clone(), &*api, &*pool, &mut pruned_log).await; | ||
} | ||
|
||
if let (true, Some(tree_route)) = (next_action.resubmit, tree_route) { | ||
|
@@ -536,7 +553,16 @@ impl<PoolApi, Block> MaintainedTransactionPool for BasicPool<PoolApi, Block> | |
.into_iter() | ||
.filter(|tx| tx.is_signed().unwrap_or(true)); | ||
|
||
resubmit_transactions.extend(block_transactions); | ||
resubmit_transactions.extend( | ||
block_transactions.into_iter().filter(|tx| { | ||
let tx_hash = pool.hash_of(&tx); | ||
let contains = pruned_log.contains(&tx_hash); | ||
if !contains { | ||
log::debug!(target: "txpool", "[{:?}]: Resubmitting from retracted block {:?}", tx_hash, hash); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More a trace log? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nah, needed for debugging why transaction is reentered the pool |
||
} | ||
!contains | ||
}) | ||
); | ||
} | ||
|
||
if let Err(e) = pool.submit_at( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about chains with longer block time / larger tx per block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xlc let's make an issue for that
This should not be a const but rather some sort of configuration (runtime/cli or deduced from other values)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only stores the hash of a transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean larger number of tx per block