Skip to content

Conversation

@sipa
Copy link
Member

@sipa sipa commented Aug 15, 2020

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.

@fanquake fanquake added the P2P label Aug 15, 2020
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.
@sipa sipa force-pushed the 202008_increase_addr_branching branch from 11e8500 to 86d4cf4 Compare August 15, 2020 06:02
@jonatack
Copy link
Member

jonatack commented Aug 15, 2020

Concept ACK

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 16, 2020
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.

Github-Pull: bitcoin#19728
Rebased-From: 86d4cf4
@practicalswift
Copy link
Contributor

Concept ACK

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 86d4cf4. 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 e736772 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

@jonatack
Copy link
Member

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

@vasild
Copy link
Contributor

vasild commented Aug 18, 2020

@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.

@sipa
Copy link
Member Author

sipa commented Aug 18, 2020

@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.

@jonatack
Copy link
Member

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

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 86d4cf4

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 090e5b4 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

...
Extended p2p_addr_relay.py diff

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 = {

@practicalswift
Copy link
Contributor

ACK 86d4cf4 -- patch looks correct

@jonatack
Copy link
Member

jonatack commented Aug 18, 2020

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.

@naumenkogs
Copy link
Contributor

ACK 86d4cf4

@jnewbery
Copy link
Contributor

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

@laanwj laanwj merged commit 416efcb into bitcoin:master Sep 5, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 9, 2020
…or unreachable networks

86d4cf4 Increase the ip address relay branching factor for unreachable networks (Pieter Wuille)

Pull request description:

  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.

ACKs for top commit:
  practicalswift:
    ACK 86d4cf4 -- patch looks correct
  naumenkogs:
    ACK 86d4cf4
  jonatack:
    ACK 86d4cf4. 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 e736772 *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.
  vasild:
    ACK 86d4cf4

Tree-SHA512: 22391e16d60bcfdec9a9336728da39d68a24a183b3d1b0e8fbc038d265ca6ddf71d16db018f3678745fd9f3e9281049e42197fa0a29124833c50a9170ed6f793
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…or unreachable networks

86d4cf4 Increase the ip address relay branching factor for unreachable networks (Pieter Wuille)

Pull request description:

  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.

ACKs for top commit:
  practicalswift:
    ACK 86d4cf4 -- patch looks correct
  naumenkogs:
    ACK 86d4cf4
  jonatack:
    ACK 86d4cf4. 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 e736772 *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.
  vasild:
    ACK 86d4cf4

Tree-SHA512: 22391e16d60bcfdec9a9336728da39d68a24a183b3d1b0e8fbc038d265ca6ddf71d16db018f3678745fd9f3e9281049e42197fa0a29124833c50a9170ed6f793
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 16, 2021
…or unreachable networks

86d4cf4 Increase the ip address relay branching factor for unreachable networks (Pieter Wuille)

Pull request description:

  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.

ACKs for top commit:
  practicalswift:
    ACK 86d4cf4 -- patch looks correct
  naumenkogs:
    ACK 86d4cf4
  jonatack:
    ACK 86d4cf4. 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 e736772 *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.
  vasild:
    ACK 86d4cf4

Tree-SHA512: 22391e16d60bcfdec9a9336728da39d68a24a183b3d1b0e8fbc038d265ca6ddf71d16db018f3678745fd9f3e9281049e42197fa0a29124833c50a9170ed6f793
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 23, 2021
Summary:
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.

This is a backport of [[bitcoin/bitcoin#19728 | core#19728]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10182
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants