From 881678b61391564d7d9d2f46a5d6fa96c93451ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 18 Apr 2016 13:16:46 +0200 Subject: [PATCH 01/10] Even more detailed errors for transaction queue --- ethcore/src/error.rs | 9 ++- miner/src/lib.rs | 11 +-- miner/src/transaction_queue.rs | 133 +++++++++++++++++++++++++-------- 3 files changed, 111 insertions(+), 42 deletions(-) diff --git a/ethcore/src/error.rs b/ethcore/src/error.rs index 02cd6678b..f68295f38 100644 --- a/ethcore/src/error.rs +++ b/ethcore/src/error.rs @@ -62,13 +62,18 @@ pub enum ExecutionError { Internal } -#[derive(Debug)] +#[derive(Debug, PartialEq)] /// Errors concerning transaction processing. pub enum TransactionError { /// Transaction is already imported to the queue AlreadyImported, /// Transaction is not valid anymore (state already has higher nonce) Old, + /// Transaction has too low fee + /// (there is already a transaction with the same sender-nonce but higher gas price) + TooCheap, + /// Transaction was not imported to the queue because limit has been reached. + LimitReached, /// Transaction's gas price is below threshold. InsufficientGasPrice { /// Minimal expected gas price @@ -153,7 +158,7 @@ pub enum BlockError { UnknownUncleParent(H256), } -#[derive(Debug)] +#[derive(Debug, PartialEq)] /// Import to the block queue result pub enum ImportError { /// Already in the block chain. diff --git a/miner/src/lib.rs b/miner/src/lib.rs index 1b080f30c..e99f9217d 100644 --- a/miner/src/lib.rs +++ b/miner/src/lib.rs @@ -61,7 +61,7 @@ extern crate rayon; mod miner; mod transaction_queue; -pub use transaction_queue::{TransactionQueue, AccountDetails}; +pub use transaction_queue::{TransactionQueue, AccountDetails, TransactionImportResult}; pub use miner::{Miner}; use util::{H256, U256, Address, Bytes}; @@ -145,15 +145,6 @@ pub trait MinerService : Send + Sync { fn sensible_gas_limit(&self) -> U256 { x!(21000) } } -/// Represents the result of importing transaction. -#[derive(Debug)] -pub enum TransactionImportResult { - /// Transaction was imported to current queue. - Current, - /// Transaction was imported to future queue. - Future -} - /// Mining status #[derive(Debug)] pub struct MinerStatus { diff --git a/miner/src/transaction_queue.rs b/miner/src/transaction_queue.rs index bd017fd69..6b2008448 100644 --- a/miner/src/transaction_queue.rs +++ b/miner/src/transaction_queue.rs @@ -86,14 +86,30 @@ use std::default::Default; use std::cmp::{Ordering}; -use std::collections::{HashMap, BTreeSet}; +use std::collections::{HashMap, HashSet, BTreeSet}; use util::numbers::{Uint, U256}; use util::hash::{Address, H256}; use util::table::*; use ethcore::transaction::*; use ethcore::error::{Error, TransactionError}; -use super::TransactionImportResult; +macro_rules! check_too_cheap { + ($is_in:expr) => { + if !($is_in) { + return Err(TransactionError::TooCheap); + } + } +} + +macro_rules! check_limit { + ($hash:expr, $dropped:expr) => { + if let Some(dropped) = $dropped { + if dropped.contains(&$hash) { + return Err(TransactionError::LimitReached); + } + } + } +} #[derive(Clone, Debug)] /// Light structure used to identify transaction and it's order @@ -206,10 +222,11 @@ impl TransactionSet { /// Remove low priority transactions if there is more then specified by given `limit`. /// /// It drops transactions from this set but also removes associated `VerifiedTransaction`. - fn enforce_limit(&mut self, by_hash: &mut HashMap) { + /// Returns hashes of transactions removed because of limit. + fn enforce_limit(&mut self, by_hash: &mut HashMap) -> Option> { let len = self.by_priority.len(); if len <= self.limit { - return; + return None; } let to_drop : Vec<(Address, U256)> = { @@ -222,10 +239,17 @@ impl TransactionSet { .collect() }; - for (sender, nonce) in to_drop { - let order = self.drop(&sender, &nonce).expect("Transaction has just been found in `by_priority`; so it is in `by_address` also."); - by_hash.remove(&order.hash).expect("Hash found in `by_priorty` matches the one dropped; so it is included in `by_hash`"); - } + Some(to_drop + .into_iter() + .map(|(sender, nonce)| { + let order = self.drop(&sender, &nonce) + .expect("Transaction has just been found in `by_priority`; so it is in `by_address` also."); + + by_hash.remove(&order.hash) + .expect("Hash found in `by_priorty` matches the one dropped; so it is included in `by_hash`"); + order.hash + }) + .collect::>()) } /// Drop transaction from this set (remove from `by_priority` and `by_address`) @@ -253,6 +277,15 @@ pub struct TransactionQueueStatus { pub future: usize, } +#[derive(Debug, PartialEq)] +/// Represents the result of importing transaction. +pub enum TransactionImportResult { + /// Transaction was imported to current queue. + Current, + /// Transaction was imported to future queue. + Future +} + /// Details of account pub struct AccountDetails { /// Most recent account nonce @@ -580,6 +613,7 @@ impl TransactionQueue { let address = tx.sender(); let nonce = tx.nonce(); + let hash = tx.hash(); let next_nonce = self.last_nonces .get(&address) @@ -589,8 +623,8 @@ impl TransactionQueue { // Check height if nonce > next_nonce { // We have a gap - put to future - Self::replace_transaction(tx, next_nonce, &mut self.future, &mut self.by_hash); - self.future.enforce_limit(&mut self.by_hash); + check_too_cheap!(Self::replace_transaction(tx, next_nonce, &mut self.future, &mut self.by_hash)); + check_limit!(hash, self.future.enforce_limit(&mut self.by_hash)); return Ok(TransactionImportResult::Future); } else if nonce < state_nonce { // Droping transaction @@ -598,7 +632,7 @@ impl TransactionQueue { return Err(TransactionError::Old); } - Self::replace_transaction(tx, state_nonce, &mut self.current, &mut self.by_hash); + check_too_cheap!(Self::replace_transaction(tx, state_nonce, &mut self.current, &mut self.by_hash)); // Keep track of highest nonce stored in current self.last_nonces.insert(address, nonce); // Update nonces of transactions in future @@ -610,10 +644,10 @@ impl TransactionQueue { if let Some(order) = self.future.drop(&address, &nonce) { // Let's insert that transaction to current (if it has higher gas_price) let future_tx = self.by_hash.remove(&order.hash).unwrap(); - Self::replace_transaction(future_tx, state_nonce, &mut self.current, &mut self.by_hash); + check_too_cheap!(Self::replace_transaction(future_tx, state_nonce, &mut self.current, &mut self.by_hash)); } // Also enforce the limit - self.current.enforce_limit(&mut self.by_hash); + check_limit!(hash, self.current.enforce_limit(&mut self.by_hash)); trace!(target: "miner", "status: {:?}", self.status()); Ok(TransactionImportResult::Current) @@ -623,13 +657,17 @@ impl TransactionQueue { /// /// If there is already transaction with same `(sender, nonce)` it will be replaced iff `gas_price` is higher. /// One of the transactions is dropped from set and also removed from queue entirely (from `by_hash`). - fn replace_transaction(tx: VerifiedTransaction, base_nonce: U256, set: &mut TransactionSet, by_hash: &mut HashMap) { + /// + /// Returns `true` if transaction actually got to the queue (`false` if there was already a transaction with higher + /// gas_price) + fn replace_transaction(tx: VerifiedTransaction, base_nonce: U256, set: &mut TransactionSet, by_hash: &mut HashMap) -> bool { let order = TransactionOrder::for_transaction(&tx, base_nonce); let hash = tx.hash(); let address = tx.sender(); let nonce = tx.nonce(); by_hash.insert(hash, tx); + if let Some(old) = set.insert(address, nonce, order.clone()) { // There was already transaction in queue. Let's check which one should stay let old_fee = old.gas_price; @@ -639,24 +677,35 @@ impl TransactionQueue { set.insert(address, nonce, old); // and remove new one by_hash.remove(&hash); + false } else { // Make sure we remove old transaction entirely by_hash.remove(&old.hash); + true } + } else { + true } } } - #[cfg(test)] mod test { extern crate rustc_serialize; use util::table::*; use util::*; use ethcore::transaction::*; + use ethcore::error::{Error, TransactionError}; use super::*; use super::{TransactionSet, TransactionOrder, VerifiedTransaction}; + fn unwrap_tx_err(err: Result) -> TransactionError { + match err.unwrap_err() { + Error::Transaction(e) => e, + _ => panic!("Expected transaction error!"), + } + } + fn new_unsigned_tx(nonce: U256) -> Transaction { Transaction { action: Action::Create, @@ -698,11 +747,16 @@ mod test { } fn new_txs(second_nonce: U256) -> (SignedTransaction, SignedTransaction) { + new_txs_with_gas_price_diff(second_nonce, U256::zero()) + } + + fn new_txs_with_gas_price_diff(second_nonce: U256, gas_price: U256) -> (SignedTransaction, SignedTransaction) { let keypair = KeyPair::create().unwrap(); let secret = &keypair.secret(); let nonce = U256::from(123); let tx = new_unsigned_tx(nonce); - let tx2 = new_unsigned_tx(nonce + second_nonce); + let mut tx2 = new_unsigned_tx(nonce + second_nonce); + tx2.gas_price = tx2.gas_price + gas_price; (tx.sign(secret), tx2.sign(secret)) } @@ -795,14 +849,14 @@ mod test { // First insert one transaction to future let res = txq.add(tx, &prev_nonce); - assert!(res.is_ok()); + assert_eq!(res.unwrap(), TransactionImportResult::Future); assert_eq!(txq.status().future, 1); // now import second transaction to current let res = txq.add(tx2.clone(), &default_nonce); // and then there should be only one transaction in current (the one with higher gas_price) - assert!(res.is_ok()); + assert_eq!(unwrap_tx_err(res), TransactionError::TooCheap); assert_eq!(txq.status().pending, 1); assert_eq!(txq.status().future, 0); assert_eq!(txq.current.by_priority.len(), 1); @@ -821,7 +875,7 @@ mod test { let res = txq.add(tx, &default_nonce); // then - assert!(res.is_ok()); + assert_eq!(res.unwrap(), TransactionImportResult::Current); let stats = txq.status(); assert_eq!(stats.pending, 1); } @@ -845,12 +899,18 @@ mod test { // given let mut txq = TransactionQueue::new(); let tx = new_tx(); - txq.set_gas_limit(tx.gas / U256::from(2)); + let gas = tx.gas; + let limit = gas / U256::from(2); + txq.set_gas_limit(limit); // when - txq.add(tx, &default_nonce).unwrap_err(); + let res = txq.add(tx, &default_nonce); // then + assert_eq!(unwrap_tx_err(res), TransactionError::GasLimitExceeded { + limit: U256::from(55_000), // Should be 110% of set_gas_limit + got: gas, + }); let stats = txq.status(); assert_eq!(stats.pending, 0); assert_eq!(stats.future, 0); @@ -868,9 +928,13 @@ mod test { }; // when - txq.add(tx, &account).unwrap_err(); + let res = txq.add(tx, &account); // then + assert_eq!(unwrap_tx_err(res), TransactionError::InsufficientBalance { + balance: U256::from(1), + cost: U256::from(100_100), + }); let stats = txq.status(); assert_eq!(stats.pending, 0); assert_eq!(stats.future, 0); @@ -884,9 +948,13 @@ mod test { txq.set_minimal_gas_price(tx.gas_price + U256::one()); // when - txq.add(tx, &default_nonce).unwrap_err(); + let res = txq.add(tx, &default_nonce); // then + assert_eq!(unwrap_tx_err(res), TransactionError::InsufficientGasPrice { + minimal: U256::from(2), + got: U256::from(1), + }); let stats = txq.status(); assert_eq!(stats.pending, 0); assert_eq!(stats.future, 0); @@ -961,10 +1029,12 @@ mod test { let (tx, tx2) = new_txs(U256::from(2)); // when - txq.add(tx.clone(), &default_nonce).unwrap(); - txq.add(tx2.clone(), &default_nonce).unwrap(); + let res1 = txq.add(tx.clone(), &default_nonce).unwrap(); + let res2 = txq.add(tx2.clone(), &default_nonce).unwrap(); // then + assert_eq!(res1, TransactionImportResult::Current); + assert_eq!(res2, TransactionImportResult::Future); let stats = txq.status(); assert_eq!(stats.pending, 1); assert_eq!(stats.future, 1); @@ -1091,10 +1161,11 @@ mod test { assert_eq!(txq.status().pending, 1); // when - txq.add(tx2.clone(), &default_nonce).unwrap(); + let res = txq.add(tx2.clone(), &default_nonce); // then let t = txq.top_transactions(); + assert_eq!(unwrap_tx_err(res), TransactionError::LimitReached); assert_eq!(txq.status().pending, 1); assert_eq!(t.len(), 1); assert_eq!(t[0], tx); @@ -1103,8 +1174,8 @@ mod test { #[test] fn should_limit_future_transactions() { let mut txq = TransactionQueue::with_limits(10, 1); - let (tx1, tx2) = new_txs(U256::from(4)); - let (tx3, tx4) = new_txs(U256::from(4)); + let (tx1, tx2) = new_txs_with_gas_price_diff(U256::from(4), U256::from(1)); + let (tx3, tx4) = new_txs_with_gas_price_diff(U256::from(4), U256::from(2)); txq.add(tx1.clone(), &default_nonce).unwrap(); txq.add(tx3.clone(), &default_nonce).unwrap(); assert_eq!(txq.status().pending, 2); @@ -1126,9 +1197,10 @@ mod test { let fetch_last_nonce = |_a: &Address| AccountDetails{ nonce: last_nonce, balance: !U256::zero() }; // when - txq.add(tx, &fetch_last_nonce).unwrap_err(); + let res = txq.add(tx, &fetch_last_nonce); // then + assert_eq!(unwrap_tx_err(res), TransactionError::Old); let stats = txq.status(); assert_eq!(stats.pending, 0); assert_eq!(stats.future, 0); @@ -1146,9 +1218,10 @@ mod test { assert_eq!(txq.status().pending, 0); // when - txq.add(tx2.clone(), &nonce).unwrap_err(); + let res = txq.add(tx2.clone(), &nonce); // then + assert_eq!(unwrap_tx_err(res), TransactionError::AlreadyImported); let stats = txq.status(); assert_eq!(stats.future, 1); assert_eq!(stats.pending, 0); From 2812f8cae60789a303e8ee2c5ad82a6fb0699a1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 18 Apr 2016 13:35:19 +0200 Subject: [PATCH 02/10] Small rename --- ethcore/src/error.rs | 2 +- miner/src/transaction_queue.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ethcore/src/error.rs b/ethcore/src/error.rs index f68295f38..d74e225f2 100644 --- a/ethcore/src/error.rs +++ b/ethcore/src/error.rs @@ -71,7 +71,7 @@ pub enum TransactionError { Old, /// Transaction has too low fee /// (there is already a transaction with the same sender-nonce but higher gas price) - TooCheap, + TooCheapToReplace, /// Transaction was not imported to the queue because limit has been reached. LimitReached, /// Transaction's gas price is below threshold. diff --git a/miner/src/transaction_queue.rs b/miner/src/transaction_queue.rs index 6b2008448..9db2fd78a 100644 --- a/miner/src/transaction_queue.rs +++ b/miner/src/transaction_queue.rs @@ -96,7 +96,7 @@ use ethcore::error::{Error, TransactionError}; macro_rules! check_too_cheap { ($is_in:expr) => { if !($is_in) { - return Err(TransactionError::TooCheap); + return Err(TransactionError::TooCheapToReplace); } } } @@ -856,7 +856,7 @@ mod test { let res = txq.add(tx2.clone(), &default_nonce); // and then there should be only one transaction in current (the one with higher gas_price) - assert_eq!(unwrap_tx_err(res), TransactionError::TooCheap); + assert_eq!(unwrap_tx_err(res), TransactionError::TooCheapToReplace); assert_eq!(txq.status().pending, 1); assert_eq!(txq.status().future, 0); assert_eq!(txq.current.by_priority.len(), 1); From 41153dd37cbeee1ba27e30688da09c55fefaea5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 18 Apr 2016 17:20:35 +0200 Subject: [PATCH 03/10] Removing macros in favour of functions+try!() --- miner/src/transaction_queue.rs | 46 +++++++++++++++++----------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/miner/src/transaction_queue.rs b/miner/src/transaction_queue.rs index 9db2fd78a..ecfb2b8b0 100644 --- a/miner/src/transaction_queue.rs +++ b/miner/src/transaction_queue.rs @@ -93,24 +93,6 @@ use util::table::*; use ethcore::transaction::*; use ethcore::error::{Error, TransactionError}; -macro_rules! check_too_cheap { - ($is_in:expr) => { - if !($is_in) { - return Err(TransactionError::TooCheapToReplace); - } - } -} - -macro_rules! check_limit { - ($hash:expr, $dropped:expr) => { - if let Some(dropped) = $dropped { - if dropped.contains(&$hash) { - return Err(TransactionError::LimitReached); - } - } - } -} - #[derive(Clone, Debug)] /// Light structure used to identify transaction and it's order struct TransactionOrder { @@ -623,8 +605,8 @@ impl TransactionQueue { // Check height if nonce > next_nonce { // We have a gap - put to future - check_too_cheap!(Self::replace_transaction(tx, next_nonce, &mut self.future, &mut self.by_hash)); - check_limit!(hash, self.future.enforce_limit(&mut self.by_hash)); + try!(check_too_cheap(Self::replace_transaction(tx, next_nonce, &mut self.future, &mut self.by_hash))); + try!(check_if_removed(&hash, self.future.enforce_limit(&mut self.by_hash))); return Ok(TransactionImportResult::Future); } else if nonce < state_nonce { // Droping transaction @@ -632,7 +614,7 @@ impl TransactionQueue { return Err(TransactionError::Old); } - check_too_cheap!(Self::replace_transaction(tx, state_nonce, &mut self.current, &mut self.by_hash)); + try!(check_too_cheap(Self::replace_transaction(tx, state_nonce, &mut self.current, &mut self.by_hash))); // Keep track of highest nonce stored in current self.last_nonces.insert(address, nonce); // Update nonces of transactions in future @@ -644,10 +626,10 @@ impl TransactionQueue { if let Some(order) = self.future.drop(&address, &nonce) { // Let's insert that transaction to current (if it has higher gas_price) let future_tx = self.by_hash.remove(&order.hash).unwrap(); - check_too_cheap!(Self::replace_transaction(future_tx, state_nonce, &mut self.current, &mut self.by_hash)); + try!(check_too_cheap(Self::replace_transaction(future_tx, state_nonce, &mut self.current, &mut self.by_hash))); } // Also enforce the limit - check_limit!(hash, self.current.enforce_limit(&mut self.by_hash)); + try!(check_if_removed(&hash, self.current.enforce_limit(&mut self.by_hash))); trace!(target: "miner", "status: {:?}", self.status()); Ok(TransactionImportResult::Current) @@ -689,6 +671,24 @@ impl TransactionQueue { } } +fn check_too_cheap(is_in: bool) -> Result<(), TransactionError> { + if !is_in { + Err(TransactionError::TooCheapToReplace) + } else { + Ok(()) + } +} + +fn check_if_removed(hash: &H256, dropped: Option>) -> Result<(), TransactionError> { + match dropped { + Some(ref dropped) if dropped.contains(hash) => { + Err(TransactionError::LimitReached) + }, + _ => Ok(()) + } +} + + #[cfg(test)] mod test { extern crate rustc_serialize; From 58c47069d8e60b6010d0e9549d8ee43f401b499e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 18 Apr 2016 18:39:14 +0200 Subject: [PATCH 04/10] Enforce-limit + last_nonces bug --- miner/src/transaction_queue.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/miner/src/transaction_queue.rs b/miner/src/transaction_queue.rs index ecfb2b8b0..5d0551c94 100644 --- a/miner/src/transaction_queue.rs +++ b/miner/src/transaction_queue.rs @@ -629,7 +629,11 @@ impl TransactionQueue { try!(check_too_cheap(Self::replace_transaction(future_tx, state_nonce, &mut self.current, &mut self.by_hash))); } // Also enforce the limit - try!(check_if_removed(&hash, self.current.enforce_limit(&mut self.by_hash))); + if let Err(e) = check_if_removed(&hash, self.current.enforce_limit(&mut self.by_hash)) { + // If current transaction was removed because of limit we need to update last_nonces also. + self.last_nonces.insert(address, nonce - U256::one()); + return Err(e); + } trace!(target: "miner", "status: {:?}", self.status()); Ok(TransactionImportResult::Current) @@ -1157,6 +1161,8 @@ mod test { // given let mut txq = TransactionQueue::with_limits(1, 1); let (tx, tx2) = new_txs(U256::one()); + let sender = tx.sender().unwrap(); + let nonce = tx.nonce; txq.add(tx.clone(), &default_nonce).unwrap(); assert_eq!(txq.status().pending, 1); @@ -1169,6 +1175,7 @@ mod test { assert_eq!(txq.status().pending, 1); assert_eq!(t.len(), 1); assert_eq!(t[0], tx); + assert_eq!(txq.last_nonce(&sender), Some(nonce)); } #[test] From caf4d179a29a7ad35534ab0032a43789c950db1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 18 Apr 2016 19:34:59 +0200 Subject: [PATCH 05/10] Even more detailed errors for transaction queue (#969) * Even more detailed errors for transaction queue * Small rename * Removing macros in favour of functions+try!() --- ethcore/src/error.rs | 9 ++- miner/src/lib.rs | 11 +-- miner/src/transaction_queue.rs | 133 +++++++++++++++++++++++++-------- 3 files changed, 111 insertions(+), 42 deletions(-) diff --git a/ethcore/src/error.rs b/ethcore/src/error.rs index 02cd6678b..d74e225f2 100644 --- a/ethcore/src/error.rs +++ b/ethcore/src/error.rs @@ -62,13 +62,18 @@ pub enum ExecutionError { Internal } -#[derive(Debug)] +#[derive(Debug, PartialEq)] /// Errors concerning transaction processing. pub enum TransactionError { /// Transaction is already imported to the queue AlreadyImported, /// Transaction is not valid anymore (state already has higher nonce) Old, + /// Transaction has too low fee + /// (there is already a transaction with the same sender-nonce but higher gas price) + TooCheapToReplace, + /// Transaction was not imported to the queue because limit has been reached. + LimitReached, /// Transaction's gas price is below threshold. InsufficientGasPrice { /// Minimal expected gas price @@ -153,7 +158,7 @@ pub enum BlockError { UnknownUncleParent(H256), } -#[derive(Debug)] +#[derive(Debug, PartialEq)] /// Import to the block queue result pub enum ImportError { /// Already in the block chain. diff --git a/miner/src/lib.rs b/miner/src/lib.rs index 1b080f30c..e99f9217d 100644 --- a/miner/src/lib.rs +++ b/miner/src/lib.rs @@ -61,7 +61,7 @@ extern crate rayon; mod miner; mod transaction_queue; -pub use transaction_queue::{TransactionQueue, AccountDetails}; +pub use transaction_queue::{TransactionQueue, AccountDetails, TransactionImportResult}; pub use miner::{Miner}; use util::{H256, U256, Address, Bytes}; @@ -145,15 +145,6 @@ pub trait MinerService : Send + Sync { fn sensible_gas_limit(&self) -> U256 { x!(21000) } } -/// Represents the result of importing transaction. -#[derive(Debug)] -pub enum TransactionImportResult { - /// Transaction was imported to current queue. - Current, - /// Transaction was imported to future queue. - Future -} - /// Mining status #[derive(Debug)] pub struct MinerStatus { diff --git a/miner/src/transaction_queue.rs b/miner/src/transaction_queue.rs index bd017fd69..ecfb2b8b0 100644 --- a/miner/src/transaction_queue.rs +++ b/miner/src/transaction_queue.rs @@ -86,14 +86,12 @@ use std::default::Default; use std::cmp::{Ordering}; -use std::collections::{HashMap, BTreeSet}; +use std::collections::{HashMap, HashSet, BTreeSet}; use util::numbers::{Uint, U256}; use util::hash::{Address, H256}; use util::table::*; use ethcore::transaction::*; use ethcore::error::{Error, TransactionError}; -use super::TransactionImportResult; - #[derive(Clone, Debug)] /// Light structure used to identify transaction and it's order @@ -206,10 +204,11 @@ impl TransactionSet { /// Remove low priority transactions if there is more then specified by given `limit`. /// /// It drops transactions from this set but also removes associated `VerifiedTransaction`. - fn enforce_limit(&mut self, by_hash: &mut HashMap) { + /// Returns hashes of transactions removed because of limit. + fn enforce_limit(&mut self, by_hash: &mut HashMap) -> Option> { let len = self.by_priority.len(); if len <= self.limit { - return; + return None; } let to_drop : Vec<(Address, U256)> = { @@ -222,10 +221,17 @@ impl TransactionSet { .collect() }; - for (sender, nonce) in to_drop { - let order = self.drop(&sender, &nonce).expect("Transaction has just been found in `by_priority`; so it is in `by_address` also."); - by_hash.remove(&order.hash).expect("Hash found in `by_priorty` matches the one dropped; so it is included in `by_hash`"); - } + Some(to_drop + .into_iter() + .map(|(sender, nonce)| { + let order = self.drop(&sender, &nonce) + .expect("Transaction has just been found in `by_priority`; so it is in `by_address` also."); + + by_hash.remove(&order.hash) + .expect("Hash found in `by_priorty` matches the one dropped; so it is included in `by_hash`"); + order.hash + }) + .collect::>()) } /// Drop transaction from this set (remove from `by_priority` and `by_address`) @@ -253,6 +259,15 @@ pub struct TransactionQueueStatus { pub future: usize, } +#[derive(Debug, PartialEq)] +/// Represents the result of importing transaction. +pub enum TransactionImportResult { + /// Transaction was imported to current queue. + Current, + /// Transaction was imported to future queue. + Future +} + /// Details of account pub struct AccountDetails { /// Most recent account nonce @@ -580,6 +595,7 @@ impl TransactionQueue { let address = tx.sender(); let nonce = tx.nonce(); + let hash = tx.hash(); let next_nonce = self.last_nonces .get(&address) @@ -589,8 +605,8 @@ impl TransactionQueue { // Check height if nonce > next_nonce { // We have a gap - put to future - Self::replace_transaction(tx, next_nonce, &mut self.future, &mut self.by_hash); - self.future.enforce_limit(&mut self.by_hash); + try!(check_too_cheap(Self::replace_transaction(tx, next_nonce, &mut self.future, &mut self.by_hash))); + try!(check_if_removed(&hash, self.future.enforce_limit(&mut self.by_hash))); return Ok(TransactionImportResult::Future); } else if nonce < state_nonce { // Droping transaction @@ -598,7 +614,7 @@ impl TransactionQueue { return Err(TransactionError::Old); } - Self::replace_transaction(tx, state_nonce, &mut self.current, &mut self.by_hash); + try!(check_too_cheap(Self::replace_transaction(tx, state_nonce, &mut self.current, &mut self.by_hash))); // Keep track of highest nonce stored in current self.last_nonces.insert(address, nonce); // Update nonces of transactions in future @@ -610,10 +626,10 @@ impl TransactionQueue { if let Some(order) = self.future.drop(&address, &nonce) { // Let's insert that transaction to current (if it has higher gas_price) let future_tx = self.by_hash.remove(&order.hash).unwrap(); - Self::replace_transaction(future_tx, state_nonce, &mut self.current, &mut self.by_hash); + try!(check_too_cheap(Self::replace_transaction(future_tx, state_nonce, &mut self.current, &mut self.by_hash))); } // Also enforce the limit - self.current.enforce_limit(&mut self.by_hash); + try!(check_if_removed(&hash, self.current.enforce_limit(&mut self.by_hash))); trace!(target: "miner", "status: {:?}", self.status()); Ok(TransactionImportResult::Current) @@ -623,13 +639,17 @@ impl TransactionQueue { /// /// If there is already transaction with same `(sender, nonce)` it will be replaced iff `gas_price` is higher. /// One of the transactions is dropped from set and also removed from queue entirely (from `by_hash`). - fn replace_transaction(tx: VerifiedTransaction, base_nonce: U256, set: &mut TransactionSet, by_hash: &mut HashMap) { + /// + /// Returns `true` if transaction actually got to the queue (`false` if there was already a transaction with higher + /// gas_price) + fn replace_transaction(tx: VerifiedTransaction, base_nonce: U256, set: &mut TransactionSet, by_hash: &mut HashMap) -> bool { let order = TransactionOrder::for_transaction(&tx, base_nonce); let hash = tx.hash(); let address = tx.sender(); let nonce = tx.nonce(); by_hash.insert(hash, tx); + if let Some(old) = set.insert(address, nonce, order.clone()) { // There was already transaction in queue. Let's check which one should stay let old_fee = old.gas_price; @@ -639,14 +659,35 @@ impl TransactionQueue { set.insert(address, nonce, old); // and remove new one by_hash.remove(&hash); + false } else { // Make sure we remove old transaction entirely by_hash.remove(&old.hash); + true } + } else { + true } } } +fn check_too_cheap(is_in: bool) -> Result<(), TransactionError> { + if !is_in { + Err(TransactionError::TooCheapToReplace) + } else { + Ok(()) + } +} + +fn check_if_removed(hash: &H256, dropped: Option>) -> Result<(), TransactionError> { + match dropped { + Some(ref dropped) if dropped.contains(hash) => { + Err(TransactionError::LimitReached) + }, + _ => Ok(()) + } +} + #[cfg(test)] mod test { @@ -654,9 +695,17 @@ mod test { use util::table::*; use util::*; use ethcore::transaction::*; + use ethcore::error::{Error, TransactionError}; use super::*; use super::{TransactionSet, TransactionOrder, VerifiedTransaction}; + fn unwrap_tx_err(err: Result) -> TransactionError { + match err.unwrap_err() { + Error::Transaction(e) => e, + _ => panic!("Expected transaction error!"), + } + } + fn new_unsigned_tx(nonce: U256) -> Transaction { Transaction { action: Action::Create, @@ -698,11 +747,16 @@ mod test { } fn new_txs(second_nonce: U256) -> (SignedTransaction, SignedTransaction) { + new_txs_with_gas_price_diff(second_nonce, U256::zero()) + } + + fn new_txs_with_gas_price_diff(second_nonce: U256, gas_price: U256) -> (SignedTransaction, SignedTransaction) { let keypair = KeyPair::create().unwrap(); let secret = &keypair.secret(); let nonce = U256::from(123); let tx = new_unsigned_tx(nonce); - let tx2 = new_unsigned_tx(nonce + second_nonce); + let mut tx2 = new_unsigned_tx(nonce + second_nonce); + tx2.gas_price = tx2.gas_price + gas_price; (tx.sign(secret), tx2.sign(secret)) } @@ -795,14 +849,14 @@ mod test { // First insert one transaction to future let res = txq.add(tx, &prev_nonce); - assert!(res.is_ok()); + assert_eq!(res.unwrap(), TransactionImportResult::Future); assert_eq!(txq.status().future, 1); // now import second transaction to current let res = txq.add(tx2.clone(), &default_nonce); // and then there should be only one transaction in current (the one with higher gas_price) - assert!(res.is_ok()); + assert_eq!(unwrap_tx_err(res), TransactionError::TooCheapToReplace); assert_eq!(txq.status().pending, 1); assert_eq!(txq.status().future, 0); assert_eq!(txq.current.by_priority.len(), 1); @@ -821,7 +875,7 @@ mod test { let res = txq.add(tx, &default_nonce); // then - assert!(res.is_ok()); + assert_eq!(res.unwrap(), TransactionImportResult::Current); let stats = txq.status(); assert_eq!(stats.pending, 1); } @@ -845,12 +899,18 @@ mod test { // given let mut txq = TransactionQueue::new(); let tx = new_tx(); - txq.set_gas_limit(tx.gas / U256::from(2)); + let gas = tx.gas; + let limit = gas / U256::from(2); + txq.set_gas_limit(limit); // when - txq.add(tx, &default_nonce).unwrap_err(); + let res = txq.add(tx, &default_nonce); // then + assert_eq!(unwrap_tx_err(res), TransactionError::GasLimitExceeded { + limit: U256::from(55_000), // Should be 110% of set_gas_limit + got: gas, + }); let stats = txq.status(); assert_eq!(stats.pending, 0); assert_eq!(stats.future, 0); @@ -868,9 +928,13 @@ mod test { }; // when - txq.add(tx, &account).unwrap_err(); + let res = txq.add(tx, &account); // then + assert_eq!(unwrap_tx_err(res), TransactionError::InsufficientBalance { + balance: U256::from(1), + cost: U256::from(100_100), + }); let stats = txq.status(); assert_eq!(stats.pending, 0); assert_eq!(stats.future, 0); @@ -884,9 +948,13 @@ mod test { txq.set_minimal_gas_price(tx.gas_price + U256::one()); // when - txq.add(tx, &default_nonce).unwrap_err(); + let res = txq.add(tx, &default_nonce); // then + assert_eq!(unwrap_tx_err(res), TransactionError::InsufficientGasPrice { + minimal: U256::from(2), + got: U256::from(1), + }); let stats = txq.status(); assert_eq!(stats.pending, 0); assert_eq!(stats.future, 0); @@ -961,10 +1029,12 @@ mod test { let (tx, tx2) = new_txs(U256::from(2)); // when - txq.add(tx.clone(), &default_nonce).unwrap(); - txq.add(tx2.clone(), &default_nonce).unwrap(); + let res1 = txq.add(tx.clone(), &default_nonce).unwrap(); + let res2 = txq.add(tx2.clone(), &default_nonce).unwrap(); // then + assert_eq!(res1, TransactionImportResult::Current); + assert_eq!(res2, TransactionImportResult::Future); let stats = txq.status(); assert_eq!(stats.pending, 1); assert_eq!(stats.future, 1); @@ -1091,10 +1161,11 @@ mod test { assert_eq!(txq.status().pending, 1); // when - txq.add(tx2.clone(), &default_nonce).unwrap(); + let res = txq.add(tx2.clone(), &default_nonce); // then let t = txq.top_transactions(); + assert_eq!(unwrap_tx_err(res), TransactionError::LimitReached); assert_eq!(txq.status().pending, 1); assert_eq!(t.len(), 1); assert_eq!(t[0], tx); @@ -1103,8 +1174,8 @@ mod test { #[test] fn should_limit_future_transactions() { let mut txq = TransactionQueue::with_limits(10, 1); - let (tx1, tx2) = new_txs(U256::from(4)); - let (tx3, tx4) = new_txs(U256::from(4)); + let (tx1, tx2) = new_txs_with_gas_price_diff(U256::from(4), U256::from(1)); + let (tx3, tx4) = new_txs_with_gas_price_diff(U256::from(4), U256::from(2)); txq.add(tx1.clone(), &default_nonce).unwrap(); txq.add(tx3.clone(), &default_nonce).unwrap(); assert_eq!(txq.status().pending, 2); @@ -1126,9 +1197,10 @@ mod test { let fetch_last_nonce = |_a: &Address| AccountDetails{ nonce: last_nonce, balance: !U256::zero() }; // when - txq.add(tx, &fetch_last_nonce).unwrap_err(); + let res = txq.add(tx, &fetch_last_nonce); // then + assert_eq!(unwrap_tx_err(res), TransactionError::Old); let stats = txq.status(); assert_eq!(stats.pending, 0); assert_eq!(stats.future, 0); @@ -1146,9 +1218,10 @@ mod test { assert_eq!(txq.status().pending, 0); // when - txq.add(tx2.clone(), &nonce).unwrap_err(); + let res = txq.add(tx2.clone(), &nonce); // then + assert_eq!(unwrap_tx_err(res), TransactionError::AlreadyImported); let stats = txq.status(); assert_eq!(stats.future, 1); assert_eq!(stats.pending, 0); From 5df817c8e0ad8231d93e7f1224f0bcf510515441 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 18 Apr 2016 23:03:41 +0200 Subject: [PATCH 06/10] Setting limit from CLI --- miner/src/lib.rs | 6 ++++++ miner/src/miner.rs | 8 ++++++++ miner/src/transaction_queue.rs | 34 ++++++++++++++++++++++++++++------ parity/main.rs | 4 ++++ 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/miner/src/lib.rs b/miner/src/lib.rs index 1b080f30c..712ceb33b 100644 --- a/miner/src/lib.rs +++ b/miner/src/lib.rs @@ -100,6 +100,12 @@ pub trait MinerService : Send + Sync { /// Set the gas limit we wish to target when sealing a new block. fn set_gas_floor_target(&self, target: U256); + /// Get current transactions limit in queue. + fn transactions_limit(&self) -> usize; + + /// Set maximal number of transactions kept in the queue (both current and future). + fn set_transactions_limit(&self, limit: usize); + /// Imports transactions to transaction queue. fn import_transactions(&self, transactions: Vec, fetch_account: T) -> Vec> diff --git a/miner/src/miner.rs b/miner/src/miner.rs index 287250f04..a45b34f85 100644 --- a/miner/src/miner.rs +++ b/miner/src/miner.rs @@ -205,6 +205,14 @@ impl MinerService for Miner { *self.gas_floor_target.read().unwrap() / x!(5) } + fn transactions_limit(&self) -> usize { + self.transaction_queue.lock().unwrap().limit() + } + + fn set_transactions_limit(&self, limit: usize) { + self.transaction_queue.lock().unwrap().set_limit(limit) + } + /// Get the author that we will seal blocks as. fn author(&self) -> Address { *self.author.read().unwrap() diff --git a/miner/src/transaction_queue.rs b/miner/src/transaction_queue.rs index bd017fd69..4c1a0f457 100644 --- a/miner/src/transaction_queue.rs +++ b/miner/src/transaction_queue.rs @@ -242,6 +242,12 @@ impl TransactionSet { self.by_priority.clear(); self.by_address.clear(); } + + /// Sets new limit for number of transactions in this `TransactionSet`. + /// Note the limit is not applied (no transactions are removed) by calling this method. + fn set_limit(&mut self, limit: usize) { + self.limit = limit; + } } #[derive(Debug)] @@ -290,20 +296,21 @@ impl Default for TransactionQueue { impl TransactionQueue { /// Creates new instance of this Queue pub fn new() -> Self { - Self::with_limits(1024, 1024) + Self::with_limit(1024) } /// Create new instance of this Queue with specified limits - pub fn with_limits(current_limit: usize, future_limit: usize) -> Self { + pub fn with_limit(limit: usize) -> Self { let current = TransactionSet { by_priority: BTreeSet::new(), by_address: Table::new(), - limit: current_limit, + limit: limit, }; + let future = TransactionSet { by_priority: BTreeSet::new(), by_address: Table::new(), - limit: future_limit, + limit: limit, }; TransactionQueue { @@ -316,6 +323,20 @@ impl TransactionQueue { } } + /// Set the new limit for `current` and `future` queue. + pub fn set_limit(&mut self, limit: usize) { + self.current.set_limit(limit); + self.future.set_limit(limit); + // And ensure the limits + self.current.enforce_limit(&mut self.by_hash); + self.future.enforce_limit(&mut self.by_hash); + } + + /// Returns current limit of transactions in the queue. + pub fn limit(&self) -> usize { + self.current.limit + } + /// Get the minimal gas price. pub fn minimal_gas_price(&self) -> &U256 { &self.minimal_gas_price @@ -1085,7 +1106,7 @@ mod test { #[test] fn should_drop_old_transactions_when_hitting_the_limit() { // given - let mut txq = TransactionQueue::with_limits(1, 1); + let mut txq = TransactionQueue::with_limit(1); let (tx, tx2) = new_txs(U256::one()); txq.add(tx.clone(), &default_nonce).unwrap(); assert_eq!(txq.status().pending, 1); @@ -1102,7 +1123,8 @@ mod test { #[test] fn should_limit_future_transactions() { - let mut txq = TransactionQueue::with_limits(10, 1); + let mut txq = TransactionQueue::with_limit(1); + txq.current.set_limit(10); let (tx1, tx2) = new_txs(U256::from(4)); let (tx3, tx4) = new_txs(U256::from(4)); txq.add(tx1.clone(), &default_nonce).unwrap(); diff --git a/parity/main.rs b/parity/main.rs index 8e9c79b27..8eb67a46f 100644 --- a/parity/main.rs +++ b/parity/main.rs @@ -172,6 +172,8 @@ Sealing/Mining Options: [default: 0037a6b811ffeb6e072da21179d11b1406371c63]. --extra-data STRING Specify a custom extra-data for authored blocks, no more than 32 characters. + --tx-limit LIMIT Limit of transactions kept in the queue (waiting to + be included in next block) [default: 1024]. Footprint Options: --pruning METHOD Configure pruning of the state/storage trie. METHOD @@ -259,6 +261,7 @@ struct Args { flag_usd_per_eth: String, flag_gas_floor_target: String, flag_extra_data: Option, + flag_tx_limit: usize, flag_logging: Option, flag_version: bool, // geth-compatibility... @@ -713,6 +716,7 @@ impl Configuration { miner.set_gas_floor_target(self.gas_floor_target()); miner.set_extra_data(self.extra_data()); miner.set_minimal_gas_price(self.gas_price()); + miner.set_transactions_limit(self.args.flag_tx_limit); // Sync let sync = EthSync::register(service.network(), sync_config, client.clone(), miner.clone()); From 98b3962412b9375d7b8db7505fd67d429f15001e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 18 Apr 2016 23:13:38 +0200 Subject: [PATCH 07/10] RPC methods to set the limits --- rpc/src/v1/impls/ethcore.rs | 11 +++++++++ rpc/src/v1/tests/ethcore.rs | 28 +++++++++++++++++++++++ rpc/src/v1/tests/helpers/miner_service.rs | 10 ++++++++ rpc/src/v1/traits/ethcore.rs | 8 +++++++ 4 files changed, 57 insertions(+) diff --git a/rpc/src/v1/impls/ethcore.rs b/rpc/src/v1/impls/ethcore.rs index d9764842c..2d1267a02 100644 --- a/rpc/src/v1/impls/ethcore.rs +++ b/rpc/src/v1/impls/ethcore.rs @@ -67,6 +67,17 @@ impl Ethcore for EthcoreClient where M: MinerService + 'static { }) } + fn set_transactions_limit(&self, params: Params) -> Result { + from_params::<(usize,)>(params).and_then(|(limit,)| { + take_weak!(self.miner).set_transactions_limit(limit); + to_value(&true) + }) + } + + fn transactions_limit(&self, _: Params) -> Result { + to_value(&take_weak!(self.miner).transactions_limit()) + } + fn min_gas_price(&self, _: Params) -> Result { to_value(&take_weak!(self.miner).minimal_gas_price()) } diff --git a/rpc/src/v1/tests/ethcore.rs b/rpc/src/v1/tests/ethcore.rs index 06d4ffc1c..bef3180cf 100644 --- a/rpc/src/v1/tests/ethcore.rs +++ b/rpc/src/v1/tests/ethcore.rs @@ -123,3 +123,31 @@ fn rpc_ethcore_set_author() { assert_eq!(io.handle_request(request), Some(response.to_owned())); assert_eq!(miner.author(), Address::from_str("cd1722f3947def4cf144679da39c4c32bdc35681").unwrap()); } + +#[test] +fn rpc_ethcore_set_transactions_limit() { + let miner = miner_service(); + let ethcore = EthcoreClient::new(&miner).to_delegate(); + let io = IoHandler::new(); + io.add_delegate(ethcore); + + let request = r#"{"jsonrpc": "2.0", "method": "ethcore_setTransactionsLimit", "params":[10240240], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","result":true,"id":1}"#; + + assert_eq!(io.handle_request(request), Some(response.to_owned())); + assert_eq!(miner.transactions_limit(), 10_240_240); +} + + +#[test] +fn rpc_ethcore_transactions_limit() { + let miner = miner_service(); + let ethcore = EthcoreClient::new(&miner).to_delegate(); + let io = IoHandler::new(); + io.add_delegate(ethcore); + + let request = r#"{"jsonrpc": "2.0", "method": "ethcore_transactionsLimit", "params":[], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","result":1024,"id":1}"#; + + assert_eq!(io.handle_request(request), Some(response.to_owned())); +} diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index 927b8ed50..7b323b198 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -39,6 +39,7 @@ pub struct TestMinerService { gas_floor_target: RwLock, author: RwLock
, extra_data: RwLock, + limit: RwLock, } impl Default for TestMinerService { @@ -52,6 +53,7 @@ impl Default for TestMinerService { gas_floor_target: RwLock::new(U256::from(12345)), author: RwLock::new(Address::zero()), extra_data: RwLock::new(vec![1, 2, 3, 4]), + limit: RwLock::new(1024), } } } @@ -84,6 +86,14 @@ impl MinerService for TestMinerService { *self.min_gas_price.write().unwrap() = min_gas_price; } + fn set_transactions_limit(&self, limit: usize) { + *self.limit.write().unwrap() = limit; + } + + fn transactions_limit(&self) -> usize { + *self.limit.read().unwrap() + } + fn author(&self) -> Address { *self.author.read().unwrap() } diff --git a/rpc/src/v1/traits/ethcore.rs b/rpc/src/v1/traits/ethcore.rs index fb96ef035..90bb112e0 100644 --- a/rpc/src/v1/traits/ethcore.rs +++ b/rpc/src/v1/traits/ethcore.rs @@ -33,6 +33,12 @@ pub trait Ethcore: Sized + Send + Sync + 'static { /// Sets new author for mined block. fn set_author(&self, _: Params) -> Result { rpc_unimplemented!() } + /// Sets the limits for transaction queue. + fn set_transactions_limit(&self, _: Params) -> Result { rpc_unimplemented!() } + + /// Returns current transactions limit. + fn transactions_limit(&self, _: Params) -> Result { rpc_unimplemented!() } + /// Returns mining extra data. fn extra_data(&self, _: Params) -> Result { rpc_unimplemented!() } @@ -49,10 +55,12 @@ pub trait Ethcore: Sized + Send + Sync + 'static { delegate.add_method("ethcore_setGasFloorTarget", Ethcore::set_gas_floor_target); delegate.add_method("ethcore_setExtraData", Ethcore::set_extra_data); delegate.add_method("ethcore_setAuthor", Ethcore::set_author); + delegate.add_method("ethcore_setTransactionsLimit", Ethcore::set_transactions_limit); delegate.add_method("ethcore_extraData", Ethcore::extra_data); delegate.add_method("ethcore_gasFloorTarget", Ethcore::gas_floor_target); delegate.add_method("ethcore_minGasPrice", Ethcore::min_gas_price); + delegate.add_method("ethcore_transactionsLimit", Ethcore::transactions_limit); delegate } } From 10e265960012035a49fd08e7fcf59575ac74ffa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Tue, 19 Apr 2016 00:00:55 +0200 Subject: [PATCH 08/10] Fixing last_nonces updating when transactions are removed because of the limit --- miner/src/transaction_queue.rs | 79 +++++++++++++++++++++++++--------- 1 file changed, 59 insertions(+), 20 deletions(-) diff --git a/miner/src/transaction_queue.rs b/miner/src/transaction_queue.rs index 53aa0fac9..d479978be 100644 --- a/miner/src/transaction_queue.rs +++ b/miner/src/transaction_queue.rs @@ -86,7 +86,8 @@ use std::default::Default; use std::cmp::{Ordering}; -use std::collections::{HashMap, HashSet, BTreeSet}; +use std::cmp; +use std::collections::{HashMap, BTreeSet}; use util::numbers::{Uint, U256}; use util::hash::{Address, H256}; use util::table::*; @@ -204,8 +205,8 @@ impl TransactionSet { /// Remove low priority transactions if there is more then specified by given `limit`. /// /// It drops transactions from this set but also removes associated `VerifiedTransaction`. - /// Returns hashes of transactions removed because of limit. - fn enforce_limit(&mut self, by_hash: &mut HashMap) -> Option> { + /// Returns addresses and highes nonces of transactions removed because of limit. + fn enforce_limit(&mut self, by_hash: &mut HashMap) -> Option> { let len = self.by_priority.len(); if len <= self.limit { return None; @@ -221,17 +222,18 @@ impl TransactionSet { .collect() }; - Some(to_drop - .into_iter() - .map(|(sender, nonce)| { + Some(to_drop.into_iter() + .fold(HashMap::new(), |mut removed, (sender, nonce)| { let order = self.drop(&sender, &nonce) .expect("Transaction has just been found in `by_priority`; so it is in `by_address` also."); by_hash.remove(&order.hash) .expect("Hash found in `by_priorty` matches the one dropped; so it is included in `by_hash`"); - order.hash - }) - .collect::>()) + + let max = removed.get(&sender).map(|val| cmp::max(*val, nonce)).unwrap_or(nonce); + removed.insert(sender, max); + removed + })) } /// Drop transaction from this set (remove from `by_priority` and `by_address`) @@ -595,7 +597,6 @@ impl TransactionQueue { let address = tx.sender(); let nonce = tx.nonce(); - let hash = tx.hash(); let next_nonce = self.last_nonces .get(&address) @@ -606,7 +607,7 @@ impl TransactionQueue { if nonce > next_nonce { // We have a gap - put to future try!(check_too_cheap(Self::replace_transaction(tx, next_nonce, &mut self.future, &mut self.by_hash))); - try!(check_if_removed(&hash, self.future.enforce_limit(&mut self.by_hash))); + try!(check_if_removed(&address, &nonce, self.future.enforce_limit(&mut self.by_hash))); return Ok(TransactionImportResult::Future); } else if nonce < state_nonce { // Droping transaction @@ -630,16 +631,29 @@ impl TransactionQueue { } // Also enforce the limit - if let Err(e) = check_if_removed(&hash, self.current.enforce_limit(&mut self.by_hash)) { - // If current transaction was removed because of limit we need to update last_nonces also. - self.last_nonces.insert(address, nonce - U256::one()); - return Err(e); - } + let removed = self.current.enforce_limit(&mut self.by_hash); + // If some transaction were removed because of limit we need to update last_nonces also. + self.update_last_nonces(&removed); + // Trigger error if we were removed. + try!(check_if_removed(&address, &nonce, removed)); trace!(target: "miner", "status: {:?}", self.status()); Ok(TransactionImportResult::Current) } + /// Updates + fn update_last_nonces(&mut self, removed_max_nonces: &Option>) { + if let Some(ref max_nonces) = *removed_max_nonces { + for (sender, nonce) in max_nonces.iter() { + if *nonce == U256::zero() { + self.last_nonces.remove(sender); + } else { + self.last_nonces.insert(*sender, *nonce - U256::one()); + } + } + } + } + /// Replaces transaction in given set (could be `future` or `current`). /// /// If there is already transaction with same `(sender, nonce)` it will be replaced iff `gas_price` is higher. @@ -684,12 +698,15 @@ fn check_too_cheap(is_in: bool) -> Result<(), TransactionError> { } } -fn check_if_removed(hash: &H256, dropped: Option>) -> Result<(), TransactionError> { +fn check_if_removed(sender: &Address, nonce: &U256, dropped: Option>) -> Result<(), TransactionError> { match dropped { - Some(ref dropped) if dropped.contains(hash) => { - Err(TransactionError::LimitReached) + Some(ref dropped) => match dropped.get(sender) { + Some(max) if nonce <= max => { + Err(TransactionError::LimitReached) + }, + _ => Ok(()), }, - _ => Ok(()) + _ => Ok(()), } } @@ -1179,6 +1196,28 @@ mod test { assert_eq!(txq.last_nonce(&sender), Some(nonce)); } + #[test] + fn should_return_correct_nonces_when_dropped_because_of_limit() { + // given + let mut txq = TransactionQueue::with_limits(2, 2); + let tx = new_tx(); + let (tx1, tx2) = new_txs(U256::one()); + let sender = tx1.sender().unwrap(); + let nonce = tx1.nonce; + txq.add(tx1.clone(), &default_nonce).unwrap(); + txq.add(tx2.clone(), &default_nonce).unwrap(); + assert_eq!(txq.status().pending, 2); + assert_eq!(txq.last_nonce(&sender), Some(nonce + U256::one())); + + // when + let res = txq.add(tx.clone(), &default_nonce); + + // then + assert_eq!(res.unwrap(), TransactionImportResult::Current); + assert_eq!(txq.status().pending, 2); + assert_eq!(txq.last_nonce(&sender), Some(nonce)); + } + #[test] fn should_limit_future_transactions() { let mut txq = TransactionQueue::with_limits(10, 1); From 314ced57c8b9ccd9520c6dd1d65919176d56bf24 Mon Sep 17 00:00:00 2001 From: debris Date: Tue, 19 Apr 2016 16:20:04 +0200 Subject: [PATCH 09/10] fixed transaction queue merge conflict --- miner/src/transaction_queue.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/miner/src/transaction_queue.rs b/miner/src/transaction_queue.rs index ac7fa5e8e..ce09b2078 100644 --- a/miner/src/transaction_queue.rs +++ b/miner/src/transaction_queue.rs @@ -1220,7 +1220,7 @@ mod test { #[test] fn should_return_correct_nonces_when_dropped_because_of_limit() { // given - let mut txq = TransactionQueue::with_limits(2, 2); + let mut txq = TransactionQueue::with_limit(2); let tx = new_tx(); let (tx1, tx2) = new_txs(U256::one()); let sender = tx1.sender().unwrap(); From 225a5ee825bff64e9151ad0ca35ef979ffbf98fa Mon Sep 17 00:00:00 2001 From: Marek Kotewicz Date: Tue, 19 Apr 2016 19:35:32 +0200 Subject: [PATCH 10/10] removed redundant unwraps (#935) * removed redundant unwraps * fixed compilation error, removed warnings * fixed transaction queue merge conflict * fixed failing ethminer doc test --- ethcore/src/client/client.rs | 8 ++++---- ethcore/src/json_tests/chain.rs | 2 +- ethcore/src/json_tests/executive.rs | 2 -- ethcore/src/service.rs | 2 +- ethcore/src/tests/client.rs | 13 +++---------- ethcore/src/tests/helpers.rs | 4 ++-- miner/src/lib.rs | 2 +- sync/src/lib.rs | 2 +- 8 files changed, 13 insertions(+), 22 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 90ec8693f..c3e8d942e 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -122,7 +122,7 @@ const CLIENT_DB_VER_STR: &'static str = "5.3"; impl Client { /// Create a new client with given spec and DB path. - pub fn new(config: ClientConfig, spec: Spec, path: &Path, message_channel: IoChannel ) -> Result, Error> { + pub fn new(config: ClientConfig, spec: Spec, path: &Path, message_channel: IoChannel ) -> Arc { Client::::new_with_verifier(config, spec, path, message_channel) } } @@ -146,7 +146,7 @@ pub fn append_path(path: &Path, item: &str) -> String { impl Client where V: Verifier { /// Create a new client with given spec and DB path and custom verifier. - pub fn new_with_verifier(config: ClientConfig, spec: Spec, path: &Path, message_channel: IoChannel ) -> Result>, Error> { + pub fn new_with_verifier(config: ClientConfig, spec: Spec, path: &Path, message_channel: IoChannel ) -> Arc> { let path = get_db_path(path, config.pruning, spec.genesis_header().hash()); let gb = spec.genesis_block(); let chain = Arc::new(BlockChain::new(config.blockchain, &gb, &path)); @@ -163,7 +163,7 @@ impl Client where V: Verifier { let panic_handler = PanicHandler::new_in_arc(); panic_handler.forward_from(&block_queue); - Ok(Arc::new(Client { + Arc::new(Client { chain: chain, engine: engine, state_db: Mutex::new(state_db), @@ -172,7 +172,7 @@ impl Client where V: Verifier { import_lock: Mutex::new(()), panic_handler: panic_handler, verifier: PhantomData, - })) + }) } /// Flush the block import queue. diff --git a/ethcore/src/json_tests/chain.rs b/ethcore/src/json_tests/chain.rs index a1154f6a9..9413d025a 100644 --- a/ethcore/src/json_tests/chain.rs +++ b/ethcore/src/json_tests/chain.rs @@ -53,7 +53,7 @@ pub fn json_chain_test(json_data: &[u8], era: ChainEra) -> Vec { let temp = RandomTempPath::new(); { - let client = Client::new(ClientConfig::default(), spec, temp.as_path(), IoChannel::disconnected()).unwrap(); + let client = Client::new(ClientConfig::default(), spec, temp.as_path(), IoChannel::disconnected()); for b in &blockchain.blocks_rlp() { if Block::is_good(&b) { let _ = client.import_block(b.clone()); diff --git a/ethcore/src/json_tests/executive.rs b/ethcore/src/json_tests/executive.rs index 0825f3c27..427868807 100644 --- a/ethcore/src/json_tests/executive.rs +++ b/ethcore/src/json_tests/executive.rs @@ -17,11 +17,9 @@ use super::test_common::*; use state::*; use executive::*; -use spec::*; use engine::*; use evm; use evm::{Schedule, Ext, Factory, VMType, ContractCreateResult, MessageCallResult}; -use ethereum; use externalities::*; use substate::*; use tests::helpers::*; diff --git a/ethcore/src/service.rs b/ethcore/src/service.rs index 38bd873b8..2464b7cc6 100644 --- a/ethcore/src/service.rs +++ b/ethcore/src/service.rs @@ -61,7 +61,7 @@ impl ClientService { info!("Starting {}", net_service.host_info()); info!("Configured for {} using {:?} engine", spec.name, spec.engine.name()); - let client = try!(Client::new(config, spec, db_path, net_service.io().channel())); + let client = Client::new(config, spec, db_path, net_service.io().channel()); panic_handler.forward_from(client.deref()); let client_io = Arc::new(ClientIoHandler { client: client.clone() diff --git a/ethcore/src/tests/client.rs b/ethcore/src/tests/client.rs index 539a33d10..926009ac4 100644 --- a/ethcore/src/tests/client.rs +++ b/ethcore/src/tests/client.rs @@ -20,17 +20,10 @@ use tests::helpers::*; use common::*; use devtools::*; -#[test] -fn created() { - let dir = RandomTempPath::new(); - let client_result = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()); - assert!(client_result.is_ok()); -} - #[test] fn imports_from_empty() { let dir = RandomTempPath::new(); - let client = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()).unwrap(); + let client = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()); client.import_verified_blocks(&IoChannel::disconnected()); client.flush_queue(); } @@ -48,7 +41,7 @@ fn returns_state_root_basic() { #[test] fn imports_good_block() { let dir = RandomTempPath::new(); - let client = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()).unwrap(); + let client = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()); let good_block = get_good_dummy_block(); if let Err(_) = client.import_block(good_block) { panic!("error importing block being good by definition"); @@ -63,7 +56,7 @@ fn imports_good_block() { #[test] fn query_none_block() { let dir = RandomTempPath::new(); - let client = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()).unwrap(); + let client = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()); let non_existant = client.block_header(BlockId::Number(188)); assert!(non_existant.is_none()); diff --git a/ethcore/src/tests/helpers.rs b/ethcore/src/tests/helpers.rs index 152ac0ef6..56e33d76b 100644 --- a/ethcore/src/tests/helpers.rs +++ b/ethcore/src/tests/helpers.rs @@ -145,7 +145,7 @@ pub fn create_test_block_with_data(header: &Header, transactions: &[&SignedTrans pub fn generate_dummy_client(block_number: u32) -> GuardedTempResult> { let dir = RandomTempPath::new(); - let client = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()).unwrap(); + let client = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()); let test_spec = get_test_spec(); let test_engine = &test_spec.engine; let state_root = test_spec.genesis_header().state_root; @@ -211,7 +211,7 @@ pub fn push_blocks_to_client(client: &Arc, timestamp_salt: u64, starting pub fn get_test_client_with_blocks(blocks: Vec) -> GuardedTempResult> { let dir = RandomTempPath::new(); - let client = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()).unwrap(); + let client = Client::new(ClientConfig::default(), get_test_spec(), dir.as_path(), IoChannel::disconnected()); for block in &blocks { if let Err(_) = client.import_block(block.clone()) { panic!("panic importing block which is well-formed"); diff --git a/miner/src/lib.rs b/miner/src/lib.rs index bdffa1073..bcfad253e 100644 --- a/miner/src/lib.rs +++ b/miner/src/lib.rs @@ -38,7 +38,7 @@ //! fn main() { //! let mut service = NetworkService::start(NetworkConfiguration::new()).unwrap(); //! let dir = env::temp_dir(); -//! let client = Client::new(ClientConfig::default(), ethereum::new_frontier(), &dir, service.io().channel()).unwrap(); +//! let client = Client::new(ClientConfig::default(), ethereum::new_frontier(), &dir, service.io().channel()); //! //! let miner: Miner = Miner::default(); //! // get status diff --git a/sync/src/lib.rs b/sync/src/lib.rs index 6e900e97e..0b95f0b92 100644 --- a/sync/src/lib.rs +++ b/sync/src/lib.rs @@ -44,7 +44,7 @@ //! fn main() { //! let mut service = NetworkService::start(NetworkConfiguration::new()).unwrap(); //! let dir = env::temp_dir(); -//! let client = Client::new(ClientConfig::default(), ethereum::new_frontier(), &dir, service.io().channel()).unwrap(); +//! let client = Client::new(ClientConfig::default(), ethereum::new_frontier(), &dir, service.io().channel()); //! let miner = Miner::new(false); //! EthSync::register(&mut service, SyncConfig::default(), client, miner); //! }