Fix parallel transactions race-condition #10995

ngotchac · 2019-08-27T13:06:00Z

This PR should fix the race-condition issue that happens when sending multiple parallel transactions to a node running a dev chain.
To reproduce the issue, simply run a node with:

> parity --config dev --tx-queue-size 60000 --no-persistent-txqueue

and run this NodeJS script https://gist.github.com/ngotchac/8b85622f6396001f243fa2323184104d which sends a random number of transactions in parallel until it hangs.

The issue seems to be that the pending transactions queue was not totally cleared on culled transaction, and multiple blocks at the same height could be produced by the instant sealing engine.

With this PR, we ensure that only one block per height is produced, and to try to seal a new block after importing a new (internal) best block if there are some pending transactions.

tomusdrw

Seems it should work fine, since maybe_update_sealing is called when we are sure that the block has already been imported (it's run in a separate task).

ethcore/engines/instant-seal/src/lib.rs

ethcore/src/miner/mod.rs

miner/src/pool/queue.rs

tomusdrw · 2019-09-04T15:04:25Z

ethcore/src/miner/miner.rs

+
+	/// Call `update_sealing` if needed
+	pub fn maybe_update_sealing<C: miner::BlockChainClient>(&self, chain: &C) {
+		if self.transaction_queue.has_local_pending_transactions() {


I'm not sure about this. The implementation is very specific for NullEngine (i.e. checks local transactions), while we have plenty of other conditions that affect when to seal. Why can't we just call update_sealing instead?

I think this is still valid.

Ah sorry, I thought you were talking about the next line, where I first used prepare_and_update_sealing. I just updated it.

I guess then there is no point in adding maybe_update_sealing to the traits and just call update_sealing directly?

tomusdrw

Looks good, apart from the race.

ethcore/engines/instant-seal/src/lib.rs

ngotchac · 2019-09-09T17:51:16Z

Waiting for paritytech/parity-common#218 to be merged.

ethcore/src/miner/miner.rs

ethcore/engines/instant-seal/Cargo.toml

Cargo.lock

dvdplm · 2019-09-10T20:47:38Z

Waiting for paritytech/parity-common#218 to be merged.

Do you need a release&publish of this?

ordian · 2019-09-11T13:05:03Z

Awesome work! Race conditions are hard to debug.

* add more tx tests (#11038) * Fix parallel transactions race-condition (#10995) * Add blake2_f precompile (#11017) * [trace] introduce trace failed to Ext (#11019) * Edit publish-onchain.sh to use https (#11016) * Fix deadlock in network-devp2p (#11013) * EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) * xDai chain support and nodes list update (#10989) * EIP 2028: transaction gas lowered from 68 to 16 (#10987) * EIP-1344 Add CHAINID op-code (#10983) * manual publish jobs for releases, no changes for nightlies (#10977) * [blooms-db] Fix benchmarks (#10974) * Verify transaction against its block during import (#10954) * Better error message for rpc gas price errors (#10931) * Fix fork choice (#10837) * Fix compilation on recent nightlies (#10991)

* add more tx tests (#11038) * Fix parallel transactions race-condition (#10995) * Add blake2_f precompile (#11017) * [trace] introduce trace failed to Ext (#11019) * Edit publish-onchain.sh to use https (#11016) * Fix deadlock in network-devp2p (#11013) * EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) * xDai chain support and nodes list update (#10989) * EIP 2028: transaction gas lowered from 68 to 16 (#10987) * EIP-1344 Add CHAINID op-code (#10983) * manual publish jobs for releases, no changes for nightlies (#10977) * [blooms-db] Fix benchmarks (#10974) * Verify transaction against its block during import (#10954) * Better error message for rpc gas price errors (#10931) * tx-pool: accept local tx with higher gas price when pool full (#10901) * Fix fork choice (#10837) * Cleanup unused vm dependencies (#10787) * Fix compilation on recent nightlies (#10991)

* master: (70 commits) ethcore: remove `test-helper feat` from build (#11047) Include test-helpers from ethjson (#11045) [ethcore]: cleanup dependencies (#11043) add more tx tests (#11038) Fix parallel transactions race-condition (#10995) [ethcore]: make it compile without `test-helpers` feature (#11036) Benchmarks for block verification (#11035) Move snapshot related traits to their proper place (#11012) cleanup json crate (#11027) [spec] add istanbul test spec (#11033) [json-spec] make blake2 pricing spec more readable (#11034) Add blake2_f precompile (#11017) Add new line after writing block to hex file. (#10984) fix: remove unused error-chain (#11028) fix: remove needless use of itertools (#11029) Convert `std::test` benchmarks to use Criterion (#10999) Fix block detail updating (#11015) [trace] introduce trace failed to Ext (#11019) cli: update usage and version headers (#10924) [private-tx] remove unused rand (#11024) ...

ngotchac added 5 commits August 15, 2019 19:10

WiP : clear pending txs cache & tick in Miner

48070be

Fixed pending transactions

e9c110f

Revert debugging code

cc653a0

Add ToDo comment

55772a0

Remove commented-out code

75921ed

ngotchac added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. M4-core ⛓ Core client code / Rust. labels Aug 27, 2019

ngotchac added 7 commits August 27, 2019 15:07

Reverse LTO setting

897eef0

Merge branch 'master' into ng-tx-queue-fix

7b8807e

WiP

9abf8ac

Merge branch 'master' into ng-tx-queue-fix

2d0c706

Try to seal a new block if there are pending transactions

6980320

Try resealing only for internal imports

8080777

Remove logging

6b5c31d

tomusdrw reviewed Sep 4, 2019

View reviewed changes

Use AtomicU64 instead of Mutex<BlockNumber>

7004b54

ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Sep 6, 2019

ngotchac marked this pull request as ready for review September 6, 2019 14:10

tomusdrw reviewed Sep 9, 2019

View reviewed changes

ethcore/engines/instant-seal/src/lib.rs Outdated Show resolved Hide resolved

ngotchac added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 9, 2019

dvdplm reviewed Sep 10, 2019

View reviewed changes

ethcore/src/miner/miner.rs Outdated Show resolved Hide resolved

ethcore/src/miner/miner.rs Outdated Show resolved Hide resolved

ethcore/src/miner/miner.rs Show resolved Hide resolved

Remove TxQueue cache clear // Update AtomicUint logic

33e5809

ordian reviewed Sep 10, 2019

View reviewed changes

ethcore/engines/instant-seal/Cargo.toml Outdated Show resolved Hide resolved

Cargo.lock Outdated Show resolved Hide resolved

ngotchac added 3 commits September 10, 2019 11:26

Update comments in Miner

eea5d32

Revert import of parking_lot

40e243a

Merge branch 'master' into ng-tx-queue-fix

b633cb8

ordian added this to the 2.7 milestone Sep 10, 2019

ordian added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Sep 10, 2019

Update transaction-pool dependency

47c102b

ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Sep 11, 2019

dvdplm requested a review from tomusdrw September 11, 2019 09:30

ngotchac requested a review from ordian September 11, 2019 12:54

ordian approved these changes Sep 11, 2019

View reviewed changes

ngotchac added 2 commits September 11, 2019 15:29

Call directly update_sealing

159cc84

Call update_sealing directly

51c188d

tomusdrw approved these changes Sep 11, 2019

View reviewed changes

ngotchac merged commit 5e2def1 into master Sep 11, 2019

ordian added a commit that referenced this pull request Sep 12, 2019

Manual port of #10995

636ffb1

s3krit added a commit that referenced this pull request Sep 12, 2019

Manual port of #10995

d9159d6

This was referenced Sep 12, 2019

v2.6.3-beta #11040

Merged

v2.5.8-stable #11041

Merged

niklasad1 deleted the ng-tx-queue-fix branch September 13, 2019 13:19

dvdplm mentioned this pull request Oct 21, 2019

Make InstantSeal Instant again #11186

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix parallel transactions race-condition #10995

Fix parallel transactions race-condition #10995

ngotchac commented Aug 27, 2019 •

edited

Loading

tomusdrw left a comment

tomusdrw Sep 4, 2019

tomusdrw Sep 11, 2019

ngotchac Sep 11, 2019

tomusdrw Sep 11, 2019

tomusdrw left a comment

ngotchac commented Sep 9, 2019

dvdplm commented Sep 10, 2019

ordian commented Sep 11, 2019

Fix parallel transactions race-condition #10995

Fix parallel transactions race-condition #10995

Conversation

ngotchac commented Aug 27, 2019 • edited Loading

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Sep 4, 2019

Choose a reason for hiding this comment

tomusdrw Sep 11, 2019

Choose a reason for hiding this comment

ngotchac Sep 11, 2019

Choose a reason for hiding this comment

tomusdrw Sep 11, 2019

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

ngotchac commented Sep 9, 2019

dvdplm commented Sep 10, 2019

ordian commented Sep 11, 2019

ngotchac commented Aug 27, 2019 •

edited

Loading