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

3 files changed, 65 insertions, 3 deletions

diff --git a/test/functional/p2p_addrfetch.py b/test/functional/p2p_addrfetch.py
new file mode 100755
index 0000000000..66ee1544a9
--- /dev/null
+++ b/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, NODE_WITNESS
+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 | NODE_WITNESS
+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()
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index ba52abc7dd..afa904c8d7 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -557,9 +557,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.
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 8afd8b3bc1..725706947f 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -186,6 +186,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.py --descriptors',