Merge bitcoin/bitcoin#18642: Use std::chrono for the time to rotate destination of addr messages + tests

2ff8f4dd81 Add tests for addr destination rotation (Gleb Naumenko)
77ccb7fce1 Use std::chrono for salting when randomizing ADDR destination (Gleb Naumenko)

Pull request description:

  We currently assign a destination peer for relaying particular addresses of nodes every 24 hours, and then rotate. This is done for rate-limiting (ultimately for privacy leak reduction I think?).

  Before this change, 24 hours was defined as uint. I replaced it with std::chrono, which is mockable and type-safe.

  Also added couple tests for this behavior.

ACKs for top commit:
  jonatack:
    ACK 2ff8f4dd81

Tree-SHA512: 16f703ef3ffee13ce3afa82ca7b4baa27308af18cd2eececdce5565badfb68656a2ad9c4594b73772e4bfa99b3fb15f8e4089c1cb4be98c0bae6730a9d2f8a25

src/net_processing.cpp

@@ -135,6 +135,8 @@ static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288;
 static constexpr auto AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL{24h};
 /** Average delay between peer address broadcasts */
 static constexpr auto AVG_ADDRESS_BROADCAST_INTERVAL{30s};
+/** Delay between rotating the peers we relay a particular address to */
+static constexpr auto ROTATE_ADDR_RELAY_DEST_INTERVAL{24h};
 /** Average delay between trickled inventory transmissions for inbound peers.
  *  Blocks and peers with NetPermissionFlags::NoBan permission bypass this. */
 static constexpr auto INBOUND_INVENTORY_BROADCAST_INTERVAL{5s};
@@ -1818,9 +1820,12 @@ void PeerManagerImpl::RelayAddress(NodeId originator,
     // Use deterministic randomness to send to the same nodes for 24 hours
     // at a time so the m_addr_knowns of the chosen nodes prevent repeats
     const uint64_t hash_addr{CServiceHash(0, 0)(addr)};
+    const auto current_time{GetTime<std::chrono::seconds>()};
+    // Adding address hash makes exact rotation time different per address, while preserving periodicity.
+    const uint64_t time_addr{(static_cast<uint64_t>(count_seconds(current_time)) + hash_addr) / count_seconds(ROTATE_ADDR_RELAY_DEST_INTERVAL)};
     const CSipHasher hasher{m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY)
                                 .Write(hash_addr)
-                                .Write((GetTime() + hash_addr) / (24 * 60 * 60))};
+                                .Write(time_addr)};
     FastRandomContext insecure_rand;
 
     // Relay reachable addresses to 2 peers. Unreachable addresses are relayed randomly to 1 or 2 peers.

test/functional/p2p_addr_relay.py

@@ -21,8 +21,19 @@ from test_framework.p2p import (
     P2P_SERVICES,
 )
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, assert_greater_than
+from test_framework.util import (
+    assert_equal,
+    assert_greater_than,
+    assert_greater_than_or_equal
+)
 
+ONE_MINUTE  = 60
+TEN_MINUTES = 10 * ONE_MINUTE
+ONE_HOUR    = 60 * ONE_MINUTE
+TWO_HOURS   =  2 * ONE_HOUR
+ONE_DAY     = 24 * ONE_HOUR
+
+ADDR_DESTINATIONS_THRESHOLD = 4
 
 class AddrReceiver(P2PInterface):
     num_ipv4_received = 0
@@ -85,6 +96,9 @@ class AddrTest(BitcoinTestFramework):
         self.relay_tests()
         self.inbound_blackhole_tests()
 
+        self.destination_rotates_once_in_24_hours_test()
+        self.destination_rotates_more_than_once_over_several_days_test()
+
         # This test populates the addrman, which can impact the node's behavior
         # in subsequent tests
         self.getaddr_tests()
@@ -362,6 +376,56 @@ class AddrTest(BitcoinTestFramework):
 
             self.nodes[0].disconnect_p2ps()
 
+    def get_nodes_that_received_addr(self, peer, receiver_peer, addr_receivers,
+                                     time_interval_1, time_interval_2):
+
+        # Clean addr response related to the initial getaddr. There is no way to avoid initial
+        # getaddr because the peer won't self-announce then.
+        for addr_receiver in addr_receivers:
+            addr_receiver.num_ipv4_received = 0
+
+        for _ in range(10):
+            self.mocktime += time_interval_1
+            self.msg.addrs[0].time = self.mocktime + TEN_MINUTES
+            self.nodes[0].setmocktime(self.mocktime)
+            with self.nodes[0].assert_debug_log(['received: addr (31 bytes) peer=0']):
+                peer.send_and_ping(self.msg)
+                self.mocktime += time_interval_2
+                self.nodes[0].setmocktime(self.mocktime)
+                receiver_peer.sync_with_ping()
+        return [node for node in addr_receivers if node.addr_received()]
+
+    def destination_rotates_once_in_24_hours_test(self):
+        self.restart_node(0, [])
+
+        self.log.info('Test within 24 hours an addr relay destination is rotated at most once')
+        self.mocktime = int(time.time())
+        self.msg = self.setup_addr_msg(1)
+        self.addr_receivers = []
+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
+        receiver_peer = self.nodes[0].add_p2p_connection(AddrReceiver())
+        addr_receivers = [self.nodes[0].add_p2p_connection(AddrReceiver()) for _ in range(20)]
+        nodes_received_addr = self.get_nodes_that_received_addr(peer, receiver_peer, addr_receivers, 0, TWO_HOURS)  # 10 intervals of 2 hours
+        # Per RelayAddress, we would announce these addrs to 2 destinations per day.
+        # Since it's at most one rotation, at most 4 nodes can receive ADDR.
+        assert_greater_than_or_equal(ADDR_DESTINATIONS_THRESHOLD, len(nodes_received_addr))
+        self.nodes[0].disconnect_p2ps()
+
+    def destination_rotates_more_than_once_over_several_days_test(self):
+        self.restart_node(0, [])
+
+        self.log.info('Test after several days an addr relay destination is rotated more than once')
+        self.msg = self.setup_addr_msg(1)
+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
+        receiver_peer = self.nodes[0].add_p2p_connection(AddrReceiver())
+        addr_receivers = [self.nodes[0].add_p2p_connection(AddrReceiver()) for _ in range(20)]
+        # 10 intervals of 1 day (+ 1 hour, which should be enough to cover 30-min Poisson in most cases)
+        nodes_received_addr = self.get_nodes_that_received_addr(peer, receiver_peer, addr_receivers, ONE_DAY, ONE_HOUR)
+        # Now that there should have been more than one rotation, more than
+        # ADDR_DESTINATIONS_THRESHOLD nodes should have received ADDR.
+        assert_greater_than(len(nodes_received_addr), ADDR_DESTINATIONS_THRESHOLD)
+        self.nodes[0].disconnect_p2ps()
+
 
 if __name__ == '__main__':
     AddrTest().main()