Merge bitcoin/bitcoin#28895: p2p: do not make automatic outbound connections to addnode peers

5e7cc4144b test: add unit test for CConnman::AddedNodesContain() (Jon Atack)
cc62716920 p2p: do not make automatic outbound connections to addnode peers (Jon Atack)

Pull request description:

  to allocate our limited outbound slots correctly, and to ensure addnode
  connections benefit from their intended protections.

  Our addnode logic usually connects the addnode peers before the automatic
  outbound logic does, but not always, as a connection race can occur.  If an
  addnode peer disconnects us and if it was the only one from its network, there
  can be a race between reconnecting to it with the addnode thread, and it being
  picked as automatic network-specific outbound peer.  Or our internet connection
  or router or the addnode peer could be temporarily offline, and then return
  online during the automatic outbound thread.  Or we could add a new manual peer
  using the addnode RPC at that time.

  The race can be more apparent when our node doesn't know many peers, or with
  networks like cjdns that currently have few bitcoin peers.

  When an addnode peer is connected as an automatic outbound peer and is the only
  connection we have to a network, it can be protected by our new outbound
  eviction logic and persist in the "wrong role".

  Finally, there does not seem to be a reason to make block-relay or short-lived
  feeler connections to addnode peers, as the addnode logic will ensure we connect
  to them if they are up, within the addnode connection limit.

  Fix these issues by checking if the address is an addnode peer in our automatic
  outbound connection logic.

ACKs for top commit:
  mzumsande:
    Tested ACK 5e7cc4144b
  brunoerg:
    utACK 5e7cc4144b
  vasild:
    ACK 5e7cc4144b
  guggero:
    utACK 5e7cc4144b

Tree-SHA512: 2438c3ec92e98aebca2a0da960534e4655a9c6e1192a24a085fc01326d95cdb1b67d8c44e4ee706bc1d8af8564126d446a21b5579dcbec61bdea5fce2f0115ee

src/net.cpp

@@ -2715,6 +2715,17 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                 continue;
             }
 
+            // Do not make automatic outbound connections to addnode peers, to
+            // not use our limited outbound slots for them and to ensure
+            // addnode connections benefit from their intended protections.
+            if (AddedNodesContain(addr)) {
+                LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "Not making automatic %s%s connection to %s peer selected for manual (addnode) connection%s\n",
+                              preferred_net.has_value() ? "network-specific " : "",
+                              ConnectionTypeAsString(conn_type), GetNetworkName(addr.GetNetwork()),
+                              fLogIPs ? strprintf(": %s", addr.ToStringAddrPort()) : "");
+                continue;
+            }
+
             addrConnect = addr;
             break;
         }
@@ -3454,6 +3465,17 @@ bool CConnman::RemoveAddedNode(const std::string& strNode)
     return false;
 }
 
+bool CConnman::AddedNodesContain(const CAddress& addr) const
+{
+    AssertLockNotHeld(m_added_nodes_mutex);
+    const std::string addr_str{addr.ToStringAddr()};
+    const std::string addr_port_str{addr.ToStringAddrPort()};
+    LOCK(m_added_nodes_mutex);
+    return (m_added_node_params.size() < 24 // bound the query to a reasonable limit
+            && std::any_of(m_added_node_params.cbegin(), m_added_node_params.cend(),
+                           [&](const auto& p) { return p.m_added_node == addr_str || p.m_added_node == addr_port_str; }));
+}
+
 size_t CConnman::GetNodeCount(ConnectionDirection flags) const
 {
     LOCK(m_nodes_mutex);

src/net.h

@@ -1185,6 +1185,7 @@ public:
 
     bool AddNode(const AddedNodeParams& add) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
     bool RemoveAddedNode(const std::string& node) EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
+    bool AddedNodesContain(const CAddress& addr) const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
     std::vector<AddedNodeInfo> GetAddedNodeInfo(bool include_connected) const EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex);
 
     /**

src/test/net_peer_connection_tests.cpp

@@ -125,6 +125,13 @@ BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection)
     BOOST_CHECK_EQUAL(connman->GetAddedNodeInfo(/*include_connected=*/true).size(), nodes.size());
     BOOST_CHECK(connman->GetAddedNodeInfo(/*include_connected=*/false).empty());
 
+    // Test AddedNodesContain()
+    for (auto node : connman->TestNodes()) {
+        BOOST_CHECK(connman->AddedNodesContain(node->addr));
+    }
+    AddPeer(id, nodes, *peerman, *connman, ConnectionType::OUTBOUND_FULL_RELAY);
+    BOOST_CHECK(!connman->AddedNodesContain(nodes.back()->addr));
+
     BOOST_TEST_MESSAGE("\nPrint GetAddedNodeInfo contents:");
     for (const auto& info : connman->GetAddedNodeInfo(/*include_connected=*/true)) {
         BOOST_TEST_MESSAGE(strprintf("\nadded node: %s", info.m_params.m_added_node));