Clean up a few CConnman cs_vNodes/CNode things #9626

1. 
   TheBlueMatt commented at 10:19 pm on January 24, 2017: member

   This should fix the long-standing (read: satoshi-era) bug where we may not delete a node if we try to connect to it (via RPC) after its already connected as we’ll have duplicate refs.

   This also removes some unused functions (some of which miss a lock, some of which should otherwise be cleaned up for #9609).

   This also ensures that if we return a CNode* from FindNode, we are still holding cs_vNodes if we use it for anything aside from existance-checking, fixing a stupid-unlikely race where it might be deleted out from under us.

2. Delete some unused (and broken) functions in CConnman 5be01906e5
3. Ensure cs_vNodes is held when using the return value from FindNode 3c37dc40d3
4. 
   theuni commented at 11:40 pm on January 24, 2017: member

   Concept ACK. Imo these are all fixes for 0.14.

   Will review in detail.

5. in src/net.cpp: in caa5487cb7 outdated

   341@@ -342,8 +342,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
   342         CNode* pnode = FindNode((CService)addrConnect);
   343         if (pnode)
   344         {
   345-            pnode->AddRef();
   346-            return pnode;
   347+            return NULL;
   

   ---

   ---

   

   theuni commented at 11:46 pm on January 24, 2017:

   I think it makes sense to log “already connected” here, otherwise it looks like the attempt fell into a black hole.

   ---

   

   TheBlueMatt commented at 3:38 pm on January 25, 2017:

   Done.
6. in src/net.cpp: in caa5487cb7 outdated

   376                 if (pnode->addrName.empty()) {
   377                     pnode->addrName = std::string(pszDest);
   378                 }
   379                 CloseSocket(hSocket);
   380-                return pnode;
   381+                return NULL;
   

   ---

   ---

   

   theuni commented at 11:47 pm on January 24, 2017:

   Same here

   ---

   

   TheBlueMatt commented at 3:38 pm on January 25, 2017:

   Done.
7. in src/net.cpp: in caa5487cb7 outdated

   389@@ -392,10 +390,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
   390         pnode->nTimeConnected = GetTime();
   391         pnode->AddRef();
   392         GetNodeSignals().InitializeNode(pnode, *this);
   393-        {
   394-            LOCK(cs_vNodes);
   395-            vNodes.push_back(pnode);
   396-        }
   

   ---

   ---

   

   theuni commented at 11:54 pm on January 24, 2017:

   This isn’t an issue for 0.14 because it’s always been this way, but I really don’t like that we pass this out through our signals before adding it locally.

   So, no objection to this change, but I’d like to clean it up some post-0.14.

   ---

   

   TheBlueMatt commented at 3:39 pm on January 25, 2017:

   OK, moved the signals to the end as well.
8. 
   pstratem commented at 1:10 am on January 25, 2017: contributor

   utACK caa5487cb720c80f8e4a75e7cb77b8c4a1d34989
9. fanquake added this to the milestone 0.14.0 on Jan 25, 2017
10. fanquake added the label P2P on Jan 25, 2017
11. 
    theuni commented at 10:09 pm on January 25, 2017: member

    utACK after squash.
12. Do not add to vNodes until fOneShot/fFeeler/fAddNode have been set 236618061a
13. TheBlueMatt force-pushed on Jan 25, 2017
14. 
    TheBlueMatt commented at 11:59 pm on January 25, 2017: member

    Squashed without changes 1a1e4b9a4 - > 236618061
15. btcdrak approved
16. 
    btcdrak commented at 10:04 am on January 30, 2017: contributor

    Easy utACK 236618061a445d2cb11e722cfac5fdae5be26abb
17. laanwj merged this on Jan 30, 2017
18. laanwj closed this on Jan 30, 2017

    ---

19. laanwj referenced this in commit 36966a1c0e on Jan 30, 2017
20. 
    laanwj commented at 11:50 am on January 30, 2017: member

    utACK 2366180
21. practicalswift referenced this in commit 393822c017 on Nov 30, 2017
22. practicalswift referenced this in commit e78f6113b0 on Nov 30, 2017
23. practicalswift referenced this in commit 6d25f6c57d on Nov 30, 2017
24. practicalswift referenced this in commit 27ec5f3953 on Nov 30, 2017
25. practicalswift referenced this in commit 663f1eb714 on Feb 4, 2018
26. practicalswift referenced this in commit 17e6988df5 on Feb 8, 2018
27. practicalswift referenced this in commit a32bc30547 on Mar 11, 2018
28. random-zebra referenced this in commit 777638e7bc on Aug 27, 2020
29. random-zebra referenced this in commit cbd9271afb on Sep 7, 2020
30. LarryRuane referenced this in commit 503297f948 on Feb 24, 2021
31. LarryRuane referenced this in commit 024a94663c on Apr 1, 2021
32. zkbot referenced this in commit 1b5f17c900 on Apr 1, 2021
33. zkbot referenced this in commit 80e66e7daa on Apr 2, 2021
34. DrahtBot locked this on Sep 8, 2021

Contributors
TheBlueMatt theuni pstratem btcdrak laanwj

Labels
P2P

Milestone
0.14.0