diff options
author | glozow <gloriajzhao@gmail.com> | 2023-11-08 11:15:41 +0000 |
---|---|---|
committer | glozow <gloriajzhao@gmail.com> | 2023-11-08 11:31:36 +0000 |
commit | 9ad19fc7c7116254015650071c59717a485b1bcb (patch) | |
tree | a52b34aa3869cc3e4f6aa9914f03ea26896ddc3e /src/test | |
parent | d690f89b5797e13d94b38abb6c456af265a9efe7 (diff) | |
parent | 0420f99f429ce2382057e101859067f40de47be0 (diff) | |
download | bitcoin-9ad19fc7c7116254015650071c59717a485b1bcb.tar.xz |
Merge bitcoin/bitcoin#28155: net: improves addnode / m_added_nodes logic
0420f99f429ce2382057e101859067f40de47be0 Create net_peer_connection unit tests (Jon Atack)
4b834f649921aceb44d3e0b5a2ffd7847903f9f7 Allow unit tests to access additional CConnman members (Jon Atack)
34b9ef443bc2655a85c8802edc5d5d48d792a286 net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request (Sergi Delgado Segura)
94e8882d820969ddc83f24f4cbe1515a886da4ea rpc: Prevents adding the same ip more than once when formatted differently (Sergi Delgado Segura)
2574b7e177ef045e64f1dd48cb000640ff5103d3 net/rpc: Check all resolved addresses in ConnectNode rather than just one (Sergi Delgado Segura)
Pull request description:
## Rationale
Currently, `addnode` has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to `m_added_nodes`, hence making Bitcoin iterate through useless data on a regular basis.
### Connecting to the same node more than once
In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via `addnode` in two different ways:
1. Calling `addnode` more than once in a short time period, using two equivalent but distinct addresses
2. Calling `addnode add` using an IP, and `addnode onetry` after with an address that resolved to the same IP
For the former, the issue boils down to `CConnman::ThreadOpenAddedConnections` calling `CConnman::GetAddedNodeInfo` once, and iterating over the result to open connections (`CConman::OpenNetworkConnection`) on the same loop for all addresses.`CConnman::ConnectNode` only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it.
An example to test this would be calling:
```
bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" add
```
And check how it allows us to perform both connections some times, and some times it fails.
The latter boils down to the same issue, but takes advantage of `onetry` bypassing the `CConnman::ThreadOpenAddedConnections` logic and calling `CConnman::OpenNetworkConnection` straightaway. A way to test this would be:
```
bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" onetry
```
### Adding the same peer with two different, yet equivalent, addresses
The current implementation of `addnode` is pretty naive when checking what data is added to `m_added_nodes`. Given the collection stores strings, the checks at `CConnman::AddNode()` basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks.
Two examples would be `127.0.0.1` being equal to `127.1` and `[::1]` being equal to `[0:0:0:0:0:0:0:1]`. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by `getaddednodeinfo`, given they map to the same `CService`.
This is less severe than the previous issue, since even tough both nodes are reported as connected by `getaddednodeinfo`, there is only a single connection to them (as properly reported by `getpeerinfo`). However, this adds redundant data to `m_added_nodes`, which is undesirable.
### Parametrize `CConnman::GetAddedNodeInfo`
Finally, this PR also parametrizes `CConnman::GetAddedNodeInfo` so it returns either all added nodes info, or only info about the nodes we are **not** connected to. This method is used both for `rpc`, in `getaddednodeinfo`, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in `CConnman::ThreadOpenAddedConnections`, in which we are currently returning more data than needed and then actively filtering using `CService.fConnected()`
ACKs for top commit:
jonatack:
re-ACK 0420f99f429ce2382057e101859067f40de47be0
kashifs:
> > tACK [0420f9](https://github.com/bitcoin/bitcoin/commit/0420f99f429ce2382057e101859067f40de47be0)
sr-gi:
> > > tACK [0420f9](https://github.com/bitcoin/bitcoin/commit/0420f99f429ce2382057e101859067f40de47be0)
mzumsande:
Tested ACK 0420f99f429ce2382057e101859067f40de47be0
Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752
Diffstat (limited to 'src/test')
-rw-r--r-- | src/test/fuzz/connman.cpp | 2 | ||||
-rw-r--r-- | src/test/net_peer_connection_tests.cpp | 147 | ||||
-rw-r--r-- | src/test/util/net.cpp | 19 | ||||
-rw-r--r-- | src/test/util/net.h | 29 |
4 files changed, 192 insertions, 5 deletions
diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 0dab2a2e97..551e1c526d 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -121,7 +121,7 @@ FUZZ_TARGET(connman, .init = initialize_connman) connman.SetTryNewOutboundPeer(fuzzed_data_provider.ConsumeBool()); }); } - (void)connman.GetAddedNodeInfo(); + (void)connman.GetAddedNodeInfo(fuzzed_data_provider.ConsumeBool()); (void)connman.GetExtraFullOutboundCount(); (void)connman.GetLocalServices(); (void)connman.GetMaxOutboundTarget(); diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp new file mode 100644 index 0000000000..3d3f296d82 --- /dev/null +++ b/src/test/net_peer_connection_tests.cpp @@ -0,0 +1,147 @@ +// Copyright (c) 2023-present 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 <chainparams.h> +#include <compat/compat.h> +#include <net.h> +#include <net_processing.h> +#include <netaddress.h> +#include <netbase.h> +#include <netgroup.h> +#include <node/connection_types.h> +#include <protocol.h> +#include <random.h> +#include <test/util/logging.h> +#include <test/util/net.h> +#include <test/util/random.h> +#include <test/util/setup_common.h> +#include <tinyformat.h> +#include <util/chaintype.h> +#include <version.h> + +#include <algorithm> +#include <cstdint> +#include <memory> +#include <optional> +#include <string> +#include <vector> + +#include <boost/test/unit_test.hpp> + +struct LogIPsTestingSetup : public TestingSetup { + LogIPsTestingSetup() + : TestingSetup{ChainType::MAIN, /*extra_args=*/{"-logips"}} {} +}; + +BOOST_FIXTURE_TEST_SUITE(net_peer_connection_tests, LogIPsTestingSetup) + +static CService ip(uint32_t i) +{ + struct in_addr s; + s.s_addr = i; + return CService{CNetAddr{s}, Params().GetDefaultPort()}; +} + +/** Create a peer and connect to it. If the optional `address` (IP/CJDNS only) isn't passed, a random address is created. */ +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) +{ + CAddress addr{}; + + if (address.has_value()) { + addr = CAddress{MaybeFlipIPv6toCJDNS(LookupNumeric(address.value(), Params().GetDefaultPort())), NODE_NONE}; + } else if (onion_peer) { + auto tor_addr{g_insecure_rand_ctx.randbytes(ADDR_TORV3_SIZE)}; + BOOST_REQUIRE(addr.SetSpecial(OnionToString(tor_addr))); + } + + while (!addr.IsLocal() && !addr.IsRoutable()) { + addr = CAddress{ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE}; + } + + BOOST_REQUIRE(addr.IsValid()); + + const bool inbound_onion{onion_peer && conn_type == ConnectionType::INBOUND}; + + nodes.emplace_back(new CNode{++id, + /*sock=*/nullptr, + addr, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + CAddress{}, + /*addrNameIn=*/"", + conn_type, + /*inbound_onion=*/inbound_onion}); + CNode& node = *nodes.back(); + node.SetCommonVersion(PROTOCOL_VERSION); + + peerman.InitializeNode(node, ServiceFlags(NODE_NETWORK | NODE_WITNESS)); + node.fSuccessfullyConnected = true; + + connman.AddTestNode(node); +} + +BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection) +{ + auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman, Params()); + auto peerman = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {}); + NodeId id{0}; + std::vector<CNode*> nodes; + + // Connect a localhost peer. + { + ASSERT_DEBUG_LOG("Added connection to 127.0.0.1:8333 peer=1"); + AddPeer(id, nodes, *peerman, *connman, ConnectionType::MANUAL, /*onion_peer=*/false, /*address=*/"127.0.0.1"); + BOOST_REQUIRE(nodes.back() != nullptr); + } + + // Call ConnectNode(), which is also called by RPC addnode onetry, for a localhost + // address that resolves to multiple IPs, including that of the connected peer. + // The connection attempt should consistently fail due to the check in ConnectNode(). + for (int i = 0; i < 10; ++i) { + ASSERT_DEBUG_LOG("Not opening a connection to localhost, already connected to 127.0.0.1:8333"); + BOOST_CHECK(!connman->ConnectNodePublic(*peerman, "localhost", ConnectionType::MANUAL)); + } + + // Add 3 more peer connections. + AddPeer(id, nodes, *peerman, *connman, ConnectionType::OUTBOUND_FULL_RELAY); + AddPeer(id, nodes, *peerman, *connman, ConnectionType::BLOCK_RELAY, /*onion_peer=*/true); + AddPeer(id, nodes, *peerman, *connman, ConnectionType::INBOUND); + + BOOST_TEST_MESSAGE("Call AddNode() for all the peers"); + for (auto node : connman->TestNodes()) { + BOOST_CHECK(connman->AddNode({/*m_added_node=*/node->addr.ToStringAddrPort(), /*m_use_v2transport=*/true})); + BOOST_TEST_MESSAGE(strprintf("peer id=%s addr=%s", node->GetId(), node->addr.ToStringAddrPort())); + } + + BOOST_TEST_MESSAGE("\nCall AddNode() with 2 addrs resolving to existing localhost addnode entry; neither should be added"); + BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.0.0.1", /*m_use_v2transport=*/true})); + BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.1", /*m_use_v2transport=*/true})); + + BOOST_TEST_MESSAGE("\nExpect GetAddedNodeInfo to return expected number of peers with `include_connected` true/false"); + BOOST_CHECK_EQUAL(connman->GetAddedNodeInfo(/*include_connected=*/true).size(), nodes.size()); + BOOST_CHECK(connman->GetAddedNodeInfo(/*include_connected=*/false).empty()); + + BOOST_TEST_MESSAGE("\nPrint GetAddedNodeInfo contents:"); + for (const auto& info : connman->GetAddedNodeInfo(/*include_connected=*/true)) { + BOOST_TEST_MESSAGE(strprintf("\nadded node: %s", info.m_params.m_added_node)); + BOOST_TEST_MESSAGE(strprintf("connected: %s", info.fConnected)); + if (info.fConnected) { + BOOST_TEST_MESSAGE(strprintf("IP address: %s", info.resolvedAddress.ToStringAddrPort())); + BOOST_TEST_MESSAGE(strprintf("direction: %s", info.fInbound ? "inbound" : "outbound")); + } + } + + BOOST_TEST_MESSAGE("\nCheck that all connected peers are correctly detected as connected"); + for (auto node : connman->TestNodes()) { + BOOST_CHECK(connman->AlreadyConnectedPublic(node->addr)); + } + + // Clean up + for (auto node : connman->TestNodes()) { + peerman->FinalizeNode(*node); + } + connman->ClearTestNodes(); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index bf5a653090..e0404e33ed 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -4,11 +4,15 @@ #include <test/util/net.h> -#include <chainparams.h> -#include <node/eviction.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> @@ -98,6 +102,17 @@ bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg&& ser_msg) co 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; diff --git a/src/test/util/net.h b/src/test/util/net.h index 497292542b..59c4ddb4b1 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -6,16 +6,30 @@ #define BITCOIN_TEST_UTIL_NET_H #include <compat/compat.h> -#include <node/eviction.h> -#include <netaddress.h> #include <net.h> +#include <net_permissions.h> +#include <net_processing.h> +#include <netaddress.h> +#include <node/connection_types.h> +#include <node/eviction.h> +#include <sync.h> #include <util/sock.h> +#include <algorithm> #include <array> #include <cassert> +#include <chrono> +#include <cstdint> #include <cstring> #include <memory> #include <string> +#include <unordered_map> +#include <vector> + +class FastRandomContext; + +template <typename C> +class Span; struct ConnmanTestMsg : public CConnman { using CConnman::CConnman; @@ -25,6 +39,12 @@ struct ConnmanTestMsg : public CConnman { m_peer_connect_timeout = timeout; } + std::vector<CNode*> TestNodes() + { + LOCK(m_nodes_mutex); + return m_nodes; + } + void AddTestNode(CNode& node) { LOCK(m_nodes_mutex); @@ -56,6 +76,11 @@ struct ConnmanTestMsg : public CConnman { bool ReceiveMsgFrom(CNode& node, CSerializedNetMsg&& ser_msg) const; void FlushSendBuffer(CNode& node) const; + + bool AlreadyConnectedPublic(const CAddress& addr) { return AlreadyConnectedToAddress(addr); }; + + CNode* ConnectNodePublic(PeerManager& peerman, const char* pszDest, ConnectionType conn_type) + EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); }; constexpr ServiceFlags ALL_SERVICE_FLAGS[]{ |