tx-pool: accept local tx with higher gas price when pool full (#10901)

* tx-pool: accept local tx with higher gas price when pool full

* Revert "tx-pool: accept local tx with higher gas price when pool full"

This reverts commit 9d4adc5a

* tx-pool: new tx with same nonce as existing is ready

* tx-pool: explicit check for replacement tx (same sender & nonce)

* Update comment

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* Replace `ReplaceOld` with `InsertNew` for replacement txs
This commit is contained in:
Andrew Jones 2019-08-13 14:20:59 +01:00 committed by GitHub
parent e551122ab9
commit bd2e4f9c13
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

View File

@ -71,6 +71,20 @@ where
let old_score = (old.priority(), old.gas_price()); let old_score = (old.priority(), old.gas_price());
let new_score = (new.priority(), new.gas_price()); let new_score = (new.priority(), new.gas_price());
if new_score > old_score { 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 match self.scoring.choose(&txs[index], new) {
Choice::ReplaceOld => Choice::InsertNew,
choice => choice,
}
}
}
let state = &self.client; let state = &self.client;
// calculate readiness based on state nonce + pooled txs from same sender // calculate readiness based on state nonce + pooled txs from same sender
let is_ready = |replace: &ReplaceTransaction<T>| { let is_ready = |replace: &ReplaceTransaction<T>| {
@ -412,4 +426,88 @@ mod tests {
assert_eq!(replace.should_replace(&old, &new), ReplaceOld); 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);
}
#[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);
}
} }