Merge bitcoin/bitcoin#30394: net: fix race condition in self-connect 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

Author: glozow

src/net_processing.cpp

@@ -222,6 +222,9 @@ struct Peer {
      * Most peers use headers-first syncing, which doesn't use this mechanism */
     uint256 m_continuation_block GUARDED_BY(m_block_inv_mutex) {};
 
+    /** Set to true once initial VERSION message was sent (only relevant for outbound peers). */
+    bool m_outbound_version_message_sent GUARDED_BY(m_block_inv_mutex){false};
+
     /** This peer's reported block height when we connected */
     std::atomic<int> m_starting_height{-1};
 
@@ -1202,9 +1205,6 @@ void PeerManagerImpl::InitializeNode(CNode *pnode)
         LOCK(m_peer_mutex);
         m_peer_map.emplace_hint(m_peer_map.end(), nodeid, std::move(peer));
     }
-    if (!pnode->IsInboundConn()) {
-        PushNodeVersion(*pnode);
-    }
 }
 
 void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
@@ -4201,6 +4201,10 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
     PeerRef peer = GetPeerRef(pfrom->GetId());
     if (peer == nullptr) return false;
 
+    // For outbound connections, ensure that the initial VERSION message
+    // has been sent first before processing any incoming messages
+    if (!pfrom->IsInboundConn() && WITH_LOCK(peer->m_block_inv_mutex, return !peer->m_outbound_version_message_sent)) return false;
+
     {
         LOCK(peer->m_getdata_requests_mutex);
         if (!peer->m_getdata_requests.empty()) {
@@ -4660,6 +4664,12 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
     // disconnect misbehaving peers even before the version handshake is complete.
     if (MaybeDiscourageAndDisconnect(*pto, *peer)) return true;
 
+    // Initiate version handshake for outbound connections
+    if (!pto->IsInboundConn() && WITH_LOCK(peer->m_block_inv_mutex, return !peer->m_outbound_version_message_sent)) {
+        PushNodeVersion(*pto);
+        WITH_LOCK(peer->m_block_inv_mutex, peer->m_outbound_version_message_sent = true);
+    }
+
     // Don't send anything until the version handshake is complete
     if (!pto->fSuccessfullyConnected || pto->fDisconnect)
         return true;
@@ -5129,4 +5139,4 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
         MaybeSendFeefilter(*pto, current_time);
     } // release cs_main
     return true;
-}
+}

src/test/fuzz/util.cpp

@@ -251,10 +251,10 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
         LOCK(node.cs_sendProcessing);
         peerman.SendMessages(&node);
     }
-    if (node.fDisconnect) return;
     assert(node.nVersion == version);
     assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
     assert(node.nServices == remote_services);
+    if (node.fDisconnect) return;
     if (node.m_tx_relay != nullptr) {
         LOCK(node.m_tx_relay->cs_filter);
         assert(node.m_tx_relay->fRelayTxes == filter_txs);

test/functional/feature_filelock.py

@@ -27,8 +27,8 @@ def run_test(self):
         self.log.info("Check that we can't start a second bitcoind instance using the same datadir")
         expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['PACKAGE_NAME']} is probably already running."
         self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir}', '-noserver'], expected_msg=expected_msg)
-        cookie_file = datadir / ".cookie"
-        assert cookie_file.exists()  # should not be deleted during the second bitcoind instance shutdown
+        cookie_file = datadir + "/.cookie"
+        assert os.path.isfile(cookie_file)  # should not be deleted during the second bitcoind instance shutdown
         if self.is_wallet_compiled():
             def check_wallet_filelock(descriptors):
                 wallet_name = ''.join([random.choice(string.ascii_lowercase) for _ in range(6)])