tx-pool: check transaction readiness before replacing (#10526)

* Update to vanilla tx pool error

* Prevent a non ready tx replacing a ready tx

* Make tests compile

* Test ready tx not replaced by future tx

* Transaction indirection

* Use StateReadiness to calculate Ready in `should_replace`

* Test existing txs from same sender are used to compute Readiness

* private-tx: Wire up ShouldReplace

* Revert "Use StateReadiness to calculate Ready in `should_replace`"

This reverts commit af9e69c8

* Make replace generic so it works with private-tx

* Rename Replace and add missing docs

* ShouldReplace no longer mutable

* tx-pool: update to transaction-pool 2.0 from crates.io

* tx-pool: generic error type alias

* Exit early for first unmatching nonce

* Fix private-tx test, use existing write lock

* Use read lock for pool scoring

Cargo.lock

@@ -951,7 +951,7 @@ dependencies = [
  "rlp 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "rustc-hex 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "trace-time 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
- "transaction-pool 1.13.3 (registry+https://github.com/rust-lang/crates.io-index)",
+ "transaction-pool 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "url 1.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
@@ -1046,7 +1046,7 @@ dependencies = [
  "serde_derive 1.0.89 (registry+https://github.com/rust-lang/crates.io-index)",
  "serde_json 1.0.39 (registry+https://github.com/rust-lang/crates.io-index)",
  "tiny-keccak 1.4.2 (registry+https://github.com/rust-lang/crates.io-index)",
- "transaction-pool 1.13.3 (registry+https://github.com/rust-lang/crates.io-index)",
+ "transaction-pool 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "trie-db 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "url 1.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
@@ -2687,7 +2687,7 @@ dependencies = [
  "tempdir 0.3.7 (registry+https://github.com/rust-lang/crates.io-index)",
  "tiny-keccak 1.4.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "tokio-timer 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
- "transaction-pool 1.13.3 (registry+https://github.com/rust-lang/crates.io-index)",
+ "transaction-pool 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "transient-hashmap 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "vm 0.1.0",
 ]
@@ -4048,10 +4048,9 @@ dependencies = [
 
 [[package]]
 name = "transaction-pool"
-version = "1.13.3"
+version = "2.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
- "error-chain 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "smallvec 0.6.5 (registry+https://github.com/rust-lang/crates.io-index)",
  "trace-time 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -4783,7 +4782,7 @@ dependencies = [
 "checksum toml 0.4.10 (registry+https://github.com/rust-lang/crates.io-index)" = "758664fc71a3a69038656bee8b6be6477d2a6c315a6b81f7081f591bffa4111f"
 "checksum toolshed 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "450441e131c7663af72e63a33c02a6a1fbaaa8601dc652ed6757813bb55aeec7"
 "checksum trace-time 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "dbe82f2f0bf1991e163e757baf044282823155dd326e70f44ce2186c3c320cc9"
-"checksum transaction-pool 1.13.3 (registry+https://github.com/rust-lang/crates.io-index)" = "e5866e5126b14358f1d7af4bf51a0be677a363799b90e655edcec8254edef1d2"
+"checksum transaction-pool 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "f8d8bd3123931aa6e49dd03bc8a2400490e14701d779458d1f1fff1f04c6f666"
 "checksum transient-hashmap 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)" = "aeb4b191d033a35edfce392a38cdcf9790b6cebcb30fa690c312c29da4dc433e"
 "checksum trezor-sys 1.0.0 (git+https://github.com/paritytech/trezor-sys)" = "<none>"
 "checksum trie-db 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)" = "3c7319e28ca295f27359d944a682f7f65b419158bf1590c92cadc0000258d788"

ethcore/private-tx/Cargo.toml

@@ -36,7 +36,7 @@ serde = "1.0"
 serde_derive = "1.0"
 serde_json = "1.0"
 tiny-keccak = "1.4"
-transaction-pool = "1.13.2"
+transaction-pool = "2.0"
 url = "1"
 
 [dev-dependencies]

ethcore/private-tx/src/error.rs

@@ -23,7 +23,10 @@ use ethcore::error::{Error as EthcoreError, ExecutionError};
 use types::transaction::Error as TransactionError;
 use ethkey::Error as KeyError;
 use ethkey::crypto::Error as CryptoError;
-use txpool::{Error as TxPoolError};
+use txpool::VerifiedTransaction;
+use private_transactions::VerifiedPrivateTransaction;
+
+type TxPoolError = txpool::Error<<VerifiedPrivateTransaction as VerifiedTransaction>::Hash>;
 
 #[derive(Debug, Display)]
 pub enum Error {

ethcore/private-tx/src/private_transactions.rs

@@ -154,7 +154,7 @@ impl Default for VerificationStore {
 
 impl VerificationStore {
 	/// Adds private transaction for verification into the store
-	pub fn add_transaction<C: pool::client::Client>(
+	pub fn add_transaction<C: pool::client::Client + pool::client::NonceClient + Clone>(
 		&self,
 		transaction: UnverifiedTransaction,
 		validator_account: Option<Address>,
@@ -164,7 +164,7 @@ impl VerificationStore {
 
 		let options = self.verification_options.clone();
 		// Use pool's verifying pipeline for original transaction's verification
-		let verifier = pool::verifier::Verifier::new(client, options, Default::default(), None);
+		let verifier = pool::verifier::Verifier::new(client.clone(), options, Default::default(), None);
 		let unverified = pool::verifier::Transaction::Unverified(transaction);
 		let verified_tx = verifier.verify_transaction(unverified)?;
 		let signed_tx: SignedTransaction = verified_tx.signed().clone();
@@ -177,8 +177,9 @@ impl VerificationStore {
 			transaction_hash: signed_hash,
 			transaction_sender: signed_sender,
 		};
-		let mut pool = self.verification_pool.write();
+		let replace = pool::replace::ReplaceByScoreAndReadiness::new(
-		pool.import(verified)?;
+			self.verification_pool.read().scoring().clone(), client);
+		self.verification_pool.write().import(verified, &replace)?;
 		Ok(())
 	}
 

miner/Cargo.toml

@@ -32,7 +32,7 @@ parking_lot = "0.7"
 price-info = { path = "./price-info", optional = true }
 rlp = { version = "0.3.0", features = ["ethereum"] }
 trace-time = "0.1"
-transaction-pool = "1.13"
+transaction-pool = "2.0"
 
 [dev-dependencies]
 env_logger = "0.5"

miner/src/pool/listener.rs

@@ -92,7 +92,7 @@ impl txpool::Listener<Transaction> for Logger {
 		}
 	}
 
-	fn rejected(&mut self, _tx: &Arc<Transaction>, reason: &txpool::ErrorKind) {
+	fn rejected<H: fmt::Debug + fmt::LowerHex>(&mut self, _tx: &Arc<Transaction>, reason: &txpool::Error<H>) {
 		trace!(target: "txqueue", "Rejected {}.", reason);
 	}
 

miner/src/pool/local_transactions.rs

@@ -171,7 +171,7 @@ impl txpool::Listener<Transaction> for LocalTransactionsList {
 		}
 	}
 
-	fn rejected(&mut self, tx: &Arc<Transaction>, reason: &txpool::ErrorKind) {
+	fn rejected<H: fmt::Debug + fmt::LowerHex>(&mut self, tx: &Arc<Transaction>, reason: &txpool::Error<H>) {
 		if !tx.priority().is_local() {
 			return;
 		}

miner/src/pool/mod.rs

@@ -27,6 +27,7 @@ mod ready;
 
 pub mod client;
 pub mod local_transactions;
+pub mod replace;
 pub mod scoring;
 pub mod verifier;
 
@@ -121,7 +122,7 @@ pub trait ScoredTransaction {
 }
 
 /// Verified transaction stored in the pool.
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub struct VerifiedTransaction {
 	transaction: transaction::PendingTransaction,
 	// TODO [ToDr] hash and sender should go directly from the transaction

miner/src/pool/queue.rs

@@ -27,7 +27,7 @@ use txpool::{self, Verifier};
 use types::transaction;
 
 use pool::{
-	self, scoring, verifier, client, ready, listener,
+	self, replace, scoring, verifier, client, ready, listener,
 	PrioritizationStrategy, PendingOrdering, PendingSettings,
 };
 use pool::local_transactions::LocalTransactionsList;
@@ -240,7 +240,7 @@ impl TransactionQueue {
 	///
 	/// Given blockchain and state access (Client)
 	/// verifies and imports transactions to the pool.
-	pub fn import<C: client::Client>(
+	pub fn import<C: client::Client + client::NonceClient + Clone>(
 		&self,
 		client: C,
 		transactions: Vec<verifier::Transaction>,
@@ -263,12 +263,14 @@ impl TransactionQueue {
 		};
 
 		let verifier = verifier::Verifier::new(
-			client,
+			client.clone(),
 			options,
 			self.insertion_id.clone(),
 			transaction_to_replace,
 		);
 
+		let mut replace = replace::ReplaceByScoreAndReadiness::new(self.pool.read().scoring().clone(), client);
+
 		let results = transactions
 			.into_iter()
 			.map(|transaction| {
@@ -286,7 +288,7 @@ impl TransactionQueue {
 				let imported = verifier
 					.verify_transaction(transaction)
 					.and_then(|verified| {
-						self.pool.write().import(verified).map_err(convert_error)
+						self.pool.write().import(verified, &mut replace).map_err(convert_error)
 					});
 
 				match imported {
@@ -579,17 +581,13 @@ impl TransactionQueue {
 	}
 }
 
-fn convert_error(err: txpool::Error) -> transaction::Error {
+fn convert_error<H: fmt::Debug + fmt::LowerHex>(err: txpool::Error<H>) -> transaction::Error {
-	use self::txpool::ErrorKind;
+	use self::txpool::Error;
 
-	match *err.kind() {
+	match err {
-		ErrorKind::AlreadyImported(..) => transaction::Error::AlreadyImported,
+		Error::AlreadyImported(..) => transaction::Error::AlreadyImported,
-		ErrorKind::TooCheapToEnter(..) => transaction::Error::LimitReached,
+		Error::TooCheapToEnter(..) => transaction::Error::LimitReached,
-		ErrorKind::TooCheapToReplace(..) => transaction::Error::TooCheapToReplace,
+		Error::TooCheapToReplace(..) => transaction::Error::TooCheapToReplace
-		ref e => {
-			warn!(target: "txqueue", "Unknown import error: {:?}", e);
-			transaction::Error::NotAllowed
-		},
 	}
 }
 

miner/src/pool/replace.rs

@@ -0,0 +1,415 @@
+// Copyright 2015-2019 Parity Technologies (UK) Ltd.
+// This file is part of Parity Ethereum.
+
+// Parity Ethereum is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Parity Ethereum is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Parity Ethereum.  If not, see <http://www.gnu.org/licenses/>.
+
+//! Replacing Transactions
+//!
+//! When queue limits are reached, a new transaction may replace one already
+//! in the pool. The decision whether to reject, replace or retain both is
+//! delegated to an implementation of `ShouldReplace`.
+//!
+//! Here we decide based on the sender, the nonce and gas price, and finally
+//! on the `Readiness` of the transactions when comparing them
+
+use std::cmp;
+
+use ethereum_types::{U256, H160 as Address};
+use txpool::{self, scoring::{Choice, Scoring}, ReplaceTransaction};
+use txpool::VerifiedTransaction;
+use super::{client, ScoredTransaction};
+
+/// Choose whether to replace based on the sender, the score and finally the
+/// `Readiness` of the transactions being compared.
+#[derive(Debug)]
+pub struct ReplaceByScoreAndReadiness<S, C> {
+	scoring: S,
+	client: C,
+}
+
+impl<S, C> ReplaceByScoreAndReadiness<S, C> {
+	/// Create a new `ReplaceByScoreAndReadiness`
+	pub fn new(scoring: S, client: C) -> Self {
+		ReplaceByScoreAndReadiness { scoring, client }
+	}
+}
+
+impl<T, S, C> txpool::ShouldReplace<T> for ReplaceByScoreAndReadiness<S, C>
+where
+	T: VerifiedTransaction<Sender = Address> + ScoredTransaction + PartialEq,
+	S: Scoring<T>,
+	C: client::NonceClient,
+{
+	fn should_replace(
+		&self,
+		old: &ReplaceTransaction<T>,
+		new: &ReplaceTransaction<T>,
+	) -> Choice {
+		let both_local = old.priority().is_local() && new.priority().is_local();
+		if old.sender() == new.sender() {
+			// prefer earliest transaction
+			match new.nonce().cmp(&old.nonce()) {
+				cmp::Ordering::Equal => self.scoring.choose(&old, &new),
+				_ if both_local => Choice::InsertNew,
+				cmp::Ordering::Less => Choice::ReplaceOld,
+				cmp::Ordering::Greater => Choice::RejectNew,
+			}
+		} else if both_local {
+			Choice::InsertNew
+		} else {
+			let old_score = (old.priority(), old.gas_price());
+			let new_score = (new.priority(), new.gas_price());
+			if new_score > old_score {
+				let state = &self.client;
+				// calculate readiness based on state nonce + pooled txs from same sender
+				let is_ready = |replace: &ReplaceTransaction<T>| {
+					let mut nonce = state.account_nonce(replace.sender());
+					if let Some(txs) = replace.pooled_by_sender {
+						for tx in txs.iter() {
+							if nonce == tx.nonce() && *tx.transaction != ***replace.transaction {
+								nonce = nonce.saturating_add(U256::from(1))
+							} else {
+								break
+							}
+						}
+					}
+					nonce == replace.nonce()
+				};
+
+				if !is_ready(new) && is_ready(old) {
+					// prevent a ready transaction being replace by a non-ready transaction
+					Choice::RejectNew
+				} else {
+					Choice::ReplaceOld
+				}
+			} else {
+				Choice::RejectNew
+			}
+		}
+	}
+}
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	use std::sync::Arc;
+	use ethkey::{Random, Generator, KeyPair};
+	use pool::tests::tx::{Tx, TxExt};
+	use pool::tests::client::TestClient;
+	use pool::scoring::*;
+	use pool::{PrioritizationStrategy, VerifiedTransaction};
+	use txpool::scoring::Choice::*;
+	use txpool::ShouldReplace;
+
+	fn local_tx_verified(tx: Tx, keypair: &KeyPair) -> VerifiedTransaction {
+		let mut verified_tx = tx.unsigned().sign(keypair.secret(), None).verified();
+		verified_tx.priority = ::pool::Priority::Local;
+		verified_tx
+	}
+
+	fn should_replace(replace: &ShouldReplace<VerifiedTransaction>, old: VerifiedTransaction, new: VerifiedTransaction) -> Choice {
+		let old_tx = txpool::Transaction { insertion_id: 0, transaction: Arc::new(old) };
+		let new_tx = txpool::Transaction { insertion_id: 0, transaction: Arc::new(new) };
+		let old = ReplaceTransaction::new(&old_tx, Default::default());
+		let new = ReplaceTransaction::new(&new_tx, Default::default());
+		replace.should_replace(&old, &new)
+	}
+
+	#[test]
+	fn should_always_accept_local_transactions_unless_same_sender_and_nonce() {
+		let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly);
+		let client = TestClient::new().with_nonce(1);
+		let replace = ReplaceByScoreAndReadiness::new(scoring, client);
+
+		// same sender txs
+		let keypair = Random.generate().unwrap();
+
+		let same_sender_tx1 = local_tx_verified(Tx {
+			nonce: 1,
+			gas_price: 1,
+			..Default::default()
+		}, &keypair);
+
+		let same_sender_tx2 = local_tx_verified(Tx {
+			nonce: 2,
+			gas_price: 100,
+			..Default::default()
+		}, &keypair);
+
+		let same_sender_tx3 = local_tx_verified(Tx {
+			nonce: 2,
+			gas_price: 200,
+			..Default::default()
+		}, &keypair);
+
+		// different sender txs
+		let sender1 = Random.generate().unwrap();
+		let different_sender_tx1 = local_tx_verified(Tx {
+			nonce: 2,
+			gas_price: 1,
+			..Default::default()
+		}, &sender1);
+
+		let sender2 = Random.generate().unwrap();
+		let different_sender_tx2 = local_tx_verified(Tx {
+			nonce: 1,
+			gas_price: 10,
+			..Default::default()
+		}, &sender2);
+
+		assert_eq!(should_replace(&replace, same_sender_tx1.clone(), same_sender_tx2.clone()), InsertNew);
+		assert_eq!(should_replace(&replace, same_sender_tx2.clone(), same_sender_tx1.clone()), InsertNew);
+
+		assert_eq!(should_replace(&replace, different_sender_tx1.clone(), different_sender_tx2.clone()), InsertNew);
+		assert_eq!(should_replace(&replace, different_sender_tx2.clone(), different_sender_tx1.clone()), InsertNew);
+
+		// txs with same sender and nonce
+		assert_eq!(should_replace(&replace, same_sender_tx2.clone(), same_sender_tx3.clone()), ReplaceOld);
+		assert_eq!(should_replace(&replace, same_sender_tx3.clone(), same_sender_tx2.clone()), RejectNew);
+	}
+
+	#[test]
+	fn should_replace_same_sender_by_nonce() {
+		let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly);
+		let client = TestClient::new().with_nonce(1);
+		let replace = ReplaceByScoreAndReadiness::new(scoring, client);
+
+		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().map(|tx| {
+			tx.unsigned().sign(keypair.secret(), None).verified()
+		}).collect::<Vec<_>>();
+
+		assert_eq!(should_replace(&replace, txs[0].clone(), txs[1].clone()), RejectNew);
+		assert_eq!(should_replace(&replace, txs[1].clone(), txs[0].clone()), ReplaceOld);
+
+		assert_eq!(should_replace(&replace, txs[1].clone(), txs[2].clone()), RejectNew);
+		assert_eq!(should_replace(&replace, txs[2].clone(), txs[1].clone()), RejectNew);
+
+		assert_eq!(should_replace(&replace, txs[1].clone(), txs[3].clone()), ReplaceOld);
+		assert_eq!(should_replace(&replace, txs[3].clone(), txs[1].clone()), RejectNew);
+	}
+
+	#[test]
+	fn should_replace_different_sender_by_priority_and_gas_price() {
+		// given
+		let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly);
+		let client = TestClient::new().with_nonce(0);
+		let replace = ReplaceByScoreAndReadiness::new(scoring, client);
+
+		let tx_regular_low_gas = {
+			let tx = Tx {
+				nonce: 1,
+				gas_price: 1,
+				..Default::default()
+			};
+			tx.signed().verified()
+		};
+		let tx_regular_high_gas = {
+			let tx = Tx {
+				nonce: 2,
+				gas_price: 10,
+				..Default::default()
+			};
+			tx.signed().verified()
+		};
+		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;
+			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;
+			verified_tx
+		};
+
+		assert_eq!(should_replace(&replace, tx_regular_low_gas.clone(), tx_regular_high_gas.clone()), ReplaceOld);
+		assert_eq!(should_replace(&replace, tx_regular_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew);
+
+		assert_eq!(should_replace(&replace, tx_regular_high_gas.clone(), tx_local_low_gas.clone()), ReplaceOld);
+		assert_eq!(should_replace(&replace, tx_local_low_gas.clone(), tx_regular_high_gas.clone()), RejectNew);
+
+		assert_eq!(should_replace(&replace, tx_local_low_gas.clone(), tx_local_high_gas.clone()), InsertNew);
+		assert_eq!(should_replace(&replace, tx_local_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew);
+	}
+
+	#[test]
+	fn should_not_replace_ready_transaction_with_future_transaction() {
+		let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly);
+		let client = TestClient::new().with_nonce(1);
+		let replace = ReplaceByScoreAndReadiness::new(scoring, client);
+
+		let tx_ready_low_score = {
+			let tx = Tx {
+				nonce: 1,
+				gas_price: 1,
+				..Default::default()
+			};
+			tx.signed().verified()
+		};
+		let tx_future_high_score = {
+			let tx = Tx {
+				nonce: 3, // future nonce
+				gas_price: 10,
+				..Default::default()
+			};
+			tx.signed().verified()
+		};
+
+		assert_eq!(should_replace(&replace, tx_ready_low_score, tx_future_high_score), RejectNew);
+	}
+
+	#[test]
+	fn should_compute_readiness_with_pooled_transactions_from_the_same_sender_as_the_existing_transaction() {
+		let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly);
+		let client = TestClient::new().with_nonce(1);
+		let replace = ReplaceByScoreAndReadiness::new(scoring, client);
+
+		let old_sender = Random.generate().unwrap();
+		let tx_old_ready_1 = {
+			let tx = Tx {
+				nonce: 1,
+				gas_price: 1,
+				..Default::default()
+			};
+			tx.unsigned().sign(&old_sender.secret(), None).verified()
+		};
+		let tx_old_ready_2 = {
+			let tx = Tx {
+				nonce: 2,
+				gas_price: 1,
+				..Default::default()
+			};
+			tx.unsigned().sign(&old_sender.secret(), None).verified()
+		};
+		let tx_old_ready_3 = {
+			let tx = Tx {
+				nonce: 3,
+				gas_price: 1,
+				..Default::default()
+			};
+			tx.unsigned().sign(&old_sender.secret(), None).verified()
+		};
+
+		let new_tx = {
+			let tx = Tx {
+				nonce: 3, // future nonce
+				gas_price: 10,
+				..Default::default()
+			};
+			tx.signed().verified()
+		};
+
+		let old_tx = txpool::Transaction { insertion_id: 0, transaction: Arc::new(tx_old_ready_3) };
+		let pooled_txs = [
+			txpool::Transaction { insertion_id: 0, transaction: Arc::new(tx_old_ready_1) },
+			txpool::Transaction { insertion_id: 0, transaction: Arc::new(tx_old_ready_2) },
+		];
+
+		let new_tx = txpool::Transaction { insertion_id: 0, transaction: Arc::new(new_tx) };
+
+		let old = ReplaceTransaction::new(&old_tx, Some(&pooled_txs));
+		let new = ReplaceTransaction::new(&new_tx, Default::default());
+
+		assert_eq!(replace.should_replace(&old, &new), RejectNew);
+	}
+
+	#[test]
+	fn should_compute_readiness_with_pooled_transactions_from_the_same_sender_as_the_new_transaction() {
+		let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly);
+		let client = TestClient::new().with_nonce(1);
+		let replace = ReplaceByScoreAndReadiness::new(scoring, client);
+
+		// current transaction is ready but has a lower gas price than the new one
+		let old_tx = {
+			let tx = Tx {
+				nonce: 1,
+				gas_price: 1,
+				..Default::default()
+			};
+			tx.signed().verified()
+		};
+
+		let new_sender = Random.generate().unwrap();
+		let tx_new_ready_1 = {
+			let tx = Tx {
+				nonce: 1,
+				gas_price: 1,
+				..Default::default()
+			};
+			tx.unsigned().sign(&new_sender.secret(), None).verified()
+		};
+		let tx_new_ready_2 = {
+			let tx = Tx {
+				nonce: 2,
+				gas_price: 1,
+				..Default::default()
+			};
+			tx.unsigned().sign(&new_sender.secret(), None).verified()
+		};
+		let tx_new_ready_3 = {
+			let tx = Tx {
+				nonce: 3,
+				gas_price: 10, // hi
+				..Default::default()
+			};
+			tx.unsigned().sign(&new_sender.secret(), None).verified()
+		};
+
+		let old_tx = txpool::Transaction { insertion_id: 0, transaction: Arc::new(old_tx) };
+
+		let new_tx = txpool::Transaction { insertion_id: 0, transaction: Arc::new(tx_new_ready_3) };
+		let pooled_txs = [
+			txpool::Transaction { insertion_id: 0, transaction: Arc::new(tx_new_ready_1) },
+			txpool::Transaction { insertion_id: 0, transaction: Arc::new(tx_new_ready_2) },
+		];
+
+		let old = ReplaceTransaction::new(&old_tx, None);
+		let new = ReplaceTransaction::new(&new_tx, Some(&pooled_txs));
+
+		assert_eq!(replace.should_replace(&old, &new), ReplaceOld);
+	}
+}

miner/src/pool/scoring.rs

@@ -122,29 +122,6 @@ impl<P> txpool::Scoring<P> for NonceAndGasPrice where P: ScoredTransaction + txp
 		}
 	}
 
-	fn should_replace(&self, old: &P, new: &P) -> scoring::Choice {
-		let both_local = old.priority().is_local() && new.priority().is_local();
-		if old.sender() == new.sender() {
-			// prefer earliest transaction
-			match new.nonce().cmp(&old.nonce()) {
-				cmp::Ordering::Equal => self.choose(old, new),
-				_ if both_local => scoring::Choice::InsertNew,
-				cmp::Ordering::Less => scoring::Choice::ReplaceOld,
-				cmp::Ordering::Greater => scoring::Choice::RejectNew,
-			}
-		} else if both_local {
-			scoring::Choice::InsertNew
-		} else {
-			let old_score = (old.priority(), old.gas_price());
-			let new_score = (new.priority(), new.gas_price());
-			if new_score > old_score {
-				scoring::Choice::ReplaceOld
-			} else {
-				scoring::Choice::RejectNew
-			}
-		}
-	}
-
 	fn should_ignore_sender_limit(&self, new: &P) -> bool {
 		new.priority().is_local()
 	}
@@ -155,156 +132,8 @@ mod tests {
 	use super::*;
 
 	use std::sync::Arc;
-	use ethkey::{Random, Generator, KeyPair};
 	use pool::tests::tx::{Tx, TxExt};
 	use txpool::Scoring;
-	use txpool::scoring::Choice::*;
-
-	fn local_tx_verified(tx: Tx, keypair: &KeyPair) -> VerifiedTransaction {
-		let mut verified_tx = tx.unsigned().sign(keypair.secret(), None).verified();
-		verified_tx.priority = ::pool::Priority::Local;
-		verified_tx
-	}
-
-	#[test]
-	fn should_always_accept_local_transactions_unless_same_sender_and_nonce() {
-		let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly);
-
-		// same sender txs
-		let keypair = Random.generate().unwrap();
-
-		let same_sender_tx1 = local_tx_verified(Tx {
-			nonce: 1,
-			gas_price: 1,
-			..Default::default()
-		}, &keypair);
-
-		let same_sender_tx2 = local_tx_verified(Tx {
-			nonce: 2,
-			gas_price: 100,
-			..Default::default()
-		}, &keypair);
-
-		let same_sender_tx3 = local_tx_verified(Tx {
-			nonce: 2,
-			gas_price: 200,
-			..Default::default()
-		}, &keypair);
-
-		// different sender txs
-		let different_sender_tx1 = local_tx_verified(Tx {
-			nonce: 2,
-			gas_price: 1,
-			..Default::default()
-		}, &Random.generate().unwrap());
-
-		let different_sender_tx2 = local_tx_verified(Tx {
-			nonce: 1,
-			gas_price: 10,
-			..Default::default()
-		}, &Random.generate().unwrap());
-
-		assert_eq!(scoring.should_replace(&same_sender_tx1, &same_sender_tx2), InsertNew);
-		assert_eq!(scoring.should_replace(&same_sender_tx2, &same_sender_tx1), InsertNew);
-
-		assert_eq!(scoring.should_replace(&different_sender_tx1, &different_sender_tx2), InsertNew);
-		assert_eq!(scoring.should_replace(&different_sender_tx2, &different_sender_tx1), InsertNew);
-
-		// txs with same sender and nonce
-		assert_eq!(scoring.should_replace(&same_sender_tx2, &same_sender_tx3), ReplaceOld);
-		assert_eq!(scoring.should_replace(&same_sender_tx3, &same_sender_tx2), RejectNew);
-	}
-
-	#[test]
-	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().map(|tx| {
-			tx.unsigned().sign(keypair.secret(), None).verified()
-		}).collect::<Vec<_>>();
-
-		assert_eq!(scoring.should_replace(&txs[0], &txs[1]), RejectNew);
-		assert_eq!(scoring.should_replace(&txs[1], &txs[0]), ReplaceOld);
-
-		assert_eq!(scoring.should_replace(&txs[1], &txs[2]), RejectNew);
-		assert_eq!(scoring.should_replace(&txs[2], &txs[1]), RejectNew);
-
-		assert_eq!(scoring.should_replace(&txs[1], &txs[3]), ReplaceOld);
-		assert_eq!(scoring.should_replace(&txs[3], &txs[1]), RejectNew);
-	}
-
-	#[test]
-	fn should_replace_different_sender_by_priority_and_gas_price() {
-		// given
-		let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly);
-		let tx_regular_low_gas = {
-			let tx = Tx {
-				nonce: 1,
-				gas_price: 1,
-				..Default::default()
-			};
-			tx.signed().verified()
-		};
-		let tx_regular_high_gas = {
-			let tx = Tx {
-				nonce: 2,
-				gas_price: 10,
-				..Default::default()
-			};
-			tx.signed().verified()
-		};
-		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;
-			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;
-			verified_tx
-		};
-
-		assert_eq!(scoring.should_replace(&tx_regular_low_gas, &tx_regular_high_gas), ReplaceOld);
-		assert_eq!(scoring.should_replace(&tx_regular_high_gas, &tx_regular_low_gas), RejectNew);
-
-		assert_eq!(scoring.should_replace(&tx_regular_high_gas, &tx_local_low_gas), ReplaceOld);
-		assert_eq!(scoring.should_replace(&tx_local_low_gas, &tx_regular_high_gas), RejectNew);
-
-		assert_eq!(scoring.should_replace(&tx_local_low_gas, &tx_local_high_gas), InsertNew);
-		assert_eq!(scoring.should_replace(&tx_local_high_gas, &tx_regular_low_gas), RejectNew);
-	}
 
 	#[test]
 	fn should_calculate_score_correctly() {

rpc/Cargo.toml

@@ -70,7 +70,7 @@ ethcore-network = { path = "../util/network" }
 fake-fetch = { path = "../util/fake-fetch" }
 macros = { path = "../util/macros" }
 pretty_assertions = "0.1"
-transaction-pool = "1.13"
+transaction-pool = "2.0"
 
 [features]
 accounts = ["ethcore-accounts"]