From af1088ef61323f171915555023d8e993aaaed755 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 8 Jun 2018 17:05:46 +0200 Subject: [PATCH] Disable parallel verification and skip verifiying already imported txs. (#8834) * Reject transactions that are already in pool without verifying them. * Avoid verifying already imported transactions. --- Cargo.lock | 1 - miner/Cargo.toml | 1 - miner/src/lib.rs | 1 - miner/src/pool/queue.rs | 11 ++++++++--- miner/src/pool/tests/client.rs | 9 +++++++++ miner/src/pool/tests/mod.rs | 34 ++++++++++++++++++++++++++++++++++ miner/src/pool/verifier.rs | 4 +++- 7 files changed, 54 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a22012300..9334d0102 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -691,7 +691,6 @@ dependencies = [ "parity-reactor 0.1.0", "parking_lot 0.5.4 (registry+https://github.com/rust-lang/crates.io-index)", "price-info 1.12.0", - "rayon 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", "rlp 0.2.1", "rustc-hex 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "trace-time 0.1.0", diff --git a/miner/Cargo.toml b/miner/Cargo.toml index 707352484..e692d2f70 100644 --- a/miner/Cargo.toml +++ b/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" } diff --git a/miner/src/lib.rs b/miner/src/lib.rs index 107b9b22b..9dc180fef 100644 --- a/miner/src/lib.rs +++ b/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; diff --git a/miner/src/pool/queue.rs b/miner/src/pool/queue.rs index 4ebdf9e3f..cf4a956f4 100644 --- a/miner/src/pool/queue.rs +++ b/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| ()) diff --git a/miner/src/pool/tests/client.rs b/miner/src/pool/tests/client.rs index 101b6cdc2..08b43f12a 100644 --- a/miner/src/pool/tests/client.rs +++ b/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 . +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, 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 { + self.verification_invoked.store(true, atomic::Ordering::SeqCst); Ok(SignedTransaction::new(tx)?) } diff --git a/miner/src/pool/tests/mod.rs b/miner/src/pool/tests/mod.rs index 552903a4b..ac2e6b008 100644 --- a/miner/src/pool/tests/mod.rs +++ b/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); +} diff --git a/miner/src/pool/verifier.rs b/miner/src/pool/verifier.rs index 467530392..5e6e22077 100644 --- a/miner/src/pool/verifier.rs +++ b/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(),