Increase the ip address relay branching factor for unreachable networks #19728

Onion addresses propagate very badly among the IPv4/IPv6 network, resulting in difficulty for those to find each other.

The branching factor 1 is probably so low that propagations die out before they reach another onion peer. Increase it to 1.5 on average.

Concept ACK

Concept ACK

ACK 86d4cf42d97abf4c436d1eabf29e2ed150f69c1e. Code review, built and running with some sanity check logging. RelayAddress() is called by ProcessMessage() ADDR msg handling, from within the loop while processing each new address to relay it to a limited number of other nodes. According to git blame, the line setting nRelayNodes hasn't been touched since 2016 in e736772c56a Move network-msg-processing code out of main to its own file, which moved the line but otherwise did not change it. Running a mixed clearnet/onion node with this patch and the logging below, I'm only seeing values of fReachable 1, nRelayNodes 2. IIUC, I need to use the settings in init.cpp that call SetReachable(*, false). Edit: with onlynet=onion am now seeing entries of fReachable 0 with nRelayNodes values of 1 and 2.

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index d637abcef3..bbe8073171 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1488,10 +1488,13 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
     uint64_t hashAddr = addr.GetHash();
     const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
     FastRandomContext insecure_rand;
+    uint64_t finalized = hasher.Finalize();
+    uint64_t bitwise_and_1 = finalized & 1;
 
     // Relay reachable addresses to 2 peers. Unreachable addresses are relayed randomly to 1 or 2 peers.
-    unsigned int nRelayNodes = (fReachable || (hasher.Finalize() & 1)) ? 2 : 1;
-
+    unsigned int nRelayNodes = (fReachable || bitwise_and_1) ? 2 : 1;
+    LogPrintf("RelayAddress hasher.Finalize() %lu, hasher & 1: %lu, fReachable %u, nRelayNodes %u\n", finalized, bitwise_and_1, fReachable, nRelayNodes);

2020-08-18T15:31:09Z RelayAddress hasher.Finalize() 1664677747050559416, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:09Z RelayAddress hasher.Finalize() 14731061525372265962, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:22Z RelayAddress hasher.Finalize() 1787604340311053927, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:31:28Z RelayAddress hasher.Finalize() 10657425253237162609, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:31:28Z RelayAddress hasher.Finalize() 18319025149127497798, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:28Z RelayAddress hasher.Finalize() 11453563408777926184, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:28Z RelayAddress hasher.Finalize() 4017958700527958668, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:28Z RelayAddress hasher.Finalize() 15886276406873487556, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:28Z RelayAddress hasher.Finalize() 1898157685244540934, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:28Z RelayAddress hasher.Finalize() 6290607360332194442, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:28Z RelayAddress hasher.Finalize() 5437874294151945053, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:31:28Z RelayAddress hasher.Finalize() 12907605550886134159, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:31:48Z RelayAddress hasher.Finalize() 10815737377431984780, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:54Z RelayAddress hasher.Finalize() 11087211122310743638, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:54Z RelayAddress hasher.Finalize() 409429789437464070, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:31:54Z RelayAddress hasher.Finalize() 12052452800598648185, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:31:54Z RelayAddress hasher.Finalize() 14040897625625156503, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:31:54Z RelayAddress hasher.Finalize() 9203608419962065951, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:31:54Z RelayAddress hasher.Finalize() 5316736545182478283, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:31:54Z RelayAddress hasher.Finalize() 10617593508301706293, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:31:54Z RelayAddress hasher.Finalize() 923666841117585603, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:32:10Z RelayAddress hasher.Finalize() 9832582743135619765, hasher & 1: 1, fReachable 1, nRelayNodes 2
2020-08-18T15:32:10Z RelayAddress hasher.Finalize() 5795304938231743402, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:32:15Z RelayAddress hasher.Finalize() 1016101328609244348, hasher & 1: 0, fReachable 1, nRelayNodes 2
2020-08-18T15:32:16Z RelayAddress hasher.Finalize() 300853884246103755, hasher & 1: 1, fReachable 1, nRelayNodes 2

With onlynet=onion, proxy=127.0.0.1:9050, externalip=****.onion, bind=127.0.0.1, listen=1:

2020-08-18T17:05:38Z RelayAddress h.finalized 4068200345127824492, &1: 0, fReachable 1, nRelayNodes 2
2020-08-18T17:05:38Z RelayAddress h.finalized 14333925968780995702, &1: 0, fReachable 0, nRelayNodes 1
2020-08-18T17:05:38Z RelayAddress h.finalized 7071100194704352672, &1: 0, fReachable 1, nRelayNodes 2
2020-08-18T17:05:39Z RelayAddress h.finalized 195717758224480417, &1: 1, fReachable 1, nRelayNodes 2
2020-08-18T17:05:45Z RelayAddress h.finalized 16817619203046534136, &1: 0, fReachable 1, nRelayNodes 2
2020-08-18T17:06:13Z RelayAddress h.finalized 7552585319767041328, &1: 0, fReachable 0, nRelayNodes 1
2020-08-18T17:06:13Z RelayAddress h.finalized 2109471816673549627, &1: 1, fReachable 0, nRelayNodes 2
2020-08-18T17:06:13Z RelayAddress h.finalized 11995838901279751680, &1: 0, fReachable 1, nRelayNodes 2
2020-08-18T17:06:13Z RelayAddress h.finalized 3893398105305938194, &1: 0, fReachable 0, nRelayNodes 1
2020-08-18T17:06:14Z RelayAddress h.finalized 8161172177677664072, &1: 0, fReachable 0, nRelayNodes 1
2020-08-18T17:06:24Z RelayAddress h.finalized 10465762448033642779, &1: 1, fReachable 0, nRelayNodes 2
2020-08-18T17:06:24Z RelayAddress h.finalized 18393435326660999961, &1: 1, fReachable 0, nRelayNodes 2
2020-08-18T17:06:24Z RelayAddress h.finalized 4267516420658618872, &1: 0, fReachable 0, nRelayNodes 1
2020-08-18T17:06:54Z RelayAddress h.finalized 15906259941719365967, &1: 1, fReachable 1, nRelayNodes 2
2020-08-18T17:07:16Z RelayAddress h.finalized 5192449381305989311, &1: 1, fReachable 0, nRelayNodes 2
2020-08-18T17:07:16Z RelayAddress h.finalized 9128843626119142903, &1: 1, fReachable 0, nRelayNodes 2
2020-08-18T17:07:16Z RelayAddress h.finalized 3101762099756148826, &1: 0, fReachable 0, nRelayNodes 1
2020-08-18T17:07:16Z RelayAddress h.finalized 11756616115885090190, &1: 0, fReachable 0, nRelayNodes 1
2020-08-18T17:07:20Z RelayAddress h.finalized 787010406092213330, &1: 0, fReachable 0, nRelayNodes 1
2020-08-18T17:07:52Z RelayAddress h.finalized 11938741117950890368, &1: 0, fReachable 1, nRelayNodes 2
2020-08-18T17:07:59Z RelayAddress h.finalized 16536503547621818460, &1: 0, fReachable 1, nRelayNodes 2
2020-08-18T17:08:45Z RelayAddress h.finalized 1661031355910999457, &1: 1, fReachable 1, nRelayNodes 2
2020-08-18T17:08:45Z RelayAddress h.finalized 9867292503841828190, &1: 0, fReachable 1, nRelayNodes 2

@jonatack to observe the effect of this either run with -onlynet=onion (as you did) or run a normal node without any special arguments - no tor proxy, just IPv4/IPv6 connectivity. This will flag TOR addresses as unreachable.

@jonatack I wouldn't expect to see observable differences from just one node doing this. The problem is that other nodes don't relay your onion addresses well unless they're on Tor themselves. Change will require widespread deployment.

@vasild @sipa ACK, thanks. Agreed, this is just me getting a handle on this and sanity checking things.

ACK 86d4cf42d

I wonder why not treat all addresses equally (ie relay always to 2 nodes, even addresses from unreachable networks)? It did that but was changed in 2012 in 090e5b40f as part of #1021. Alas, no explanation why.

I tested this by extending the test in p2p_addr_relay.py as per the patch below.

Short story: it works as expected.

Long story: we send 5 TOR addresses from mininode1 to bitcoind, connect 2 other mininodes (mininode2 and mininode3) to bitcoind and observe what will be relayed to them.

Without the patch the sum of the TOR addresses received by mininode2 and mininode3 is always equal to 5 (because each of the 5 addresses is relayed to exactly one of mininode2 or mininode3).

run1:
TestFramework (INFO): Receiver1 got 5 IPv4 and 2 TOR addresses
TestFramework (INFO): Receiver2 got 5 IPv4 and 3 TOR addresses

run2:
TestFramework (INFO): Receiver1 got 5 IPv4 and 4 TOR addresses
TestFramework (INFO): Receiver2 got 5 IPv4 and 1 TOR addresses

...

With the patch the sum of the TOR address received by both nodes is between 5 and 10.

run1:
TestFramework (INFO): Receiver1 got 5 IPv4 and 3 TOR addresses
TestFramework (INFO): Receiver2 got 5 IPv4 and 3 TOR addresses

run2:
TestFramework (INFO): Receiver1 got 5 IPv4 and 4 TOR addresses
TestFramework (INFO): Receiver2 got 5 IPv4 and 5 TOR addresses

...

<details> <summary>Extended p2p_addr_relay.py diff</summary>

The changes in src/net_processing.cpp fix an unrelated issue - don't consider for relaying a node that knows about that address because PushAddress() will just ignore it and so that peer will have been added in vain to best. It is needed to get predictable results from the test - otherwise sometimes the node which sent us the address is considered for relay and the address is not relayed.

diff --git i/src/net_processing.cpp w/src/net_processing.cpp
index d637abcef..ab000da9c 100644
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -1492,14 +1492,14 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
     // Relay reachable addresses to 2 peers. Unreachable addresses are relayed randomly to 1 or 2 peers.
     unsigned int nRelayNodes = (fReachable || (hasher.Finalize() & 1)) ? 2 : 1;
 
     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->IsAddrRelayPeer()) {
+    auto sortfunc = [&addr, &best, &hasher, nRelayNodes](CNode* pnode) {
+        if (pnode->IsAddrRelayPeer() && !pnode->m_addr_known->contains(addr.GetKey())) {
             uint64_t hashKey = CSipHasher(hasher).Write(pnode->GetId()).Finalize();
             for (unsigned int i = 0; i < nRelayNodes; i++) {
                  if (hashKey > best[i].first) {
                      std::copy(best.begin() + i, best.begin() + nRelayNodes - 1, best.begin() + i + 1);
                      best[i] = std::make_pair(hashKey, pnode);
                      break;
diff --git i/test/functional/p2p_addr_relay.py w/test/functional/p2p_addr_relay.py
index 5c7e27a3a..f7315f97c 100755
--- i/test/functional/p2p_addr_relay.py
+++ w/test/functional/p2p_addr_relay.py
@@ -18,28 +18,56 @@ from test_framework.mininode import (
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
 )
 import time
 
+# Keep this with length <= 10. Addresses from larger messages are not relayed.
 ADDRS = []
-for i in range(10):
+
+num_ipv4_addrs = 5
+for i in range(num_ipv4_addrs):
     addr = CAddress()
     addr.time = int(time.time()) + i
     addr.nServices = NODE_NETWORK | NODE_WITNESS
     addr.ip = "123.123.123.{}".format(i % 256)
     addr.port = 8333 + i
     ADDRS.append(addr)
 
+i = 0
+for tor_addr in [ \
+        "2empatdfea6vwete.onion", \
+        "34aqcwnnuiqh234f.onion", \
+        "3gxqibajrtysyp5o.onion", \
+        "3sami4tg4yhctjyc.onion", \
+        "3w77hrilg6q64opl.onion" \
+    ]:
+    addr = CAddress()
+    addr.time = int(time.time()) + i
+    addr.nServices = NODE_NETWORK | NODE_WITNESS
+    addr.ip = tor_addr
+    addr.port = 8433 + i
+    ADDRS.append(addr)
+    i += 1
+
 
 class AddrReceiver(P2PInterface):
+    num_ipv4_received = 0
+    num_tor_received = 0
+
     def on_addr(self, message):
         for addr in message.addrs:
             assert_equal(addr.nServices, 9)
-            assert addr.ip.startswith('123.123.123.')
-            assert (8333 <= addr.port < 8343)
+            if addr.ip.startswith('123.123.123.'):
+                assert (8333 <= addr.port < 8338)
+                self.num_ipv4_received += 1
+            elif addr.ip.endswith('.onion'):
+                assert (8433 <= addr.port < 8438)
+                self.num_tor_received += 1
+            else:
+                assert False
 
 
 class AddrTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = False
         self.num_nodes = 1
@@ -47,25 +75,39 @@ class AddrTest(BitcoinTestFramework):
     def run_test(self):
         self.log.info('Create connection that sends addr messages')
         addr_source = self.nodes[0].add_p2p_connection(P2PInterface())
         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())
+        # Every IPv4 address must be relayed to both peers.
+        # Every TOR address must be relayed to just one or both peers.
+        self.log.info('Send normal addr message and check that addresses are relayed and added to addrman')
+        addr_receiver1 = self.nodes[0].add_p2p_connection(AddrReceiver())
+        addr_receiver2 = 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',
+                '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()
+            addr_receiver1.sync_with_ping()
+            addr_receiver2.sync_with_ping()
+
+        assert addr_receiver1.num_ipv4_received == 5
+        assert addr_receiver2.num_ipv4_received == 5
+
+        assert 0 <= addr_receiver1.num_tor_received <= 5
+        assert 0 <= addr_receiver2.num_tor_received <= 5
+
+        self.log.info('Receiver1 got {} IPv4 and {} TOR addresses'.format(
+                      addr_receiver1.num_ipv4_received, addr_receiver1.num_tor_received))
+        self.log.info('Receiver2 got {} IPv4 and {} TOR addresses'.format(
+                      addr_receiver2.num_ipv4_received, addr_receiver2.num_tor_received))
 
 
 if __name__ == '__main__':
     AddrTest().main()
diff --git i/test/functional/test_framework/messages.py w/test/functional/test_framework/messages.py
index 5207b563a..350bed670 100755
--- i/test/functional/test_framework/messages.py
+++ w/test/functional/test_framework/messages.py
@@ -15,12 +15,13 @@ msg_block, msg_tx, msg_headers, etc.:
 
 ser_*, deser_*: functions that handle serialization/deserialization.
 
 Classes use __slots__ to ensure extraneous attributes aren't accidentally added
 by tests, compromising their intended effect.
 """
+import base64
 from codecs import encode
 import copy
 import hashlib
 from io import BytesIO
 import random
 import socket
@@ -197,44 +198,52 @@ def ToHex(obj):
     return obj.serialize().hex()
 
 # Objects that map to bitcoind objects, which can be serialized/deserialized
 
 
 class CAddress:
-    __slots__ = ("ip", "nServices", "pchReserved", "port", "time")
+    __slots__ = ("ip", "nServices", "pchReserved", "pchOnionCat", "port", "time")
 
     def __init__(self):
         self.time = 0
         self.nServices = 1
         self.pchReserved = b"\x00" * 10 + b"\xff" * 2
+        self.pchOnionCat = b"\xfd\x87\xd8\x7e\xeb\x43"
         self.ip = "0.0.0.0"
         self.port = 0
 
     def deserialize(self, f, *, with_time=True):
         if with_time:
             # VERSION messages serialize CAddress objects without time
             self.time = struct.unpack("<i", f.read(4))[0]
         self.nServices = struct.unpack("<Q", f.read(8))[0]
-        self.pchReserved = f.read(12)
-        self.ip = socket.inet_ntoa(f.read(4))
+        buf = f.read(16)
+        if buf.startswith(self.pchOnionCat):
+            self.ip = base64.b32encode(buf[len(self.pchOnionCat):]).decode().lower() + ".onion"
+        else:
+            self.ip = socket.inet_ntoa(buf[len(self.pchReserved):])
         self.port = struct.unpack(">H", f.read(2))[0]
 
     def serialize(self, *, with_time=True):
         r = b""
         if with_time:
             # VERSION messages serialize CAddress objects without time
             r += struct.pack("<i", self.time)
         r += struct.pack("<Q", self.nServices)
-        r += self.pchReserved
-        r += socket.inet_aton(self.ip)
+        if self.ip.endswith(".onion"):
+            r += self.pchOnionCat
+            r += base64.b32decode(self.ip[:-len(".onion")], casefold=True)
+        else:
+            r += self.pchReserved
+            r += socket.inet_aton(self.ip)
         r += struct.pack(">H", self.port)
         return r
 
     def __repr__(self):
-        return "CAddress(nServices=%i ip=%s port=%i)" % (self.nServices,
-                                                         self.ip, self.port)
+        return "CAddress(nServices=%i addr=%s port=%i)" % (self.nServices,
+                                                           self.ip, self.port)
 
 
 class CInv:
     __slots__ = ("hash", "type")
 
     typemap = {

</details>

ACK 86d4cf42d97abf4c436d1eabf29e2ed150f69c1e -- patch looks correct

Probably obvious but confirming the PR description: testing with Tor turned off and no bitcoind onion conf args, I see a few fReachable 0 values but they are rare. OTOH, with onlynet=onion, unreachables are the majority.

ACK 86d4cf4

Concept ACK. R₀ needs to be greater than one, or it'll burn out before infecting the population. 🦠