Optimize pending transactions filter #9026

tomusdrw · 2018-07-02T13:08:53Z

Re-opening #9014

Also fix ethstats reporting of pending transactions (in case pending block is missing).

Context:
Currently pending transactions filter is using the very inefficient ready_transactions method from miner. For large pools it takes significant amount of time to construct the pending set, because:

We return transactions ordered by priority (gas price)
We check for readiness of transactions - i.e. we need to check the state nonce for every sender, and it's really expensive.
Because of that it can take seconds to construct an ordered pending set of 128k transactions (~110k senders).
The pending transactions filter doesn't require such pending set though, we are ok with unordered hashes and we don't need to be super sure about the nonce, so this PR:

Uses unordered transactions from the queue
Doesn't use the default Ready (which goes to the state), but rather assumes the transactions in the pool are ready - this may give us a couple of false-positives, but it's not critical for pending transactions hashes.
Thanks to this it works really fast (just constructs the BTreeSet and then does the difference).

Only clear the nonce cache when a block is retracted

This reverts commit b382c19.

andresilva · 2018-07-02T15:44:12Z

ethcore/src/miner/miner.rs


 		// First update gas limit in transaction queue and minimal gas price.
 		let gas_limit = *chain.best_block_header().gas_limit();
 		self.update_transaction_queue_limits(gas_limit);

+


Stray newline?

andresilva · 2018-07-02T15:51:15Z

miner/src/pool/ready.rs

@@ -129,6 +129,38 @@ impl txpool::Ready<VerifiedTransaction> for Condition {
 	}
 }

+pub struct OptionalState<C> {


Can we add some docs here?

"Checks readiness of transactions by comparing the nonce to state nonce. If nonce isn't found in provided state nonce store, defaults to the tx nonce and updates the nonce store. Useful for using with a state nonce cache when false positives are allowed." ?

dvdplm · 2018-07-02T17:08:27Z

this may give us a couple of false-positives, but it's not critical for pending transactions hashes.

Can you elaborate on this point: why is it not crucial?

dvdplm

Code lgtm. I have a question about the choice of BTreeSet: was the performance better than for a HashSet?

dvdplm · 2018-07-02T17:36:01Z

rpc/src/v1/helpers/poll_filter.rs

+		f(&mut self.0.lock())
+	}
+}
+
 /// Filter state.
 #[derive(Clone)]
 pub enum PollFilter {
 	/// Number of last block which client was notified about.
 	Block(BlockNumber),
 	/// Hashes of all transactions which client was notified about.


Maybe /// Hashes of all pending transaction the client knows about?

dvdplm · 2018-07-02T17:40:33Z

ethcore/src/miner/miner.rs

@@ -851,6 +851,37 @@ impl miner::MinerService for Miner {
 		self.transaction_queue.all_transactions()
 	}

+	fn pending_transactions_hashes<C>(&self, chain: &C) -> BTreeSet<H256> where


I think it should be pending_transaction_hashes (no double plural s), see e.g. this.

(I am not a native speaker so take it with a grain of salt)

I checked and we're having both usages. This probably should be unified to a single one in the future:

In eth_filter we have pending_transactions_hashes.

In ethcore's Block's views and encoded, we have transaction_hashes.

5chdn · 2018-07-03T09:35:23Z

Please resolve conflicts.

tomusdrw · 2018-07-03T12:28:42Z

this may give us a couple of false-positives, but it's not critical for pending transactions hashes.

Can you elaborate on this point: why is it not crucial?

So the queue in general should contain only transactions that are Ready, however, if we miss a notification from chain_new_blocks (which may happen if the client goes from normal mode to sync mode (i.e. prints Syncing.. instead of Imported) it's not necessarily the case, the queue will contain a bunch of transactions that are already included in the chain or old.
Another case is when transactions in the pool is Future, which means that there is a nonce gap between that particular transaction and the state nonce - that case is really rare though, and even if it happens usually it means that we will just receive the missing transaction soon(ish) and that transaction will become valid (Ready).

So because of the nature of the filters itself, when the caller is receiving the data it might be already outdated (since a new block could be imported, or the transaction could be replaced), that's why it doesn't make much sense to cover the case where the caller sometimes gets false-positives.

Regarding the set performance, TBH I wasn't benchmarking that. I chose BTreeSet for consistency (we return BTreeMap in couple of other places) and testability (it has predictable order).
I'm not entirely sure how efficient difference is in both implementations, might be worth checking. I suspect that both are O(n), but my gut feeling is that BTreeSet should have smaller constant in front.

CC @dvdplm

5chdn · 2018-07-03T13:19:04Z

Does not compile.

Is this in progress or on ice? It already has 3 reviews.

tomusdrw · 2018-07-03T16:13:47Z

@5chdn Fixed, sorry. It's neither in progress nor on ice, it's being reviewed. I thought that after issues are addressed we are going back to pleasereview label, so that people can review the changes I made again.

5chdn · 2018-07-04T07:36:28Z

Ok, leaving this for @andresilva @sorpaas or @dvdplm to sign this off and merge.

dvdplm · 2018-07-04T14:48:38Z

My review stays: LGTM.

@tomusdrw thank you for the extra explanations!

* 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 b382c19. * 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.

* 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 b382c19. * 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

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Fixes for misbehavior reporting in AuthorityRound (openethereum#8998) A last bunch of txqueue performance optimizations (openethereum#9024) reduce number of constraints for triedb types (openethereum#9043) bump fs-swap to 0.2.3 so it is compatible with osx 10.11 again (openethereum#9050) Recursive test (openethereum#9042) Introduce more optional features in ethcore (openethereum#9020) Update ETSC bootnodes (openethereum#9038) Optimize pending transactions filter (openethereum#9026) eip160/eip161 spec: u64 -> BlockNumber (openethereum#9044)

andresilva and others added 9 commits June 29, 2018 12:53

rpc: return unordered transactions in pending transactions filter

52eb8a1

ethcore: use LruCache for nonce cache

473791d

Only clear the nonce cache when a block is retracted

Revert "ethcore: use LruCache for nonce cache"

d5fff50

This reverts commit b382c19.

Use only cached nonces when computing pending hashes.

a44c271

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

30857cf

Fix pending transaction count if not sealing.

cd7a6c7

Clear cache only when block is enacted.

cad3a1e

Merge branch 'master' into andre/optimize-pending-set-filter

656d03d

Fix RPC tests.

bbf7997

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. B0-patchthis M4-core ⛓ Core client code / Rust. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. labels Jul 2, 2018

tomusdrw requested review from andresilva and sorpaas July 2, 2018 13:08

5chdn mentioned this pull request Jul 2, 2018

Beta: 1.11.6 backports #9015

Merged

12 tasks

5chdn added this to the 1.12 milestone Jul 2, 2018

andresilva approved these changes Jul 2, 2018

View reviewed changes

andresilva added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 2, 2018

dvdplm approved these changes Jul 2, 2018

View reviewed changes

sorpaas approved these changes Jul 2, 2018

View reviewed changes

5chdn added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jul 3, 2018

Merge branch 'master' into andre/optimize-pending-set-filter

a02d32a

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Jul 3, 2018

Address review comments.

7a504ab

tomusdrw force-pushed the andre/optimize-pending-set-filter branch from b0f03b4 to 7a504ab Compare July 3, 2018 16:11

andresilva approved these changes Jul 4, 2018

View reviewed changes

andresilva merged commit f4c5ea8 into master Jul 4, 2018

andresilva added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 4, 2018

andresilva mentioned this pull request Jul 4, 2018

A last bunch of txqueue performance optimizations #9024

Merged

3 tasks

5chdn deleted the andre/optimize-pending-set-filter branch July 5, 2018 05:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize pending transactions filter #9026

Optimize pending transactions filter #9026

tomusdrw commented Jul 2, 2018

andresilva Jul 2, 2018

andresilva Jul 2, 2018

dvdplm commented Jul 2, 2018

dvdplm left a comment

dvdplm Jul 2, 2018

dvdplm Jul 2, 2018

sorpaas Jul 2, 2018

5chdn commented Jul 3, 2018 •

edited

Loading

tomusdrw commented Jul 3, 2018 •

edited

Loading

5chdn commented Jul 3, 2018

tomusdrw commented Jul 3, 2018

5chdn commented Jul 4, 2018

dvdplm commented Jul 4, 2018

Optimize pending transactions filter #9026

Optimize pending transactions filter #9026

Conversation

tomusdrw commented Jul 2, 2018

andresilva Jul 2, 2018

Choose a reason for hiding this comment

andresilva Jul 2, 2018

Choose a reason for hiding this comment

dvdplm commented Jul 2, 2018

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm Jul 2, 2018

Choose a reason for hiding this comment

dvdplm Jul 2, 2018

Choose a reason for hiding this comment

sorpaas Jul 2, 2018

Choose a reason for hiding this comment

5chdn commented Jul 3, 2018 • edited Loading

tomusdrw commented Jul 3, 2018 • edited Loading

5chdn commented Jul 3, 2018

tomusdrw commented Jul 3, 2018

5chdn commented Jul 4, 2018

dvdplm commented Jul 4, 2018

5chdn commented Jul 3, 2018 •

edited

Loading

tomusdrw commented Jul 3, 2018 •

edited

Loading