Skip to content

Commit 6cf206c

Browse files
committed
merge bitcoin#28155: improves addnode / m_added_nodes logic
1 parent 11d654a commit 6cf206c

File tree

9 files changed

+240
-36
lines changed

9 files changed

+240
-36
lines changed

src/Makefile.test.include

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@ BITCOIN_TESTS =\
134134
test/minisketch_tests.cpp \
135135
test/miner_tests.cpp \
136136
test/multisig_tests.cpp \
137+
test/net_peer_connection_tests.cpp \
137138
test/net_peer_eviction_tests.cpp \
138139
test/net_tests.cpp \
139140
test/netbase_tests.cpp \

src/net.cpp

Lines changed: 39 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -523,21 +523,25 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
523523
const uint16_t default_port{pszDest != nullptr ? Params().GetDefaultPort(pszDest) :
524524
Params().GetDefaultPort()};
525525
if (pszDest) {
526-
const std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
526+
std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
527527
if (!resolved.empty()) {
528-
const CService& rnd{resolved[GetRand(resolved.size())]};
529-
addrConnect = CAddress{MaybeFlipIPv6toCJDNS(rnd), NODE_NONE};
530-
if (!addrConnect.IsValid()) {
531-
LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToStringAddrPort(), pszDest);
532-
return nullptr;
533-
}
534-
// It is possible that we already have a connection to the IP/port pszDest resolved to.
535-
// In that case, drop the connection that was just created.
536-
LOCK(m_nodes_mutex);
537-
CNode* pnode = FindNode(static_cast<CService>(addrConnect));
538-
if (pnode) {
539-
LogPrintf("Failed to open new connection, already connected\n");
540-
return nullptr;
528+
Shuffle(resolved.begin(), resolved.end(), FastRandomContext());
529+
// If the connection is made by name, it can be the case that the name resolves to more than one address.
530+
// We don't want to connect any more of them if we are already connected to one
531+
for (const auto& r : resolved) {
532+
addrConnect = CAddress{MaybeFlipIPv6toCJDNS(r), NODE_NONE};
533+
if (!addrConnect.IsValid()) {
534+
LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToStringAddrPort(), pszDest);
535+
return nullptr;
536+
}
537+
// It is possible that we already have a connection to the IP/port pszDest resolved to.
538+
// In that case, drop the connection that was just created.
539+
LOCK(m_nodes_mutex);
540+
CNode* pnode = FindNode(static_cast<CService>(addrConnect));
541+
if (pnode) {
542+
LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort());
543+
return nullptr;
544+
}
541545
}
542546
}
543547
}
@@ -3587,7 +3591,7 @@ std::vector<CAddress> CConnman::GetCurrentBlockRelayOnlyConns() const
35873591
return ret;
35883592
}
35893593

3590-
std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
3594+
std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo(bool include_connected) const
35913595
{
35923596
std::vector<AddedNodeInfo> ret;
35933597

@@ -3622,6 +3626,9 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
36223626
// strAddNode is an IP:port
36233627
auto it = mapConnected.find(service);
36243628
if (it != mapConnected.end()) {
3629+
if (!include_connected) {
3630+
continue;
3631+
}
36253632
addedNode.resolvedAddress = service;
36263633
addedNode.fConnected = true;
36273634
addedNode.fInbound = it->second;
@@ -3630,6 +3637,9 @@ std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo() const
36303637
// strAddNode is a name
36313638
auto it = mapConnectedByName.find(addr.m_added_node);
36323639
if (it != mapConnectedByName.end()) {
3640+
if (!include_connected) {
3641+
continue;
3642+
}
36333643
addedNode.resolvedAddress = it->second.second;
36343644
addedNode.fConnected = true;
36353645
addedNode.fInbound = it->second.first;
@@ -3648,21 +3658,19 @@ void CConnman::ThreadOpenAddedConnections()
36483658
while (true)
36493659
{
36503660
CSemaphoreGrant grant(*semAddnode);
3651-
std::vector<AddedNodeInfo> vInfo = GetAddedNodeInfo();
3661+
std::vector<AddedNodeInfo> vInfo = GetAddedNodeInfo(/*include_connected=*/false);
36523662
bool tried = false;
36533663
for (const AddedNodeInfo& info : vInfo) {
3654-
if (!info.fConnected) {
3655-
if (!grant) {
3656-
// If we've used up our semaphore and need a new one, let's not wait here since while we are waiting
3657-
// the addednodeinfo state might change.
3658-
break;
3659-
}
3660-
tried = true;
3661-
CAddress addr(CService(), NODE_NONE);
3662-
OpenNetworkConnection(addr, false, std::move(grant), info.m_params.m_added_node.c_str(), ConnectionType::MANUAL, info.m_params.m_use_v2transport);
3663-
if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return;
3664-
grant = CSemaphoreGrant(*semAddnode, /*fTry=*/true);
3664+
if (!grant) {
3665+
// If we've used up our semaphore and need a new one, let's not wait here since while we are waiting
3666+
// the addednodeinfo state might change.
3667+
break;
36653668
}
3669+
tried = true;
3670+
CAddress addr(CService(), NODE_NONE);
3671+
OpenNetworkConnection(addr, false, std::move(grant), info.m_params.m_added_node.c_str(), ConnectionType::MANUAL, info.m_params.m_use_v2transport);
3672+
if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return;
3673+
grant = CSemaphoreGrant(*semAddnode, /*fTry=*/true);
36663674
}
36673675
// See if any reconnections are desired.
36683676
PerformReconnections();
@@ -4549,9 +4557,12 @@ std::vector<CAddress> CConnman::GetAddresses(CNode& requestor, size_t max_addres
45494557

45504558
bool CConnman::AddNode(const AddedNodeParams& add)
45514559
{
4560+
const CService resolved(LookupNumeric(add.m_added_node, Params().GetDefaultPort(add.m_added_node)));
4561+
const bool resolved_is_valid{resolved.IsValid()};
4562+
45524563
LOCK(m_added_nodes_mutex);
45534564
for (const auto& it : m_added_node_params) {
4554-
if (add.m_added_node == it.m_added_node) return false;
4565+
if (add.m_added_node == it.m_added_node || (resolved_is_valid && resolved == LookupNumeric(it.m_added_node, Params().GetDefaultPort(it.m_added_node)))) return false;
45554566
}
45564567

45574568
m_added_node_params.push_back(add);

src/net.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1481,7 +1481,7 @@ friend class CNode;
14811481

14821482
bool AddNode(const AddedNodeParams& add) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
14831483
bool RemoveAddedNode(const std::string& node) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
1484-
std::vector<AddedNodeInfo> GetAddedNodeInfo() const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
1484+
std::vector<AddedNodeInfo> GetAddedNodeInfo(bool include_connected) const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
14851485

14861486
/**
14871487
* Attempts to open a connection. Currently only used from tests.

src/rpc/net.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -498,7 +498,7 @@ static RPCHelpMan getaddednodeinfo()
498498
const NodeContext& node = EnsureAnyNodeContext(request.context);
499499
const CConnman& connman = EnsureConnman(node);;
500500

501-
std::vector<AddedNodeInfo> vInfo = connman.GetAddedNodeInfo();
501+
std::vector<AddedNodeInfo> vInfo = connman.GetAddedNodeInfo(/*include_connected=*/true);
502502

503503
if (!request.params[0].isNull()) {
504504
bool found = false;

src/test/fuzz/connman.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman)
124124
connman.SetTryNewOutboundPeer(fuzzed_data_provider.ConsumeBool());
125125
});
126126
}
127-
(void)connman.GetAddedNodeInfo();
127+
(void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool());
128128
(void)connman.GetExtraFullOutboundCount();
129129
(void)connman.GetLocalServices();
130130
assert(connman.GetMaxOutboundTarget() == max_outbound_limit);
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
// Copyright (c) 2023-present The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <chainparams.h>
6+
#include <compat.h>
7+
#include <net.h>
8+
#include <net_processing.h>
9+
#include <netaddress.h>
10+
#include <netbase.h>
11+
#include <netgroup.h>
12+
#include <node/connection_types.h>
13+
#include <protocol.h>
14+
#include <random.h>
15+
#include <test/util/logging.h>
16+
#include <test/util/net.h>
17+
#include <test/util/setup_common.h>
18+
#include <tinyformat.h>
19+
#include <version.h>
20+
21+
#include <algorithm>
22+
#include <cstdint>
23+
#include <memory>
24+
#include <optional>
25+
#include <string>
26+
#include <vector>
27+
28+
#include <boost/test/unit_test.hpp>
29+
30+
struct LogIPsTestingSetup : public TestingSetup {
31+
LogIPsTestingSetup()
32+
: TestingSetup{CBaseChainParams::MAIN, /*extra_args=*/{"-logips"}} {}
33+
};
34+
35+
BOOST_FIXTURE_TEST_SUITE(net_peer_connection_tests, LogIPsTestingSetup)
36+
37+
static CService ip(uint32_t i)
38+
{
39+
struct in_addr s;
40+
s.s_addr = i;
41+
return CService{CNetAddr{s}, Params().GetDefaultPort()};
42+
}
43+
44+
/** Create a peer and connect to it. If the optional `address` (IP/CJDNS only) isn't passed, a random address is created. */
45+
static void AddPeer(NodeId& id, std::vector<CNode*>& nodes, PeerManager& peerman, ConnmanTestMsg& connman, ConnectionType conn_type, bool onion_peer = false, std::optional<std::string> address = std::nullopt)
46+
{
47+
CAddress addr{};
48+
49+
if (address.has_value()) {
50+
addr = CAddress{MaybeFlipIPv6toCJDNS(LookupNumeric(address.value(), Params().GetDefaultPort())), NODE_NONE};
51+
} else if (onion_peer) {
52+
auto tor_addr{g_insecure_rand_ctx.randbytes(ADDR_TORV3_SIZE)};
53+
BOOST_REQUIRE(addr.SetSpecial(OnionToString(tor_addr)));
54+
}
55+
56+
while (!addr.IsLocal() && !addr.IsRoutable()) {
57+
addr = CAddress{ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE};
58+
}
59+
60+
BOOST_REQUIRE(addr.IsValid());
61+
62+
const bool inbound_onion{onion_peer && conn_type == ConnectionType::INBOUND};
63+
64+
nodes.emplace_back(new CNode{++id,
65+
/*sock=*/nullptr,
66+
addr,
67+
/*nKeyedNetGroupIn=*/0,
68+
/*nLocalHostNonceIn=*/0,
69+
CAddress{},
70+
/*addrNameIn=*/"",
71+
conn_type,
72+
/*inbound_onion=*/inbound_onion});
73+
CNode& node = *nodes.back();
74+
node.SetCommonVersion(PROTOCOL_VERSION);
75+
76+
peerman.InitializeNode(node, ServiceFlags(NODE_NETWORK));
77+
node.fSuccessfullyConnected = true;
78+
79+
connman.AddTestNode(node);
80+
}
81+
82+
BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection)
83+
{
84+
const auto& chainparams = Params();
85+
auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman);
86+
auto peerman = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr,
87+
*m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
88+
*m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman,
89+
m_node.cj_ctx, m_node.llmq_ctx, /* ignore_incoming_txs = */ false);
90+
NodeId id{0};
91+
std::vector<CNode*> nodes;
92+
93+
// Connect a localhost peer.
94+
{
95+
ASSERT_DEBUG_LOG("Added connection to 127.0.0.1:9999 peer=1");
96+
AddPeer(id, nodes, *peerman, *connman, ConnectionType::MANUAL, /*onion_peer=*/false, /*address=*/"127.0.0.1");
97+
BOOST_REQUIRE(nodes.back() != nullptr);
98+
}
99+
100+
// Call ConnectNode(), which is also called by RPC addnode onetry, for a localhost
101+
// address that resolves to multiple IPs, including that of the connected peer.
102+
// The connection attempt should consistently fail due to the check in ConnectNode().
103+
for (int i = 0; i < 10; ++i) {
104+
ASSERT_DEBUG_LOG("Not opening a connection to localhost, already connected to 127.0.0.1:9999");
105+
BOOST_CHECK(!connman->ConnectNodePublic(*peerman, "localhost", ConnectionType::MANUAL));
106+
}
107+
108+
// Add 3 more peer connections.
109+
AddPeer(id, nodes, *peerman, *connman, ConnectionType::OUTBOUND_FULL_RELAY);
110+
AddPeer(id, nodes, *peerman, *connman, ConnectionType::BLOCK_RELAY, /*onion_peer=*/true);
111+
AddPeer(id, nodes, *peerman, *connman, ConnectionType::INBOUND);
112+
113+
BOOST_TEST_MESSAGE("Call AddNode() for all the peers");
114+
for (auto node : connman->TestNodes()) {
115+
BOOST_CHECK(connman->AddNode({/*m_added_node=*/node->addr.ToStringAddrPort(), /*m_use_v2transport=*/true}));
116+
BOOST_TEST_MESSAGE(strprintf("peer id=%s addr=%s", node->GetId(), node->addr.ToStringAddrPort()));
117+
}
118+
119+
BOOST_TEST_MESSAGE("\nCall AddNode() with 2 addrs resolving to existing localhost addnode entry; neither should be added");
120+
BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.0.0.1", /*m_use_v2transport=*/true}));
121+
BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.1", /*m_use_v2transport=*/true}));
122+
123+
BOOST_TEST_MESSAGE("\nExpect GetAddedNodeInfo to return expected number of peers with `include_connected` true/false");
124+
BOOST_CHECK_EQUAL(connman->GetAddedNodeInfo(/*include_connected=*/true).size(), nodes.size());
125+
BOOST_CHECK(connman->GetAddedNodeInfo(/*include_connected=*/false).empty());
126+
127+
BOOST_TEST_MESSAGE("\nPrint GetAddedNodeInfo contents:");
128+
for (const auto& info : connman->GetAddedNodeInfo(/*include_connected=*/true)) {
129+
BOOST_TEST_MESSAGE(strprintf("\nadded node: %s", info.m_params.m_added_node));
130+
BOOST_TEST_MESSAGE(strprintf("connected: %s", info.fConnected));
131+
if (info.fConnected) {
132+
BOOST_TEST_MESSAGE(strprintf("IP address: %s", info.resolvedAddress.ToStringAddrPort()));
133+
BOOST_TEST_MESSAGE(strprintf("direction: %s", info.fInbound ? "inbound" : "outbound"));
134+
}
135+
}
136+
137+
BOOST_TEST_MESSAGE("\nCheck that all connected peers are correctly detected as connected");
138+
for (auto node : connman->TestNodes()) {
139+
BOOST_CHECK(connman->AlreadyConnectedPublic(node->addr));
140+
}
141+
142+
// Clean up
143+
for (auto node : connman->TestNodes()) {
144+
peerman->FinalizeNode(*node);
145+
}
146+
connman->ClearTestNodes();
147+
}
148+
149+
BOOST_AUTO_TEST_SUITE_END()

src/test/util/net.cpp

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,15 @@
44

55
#include <test/util/net.h>
66

7-
#include <chainparams.h>
8-
#include <node/eviction.h>
97
#include <net.h>
108
#include <net_processing.h>
9+
#include <netaddress.h>
1110
#include <netmessagemaker.h>
11+
#include <node/connection_types.h>
12+
#include <node/eviction.h>
13+
#include <protocol.h>
14+
#include <random.h>
15+
#include <serialize.h>
1216
#include <span.h>
1317

1418
#include <vector>
@@ -98,6 +102,17 @@ bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg&& ser_msg) co
98102
return complete;
99103
}
100104

105+
CNode* ConnmanTestMsg::ConnectNodePublic(PeerManager& peerman, const char* pszDest, ConnectionType conn_type)
106+
{
107+
CNode* node = ConnectNode(CAddress{}, pszDest, /*fCountFailure=*/false, conn_type, /*use_v2transport=*/true);
108+
if (!node) return nullptr;
109+
node->SetCommonVersion(PROTOCOL_VERSION);
110+
peerman.InitializeNode(*node, ServiceFlags(NODE_NETWORK));
111+
node->fSuccessfullyConnected = true;
112+
AddTestNode(*node);
113+
return node;
114+
}
115+
101116
std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context)
102117
{
103118
std::vector<NodeEvictionCandidate> candidates;

src/test/util/net.h

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,30 @@
66
#define BITCOIN_TEST_UTIL_NET_H
77

88
#include <compat.h>
9-
#include <node/eviction.h>
10-
#include <netaddress.h>
119
#include <net.h>
10+
#include <net_permissions.h>
11+
#include <net_processing.h>
12+
#include <netaddress.h>
13+
#include <node/connection_types.h>
14+
#include <node/eviction.h>
15+
#include <sync.h>
1216
#include <util/sock.h>
1317

18+
#include <algorithm>
1419
#include <array>
1520
#include <cassert>
21+
#include <chrono>
22+
#include <cstdint>
1623
#include <cstring>
1724
#include <memory>
1825
#include <string>
26+
#include <unordered_map>
27+
#include <vector>
28+
29+
class FastRandomContext;
30+
31+
template <typename C>
32+
class Span;
1933

2034
struct ConnmanTestMsg : public CConnman {
2135
using CConnman::CConnman;
@@ -25,6 +39,12 @@ struct ConnmanTestMsg : public CConnman {
2539
m_peer_connect_timeout = timeout;
2640
}
2741

42+
std::vector<CNode*> TestNodes()
43+
{
44+
LOCK(m_nodes_mutex);
45+
return m_nodes;
46+
}
47+
2848
void AddTestNode(CNode& node)
2949
{
3050
LOCK(m_nodes_mutex);
@@ -56,6 +76,11 @@ struct ConnmanTestMsg : public CConnman {
5676

5777
bool ReceiveMsgFrom(CNode& node, CSerializedNetMsg&& ser_msg) const;
5878
void FlushSendBuffer(CNode& node) const;
79+
80+
bool AlreadyConnectedPublic(const CAddress& addr) { return AlreadyConnectedToAddress(addr); };
81+
82+
CNode* ConnectNodePublic(PeerManager& peerman, const char* pszDest, ConnectionType conn_type)
83+
EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
5984
};
6085

6186
constexpr ServiceFlags ALL_SERVICE_FLAGS[]{

0 commit comments

Comments
 (0)