From 7f4d825d64eb1f3892f81f5e633185d9bd3079dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 11 Jun 2018 10:20:56 +0200 Subject: [PATCH 1/6] Minimal effective gas price. --- miner/src/pool/queue.rs | 3 +- miner/src/pool/scoring.rs | 8 ++++- miner/src/pool/tests/mod.rs | 7 +++++ miner/src/pool/verifier.rs | 59 +++++++++++++++++++++++++----------- transaction-pool/src/pool.rs | 20 ++++++++++++ 5 files changed, 78 insertions(+), 19 deletions(-) diff --git a/miner/src/pool/queue.rs b/miner/src/pool/queue.rs index 4ebdf9e3f1f..0ba917c6f15 100644 --- a/miner/src/pool/queue.rs +++ b/miner/src/pool/queue.rs @@ -177,7 +177,8 @@ impl TransactionQueue { let _timer = ::trace_time::PerfTimer::new("pool::verify_and_import"); let options = self.options.read().clone(); - let verifier = verifier::Verifier::new(client, options, self.insertion_id.clone()); + let min_effective_gas_price = self.pool.read().minimal_entry_score().map(scoring::bump_gas_price); + let verifier = verifier::Verifier::new(client, options, self.insertion_id.clone(), min_effective_gas_price); let results = transactions .into_par_iter() .map(|transaction| verifier.verify_transaction(transaction)) diff --git a/miner/src/pool/scoring.rs b/miner/src/pool/scoring.rs index e7551ed6a3d..5a898eeaaea 100644 --- a/miner/src/pool/scoring.rs +++ b/miner/src/pool/scoring.rs @@ -37,6 +37,12 @@ use super::{PrioritizationStrategy, VerifiedTransaction}; /// `new_gas_price > old_gas_price + old_gas_price >> SHIFT` const GAS_PRICE_BUMP_SHIFT: usize = 3; // 2 = 25%, 3 = 12.5%, 4 = 6.25% +/// Calculate minimal gas price requirement. +#[inline] +pub fn bump_gas_price(old_gp: U256) -> U256 { + old_gp.saturating_add(old_gp >> GAS_PRICE_BUMP_SHIFT) +} + /// Simple, gas-price based scoring for transactions. /// /// NOTE: Currently penalization does not apply to new transactions that enter the pool. @@ -60,7 +66,7 @@ impl txpool::Scoring for NonceAndGasPrice { let old_gp = old.transaction.gas_price; let new_gp = new.transaction.gas_price; - let min_required_gp = old_gp + (old_gp >> GAS_PRICE_BUMP_SHIFT); + let min_required_gp = bump_gas_price(old_gp); match min_required_gp.cmp(&new_gp) { cmp::Ordering::Greater => txpool::scoring::Choice::RejectNew, diff --git a/miner/src/pool/tests/mod.rs b/miner/src/pool/tests/mod.rs index 552903a4bb2..e28057d39f8 100644 --- a/miner/src/pool/tests/mod.rs +++ b/miner/src/pool/tests/mod.rs @@ -796,3 +796,10 @@ fn should_include_local_transaction_to_a_full_pool() { // then assert_eq!(txq.status().status.transaction_count, 1); } + + +#[test] +fn should_reject_early_in_case_gas_price_is_less_than_min_effective() { + // TODO [ToDr] + assert_eq!(true, false); +} diff --git a/miner/src/pool/verifier.rs b/miner/src/pool/verifier.rs index 4675303928d..0560982c7f8 100644 --- a/miner/src/pool/verifier.rs +++ b/miner/src/pool/verifier.rs @@ -129,15 +129,22 @@ impl Transaction { pub struct Verifier { client: C, options: Options, + min_effective_gas_price: Option, id: Arc, } impl Verifier { /// Creates new transaction verfier with specified options. - pub fn new(client: C, options: Options, id: Arc) -> Self { + pub fn new( + client: C, + options: Options, + id: Arc, + min_effective_gas_price: Option, + ) -> Self { Verifier { client, options, + min_effective_gas_price, id, } } @@ -190,22 +197,40 @@ impl txpool::Verifier for Verifier { } let is_own = tx.is_local(); - // Quick exit for non-service transactions - if tx.gas_price() < &self.options.minimal_gas_price - && !tx.gas_price().is_zero() - && !is_own - { - trace!( - target: "txqueue", - "[{:?}] Rejected tx below minimal gas price threshold: {} < {}", - hash, - tx.gas_price(), - self.options.minimal_gas_price, - ); - bail!(transaction::Error::InsufficientGasPrice { - minimal: self.options.minimal_gas_price, - got: *tx.gas_price(), - }); + // Quick exit for non-service and non-local transactions + // + // We're checking if the transaction is below configured minimal gas price + // or the effective minimal gas price in case the pool is full. + if !tx.gas_price().is_zero() && !is_own { + if tx.gas_price() < &self.options.minimal_gas_price { + trace!( + target: "txqueue", + "[{:?}] Rejected tx below minimal gas price threshold: {} < {}", + hash, + tx.gas_price(), + self.options.minimal_gas_price, + ); + bail!(transaction::Error::InsufficientGasPrice { + minimal: self.options.minimal_gas_price, + got: *tx.gas_price(), + }); + } + + if let Some(ref gp) = self.min_effective_gas_price { + if tx.gas_price() < gp { + trace!( + target: "txqueue", + "[{:?}] Rejected tx below minimal effective gas price threshold: {} < {}", + hash, + tx.gas_price(), + gp, + ); + bail!(transaction::Error::InsufficientGasPrice { + minimal: *gp, + got: *tx.gas_price(), + }); + } + } } // Some more heavy checks below. diff --git a/transaction-pool/src/pool.rs b/transaction-pool/src/pool.rs index dcd52a3e7e6..84e256ed33e 100644 --- a/transaction-pool/src/pool.rs +++ b/transaction-pool/src/pool.rs @@ -392,6 +392,26 @@ impl Pool where self.worst_transactions.iter().next().map(|x| x.transaction.transaction.clone()) } + /// Returns the score of the worst transaction if the pool is almost full. + /// + /// This method can be used to determine what is the minimal required score + /// for the replacement transaction. If `None` is returned it means that + /// there is still plenty of room in the pool. Otherwise we return + /// `Some` with the score of the worst transaction. + pub fn minimal_entry_score(&self) -> Option { + let threshold = |x: usize| x * 9 / 10; + let is_full = { + self.by_hash.len() > threshold(self.options.max_count) + || self.mem_usage > threshold(self.options.max_mem_usage) + }; + + if !is_full { + return None + } + + self.worst_transactions.iter().next().map(|x| x.score.clone()) + } + /// Returns an iterator of pending (ready) transactions. pub fn pending>(&self, ready: R) -> PendingIterator { PendingIterator { From 04bb3895922d99f65da6127e885341aff036cdc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 11 Jun 2018 11:02:43 +0200 Subject: [PATCH 2/6] Fix naming, add test --- miner/src/pool/tests/mod.rs | 36 ++++++++++++++++++++++++++++++++++-- miner/src/pool/verifier.rs | 4 ++-- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/miner/src/pool/tests/mod.rs b/miner/src/pool/tests/mod.rs index e28057d39f8..9b659b03792 100644 --- a/miner/src/pool/tests/mod.rs +++ b/miner/src/pool/tests/mod.rs @@ -800,6 +800,38 @@ fn should_include_local_transaction_to_a_full_pool() { #[test] fn should_reject_early_in_case_gas_price_is_less_than_min_effective() { - // TODO [ToDr] - assert_eq!(true, false); + // 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().with_balance(1_000_000_000); + let tx1 = Tx::gas_price(2).signed().unverified(); + + let res = txq.import(client.clone(), vec![tx1]); + assert_eq!(res, vec![Ok(())]); + assert_eq!(txq.status().status.transaction_count, 1); + assert!(client.was_verification_triggered()); + + // when + let client = TestClient::new(); + let tx1 = Tx::default().signed().unverified(); + let res = txq.import(client.clone(), vec![tx1]); + assert_eq!(res, vec![Err(transaction::Error::InsufficientGasPrice { + minimal: 2.into(), + got: 1.into(), + })]); + 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 0560982c7f8..38af4a484cd 100644 --- a/miner/src/pool/verifier.rs +++ b/miner/src/pool/verifier.rs @@ -169,7 +169,7 @@ impl txpool::Verifier for Verifier { if tx.gas() > &gas_limit { debug!( target: "txqueue", - "[{:?}] Dropping transaction above gas limit: {} > min({}, {})", + "[{:?}] Rejected transaction above gas limit: {} > min({}, {})", hash, tx.gas(), self.options.block_gas_limit, @@ -184,7 +184,7 @@ impl txpool::Verifier for Verifier { let minimal_gas = self.client.required_gas(tx.transaction()); if tx.gas() < &minimal_gas { trace!(target: "txqueue", - "[{:?}] Dropping transaction with insufficient gas: {} < {}", + "[{:?}] Rejected transaction with insufficient gas: {} < {}", hash, tx.gas(), minimal_gas, From b56200916dfb90149ab22901dac5ef951688974b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 21 Jun 2018 12:08:09 +0200 Subject: [PATCH 3/6] Fix minimal entry score and add test. --- transaction-pool/src/pool.rs | 2 +- transaction-pool/src/tests/mod.rs | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/transaction-pool/src/pool.rs b/transaction-pool/src/pool.rs index 59a9a493a91..ab5a97b2945 100644 --- a/transaction-pool/src/pool.rs +++ b/transaction-pool/src/pool.rs @@ -410,7 +410,7 @@ impl Pool where return None } - self.worst_transactions.iter().next().map(|x| x.score.clone()) + self.worst_transactions.iter().next_back().map(|x| x.score.clone()) } /// Returns an iterator of pending (ready) transactions. diff --git a/transaction-pool/src/tests/mod.rs b/transaction-pool/src/tests/mod.rs index 77c25287570..761217245ee 100644 --- a/transaction-pool/src/tests/mod.rs +++ b/transaction-pool/src/tests/mod.rs @@ -48,6 +48,15 @@ pub type SharedTransaction = Arc; type TestPool = Pool; +impl TestPool { + pub fn with_limit(max_count: usize) -> Self { + Self::with_options(Options { + max_count, + ..Default::default() + }) + } +} + #[test] fn should_clear_queue() { // given @@ -510,6 +519,23 @@ fn should_return_worst_transaction() { assert!(txq.worst_transaction().is_some()); } +#[test] +fn should_return_min_entry_score_based_on_the_worst_transaction() { + // given + let b = TransactionBuilder::default(); + let mut txq = TestPool::with_limit(2); + assert!(txq.worst_transaction().is_none()); + + // when + txq.import(b.tx().nonce(0).gas_price(110).new()).unwrap(); + assert_eq!(txq.minimal_entry_score(), None); + + txq.import(b.tx().sender(1).nonce(0).gas_price(100).new()).unwrap(); + + // then + assert_eq!(txq.minimal_entry_score(), Some(100.into())); +} + mod listener { use std::cell::RefCell; use std::rc::Rc; From 733091c180553dfd8e099c20417858ff7b6ec272 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 22 Jun 2018 11:45:13 +0200 Subject: [PATCH 4/6] Fix worst_transaction. --- transaction-pool/src/pool.rs | 2 +- transaction-pool/src/tests/mod.rs | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/transaction-pool/src/pool.rs b/transaction-pool/src/pool.rs index ab5a97b2945..173eedaf721 100644 --- a/transaction-pool/src/pool.rs +++ b/transaction-pool/src/pool.rs @@ -390,7 +390,7 @@ impl Pool where /// Returns worst transaction in the queue (if any). pub fn worst_transaction(&self) -> Option> { - self.worst_transactions.iter().next().map(|x| x.transaction.transaction.clone()) + self.worst_transactions.iter().next_back().map(|x| x.transaction.transaction.clone()) } /// Returns the score of the worst transaction if the pool is almost full. diff --git a/transaction-pool/src/tests/mod.rs b/transaction-pool/src/tests/mod.rs index 761217245ee..beb72e402a4 100644 --- a/transaction-pool/src/tests/mod.rs +++ b/transaction-pool/src/tests/mod.rs @@ -514,9 +514,10 @@ fn should_return_worst_transaction() { // when txq.import(b.tx().nonce(0).gas_price(5).new()).unwrap(); + txq.import(b.tx().sender(1).nonce(0).gas_price(4).new()).unwrap(); // then - assert!(txq.worst_transaction().is_some()); + assert_eq!(txq.worst_transaction().unwrap().gas_price, 4.into()); } #[test] From b3a77c0d60fa798e5aaa44a40e3877a5834f5e84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Wed, 27 Jun 2018 11:22:47 +0200 Subject: [PATCH 5/6] Remove effective gas price threshold. --- transaction-pool/src/pool.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/transaction-pool/src/pool.rs b/transaction-pool/src/pool.rs index 173eedaf721..2c7dd4af19e 100644 --- a/transaction-pool/src/pool.rs +++ b/transaction-pool/src/pool.rs @@ -400,10 +400,9 @@ impl Pool where /// there is still plenty of room in the pool. Otherwise we return /// `Some` with the score of the worst transaction. pub fn minimal_entry_score(&self) -> Option { - let threshold = |x: usize| x * 9 / 10; let is_full = { - self.by_hash.len() > threshold(self.options.max_count) - || self.mem_usage > threshold(self.options.max_mem_usage) + self.by_hash.len() >= self.options.max_count + || self.mem_usage >= self.options.max_mem_usage }; if !is_full { From 38debef5b98e337fc8fcdae62fa50de621505ffb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Wed, 27 Jun 2018 12:29:16 +0200 Subject: [PATCH 6/6] Don't leak gas_price decisions out of Scoring. --- miner/src/pool/queue.rs | 16 +++++++++++-- miner/src/pool/scoring.rs | 23 +++++++++++++++--- miner/src/pool/verifier.rs | 39 ++++++++++++++++--------------- transaction-pool/src/pool.rs | 24 +++++++------------ transaction-pool/src/tests/mod.rs | 8 +++---- 5 files changed, 66 insertions(+), 44 deletions(-) diff --git a/miner/src/pool/queue.rs b/miner/src/pool/queue.rs index 006b5909d89..450075faa6e 100644 --- a/miner/src/pool/queue.rs +++ b/miner/src/pool/queue.rs @@ -187,8 +187,20 @@ impl TransactionQueue { trace_time!("pool::verify_and_import"); let options = self.options.read().clone(); - let min_effective_gas_price = self.pool.read().minimal_entry_score().map(scoring::bump_gas_price); - let verifier = verifier::Verifier::new(client, options, self.insertion_id.clone(), min_effective_gas_price); + let transaction_to_replace = { + let pool = self.pool.read(); + if pool.is_full() { + pool.worst_transaction().map(|worst| (pool.scoring().clone(), worst)) + } else { + None + } + }; + let verifier = verifier::Verifier::new( + client, + options, + self.insertion_id.clone(), + transaction_to_replace, + ); let results = transactions .into_iter() .map(|transaction| { diff --git a/miner/src/pool/scoring.rs b/miner/src/pool/scoring.rs index 5a898eeaaea..259bbec8789 100644 --- a/miner/src/pool/scoring.rs +++ b/miner/src/pool/scoring.rs @@ -31,7 +31,7 @@ use std::cmp; use ethereum_types::U256; use txpool; -use super::{PrioritizationStrategy, VerifiedTransaction}; +use super::{verifier, PrioritizationStrategy, VerifiedTransaction}; /// Transaction with the same (sender, nonce) can be replaced only if /// `new_gas_price > old_gas_price + old_gas_price >> SHIFT` @@ -39,7 +39,7 @@ const GAS_PRICE_BUMP_SHIFT: usize = 3; // 2 = 25%, 3 = 12.5%, 4 = 6.25% /// Calculate minimal gas price requirement. #[inline] -pub fn bump_gas_price(old_gp: U256) -> U256 { +fn bump_gas_price(old_gp: U256) -> U256 { old_gp.saturating_add(old_gp >> GAS_PRICE_BUMP_SHIFT) } @@ -47,9 +47,26 @@ pub fn bump_gas_price(old_gp: U256) -> U256 { /// /// NOTE: Currently penalization does not apply to new transactions that enter the pool. /// We might want to store penalization status in some persistent state. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct NonceAndGasPrice(pub PrioritizationStrategy); +impl NonceAndGasPrice { + /// Decide if the transaction should even be considered into the pool (if the pool is full). + /// + /// Used by Verifier to quickly reject transactions that don't have any chance to get into the pool later on, + /// and save time on more expensive checks like sender recovery, etc. + /// + /// NOTE The method is never called for zero-gas-price transactions or local transactions + /// (such transactions are always considered to the pool and potentially rejected later on) + pub fn should_reject_early(&self, old: &VerifiedTransaction, new: &verifier::Transaction) -> bool { + if old.priority().is_local() { + return true + } + + &old.transaction.gas_price > new.gas_price() + } +} + impl txpool::Scoring for NonceAndGasPrice { type Score = U256; type Event = (); diff --git a/miner/src/pool/verifier.rs b/miner/src/pool/verifier.rs index c957fcab3fb..4703088ffb6 100644 --- a/miner/src/pool/verifier.rs +++ b/miner/src/pool/verifier.rs @@ -85,19 +85,20 @@ impl Transaction { } } - fn gas(&self) -> &U256 { + /// Return transaction gas price + pub fn gas_price(&self) -> &U256 { match *self { - Transaction::Unverified(ref tx) => &tx.gas, - Transaction::Retracted(ref tx) => &tx.gas, - Transaction::Local(ref tx) => &tx.gas, + Transaction::Unverified(ref tx) => &tx.gas_price, + Transaction::Retracted(ref tx) => &tx.gas_price, + Transaction::Local(ref tx) => &tx.gas_price, } } - fn gas_price(&self) -> &U256 { + fn gas(&self) -> &U256 { match *self { - Transaction::Unverified(ref tx) => &tx.gas_price, - Transaction::Retracted(ref tx) => &tx.gas_price, - Transaction::Local(ref tx) => &tx.gas_price, + Transaction::Unverified(ref tx) => &tx.gas, + Transaction::Retracted(ref tx) => &tx.gas, + Transaction::Local(ref tx) => &tx.gas, } } @@ -128,31 +129,31 @@ impl Transaction { /// /// Verification can be run in parallel for all incoming transactions. #[derive(Debug)] -pub struct Verifier { +pub struct Verifier { client: C, options: Options, - min_effective_gas_price: Option, id: Arc, + transaction_to_replace: Option<(S, Arc)>, } -impl Verifier { +impl Verifier { /// Creates new transaction verfier with specified options. pub fn new( client: C, options: Options, id: Arc, - min_effective_gas_price: Option, + transaction_to_replace: Option<(S, Arc)>, ) -> Self { Verifier { client, options, - min_effective_gas_price, id, + transaction_to_replace, } } } -impl txpool::Verifier for Verifier { +impl txpool::Verifier for Verifier { type Error = transaction::Error; type VerifiedTransaction = VerifiedTransaction; @@ -218,17 +219,17 @@ impl txpool::Verifier for Verifier { }); } - if let Some(ref gp) = self.min_effective_gas_price { - if tx.gas_price() < gp { + if let Some((ref scoring, ref vtx)) = self.transaction_to_replace { + if scoring.should_reject_early(vtx, &tx) { trace!( target: "txqueue", - "[{:?}] Rejected tx below minimal effective gas price threshold: {} < {}", + "[{:?}] Rejected tx early, cause it doesn't have any chance to get to the pool: (gas price: {} < {})", hash, tx.gas_price(), - gp, + vtx.transaction.gas_price, ); bail!(transaction::Error::InsufficientGasPrice { - minimal: *gp, + minimal: vtx.transaction.gas_price, got: *tx.gas_price(), }); } diff --git a/transaction-pool/src/pool.rs b/transaction-pool/src/pool.rs index 2c7dd4af19e..67da1b1d49c 100644 --- a/transaction-pool/src/pool.rs +++ b/transaction-pool/src/pool.rs @@ -393,23 +393,10 @@ impl Pool where self.worst_transactions.iter().next_back().map(|x| x.transaction.transaction.clone()) } - /// Returns the score of the worst transaction if the pool is almost full. - /// - /// This method can be used to determine what is the minimal required score - /// for the replacement transaction. If `None` is returned it means that - /// there is still plenty of room in the pool. Otherwise we return - /// `Some` with the score of the worst transaction. - pub fn minimal_entry_score(&self) -> Option { - let is_full = { - self.by_hash.len() >= self.options.max_count + /// Returns true if the pool is at it's capacity. + pub fn is_full(&self) -> bool { + self.by_hash.len() >= self.options.max_count || self.mem_usage >= self.options.max_mem_usage - }; - - if !is_full { - return None - } - - self.worst_transactions.iter().next_back().map(|x| x.score.clone()) } /// Returns an iterator of pending (ready) transactions. @@ -505,6 +492,11 @@ impl Pool where &self.listener } + /// Borrows scoring instance. + pub fn scoring(&self) -> &S { + &self.scoring + } + /// Borrows listener mutably. pub fn listener_mut(&mut self) -> &mut L { &mut self.listener diff --git a/transaction-pool/src/tests/mod.rs b/transaction-pool/src/tests/mod.rs index beb72e402a4..808f804cce9 100644 --- a/transaction-pool/src/tests/mod.rs +++ b/transaction-pool/src/tests/mod.rs @@ -521,20 +521,20 @@ fn should_return_worst_transaction() { } #[test] -fn should_return_min_entry_score_based_on_the_worst_transaction() { +fn should_return_is_full() { // given let b = TransactionBuilder::default(); let mut txq = TestPool::with_limit(2); - assert!(txq.worst_transaction().is_none()); + assert!(!txq.is_full()); // when txq.import(b.tx().nonce(0).gas_price(110).new()).unwrap(); - assert_eq!(txq.minimal_entry_score(), None); + assert!(!txq.is_full()); txq.import(b.tx().sender(1).nonce(0).gas_price(100).new()).unwrap(); // then - assert_eq!(txq.minimal_entry_score(), Some(100.into())); + assert!(txq.is_full()); } mod listener {