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] 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);