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

1. 
   skeees commented at 5:23 pm on June 6, 2018: contributor

   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.

2. skeees force-pushed on Jun 6, 2018
3. 
   MarcoFalke commented at 6:34 pm on June 6, 2018: member

   Concept ACK. I believe this makes dandelion easier to implement.
4. fanquake added the label Refactoring on Jun 7, 2018
5. 
   DrahtBot commented at 3:20 pm on June 7, 2018: member

   • #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.

6. 
   Empact commented at 9:23 pm on June 7, 2018: member

   Concept ACK. Could make this easier to review by adjusting to minimize changes to git diff --color-moved=dimmed_zebra --ignore-space-change head^
7. in src/net_processing.cpp:2206 in 4c9e9e7c74 outdated

   2301-                unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTx);
   2302-                if (nEvicted > 0) {
   2303-                    LogPrint(BCLog::MEMPOOL, "mapOrphan overflow, removed %u tx\n", nEvicted);
   2304-                }
   2305+        if (!AlreadyHave(inv)) {
   2306+            if(AcceptToMemoryPool(mempool, state, ptx, &fMissingInputs, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) {
   

   ---

   ---

   

   jamesob commented at 6:02 pm on June 19, 2018:

   if (
8. in src/net_processing.cpp:3017 in 4c9e9e7c74 outdated

   2982+            {
   2983+                int nDos = 0;
   2984+                if (stateDummy.IsInvalid(nDos) && nDos > 0)
   2985+                {
   2986+                    // Punish peer that gave us an invalid orphan tx
   2987+                    Misbehaving(fromPeer, nDos);
   

   ---

   ---

   

   jamesob commented at 5:18 pm on June 20, 2018:

   This seems to require cs_main, which is missing from this function’s EXCLUSIVE_LOCKS_REQUIRED annotation.
9. 
   jamesob commented at 6:10 pm on June 20, 2018: member

   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).
10. skeees force-pushed on Jun 20, 2018
11. 
    skeees commented at 8:16 pm on June 20, 2018: contributor

    Thanks @jamesob - addressed comments
12. skeees force-pushed on Jun 22, 2018
13. jamesob approved
14. 
    jamesob commented at 8:03 pm on June 22, 2018: member

    utACK https://github.com/bitcoin/bitcoin/pull/13407/commits/2567ef43f3efd99ed08a5eb841251ed9111732d3

    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.

15. 
    sipa commented at 11:52 pm on June 26, 2018: member

    Concept ACK
16. 
    Empact commented at 8:59 pm on July 2, 2018: member

    Could benefit from clang-format selectively applied to the new code (e.g. the static method declarations).
17. sipa referenced this in commit 1e90862f5d on Jul 14, 2018
18. DrahtBot added the label Needs rebase on Jul 14, 2018
19. skeees force-pushed on Jul 25, 2018
20. DrahtBot removed the label Needs rebase on Jul 25, 2018
21. 
    skeees commented at 5:47 pm on July 25, 2018: contributor

    rebased
22. DrahtBot added the label Needs rebase on Aug 7, 2018
23. 
    MarcoFalke commented at 5:15 pm on August 10, 2018: member

    Needs rebase

    due to “policy: Remove promiscuousmempoolflags #13527”

24. skeees force-pushed on Aug 12, 2018
25. 
    skeees commented at 2:02 pm on August 12, 2018: contributor

    rebased (dropped cf36b6) as @MarcoFalke has addressed that in #13527
26. DrahtBot removed the label Needs rebase on Aug 12, 2018
27. [refactor, move-only-ish] Refactor mempool accept/reject logic

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

    4e918b201a
28. skeees force-pushed on Aug 12, 2018
29. 
    DrahtBot commented at 3:36 pm on September 4, 2018: member
30. DrahtBot added the label Needs rebase on Sep 4, 2018
31. DrahtBot added the label Up for grabs on Dec 12, 2018
32. 
    DrahtBot commented at 7:23 pm on December 12, 2018: member
33. DrahtBot closed this on Dec 12, 2018

    ---

34. laanwj removed the label Needs rebase on Oct 24, 2019
35. PastaPastaPasta referenced this in commit 5c6fb6e75c on Apr 15, 2020
36. PastaPastaPasta referenced this in commit a3f0f3cf63 on Apr 16, 2020
37. PastaPastaPasta referenced this in commit 504b696b78 on Apr 16, 2020
38. PastaPastaPasta referenced this in commit 7e588c7594 on Apr 19, 2020
39. PastaPastaPasta referenced this in commit 6cce7b22db on May 10, 2020
40. PastaPastaPasta referenced this in commit abc246a698 on May 12, 2020
41. PastaPastaPasta referenced this in commit 5b72c199ff on Jun 9, 2020
42. PastaPastaPasta referenced this in commit 9c6d174c4d on Jun 9, 2020
43. PastaPastaPasta referenced this in commit cfac0a9f3f on Jun 10, 2020
44. PastaPastaPasta referenced this in commit c26f14a99c on Jun 11, 2020
45. MarcoFalke locked this on Dec 16, 2021

Contributors
skeees MarcoFalke DrahtBot Empact jamesob sipa

Labels
Refactoring Up for grabs