[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p by skeees · Pull Request #13413 · bitcoin/bitcoin

skeees · 2018-06-07T15:22:52Z

This depends on #12934 and #13407: review those first

As a follow up to #12934 which introduces some armature code to make message passing based communication a bit easier and moves ProcessNewBlock into its own thread, this does similar for AcceptToMemoryPool() in p2p

If this approach seems reasonable, I think logical next steps are (in forthcoming prs) to try to reduce dependency on cs_main in p2p so that the benefits of this parallelism can be better realized

DrahtBot · 2018-06-07T19:33:00Z

Note to reviewers: This pull request conflicts with the following ones:

policy: Remove promiscuousmempoolflags #13527 (policy: Remove promiscuousmempoolflags by MarcoFalke)
doc: Rewrite some validation docs as lock annotations #13481 (doc: Rewrite some validation docs as lock annotations by MarcoFalke)
[bugfix] Fix encoding issue for Windows #13426 ([bugfix] Add u8path and u8string to fix Invalid wallet path with Chinese characters in windows #13103 by ken2812221)
[net] Thread safety annotations in net_processing #13423 ([net] Thread safety annotations in net_processing by skeees)
[net] Tighten scope in net_processing #13417 ([net] Tighten scope in net_processing by skeees)
[refactor, move-only-ish] Refactor mempool accept/reject logic #13407 ([refactor, move-only-ish] Refactor mempool accept/reject logic by skeees)
rpc: Add submitheader #13399 (rpc: Add submitblockheader by MarcoFalke)
Don't edit Chainparams after initialization #13311 (Don't edit Chainparams after initialization by jtimon)
Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. #13249 (Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. by practicalswift)
Break circular dependency: init -> * -> init by extracting shutdown.h #13235 (Break circular dependency: init -> * -> init by extracting shutdown.h by Empact)
Thread names in logs and deadlock debug tools (take 2) #13168 (Thread names in logs and deadlock debug tools (take 2) by jamesob)
Use common getPath method to create temp directory in tests. #13145 (Use common getPath method to create temp directory in tests. by winder)
net: Add Clang thread safety annotations for guarded variables in the networking code #13123 (net: Add Clang thread safety annotations for guarded variables in the networking code by practicalswift)
Add compile time checking for cs_main runtime locking assertions #13083 (Add compile time checking for cs_main runtime locking assertions by practicalswift)
Fix ScanForWalletTransactions to return an enum indicating scan result: success / failure / user_abort #13076 (Fix ScanForWalletTransactions to return an enum indicating scan result: success / failure / user_abort by Empact)
Interpret absense of prune= as prune=1 if there are pruned blocks #13029 (Interpret absense of prune= as prune=1 if there are pruned blocks by Sjors)
Bury bip9 deployments #12360 (Bury bip9 deployments by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot · 2018-06-15T20:08:09Z

Needs rebase

…nsumerQueue

…ayer

…ate()

Create separate functions to handle responses when a transaction received from a peer is accepted or rejected. This commit introduces one behavior change: Currently, if -promiscuousmempool is set (only allowed on regtest and testnet), it is possible for AcceptToMemoryPool() to return true, but for the CValidationState() to contain DoS points if the transaction failed validation with policy standard script flags but then passed with promiscuous flags. This will cause you to ban the peer who sent the transaction. This commit fixes this behavior by reseting the validation state in that one situation.

DrahtBot · 2018-07-07T08:37:35Z

Needs rebase

3339ba2 Make g_enable_bip61 a member variable of PeerLogicValidation (Jesse Cohen) 6690a28 Restrict as much as possible in net_processing to translation unit (Jesse Cohen) 1d4df02 [move-only] Move things only referenced in net_processing out of header file (Jesse Cohen) 02bbc05 Rescope g_enable_bip61 to net_processing (Jesse Cohen) Pull request description: As part of a larger effort to decouple net_processing and validation a bit, these are a bunch of simple scope cleanups. I've moved things out of the header file that are only referenced in net_processing and added static (or anonymous namespace) modifiers to everything possible in net_processing. There are a handful of functions which could be static except that they are exposed for the sake of unit testing - these are explicitly commented. There has been some discussion of a compile time annotation, but no conclusion has been reached on that yet. This is somewhat related to other prs #12934 #13413 #13407 and will be followed by prs that reduce reliance on cs_main to synchronize data structures which are translation unit local to net_processing Tree-SHA512: 46c9660ee4e06653feb42ba92189565b0aea17aac2375c20747c0d091054c63829cbf66d2daddf65682b58ce1d6922e23aefea051a7f2c8abbb6db253a609082

practicalswift · 2018-09-01T21:17:51Z

src/core/consumerthread.h

+
+                // resubmit it so that it gets a chance to get to the right thread
+                // when resubmitting, do not block and do not care about failures
+                // theres a potential deadlock where we try to push this to a queue thats


Typo found by codespell: theres (should be “there is”)

Also: “thats” should be “that is”

practicalswift · 2018-09-01T21:18:14Z

src/core/producerconsumerqueue.h

+
+        T ret;
+
+        // use a temporary so theres no side effecting code inside an assert which could be disabled


Same here: “theres” should be “there is”

maflcko · 2018-12-03T16:10:35Z

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

3339ba2 Make g_enable_bip61 a member variable of PeerLogicValidation (Jesse Cohen) 6690a28 Restrict as much as possible in net_processing to translation unit (Jesse Cohen) 1d4df02 [move-only] Move things only referenced in net_processing out of header file (Jesse Cohen) 02bbc05 Rescope g_enable_bip61 to net_processing (Jesse Cohen) Pull request description: As part of a larger effort to decouple net_processing and validation a bit, these are a bunch of simple scope cleanups. I've moved things out of the header file that are only referenced in net_processing and added static (or anonymous namespace) modifiers to everything possible in net_processing. There are a handful of functions which could be static except that they are exposed for the sake of unit testing - these are explicitly commented. There has been some discussion of a compile time annotation, but no conclusion has been reached on that yet. This is somewhat related to other prs bitcoin#12934 bitcoin#13413 bitcoin#13407 and will be followed by prs that reduce reliance on cs_main to synchronize data structures which are translation unit local to net_processing Tree-SHA512: 46c9660ee4e06653feb42ba92189565b0aea17aac2375c20747c0d091054c63829cbf66d2daddf65682b58ce1d6922e23aefea051a7f2c8abbb6db253a609082 Signed-off-by: Pasta <[email protected]> # Conflicts: # src/init.cpp # src/net_processing.cpp # src/net_processing.h # src/test/test_dash.cpp

skeees force-pushed the mempool-async branch 2 times, most recently from a0e87b1 to 0154564 Compare June 7, 2018 16:03

skeees mentioned this pull request Jun 7, 2018

[net] Tighten scope in net_processing #13417

Merged

This was referenced Jun 14, 2018

rpc: Avoid "duplicate" return value for invalid submitblock #13439

Merged

[bugfix] Fix encoding issue for Windows #13426

Closed

DrahtBot added the Needs rebase label Jun 15, 2018

skeees added 13 commits June 19, 2018 15:56

Implement a thread-safe FIFO (producer/consumer style) queue

9668666

Unit tests for ProducerConsumerQueue

52ac3b5

Add ConsumerThread: to consumer and operate on work from a ProducerCo…

6c62423

…nsumerQueue

ConsumerThread unit tests

fb022e3

ValidationLayer() - interface for calls into block validation

5d40aa8

Call ProcessNewBlock() asynchronously in a separate thread from p2p l…

8f0d086

…ayer

Replace all instances of ProcessNewBlock() with ValidationLayer.Valid…

0428d43

…ate()

Limit available scope of ProcessNewBlock to ValidationLayer (move-only)

f31b454

Fix whitespace in test_bitcoin.cpp (whitespace,move-only)

65ae792

[refactor] extract reusable logic in ValidationLayer into AsyncLayer

38f649a

ValidationLayer uses AsyncLayer

523d39f

Add MempoolLayer() for async invocation of AcceptToMemoryPool()

bd24661

skeees force-pushed the mempool-async branch 3 times, most recently from ff39194 to adcc640 Compare June 19, 2018 20:14

DrahtBot removed the Needs rebase label Jun 19, 2018

DrahtBot mentioned this pull request Jun 19, 2018

doc: Rewrite some validation docs as lock annotations #13481

Merged

DrahtBot mentioned this pull request Jun 19, 2018

[net] Thread safety annotations in net_processing #13423

Merged

Call AcceptToMemoryPool() asynchronously in net_processing

a71183b

skeees force-pushed the mempool-async branch from adcc640 to a71183b Compare June 20, 2018 13:28

DrahtBot mentioned this pull request Jun 26, 2018

policy: Remove promiscuousmempoolflags #13527

Merged

DrahtBot added the Needs rebase label Jul 7, 2018

practicalswift reviewed Sep 1, 2018

View reviewed changes

HashUnlimited mentioned this pull request Sep 11, 2018

[net] Tighten scope in net_processing chaincoin/chaincoin#517

Merged

DrahtBot added the Up for grabs label Dec 3, 2018

maflcko closed this Dec 3, 2018

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p#13413

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p#13413
skeees wants to merge 14 commits intobitcoin:masterfrom
skeees:mempool-async

skeees commented Jun 7, 2018

Uh oh!

DrahtBot commented Jun 7, 2018 •

edited

Loading

Uh oh!

DrahtBot commented Jun 15, 2018

Uh oh!

DrahtBot commented Jul 7, 2018

Uh oh!

practicalswift Sep 1, 2018

Uh oh!

practicalswift Sep 1, 2018

Uh oh!

practicalswift Sep 1, 2018

Uh oh!

maflcko commented Dec 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants


		T ret;

		// use a temporary so theres no side effecting code inside an assert which could be disabled

Conversation

skeees commented Jun 7, 2018

Uh oh!

DrahtBot commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 15, 2018

Uh oh!

DrahtBot commented Jul 7, 2018

Uh oh!

practicalswift Sep 1, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Sep 1, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Sep 1, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrahtBot commented Jun 7, 2018 •

edited

Loading