From c348508b405fc50117e90c27b26cecfc34ec453f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= <tomasz@ethcore.io>
Date: Mon, 20 Jun 2016 12:13:10 +0200
Subject: [PATCH] Fixing future order and drops because of limit errors

---
 ethcore/src/miner/transaction_queue.rs | 40 +++++++++++++++++++-------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/ethcore/src/miner/transaction_queue.rs b/ethcore/src/miner/transaction_queue.rs
index 98aa492d1..a17ff93b6 100644
--- a/ethcore/src/miner/transaction_queue.rs
+++ b/ethcore/src/miner/transaction_queue.rs
@@ -244,13 +244,12 @@ 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 addresses and highes nonces of transactions removed because of limit.
+	/// Returns addresses and lowest nonces of transactions removed because of limit.
 	fn enforce_limit(&mut self, by_hash: &mut HashMap<H256, VerifiedTransaction>) -> Option<HashMap<Address, U256>> {
 		let len = self.by_priority.len();
 		if len <= self.limit {
 			return None;
 		}
-
 		let to_drop : Vec<(Address, U256)> = {
 			self.by_priority
 				.iter()
@@ -269,8 +268,8 @@ impl TransactionSet {
 				by_hash.remove(&order.hash)
 					.expect("Hash found in `by_priorty` matches the one dropped; so it is included in `by_hash`");
 
-				let max = removed.get(&sender).map_or(nonce, |val| cmp::max(*val, nonce));
-				removed.insert(sender, max);
+				let min = removed.get(&sender).map_or(nonce, |val| cmp::min(*val, nonce));
+				removed.insert(sender, min);
 				removed
 			}))
 	}
@@ -666,8 +665,12 @@ impl TransactionQueue {
 
 		// Check height
 		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)));
+			// We have a gap - put to future.
+			// Update nonces of transactions in future (remove old transactions)
+			self.update_future(&address, state_nonce);
+			// Insert transaction (or replace old one with lower gas price)
+			try!(check_too_cheap(Self::replace_transaction(tx, state_nonce, &mut self.future, &mut self.by_hash)));
+			// Return an error if this transaction is not imported because of limit.
 			try!(check_if_removed(&address, &nonce, self.future.enforce_limit(&mut self.by_hash)));
 			return Ok(TransactionImportResult::Future);
 		} else if nonce < state_nonce {
@@ -703,9 +706,9 @@ impl TransactionQueue {
 	}
 
 	/// Updates
-	fn update_last_nonces(&mut self, removed_max_nonces: &Option<HashMap<Address, U256>>) {
-		if let Some(ref max_nonces) = *removed_max_nonces {
-			for (sender, nonce) in max_nonces.iter() {
+	fn update_last_nonces(&mut self, removed_min_nonces: &Option<HashMap<Address, U256>>) {
+		if let Some(ref min_nonces) = *removed_min_nonces {
+			for (sender, nonce) in min_nonces.iter() {
 				if *nonce == U256::zero() {
 					self.last_nonces.remove(sender);
 				} else {
@@ -762,7 +765,7 @@ fn check_too_cheap(is_in: bool) -> Result<(), TransactionError> {
 fn check_if_removed(sender: &Address, nonce: &U256, dropped: Option<HashMap<Address, U256>>) -> Result<(), TransactionError> {
 	match dropped {
 		Some(ref dropped) => match dropped.get(sender) {
-			Some(max) if nonce <= max => {
+			Some(min) if nonce >= min => {
 				Err(TransactionError::LimitReached)
 			},
 			_ => Ok(()),
@@ -1555,4 +1558,21 @@ mod test {
 		assert_eq!(txq.has_local_pending_transactions(), true);
 	}
 
+	#[test]
+	fn should_keep_right_order_in_future() {
+		// given
+		let mut txq = TransactionQueue::with_limit(1);
+		let (tx1, tx2) = new_txs(U256::from(1));
+		let prev_nonce = |a: &Address| AccountDetails { nonce: default_nonce(a).nonce - U256::one(), balance:
+			default_nonce(a).balance };
+
+		// when
+		assert_eq!(txq.add(tx2, &prev_nonce, TransactionOrigin::External).unwrap(), TransactionImportResult::Future);
+		assert_eq!(txq.add(tx1.clone(), &prev_nonce, TransactionOrigin::External).unwrap(), TransactionImportResult::Future);
+
+		// then
+		assert_eq!(txq.future.by_priority.len(), 1);
+		assert_eq!(txq.future.by_priority.iter().next().unwrap().hash, tx1.hash());
+	}
+
 }