Merge bitcoin/bitcoin#22096: p2p: AddrFetch - don't disconnect on self-announcements

5730a43703f7e5a5ca26245ba3b55fbdd027d0b6 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f93979b274a37de013502b05d25fad8 net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d9072b7d5a36a6491784bdeb9247e91fb0 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450dde6a54bd785504c923adfb45c7060 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)

Pull request description:

  AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.

  This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
  So we'll disconnect before the peer gets a chance to answer our `getaddr`.

  I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.

  The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers. 

  Fix this by only disconnecting after receiving an `addr` message of size > 1.

  [Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.

ACKs for top commit:
  amitiuttarwar:
    reACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
  naumenkogs:
    ACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
  jnewbery:
    ACK 5730a43703

Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9

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',