From d9673b0d6b1bf44eccab0d68d837594835788004 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 1 Apr 2019 09:48:51 +0100 Subject: [PATCH] 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 | 11 +- ethcore/private-tx/Cargo.toml | 2 +- ethcore/private-tx/src/error.rs | 5 +- .../private-tx/src/private_transactions.rs | 9 +- miner/Cargo.toml | 2 +- miner/src/pool/listener.rs | 2 +- miner/src/pool/local_transactions.rs | 2 +- miner/src/pool/mod.rs | 3 +- miner/src/pool/queue.rs | 26 +- miner/src/pool/replace.rs | 415 ++++++++++++++++++ miner/src/pool/scoring.rs | 171 -------- rpc/Cargo.toml | 2 +- 12 files changed, 448 insertions(+), 202 deletions(-) create mode 100644 miner/src/pool/replace.rs diff --git a/Cargo.lock b/Cargo.lock index 1d43c68c5..c1b7f2479 100644 --- a/Cargo.lock +++ b/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)" = "" "checksum trie-db 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)" = "3c7319e28ca295f27359d944a682f7f65b419158bf1590c92cadc0000258d788" diff --git a/ethcore/private-tx/Cargo.toml b/ethcore/private-tx/Cargo.toml index a50d704ab..2ce127a8b 100644 --- a/ethcore/private-tx/Cargo.toml +++ b/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] diff --git a/ethcore/private-tx/src/error.rs b/ethcore/private-tx/src/error.rs index 8c86bf0ca..eda08b2a5 100644 --- a/ethcore/private-tx/src/error.rs +++ b/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<::Hash>; #[derive(Debug, Display)] pub enum Error { diff --git a/ethcore/private-tx/src/private_transactions.rs b/ethcore/private-tx/src/private_transactions.rs index 0182c82dd..d0456657b 100644 --- a/ethcore/private-tx/src/private_transactions.rs +++ b/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( + pub fn add_transaction( &self, transaction: UnverifiedTransaction, validator_account: Option
, @@ -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(); - pool.import(verified)?; + let replace = pool::replace::ReplaceByScoreAndReadiness::new( + self.verification_pool.read().scoring().clone(), client); + self.verification_pool.write().import(verified, &replace)?; Ok(()) } diff --git a/miner/Cargo.toml b/miner/Cargo.toml index 02e64ad4f..f7dfd8f08 100644 --- a/miner/Cargo.toml +++ b/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" diff --git a/miner/src/pool/listener.rs b/miner/src/pool/listener.rs index fac98b0a1..67034aa52 100644 --- a/miner/src/pool/listener.rs +++ b/miner/src/pool/listener.rs @@ -92,7 +92,7 @@ impl txpool::Listener for Logger { } } - fn rejected(&mut self, _tx: &Arc, reason: &txpool::ErrorKind) { + fn rejected(&mut self, _tx: &Arc, reason: &txpool::Error) { trace!(target: "txqueue", "Rejected {}.", reason); } diff --git a/miner/src/pool/local_transactions.rs b/miner/src/pool/local_transactions.rs index c805484d1..346877d03 100644 --- a/miner/src/pool/local_transactions.rs +++ b/miner/src/pool/local_transactions.rs @@ -171,7 +171,7 @@ impl txpool::Listener for LocalTransactionsList { } } - fn rejected(&mut self, tx: &Arc, reason: &txpool::ErrorKind) { + fn rejected(&mut self, tx: &Arc, reason: &txpool::Error) { if !tx.priority().is_local() { return; } diff --git a/miner/src/pool/mod.rs b/miner/src/pool/mod.rs index 561b85f4b..40a226d9f 100644 --- a/miner/src/pool/mod.rs +++ b/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 diff --git a/miner/src/pool/queue.rs b/miner/src/pool/queue.rs index 51c46ad82..ad7c9e6f1 100644 --- a/miner/src/pool/queue.rs +++ b/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( + pub fn import( &self, client: C, transactions: Vec, @@ -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 { - use self::txpool::ErrorKind; +fn convert_error(err: txpool::Error) -> transaction::Error { + use self::txpool::Error; - match *err.kind() { - ErrorKind::AlreadyImported(..) => transaction::Error::AlreadyImported, - ErrorKind::TooCheapToEnter(..) => transaction::Error::LimitReached, - ErrorKind::TooCheapToReplace(..) => transaction::Error::TooCheapToReplace, - ref e => { - warn!(target: "txqueue", "Unknown import error: {:?}", e); - transaction::Error::NotAllowed - }, + match err { + Error::AlreadyImported(..) => transaction::Error::AlreadyImported, + Error::TooCheapToEnter(..) => transaction::Error::LimitReached, + Error::TooCheapToReplace(..) => transaction::Error::TooCheapToReplace } } diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs new file mode 100644 index 000000000..b1112dcae --- /dev/null +++ b/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 . + +//! 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 { + scoring: S, + client: C, +} + +impl ReplaceByScoreAndReadiness { + /// Create a new `ReplaceByScoreAndReadiness` + pub fn new(scoring: S, client: C) -> Self { + ReplaceByScoreAndReadiness { scoring, client } + } +} + +impl txpool::ShouldReplace for ReplaceByScoreAndReadiness +where + T: VerifiedTransaction + ScoredTransaction + PartialEq, + S: Scoring, + C: client::NonceClient, +{ + fn should_replace( + &self, + old: &ReplaceTransaction, + new: &ReplaceTransaction, + ) -> 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| { + 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, 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::>(); + + 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); + } +} diff --git a/miner/src/pool/scoring.rs b/miner/src/pool/scoring.rs index aff7ac49e..0360bec35 100644 --- a/miner/src/pool/scoring.rs +++ b/miner/src/pool/scoring.rs @@ -122,29 +122,6 @@ impl

txpool::Scoring

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::>(); - - 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() { diff --git a/rpc/Cargo.toml b/rpc/Cargo.toml index aea96f663..33559a559 100644 --- a/rpc/Cargo.toml +++ b/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"]