[refactor, move-only-ish] Refactor mempool accept/reject logic #13407

skeees · 2018-06-06T17:23:35Z

This commit is almost a move-only* and a fairly straightforward refactor which moves the p2p logic when a transaction received over the wire is accepted or rejected into their own functions instead of being embedded directly in ProcessMessages().

Motivation: as a follow-up to #12934, I'm working on putting mempool acceptance behind a similar type of asynchronous queue. In order to do that, ATMP response logic in p2p needs to be cleanly separated out into separate functions.

*Note that 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.

maflcko · 2018-06-06T18:34:21Z

Concept ACK. I believe this makes dandelion easier to implement.

DrahtBot · 2018-06-07T15:20:40Z

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

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)

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.

Empact · 2018-06-07T21:23:20Z

Concept ACK. Could make this easier to review by adjusting to minimize changes to git diff --color-moved=dimmed_zebra --ignore-space-change head^

jamesob

Looks good, though it would be nice to at least separate the behavior change and the refactoring into separate commits (if not separate PRs - though I can see how the process here motivates you to bundle the two).

jamesob · 2018-06-19T18:02:52Z

src/net_processing.cpp

-                    LogPrint(BCLog::MEMPOOL, "mapOrphan overflow, removed %u tx\n", nEvicted);
-                }
+        if (!AlreadyHave(inv)) {
+            if(AcceptToMemoryPool(mempool, state, ptx, &fMissingInputs, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) {


jamesob · 2018-06-20T17:18:41Z

src/net_processing.cpp

+                if (stateDummy.IsInvalid(nDos) && nDos > 0)
+                {
+                    // Punish peer that gave us an invalid orphan tx
+                    Misbehaving(fromPeer, nDos);


This seems to require cs_main, which is missing from this function's EXCLUSIVE_LOCKS_REQUIRED annotation.

skeees · 2018-06-20T20:16:08Z

Thanks @jamesob - addressed comments

jamesob

utACK 2567ef4

In my last set of comments, I reviewed https://github.com/bitcoin/bitcoin/commit/4c9e9e7c7468b60172d4736455a0b5b11ca4f354.diff (which can be verified by checking the URL of the "View Changes" button associated with those comments). The change since then only addresses my three comments.

The changes in this PR should be pretty well covered by test/functional/mempool_*. The only non-move difference I can see beyond the noted -promiscuousmempool change are two assertions introduced in ProcessMessage (https://github.com/bitcoin/bitcoin/pull/13407/files#diff-eff7adeaec73a769788bb78858815c91R2207, https://github.com/bitcoin/bitcoin/pull/13407/files#diff-eff7adeaec73a769788bb78858815c91R2210) which look sensible.

Generally, I think this kind of refactoring is good - extracting functions makes important code like this easier to follow and more testable. Not to mention the motivating change (making mempool acceptance async) is very worthwhile.

sipa · 2018-06-26T23:52:49Z

Concept ACK

Empact · 2018-07-02T20:59:01Z

Could benefit from clang-format selectively applied to the new code (e.g. the static method declarations).

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

skeees · 2018-07-25T17:47:53Z

rebased

maflcko · 2018-08-10T17:15:47Z

Needs rebase

due to "policy: Remove promiscuousmempoolflags #13527"

skeees · 2018-08-12T14:02:03Z

rebased (dropped cf36b6) as @MarcoFalke has addressed that in #13527

Create separate functions to handle responses when a transaction received from a peer is accepted or rejected.

DrahtBot · 2018-09-04T15:36:52Z

Needs rebase

DrahtBot · 2018-12-12T19:23:19Z

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 <pasta@dashboost.org> # Conflicts: # src/init.cpp # src/net_processing.cpp # src/net_processing.h # src/test/test_dash.cpp

skeees force-pushed the atmp-p2p-refactor branch from d17d460 to 4c9e9e7 Compare June 6, 2018 18:00

fanquake added the Refactoring label Jun 7, 2018

This was referenced Jun 7, 2018

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

Closed

[net] Tighten scope in net_processing #13417

Merged

DrahtBot mentioned this pull request Jun 19, 2018

[net] Thread safety annotations in net_processing #13423

Merged

jamesob reviewed Jun 20, 2018

View reviewed changes

skeees force-pushed the atmp-p2p-refactor branch from 4c9e9e7 to 49a942d Compare June 20, 2018 20:15

skeees force-pushed the atmp-p2p-refactor branch from 49a942d to 2567ef4 Compare June 22, 2018 18:02

jamesob approved these changes Jun 22, 2018

View reviewed changes

maflcko mentioned this pull request Jun 23, 2018

policy: Remove promiscuousmempoolflags #13527

Merged

DrahtBot added the Needs rebase label Jul 14, 2018

skeees force-pushed the atmp-p2p-refactor branch from 2567ef4 to 65ad5db Compare July 25, 2018 15:16

DrahtBot removed the Needs rebase label Jul 25, 2018

DrahtBot added the Needs rebase label Aug 7, 2018

skeees force-pushed the atmp-p2p-refactor branch from 65ad5db to 37c9f74 Compare August 12, 2018 14:01

DrahtBot removed the Needs rebase label Aug 12, 2018

[refactor, move-only-ish] Refactor mempool accept/reject logic

4e918b2

Create separate functions to handle responses when a transaction received from a peer is accepted or rejected.

skeees force-pushed the atmp-p2p-refactor branch from 37c9f74 to 4e918b2 Compare August 12, 2018 15:11

DrahtBot added the Needs rebase label Sep 4, 2018

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 12, 2018

DrahtBot closed this Dec 12, 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

Uh oh!

[refactor, move-only-ish] Refactor mempool accept/reject logic #13407

[refactor, move-only-ish] Refactor mempool accept/reject logic #13407

Uh oh!

skeees commented Jun 6, 2018 •

edited

Loading

Uh oh!

maflcko commented Jun 6, 2018

Uh oh!

DrahtBot commented Jun 7, 2018 •

edited

Loading

Uh oh!

Empact commented Jun 7, 2018

Uh oh!

jamesob left a comment

Uh oh!

jamesob Jun 19, 2018

Uh oh!

jamesob Jun 20, 2018

Uh oh!

skeees commented Jun 20, 2018

Uh oh!

jamesob left a comment

Uh oh!

sipa commented Jun 26, 2018

Uh oh!

Empact commented Jul 2, 2018 •

edited

Loading

Uh oh!

skeees commented Jul 25, 2018

Uh oh!

maflcko commented Aug 10, 2018

Uh oh!

skeees commented Aug 12, 2018

Uh oh!

DrahtBot commented Sep 4, 2018

Uh oh!

DrahtBot commented Dec 12, 2018

Uh oh!

Uh oh!

[refactor, move-only-ish] Refactor mempool accept/reject logic #13407

[refactor, move-only-ish] Refactor mempool accept/reject logic #13407

Uh oh!

Conversation

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

Uh oh!

Uh oh!

maflcko commented Jun 6, 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!

Empact commented Jun 7, 2018

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

jamesob Jun 20, 2018

Choose a reason for hiding this comment

Uh oh!

skeees commented Jun 20, 2018

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 26, 2018

Uh oh!

Empact commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skeees commented Jul 25, 2018

Uh oh!

maflcko commented Aug 10, 2018

Uh oh!

skeees commented Aug 12, 2018

Uh oh!

DrahtBot commented Sep 4, 2018

Uh oh!

DrahtBot commented Dec 12, 2018

Uh oh!

Uh oh!

skeees commented Jun 6, 2018 •

edited

Loading

DrahtBot commented Jun 7, 2018 •

edited

Loading

Empact commented Jul 2, 2018 •

edited

Loading