Fix a import+prune+replace case for multi-provides transactions. (#3939)

* Fix a import+prune+replace case for multi-provides transactions.

* Fix tests.

Author: tomusdrw

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 {
 		}
 	}
 }
+

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);
 			}
 		}

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);
+}