Disable parallel verification and skip verifiying already imported tx…

…s. (#8834)

* Reject transactions that are already in pool without verifying them.

* Avoid verifying already imported transactions.

miner/Cargo.toml

@@ -27,7 +27,6 @@ linked-hash-map = "0.5"
 log = "0.3"
 parking_lot = "0.5"
 price-info = { path = "../price-info" }
-rayon = "1.0"
 rlp = { path = "../util/rlp" }
 trace-time = { path = "../util/trace-time" }
 transaction-pool = { path = "../transaction-pool" }

miner/src/lib.rs

@@ -29,7 +29,6 @@ extern crate keccak_hash as hash;
 extern crate linked_hash_map;
 extern crate parking_lot;
 extern crate price_info;
-extern crate rayon;
 extern crate rlp;
 extern crate trace_time;
 extern crate transaction_pool as txpool;

miner/src/pool/queue.rs

@@ -23,7 +23,6 @@ use std::collections::BTreeMap;
 
 use ethereum_types::{H256, U256, Address};
 use parking_lot::RwLock;
-use rayon::prelude::*;
 use transaction;
 use txpool::{self, Verifier};
 
@@ -179,8 +178,14 @@ impl TransactionQueue {
 
 		let verifier = verifier::Verifier::new(client, options, self.insertion_id.clone());
 		let results = transactions
-			.into_par_iter()
-			.map(|transaction| verifier.verify_transaction(transaction))
+			.into_iter()
+			.map(|transaction| {
+				if self.pool.read().find(&transaction.hash()).is_some() {
+					bail!(transaction::Error::AlreadyImported)
+				}
+
+				verifier.verify_transaction(transaction)
+			})
 			.map(|result| result.and_then(|verified| {
 				self.pool.write().import(verified)
 					.map(|_imported| ())

miner/src/pool/tests/client.rs

@@ -14,6 +14,8 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
+use std::sync::{atomic, Arc};
+
 use ethereum_types::{U256, H256, Address};
 use rlp::Rlp;
 use transaction::{self, Transaction, SignedTransaction, UnverifiedTransaction};
@@ -25,6 +27,7 @@ const MAX_TRANSACTION_SIZE: usize = 15 * 1024;
 
 #[derive(Debug, Clone)]
 pub struct TestClient {
+	verification_invoked: Arc<atomic::AtomicBool>,
 	account_details: AccountDetails,
 	gas_required: U256,
 	is_service_transaction: bool,
@@ -35,6 +38,7 @@ pub struct TestClient {
 impl Default for TestClient {
 	fn default() -> Self {
 		TestClient {
+			verification_invoked: Default::default(),
 			account_details: AccountDetails {
 				nonce: 123.into(),
 				balance: 63_100.into(),
@@ -88,6 +92,10 @@ impl TestClient {
 			insertion_id: 1,
 		}
 	}
+
+	pub fn was_verification_triggered(&self) -> bool {
+		self.verification_invoked.load(atomic::Ordering::SeqCst)
+	}
 }
 
 impl pool::client::Client for TestClient {
@@ -98,6 +106,7 @@ impl pool::client::Client for TestClient {
 	fn verify_transaction(&self, tx: UnverifiedTransaction)
 		-> Result<SignedTransaction, transaction::Error>
 	{
+		self.verification_invoked.store(true, atomic::Ordering::SeqCst);
 		Ok(SignedTransaction::new(tx)?)
 	}
 

miner/src/pool/tests/mod.rs

@@ -796,3 +796,37 @@ fn should_include_local_transaction_to_a_full_pool() {
 	// then
 	assert_eq!(txq.status().status.transaction_count, 1);
 }
+
+#[test]
+fn should_avoid_verifying_transaction_already_in_pool() {
+	// given
+	let txq = TransactionQueue::new(
+		txpool::Options {
+			max_count: 1,
+			max_per_sender: 2,
+			max_mem_usage: 50
+		},
+		verifier::Options {
+			minimal_gas_price: 1.into(),
+			block_gas_limit: 1_000_000.into(),
+			tx_gas_limit: 1_000_000.into(),
+		},
+		PrioritizationStrategy::GasPriceOnly,
+	);
+	let client = TestClient::new();
+	let tx1 = Tx::default().signed().unverified();
+
+	let res = txq.import(client.clone(), vec![tx1.clone()]);
+	assert_eq!(res, vec![Ok(())]);
+	assert_eq!(txq.status().status.transaction_count, 1);
+	assert!(client.was_verification_triggered());
+
+	// when
+	let client = TestClient::new();
+	let res = txq.import(client.clone(), vec![tx1]);
+	assert_eq!(res, vec![Err(transaction::Error::AlreadyImported)]);
+	assert!(!client.was_verification_triggered());
+
+	// then
+	assert_eq!(txq.status().status.transaction_count, 1);
+}

miner/src/pool/verifier.rs

@@ -57,6 +57,7 @@ impl Default for Options {
 }
 
 /// Transaction to verify.
+#[cfg_attr(test, derive(Clone))]
 pub enum Transaction {
 	/// Fresh, never verified transaction.
 	///
@@ -75,7 +76,8 @@ pub enum Transaction {
 }
 
 impl Transaction {
-	fn hash(&self) -> H256 {
+	/// Return transaction hash
+	pub fn hash(&self) -> H256 {
 		match *self {
 			Transaction::Unverified(ref tx) => tx.hash(),
 			Transaction::Retracted(ref tx) => tx.hash(),