diff --git a/src/net.h b/src/net.h index 34ddd5c80de..a4e4b503607 100644 --- a/src/net.h +++ b/src/net.h @@ -991,8 +991,8 @@ public: /** Mutex for anything that is only accessed via the msg processing thread */ static Mutex g_msgproc_mutex; - /** Initialize a peer (setup state, queue any initial messages) */ - virtual void InitializeNode(CNode& node, ServiceFlags our_services) = 0; + /** Initialize a peer (setup state) */ + virtual void InitializeNode(const CNode& node, ServiceFlags our_services) = 0; /** Handle removal of a peer (clear state) */ virtual void FinalizeNode(const CNode& node) = 0; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2764562f577..d674758abd8 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -243,6 +243,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(NetEventsInterface::g_msgproc_mutex){false}; + /** This peer's reported block height when we connected */ std::atomic m_starting_height{-1}; @@ -498,7 +501,7 @@ public: EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex); /** Implement NetEventsInterface */ - void InitializeNode(CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); + void InitializeNode(const CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex); bool HasAllDesirableServiceFlags(ServiceFlags services) const override; bool ProcessMessages(CNode* pfrom, std::atomic& interrupt) override @@ -1659,7 +1662,7 @@ void PeerManagerImpl::UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_s if (state) state->m_last_block_announcement = time_in_seconds; } -void PeerManagerImpl::InitializeNode(CNode& node, ServiceFlags our_services) +void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_services) { NodeId nodeid = node.GetId(); { @@ -1677,9 +1680,6 @@ void PeerManagerImpl::InitializeNode(CNode& node, ServiceFlags our_services) LOCK(m_peer_mutex); m_peer_map.emplace_hint(m_peer_map.end(), nodeid, peer); } - if (!node.IsInboundConn()) { - PushNodeVersion(node, *peer); - } } void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) @@ -5326,6 +5326,10 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& 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() && !peer->m_outbound_version_message_sent) return false; + { LOCK(peer->m_getdata_requests_mutex); if (!peer->m_getdata_requests.empty()) { @@ -5817,6 +5821,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() && !peer->m_outbound_version_message_sent) { + PushNodeVersion(*pto, *peer); + peer->m_outbound_version_message_sent = true; + } + // Don't send anything until the version handshake is complete if (!pto->fSuccessfullyConnected || pto->fDisconnect) return true; diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index d1903df6ac8..beefc32bee4 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -28,7 +28,8 @@ void ConnmanTestMsg::Handshake(CNode& node, auto& connman{*this}; peerman.InitializeNode(node, local_services); - FlushSendBuffer(node); // Drop the version message added by InitializeNode. + peerman.SendMessages(&node); + FlushSendBuffer(node); // Drop the version message added by SendMessages. CSerializedNetMsg msg_version{ NetMsg::Make(NetMsgType::VERSION, diff --git a/test/functional/p2p_handshake.py b/test/functional/p2p_handshake.py index 21959ae522e..9536e748931 100755 --- a/test/functional/p2p_handshake.py +++ b/test/functional/p2p_handshake.py @@ -17,6 +17,7 @@ from test_framework.messages import ( NODE_WITNESS, ) from test_framework.p2p import P2PInterface +from test_framework.util import p2p_port # Desirable service flags for outbound non-pruned and pruned peers. Note that @@ -88,9 +89,11 @@ class P2PHandshakeTest(BitcoinTestFramework): with node.assert_debug_log([f"feeler connection completed"]): self.add_outbound_connection(node, "feeler", NODE_NONE, wait_for_disconnect=True) - # TODO: re-add test introduced in commit 5d2fb14bafe4e80c0a482d99e5ebde07c477f000 - # ("test: p2p: check that connecting to ourself leads to disconnect") once - # the race condition causing issue #30368 is fixed + self.log.info("Check that connecting to ourself leads to immediate disconnect") + with node.assert_debug_log(["connected to self", "disconnecting"]): + node_listen_addr = f"127.0.0.1:{p2p_port(0)}" + node.addconnection(node_listen_addr, "outbound-full-relay", self.options.v2transport) + self.wait_until(lambda: len(node.getpeerinfo()) == 0) if __name__ == '__main__':