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

@@ -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.

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 {

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<H256, VerifiedTransaction>) {
+	/// Returns hashes of transactions removed because of limit.
+	fn enforce_limit(&mut self, by_hash: &mut HashMap<H256, VerifiedTransaction>) -> Option<HashSet<H256>> {
 		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 {
+		Some(to_drop
-			let order = self.drop(&sender, &nonce).expect("Transaction has just been found in `by_priority`; so it is in `by_address` also.");
+			.into_iter()
-			by_hash.remove(&order.hash).expect("Hash found in `by_priorty` matches the one dropped; so it is included in `by_hash`");
+			.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::<HashSet<H256>>())
 	}
 
 	/// 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);
+			try!(check_too_cheap(Self::replace_transaction(tx, next_nonce, &mut self.future, &mut self.by_hash)));
-			self.future.enforce_limit(&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<H256, VerifiedTransaction>) {
+	///
+	/// 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<H256, VerifiedTransaction>) -> 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<HashSet<H256>>) -> 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<TransactionImportResult, Error>) -> 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();
+		let res1 = txq.add(tx.clone(), &default_nonce).unwrap();
-		txq.add(tx2.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 (tx1, tx2) = new_txs_with_gas_price_diff(U256::from(4), U256::from(1));
-		let (tx3, tx4) = new_txs(U256::from(4));
+		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);