Merge #5884: backport: Merge bitcoin#22423, 22096

6d44f36afd Merge bitcoin/bitcoin#22096: p2p: AddrFetch - don't disconnect on self-announcements (fanquake)

Pull request description:

  bitcoin backports

Top commit has no ACKs.

Tree-SHA512: 0a7def9eedeab85f4fb7a34e88ce7c051e3e97b7ec68cb91d02e3387de65d0756e11f0d53c66b8d6a2c886f67649b7887349ea48aaf1dd3af86e4e1ca81c29f1

src/net.cpp

@@ -1296,16 +1296,29 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket,
 
 bool CConnman::AddConnection(const std::string& address, ConnectionType conn_type)
 {
-    if (conn_type != ConnectionType::OUTBOUND_FULL_RELAY && conn_type != ConnectionType::BLOCK_RELAY) return false;
-
-    const int max_connections = conn_type == ConnectionType::OUTBOUND_FULL_RELAY ? m_max_outbound_full_relay : m_max_outbound_block_relay;
+    std::optional<int> max_connections;
+    switch (conn_type) {
+    case ConnectionType::INBOUND:
+    case ConnectionType::MANUAL:
+    case ConnectionType::FEELER:
+        return false;
+    case ConnectionType::OUTBOUND_FULL_RELAY:
+        max_connections = m_max_outbound_full_relay;
+        break;
+    case ConnectionType::BLOCK_RELAY:
+        max_connections = m_max_outbound_block_relay;
+        break;
+    // no limit for ADDR_FETCH because -seednode has no limit either
+    case ConnectionType::ADDR_FETCH:
+        break;
+    } // no default case, so the compiler can warn about missing cases
 
     // 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; }););
 
     // Max connections of specified type already exist
-    if (existing_connections >= max_connections) return false;
+    if (max_connections != std::nullopt && existing_connections >= max_connections) return false;
 
     // Max total outbound connections already exist
     CSemaphoreGrant grant(*semOutbound, true);

src/net.h

@@ -1106,6 +1106,7 @@ public:
      *
      * @param[in]   address     Address of node to try connecting to
      * @param[in]   conn_type   ConnectionType::OUTBOUND or ConnectionType::BLOCK_RELAY
+     *                          or ConnectionType::ADDR_FETCH
      * @return      bool        Returns false if there are no available
      *                          slots for this connection:
      *                          - conn_type not a supported ConnectionType

src/net_processing.cpp

@@ -3657,7 +3657,9 @@ void PeerManagerImpl::ProcessMessage(
 
         m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60);
         if (vAddr.size() < 1000) peer->m_getaddr_sent = false;
-        if (pfrom.IsAddrFetchConn()) {
+
+        // AddrFetch: Require multiple addresses to avoid disconnecting on self-announcements
+        if (pfrom.IsAddrFetchConn() && vAddr.size() > 1) {
             LogPrint(BCLog::NET_NETCONN, "addrfetch connection completed peer=%d; disconnecting\n", pfrom.GetId());
             pfrom.fDisconnect = true;
         }
@@ -5351,6 +5353,12 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
 
     const auto current_time = GetTime<std::chrono::microseconds>();
 
+    if (pto->IsAddrFetchConn() && current_time - std::chrono::seconds(pto->nTimeConnected) > 10 * AVG_ADDRESS_BROADCAST_INTERVAL) {
+        LogPrint(BCLog::NET_NETCONN, "addrfetch connection timeout; disconnecting peer=%d\n", pto->GetId());
+        pto->fDisconnect = true;
+        return true;
+    }
+
     MaybeSendPing(*pto, *peer, current_time);
 
     // MaybeSendPing may have marked peer for disconnection

src/rpc/net.cpp

@@ -352,7 +352,7 @@ static RPCHelpMan addconnection()
         "\nOpen an outbound connection to a specified node. This RPC is for testing only.\n",
         {
             {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP address and port to attempt connecting to."},
-            {"connection_type", RPCArg::Type::STR, RPCArg::Optional::NO, "Type of connection to open, either \"outbound-full-relay\" or \"block-relay-only\"."},
+            {"connection_type", RPCArg::Type::STR, RPCArg::Optional::NO, "Type of connection to open (\"outbound-full-relay\", \"block-relay-only\" or \"addr-fetch\")."},
         },
         RPCResult{
             RPCResult::Type::OBJ, "", "",
@@ -378,6 +378,8 @@ static RPCHelpMan addconnection()
         conn_type = ConnectionType::OUTBOUND_FULL_RELAY;
     } else if (conn_type_in == "block-relay-only") {
         conn_type = ConnectionType::BLOCK_RELAY;
+    } else if (conn_type_in == "addr-fetch") {
+        conn_type = ConnectionType::ADDR_FETCH;
     } else {
         throw JSONRPCError(RPC_INVALID_PARAMETER, self.ToString());
     }

test/functional/p2p_addrfetch.py

@@ -0,0 +1,62 @@
+#!/usr/bin/env python3
+# Copyright (c) 2021 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Test p2p addr-fetch connections
+"""
+
+import time
+
+from test_framework.messages import msg_addr, CAddress, NODE_NETWORK
+from test_framework.p2p import P2PInterface, p2p_lock
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+
+ADDR = CAddress()
+ADDR.time = int(time.time())
+ADDR.nServices = NODE_NETWORK
+ADDR.ip = "192.0.0.8"
+ADDR.port = 18444
+
+
+class P2PAddrFetch(BitcoinTestFramework):
+
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+
+    def run_test(self):
+        node = self.nodes[0]
+        self.log.info("Connect to an addr-fetch peer")
+        peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="addr-fetch")
+        info = node.getpeerinfo()
+        assert_equal(len(info), 1)
+        assert_equal(info[0]['connection_type'], 'addr-fetch')
+
+        self.log.info("Check that we send getaddr but don't try to sync headers with the addr-fetch peer")
+        peer.sync_send_with_ping()
+        with p2p_lock:
+            assert peer.message_count['getaddr'] == 1
+            assert peer.message_count['getheaders'] == 0
+
+        self.log.info("Check that answering the getaddr with a single address does not lead to disconnect")
+        # This prevents disconnecting on self-announcements
+        msg = msg_addr()
+        msg.addrs = [ADDR]
+        peer.send_and_ping(msg)
+        assert_equal(len(node.getpeerinfo()), 1)
+
+        self.log.info("Check that answering with larger addr messages leads to disconnect")
+        msg.addrs = [ADDR] * 2
+        peer.send_message(msg)
+        peer.wait_for_disconnect(timeout=5)
+
+        self.log.info("Check timeout for addr-fetch peer that does not send addrs")
+        peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, connection_type="addr-fetch")
+        node.setmocktime(int(time.time()) + 301)  # Timeout: 5 minutes
+        peer.wait_for_disconnect(timeout=5)
+
+
+if __name__ == '__main__':
+    P2PAddrFetch().main()

test/functional/test_framework/test_node.py

@@ -571,9 +571,8 @@ class TestNode():
         return p2p_conn
 
     def add_outbound_p2p_connection(self, p2p_conn, *, p2p_idx, connection_type="outbound-full-relay", **kwargs):
-        """Add an outbound p2p connection from node. Either
-        full-relay("outbound-full-relay") or
-        block-relay-only("block-relay-only") connection.
+        """Add an outbound p2p connection from node. Must be an
+        "outbound-full-relay", "block-relay-only" or "addr-fetch" connection.
 
         This method adds the p2p connection to the self.p2ps list and returns
         the connection to the caller.

test/functional/test_runner.py

@@ -207,6 +207,7 @@ BASE_SCRIPTS = [
     'p2p_addr_relay.py',
     'p2p_getaddr_caching.py',
     'p2p_getdata.py',
+    'p2p_addrfetch.py',
     'rpc_net.py',
     'wallet_keypool.py --legacy-wallet',
     'wallet_keypool_hd.py --legacy-wallet',