Merge #19763: net: don't try to relay to the address' originator

7fabe0f359ae16ed36ce4ca2c33631d038c21448 net: don't relay to the address' originator (Vasil Dimov)

Pull request description:

  For each address to be relayed we "randomly" pick 2 nodes to send the
  address to (in `RelayAddress()`). However we do not take into
  consideration that it does not make sense to relay the address back to
  its originator (`CNode::PushAddress()` will do nothing in that case).

  This means that if the originator is among the "randomly" picked nodes,
  then we will relay to one node less than intended.

  Fix this by skipping the originating node when choosing candidates to
  relay to.

ACKs for top commit:
  sdaftuar:
    ACK 7fabe0f359ae16ed36ce4ca2c33631d038c21448 (this time I looked at the test, and verified the test breaks in expected ways if I break the code).
  jnewbery:
    utACK 7fabe0f359ae16ed36ce4ca2c33631d038c21448 (only net_processing changes. I haven't reviewed the test changes)
  jonatack:
    re-ACK 7fabe0f359ae16ed36ce4ca2c33631d038c21448 per `git range-diff b76abae fd897f8 7fabe0f`, change since last review is rebase and more readable Doxygen documentation

Tree-SHA512: c6a9d11c7afc97ab4e8960513f6416648d4a8c0c64b713c145a7482a7b9e54946f81386a3351e3ec0011e5594ba5ccff4d10c6f656bb80680d9f0d0a63366165

2 files changed, 48 insertions, 13 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index fd96cdfa97..ac4551faa7 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1410,7 +1410,23 @@ void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman&
     });
 }
 
-static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman& connman)
+/**
+ * Relay (gossip) an address to a few randomly chosen nodes.
+ * We choose the same nodes within a given 24h window (if the list of connected
+ * nodes does not change) and we don't relay to nodes that already know an
+ * address. So within 24h we will likely relay a given address once. This is to
+ * prevent a peer from unjustly giving their address better propagation by sending
+ * it to us repeatedly.
+ * @param[in] originator The peer that sent us the address. We don't want to relay it back.
+ * @param[in] addr Address to relay.
+ * @param[in] fReachable Whether the address' network is reachable. We relay unreachable
+ * addresses less.
+ * @param[in] connman Connection manager to choose nodes to relay to.
+ */
+static void RelayAddress(const CNode& originator,
+                         const CAddress& addr,
+                         bool fReachable,
+                         const CConnman& connman)
 {
     if (!fReachable && !addr.IsRelayable()) return;
 
@@ -1427,8 +1443,8 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
     std::array<std::pair<uint64_t, CNode*>,2> best{{{0, nullptr}, {0, nullptr}}};
     assert(nRelayNodes <= best.size());
 
-    auto sortfunc = [&best, &hasher, nRelayNodes](CNode* pnode) {
-        if (pnode->RelayAddrsWithConn()) {
+    auto sortfunc = [&best, &hasher, nRelayNodes, &originator](CNode* pnode) {
+        if (pnode->RelayAddrsWithConn() && pnode != &originator) {
             uint64_t hashKey = CSipHasher(hasher).Write(pnode->GetId()).Finalize();
             for (unsigned int i = 0; i < nRelayNodes; i++) {
                  if (hashKey > best[i].first) {
@@ -2570,7 +2586,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
             if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
             {
                 // Relay to a limited number of other nodes
-                RelayAddress(addr, fReachable, m_connman);
+                RelayAddress(pfrom, addr, fReachable, m_connman);
             }
             // Do not store addresses outside our network
             if (fReachable)
diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py
index 80f262d0d3..91fbd722cf 100755
--- a/test/functional/p2p_addr_relay.py
+++ b/test/functional/p2p_addr_relay.py
@@ -19,8 +19,11 @@ from test_framework.util import (
 )
 import time
 
+# Keep this with length <= 10. Addresses from larger messages are not relayed.
 ADDRS = []
-for i in range(10):
+num_ipv4_addrs = 10
+
+for i in range(num_ipv4_addrs):
     addr = CAddress()
     addr.time = int(time.time()) + i
     addr.nServices = NODE_NETWORK | NODE_WITNESS
@@ -30,11 +33,15 @@ for i in range(10):
 
 
 class AddrReceiver(P2PInterface):
+    num_ipv4_received = 0
+
     def on_addr(self, message):
         for addr in message.addrs:
             assert_equal(addr.nServices, 9)
+            if not 8333 <= addr.port < 8343:
+                raise AssertionError("Invalid addr.port of {} (8333-8342 expected)".format(addr.port))
             assert addr.ip.startswith('123.123.123.')
-            assert (8333 <= addr.port < 8343)
+            self.num_ipv4_received += 1
 
 
 class AddrTest(BitcoinTestFramework):
@@ -48,21 +55,33 @@ class AddrTest(BitcoinTestFramework):
         msg = msg_addr()
 
         self.log.info('Send too-large addr message')
-        msg.addrs = ADDRS * 101
+        msg.addrs = ADDRS * 101  # more than 1000 addresses in one message
         with self.nodes[0].assert_debug_log(['addr message size = 1010']):
             addr_source.send_and_ping(msg)
 
         self.log.info('Check that addr message content is relayed and added to addrman')
-        addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
+        num_receivers = 7
+        receivers = []
+        for _ in range(num_receivers):
+            receivers.append(self.nodes[0].add_p2p_connection(AddrReceiver()))
         msg.addrs = ADDRS
-        with self.nodes[0].assert_debug_log([
-                'Added 10 addresses from 127.0.0.1: 0 tried',
+        with self.nodes[0].assert_debug_log(
+            [
+                'Added {} addresses from 127.0.0.1: 0 tried'.format(num_ipv4_addrs),
                 'received: addr (301 bytes) peer=0',
-                'sending addr (301 bytes) peer=1',
-        ]):
+            ]
+        ):
             addr_source.send_and_ping(msg)
             self.nodes[0].setmocktime(int(time.time()) + 30 * 60)
-            addr_receiver.sync_with_ping()
+            for receiver in receivers:
+                receiver.sync_with_ping()
+
+        total_ipv4_received = sum(r.num_ipv4_received for r in receivers)
+
+        # Every IPv4 address must be relayed to two peers, other than the
+        # originating node (addr_source).
+        ipv4_branching_factor = 2
+        assert_equal(total_ipv4_received, num_ipv4_addrs * ipv4_branching_factor)
 
 
 if __name__ == '__main__':