Protect onion+localhost peers in ProtectEvictionCandidatesByRatio()

Now that we have a reliable way to detect inbound onion peers, this commit
updates our existing eviction protection of 1/4 localhost peers to instead
protect up to 1/4 onion peers (connected via our tor control service), sorted by
longest uptime. Any remaining slots of the 1/4 are then allocated to protect
localhost peers, or 2 localhost peers if no slots remain and 2 or more onion
peers are protected, sorted by longest uptime.

The goal is to avoid penalizing onion peers, due to their higher min ping times
relative to IPv4 and IPv6 peers, and improve our diversity of peer connections.

Thank you to Gregory Maxwell, Suhas Daftuar, Vasil Dimov and Pieter Wuille
for valuable review feedback that shaped the direction.

src/net.cpp

@@ -840,6 +840,12 @@ static bool CompareLocalHostTimeConnected(const NodeEvictionCandidate &a, const
     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;
 }
@@ -885,16 +891,31 @@ void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& vEvict
 {
     // 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.
-    // Reserve half of these protected spots for localhost peers, even if
-    // they're not longest-uptime overall. This helps protect tor peers, which
-    // tend to be otherwise disadvantaged under our eviction criteria.
+    // 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
+    // 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;
 
-    // Pick out up to 1/4 peers that are localhost, sorted by longest uptime.
-    const size_t local_erase_size = total_protect_size / 2;
-    EraseLastKElements(vEvictionCandidates, CompareLocalHostTimeConnected, local_erase_size,
-                       [](const NodeEvictionCandidate& n) { return n.m_is_local; });
+    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; });
+    }
 
     // Calculate how many we removed, and update our total number of peers that
     // we want to protect based on uptime accordingly.

src/net.h

@@ -1288,9 +1288,9 @@ struct NodeEvictionCandidate
 /**
  * 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 ratios of
- * desirable or disadvantaged peers. If any eviction candidates remain, the
- * selection logic chooses a peer to evict.
+ * 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<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates);
 
@@ -1300,9 +1300,13 @@ 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 localhost
- * peers, if any, sorted by longest uptime, even if they're not longest uptime
- * overall.
+ * 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.
  *
  * This helps protect onion peers, which tend to be otherwise disadvantaged
  * under our eviction criteria for their higher min ping times relative to IPv4

src/test/net_peer_eviction_tests.cpp

@@ -94,7 +94,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
             c.nTimeConnected = c.id;
-            c.m_is_local = false;
+            c.m_is_onion = c.m_is_local = false;
         },
         /* protected_peer_ids */ {0, 1, 2, 3, 4, 5},
         /* unprotected_peer_ids */ {6, 7, 8, 9, 10, 11},
@@ -104,7 +104,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     BOOST_CHECK(IsProtected(
         num_peers, [num_peers](NodeEvictionCandidate& c) {
             c.nTimeConnected = num_peers - c.id;
-            c.m_is_local = false;
+            c.m_is_onion = c.m_is_local = false;
         },
         /* protected_peer_ids */ {6, 7, 8, 9, 10, 11},
         /* unprotected_peer_ids */ {0, 1, 2, 3, 4, 5},
@@ -113,20 +113,22 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // Test protection of localhost peers...
 
     // Expect 1/4 localhost peers to be protected from eviction,
-    // independently of other characteristics.
+    // if no onion peers.
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
+            c.m_is_onion = false;
             c.m_is_local = (c.id == 1 || c.id == 9 || c.id == 11);
         },
         /* protected_peer_ids */ {1, 9, 11},
         /* unprotected_peer_ids */ {},
         random_context));
 
-    // Expect 1/4 localhost peers and 1/4 of the others to be protected
-    // from eviction, sorted by longest uptime (lowest nTimeConnected).
+    // Expect 1/4 localhost peers and 1/4 of the other peers to be protected,
+    // sorted by longest uptime (lowest nTimeConnected), if no onion peers.
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
             c.nTimeConnected = c.id;
+            c.m_is_onion = false;
             c.m_is_local = (c.id > 6);
         },
         /* protected_peer_ids */ {0, 1, 2, 7, 8, 9},