Merge bitcoin/bitcoin#21261: p2p: update inbound eviction protection for multiple networks, add I2P peers

1b1088d test: add combined I2P/onion/localhost eviction protection tests (Jon Atack)
7c2284e test: add tests for inbound eviction protection of I2P peers (Jon Atack)
ce02dd1 p2p: extend inbound eviction protection by network to I2P peers (Jon Atack)
70bbc62 test: add combined onion/localhost eviction protection coverage (Jon Atack)
045cb40 p2p: remove unused m_is_onion member from NodeEvictionCandidate struct (Jon Atack)
310fab4 p2p: remove unused CompareLocalHostTimeConnected() (Jon Atack)
9e889e8 p2p: remove unused CompareOnionTimeConnected() (Jon Atack)
787d46b p2p: update ProtectEvictionCandidatesByRatio() doxygen docs (Jon Atack)
1e15acf p2p: make ProtectEvictionCandidatesByRatio() fully ratio-based (Jon Atack)
3f8105c test: remove combined onion/localhost eviction protection tests (Jon Atack)
38a81a8 p2p: add CompareNodeNetworkTime() comparator struct (Jon Atack)
4ee7aec p2p: add m_network to NodeEvictionCandidate struct (Jon Atack)
7321e6f p2p, refactor: rename vEvictionCandidates to eviction_candidates (Jon Atack)
ec590f1 p2p, refactor: improve constness in ProtectEvictionCandidatesByRatio() (Jon Atack)
4a19f50 test: add ALL_NETWORKS to test utilities (Jon Atack)
519e76b test: speed up and simplify peer_eviction_test (Jon Atack)
1cde800 p2p, refactor: rm redundant erase_size calculation in SelectNodeToEvict() (Jon Atack)

Pull request description:

  Continuing the work in #20197 and #20685, this pull updates and abstracts our inbound eviction protection to make it fully ratio-based and easily extensible to peers connected via high-latency privacy networks that we newly support, like I2P and perhaps others soon, as these peers are disadvantaged by the latency criteria of our eviction logic.

  It then adds eviction protection for peers connected over I2P.  As described in bitcoin/bitcoin#20685 (comment), we've observed over the past few months that I2P peers have a min ping latency similar to or greater than that of onion peers.

  The algorithm is a basically a multi-pass knapsack:

  - Count the number of eviction candidates in each of the disadvantaged
    privacy networks.

  - Sort the networks from lower to higher candidate counts, so that
    a network with fewer candidates will have the first opportunity
    for any unused slots remaining from the previous iteration.  In
    the case of a tie in candidate counts, priority is given by array
    member order from first to last, guesstimated to favor more unusual
    networks.

  - Iterate through the networks in this order.  On each iteration,
    allocate each network an equal number of protected slots targeting
    a total number of candidates to protect, provided any slots remain
    in the knapsack.

  - Protect the candidates in that network having the longest uptime,
    if any in that network are present.

  - Continue iterating as long as we have non-allocated slots
    remaining and candidates available to protect.

  The goal of this logic is to favorise the diversity of our peer connections.

  The individual commit messages describe each change in more detail.

  Special thank you to Vasil Dimov for the excellent review feedback and the algorithm improvement that made this change much better than it would have been otherwise. Thanks also to Antoine Riard, whose review feedback nudged this change to protect disadvantaged networks having fewer, rather than more, eviction candidates.

ACKs for top commit:
  laanwj:
    Code review re-ACK 1b1088d
  vasild:
    ACK 1b1088d

Tree-SHA512: 722f790ff11f2969c79e45a5e0e938d94df78df8687e77002f32e3ef5c72a9ac10ebf8c7a9eb7f71882c97ab0e67b2778191effdb747d9ca54d7c23c2ed19a90

Author: laanwj

src/net.cpp

@@ -42,6 +42,7 @@
 #endif
 
 #include <algorithm>
+#include <array>
 #include <cstdint>
 #include <functional>
 #include <optional>
@@ -841,18 +842,6 @@ static bool ReverseCompareNodeTimeConnected(const NodeEvictionCandidate &a, cons
     return a.nTimeConnected > b.nTimeConnected;
 }
 
-static bool CompareLocalHostTimeConnected(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b)
-{
-    if (a.m_is_local != b.m_is_local) return b.m_is_local;
-    return a.nTimeConnected > b.nTimeConnected;
-}
-
-static bool CompareOnionTimeConnected(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
-{
-    if (a.m_is_onion != b.m_is_onion) return b.m_is_onion;
-    return a.nTimeConnected > b.nTimeConnected;
-}
-
 static bool CompareNetGroupKeyed(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) {
     return a.nKeyedNetGroup < b.nKeyedNetGroup;
 }
@@ -883,6 +872,26 @@ static bool CompareNodeBlockRelayOnlyTime(const NodeEvictionCandidate &a, const
     return a.nTimeConnected > b.nTimeConnected;
 }
 
+/**
+ * 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.nTimeConnected > b.nTimeConnected;
+    };
+};
+
 //! Sort an array by the specified comparator, then erase the last K elements where predicate is true.
 template <typename T, typename Comparator>
 static void EraseLastKElements(
@@ -894,40 +903,72 @@ static void EraseLastKElements(
     elements.erase(std::remove_if(elements.end() - eraseSize, elements.end(), predicate), elements.end());
 }
 
-void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& vEvictionCandidates)
+void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& 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 + 2) of
-    // these protected spots for onion and localhost peers, if any, even if they're not
-    // longest uptime overall. This helps protect tor peers, which tend to be otherwise
+    // To favorise the diversity of our peer connections, reserve up to half of these protected
+    // spots for Tor/onion, localhost and I2P 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 = vEvictionCandidates.size();
-    size_t total_protect_size = initial_size / 2;
-    const size_t onion_protect_size = total_protect_size / 2;
-
-    if (onion_protect_size) {
-        // Pick out up to 1/4 peers connected via our onion service, sorted by longest uptime.
-        EraseLastKElements(vEvictionCandidates, CompareOnionTimeConnected, onion_protect_size,
-                           [](const NodeEvictionCandidate& n) { return n.m_is_onion; });
-    }
-
-    const size_t localhost_min_protect_size{2};
-    if (onion_protect_size >= localhost_min_protect_size) {
-        // Allocate any remaining slots of the 1/4, or minimum 2 additional slots,
-        // to localhost peers, sorted by longest uptime, as manually configured
-        // hidden services not using `-bind=addr[:port]=onion` will not be detected
-        // as inbound onion connections.
-        const size_t remaining_tor_slots{onion_protect_size - (initial_size - vEvictionCandidates.size())};
-        const size_t localhost_protect_size{std::max(remaining_tor_slots, localhost_min_protect_size)};
-        EraseLastKElements(vEvictionCandidates, CompareLocalHostTimeConnected, localhost_protect_size,
-                           [](const NodeEvictionCandidate& n) { return n.m_is_local; });
+    const size_t initial_size = eviction_candidates.size();
+    const size_t total_protect_size{initial_size / 2};
+
+    // Disadvantaged networks to protect: I2P, localhost, Tor/onion. In case of equal counts, earlier
+    // array members have first opportunity to recover unused slots from the previous iteration.
+    struct Net { bool is_local; Network id; size_t count; };
+    std::array<Net, 3> networks{
+        {{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) {
+        const size_t disadvantaged_to_protect{max_protect_by_network - num_protected};
+        const size_t protect_per_network{
+            std::max(disadvantaged_to_protect / networks.size(), static_cast<size_t>(1))};
+
+        // Early exit flag if there are no remaining candidates by disadvantaged network.
+        bool protected_at_least_one{false};
+
+        for (const 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;
+                num_protected += before - after;
+                if (num_protected >= max_protect_by_network) {
+                    break;
+                }
+            }
+        }
+        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.
-    total_protect_size -= initial_size - vEvictionCandidates.size();
-    EraseLastKElements(vEvictionCandidates, ReverseCompareNodeTimeConnected, total_protect_size);
+    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<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates)
@@ -944,8 +985,7 @@ void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& vEvict
     // 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.
-    const size_t erase_size = std::min(size_t(8), vEvictionCandidates.size());
-    EraseLastKElements(vEvictionCandidates, CompareNodeBlockRelayOnlyTime, erase_size,
+    EraseLastKElements(vEvictionCandidates, CompareNodeBlockRelayOnlyTime, 8,
                        [](const NodeEvictionCandidate& n) { return !n.fRelayTxes && n.fRelevantServices; });
 
     // Protect 4 nodes that most recently sent us novel blocks.
@@ -1024,7 +1064,7 @@ bool CConnman::AttemptToEvictConnection()
                                                HasAllDesirableServiceFlags(node->nServices),
                                                peer_relay_txes, peer_filter_not_null, node->nKeyedNetGroup,
                                                node->m_prefer_evict, node->addr.IsLocal(),
-                                               node->m_inbound_onion};
+                                               node->ConnectedThroughNetwork()};
             vEvictionCandidates.push_back(candidate);
         }
     }

src/net.h

@@ -1209,7 +1209,7 @@ struct NodeEvictionCandidate
     uint64_t nKeyedNetGroup;
     bool prefer_evict;
     bool m_is_local;
-    bool m_is_onion;
+    Network m_network;
 };
 
 /**
@@ -1227,20 +1227,20 @@ struct NodeEvictionCandidate
  * 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 onion peers
- * connected via our tor control service, if any, sorted by longest uptime, even
- * if they're not longest uptime overall. Any remaining slots of the 1/4 are
- * then allocated to protect localhost peers, if any (or up to 2 localhost peers
- * if no slots remain and 2 or more onion peers were protected), sorted by
- * longest uptime, as manually configured hidden services not using
- * `-bind=addr[:port]=onion` will not be detected as inbound onion connections.
+ * 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
  *
- * This helps protect onion peers, which tend to be otherwise disadvantaged
- * under our eviction criteria for their higher min ping times relative to IPv4
- * and IPv6 peers, and favorise the diversity of peer connections.
+ * - I2P peers
  *
- * This function was extracted from SelectNodeToEvict() to be able to test the
- * ratio-based protection logic deterministically.
+ * 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<NodeEvictionCandidate>& vEvictionCandidates);
 

src/test/fuzz/node_eviction.cpp

@@ -31,7 +31,7 @@ FUZZ_TARGET(node_eviction)
             /* nKeyedNetGroup */ fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
             /* prefer_evict */ fuzzed_data_provider.ConsumeBool(),
             /* m_is_local */ fuzzed_data_provider.ConsumeBool(),
-            /* m_is_onion */ fuzzed_data_provider.ConsumeBool(),
+            /* m_network */ fuzzed_data_provider.PickValueInArray(ALL_NETWORKS),
         });
     }
     // Make a copy since eviction_candidates may be in some valid but otherwise