Transaction queue tests update (#4734)

* shuffle tests

* update tests

* inc_nonce -> increment_nonce

substrate/client/consensus/manual-seal/src/lib.rs

@@ -231,7 +231,7 @@ mod tests {
 	use sc_transaction_pool::{
 		BasicPool,
 		txpool::Options,
-		testing::*,
+		testing::api::*,
 	};
 	use sp_transaction_pool::TransactionPool;
 	use sp_runtime::generic::BlockId;
@@ -242,10 +242,7 @@ mod tests {
 	use sc_basic_authorship::ProposerFactory;
 
 	fn api() -> TestApi {
-		let mut api = TestApi::default();
-		api.nonce_offset = 0;
-
-		api
+		TestApi::empty()
 	}
 
 	#[tokio::test]
@@ -422,6 +419,7 @@ mod tests {
 			finalize: false,
 		}).await.unwrap();
 		let created_block = rx.await.unwrap().unwrap();
+		pool.api().increment_nonce(Alice.into());
 
 		// assert that the background task returns ok
 		assert_eq!(
@@ -451,6 +449,7 @@ mod tests {
 		}).await.is_ok());
 		assert!(rx1.await.unwrap().is_ok());
 		assert!(backend.blockchain().header(BlockId::Number(1)).unwrap().is_some());
+		pool.api().increment_nonce(Alice.into());
 
 		assert!(pool.submit_one(&BlockId::Number(2), uxt(Alice, 2)).await.is_ok());
 		let (tx2, rx2) = futures::channel::oneshot::channel();

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

@@ -22,8 +22,6 @@
 mod api;
 pub mod error;
 
-#[cfg(test)]
-mod tests;
 #[cfg(any(feature = "test-helpers", test))]
 pub mod testing;
 
@@ -114,7 +112,8 @@ impl<PoolApi, Block> BasicPool<PoolApi, Block>
 		&self.pool
 	}
 
-	#[cfg(test)]
+	/// Get reference to the inner chain api, for tests only.
+	#[cfg(any(feature = "test-helpers", test))]
 	pub fn api(&self) -> &Arc<PoolApi> {
 		&self.api
 	}

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

@@ -1,4 +1,4 @@
-// Copyright 2018-2020 Parity Technologies (UK) Ltd.
+// Copyright 2020 Parity Technologies (UK) Ltd.
 // This file is part of Substrate.
 
 // Substrate is free software: you can redistribute it and/or modify
@@ -14,56 +14,74 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
-//! Utilities for testing transaction pool
-use super::*;
+//! Test ChainApi
+
+use crate::*;
 use codec::Encode;
+use parking_lot::RwLock;
 use sp_runtime::{
-	generic::{BlockId},
-	traits::{Hash as HashT, BlakeTwo256},
+	generic::{self, BlockId},
+	traits::{BlakeTwo256, Hash as HashT},
 	transaction_validity::{TransactionValidity, ValidTransaction, TransactionValidityError, InvalidTransaction},
 };
 use std::collections::HashSet;
-use crate::BasicPool;
-use parking_lot::RwLock;
-use sc_transaction_graph::{self, ExHash, Pool};
 use substrate_test_runtime_client::{
-	runtime::{AccountId, Block, BlockNumber, Extrinsic, Hash, Header, Index, Transfer},
-	AccountKeyring,
+	runtime::{Index, AccountId, Block, BlockNumber, Extrinsic, Hash, Header, Transfer},
+	AccountKeyring::{self, *},
 };
 
-/// `ChainApi` for test environments
+#[derive(Default)]
+struct ChainState {
+	pub block_by_number: HashMap<BlockNumber, Vec<Extrinsic>>,
+	pub block_by_hash: HashMap<Hash, Vec<Extrinsic>>,
+	pub header_by_number: HashMap<BlockNumber, Header>,
+	pub nonces: HashMap<AccountId, u64>,
+	pub invalid_hashes: HashSet<Hash>,
+}
+
+/// Test Api for transaction pool.
 pub struct TestApi {
-	/// transaction modifier
-	pub modifier: RwLock<Box<dyn Fn(&mut ValidTransaction) + Send + Sync>>,
-	/// cache of block number to extrinsics
-	pub chain_block_by_number: RwLock<HashMap<BlockNumber, Vec<Extrinsic>>>,
-	/// cache of block number to header
-	pub chain_headers_by_number: RwLock<HashMap<BlockNumber, Header>>,
-	/// cache of invalid hashes
-	pub invalid_hashes: RwLock<HashSet<ExHash<Self>>>,
-	/// validation requests
-	pub validation_requests: RwLock<Vec<Extrinsic>>,
-	/// used for calculating nonce for extrinsics
-	pub nonce_offset: u64,
+	valid_modifier: RwLock<Box<dyn Fn(&mut ValidTransaction) + Send + Sync>>,
+	chain: RwLock<ChainState>,
+	validation_requests: RwLock<Vec<Extrinsic>>,
 }
 
 impl TestApi {
-	/// create an instance of `TestApi`
-	pub fn default() -> Self {
-		TestApi {
-			modifier: RwLock::new(Box::new(|_| {})),
-			chain_block_by_number: RwLock::new(HashMap::new()),
-			invalid_hashes: RwLock::new(HashSet::new()),
-			chain_headers_by_number: RwLock::new(HashMap::new()),
+
+	/// Test Api with Alice nonce set initially.
+	pub fn with_alice_nonce(nonce: u64) -> Self {
+		let api = TestApi {
+			valid_modifier: RwLock::new(Box::new(|_| {})),
+			chain: Default::default(),
 			validation_requests: RwLock::new(Default::default()),
-			nonce_offset: 209,
-		}
+		};
+
+		api.chain.write().nonces.insert(Alice.into(), nonce);
+
+		api
 	}
 
-	/// add a block to `TestApi`
+	/// Default Test Api
+	pub fn empty() -> Self {
+		let api = TestApi {
+			valid_modifier: RwLock::new(Box::new(|_| {})),
+			chain: Default::default(),
+			validation_requests: RwLock::new(Default::default()),
+		};
+
+		api
+	}
+
+	/// Set hook on modify valid result of transaction.
+	pub fn set_valid_modifier(&self, modifier: Box<dyn Fn(&mut ValidTransaction) + Send + Sync>) {
+		*self.valid_modifier.write() = modifier;
+	}
+
+	/// Push block as a part of canonical chain under given number.
 	pub fn push_block(&self, block_number: BlockNumber, xts: Vec<Extrinsic>) {
-		self.chain_block_by_number.write().insert(block_number, xts);
-		self.chain_headers_by_number.write().insert(block_number, Header {
+		let mut chain = self.chain.write();
+		chain.block_by_number.insert(block_number, xts);
+		chain.header_by_number.insert(block_number, Header {
 			number: block_number,
 			digest: Default::default(),
 			extrinsics_root:  Default::default(),
@@ -71,6 +89,40 @@ impl TestApi {
 			state_root: Default::default(),
 		});
 	}
+
+	/// Push a block without a number.
+	///
+	/// As a part of non-canonical chain.
+	pub fn push_fork_block(&self, block_hash: Hash, xts: Vec<Extrinsic>) {
+		let mut chain = self.chain.write();
+		chain.block_by_hash.insert(block_hash, xts);
+	}
+
+	fn hash_and_length_inner(ex: &Extrinsic) -> (Hash, usize) {
+		let encoded = ex.encode();
+		(BlakeTwo256::hash(&encoded), encoded.len())
+	}
+
+	/// Mark some transaction is invalid.
+	///
+	/// Next time transaction pool will try to validate this
+	/// extrinsic, api will return invalid result.
+	pub fn add_invalid(&self, xts: &Extrinsic) {
+		self.chain.write().invalid_hashes.insert(
+			Self::hash_and_length_inner(xts).0
+		);
+	}
+
+	/// Query validation requests received.
+	pub fn validation_requests(&self) -> Vec<Extrinsic> {
+		self.validation_requests.read().clone()
+	}
+
+	/// Increment nonce in the inner state.
+	pub fn increment_nonce(&self, account: AccountId) {
+		let mut chain = self.chain.write();
+		chain.nonces.entry(account).and_modify(|n| *n += 1).or_insert(1);
+	}
 }
 
 impl sc_transaction_graph::ChainApi for TestApi {
@@ -82,21 +134,20 @@ impl sc_transaction_graph::ChainApi for TestApi {
 
 	fn validate_transaction(
 		&self,
-		at: &BlockId<Self::Block>,
+		_at: &BlockId<Self::Block>,
 		uxt: sc_transaction_graph::ExtrinsicFor<Self>,
 	) -> Self::ValidationFuture {
-
 		self.validation_requests.write().push(uxt.clone());
 
-		let expected = index(at, self.nonce_offset);
-		let requires = if expected == uxt.transfer().nonce {
+		let chain_nonce = self.chain.read().nonces.get(&uxt.transfer().from).cloned().unwrap_or(0);
+		let requires = if chain_nonce == uxt.transfer().nonce {
 			vec![]
 		} else {
-			vec![vec![uxt.transfer().nonce as u8 - 1]]
+			vec![vec![chain_nonce as u8]]
 		};
 		let provides = vec![vec![uxt.transfer().nonce as u8]];
 
-		if self.invalid_hashes.read().contains(&self.hash_and_length(&uxt).0) {
+		if self.chain.read().invalid_hashes.contains(&self.hash_and_length(&uxt).0) {
 			return futures::future::ready(Ok(
 				Err(TransactionValidityError::Invalid(InvalidTransaction::Custom(0)))
 			))
@@ -110,7 +161,7 @@ impl sc_transaction_graph::ChainApi for TestApi {
 			propagate: true,
 		};
 
-		(self.modifier.read())(&mut validity);
+		(self.valid_modifier.read())(&mut validity);
 
 		futures::future::ready(Ok(Ok(validity)))
 	}
@@ -127,7 +178,7 @@ impl sc_transaction_graph::ChainApi for TestApi {
 		at: &BlockId<Self::Block>,
 	) -> error::Result<Option<sc_transaction_graph::BlockHash<Self>>> {
 		Ok(match at {
-			BlockId::Hash(x) => Some(x.clone()),
+			generic::BlockId::Hash(x) => Some(x.clone()),
 			_ => Some(Default::default()),
 		})
 	}
@@ -136,38 +187,28 @@ impl sc_transaction_graph::ChainApi for TestApi {
 		&self,
 		ex: &sc_transaction_graph::ExtrinsicFor<Self>,
 	) -> (Self::Hash, usize) {
-		let encoded = ex.encode();
-		(BlakeTwo256::hash(&encoded), encoded.len())
+		Self::hash_and_length_inner(ex)
 	}
 
 	fn block_body(&self, id: &BlockId<Self::Block>) -> Self::BodyFuture {
 		futures::future::ready(Ok(if let BlockId::Number(num) = id {
-			self.chain_block_by_number.read().get(num).cloned()
+			self.chain.read().block_by_number.get(num).cloned()
 		} else {
 			None
 		}))
 	}
 }
 
-/// get an instance of `BasicPool` with default options and `TestApi`
-/// as the `ChainApi`
-pub fn maintained_pool() -> BasicPool<TestApi, Block> {
-	BasicPool::new(Default::default(), TestApi::default())
-}
-
-/// generate nonce to be used with testing TestApi
-pub fn index(at: &BlockId<Block>, offset: u64) -> u64 {
-	offset + number_of(at)
-}
-
 fn number_of(at: &BlockId<Block>) -> u64 {
 	match at {
-		BlockId::Number(n) => *n as u64,
+		generic::BlockId::Number(n) => *n as u64,
 		_ => 0,
 	}
 }
 
-/// generates a transfer extrinsic, given a keyring and a nonce.
+/// Generate transfer extrinsic with a given nonce.
+///
+/// Part of the test api.
 pub fn uxt(who: AccountKeyring, nonce: Index) -> Extrinsic {
 	let transfer = Transfer {
 		from: who.into(),
@@ -179,7 +220,3 @@ pub fn uxt(who: AccountKeyring, nonce: Index) -> Extrinsic {
 	Extrinsic::Transfer(transfer, signature.into())
 }
 
-/// creates a transaction pool with the TestApi ChainApi
-pub fn pool() -> Pool<TestApi> {
-	Pool::new(Default::default(), Arc::new(TestApi::default()))
-}

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

@@ -0,0 +1,23 @@
+// Copyright 2020 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+//! Tests for top-level transaction pool api
+
+#[cfg(any(feature = "test-helpers", test))]
+pub mod api;
+
+#[cfg(test)]
+mod pool;

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

@@ -1,34 +1,28 @@
-// Copyright 2018-2020 Parity Technologies (UK) Ltd.
-// This file is part of Substrate.
-
-// Substrate is free software: you can redistribute it and/or modify
-// it under the terms of the GNU General Public License as published by
-// the Free Software Foundation, either version 3 of the License, or
-// (at your option) any later version.
-
-// Substrate is distributed in the hope that it will be useful,
-// but WITHOUT ANY WARRANTY; without even the implied warranty of
-// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-// GNU General Public License for more details.
-
-// You should have received a copy of the GNU General Public License
-// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
-
-use super::*;
 
+use crate::*;
+use sc_transaction_graph::{self, Pool};
 use futures::executor::block_on;
-use txpool::{self, Pool};
-use substrate_test_runtime_client::{runtime::Index, AccountKeyring::*};
 use sp_runtime::{
 	generic::BlockId,
 	transaction_validity::ValidTransaction,
 };
-use testing::*;
+use substrate_test_runtime_client::{
+	runtime::{Block, Hash, Index},
+	AccountKeyring::*,
+};
+use crate::testing::api::{TestApi, uxt};
+
+fn pool() -> Pool<TestApi> {
+	Pool::new(Default::default(), TestApi::with_alice_nonce(209).into())
+}
+
+fn maintained_pool() -> BasicPool<TestApi, Block> {
+	BasicPool::new(Default::default(), TestApi::with_alice_nonce(209))
+}
 
 #[test]
 fn submission_should_work() {
 	let pool = pool();
-	assert_eq!(209, index(&BlockId::number(0), 209));
 	block_on(pool.submit_one(&BlockId::number(0), uxt(Alice, 209))).unwrap();
 
 	let pending: Vec<_> = pool.ready().map(|a| a.data.transfer().nonce).collect();
@@ -70,13 +64,19 @@ fn late_nonce_should_be_queued() {
 #[test]
 fn prune_tags_should_work() {
 	let pool = pool();
-	block_on(pool.submit_one(&BlockId::number(0), uxt(Alice, 209))).unwrap();
+	let hash209 = block_on(pool.submit_one(&BlockId::number(0), uxt(Alice, 209))).unwrap();
 	block_on(pool.submit_one(&BlockId::number(0), uxt(Alice, 210))).unwrap();
 
 	let pending: Vec<_> = pool.ready().map(|a| a.data.transfer().nonce).collect();
 	assert_eq!(pending, vec![209, 210]);
 
-	block_on(pool.prune_tags(&BlockId::number(1), vec![vec![209]], vec![])).unwrap();
+	block_on(
+		pool.prune_tags(
+			&BlockId::number(1),
+			vec![vec![209]],
+			vec![hash209],
+		)
+	).expect("Prune tags");
 
 	let pending: Vec<_> = pool.ready().map(|a| a.data.transfer().nonce).collect();
 	assert_eq!(pending, vec![210]);
@@ -100,22 +100,24 @@ fn should_ban_invalid_transactions() {
 
 #[test]
 fn should_correctly_prune_transactions_providing_more_than_one_tag() {
-	let api = TestApi::default();
-	*api.modifier.write() = Box::new(|v: &mut ValidTransaction| {
+	let api = Arc::new(TestApi::with_alice_nonce(209));
+	api.set_valid_modifier(Box::new(|v: &mut ValidTransaction| {
 		v.provides.push(vec![155]);
-	});
-	let pool = Pool::new(Default::default(), Arc::new(api));
+	}));
+	let pool = Pool::new(Default::default(), api.clone());
 	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.
+	api.increment_nonce(Alice.into());
 	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
+	api.increment_nonce(Alice.into());
 	let xt = uxt(Alice, 211);
 	block_on(pool.submit_one(&BlockId::number(2), xt.clone())).expect("2. Imported");
 	assert_eq!(pool.status().ready, 1);
@@ -124,6 +126,7 @@ fn should_correctly_prune_transactions_providing_more_than_one_tag() {
 	assert_eq!(pending, vec![211]);
 
 	// prune it and make sure the pool is empty
+	api.increment_nonce(Alice.into());
 	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);
@@ -144,7 +147,6 @@ fn should_prune_old_during_maintenance() {
 	assert_eq!(pool.status().ready, 0);
 }
 
-
 #[test]
 fn should_revalidate_during_maintenance() {
 	let xt1 = uxt(Alice, 209);
@@ -154,12 +156,47 @@ fn should_revalidate_during_maintenance() {
 	block_on(pool.submit_one(&BlockId::number(0), xt1.clone())).expect("1. Imported");
 	block_on(pool.submit_one(&BlockId::number(0), xt2.clone())).expect("2. Imported");
 	assert_eq!(pool.status().ready, 2);
-	assert_eq!(pool.api.validation_requests.read().len(), 2);
+	assert_eq!(pool.api.validation_requests().len(), 2);
 
 	pool.api.push_block(1, vec![xt1.clone()]);
 
 	block_on(pool.maintain(&BlockId::number(1), &[]));
 	assert_eq!(pool.status().ready, 1);
 	// test that pool revalidated transaction that left ready and not included in the block
-	assert_eq!(pool.api.validation_requests.read().len(), 3);
+	assert_eq!(pool.api.validation_requests().len(), 3);
+}
+
+#[test]
+fn should_resubmit_from_retracted_during_maintaince() {
+	let xt = uxt(Alice, 209);
+	let retracted_hash = Hash::random();
+
+	let pool = maintained_pool();
+
+	block_on(pool.submit_one(&BlockId::number(0), xt.clone())).expect("1. Imported");
+	assert_eq!(pool.status().ready, 1);
+
+	pool.api.push_block(1, vec![]);
+	pool.api.push_fork_block(retracted_hash, vec![xt.clone()]);
+
+	block_on(pool.maintain(&BlockId::number(1), &[retracted_hash]));
+	assert_eq!(pool.status().ready, 1);
+}
+
+#[test]
+fn should_not_retain_invalid_hashes_from_retracted() {
+	let xt = uxt(Alice, 209);
+	let retracted_hash = Hash::random();
+
+	let pool = maintained_pool();
+
+	block_on(pool.submit_one(&BlockId::number(0), xt.clone())).expect("1. Imported");
+	assert_eq!(pool.status().ready, 1);
+
+	pool.api.push_block(1, vec![]);
+	pool.api.push_fork_block(retracted_hash, vec![xt.clone()]);
+	pool.api.add_invalid(&xt);
+
+	block_on(pool.maintain(&BlockId::number(1), &[retracted_hash]));
+	assert_eq!(pool.status().ready, 0);
 }