Remove transaction-pool test-helpers feature (#10571)

* Remove transaction-pool `test-helpers` feature

`test-helpers` feature is a bad idea in general, because once the feature is enabled somewhere in
the workspace, it is enabled anywhere. While removing the feature, the tests were also rewritten to
get rid off other "only test" related code.

Contributes towards: https://github.com/paritytech/substrate/issues/9727

* Apply suggestions from code review

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

* Fix benches

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

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

@@ -444,164 +444,17 @@ impl<B: ChainApi> Clone for Pool<B> {
 #[cfg(test)]
 mod tests {
 	use super::{super::base_pool::Limit, *};
+	use crate::tests::{pool, uxt, TestApi, INVALID_NONCE};
 	use assert_matches::assert_matches;
-	use codec::Encode;
 	use futures::executor::block_on;
 	use parking_lot::Mutex;
 	use sc_transaction_pool_api::TransactionStatus;
-	use sp_runtime::{
-		traits::Hash,
-		transaction_validity::{InvalidTransaction, TransactionSource, ValidTransaction},
-	};
-	use std::{
-		collections::{HashMap, HashSet},
-		time::Instant,
-	};
-	use substrate_test_runtime::{AccountId, Block, Extrinsic, Hashing, Transfer, H256};
+	use sp_runtime::transaction_validity::TransactionSource;
+	use std::{collections::HashMap, time::Instant};
+	use substrate_test_runtime::{AccountId, Extrinsic, Transfer, H256};
 
-	const INVALID_NONCE: u64 = 254;
 	const SOURCE: TransactionSource = TransactionSource::External;
 
-	#[derive(Clone, Debug, Default)]
-	struct TestApi {
-		delay: Arc<Mutex<Option<std::sync::mpsc::Receiver<()>>>>,
-		invalidate: Arc<Mutex<HashSet<H256>>>,
-		clear_requirements: Arc<Mutex<HashSet<H256>>>,
-		add_requirements: Arc<Mutex<HashSet<H256>>>,
-	}
-
-	impl ChainApi for TestApi {
-		type Block = Block;
-		type Error = error::Error;
-		type ValidationFuture = futures::future::Ready<error::Result<TransactionValidity>>;
-		type BodyFuture = futures::future::Ready<error::Result<Option<Vec<Extrinsic>>>>;
-
-		/// Verify extrinsic at given block.
-		fn validate_transaction(
-			&self,
-			at: &BlockId<Self::Block>,
-			_source: TransactionSource,
-			uxt: ExtrinsicFor<Self>,
-		) -> Self::ValidationFuture {
-			let hash = self.hash_and_length(&uxt).0;
-			let block_number = self.block_id_to_number(at).unwrap().unwrap();
-
-			let res = match uxt {
-				Extrinsic::Transfer { transfer, .. } => {
-					let nonce = transfer.nonce;
-
-					// This is used to control the test flow.
-					if nonce > 0 {
-						let opt = self.delay.lock().take();
-						if let Some(delay) = opt {
-							if delay.recv().is_err() {
-								println!("Error waiting for delay!");
-							}
-						}
-					}
-
-					if self.invalidate.lock().contains(&hash) {
-						InvalidTransaction::Custom(0).into()
-					} else if nonce < block_number {
-						InvalidTransaction::Stale.into()
-					} else {
-						let mut transaction = ValidTransaction {
-							priority: 4,
-							requires: if nonce > block_number {
-								vec![vec![nonce as u8 - 1]]
-							} else {
-								vec![]
-							},
-							provides: if nonce == INVALID_NONCE {
-								vec![]
-							} else {
-								vec![vec![nonce as u8]]
-							},
-							longevity: 3,
-							propagate: true,
-						};
-
-						if self.clear_requirements.lock().contains(&hash) {
-							transaction.requires.clear();
-						}
-
-						if self.add_requirements.lock().contains(&hash) {
-							transaction.requires.push(vec![128]);
-						}
-
-						Ok(transaction)
-					}
-				},
-				Extrinsic::IncludeData(_) => Ok(ValidTransaction {
-					priority: 9001,
-					requires: vec![],
-					provides: vec![vec![42]],
-					longevity: 9001,
-					propagate: false,
-				}),
-				Extrinsic::Store(_) => Ok(ValidTransaction {
-					priority: 9001,
-					requires: vec![],
-					provides: vec![vec![43]],
-					longevity: 9001,
-					propagate: false,
-				}),
-				_ => unimplemented!(),
-			};
-
-			futures::future::ready(Ok(res))
-		}
-
-		/// Returns a block number given the block id.
-		fn block_id_to_number(
-			&self,
-			at: &BlockId<Self::Block>,
-		) -> Result<Option<NumberFor<Self>>, Self::Error> {
-			Ok(match at {
-				BlockId::Number(num) => Some(*num),
-				BlockId::Hash(_) => None,
-			})
-		}
-
-		/// Returns a block hash given the block id.
-		fn block_id_to_hash(
-			&self,
-			at: &BlockId<Self::Block>,
-		) -> Result<Option<<Self::Block as BlockT>::Hash>, Self::Error> {
-			Ok(match at {
-				BlockId::Number(num) => Some(H256::from_low_u64_be(*num)).into(),
-				BlockId::Hash(_) => None,
-			})
-		}
-
-		/// Hash the extrinsic.
-		fn hash_and_length(&self, uxt: &ExtrinsicFor<Self>) -> (BlockHash<Self>, usize) {
-			let encoded = uxt.encode();
-			let len = encoded.len();
-			(Hashing::hash(&encoded), len)
-		}
-
-		fn block_body(&self, _id: &BlockId<Self::Block>) -> Self::BodyFuture {
-			futures::future::ready(Ok(None))
-		}
-
-		fn block_header(
-			&self,
-			_: &BlockId<Self::Block>,
-		) -> Result<Option<<Self::Block as BlockT>::Header>, Self::Error> {
-			Ok(None)
-		}
-	}
-
-	fn uxt(transfer: Transfer) -> Extrinsic {
-		let signature = TryFrom::try_from(&[0; 64][..]).unwrap();
-		Extrinsic::Transfer { transfer, signature, exhaust_resources_when_not_first: false }
-	}
-
-	fn pool() -> Pool<TestApi> {
-		Pool::new(Default::default(), true.into(), TestApi::default().into())
-	}
-
 	#[test]
 	fn should_validate_and_import_transaction() {
 		// given
@@ -636,7 +489,7 @@ mod tests {
 		});
 
 		// when
-		pool.validated_pool.rotator().ban(&Instant::now(), vec![pool.hash_of(&uxt)]);
+		pool.validated_pool.ban(&Instant::now(), vec![pool.hash_of(&uxt)]);
 		let res = block_on(pool.submit_one(&BlockId::Number(0), SOURCE, uxt));
 		assert_eq!(pool.validated_pool().status().ready, 0);
 		assert_eq!(pool.validated_pool().status().future, 0);
@@ -767,9 +620,9 @@ mod tests {
 		assert_eq!(pool.validated_pool().status().future, 0);
 		assert_eq!(pool.validated_pool().status().ready, 0);
 		// make sure they are temporarily banned as well
-		assert!(pool.validated_pool.rotator().is_banned(&hash1));
-		assert!(pool.validated_pool.rotator().is_banned(&hash2));
-		assert!(pool.validated_pool.rotator().is_banned(&hash3));
+		assert!(pool.validated_pool.is_banned(&hash1));
+		assert!(pool.validated_pool.is_banned(&hash2));
+		assert!(pool.validated_pool.is_banned(&hash3));
 	}
 
 	#[test]
@@ -792,7 +645,7 @@ mod tests {
 		block_on(pool.prune_tags(&BlockId::Number(1), vec![vec![0]], vec![hash1.clone()])).unwrap();
 
 		// then
-		assert!(pool.validated_pool.rotator().is_banned(&hash1));
+		assert!(pool.validated_pool.is_banned(&hash1));
 	}
 
 	#[test]
@@ -832,8 +685,8 @@ mod tests {
 
 		// then
 		assert_eq!(pool.validated_pool().status().future, 1);
-		assert!(pool.validated_pool.rotator().is_banned(&hash1));
-		assert!(!pool.validated_pool.rotator().is_banned(&hash2));
+		assert!(pool.validated_pool.is_banned(&hash1));
+		assert!(!pool.validated_pool.is_banned(&hash2));
 	}
 
 	#[test]

substrate/client/transaction-pool/src/graph/validated_pool.rs

@@ -569,12 +569,6 @@ impl<B: ChainApi> ValidatedPool<B> {
 		Ok(())
 	}
 
-	/// Get rotator reference.
-	#[cfg(feature = "test-helpers")]
-	pub fn rotator(&self) -> &PoolRotator<ExtrinsicHash<B>> {
-		&self.rotator
-	}
-
 	/// Get api reference.
 	pub fn api(&self) -> &B {
 		&self.api

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

@@ -23,20 +23,12 @@
 #![warn(unused_extern_crates)]
 
 mod api;
+pub mod error;
 mod graph;
 mod metrics;
 mod revalidation;
-
-pub mod error;
-
-/// Common types for testing the transaction pool
-#[cfg(feature = "test-helpers")]
-pub mod test_helpers {
-	pub use super::{
-		graph::{BlockHash, ChainApi, ExtrinsicFor, NumberFor, Pool},
-		revalidation::RevalidationQueue,
-	};
-}
+#[cfg(test)]
+mod tests;
 
 pub use crate::api::FullChainApi;
 use futures::{
@@ -170,13 +162,10 @@ where
 	PoolApi: graph::ChainApi<Block = Block> + 'static,
 {
 	/// Create new basic transaction pool with provided api, for tests.
-	#[cfg(feature = "test-helpers")]
-	pub fn new_test(
-		pool_api: Arc<PoolApi>,
-	) -> (Self, Pin<Box<dyn Future<Output = ()> + Send>>, intervalier::BackSignalControl) {
+	pub fn new_test(pool_api: Arc<PoolApi>) -> (Self, Pin<Box<dyn Future<Output = ()> + Send>>) {
 		let pool = Arc::new(graph::Pool::new(Default::default(), true.into(), pool_api.clone()));
-		let (revalidation_queue, background_task, notifier) =
-			revalidation::RevalidationQueue::new_test(pool_api.clone(), pool.clone());
+		let (revalidation_queue, background_task) =
+			revalidation::RevalidationQueue::new_background(pool_api.clone(), pool.clone());
 		(
 			Self {
 				api: pool_api,
@@ -187,7 +176,6 @@ where
 				metrics: Default::default(),
 			},
 			background_task,
-			notifier,
 		)
 	}
 
@@ -237,7 +225,6 @@ where
 	}
 
 	/// Get access to the underlying api
-	#[cfg(feature = "test-helpers")]
 	pub fn api(&self) -> &PoolApi {
 		&self.api
 	}

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

@@ -35,10 +35,7 @@ use sp_runtime::{
 use futures::prelude::*;
 use std::time::Duration;
 
-#[cfg(not(feature = "test-helpers"))]
 const BACKGROUND_REVALIDATION_INTERVAL: Duration = Duration::from_millis(200);
-#[cfg(feature = "test-helpers")]
-pub const BACKGROUND_REVALIDATION_INTERVAL: Duration = Duration::from_millis(1);
 
 const MIN_BACKGROUND_REVALIDATION_BATCH_SIZE: usize = 20;
 
@@ -213,36 +210,25 @@ impl<Api: ChainApi> RevalidationWorker<Api> {
 	/// It does two things: periodically tries to process some transactions
 	/// from the queue and also accepts messages to enqueue some more
 	/// transactions from the pool.
-	pub async fn run<R: intervalier::IntoStream>(
+	pub async fn run(
 		mut self,
 		from_queue: TracingUnboundedReceiver<WorkerPayload<Api>>,
-		interval: R,
-	) where
-		R: Send,
-		R::Guard: Send,
-	{
-		let interval = interval.into_stream().fuse();
+		interval: Duration,
+	) {
+		let interval_fut = futures_timer::Delay::new(interval);
 		let from_queue = from_queue.fuse();
-		futures::pin_mut!(interval, from_queue);
+		futures::pin_mut!(interval_fut, from_queue);
 		let this = &mut self;
 
 		loop {
 			futures::select! {
-				_guard = interval.next() => {
+				// Using `fuse()` in here is okay, because we reset the interval when it has fired.
+				_ = (&mut interval_fut).fuse() => {
 					let next_batch = this.prepare_batch();
 					let batch_len = next_batch.len();
 
 					batch_revalidate(this.pool.clone(), this.api.clone(), this.best_block, next_batch).await;
 
-					#[cfg(feature = "test-helpers")]
-					{
-						use intervalier::Guard;
-						// only trigger test events if something was processed
-						if batch_len == 0 {
-							_guard.expect("Always some() in tests").skip();
-						}
-					}
-
 					if batch_len > 0 || this.len() > 0 {
 						log::debug!(
 							target: "txpool",
@@ -251,6 +237,8 @@ impl<Api: ChainApi> RevalidationWorker<Api> {
 							this.len(),
 						);
 					}
+
+					interval_fut.reset(interval);
 				},
 				workload = from_queue.next() => {
 					match workload {
@@ -298,15 +286,11 @@ where
 	}
 
 	/// New revalidation queue with background worker.
-	pub fn new_with_interval<R: intervalier::IntoStream>(
+	pub fn new_with_interval(
 		api: Arc<Api>,
 		pool: Arc<Pool<Api>>,
-		interval: R,
-	) -> (Self, Pin<Box<dyn Future<Output = ()> + Send>>)
-	where
-		R: Send + 'static,
-		R::Guard: Send,
-	{
+		interval: Duration,
+	) -> (Self, Pin<Box<dyn Future<Output = ()> + Send>>) {
 		let (to_worker, from_queue) = tracing_unbounded("mpsc_revalidation_queue");
 
 		let worker = RevalidationWorker::new(api.clone(), pool.clone());
@@ -321,24 +305,7 @@ where
 		api: Arc<Api>,
 		pool: Arc<Pool<Api>>,
 	) -> (Self, Pin<Box<dyn Future<Output = ()> + Send>>) {
-		Self::new_with_interval(
-			api,
-			pool,
-			intervalier::Interval::new(BACKGROUND_REVALIDATION_INTERVAL),
-		)
-	}
-
-	/// New revalidation queue with background worker and test signal.
-	#[cfg(feature = "test-helpers")]
-	pub fn new_test(
-		api: Arc<Api>,
-		pool: Arc<Pool<Api>>,
-	) -> (Self, Pin<Box<dyn Future<Output = ()> + Send>>, intervalier::BackSignalControl) {
-		let (interval, notifier) =
-			intervalier::BackSignalInterval::new(BACKGROUND_REVALIDATION_INTERVAL);
-		let (queue, background) = Self::new_with_interval(api, pool, interval);
-
-		(queue, background, notifier)
+		Self::new_with_interval(api, pool, BACKGROUND_REVALIDATION_INTERVAL)
 	}
 
 	/// Queue some transaction for later revalidation.
@@ -371,4 +338,41 @@ where
 }
 
 #[cfg(test)]
-mod tests {}
+mod tests {
+	use super::*;
+	use crate::{
+		graph::Pool,
+		tests::{uxt, TestApi},
+	};
+	use futures::executor::block_on;
+	use sc_transaction_pool_api::TransactionSource;
+	use sp_runtime::generic::BlockId;
+	use substrate_test_runtime::{AccountId, Transfer, H256};
+
+	#[test]
+	fn revalidation_queue_works() {
+		let api = Arc::new(TestApi::default());
+		let pool = Arc::new(Pool::new(Default::default(), true.into(), api.clone()));
+		let queue = Arc::new(RevalidationQueue::new(api.clone(), pool.clone()));
+
+		let uxt = uxt(Transfer {
+			from: AccountId::from_h256(H256::from_low_u64_be(1)),
+			to: AccountId::from_h256(H256::from_low_u64_be(2)),
+			amount: 5,
+			nonce: 0,
+		});
+		let uxt_hash = block_on(pool.submit_one(
+			&BlockId::number(0),
+			TransactionSource::External,
+			uxt.clone(),
+		))
+		.expect("Should be valid");
+
+		block_on(queue.revalidate_later(0, vec![uxt_hash]));
+
+		// revalidated in sync offload 2nd time
+		assert_eq!(api.validation_requests().len(), 2);
+		// number of ready
+		assert_eq!(pool.validated_pool().status().ready, 1);
+	}
+}

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

@@ -0,0 +1,185 @@
+// This file is part of Substrate.
+
+// Copyright (C) 2018-2021 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0
+
+// This program 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.
+
+// This program 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 this program. If not, see <https://www.gnu.org/licenses/>.
+
+//! Testing related primitives for internal usage in this crate.
+
+use crate::graph::{BlockHash, ChainApi, ExtrinsicFor, NumberFor, Pool};
+use codec::Encode;
+use parking_lot::Mutex;
+use sc_transaction_pool_api::error;
+use sp_runtime::{
+	generic::BlockId,
+	traits::{Block as BlockT, Hash},
+	transaction_validity::{
+		InvalidTransaction, TransactionSource, TransactionValidity, ValidTransaction,
+	},
+};
+use std::{collections::HashSet, sync::Arc};
+use substrate_test_runtime::{Block, Extrinsic, Hashing, Transfer, H256};
+
+pub(crate) const INVALID_NONCE: u64 = 254;
+
+/// Test api that implements [`ChainApi`].
+#[derive(Clone, Debug, Default)]
+pub(crate) struct TestApi {
+	pub delay: Arc<Mutex<Option<std::sync::mpsc::Receiver<()>>>>,
+	pub invalidate: Arc<Mutex<HashSet<H256>>>,
+	pub clear_requirements: Arc<Mutex<HashSet<H256>>>,
+	pub add_requirements: Arc<Mutex<HashSet<H256>>>,
+	pub validation_requests: Arc<Mutex<Vec<Extrinsic>>>,
+}
+
+impl TestApi {
+	/// Query validation requests received.
+	pub fn validation_requests(&self) -> Vec<Extrinsic> {
+		self.validation_requests.lock().clone()
+	}
+}
+
+impl ChainApi for TestApi {
+	type Block = Block;
+	type Error = error::Error;
+	type ValidationFuture = futures::future::Ready<error::Result<TransactionValidity>>;
+	type BodyFuture = futures::future::Ready<error::Result<Option<Vec<Extrinsic>>>>;
+
+	/// Verify extrinsic at given block.
+	fn validate_transaction(
+		&self,
+		at: &BlockId<Self::Block>,
+		_source: TransactionSource,
+		uxt: ExtrinsicFor<Self>,
+	) -> Self::ValidationFuture {
+		self.validation_requests.lock().push(uxt.clone());
+		let hash = self.hash_and_length(&uxt).0;
+		let block_number = self.block_id_to_number(at).unwrap().unwrap();
+
+		let res = match uxt {
+			Extrinsic::Transfer { transfer, .. } => {
+				let nonce = transfer.nonce;
+
+				// This is used to control the test flow.
+				if nonce > 0 {
+					let opt = self.delay.lock().take();
+					if let Some(delay) = opt {
+						if delay.recv().is_err() {
+							println!("Error waiting for delay!");
+						}
+					}
+				}
+
+				if self.invalidate.lock().contains(&hash) {
+					InvalidTransaction::Custom(0).into()
+				} else if nonce < block_number {
+					InvalidTransaction::Stale.into()
+				} else {
+					let mut transaction = ValidTransaction {
+						priority: 4,
+						requires: if nonce > block_number {
+							vec![vec![nonce as u8 - 1]]
+						} else {
+							vec![]
+						},
+						provides: if nonce == INVALID_NONCE {
+							vec![]
+						} else {
+							vec![vec![nonce as u8]]
+						},
+						longevity: 3,
+						propagate: true,
+					};
+
+					if self.clear_requirements.lock().contains(&hash) {
+						transaction.requires.clear();
+					}
+
+					if self.add_requirements.lock().contains(&hash) {
+						transaction.requires.push(vec![128]);
+					}
+
+					Ok(transaction)
+				}
+			},
+			Extrinsic::IncludeData(_) => Ok(ValidTransaction {
+				priority: 9001,
+				requires: vec![],
+				provides: vec![vec![42]],
+				longevity: 9001,
+				propagate: false,
+			}),
+			Extrinsic::Store(_) => Ok(ValidTransaction {
+				priority: 9001,
+				requires: vec![],
+				provides: vec![vec![43]],
+				longevity: 9001,
+				propagate: false,
+			}),
+			_ => unimplemented!(),
+		};
+
+		futures::future::ready(Ok(res))
+	}
+
+	/// Returns a block number given the block id.
+	fn block_id_to_number(
+		&self,
+		at: &BlockId<Self::Block>,
+	) -> Result<Option<NumberFor<Self>>, Self::Error> {
+		Ok(match at {
+			BlockId::Number(num) => Some(*num),
+			BlockId::Hash(_) => None,
+		})
+	}
+
+	/// Returns a block hash given the block id.
+	fn block_id_to_hash(
+		&self,
+		at: &BlockId<Self::Block>,
+	) -> Result<Option<<Self::Block as BlockT>::Hash>, Self::Error> {
+		Ok(match at {
+			BlockId::Number(num) => Some(H256::from_low_u64_be(*num)).into(),
+			BlockId::Hash(_) => None,
+		})
+	}
+
+	/// Hash the extrinsic.
+	fn hash_and_length(&self, uxt: &ExtrinsicFor<Self>) -> (BlockHash<Self>, usize) {
+		let encoded = uxt.encode();
+		let len = encoded.len();
+		(Hashing::hash(&encoded), len)
+	}
+
+	fn block_body(&self, _id: &BlockId<Self::Block>) -> Self::BodyFuture {
+		futures::future::ready(Ok(None))
+	}
+
+	fn block_header(
+		&self,
+		_: &BlockId<Self::Block>,
+	) -> Result<Option<<Self::Block as BlockT>::Header>, Self::Error> {
+		Ok(None)
+	}
+}
+
+pub(crate) fn uxt(transfer: Transfer) -> Extrinsic {
+	let signature = TryFrom::try_from(&[0; 64][..]).unwrap();
+	Extrinsic::Transfer { transfer, signature, exhaust_resources_when_not_first: false }
+}
+
+pub(crate) fn pool() -> Pool<TestApi> {
+	Pool::new(Default::default(), true.into(), TestApi::default().into())
+}