net: fix race condition in self-connect detection #30394

theStack · 2024-07-04T19:06:31Z

This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).

Initiating an outbound network connection currently involves the following steps after the socket connection is established (see CConnman::OpenNetworkConnection method):

set up node state
queue VERSION message (both steps 1 and 2 happen in InitializeNode)
add new node to vector m_nodes

If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally before the node object is added to the connection manager's m_nodes vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in m_nodes yet (see CConnman::CheckIncomingNonce).

Fix this by swapping the order of 2. and 3., by taking the PushNodeVersion call out of InitializeNode and doing that in the SendMessages method instead, which is only called for CNode instances in m_nodes.

The temporarily reverted test introduced in #30362 is readded. Fixes #30368.

Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see #30368 (comment) ff. and #30394 (comment)).

DrahtBot · 2024-07-04T19:06:34Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	naiyoma, mzumsande, tdb3, glozow, dergoegge
Stale ACK	maflcko, furszy, sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30111 (locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip by glozow)

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.

tdb3

Approach ACK

Great job finding the bug and fixing it.

To sanity check, re-arranged calls to mimick failure recreation (#30368 (comment)). p2p_handshake failed as expected after the re-arrange.

diff --git a/src/net.cpp b/src/net.cpp
index a10be3e33a..6ed167d6fd 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2921,6 +2921,8 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
     pnode->grantOutbound = std::move(grant_outbound);
 
     m_msgproc->InitializeNode(*pnode, nLocalServices);
+    m_msgproc->SendInitialMessages(*pnode);
+    std::this_thread::sleep_for(std::chrono::seconds{1});
     {
         LOCK(m_nodes_mutex);
         m_nodes.push_back(pnode);
@@ -2928,7 +2930,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
         // update connection count by network
         if (pnode->IsManualOrFullOutboundConn()) ++m_network_conn_counts[pnode->addr.GetNetwork()];
     }
-    m_msgproc->SendInitialMessages(*pnode);
+    //m_msgproc->SendInitialMessages(*pnode);
 }

Also started on signet, connected to peers, and synced the latest ~1500 blocks successfully.

The name SendInitialMessages() seems reasonable to me, since it's generic enough to adapt to future change without rename.

I'd like to double check @vasild's comment (#30368 (comment)) before full ACK.

In case you settle with adding to m_nodes before PushNodeVersion() and I do not manage to review, it is good to ensure that no code will be upset by an existence of an (oubound) entry in m_nodes that does not have version pushed.

maflcko · 2024-07-05T09:36:00Z

ACK 7c9684a

src/net.h

theStack · 2024-07-08T17:38:32Z

Thanks for the reviews! I force-pushed following @dergoegge's suggestion of doing the version message push in SendMessages instead, with the advantage of not needing to change the NetEventsInterface (sorry for invalidating the ACK @maflcko).
The previous version is still available on the following branch: https://github.com/theStack/bitcoin/tree/202407-p2p-fix_selfdetection_racecond_oldversion

src/net.h

src/net_processing.cpp

dergoegge

Code review ACK fb587ce

maflcko

left two nits.

test/functional/p2p_handshake.py

src/net_processing.cpp

maflcko · 2024-07-09T12:34:04Z

ACK fb587ce

furszy

Code review ACK fb587ce

Nice finding.

glozow

lgtm, do you plan to take any of the suggestions?

src/net.h

theStack · 2024-07-09T15:40:17Z

lgtm, do you plan to take any of the suggestions?

Tackled all of the ordinary and nitty nits (thanks Marco and Gloria), re-review should be trivial.

sipa · 2024-07-09T15:55:57Z

utACK a3122d3, only reviewed the test change lightly.

mzumsande

I wonder if this could this lead to problems with "non-shy" software that would send VERSION immediately on incoming connections (which bitcoin core doesn't do thanks to Hal Finney's #101, but alternative clients might).

When Bitcoin Core makes an outgoing connection after this patch, it won't send out the version message immediately, so it could receive a version message before sending one. In that case, I think that ProcessMessage could send out other messages (WTXIDRELAY etc.) before sending the version message, which would be a protocol violation.

One possible fix for this might be to not process messages of outbound peers before having sent the version message.

I didn't test any of this, this is just from reading the code.

theStack · 2024-07-09T17:23:12Z

@mzumsande: Good catch! With that in my mind, I see the following paths forward:

Queue outbound VERSION message immediately in CConnman::OpenNetworkConnection (as done in a previous version of this PR: https://github.com/theStack/bitcoin/tree/202407-p2p-fix_selfdetection_racecond_oldversion, extending the NetEventsInterface with a new SendInitialMessages method, called at the end)
Adapt the current state of this PR to only start processing messages of outbound peers after m_outbound_version_message_sent is set (as you suggested above)
Keep it as it is, as there are probably no relevant node implementations out there that are still non-shy (light clients generally don't accept inbound connections as far as I'm aware, so they wouldn't be affected by it)

Any other options? 2. seems overly complex and 3. is more of a strong gut feeling (but can hardly be proven and it's kind of irresponsible, so I guess we really shouldn't do that), so I would tend to go for option 1.

dergoegge · 2024-07-09T17:30:50Z

Option 2. seems the best to me.

sipa · 2024-07-09T17:34:27Z

I think (2) is a 1-line change: return immediately from ProcessMessages if no version message has been sent by us to the peer.

theStack · 2024-07-09T17:59:53Z

Option 2. seems the best to me.

I think (2) is a 1-line change: return immediately from ProcessMessages if no version message has been sent by us to the peer.

Thanks for the quick feedback, done.

Initiating an outbound network connection currently involves the following steps after the socket connection is established (see `CConnman::OpenNetworkConnection` method): 1. set up node state 2. queue VERSION message 3. add new node to vector `m_nodes` If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`). Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`. Thanks go to vasild, mzumsande, dergoegge and sipa for suggestions on how to fix this. Github-Pull: bitcoin#30394 Rebased-From: 66673f1

Now that the queueing of the VERSION messages has been moved out of `InitializeNode`, there is no need to pass a mutable `CNode` reference any more. With a const reference, trying to send messages in this method would lead to a compile-time error, e.g.: ---------------------------------------------------------------------------------------------------------------------------------- ... net_processing.cpp: In member function ‘virtual void {anonymous}::PeerManagerImpl::InitializeNode(const CNode&, ServiceFlags)’: net_processing.cpp:1683:21: error: binding reference of type ‘CNode&’ to ‘const CNode’ discards qualifiers 1683 | PushNodeVersion(node, *peer); ... ---------------------------------------------------------------------------------------------------------------------------------- Github-Pull: bitcoin#30394 Rebased-From: 0dbcd4c

…ect" This reverts commit 9ec2c53 with a tiny change included (identation of the wait_until call). Github-Pull: bitcoin#30394 Rebased-From: 16bd283

Initiating an outbound network connection currently involves the following steps after the socket connection is established (see `CConnman::OpenNetworkConnection` method): 1. set up node state 2. queue VERSION message 3. add new node to vector `m_nodes` If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`). Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`. Thanks go to vasild, mzumsande, dergoegge and sipa for suggestions on how to fix this. Github-Pull: bitcoin#30394 Rebased-From: 66673f1

Now that the queueing of the VERSION messages has been moved out of `InitializeNode`, there is no need to pass a mutable `CNode` reference any more. With a const reference, trying to send messages in this method would lead to a compile-time error, e.g.: ---------------------------------------------------------------------------------------------------------------------------------- ... net_processing.cpp: In member function ‘virtual void {anonymous}::PeerManagerImpl::InitializeNode(const CNode&, ServiceFlags)’: net_processing.cpp:1683:21: error: binding reference of type ‘CNode&’ to ‘const CNode’ discards qualifiers 1683 | PushNodeVersion(node, *peer); ... ---------------------------------------------------------------------------------------------------------------------------------- Github-Pull: bitcoin#30394 Rebased-From: 0dbcd4c

…ect" This reverts commit 9ec2c53 with a tiny change included (identation of the wait_until call). Github-Pull: bitcoin#30394 Rebased-From: 16bd283

fanquake · 2024-07-17T10:32:38Z

Backported to 27.x in #30467.

faed5d3 test: Non-Shy version sender (MarcoFalke) Pull request description: After `add_outbound_p2p_connection`, the test framework normally sends a version message only in reply to a received version. This is fine, but the protocol does not require this and tolerates a version to be sent earlier. However, this is untested, and the missing test coverage leads to bugs being missed. For example #30394 (review) Fix it by adding a test. ACKs for top commit: brunoerg: ACK faed5d3 tdb3: ACK faed5d3 theStack: tACK faed5d3 glozow: ACK faed5d3 Tree-SHA512: dbf527a39c932e994a1e8248ba78058000811a4bf69275278f1fd1e545716ac4d2d3be5dcf362976bbafa2a49f91d13e3601daf71d29e9c556179b01af62c03c

4f23c86 [WIP] doc: update release notes for 27.x (fanquake) 54bb9b0 test: add test for modififed walletprocesspsbt calls (willcl-ark) f22b9ca wallet: fix FillPSBT errantly showing as complete (willcl-ark) 05192ba init: change shutdown order of load block thread and scheduler (Martin Zumsande) ab42206 Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner) 064f214 net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner) 0933cf5 net: fix race condition in self-connect detection (Sebastian Falbesoner) fa90989 psbt: Check non witness utxo outpoint early (Ava Chow) Pull request description: Backports: * #29855 * #30357 * #30394 (modified test commit) * #30435 ACKs for top commit: stickies-v: ACK 4f23c86 willcl-ark: ACK 4f23c86 Tree-SHA512: 5c26445f0855f9d14890369ce19873b0686804eeb659e7d6da36a6f404f64d019436e1e6471579eaa60a96ebf8f64311883b4aef9d0ed528a95bd610c101c079

Initiating an outbound network connection currently involves the following steps after the socket connection is established (see `CConnman::OpenNetworkConnection` method): 1. set up node state 2. queue VERSION message 3. add new node to vector `m_nodes` If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`). Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`. Thanks go to vasild, mzumsande, dergoegge and sipa for suggestions on how to fix this. Github-Pull: bitcoin#30394 Rebased-From: 66673f1

Now that the queueing of the VERSION messages has been moved out of `InitializeNode`, there is no need to pass a mutable `CNode` reference any more. With a const reference, trying to send messages in this method would lead to a compile-time error, e.g.: ---------------------------------------------------------------------------------------------------------------------------------- ... net_processing.cpp: In member function ‘virtual void {anonymous}::PeerManagerImpl::InitializeNode(const CNode&, ServiceFlags)’: net_processing.cpp:1683:21: error: binding reference of type ‘CNode&’ to ‘const CNode’ discards qualifiers 1683 | PushNodeVersion(node, *peer); ... ---------------------------------------------------------------------------------------------------------------------------------- Github-Pull: bitcoin#30394 Rebased-From: 0dbcd4c

…ect" This reverts commit 9ec2c53 with a tiny change included (identation of the wait_until call). Github-Pull: bitcoin#30394 Rebased-From: 16bd283

…detection 16bd283 Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner) 0dbcd4c net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner) 66673f1 net: fix race condition in self-connect detection (Sebastian Falbesoner) Pull request description: This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368). Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net.cpp#L2923-L2930) method): 1. set up node state 2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net_processing.cpp#L1662-L1683)) 3. add new node to vector `m_nodes` If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`). Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`. The temporarily reverted test introduced in #30362 is readded. Fixes #30368. Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see bitcoin/bitcoin#30368 (comment) ff. and bitcoin/bitcoin#30394 (comment)). ACKs for top commit: naiyoma: tested ACK [https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6](https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6), built and tested locally, test passes successfully. mzumsande: ACK 16bd283 tdb3: ACK 16bd283 glozow: ACK 16bd283 dergoegge: ACK 16bd283 Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445

faed5d3 test: Non-Shy version sender (MarcoFalke) Pull request description: After `add_outbound_p2p_connection`, the test framework normally sends a version message only in reply to a received version. This is fine, but the protocol does not require this and tolerates a version to be sent earlier. However, this is untested, and the missing test coverage leads to bugs being missed. For example bitcoin#30394 (review) Fix it by adding a test. ACKs for top commit: brunoerg: ACK faed5d3 tdb3: ACK faed5d3 theStack: tACK faed5d3 glozow: ACK faed5d3 Tree-SHA512: dbf527a39c932e994a1e8248ba78058000811a4bf69275278f1fd1e545716ac4d2d3be5dcf362976bbafa2a49f91d13e3601daf71d29e9c556179b01af62c03c

…detection 16bd283 Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner) 0dbcd4c net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner) 66673f1 net: fix race condition in self-connect detection (Sebastian Falbesoner) Pull request description: This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368). Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net.cpp#L2923-L2930) method): 1. set up node state 2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net_processing.cpp#L1662-L1683)) 3. add new node to vector `m_nodes` If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`). Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`. The temporarily reverted test introduced in #30362 is readded. Fixes #30368. Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see bitcoin/bitcoin#30368 (comment) ff. and bitcoin/bitcoin#30394 (comment)). ACKs for top commit: naiyoma: tested ACK [https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6](https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6), built and tested locally, test passes successfully. mzumsande: ACK 16bd283 tdb3: ACK 16bd283 glozow: ACK 16bd283 dergoegge: ACK 16bd283 Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445

DrahtBot added the P2P label Jul 4, 2024

tdb3 reviewed Jul 4, 2024

View reviewed changes

This was referenced Jul 5, 2024

[WIP] net: return result from addnode RPC #30381

Closed

locks: introduce mutex for tx download, flush rejection filters once per tip change #30111

Merged

DrahtBot requested a review from tdb3 July 5, 2024 09:36

dergoegge reviewed Jul 8, 2024

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

theStack force-pushed the 202407-p2p-fix_selfdetection_racecond branch from 7c9684a to 56d24db Compare July 8, 2024 17:32

dergoegge reviewed Jul 9, 2024

View reviewed changes

src/net.h Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

theStack force-pushed the 202407-p2p-fix_selfdetection_racecond branch from 56d24db to fb587ce Compare July 9, 2024 10:29

dergoegge approved these changes Jul 9, 2024

View reviewed changes

DrahtBot requested a review from maflcko July 9, 2024 10:35

maflcko reviewed Jul 9, 2024

View reviewed changes

test/functional/p2p_handshake.py Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from maflcko July 9, 2024 12:34

furszy reviewed Jul 9, 2024

View reviewed changes

glozow reviewed Jul 9, 2024

View reviewed changes

src/net.h Show resolved Hide resolved

theStack force-pushed the 202407-p2p-fix_selfdetection_racecond branch from fb587ce to a3122d3 Compare July 9, 2024 15:38

DrahtBot requested review from dergoegge and furszy July 9, 2024 15:56

mzumsande reviewed Jul 9, 2024

View reviewed changes

theStack force-pushed the 202407-p2p-fix_selfdetection_racecond branch from a3122d3 to bfd8dc4 Compare July 9, 2024 17:58

fanquake added the Needs backport (27.x) label Jul 16, 2024

fanquake mentioned this pull request Jul 17, 2024

[27.x] More backports #30467

Merged

fanquake removed the Needs backport (27.x) label Jul 24, 2024

jamillambert mentioned this pull request May 26, 2025

Random integration test failures because: it appears that bitcoind is not reachable rust-bitcoin/corepc#205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

net: fix race condition in self-connect detection #30394

net: fix race condition in self-connect detection #30394

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

net: fix race condition in self-connect detection #30394

net: fix race condition in self-connect detection #30394

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!