From 9d4adc5ab6fe17efef0c025922302476307a5f9c Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 18 Jul 2019 14:41:52 +0100 Subject: [PATCH 1/6] tx-pool: accept local tx with higher gas price when pool full --- miner/src/pool/replace.rs | 67 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 5 deletions(-) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index b1112dcae1f..17e53d12f74 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -75,21 +75,36 @@ where // calculate readiness based on state nonce + pooled txs from same sender let is_ready = |replace: &ReplaceTransaction| { let mut nonce = state.account_nonce(replace.sender()); + let mut tx_same_nonce = None; 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)) + nonce = nonce.saturating_add(U256::from(1)); + if tx.nonce() == replace.nonce() { + tx_same_nonce = Some(tx.clone()); + } } else { break } } } - nonce == replace.nonce() + (nonce == replace.nonce(), tx_same_nonce) }; - if !is_ready(new) && is_ready(old) { - // prevent a ready transaction being replace by a non-ready transaction - Choice::RejectNew + let (new_is_ready, tx_same_nonce) = is_ready(new); + let (old_is_ready, _) = is_ready(old); + + if !new_is_ready && old_is_ready { + let new_is_same_nonce_and_better = tx_same_nonce + .map_or(false, |existing_tx| { + new.priority().is_local() && self.scoring.choose(&existing_tx, &new) != Choice::RejectNew + }); + if new_is_same_nonce_and_better { + Choice::InsertNew + } else { + // prevent a ready transaction being replace by a non-ready transaction + Choice::RejectNew + } } else { Choice::ReplaceOld } @@ -412,4 +427,46 @@ mod tests { assert_eq!(replace.should_replace(&old, &new), ReplaceOld); } + + #[test] + fn should_accept_local_tx_with_same_sender_and_nonce_with_better_gas_price() { + let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); + let client = TestClient::new().with_nonce(1); + let replace = ReplaceByScoreAndReadiness::new(scoring, client); + + // current transaction is ready + 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 = local_tx_verified(Tx { + nonce: 1, + gas_price: 1, + ..Default::default() + }, &new_sender); + + let tx_new_ready_2 = local_tx_verified(Tx { + nonce: 1, + gas_price: 2, // same nonce, higher gas price + ..Default::default() + }, &new_sender); + + 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_2) }; + let pooled_txs = [ + txpool::Transaction { insertion_id: 0, transaction: Arc::new(tx_new_ready_1) }, + ]; + + let old = ReplaceTransaction::new(&old_tx, None); + let new = ReplaceTransaction::new(&new_tx, Some(&pooled_txs)); + + assert_eq!(replace.should_replace(&old, &new), InsertNew); + } } From b1fd008dd88bea4bcf2965c0277100aa5e366b2b Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Fri, 19 Jul 2019 13:17:29 +0100 Subject: [PATCH 2/6] Revert "tx-pool: accept local tx with higher gas price when pool full" This reverts commit 9d4adc5a --- miner/src/pool/replace.rs | 67 +++------------------------------------ 1 file changed, 5 insertions(+), 62 deletions(-) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index 17e53d12f74..b1112dcae1f 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -75,36 +75,21 @@ where // calculate readiness based on state nonce + pooled txs from same sender let is_ready = |replace: &ReplaceTransaction| { let mut nonce = state.account_nonce(replace.sender()); - let mut tx_same_nonce = None; 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)); - if tx.nonce() == replace.nonce() { - tx_same_nonce = Some(tx.clone()); - } + nonce = nonce.saturating_add(U256::from(1)) } else { break } } } - (nonce == replace.nonce(), tx_same_nonce) + nonce == replace.nonce() }; - let (new_is_ready, tx_same_nonce) = is_ready(new); - let (old_is_ready, _) = is_ready(old); - - if !new_is_ready && old_is_ready { - let new_is_same_nonce_and_better = tx_same_nonce - .map_or(false, |existing_tx| { - new.priority().is_local() && self.scoring.choose(&existing_tx, &new) != Choice::RejectNew - }); - if new_is_same_nonce_and_better { - Choice::InsertNew - } else { - // prevent a ready transaction being replace by a non-ready transaction - Choice::RejectNew - } + if !is_ready(new) && is_ready(old) { + // prevent a ready transaction being replace by a non-ready transaction + Choice::RejectNew } else { Choice::ReplaceOld } @@ -427,46 +412,4 @@ mod tests { assert_eq!(replace.should_replace(&old, &new), ReplaceOld); } - - #[test] - fn should_accept_local_tx_with_same_sender_and_nonce_with_better_gas_price() { - let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); - let client = TestClient::new().with_nonce(1); - let replace = ReplaceByScoreAndReadiness::new(scoring, client); - - // current transaction is ready - 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 = local_tx_verified(Tx { - nonce: 1, - gas_price: 1, - ..Default::default() - }, &new_sender); - - let tx_new_ready_2 = local_tx_verified(Tx { - nonce: 1, - gas_price: 2, // same nonce, higher gas price - ..Default::default() - }, &new_sender); - - 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_2) }; - let pooled_txs = [ - txpool::Transaction { insertion_id: 0, transaction: Arc::new(tx_new_ready_1) }, - ]; - - let old = ReplaceTransaction::new(&old_tx, None); - let new = ReplaceTransaction::new(&new_tx, Some(&pooled_txs)); - - assert_eq!(replace.should_replace(&old, &new), InsertNew); - } } From 2dec19dadbc879c1c491f452f1498dc875cd7bf2 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 18 Jul 2019 15:20:15 +0100 Subject: [PATCH 3/6] tx-pool: new tx with same nonce as existing is ready --- miner/src/pool/replace.rs | 44 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index b1112dcae1f..4b4aa87287a 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -77,7 +77,7 @@ where 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 { + if nonce == tx.nonce() && nonce != replace.nonce() && *tx.transaction != ***replace.transaction { nonce = nonce.saturating_add(U256::from(1)) } else { break @@ -412,4 +412,46 @@ mod tests { assert_eq!(replace.should_replace(&old, &new), ReplaceOld); } + + #[test] + fn should_accept_local_tx_with_same_sender_and_nonce_with_better_gas_price() { + let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); + let client = TestClient::new().with_nonce(1); + let replace = ReplaceByScoreAndReadiness::new(scoring, client); + + // current transaction is ready + 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 = local_tx_verified(Tx { + nonce: 1, + gas_price: 1, + ..Default::default() + }, &new_sender); + + let tx_new_ready_2 = local_tx_verified(Tx { + nonce: 1, + gas_price: 2, // same nonce, higher gas price + ..Default::default() + }, &new_sender); + + 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_2) }; + let pooled_txs = [ + txpool::Transaction { insertion_id: 0, transaction: Arc::new(tx_new_ready_1) }, + ]; + + 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 526722523ac9dd2fbad077fed94c072462695c53 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Fri, 19 Jul 2019 13:15:20 +0100 Subject: [PATCH 4/6] tx-pool: explicit check for replacement tx (same sender & nonce) --- miner/src/pool/replace.rs | 51 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index 4b4aa87287a..e10f3df71d7 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -71,13 +71,20 @@ where let old_score = (old.priority(), old.gas_price()); let new_score = (new.priority(), new.gas_price()); if new_score > old_score { + // check for an existing transaction with the same nonce + if let Some(txs) = new.pooled_by_sender { + if let Ok(index) = txs.binary_search_by(|old| self.scoring.compare(old, new)) { + return self.scoring.choose(&txs[index], new) + } + } + let state = &self.client; // calculate readiness based on state nonce + pooled txs from same sender let is_ready = |replace: &ReplaceTransaction| { let mut nonce = state.account_nonce(replace.sender()); if let Some(txs) = replace.pooled_by_sender { for tx in txs.iter() { - if nonce == tx.nonce() && nonce != replace.nonce() && *tx.transaction != ***replace.transaction { + if nonce == tx.nonce() && *tx.transaction != ***replace.transaction { nonce = nonce.saturating_add(U256::from(1)) } else { break @@ -454,4 +461,46 @@ mod tests { assert_eq!(replace.should_replace(&old, &new), ReplaceOld); } + + #[test] + fn should_reject_local_tx_with_same_sender_and_nonce_with_worse_gas_price() { + let scoring = NonceAndGasPrice(PrioritizationStrategy::GasPriceOnly); + let client = TestClient::new().with_nonce(1); + let replace = ReplaceByScoreAndReadiness::new(scoring, client); + + // current transaction is ready + 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 = local_tx_verified(Tx { + nonce: 1, + gas_price: 2, + ..Default::default() + }, &new_sender); + + let tx_new_ready_2 = local_tx_verified(Tx { + nonce: 1, + gas_price: 1, // same nonce, lower gas price + ..Default::default() + }, &new_sender); + + 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_2) }; + let pooled_txs = [ + txpool::Transaction { insertion_id: 0, transaction: Arc::new(tx_new_ready_1) }, + ]; + + let old = ReplaceTransaction::new(&old_tx, None); + let new = ReplaceTransaction::new(&new_tx, Some(&pooled_txs)); + + assert_eq!(replace.should_replace(&old, &new), RejectNew); + } } From 7a38eb3b9876ed8488a2bc94b9e8524d8868ce3b Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Tue, 13 Aug 2019 11:55:38 +0100 Subject: [PATCH 5/6] Update comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Tomasz Drwięga --- miner/src/pool/replace.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index e10f3df71d7..c575fb7244f 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -71,7 +71,11 @@ where let old_score = (old.priority(), old.gas_price()); let new_score = (new.priority(), new.gas_price()); if new_score > old_score { - // check for an existing transaction with the same nonce + // Check if this is a replacement transaction. + // + // With replacement transactions we can safely return `InsertNew` here, because + // we don't need to remove `old` (worst transaction in the pool) since `new` will replace + // some other transaction in the pool so we will never go above limit anyway. if let Some(txs) = new.pooled_by_sender { if let Ok(index) = txs.binary_search_by(|old| self.scoring.compare(old, new)) { return self.scoring.choose(&txs[index], new) From 895ae41afcfe5604c1fc79e1341de92ec0750661 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Tue, 13 Aug 2019 12:17:23 +0100 Subject: [PATCH 6/6] Replace `ReplaceOld` with `InsertNew` for replacement txs --- miner/src/pool/replace.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/miner/src/pool/replace.rs b/miner/src/pool/replace.rs index c575fb7244f..0655af59990 100644 --- a/miner/src/pool/replace.rs +++ b/miner/src/pool/replace.rs @@ -72,13 +72,16 @@ where let new_score = (new.priority(), new.gas_price()); if new_score > old_score { // Check if this is a replacement transaction. - // + // // With replacement transactions we can safely return `InsertNew` here, because // we don't need to remove `old` (worst transaction in the pool) since `new` will replace // some other transaction in the pool so we will never go above limit anyway. if let Some(txs) = new.pooled_by_sender { if let Ok(index) = txs.binary_search_by(|old| self.scoring.compare(old, new)) { - return self.scoring.choose(&txs[index], new) + return match self.scoring.choose(&txs[index], new) { + Choice::ReplaceOld => Choice::InsertNew, + choice => choice, + } } } @@ -463,7 +466,7 @@ mod tests { let old = ReplaceTransaction::new(&old_tx, None); let new = ReplaceTransaction::new(&new_tx, Some(&pooled_txs)); - assert_eq!(replace.should_replace(&old, &new), ReplaceOld); + assert_eq!(replace.should_replace(&old, &new), InsertNew); } #[test]