Limit the number of transactions in pending set #8777

tomusdrw · 2018-06-04T07:23:02Z

With large pools and in case the node is not --force-sealing the pending set was actually being computed in the sync thread (propagate transactions on timeout). That could cause a significant delay, since there was nothing in the cache and the pool was pretty huge, but for propagation we never needed more than 64 first transactions.

This PR introduces max_len argument to avoid constructing too large pending sets.
Additionally we introduce UnorderedIterator that can be computed faster than the priority one. This for now is only used for parity_allTransactions RPC, but in future could be used in places where we don't really care that much about strict ordering (RPC filters maybe)

CC @folsen

Partially related: #8696

folsen

LGTM, have tested this on my machine to confirm it fixes the slow RPC issue with very large --tx-queue-size

jimpo

Still not too familiar with the codebase, but LGTM overall. All comments are minor.

jimpo · 2018-06-04T21:33:51Z

transaction-pool/src/tests/mod.rs

+	let tx0 = txq.import(b.tx().nonce(0).gas_price(5).new()).unwrap();
+	let tx1 = txq.import(b.tx().nonce(1).gas_price(5).new()).unwrap();
+	let tx2 = txq.import(b.tx().nonce(2).new()).unwrap();
+	let tx3 =txq.import(b.tx().nonce(3).gas_price(4).new()).unwrap();


nit: Space after =.

jimpo · 2018-06-04T21:34:43Z

transaction-pool/src/tests/mod.rs

+	// gap
+	txq.import(b.tx().sender(1).nonce(5).new()).unwrap();
+
+	let tx9 = txq.import(b.tx().sender(2).nonce(0).new()).unwrap();


It's a bit odd that this is tx9 (no gap in tx numbering), whereas there's a gap in numbering between tx3 and tx5.

It's because tx3 and tx5 are coming from the same sender and tx9 is from a different one.

I guess tx1_1 would be a bit more appropriate to describe all the details. Can change it if you believe it makes things clearer.

jimpo · 2018-06-04T23:25:24Z

ethcore/sync/src/api.rs

@@ -373,7 +377,9 @@ struct SyncProtocolHandler {
 impl NetworkProtocolHandler for SyncProtocolHandler {
 	fn initialize(&self, io: &NetworkContext) {
 		if io.subprotocol_name() != WARP_SYNC_PROTOCOL_ID {
-			io.register_timer(0, Duration::from_secs(1)).expect("Error registering sync timer");
+			io.register_timer(PEERS_TIMER, Duration::from_millis(700)).expect("Error registering sync timer");


s/sync/peers/ in the expect message

jimpo · 2018-06-04T23:25:56Z

ethcore/sync/src/api.rs

-			io.register_timer(0, Duration::from_secs(1)).expect("Error registering sync timer");
+			io.register_timer(PEERS_TIMER, Duration::from_millis(700)).expect("Error registering sync timer");
+			io.register_timer(SYNC_TIMER, Duration::from_millis(1100)).expect("Error registering sync timer");
+			io.register_timer(TX_TIMER, Duration::from_millis(1300)).expect("Error registering sync timer");


The expect message should be changed probably.

jimpo · 2018-06-04T23:30:43Z

miner/src/pool/queue.rs

@@ -261,15 +280,27 @@ impl TransactionQueue {
 			scoring::NonceAndGasPrice,
 			Listener,
 		>) -> T,
+	{
+		debug!(target: "txqueue", "Re-computing pending set for block: {}", block_number);
+		let _timer = ::trace_time::PerfTimer::new("pool::collect_pending");


Any reason not to use the trace_time! macro?

jimpo · 2018-06-04T23:52:17Z

miner/src/pool/queue.rs

 			return pending;
 		}

-		let pending: Vec<_> = self.collect_pending(client, block_number, current_timestamp, nonce_cap, |i| i.collect());
+		// In case we don't have a cached set, but we don't care about order


Might be nice to test this case, and assert unordered results and that the cache is left alone.

Test added.

debris

I'm not very familiar with the context, but LGTM

5chdn · 2018-06-08T14:42:48Z

Does not compile.

   Compiling parity-rpc v1.12.0 (file:///builds/parity/parity/rpc)
error[E0050]: method `ready_transactions` has 2 parameters but the declaration in trait `ethcore::miner::MinerService::ready_transactions` has 4
   --> rpc/src/v1/tests/helpers/miner_service.rs:211:42
    |
211 |     fn ready_transactions<C>(&self, _chain: &C) -> Vec<Arc<VerifiedTransaction>> {
    |                                             ^^ expected 4 parameters, found 2
    |
    = note: `ready_transactions` from trait: `fn(&Self, &C, usize, ethcore::miner::PendingOrdering) -> std::vec::Vec<std::sync::Arc<miner::pool::VerifiedTransaction>>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0050`.
error: Could not compile `parity-rpc`.

dvdplm · 2018-06-11T12:04:02Z

Does not compile.

Confirmed. And while the above is easy enough to fix (fn ready_transactions<C>(&self, _chain: &C, max_len: usize, ordering: PendingOrdering) -> Vec<Arc<VerifiedTransaction>>) there are other errors after it; they seem to stem from the TestBlockChainClient and ChainNotificationHandler not working well together anymore: the tests assume a new_transcations() method that is not available.

Also needs a ethcore = { path = "../ethcore", features = ["test-helpers"] } in Cargo.toml for test helpers to be compiled.

* master: Tx permission contract improvement (#8400) Limit the number of transactions in pending set (#8777) Use sealing.enabled to emit eth_mining information (#8844) Don't allocate in expect_valid_rlp unless necessary (#8867) Fix Cli Return Code on --help for ethkey, ethstore & whisper (#8863)

* Unordered iterator. * Use unordered and limited set if full not required. * Split timeout work into smaller timers. * Avoid collecting all pending transactions when mining * Remove println. * Use priority ordering in eth-filter. * Fix ethcore-miner tests and tx propagation. * Review grumbles addressed. * Add test for unordered not populating the cache. * Fix ethcore tests. * Fix light tests. * Fix ethcore-sync tests. * Fix RPC tests.

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: (29 commits) Block 0 is valid in queries (openethereum#8891) fixed osx permissions (openethereum#8901) Atomic create new files with permissions to owner in ethstore (openethereum#8896) Add ETC Cooperative-run load balanced parity node (openethereum#8892) Add support for --chain tobalaba (openethereum#8870) fix some warns on nightly (openethereum#8889) Add new ovh bootnodes and fix port for foundation bootnode 3.2 (openethereum#8886) SecretStore: service pack 1 (openethereum#8435) Handle removed logs in filter changes and add geth compatibility field (openethereum#8796) fixed ipc leak, closes openethereum#8774 (openethereum#8876) scripts: remove md5 checksums (openethereum#8884) hardware_wallet/Ledger `Sign messages` + some refactoring (openethereum#8868) Check whether we need resealing in miner and unwrap has_account in account_provider (openethereum#8853) docker: Fix alpine build (openethereum#8878) Remove mac os installers etc (openethereum#8875) README.md: update the list of dependencies (openethereum#8864) Fix concurrent access to signer queue (openethereum#8854) Tx permission contract improvement (openethereum#8400) Limit the number of transactions in pending set (openethereum#8777) Use sealing.enabled to emit eth_mining information (openethereum#8844) ...

* Unordered iterator. * Use unordered and limited set if full not required. * Split timeout work into smaller timers. * Avoid collecting all pending transactions when mining * Remove println. * Use priority ordering in eth-filter. * Fix ethcore-miner tests and tx propagation. * Review grumbles addressed. * Add test for unordered not populating the cache. * Fix ethcore tests. * Fix light tests. * Fix ethcore-sync tests. * Fix RPC tests.

* parity-version: stabelize 1.11 * parity-version: bump stable to 1.11.7 * Don't fetch snapshot chunks at random (#9088) * Offload cull to IoWorker. * Limit the number of transactions in pending set (#8777) * Unordered iterator. * Use unordered and limited set if full not required. * Split timeout work into smaller timers. * Avoid collecting all pending transactions when mining * Remove println. * Use priority ordering in eth-filter. * Fix ethcore-miner tests and tx propagation. * Review grumbles addressed. * Add test for unordered not populating the cache. * Fix ethcore tests. * Fix light tests. * Fix ethcore-sync tests. * Fix RPC tests. * Make sure to produce full blocks. * Update hidapi, fixes #7542 (#9108) * docker: add cmake dependency (#9111) * Fix miner tests. * Revert "Make sure to produce full blocks." This reverts commit b12d592. * Update light client hardcoded headers (#9098) * Insert Kovan hardcoded headers until #7690241 * Insert Kovan hardcoded headers until block 7690241 * Insert Ropsten hardcoded headers until #3612673 * Insert Mainnet hardcoded headers until block 5941249 * Make sure to produce full blocks. (#9115) * Insert ETC (classic) hardcoded headers until block #6170625 (#9121) * fix verification in ethcore-sync collect_blocks (#9135) * `evm bench` fix broken dependencies (#9134) * `evm bench` use valid dependencies Benchmarks of the `evm` used stale versions of a couple a crates that this commit fixes! * fix warnings

tomusdrw added 8 commits June 1, 2018 13:09

Unordered iterator.

69ca2ac

Use unordered and limited set if full not required.

9a57cd8

Split timeout work into smaller timers.

bae903e

Merge branch 'master' into td-txpropagation

bf54784

Avoid collecting all pending transactions when mining

3804063

Remove println.

1c20ac1

Merge branch 'master' into td-txpropagation

33bd7ee

Use priority ordering in eth-filter.

6f154df

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 4, 2018

folsen approved these changes Jun 4, 2018

View reviewed changes

5chdn added this to the 1.12 milestone Jun 4, 2018

Fix ethcore-miner tests and tx propagation.

45e8f77

jimpo reviewed Jun 5, 2018

View reviewed changes

tomusdrw added 3 commits June 5, 2018 15:58

Review grumbles addressed.

2965e5b

Add test for unordered not populating the cache.

6e219f4

Fix ethcore tests.

c805564

debris approved these changes Jun 5, 2018

View reviewed changes

debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 5, 2018

tomusdrw and others added 2 commits June 6, 2018 10:22

Fix light tests.

1285431

Merge branch 'master' into td-txpropagation

d7b7ca0

5chdn added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jun 6, 2018

tomusdrw added 2 commits June 7, 2018 11:42

Fix ethcore-sync tests.

f8fdea4

Merge branch 'master' into td-txpropagation

5ded25b

Merge branch 'master' into td-txpropagation

c000b78

Fix RPC tests.

9fe496d

debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Jun 12, 2018

debris merged commit 4938d5d into master Jun 12, 2018

debris deleted the td-txpropagation branch June 12, 2018 06:22

tomusdrw mentioned this pull request Jun 25, 2018

Massive increase in CPU usage with 1.11.1 #8696

Closed

andresilva mentioned this pull request Jul 7, 2018

Beta: 1.11.6 backports #9015

Merged

12 tasks

tomusdrw mentioned this pull request Jul 13, 2018

Backports to 1.11.7-stable #9093

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit the number of transactions in pending set #8777

Limit the number of transactions in pending set #8777

tomusdrw commented Jun 4, 2018 •

edited

Loading

folsen left a comment

jimpo left a comment

jimpo Jun 4, 2018

jimpo Jun 4, 2018

tomusdrw Jun 5, 2018

jimpo Jun 4, 2018

jimpo Jun 4, 2018

jimpo Jun 4, 2018

jimpo Jun 4, 2018

tomusdrw Jun 5, 2018

debris left a comment

5chdn commented Jun 8, 2018

dvdplm commented Jun 11, 2018 •

edited

Loading

Limit the number of transactions in pending set #8777

Limit the number of transactions in pending set #8777

Conversation

tomusdrw commented Jun 4, 2018 • edited Loading

folsen left a comment

Choose a reason for hiding this comment

jimpo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debris left a comment

Choose a reason for hiding this comment

5chdn commented Jun 8, 2018

dvdplm commented Jun 11, 2018 • edited Loading

tomusdrw commented Jun 4, 2018 •

edited

Loading

dvdplm commented Jun 11, 2018 •

edited

Loading