Transaction Pool re-implementation #6994

tomusdrw · 2017-11-06T15:44:19Z

Initial version of new transaction pool.
Currently implement as a separate crate (and abstract over Transaction).

In subsequent PRs I plan to:

integrate with Parity (implement Ready, Scoring and Verifier, penalties, own transactions, etc)
Run some benchmarks (and compare with old queue)
Migrate old tests to make sure the behaviour is consistent.
Cache pending set (and then update it in-place)

The details are explained in docs, but the big picture is this:

The pool maintains a list of transactions per sender ordered by Score (think: nonce). (HashMap<Sender, Vec<Transaction>>)
We also know best and worst transaction from every sender and keep a queue of best-first transactions and worst-last transactions.
Inserting to the queue is cheap (O(no-of-tx-per-sender + log(no-of-senders)) (worst case), usually O(log(no-of-tx-per-sender) + log(no-of-senders)))
Constructing pending set is relatively cheap (it's not O(1) as it used to be, but we are within O(nlog(n)) (all transactions from a single sender), usually O(n))
Pluggable scoring allows us to do some interesting prioritizations (like high priority for a chain of small transactions not just looking at a single one = bump priority by sending a very expensive transaction later)
Decoupling from nonce/gasPrice will allow to easily integrate account abstraction transactions
Readiness abstraction allows to easily implement scheduled transactions

0x7CFE · 2017-11-07T13:08:28Z

transaction-pool/src/error.rs

+			display("[{:?}] transaction too cheap to enter the pool", hash)
+		}
+		TooCheapToReplace(old_hash: H256, hash: H256) {
+			description("transaction is too cheap too replace existing transaction in the queue"),


s/too replace/to replace/

0x7CFE · 2017-11-07T13:55:05Z

transaction-pool/src/pool.rs

+		};
+
+		// update best and worst transactions from this sender (if required)
+		self.update_senders_worst_and_best(result.1, result.2);


Should we really update the values even if not AddResult::Ok?

We don't need to update it if we know that new transaction was rejected (or only in case AddResult::Ok | AddResult::PushedOut | AddResult::Replaced).
I did this for simplicity though, the values would be the same and it is not expensive to compute & compare them so I don't think it will have any performance implications.

0x7CFE · 2017-11-07T14:02:21Z

transaction-pool/src/pool.rs

+			}
+		}
+
+		let result = {


I think, it's better to re-bind values by name here.

0x7CFE · 2017-11-07T14:07:50Z

transaction-pool/src/pool.rs

+	}
+
+	/// Updates state of the pool statistics if the transaction was added to a set.
+	fn added(&mut self, new: &Arc<T>, old: Option<&Arc<T>>) {


I believe, these functions should be named like finalize_* or commit_*. Simple added is not clear and may be confused with the listener's, at least for me.

0x7CFE · 2017-11-07T14:13:47Z

transaction-pool/src/pool.rs

+		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 is no transaction to remove.");


Maybe are no transactions to remove?

0x7CFE · 2017-11-07T14:29:36Z

transaction-pool/src/pool.rs

+			Some(removed) => {
+				let len = removed.len();
+				for tx in removed {
+					self.removed(tx.hash());


Too much removed per line in this function, IMO.

0x7CFE · 2017-11-07T14:33:58Z

transaction-pool/src/pool.rs

+	pub fn pending<R: Ready<T>>(&self, ready: R) -> PendingIterator<T, R, S, L> {
+		PendingIterator {
+			ready,
+			best_transactions: self.best_transactions.clone(),


Maybe use COW instead?

Needs to be cloned on the first next() anyway, do you think it's worth it?

Probably not

0x7CFE · 2017-11-07T14:40:33Z

transaction-pool/src/pool.rs

+	pub fn status<R: Ready<T>>(&self, mut ready: R) -> Status {
+		let mut stalled = 0;
+		let mut pending = 0;
+		let mut future = 0;


let mut Status?

0x7CFE · 2017-11-07T14:44:01Z

transaction-pool/src/ready.rs

+
+impl From<bool> for Readiness {
+	fn from(b: bool) -> Self {
+		if b { Readiness::Ready } else { Readiness::Future }


This is very tricky and opaque, IMO. Easy to miss the thing if several negations will take place in an expression.

0x7CFE · 2017-11-07T15:05:21Z

transaction-pool/src/scoring.rs

+	fn choose(&self, old: &T, new: &T) -> Choice;
+
+	/// Updates the transaction scores given a list of transactions and a change to previous scoring.
+	/// NOTE: `txs.len() === scores.len()`


Better to write explicitly

0x7CFE

A little bit more grumble comments.

0x7CFE · 2017-11-16T12:52:01Z

transaction-pool/src/transactions.rs

+			if index >= self.scores.len() {
+				None
+			} else {
+				Some((self.scores[index].clone(), self.transactions[index].clone()))


Wouldn't it be more clear like this?

if index > self.scores.len() { None } else { let index = index + 1; Some((self.scores[index].clone(), self.transactions[index].clone())) }

That's not exactly the same, but changed condition to:

let index = index + 1; if index < self.scores.len() { Some(...) } else { None }

which I think makes it clearer.

0x7CFE · 2017-11-16T13:34:00Z

transaction-pool/src/transactions.rs

+		let len = self.transactions.len();
+		if index == len {
+			// trying to insert least worth transaction
+			return if len == max_count {


This statement is part of a rather long method so all early returns should be easily noticeable. When skimming through the code, one may easily misinterpret this statement as return from the first branch only.

I believe, this return statement should be refactored in favour of two explicit returns in each condition branch. Another option is to introduce a temporary value, like result, and add explicit return result after condition block.

P.S.: Maybe consider splitting this method into smaller parts?

Good point. Refactored push_to_the_end() as a separate method.

0x7CFE · 2017-11-16T13:43:37Z

transaction-pool/src/transactions.rs

+		}
+
+		// Decide if the transaction should be replaced
+		match scoring.choose(&self.transactions[index], &tx) {


If I were you, I'd introduce result value here to prepare the reader and put an emphasis on the fact that we expect a value from this match.

0x7CFE · 2017-11-16T13:47:40Z

transaction-pool/src/tests/helpers.rs

+			scoring::Choice::InsertNew
+		};
+
+		decision


Since this method is small and intentions are crystal clear, I may suggest omitting the decision value. Two nested ifs would be OK, in my opinion.

Left overs from println debugging :)

0x7CFE · 2017-11-16T13:49:49Z

transaction-pool/src/status.rs

+	/// Memory usage in bytes.
+	pub mem_usage: usize,
+	/// Total number of transactions in the pool.
+	pub count: usize,


Better to s/count/transaction_count/. More status fields may be added in future which may cause confusion.

dkashitsyn

Looks good to me, but someone else should check.

Personally, I would love to see more comments in the algorithmic part of the code about decision making and overall logic.

dkashitsyn · 2017-12-01T09:35:12Z

transaction-pool/src/tests/helpers.rs

+impl Scoring<Transaction> for DummyScoring {
+	type Score = U256;
+
+	fn compare(&self, old: &Transaction, other: &Transaction) -> cmp::Ordering {


s/other/new/ ?

dkashitsyn · 2017-12-01T09:43:45Z

transaction-pool/src/transactions.rs

+	// TODO [ToDr] Consider using something that doesn't require shifting all records.
+	transactions: SmallVec<[Arc<T>; PER_SENDER]>,
+	scores: SmallVec<[S::Score; PER_SENDER]>,
+	_score: PhantomData<S>,


I think, it would be nice to write a comment, why do we need phantom here.

Actually we don't, thanks for noticing :)

dkashitsyn · 2017-12-01T10:08:26Z

transaction-pool/src/transactions.rs

+		return true;
+	}
+
+	pub fn cull<R: Ready<T>>(&mut self, ready: &mut R, scoring: &S) -> SmallVec<[Arc<T>; PER_SENDER]> {


Something makes me think, that this method may be refactored using iterators instead of reverses.

Since we use SmallVec it doesn't have methods that allow easily to consume the content.
Current method doesn't require allocation, but is indeed O(n), since number of transactions per sender is limited I don't expect it to be a problem.

With Vec we could use split_off, although instead of Vec it would be more wise to use VecDeque, but that would require more allocations - might be worth to consider though after some initial tests.

0x7CFE

I completely agree with my alter ego (sorry, logged with wrong account :)

tomusdrw · 2017-12-01T13:01:34Z

Thank you for the review @0x7CFE. Addressed grumbles and added some more detailed docs about the algo.

tomusdrw added 17 commits October 27, 2017 12:43

Initial design and some tests.

abfc463

Insertion & limits.

4bd050a

Constructing pending block.

0e8dae2

Change to PendingIterator.

8430cc8

Removing/cancelling transactions.

b0d68df

Full status.

38d3c66

Culling transactions.

3860309

Use bigint.

e2bfce3

Add listener tests.

5f60623

Clean up listener types.

9168d9d

Split into multiple files.

a1847f8

Add copyright notice.

6d9b71e

Documentation.

b56dbad

Don't require ownership.

0587e03

Fix cull to remove from by_hash.

a4f0929

Merge branch 'master' into td-txqueue

5beee72

Make the queue generic over transactions.

54d5440

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 6, 2017

tomusdrw requested review from 0x7CFE, arkpar and rphmeier November 6, 2017 15:44

5chdn added this to the 1.9 milestone Nov 6, 2017

0x7CFE reviewed Nov 7, 2017

View reviewed changes

5chdn added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 10, 2017

0x7CFE reviewed Nov 16, 2017

View reviewed changes

Address code review.

845f640

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 20, 2017

tomusdrw added 2 commits November 30, 2017 11:08

Merge branch 'master' into td-txqueue

b08791e

Update wasm submodules.

468cf03

tomusdrw mentioned this pull request Nov 30, 2017

add newPendingTransactions to subscriptions #6426

Closed

dkashitsyn approved these changes Dec 1, 2017

View reviewed changes

0x7CFE approved these changes Dec 1, 2017

View reviewed changes

tomusdrw added 2 commits December 1, 2017 12:57

Fix review grumbles.

2034175

Add some docs.

ad0f104

malzzz mentioned this pull request Dec 14, 2017

Pending transaction filter stops listening after several minutes hyperledger/web3j#144

Closed

svyatonik approved these changes Dec 15, 2017

View reviewed changes

svyatonik added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 15, 2017

tomusdrw merged commit 1d92067 into master Dec 19, 2017

tomusdrw deleted the td-txqueue branch December 19, 2017 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction Pool re-implementation #6994

Transaction Pool re-implementation #6994

tomusdrw commented Nov 6, 2017 •

edited

0x7CFE Nov 7, 2017

0x7CFE Nov 7, 2017

tomusdrw Nov 20, 2017

0x7CFE Nov 7, 2017

0x7CFE Nov 7, 2017

0x7CFE Nov 7, 2017

0x7CFE Nov 7, 2017

0x7CFE Nov 7, 2017

tomusdrw Nov 20, 2017

0x7CFE Nov 20, 2017

0x7CFE Nov 7, 2017

0x7CFE Nov 7, 2017

0x7CFE Nov 7, 2017

0x7CFE left a comment

0x7CFE Nov 16, 2017

tomusdrw Nov 20, 2017

0x7CFE Nov 16, 2017 •

edited

tomusdrw Nov 20, 2017

0x7CFE Nov 16, 2017 •

edited

0x7CFE Nov 16, 2017

tomusdrw Nov 20, 2017

0x7CFE Nov 16, 2017

dkashitsyn left a comment

dkashitsyn Dec 1, 2017

dkashitsyn Dec 1, 2017

tomusdrw Dec 1, 2017

dkashitsyn Dec 1, 2017

tomusdrw Dec 1, 2017

0x7CFE left a comment

tomusdrw commented Dec 1, 2017

Transaction Pool re-implementation #6994

Transaction Pool re-implementation #6994

Conversation

tomusdrw commented Nov 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x7CFE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x7CFE Nov 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x7CFE Nov 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkashitsyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x7CFE left a comment

Choose a reason for hiding this comment

tomusdrw commented Dec 1, 2017

tomusdrw commented Nov 6, 2017 •

edited

0x7CFE Nov 16, 2017 •

edited

0x7CFE Nov 16, 2017 •

edited