Clean up a few CConnman cs_vNodes/CNode things #9626

TheBlueMatt · 2017-01-24T22:19:42Z

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.

theuni · 2017-01-24T23:40:21Z

Concept ACK. Imo these are all fixes for 0.14.

Will review in detail.

theuni · 2017-01-24T23:46:35Z

src/net.cpp

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


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

theuni · 2017-01-24T23:47:41Z

src/net.cpp

                if (pnode->addrName.empty()) {
                    pnode->addrName = std::string(pszDest);
                }
                CloseSocket(hSocket);
-                return pnode;
+                return NULL;


theuni · 2017-01-24T23:54:55Z

src/net.cpp

-        {
-            LOCK(cs_vNodes);
-            vNodes.push_back(pnode);
-        }


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.

OK, moved the signals to the end as well.

pstratem · 2017-01-25T01:10:32Z

utACK caa5487cb720c80f8e4a75e7cb77b8c4a1d34989

theuni · 2017-01-25T22:09:09Z

utACK after squash.

TheBlueMatt · 2017-01-25T23:59:32Z

Squashed without changes 1a1e4b9a4 - > 2366180

btcdrak

Easy utACK 2366180

2366180 Do not add to vNodes until fOneShot/fFeeler/fAddNode have been set (Matt Corallo) 3c37dc4 Ensure cs_vNodes is held when using the return value from FindNode (Matt Corallo) 5be0190 Delete some unused (and broken) functions in CConnman (Matt Corallo)

laanwj · 2017-01-30T11:50:17Z

utACK 2366180

…deExists(...). 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 bitcoin#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`.

e733939 [Cleanup] Remove CConnman::Copy(Release)NodeVector, now unused (random-zebra) b09da57 [Refactor] Proper CConnman encapsulation of mnsync (random-zebra) 219061e [Refactor] Decouple SyncWithNode from CMasternodeSync::Process() (random-zebra) e1f3620 [Refactor] Proper CConnman encapsulation of proposals / votes sync (random-zebra) f38c9f9 miner: don't try to access connman if it doesn't exist (Fuzzbawls) 92ab619 Address feedback (Fuzzbawls) 9d5346a Do not set an addr time penalty when a peer advertises itself. (Gregory Maxwell) fe12ff9 net: No longer send local address in addrMe (Wladimir J. van der Laan) 14d6917 net: misc header cleanups (Cory Fields) 34ba0de net: make proxy receives interruptible (Cory Fields) 1bd97ef net: make net processing interruptable (Fuzzbawls) e24b4cc net: make net interruptible (Cory Fields) 814d0de net: add CThreadInterrupt and InterruptibleSleep (Cory Fields) 1443f2a net: a few small cleanups before replacing boost threads (Cory Fields) 58eabe4 net: move MAX_FEELER_CONNECTIONS into connman (Cory Fields) 874baba Convert ForEachNode* functions to take a templated function argument rather than a std::function to eliminate std::function overhead (Jeremy Rubin) 9485ab0 Made the ForEachNode* functions in src/net.cpp more pragmatic and self documenting (Jeremy Rubin) c1e59ad minor net cleanups (Fuzzbawls) 07ae004 net: move vNodesDisconnected into CConnman (Cory Fields) 276c946 net: add nSendBufferMaxSize/nReceiveFloodSize to CConnection::Options (Cory Fields) 22a9aff net: Introduce CConnection::Options to avoid passing so many params (Cory Fields) e4891bf net: Drop StartNode/StopNode and use CConnman directly (Cory Fields) 431575c net: pass CClientUIInterface into CConnman (Cory Fields) 48de47e net: Pass best block known height into CConnman (Cory Fields) 15eed91 net: move max/max-outbound to CConnman (Cory Fields) 2bf0921 net: move semOutbound to CConnman (Cory Fields) 481929f net: move nLocalServices/nRelevantServices to CConnman (Cory Fields) bcee6ae net: move SendBufferSize/ReceiveFloodSize to CConnman (Cory Fields) 6865469 net: move send/recv statistics to CConnman (Cory Fields) 1cec418 net: SocketSendData returns written size (Cory Fields) 2bb9dfa net: move messageHandlerCondition to CConnman (Cory Fields) 9c5a0df net: move nLocalHostNonce to CConnman (Cory Fields) a1394ef net: move nLastNodeId to CConnman (Cory Fields) 3804c29 net: move whitelist functions into CConnman (Cory Fields) dbde9be net: create generic functor accessors and move vNodes to CConnman (Cory Fields) 2e02467 net: Add most functions needed for vNodes to CConnman (Cory Fields) 5667e61 net: move added node functions to CConnman (Cory Fields) 37487ed net: Add oneshot functions to CConnman (Cory Fields) facf878 net: move ban and addrman functions into CConnman (Cory Fields) 091aaf2 net: handle nodesignals in CConnman (Cory Fields) 1e9fa0f net: move OpenNetworkConnection into CConnman (Cory Fields) 573200f net: Move socket binding into CConnman (Cory Fields) 7762b97 net: Pass CConnection to wallet rather than using the global (Fuzzbawls) 00591b8 net: Pass CConnman around as needed (Cory Fields) 2cd3d39 net: Add rpc error for missing/disabled p2p functionality (Cory Fields) f08e316 net: Create CConnman to encapsulate p2p connections (Cory Fields) 66337dc net: move CBanDB and CAddrDB out of net.h/cpp (Fuzzbawls) 10969f6 gui: add NodeID to the peer table (Cory Fields) 58044ac Fix some locks (Pieter Wuille) f9f8926 Do not shadow variables in networking code (Pavel Janík) Pull request description: This is the finalization of the upstream PRs being tracked in #1374 to update much of our networking code to more closely follow upstream improvements. The following PRs are included here: - bitcoin#8466 Do not shadow variables in networking code - bitcoin#8606 Fix some locks - bitcoin#8085 Begin encapsulation - bitcoin#9289 drop boost::thread_group - bitcoin#8740 No longer send local address in addrMe - bitcoin#8661 Do not set an addr time penalty when a peer advertises itself Additionally, during conflict resolution and backporting of 8085, the following additional upstream PR was included: - bitcoin#8715 only delete CConnman if it's been created Still TODO in future PRs: - bitcoin#9037 - bitcoin#9441 - bitcoin#9609 - bitcoin#9626 - bitcoin#9708 - bitcoin#12381 - bitcoin#18584 ACKs for top commit: furszy: same here, code ACK e733939. Nice work ☕ . furszy: ACK e733939 . random-zebra: ACK e733939 and merging... Tree-SHA512: 0fc3cca76d9ddc13f75fc9d48c48d215e6c0e0381377c0318436176a0e0ead73b511e0061a4b63f7052874730b6da8b0ffc2c94cba034bcc39aad4212b69ee22

30d5c66 net: correct addrman logging (Fuzzbawls) 8a2b7fe Don't send layer2 messages to peers that haven't completed the handshake (Fuzzbawls) dc10100 [bugfix] Making tier two thread interruptable. (furszy) 2ae76aa Move CNode::addrLocal access behind locked accessors (Fuzzbawls) 470482f Move CNode::addrName accesses behind locked accessors (Fuzzbawls) 35365e1 Move [clean|str]SubVer writes/copyStats into a lock (Fuzzbawls) d816a86 Make nServices atomic (Matt Corallo) 8a66add Make nStartingHeight atomic (Matt Corallo) 567c9b5 Avoid copying CNodeStats to make helgrind OK with buggy std::string (Matt Corallo) aea5211 Make nTimeConnected const in CNode (Matt Corallo) cf46680 net: fix a few races. (Fuzzbawls) c916fcf net: add a lock around hSocket (Cory Fields) cc8a93c net: rearrange so that socket accesses can be grouped together (Cory Fields) 6f731dc Do not add to vNodes until fOneShot/fFeeler/fAddNode have been set (Matt Corallo) 07c8d33 Ensure cs_vNodes is held when using the return value from FindNode (Matt Corallo) 110a44b Delete some unused (and broken) functions in CConnman (Matt Corallo) 08a12e0 net: log an error rather than asserting if send version is misused (Cory Fields) cd8b82c net: Disallow sending messages until the version handshake is complete (Fuzzbawls) 54b454b net: don't run callbacks on nodes that haven't completed the version handshake (Cory Fields) 2be6877 net: deserialize the entire version message locally (Fuzzbawls) 444f599 Dont deserialize nVersion into CNode (Fuzzbawls) f30f10e net: remove cs_vRecvMsg (Fuzzbawls) 5812f9e net: add a flag to indicate when a node's send buffer is full (Fuzzbawls) 5ec4db2 net: Hardcode protocol sizes and use fixed-size types (Wladimir J. van der Laan) de87ea6 net: Consistent checksum handling (Wladimir J. van der Laan) d4bcd25 net: push only raw data into CConnman (Cory Fields) b79e416 net: add CVectorWriter and CNetMsgMaker (Cory Fields) 63c51d3 net: No need to check individually for disconnection anymore (Cory Fields) 07d8c7b net: don't send any messages before handshake or after fdisconnect (Cory Fields) 9adfc7f net: Set feelers to disconnect at the end of the version message (Cory Fields) f88c06c net: handle version push in InitializeNode (Cory Fields) 04d39c8 net: construct CNodeStates in place (Cory Fields) 40a6c5d net: remove now-unused ssSend and Fuzz (Cory Fields) 681c62d drop the optimistic write counter hack (Cory Fields) 9f939f3 net: switch all callers to connman for pushing messages (Cory Fields) 8f9011d connman is in charge of pushing messages (Cory Fields) f558bb7 serialization: teach serializers variadics (Cory Fields) 01ea667 net: Use deterministic randomness for CNode's nonce, and make it const (Cory Fields) de1ad13 net: constify a few CNode vars to indicate that they're threadsafe (Cory Fields) 34050a3 Move static global randomizer seeds into CConnman (Pieter Wuille) 1ce349f net: add a flag to indicate when a node's process queue is full (Fuzzbawls) 5581b47 net: add a new message queue for the message processor (Fuzzbawls) 701b578 net: rework the way that the messagehandler sleeps (Fuzzbawls) 7e55dbf net: Add a simple function for waking the message handler (Cory Fields) 47ea844 net: record bytes written before notifying the message processor (Cory Fields) ffd4859 net: handle message accounting in ReceiveMsgBytes (Cory Fields) 8cee696 net: log bytes recv/sent per command (Fuzzbawls) 754400e net: set message deserialization version when it's time to deserialize (Fuzzbawls) d2b8e0a net: make CMessageHeader a dumb storage class (Fuzzbawls) cc24eff net: remove redundant max sendbuffer size check (Fuzzbawls) 32ab0c0 net: wait until the node is destroyed to delete its recv buffer (Cory Fields) 6e3f71b net: only disconnect if fDisconnect has been set (Cory Fields) 1b0beb6 net: make GetReceiveFloodSize public (Cory Fields) 229697a net: make vRecvMsg a list so that we can use splice() (Fuzzbawls) d2d71ba net: fix typo causing the wrong receive buffer size (Cory Fields) 50bb09d Add test-before-evict discipline to addrman (Ethan Heilman) Pull request description: This is a combination of multiple upstream PRs focused on optimizing the P2P networking flow after the introduction of CConnman encapsulation, and a few older PRs that were previously missed to support the later optimizations. The PRs are as follows: - bitcoin#9037 - net: Add test-before-evict discipline to addrman - bitcoin#5151 - make CMessageHeader a dumb storage class - bitcoin#6589 - log bytes recv/sent per command - bitcoin#8688 - Move static global randomizer seeds into CConnman - bitcoin#9050 - net: make a few values immutable, and use deterministic randomness for the localnonce - bitcoin#8708 - net: have CConnman handle message sending - bitcoin#9128 - net: Decouple CConnman and message serialization - bitcoin#8822 - net: Consistent checksum handling - bitcoin#9441 - Net: Massive speedup. Net locks overhaul - bitcoin#9609 - net: fix remaining net assertions - bitcoin#9626 - Clean up a few CConnman cs_vNodes/CNode things - bitcoin#9698 - net: fix socket close race - bitcoin#9708 - Clean up all known races/platform-specific UB at the time PR was opened - Excluded bitcoin/bitcoin@512731b and bitcoin/bitcoin@d8f2b8a, to be done in a separate PR ACKs for top commit: furszy: code ACK 30d5c66 , testnet sync from scratch went well and tested with #1829 on top as well and all good. furszy: mainnet sync went fine, ACK 30d5c66 . random-zebra: ACK 30d5c66 and merging... Tree-SHA512: 09689554f53115a45f810b47ff75d887fa9097ea05992a638dbb6055262aeecd82d6ce5aaa2284003399d839b6f2c36f897413da96cfa2cd3b858387c3f752c1

zcash: cherry picked from commit 3c37dc4 zcash: bitcoin/bitcoin#9626

Bitcoin 0.14 locking PRs These are locking changes from upstream (bitcoin core) release 0.14, oldest to newest (when merged to the master branch). Each commit also includes a reference both to the PR and the upstream commit. - bitcoin/bitcoin#8472 - bitcoin/bitcoin#8606 - Excludes a lock move because we don't have bitcoin/bitcoin#7840 which this move was partially-reverting. - bitcoin/bitcoin#9230 - Only first commit (we don't have `LogTimestampStr` anymore). - bitcoin/bitcoin#9243 - Only the sixth commit, excluding `IsArgSet` locking (we haven't pulled that function in yet). - bitcoin/bitcoin#9626 - The cherry-picked commit does not match the upstream at all, but the resulting lock is useful. - bitcoin/bitcoin#9679 - bitcoin/bitcoin#9227 - bitcoin/bitcoin#9698 - Excludes change to `CConnman::PushMessage` in second commit (which we don't have yet). - bitcoin/bitcoin#9708 - bitcoin/bitcoin#9771

TheBlueMatt added 2 commits January 24, 2017 17:01

Delete some unused (and broken) functions in CConnman

5be0190

Ensure cs_vNodes is held when using the return value from FindNode

3c37dc4

theuni reviewed Jan 24, 2017

View reviewed changes

fanquake added this to the 0.14.0 milestone Jan 25, 2017

fanquake added the P2P label Jan 25, 2017

Do not add to vNodes until fOneShot/fFeeler/fAddNode have been set

2366180

TheBlueMatt force-pushed the 2017-01-remove-broken-unused-funcs branch from 1a1e4b9 to 2366180 Compare January 25, 2017 23:59

btcdrak approved these changes Jan 30, 2017

View reviewed changes

laanwj merged commit 2366180 into bitcoin:master Jan 30, 2017

maflcko mentioned this pull request Jan 30, 2017

net: fix remaining net assertions #9609

Merged

OlegGirko mentioned this pull request Jul 14, 2017

Backport Bitcoin PR#8113: Rework addnode behaviour dashpay/dash#1525

Merged

This was referenced Aug 22, 2017

Backport Bitcoin PR#9441: Net: Massive speedup. Net locks overhaul dashpay/dash#1586

Merged

Partially backport Bitcoin PR#9626: Clean up a few CConnman cs_vNodes/CNode things dashpay/dash#1591

Merged

fanquake mentioned this pull request Nov 20, 2017

qt: Remove redundant locks #11733

Merged

practicalswift mentioned this pull request Nov 30, 2017

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

Closed

codablock mentioned this pull request Jan 17, 2018

Backport missing PRs from Bitcoin 0.14 - Part 7 dashpay/dash#1856

Merged

dagurval mentioned this pull request Jun 8, 2018

More net cherries bitcoinxt/bitcoinxt#444

Merged

str4d mentioned this pull request Jun 24, 2020

Fix broken reference counting of CNode objects zcash/zcash#4577

Open

Fuzzbawls mentioned this pull request Aug 19, 2020

[P2P] Begin Network Encapsulation PIVX-Project/PIVX#1812

Merged

Fuzzbawls mentioned this pull request Sep 3, 2020

[Net] Massive network refactoring and speedup PIVX-Project/PIVX#1835

Merged

LarryRuane mentioned this pull request Feb 24, 2021

Bitcoin 0.14 locking PRs zcash/zcash#5009

Merged

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 1, 2021

Ensure cs_vNodes is held when using the return value from FindNode

024a946

zcash: cherry picked from commit 3c37dc4 zcash: bitcoin/bitcoin#9626

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Clean up a few CConnman cs_vNodes/CNode things #9626

Clean up a few CConnman cs_vNodes/CNode things #9626

Uh oh!

TheBlueMatt commented Jan 24, 2017

Uh oh!

theuni commented Jan 24, 2017

Uh oh!

theuni Jan 24, 2017

Uh oh!

TheBlueMatt Jan 25, 2017

Uh oh!

theuni Jan 24, 2017

Uh oh!

TheBlueMatt Jan 25, 2017

Uh oh!

theuni Jan 24, 2017

Uh oh!

TheBlueMatt Jan 25, 2017

Uh oh!

pstratem commented Jan 25, 2017

Uh oh!

theuni commented Jan 25, 2017

Uh oh!

TheBlueMatt commented Jan 25, 2017

Uh oh!

btcdrak left a comment

Uh oh!

laanwj commented Jan 30, 2017

Uh oh!

Uh oh!

Clean up a few CConnman cs_vNodes/CNode things #9626

Clean up a few CConnman cs_vNodes/CNode things #9626

Uh oh!

Conversation

TheBlueMatt commented Jan 24, 2017

Uh oh!

theuni commented Jan 24, 2017

Uh oh!

theuni Jan 24, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jan 25, 2017

Choose a reason for hiding this comment

Uh oh!

theuni Jan 24, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jan 25, 2017

Choose a reason for hiding this comment

Uh oh!

theuni Jan 24, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jan 25, 2017

Choose a reason for hiding this comment

Uh oh!

pstratem commented Jan 25, 2017

Uh oh!

theuni commented Jan 25, 2017

Uh oh!

TheBlueMatt commented Jan 25, 2017

Uh oh!

btcdrak left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jan 30, 2017

Uh oh!

Uh oh!