Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Transaction pool: Ensure that we prune transactions properly (#8963)
Browse files Browse the repository at this point in the history
* Transaction pool: Ensure that we prune transactions properly

There was a bug in the transaction pool that we didn't pruned
transactions properly because we called `prune_known`, instead of `prune`.

This bug was introduced by:
paritytech/substrate#4629

This is required to have stale extrinsics being removed properly, so
that they don't fill up the tx pool.

* Fix compilation

* Fix benches

* ...
bkchr authored and robin committed Jun 10, 2021
1 parent 3383d3e commit 513243d
Showing 12 changed files with 183 additions and 77 deletions.
2 changes: 1 addition & 1 deletion bin/node/cli/src/chain_spec.rs
Original file line number Diff line number Diff line change
@@ -614,7 +614,7 @@ pub(crate) mod tests {

#[test]
fn test_staging_test_net_chain_spec() {
staging_staging_config().build_storage().unwrap();
staging_config().build_storage().unwrap();
}

#[test]
4 changes: 2 additions & 2 deletions bin/node/executor/tests/basic.rs
Original file line number Diff line number Diff line change
@@ -382,7 +382,7 @@ fn full_native_block_import_works() {
);
assert_eq!(
Balances::total_balance(&bob()),
179 * DOLLARS - fees,
(179 * DOLLARS - fees) + 10_000_000,
);
let events = vec![
EventRecord {
@@ -483,7 +483,7 @@ fn full_wasm_block_import_works() {
);
assert_eq!(
Balances::total_balance(&bob()),
179 * DOLLARS - 1 * fees,
(179 * DOLLARS - 1 * fees) + 10_000_000,
);
});
}
4 changes: 2 additions & 2 deletions client/cli/src/commands/vanity.rs
Original file line number Diff line number Diff line change
@@ -196,7 +196,7 @@ mod tests {

#[test]
fn test_generation_with_single_char() {
let seed = generate_key::<sr25519::Pair>("ab", Default::default()).unwrap();
let seed = generate_key::<sr25519::Pair>("ab", Default::default(), false).unwrap();
assert!(
sr25519::Pair::from_seed_slice(&hex::decode(&seed[2..]).unwrap())
.unwrap()
@@ -208,7 +208,7 @@ mod tests {

#[test]
fn generate_key_respects_network_override() {
let seed = generate_key::<sr25519::Pair>("ab", Ss58AddressFormat::PolkadotAccount).unwrap();
let seed = generate_key::<sr25519::Pair>("ab", Ss58AddressFormat::PolkadotAccount, false).unwrap();
assert!(
sr25519::Pair::from_seed_slice(&hex::decode(&seed[2..]).unwrap())
.unwrap()
9 changes: 8 additions & 1 deletion client/transaction-pool/graph/benches/basics.rs
Original file line number Diff line number Diff line change
@@ -23,7 +23,7 @@ use sc_transaction_graph::*;
use codec::Encode;
use substrate_test_runtime::{Block, Extrinsic, Transfer, H256, AccountId};
use sp_runtime::{
generic::BlockId,
generic::BlockId, traits::Block as BlockT,
transaction_validity::{
ValidTransaction, InvalidTransaction, TransactionValidity, TransactionTag as Tag,
TransactionSource,
@@ -114,6 +114,13 @@ impl ChainApi for TestApi {
fn block_body(&self, _id: &BlockId<Self::Block>) -> Self::BodyFuture {
ready(Ok(None))
}

fn block_header(
&self,
_: &BlockId<Self::Block>,
) -> Result<Option<<Self::Block as BlockT>::Header>, Self::Error> {
Ok(None)
}
}

fn uxt(transfer: Transfer) -> Extrinsic {
8 changes: 2 additions & 6 deletions client/transaction-pool/graph/src/listener.rs
Original file line number Diff line number Diff line change
@@ -97,12 +97,8 @@ impl<H: hash::Hash + traits::Member + Serialize, C: ChainApi> Listener<H, C> {
}

/// Transaction was removed as invalid.
pub fn invalid(&mut self, tx: &H, warn: bool) {
if warn {
warn!(target: "txpool", "[{:?}] Extrinsic invalid", tx);
} else {
debug!(target: "txpool", "[{:?}] Extrinsic invalid", tx);
}
pub fn invalid(&mut self, tx: &H) {
debug!(target: "txpool", "[{:?}] Extrinsic invalid", tx);
self.fire(tx, |watcher| watcher.invalid());
}

15 changes: 14 additions & 1 deletion client/transaction-pool/graph/src/pool.rs
Original file line number Diff line number Diff line change
@@ -96,6 +96,12 @@ pub trait ChainApi: Send + Sync {

/// Returns a block body given the block id.
fn block_body(&self, at: &BlockId<Self::Block>) -> Self::BodyFuture;

/// Returns a block header given the block id.
fn block_header(
&self,
at: &BlockId<Self::Block>,
) -> Result<Option<<Self::Block as BlockT>::Header>, Self::Error>;
}

/// Pool configuration options.
@@ -238,7 +244,7 @@ impl<B: ChainApi> Pool<B> {
) -> Result<(), B::Error> {
// Get details of all extrinsics that are already in the pool
let in_pool_tags = self.validated_pool.extrinsics_tags(hashes)
.into_iter().filter_map(|x| x).flat_map(|x| x);
.into_iter().filter_map(|x| x).flatten();

// Prune all transactions that provide given tags
let prune_status = self.validated_pool.prune_tags(in_pool_tags)?;
@@ -583,6 +589,13 @@ mod tests {
fn block_body(&self, _id: &BlockId<Self::Block>) -> Self::BodyFuture {
futures::future::ready(Ok(None))
}

fn block_header(
&self,
_: &BlockId<Self::Block>,
) -> Result<Option<<Self::Block as BlockT>::Header>, Self::Error> {
Ok(None)
}
}

fn uxt(transfer: Transfer) -> Extrinsic {
26 changes: 11 additions & 15 deletions client/transaction-pool/graph/src/validated_pool.rs
Original file line number Diff line number Diff line change
@@ -232,7 +232,7 @@ impl<B: ChainApi> ValidatedPool<B> {
Err(err.into())
},
ValidatedTransaction::Unknown(hash, err) => {
self.listener.write().invalid(&hash, false);
self.listener.write().invalid(&hash);
Err(err.into())
},
}
@@ -417,18 +417,20 @@ impl<B: ChainApi> ValidatedPool<B> {
Status::Future => listener.future(&hash),
Status::Ready => listener.ready(&hash, None),
Status::Dropped => listener.dropped(&hash, None),
Status::Failed => listener.invalid(&hash, initial_status.is_some()),
Status::Failed => listener.invalid(&hash),
}
}
}
}

/// For each extrinsic, returns tags that it provides (if known), or None (if it is unknown).
pub fn extrinsics_tags(&self, hashes: &[ExtrinsicHash<B>]) -> Vec<Option<Vec<Tag>>> {
self.pool.read().by_hashes(&hashes)
self.pool.read()
.by_hashes(&hashes)
.into_iter()
.map(|existing_in_pool| existing_in_pool
.map(|transaction| transaction.provides.iter().cloned().collect()))
.map(|existing_in_pool|
existing_in_pool.map(|transaction| transaction.provides.to_vec())
)
.collect()
}

@@ -601,7 +603,7 @@ impl<B: ChainApi> ValidatedPool<B> {

let mut listener = self.listener.write();
for tx in &invalid {
listener.invalid(&tx.hash, true);
listener.invalid(&tx.hash);
}

invalid
@@ -640,15 +642,9 @@ fn fire_events<H, B, Ex>(
match *imported {
base::Imported::Ready { ref promoted, ref failed, ref removed, ref hash } => {
listener.ready(hash, None);
for f in failed {
listener.invalid(f, true);
}
for r in removed {
listener.dropped(&r.hash, Some(hash));
}
for p in promoted {
listener.ready(p, None);
}
failed.into_iter().for_each(|f| listener.invalid(f));
removed.into_iter().for_each(|r| listener.dropped(&r.hash, Some(hash)));
promoted.into_iter().for_each(|p| listener.ready(p, None));
},
base::Imported::Future { ref hash } => {
listener.future(hash)
20 changes: 17 additions & 3 deletions client/transaction-pool/src/api.rs
Original file line number Diff line number Diff line change
@@ -81,7 +81,7 @@ impl<Client, Block> FullChainApi<Client, Block> {
impl<Client, Block> sc_transaction_graph::ChainApi for FullChainApi<Client, Block>
where
Block: BlockT,
Client: ProvideRuntimeApi<Block> + BlockBackend<Block> + BlockIdTo<Block>,
Client: ProvideRuntimeApi<Block> + BlockBackend<Block> + BlockIdTo<Block> + HeaderBackend<Block>,
Client: Send + Sync + 'static,
Client::Api: TaggedTransactionQueue<Block>,
{
@@ -150,6 +150,13 @@ where
(<traits::HashFor::<Block> as traits::Hash>::hash(x), x.len())
})
}

fn block_header(
&self,
at: &BlockId<Self::Block>,
) -> Result<Option<<Self::Block as BlockT>::Header>, Self::Error> {
self.client.header(*at).map_err(Into::into)
}
}

/// Helper function to validate a transaction using a full chain API.
@@ -162,7 +169,7 @@ fn validate_transaction_blocking<Client, Block>(
) -> error::Result<TransactionValidity>
where
Block: BlockT,
Client: ProvideRuntimeApi<Block> + BlockBackend<Block> + BlockIdTo<Block>,
Client: ProvideRuntimeApi<Block> + BlockBackend<Block> + BlockIdTo<Block> + HeaderBackend<Block>,
Client: Send + Sync + 'static,
Client::Api: TaggedTransactionQueue<Block>,
{
@@ -193,7 +200,7 @@ where
impl<Client, Block> FullChainApi<Client, Block>
where
Block: BlockT,
Client: ProvideRuntimeApi<Block> + BlockBackend<Block> + BlockIdTo<Block>,
Client: ProvideRuntimeApi<Block> + BlockBackend<Block> + BlockIdTo<Block> + HeaderBackend<Block>,
Client: Send + Sync + 'static,
Client::Api: TaggedTransactionQueue<Block>,
{
@@ -333,4 +340,11 @@ impl<Client, F, Block> sc_transaction_graph::ChainApi for
Ok(Some(transactions))
}.boxed()
}

fn block_header(
&self,
at: &BlockId<Self::Block>,
) -> Result<Option<<Self::Block as BlockT>::Header>, Self::Error> {
self.client.header(*at).map_err(Into::into)
}
}
33 changes: 26 additions & 7 deletions client/transaction-pool/src/lib.rs
Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@ use parking_lot::Mutex;

use sp_runtime::{
generic::BlockId,
traits::{Block as BlockT, NumberFor, AtLeast32Bit, Extrinsic, Zero},
traits::{Block as BlockT, NumberFor, AtLeast32Bit, Extrinsic, Zero, Header as HeaderT},
};
use sp_core::traits::SpawnNamed;
use sp_transaction_pool::{
@@ -357,8 +357,13 @@ where
Block: BlockT,
Client: sp_api::ProvideRuntimeApi<Block>
+ sc_client_api::BlockBackend<Block>
+ sp_runtime::traits::BlockIdTo<Block>,
Client: sc_client_api::ExecutorProvider<Block> + Send + Sync + 'static,
+ sc_client_api::blockchain::HeaderBackend<Block>
+ sp_runtime::traits::BlockIdTo<Block>
+ sc_client_api::ExecutorProvider<Block>
+ sc_client_api::UsageProvider<Block>
+ Send
+ Sync
+ 'static,
Client::Api: sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block>,
{
/// Create new basic transaction pool for a full node with the provided api.
@@ -387,6 +392,7 @@ where
Block: BlockT,
Client: sp_api::ProvideRuntimeApi<Block>
+ sc_client_api::BlockBackend<Block>
+ sc_client_api::blockchain::HeaderBackend<Block>
+ sp_runtime::traits::BlockIdTo<Block>,
Client: Send + Sync + 'static,
Client::Api: sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block>,
@@ -523,19 +529,32 @@ async fn prune_known_txs_for_block<Block: BlockT, Api: ChainApi<Block = Block>>(
api: &Api,
pool: &sc_transaction_graph::Pool<Api>,
) -> Vec<ExtrinsicHash<Api>> {
let hashes = api.block_body(&block_id).await
let extrinsics = api.block_body(&block_id).await
.unwrap_or_else(|e| {
log::warn!("Prune known transactions: error request {:?}!", e);
None
})
.unwrap_or_default()
.into_iter()
.unwrap_or_default();

let hashes = extrinsics.iter()
.map(|tx| pool.hash_of(&tx))
.collect::<Vec<_>>();

log::trace!(target: "txpool", "Pruning transactions: {:?}", hashes);

if let Err(e) = pool.prune_known(&block_id, &hashes) {
let header = match api.block_header(&block_id) {
Ok(Some(h)) => h,
Ok(None) => {
log::debug!(target: "txpool", "Could not find header for {:?}.", block_id);
return hashes
},
Err(e) => {
log::debug!(target: "txpool", "Error retrieving header for {:?}: {:?}", block_id, e);
return hashes
}
};

if let Err(e) = pool.prune(&block_id, &BlockId::hash(*header.parent_hash()), &extrinsics).await {
log::error!("Cannot prune known in the pool {:?}!", e);
}

Loading

0 comments on commit 513243d

Please sign in to comment.