Merge #18991: Cache responses to GETADDR to prevent topology leaks

3bd67ba5a4ef3c20ef1f873b63c9f53a6c8608b6 Test addr response caching (Gleb Naumenko)
cf1569e074505dbbb9d29422803dd31bb62072d4 Add addr permission flag enabling non-cached addr sharing (Gleb Naumenko)
acd6135b43941fa51d52f5fcdb2ce944280ad01e Cache responses to addr requests (Gleb Naumenko)
7cc0e8101f01891aa8be093a00d993bb7579c385 Remove useless 2500 limit on AddrMan queries (Gleb Naumenko)
ded742bc5b96e3215d69c11fb3628d224e7ae034 Move filtering banned addrs inside GetAddresses() (Gleb Naumenko)

Pull request description:

  This is a very simple code change with a big p2p privacy benefit.

  It’s currently trivial to scrape any reachable node’s AddrMan (a database of all nodes known to them along with the timestamps).
  We do have a limit of one GETADDR per connection, but a spy can disconnect and reconnect even from the same IP, and send GETADDR again and again.

  Since we respond with 1,000 random records at most, depending on the AddrMan size it takes probably up to 100 requests for an spy to make sure they scraped (almost) everything.
  I even have a script for that. It is totally doable within couple minutes.

  Then, with some extra protocol knowledge a spy can infer the direct peers of the victim, and other topological stuff.

  I suggest to cache responses to GETADDR on a daily basis, so that an attacker gets at most 1,000 records per day, and can’t track the changes in real time. I will be following up with more improvements to addr relay privacy, but this one alone is a very effective. And simple!

  I doubt any of the real software does *reconnect to get new addrs from a given peer*, so we shouldn’t be cutting anyone.
  I also believe it doesn’t have any negative implications on the overall topology quality. And the records being “outdated” for at most a day doesn’t break any honest assumptions either.

ACKs for top commit:
  jnewbery:
    reACK 3bd67ba5a4ef3c20ef1f873b63c9f53a6c8608b6
  promag:
    Code review ACK 3bd67ba5a4ef3c20ef1f873b63c9f53a6c8608b6.
  ariard:
    Code Review ACK 3bd67ba

Tree-SHA512: dfa5d03205c2424e40a3f8a41af9306227e1ca18beead3b3dda44aa2a082175bb1c6d929dbc7ea8e48e01aed0d50f0d54491caa1147471a2b72a46c3ca06b66f

3 files changed, 111 insertions, 1 deletions

diff --git a/test/functional/p2p_getaddr_caching.py b/test/functional/p2p_getaddr_caching.py
new file mode 100755
index 0000000000..c9278eab92
--- /dev/null
+++ b/test/functional/p2p_getaddr_caching.py
@@ -0,0 +1,109 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 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 addr response caching"""
+
+import time
+from test_framework.messages import (
+    CAddress,
+    NODE_NETWORK,
+    NODE_WITNESS,
+    msg_addr,
+    msg_getaddr,
+)
+from test_framework.mininode import (
+    P2PInterface,
+    mininode_lock
+)
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+)
+
+MAX_ADDR_TO_SEND = 1000
+
+def gen_addrs(n):
+    addrs = []
+    for i in range(n):
+        addr = CAddress()
+        addr.time = int(time.time())
+        addr.nServices = NODE_NETWORK | NODE_WITNESS
+        # Use first octets to occupy different AddrMan buckets
+        first_octet = i >> 8
+        second_octet = i % 256
+        addr.ip = "{}.{}.1.1".format(first_octet, second_octet)
+        addr.port = 8333
+        addrs.append(addr)
+    return addrs
+
+class AddrReceiver(P2PInterface):
+
+    def __init__(self):
+        super().__init__()
+        self.received_addrs = None
+
+    def get_received_addrs(self):
+        with mininode_lock:
+            return self.received_addrs
+
+    def on_addr(self, message):
+        self.received_addrs = []
+        for addr in message.addrs:
+            self.received_addrs.append(addr.ip)
+
+    def addr_received(self):
+        return self.received_addrs is not None
+
+
+class AddrTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = False
+        self.num_nodes = 1
+
+    def run_test(self):
+        self.log.info('Create connection that sends and requests addr messages')
+        addr_source = self.nodes[0].add_p2p_connection(P2PInterface())
+
+        msg_send_addrs = msg_addr()
+        self.log.info('Fill peer AddrMan with a lot of records')
+        # Since these addrs are sent from the same source, not all of them will be stored,
+        # because we allocate a limited number of AddrMan buckets per addr source.
+        total_addrs = 10000
+        addrs = gen_addrs(total_addrs)
+        for i in range(int(total_addrs/MAX_ADDR_TO_SEND)):
+            msg_send_addrs.addrs = addrs[i * MAX_ADDR_TO_SEND:(i + 1) * MAX_ADDR_TO_SEND]
+            addr_source.send_and_ping(msg_send_addrs)
+
+        responses = []
+        self.log.info('Send many addr requests within short time to receive same response')
+        N = 5
+        cur_mock_time = int(time.time())
+        for i in range(N):
+            addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
+            addr_receiver.send_and_ping(msg_getaddr())
+            # Trigger response
+            cur_mock_time += 5 * 60
+            self.nodes[0].setmocktime(cur_mock_time)
+            addr_receiver.wait_until(addr_receiver.addr_received)
+            responses.append(addr_receiver.get_received_addrs())
+        for response in responses[1:]:
+            assert_equal(response, responses[0])
+        assert(len(response) < MAX_ADDR_TO_SEND)
+
+        cur_mock_time += 3 * 24 * 60 * 60
+        self.nodes[0].setmocktime(cur_mock_time)
+
+        self.log.info('After time passed, see a new response to addr request')
+        last_addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
+        last_addr_receiver.send_and_ping(msg_getaddr())
+        # Trigger response
+        cur_mock_time += 5 * 60
+        self.nodes[0].setmocktime(cur_mock_time)
+        last_addr_receiver.wait_until(last_addr_receiver.addr_received)
+        # new response is different
+        assert(set(responses[0]) != set(last_addr_receiver.get_received_addrs()))
+
+
+if __name__ == '__main__':
+    AddrTest().main()
diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py
index 32a795e345..254352c816 100755
--- a/test/functional/p2p_permissions.py
+++ b/test/functional/p2p_permissions.py
@@ -96,7 +96,7 @@ class P2PPermissionsTests(BitcoinTestFramework):
         self.checkpermission(
             # all permission added
             ["-whitelist=all@127.0.0.1"],
-            ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download"],
+            ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download", "addr"],
             False)
 
         self.stop_node(1)
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 95c2b7c5ec..b090e93394 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -159,6 +159,7 @@ BASE_SCRIPTS = [
     'rpc_deprecated.py',
     'wallet_disable.py',
     'p2p_addr_relay.py',
+    'p2p_getaddr_caching.py',
     'p2p_getdata.py',
     'rpc_net.py',
     'wallet_keypool.py',