-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Increase the ip address relay branching factor for unreachable networks #19728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
11e8500 to
86d4cf4
Compare
|
Concept ACK |
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
|
Concept ACK |
There was a problem hiding this 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
|
With |
|
@jonatack to observe the effect of this either run with |
|
@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
left a comment
There was a problem hiding this 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 = {|
ACK 86d4cf4 -- 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 |
|
ACK 86d4cf4 |
|
Concept ACK. R0 needs to be greater than one, or it'll burn out before infecting the population. 🦠 |
…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
…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
…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
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
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.