From b50febc0f0d0bfcd5cf5a92ef9a79302b71acb6d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 11 Mar 2022 10:14:31 +0100 Subject: [PATCH 01/12] merge bitcoin#24531: Use designated initializers --- src/Makefile.am | 1 + src/net.cpp | 21 +++++++++++++++------ src/util/designator.h | 21 +++++++++++++++++++++ 3 files changed, 37 insertions(+), 6 deletions(-) create mode 100644 src/util/designator.h diff --git a/src/Makefile.am b/src/Makefile.am index ae6b3863e2..bdad5e20c7 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -336,6 +336,7 @@ BITCOIN_CORE_H = \ util/bip32.h \ util/bytevectorhash.h \ util/check.h \ + util/designator.h \ util/edge.h \ util/enumerate.h \ util/epochguard.h \ diff --git a/src/net.cpp b/src/net.cpp index d7863ab05b..ba03e2703a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -1329,12 +1330,20 @@ bool CConnman::AttemptToEvictConnection() } } - NodeEvictionCandidate candidate = {node->GetId(), node->m_connected, node->m_min_ping_time, - node->m_last_block_time, node->m_last_tx_time, - node->m_has_all_wanted_services, - node->m_relays_txs.load(), node->m_bloom_filter_loaded.load(), - node->nKeyedNetGroup, node->m_prefer_evict, node->addr.IsLocal(), - node->ConnectedThroughNetwork()}; + NodeEvictionCandidate candidate{ + Desig(id) node->GetId(), + Desig(m_connected) node->m_connected, + Desig(m_min_ping_time) node->m_min_ping_time, + Desig(m_last_block_time) node->m_last_block_time, + Desig(m_last_tx_time) node->m_last_tx_time, + Desig(fRelevantServices) node->m_has_all_wanted_services, + Desig(m_relay_txs) node->m_relays_txs.load(), + Desig(fBloomFilter) node->m_bloom_filter_loaded.load(), + Desig(nKeyedNetGroup) node->nKeyedNetGroup, + Desig(prefer_evict) node->m_prefer_evict, + Desig(m_is_local) node->addr.IsLocal(), + Desig(m_network) node->ConnectedThroughNetwork(), + }; vEvictionCandidates.push_back(candidate); } } diff --git a/src/util/designator.h b/src/util/designator.h new file mode 100644 index 0000000000..3670b11e00 --- /dev/null +++ b/src/util/designator.h @@ -0,0 +1,21 @@ +// Copyright (c) 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. + +#ifndef BITCOIN_UTIL_DESIGNATOR_H +#define BITCOIN_UTIL_DESIGNATOR_H + +/** + * Designated initializers can be used to avoid ordering mishaps in aggregate + * initialization. However, they do not prevent uninitialized members. The + * checks can be disabled by defining DISABLE_DESIGNATED_INITIALIZER_ERRORS. + * This should only be needed on MSVC 2019. MSVC 2022 supports them with the + * option "/std:c++20" + */ +#ifndef DISABLE_DESIGNATED_INITIALIZER_ERRORS +#define Desig(field_name) .field_name = +#else +#define Desig(field_name) +#endif + +#endif // BITCOIN_UTIL_DESIGNATOR_H From 54bb3a438f6fc843863ac8dbf476bd22606440db Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 26 May 2022 15:40:21 +0200 Subject: [PATCH 02/12] merge bitcoin#25500: Move inbound eviction logic to its own translation unit --- src/Makefile.am | 4 + src/net.cpp | 231 +----------------------------- src/net.h | 125 +---------------- src/node/connection_types.cpp | 26 ++++ src/node/connection_types.h | 82 +++++++++++ src/node/eviction.cpp | 240 ++++++++++++++++++++++++++++++++ src/node/eviction.h | 69 +++++++++ src/test/fuzz/node_eviction.cpp | 2 + src/test/util/net.cpp | 3 + src/test/util/net.h | 1 + 10 files changed, 431 insertions(+), 352 deletions(-) create mode 100644 src/node/connection_types.cpp create mode 100644 src/node/connection_types.h create mode 100644 src/node/eviction.cpp create mode 100644 src/node/eviction.h diff --git a/src/Makefile.am b/src/Makefile.am index bdad5e20c7..0813c41c48 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -271,7 +271,9 @@ BITCOIN_CORE_H = \ node/blockstorage.h \ node/coin.h \ node/coinstats.h \ + node/connection_types.h \ node/context.h \ + node/eviction.h \ node/psbt.h \ node/transaction.h \ node/ui_interface.h \ @@ -499,7 +501,9 @@ libbitcoin_server_a_SOURCES = \ node/blockstorage.cpp \ node/coin.cpp \ node/coinstats.cpp \ + node/connection_types.cpp \ node/context.cpp \ + node/eviction.cpp \ node/interfaces.cpp \ node/psbt.cpp \ node/transaction.cpp \ diff --git a/src/net.cpp b/src/net.cpp index ba03e2703a..e114699b4b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -680,26 +681,6 @@ bool CNode::IsBlockRelayOnly() const { return (ignores_incoming_txs && !HasPermission(NetPermissionFlags::Relay)) || IsBlockOnlyConn(); } -std::string ConnectionTypeAsString(ConnectionType conn_type) -{ - switch (conn_type) { - case ConnectionType::INBOUND: - return "inbound"; - case ConnectionType::MANUAL: - return "manual"; - case ConnectionType::FEELER: - return "feeler"; - case ConnectionType::OUTBOUND_FULL_RELAY: - return "outbound-full-relay"; - case ConnectionType::BLOCK_RELAY: - return "block-relay-only"; - case ConnectionType::ADDR_FETCH: - return "addr-fetch"; - } // no default case, so the compiler can warn about missing cases - - assert(false); -} - CService CNode::GetAddrLocal() const { AssertLockNotHeld(m_addr_local_mutex); @@ -1085,210 +1066,6 @@ std::pair CConnman::SocketSendData(CNode& node) const return {nSentSize, data_left}; } -static bool ReverseCompareNodeMinPingTime(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b) -{ - return a.m_min_ping_time > b.m_min_ping_time; -} - -static bool ReverseCompareNodeTimeConnected(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b) -{ - return a.m_connected > b.m_connected; -} - -static bool CompareNetGroupKeyed(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) { - return a.nKeyedNetGroup < b.nKeyedNetGroup; -} - -static bool CompareNodeBlockTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) -{ - // There is a fall-through here because it is common for a node to have many peers which have not yet relayed a block. - if (a.m_last_block_time != b.m_last_block_time) return a.m_last_block_time < b.m_last_block_time; - if (a.fRelevantServices != b.fRelevantServices) return b.fRelevantServices; - return a.m_connected > b.m_connected; -} - -static bool CompareNodeTXTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) -{ - // There is a fall-through here because it is common for a node to have more than a few peers that have not yet relayed txn. - if (a.m_last_tx_time != b.m_last_tx_time) return a.m_last_tx_time < b.m_last_tx_time; - if (a.m_relay_txs != b.m_relay_txs) return b.m_relay_txs; - if (a.fBloomFilter != b.fBloomFilter) return a.fBloomFilter; - return a.m_connected > b.m_connected; -} - -// Pick out the potential block-relay only peers, and sort them by last block time. -static bool CompareNodeBlockRelayOnlyTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) -{ - if (a.m_relay_txs != b.m_relay_txs) return a.m_relay_txs; - if (a.m_last_block_time != b.m_last_block_time) return a.m_last_block_time < b.m_last_block_time; - if (a.fRelevantServices != b.fRelevantServices) return b.fRelevantServices; - return a.m_connected > b.m_connected; -} - -/** - * Sort eviction candidates by network/localhost and connection uptime. - * Candidates near the beginning are more likely to be evicted, and those - * near the end are more likely to be protected, e.g. less likely to be evicted. - * - First, nodes that are not `is_local` and that do not belong to `network`, - * sorted by increasing uptime (from most recently connected to connected longer). - * - Then, nodes that are `is_local` or belong to `network`, sorted by increasing uptime. - */ -struct CompareNodeNetworkTime { - const bool m_is_local; - const Network m_network; - CompareNodeNetworkTime(bool is_local, Network network) : m_is_local(is_local), m_network(network) {} - bool operator()(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b) const - { - if (m_is_local && a.m_is_local != b.m_is_local) return b.m_is_local; - if ((a.m_network == m_network) != (b.m_network == m_network)) return b.m_network == m_network; - return a.m_connected > b.m_connected; - }; -}; - -//! Sort an array by the specified comparator, then erase the last K elements where predicate is true. -template -static void EraseLastKElements( - std::vector& elements, Comparator comparator, size_t k, - std::function predicate = [](const NodeEvictionCandidate& n) { return true; }) -{ - std::sort(elements.begin(), elements.end(), comparator); - size_t eraseSize = std::min(k, elements.size()); - elements.erase(std::remove_if(elements.end() - eraseSize, elements.end(), predicate), elements.end()); -} - -void ProtectEvictionCandidatesByRatio(std::vector& eviction_candidates) -{ - // Protect the half of the remaining nodes which have been connected the longest. - // This replicates the non-eviction implicit behavior, and precludes attacks that start later. - // To favorise the diversity of our peer connections, reserve up to half of these protected - // spots for Tor/onion, localhost, I2P, and CJDNS peers, even if they're not longest uptime - // overall. This helps protect these higher-latency peers that tend to be otherwise - // disadvantaged under our eviction criteria. - const size_t initial_size = eviction_candidates.size(); - const size_t total_protect_size{initial_size / 2}; - - // Disadvantaged networks to protect. In the case of equal counts, earlier array members - // have the first opportunity to recover unused slots from the previous iteration. - struct Net { bool is_local; Network id; size_t count; }; - std::array networks{ - {{false, NET_CJDNS, 0}, {false, NET_I2P, 0}, {/*localhost=*/true, NET_MAX, 0}, {false, NET_ONION, 0}}}; - - // Count and store the number of eviction candidates per network. - for (Net& n : networks) { - n.count = std::count_if(eviction_candidates.cbegin(), eviction_candidates.cend(), - [&n](const NodeEvictionCandidate& c) { - return n.is_local ? c.m_is_local : c.m_network == n.id; - }); - } - // Sort `networks` by ascending candidate count, to give networks having fewer candidates - // the first opportunity to recover unused protected slots from the previous iteration. - std::stable_sort(networks.begin(), networks.end(), [](Net a, Net b) { return a.count < b.count; }); - - // Protect up to 25% of the eviction candidates by disadvantaged network. - const size_t max_protect_by_network{total_protect_size / 2}; - size_t num_protected{0}; - - while (num_protected < max_protect_by_network) { - // Count the number of disadvantaged networks from which we have peers to protect. - auto num_networks = std::count_if(networks.begin(), networks.end(), [](const Net& n) { return n.count; }); - if (num_networks == 0) { - break; - } - const size_t disadvantaged_to_protect{max_protect_by_network - num_protected}; - const size_t protect_per_network{std::max(disadvantaged_to_protect / num_networks, static_cast(1))}; - // Early exit flag if there are no remaining candidates by disadvantaged network. - bool protected_at_least_one{false}; - - for (Net& n : networks) { - if (n.count == 0) continue; - const size_t before = eviction_candidates.size(); - EraseLastKElements(eviction_candidates, CompareNodeNetworkTime(n.is_local, n.id), - protect_per_network, [&n](const NodeEvictionCandidate& c) { - return n.is_local ? c.m_is_local : c.m_network == n.id; - }); - const size_t after = eviction_candidates.size(); - if (before > after) { - protected_at_least_one = true; - const size_t delta{before - after}; - num_protected += delta; - if (num_protected >= max_protect_by_network) { - break; - } - n.count -= delta; - } - } - if (!protected_at_least_one) { - break; - } - } - - // Calculate how many we removed, and update our total number of peers that - // we want to protect based on uptime accordingly. - assert(num_protected == initial_size - eviction_candidates.size()); - const size_t remaining_to_protect{total_protect_size - num_protected}; - EraseLastKElements(eviction_candidates, ReverseCompareNodeTimeConnected, remaining_to_protect); -} - -[[nodiscard]] std::optional SelectNodeToEvict(std::vector&& vEvictionCandidates) -{ - // Protect connections with certain characteristics - - // Deterministically select 4 peers to protect by netgroup. - // An attacker cannot predict which netgroups will be protected - EraseLastKElements(vEvictionCandidates, CompareNetGroupKeyed, 4); - // Protect the 8 nodes with the lowest minimum ping time. - // An attacker cannot manipulate this metric without physically moving nodes closer to the target. - EraseLastKElements(vEvictionCandidates, ReverseCompareNodeMinPingTime, 8); - // Protect 4 nodes that most recently sent us novel transactions accepted into our mempool. - // An attacker cannot manipulate this metric without performing useful work. - EraseLastKElements(vEvictionCandidates, CompareNodeTXTime, 4); - // Protect up to 8 non-tx-relay peers that have sent us novel blocks. - EraseLastKElements(vEvictionCandidates, CompareNodeBlockRelayOnlyTime, 8, - [](const NodeEvictionCandidate& n) { return !n.m_relay_txs && n.fRelevantServices; }); - - // Protect 4 nodes that most recently sent us novel blocks. - // An attacker cannot manipulate this metric without performing useful work. - EraseLastKElements(vEvictionCandidates, CompareNodeBlockTime, 4); - - // Protect some of the remaining eviction candidates by ratios of desirable - // or disadvantaged characteristics. - ProtectEvictionCandidatesByRatio(vEvictionCandidates); - - if (vEvictionCandidates.empty()) return std::nullopt; - - // If any remaining peers are preferred for eviction consider only them. - // This happens after the other preferences since if a peer is really the best by other criteria (esp relaying blocks) - // then we probably don't want to evict it no matter what. - if (std::any_of(vEvictionCandidates.begin(),vEvictionCandidates.end(),[](NodeEvictionCandidate const &n){return n.prefer_evict;})) { - vEvictionCandidates.erase(std::remove_if(vEvictionCandidates.begin(),vEvictionCandidates.end(), - [](NodeEvictionCandidate const &n){return !n.prefer_evict;}),vEvictionCandidates.end()); - } - - // Identify the network group with the most connections and youngest member. - // (vEvictionCandidates is already sorted by reverse connect time) - uint64_t naMostConnections; - unsigned int nMostConnections = 0; - std::chrono::seconds nMostConnectionsTime{0}; - std::map > mapNetGroupNodes; - for (const NodeEvictionCandidate &node : vEvictionCandidates) { - std::vector &group = mapNetGroupNodes[node.nKeyedNetGroup]; - group.push_back(node); - const auto grouptime{group[0].m_connected}; - - if (group.size() > nMostConnections || (group.size() == nMostConnections && grouptime > nMostConnectionsTime)) { - nMostConnections = group.size(); - nMostConnectionsTime = grouptime; - naMostConnections = node.nKeyedNetGroup; - } - } - - // Reduce to the network group with the most connections - vEvictionCandidates = std::move(mapNetGroupNodes[naMostConnections]); - - // Disconnect from the network group with the most connections - return vEvictionCandidates.front().id; -} - /** Try to find a connection to evict when the node is full. * Extreme care must be taken to avoid opening the node to attacker * triggered network partitioning. @@ -1304,10 +1081,6 @@ bool CConnman::AttemptToEvictConnection() LOCK(m_nodes_mutex); for (const CNode* node : m_nodes) { - if (node->HasPermission(NetPermissionFlags::NoBan)) - continue; - if (!node->IsInboundConn()) - continue; if (node->fDisconnect) continue; @@ -1343,6 +1116,8 @@ bool CConnman::AttemptToEvictConnection() Desig(prefer_evict) node->m_prefer_evict, Desig(m_is_local) node->addr.IsLocal(), Desig(m_network) node->ConnectedThroughNetwork(), + Desig(m_noban) node->HasPermission(NetPermissionFlags::NoBan), + Desig(m_conn_type) node->m_conn_type, }; vEvictionCandidates.push_back(candidate); } diff --git a/src/net.h b/src/net.h index 2cc3626170..7dae298373 100644 --- a/src/net.h +++ b/src/net.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -158,78 +159,6 @@ struct CSerializedNetMsg { size_t GetMemoryUsage() const noexcept; }; -/** Different types of connections to a peer. This enum encapsulates the - * information we have available at the time of opening or accepting the - * connection. Aside from INBOUND, all types are initiated by us. - * - * If adding or removing types, please update CONNECTION_TYPE_DOC in - * src/rpc/net.cpp and src/qt/rpcconsole.cpp, as well as the descriptions in - * src/qt/guiutil.cpp and src/bitcoin-cli.cpp::NetinfoRequestHandler. */ -enum class ConnectionType { - /** - * Inbound connections are those initiated by a peer. This is the only - * property we know at the time of connection, until P2P messages are - * exchanged. - */ - INBOUND, - - /** - * These are the default connections that we use to connect with the - * network. There is no restriction on what is relayed; by default we relay - * blocks, addresses & transactions. We automatically attempt to open - * MAX_OUTBOUND_FULL_RELAY_CONNECTIONS using addresses from our AddrMan. - */ - OUTBOUND_FULL_RELAY, - - - /** - * We open manual connections to addresses that users explicitly requested - * via the addnode RPC or the -addnode/-connect configuration options. Even if a - * manual connection is misbehaving, we do not automatically disconnect or - * add it to our discouragement filter. - */ - MANUAL, - - /** - * Feeler connections are short-lived connections made to check that a node - * is alive. They can be useful for: - * - test-before-evict: if one of the peers is considered for eviction from - * our AddrMan because another peer is mapped to the same slot in the tried table, - * evict only if this longer-known peer is offline. - * - move node addresses from New to Tried table, so that we have more - * connectable addresses in our AddrMan. - * Note that in the literature ("Eclipse Attacks on Bitcoin’s Peer-to-Peer Network") - * only the latter feature is referred to as "feeler connections", - * although in our codebase feeler connections encompass test-before-evict as well. - * We make these connections approximately every FEELER_INTERVAL: - * first we resolve previously found collisions if they exist (test-before-evict), - * otherwise we connect to a node from the new table. - */ - FEELER, - - /** - * We use block-relay-only connections to help prevent against partition - * attacks. By not relaying transactions or addresses, these connections - * are harder to detect by a third party, thus helping obfuscate the - * network topology. We automatically attempt to open - * MAX_BLOCK_RELAY_ONLY_ANCHORS using addresses from our anchors.dat. Then - * addresses from our AddrMan if MAX_BLOCK_RELAY_ONLY_CONNECTIONS - * isn't reached yet. - */ - BLOCK_RELAY, - - /** - * AddrFetch connections are short lived connections used to solicit - * addresses from peers. These are initiated to addresses submitted via the - * -seednode command line argument, or under certain conditions when the - * AddrMan is empty. - */ - ADDR_FETCH, -}; - -/** Convert ConnectionType enum to a string value */ -std::string ConnectionTypeAsString(ConnectionType conn_type); - /** * Look up IP addresses from all interfaces on the machine and add them to the * list of local addresses to self-advertise. @@ -1711,62 +1640,10 @@ extern std::function CaptureMessage; -struct NodeEvictionCandidate -{ - NodeId id; - std::chrono::seconds m_connected; - std::chrono::microseconds m_min_ping_time; - std::chrono::seconds m_last_block_time; - std::chrono::seconds m_last_tx_time; - bool fRelevantServices; - bool m_relay_txs; - bool fBloomFilter; - uint64_t nKeyedNetGroup; - bool prefer_evict; - bool m_is_local; - Network m_network; -}; - -/** - * Select an inbound peer to evict after filtering out (protecting) peers having - * distinct, difficult-to-forge characteristics. The protection logic picks out - * fixed numbers of desirable peers per various criteria, followed by (mostly) - * ratios of desirable or disadvantaged peers. If any eviction candidates - * remain, the selection logic chooses a peer to evict. - */ -[[nodiscard]] std::optional SelectNodeToEvict(std::vector&& vEvictionCandidates); - class CExplicitNetCleanup { public: static void callCleanup(); }; -extern RecursiveMutex cs_main; - -/** Protect desirable or disadvantaged inbound peers from eviction by ratio. - * - * This function protects half of the peers which have been connected the - * longest, to replicate the non-eviction implicit behavior and preclude attacks - * that start later. - * - * Half of these protected spots (1/4 of the total) are reserved for the - * following categories of peers, sorted by longest uptime, even if they're not - * longest uptime overall: - * - * - onion peers connected via our tor control service - * - * - localhost peers, as manually configured hidden services not using - * `-bind=addr[:port]=onion` will not be detected as inbound onion connections - * - * - I2P peers - * - * - CJDNS peers - * - * This helps protect these privacy network peers, which tend to be otherwise - * disadvantaged under our eviction criteria for their higher min ping times - * relative to IPv4/IPv6 peers, and favorise the diversity of peer connections. - */ -void ProtectEvictionCandidatesByRatio(std::vector& vEvictionCandidates); - #endif // BITCOIN_NET_H diff --git a/src/node/connection_types.cpp b/src/node/connection_types.cpp new file mode 100644 index 0000000000..904f4371aa --- /dev/null +++ b/src/node/connection_types.cpp @@ -0,0 +1,26 @@ +// Copyright (c) 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 +#include + +std::string ConnectionTypeAsString(ConnectionType conn_type) +{ + switch (conn_type) { + case ConnectionType::INBOUND: + return "inbound"; + case ConnectionType::MANUAL: + return "manual"; + case ConnectionType::FEELER: + return "feeler"; + case ConnectionType::OUTBOUND_FULL_RELAY: + return "outbound-full-relay"; + case ConnectionType::BLOCK_RELAY: + return "block-relay-only"; + case ConnectionType::ADDR_FETCH: + return "addr-fetch"; + } // no default case, so the compiler can warn about missing cases + + assert(false); +} diff --git a/src/node/connection_types.h b/src/node/connection_types.h new file mode 100644 index 0000000000..5e1abcace6 --- /dev/null +++ b/src/node/connection_types.h @@ -0,0 +1,82 @@ +// Copyright (c) 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. + +#ifndef BITCOIN_NODE_CONNECTION_TYPES_H +#define BITCOIN_NODE_CONNECTION_TYPES_H + +#include + +/** Different types of connections to a peer. This enum encapsulates the + * information we have available at the time of opening or accepting the + * connection. Aside from INBOUND, all types are initiated by us. + * + * If adding or removing types, please update CONNECTION_TYPE_DOC in + * src/rpc/net.cpp and src/qt/rpcconsole.cpp, as well as the descriptions in + * src/qt/guiutil.cpp and src/bitcoin-cli.cpp::NetinfoRequestHandler. */ +enum class ConnectionType { + /** + * Inbound connections are those initiated by a peer. This is the only + * property we know at the time of connection, until P2P messages are + * exchanged. + */ + INBOUND, + + /** + * These are the default connections that we use to connect with the + * network. There is no restriction on what is relayed; by default we relay + * blocks, addresses & transactions. We automatically attempt to open + * MAX_OUTBOUND_FULL_RELAY_CONNECTIONS using addresses from our AddrMan. + */ + OUTBOUND_FULL_RELAY, + + + /** + * We open manual connections to addresses that users explicitly requested + * via the addnode RPC or the -addnode/-connect configuration options. Even if a + * manual connection is misbehaving, we do not automatically disconnect or + * add it to our discouragement filter. + */ + MANUAL, + + /** + * Feeler connections are short-lived connections made to check that a node + * is alive. They can be useful for: + * - test-before-evict: if one of the peers is considered for eviction from + * our AddrMan because another peer is mapped to the same slot in the tried table, + * evict only if this longer-known peer is offline. + * - move node addresses from New to Tried table, so that we have more + * connectable addresses in our AddrMan. + * Note that in the literature ("Eclipse Attacks on Bitcoin’s Peer-to-Peer Network") + * only the latter feature is referred to as "feeler connections", + * although in our codebase feeler connections encompass test-before-evict as well. + * We make these connections approximately every FEELER_INTERVAL: + * first we resolve previously found collisions if they exist (test-before-evict), + * otherwise we connect to a node from the new table. + */ + FEELER, + + /** + * We use block-relay-only connections to help prevent against partition + * attacks. By not relaying transactions or addresses, these connections + * are harder to detect by a third party, thus helping obfuscate the + * network topology. We automatically attempt to open + * MAX_BLOCK_RELAY_ONLY_ANCHORS using addresses from our anchors.dat. Then + * addresses from our AddrMan if MAX_BLOCK_RELAY_ONLY_CONNECTIONS + * isn't reached yet. + */ + BLOCK_RELAY, + + /** + * AddrFetch connections are short lived connections used to solicit + * addresses from peers. These are initiated to addresses submitted via the + * -seednode command line argument, or under certain conditions when the + * AddrMan is empty. + */ + ADDR_FETCH, +}; + +/** Convert ConnectionType enum to a string value */ +std::string ConnectionTypeAsString(ConnectionType conn_type); + +#endif // BITCOIN_NODE_CONNECTION_TYPES_H diff --git a/src/node/eviction.cpp b/src/node/eviction.cpp new file mode 100644 index 0000000000..33406931d4 --- /dev/null +++ b/src/node/eviction.cpp @@ -0,0 +1,240 @@ +// Copyright (c) 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 + +#include +#include +#include +#include +#include +#include +#include + + +static bool ReverseCompareNodeMinPingTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) +{ + return a.m_min_ping_time > b.m_min_ping_time; +} + +static bool ReverseCompareNodeTimeConnected(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) +{ + return a.m_connected > b.m_connected; +} + +static bool CompareNetGroupKeyed(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) { + return a.nKeyedNetGroup < b.nKeyedNetGroup; +} + +static bool CompareNodeBlockTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) +{ + // There is a fall-through here because it is common for a node to have many peers which have not yet relayed a block. + if (a.m_last_block_time != b.m_last_block_time) return a.m_last_block_time < b.m_last_block_time; + if (a.fRelevantServices != b.fRelevantServices) return b.fRelevantServices; + return a.m_connected > b.m_connected; +} + +static bool CompareNodeTXTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) +{ + // There is a fall-through here because it is common for a node to have more than a few peers that have not yet relayed txn. + if (a.m_last_tx_time != b.m_last_tx_time) return a.m_last_tx_time < b.m_last_tx_time; + if (a.m_relay_txs != b.m_relay_txs) return b.m_relay_txs; + if (a.fBloomFilter != b.fBloomFilter) return a.fBloomFilter; + return a.m_connected > b.m_connected; +} + +// Pick out the potential block-relay only peers, and sort them by last block time. +static bool CompareNodeBlockRelayOnlyTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) +{ + if (a.m_relay_txs != b.m_relay_txs) return a.m_relay_txs; + if (a.m_last_block_time != b.m_last_block_time) return a.m_last_block_time < b.m_last_block_time; + if (a.fRelevantServices != b.fRelevantServices) return b.fRelevantServices; + return a.m_connected > b.m_connected; +} + +/** + * Sort eviction candidates by network/localhost and connection uptime. + * Candidates near the beginning are more likely to be evicted, and those + * near the end are more likely to be protected, e.g. less likely to be evicted. + * - First, nodes that are not `is_local` and that do not belong to `network`, + * sorted by increasing uptime (from most recently connected to connected longer). + * - Then, nodes that are `is_local` or belong to `network`, sorted by increasing uptime. + */ +struct CompareNodeNetworkTime { + const bool m_is_local; + const Network m_network; + CompareNodeNetworkTime(bool is_local, Network network) : m_is_local(is_local), m_network(network) {} + bool operator()(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b) const + { + if (m_is_local && a.m_is_local != b.m_is_local) return b.m_is_local; + if ((a.m_network == m_network) != (b.m_network == m_network)) return b.m_network == m_network; + return a.m_connected > b.m_connected; + }; +}; + +//! Sort an array by the specified comparator, then erase the last K elements where predicate is true. +template +static void EraseLastKElements( + std::vector& elements, Comparator comparator, size_t k, + std::function predicate = [](const NodeEvictionCandidate& n) { return true; }) +{ + std::sort(elements.begin(), elements.end(), comparator); + size_t eraseSize = std::min(k, elements.size()); + elements.erase(std::remove_if(elements.end() - eraseSize, elements.end(), predicate), elements.end()); +} + +void ProtectNoBanConnections(std::vector& eviction_candidates) +{ + eviction_candidates.erase(std::remove_if(eviction_candidates.begin(), eviction_candidates.end(), + [](NodeEvictionCandidate const& n) { + return n.m_noban; + }), + eviction_candidates.end()); +} + +void ProtectOutboundConnections(std::vector& eviction_candidates) +{ + eviction_candidates.erase(std::remove_if(eviction_candidates.begin(), eviction_candidates.end(), + [](NodeEvictionCandidate const& n) { + return n.m_conn_type != ConnectionType::INBOUND; + }), + eviction_candidates.end()); +} + +void ProtectEvictionCandidatesByRatio(std::vector& eviction_candidates) +{ + // Protect the half of the remaining nodes which have been connected the longest. + // This replicates the non-eviction implicit behavior, and precludes attacks that start later. + // To favorise the diversity of our peer connections, reserve up to half of these protected + // spots for Tor/onion, localhost, I2P, and CJDNS peers, even if they're not longest uptime + // overall. This helps protect these higher-latency peers that tend to be otherwise + // disadvantaged under our eviction criteria. + const size_t initial_size = eviction_candidates.size(); + const size_t total_protect_size{initial_size / 2}; + + // Disadvantaged networks to protect. In the case of equal counts, earlier array members + // have the first opportunity to recover unused slots from the previous iteration. + struct Net { bool is_local; Network id; size_t count; }; + std::array networks{ + {{false, NET_CJDNS, 0}, {false, NET_I2P, 0}, {/*localhost=*/true, NET_MAX, 0}, {false, NET_ONION, 0}}}; + + // Count and store the number of eviction candidates per network. + for (Net& n : networks) { + n.count = std::count_if(eviction_candidates.cbegin(), eviction_candidates.cend(), + [&n](const NodeEvictionCandidate& c) { + return n.is_local ? c.m_is_local : c.m_network == n.id; + }); + } + // Sort `networks` by ascending candidate count, to give networks having fewer candidates + // the first opportunity to recover unused protected slots from the previous iteration. + std::stable_sort(networks.begin(), networks.end(), [](Net a, Net b) { return a.count < b.count; }); + + // Protect up to 25% of the eviction candidates by disadvantaged network. + const size_t max_protect_by_network{total_protect_size / 2}; + size_t num_protected{0}; + + while (num_protected < max_protect_by_network) { + // Count the number of disadvantaged networks from which we have peers to protect. + auto num_networks = std::count_if(networks.begin(), networks.end(), [](const Net& n) { return n.count; }); + if (num_networks == 0) { + break; + } + const size_t disadvantaged_to_protect{max_protect_by_network - num_protected}; + const size_t protect_per_network{std::max(disadvantaged_to_protect / num_networks, static_cast(1))}; + // Early exit flag if there are no remaining candidates by disadvantaged network. + bool protected_at_least_one{false}; + + for (Net& n : networks) { + if (n.count == 0) continue; + const size_t before = eviction_candidates.size(); + EraseLastKElements(eviction_candidates, CompareNodeNetworkTime(n.is_local, n.id), + protect_per_network, [&n](const NodeEvictionCandidate& c) { + return n.is_local ? c.m_is_local : c.m_network == n.id; + }); + const size_t after = eviction_candidates.size(); + if (before > after) { + protected_at_least_one = true; + const size_t delta{before - after}; + num_protected += delta; + if (num_protected >= max_protect_by_network) { + break; + } + n.count -= delta; + } + } + if (!protected_at_least_one) { + break; + } + } + + // Calculate how many we removed, and update our total number of peers that + // we want to protect based on uptime accordingly. + assert(num_protected == initial_size - eviction_candidates.size()); + const size_t remaining_to_protect{total_protect_size - num_protected}; + EraseLastKElements(eviction_candidates, ReverseCompareNodeTimeConnected, remaining_to_protect); +} + +[[nodiscard]] std::optional SelectNodeToEvict(std::vector&& vEvictionCandidates) +{ + // Protect connections with certain characteristics + + ProtectNoBanConnections(vEvictionCandidates); + + ProtectOutboundConnections(vEvictionCandidates); + + // Deterministically select 4 peers to protect by netgroup. + // An attacker cannot predict which netgroups will be protected + EraseLastKElements(vEvictionCandidates, CompareNetGroupKeyed, 4); + // Protect the 8 nodes with the lowest minimum ping time. + // An attacker cannot manipulate this metric without physically moving nodes closer to the target. + EraseLastKElements(vEvictionCandidates, ReverseCompareNodeMinPingTime, 8); + // Protect 4 nodes that most recently sent us novel transactions accepted into our mempool. + // An attacker cannot manipulate this metric without performing useful work. + EraseLastKElements(vEvictionCandidates, CompareNodeTXTime, 4); + // Protect up to 8 non-tx-relay peers that have sent us novel blocks. + EraseLastKElements(vEvictionCandidates, CompareNodeBlockRelayOnlyTime, 8, + [](const NodeEvictionCandidate& n) { return !n.m_relay_txs && n.fRelevantServices; }); + + // Protect 4 nodes that most recently sent us novel blocks. + // An attacker cannot manipulate this metric without performing useful work. + EraseLastKElements(vEvictionCandidates, CompareNodeBlockTime, 4); + + // Protect some of the remaining eviction candidates by ratios of desirable + // or disadvantaged characteristics. + ProtectEvictionCandidatesByRatio(vEvictionCandidates); + + if (vEvictionCandidates.empty()) return std::nullopt; + + // If any remaining peers are preferred for eviction consider only them. + // This happens after the other preferences since if a peer is really the best by other criteria (esp relaying blocks) + // then we probably don't want to evict it no matter what. + if (std::any_of(vEvictionCandidates.begin(),vEvictionCandidates.end(),[](NodeEvictionCandidate const &n){return n.prefer_evict;})) { + vEvictionCandidates.erase(std::remove_if(vEvictionCandidates.begin(),vEvictionCandidates.end(), + [](NodeEvictionCandidate const &n){return !n.prefer_evict;}),vEvictionCandidates.end()); + } + + // Identify the network group with the most connections and youngest member. + // (vEvictionCandidates is already sorted by reverse connect time) + uint64_t naMostConnections; + unsigned int nMostConnections = 0; + std::chrono::seconds nMostConnectionsTime{0}; + std::map > mapNetGroupNodes; + for (const NodeEvictionCandidate &node : vEvictionCandidates) { + std::vector &group = mapNetGroupNodes[node.nKeyedNetGroup]; + group.push_back(node); + const auto grouptime{group[0].m_connected}; + + if (group.size() > nMostConnections || (group.size() == nMostConnections && grouptime > nMostConnectionsTime)) { + nMostConnections = group.size(); + nMostConnectionsTime = grouptime; + naMostConnections = node.nKeyedNetGroup; + } + } + + // Reduce to the network group with the most connections + vEvictionCandidates = std::move(mapNetGroupNodes[naMostConnections]); + + // Disconnect from the network group with the most connections + return vEvictionCandidates.front().id; +} diff --git a/src/node/eviction.h b/src/node/eviction.h new file mode 100644 index 0000000000..1bb32e5327 --- /dev/null +++ b/src/node/eviction.h @@ -0,0 +1,69 @@ +// Copyright (c) 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. + +#ifndef BITCOIN_NODE_EVICTION_H +#define BITCOIN_NODE_EVICTION_H + +#include +#include + +#include +#include +#include +#include + +typedef int64_t NodeId; + +struct NodeEvictionCandidate { + NodeId id; + std::chrono::seconds m_connected; + std::chrono::microseconds m_min_ping_time; + std::chrono::seconds m_last_block_time; + std::chrono::seconds m_last_tx_time; + bool fRelevantServices; + bool m_relay_txs; + bool fBloomFilter; + uint64_t nKeyedNetGroup; + bool prefer_evict; + bool m_is_local; + Network m_network; + bool m_noban; + ConnectionType m_conn_type; +}; + +/** + * Select an inbound peer to evict after filtering out (protecting) peers having + * distinct, difficult-to-forge characteristics. The protection logic picks out + * fixed numbers of desirable peers per various criteria, followed by (mostly) + * ratios of desirable or disadvantaged peers. If any eviction candidates + * remain, the selection logic chooses a peer to evict. + */ +[[nodiscard]] std::optional SelectNodeToEvict(std::vector&& vEvictionCandidates); + +/** Protect desirable or disadvantaged inbound peers from eviction by ratio. + * + * This function protects half of the peers which have been connected the + * longest, to replicate the non-eviction implicit behavior and preclude attacks + * that start later. + * + * Half of these protected spots (1/4 of the total) are reserved for the + * following categories of peers, sorted by longest uptime, even if they're not + * longest uptime overall: + * + * - onion peers connected via our tor control service + * + * - localhost peers, as manually configured hidden services not using + * `-bind=addr[:port]=onion` will not be detected as inbound onion connections + * + * - I2P peers + * + * - CJDNS peers + * + * This helps protect these privacy network peers, which tend to be otherwise + * disadvantaged under our eviction criteria for their higher min ping times + * relative to IPv4/IPv6 peers, and favorise the diversity of peer connections. + */ +void ProtectEvictionCandidatesByRatio(std::vector& vEvictionCandidates); + +#endif // BITCOIN_NODE_EVICTION_H diff --git a/src/test/fuzz/node_eviction.cpp b/src/test/fuzz/node_eviction.cpp index 0835b399e0..1d6257e28c 100644 --- a/src/test/fuzz/node_eviction.cpp +++ b/src/test/fuzz/node_eviction.cpp @@ -32,6 +32,8 @@ FUZZ_TARGET(node_eviction) /* prefer_evict */ fuzzed_data_provider.ConsumeBool(), /* m_is_local */ fuzzed_data_provider.ConsumeBool(), /* m_network */ fuzzed_data_provider.PickValueInArray(ALL_NETWORKS), + /* m_noban */ fuzzed_data_provider.ConsumeBool(), + /* m_conn_type */ fuzzed_data_provider.PickValueInArray(ALL_CONNECTION_TYPES), }); } // Make a copy since eviction_candidates may be in some valid but otherwise diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index fc6a29d424..79dbc8ab9a 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -128,6 +129,8 @@ std::vector GetRandomNodeEvictionCandidates(int n_candida /* 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; diff --git a/src/test/util/net.h b/src/test/util/net.h index dc758bd43e..7f203c6d55 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -6,6 +6,7 @@ #define BITCOIN_TEST_UTIL_NET_H #include +#include #include #include #include From 6d4945418a472d9e25721baf3f767ca7f35c0781 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 30 Aug 2024 19:57:59 +0000 Subject: [PATCH 03/12] partial bitcoin#25472: Increase MS Visual Studio minimum version contains: - 630c1711 (excl. changes to mempool as they haven't been backported yet) --- src/Makefile.am | 1 - src/net.cpp | 29 ++++++++++++++--------------- src/util/designator.h | 21 --------------------- 3 files changed, 14 insertions(+), 37 deletions(-) delete mode 100644 src/util/designator.h diff --git a/src/Makefile.am b/src/Makefile.am index 0813c41c48..7ca5f7ce75 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -338,7 +338,6 @@ BITCOIN_CORE_H = \ util/bip32.h \ util/bytevectorhash.h \ util/check.h \ - util/designator.h \ util/edge.h \ util/enumerate.h \ util/epochguard.h \ diff --git a/src/net.cpp b/src/net.cpp index e114699b4b..806543d96f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -28,7 +28,6 @@ #include #include #include -#include #include #include #include @@ -1104,20 +1103,20 @@ bool CConnman::AttemptToEvictConnection() } NodeEvictionCandidate candidate{ - Desig(id) node->GetId(), - Desig(m_connected) node->m_connected, - Desig(m_min_ping_time) node->m_min_ping_time, - Desig(m_last_block_time) node->m_last_block_time, - Desig(m_last_tx_time) node->m_last_tx_time, - Desig(fRelevantServices) node->m_has_all_wanted_services, - Desig(m_relay_txs) node->m_relays_txs.load(), - Desig(fBloomFilter) node->m_bloom_filter_loaded.load(), - Desig(nKeyedNetGroup) node->nKeyedNetGroup, - Desig(prefer_evict) node->m_prefer_evict, - Desig(m_is_local) node->addr.IsLocal(), - Desig(m_network) node->ConnectedThroughNetwork(), - Desig(m_noban) node->HasPermission(NetPermissionFlags::NoBan), - Desig(m_conn_type) node->m_conn_type, + .id = node->GetId(), + .m_connected = node->m_connected, + .m_min_ping_time = node->m_min_ping_time, + .m_last_block_time = node->m_last_block_time, + .m_last_tx_time = node->m_last_tx_time, + .fRelevantServices = node->m_has_all_wanted_services, + .m_relay_txs = node->m_relays_txs.load(), + .fBloomFilter = node->m_bloom_filter_loaded.load(), + .nKeyedNetGroup = node->nKeyedNetGroup, + .prefer_evict = node->m_prefer_evict, + .m_is_local = node->addr.IsLocal(), + .m_network = node->ConnectedThroughNetwork(), + .m_noban = node->HasPermission(NetPermissionFlags::NoBan), + .m_conn_type = node->m_conn_type, }; vEvictionCandidates.push_back(candidate); } diff --git a/src/util/designator.h b/src/util/designator.h deleted file mode 100644 index 3670b11e00..0000000000 --- a/src/util/designator.h +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright (c) 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. - -#ifndef BITCOIN_UTIL_DESIGNATOR_H -#define BITCOIN_UTIL_DESIGNATOR_H - -/** - * Designated initializers can be used to avoid ordering mishaps in aggregate - * initialization. However, they do not prevent uninitialized members. The - * checks can be disabled by defining DISABLE_DESIGNATED_INITIALIZER_ERRORS. - * This should only be needed on MSVC 2019. MSVC 2022 supports them with the - * option "/std:c++20" - */ -#ifndef DISABLE_DESIGNATED_INITIALIZER_ERRORS -#define Desig(field_name) .field_name = -#else -#define Desig(field_name) -#endif - -#endif // BITCOIN_UTIL_DESIGNATOR_H From 79e67fd96a3ba1680fd24ac0110faa8ea1464c84 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 10 Aug 2022 15:09:29 +0200 Subject: [PATCH 04/12] merge bitcoin#25814: simplify GetLocalAddress() --- src/net.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 806543d96f..5de0d9740a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -238,12 +238,11 @@ static std::vector ConvertSeeds(const std::vector &vSeedsIn) // one by discovery. CService GetLocalAddress(const CNetAddr& addrPeer) { - CService ret{CNetAddr(), GetListenPort()}; CService addr; if (GetLocal(addr, &addrPeer)) { - ret = CService{addr}; + return addr; } - return ret; + return CService{CNetAddr(), GetListenPort()}; } static int GetnScore(const CService& addr) From d9e56f3e780f597de1f7bf4ec88fce345267253b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 31 Aug 2022 17:04:13 +1000 Subject: [PATCH 05/12] merge bitcoin#25962: Add CNodeOptions and increase constness --- src/net.cpp | 46 ++++++++++++++++-------------- src/net.h | 21 +++++++++----- src/qt/rpcconsole.cpp | 4 +-- src/rpc/net.cpp | 2 +- src/test/denialofservice_tests.cpp | 1 - src/test/fuzz/util.cpp | 1 - src/test/fuzz/util.h | 7 +++-- src/test/util/net.cpp | 2 -- src/test/util/net.h | 1 - 9 files changed, 47 insertions(+), 38 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 5de0d9740a..30777a3f59 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -617,7 +617,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo pszDest ? pszDest : "", conn_type, /*inbound_onion=*/false, - std::move(i2p_transient_session)); + CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session) }); pnode->AddRef(); ::g_stats_client->inc("peers.connect", 1.0f); @@ -741,7 +741,7 @@ void CNode::CopyStats(CNodeStats& stats) X(nRecvBytes); } X(m_legacyWhitelisted); - X(m_permissionFlags); + X(m_permission_flags); X(m_last_ping_time); X(m_min_ping_time); @@ -1157,14 +1157,14 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket, CMasternodeSy const CAddress addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock)), NODE_NONE}; - NetPermissionFlags permissionFlags = NetPermissionFlags::None; - hListenSocket.AddSocketPermissionFlags(permissionFlags); + NetPermissionFlags permission_flags = NetPermissionFlags::None; + hListenSocket.AddSocketPermissionFlags(permission_flags); - CreateNodeFromAcceptedSocket(std::move(sock), permissionFlags, addr_bind, addr, mn_sync); + CreateNodeFromAcceptedSocket(std::move(sock), permission_flags, addr_bind, addr, mn_sync); } void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, - NetPermissionFlags permissionFlags, + NetPermissionFlags permission_flags, const CAddress& addr_bind, const CAddress& addr, CMasternodeSync& mn_sync) @@ -1173,14 +1173,14 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, int nVerifiedInboundMasternodes = 0; int nMaxInbound = nMaxConnections - m_max_outbound; - AddWhitelistPermissionFlags(permissionFlags, addr); + AddWhitelistPermissionFlags(permission_flags, addr); bool legacyWhitelisted = false; - if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::Implicit)) { - NetPermissions::ClearFlag(permissionFlags, NetPermissionFlags::Implicit); - if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::ForceRelay); - if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::Relay); - NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::Mempool); - NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::NoBan); + if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::Implicit)) { + NetPermissions::ClearFlag(permission_flags, NetPermissionFlags::Implicit); + if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permission_flags, NetPermissionFlags::ForceRelay); + if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Relay); + NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Mempool); + NetPermissions::AddFlag(permission_flags, NetPermissionFlags::NoBan); legacyWhitelisted = true; } @@ -1225,7 +1225,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, // Don't accept connections from banned peers. bool banned = m_banman && m_banman->IsBanned(addr); - if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::NoBan) && banned) + if (!NetPermissions::HasFlag(permission_flags, NetPermissionFlags::NoBan) && banned) { LogPrint(BCLog::NET, "%s (banned)\n", strDropped); return; @@ -1233,7 +1233,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, // Only accept connections from discouraged peers if our inbound slots aren't (almost) full. bool discouraged = m_banman && m_banman->IsDiscouraged(addr); - if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::NoBan) && nInbound + 1 >= nMaxInbound && discouraged) + if (!NetPermissions::HasFlag(permission_flags, NetPermissionFlags::NoBan) && nInbound + 1 >= nMaxInbound && discouraged) { LogPrint(BCLog::NET, "connection from %s dropped (discouraged)\n", addr.ToStringAddrPort()); return; @@ -1264,7 +1264,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); ServiceFlags nodeServices = nLocalServices; - if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::BloomFilter)) { + if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::BloomFilter)) { nodeServices = static_cast(nodeServices | NODE_BLOOM); } @@ -1277,12 +1277,14 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, addr_bind, /*addrNameIn=*/"", ConnectionType::INBOUND, - inbound_onion); + inbound_onion, + CNodeOptions{ + .permission_flags = permission_flags, + .prefer_evict = discouraged, + }); pnode->AddRef(); - pnode->m_permissionFlags = permissionFlags; // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility) pnode->m_legacyWhitelisted = legacyWhitelisted; - pnode->m_prefer_evict = discouraged; m_msgproc->InitializeNode(*pnode, nodeServices); { @@ -4043,19 +4045,21 @@ CNode::CNode(NodeId idIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, - std::unique_ptr&& i2p_sam_session) + CNodeOptions&& node_opts) : m_transport{std::make_unique(idIn, SER_NETWORK, INIT_PROTO_VERSION)}, + m_permission_flags{node_opts.permission_flags}, m_sock{sock}, m_connected{GetTime()}, addr{addrIn}, addrBind{addrBindIn}, m_addr_name{addrNameIn.empty() ? addr.ToStringAddrPort() : addrNameIn}, m_inbound_onion{inbound_onion}, + m_prefer_evict{node_opts.prefer_evict}, nKeyedNetGroup{nKeyedNetGroupIn}, id{idIn}, nLocalHostNonce{nLocalHostNonceIn}, m_conn_type{conn_type_in}, - m_i2p_sam_session{std::move(i2p_sam_session)} + m_i2p_sam_session{std::move(node_opts.i2p_sam_session)} { if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND); diff --git a/src/net.h b/src/net.h index 7dae298373..9bb3cddfd9 100644 --- a/src/net.h +++ b/src/net.h @@ -242,7 +242,7 @@ public: mapMsgTypeSize mapSendBytesPerMsgType; uint64_t nRecvBytes; mapMsgTypeSize mapRecvBytesPerMsgType; - NetPermissionFlags m_permissionFlags; + NetPermissionFlags m_permission_flags; bool m_legacyWhitelisted; std::chrono::microseconds m_last_ping_time; std::chrono::microseconds m_min_ping_time; @@ -449,6 +449,13 @@ public: size_t GetSendMemoryUsage() const noexcept override EXCLUSIVE_LOCKS_REQUIRED(!m_send_mutex); }; +struct CNodeOptions +{ + NetPermissionFlags permission_flags = NetPermissionFlags::None; + std::unique_ptr i2p_sam_session = nullptr; + bool prefer_evict = false; +}; + /** Information about a peer */ class CNode { @@ -460,7 +467,7 @@ public: * the sending side functions are only called under cs_vSend. */ const std::unique_ptr m_transport; - NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester + const NetPermissionFlags m_permission_flags; /** * Socket used for communication with the node. @@ -512,9 +519,9 @@ public: * from the wire. This cleaned string can safely be logged or displayed. */ std::string cleanSubVer GUARDED_BY(m_subver_mutex){}; - bool m_prefer_evict{false}; // This peer is preferred for eviction. (treated as const) + const bool m_prefer_evict{false}; // This peer is preferred for eviction. bool HasPermission(NetPermissionFlags permission) const { - return NetPermissions::HasFlag(m_permissionFlags, permission); + return NetPermissions::HasFlag(m_permission_flags, permission); } // This boolean is unusued in actual processing, only present for backward compatibility at RPC/QT level bool m_legacyWhitelisted{false}; @@ -669,7 +676,7 @@ public: const std::string &addrNameIn, ConnectionType conn_type_in, bool inbound_onion, - std::unique_ptr&& i2p_sam_session = nullptr); + CNodeOptions&& node_opts = {}); CNode(const CNode&) = delete; CNode& operator=(const CNode&) = delete; @@ -1275,12 +1282,12 @@ private: * Create a `CNode` object from a socket that has just been accepted and add the node to * the `m_nodes` member. * @param[in] sock Connected socket to communicate with the peer. - * @param[in] permissionFlags The peer's permissions. + * @param[in] permission_flags The peer's permissions. * @param[in] addr_bind The address and port at our side of the connection. * @param[in] addr The address and port at the peer's side of the connection. */ void CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, - NetPermissionFlags permissionFlags, + NetPermissionFlags permission_flags, const CAddress& addr_bind, const CAddress& addr, CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 5521f75d57..969fd9af22 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1234,11 +1234,11 @@ void RPCConsole::updateDetailWidget() ui->peerSubversion->setText(QString::fromStdString(stats->nodeStats.cleanSubVer)); ui->peerConnectionType->setText(GUIUtil::ConnectionTypeToQString(stats->nodeStats.m_conn_type, /* prepend_direction */ true)); ui->peerNetwork->setText(GUIUtil::NetworkToQString(stats->nodeStats.m_network)); - if (stats->nodeStats.m_permissionFlags == NetPermissionFlags::None) { + if (stats->nodeStats.m_permission_flags == NetPermissionFlags::None) { ui->peerPermissions->setText(ts.na); } else { QStringList permissions; - for (const auto& permission : NetPermissions::ToStrings(stats->nodeStats.m_permissionFlags)) { + for (const auto& permission : NetPermissions::ToStrings(stats->nodeStats.m_permission_flags)) { permissions.append(QString::fromStdString(permission)); } ui->peerPermissions->setText(permissions.join(" & ")); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 77ed43354f..b4c962fd87 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -260,7 +260,7 @@ static RPCHelpMan getpeerinfo() obj.pushKV("whitelisted", stats.m_legacyWhitelisted); } UniValue permissions(UniValue::VARR); - for (const auto& permission : NetPermissions::ToStrings(stats.m_permissionFlags)) { + for (const auto& permission : NetPermissions::ToStrings(stats.m_permission_flags)) { permissions.push_back(permission); } obj.pushKV("permissions", permissions); diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index d34f15b78e..30046a3052 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -71,7 +71,6 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) /*successfully_connected=*/true, /*remote_services=*/ServiceFlags(NODE_NETWORK), /*local_services=*/ServiceFlags(NODE_NETWORK), - /*permission_flags=*/NetPermissionFlags::None, /*version=*/PROTOCOL_VERSION, /*relay_txs=*/true); TestOnlyResetTimeData(); diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index 7141037f95..11e9adf2dd 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -288,7 +288,6 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, /*successfully_connected=*/fuzzed_data_provider.ConsumeBool(), /*remote_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS), /*local_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS), - /*permission_flags=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS), /*version=*/fuzzed_data_provider.ConsumeIntegralInRange(MIN_PEER_PROTO_VERSION, std::numeric_limits::max()), /*relay_txs=*/fuzzed_data_provider.ConsumeBool()); } diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 38704f6d2e..b248f2fc1b 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -366,6 +366,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional(node_id, sock, @@ -375,7 +376,8 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional& node_id_in = std::nullopt) { return ConsumeNode(fdp, node_id_in); } diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 79dbc8ab9a..62c8d652e2 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -17,7 +17,6 @@ void ConnmanTestMsg::Handshake(CNode& node, bool successfully_connected, ServiceFlags remote_services, ServiceFlags local_services, - NetPermissionFlags permission_flags, int32_t version, bool relay_txs) { @@ -55,7 +54,6 @@ void ConnmanTestMsg::Handshake(CNode& node, assert(peerman.GetNodeStateStats(node.GetId(), statestats)); assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn())); assert(statestats.their_services == remote_services); - node.m_permissionFlags = permission_flags; if (successfully_connected) { CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)}; (void)connman.ReceiveMsgFrom(node, std::move(msg_verack)); diff --git a/src/test/util/net.h b/src/test/util/net.h index 7f203c6d55..893ade2430 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -43,7 +43,6 @@ struct ConnmanTestMsg : public CConnman { bool successfully_connected, ServiceFlags remote_services, ServiceFlags local_services, - NetPermissionFlags permission_flags, int32_t version, bool relay_txs) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex); From d3f5b3881b80b3d590bb20ec60199b89dde3f7dd Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 13 Jan 2023 14:44:23 +0100 Subject: [PATCH 06/12] merge bitcoin#26888: simplify the call to vProcessMsg.splice() --- src/net.cpp | 7 +++---- src/test/util/net.cpp | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 30777a3f59..77f95a2b87 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2097,15 +2097,14 @@ size_t CConnman::SocketRecvData(CNode *pnode) RecordBytesRecv(nBytes); if (notify) { size_t nSizeAdded = 0; - auto it(pnode->vRecvMsg.begin()); - for (; it != pnode->vRecvMsg.end(); ++it) { + for (const auto& msg : pnode->vRecvMsg) { // vRecvMsg contains only completed CNetMessage // the single possible partially deserialized message are held by TransportDeserializer - nSizeAdded += it->m_raw_message_size; + nSizeAdded += msg.m_raw_message_size; } { LOCK(pnode->cs_vProcessMsg); - pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), it); + pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg); pnode->nProcessQueueSize += nSizeAdded; pnode->fPauseRecv = pnode->nProcessQueueSize > nReceiveFloodSize; } diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 62c8d652e2..e89e6763df 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -69,15 +69,14 @@ void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_by assert(node.ReceiveMsgBytes(msg_bytes, complete)); if (complete) { size_t nSizeAdded = 0; - auto it(node.vRecvMsg.begin()); - for (; it != node.vRecvMsg.end(); ++it) { + for (const auto& msg : node.vRecvMsg) { // vRecvMsg contains only completed CNetMessage // the single possible partially deserialized message are held by TransportDeserializer - nSizeAdded += it->m_raw_message_size; + nSizeAdded += msg.m_raw_message_size; } { LOCK(node.cs_vProcessMsg); - node.vProcessMsg.splice(node.vProcessMsg.end(), node.vRecvMsg, node.vRecvMsg.begin(), it); + node.vProcessMsg.splice(node.vProcessMsg.end(), node.vRecvMsg); node.nProcessQueueSize += nSizeAdded; node.fPauseRecv = node.nProcessQueueSize > nReceiveFloodSize; } From 3465df2689ea7b9fbdb67fc41afc90f705d20dde Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 14 Sep 2021 14:54:40 +0300 Subject: [PATCH 07/12] merge bitcoin#27264: Improve diversification of new connections --- src/net.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 77f95a2b87..b2fb0a7ff1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2482,19 +2482,20 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe if (pnode->IsBlockOnlyConn()) nOutboundBlockRelay++; if (pnode->IsFullOutboundConn() && pnode->ConnectedThroughNetwork() == Network::NET_ONION) nOutboundOnionRelay++; - // Netgroups for inbound and manual peers are not excluded because our goal here - // is to not use multiple of our limited outbound slots on a single netgroup - // but inbound and manual peers do not use our outbound slots. Inbound peers - // also have the added issue that they could be attacker controlled and used - // to prevent us from connecting to particular hosts if we used them here. + // Make sure our persistent outbound slots belong to different netgroups. switch (pnode->m_conn_type) { + // We currently don't take inbound connections into account. Since they are + // free to make, an attacker could make them to prevent us from connecting to + // certain peers. case ConnectionType::INBOUND: - case ConnectionType::MANUAL: - break; - case ConnectionType::OUTBOUND_FULL_RELAY: - case ConnectionType::BLOCK_RELAY: + // Short-lived outbound connections should not affect how we select outbound + // peers from addrman. case ConnectionType::ADDR_FETCH: case ConnectionType::FEELER: + break; + case ConnectionType::MANUAL: + case ConnectionType::OUTBOUND_FULL_RELAY: + case ConnectionType::BLOCK_RELAY: setConnected.insert(m_netgroupman.GetGroup(pnode->addr)); } // no default case, so the compiler can warn about missing cases } From 9023dd25af698ee163bc5438f84588f09221f61c Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 31 Aug 2024 08:18:12 +0000 Subject: [PATCH 08/12] merge bitcoin#27257: End friendship of CNode, CConnman and ConnmanTestMsg --- src/net.cpp | 64 +++++++++++++++++++++++++----------------- src/net.h | 54 ++++++++++++++++++++++++++++++----- src/net_processing.cpp | 19 +++++-------- src/test/util/net.cpp | 13 +-------- 4 files changed, 93 insertions(+), 57 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index b2fb0a7ff1..9c9e8a8d38 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1034,7 +1034,7 @@ std::pair CConnman::SocketSendData(CNode& node) const // Notify transport that bytes have been processed. node.m_transport->MarkBytesSent(nBytes); // Update statistics per message type. - node.mapSendBytesPerMsgType[msg_type] += nBytes; + node.AccountForSentBytes(msg_type, nBytes); nSentSize += nBytes; if ((size_t)nBytes != data.size()) { // could not send full message; stop sending more @@ -1115,7 +1115,7 @@ bool CConnman::AttemptToEvictConnection() .m_is_local = node->addr.IsLocal(), .m_network = node->ConnectedThroughNetwork(), .m_noban = node->HasPermission(NetPermissionFlags::NoBan), - .m_conn_type = node->m_conn_type, + .m_conn_type = node->GetConnectionType(), }; vEvictionCandidates.push_back(candidate); } @@ -1339,7 +1339,7 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ // Count existing connections int existing_connections = WITH_LOCK(m_nodes_mutex, - return std::count_if(m_nodes.begin(), m_nodes.end(), [conn_type](CNode* node) { return node->m_conn_type == conn_type; });); + return std::count_if(m_nodes.begin(), m_nodes.end(), [conn_type](CNode* node) { return node->GetConnectionType() == conn_type; });); // Max connections of specified type already exist if (max_connections != std::nullopt && existing_connections >= max_connections) return false; @@ -1494,16 +1494,8 @@ void CConnman::CalculateNumConnectionsChangedStats() mapSentBytesMsgStats[NET_MESSAGE_TYPE_OTHER] = 0; const NodesSnapshot snap{*this, /* filter = */ CConnman::FullyConnectedOnly}; for (auto pnode : snap.Nodes()) { - { - LOCK(pnode->cs_vRecv); - for (const mapMsgTypeSize::value_type &i : pnode->mapRecvBytesPerMsgType) - mapRecvBytesMsgStats[i.first] += i.second; - } - { - LOCK(pnode->cs_vSend); - for (const mapMsgTypeSize::value_type &i : pnode->mapSendBytesPerMsgType) - mapSentBytesMsgStats[i.first] += i.second; - } + WITH_LOCK(pnode->cs_vRecv, pnode->UpdateRecvMapWithStats(mapRecvBytesMsgStats)); + WITH_LOCK(pnode->cs_vSend, pnode->UpdateSentMapWithStats(mapSentBytesMsgStats)); if (pnode->m_bloom_filter_loaded.load()) { spvNodes++; } else { @@ -2096,18 +2088,7 @@ size_t CConnman::SocketRecvData(CNode *pnode) } RecordBytesRecv(nBytes); if (notify) { - size_t nSizeAdded = 0; - for (const auto& msg : pnode->vRecvMsg) { - // vRecvMsg contains only completed CNetMessage - // the single possible partially deserialized message are held by TransportDeserializer - nSizeAdded += msg.m_raw_message_size; - } - { - LOCK(pnode->cs_vProcessMsg); - pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg); - pnode->nProcessQueueSize += nSizeAdded; - pnode->fPauseRecv = pnode->nProcessQueueSize > nReceiveFloodSize; - } + pnode->MarkReceivedMsgsForProcessing(nReceiveFloodSize); WakeMessageHandler(); } } @@ -2483,7 +2464,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe if (pnode->IsFullOutboundConn() && pnode->ConnectedThroughNetwork() == Network::NET_ONION) nOutboundOnionRelay++; // Make sure our persistent outbound slots belong to different netgroups. - switch (pnode->m_conn_type) { + switch (pnode->GetConnectionType()) { // We currently don't take inbound connections into account. Since they are // free to make, an attacker could make them to prevent us from connecting to // certain peers. @@ -4074,6 +4055,37 @@ CNode::CNode(NodeId idIn, } } +void CNode::MarkReceivedMsgsForProcessing(unsigned int recv_flood_size) +{ + AssertLockNotHeld(m_msg_process_queue_mutex); + + size_t nSizeAdded = 0; + for (const auto& msg : vRecvMsg) { + // vRecvMsg contains only completed CNetMessage + // the single possible partially deserialized message are held by TransportDeserializer + nSizeAdded += msg.m_raw_message_size; + } + + LOCK(m_msg_process_queue_mutex); + m_msg_process_queue.splice(m_msg_process_queue.end(), vRecvMsg); + m_msg_process_queue_size += nSizeAdded; + fPauseRecv = m_msg_process_queue_size > recv_flood_size; +} + +std::optional> CNode::PollMessage(size_t recv_flood_size) +{ + LOCK(m_msg_process_queue_mutex); + if (m_msg_process_queue.empty()) return std::nullopt; + + std::list msgs; + // Just take one message + msgs.splice(msgs.begin(), m_msg_process_queue, m_msg_process_queue.begin()); + m_msg_process_queue_size -= msgs.front().m_raw_message_size; + fPauseRecv = m_msg_process_queue_size > recv_flood_size; + + return std::make_pair(std::move(msgs.front()), !m_msg_process_queue.empty()); +} + bool CConnman::NodeFullyConnected(const CNode* pnode) { return pnode && pnode->fSuccessfullyConnected && !pnode->fDisconnect; diff --git a/src/net.h b/src/net.h index 9bb3cddfd9..e48aef8702 100644 --- a/src/net.h +++ b/src/net.h @@ -459,9 +459,6 @@ struct CNodeOptions /** Information about a peer */ class CNode { - friend class CConnman; - friend struct ConnmanTestMsg; - public: /** Transport serializer/deserializer. The receive side functions are only called under cs_vRecv, while * the sending side functions are only called under cs_vSend. */ @@ -490,10 +487,6 @@ public: Mutex m_sock_mutex; Mutex cs_vRecv; - RecursiveMutex cs_vProcessMsg; - std::list vProcessMsg GUARDED_BY(cs_vProcessMsg); - size_t nProcessQueueSize GUARDED_BY(cs_vProcessMsg){0}; - uint64_t nRecvBytes GUARDED_BY(cs_vRecv){0}; std::atomic m_last_send{0s}; @@ -553,6 +546,48 @@ public: std::atomic_bool fHasRecvData{false}; std::atomic_bool fCanSendData{false}; + const ConnectionType& GetConnectionType() const + { + return m_conn_type; + } + + /** Move all messages from the received queue to the processing queue. */ + void MarkReceivedMsgsForProcessing(unsigned int recv_flood_size) + EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex); + + /** Poll the next message from the processing queue of this connection. + * + * Returns std::nullopt if the processing queue is empty, or a pair + * consisting of the message and a bool that indicates if the processing + * queue has more entries. */ + std::optional> PollMessage(size_t recv_flood_size) + EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex); + + /** Account for the total size of a sent message in the per msg type connection stats. */ + void AccountForSentBytes(const std::string& msg_type, size_t sent_bytes) + EXCLUSIVE_LOCKS_REQUIRED(cs_vSend) + { + mapSendBytesPerMsgType[msg_type] += sent_bytes; + } + + /** Update a supplied map with bytes sent for each msg type for this node */ + void UpdateSentMapWithStats(mapMsgTypeSize& map_sentbytes_msg) + EXCLUSIVE_LOCKS_REQUIRED(cs_vSend) + { + for (auto& [msg_type, bytes] : mapSendBytesPerMsgType) { + map_sentbytes_msg[msg_type] += bytes; + } + } + + /** Update a supplied map with bytes recv for each msg type for this node */ + void UpdateRecvMapWithStats(mapMsgTypeSize& map_recvbytes_msg) + EXCLUSIVE_LOCKS_REQUIRED(cs_vRecv) + { + for (auto& [msg_type, bytes] : mapRecvBytesPerMsgType) { + map_recvbytes_msg[msg_type] += bytes; + } + } + /** * Get network the peer connected through. * @@ -564,6 +599,7 @@ public: * @return network the peer connected through. */ Network ConnectedThroughNetwork() const; + bool IsOutboundOrBlockRelayConn() const { switch (m_conn_type) { case ConnectionType::OUTBOUND_FULL_RELAY: @@ -794,6 +830,10 @@ private: std::list vRecvMsg; // Used only by SocketHandler thread + Mutex m_msg_process_queue_mutex; + std::list m_msg_process_queue GUARDED_BY(m_msg_process_queue_mutex); + size_t m_msg_process_queue_size GUARDED_BY(m_msg_process_queue_mutex){0}; + // Our address, as reported by the peer CService addrLocal GUARDED_BY(m_addr_local_mutex); mutable Mutex m_addr_local_mutex; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1735df8669..3213904ac1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5013,8 +5013,6 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt { AssertLockHeld(g_msgproc_mutex); - bool fMoreWork = false; - PeerRef peer = GetPeerRef(pfrom->GetId()); if (peer == nullptr) return false; @@ -5050,17 +5048,14 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt // Don't bother if send buffer is too full to respond anyway if (pfrom->fPauseSend) return false; - std::list msgs; - { - LOCK(pfrom->cs_vProcessMsg); - if (pfrom->vProcessMsg.empty()) return false; - // Just take one message - msgs.splice(msgs.begin(), pfrom->vProcessMsg, pfrom->vProcessMsg.begin()); - pfrom->nProcessQueueSize -= msgs.front().m_raw_message_size; - pfrom->fPauseRecv = pfrom->nProcessQueueSize > m_connman.GetReceiveFloodSize(); - fMoreWork = !pfrom->vProcessMsg.empty(); + auto poll_result{pfrom->PollMessage(m_connman.GetReceiveFloodSize())}; + if (!poll_result) { + // No message to process + return false; } - CNetMessage& msg(msgs.front()); + + CNetMessage& msg{poll_result->first}; + bool fMoreWork = poll_result->second; TRACE6(net, inbound_message, pfrom->GetId(), diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index e89e6763df..1b8680750b 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -68,18 +68,7 @@ void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_by { assert(node.ReceiveMsgBytes(msg_bytes, complete)); if (complete) { - size_t nSizeAdded = 0; - for (const auto& msg : node.vRecvMsg) { - // vRecvMsg contains only completed CNetMessage - // the single possible partially deserialized message are held by TransportDeserializer - nSizeAdded += msg.m_raw_message_size; - } - { - LOCK(node.cs_vProcessMsg); - node.vProcessMsg.splice(node.vProcessMsg.end(), node.vRecvMsg); - node.nProcessQueueSize += nSizeAdded; - node.fPauseRecv = node.nProcessQueueSize > nReceiveFloodSize; - } + node.MarkReceivedMsgsForProcessing(nReceiveFloodSize); } } From ab11e0f998cf41f62e4f3601eb21f134c6f4994a Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 6 Sep 2024 08:27:39 +0000 Subject: [PATCH 09/12] merge bitcoin#27324: bitcoin#27257 follow-ups --- src/net.cpp | 31 +++++++++++++++++-------------- src/net.h | 24 ++++++++++++++---------- src/net_processing.cpp | 2 +- src/test/fuzz/connman.cpp | 1 - src/test/util/net.cpp | 2 +- 5 files changed, 33 insertions(+), 27 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 9c9e8a8d38..f09ed62686 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -617,7 +617,10 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo pszDest ? pszDest : "", conn_type, /*inbound_onion=*/false, - CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session) }); + CNodeOptions{ + .i2p_sam_session = std::move(i2p_transient_session), + .recv_flood_size = nReceiveFloodSize, + }); pnode->AddRef(); ::g_stats_client->inc("peers.connect", 1.0f); @@ -1115,7 +1118,7 @@ bool CConnman::AttemptToEvictConnection() .m_is_local = node->addr.IsLocal(), .m_network = node->ConnectedThroughNetwork(), .m_noban = node->HasPermission(NetPermissionFlags::NoBan), - .m_conn_type = node->GetConnectionType(), + .m_conn_type = node->m_conn_type, }; vEvictionCandidates.push_back(candidate); } @@ -1279,8 +1282,9 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, ConnectionType::INBOUND, inbound_onion, CNodeOptions{ - .permission_flags = permission_flags, - .prefer_evict = discouraged, + .permission_flags = permission_flags, + .prefer_evict = discouraged, + .recv_flood_size = nReceiveFloodSize, }); pnode->AddRef(); // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility) @@ -1339,7 +1343,7 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ // Count existing connections int existing_connections = WITH_LOCK(m_nodes_mutex, - return std::count_if(m_nodes.begin(), m_nodes.end(), [conn_type](CNode* node) { return node->GetConnectionType() == conn_type; });); + return std::count_if(m_nodes.begin(), m_nodes.end(), [conn_type](CNode* node) { return node->m_conn_type == conn_type; });); // Max connections of specified type already exist if (max_connections != std::nullopt && existing_connections >= max_connections) return false; @@ -2088,7 +2092,7 @@ size_t CConnman::SocketRecvData(CNode *pnode) } RecordBytesRecv(nBytes); if (notify) { - pnode->MarkReceivedMsgsForProcessing(nReceiveFloodSize); + pnode->MarkReceivedMsgsForProcessing(); WakeMessageHandler(); } } @@ -2464,7 +2468,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe if (pnode->IsFullOutboundConn() && pnode->ConnectedThroughNetwork() == Network::NET_ONION) nOutboundOnionRelay++; // Make sure our persistent outbound slots belong to different netgroups. - switch (pnode->GetConnectionType()) { + switch (pnode->m_conn_type) { // We currently don't take inbound connections into account. Since they are // free to make, an attacker could make them to prevent us from connecting to // certain peers. @@ -4015,8 +4019,6 @@ ServiceFlags CConnman::GetLocalServices() const return nLocalServices; } -unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } - CNode::CNode(NodeId idIn, std::shared_ptr sock, const CAddress& addrIn, @@ -4037,9 +4039,10 @@ CNode::CNode(NodeId idIn, m_inbound_onion{inbound_onion}, m_prefer_evict{node_opts.prefer_evict}, nKeyedNetGroup{nKeyedNetGroupIn}, + m_conn_type{conn_type_in}, id{idIn}, nLocalHostNonce{nLocalHostNonceIn}, - m_conn_type{conn_type_in}, + m_recv_flood_size{node_opts.recv_flood_size}, m_i2p_sam_session{std::move(node_opts.i2p_sam_session)} { if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND); @@ -4055,7 +4058,7 @@ CNode::CNode(NodeId idIn, } } -void CNode::MarkReceivedMsgsForProcessing(unsigned int recv_flood_size) +void CNode::MarkReceivedMsgsForProcessing() { AssertLockNotHeld(m_msg_process_queue_mutex); @@ -4069,10 +4072,10 @@ void CNode::MarkReceivedMsgsForProcessing(unsigned int recv_flood_size) LOCK(m_msg_process_queue_mutex); m_msg_process_queue.splice(m_msg_process_queue.end(), vRecvMsg); m_msg_process_queue_size += nSizeAdded; - fPauseRecv = m_msg_process_queue_size > recv_flood_size; + fPauseRecv = m_msg_process_queue_size > m_recv_flood_size; } -std::optional> CNode::PollMessage(size_t recv_flood_size) +std::optional> CNode::PollMessage() { LOCK(m_msg_process_queue_mutex); if (m_msg_process_queue.empty()) return std::nullopt; @@ -4081,7 +4084,7 @@ std::optional> CNode::PollMessage(size_t recv_flood // Just take one message msgs.splice(msgs.begin(), m_msg_process_queue, m_msg_process_queue.begin()); m_msg_process_queue_size -= msgs.front().m_raw_message_size; - fPauseRecv = m_msg_process_queue_size > recv_flood_size; + fPauseRecv = m_msg_process_queue_size > m_recv_flood_size; return std::make_pair(std::move(msgs.front()), !m_msg_process_queue.empty()); } diff --git a/src/net.h b/src/net.h index e48aef8702..0c8c545946 100644 --- a/src/net.h +++ b/src/net.h @@ -277,6 +277,14 @@ public: std::string m_type; CNetMessage(CDataStream&& recv_in) : m_recv(std::move(recv_in)) {} + // Only one CNetMessage object will exist for the same message on either + // the receive or processing queue. For performance reasons we therefore + // delete the copy constructor and assignment operator to avoid the + // possibility of copying CNetMessage objects. + CNetMessage(CNetMessage&&) = default; + CNetMessage(const CNetMessage&) = delete; + CNetMessage& operator=(CNetMessage&&) = default; + CNetMessage& operator=(const CNetMessage&) = delete; void SetVersion(int nVersionIn) { @@ -454,6 +462,7 @@ struct CNodeOptions NetPermissionFlags permission_flags = NetPermissionFlags::None; std::unique_ptr i2p_sam_session = nullptr; bool prefer_evict = false; + size_t recv_flood_size{DEFAULT_MAXRECEIVEBUFFER * 1000}; }; /** Information about a peer */ @@ -546,13 +555,10 @@ public: std::atomic_bool fHasRecvData{false}; std::atomic_bool fCanSendData{false}; - const ConnectionType& GetConnectionType() const - { - return m_conn_type; - } + const ConnectionType m_conn_type; /** Move all messages from the received queue to the processing queue. */ - void MarkReceivedMsgsForProcessing(unsigned int recv_flood_size) + void MarkReceivedMsgsForProcessing() EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex); /** Poll the next message from the processing queue of this connection. @@ -560,7 +566,7 @@ public: * Returns std::nullopt if the processing queue is empty, or a pair * consisting of the message and a bool that indicates if the processing * queue has more entries. */ - std::optional> PollMessage(size_t recv_flood_size) + std::optional> PollMessage() EXCLUSIVE_LOCKS_REQUIRED(!m_msg_process_queue_mutex); /** Account for the total size of a sent message in the per msg type connection stats. */ @@ -825,10 +831,10 @@ public: private: const NodeId id; const uint64_t nLocalHostNonce; - const ConnectionType m_conn_type; std::atomic m_greatest_common_version{INIT_PROTO_VERSION}; - std::list vRecvMsg; // Used only by SocketHandler thread + const size_t m_recv_flood_size; + std::list vRecvMsg; // Used only by SocketHandler thread Mutex m_msg_process_queue_mutex; std::list m_msg_process_queue GUARDED_BY(m_msg_process_queue_mutex); @@ -1257,8 +1263,6 @@ public: /** Get a unique deterministic randomizer. */ CSipHasher GetDeterministicRandomizer(uint64_t id) const; - unsigned int GetReceiveFloodSize() const; - void WakeMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); /** Return true if we should disconnect the peer for failing an inactivity check. */ diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3213904ac1..c69ec237c1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5048,7 +5048,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt // Don't bother if send buffer is too full to respond anyway if (pfrom->fPauseSend) return false; - auto poll_result{pfrom->PollMessage(m_connman.GetReceiveFloodSize())}; + auto poll_result{pfrom->PollMessage()}; if (!poll_result) { // No message to process return false; diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 95f1b6516e..ead124552b 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -126,7 +126,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman) std::vector stats; connman.GetNodeStats(stats); (void)connman.GetOutboundTargetBytesLeft(); - (void)connman.GetReceiveFloodSize(); (void)connman.GetTotalBytesRecv(); (void)connman.GetTotalBytesSent(); (void)connman.GetTryNewOutboundPeer(); diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 1b8680750b..e9f43e69cf 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -68,7 +68,7 @@ void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span msg_by { assert(node.ReceiveMsgBytes(msg_bytes, complete)); if (complete) { - node.MarkReceivedMsgsForProcessing(nReceiveFloodSize); + node.MarkReceivedMsgsForProcessing(); } } From a52b3a3bf0891bed8822453e45d310160cf6dd8f Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 13 Sep 2024 08:14:33 +0000 Subject: [PATCH 10/12] merge bitcoin#27374: skip netgroup diversity of new connections for tor/i2p/cjdns --- src/net.cpp | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index f09ed62686..d83d377ccc 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2454,12 +2454,14 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe // CAddress addrConnect; - // Only connect out to one peer per network group (/16 for IPv4). + // Only connect out to one peer per ipv4/ipv6 network group (/16 for IPv4). // This is only done for mainnet and testnet int nOutboundFullRelay = 0; int nOutboundBlockRelay = 0; int nOutboundOnionRelay = 0; - std::set > setConnected; + int outbound_privacy_network_peers = 0; + std::set> setConnected; // netgroups of our ipv4/ipv6 outbound peers + if (!Params().AllowMultipleAddressesFromGroup()) { LOCK(m_nodes_mutex); for (const CNode* pnode : m_nodes) { @@ -2467,7 +2469,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe if (pnode->IsBlockOnlyConn()) nOutboundBlockRelay++; if (pnode->IsFullOutboundConn() && pnode->ConnectedThroughNetwork() == Network::NET_ONION) nOutboundOnionRelay++; - // Make sure our persistent outbound slots belong to different netgroups. + // Make sure our persistent outbound slots to ipv4/ipv6 peers belong to different netgroups. switch (pnode->m_conn_type) { // We currently don't take inbound connections into account. Since they are // free to make, an attacker could make them to prevent us from connecting to @@ -2481,7 +2483,19 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe case ConnectionType::MANUAL: case ConnectionType::OUTBOUND_FULL_RELAY: case ConnectionType::BLOCK_RELAY: - setConnected.insert(m_netgroupman.GetGroup(pnode->addr)); + CAddress address{pnode->addr}; + if (address.IsTor() || address.IsI2P() || address.IsCJDNS()) { + // Since our addrman-groups for these networks are + // random, without relation to the route we + // take to connect to these peers or to the + // difficulty in obtaining addresses with diverse + // groups, we don't worry about diversity with + // respect to our addrman groups when connecting to + // these networks. + ++outbound_privacy_network_peers; + } else { + setConnected.insert(m_netgroupman.GetGroup(address)); + } } // no default case, so the compiler can warn about missing cases } } @@ -2675,8 +2689,11 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe LogPrint(BCLog::NET, "Making feeler connection\n"); } } - - OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type); + // Record addrman failure attempts when node has at least 2 persistent outbound connections to peers with + // different netgroups in ipv4/ipv6 networks + all peers in Tor/I2P/CJDNS networks. + // Don't record addrman failure attempts when node is offline. This can be identified since all local + // network connections(if any) belong in the same netgroup and size of setConnected would only be 1. + OpenNetworkConnection(addrConnect, (int)setConnected.size() + outbound_privacy_network_peers >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type); } } } From 1376289b11c624a2b648ae1b117d398303e10bfa Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 6 Sep 2024 08:28:53 +0000 Subject: [PATCH 11/12] merge bitcoin#27467: skip netgroup diversity follow-up --- src/net.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index d83d377ccc..bb71c150b0 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2460,7 +2460,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe int nOutboundBlockRelay = 0; int nOutboundOnionRelay = 0; int outbound_privacy_network_peers = 0; - std::set> setConnected; // netgroups of our ipv4/ipv6 outbound peers + std::set> outbound_ipv46_peer_netgroups; if (!Params().AllowMultipleAddressesFromGroup()) { LOCK(m_nodes_mutex); @@ -2483,7 +2483,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe case ConnectionType::MANUAL: case ConnectionType::OUTBOUND_FULL_RELAY: case ConnectionType::BLOCK_RELAY: - CAddress address{pnode->addr}; + const CAddress address{pnode->addr}; if (address.IsTor() || address.IsI2P() || address.IsCJDNS()) { // Since our addrman-groups for these networks are // random, without relation to the route we @@ -2494,7 +2494,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe // these networks. ++outbound_privacy_network_peers; } else { - setConnected.insert(m_netgroupman.GetGroup(address)); + outbound_ipv46_peer_netgroups.insert(m_netgroupman.GetGroup(address)); } } // no default case, so the compiler can warn about missing cases } @@ -2585,7 +2585,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe m_anchors.pop_back(); if (!addr.IsValid() || IsLocal(addr) || !IsReachable(addr) || !HasAllDesirableServiceFlags(addr.nServices) || - setConnected.count(m_netgroupman.GetGroup(addr))) continue; + outbound_ipv46_peer_netgroups.count(m_netgroupman.GetGroup(addr))) continue; addrConnect = addr; LogPrint(BCLog::NET, "Trying to make an anchor connection to %s\n", addrConnect.ToStringAddrPort()); break; @@ -2628,13 +2628,13 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe auto dmn = mnList.GetMNByService(addr); bool isMasternode = dmn != nullptr; - // Require outbound connections, other than feelers, to be to distinct network groups - if (!fFeeler && setConnected.count(m_netgroupman.GetGroup(addr))) { + // Require outbound IPv4/IPv6 connections, other than feelers, to be to distinct network groups + if (!fFeeler && outbound_ipv46_peer_netgroups.count(m_netgroupman.GetGroup(addr))) { break; } // if we selected an invalid address, restart - if (!addr.IsValid() || setConnected.count(m_netgroupman.GetGroup(addr))) + if (!addr.IsValid() || outbound_ipv46_peer_netgroups.count(m_netgroupman.GetGroup(addr))) break; // don't try to connect to masternodes that we already have a connection to (most likely inbound) @@ -2692,8 +2692,9 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe // Record addrman failure attempts when node has at least 2 persistent outbound connections to peers with // different netgroups in ipv4/ipv6 networks + all peers in Tor/I2P/CJDNS networks. // Don't record addrman failure attempts when node is offline. This can be identified since all local - // network connections(if any) belong in the same netgroup and size of setConnected would only be 1. - OpenNetworkConnection(addrConnect, (int)setConnected.size() + outbound_privacy_network_peers >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type); + // network connections (if any) belong in the same netgroup, and the size of `outbound_ipv46_peer_netgroups` would only be 1. + const bool count_failures{((int)outbound_ipv46_peer_netgroups.size() + outbound_privacy_network_peers) >= std::min(nMaxConnections - 1, 2)}; + OpenNetworkConnection(addrConnect, count_failures, &grant, /*strDest=*/nullptr, conn_type); } } } From 8320e0ca8ea3adda8dd672f716bf6214d034e5de Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 13 Sep 2024 08:46:28 +0000 Subject: [PATCH 12/12] merge bitcoin#27411: Restrict self-advertisements with privacy networks to avoid fingerprinting The old `GetLocal()` has been moved to `masternode/node.cpp` due to its use in determining a node's external address. We don't want the old variant to be used otherwise so we'll move it out of `net.{cpp,h}` for good measure. Co-authored-by: UdjinM6 --- src/masternode/node.cpp | 39 ++++++++++++- src/net.cpp | 20 +++++-- src/net.h | 4 +- src/net_processing.cpp | 2 +- src/netaddress.cpp | 16 ++---- src/netaddress.h | 2 +- src/test/fuzz/netaddress.cpp | 2 +- src/test/net_tests.cpp | 105 +++++++++++++++++++++++++++++++++++ 8 files changed, 168 insertions(+), 22 deletions(-) diff --git a/src/masternode/node.cpp b/src/masternode/node.cpp index 91b9f838ac..61d42c4d31 100644 --- a/src/masternode/node.cpp +++ b/src/masternode/node.cpp @@ -15,6 +15,42 @@ #include #include +namespace { +bool GetLocal(CService& addr, const CNetAddr* paddrPeer) +{ + if (!fListen) + return false; + + int nBestScore = -1; + { + LOCK(g_maplocalhost_mutex); + int nBestReachability = -1; + for (const auto& entry : mapLocalHost) + { + // For privacy reasons, don't advertise our privacy-network address + // to other networks and don't advertise our other-network address + // to privacy networks. + const Network our_net{entry.first.GetNetwork()}; + const Network peers_net{paddrPeer->GetNetwork()}; + if (our_net != peers_net && + (our_net == NET_ONION || our_net == NET_I2P || + peers_net == NET_ONION || peers_net == NET_I2P)) { + continue; + } + int nScore = entry.second.nScore; + int nReachability = entry.first.GetReachabilityFrom(*paddrPeer); + if (nReachability > nBestReachability || (nReachability == nBestReachability && nScore > nBestScore)) + { + addr = CService(entry.first, entry.second.nPort); + nBestReachability = nReachability; + nBestScore = nScore; + } + } + } + return nBestScore >= 0; +} +} // anonymous namespace + CActiveMasternodeManager::CActiveMasternodeManager(const CBLSSecretKey& sk, CConnman& connman, const std::unique_ptr& dmnman) : m_info(sk, sk.GetPublicKey()), m_connman{connman}, @@ -204,8 +240,7 @@ bool CActiveMasternodeManager::GetLocalAddress(CService& addrRet) auto service = m_info.service; m_connman.ForEachNodeContinueIf(CConnman::AllNodes, [&](CNode* pnode) { empty = false; - if (pnode->addr.IsIPv4()) - fFoundLocal = GetLocal(service, &pnode->addr) && IsValidNetAddr(service); + if (pnode->addr.IsIPv4()) fFoundLocal = GetLocal(service, *pnode) && IsValidNetAddr(service); return !fFoundLocal; }); // nothing and no live connections, can't do anything for now diff --git a/src/net.cpp b/src/net.cpp index bb71c150b0..5ee73ee10e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -186,7 +186,7 @@ uint16_t GetListenPort() } // find 'best' local address for a particular peer -bool GetLocal(CService& addr, const CNetAddr *paddrPeer) +bool GetLocal(CService& addr, const CNode& peer) { if (!fListen) return false; @@ -197,8 +197,18 @@ bool GetLocal(CService& addr, const CNetAddr *paddrPeer) LOCK(g_maplocalhost_mutex); for (const auto& entry : mapLocalHost) { + // For privacy reasons, don't advertise our privacy-network address + // to other networks and don't advertise our other-network address + // to privacy networks. + const Network our_net{entry.first.GetNetwork()}; + const Network peers_net{peer.ConnectedThroughNetwork()}; + if (our_net != peers_net && + (our_net == NET_ONION || our_net == NET_I2P || + peers_net == NET_ONION || peers_net == NET_I2P)) { + continue; + } int nScore = entry.second.nScore; - int nReachability = entry.first.GetReachabilityFrom(paddrPeer); + int nReachability = entry.first.GetReachabilityFrom(peer.addr); if (nReachability > nBestReachability || (nReachability == nBestReachability && nScore > nBestScore)) { addr = CService(entry.first, entry.second.nPort); @@ -236,10 +246,10 @@ static std::vector ConvertSeeds(const std::vector &vSeedsIn) // Otherwise, return the unroutable 0.0.0.0 but filled in with // the normal parameters, since the IP may be changed to a useful // one by discovery. -CService GetLocalAddress(const CNetAddr& addrPeer) +CService GetLocalAddress(const CNode& peer) { CService addr; - if (GetLocal(addr, &addrPeer)) { + if (GetLocal(addr, peer)) { return addr; } return CService{CNetAddr(), GetListenPort()}; @@ -262,7 +272,7 @@ bool IsPeerAddrLocalGood(CNode *pnode) std::optional GetLocalAddrForPeer(CNode& node) { - CService addrLocal{GetLocalAddress(node.addr)}; + CService addrLocal{GetLocalAddress(node)}; // If discovery is enabled, sometimes give our peer the address it // tells us that it sees us as in case it has a better idea of our // address than we do. diff --git a/src/net.h b/src/net.h index 0c8c545946..732bf328e5 100644 --- a/src/net.h +++ b/src/net.h @@ -199,8 +199,8 @@ bool AddLocal(const CNetAddr& addr, int nScore = LOCAL_NONE); void RemoveLocal(const CService& addr); bool SeenLocal(const CService& addr); bool IsLocal(const CService& addr); -bool GetLocal(CService &addr, const CNetAddr *paddrPeer = nullptr); -CService GetLocalAddress(const CNetAddr& addrPeer); +bool GetLocal(CService& addr, const CNode& peer); +CService GetLocalAddress(const CNode& peer); extern bool fDiscover; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c69ec237c1..3f660baa25 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3460,7 +3460,7 @@ void PeerManagerImpl::ProcessMessage( // indicate to the peer that we will participate in addr relay. if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) { - CAddress addr{GetLocalAddress(pfrom.addr), peer->m_our_services, (uint32_t)GetAdjustedTime()}; + CAddress addr{GetLocalAddress(pfrom), peer->m_our_services, (uint32_t)GetAdjustedTime()}; FastRandomContext insecure_rand; if (addr.IsRoutable()) { diff --git a/src/netaddress.cpp b/src/netaddress.cpp index 87e0a71838..444b356b4d 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -742,19 +742,16 @@ uint64_t CNetAddr::GetHash() const // private extensions to enum Network, only returned by GetExtNetwork, // and only used in GetReachabilityFrom -static const int NET_UNKNOWN = NET_MAX + 0; -static const int NET_TEREDO = NET_MAX + 1; -int static GetExtNetwork(const CNetAddr *addr) +static const int NET_TEREDO = NET_MAX; +int static GetExtNetwork(const CNetAddr& addr) { - if (addr == nullptr) - return NET_UNKNOWN; - if (addr->IsRFC4380()) + if (addr.IsRFC4380()) return NET_TEREDO; - return addr->GetNetwork(); + return addr.GetNetwork(); } /** Calculates a metric for how reachable (*this) is from a given partner */ -int CNetAddr::GetReachabilityFrom(const CNetAddr *paddrPartner) const +int CNetAddr::GetReachabilityFrom(const CNetAddr& paddrPartner) const { enum Reachability { REACH_UNREACHABLE, @@ -769,7 +766,7 @@ int CNetAddr::GetReachabilityFrom(const CNetAddr *paddrPartner) const if (!IsRoutable() || IsInternal()) return REACH_UNREACHABLE; - int ourNet = GetExtNetwork(this); + int ourNet = GetExtNetwork(*this); int theirNet = GetExtNetwork(paddrPartner); bool fTunnel = IsRFC3964() || IsRFC6052() || IsRFC6145(); @@ -809,7 +806,6 @@ int CNetAddr::GetReachabilityFrom(const CNetAddr *paddrPartner) const case NET_IPV6: return REACH_IPV6_WEAK; case NET_IPV4: return REACH_IPV4; } - case NET_UNKNOWN: case NET_UNROUTABLE: default: switch(ourNet) { diff --git a/src/netaddress.h b/src/netaddress.h index 9781302e2f..5f540b480c 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -211,7 +211,7 @@ public: bool HasLinkedIPv4() const; std::vector GetAddrBytes() const; - int GetReachabilityFrom(const CNetAddr *paddrPartner = nullptr) const; + int GetReachabilityFrom(const CNetAddr& paddrPartner) const; explicit CNetAddr(const struct in6_addr& pipv6Addr, const uint32_t scope = 0); bool GetIn6Addr(struct in6_addr* pipv6Addr) const; diff --git a/src/test/fuzz/netaddress.cpp b/src/test/fuzz/netaddress.cpp index db16ed8ca8..7348507b15 100644 --- a/src/test/fuzz/netaddress.cpp +++ b/src/test/fuzz/netaddress.cpp @@ -83,7 +83,7 @@ FUZZ_TARGET(netaddress) (void)service.ToStringAddrPort(); const CNetAddr other_net_addr = ConsumeNetAddr(fuzzed_data_provider); - (void)net_addr.GetReachabilityFrom(&other_net_addr); + (void)net_addr.GetReachabilityFrom(other_net_addr); (void)sub_net.Match(other_net_addr); const CService other_service{net_addr, fuzzed_data_provider.ConsumeIntegral()}; diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index ae74fea933..d6235ce623 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -908,4 +908,109 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) TestOnlyResetTimeData(); } + +BOOST_AUTO_TEST_CASE(advertise_local_address) +{ + auto CreatePeer = [](const CAddress& addr) { + return std::make_unique(/*id=*/0, + /*sock=*/nullptr, + addr, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + CAddress{}, + /*pszDest=*/std::string{}, + ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false); + }; + SetReachable(NET_CJDNS, true); + + CAddress addr_ipv4{Lookup("1.2.3.4", 8333, false).value(), NODE_NONE}; + BOOST_REQUIRE(addr_ipv4.IsValid()); + BOOST_REQUIRE(addr_ipv4.IsIPv4()); + + CAddress addr_ipv6{Lookup("1122:3344:5566:7788:9900:aabb:ccdd:eeff", 8333, false).value(), NODE_NONE}; + BOOST_REQUIRE(addr_ipv6.IsValid()); + BOOST_REQUIRE(addr_ipv6.IsIPv6()); + + CAddress addr_ipv6_tunnel{Lookup("2002:3344:5566:7788:9900:aabb:ccdd:eeff", 8333, false).value(), NODE_NONE}; + BOOST_REQUIRE(addr_ipv6_tunnel.IsValid()); + BOOST_REQUIRE(addr_ipv6_tunnel.IsIPv6()); + BOOST_REQUIRE(addr_ipv6_tunnel.IsRFC3964()); + + CAddress addr_teredo{Lookup("2001:0000:5566:7788:9900:aabb:ccdd:eeff", 8333, false).value(), NODE_NONE}; + BOOST_REQUIRE(addr_teredo.IsValid()); + BOOST_REQUIRE(addr_teredo.IsIPv6()); + BOOST_REQUIRE(addr_teredo.IsRFC4380()); + + CAddress addr_onion; + BOOST_REQUIRE(addr_onion.SetSpecial("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion")); + BOOST_REQUIRE(addr_onion.IsValid()); + BOOST_REQUIRE(addr_onion.IsTor()); + + CAddress addr_i2p; + BOOST_REQUIRE(addr_i2p.SetSpecial("udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p")); + BOOST_REQUIRE(addr_i2p.IsValid()); + BOOST_REQUIRE(addr_i2p.IsI2P()); + + CService service_cjdns{Lookup("fc00:3344:5566:7788:9900:aabb:ccdd:eeff", 8333, false).value(), NODE_NONE}; + CAddress addr_cjdns{MaybeFlipIPv6toCJDNS(service_cjdns), NODE_NONE}; + BOOST_REQUIRE(addr_cjdns.IsValid()); + BOOST_REQUIRE(addr_cjdns.IsCJDNS()); + + const auto peer_ipv4{CreatePeer(addr_ipv4)}; + const auto peer_ipv6{CreatePeer(addr_ipv6)}; + const auto peer_ipv6_tunnel{CreatePeer(addr_ipv6_tunnel)}; + const auto peer_teredo{CreatePeer(addr_teredo)}; + const auto peer_onion{CreatePeer(addr_onion)}; + const auto peer_i2p{CreatePeer(addr_i2p)}; + const auto peer_cjdns{CreatePeer(addr_cjdns)}; + + // one local clearnet address - advertise to all but privacy peers + AddLocal(addr_ipv4); + BOOST_CHECK(GetLocalAddress(*peer_ipv4) == addr_ipv4); + BOOST_CHECK(GetLocalAddress(*peer_ipv6) == addr_ipv4); + BOOST_CHECK(GetLocalAddress(*peer_ipv6_tunnel) == addr_ipv4); + BOOST_CHECK(GetLocalAddress(*peer_teredo) == addr_ipv4); + BOOST_CHECK(GetLocalAddress(*peer_cjdns) == addr_ipv4); + BOOST_CHECK(!GetLocalAddress(*peer_onion).IsValid()); + BOOST_CHECK(!GetLocalAddress(*peer_i2p).IsValid()); + RemoveLocal(addr_ipv4); + + // local privacy addresses - don't advertise to clearnet peers + AddLocal(addr_onion); + AddLocal(addr_i2p); + BOOST_CHECK(!GetLocalAddress(*peer_ipv4).IsValid()); + BOOST_CHECK(!GetLocalAddress(*peer_ipv6).IsValid()); + BOOST_CHECK(!GetLocalAddress(*peer_ipv6_tunnel).IsValid()); + BOOST_CHECK(!GetLocalAddress(*peer_teredo).IsValid()); + BOOST_CHECK(!GetLocalAddress(*peer_cjdns).IsValid()); + BOOST_CHECK(GetLocalAddress(*peer_onion) == addr_onion); + BOOST_CHECK(GetLocalAddress(*peer_i2p) == addr_i2p); + RemoveLocal(addr_onion); + RemoveLocal(addr_i2p); + + // local addresses from all networks + AddLocal(addr_ipv4); + AddLocal(addr_ipv6); + AddLocal(addr_ipv6_tunnel); + AddLocal(addr_teredo); + AddLocal(addr_onion); + AddLocal(addr_i2p); + AddLocal(addr_cjdns); + BOOST_CHECK(GetLocalAddress(*peer_ipv4) == addr_ipv4); + BOOST_CHECK(GetLocalAddress(*peer_ipv6) == addr_ipv6); + BOOST_CHECK(GetLocalAddress(*peer_ipv6_tunnel) == addr_ipv6); + BOOST_CHECK(GetLocalAddress(*peer_teredo) == addr_ipv4); + BOOST_CHECK(GetLocalAddress(*peer_onion) == addr_onion); + BOOST_CHECK(GetLocalAddress(*peer_i2p) == addr_i2p); + BOOST_CHECK(GetLocalAddress(*peer_cjdns) == addr_cjdns); + RemoveLocal(addr_ipv4); + RemoveLocal(addr_ipv6); + RemoveLocal(addr_ipv6_tunnel); + RemoveLocal(addr_teredo); + RemoveLocal(addr_onion); + RemoveLocal(addr_i2p); + RemoveLocal(addr_cjdns); +} + BOOST_AUTO_TEST_SUITE_END()