From 9b5483a71b8f95896c40ddb518ac08f232ddeee7 Mon Sep 17 00:00:00 2001 From: Jim Posen Date: Wed, 27 Jun 2018 15:59:36 -0400 Subject: [PATCH] Improve should_replace on NonceAndGasPrice (#8980) * Additional tests for NonceAndGasPrice::should_replace. * Fix should_replace in the distinct sender case. * Use natural priority ordering to simplify should_replace. --- miner/src/pool/mod.rs | 16 ++--- miner/src/pool/scoring.rs | 129 ++++++++++++++++++++++++++++++------- miner/src/pool/tests/tx.rs | 6 +- 3 files changed, 116 insertions(+), 35 deletions(-) diff --git a/miner/src/pool/mod.rs b/miner/src/pool/mod.rs index fd4ef6ef2..4a1223226 100644 --- a/miner/src/pool/mod.rs +++ b/miner/src/pool/mod.rs @@ -83,20 +83,20 @@ impl PendingSettings { } /// Transaction priority. -#[derive(Debug, PartialEq, Eq, Clone, Copy)] +#[derive(Debug, PartialEq, Eq, PartialOrd, Clone, Copy)] pub(crate) enum Priority { - /// Local transactions (high priority) - /// - /// Transactions either from a local account or - /// submitted over local RPC connection via `eth_sendRawTransaction` - Local, + /// Regular transactions received over the network. (no priority boost) + Regular, /// Transactions from retracted blocks (medium priority) /// /// When block becomes non-canonical we re-import the transactions it contains /// to the queue and boost their priority. Retracted, - /// Regular transactions received over the network. (no priority boost) - Regular, + /// Local transactions (high priority) + /// + /// Transactions either from a local account or + /// submitted over local RPC connection via `eth_sendRawTransaction` + Local, } impl Priority { diff --git a/miner/src/pool/scoring.rs b/miner/src/pool/scoring.rs index e7551ed6a..6a8b11506 100644 --- a/miner/src/pool/scoring.rs +++ b/miner/src/pool/scoring.rs @@ -102,21 +102,16 @@ impl txpool::Scoring for NonceAndGasPrice { fn should_replace(&self, old: &VerifiedTransaction, new: &VerifiedTransaction) -> bool { if old.sender == new.sender { // prefer earliest transaction - if new.transaction.nonce < old.transaction.nonce { - return true + match new.transaction.nonce.cmp(&old.transaction.nonce) { + cmp::Ordering::Less => true, + cmp::Ordering::Greater => false, + cmp::Ordering::Equal => self.choose(old, new) == txpool::scoring::Choice::ReplaceOld, } + } else { + let old_score = (old.priority(), old.transaction.gas_price); + let new_score = (new.priority(), new.transaction.gas_price); + new_score > old_score } - - // Always kick out non-local transactions in favour of local ones. - if new.priority().is_local() && !old.priority().is_local() { - return true; - } - // And never kick out local transactions in favour of external ones. - if !new.priority().is_local() && old.priority.is_local() { - return false; - } - - self.choose(old, new) == txpool::scoring::Choice::ReplaceOld } } @@ -125,31 +120,117 @@ mod tests { use super::*; use std::sync::Arc; + use ethkey::{Random, Generator}; use pool::tests::tx::{Tx, TxExt}; use txpool::Scoring; #[test] - fn should_replace_non_local_transaction_with_local_one() { + fn should_replace_same_sender_by_nonce() { + let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); + + let tx1 = Tx { + nonce: 1, + gas_price: 1, + ..Default::default() + }; + let tx2 = Tx { + nonce: 2, + gas_price: 100, + ..Default::default() + }; + let tx3 = Tx { + nonce: 2, + gas_price: 110, + ..Default::default() + }; + let tx4 = Tx { + nonce: 2, + gas_price: 130, + ..Default::default() + }; + + let keypair = Random.generate().unwrap(); + let txs = vec![tx1, tx2, tx3, tx4].into_iter().enumerate().map(|(i, tx)| { + let verified = tx.unsigned().sign(keypair.secret(), None).verified(); + txpool::Transaction { + insertion_id: i as u64, + transaction: Arc::new(verified), + } + }).collect::>(); + + assert!(!scoring.should_replace(&txs[0], &txs[1])); + assert!(scoring.should_replace(&txs[1], &txs[0])); + + assert!(!scoring.should_replace(&txs[1], &txs[2])); + assert!(!scoring.should_replace(&txs[2], &txs[1])); + + assert!(scoring.should_replace(&txs[1], &txs[3])); + assert!(!scoring.should_replace(&txs[3], &txs[1])); + } + + #[test] + fn should_replace_different_sender_by_priority_and_gas_price() { // given let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); - let tx1 = { - let tx = Tx::default().signed().verified(); + let tx_regular_low_gas = { + let tx = Tx { + nonce: 1, + gas_price: 1, + ..Default::default() + }; + let verified_tx = tx.signed().verified(); txpool::Transaction { insertion_id: 0, - transaction: Arc::new(tx), + transaction: Arc::new(verified_tx), } }; - let tx2 = { - let mut tx = Tx::default().signed().verified(); - tx.priority = ::pool::Priority::Local; + let tx_regular_high_gas = { + let tx = Tx { + nonce: 2, + gas_price: 10, + ..Default::default() + }; + let verified_tx = tx.signed().verified(); txpool::Transaction { - insertion_id: 0, - transaction: Arc::new(tx), + insertion_id: 1, + transaction: Arc::new(verified_tx), + } + }; + let tx_local_low_gas = { + let tx = Tx { + nonce: 2, + gas_price: 1, + ..Default::default() + }; + let mut verified_tx = tx.signed().verified(); + verified_tx.priority = ::pool::Priority::Local; + txpool::Transaction { + insertion_id: 2, + transaction: Arc::new(verified_tx), + } + }; + let tx_local_high_gas = { + let tx = Tx { + nonce: 1, + gas_price: 10, + ..Default::default() + }; + let mut verified_tx = tx.signed().verified(); + verified_tx.priority = ::pool::Priority::Local; + txpool::Transaction { + insertion_id: 3, + transaction: Arc::new(verified_tx), } }; - assert!(scoring.should_replace(&tx1, &tx2)); - assert!(!scoring.should_replace(&tx2, &tx1)); + assert!(scoring.should_replace(&tx_regular_low_gas, &tx_regular_high_gas)); + assert!(!scoring.should_replace(&tx_regular_high_gas, &tx_regular_low_gas)); + + assert!(scoring.should_replace(&tx_regular_high_gas, &tx_local_low_gas)); + assert!(!scoring.should_replace(&tx_local_low_gas, &tx_regular_high_gas)); + + assert!(scoring.should_replace(&tx_local_low_gas, &tx_local_high_gas)); + assert!(!scoring.should_replace(&tx_local_high_gas, &tx_regular_low_gas)); } #[test] diff --git a/miner/src/pool/tests/tx.rs b/miner/src/pool/tests/tx.rs index a8b06f543..78cd85024 100644 --- a/miner/src/pool/tests/tx.rs +++ b/miner/src/pool/tests/tx.rs @@ -23,9 +23,9 @@ use pool::{verifier, VerifiedTransaction}; #[derive(Clone)] pub struct Tx { - nonce: u64, - gas: u64, - gas_price: u64, + pub nonce: u64, + pub gas: u64, + pub gas_price: u64, } impl Default for Tx {