From a31ff590f6fe1612199efa399ebd9d676e611cf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Jun 2020 17:52:13 +0200 Subject: [PATCH 1/2] 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. --- client/transaction-pool/graph/src/ready.rs | 24 ++++++++---------- client/transaction-pool/src/revalidation.rs | 8 +++--- client/transaction-pool/src/testing/pool.rs | 25 +++++++++++++++++++ test-utils/runtime/src/lib.rs | 14 +++++++++-- .../runtime/transaction-pool/src/lib.rs | 16 ++++++++---- 5 files changed, 63 insertions(+), 24 deletions(-) diff --git a/client/transaction-pool/graph/src/ready.rs b/client/transaction-pool/graph/src/ready.rs index 47289f26f02a9..b98512b05d5cf 100644 --- a/client/transaction-pool/graph/src/ready.rs +++ b/client/transaction-pool/graph/src/ready.rs @@ -275,12 +275,7 @@ impl ReadyTransactions { ) -> Vec>> { let mut removed = vec![]; let mut ready = self.ready.write(); - loop { - let hash = match to_remove.pop() { - Some(hash) => hash, - None => return removed, - }; - + while let Some(hash) = to_remove.pop() { if let Some(mut tx) = ready.remove(&hash) { let invalidated = tx.transaction.transaction.provides .iter() @@ -319,6 +314,8 @@ impl ReadyTransactions { removed.push(tx.transaction.transaction); } } + + removed } /// Removes transactions that provide given tag. @@ -330,17 +327,16 @@ impl ReadyTransactions { let mut removed = vec![]; let mut to_remove = vec![tag]; - loop { - let tag = match to_remove.pop() { - Some(tag) => tag, - None => return removed, - }; - + while let Some(tag) = to_remove.pop() { let res = self.provided_tags.remove(&tag) - .and_then(|hash| self.ready.write().remove(&hash)); + .and_then(|hash| self.ready.write().remove(&hash)); if let Some(tx) = res { let unlocks = tx.unlocks; + + // Make sure we remove it from best txs + self.best.remove(&tx.transaction); + let tx = tx.transaction.transaction; // prune previous transactions as well @@ -403,6 +399,8 @@ impl ReadyTransactions { removed.push(tx); } } + + removed } /// Checks if the transaction is providing the same tags as other transactions. diff --git a/client/transaction-pool/src/revalidation.rs b/client/transaction-pool/src/revalidation.rs index cb49560662c85..af9a76c055b6b 100644 --- a/client/transaction-pool/src/revalidation.rs +++ b/client/transaction-pool/src/revalidation.rs @@ -141,14 +141,14 @@ impl RevalidationWorker { // 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::>(); 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 RevalidationWorker { } if block_drained { - self.block_ordered.remove(&first_block); + self.block_ordered.remove(&first_block); } } diff --git a/client/transaction-pool/src/testing/pool.rs b/client/transaction-pool/src/testing/pool.rs index 61aba5efe3bfa..440ccb1fa4468 100644 --- a/client/transaction-pool/src/testing/pool.rs +++ b/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("1. 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()); +} diff --git a/test-utils/runtime/src/lib.rs b/test-utils/runtime/src/lib.rs index 1d376a0940bbe..06054c1240f3b 100644 --- a/test-utils/runtime/src/lib.rs +++ b/test-utils/runtime/src/lib.rs @@ -194,10 +194,20 @@ impl sp_runtime::traits::Dispatchable for Extrinsic { } impl Extrinsic { + /// Convert `&self` into `&Transfer`. + /// + /// Panics if this is no `Transfer` extrinsic. pub fn transfer(&self) -> &Transfer { + self.try_transfer().expect("cannot convert to transfer ref") + } + + /// Try to convert `&self` into `&Transfer`. + /// + /// Returns `None` if this is no `Transfer` extrinsic. + pub fn try_transfer(&self) -> Option<&Transfer> { match self { - Extrinsic::Transfer { ref transfer, .. } => transfer, - _ => panic!("cannot convert to transfer ref"), + Extrinsic::Transfer { ref transfer, .. } => Some(transfer), + _ => None, } } } diff --git a/test-utils/runtime/transaction-pool/src/lib.rs b/test-utils/runtime/transaction-pool/src/lib.rs index 5140cb8b9258f..17cecd394ab91 100644 --- a/test-utils/runtime/transaction-pool/src/lib.rs +++ b/test-utils/runtime/transaction-pool/src/lib.rs @@ -209,13 +209,19 @@ impl sc_transaction_graph::ChainApi for TestApi { ) -> Self::ValidationFuture { self.validation_requests.write().push(uxt.clone()); - let chain_nonce = self.chain.read().nonces.get(&uxt.transfer().from).cloned().unwrap_or(0); - let requires = if chain_nonce == uxt.transfer().nonce { - vec![] + let (requires, provides) = if let Some(transfer) = uxt.try_transfer() { + let chain_nonce = self.chain.read().nonces.get(&transfer.from).cloned().unwrap_or(0); + let requires = if chain_nonce == transfer.nonce { + vec![] + } else { + vec![vec![chain_nonce as u8]] + }; + let provides = vec![vec![transfer.nonce as u8]]; + + (requires, provides) } else { - vec![vec![chain_nonce as u8]] + (Vec::new(), vec![uxt.encode()]) }; - let provides = vec![vec![uxt.transfer().nonce as u8]]; if self.chain.read().invalid_hashes.contains(&self.hash_and_length(&uxt).0) { return futures::future::ready(Ok( From 4cce62cfedb97c3951e7329a979af9fdbb81d6ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Jun 2020 18:58:55 +0200 Subject: [PATCH 2/2] Update client/transaction-pool/src/testing/pool.rs Co-authored-by: Nikolay Volf --- client/transaction-pool/src/testing/pool.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/transaction-pool/src/testing/pool.rs b/client/transaction-pool/src/testing/pool.rs index 440ccb1fa4468..5ad79a6f75d87 100644 --- a/client/transaction-pool/src/testing/pool.rs +++ b/client/transaction-pool/src/testing/pool.rs @@ -1081,7 +1081,7 @@ fn pruning_a_transaction_should_remove_it_from_best_transaction() { block_on(pool.maintain(block_event(header))); // Submit the tx again. - block_on(pool.submit_one(&BlockId::number(1), SOURCE, xt1.clone())).expect("1. Imported"); + block_on(pool.submit_one(&BlockId::number(1), SOURCE, xt1.clone())).expect("2. Imported"); let mut iterator = block_on(pool.ready_at(1));