forked from bitcoin/bitcoin
-
Notifications
You must be signed in to change notification settings - Fork 4
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge bitcoin#28155: net: improves addnode / m_added_nodes logic
0420f99 Create net_peer_connection unit tests (Jon Atack) 4b834f6 Allow unit tests to access additional CConnman members (Jon Atack) 34b9ef4 net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request (Sergi Delgado Segura) 94e8882 rpc: Prevents adding the same ip more than once when formatted differently (Sergi Delgado Segura) 2574b7e 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 0420f99 kashifs: > > tACK [0420f9](bitcoin@0420f99) sr-gi: > > > tACK [0420f9](bitcoin@0420f99) mzumsande: Tested ACK 0420f99 Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752
- Loading branch information
Showing
9 changed files
with
238 additions
and
36 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.