Make orphan processing interruptible #15644

1. 
   sipa commented at 4:27 pm on March 22, 2019: member

   As individual orphan transactions can be relatively expensive to handle, it’s undesirable to process all of them (max 100) as soon as the parent becomes available, as it pegs the net processing the whole time.

   Change this by interrupting orphan handling after every transactions, and continue in the next processing slot of the peer that gave us the parent - similar to how getdata processing works now. Messages from other peers arriving in the mean time are processed normally, but other messages from the peer that gave us the parent have to wait until all orphan processing is done.

2. fanquake added the label P2P on Mar 22, 2019
3. 
   DrahtBot commented at 5:31 pm on March 22, 2019: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #15606 ([experimental] UTXO snapshots by jamesob)
   • #15437 (p2p: Remove BIP61 reject messages by MarcoFalke)
   • #15253 (Net: Consistently log outgoing INV messages by Empact)
   • #15141 (Rewrite DoS interface between validation and net_processing by sdaftuar)

   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.

4. in src/net_processing.cpp:3183 in ebf33310c3 outdated

   3178@@ -3169,11 +3179,20 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
   3179     if (!pfrom->vRecvGetData.empty())
   3180         ProcessGetData(pfrom, chainparams, connman, interruptMsgProc);
   3181 
   3182+    if (!pfrom->orphan_work_queue.empty()) {
   3183+        std::list<CTransactionRef> removed_txn;
   3184+        ProcessOrphanTx(connman, pfrom->orphan_work_queue, pfrom->orphan_work_set, removed_txn);
   3185+        for (const CTransactionRef& removedTx : removed_txn) {
   3186+            AddToCompactExtraTransactions(removedTx);
   

   ---

   ---

   

   MarcoFalke commented at 5:41 pm on March 22, 2019:

   0net_processing.cpp:3186:13: error: calling function 'AddToCompactExtraTransactions' requires holding mutex 'g_cs_orphans' exclusively [-Werror,-Wthread-safety-analysis]
   1            AddToCompactExtraTransactions(removedTx);
   2            ^
   

   ---

   

   sipa commented at 6:12 pm on March 22, 2019:

   Fixed.
5. in src/net_processing.cpp:1716 in ebf33310c3 outdated

   1712@@ -1713,6 +1713,70 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
   1713     return true;
   1714 }
   1715 
   1716+void static ProcessOrphanTx(CConnman* connman, std::deque<std::set<CTransactionRef>::iterator>& orphan_work_queue, std::set<CTransactionRef>& orphan_work_set, std::list<CTransactionRef>& removed_txn)
   

   ---

   ---

   

   practicalswift commented at 6:00 pm on March 22, 2019:

   Missing locking annotation?

   ---

   

   sipa commented at 6:12 pm on March 22, 2019:

   Fixed.
6. sipa force-pushed on Mar 22, 2019
7. Simplify orphan processing in preparation for interruptibility 9453018fdc
8. [MOVEONLY] Move processing of orphan queue to ProcessOrphanTx 6e051f3d32
9. Interrupt orphan processing after every transaction

   This makes orphan processing work like handling getdata messages:
   After every actual transaction validation attempt, interrupt
   processing to deal with messages arriving from other peers.

   866c8058a7
10. sipa force-pushed on Mar 23, 2019
11. 
    gmaxwell commented at 0:32 am on March 25, 2019: contributor

    ACK
12. 
    TheBlueMatt commented at 4:48 pm on March 26, 2019: member

    utACK
13. 
    sdaftuar commented at 6:20 pm on March 28, 2019: member

    ACK 866c8058a706931f025335b3e794ed2f4d287918
14. 
    MarcoFalke commented at 6:39 pm on March 28, 2019: member

    utACK 866c8058a706931f025335b3e794ed2f4d287918
15. MarcoFalke added the label Needs backport on Mar 28, 2019
16. MarcoFalke added this to the milestone 0.18.0 on Mar 28, 2019
17. MarcoFalke referenced this in commit 6355214fd7 on Mar 28, 2019
18. MarcoFalke referenced this in commit bb60121da1 on Mar 28, 2019
19. MarcoFalke referenced this in commit 50c56f2fcf on Mar 28, 2019
20. in src/net_processing.cpp:2345 in 9453018fdc outdated

    2341@@ -2342,8 +2342,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
    2342             return true;
    2343         }
    2344 
    2345-        std::deque<COutPoint> vWorkQueue;
    2346-        std::vector<uint256> vEraseQueue;
    2347+        std::set<uint256> orphan_work_set;
    

    ---

    ---

    

    promag commented at 3:31 pm on March 29, 2019:

    Commit 9453018

    Why std::set? Are duplicate expected?

    ---

    

    sipa commented at 6:20 pm on March 29, 2019:

    Yes.
21. in src/net_processing.cpp:2346 in 9453018fdc outdated

    2341@@ -2342,8 +2342,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
    2342             return true;
    2343         }
    2344 
    2345-        std::deque<COutPoint> vWorkQueue;
    2346-        std::vector<uint256> vEraseQueue;
    2347+        std::set<uint256> orphan_work_set;
    2348+
    

    ---

    ---

    

    promag commented at 3:31 pm on March 29, 2019:

    Commit 9453018:

    nit, remove empty line.

    ---

    

    sipa commented at 6:26 pm on March 29, 2019:

    Meh.
22. in src/net_processing.cpp:3178 in 866c8058a7

    3174@@ -3169,11 +3175,21 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
    3175     if (!pfrom->vRecvGetData.empty())
    3176         ProcessGetData(pfrom, chainparams, connman, interruptMsgProc);
    3177 
    3178+    if (!pfrom->orphan_work_set.empty()) {
    

    ---

    ---

    

    promag commented at 3:48 pm on March 29, 2019:

    Commit 866c805

    Not sure if orphan_work_set should be guarded by g_cs_orphans?

    ---

    

    sipa commented at 6:25 pm on March 29, 2019:

    No, it’s only accessed from the network processing thread like vRecvGetData, so it doesn’t need locking.

    Ideally it should move to net_processing’s structures instead of CNode, along with a bunch of other things, but I wanted to keep the diff small for now.

23. 
    promag commented at 3:49 pm on March 29, 2019: member

    Refactor LGTM, some comments though.
24. 
    promag commented at 6:41 pm on March 29, 2019: member

    utACK 866c805. Verified refactor in 9453018fdc8f02d42832374bcf1d6e3a1df02281 and moved code in 6e051f3d323af1d209c02e7a4319834f1947ffa7. Not so sure about change in 866c8058a706931f025335b3e794ed2f4d287918 just because I’m not familiar with net processing.
25. MarcoFalke referenced this in commit 35477e9e4e on Apr 1, 2019
26. MarcoFalke merged this on Apr 1, 2019
27. MarcoFalke closed this on Apr 1, 2019

    ---

28. fanquake removed the label Needs backport on Apr 2, 2019
29. 
    fanquake commented at 12:06 pm on April 2, 2019: member

    Backported in #15691.
30. HashUnlimited referenced this in commit 33a3d7eea5 on Apr 19, 2019
31. HashUnlimited referenced this in commit 8d7ca76bcb on Apr 19, 2019
32. HashUnlimited referenced this in commit 408f6290b1 on Apr 19, 2019
33. HashUnlimited referenced this in commit 51dab933bd on Aug 23, 2019
34. HashUnlimited referenced this in commit 1d4566d04a on Aug 23, 2019
35. HashUnlimited referenced this in commit 3b3f68d79d on Aug 23, 2019
36. deadalnix referenced this in commit 304dfe481a on May 22, 2020
37. deadalnix referenced this in commit 5877d0a945 on May 23, 2020
38. deadalnix referenced this in commit c0599ad713 on May 23, 2020
39. fanquake added the label Review club on Jul 15, 2020
40. Munkybooty referenced this in commit 3a79bee320 on Sep 27, 2021
41. DrahtBot locked this on Feb 15, 2022

Contributors
sipa DrahtBot MarcoFalke practicalswift gmaxwell TheBlueMatt sdaftuar promag fanquake

Labels
P2P Review club

Milestone
0.18.0