Files
bitcoin/src/test/util/net.cpp
glozow 35dddbccf1 Merge bitcoin/bitcoin#30394: net: fix race condition in self-connect detection
16bd283b3a Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
0dbcd4c148 net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner)
66673f1c13 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`](bd5d1688b4/src/net.cpp (L2923-L2930)) method):
  1. set up node state
  2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](bd5d1688b4/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 https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200625017 ff. and https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789).

ACKs for top commit:
  naiyoma:
    tested ACK [16bd283b3a),  built and tested locally,  test passes successfully.
  mzumsande:
    ACK 16bd283b3a
  tdb3:
    ACK 16bd283b3a
  glozow:
    ACK 16bd283b3a
  dergoegge:
    ACK 16bd283b3a

Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445
2024-07-16 09:40:53 +01:00

140 lines
5.5 KiB
C++

// Copyright (c) 2020-2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <test/util/net.h>
#include <net.h>
#include <net_processing.h>
#include <netaddress.h>
#include <netmessagemaker.h>
#include <node/connection_types.h>
#include <node/eviction.h>
#include <protocol.h>
#include <random.h>
#include <serialize.h>
#include <span.h>
#include <vector>
void ConnmanTestMsg::Handshake(CNode& node,
bool successfully_connected,
ServiceFlags remote_services,
ServiceFlags local_services,
int32_t version,
bool relay_txs)
{
auto& peerman{static_cast<PeerManager&>(*m_msgproc)};
auto& connman{*this};
peerman.InitializeNode(node, local_services);
peerman.SendMessages(&node);
FlushSendBuffer(node); // Drop the version message added by SendMessages.
CSerializedNetMsg msg_version{
NetMsg::Make(NetMsgType::VERSION,
version, //
Using<CustomUintFormatter<8>>(remote_services), //
int64_t{}, // dummy time
int64_t{}, // ignored service bits
CNetAddr::V1(CService{}), // dummy
int64_t{}, // ignored service bits
CNetAddr::V1(CService{}), // ignored
uint64_t{1}, // dummy nonce
std::string{}, // dummy subver
int32_t{}, // dummy starting_height
relay_txs),
};
(void)connman.ReceiveMsgFrom(node, std::move(msg_version));
node.fPauseSend = false;
connman.ProcessMessagesOnce(node);
peerman.SendMessages(&node);
FlushSendBuffer(node); // Drop the verack message added by SendMessages.
if (node.fDisconnect) return;
assert(node.nVersion == version);
assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
CNodeStateStats statestats;
assert(peerman.GetNodeStateStats(node.GetId(), statestats));
assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn()));
assert(statestats.their_services == remote_services);
if (successfully_connected) {
CSerializedNetMsg msg_verack{NetMsg::Make(NetMsgType::VERACK)};
(void)connman.ReceiveMsgFrom(node, std::move(msg_verack));
node.fPauseSend = false;
connman.ProcessMessagesOnce(node);
peerman.SendMessages(&node);
assert(node.fSuccessfullyConnected == true);
}
}
void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const
{
assert(node.ReceiveMsgBytes(msg_bytes, complete));
if (complete) {
node.MarkReceivedMsgsForProcessing();
}
}
void ConnmanTestMsg::FlushSendBuffer(CNode& node) const
{
LOCK(node.cs_vSend);
node.vSendMsg.clear();
node.m_send_memusage = 0;
while (true) {
const auto& [to_send, _more, _msg_type] = node.m_transport->GetBytesToSend(false);
if (to_send.empty()) break;
node.m_transport->MarkBytesSent(to_send.size());
}
}
bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg&& ser_msg) const
{
bool queued = node.m_transport->SetMessageToSend(ser_msg);
assert(queued);
bool complete{false};
while (true) {
const auto& [to_send, _more, _msg_type] = node.m_transport->GetBytesToSend(false);
if (to_send.empty()) break;
NodeReceiveMsgBytes(node, to_send, complete);
node.m_transport->MarkBytesSent(to_send.size());
}
return complete;
}
CNode* ConnmanTestMsg::ConnectNodePublic(PeerManager& peerman, const char* pszDest, ConnectionType conn_type)
{
CNode* node = ConnectNode(CAddress{}, pszDest, /*fCountFailure=*/false, conn_type, /*use_v2transport=*/true);
if (!node) return nullptr;
node->SetCommonVersion(PROTOCOL_VERSION);
peerman.InitializeNode(*node, ServiceFlags(NODE_NETWORK | NODE_WITNESS));
node->fSuccessfullyConnected = true;
AddTestNode(*node);
return node;
}
std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context)
{
std::vector<NodeEvictionCandidate> candidates;
candidates.reserve(n_candidates);
for (int id = 0; id < n_candidates; ++id) {
candidates.push_back({
.id=id,
.m_connected=std::chrono::seconds{random_context.randrange(100)},
.m_min_ping_time=std::chrono::microseconds{random_context.randrange(100)},
.m_last_block_time=std::chrono::seconds{random_context.randrange(100)},
.m_last_tx_time=std::chrono::seconds{random_context.randrange(100)},
.fRelevantServices=random_context.randbool(),
.m_relay_txs=random_context.randbool(),
.fBloomFilter=random_context.randbool(),
.nKeyedNetGroup=random_context.randrange(100u),
.prefer_evict=random_context.randbool(),
.m_is_local=random_context.randbool(),
.m_network=ALL_NETWORKS[random_context.randrange(ALL_NETWORKS.size())],
.m_noban=false,
.m_conn_type=ConnectionType::INBOUND,
});
}
return candidates;
}