From 35ecb396b60886fc89af3bfee84f67d523d33513 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Tue, 23 Aug 2016 13:30:33 +0200 Subject: [PATCH] Market-orientated transaction pricing (#1963) * Market-orientated transaction pricing Avoid a strict gas-limit and let the market decide through using a priority queue based around gas pricing for transactions. In periods of low transaction volume, they'll be processed for a lower fee. * Fix tests, add/clarify documentation, fix some logic. * Change default to reflect CLI. * Specify type. * Make test more precise. * Fix doc test --- ethcore/src/miner/transaction_queue.rs | 466 ++++++++++++++++--------- parity/cli.rs | 2 +- parity/params.rs | 2 +- 3 files changed, 295 insertions(+), 175 deletions(-) diff --git a/ethcore/src/miner/transaction_queue.rs b/ethcore/src/miner/transaction_queue.rs index 9d3a85995..8a2a37145 100644 --- a/ethcore/src/miner/transaction_queue.rs +++ b/ethcore/src/miner/transaction_queue.rs @@ -43,14 +43,14 @@ //! //! let st1 = t1.sign(&key.secret()); //! let st2 = t2.sign(&key.secret()); -//! let default_nonce = |_a: &Address| AccountDetails { +//! let default_account_details = |_a: &Address| AccountDetails { //! nonce: U256::from(10), //! balance: U256::from(1_000_000), //! }; //! //! let mut txq = TransactionQueue::new(); -//! txq.add(st2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); -//! txq.add(st1.clone(), &default_nonce, TransactionOrigin::External).unwrap(); +//! txq.add(st2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); +//! txq.add(st1.clone(), &default_account_details, TransactionOrigin::External).unwrap(); //! //! // Check status //! assert_eq!(txq.status().pending, 2); @@ -62,7 +62,7 @@ //! //! // And when transaction is removed (but nonce haven't changed) //! // it will move subsequent transactions to future -//! txq.remove_invalid(&st1.hash(), &default_nonce); +//! txq.remove_invalid(&st1.hash(), &default_account_details); //! assert_eq!(txq.status().pending, 0); //! assert_eq!(txq.status().future, 1); //! assert_eq!(txq.top_transactions().len(), 0); @@ -82,7 +82,7 @@ use std::cmp::Ordering; use std::cmp; -use std::collections::{HashMap, BTreeSet}; +use std::collections::{HashSet, HashMap, BTreeSet, BTreeMap}; use util::{Address, H256, Uint, U256}; use util::table::Table; use transaction::*; @@ -226,23 +226,34 @@ impl VerifiedTransaction { struct TransactionSet { by_priority: BTreeSet, by_address: Table, + by_gas_price: BTreeMap>, limit: usize, } impl TransactionSet { - /// Inserts `TransactionOrder` to this set + /// Inserts `TransactionOrder` to this set. Transaction does not need to be unique - + /// the same transaction may be validly inserted twice. Any previous transaction that + /// it replaces (i.e. with the same `sender` and `nonce`) should be returned. fn insert(&mut self, sender: Address, nonce: U256, order: TransactionOrder) -> Option { - self.by_priority.insert(order.clone()); - let r = self.by_address.insert(sender, nonce, order); - // If transaction was replaced remove it from priority queue - if let Some(ref old_order) = r { - self.by_priority.remove(old_order); + if !self.by_priority.insert(order.clone()) { + return Some(order.clone()); } - assert_eq!(self.by_priority.len(), self.by_address.len()); - r + let order_hash = order.hash.clone(); + let order_gas_price = order.gas_price.clone(); + let by_address_replaced = self.by_address.insert(sender, nonce, order); + // If transaction was replaced remove it from priority queue + if let Some(ref old_order) = by_address_replaced { + assert!(self.by_priority.remove(old_order), "hash is in `by_address`; all transactions in `by_address` must be in `by_priority`; qed"); + assert!(Self::remove_item(&mut self.by_gas_price, &old_order.gas_price, &old_order.hash), + "hash is in `by_address`; all transactions' gas_prices in `by_address` must be in `by_gas_limit`; qed"); + } + Self::insert_item(&mut self.by_gas_price, order_gas_price, order_hash); + debug_assert_eq!(self.by_priority.len(), self.by_address.len()); + debug_assert_eq!(self.by_gas_price.iter().map(|(_, v)| v.len()).fold(0, |a, b| a + b), self.by_address.len()); + by_address_replaced } - /// Remove low priority transactions if there is more then specified by given `limit`. + /// Remove low priority transactions if there is more than specified by given `limit`. /// /// It drops transactions from this set but also removes associated `VerifiedTransaction`. /// Returns addresses and lowest nonces of transactions removed because of limit. @@ -267,7 +278,7 @@ impl TransactionSet { .expect("Transaction has just been found in `by_priority`; so it is in `by_address` also."); by_hash.remove(&order.hash) - .expect("Hash found in `by_priorty` matches the one dropped; so it is included in `by_hash`"); + .expect("hash is in `by_priorty`; all hashes in `by_priority` must be in `by_hash`; qed"); let min = removed.get(&sender).map_or(nonce, |val| cmp::min(*val, nonce)); removed.insert(sender, min); @@ -278,6 +289,8 @@ impl TransactionSet { /// Drop transaction from this set (remove from `by_priority` and `by_address`) fn drop(&mut self, sender: &Address, nonce: &U256) -> Option { if let Some(tx_order) = self.by_address.remove(sender, nonce) { + assert!(Self::remove_item(&mut self.by_gas_price, &tx_order.gas_price, &tx_order.hash), + "hash is in `by_address`; all transactions' gas_prices in `by_address` must be in `by_gas_limit`; qed"); self.by_priority.remove(&tx_order); assert_eq!(self.by_priority.len(), self.by_address.len()); return Some(tx_order); @@ -290,6 +303,7 @@ impl TransactionSet { fn clear(&mut self) { self.by_priority.clear(); self.by_address.clear(); + self.by_gas_price.clear(); } /// Sets new limit for number of transactions in this `TransactionSet`. @@ -297,6 +311,41 @@ impl TransactionSet { fn set_limit(&mut self, limit: usize) { self.limit = limit; } + + /// Get the minimum gas price that we can accept into this queue that wouldn't cause the transaction to + /// immediately be dropped. 0 if the queue isn't at capacity; 1 plus the lowest if it is. + fn gas_price_entry_limit(&self) -> U256 { + match self.by_gas_price.keys().next() { + Some(k) if self.by_priority.len() >= self.limit => *k + 1.into(), + _ => U256::default(), + } + } + + /// Insert an item into a BTreeMap/HashSet "multimap". + fn insert_item(into: &mut BTreeMap>, gas_price: U256, hash: H256) -> bool { + into.entry(gas_price).or_insert_with(Default::default).insert(hash) + } + + /// Remove an item from a BTreeMap/HashSet "multimap". + /// Returns true if the item was removed successfully. + fn remove_item(from: &mut BTreeMap>, gas_price: &U256, hash: &H256) -> bool { + if let Some(mut hashes) = from.get_mut(gas_price) { + let only_one_left = hashes.len() == 1; + if !only_one_left { + // Operation may be ok: only if hash is in gas-price's Set. + return hashes.remove(hash); + } + if hashes.iter().next().unwrap() != hash { + // Operation failed: hash not the single item in gas-price's Set. + return false; + } + } else { + // Operation failed: gas-price not found in Map. + return false; + } + // Operation maybe ok: only if hash not found in gas-price Set. + from.remove(gas_price).is_some() + } } #[derive(Debug)] @@ -316,7 +365,6 @@ pub struct AccountDetails { pub balance: U256, } - /// Transactions with `gas > (gas_limit + gas_limit * Factor(in percents))` are not imported to the queue. const GAS_LIMIT_HYSTERESIS: usize = 10; // % @@ -355,12 +403,14 @@ impl TransactionQueue { let current = TransactionSet { by_priority: BTreeSet::new(), by_address: Table::new(), + by_gas_price: Default::default(), limit: limit, }; let future = TransactionSet { by_priority: BTreeSet::new(), by_address: Table::new(), + by_gas_price: Default::default(), limit: limit, }; @@ -400,6 +450,12 @@ impl TransactionQueue { self.minimal_gas_price = min_gas_price; } + /// Get one more than the lowest gas price in the queue iff the pool is + /// full, otherwise 0. + pub fn effective_minimum_gas_price(&self) -> U256 { + self.current.gas_price_entry_limit() + } + /// Sets new gas limit. Transactions with gas slightly (`GAS_LIMIT_HYSTERESIS`) above the limit won't be imported. /// Any transaction already imported to the queue is not affected. pub fn set_gas_limit(&mut self, gas_limit: U256) { @@ -445,6 +501,21 @@ impl TransactionQueue { })); } + let full_queues_lowest = self.effective_minimum_gas_price(); + if tx.gas_price < full_queues_lowest && origin != TransactionOrigin::Local { + trace!(target: "txqueue", + "Dropping transaction below lowest gas price in a full queue: {:?} (gp: {} < {})", + tx.hash(), + tx.gas_price, + full_queues_lowest + ); + + return Err(Error::Transaction(TransactionError::InsufficientGasPrice { + minimal: full_queues_lowest, + got: tx.gas_price, + })); + } + try!(tx.check_low_s()); if tx.gas > self.gas_limit || tx.gas > self.tx_gas_limit { @@ -811,59 +882,86 @@ mod test { } } - fn new_unsigned_tx(nonce: U256) -> Transaction { + fn default_nonce() -> U256 { 123.into() } + fn default_gas_price() -> U256 { 1.into() } + + fn new_unsigned_tx(nonce: U256, gas_price: U256) -> Transaction { Transaction { action: Action::Create, value: U256::from(100), data: "3331600055".from_hex().unwrap(), gas: U256::from(100_000), - gas_price: U256::one(), + gas_price: gas_price, nonce: nonce } } - fn new_tx() -> SignedTransaction { + fn new_tx(nonce: U256, gas_price: U256) -> SignedTransaction { let keypair = KeyPair::create().unwrap(); - new_unsigned_tx(U256::from(123)).sign(keypair.secret()) + new_unsigned_tx(nonce, gas_price).sign(keypair.secret()) } - - fn default_nonce_val() -> U256 { - U256::from(123) + fn new_tx_default() -> SignedTransaction { + new_tx(default_nonce(), default_gas_price()) } - fn default_nonce(_address: &Address) -> AccountDetails { + fn default_account_details(_address: &Address) -> AccountDetails { AccountDetails { - nonce: default_nonce_val(), + nonce: default_nonce(), balance: !U256::zero() } } - /// Returns two transactions with identical (sender, nonce) but different hashes - fn new_similar_txs() -> (SignedTransaction, SignedTransaction) { + fn new_tx_pair(nonce: U256, gas_price: U256, nonce_increment: U256, gas_price_increment: U256) -> (SignedTransaction, SignedTransaction) { + let tx1 = new_unsigned_tx(nonce, gas_price); + let tx2 = new_unsigned_tx(nonce + nonce_increment, gas_price + gas_price_increment); + let keypair = KeyPair::create().unwrap(); let secret = &keypair.secret(); - let nonce = U256::from(123); - let tx = new_unsigned_tx(nonce); - let mut tx2 = new_unsigned_tx(nonce); - tx2.gas_price = U256::from(2); - - (tx.sign(secret), tx2.sign(secret)) + (tx1.sign(secret), tx2.sign(secret)) } - fn new_txs(second_nonce: U256) -> (SignedTransaction, SignedTransaction) { - new_txs_with_gas_price_diff(second_nonce, U256::zero()) + fn new_tx_pair_default(nonce_increment: U256, gas_price_increment: U256) -> (SignedTransaction, SignedTransaction) { + new_tx_pair(default_nonce(), default_gas_price(), nonce_increment, gas_price_increment) } - fn new_txs_with_gas_price_diff(second_nonce: U256, gas_price: U256) -> (SignedTransaction, SignedTransaction) { - let keypair = KeyPair::create().unwrap(); - let secret = &keypair.secret(); - let nonce = U256::from(123); - let tx = new_unsigned_tx(nonce); - let mut tx2 = new_unsigned_tx(nonce + second_nonce); - tx2.gas_price = tx2.gas_price + gas_price; + /// Returns two transactions with identical (sender, nonce) but different gas_price/hash. + fn new_similar_tx_pair() -> (SignedTransaction, SignedTransaction) { + new_tx_pair_default(0.into(), 1.into()) + } - (tx.sign(secret), tx2.sign(secret)) + #[test] + fn should_return_correct_nonces_when_dropped_because_of_limit() { + // given + let mut txq = TransactionQueue::with_limits(2, !U256::zero()); + let (tx1, tx2) = new_tx_pair(123.into(), 1.into(), 1.into(), 0.into()); + let sender = tx1.sender().unwrap(); + let nonce = tx1.nonce; + txq.add(tx1.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + assert_eq!(txq.status().pending, 2); + assert_eq!(txq.last_nonce(&sender), Some(nonce + U256::one())); + + // when + let tx = new_tx(123.into(), 1.into()); + let res = txq.add(tx.clone(), &default_account_details, TransactionOrigin::External); + + // then + // No longer the case as we don't even consider a transaction that isn't above a full + // queue's minimum gas price. + // We may want to reconsider this in the near future so leaving this code in as a + // possible alternative. + /* + assert_eq!(res.unwrap(), TransactionImportResult::Current); + assert_eq!(txq.status().pending, 2); + assert_eq!(txq.last_nonce(&sender), Some(nonce)); + */ + assert_eq!(unwrap_tx_err(res), TransactionError::InsufficientGasPrice { + minimal: 2.into(), + got: 1.into(), + }); + assert_eq!(txq.status().pending, 2); + assert_eq!(txq.last_nonce(&sender), Some(tx2.nonce)); } #[test] @@ -872,9 +970,10 @@ mod test { let mut set = TransactionSet { by_priority: BTreeSet::new(), by_address: Table::new(), + by_gas_price: Default::default(), limit: 1 }; - let (tx1, tx2) = new_txs(U256::from(1)); + let (tx1, tx2) = new_tx_pair_default(1.into(), 0.into()); let tx1 = VerifiedTransaction::new(tx1, TransactionOrigin::External).unwrap(); let tx2 = VerifiedTransaction::new(tx2, TransactionOrigin::External).unwrap(); let mut by_hash = { @@ -911,11 +1010,12 @@ mod test { let mut set = TransactionSet { by_priority: BTreeSet::new(), by_address: Table::new(), + by_gas_price: Default::default(), limit: 1 }; // Create two transactions with same nonce // (same hash) - let (tx1, tx2) = new_txs(U256::from(0)); + let (tx1, tx2) = new_tx_pair_default(0.into(), 0.into()); let tx1 = VerifiedTransaction::new(tx1, TransactionOrigin::External).unwrap(); let tx2 = VerifiedTransaction::new(tx2, TransactionOrigin::External).unwrap(); let by_hash = { @@ -931,25 +1031,68 @@ mod test { set.insert(tx1.sender(), tx1.nonce(), order1.clone()); assert_eq!(set.by_priority.len(), 1); assert_eq!(set.by_address.len(), 1); + assert_eq!(set.by_gas_price.len(), 1); + assert_eq!(*set.by_gas_price.iter().next().unwrap().0, 1.into()); + assert_eq!(set.by_gas_price.iter().next().unwrap().1.len(), 1); // Two different orders (imagine nonce changed in the meantime) let order2 = TransactionOrder::for_transaction(&tx2, U256::one()); set.insert(tx2.sender(), tx2.nonce(), order2.clone()); assert_eq!(set.by_priority.len(), 1); assert_eq!(set.by_address.len(), 1); + assert_eq!(set.by_gas_price.len(), 1); + assert_eq!(*set.by_gas_price.iter().next().unwrap().0, 1.into()); + assert_eq!(set.by_gas_price.iter().next().unwrap().1.len(), 1); // then assert_eq!(by_hash.len(), 1); assert_eq!(set.by_priority.len(), 1); assert_eq!(set.by_address.len(), 1); + assert_eq!(set.by_gas_price.len(), 1); + assert_eq!(*set.by_gas_price.iter().next().unwrap().0, 1.into()); + assert_eq!(set.by_gas_price.iter().next().unwrap().1.len(), 1); assert_eq!(set.by_priority.iter().next().unwrap().clone(), order2); } + #[test] + fn should_not_insert_same_transaction_twice_into_set() { + let mut set = TransactionSet { + by_priority: BTreeSet::new(), + by_address: Table::new(), + by_gas_price: Default::default(), + limit: 2 + }; + let tx = new_tx_default(); + let tx1 = VerifiedTransaction::new(tx.clone(), TransactionOrigin::External).unwrap(); + let order1 = TransactionOrder::for_transaction(&tx1, U256::zero()); + assert!(set.insert(tx1.sender(), tx1.nonce(), order1).is_none()); + let tx2 = VerifiedTransaction::new(tx, TransactionOrigin::External).unwrap(); + let order2 = TransactionOrder::for_transaction(&tx2, U256::zero()); + assert!(set.insert(tx2.sender(), tx2.nonce(), order2).is_some()); + } + + #[test] + fn should_give_correct_gas_price_entry_limit() { + let mut set = TransactionSet { + by_priority: BTreeSet::new(), + by_address: Table::new(), + by_gas_price: Default::default(), + limit: 1 + }; + + assert_eq!(set.gas_price_entry_limit(), 0.into()); + let tx = new_tx_default(); + let tx1 = VerifiedTransaction::new(tx.clone(), TransactionOrigin::External).unwrap(); + let order1 = TransactionOrder::for_transaction(&tx1, U256::zero()); + assert!(set.insert(tx1.sender(), tx1.nonce(), order1.clone()).is_none()); + assert_eq!(set.gas_price_entry_limit(), 2.into()); + } + #[test] fn should_handle_same_transaction_imported_twice_with_different_state_nonces() { // given let mut txq = TransactionQueue::new(); - let (tx, tx2) = new_similar_txs(); - let prev_nonce = |a: &Address| AccountDetails{ nonce: default_nonce(a).nonce - U256::one(), balance: + let (tx, tx2) = new_similar_tx_pair(); + let prev_nonce = |a: &Address| AccountDetails{ nonce: default_account_details(a).nonce - U256::one(), balance: !U256::zero() }; // First insert one transaction to future @@ -958,7 +1101,7 @@ mod test { assert_eq!(txq.status().future, 1); // now import second transaction to current - let res = txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External); + let res = txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External); // and then there should be only one transaction in current (the one with higher gas_price) assert_eq!(res.unwrap(), TransactionImportResult::Current); @@ -974,10 +1117,10 @@ mod test { fn should_import_tx() { // given let mut txq = TransactionQueue::new(); - let tx = new_tx(); + let tx = new_tx_default(); // when - let res = txq.add(tx, &default_nonce, TransactionOrigin::External); + let res = txq.add(tx, &default_account_details, TransactionOrigin::External); // then assert_eq!(res.unwrap(), TransactionImportResult::Current); @@ -1003,13 +1146,13 @@ mod test { fn should_not_import_transaction_above_gas_limit() { // given let mut txq = TransactionQueue::new(); - let tx = new_tx(); + let tx = new_tx_default(); let gas = tx.gas; let limit = gas / U256::from(2); txq.set_gas_limit(limit); // when - let res = txq.add(tx, &default_nonce, TransactionOrigin::External); + let res = txq.add(tx, &default_account_details, TransactionOrigin::External); // then assert_eq!(unwrap_tx_err(res), TransactionError::GasLimitExceeded { @@ -1026,9 +1169,9 @@ mod test { fn should_drop_transactions_from_senders_without_balance() { // given let mut txq = TransactionQueue::new(); - let tx = new_tx(); + let tx = new_tx_default(); let account = |a: &Address| AccountDetails { - nonce: default_nonce(a).nonce, + nonce: default_account_details(a).nonce, balance: U256::one() }; @@ -1049,11 +1192,11 @@ mod test { fn should_not_import_transaction_below_min_gas_price_threshold_if_external() { // given let mut txq = TransactionQueue::new(); - let tx = new_tx(); + let tx = new_tx_default(); txq.set_minimal_gas_price(tx.gas_price + U256::one()); // when - let res = txq.add(tx, &default_nonce, TransactionOrigin::External); + let res = txq.add(tx, &default_account_details, TransactionOrigin::External); // then assert_eq!(unwrap_tx_err(res), TransactionError::InsufficientGasPrice { @@ -1069,11 +1212,11 @@ mod test { fn should_import_transaction_below_min_gas_price_threshold_if_local() { // given let mut txq = TransactionQueue::new(); - let tx = new_tx(); + let tx = new_tx_default(); txq.set_minimal_gas_price(tx.gas_price + U256::one()); // when - let res = txq.add(tx, &default_nonce, TransactionOrigin::Local); + let res = txq.add(tx, &default_account_details, TransactionOrigin::Local); // then assert_eq!(res.unwrap(), TransactionImportResult::Current); @@ -1086,7 +1229,7 @@ mod test { fn should_reject_incorectly_signed_transaction() { // given let mut txq = TransactionQueue::new(); - let tx = new_unsigned_tx(U256::from(123)); + let tx = new_unsigned_tx(123.into(), 1.into()); let stx = { let mut s = RlpStream::new_list(9); s.append(&tx.nonce); @@ -1101,7 +1244,7 @@ mod test { decode(s.as_raw()) }; // when - let res = txq.add(stx, &default_nonce, TransactionOrigin::External); + let res = txq.add(stx, &default_account_details, TransactionOrigin::External); // then assert!(res.is_err()); @@ -1112,11 +1255,11 @@ mod test { // given let mut txq = TransactionQueue::new(); - let (tx, tx2) = new_txs(U256::from(1)); + let (tx, tx2) = new_tx_pair_default(1.into(), 0.into()); // when - txq.add(tx.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); // then let top = txq.top_transactions(); @@ -1129,15 +1272,15 @@ mod test { fn should_prioritize_local_transactions_within_same_nonce_height() { // given let mut txq = TransactionQueue::new(); - let tx = new_tx(); + let tx = new_tx_default(); // the second one has same nonce but higher `gas_price` - let (_, tx2) = new_similar_txs(); + let (_, tx2) = new_similar_tx_pair(); // when // first insert the one with higher gas price - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); // then the one with lower gas price, but local - txq.add(tx.clone(), &default_nonce, TransactionOrigin::Local).unwrap(); + txq.add(tx.clone(), &default_account_details, TransactionOrigin::Local).unwrap(); // then let top = txq.top_transactions(); @@ -1150,11 +1293,11 @@ mod test { fn should_not_prioritize_local_transactions_with_different_nonce_height() { // given let mut txq = TransactionQueue::new(); - let (tx, tx2) = new_txs(U256::from(1)); + let (tx, tx2) = new_tx_pair_default(1.into(), 0.into()); // when - txq.add(tx.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::Local).unwrap(); + txq.add(tx.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::Local).unwrap(); // then let top = txq.top_transactions(); @@ -1168,11 +1311,11 @@ mod test { // given let mut txq = TransactionQueue::new(); - let (tx, tx2) = new_txs(U256::from(1)); + let (tx, tx2) = new_tx_pair_default(1.into(), 0.into()); // when - txq.add(tx.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); // then let top = txq.pending_hashes(); @@ -1186,11 +1329,11 @@ mod test { // given let mut txq = TransactionQueue::new(); - let (tx, tx2) = new_txs(U256::from(2)); + let (tx, tx2) = new_tx_pair_default(2.into(), 0.into()); // when - let res1 = txq.add(tx.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - let res2 = txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + let res1 = txq.add(tx.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + let res2 = txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); // then assert_eq!(res1, TransactionImportResult::Current); @@ -1206,13 +1349,13 @@ mod test { #[test] fn should_correctly_update_futures_when_removing() { // given - let prev_nonce = |a: &Address| AccountDetails{ nonce: default_nonce(a).nonce - U256::one(), balance: + let prev_nonce = |a: &Address| AccountDetails{ nonce: default_account_details(a).nonce - U256::one(), balance: !U256::zero() }; - let next2_nonce = default_nonce_val() + U256::from(3); + let next2_nonce = default_nonce() + U256::from(3); let mut txq = TransactionQueue::new(); - let (tx, tx2) = new_txs(U256::from(1)); + let (tx, tx2) = new_tx_pair_default(1.into(), 0.into()); txq.add(tx.clone(), &prev_nonce, TransactionOrigin::External).unwrap(); txq.add(tx2.clone(), &prev_nonce, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().future, 2); @@ -1232,17 +1375,17 @@ mod test { let mut txq = TransactionQueue::new(); let kp = KeyPair::create().unwrap(); let secret = kp.secret(); - let tx = new_unsigned_tx(U256::from(123)).sign(secret); - let tx1 = new_unsigned_tx(U256::from(124)).sign(secret); - let tx2 = new_unsigned_tx(U256::from(125)).sign(secret); + let tx = new_unsigned_tx(123.into(), 1.into()).sign(secret); + let tx1 = new_unsigned_tx(124.into(), 1.into()).sign(secret); + let tx2 = new_unsigned_tx(125.into(), 1.into()).sign(secret); - txq.add(tx, &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx, &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().pending, 1); - txq.add(tx2, &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx2, &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().future, 1); // when - txq.add(tx1, &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx1, &default_account_details, TransactionOrigin::External).unwrap(); // then let stats = txq.status(); @@ -1254,9 +1397,9 @@ mod test { fn should_remove_transaction() { // given let mut txq2 = TransactionQueue::new(); - let (tx, tx2) = new_txs(U256::from(3)); - txq2.add(tx.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - txq2.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + let (tx, tx2) = new_tx_pair_default(3.into(), 0.into()); + txq2.add(tx.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + txq2.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq2.status().pending, 1); assert_eq!(txq2.status().future, 1); @@ -1275,16 +1418,16 @@ mod test { fn should_move_transactions_to_future_if_gap_introduced() { // given let mut txq = TransactionQueue::new(); - let (tx, tx2) = new_txs(U256::from(1)); - let tx3 = new_tx(); - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + let (tx, tx2) = new_tx_pair_default(1.into(), 0.into()); + let tx3 = new_tx_default(); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().future, 1); - txq.add(tx3.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx3.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx.clone(), &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().pending, 3); // when - txq.remove_invalid(&tx.hash(), &default_nonce); + txq.remove_invalid(&tx.hash(), &default_account_details); // then let stats = txq.status(); @@ -1296,11 +1439,11 @@ mod test { fn should_clear_queue() { // given let mut txq = TransactionQueue::new(); - let (tx, tx2) = new_txs(U256::one()); + let (tx, tx2) = new_tx_pair_default(1.into(), 0.into()); // add - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx.clone(), &default_account_details, TransactionOrigin::External).unwrap(); let stats = txq.status(); assert_eq!(stats.pending, 2); @@ -1316,60 +1459,38 @@ mod test { fn should_drop_old_transactions_when_hitting_the_limit() { // given let mut txq = TransactionQueue::with_limits(1, !U256::zero()); - let (tx, tx2) = new_txs(U256::one()); + let (tx, tx2) = new_tx_pair_default(1.into(), 0.into()); let sender = tx.sender().unwrap(); let nonce = tx.nonce; - txq.add(tx.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx.clone(), &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().pending, 1); // when - let res = txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External); + let res = txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External); // then let t = txq.top_transactions(); - assert_eq!(unwrap_tx_err(res), TransactionError::LimitReached); + assert_eq!(unwrap_tx_err(res), TransactionError::InsufficientGasPrice { minimal: 2.into(), got: 1.into() }); assert_eq!(txq.status().pending, 1); assert_eq!(t.len(), 1); assert_eq!(t[0], tx); assert_eq!(txq.last_nonce(&sender), Some(nonce)); } - #[test] - fn should_return_correct_nonces_when_dropped_because_of_limit() { - // given - let mut txq = TransactionQueue::with_limits(2, !U256::zero()); - let tx = new_tx(); - let (tx1, tx2) = new_txs(U256::one()); - let sender = tx1.sender().unwrap(); - let nonce = tx1.nonce; - txq.add(tx1.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - assert_eq!(txq.status().pending, 2); - assert_eq!(txq.last_nonce(&sender), Some(nonce + U256::one())); - - // when - let res = txq.add(tx.clone(), &default_nonce, TransactionOrigin::External); - - // then - assert_eq!(res.unwrap(), TransactionImportResult::Current); - assert_eq!(txq.status().pending, 2); - assert_eq!(txq.last_nonce(&sender), Some(nonce)); - } - #[test] fn should_limit_future_transactions() { let mut txq = TransactionQueue::with_limits(1, !U256::zero()); txq.current.set_limit(10); - let (tx1, tx2) = new_txs_with_gas_price_diff(U256::from(4), U256::from(1)); - let (tx3, tx4) = new_txs_with_gas_price_diff(U256::from(4), U256::from(2)); - txq.add(tx1.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx3.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + let (tx1, tx2) = new_tx_pair_default(4.into(), 1.into()); + let (tx3, tx4) = new_tx_pair_default(4.into(), 2.into()); + txq.add(tx1.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx3.clone(), &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().pending, 2); // when - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().future, 1); - txq.add(tx4.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx4.clone(), &default_account_details, TransactionOrigin::External).unwrap(); // then assert_eq!(txq.status().future, 1); @@ -1378,7 +1499,7 @@ mod test { #[test] fn should_drop_transactions_with_old_nonces() { let mut txq = TransactionQueue::new(); - let tx = new_tx(); + let tx = new_tx_default(); let last_nonce = tx.nonce + U256::one(); let fetch_last_nonce = |_a: &Address| AccountDetails{ nonce: last_nonce, balance: !U256::zero() }; @@ -1395,11 +1516,11 @@ mod test { #[test] fn should_not_insert_same_transaction_twice() { // given - let nonce = |a: &Address| AccountDetails { nonce: default_nonce(a).nonce + U256::one(), + let nonce = |a: &Address| AccountDetails { nonce: default_account_details(a).nonce + U256::one(), balance: !U256::zero() }; let mut txq = TransactionQueue::new(); - let (_tx1, tx2) = new_txs(U256::from(1)); - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + let (_tx1, tx2) = new_tx_pair_default(1.into(), 0.into()); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().future, 1); assert_eq!(txq.status().pending, 0); @@ -1417,16 +1538,16 @@ mod test { fn should_accept_same_transaction_twice_if_removed() { // given let mut txq = TransactionQueue::new(); - let (tx1, tx2) = new_txs(U256::from(1)); - txq.add(tx1.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + let (tx1, tx2) = new_tx_pair_default(1.into(), 0.into()); + txq.add(tx1.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().pending, 2); // when - txq.remove_invalid(&tx1.hash(), &default_nonce); + txq.remove_invalid(&tx1.hash(), &default_account_details); assert_eq!(txq.status().pending, 0); assert_eq!(txq.status().future, 1); - txq.add(tx1.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx1.clone(), &default_account_details, TransactionOrigin::External).unwrap(); // then let stats = txq.status(); @@ -1438,17 +1559,17 @@ mod test { fn should_not_move_to_future_if_state_nonce_is_higher() { // given let mut txq = TransactionQueue::new(); - let (tx, tx2) = new_txs(U256::from(1)); - let tx3 = new_tx(); - txq.add(tx2.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + let (tx, tx2) = new_tx_pair_default(1.into(), 0.into()); + let tx3 = new_tx_default(); + txq.add(tx2.clone(), &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().future, 1); - txq.add(tx3.clone(), &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx.clone(), &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx3.clone(), &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx.clone(), &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().pending, 3); // when let sender = tx.sender().unwrap(); - txq.remove_all(sender, default_nonce_val() + U256::one()); + txq.remove_all(sender, default_nonce() + U256::one()); // then let stats = txq.status(); @@ -1462,7 +1583,7 @@ mod test { // given let mut txq = TransactionQueue::new(); let keypair = KeyPair::create().unwrap(); - let tx = new_unsigned_tx(U256::from(123)).sign(keypair.secret()); + let tx = new_unsigned_tx(123.into(), 1.into()).sign(keypair.secret()); let tx2 = { let mut tx2 = (*tx).clone(); tx2.gas_price = U256::from(200); @@ -1470,8 +1591,8 @@ mod test { }; // when - txq.add(tx, &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx2, &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx, &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx2, &default_account_details, TransactionOrigin::External).unwrap(); // then let stats = txq.status(); @@ -1485,7 +1606,7 @@ mod test { // given let mut txq = TransactionQueue::new(); let keypair = KeyPair::create().unwrap(); - let tx0 = new_unsigned_tx(U256::from(123)).sign(keypair.secret()); + let tx0 = new_unsigned_tx(123.into(), 1.into()).sign(keypair.secret()); let tx1 = { let mut tx1 = (*tx0).clone(); tx1.nonce = U256::from(124); @@ -1498,10 +1619,10 @@ mod test { }; // when - txq.add(tx1, &default_nonce, TransactionOrigin::External).unwrap(); - txq.add(tx2, &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx1, &default_account_details, TransactionOrigin::External).unwrap(); + txq.add(tx2, &default_account_details, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().future, 1); - txq.add(tx0, &default_nonce, TransactionOrigin::External).unwrap(); + txq.add(tx0, &default_account_details, TransactionOrigin::External).unwrap(); // then let stats = txq.status(); @@ -1513,12 +1634,12 @@ mod test { #[test] fn should_recalculate_height_when_removing_from_future() { // given - let previous_nonce = |a: &Address| AccountDetails{ nonce: default_nonce(a).nonce - U256::one(), balance: + let previous_nonce = |a: &Address| AccountDetails{ nonce: default_account_details(a).nonce - U256::one(), balance: !U256::zero() }; - let next_nonce = |a: &Address| AccountDetails{ nonce: default_nonce(a).nonce + U256::one(), balance: + let next_nonce = |a: &Address| AccountDetails{ nonce: default_account_details(a).nonce + U256::one(), balance: !U256::zero() }; let mut txq = TransactionQueue::new(); - let (tx1, tx2) = new_txs(U256::one()); + let (tx1, tx2) = new_tx_pair_default(1.into(), 0.into()); txq.add(tx1.clone(), &previous_nonce, TransactionOrigin::External).unwrap(); txq.add(tx2, &previous_nonce, TransactionOrigin::External).unwrap(); assert_eq!(txq.status().future, 2); @@ -1545,7 +1666,7 @@ mod test { fn should_return_correct_nonce_when_transactions_from_given_address_exist() { // given let mut txq = TransactionQueue::new(); - let tx = new_tx(); + let tx = new_tx_default(); let from = tx.sender().unwrap(); let nonce = tx.nonce; let details = |_a: &Address| AccountDetails { nonce: nonce, balance: !U256::zero() }; @@ -1561,7 +1682,7 @@ mod test { fn should_remove_old_transaction_even_if_newer_transaction_was_not_known() { // given let mut txq = TransactionQueue::new(); - let (tx1, tx2) = new_txs(U256::one()); + let (tx1, tx2) = new_tx_pair_default(1.into(), 0.into()); let (nonce1, nonce2) = (tx1.nonce, tx2.nonce); let details1 = |_a: &Address| AccountDetails { nonce: nonce1, balance: !U256::zero() }; @@ -1579,7 +1700,7 @@ mod test { fn should_return_valid_last_nonce_after_remove_all() { // given let mut txq = TransactionQueue::new(); - let (tx1, tx2) = new_txs(U256::from(4)); + let (tx1, tx2) = new_tx_pair_default(4.into(), 0.into()); let sender = tx1.sender().unwrap(); let (nonce1, nonce2) = (tx1.nonce, tx2.nonce); let details1 = |_a: &Address| AccountDetails { nonce: nonce1, balance: !U256::zero() }; @@ -1603,13 +1724,13 @@ mod test { fn should_return_true_if_there_is_local_transaction_pending() { // given let mut txq = TransactionQueue::new(); - let (tx1, tx2) = new_txs(U256::from(1)); + let (tx1, tx2) = new_tx_pair_default(1.into(), 0.into()); assert_eq!(txq.has_local_pending_transactions(), false); // when - assert_eq!(txq.add(tx1, &default_nonce, TransactionOrigin::External).unwrap(), TransactionImportResult::Current); + assert_eq!(txq.add(tx1, &default_account_details, TransactionOrigin::External).unwrap(), TransactionImportResult::Current); assert_eq!(txq.has_local_pending_transactions(), false); - assert_eq!(txq.add(tx2, &default_nonce, TransactionOrigin::Local).unwrap(), TransactionImportResult::Current); + assert_eq!(txq.add(tx2, &default_account_details, TransactionOrigin::Local).unwrap(), TransactionImportResult::Current); // then assert_eq!(txq.has_local_pending_transactions(), true); @@ -1619,9 +1740,9 @@ mod test { fn should_keep_right_order_in_future() { // given let mut txq = TransactionQueue::with_limits(1, !U256::zero()); - let (tx1, tx2) = new_txs(U256::from(1)); - let prev_nonce = |a: &Address| AccountDetails { nonce: default_nonce(a).nonce - U256::one(), balance: - default_nonce(a).balance }; + let (tx1, tx2) = new_tx_pair_default(1.into(), 0.into()); + let prev_nonce = |a: &Address| AccountDetails { nonce: default_account_details(a).nonce - U256::one(), balance: + default_account_details(a).balance }; // when assert_eq!(txq.add(tx2, &prev_nonce, TransactionOrigin::External).unwrap(), TransactionImportResult::Future); @@ -1639,25 +1760,24 @@ mod test { let (tx1, tx2, tx2_2, tx3) = { let keypair = KeyPair::create().unwrap(); let secret = &keypair.secret(); - let nonce = U256::from(123); - let tx = new_unsigned_tx(nonce); - let tx2 = new_unsigned_tx(nonce + 1.into()); - let mut tx2_2 = new_unsigned_tx(nonce + 1.into()); - tx2_2.gas_price = U256::from(5); - let tx3 = new_unsigned_tx(nonce + 2.into()); + let nonce = 123.into(); + let tx = new_unsigned_tx(nonce, 1.into()); + let tx2 = new_unsigned_tx(nonce + 1.into(), 1.into()); + let tx2_2 = new_unsigned_tx(nonce + 1.into(), 5.into()); + let tx3 = new_unsigned_tx(nonce + 2.into(), 1.into()); (tx.sign(secret), tx2.sign(secret), tx2_2.sign(secret), tx3.sign(secret)) }; let sender = tx1.sender().unwrap(); - txq.add(tx1, &default_nonce, TransactionOrigin::Local).unwrap(); - txq.add(tx2, &default_nonce, TransactionOrigin::Local).unwrap(); - txq.add(tx3, &default_nonce, TransactionOrigin::Local).unwrap(); + txq.add(tx1, &default_account_details, TransactionOrigin::Local).unwrap(); + txq.add(tx2, &default_account_details, TransactionOrigin::Local).unwrap(); + txq.add(tx3, &default_account_details, TransactionOrigin::Local).unwrap(); assert_eq!(txq.future.by_priority.len(), 0); assert_eq!(txq.current.by_priority.len(), 3); // when - let res = txq.add(tx2_2, &default_nonce, TransactionOrigin::Local); + let res = txq.add(tx2_2, &default_account_details, TransactionOrigin::Local); // then assert_eq!(txq.last_nonce(&sender).unwrap(), 125.into()); diff --git a/parity/cli.rs b/parity/cli.rs index cb354aff5..5f7448772 100644 --- a/parity/cli.rs +++ b/parity/cli.rs @@ -170,7 +170,7 @@ Sealing/Mining Options: lenient - Same as strict when mining, and cheap when not [default: cheap]. --usd-per-tx USD Amount of USD to be paid for a basic transaction - [default: 0.005]. The minimum gas price is set + [default: 0]. The minimum gas price is set accordingly. --usd-per-eth SOURCE USD value of a single ETH. SOURCE may be either an amount in USD, a web service or 'auto' to use each diff --git a/parity/params.rs b/parity/params.rs index bc58e455d..54a680414 100644 --- a/parity/params.rs +++ b/parity/params.rs @@ -179,7 +179,7 @@ pub enum GasPricerConfig { impl Default for GasPricerConfig { fn default() -> Self { GasPricerConfig::Calibrated { - usd_per_tx: 0.005, + usd_per_tx: 0f32, recalibration_period: Duration::from_secs(3600), } }