Beta: 1.11.6 backports (#9015)

* parity-version: bump beta to 1.11.6

* scripts: remove md5 checksums (#8884)

* Add support for --chain tobalaba

* Convert indents to tabs :)

* Fixes for misbehavior reporting in AuthorityRound (#8998)

* aura: only report after checking for repeated skipped primaries

* aura: refactor duplicate code for getting epoch validator set

* aura: verify_external: report on validator set contract instance

* aura: use correct validator set epoch number when reporting

* aura: use epoch set when verifying blocks

* aura: report skipped primaries when generating seal

* aura: handle immediate transitions

* aura: don't report skipped steps from genesis to first block

* aura: fix reporting test

* aura: refactor duplicate code to handle immediate_transitions

* aura: let reporting fail on verify_block_basic

* aura: add comment about possible failure of reporting

* Only return error log for rustls (#9025)

* Transaction Pool improvements (#8470)

* Don't use ethereum_types in transaction pool.

* Hide internal insertion_id.

* Fix tests.

* Review grumbles.

* Improve should_replace on NonceAndGasPrice (#8980)

* Additional tests for NonceAndGasPrice::should_replace.

* Fix should_replace in the distinct sender case.

* Use natural priority ordering to simplify should_replace.

* Minimal effective gas price in the queue (#8934)

* Minimal effective gas price.

* Fix naming, add test

* Fix minimal entry score and add test.

* Fix worst_transaction.

* Remove effective gas price threshold.

* Don't leak gas_price decisions out of Scoring.

* Never drop local transactions from different senders. (#9002)

* Recently rejected cache for transaction queue (#9005)

* Store recently rejected transactions.

* Don't cache AlreadyImported rejections.

* Make the size of transaction verification queue dependent on pool size.

* Add a test for recently rejected.

* Fix logging for recently rejected.

* Make rejection cache smaller.

* obsolete test removed

* obsolete test removed

* Construct cache with_capacity.

* Optimize pending transactions filter (#9026)

* rpc: return unordered transactions in pending transactions filter

* ethcore: use LruCache for nonce cache

Only clear the nonce cache when a block is retracted

* Revert "ethcore: use LruCache for nonce cache"

This reverts commit b382c19abdb9985be1724c3b8cde83906da07d68.

* Use only cached nonces when computing pending hashes.

* Give filters their own locks, so that they don't block one another.

* Fix pending transaction count if not sealing.

* Clear cache only when block is enacted.

* Fix RPC tests.

* Address review comments.

* A last bunch of txqueue performance optimizations (#9024)

* Clear cache only when block is enacted.

* Add tracing for cull.

* Cull split.

* Cull after creating pending block.

* Add constant, remove sync::read tracing.

* Reset debug.

* Remove excessive tracing.

* Use struct for NonceCache.

* Fix build

* Remove warnings.

* Fix build again.

* miner: add missing macro use for trace_time

* ci: remove md5 merge leftovers

transaction-pool/src/error.rs

@@ -14,24 +14,26 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
-use ethereum_types::H256;
+/// Error chain doesn't let us have generic types.
+/// So the hashes are converted to debug strings for easy display.
+type Hash = String;
 
 error_chain! {
 	errors {
 		/// Transaction is already imported
-		AlreadyImported(hash: H256) {
+		AlreadyImported(hash: Hash) {
 			description("transaction is already in the pool"),
-			display("[{:?}] already imported", hash)
+			display("[{}] already imported", hash)
 		}
 		/// Transaction is too cheap to enter the queue
-		TooCheapToEnter(hash: H256, min_score: String) {
+		TooCheapToEnter(hash: Hash, min_score: String) {
 			description("the pool is full and transaction is too cheap to replace any transaction"),
-			display("[{:?}] too cheap to enter the pool. Min score: {}", hash, min_score)
+			display("[{}] too cheap to enter the pool. Min score: {}", hash, min_score)
 		}
 		/// Transaction is too cheap to replace existing transaction that occupies the same slot.
-		TooCheapToReplace(old_hash: H256, hash: H256) {
+		TooCheapToReplace(old_hash: Hash, hash: Hash) {
 			description("transaction is too cheap to replace existing transaction in the pool"),
-			display("[{:?}] too cheap to replace: {:?}", hash, old_hash)
+			display("[{}] too cheap to replace: {}", hash, old_hash)
 		}
 	}
 }

transaction-pool/src/lib.rs

@@ -69,14 +69,15 @@
 #![warn(missing_docs)]
 
 extern crate smallvec;
-extern crate ethereum_types;
+extern crate trace_time;
 
 #[macro_use]
 extern crate error_chain;
 #[macro_use]
 extern crate log;
 
-extern crate trace_time;
+#[cfg(test)]
+extern crate ethereum_types;
 
 #[cfg(test)]
 mod tests;
@@ -95,27 +96,29 @@ pub mod scoring;
 pub use self::error::{Error, ErrorKind};
 pub use self::listener::{Listener, NoopListener};
 pub use self::options::Options;
-pub use self::pool::{Pool, PendingIterator};
+pub use self::pool::{Pool, PendingIterator, Transaction};
 pub use self::ready::{Ready, Readiness};
 pub use self::scoring::Scoring;
 pub use self::status::{LightStatus, Status};
 pub use self::verifier::Verifier;
 
 use std::fmt;
-
-use ethereum_types::{H256, Address};
+use std::hash::Hash;
 
 /// Already verified transaction that can be safely queued.
 pub trait VerifiedTransaction: fmt::Debug {
+	/// Transaction hash type.
+	type Hash: fmt::Debug + fmt::LowerHex + Eq + Clone + Hash;
+
+	/// Transaction sender type.
+	type Sender: fmt::Debug + Eq + Clone + Hash;
+
 	/// Transaction hash
-	fn hash(&self) -> &H256;
+	fn hash(&self) -> &Self::Hash;
 
 	/// Memory usage
 	fn mem_usage(&self) -> usize;
 
 	/// Transaction sender
-	fn sender(&self) -> &Address;
-
-	/// Unique index of insertion (lower = older).
-	fn insertion_id(&self) -> u64;
+	fn sender(&self) -> &Self::Sender;
 }

transaction-pool/src/pool.rs

@@ -17,33 +17,61 @@
 use std::sync::Arc;
 use std::collections::{HashMap, BTreeSet};
 
-use ethereum_types::{H160, H256};
-
 use error;
 use listener::{Listener, NoopListener};
 use options::Options;
 use ready::{Ready, Readiness};
-use scoring::{Scoring, ScoreWithRef};
+use scoring::{self, Scoring, ScoreWithRef};
 use status::{LightStatus, Status};
 use transactions::{AddResult, Transactions};
 
 use {VerifiedTransaction};
 
-type Sender = H160;
+/// Internal representation of transaction.
+///
+/// Includes unique insertion id that can be used for scoring explictly,
+/// but internally is used to resolve conflicts in case of equal scoring
+/// (newer transactionsa are preferred).
+#[derive(Debug)]
+pub struct Transaction<T> {
+	/// Sequential id of the transaction
+	pub insertion_id: u64,
+	/// Shared transaction
+	pub transaction: Arc<T>,
+}
+
+impl<T> Clone for Transaction<T> {
+	fn clone(&self) -> Self {
+		Transaction {
+			insertion_id: self.insertion_id,
+			transaction: self.transaction.clone(),
+		}
+	}
+}
+
+impl<T> ::std::ops::Deref for Transaction<T> {
+	type Target = Arc<T>;
+
+	fn deref(&self) -> &Self::Target {
+		&self.transaction
+	}
+}
 
 /// A transaction pool.
 #[derive(Debug)]
-pub struct Pool<T, S: Scoring<T>, L = NoopListener> {
+pub struct Pool<T: VerifiedTransaction, S: Scoring<T>, L = NoopListener> {
 	listener: L,
 	scoring: S,
 	options: Options,
 	mem_usage: usize,
 
-	transactions: HashMap<Sender, Transactions<T, S>>,
-	by_hash: HashMap<H256, Arc<T>>,
+	transactions: HashMap<T::Sender, Transactions<T, S>>,
+	by_hash: HashMap<T::Hash, Transaction<T>>,
 
 	best_transactions: BTreeSet<ScoreWithRef<T, S::Score>>,
 	worst_transactions: BTreeSet<ScoreWithRef<T, S::Score>>,
+
+	insertion_id: u64,
 }
 
 impl<T: VerifiedTransaction, S: Scoring<T> + Default> Default for Pool<T, S> {
@@ -89,6 +117,7 @@ impl<T, S, L> Pool<T, S, L> where
 			by_hash,
 			best_transactions: Default::default(),
 			worst_transactions: Default::default(),
+			insertion_id: 0,
 		}
 
 	}
@@ -104,41 +133,52 @@ impl<T, S, L> Pool<T, S, L> where
 	/// If any limit is reached the transaction with the lowest `Score` is evicted to make room.
 	///
 	/// The `Listener` will be informed on any drops or rejections.
-	pub fn import(&mut self, mut transaction: T) -> error::Result<Arc<T>> {
+	pub fn import(&mut self, transaction: T) -> error::Result<Arc<T>> {
 		let mem_usage = transaction.mem_usage();
 
-		ensure!(!self.by_hash.contains_key(transaction.hash()), error::ErrorKind::AlreadyImported(*transaction.hash()));
+		ensure!(!self.by_hash.contains_key(transaction.hash()), error::ErrorKind::AlreadyImported(format!("{:?}", transaction.hash())));
+
+		self.insertion_id += 1;
+		let transaction = Transaction {
+			insertion_id: self.insertion_id,
+			transaction: Arc::new(transaction),
+		};
 
 		// TODO [ToDr] Most likely move this after the transaction is inserted.
 		// Avoid using should_replace, but rather use scoring for that.
 		{
 			let remove_worst = |s: &mut Self, transaction| {
-				match s.remove_worst(&transaction) {
+				match s.remove_worst(transaction) {
 					Err(err) => {
-						s.listener.rejected(&Arc::new(transaction), err.kind());
+						s.listener.rejected(transaction, err.kind());
 						Err(err)
 					},
-					Ok(removed) => {
-						s.listener.dropped(&removed, Some(&transaction));
+					Ok(None) => Ok(false),
+					Ok(Some(removed)) => {
+						s.listener.dropped(&removed, Some(transaction));
 						s.finalize_remove(removed.hash());
-						Ok(transaction)
+						Ok(true)
 					},
 				}
 			};
 
 			while self.by_hash.len() + 1 > self.options.max_count {
 				trace!("Count limit reached: {} > {}", self.by_hash.len() + 1, self.options.max_count);
-				transaction = remove_worst(self, transaction)?;
+				if !remove_worst(self, &transaction)? {
+					break;
+				}
 			}
 
 			while self.mem_usage + mem_usage > self.options.max_mem_usage {
 				trace!("Mem limit reached: {} > {}", self.mem_usage + mem_usage, self.options.max_mem_usage);
-				transaction = remove_worst(self, transaction)?;
+				if !remove_worst(self, &transaction)? {
+					break;
+				}
 			}
 		}
 
 		let (result, prev_state, current_state) = {
-			let transactions = self.transactions.entry(*transaction.sender()).or_insert_with(Transactions::default);
+			let transactions = self.transactions.entry(transaction.sender().clone()).or_insert_with(Transactions::default);
 			// get worst and best transactions for comparison
 			let prev = transactions.worst_and_best();
 			let result = transactions.add(transaction, &self.scoring, self.options.max_per_sender);
@@ -153,31 +193,31 @@ impl<T, S, L> Pool<T, S, L> where
 			AddResult::Ok(tx) => {
 				self.listener.added(&tx, None);
 				self.finalize_insert(&tx, None);
-				Ok(tx)
+				Ok(tx.transaction)
 			},
 			AddResult::PushedOut { new, old } |
 			AddResult::Replaced { new, old } => {
 				self.listener.added(&new, Some(&old));
 				self.finalize_insert(&new, Some(&old));
-				Ok(new)
+				Ok(new.transaction)
 			},
 			AddResult::TooCheap { new, old } => {
-				let error = error::ErrorKind::TooCheapToReplace(*old.hash(), *new.hash());
-				self.listener.rejected(&Arc::new(new), &error);
+				let error = error::ErrorKind::TooCheapToReplace(format!("{:x}", old.hash()), format!("{:x}", new.hash()));
+				self.listener.rejected(&new, &error);
 				bail!(error)
 			},
 			AddResult::TooCheapToEnter(new, score) => {
-				let error = error::ErrorKind::TooCheapToEnter(*new.hash(), format!("{:?}", score));
-				self.listener.rejected(&Arc::new(new), &error);
+				let error = error::ErrorKind::TooCheapToEnter(format!("{:x}", new.hash()), format!("{:?}", score));
+				self.listener.rejected(&new, &error);
 				bail!(error)
 			}
 		}
 	}
 
 	/// Updates state of the pool statistics if the transaction was added to a set.
-	fn finalize_insert(&mut self, new: &Arc<T>, old: Option<&Arc<T>>) {
+	fn finalize_insert(&mut self, new: &Transaction<T>, old: Option<&Transaction<T>>) {
 		self.mem_usage += new.mem_usage();
-		self.by_hash.insert(*new.hash(), new.clone());
+		self.by_hash.insert(new.hash().clone(), new.clone());
 
 		if let Some(old) = old {
 			self.finalize_remove(old.hash());
@@ -185,23 +225,23 @@ impl<T, S, L> Pool<T, S, L> where
 	}
 
 	/// Updates the pool statistics if transaction was removed.
-	fn finalize_remove(&mut self, hash: &H256) -> Option<Arc<T>> {
+	fn finalize_remove(&mut self, hash: &T::Hash) -> Option<Arc<T>> {
 		self.by_hash.remove(hash).map(|old| {
-			self.mem_usage -= old.mem_usage();
-			old
+			self.mem_usage -= old.transaction.mem_usage();
+			old.transaction
 		})
 	}
 
 	/// Updates best and worst transactions from a sender.
 	fn update_senders_worst_and_best(
 		&mut self,
-		previous: Option<((S::Score, Arc<T>), (S::Score, Arc<T>))>,
-		current: Option<((S::Score, Arc<T>), (S::Score, Arc<T>))>,
+		previous: Option<((S::Score, Transaction<T>), (S::Score, Transaction<T>))>,
+		current: Option<((S::Score, Transaction<T>), (S::Score, Transaction<T>))>,
 	) {
 		let worst_collection = &mut self.worst_transactions;
 		let best_collection = &mut self.best_transactions;
 
-		let is_same = |a: &(S::Score, Arc<T>), b: &(S::Score, Arc<T>)| {
+		let is_same = |a: &(S::Score, Transaction<T>), b: &(S::Score, Transaction<T>)| {
 			a.0 == b.0 && a.1.hash() == b.1.hash()
 		};
 
@@ -238,32 +278,42 @@ impl<T, S, L> Pool<T, S, L> where
 	}
 
 	/// Attempts to remove the worst transaction from the pool if it's worse than the given one.
-	fn remove_worst(&mut self, transaction: &T) -> error::Result<Arc<T>> {
+	///
+	/// Returns `None` in case we couldn't decide if the transaction should replace the worst transaction or not.
+	/// In such case we will accept the transaction even though it is going to exceed the limit.
+	fn remove_worst(&mut self, transaction: &Transaction<T>) -> error::Result<Option<Transaction<T>>> {
 		let to_remove = match self.worst_transactions.iter().next_back() {
 			// No elements to remove? and the pool is still full?
 			None => {
 				warn!("The pool is full but there are no transactions to remove.");
-				return Err(error::ErrorKind::TooCheapToEnter(*transaction.hash(), "unknown".into()).into());
+				return Err(error::ErrorKind::TooCheapToEnter(format!("{:?}", transaction.hash()), "unknown".into()).into());
 			},
-			Some(old) => if self.scoring.should_replace(&old.transaction, transaction) {
+			Some(old) => match self.scoring.should_replace(&old.transaction, transaction) {
+				// We can't decide which of them should be removed, so accept both.
+				scoring::Choice::InsertNew => None,
 				// New transaction is better than the worst one so we can replace it.
-				old.clone()
-			} else {
+				scoring::Choice::ReplaceOld => Some(old.clone()),
 				// otherwise fail
-				return Err(error::ErrorKind::TooCheapToEnter(*transaction.hash(), format!("{:?}", old.score)).into())
+				scoring::Choice::RejectNew => {
+					return Err(error::ErrorKind::TooCheapToEnter(format!("{:?}", transaction.hash()), format!("{:?}", old.score)).into())
+				},
 			},
 		};
 
-		// Remove from transaction set
-		self.remove_from_set(to_remove.transaction.sender(), |set, scoring| {
-			set.remove(&to_remove.transaction, scoring)
-		});
+		if let Some(to_remove) = to_remove {
+			// Remove from transaction set
+			self.remove_from_set(to_remove.transaction.sender(), |set, scoring| {
+				set.remove(&to_remove.transaction, scoring)
+			});
 
-		Ok(to_remove.transaction)
+			Ok(Some(to_remove.transaction))
+		} else {
+			Ok(None)
+		}
 	}
 
 	/// Removes transaction from sender's transaction `HashMap`.
-	fn remove_from_set<R, F: FnOnce(&mut Transactions<T, S>, &S) -> R>(&mut self, sender: &Sender, f: F) -> Option<R> {
+	fn remove_from_set<R, F: FnOnce(&mut Transactions<T, S>, &S) -> R>(&mut self, sender: &T::Sender, f: F) -> Option<R> {
 		let (prev, next, result) = if let Some(set) = self.transactions.get_mut(sender) {
 			let prev = set.worst_and_best();
 			let result = f(set, &self.scoring);
@@ -286,14 +336,14 @@ impl<T, S, L> Pool<T, S, L> where
 		self.worst_transactions.clear();
 
 		for (_hash, tx) in self.by_hash.drain() {
-			self.listener.dropped(&tx, None)
+			self.listener.dropped(&tx.transaction, None)
 		}
 	}
 
 	/// Removes single transaction from the pool.
 	/// Depending on the `is_invalid` flag the listener
 	/// will either get a `cancelled` or `invalid` notification.
-	pub fn remove(&mut self, hash: &H256, is_invalid: bool) -> Option<Arc<T>> {
+	pub fn remove(&mut self, hash: &T::Hash, is_invalid: bool) -> Option<Arc<T>> {
 		if let Some(tx) = self.finalize_remove(hash) {
 			self.remove_from_set(tx.sender(), |set, scoring| {
 				set.remove(&tx, scoring)
@@ -310,7 +360,7 @@ impl<T, S, L> Pool<T, S, L> where
 	}
 
 	/// Removes all stalled transactions from given sender.
-	fn remove_stalled<R: Ready<T>>(&mut self, sender: &Sender, ready: &mut R) -> usize {
+	fn remove_stalled<R: Ready<T>>(&mut self, sender: &T::Sender, ready: &mut R) -> usize {
 		let removed_from_set = self.remove_from_set(sender, |transactions, scoring| {
 			transactions.cull(ready, scoring)
 		});
@@ -329,7 +379,7 @@ impl<T, S, L> Pool<T, S, L> where
 	}
 
 	/// Removes all stalled transactions from given sender list (or from all senders).
-	pub fn cull<R: Ready<T>>(&mut self, senders: Option<&[Sender]>, mut ready: R) -> usize {
+	pub fn cull<R: Ready<T>>(&mut self, senders: Option<&[T::Sender]>, mut ready: R) -> usize {
 		let mut removed = 0;
 		match senders {
 			Some(senders) => {
@@ -349,13 +399,24 @@ impl<T, S, L> Pool<T, S, L> where
 	}
 
 	/// Returns a transaction if it's part of the pool or `None` otherwise.
-	pub fn find(&self, hash: &H256) -> Option<Arc<T>> {
-		self.by_hash.get(hash).cloned()
+	pub fn find(&self, hash: &T::Hash) -> Option<Arc<T>> {
+		self.by_hash.get(hash).map(|t| t.transaction.clone())
 	}
 
 	/// Returns worst transaction in the queue (if any).
 	pub fn worst_transaction(&self) -> Option<Arc<T>> {
-		self.worst_transactions.iter().next().map(|x| x.transaction.clone())
+		self.worst_transactions.iter().next_back().map(|x| x.transaction.transaction.clone())
+	}
+
+	/// Returns true if the pool is at it's capacity.
+	pub fn is_full(&self) -> bool {
+		self.by_hash.len() >= self.options.max_count
+			|| self.mem_usage >= self.options.max_mem_usage
+	}
+
+	/// Returns senders ordered by priority of their transactions.
+	pub fn senders(&self) -> impl Iterator<Item=&T::Sender> {
+		self.best_transactions.iter().map(|tx| tx.transaction.sender())
 	}
 
 	/// Returns an iterator of pending (ready) transactions.
@@ -368,7 +429,7 @@ impl<T, S, L> Pool<T, S, L> where
 	}
 
 	/// Returns pending (ready) transactions from given sender.
-	pub fn pending_from_sender<R: Ready<T>>(&self, ready: R, sender: &Sender) -> PendingIterator<T, R, S, L> {
+	pub fn pending_from_sender<R: Ready<T>>(&self, ready: R, sender: &T::Sender) -> PendingIterator<T, R, S, L> {
 		let best_transactions = self.transactions.get(sender)
 			.and_then(|transactions| transactions.worst_and_best())
 			.map(|(_, best)| ScoreWithRef::new(best.0, best.1))
@@ -387,7 +448,7 @@ impl<T, S, L> Pool<T, S, L> where
 	}
 
 	/// Update score of transactions of a particular sender.
-	pub fn update_scores(&mut self, sender: &Sender, event: S::Event) {
+	pub fn update_scores(&mut self, sender: &T::Sender, event: S::Event) {
 		let res = if let Some(set) = self.transactions.get_mut(sender) {
 			let prev = set.worst_and_best();
 			set.update_scores(&self.scoring, event);
@@ -410,7 +471,7 @@ impl<T, S, L> Pool<T, S, L> where
 			let len = transactions.len();
 			for (idx, tx) in transactions.iter().enumerate() {
 				match ready.is_ready(tx) {
-					Readiness::Stalled => status.stalled += 1,
+					Readiness::Stale => status.stalled += 1,
 					Readiness::Ready => status.pending += 1,
 					Readiness::Future => {
 						status.future += len - idx;
@@ -442,6 +503,11 @@ impl<T, S, L> Pool<T, S, L> where
 		&self.listener
 	}
 
+	/// Borrows scoring instance.
+	pub fn scoring(&self) -> &S {
+		&self.scoring
+	}
+
 	/// Borrows listener mutably.
 	pub fn listener_mut(&mut self) -> &mut L {
 		&mut self.listener
@@ -485,7 +551,7 @@ impl<'a, T, R, S, L> Iterator for PendingIterator<'a, T, R, S, L> where
 						self.best_transactions.insert(ScoreWithRef::new(score, tx));
 					}
 
-					return Some(best.transaction)
+					return Some(best.transaction.transaction)
 				},
 				state => trace!("[{:?}] Ignoring {:?} transaction.", best.transaction.hash(), state),
 			}

transaction-pool/src/ready.rs

@@ -17,8 +17,8 @@
 /// Transaction readiness.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum Readiness {
-	/// The transaction is stalled (and should/will be removed from the pool).
-	Stalled,
+	/// The transaction is stale (and should/will be removed from the pool).
+	Stale,
 	/// The transaction is ready to be included in pending set.
 	Ready,
 	/// The transaction is not yet ready.

transaction-pool/src/scoring.rs

@@ -17,9 +17,7 @@
 //! A transactions ordering abstraction.
 
 use std::{cmp, fmt};
-use std::sync::Arc;
-
-use {VerifiedTransaction};
+use pool::Transaction;
 
 /// Represents a decision what to do with
 /// a new transaction that tries to enter the pool.
@@ -98,10 +96,12 @@ pub trait Scoring<T>: fmt::Debug {
 	/// Updates the transaction scores given a list of transactions and a change to previous scoring.
 	/// NOTE: you can safely assume that both slices have the same length.
 	/// (i.e. score at index `i` represents transaction at the same index)
-	fn update_scores(&self, txs: &[Arc<T>], scores: &mut [Self::Score], change: Change<Self::Event>);
+	fn update_scores(&self, txs: &[Transaction<T>], scores: &mut [Self::Score], change: Change<Self::Event>);
 
 	/// Decides if `new` should push out `old` transaction from the pool.
-	fn should_replace(&self, old: &T, new: &T) -> bool;
+	///
+	/// NOTE returning `InsertNew` here can lead to some transactions being accepted above pool limits.
+	fn should_replace(&self, old: &T, new: &T) -> Choice;
 }
 
 /// A score with a reference to the transaction.
@@ -110,7 +110,14 @@ pub struct ScoreWithRef<T, S> {
 	/// Score
 	pub score: S,
 	/// Shared transaction
-	pub transaction: Arc<T>,
+	pub transaction: Transaction<T>,
+}
+
+impl<T, S> ScoreWithRef<T, S> {
+	/// Creates a new `ScoreWithRef`
+	pub fn new(score: S, transaction: Transaction<T>) -> Self {
+		ScoreWithRef { score, transaction }
+	}
 }
 
 impl<T, S: Clone> Clone for ScoreWithRef<T, S> {
@@ -122,30 +129,23 @@ impl<T, S: Clone> Clone for ScoreWithRef<T, S> {
 	}
 }
 
-impl<T, S> ScoreWithRef<T, S> {
-	/// Creates a new `ScoreWithRef`
-	pub fn new(score: S, transaction: Arc<T>) -> Self {
-		ScoreWithRef { score, transaction }
-	}
-}
-
-impl<S: cmp::Ord, T: VerifiedTransaction> Ord for ScoreWithRef<T, S> {
+impl<S: cmp::Ord, T> Ord for ScoreWithRef<T, S> {
 	fn cmp(&self, other: &Self) -> cmp::Ordering {
 		other.score.cmp(&self.score)
-			.then(other.transaction.insertion_id().cmp(&self.transaction.insertion_id()))
+			.then(other.transaction.insertion_id.cmp(&self.transaction.insertion_id))
 	}
 }
 
-impl<S: cmp::Ord, T: VerifiedTransaction> PartialOrd for ScoreWithRef<T, S> {
+impl<S: cmp::Ord, T> PartialOrd for ScoreWithRef<T, S> {
 	fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
 		Some(self.cmp(other))
 	}
 }
 
-impl<S: cmp::Ord, T: VerifiedTransaction>  PartialEq for ScoreWithRef<T, S> {
+impl<S: cmp::Ord, T>  PartialEq for ScoreWithRef<T, S> {
 	fn eq(&self, other: &Self) -> bool {
-		self.score == other.score && self.transaction.insertion_id() == other.transaction.insertion_id()
+		self.score == other.score && self.transaction.insertion_id == other.transaction.insertion_id
 	}
 }
 
-impl<S: cmp::Ord, T: VerifiedTransaction> Eq for ScoreWithRef<T, S> {}
+impl<S: cmp::Ord, T> Eq for ScoreWithRef<T, S> {}

transaction-pool/src/tests/helpers.rs

@@ -17,12 +17,22 @@
 use std::cmp;
 use std::collections::HashMap;
 
-use ethereum_types::U256;
-use {scoring, Scoring, Ready, Readiness, Address as Sender};
-use super::{Transaction, SharedTransaction};
+use ethereum_types::{H160 as Sender, U256};
+use {pool, scoring, Scoring, Ready, Readiness};
+use super::Transaction;
 
 #[derive(Debug, Default)]
-pub struct DummyScoring;
+pub struct DummyScoring {
+	always_insert: bool,
+}
+
+impl DummyScoring {
+	pub fn always_insert() -> Self {
+		DummyScoring {
+			always_insert: true,
+		}
+	}
+}
 
 impl Scoring<Transaction> for DummyScoring {
 	type Score = U256;
@@ -44,7 +54,7 @@ impl Scoring<Transaction> for DummyScoring {
 		}
 	}
 
-	fn update_scores(&self, txs: &[SharedTransaction], scores: &mut [Self::Score], change: scoring::Change) {
+	fn update_scores(&self, txs: &[pool::Transaction<Transaction>], scores: &mut [Self::Score], change: scoring::Change) {
 		if let scoring::Change::Event(_) = change {
 			// In case of event reset all scores to 0
 			for i in 0..txs.len() {
@@ -58,8 +68,14 @@ impl Scoring<Transaction> for DummyScoring {
 		}
 	}
 
-	fn should_replace(&self, old: &Transaction, new: &Transaction) -> bool {
-		new.gas_price > old.gas_price
+	fn should_replace(&self, old: &Transaction, new: &Transaction) -> scoring::Choice {
+		if self.always_insert {
+			scoring::Choice::InsertNew
+		} else if new.gas_price > old.gas_price {
+			scoring::Choice::ReplaceOld
+		} else {
+			scoring::Choice::RejectNew
+		}
 	}
 }
 
@@ -84,7 +100,7 @@ impl Ready<Transaction> for NonceReady {
 				*nonce = *nonce + 1.into();
 				Readiness::Ready
 			},
-			cmp::Ordering::Less => Readiness::Stalled,
+			cmp::Ordering::Less => Readiness::Stale,
 		}
 	}
 }

transaction-pool/src/tests/mod.rs

@@ -32,21 +32,31 @@ pub struct Transaction {
 	pub gas_price: U256,
 	pub gas: U256,
 	pub sender: Address,
-	pub insertion_id: u64,
 	pub mem_usage: usize,
 }
 
 impl VerifiedTransaction for Transaction {
+	type Hash = H256;
+	type Sender = Address;
+
 	fn hash(&self) -> &H256 { &self.hash }
 	fn mem_usage(&self) -> usize { self.mem_usage }
 	fn sender(&self) -> &Address { &self.sender }
-	fn insertion_id(&self) -> u64 { self.insertion_id }
 }
 
 pub type SharedTransaction = Arc<Transaction>;
 
 type TestPool = Pool<Transaction, DummyScoring>;
 
+impl TestPool {
+	pub fn with_limit(max_count: usize) -> Self {
+		Self::with_options(Options {
+			max_count,
+			..Default::default()
+		})
+	}
+}
+
 #[test]
 fn should_clear_queue() {
 	// given
@@ -123,7 +133,7 @@ fn should_reject_if_above_count() {
 	// Reject second
 	let tx1 = b.tx().nonce(0).new();
 	let tx2 = b.tx().nonce(1).new();
-	let hash = *tx2.hash();
+	let hash = format!("{:?}", tx2.hash());
 	txq.import(tx1).unwrap();
 	assert_eq!(txq.import(tx2).unwrap_err().kind(), &error::ErrorKind::TooCheapToEnter(hash, "0x0".into()));
 	assert_eq!(txq.light_status().transaction_count, 1);
@@ -149,7 +159,7 @@ fn should_reject_if_above_mem_usage() {
 	// Reject second
 	let tx1 = b.tx().nonce(1).mem_usage(1).new();
 	let tx2 = b.tx().nonce(2).mem_usage(2).new();
-	let hash = *tx2.hash();
+	let hash = format!("{:?}", tx2.hash());
 	txq.import(tx1).unwrap();
 	assert_eq!(txq.import(tx2).unwrap_err().kind(), &error::ErrorKind::TooCheapToEnter(hash, "0x0".into()));
 	assert_eq!(txq.light_status().transaction_count, 1);
@@ -175,7 +185,7 @@ fn should_reject_if_above_sender_count() {
 	// Reject second
 	let tx1 = b.tx().nonce(1).new();
 	let tx2 = b.tx().nonce(2).new();
-	let hash = *tx2.hash();
+	let hash = format!("{:x}", tx2.hash());
 	txq.import(tx1).unwrap();
 	assert_eq!(txq.import(tx2).unwrap_err().kind(), &error::ErrorKind::TooCheapToEnter(hash, "0x0".into()));
 	assert_eq!(txq.light_status().transaction_count, 1);
@@ -185,7 +195,7 @@ fn should_reject_if_above_sender_count() {
 	// Replace first
 	let tx1 = b.tx().nonce(1).new();
 	let tx2 = b.tx().nonce(2).gas_price(2).new();
-	let hash = *tx2.hash();
+	let hash = format!("{:x}", tx2.hash());
 	txq.import(tx1).unwrap();
 	// This results in error because we also compare nonces
 	assert_eq!(txq.import(tx2).unwrap_err().kind(), &error::ErrorKind::TooCheapToEnter(hash, "0x0".into()));
@@ -444,9 +454,81 @@ fn should_return_worst_transaction() {
 
 	// when
 	txq.import(b.tx().nonce(0).gas_price(5).new()).unwrap();
+	txq.import(b.tx().sender(1).nonce(0).gas_price(4).new()).unwrap();
 
 	// then
-	assert!(txq.worst_transaction().is_some());
+	assert_eq!(txq.worst_transaction().unwrap().gas_price, 4.into());
+}
+
+#[test]
+fn should_return_is_full() {
+	// given
+	let b = TransactionBuilder::default();
+	let mut txq = TestPool::with_limit(2);
+	assert!(!txq.is_full());
+
+	// when
+	txq.import(b.tx().nonce(0).gas_price(110).new()).unwrap();
+	assert!(!txq.is_full());
+
+	txq.import(b.tx().sender(1).nonce(0).gas_price(100).new()).unwrap();
+
+	// then
+	assert!(txq.is_full());
+}
+
+#[test]
+fn should_import_even_if_limit_is_reached_and_should_replace_returns_insert_new() {
+	// given
+	let b = TransactionBuilder::default();
+	let mut txq = TestPool::with_scoring(DummyScoring::always_insert(), Options {
+		max_count: 1,
+		..Default::default()
+	});
+	txq.import(b.tx().nonce(0).gas_price(5).new()).unwrap();
+	assert_eq!(txq.light_status(), LightStatus {
+		transaction_count: 1,
+		senders: 1,
+		mem_usage: 0,
+	});
+
+	// when
+	txq.import(b.tx().nonce(1).gas_price(5).new()).unwrap();
+
+	// then
+	assert_eq!(txq.light_status(), LightStatus {
+		transaction_count: 2,
+		senders: 1,
+		mem_usage: 0,
+	});
+}
+
+#[test]
+fn should_not_import_even_if_limit_is_reached_and_should_replace_returns_false() {
+	// given
+	let b = TransactionBuilder::default();
+	let mut txq = TestPool::with_scoring(DummyScoring::default(), Options {
+		max_count: 1,
+		..Default::default()
+	});
+	txq.import(b.tx().nonce(0).gas_price(5).new()).unwrap();
+	assert_eq!(txq.light_status(), LightStatus {
+		transaction_count: 1,
+		senders: 1,
+		mem_usage: 0,
+	});
+
+	// when
+	let err = txq.import(b.tx().nonce(1).gas_price(5).new()).unwrap_err();
+
+	// then
+	assert_eq!(err.kind(),
+	&error::ErrorKind::TooCheapToEnter("0x00000000000000000000000000000000000000000000000000000000000001f5".into(), "0x5".into()));
+	assert_eq!(txq.light_status(), LightStatus {
+		transaction_count: 1,
+		senders: 1,
+		mem_usage: 0,
+	});
 }
 
 mod listener {
@@ -489,7 +571,7 @@ mod listener {
 		let b = TransactionBuilder::default();
 		let listener = MyListener::default();
 		let results = listener.0.clone();
-		let mut txq = Pool::new(listener, DummyScoring, Options {
+		let mut txq = Pool::new(listener, DummyScoring::default(), Options {
 			max_per_sender: 1,
 			max_count: 2,
 			..Default::default()
@@ -527,7 +609,7 @@ mod listener {
 		let b = TransactionBuilder::default();
 		let listener = MyListener::default();
 		let results = listener.0.clone();
-		let mut txq = Pool::new(listener, DummyScoring, Options::default());
+		let mut txq = Pool::new(listener, DummyScoring::default(), Options::default());
 
 		// insert
 		let tx1 = txq.import(b.tx().nonce(1).new()).unwrap();
@@ -546,7 +628,7 @@ mod listener {
 		let b = TransactionBuilder::default();
 		let listener = MyListener::default();
 		let results = listener.0.clone();
-		let mut txq = Pool::new(listener, DummyScoring, Options::default());
+		let mut txq = Pool::new(listener, DummyScoring::default(), Options::default());
 
 		// insert
 		txq.import(b.tx().nonce(1).new()).unwrap();
@@ -564,7 +646,7 @@ mod listener {
 		let b = TransactionBuilder::default();
 		let listener = MyListener::default();
 		let results = listener.0.clone();
-		let mut txq = Pool::new(listener, DummyScoring, Options::default());
+		let mut txq = Pool::new(listener, DummyScoring::default(), Options::default());
 
 		// insert
 		txq.import(b.tx().nonce(1).new()).unwrap();

transaction-pool/src/tests/tx_builder.rs

@@ -14,9 +14,6 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::rc::Rc;
-use std::cell::Cell;
-
 use super::{Transaction, U256, Address};
 
 #[derive(Debug, Default, Clone)]
@@ -26,7 +23,6 @@ pub struct TransactionBuilder {
 	gas: U256,
 	sender: Address,
 	mem_usage: usize,
-	insertion_id: Rc<Cell<u64>>,
 }
 
 impl TransactionBuilder {
@@ -55,11 +51,6 @@ impl TransactionBuilder {
 	}
 
 	pub fn new(self) -> Transaction {
-		let insertion_id = {
-			let id = self.insertion_id.get() + 1;
-			self.insertion_id.set(id);
-			id
-		};
 		let hash = self.nonce ^ (U256::from(100) * self.gas_price) ^ (U256::from(100_000) * U256::from(self.sender.low_u64()));
 		Transaction {
 			hash: hash.into(),
@@ -67,7 +58,6 @@ impl TransactionBuilder {
 			gas_price: self.gas_price,
 			gas: 21_000.into(),
 			sender: self.sender,
-			insertion_id,
 			mem_usage: self.mem_usage,
 		}
 	}

transaction-pool/src/transactions.rs

@@ -15,28 +15,28 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
 use std::{fmt, mem};
-use std::sync::Arc;
 
 use smallvec::SmallVec;
 
 use ready::{Ready, Readiness};
 use scoring::{self, Scoring};
+use pool::Transaction;
 
 #[derive(Debug)]
 pub enum AddResult<T, S> {
-	Ok(Arc<T>),
+	Ok(T),
 	TooCheapToEnter(T, S),
 	TooCheap {
-		old: Arc<T>,
+		old: T,
 		new: T,
 	},
 	Replaced {
-		old: Arc<T>,
-		new: Arc<T>,
+		old: T,
+		new: T,
 	},
 	PushedOut {
-		old: Arc<T>,
-		new: Arc<T>,
+		old: T,
+		new: T,
 	},
 }
 
@@ -45,7 +45,7 @@ const PER_SENDER: usize = 8;
 #[derive(Debug)]
 pub struct Transactions<T, S: Scoring<T>> {
 	// TODO [ToDr] Consider using something that doesn't require shifting all records.
-	transactions: SmallVec<[Arc<T>; PER_SENDER]>,
+	transactions: SmallVec<[Transaction<T>; PER_SENDER]>,
 	scores: SmallVec<[S::Score; PER_SENDER]>,
 }
 
@@ -67,11 +67,11 @@ impl<T: fmt::Debug, S: Scoring<T>> Transactions<T, S> {
 		self.transactions.len()
 	}
 
-	pub fn iter(&self) -> ::std::slice::Iter<Arc<T>> {
+	pub fn iter(&self) -> ::std::slice::Iter<Transaction<T>> {
 		self.transactions.iter()
 	}
 
-	pub fn worst_and_best(&self) -> Option<((S::Score, Arc<T>), (S::Score, Arc<T>))> {
+	pub fn worst_and_best(&self) -> Option<((S::Score, Transaction<T>), (S::Score, Transaction<T>))> {
 		let len = self.scores.len();
 		self.scores.get(0).cloned().map(|best| {
 			let worst = self.scores[len - 1].clone();
@@ -82,7 +82,7 @@ impl<T: fmt::Debug, S: Scoring<T>> Transactions<T, S> {
 		})
 	}
 
-	pub fn find_next(&self, tx: &T, scoring: &S) -> Option<(S::Score, Arc<T>)> {
+	pub fn find_next(&self, tx: &T, scoring: &S) -> Option<(S::Score, Transaction<T>)> {
 		self.transactions.binary_search_by(|old| scoring.compare(old, &tx)).ok().and_then(|index| {
 			let index = index + 1;
 			if index < self.scores.len() {
@@ -93,18 +93,17 @@ impl<T: fmt::Debug, S: Scoring<T>> Transactions<T, S> {
 		})
 	}
 
-	fn push_cheapest_transaction(&mut self, tx: T, scoring: &S, max_count: usize) -> AddResult<T, S::Score> {
+	fn push_cheapest_transaction(&mut self, tx: Transaction<T>, scoring: &S, max_count: usize) -> AddResult<Transaction<T>, S::Score> {
 		let index = self.transactions.len();
 		if index == max_count {
 			let min_score = self.scores[index - 1].clone();
 			AddResult::TooCheapToEnter(tx, min_score)
 		} else {
-			let shared = Arc::new(tx);
-			self.transactions.push(shared.clone());
+			self.transactions.push(tx.clone());
 			self.scores.push(Default::default());
 			scoring.update_scores(&self.transactions, &mut self.scores, scoring::Change::InsertedAt(index));
 
-			AddResult::Ok(shared)
+			AddResult::Ok(tx)
 		}
 	}
 
@@ -112,28 +111,26 @@ impl<T: fmt::Debug, S: Scoring<T>> Transactions<T, S> {
 		scoring.update_scores(&self.transactions, &mut self.scores, scoring::Change::Event(event));
 	}
 
-	pub fn add(&mut self, tx: T, scoring: &S, max_count: usize) -> AddResult<T, S::Score> {
-		let index = match self.transactions.binary_search_by(|old| scoring.compare(old, &tx)) {
+	pub fn add(&mut self, new: Transaction<T>, scoring: &S, max_count: usize) -> AddResult<Transaction<T>, S::Score> {
+		let index = match self.transactions.binary_search_by(|old| scoring.compare(old, &new)) {
 			Ok(index) => index,
 			Err(index) => index,
 		};
 
 		// Insert at the end.
 		if index == self.transactions.len() {
-			return self.push_cheapest_transaction(tx, scoring, max_count)
+			return self.push_cheapest_transaction(new, scoring, max_count)
 		}
 
 		// Decide if the transaction should replace some other.
-		match scoring.choose(&self.transactions[index], &tx) {
+		match scoring.choose(&self.transactions[index], &new) {
 			// New transaction should be rejected
 			scoring::Choice::RejectNew => AddResult::TooCheap {
 				old: self.transactions[index].clone(),
-				new: tx,
+				new,
 			},
 			// New transaction should be kept along with old ones.
 			scoring::Choice::InsertNew => {
-				let new = Arc::new(tx);
-
 				self.transactions.insert(index, new.clone());
 				self.scores.insert(index, Default::default());
 				scoring.update_scores(&self.transactions, &mut self.scores, scoring::Change::InsertedAt(index));
@@ -153,7 +150,6 @@ impl<T: fmt::Debug, S: Scoring<T>> Transactions<T, S> {
 			},
 			// New transaction is replacing some other transaction already in the queue.
 			scoring::Choice::ReplaceOld => {
-				let new = Arc::new(tx);
 				let old = mem::replace(&mut self.transactions[index], new.clone());
 				scoring.update_scores(&self.transactions, &mut self.scores, scoring::Change::ReplacedAt(index));
 
@@ -181,7 +177,7 @@ impl<T: fmt::Debug, S: Scoring<T>> Transactions<T, S> {
 		return true;
 	}
 
-	pub fn cull<R: Ready<T>>(&mut self, ready: &mut R, scoring: &S) -> SmallVec<[Arc<T>; PER_SENDER]> {
+	pub fn cull<R: Ready<T>>(&mut self, ready: &mut R, scoring: &S) -> SmallVec<[Transaction<T>; PER_SENDER]> {
 		let mut result = SmallVec::new();
 		if self.is_empty() {
 			return result;
@@ -190,7 +186,7 @@ impl<T: fmt::Debug, S: Scoring<T>> Transactions<T, S> {
 		let mut first_non_stalled = 0;
 		for tx in &self.transactions {
 			match ready.is_ready(tx) {
-				Readiness::Stalled => {
+				Readiness::Stale => {
 					first_non_stalled += 1;
 				},
 				Readiness::Ready | Readiness::Future => break,