Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Minimal effective gas price in the queue #8934

Merged
merged 8 commits into from
Jun 30, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion miner/src/pool/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,8 @@ impl TransactionQueue {
trace_time!("pool::verify_and_import");
let options = self.options.read().clone();

let verifier = verifier::Verifier::new(client, options, self.insertion_id.clone());
let min_effective_gas_price = self.pool.read().minimal_entry_score().map(scoring::bump_gas_price);
let verifier = verifier::Verifier::new(client, options, self.insertion_id.clone(), min_effective_gas_price);
let results = transactions
.into_iter()
.map(|transaction| {
Expand Down
8 changes: 7 additions & 1 deletion miner/src/pool/scoring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ use super::{PrioritizationStrategy, VerifiedTransaction};
/// `new_gas_price > old_gas_price + old_gas_price >> SHIFT`
const GAS_PRICE_BUMP_SHIFT: usize = 3; // 2 = 25%, 3 = 12.5%, 4 = 6.25%

/// Calculate minimal gas price requirement.
#[inline]
pub fn bump_gas_price(old_gp: U256) -> U256 {
old_gp.saturating_add(old_gp >> GAS_PRICE_BUMP_SHIFT)
}

/// Simple, gas-price based scoring for transactions.
///
/// NOTE: Currently penalization does not apply to new transactions that enter the pool.
Expand All @@ -60,7 +66,7 @@ impl txpool::Scoring<VerifiedTransaction> for NonceAndGasPrice {
let old_gp = old.transaction.gas_price;
let new_gp = new.transaction.gas_price;

let min_required_gp = old_gp + (old_gp >> GAS_PRICE_BUMP_SHIFT);
let min_required_gp = bump_gas_price(old_gp);

match min_required_gp.cmp(&new_gp) {
cmp::Ordering::Greater => txpool::scoring::Choice::RejectNew,
Expand Down
42 changes: 40 additions & 2 deletions miner/src/pool/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -875,8 +875,8 @@ fn should_avoid_verifying_transaction_already_in_pool() {
},
PrioritizationStrategy::GasPriceOnly,
);
let client = TestClient::new();
let tx1 = Tx::default().signed().unverified();
let client = TestClient::new().with_balance(1_000_000_000);
let tx1 = Tx::gas_price(2).signed().unverified();

let res = txq.import(client.clone(), vec![tx1.clone()]);
assert_eq!(res, vec![Ok(())]);
Expand All @@ -892,3 +892,41 @@ fn should_avoid_verifying_transaction_already_in_pool() {
// then
assert_eq!(txq.status().status.transaction_count, 1);
}

#[test]
fn should_reject_early_in_case_gas_price_is_less_than_min_effective() {
// given
let txq = TransactionQueue::new(
txpool::Options {
max_count: 1,
max_per_sender: 2,
max_mem_usage: 50
},
verifier::Options {
minimal_gas_price: 1.into(),
block_gas_limit: 1_000_000.into(),
tx_gas_limit: 1_000_000.into(),
},
PrioritizationStrategy::GasPriceOnly,
);
let client = TestClient::new().with_balance(1_000_000_000);
let tx1 = Tx::gas_price(2).signed().unverified();

let res = txq.import(client.clone(), vec![tx1]);
assert_eq!(res, vec![Ok(())]);
assert_eq!(txq.status().status.transaction_count, 1);
assert!(client.was_verification_triggered());

// when
let client = TestClient::new();
let tx1 = Tx::default().signed().unverified();
let res = txq.import(client.clone(), vec![tx1]);
assert_eq!(res, vec![Err(transaction::Error::InsufficientGasPrice {
minimal: 2.into(),
got: 1.into(),
})]);
assert!(!client.was_verification_triggered());

// then
assert_eq!(txq.status().status.transaction_count, 1);
}
63 changes: 44 additions & 19 deletions miner/src/pool/verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,15 +131,22 @@ impl Transaction {
pub struct Verifier<C> {
client: C,
options: Options,
min_effective_gas_price: Option<U256>,
id: Arc<AtomicUsize>,
}

impl<C> Verifier<C> {
/// Creates new transaction verfier with specified options.
pub fn new(client: C, options: Options, id: Arc<AtomicUsize>) -> Self {
pub fn new(
client: C,
options: Options,
id: Arc<AtomicUsize>,
min_effective_gas_price: Option<U256>,
) -> Self {
Verifier {
client,
options,
min_effective_gas_price,
id,
}
}
Expand All @@ -164,7 +171,7 @@ impl<C: Client> txpool::Verifier<Transaction> for Verifier<C> {
if tx.gas() > &gas_limit {
debug!(
target: "txqueue",
"[{:?}] Dropping transaction above gas limit: {} > min({}, {})",
"[{:?}] Rejected transaction above gas limit: {} > min({}, {})",
hash,
tx.gas(),
self.options.block_gas_limit,
Expand All @@ -179,7 +186,7 @@ impl<C: Client> txpool::Verifier<Transaction> for Verifier<C> {
let minimal_gas = self.client.required_gas(tx.transaction());
if tx.gas() < &minimal_gas {
trace!(target: "txqueue",
"[{:?}] Dropping transaction with insufficient gas: {} < {}",
"[{:?}] Rejected transaction with insufficient gas: {} < {}",
hash,
tx.gas(),
minimal_gas,
Expand All @@ -192,22 +199,40 @@ impl<C: Client> txpool::Verifier<Transaction> for Verifier<C> {
}

let is_own = tx.is_local();
// Quick exit for non-service transactions
if tx.gas_price() < &self.options.minimal_gas_price
&& !tx.gas_price().is_zero()
&& !is_own
{
trace!(
target: "txqueue",
"[{:?}] Rejected tx below minimal gas price threshold: {} < {}",
hash,
tx.gas_price(),
self.options.minimal_gas_price,
);
bail!(transaction::Error::InsufficientGasPrice {
minimal: self.options.minimal_gas_price,
got: *tx.gas_price(),
});
// Quick exit for non-service and non-local transactions
//
// We're checking if the transaction is below configured minimal gas price
// or the effective minimal gas price in case the pool is full.
if !tx.gas_price().is_zero() && !is_own {
if tx.gas_price() < &self.options.minimal_gas_price {
trace!(
target: "txqueue",
"[{:?}] Rejected tx below minimal gas price threshold: {} < {}",
hash,
tx.gas_price(),
self.options.minimal_gas_price,
);
bail!(transaction::Error::InsufficientGasPrice {
minimal: self.options.minimal_gas_price,
got: *tx.gas_price(),
});
}

if let Some(ref gp) = self.min_effective_gas_price {
if tx.gas_price() < gp {
trace!(
target: "txqueue",
"[{:?}] Rejected tx below minimal effective gas price threshold: {} < {}",
hash,
tx.gas_price(),
gp,
);
bail!(transaction::Error::InsufficientGasPrice {
minimal: *gp,
got: *tx.gas_price(),
});
}
}
}

// Some more heavy checks below.
Expand Down
20 changes: 20 additions & 0 deletions transaction-pool/src/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,26 @@ impl<T, S, L> Pool<T, S, L> where
self.worst_transactions.iter().next().map(|x| x.transaction.transaction.clone())
}

/// Returns the score of the worst transaction if the pool is almost full.
///
/// This method can be used to determine what is the minimal required score
/// for the replacement transaction. If `None` is returned it means that
/// there is still plenty of room in the pool. Otherwise we return
/// `Some` with the score of the worst transaction.
pub fn minimal_entry_score(&self) -> Option<S::Score> {
let threshold = |x: usize| x * 9 / 10;
let is_full = {
self.by_hash.len() > threshold(self.options.max_count)
|| self.mem_usage > threshold(self.options.max_mem_usage)
};

if !is_full {
return None
}

self.worst_transactions.iter().next().map(|x| x.score.clone())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using next() here which looks correct (worst_transactions is ordered by ascending score). However I noticed: https://github.com/paritytech/parity/blob/0335e927e5d2133ceba4b54579094f1b4d810eac/transaction-pool/src/pool.rs#L277
Is the next_back() wrong then? Seems like it would check for the least worst transaction to replace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch, I'll prepare a test and fix that issue with remove_worst

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's correct in remove_worst. ScoreWithRef has a custom Ord implementation that is ascending if the internal score is descending. So the error is actually here, will make sure to add a correct test to cover this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, preparing a test case to cover that and a fix.

}

/// Returns an iterator of pending (ready) transactions.
pub fn pending<R: Ready<T>>(&self, ready: R) -> PendingIterator<T, R, S, L> {
PendingIterator {
Expand Down