net: Avoid locking cs_vNodes twice when calling FindNode(…). Add NodeExists(…). #11795

1. 
   practicalswift commented at 9:15 am on November 30, 2017: contributor

   Prior to this commit callers of FindNode(...) were required to be holding cs_vNodes in the cases where they used the returned CNode* for anything aside from existence-checking (see #9626).

   This resulted in locking cs_vNodes twice since FindNode(...) had a LOCK(cs_vNodes);.

   To solve this and to make the locking requirements more explicit this commit does the following:

   • Add explicit locking requirements for FindNode(...) using EXCLUSIVE_LOCKS_REQUIRED(cs_vNodes).
   • Remove the now redundant LOCK(cs_vNodes) in FindNode(...).
   • Add a method CConnman::NodeExists(...) for existence-checking. Calling this method does not require holding cs_vNodes.
2. fanquake added the label P2P on Nov 30, 2017
3. fanquake added the label Refactoring on Nov 30, 2017
4. practicalswift force-pushed on Nov 30, 2017
5. practicalswift force-pushed on Nov 30, 2017
6. 
   TheBlueMatt commented at 3:22 am on December 2, 2017: member

   Prefer #11604.
7. 
   practicalswift commented at 12:52 pm on December 3, 2017: contributor

   @TheBlueMatt But #11604 doesn’t touch FindNode(…) from what I can see? :-)
8. practicalswift force-pushed on Feb 4, 2018
9. 
   practicalswift commented at 1:44 pm on February 4, 2018: contributor

   Rebased!
10. practicalswift force-pushed on Feb 8, 2018
11. 
    practicalswift commented at 9:07 am on February 8, 2018: contributor

    Rebased!
12. net: Avoid locking cs_vNodes twice when calling FindNode(...). Add NodeExists(...).

    Prior to this commit callers of `FindNode(...)` were required to
    be holding `cs_vNodes` only in the cases where they used the
    returned `CNode*` for anything aside from existence-checking
    (see #9626).
    
    This resulted in locking `cs_vNodes` twice since `FindNode(...)`
    had a `LOCK(cs_vNodes);`.
    
    To solve this and to make the locking requirements more explicit this
    commit does the following:
    * Add explicit locking requirements for `FindNode(...)`
      using `EXCLUSIVE_LOCKS_REQUIRED(cs_vNodes)`.
    * Remove the now redundant `LOCK(cs_vNodes)` in `FindNode(...)`.
    * Add a method `CConnman::NodeExists(...)` for existence-checking.
      Calling this method does not require holding `cs_vNodes`.

    a32bc30547
13. practicalswift force-pushed on Mar 11, 2018
14. 
    practicalswift commented at 12:19 pm on March 20, 2018: contributor

    @TheBlueMatt Could you explain how #11604 overlaps with the FindNode(…) work in this PR? :-)
15. 
    practicalswift commented at 11:35 pm on July 11, 2018: contributor

    Friendly ping @TheBlueMatt :-)
16. DrahtBot closed this on Jul 21, 2018

    ---

17. 
    DrahtBot commented at 5:29 pm on July 21, 2018: member
18. DrahtBot reopened this on Jul 21, 2018

    ---

19. 
    sipa commented at 9:19 pm on July 21, 2018: member

    Opinion, @theuni?
20. 
    theuni commented at 8:33 pm on July 24, 2018: member

    I think it’s really confusing to require the lock for some functions, and forbid it for others.

    It’s also kinda a step in the wrong direction… ideally cs_vNodes wouldn’t be held while using a Node*. Instead, it should just have its refcount increased for the duration of its use.

21. 
    promag commented at 8:39 pm on July 24, 2018: member

    @theuni that’s how it’s done with wallets:

    0static CCriticalSection cs_wallets;
    1static std::vector<std::shared_ptr<CWallet>> vpwallets GUARDED_BY(cs_wallets);
    

    Where cs_wallets is locked when accessing/modifying vpwallets, not while a wallet is used.

22. 
    practicalswift commented at 8:42 am on August 1, 2018: contributor

    @sipa @theuni @promag Thanks for your input! Closing this PR due to lack of consensus ACK :-)
23. practicalswift closed this on Aug 1, 2018

    ---

24. practicalswift deleted the branch on Apr 10, 2021
25. DrahtBot locked this on Aug 18, 2022

Contributors
practicalswift TheBlueMatt DrahtBot sipa theuni promag

Labels
Refactoring P2P