net: Add missing cs_vNodes lock #18458

1. 
   MarcoFalke commented at 2:14 pm on March 28, 2020: member

   Fixes #18457
2. MarcoFalke force-pushed on Mar 28, 2020
3. DrahtBot added the label P2P on Mar 28, 2020
4. laanwj added this to the milestone 0.20.0 on Mar 28, 2020
5. in src/net.cpp:93 in 8888d9f0ac outdated

   89@@ -90,6 +90,9 @@ std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost);
   90 static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {};
   91 std::string strSubVersion;
   92 
   93+extern RecursiveMutex cs_main;
   

   ---

   ---

   

   laanwj commented at 4:00 pm on March 28, 2020:

   It seems like kind of a layer violation to import these here, but, I don’t know a better solution either.

   ---

   

   MarcoFalke commented at 4:32 pm on March 28, 2020:

   Agree that it is a layer violation. If I included it via the header, it would be a circular dependency according to some linter we run. But the same pattern is used in a lot of other places, see git grep 'extern RecursiveMutex cs_main;'. Those philosophical locking “issues” and code dependencies/layer violations can be fixed later, if needed.

   ---

   

   sipa commented at 6:48 pm on March 28, 2020:

   This is pretty ugly, but I see it may be needed. Agreed with fixing it this way for now.

   FWIW, IMO having an ’extern’ like this is as much a circular dependency as what you’re trying to avoid, just one that isn’t detected by the linter.

   ---

   

   MarcoFalke commented at 7:16 pm on March 28, 2020:

   At least with extern we can avoid including the full header and littering the namespace here ;)

   ---

   

   MarcoFalke commented at 1:36 pm on March 29, 2020:

   Fixed
6. 
   practicalswift commented at 12:17 pm on March 29, 2020: contributor

   Concept ACK

   Very nice to see a NO_THREAD_SAFETY_ANALYSIS annotation go :)

   FWIW:

   0$ git grep -E "^[^/]*NO_THREAD_SAFETY_ANALYSIS" -- ":(exclude)src/leveldb/" ":(exclude)src/threadsafety.h"
   1src/net.h:    void Stop() NO_THREAD_SAFETY_ANALYSIS;
   2src/wallet/wallet.h:    CAmount GetAvailableCredit(bool fUseCache = true, const isminefilter& filter = ISMINE_SPENDABLE) const NO_THREAD_SAFETY_ANALYSIS;
   3src/wallet/wallet.h:    std::set<uint256> GetConflicts() const NO_THREAD_SAFETY_ANALYSIS;
   4src/wallet/wallet.h:    int GetDepthInMainChain() const NO_THREAD_SAFETY_ANALYSIS;
   

7. MarcoFalke force-pushed on Mar 29, 2020
8. 
   MarcoFalke commented at 1:36 pm on March 29, 2020: member

   Fixed circular dependency as requested by @laanwj and @sipa
9. MarcoFalke force-pushed on Mar 29, 2020
10. MarcoFalke force-pushed on Mar 29, 2020
11. 
    promag commented at 3:07 pm on March 29, 2020: member

    Concept ACK.

    How about split CConnman::Stop? In Shutdown(NodeContext& node) we would then have:

    0if (node.connman) {
    1    node.connman->BeforeStop();
    2    LOCK2(::cs_main, ::g_cs_orphans);
    3    node.connman->Stop();
    4}
    

    You could also add annotation to Stop.

12. net: Add missing cs_vNodes lock fa369651c5
13. MarcoFalke force-pushed on Mar 29, 2020
14. 
    MarcoFalke commented at 3:51 pm on March 29, 2020: member

    Took suggestion by @promag

    You could also add annotation to Stop.

    No, that would be incorrect. The locks are not needed for the correct functionality of the connection manager itself. They are only needed when the connection manager is used in the context of a full node that also runs net processing and a scheduler. I think init.cpp is the best place to document this, since that is the place where the full node gets pieced together.

15. 
    promag commented at 4:01 pm on March 29, 2020: member

    Code review ACK fa369651c5523f393e0bfcfa328b8e27006711aa.
16. 
    DrahtBot commented at 7:34 pm on March 29, 2020: 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:

    • #14053 (Add address-based index (attempt 4?) by marcinja)

    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.

17. 
    MarcoFalke commented at 4:54 pm on April 2, 2020: member

    Would be nice to get one or two more ACKs on this
18. 
    promag commented at 11:29 pm on April 2, 2020: member

    ACK ACK, you are welcome.
19. 
    laanwj commented at 6:54 pm on April 6, 2020: member

    ACK fa369651c5523f393e0bfcfa328b8e27006711aa
20. laanwj merged this on Apr 6, 2020
21. laanwj closed this on Apr 6, 2020

    ---

22. MarcoFalke deleted the branch on Apr 6, 2020
23. sidhujag referenced this in commit 81f3ca7b68 on Apr 8, 2020
24. jasonbcox referenced this in commit 781244d1ff on Nov 18, 2020
25. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke laanwj sipa practicalswift promag DrahtBot

Labels
P2P

Milestone
0.20.0