diff --git a/ethcore/src/miner/transaction_queue.rs b/ethcore/src/miner/transaction_queue.rs index fb00a8aa8..4b1e8abec 100644 --- a/ethcore/src/miner/transaction_queue.rs +++ b/ethcore/src/miner/transaction_queue.rs @@ -81,6 +81,7 @@ //! - It removes all transactions (either from `current` or `future`) with nonce < client nonce //! - It moves matching `future` transactions to `current` +use std::ops::Deref; use std::cmp::Ordering; use std::cmp; use std::collections::{HashSet, HashMap, BTreeSet, BTreeMap}; @@ -215,7 +216,48 @@ impl VerifiedTransaction { } fn sender(&self) -> Address { - self.transaction.sender().unwrap() + self.transaction.sender().expect("Sender is verified in new; qed") + } +} + +#[derive(Debug, Default)] +struct GasPriceQueue { + backing: BTreeMap>, +} + +impl GasPriceQueue { + /// Insert an item into a BTreeMap/HashSet "multimap". + pub fn insert(&mut self, gas_price: U256, hash: H256) -> bool { + self.backing.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. + pub fn remove(&mut self, gas_price: &U256, hash: &H256) -> bool { + if let Some(mut hashes) = self.backing.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 hash != hashes.iter().next().expect("We know there is only one element in collection, tested above; qed") { + // 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. + self.backing.remove(gas_price).is_some() + } +} + +impl Deref for GasPriceQueue { + type Target=BTreeMap>; + + fn deref(&self) -> &Self::Target { + &self.backing } } @@ -227,7 +269,7 @@ impl VerifiedTransaction { struct TransactionSet { by_priority: BTreeSet, by_address: Table, - by_gas_price: BTreeMap>, + by_gas_price: GasPriceQueue, limit: usize, } @@ -245,12 +287,12 @@ impl TransactionSet { // 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), + assert!(self.by_gas_price.remove(&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()); + self.by_gas_price.insert(order_gas_price, order_hash); + assert_eq!(self.by_priority.len(), self.by_address.len()); + assert_eq!(self.by_gas_price.values().map(|v| v.len()).fold(0, |a, b| a + b), self.by_address.len()); by_address_replaced } @@ -263,6 +305,7 @@ impl TransactionSet { if len <= self.limit { return None; } + let to_drop : Vec<(Address, U256)> = { self.by_priority .iter() @@ -290,13 +333,16 @@ 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), + assert!(self.by_gas_price.remove(&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!(self.by_priority.remove(&tx_order), + "hash is in `by_address`; all transactions' gas_prices in `by_address` must be in `by_priority`; qed"); assert_eq!(self.by_priority.len(), self.by_address.len()); + assert_eq!(self.by_gas_price.values().map(|v| v.len()).fold(0, |a, b| a + b), self.by_address.len()); return Some(tx_order); } assert_eq!(self.by_priority.len(), self.by_address.len()); + assert_eq!(self.by_gas_price.values().map(|v| v.len()).fold(0, |a, b| a + b), self.by_address.len()); None } @@ -304,7 +350,7 @@ impl TransactionSet { fn clear(&mut self) { self.by_priority.clear(); self.by_address.clear(); - self.by_gas_price.clear(); + self.by_gas_price.backing.clear(); } /// Sets new limit for number of transactions in this `TransactionSet`. @@ -321,32 +367,6 @@ impl TransactionSet { _ => 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)] @@ -588,7 +608,7 @@ impl TransactionQueue { return; } - let transaction = transaction.unwrap(); + let transaction = transaction.expect("None is tested in early-exit condition above; qed"); let sender = transaction.sender(); let nonce = transaction.nonce(); let current_nonce = fetch_account(&sender).nonce; @@ -623,7 +643,7 @@ impl TransactionQueue { None => vec![], }; for k in all_nonces_from_sender { - let order = self.future.drop(sender, &k).unwrap(); + let order = self.future.drop(sender, &k).expect("iterating over a collection that has been retrieved above; qed"); if k >= current_nonce { self.future.insert(*sender, k, order.update_height(k, current_nonce)); } else { @@ -644,7 +664,8 @@ impl TransactionQueue { for k in all_nonces_from_sender { // Goes to future or is removed - let order = self.current.drop(sender, &k).unwrap(); + let order = self.current.drop(sender, &k).expect("iterating over a collection that has been retrieved above; + qed"); if k >= current_nonce { self.future.insert(*sender, k, order.update_height(k, current_nonce)); } else { @@ -704,10 +725,11 @@ impl TransactionQueue { if let None = by_nonce { return; } - let mut by_nonce = by_nonce.unwrap(); + let mut by_nonce = by_nonce.expect("None is tested in early-exit condition above; qed"); while let Some(order) = by_nonce.remove(¤t_nonce) { - // remove also from priority and hash + // remove also from priority and gas_price self.future.by_priority.remove(&order); + self.future.by_gas_price.remove(&order.gas_price, &order.hash); // Put to current let order = order.update_height(current_nonce, first_nonce); self.current.insert(address, current_nonce, order); @@ -1395,6 +1417,9 @@ mod test { let stats = txq.status(); assert_eq!(stats.pending, 3); assert_eq!(stats.future, 0); + assert_eq!(txq.future.by_priority.len(), 0); + assert_eq!(txq.future.by_address.len(), 0); + assert_eq!(txq.future.by_gas_price.len(), 0); } #[test]