[ethcore]: apply filter when PendingSet::AlwaysQueue in ready_transactions_filtered (#11227)

* [ethcore]: apply filter when `from_queue`

In `ready_transactions_filtered` the filter were never applied when
the options PendingSet::AlwaysQueue` was configured which this fixes

It also adds a two tests for it

* [ethcore test-helpers]: stray printlns

* docs(ethcore filter options): more generic desc

* tests(ethcore miner): simply filter tests

* [ethcore filter_options]: fix nits

* doc: nit

Co-Authored-By: David <dvdplm@gmail.com>

* doc: nit

Co-Authored-By: David <dvdplm@gmail.com>

* doc: nit

Co-Authored-By: David <dvdplm@gmail.com>

* doc: nit

Co-Authored-By: David <dvdplm@gmail.com>

* doc: nit

Co-Authored-By: David <dvdplm@gmail.com>

* doc(miner filter): simplify documentation

* [rpc]: make tests compile

ethcore/src/miner/filter_options.rs

@@ -14,28 +14,70 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity Ethereum.  If not, see <http://www.gnu.org/licenses/>.
 
-use ethereum_types::{Address, U256};
-use serde::de::{Deserialize, Deserializer, Error, MapAccess, Visitor};
 use std::fmt;
 use std::marker::PhantomData;
 
-/// This structure provides filtering options for the pending transactions RPC API call
+use ethereum_types::{Address, U256};
+use serde::de::{Deserialize, Deserializer, Error, MapAccess, Visitor};
+use types::transaction::SignedTransaction;
+
+/// Filtering options for the pending transactions
+/// May be used for filtering transactions based on gas, gas price, value and/or nonce.
+// NOTE: the fields are only `pub` because they are needed for tests
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct FilterOptions {
-    /// Contains the operator to filter the from value of the transaction
+	/// Filter based on the `sender` of the transaction.
 	pub from: FilterOperator<Address>,
-    /// Contains the operator to filter the to value of the transaction
+	/// Filter based on `receiver` of the transaction.
 	pub to: FilterOperator<Option<Address>>,
-    /// Contains the operator to filter the gas value of the transaction
+	/// Filter based on `gas` of the transaction.
 	pub gas: FilterOperator<U256>,
-    /// Contains the operator to filter the gas price value of the transaction
+	/// Filter based on `gas price` of the transaction.
 	pub gas_price: FilterOperator<U256>,
-    /// Contains the operator to filter the transaction value
+	/// Filter based on `value` of the transaction.
 	pub value: FilterOperator<U256>,
-    /// Contains the operator to filter the nonce value of the transaction
+	/// Filter based on `nonce` of the transaction.
 	pub nonce: FilterOperator<U256>,
 }
 
+impl FilterOptions {
+	fn sender_matcher(filter: &FilterOperator<Address>, candidate: &Address) -> bool {
+		match filter {
+			FilterOperator::Eq(address) => candidate == address,
+			FilterOperator::Any => true,
+			// Handled during deserialization
+			_ => unreachable!(),
+		}
+	}
+
+	fn receiver_matcher(filter: &FilterOperator<Option<Address>>, candidate: &Option<Address>) -> bool {
+		match filter {
+			FilterOperator::Eq(address) => candidate == address,
+			FilterOperator::Any => true,
+			// Handled during deserialization
+			_ => unreachable!(),
+		}
+	}
+	fn value_matcher(filter: &FilterOperator<U256>, tx_value: &U256) -> bool {
+		match filter {
+			FilterOperator::Eq(ref value) => tx_value == value,
+			FilterOperator::GreaterThan(ref value) => tx_value > value,
+			FilterOperator::LessThan(ref value) => tx_value < value,
+			FilterOperator::Any => true,
+		}
+	}
+
+	/// Determines whether a transaction passes the configured filter
+	pub fn matches(&self, tx: &SignedTransaction) -> bool {
+		Self::sender_matcher(&self.from, &tx.sender()) &&
+		Self::receiver_matcher(&self.to, &tx.receiver()) &&
+		Self::value_matcher(&self.gas, &tx.gas) &&
+		Self::value_matcher(&self.gas_price, &tx.gas_price) &&
+		Self::value_matcher(&self.value, &tx.value) &&
+		Self::value_matcher(&self.nonce, &tx.nonce)
+	}
+}
+
 impl Default for FilterOptions {
 	fn default() -> Self {
 		FilterOptions {
@@ -54,8 +96,10 @@ impl Default for FilterOptions {
 /// gets returned explicitly. Therefore this Wrapper will be used for
 /// deserialization, directly identifying the contract creation.
 enum Wrapper<T> {
+	/// FilterOperations
 	O(FilterOperator<T>),
-    CC, // Contract Creation
+	/// Contract Creation
+	CC,
 }
 
 /// Available operators for filtering options.
@@ -63,9 +107,13 @@ enum Wrapper<T> {
 /// The `to` filter only accepts Any, Eq(Address) and Eq(None) for contract creation.
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub enum FilterOperator<T> {
+	/// Any (no filter)
 	Any,
+	/// Equal
 	Eq(T),
+	/// Greather than
 	GreaterThan(T),
+	/// Less than
 	LessThan(T),
 }
 
@@ -84,7 +132,8 @@ trait Validate<'de, T, M: MapAccess<'de>> {
 }
 
 impl<'de, T, M> Validate<'de, T, M> for M
-    where T: Deserialize<'de>, M: MapAccess<'de>
+where
+	T: Deserialize<'de>, M: MapAccess<'de>
 {
 	fn validate_from(&mut self) -> Result<FilterOperator<T>, M::Error> {
 		use self::Wrapper as W;
@@ -189,7 +238,6 @@ impl<'de> Deserialize<'de> for FilterOptions {
 						}
 					}
 				}
-
 				Ok(filter)
 			}
 		}

ethcore/src/miner/miner.rs

@@ -31,7 +31,7 @@ use ethcore_miner::work_notify::NotifyWork;
 use ethereum_types::{H256, U256, Address};
 use futures::sync::mpsc;
 use io::IoChannel;
-use miner::filter_options::{FilterOptions, FilterOperator};
+use miner::filter_options::FilterOptions;
 use miner::pool_client::{PoolClient, CachedNonceClient, NonceCache};
 use miner::{self, MinerService};
 use parking_lot::{Mutex, RwLock};
@@ -1095,7 +1095,8 @@ impl miner::MinerService for Miner {
 		max_len: usize,
 		filter: Option<FilterOptions>,
 		ordering: miner::PendingOrdering,
-	) -> Vec<Arc<VerifiedTransaction>> where
+	) -> Vec<Arc<VerifiedTransaction>>
+	where
 		C: ChainInfo + Nonce + Sync,
 	{
 		let chain_info = chain.chain_info();
@@ -1106,7 +1107,7 @@ impl miner::MinerService for Miner {
 			// those transactions are valid and will just be ready to be included in next block.
 			let nonce_cap = None;
 
-			self.transaction_queue.pending(
+			let mut pending = self.transaction_queue.pending(
 				CachedNonceClient::new(chain, &self.nonce_cache),
 				pool::PendingSettings {
 					block_number: chain_info.best_block_number,
@@ -1115,77 +1116,26 @@ impl miner::MinerService for Miner {
 					max_len,
 					ordering,
 				},
-			)
+			);
+
+			pending.retain(|tx| {
+				filter.as_ref().map_or(true, |filter| {
+					filter.matches(tx.signed())
+				})
+			});
+
+			pending
 		};
 
-		use miner::filter_options::FilterOperator::*;
 		let from_pending = || {
 			self.map_existing_pending_block(|sealing| {
-				// This filter is used for gas, gas price, value and nonce.
-				// Sender and receiver have their custom matches, since those
-				// allow/disallow different operators.
-				fn match_common_filter(operator: &FilterOperator<U256>, tx_value: &U256) -> bool {
-					match operator {
-						Eq(value) => tx_value == value,
-						GreaterThan(value) => tx_value > value,
-						LessThan(value) => tx_value < value,
-						// Will always occur on `Any`, other operators
-						// get handled during deserialization
-						_ => true,
-					}
-				}
-
 				sealing.transactions
 					.iter()
 					.map(|signed| pool::VerifiedTransaction::from_pending_block_transaction(signed.clone()))
 					.map(Arc::new)
-					// Filter by sender
 					.filter(|tx| {
 						filter.as_ref().map_or(true, |filter| {
-							let sender = tx.signed().sender();
+							filter.matches(tx.signed())
-							match filter.from {
-								Eq(value) => sender == value,
-								// Will always occur on `Any`, other operators
-								// get handled during deserialization
-								_ => true,
-							}
-						})
-					})
-					// Filter by receiver
-					.filter(|tx| {
-						filter.as_ref().map_or(true, |filter| {
-							let receiver = (*tx.signed()).receiver();
-							match filter.to {
-								// Could apply to `Some(Address)` or `None` (for contract creation)
-								Eq(value) => receiver == value,
-								// Will always occur on `Any`, other operators
-								// get handled during deserialization
-								_ => true,
-							}
-						})
-					})
-					// Filter by gas
-					.filter(|tx| {
-						filter.as_ref().map_or(true, |filter| {
-							match_common_filter(&filter.gas, &(*tx.signed()).gas)
-						})
-					})
-					// Filter by gas price
-					.filter(|tx| {
-						filter.as_ref().map_or(true, |filter| {
-							match_common_filter(&filter.gas_price, &(*tx.signed()).gas_price)
-						})
-					})
-					// Filter by tx value
-					.filter(|tx| {
-						filter.as_ref().map_or(true, |filter| {
-							match_common_filter(&filter.value, &(*tx.signed()).value)
-						})
-					})
-					// Filter by nonce
-					.filter(|tx| {
-						filter.as_ref().map_or(true, |filter| {
-							match_common_filter(&filter.nonce, &(*tx.signed()).nonce)
 						})
 					})
 					.take(max_len)
@@ -1520,7 +1470,7 @@ mod tests {
 
 	use client_traits::ChainInfo;
 	use client::ImportSealedBlock;
-	use miner::{MinerService, PendingOrdering};
+	use miner::{MinerService, PendingOrdering, filter_options::FilterOperator};
 	use test_helpers::{
 		generate_dummy_client, generate_dummy_client_with_spec, TestBlockChainClient, EachBlockWith
 	};
@@ -1937,4 +1887,93 @@ mod tests {
 
 		assert!(received_error_msg == expected_error_msg);
 	}
+
+	fn filter_tester(option: PendingSet) {
+		let client = TestBlockChainClient::default();
+		let mut miner = miner();
+		miner.options.pending_set = option;
+		let transaction = transaction();
+		let best_block = 10;
+		let mut sender = transaction.sender();
+
+		miner.import_own_transaction(&client, PendingTransaction::new(transaction, None)).unwrap();
+
+		assert_eq!(miner.pending_transactions(best_block), None);
+		assert_eq!(miner.pending_receipts(best_block), None);
+		assert_eq!(
+			miner.ready_transactions_filtered(&client, 10, Some(FilterOptions::default()), PendingOrdering::Priority).len(),
+			1
+		);
+
+		// sender filter
+		{
+			// reverse address for false match
+			for byte in sender.as_bytes_mut() {
+				*byte = !*byte;
+			}
+			let mut filter = FilterOptions::default();
+			filter.from = FilterOperator::Eq(sender);
+			assert_eq!(
+				miner.ready_transactions_filtered(&client, 10, Some(filter), PendingOrdering::Priority).len(),
+				0
+			);
+		}
+
+		// receiver filter
+		{
+			let mut filter = FilterOptions::default();
+			filter.to = FilterOperator::Eq(Some(sender));
+			assert_eq!(
+				miner.ready_transactions_filtered(&client, 10, Some(filter), PendingOrdering::Priority).len(),
+				0
+			);
+		}
+
+		// gas filter
+		{
+			let mut filter = FilterOptions::default();
+			filter.gas = FilterOperator::LessThan(U256::from(100_000));
+			assert_eq!(
+				miner.ready_transactions_filtered(&client, 10, Some(filter), PendingOrdering::Priority).len(),
+				0
+			);
+		}
+
+		// gas_price filter
+		{
+			let mut filter = FilterOptions::default();
+			filter.gas_price = FilterOperator::GreaterThan(U256::from(10));
+			assert_eq!(
+				miner.ready_transactions_filtered(&client, 10, Some(filter), PendingOrdering::Priority).len(),
+				0
+			);
+		}
+
+		// value filter
+		{
+			let mut filter = FilterOptions::default();
+			filter.value = FilterOperator::Eq(U256::from(19));
+			assert_eq!(
+				miner.ready_transactions_filtered(&client, 10, Some(filter), PendingOrdering::Priority).len(),
+				0
+			);
+		}
+
+		// nonce filter
+		{
+			let mut filter = FilterOptions::default();
+			filter.nonce = FilterOperator::GreaterThan(U256::from(10));
+			assert_eq!(
+				miner.ready_transactions_filtered(&client, 10, Some(filter), PendingOrdering::Priority).len(),
+				0
+			);
+		}
+	}
+
+	#[test]
+	fn transaction_filtering() {
+		filter_tester(PendingSet::AlwaysQueue);
+		filter_tester(PendingSet::AlwaysSealing);
+		filter_tester(PendingSet::SealingOrElseQueue);
+	}
 }

rpc/src/v1/tests/helpers/miner_service.rs

@@ -229,8 +229,23 @@ impl MinerService for TestMinerService {
 		self.queued_transactions()
 	}
 
-	fn ready_transactions_filtered<C>(&self, _chain: &C, _max_len: usize, _filter: Option<FilterOptions>, _ordering: miner::PendingOrdering) -> Vec<Arc<VerifiedTransaction>> {
+	fn ready_transactions_filtered<C>(
+		&self,
+		_chain: &C,
+		max_len: usize,
+		filter: Option<FilterOptions>,
+		_ordering: miner::PendingOrdering
+	) -> Vec<Arc<VerifiedTransaction>> {
 		self.queued_transactions()
+			.iter()
+			.cloned()
+			.filter(|tx| {
+				filter.as_ref().map_or(true, |filter| {
+					filter.matches(tx.signed())
+				})
+			})
+			.take(max_len)
+			.collect()
 	}
 
 	fn pending_transaction_hashes<C>(&self, _chain: &C) -> BTreeSet<H256> {