Improve should_replace on NonceAndGasPrice (#8980)

* Additional tests for NonceAndGasPrice::should_replace.

* Fix should_replace in the distinct sender case.

* Use natural priority ordering to simplify should_replace.

miner/src/pool/mod.rs

@@ -83,20 +83,20 @@ impl PendingSettings {
 }
 
 /// Transaction priority.
-#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+#[derive(Debug, PartialEq, Eq, PartialOrd,  Clone, Copy)]
 pub(crate) enum Priority {
-	/// Local transactions (high priority)
-	///
-	/// Transactions either from a local account or
-	/// submitted over local RPC connection via `eth_sendRawTransaction`
-	Local,
+	/// Regular transactions received over the network. (no priority boost)
+	Regular,
 	/// Transactions from retracted blocks (medium priority)
 	///
 	/// When block becomes non-canonical we re-import the transactions it contains
 	/// to the queue and boost their priority.
 	Retracted,
-	/// Regular transactions received over the network. (no priority boost)
-	Regular,
+	/// Local transactions (high priority)
+	///
+	/// Transactions either from a local account or
+	/// submitted over local RPC connection via `eth_sendRawTransaction`
+	Local,
 }
 
 impl Priority {

miner/src/pool/scoring.rs

@@ -102,21 +102,16 @@ impl txpool::Scoring<VerifiedTransaction> for NonceAndGasPrice {
 	fn should_replace(&self, old: &VerifiedTransaction, new: &VerifiedTransaction) -> bool {
 		if old.sender == new.sender {
 			// prefer earliest transaction
-			if new.transaction.nonce < old.transaction.nonce {
-				return true
+			match new.transaction.nonce.cmp(&old.transaction.nonce) {
+				cmp::Ordering::Less => true,
+				cmp::Ordering::Greater => false,
+				cmp::Ordering::Equal => self.choose(old, new) == txpool::scoring::Choice::ReplaceOld,
 			}
+		} else {
+			let old_score = (old.priority(), old.transaction.gas_price);
+			let new_score = (new.priority(), new.transaction.gas_price);
+			new_score > old_score
 		}
-
-		// Always kick out non-local transactions in favour of local ones.
-		if new.priority().is_local() && !old.priority().is_local() {
-			return true;
-		}
-		// And never kick out local transactions in favour of external ones.
-		if !new.priority().is_local() && old.priority.is_local() {
-			return false;
-		}
-
-		self.choose(old, new) == txpool::scoring::Choice::ReplaceOld
 	}
 }
 
@@ -125,31 +120,117 @@ mod tests {
 	use super::*;
 
 	use std::sync::Arc;
+	use ethkey::{Random, Generator};
 	use pool::tests::tx::{Tx, TxExt};
 	use txpool::Scoring;
 
 	#[test]
-	fn should_replace_non_local_transaction_with_local_one() {
+	fn should_replace_same_sender_by_nonce() {
+		let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly);
+
+		let tx1 = Tx {
+			nonce: 1,
+			gas_price: 1,
+			..Default::default()
+		};
+		let tx2 = Tx {
+			nonce: 2,
+			gas_price: 100,
+			..Default::default()
+		};
+		let tx3 = Tx {
+			nonce: 2,
+			gas_price: 110,
+			..Default::default()
+		};
+		let tx4 = Tx {
+			nonce: 2,
+			gas_price: 130,
+			..Default::default()
+		};
+
+		let keypair = Random.generate().unwrap();
+		let txs = vec![tx1, tx2, tx3, tx4].into_iter().enumerate().map(|(i, tx)| {
+			let verified = tx.unsigned().sign(keypair.secret(), None).verified();
+			txpool::Transaction {
+				insertion_id: i as u64,
+				transaction: Arc::new(verified),
+			}
+		}).collect::<Vec<_>>();
+
+		assert!(!scoring.should_replace(&txs[0], &txs[1]));
+		assert!(scoring.should_replace(&txs[1], &txs[0]));
+
+		assert!(!scoring.should_replace(&txs[1], &txs[2]));
+		assert!(!scoring.should_replace(&txs[2], &txs[1]));
+
+		assert!(scoring.should_replace(&txs[1], &txs[3]));
+		assert!(!scoring.should_replace(&txs[3], &txs[1]));
+	}
+
+	#[test]
+	fn should_replace_different_sender_by_priority_and_gas_price() {
 		// given
 		let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly);
-		let tx1 = {
-			let tx = Tx::default().signed().verified();
+		let tx_regular_low_gas = {
+			let tx = Tx {
+				nonce: 1,
+				gas_price: 1,
+				..Default::default()
+			};
+			let verified_tx = tx.signed().verified();
 			txpool::Transaction {
 				insertion_id: 0,
-				transaction: Arc::new(tx),
+				transaction: Arc::new(verified_tx),
 			}
 		};
-		let tx2 = {
-			let mut tx = Tx::default().signed().verified();
-			tx.priority = ::pool::Priority::Local;
+		let tx_regular_high_gas = {
+			let tx = Tx {
+				nonce: 2,
+				gas_price: 10,
+				..Default::default()
+			};
+			let verified_tx = tx.signed().verified();
 			txpool::Transaction {
-				insertion_id: 0,
-				transaction: Arc::new(tx),
+				insertion_id: 1,
+				transaction: Arc::new(verified_tx),
+			}
+		};
+		let tx_local_low_gas = {
+			let tx = Tx {
+				nonce: 2,
+				gas_price: 1,
+				..Default::default()
+			};
+			let mut verified_tx = tx.signed().verified();
+			verified_tx.priority = ::pool::Priority::Local;
+			txpool::Transaction {
+				insertion_id: 2,
+				transaction: Arc::new(verified_tx),
+			}
+		};
+		let tx_local_high_gas = {
+			let tx = Tx {
+				nonce: 1,
+				gas_price: 10,
+				..Default::default()
+			};
+			let mut verified_tx = tx.signed().verified();
+			verified_tx.priority = ::pool::Priority::Local;
+			txpool::Transaction {
+				insertion_id: 3,
+				transaction: Arc::new(verified_tx),
 			}
 		};
 
-		assert!(scoring.should_replace(&tx1, &tx2));
-		assert!(!scoring.should_replace(&tx2, &tx1));
+		assert!(scoring.should_replace(&tx_regular_low_gas, &tx_regular_high_gas));
+		assert!(!scoring.should_replace(&tx_regular_high_gas, &tx_regular_low_gas));
+
+		assert!(scoring.should_replace(&tx_regular_high_gas, &tx_local_low_gas));
+		assert!(!scoring.should_replace(&tx_local_low_gas, &tx_regular_high_gas));
+
+		assert!(scoring.should_replace(&tx_local_low_gas, &tx_local_high_gas));
+		assert!(!scoring.should_replace(&tx_local_high_gas, &tx_regular_low_gas));
 	}
 
 	#[test]

miner/src/pool/tests/tx.rs

@@ -23,9 +23,9 @@ use pool::{verifier, VerifiedTransaction};
 
 #[derive(Clone)]
 pub struct Tx {
-	nonce: u64,
-	gas: u64,
-	gas_price: u64,
+	pub nonce: u64,
+	pub gas: u64,
+	pub gas_price: u64,
 }
 
 impl Default for Tx {