From 06433c98896de9ea435ca276d846d01de662ed5c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= <tomusdrw@users.noreply.github.com>
Date: Mon, 28 Oct 2019 16:06:20 +0100
Subject: [PATCH] Fix a import+prune+replace case for multi-provides
transactions. (#3939)
* Fix a import+prune+replace case for multi-provides transactions.
* Fix tests.
---
.../core/transaction-pool/graph/src/pool.rs | 3 +
.../core/transaction-pool/graph/src/ready.rs | 15 ++++-
substrate/core/transaction-pool/src/tests.rs | 57 ++++++++++++++++---
3 files changed, 65 insertions(+), 10 deletions(-)
diff --git a/substrate/core/transaction-pool/graph/src/pool.rs b/substrate/core/transaction-pool/graph/src/pool.rs
index 97244f1cec3..dcb54f710f1 100644
--- a/substrate/core/transaction-pool/graph/src/pool.rs
+++ b/substrate/core/transaction-pool/graph/src/pool.rs
@@ -240,6 +240,7 @@ impl<B: ChainApi> Pool<B> {
tags: impl IntoIterator<Item=Tag>,
known_imported_hashes: impl IntoIterator<Item=ExHash<B>> + Clone,
) -> impl Future<Output=Result<(), B::Error>> {
+ log::trace!(target: "txpool", "Pruning at {:?}", at);
// Prune all transactions that provide given tags
let prune_status = match self.validated_pool.prune_tags(tags) {
Ok(prune_status) => prune_status,
@@ -257,6 +258,7 @@ impl<B: ChainApi> Pool<B> {
let pruned_transactions = prune_status.pruned.into_iter().map(|tx| tx.data.clone());
let reverify_future = self.verify(at, pruned_transactions, false);
+ log::trace!(target: "txpool", "Prunning at {:?}. Resubmitting transactions.", at);
// And finally - submit reverified transactions back to the pool
let at = at.clone();
let validated_pool = self.validated_pool.clone();
@@ -908,3 +910,4 @@ mod tests {
}
}
}
+
diff --git a/substrate/core/transaction-pool/graph/src/ready.rs b/substrate/core/transaction-pool/graph/src/ready.rs
index 85bb4dd783c..3698bf447ee 100644
--- a/substrate/core/transaction-pool/graph/src/ready.rs
+++ b/substrate/core/transaction-pool/graph/src/ready.rs
@@ -338,7 +338,20 @@ impl<Hash: hash::Hash + Member + Serialize, Ex> ReadyTransactions<Hash, Ex> {
}
}
- debug!(target: "txpool", "[{:?}] Pruned.", tx.hash);
+ // we also need to remove all other tags that this transaction provides,
+ // but since all the hard work is done, we only clear the provided_tag -> hash
+ // mapping.
+ let current_tag = &tag;
+ for tag in &tx.provides {
+ let removed = self.provided_tags.remove(tag);
+ assert_eq!(
+ removed.as_ref(),
+ if current_tag == tag { None } else { Some(&tx.hash) },
+ "The pool contains exactly one transaction providing given tag; the removed transaction
+ claims to provide that tag, so it has to be mapped to it's hash; qed"
+ );
+ }
+
removed.push(tx);
}
}
diff --git a/substrate/core/transaction-pool/src/tests.rs b/substrate/core/transaction-pool/src/tests.rs
index d1ad27dd260..60a9e0562fc 100644
--- a/substrate/core/transaction-pool/src/tests.rs
+++ b/substrate/core/transaction-pool/src/tests.rs
@@ -27,11 +27,15 @@ use sr_primitives::{
transaction_validity::{TransactionValidity, ValidTransaction},
};
-struct TestApi;
+struct TestApi {
+ pub modifier: Box<dyn Fn(&mut ValidTransaction) + Send + Sync>,
+}
impl TestApi {
fn default() -> Self {
- TestApi
+ TestApi {
+ modifier: Box::new(|_| {}),
+ }
}
}
@@ -54,14 +58,18 @@ impl txpool::ChainApi for TestApi {
};
let provides = vec![vec![uxt.transfer().nonce as u8]];
+ let mut validity = ValidTransaction {
+ priority: 1,
+ requires,
+ provides,
+ longevity: 64,
+ propagate: true,
+ };
+
+ (self.modifier)(&mut validity);
+
futures::future::ready(Ok(
- Ok(ValidTransaction {
- priority: 1,
- requires,
- provides,
- longevity: 64,
- propagate: true,
- })
+ Ok(validity)
))
}
@@ -181,3 +189,34 @@ fn should_ban_invalid_transactions() {
// then
block_on(pool.submit_one(&BlockId::number(0), uxt.clone())).unwrap_err();
}
+
+#[test]
+fn should_correctly_prune_transactions_providing_more_than_one_tag() {
+ let mut api = TestApi::default();
+ api.modifier = Box::new(|v: &mut ValidTransaction| {
+ v.provides.push(vec![155]);
+ });
+ let pool = Pool::new(Default::default(), api);
+ let xt = uxt(Alice, 209);
+ block_on(pool.submit_one(&BlockId::number(0), xt.clone())).expect("1. Imported");
+ assert_eq!(pool.status().ready, 1);
+
+ // remove the transaction that just got imported.
+ block_on(pool.prune_tags(&BlockId::number(1), vec![vec![209]], vec![])).expect("1. Pruned");
+ assert_eq!(pool.status().ready, 0);
+ // it's re-imported to future
+ assert_eq!(pool.status().future, 1);
+
+ // so now let's insert another transaction that also provides the 155
+ let xt = uxt(Alice, 211);
+ block_on(pool.submit_one(&BlockId::number(2), xt.clone())).expect("2. Imported");
+ assert_eq!(pool.status().ready, 1);
+ assert_eq!(pool.status().future, 1);
+ let pending: Vec<_> = pool.ready().map(|a| a.data.transfer().nonce).collect();
+ assert_eq!(pending, vec![211]);
+
+ // prune it and make sure the pool is empty
+ block_on(pool.prune_tags(&BlockId::number(3), vec![vec![155]], vec![])).expect("2. Pruned");
+ assert_eq!(pool.status().ready, 0);
+ assert_eq!(pool.status().future, 2);
+}
--
GitLab