Merge bitcoin/bitcoin#27374: p2p: skip netgroup diversity of new connections for tor/i2p/cjdns

b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70 p2p: skip netgroup diversity of new connections for tor/i2p/cjdns networks (stratospher)

Pull request description:

  Follow up for #27264.

  In order to make sure that our persistent outbound slots belong to different netgroups, distinct net groups of our peers are added to `setConnected`. We’d only open a persistent outbound connection to peers which have a different netgroup compared to those netgroups present in `setConnected`.

  Current `GetGroup()` logic assumes route-based diversification behaviour for tor/i2p/cjdns addresses (addresses are public key based and not route-based). Distinct netgroups possible (according to the current `GetGroup()` logic) for:
  1. tor => 030f, 031f, .. 03ff (16 possibilities)
  2. i2p => 040f, 041f, .. 04ff (16 possibilities)
  3. cjdns => 05fc0f, 05fc1f, ... 05fcff (16 possibilities)

  `setConnected` is used in `ThreadOpenConnections()` before making [outbound](https://github.com/bitcoin/bitcoin/blob/84f4ac39fda7ffa5dc84e92d92dd1eeeb5e20f8c/src/net.cpp#L1846) and [anchor](https://github.com/bitcoin/bitcoin/blob/84f4ac39fda7ffa5dc84e92d92dd1eeeb5e20f8c/src/net.cpp#L1805) connections to new peers so that they belong to distinct netgroups.

  **behaviour on master**

  - if we run a node only on tor/i2p/cjdns
  - we wouldn't be able to open more than 16 outbound connections(manual, block-relay-only anchor, outbound full relay, block-relay-only connections) because we run out of possible netgroups.
  - see https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1481322628
  - tested by changing `MAX_OUTBOUND_FULL_RELAY_CONNECTIONS` to 17 with `onlynet=onion` and observed how node wouldn't make more than 16 outbound connections.

  **behaviour on PR**

  - netgroup diversity checks are skipped for tor/i2p/cjdns addresses.
  - we don't insert tor/i2p/cjdns address in `setConnected` and `GetGroup` doesn't get called on tor/i2p/cjdns(see #27369)

ACKs for top commit:
  achow101:
    ACK b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70
  mzumsande:
    ACK b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70
  vasild:
    ACK b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70

Tree-SHA512: c120b3f9ca7f0be3f29ea665cd2f7dfb40cd1d7ec7058984252fb6e0295e414f736c5b4fba03c31188188a5ae4f543fb2654f6ee9776bad745c7ca72d23d5b9b

1 files changed, 22 insertions, 6 deletions

diff --git a/src/net.cpp b/src/net.cpp
index 4f4c0a4079..903fedb2fb 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1703,10 +1703,11 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
         //
         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).
         int nOutboundFullRelay = 0;
         int nOutboundBlockRelay = 0;
-        std::set<std::vector<unsigned char> > setConnected;
+        int outbound_privacy_network_peers = 0;
+        std::set<std::vector<unsigned char>> setConnected; // netgroups of our ipv4/ipv6 outbound peers
 
         {
             LOCK(m_nodes_mutex);
@@ -1714,7 +1715,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                 if (pnode->IsFullOutboundConn()) nOutboundFullRelay++;
                 if (pnode->IsBlockOnlyConn()) nOutboundBlockRelay++;
 
-                // 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
@@ -1728,7 +1729,19 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                     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
             }
         }
@@ -1886,8 +1899,11 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                 }
                 LogPrint(BCLog::NET, "Making feeler connection to %s\n", addrConnect.ToStringAddrPort());
             }
-
-            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);
         }
     }
 }