From 80163d27b75dce5053a7e4d99489bf2dc467c1c0 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 18 Mar 2019 09:54:12 +0000 Subject: [PATCH 01/17] Update to vanilla tx pool error --- miner/src/pool/listener.rs | 2 +- miner/src/pool/local_transactions.rs | 2 +- miner/src/pool/queue.rs | 12 ++++++------ 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/miner/src/pool/listener.rs b/miner/src/pool/listener.rs index fac98b0a17c..67034aa5239 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 c805484d1ce..346877d0301 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/queue.rs b/miner/src/pool/queue.rs index 51c46ad823c..a50196d7f8f 100644 --- a/miner/src/pool/queue.rs +++ b/miner/src/pool/queue.rs @@ -579,13 +579,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, + match err { + Error::AlreadyImported(..) => transaction::Error::AlreadyImported, + Error::TooCheapToEnter(..) => transaction::Error::LimitReached, + Error::TooCheapToReplace(..) => transaction::Error::TooCheapToReplace, ref e => { warn!(target: "txqueue", "Unknown import error: {:?}", e); transaction::Error::NotAllowed From c594041edacd42a4140b4618b30710a726f66675 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Wed, 20 Mar 2019 14:53:58 +0000 Subject: [PATCH 02/17] Prevent a non ready tx replacing a ready tx --- miner/src/pool/mod.rs | 1 + miner/src/pool/queue.rs | 10 +- miner/src/pool/replace.rs | 253 ++++++++++++++++++++++++++++++++++++++ miner/src/pool/scoring.rs | 171 -------------------------- 4 files changed, 260 insertions(+), 175 deletions(-) create mode 100644 miner/src/pool/replace.rs diff --git a/miner/src/pool/mod.rs b/miner/src/pool/mod.rs index 561b85f4bc5..0771f1c42b5 100644 --- a/miner/src/pool/mod.rs +++ b/miner/src/pool/mod.rs @@ -24,6 +24,7 @@ use txpool; mod listener; mod queue; mod ready; +mod replace; pub mod client; pub mod local_transactions; diff --git a/miner/src/pool/queue.rs b/miner/src/pool/queue.rs index a50196d7f8f..0e036e6e137 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::NonceAndGasPriceAndReadiness::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 { diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs new file mode 100644 index 00000000000..d8a8568af74 --- /dev/null +++ b/miner/src/pool/replace.rs @@ -0,0 +1,253 @@ +// 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 . + +use std::cmp; +use std::collections::HashMap; + +use ethereum_types::U256; +use txpool::{self, scoring::{Choice, Scoring}, Transaction, Readiness, ReplaceTransaction}; +use txpool::{VerifiedTransaction as PoolVerifiedTransaction}; +use super::{client, ScoredTransaction, VerifiedTransaction}; + +#[derive(Debug)] +pub struct NonceAndGasPriceAndReadiness { + scoring: S, + client: C, +} + +impl NonceAndGasPriceAndReadiness { + pub fn new(scoring: S, client: C) -> Self { + NonceAndGasPriceAndReadiness { scoring, client } + } +} + +impl txpool::ShouldReplace for NonceAndGasPriceAndReadiness + where S: Scoring, C: client::NonceClient { + + fn should_replace( + &mut self, + old_replace: txpool::ReplaceTransaction, + new_replace: txpool::ReplaceTransaction, + ) -> Choice { + let old = old_replace.transaction; + let new = new_replace.transaction; + 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 state_nonce = state.account_nonce(replace.transaction.sender()); + let nonce = + replace.pooled_by_sender.map_or(state_nonce, |txs| { + txs.iter().fold(state_nonce, |nonce, tx| { + if nonce == tx.transaction.nonce() && tx.transaction != replace.transaction.transaction { + nonce.saturating_add(U256::from(1)) + } else { + nonce + } + }) + }); + nonce == replace.transaction.transaction.nonce() + }; + + if !is_ready(new_replace) && is_ready(old_replace) { + // 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 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 + } + + #[test] + fn should_always_accept_local_transactions_unless_same_sender_and_nonce() { + let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); + let mut replace = NonceAndGasPriceAndReadiness::new(scoring, TestClient::new()); + + // 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!(replace.should_replace(&same_sender_tx1, &same_sender_tx2, None), InsertNew); + assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx1, None), InsertNew); + + assert_eq!(replace.should_replace(&different_sender_tx1, &different_sender_tx2, None), InsertNew); + assert_eq!(replace.should_replace(&different_sender_tx2, &different_sender_tx1, None), InsertNew); + + // txs with same sender and nonce + assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx3, None), ReplaceOld); + assert_eq!(replace.should_replace(&same_sender_tx3, &same_sender_tx2, None), RejectNew); + } + + #[test] + fn should_replace_same_sender_by_nonce() { + let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); + let mut replace = NonceAndGasPriceAndReadiness::new(scoring, TestClient::new()); + + 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!(replace.should_replace(&txs[0], &txs[1], None), RejectNew); + assert_eq!(replace.should_replace(&txs[1], &txs[0], None), ReplaceOld); + + assert_eq!(replace.should_replace(&txs[1], &txs[2], None), RejectNew); + assert_eq!(replace.should_replace(&txs[2], &txs[1], None), RejectNew); + + assert_eq!(replace.should_replace(&txs[1], &txs[3], None), ReplaceOld); + assert_eq!(replace.should_replace(&txs[3], &txs[1], None), RejectNew); + } + + #[test] + fn should_replace_different_sender_by_priority_and_gas_price() { + // given + let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); + let mut replace = NonceAndGasPriceAndReadiness::new(scoring, TestClient::new()); + + 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!(replace.should_replace(&tx_regular_low_gas, &tx_regular_high_gas, None), ReplaceOld); + assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_regular_low_gas, None), RejectNew); + + assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_local_low_gas, None), ReplaceOld); + assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_regular_high_gas, None), RejectNew); + + assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_local_high_gas, None), InsertNew); + assert_eq!(replace.should_replace(&tx_local_high_gas, &tx_regular_low_gas, None), RejectNew); + } +} diff --git a/miner/src/pool/scoring.rs b/miner/src/pool/scoring.rs index aff7ac49ef4..0360bec3547 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() { From 00f07c3a686714b9795cba83f893925257f9e3ff Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 21 Mar 2019 15:46:31 +0000 Subject: [PATCH 03/17] Make tests compile --- miner/src/pool/replace.rs | 105 ++++++++++++++++++++++++-------------- 1 file changed, 68 insertions(+), 37 deletions(-) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index d8a8568af74..452760d4fb9 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -15,10 +15,9 @@ // along with Parity Ethereum. If not, see . use std::cmp; -use std::collections::HashMap; use ethereum_types::U256; -use txpool::{self, scoring::{Choice, Scoring}, Transaction, Readiness, ReplaceTransaction}; +use txpool::{self, scoring::{Choice, Scoring}, ReplaceTransaction}; use txpool::{VerifiedTransaction as PoolVerifiedTransaction}; use super::{client, ScoredTransaction, VerifiedTransaction}; @@ -39,16 +38,14 @@ impl txpool::ShouldReplace for NonceAndGasPriceAndRea fn should_replace( &mut self, - old_replace: txpool::ReplaceTransaction, - new_replace: txpool::ReplaceTransaction, + old: &ReplaceTransaction, + new: &ReplaceTransaction, ) -> Choice { - let old = old_replace.transaction; - let new = new_replace.transaction; 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), + cmp::Ordering::Equal => self.scoring.choose(&old, &new), _ if both_local => Choice::InsertNew, cmp::Ordering::Less => Choice::ReplaceOld, cmp::Ordering::Greater => Choice::RejectNew, @@ -61,8 +58,8 @@ impl txpool::ShouldReplace for NonceAndGasPriceAndRea 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 state_nonce = state.account_nonce(replace.transaction.sender()); + let is_ready = |replace: &ReplaceTransaction| { + let state_nonce = state.account_nonce(replace.transaction.sender()); let nonce = replace.pooled_by_sender.map_or(state_nonce, |txs| { txs.iter().fold(state_nonce, |nonce, tx| { @@ -76,7 +73,7 @@ impl txpool::ShouldReplace for NonceAndGasPriceAndRea nonce == replace.transaction.transaction.nonce() }; - if !is_ready(new_replace) && is_ready(old_replace) { + if !is_ready(new) && is_ready(old) { // prevent a ready transaction being replace by a non-ready transaction Choice::RejectNew } else { @@ -93,6 +90,7 @@ impl txpool::ShouldReplace for NonceAndGasPriceAndRea mod tests { use super::*; + use std::sync::Arc; use ethkey::{Random, Generator, KeyPair}; use pool::tests::tx::{Tx, TxExt}; use pool::tests::client::TestClient; @@ -101,10 +99,18 @@ mod tests { use txpool::scoring::Choice::*; use txpool::ShouldReplace; - fn local_tx_verified(tx: Tx, keypair: &KeyPair) -> VerifiedTransaction { + fn local_tx_verified(tx: Tx, keypair: &KeyPair) -> ReplaceTransaction { let mut verified_tx = tx.unsigned().sign(keypair.secret(), None).verified(); verified_tx.priority = ::pool::Priority::Local; - verified_tx + replace_tx(verified_tx) + } + + fn replace_tx<'a>(tx: VerifiedTransaction, ) -> ReplaceTransaction<'a, VerifiedTransaction> { + let tx = txpool::Transaction { + insertion_id: 0, + transaction: Arc::new(tx), + }; + ReplaceTransaction::new(tx, Default::default()) } #[test] @@ -134,27 +140,29 @@ mod tests { }, &keypair); // different sender txs + let sender1 = Random.generate().unwrap(); let different_sender_tx1 = local_tx_verified(Tx { nonce: 2, gas_price: 1, ..Default::default() - }, &Random.generate().unwrap()); + }, &sender1); + let sender2 = Random.generate().unwrap(); let different_sender_tx2 = local_tx_verified(Tx { nonce: 1, gas_price: 10, ..Default::default() - }, &Random.generate().unwrap()); + }, &sender2); - assert_eq!(replace.should_replace(&same_sender_tx1, &same_sender_tx2, None), InsertNew); - assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx1, None), InsertNew); + assert_eq!(replace.should_replace(&same_sender_tx1, &same_sender_tx2), InsertNew); + assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx1), InsertNew); - assert_eq!(replace.should_replace(&different_sender_tx1, &different_sender_tx2, None), InsertNew); - assert_eq!(replace.should_replace(&different_sender_tx2, &different_sender_tx1, None), InsertNew); + assert_eq!(replace.should_replace(&different_sender_tx1, &different_sender_tx2), InsertNew); + assert_eq!(replace.should_replace(&different_sender_tx2, &different_sender_tx1), InsertNew); // txs with same sender and nonce - assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx3, None), ReplaceOld); - assert_eq!(replace.should_replace(&same_sender_tx3, &same_sender_tx2, None), RejectNew); + assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx3), ReplaceOld); + assert_eq!(replace.should_replace(&same_sender_tx3, &same_sender_tx2), RejectNew); } #[test] @@ -185,17 +193,17 @@ mod tests { let keypair = Random.generate().unwrap(); let txs = vec![tx1, tx2, tx3, tx4].into_iter().map(|tx| { - tx.unsigned().sign(keypair.secret(), None).verified() + replace_tx(tx.unsigned().sign(keypair.secret(), None).verified()) }).collect::>(); - assert_eq!(replace.should_replace(&txs[0], &txs[1], None), RejectNew); - assert_eq!(replace.should_replace(&txs[1], &txs[0], None), ReplaceOld); + assert_eq!(replace.should_replace(&txs[0], &txs[1]), RejectNew); + assert_eq!(replace.should_replace(&txs[1], &txs[0]), ReplaceOld); - assert_eq!(replace.should_replace(&txs[1], &txs[2], None), RejectNew); - assert_eq!(replace.should_replace(&txs[2], &txs[1], None), RejectNew); + assert_eq!(replace.should_replace(&txs[1], &txs[2]), RejectNew); + assert_eq!(replace.should_replace(&txs[2], &txs[1]), RejectNew); - assert_eq!(replace.should_replace(&txs[1], &txs[3], None), ReplaceOld); - assert_eq!(replace.should_replace(&txs[3], &txs[1], None), RejectNew); + assert_eq!(replace.should_replace(&txs[1], &txs[3]), ReplaceOld); + assert_eq!(replace.should_replace(&txs[3], &txs[1]), RejectNew); } #[test] @@ -210,7 +218,7 @@ mod tests { gas_price: 1, ..Default::default() }; - tx.signed().verified() + replace_tx(tx.signed().verified()) }; let tx_regular_high_gas = { let tx = Tx { @@ -218,7 +226,7 @@ mod tests { gas_price: 10, ..Default::default() }; - tx.signed().verified() + replace_tx(tx.signed().verified()) }; let tx_local_low_gas = { let tx = Tx { @@ -228,7 +236,7 @@ mod tests { }; let mut verified_tx = tx.signed().verified(); verified_tx.priority = ::pool::Priority::Local; - verified_tx + replace_tx(verified_tx) }; let tx_local_high_gas = { let tx = Tx { @@ -238,16 +246,39 @@ mod tests { }; let mut verified_tx = tx.signed().verified(); verified_tx.priority = ::pool::Priority::Local; - verified_tx + replace_tx(verified_tx) }; - assert_eq!(replace.should_replace(&tx_regular_low_gas, &tx_regular_high_gas, None), ReplaceOld); - assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_regular_low_gas, None), RejectNew); + assert_eq!(replace.should_replace(&tx_regular_low_gas, &tx_regular_high_gas), ReplaceOld); + assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_regular_low_gas), RejectNew); - assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_local_low_gas, None), ReplaceOld); - assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_regular_high_gas, None), RejectNew); + assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_local_low_gas), ReplaceOld); + assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_regular_high_gas), RejectNew); - assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_local_high_gas, None), InsertNew); - assert_eq!(replace.should_replace(&tx_local_high_gas, &tx_regular_low_gas, None), RejectNew); + assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_local_high_gas), InsertNew); + assert_eq!(replace.should_replace(&tx_local_high_gas, &tx_regular_low_gas), RejectNew); } + +// #[test] +// fn should_not_replace_ready_transaction_with_future_transaction() { +// let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); +// 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(&scoring, &tx_ready_low_score, &tx_future_high_score), RejectNew); +// } } From 63d43db0f4ff9f560b0a32ffa9625bef841ce06b Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 21 Mar 2019 16:27:58 +0000 Subject: [PATCH 04/17] Test ready tx not replaced by future tx --- miner/src/pool/replace.rs | 57 ++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index 452760d4fb9..d7ff57e23d9 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -70,6 +70,7 @@ impl txpool::ShouldReplace for NonceAndGasPriceAndRea } }) }); + println!("{} {}", nonce, replace.transaction.transaction.nonce()); nonce == replace.transaction.transaction.nonce() }; @@ -116,7 +117,8 @@ mod tests { #[test] fn should_always_accept_local_transactions_unless_same_sender_and_nonce() { let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); - let mut replace = NonceAndGasPriceAndReadiness::new(scoring, TestClient::new()); + let client = TestClient::new().with_nonce(1); + let mut replace = NonceAndGasPriceAndReadiness::new(scoring, client); // same sender txs let keypair = Random.generate().unwrap(); @@ -168,7 +170,8 @@ mod tests { #[test] fn should_replace_same_sender_by_nonce() { let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); - let mut replace = NonceAndGasPriceAndReadiness::new(scoring, TestClient::new()); + let client = TestClient::new().with_nonce(1); + let mut replace = NonceAndGasPriceAndReadiness::new(scoring, client); let tx1 = Tx { nonce: 1, @@ -210,7 +213,8 @@ mod tests { fn should_replace_different_sender_by_priority_and_gas_price() { // given let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); - let mut replace = NonceAndGasPriceAndReadiness::new(scoring, TestClient::new()); + let client = TestClient::new().with_nonce(0); + let mut replace = NonceAndGasPriceAndReadiness::new(scoring, client); let tx_regular_low_gas = { let tx = Tx { @@ -259,26 +263,29 @@ mod tests { assert_eq!(replace.should_replace(&tx_local_high_gas, &tx_regular_low_gas), RejectNew); } -// #[test] -// fn should_not_replace_ready_transaction_with_future_transaction() { -// let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); -// 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(&scoring, &tx_ready_low_score, &tx_future_high_score), RejectNew); -// } + #[test] + fn should_not_replace_ready_transaction_with_future_transaction() { + let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); + let client = TestClient::new().with_nonce(1); + let mut replace = NonceAndGasPriceAndReadiness::new(scoring, client); + + let tx_ready_low_score = { + let tx = Tx { + nonce: 1, + gas_price: 1, + ..Default::default() + }; + replace_tx(tx.signed().verified()) + }; + let tx_future_high_score = { + let tx = Tx { + nonce: 3, // future nonce + gas_price: 10, + ..Default::default() + }; + replace_tx(tx.signed().verified()) + }; + + assert_eq!(replace.should_replace(&tx_ready_low_score, &tx_future_high_score), RejectNew); + } } From f47efac06ebd19740af9462f56fff0dc3f28ff25 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 21 Mar 2019 16:37:27 +0000 Subject: [PATCH 05/17] Transaction indirection --- miner/src/pool/replace.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index d7ff57e23d9..63818efca05 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -59,19 +59,18 @@ impl txpool::ShouldReplace for NonceAndGasPriceAndRea let state = &self.client; // calculate readiness based on state nonce + pooled txs from same sender let is_ready = |replace: &ReplaceTransaction| { - let state_nonce = state.account_nonce(replace.transaction.sender()); + let state_nonce = state.account_nonce(replace.sender()); let nonce = replace.pooled_by_sender.map_or(state_nonce, |txs| { txs.iter().fold(state_nonce, |nonce, tx| { - if nonce == tx.transaction.nonce() && tx.transaction != replace.transaction.transaction { + if nonce == tx.nonce() && tx.transaction != *replace.transaction { nonce.saturating_add(U256::from(1)) } else { nonce } }) }); - println!("{} {}", nonce, replace.transaction.transaction.nonce()); - nonce == replace.transaction.transaction.nonce() + nonce == replace.nonce() }; if !is_ready(new) && is_ready(old) { From af9e69c89e83e35cc42fa868b3ae5622fb663a2c Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 25 Mar 2019 12:40:06 +0000 Subject: [PATCH 06/17] Use StateReadiness to calculate Ready in `should_replace` --- Cargo.lock | 18 ++++++- miner/Cargo.toml | 2 +- miner/src/pool/mod.rs | 2 +- miner/src/pool/queue.rs | 14 ++--- miner/src/pool/ready.rs | 27 +++++----- miner/src/pool/replace.rs | 105 +++++++++++++++++++------------------- 6 files changed, 91 insertions(+), 77 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b6c0b3ef61f..404d73f2fd6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -939,7 +939,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", "url 1.7.1 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -4028,6 +4028,13 @@ dependencies = [ "fxhash 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "trace-time" +version = "0.1.1" +dependencies = [ + "log 0.4.5 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "trace-time" version = "0.1.1" @@ -4047,6 +4054,15 @@ dependencies = [ "trace-time 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "transaction-pool" +version = "2.0.0" +dependencies = [ + "log 0.4.5 (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", +] + [[package]] name = "transient-hashmap" version = "0.4.1" diff --git a/miner/Cargo.toml b/miner/Cargo.toml index 02e64ad4f8d..f7dfd8f0836 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/mod.rs b/miner/src/pool/mod.rs index 0771f1c42b5..f1cb1648fad 100644 --- a/miner/src/pool/mod.rs +++ b/miner/src/pool/mod.rs @@ -122,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 0e036e6e137..4ffe2db13b8 100644 --- a/miner/src/pool/queue.rs +++ b/miner/src/pool/queue.rs @@ -362,7 +362,7 @@ impl TransactionQueue { // In case we don't have a cached set, but we don't care about order // just return the unordered set. if let PendingOrdering::Unordered = ordering { - let ready = Self::ready(client, block_number, current_timestamp, nonce_cap); + let ready = Self::ready(&client, block_number, current_timestamp, nonce_cap); return self.pool.read().unordered_pending(ready).take(max_len).collect(); } @@ -404,12 +404,12 @@ impl TransactionQueue { { debug!(target: "txqueue", "Re-computing pending set for block: {}", block_number); trace_time!("pool::collect_pending"); - let ready = Self::ready(client, block_number, current_timestamp, nonce_cap); + let ready = Self::ready(&client, block_number, current_timestamp, nonce_cap); collect(self.pool.read().pending(ready)) } fn ready( - client: C, + client: &C, block_number: u64, current_timestamp: u64, nonce_cap: Option, @@ -454,7 +454,7 @@ impl TransactionQueue { }; for chunk in senders.chunks(CULL_SENDERS_CHUNK) { trace_time!("pool::cull::chunk"); - let state_readiness = ready::State::new(client.clone(), stale_id, nonce_cap); + let state_readiness = ready::State::new(&client, stale_id, nonce_cap); removed += self.pool.write().cull(Some(chunk), state_readiness); } debug!(target: "txqueue", "Removed {} stalled transactions. {}", removed, self.status()); @@ -472,7 +472,7 @@ impl TransactionQueue { // Also we ignore stale transactions in the queue. let stale_id = None; - let state_readiness = ready::State::new(client, stale_id, nonce_cap); + let state_readiness = ready::State::new(&client, stale_id, nonce_cap); self.pool.read().pending_from_sender(state_readiness, address) .last() @@ -588,10 +588,6 @@ fn convert_error(err: txpool::Error) -> transa Error::AlreadyImported(..) => transaction::Error::AlreadyImported, Error::TooCheapToEnter(..) => transaction::Error::LimitReached, Error::TooCheapToReplace(..) => transaction::Error::TooCheapToReplace, - ref e => { - warn!(target: "txqueue", "Unknown import error: {:?}", e); - transaction::Error::NotAllowed - }, } } diff --git a/miner/src/pool/ready.rs b/miner/src/pool/ready.rs index 3accba13903..21212c27089 100644 --- a/miner/src/pool/ready.rs +++ b/miner/src/pool/ready.rs @@ -50,17 +50,17 @@ use super::VerifiedTransaction; /// Checks readiness of transactions by comparing the nonce to state nonce. #[derive(Debug)] -pub struct State { +pub struct State<'a, C> { nonces: HashMap, - state: C, + state: &'a C, max_nonce: Option, stale_id: Option, } -impl State { +impl<'a, C> State<'a, C> { /// Create new State checker, given client interface. pub fn new( - state: C, + state: &'a C, stale_id: Option, max_nonce: Option, ) -> Self { @@ -73,7 +73,7 @@ impl State { } } -impl txpool::Ready for State { +impl<'a, C: NonceClient> txpool::Ready for State<'a, C> { fn is_ready(&mut self, tx: &VerifiedTransaction) -> txpool::Readiness { // Check max nonce match self.max_nonce { @@ -84,7 +84,7 @@ impl txpool::Ready for State { } let sender = tx.sender(); - let state = &self.state; + let state = self.state; let state_nonce = || state.account_nonce(sender); let nonce = self.nonces.entry(*sender).or_insert_with(state_nonce); match tx.transaction.nonce.cmp(nonce) { @@ -180,10 +180,11 @@ mod tests { let (tx1, tx2, tx3) = (tx1.verified(), tx2.verified(), tx3.verified()); // when - assert_eq!(State::new(TestClient::new(), None, None).is_ready(&tx3), txpool::Readiness::Future); - assert_eq!(State::new(TestClient::new(), None, None).is_ready(&tx2), txpool::Readiness::Future); + assert_eq!(State::new(&TestClient::new(), None, None).is_ready(&tx3), txpool::Readiness::Future); + assert_eq!(State::new(&TestClient::new(), None, None).is_ready(&tx2), txpool::Readiness::Future); - let mut ready = State::new(TestClient::new(), None, None); + let client = TestClient::new(); + let mut ready = State::new(&client, None, None); // then assert_eq!(ready.is_ready(&tx1), txpool::Readiness::Ready); @@ -197,8 +198,8 @@ mod tests { let tx = Tx::default().signed().verified(); // when - let res1 = State::new(TestClient::new(), None, Some(10.into())).is_ready(&tx); - let res2 = State::new(TestClient::new(), None, Some(124.into())).is_ready(&tx); + let res1 = State::new(&TestClient::new(), None, Some(10.into())).is_ready(&tx); + let res2 = State::new(&TestClient::new(), None, Some(124.into())).is_ready(&tx); // then assert_eq!(res1, txpool::Readiness::Future); @@ -211,7 +212,7 @@ mod tests { let tx = Tx::default().signed().verified(); // when - let res = State::new(TestClient::new().with_nonce(125), None, None).is_ready(&tx); + let res = State::new(&TestClient::new().with_nonce(125), None, None).is_ready(&tx); // then assert_eq!(res, txpool::Readiness::Stale); @@ -223,7 +224,7 @@ mod tests { let (_, tx) = Tx::default().signed_pair().verified(); // when - let res = State::new(TestClient::new(), Some(1), None).is_ready(&tx); + let res = State::new(&TestClient::new(), Some(1), None).is_ready(&tx); // then assert_eq!(res, txpool::Readiness::Stale); diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index 63818efca05..812b835c37c 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -16,10 +16,9 @@ use std::cmp; -use ethereum_types::U256; -use txpool::{self, scoring::{Choice, Scoring}, ReplaceTransaction}; +use txpool::{self, scoring::{Choice, Scoring}, Readiness, Ready, ReplaceTransaction}; use txpool::{VerifiedTransaction as PoolVerifiedTransaction}; -use super::{client, ScoredTransaction, VerifiedTransaction}; +use super::{client, ready::State, ScoredTransaction, VerifiedTransaction}; #[derive(Debug)] pub struct NonceAndGasPriceAndReadiness { @@ -34,7 +33,9 @@ impl NonceAndGasPriceAndReadiness { } impl txpool::ShouldReplace for NonceAndGasPriceAndReadiness - where S: Scoring, C: client::NonceClient { +where + S: Scoring, + C: client::NonceClient { fn should_replace( &mut self, @@ -56,21 +57,21 @@ impl txpool::ShouldReplace for NonceAndGasPriceAndRea 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; + let client = &self.client; + let mut state_readiness = State::new(client, None, None); + // calculate readiness based on state nonce + pooled txs from same sender - let is_ready = |replace: &ReplaceTransaction| { - let state_nonce = state.account_nonce(replace.sender()); - let nonce = - replace.pooled_by_sender.map_or(state_nonce, |txs| { - txs.iter().fold(state_nonce, |nonce, tx| { - if nonce == tx.nonce() && tx.transaction != *replace.transaction { - nonce.saturating_add(U256::from(1)) - } else { - nonce - } - }) - }); - nonce == replace.nonce() + let mut is_ready = |replace: &ReplaceTransaction| { + if let Some(txs) = replace.pooled_by_sender { + // replay the readiness of currently pooled txs from the same sender + for tx in txs { + if *tx.transaction != ***replace.transaction { + state_readiness.is_ready(tx); + } + } + }; + let readiness = state_readiness.is_ready(replace.transaction); + readiness == Readiness::Ready }; if !is_ready(new) && is_ready(old) { @@ -99,18 +100,18 @@ mod tests { use txpool::scoring::Choice::*; use txpool::ShouldReplace; - fn local_tx_verified(tx: Tx, keypair: &KeyPair) -> ReplaceTransaction { + 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; - replace_tx(verified_tx) + verified_tx } - fn replace_tx<'a>(tx: VerifiedTransaction, ) -> ReplaceTransaction<'a, VerifiedTransaction> { - let tx = txpool::Transaction { - insertion_id: 0, - transaction: Arc::new(tx), - }; - ReplaceTransaction::new(tx, Default::default()) + fn should_replace(replace: &mut 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] @@ -155,15 +156,15 @@ mod tests { ..Default::default() }, &sender2); - assert_eq!(replace.should_replace(&same_sender_tx1, &same_sender_tx2), InsertNew); - assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx1), InsertNew); + assert_eq!(should_replace(&mut replace, same_sender_tx1.clone(), same_sender_tx2.clone()), InsertNew); + assert_eq!(should_replace(&mut replace, same_sender_tx2.clone(), same_sender_tx1.clone()), InsertNew); - assert_eq!(replace.should_replace(&different_sender_tx1, &different_sender_tx2), InsertNew); - assert_eq!(replace.should_replace(&different_sender_tx2, &different_sender_tx1), InsertNew); + assert_eq!(should_replace(&mut replace, different_sender_tx1.clone(), different_sender_tx2.clone()), InsertNew); + assert_eq!(should_replace(&mut replace, different_sender_tx2.clone(), different_sender_tx1.clone()), InsertNew); // txs with same sender and nonce - assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx3), ReplaceOld); - assert_eq!(replace.should_replace(&same_sender_tx3, &same_sender_tx2), RejectNew); + assert_eq!(should_replace(&mut replace, same_sender_tx2.clone(), same_sender_tx3.clone()), ReplaceOld); + assert_eq!(should_replace(&mut replace, same_sender_tx3.clone(), same_sender_tx2.clone()), RejectNew); } #[test] @@ -195,17 +196,17 @@ mod tests { let keypair = Random.generate().unwrap(); let txs = vec![tx1, tx2, tx3, tx4].into_iter().map(|tx| { - replace_tx(tx.unsigned().sign(keypair.secret(), None).verified()) + tx.unsigned().sign(keypair.secret(), None).verified() }).collect::>(); - assert_eq!(replace.should_replace(&txs[0], &txs[1]), RejectNew); - assert_eq!(replace.should_replace(&txs[1], &txs[0]), ReplaceOld); + assert_eq!(should_replace(&mut replace, txs[0].clone(), txs[1].clone()), RejectNew); + assert_eq!(should_replace(&mut replace, txs[1].clone(), txs[0].clone()), ReplaceOld); - assert_eq!(replace.should_replace(&txs[1], &txs[2]), RejectNew); - assert_eq!(replace.should_replace(&txs[2], &txs[1]), RejectNew); + assert_eq!(should_replace(&mut replace, txs[1].clone(), txs[2].clone()), RejectNew); + assert_eq!(should_replace(&mut replace, txs[2].clone(), txs[1].clone()), RejectNew); - assert_eq!(replace.should_replace(&txs[1], &txs[3]), ReplaceOld); - assert_eq!(replace.should_replace(&txs[3], &txs[1]), RejectNew); + assert_eq!(should_replace(&mut replace, txs[1].clone(), txs[3].clone()), ReplaceOld); + assert_eq!(should_replace(&mut replace, txs[3].clone(), txs[1].clone()), RejectNew); } #[test] @@ -221,7 +222,7 @@ mod tests { gas_price: 1, ..Default::default() }; - replace_tx(tx.signed().verified()) + tx.signed().verified() }; let tx_regular_high_gas = { let tx = Tx { @@ -229,7 +230,7 @@ mod tests { gas_price: 10, ..Default::default() }; - replace_tx(tx.signed().verified()) + tx.signed().verified() }; let tx_local_low_gas = { let tx = Tx { @@ -239,7 +240,7 @@ mod tests { }; let mut verified_tx = tx.signed().verified(); verified_tx.priority = ::pool::Priority::Local; - replace_tx(verified_tx) + verified_tx }; let tx_local_high_gas = { let tx = Tx { @@ -249,17 +250,17 @@ mod tests { }; let mut verified_tx = tx.signed().verified(); verified_tx.priority = ::pool::Priority::Local; - replace_tx(verified_tx) + verified_tx }; - assert_eq!(replace.should_replace(&tx_regular_low_gas, &tx_regular_high_gas), ReplaceOld); - assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_regular_low_gas), RejectNew); + assert_eq!(should_replace(&mut replace, tx_regular_low_gas.clone(), tx_regular_high_gas.clone()), ReplaceOld); + assert_eq!(should_replace(&mut replace, tx_regular_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew); - assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_local_low_gas), ReplaceOld); - assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_regular_high_gas), RejectNew); + assert_eq!(should_replace(&mut replace, tx_regular_high_gas.clone(), tx_local_low_gas.clone()), ReplaceOld); + assert_eq!(should_replace(&mut replace, tx_local_low_gas.clone(), tx_regular_high_gas.clone()), RejectNew); - assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_local_high_gas), InsertNew); - assert_eq!(replace.should_replace(&tx_local_high_gas, &tx_regular_low_gas), RejectNew); + assert_eq!(should_replace(&mut replace, tx_local_low_gas.clone(), tx_local_high_gas.clone()), InsertNew); + assert_eq!(should_replace(&mut replace, tx_local_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew); } #[test] @@ -274,7 +275,7 @@ mod tests { gas_price: 1, ..Default::default() }; - replace_tx(tx.signed().verified()) + tx.signed().verified() }; let tx_future_high_score = { let tx = Tx { @@ -282,9 +283,9 @@ mod tests { gas_price: 10, ..Default::default() }; - replace_tx(tx.signed().verified()) + tx.signed().verified() }; - assert_eq!(replace.should_replace(&tx_ready_low_score, &tx_future_high_score), RejectNew); + assert_eq!(should_replace(&mut replace, tx_ready_low_score, tx_future_high_score), RejectNew); } } From ebe5d8ecd4726d6f63fa3a83e6716fac0360eeb7 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 25 Mar 2019 15:31:03 +0000 Subject: [PATCH 07/17] Test existing txs from same sender are used to compute Readiness --- miner/src/pool/replace.rs | 111 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index 812b835c37c..ae3f604f330 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -288,4 +288,115 @@ mod tests { assert_eq!(should_replace(&mut 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 mut replace = NonceAndGasPriceAndReadiness::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 mut replace = NonceAndGasPriceAndReadiness::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); + } } From a86bee0eba585f336a2868c3995ec21d0b1845c4 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 25 Mar 2019 18:09:35 +0000 Subject: [PATCH 08/17] private-tx: Wire up ShouldReplace --- ethcore/private-tx/Cargo.toml | 2 +- ethcore/private-tx/src/error.rs | 4 +++- ethcore/private-tx/src/private_transactions.rs | 8 +++++--- miner/src/pool/mod.rs | 2 +- rpc/Cargo.toml | 2 +- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/ethcore/private-tx/Cargo.toml b/ethcore/private-tx/Cargo.toml index fdf6e52baab..257095bd838 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 60eb17987ab..a8d9c9bdb0d 100644 --- a/ethcore/private-tx/src/error.rs +++ b/ethcore/private-tx/src/error.rs @@ -25,7 +25,9 @@ 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; + +type TxPoolError = txpool::Error<::Hash>; error_chain! { foreign_links { diff --git a/ethcore/private-tx/src/private_transactions.rs b/ethcore/private-tx/src/private_transactions.rs index 21989025b15..eb588f7af51 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(); @@ -178,7 +178,9 @@ impl VerificationStore { transaction_sender: signed_sender, }; let mut pool = self.verification_pool.write(); - pool.import(verified)?; + let mut replace = + pool::replace::NonceAndGasPriceAndReadiness::new(self.verification_pool.read().scoring().clone(), client); + pool.import(verified, &mut replace)?; Ok(()) } diff --git a/miner/src/pool/mod.rs b/miner/src/pool/mod.rs index f1cb1648fad..40a226d9fcd 100644 --- a/miner/src/pool/mod.rs +++ b/miner/src/pool/mod.rs @@ -24,10 +24,10 @@ use txpool; mod listener; mod queue; mod ready; -mod replace; pub mod client; pub mod local_transactions; +pub mod replace; pub mod scoring; pub mod verifier; diff --git a/rpc/Cargo.toml b/rpc/Cargo.toml index c6c59dc15a4..fd111116e47 100644 --- a/rpc/Cargo.toml +++ b/rpc/Cargo.toml @@ -72,7 +72,7 @@ fake-fetch = { path = "../util/fake-fetch" } kvdb-memorydb = "0.1" macros = { path = "../util/macros" } pretty_assertions = "0.1" -transaction-pool = "1.13" +transaction-pool = "2.0" [features] accounts = ["ethcore-accounts"] From 8797df43e5258ff9fdc01a60047609283b90f0c1 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Tue, 26 Mar 2019 10:55:29 +0000 Subject: [PATCH 09/17] Revert "Use StateReadiness to calculate Ready in `should_replace`" This reverts commit af9e69c8 --- miner/src/pool/mod.rs | 2 +- miner/src/pool/queue.rs | 14 +++-- miner/src/pool/ready.rs | 27 +++++----- miner/src/pool/replace.rs | 105 +++++++++++++++++++------------------- 4 files changed, 75 insertions(+), 73 deletions(-) diff --git a/miner/src/pool/mod.rs b/miner/src/pool/mod.rs index 40a226d9fcd..4e9f25d1801 100644 --- a/miner/src/pool/mod.rs +++ b/miner/src/pool/mod.rs @@ -122,7 +122,7 @@ pub trait ScoredTransaction { } /// Verified transaction stored in the pool. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, 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 4ffe2db13b8..0e036e6e137 100644 --- a/miner/src/pool/queue.rs +++ b/miner/src/pool/queue.rs @@ -362,7 +362,7 @@ impl TransactionQueue { // In case we don't have a cached set, but we don't care about order // just return the unordered set. if let PendingOrdering::Unordered = ordering { - let ready = Self::ready(&client, block_number, current_timestamp, nonce_cap); + let ready = Self::ready(client, block_number, current_timestamp, nonce_cap); return self.pool.read().unordered_pending(ready).take(max_len).collect(); } @@ -404,12 +404,12 @@ impl TransactionQueue { { debug!(target: "txqueue", "Re-computing pending set for block: {}", block_number); trace_time!("pool::collect_pending"); - let ready = Self::ready(&client, block_number, current_timestamp, nonce_cap); + let ready = Self::ready(client, block_number, current_timestamp, nonce_cap); collect(self.pool.read().pending(ready)) } fn ready( - client: &C, + client: C, block_number: u64, current_timestamp: u64, nonce_cap: Option, @@ -454,7 +454,7 @@ impl TransactionQueue { }; for chunk in senders.chunks(CULL_SENDERS_CHUNK) { trace_time!("pool::cull::chunk"); - let state_readiness = ready::State::new(&client, stale_id, nonce_cap); + let state_readiness = ready::State::new(client.clone(), stale_id, nonce_cap); removed += self.pool.write().cull(Some(chunk), state_readiness); } debug!(target: "txqueue", "Removed {} stalled transactions. {}", removed, self.status()); @@ -472,7 +472,7 @@ impl TransactionQueue { // Also we ignore stale transactions in the queue. let stale_id = None; - let state_readiness = ready::State::new(&client, stale_id, nonce_cap); + let state_readiness = ready::State::new(client, stale_id, nonce_cap); self.pool.read().pending_from_sender(state_readiness, address) .last() @@ -588,6 +588,10 @@ fn convert_error(err: txpool::Error) -> transa Error::AlreadyImported(..) => transaction::Error::AlreadyImported, Error::TooCheapToEnter(..) => transaction::Error::LimitReached, Error::TooCheapToReplace(..) => transaction::Error::TooCheapToReplace, + ref e => { + warn!(target: "txqueue", "Unknown import error: {:?}", e); + transaction::Error::NotAllowed + }, } } diff --git a/miner/src/pool/ready.rs b/miner/src/pool/ready.rs index 21212c27089..3accba13903 100644 --- a/miner/src/pool/ready.rs +++ b/miner/src/pool/ready.rs @@ -50,17 +50,17 @@ use super::VerifiedTransaction; /// Checks readiness of transactions by comparing the nonce to state nonce. #[derive(Debug)] -pub struct State<'a, C> { +pub struct State { nonces: HashMap, - state: &'a C, + state: C, max_nonce: Option, stale_id: Option, } -impl<'a, C> State<'a, C> { +impl State { /// Create new State checker, given client interface. pub fn new( - state: &'a C, + state: C, stale_id: Option, max_nonce: Option, ) -> Self { @@ -73,7 +73,7 @@ impl<'a, C> State<'a, C> { } } -impl<'a, C: NonceClient> txpool::Ready for State<'a, C> { +impl txpool::Ready for State { fn is_ready(&mut self, tx: &VerifiedTransaction) -> txpool::Readiness { // Check max nonce match self.max_nonce { @@ -84,7 +84,7 @@ impl<'a, C: NonceClient> txpool::Ready for State<'a, C> { } let sender = tx.sender(); - let state = self.state; + let state = &self.state; let state_nonce = || state.account_nonce(sender); let nonce = self.nonces.entry(*sender).or_insert_with(state_nonce); match tx.transaction.nonce.cmp(nonce) { @@ -180,11 +180,10 @@ mod tests { let (tx1, tx2, tx3) = (tx1.verified(), tx2.verified(), tx3.verified()); // when - assert_eq!(State::new(&TestClient::new(), None, None).is_ready(&tx3), txpool::Readiness::Future); - assert_eq!(State::new(&TestClient::new(), None, None).is_ready(&tx2), txpool::Readiness::Future); + assert_eq!(State::new(TestClient::new(), None, None).is_ready(&tx3), txpool::Readiness::Future); + assert_eq!(State::new(TestClient::new(), None, None).is_ready(&tx2), txpool::Readiness::Future); - let client = TestClient::new(); - let mut ready = State::new(&client, None, None); + let mut ready = State::new(TestClient::new(), None, None); // then assert_eq!(ready.is_ready(&tx1), txpool::Readiness::Ready); @@ -198,8 +197,8 @@ mod tests { let tx = Tx::default().signed().verified(); // when - let res1 = State::new(&TestClient::new(), None, Some(10.into())).is_ready(&tx); - let res2 = State::new(&TestClient::new(), None, Some(124.into())).is_ready(&tx); + let res1 = State::new(TestClient::new(), None, Some(10.into())).is_ready(&tx); + let res2 = State::new(TestClient::new(), None, Some(124.into())).is_ready(&tx); // then assert_eq!(res1, txpool::Readiness::Future); @@ -212,7 +211,7 @@ mod tests { let tx = Tx::default().signed().verified(); // when - let res = State::new(&TestClient::new().with_nonce(125), None, None).is_ready(&tx); + let res = State::new(TestClient::new().with_nonce(125), None, None).is_ready(&tx); // then assert_eq!(res, txpool::Readiness::Stale); @@ -224,7 +223,7 @@ mod tests { let (_, tx) = Tx::default().signed_pair().verified(); // when - let res = State::new(&TestClient::new(), Some(1), None).is_ready(&tx); + let res = State::new(TestClient::new(), Some(1), None).is_ready(&tx); // then assert_eq!(res, txpool::Readiness::Stale); diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index ae3f604f330..5dcde38bb95 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -16,9 +16,10 @@ use std::cmp; -use txpool::{self, scoring::{Choice, Scoring}, Readiness, Ready, ReplaceTransaction}; +use ethereum_types::U256; +use txpool::{self, scoring::{Choice, Scoring}, ReplaceTransaction}; use txpool::{VerifiedTransaction as PoolVerifiedTransaction}; -use super::{client, ready::State, ScoredTransaction, VerifiedTransaction}; +use super::{client, ScoredTransaction, VerifiedTransaction}; #[derive(Debug)] pub struct NonceAndGasPriceAndReadiness { @@ -33,9 +34,7 @@ impl NonceAndGasPriceAndReadiness { } impl txpool::ShouldReplace for NonceAndGasPriceAndReadiness -where - S: Scoring, - C: client::NonceClient { + where S: Scoring, C: client::NonceClient { fn should_replace( &mut self, @@ -57,21 +56,21 @@ where let old_score = (old.priority(), old.gas_price()); let new_score = (new.priority(), new.gas_price()); if new_score > old_score { - let client = &self.client; - let mut state_readiness = State::new(client, None, None); - + let state = &self.client; // calculate readiness based on state nonce + pooled txs from same sender - let mut is_ready = |replace: &ReplaceTransaction| { - if let Some(txs) = replace.pooled_by_sender { - // replay the readiness of currently pooled txs from the same sender - for tx in txs { - if *tx.transaction != ***replace.transaction { - state_readiness.is_ready(tx); - } - } - }; - let readiness = state_readiness.is_ready(replace.transaction); - readiness == Readiness::Ready + let is_ready = |replace: &ReplaceTransaction| { + let state_nonce = state.account_nonce(replace.sender()); + let nonce = + replace.pooled_by_sender.map_or(state_nonce, |txs| { + txs.iter().fold(state_nonce, |nonce, tx| { + if nonce == tx.nonce() && tx.transaction != *replace.transaction { + nonce.saturating_add(U256::from(1)) + } else { + nonce + } + }) + }); + nonce == replace.nonce() }; if !is_ready(new) && is_ready(old) { @@ -100,18 +99,18 @@ mod tests { use txpool::scoring::Choice::*; use txpool::ShouldReplace; - fn local_tx_verified(tx: Tx, keypair: &KeyPair) -> VerifiedTransaction { + fn local_tx_verified(tx: Tx, keypair: &KeyPair) -> ReplaceTransaction { let mut verified_tx = tx.unsigned().sign(keypair.secret(), None).verified(); verified_tx.priority = ::pool::Priority::Local; - verified_tx + replace_tx(verified_tx) } - fn should_replace(replace: &mut 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) + fn replace_tx<'a>(tx: VerifiedTransaction, ) -> ReplaceTransaction<'a, VerifiedTransaction> { + let tx = txpool::Transaction { + insertion_id: 0, + transaction: Arc::new(tx), + }; + ReplaceTransaction::new(tx, Default::default()) } #[test] @@ -156,15 +155,15 @@ mod tests { ..Default::default() }, &sender2); - assert_eq!(should_replace(&mut replace, same_sender_tx1.clone(), same_sender_tx2.clone()), InsertNew); - assert_eq!(should_replace(&mut replace, same_sender_tx2.clone(), same_sender_tx1.clone()), InsertNew); + assert_eq!(replace.should_replace(&same_sender_tx1, &same_sender_tx2), InsertNew); + assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx1), InsertNew); - assert_eq!(should_replace(&mut replace, different_sender_tx1.clone(), different_sender_tx2.clone()), InsertNew); - assert_eq!(should_replace(&mut replace, different_sender_tx2.clone(), different_sender_tx1.clone()), InsertNew); + assert_eq!(replace.should_replace(&different_sender_tx1, &different_sender_tx2), InsertNew); + assert_eq!(replace.should_replace(&different_sender_tx2, &different_sender_tx1), InsertNew); // txs with same sender and nonce - assert_eq!(should_replace(&mut replace, same_sender_tx2.clone(), same_sender_tx3.clone()), ReplaceOld); - assert_eq!(should_replace(&mut replace, same_sender_tx3.clone(), same_sender_tx2.clone()), RejectNew); + assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx3), ReplaceOld); + assert_eq!(replace.should_replace(&same_sender_tx3, &same_sender_tx2), RejectNew); } #[test] @@ -196,17 +195,17 @@ mod tests { let keypair = Random.generate().unwrap(); let txs = vec![tx1, tx2, tx3, tx4].into_iter().map(|tx| { - tx.unsigned().sign(keypair.secret(), None).verified() + replace_tx(tx.unsigned().sign(keypair.secret(), None).verified()) }).collect::>(); - assert_eq!(should_replace(&mut replace, txs[0].clone(), txs[1].clone()), RejectNew); - assert_eq!(should_replace(&mut replace, txs[1].clone(), txs[0].clone()), ReplaceOld); + assert_eq!(replace.should_replace(&txs[0], &txs[1]), RejectNew); + assert_eq!(replace.should_replace(&txs[1], &txs[0]), ReplaceOld); - assert_eq!(should_replace(&mut replace, txs[1].clone(), txs[2].clone()), RejectNew); - assert_eq!(should_replace(&mut replace, txs[2].clone(), txs[1].clone()), RejectNew); + assert_eq!(replace.should_replace(&txs[1], &txs[2]), RejectNew); + assert_eq!(replace.should_replace(&txs[2], &txs[1]), RejectNew); - assert_eq!(should_replace(&mut replace, txs[1].clone(), txs[3].clone()), ReplaceOld); - assert_eq!(should_replace(&mut replace, txs[3].clone(), txs[1].clone()), RejectNew); + assert_eq!(replace.should_replace(&txs[1], &txs[3]), ReplaceOld); + assert_eq!(replace.should_replace(&txs[3], &txs[1]), RejectNew); } #[test] @@ -222,7 +221,7 @@ mod tests { gas_price: 1, ..Default::default() }; - tx.signed().verified() + replace_tx(tx.signed().verified()) }; let tx_regular_high_gas = { let tx = Tx { @@ -230,7 +229,7 @@ mod tests { gas_price: 10, ..Default::default() }; - tx.signed().verified() + replace_tx(tx.signed().verified()) }; let tx_local_low_gas = { let tx = Tx { @@ -240,7 +239,7 @@ mod tests { }; let mut verified_tx = tx.signed().verified(); verified_tx.priority = ::pool::Priority::Local; - verified_tx + replace_tx(verified_tx) }; let tx_local_high_gas = { let tx = Tx { @@ -250,17 +249,17 @@ mod tests { }; let mut verified_tx = tx.signed().verified(); verified_tx.priority = ::pool::Priority::Local; - verified_tx + replace_tx(verified_tx) }; - assert_eq!(should_replace(&mut replace, tx_regular_low_gas.clone(), tx_regular_high_gas.clone()), ReplaceOld); - assert_eq!(should_replace(&mut replace, tx_regular_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew); + assert_eq!(replace.should_replace(&tx_regular_low_gas, &tx_regular_high_gas), ReplaceOld); + assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_regular_low_gas), RejectNew); - assert_eq!(should_replace(&mut replace, tx_regular_high_gas.clone(), tx_local_low_gas.clone()), ReplaceOld); - assert_eq!(should_replace(&mut replace, tx_local_low_gas.clone(), tx_regular_high_gas.clone()), RejectNew); + assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_local_low_gas), ReplaceOld); + assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_regular_high_gas), RejectNew); - assert_eq!(should_replace(&mut replace, tx_local_low_gas.clone(), tx_local_high_gas.clone()), InsertNew); - assert_eq!(should_replace(&mut replace, tx_local_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew); + assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_local_high_gas), InsertNew); + assert_eq!(replace.should_replace(&tx_local_high_gas, &tx_regular_low_gas), RejectNew); } #[test] @@ -275,7 +274,7 @@ mod tests { gas_price: 1, ..Default::default() }; - tx.signed().verified() + replace_tx(tx.signed().verified()) }; let tx_future_high_score = { let tx = Tx { @@ -283,10 +282,10 @@ mod tests { gas_price: 10, ..Default::default() }; - tx.signed().verified() + replace_tx(tx.signed().verified()) }; - assert_eq!(should_replace(&mut replace, tx_ready_low_score, tx_future_high_score), RejectNew); + assert_eq!(replace.should_replace(&tx_ready_low_score, &tx_future_high_score), RejectNew); } #[test] From c35dbfabd215261f2aa5055639902c401c01e3f7 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Tue, 26 Mar 2019 11:21:37 +0000 Subject: [PATCH 10/17] Make replace generic so it works with private-tx --- Cargo.lock | 16 +------ miner/src/pool/mod.rs | 2 +- miner/src/pool/queue.rs | 6 +-- miner/src/pool/replace.rs | 91 ++++++++++++++++++++------------------- 4 files changed, 51 insertions(+), 64 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 404d73f2fd6..bceceda25e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1034,7 +1034,7 @@ dependencies = [ "serde_derive 1.0.80 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.32 (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", "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)", ] @@ -2681,7 +2681,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", "transient-hashmap 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)", "trie-db 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)", "vm 0.1.0", @@ -4043,17 +4043,6 @@ dependencies = [ "log 0.4.5 (registry+https://github.com/rust-lang/crates.io-index)", ] -[[package]] -name = "transaction-pool" -version = "1.13.3" -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.5 (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)", -] - [[package]] name = "transaction-pool" version = "2.0.0" @@ -4787,7 +4776,6 @@ dependencies = [ "checksum toml 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)" = "4a2ecc31b0351ea18b3fe11274b8db6e4d82bce861bbb22e6dbed40417902c65" "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 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/miner/src/pool/mod.rs b/miner/src/pool/mod.rs index 4e9f25d1801..40a226d9fcd 100644 --- a/miner/src/pool/mod.rs +++ b/miner/src/pool/mod.rs @@ -122,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 0e036e6e137..fadfe9357a1 100644 --- a/miner/src/pool/queue.rs +++ b/miner/src/pool/queue.rs @@ -587,11 +587,7 @@ fn convert_error(err: txpool::Error) -> transa match err { Error::AlreadyImported(..) => transaction::Error::AlreadyImported, Error::TooCheapToEnter(..) => transaction::Error::LimitReached, - Error::TooCheapToReplace(..) => transaction::Error::TooCheapToReplace, - ref e => { - warn!(target: "txqueue", "Unknown import error: {:?}", e); - transaction::Error::NotAllowed - }, + Error::TooCheapToReplace(..) => transaction::Error::TooCheapToReplace } } diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index 5dcde38bb95..6c3de2a87d4 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -16,10 +16,10 @@ use std::cmp; -use ethereum_types::U256; +use ethereum_types::{U256, H160 as Address}; use txpool::{self, scoring::{Choice, Scoring}, ReplaceTransaction}; -use txpool::{VerifiedTransaction as PoolVerifiedTransaction}; -use super::{client, ScoredTransaction, VerifiedTransaction}; +use txpool::VerifiedTransaction; +use super::{client, ScoredTransaction}; #[derive(Debug)] pub struct NonceAndGasPriceAndReadiness { @@ -33,13 +33,16 @@ impl NonceAndGasPriceAndReadiness { } } -impl txpool::ShouldReplace for NonceAndGasPriceAndReadiness - where S: Scoring, C: client::NonceClient { - +impl txpool::ShouldReplace for NonceAndGasPriceAndReadiness +where + T: VerifiedTransaction + ScoredTransaction + PartialEq, + S: Scoring, + C: client::NonceClient, +{ fn should_replace( &mut self, - old: &ReplaceTransaction, - new: &ReplaceTransaction, + old: &ReplaceTransaction, + new: &ReplaceTransaction, ) -> Choice { let both_local = old.priority().is_local() && new.priority().is_local(); if old.sender() == new.sender() { @@ -58,12 +61,12 @@ impl txpool::ShouldReplace for NonceAndGasPriceAndRea 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 is_ready = |replace: &ReplaceTransaction| { let state_nonce = state.account_nonce(replace.sender()); let nonce = replace.pooled_by_sender.map_or(state_nonce, |txs| { txs.iter().fold(state_nonce, |nonce, tx| { - if nonce == tx.nonce() && tx.transaction != *replace.transaction { + if nonce == tx.nonce() && *tx.transaction != ***replace.transaction { nonce.saturating_add(U256::from(1)) } else { nonce @@ -99,18 +102,18 @@ mod tests { use txpool::scoring::Choice::*; use txpool::ShouldReplace; - fn local_tx_verified(tx: Tx, keypair: &KeyPair) -> ReplaceTransaction { + 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; - replace_tx(verified_tx) + verified_tx } - fn replace_tx<'a>(tx: VerifiedTransaction, ) -> ReplaceTransaction<'a, VerifiedTransaction> { - let tx = txpool::Transaction { - insertion_id: 0, - transaction: Arc::new(tx), - }; - ReplaceTransaction::new(tx, Default::default()) + fn should_replace(replace: &mut 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] @@ -155,15 +158,15 @@ mod tests { ..Default::default() }, &sender2); - assert_eq!(replace.should_replace(&same_sender_tx1, &same_sender_tx2), InsertNew); - assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx1), InsertNew); + assert_eq!(should_replace(&mut replace, same_sender_tx1.clone(), same_sender_tx2.clone()), InsertNew); + assert_eq!(should_replace(&mut replace, same_sender_tx2.clone(), same_sender_tx1.clone()), InsertNew); - assert_eq!(replace.should_replace(&different_sender_tx1, &different_sender_tx2), InsertNew); - assert_eq!(replace.should_replace(&different_sender_tx2, &different_sender_tx1), InsertNew); + assert_eq!(should_replace(&mut replace, different_sender_tx1.clone(), different_sender_tx2.clone()), InsertNew); + assert_eq!(should_replace(&mut replace, different_sender_tx2.clone(), different_sender_tx1.clone()), InsertNew); // txs with same sender and nonce - assert_eq!(replace.should_replace(&same_sender_tx2, &same_sender_tx3), ReplaceOld); - assert_eq!(replace.should_replace(&same_sender_tx3, &same_sender_tx2), RejectNew); + assert_eq!(should_replace(&mut replace, same_sender_tx2.clone(), same_sender_tx3.clone()), ReplaceOld); + assert_eq!(should_replace(&mut replace, same_sender_tx3.clone(), same_sender_tx2.clone()), RejectNew); } #[test] @@ -195,17 +198,17 @@ mod tests { let keypair = Random.generate().unwrap(); let txs = vec![tx1, tx2, tx3, tx4].into_iter().map(|tx| { - replace_tx(tx.unsigned().sign(keypair.secret(), None).verified()) + tx.unsigned().sign(keypair.secret(), None).verified() }).collect::>(); - assert_eq!(replace.should_replace(&txs[0], &txs[1]), RejectNew); - assert_eq!(replace.should_replace(&txs[1], &txs[0]), ReplaceOld); + assert_eq!(should_replace(&mut replace, txs[0].clone(), txs[1].clone()), RejectNew); + assert_eq!(should_replace(&mut replace, txs[1].clone(), txs[0].clone()), ReplaceOld); - assert_eq!(replace.should_replace(&txs[1], &txs[2]), RejectNew); - assert_eq!(replace.should_replace(&txs[2], &txs[1]), RejectNew); + assert_eq!(should_replace(&mut replace, txs[1].clone(), txs[2].clone()), RejectNew); + assert_eq!(should_replace(&mut replace, txs[2].clone(), txs[1].clone()), RejectNew); - assert_eq!(replace.should_replace(&txs[1], &txs[3]), ReplaceOld); - assert_eq!(replace.should_replace(&txs[3], &txs[1]), RejectNew); + assert_eq!(should_replace(&mut replace, txs[1].clone(), txs[3].clone()), ReplaceOld); + assert_eq!(should_replace(&mut replace, txs[3].clone(), txs[1].clone()), RejectNew); } #[test] @@ -221,7 +224,7 @@ mod tests { gas_price: 1, ..Default::default() }; - replace_tx(tx.signed().verified()) + tx.signed().verified() }; let tx_regular_high_gas = { let tx = Tx { @@ -229,7 +232,7 @@ mod tests { gas_price: 10, ..Default::default() }; - replace_tx(tx.signed().verified()) + tx.signed().verified() }; let tx_local_low_gas = { let tx = Tx { @@ -239,7 +242,7 @@ mod tests { }; let mut verified_tx = tx.signed().verified(); verified_tx.priority = ::pool::Priority::Local; - replace_tx(verified_tx) + verified_tx }; let tx_local_high_gas = { let tx = Tx { @@ -249,17 +252,17 @@ mod tests { }; let mut verified_tx = tx.signed().verified(); verified_tx.priority = ::pool::Priority::Local; - replace_tx(verified_tx) + verified_tx }; - assert_eq!(replace.should_replace(&tx_regular_low_gas, &tx_regular_high_gas), ReplaceOld); - assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_regular_low_gas), RejectNew); + assert_eq!(should_replace(&mut replace, tx_regular_low_gas.clone(), tx_regular_high_gas.clone()), ReplaceOld); + assert_eq!(should_replace(&mut replace, tx_regular_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew); - assert_eq!(replace.should_replace(&tx_regular_high_gas, &tx_local_low_gas), ReplaceOld); - assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_regular_high_gas), RejectNew); + assert_eq!(should_replace(&mut replace, tx_regular_high_gas.clone(), tx_local_low_gas.clone()), ReplaceOld); + assert_eq!(should_replace(&mut replace, tx_local_low_gas.clone(), tx_regular_high_gas.clone()), RejectNew); - assert_eq!(replace.should_replace(&tx_local_low_gas, &tx_local_high_gas), InsertNew); - assert_eq!(replace.should_replace(&tx_local_high_gas, &tx_regular_low_gas), RejectNew); + assert_eq!(should_replace(&mut replace, tx_local_low_gas.clone(), tx_local_high_gas.clone()), InsertNew); + assert_eq!(should_replace(&mut replace, tx_local_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew); } #[test] @@ -274,7 +277,7 @@ mod tests { gas_price: 1, ..Default::default() }; - replace_tx(tx.signed().verified()) + tx.signed().verified() }; let tx_future_high_score = { let tx = Tx { @@ -282,10 +285,10 @@ mod tests { gas_price: 10, ..Default::default() }; - replace_tx(tx.signed().verified()) + tx.signed().verified() }; - assert_eq!(replace.should_replace(&tx_ready_low_score, &tx_future_high_score), RejectNew); + assert_eq!(should_replace(&mut replace, tx_ready_low_score, tx_future_high_score), RejectNew); } #[test] From bbf1666d94baeec0ee7b43f6f0ce7d7f78a0ee4c Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Tue, 26 Mar 2019 11:41:36 +0000 Subject: [PATCH 11/17] Rename Replace and add missing docs --- .../private-tx/src/private_transactions.rs | 2 +- miner/src/pool/queue.rs | 2 +- miner/src/pool/replace.rs | 32 +++++++++++++------ 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/ethcore/private-tx/src/private_transactions.rs b/ethcore/private-tx/src/private_transactions.rs index eb588f7af51..b77f0293964 100644 --- a/ethcore/private-tx/src/private_transactions.rs +++ b/ethcore/private-tx/src/private_transactions.rs @@ -179,7 +179,7 @@ impl VerificationStore { }; let mut pool = self.verification_pool.write(); let mut replace = - pool::replace::NonceAndGasPriceAndReadiness::new(self.verification_pool.read().scoring().clone(), client); + pool::replace::ReplaceByScoreAndReadiness::new(self.verification_pool.read().scoring().clone(), client); pool.import(verified, &mut replace)?; Ok(()) } diff --git a/miner/src/pool/queue.rs b/miner/src/pool/queue.rs index fadfe9357a1..ad7c9e6f125 100644 --- a/miner/src/pool/queue.rs +++ b/miner/src/pool/queue.rs @@ -269,7 +269,7 @@ impl TransactionQueue { transaction_to_replace, ); - let mut replace = replace::NonceAndGasPriceAndReadiness::new(self.pool.read().scoring().clone(), client); + let mut replace = replace::ReplaceByScoreAndReadiness::new(self.pool.read().scoring().clone(), client); let results = transactions .into_iter() diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index 6c3de2a87d4..6dcbe59d020 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -14,6 +14,15 @@ // 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}; @@ -21,19 +30,22 @@ 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 NonceAndGasPriceAndReadiness { +pub struct ReplaceByScoreAndReadiness { scoring: S, client: C, } -impl NonceAndGasPriceAndReadiness { +impl ReplaceByScoreAndReadiness { + /// Create a new `ReplaceByScoreAndReadiness` pub fn new(scoring: S, client: C) -> Self { - NonceAndGasPriceAndReadiness { scoring, client } + ReplaceByScoreAndReadiness { scoring, client } } } -impl txpool::ShouldReplace for NonceAndGasPriceAndReadiness +impl txpool::ShouldReplace for ReplaceByScoreAndReadiness where T: VerifiedTransaction + ScoredTransaction + PartialEq, S: Scoring, @@ -120,7 +132,7 @@ mod tests { fn should_always_accept_local_transactions_unless_same_sender_and_nonce() { let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); let client = TestClient::new().with_nonce(1); - let mut replace = NonceAndGasPriceAndReadiness::new(scoring, client); + let mut replace = ReplaceByScoreAndReadiness::new(scoring, client); // same sender txs let keypair = Random.generate().unwrap(); @@ -173,7 +185,7 @@ mod tests { fn should_replace_same_sender_by_nonce() { let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); let client = TestClient::new().with_nonce(1); - let mut replace = NonceAndGasPriceAndReadiness::new(scoring, client); + let mut replace = ReplaceByScoreAndReadiness::new(scoring, client); let tx1 = Tx { nonce: 1, @@ -216,7 +228,7 @@ mod tests { // given let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); let client = TestClient::new().with_nonce(0); - let mut replace = NonceAndGasPriceAndReadiness::new(scoring, client); + let mut replace = ReplaceByScoreAndReadiness::new(scoring, client); let tx_regular_low_gas = { let tx = Tx { @@ -269,7 +281,7 @@ mod tests { fn should_not_replace_ready_transaction_with_future_transaction() { let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); let client = TestClient::new().with_nonce(1); - let mut replace = NonceAndGasPriceAndReadiness::new(scoring, client); + let mut replace = ReplaceByScoreAndReadiness::new(scoring, client); let tx_ready_low_score = { let tx = Tx { @@ -295,7 +307,7 @@ mod tests { 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 mut replace = NonceAndGasPriceAndReadiness::new(scoring, client); + let mut replace = ReplaceByScoreAndReadiness::new(scoring, client); let old_sender = Random.generate().unwrap(); let tx_old_ready_1 = { @@ -350,7 +362,7 @@ mod tests { 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 mut replace = NonceAndGasPriceAndReadiness::new(scoring, client); + let mut replace = ReplaceByScoreAndReadiness::new(scoring, client); // current transaction is ready but has a lower gas price than the new one let old_tx = { From 24e93a7a5349aa7ae95c3e01e26d267f5d6ac046 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Tue, 26 Mar 2019 12:16:03 +0000 Subject: [PATCH 12/17] ShouldReplace no longer mutable --- .../private-tx/src/private_transactions.rs | 6 +-- miner/src/pool/replace.rs | 54 +++++++++---------- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/ethcore/private-tx/src/private_transactions.rs b/ethcore/private-tx/src/private_transactions.rs index b77f0293964..d90e376be4a 100644 --- a/ethcore/private-tx/src/private_transactions.rs +++ b/ethcore/private-tx/src/private_transactions.rs @@ -178,9 +178,9 @@ impl VerificationStore { transaction_sender: signed_sender, }; let mut pool = self.verification_pool.write(); - let mut replace = - pool::replace::ReplaceByScoreAndReadiness::new(self.verification_pool.read().scoring().clone(), client); - pool.import(verified, &mut replace)?; + let replace = pool::replace::ReplaceByScoreAndReadiness::new( + self.verification_pool.read().scoring().clone(), client); + pool.import(verified, &replace)?; Ok(()) } diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index 6dcbe59d020..7a70260d162 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -52,7 +52,7 @@ where C: client::NonceClient, { fn should_replace( - &mut self, + &self, old: &ReplaceTransaction, new: &ReplaceTransaction, ) -> Choice { @@ -120,7 +120,7 @@ mod tests { verified_tx } - fn should_replace(replace: &mut ShouldReplace, old: VerifiedTransaction, new: VerifiedTransaction) -> Choice { + 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()); @@ -132,7 +132,7 @@ mod tests { fn should_always_accept_local_transactions_unless_same_sender_and_nonce() { let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); let client = TestClient::new().with_nonce(1); - let mut replace = ReplaceByScoreAndReadiness::new(scoring, client); + let replace = ReplaceByScoreAndReadiness::new(scoring, client); // same sender txs let keypair = Random.generate().unwrap(); @@ -170,22 +170,22 @@ mod tests { ..Default::default() }, &sender2); - assert_eq!(should_replace(&mut replace, same_sender_tx1.clone(), same_sender_tx2.clone()), InsertNew); - assert_eq!(should_replace(&mut replace, same_sender_tx2.clone(), same_sender_tx1.clone()), InsertNew); + 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(&mut replace, different_sender_tx1.clone(), different_sender_tx2.clone()), InsertNew); - assert_eq!(should_replace(&mut replace, different_sender_tx2.clone(), different_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(&mut replace, same_sender_tx2.clone(), same_sender_tx3.clone()), ReplaceOld); - assert_eq!(should_replace(&mut replace, same_sender_tx3.clone(), same_sender_tx2.clone()), RejectNew); + 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 mut replace = ReplaceByScoreAndReadiness::new(scoring, client); + let replace = ReplaceByScoreAndReadiness::new(scoring, client); let tx1 = Tx { nonce: 1, @@ -213,14 +213,14 @@ mod tests { tx.unsigned().sign(keypair.secret(), None).verified() }).collect::>(); - assert_eq!(should_replace(&mut replace, txs[0].clone(), txs[1].clone()), RejectNew); - assert_eq!(should_replace(&mut replace, txs[1].clone(), txs[0].clone()), ReplaceOld); + 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(&mut replace, txs[1].clone(), txs[2].clone()), RejectNew); - assert_eq!(should_replace(&mut replace, txs[2].clone(), txs[1].clone()), RejectNew); + 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(&mut replace, txs[1].clone(), txs[3].clone()), ReplaceOld); - assert_eq!(should_replace(&mut replace, txs[3].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] @@ -228,7 +228,7 @@ mod tests { // given let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); let client = TestClient::new().with_nonce(0); - let mut replace = ReplaceByScoreAndReadiness::new(scoring, client); + let replace = ReplaceByScoreAndReadiness::new(scoring, client); let tx_regular_low_gas = { let tx = Tx { @@ -267,21 +267,21 @@ mod tests { verified_tx }; - assert_eq!(should_replace(&mut replace, tx_regular_low_gas.clone(), tx_regular_high_gas.clone()), ReplaceOld); - assert_eq!(should_replace(&mut replace, tx_regular_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew); + assert_eq!(should_replace(&replace, tx_regular_low_gas.clone(), tx_regular_high_gas.clone()), ReplaceOld); + assert_eq!(should_replace(&replacej, tx_regular_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew); - assert_eq!(should_replace(&mut replace, tx_regular_high_gas.clone(), tx_local_low_gas.clone()), ReplaceOld); - assert_eq!(should_replace(&mut replace, tx_local_low_gas.clone(), tx_regular_high_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(&mut replace, tx_local_low_gas.clone(), tx_local_high_gas.clone()), InsertNew); - assert_eq!(should_replace(&mut replace, tx_local_high_gas.clone(), tx_regular_low_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 mut replace = ReplaceByScoreAndReadiness::new(scoring, client); + let replace = ReplaceByScoreAndReadiness::new(scoring, client); let tx_ready_low_score = { let tx = Tx { @@ -300,14 +300,14 @@ mod tests { tx.signed().verified() }; - assert_eq!(should_replace(&mut replace, tx_ready_low_score, tx_future_high_score), RejectNew); + 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 mut replace = ReplaceByScoreAndReadiness::new(scoring, client); + let replace = ReplaceByScoreAndReadiness::new(scoring, client); let old_sender = Random.generate().unwrap(); let tx_old_ready_1 = { @@ -362,7 +362,7 @@ mod tests { 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 mut replace = ReplaceByScoreAndReadiness::new(scoring, client); + let replace = ReplaceByScoreAndReadiness::new(scoring, client); // current transaction is ready but has a lower gas price than the new one let old_tx = { From 802c7f0dc9adbf8d303ef89e32ceae10e8b08598 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 28 Mar 2019 15:12:08 +0000 Subject: [PATCH 13/17] tx-pool: update to transaction-pool 2.0 from crates.io --- Cargo.lock | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bceceda25e4..a1ceb1a2106 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -939,7 +939,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 2.0.0", + "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)", ] @@ -1034,7 +1034,7 @@ dependencies = [ "serde_derive 1.0.80 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.32 (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 2.0.0", + "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)", ] @@ -2681,7 +2681,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 2.0.0", + "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)", "trie-db 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)", "vm 0.1.0", @@ -4028,13 +4028,6 @@ dependencies = [ "fxhash 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", ] -[[package]] -name = "trace-time" -version = "0.1.1" -dependencies = [ - "log 0.4.5 (registry+https://github.com/rust-lang/crates.io-index)", -] - [[package]] name = "trace-time" version = "0.1.1" @@ -4046,10 +4039,11 @@ dependencies = [ [[package]] name = "transaction-pool" version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "log 0.4.5 (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", + "trace-time 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -4776,6 +4770,7 @@ dependencies = [ "checksum toml 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)" = "4a2ecc31b0351ea18b3fe11274b8db6e4d82bce861bbb22e6dbed40417902c65" "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 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" From 49cdd2c48fd338a16f4596728f3343cf1dbf544e Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 28 Mar 2019 15:26:34 +0000 Subject: [PATCH 14/17] tx-pool: generic error type alias --- ethcore/private-tx/src/error.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ethcore/private-tx/src/error.rs b/ethcore/private-tx/src/error.rs index 8c86bf0cad3..eda08b2a567 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 { From 42f5fece1d7857df4b96e110d79dfe27009b750f Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Fri, 29 Mar 2019 10:30:11 +0000 Subject: [PATCH 15/17] Exit early for first unmatching nonce --- miner/src/pool/replace.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index 7a70260d162..b1112dcae1f 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -74,17 +74,16 @@ where let state = &self.client; // calculate readiness based on state nonce + pooled txs from same sender let is_ready = |replace: &ReplaceTransaction| { - let state_nonce = state.account_nonce(replace.sender()); - let nonce = - replace.pooled_by_sender.map_or(state_nonce, |txs| { - txs.iter().fold(state_nonce, |nonce, tx| { - if nonce == tx.nonce() && *tx.transaction != ***replace.transaction { - nonce.saturating_add(U256::from(1)) - } else { - nonce - } - }) - }); + 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() }; @@ -268,7 +267,7 @@ mod tests { }; assert_eq!(should_replace(&replace, tx_regular_low_gas.clone(), tx_regular_high_gas.clone()), ReplaceOld); - assert_eq!(should_replace(&replacej, tx_regular_high_gas.clone(), tx_regular_low_gas.clone()), RejectNew); + 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); From 89eed3c15e9f146329360a1fa30895eaa74b485e Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Sun, 31 Mar 2019 20:13:30 +0100 Subject: [PATCH 16/17] Fix private-tx test, use existing write lock --- ethcore/private-tx/src/private_transactions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/private-tx/src/private_transactions.rs b/ethcore/private-tx/src/private_transactions.rs index 368bc00fda1..7f10e4fe184 100644 --- a/ethcore/private-tx/src/private_transactions.rs +++ b/ethcore/private-tx/src/private_transactions.rs @@ -179,7 +179,7 @@ impl VerificationStore { }; let mut pool = self.verification_pool.write(); let replace = pool::replace::ReplaceByScoreAndReadiness::new( - self.verification_pool.read().scoring().clone(), client); + pool.scoring().clone(), client); pool.import(verified, &replace)?; Ok(()) } From 7091f8b226d49fee8eab976266b949ed6394dfcb Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Sun, 31 Mar 2019 20:23:51 +0100 Subject: [PATCH 17/17] Use read lock for pool scoring --- ethcore/private-tx/src/private_transactions.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ethcore/private-tx/src/private_transactions.rs b/ethcore/private-tx/src/private_transactions.rs index 7f10e4fe184..d0456657b06 100644 --- a/ethcore/private-tx/src/private_transactions.rs +++ b/ethcore/private-tx/src/private_transactions.rs @@ -177,10 +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.scoring().clone(), client); - pool.import(verified, &replace)?; + self.verification_pool.read().scoring().clone(), client); + self.verification_pool.write().import(verified, &replace)?; Ok(()) }