Fix tx-pool returning the same transaction multiple times (#6535)

* Fix tx-pool returning the same transaction multiple times

This fixes a bug that lead to returning the same transaction multiple
times when iterating the `ready` iterator. Internally the transaction
was kept in the `best` list and could be duplicated in that list be
re-inserting it again. This `best` list is using a `TransactionRef`
which internally uses a `insertion_id`. This `insertion_id` could lead
to the same transaction being inserted multiple times into the `best`
list.

* Update client/transaction-pool/src/testing/pool.rs

Co-authored-by: Nikolay Volf <nikvolf@gmail.com>

Co-authored-by: Nikolay Volf <nikvolf@gmail.com>

substrate/client/transaction-pool/src/revalidation.rs

@@ -141,14 +141,14 @@ impl<Api: ChainApi> RevalidationWorker<Api> {
 		// which they got into the pool
 		while left > 0 {
 			let first_block = match self.block_ordered.keys().next().cloned() {
-			  Some(bn) => bn,
-			  None => break,
+				Some(bn) => bn,
+				None => break,
 			};
 			let mut block_drained = false;
 			if let Some(extrinsics) = self.block_ordered.get_mut(&first_block) {
 				let to_queue = extrinsics.iter().take(left).cloned().collect::<Vec<_>>();
 				if to_queue.len() == extrinsics.len() {
-				   block_drained = true;
+					block_drained = true;
 				} else {
 					for xt in &to_queue {
 						extrinsics.remove(xt);
@@ -159,7 +159,7 @@ impl<Api: ChainApi> RevalidationWorker<Api> {
 			}
 
 			if block_drained {
-			  self.block_ordered.remove(&first_block);
+				self.block_ordered.remove(&first_block);
 			}
 		}
 

substrate/client/transaction-pool/src/testing/pool.rs

@@ -1066,3 +1066,28 @@ fn import_notification_to_pool_maintain_works() {
 	block_on(pool.maintain(evt.into()));
 	assert_eq!(pool.status().ready, 0);
 }
+
+// When we prune transactions, we need to make sure that we remove
+#[test]
+fn pruning_a_transaction_should_remove_it_from_best_transaction() {
+	let (pool, _guard, _notifier) = maintained_pool();
+
+	let xt1 = Extrinsic::IncludeData(Vec::new());
+
+	block_on(pool.submit_one(&BlockId::number(0), SOURCE, xt1.clone())).expect("1. Imported");
+	let header = pool.api.push_block(1, vec![xt1.clone()]);
+
+	// This will prune `xt1`.
+	block_on(pool.maintain(block_event(header)));
+
+	// Submit the tx again.
+	block_on(pool.submit_one(&BlockId::number(1), SOURCE, xt1.clone())).expect("2. Imported");
+
+	let mut iterator = block_on(pool.ready_at(1));
+
+	assert_eq!(iterator.next().unwrap().data, xt1.clone());
+
+	// If the tx was not removed from the best txs, the tx would be
+	// returned a second time by the iterator.
+	assert!(iterator.next().is_none());
+}