From 51edeb082c4a63c5d67d4f601e67603579cd3809 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 12 Apr 2024 16:37:49 +0000 Subject: [PATCH 01/13] merge bitcoin#21594: add network field to getnodeaddresses --- doc/release-notes-5978.md | 5 +++++ src/rpc/net.cpp | 22 ++++++++++------------ test/functional/rpc_net.py | 3 ++- 3 files changed, 17 insertions(+), 13 deletions(-) create mode 100644 doc/release-notes-5978.md diff --git a/doc/release-notes-5978.md b/doc/release-notes-5978.md new file mode 100644 index 0000000000000..7b914b0b32dc6 --- /dev/null +++ b/doc/release-notes-5978.md @@ -0,0 +1,5 @@ +RPC changes +----------------------- + +- The `getnodeaddresses` RPC now returns a "network" field indicating the + network type (ipv4, ipv6, onion, or i2p) for each address. diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index e906e4759af4e..ef184cd5559fd 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -901,7 +901,7 @@ static RPCHelpMan setnetworkactive() static RPCHelpMan getnodeaddresses() { return RPCHelpMan{"getnodeaddresses", - "\nReturn known addresses which can potentially be used to find new nodes in the network\n", + "\nReturn known addresses, which can potentially be used to find new nodes in the network.\n", { {"count", RPCArg::Type::NUM, /* default */ "1", "The maximum number of addresses to return. Specify 0 to return all known addresses."}, }, @@ -910,10 +910,11 @@ static RPCHelpMan getnodeaddresses() { {RPCResult::Type::OBJ, "", "", { - {RPCResult::Type::NUM_TIME, "time", "The " + UNIX_EPOCH_TIME + " of when the node was last seen"}, - {RPCResult::Type::NUM, "services", "The services offered"}, + {RPCResult::Type::NUM_TIME, "time", "The " + UNIX_EPOCH_TIME + " when the node was last seen"}, + {RPCResult::Type::NUM, "services", "The services offered by the node"}, {RPCResult::Type::STR, "address", "The address of the node"}, - {RPCResult::Type::NUM, "port", "The port of the node"}, + {RPCResult::Type::NUM, "port", "The port number of the node"}, + {RPCResult::Type::STR, "network", "The network (" + Join(GetNetworkNames(), ", ") + ") the node connected through"}, }}, } }, @@ -928,15 +929,11 @@ static RPCHelpMan getnodeaddresses() throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled"); } - int count = 1; - if (!request.params[0].isNull()) { - count = request.params[0].get_int(); - if (count < 0) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range"); - } - } + const int count{request.params[0].isNull() ? 1 : request.params[0].get_int()}; + if (count < 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range"); + // returns a shuffled list of CAddress - std::vector vAddr = node.connman->GetAddresses(count, /* max_pct */ 0, /* network */ std::nullopt); + const std::vector vAddr{node.connman->GetAddresses(count, /* max_pct */ 0, /* network */ std::nullopt)}; UniValue ret(UniValue::VARR); for (const CAddress& addr : vAddr) { @@ -945,6 +942,7 @@ static RPCHelpMan getnodeaddresses() obj.pushKV("services", (uint64_t)addr.nServices); obj.pushKV("address", addr.ToStringIP()); obj.pushKV("port", addr.GetPort()); + obj.pushKV("network", GetNetworkName(addr.GetNetClass())); ret.push_back(obj); } return ret; diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 75d5ca4973af4..a5f91e407b53c 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -212,7 +212,7 @@ def test_getnodeaddresses(self): for i in range(10000): first_octet = i >> 8 second_octet = i % 256 - a = "{}.{}.1.1".format(first_octet, second_octet) + a = "{}.{}.1.1".format(first_octet, second_octet) # IPV4 imported_addrs.append(a) self.nodes[0].addpeeraddress(a, 8333) @@ -229,6 +229,7 @@ def test_getnodeaddresses(self): assert_equal(a["services"], NODE_NETWORK) assert a["address"] in imported_addrs assert_equal(a["port"], 8333) + assert_equal(a["network"], "ipv4") node_addresses = self.nodes[0].getnodeaddresses(1) assert_equal(len(node_addresses), 1) From ff3497c18b6c281997dabba15b1029e1a4705324 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 12 Apr 2024 16:38:34 +0000 Subject: [PATCH 02/13] merge bitcoin#21843: enable GetAddr, GetAddresses, and getnodeaddresses by network continuation of cf27db8574ca280e2a6564a42e65eb3b35bfbb38 from dash#5491 includes: - 6c98c09 - 3f89c0e - ce6bca8 --- doc/release-notes-5978.md | 3 +++ src/rpc/net.cpp | 15 +++++++++--- test/functional/rpc_net.py | 49 +++++++++++++++++++++++--------------- 3 files changed, 45 insertions(+), 22 deletions(-) diff --git a/doc/release-notes-5978.md b/doc/release-notes-5978.md index 7b914b0b32dc6..da532820511cf 100644 --- a/doc/release-notes-5978.md +++ b/doc/release-notes-5978.md @@ -3,3 +3,6 @@ RPC changes - The `getnodeaddresses` RPC now returns a "network" field indicating the network type (ipv4, ipv6, onion, or i2p) for each address. + +- `getnodeaddresses` now also accepts a "network" argument (ipv4, ipv6, onion, + or i2p) to return only addresses of the specified network. diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index ef184cd5559fd..eb6fda687fd33 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -904,6 +904,7 @@ static RPCHelpMan getnodeaddresses() "\nReturn known addresses, which can potentially be used to find new nodes in the network.\n", { {"count", RPCArg::Type::NUM, /* default */ "1", "The maximum number of addresses to return. Specify 0 to return all known addresses."}, + {"network", RPCArg::Type::STR, /* default */ "all networks", "Return only addresses of the specified network. Can be one of: " + Join(GetNetworkNames(), ", ") + "."}, }, RPCResult{ RPCResult::Type::ARR, "", "", @@ -920,7 +921,10 @@ static RPCHelpMan getnodeaddresses() }, RPCExamples{ HelpExampleCli("getnodeaddresses", "8") - + HelpExampleRpc("getnodeaddresses", "8") + + HelpExampleCli("getnodeaddresses", "4 \"i2p\"") + + HelpExampleCli("-named getnodeaddresses", "network=onion count=12") + + HelpExampleRpc("getnodeaddresses", "8") + + HelpExampleRpc("getnodeaddresses", "4, \"i2p\"") }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { @@ -932,8 +936,13 @@ static RPCHelpMan getnodeaddresses() const int count{request.params[0].isNull() ? 1 : request.params[0].get_int()}; if (count < 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "Address count out of range"); + const std::optional network{request.params[1].isNull() ? std::nullopt : std::optional{ParseNetwork(request.params[1].get_str())}}; + if (network == NET_UNROUTABLE) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Network not recognized: %s", request.params[1].get_str())); + } + // returns a shuffled list of CAddress - const std::vector vAddr{node.connman->GetAddresses(count, /* max_pct */ 0, /* network */ std::nullopt)}; + const std::vector vAddr{node.connman->GetAddresses(count, /* max_pct */ 0, network)}; UniValue ret(UniValue::VARR); for (const CAddress& addr : vAddr) { @@ -1020,7 +1029,7 @@ static const CRPCCommand commands[] = { "network", "clearbanned", &clearbanned, {} }, { "network", "cleardiscouraged", &cleardiscouraged, {} }, { "network", "setnetworkactive", &setnetworkactive, {"state"} }, - { "network", "getnodeaddresses", &getnodeaddresses, {"count"} }, + { "network", "getnodeaddresses", &getnodeaddresses, {"count", "network"} }, { "hidden", "addconnection", &addconnection, {"address", "connection_type"} }, { "hidden", "addpeeraddress", &addpeeraddress, {"address", "port"} }, diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index a5f91e407b53c..726294a874bcf 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -204,43 +204,54 @@ def test_service_flags(self): def test_getnodeaddresses(self): self.log.info("Test getnodeaddresses") self.nodes[0].add_p2p_connection(P2PInterface()) + services = NODE_NETWORK - # Add some addresses to the Address Manager over RPC. Due to the way - # bucket and bucket position are calculated, some of these addresses - # will collide. + # Add an IPv6 address to the address manager. + ipv6_addr = "1233:3432:2434:2343:3234:2345:6546:4534" + self.nodes[0].addpeeraddress(address=ipv6_addr, port=8333) + + # Add 10,000 IPv4 addresses to the address manager. Due to the way bucket + # and bucket positions are calculated, some of these addresses will collide. imported_addrs = [] for i in range(10000): first_octet = i >> 8 second_octet = i % 256 - a = "{}.{}.1.1".format(first_octet, second_octet) # IPV4 + a = f"{first_octet}.{second_octet}.1.1" imported_addrs.append(a) self.nodes[0].addpeeraddress(a, 8333) - # Obtain addresses via rpc call and check they were ones sent in before. - # - # Maximum possible addresses in addrman is 10000, although actual - # number will usually be less due to bucket and bucket position - # collisions. - node_addresses = self.nodes[0].getnodeaddresses(0) + # Fetch the addresses via the RPC and test the results. + assert_equal(len(self.nodes[0].getnodeaddresses()), 1) # default count is 1 + assert_equal(len(self.nodes[0].getnodeaddresses(count=2)), 2) + assert_equal(len(self.nodes[0].getnodeaddresses(network="ipv4", count=8)), 8) + + # Maximum possible addresses in AddrMan is 10000. The actual number will + # usually be less due to bucket and bucket position collisions. + node_addresses = self.nodes[0].getnodeaddresses(0, "ipv4") assert_greater_than(len(node_addresses), 5000) assert_greater_than(10000, len(node_addresses)) for a in node_addresses: assert_equal(a["time"], self.mocktime) - assert_equal(a["services"], NODE_NETWORK) + assert_equal(a["services"], services) assert a["address"] in imported_addrs assert_equal(a["port"], 8333) assert_equal(a["network"], "ipv4") - node_addresses = self.nodes[0].getnodeaddresses(1) - assert_equal(len(node_addresses), 1) + # Test the IPv6 address. + res = self.nodes[0].getnodeaddresses(0, "ipv6") + assert_equal(len(res), 1) + assert_equal(res[0]["address"], ipv6_addr) + assert_equal(res[0]["network"], "ipv6") + assert_equal(res[0]["port"], 8333) + assert_equal(res[0]["services"], services) - assert_raises_rpc_error(-8, "Address count out of range", self.nodes[0].getnodeaddresses, -1) + # Test for the absence of onion and I2P addresses. + for network in ["onion", "i2p"]: + assert_equal(self.nodes[0].getnodeaddresses(0, network), []) - # addrman's size cannot be known reliably after insertion, as hash collisions may occur - # so only test that requesting a large number of addresses returns less than that - LARGE_REQUEST_COUNT = 10000 - node_addresses = self.nodes[0].getnodeaddresses(LARGE_REQUEST_COUNT) - assert_greater_than(LARGE_REQUEST_COUNT, len(node_addresses)) + # Test invalid arguments. + assert_raises_rpc_error(-8, "Address count out of range", self.nodes[0].getnodeaddresses, -1) + assert_raises_rpc_error(-8, "Network not recognized: Foo", self.nodes[0].getnodeaddresses, 1, "Foo") if __name__ == '__main__': From 7e08db55fe7ba42a3f364c7e87b2ea1e93cbc33c Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 3 Apr 2024 11:19:36 +0000 Subject: [PATCH 03/13] merge bitcoin#22306: Improvements to p2p_addr_relay.py --- test/functional/p2p_addr_relay.py | 61 ++++++++++++++++--------------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index f9c727d607760..1746d7dc81ee5 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -21,30 +21,29 @@ class AddrReceiver(P2PInterface): num_ipv4_received = 0 + test_addr_contents = False - def on_addr(self, message): - for addr in message.addrs: - assert_equal(addr.nServices, 1) - 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.') - self.num_ipv4_received += 1 - - -class GetAddrStore(P2PInterface): - getaddr_received = False - num_ipv4_received = 0 - - def on_getaddr(self, message): - self.getaddr_received = True + def __init__(self, test_addr_contents=False): + super().__init__() + self.test_addr_contents = test_addr_contents def on_addr(self, message): for addr in message.addrs: self.num_ipv4_received += 1 + if(self.test_addr_contents): + # relay_tests checks the content of the addr messages match + # expectations based on the message creation in setup_addr_msg + assert_equal(addr.nServices, 1) + 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.') def addr_received(self): return self.num_ipv4_received != 0 + def getaddr_received(self): + return self.message_count['getaddr'] > 0 + class AddrTest(BitcoinTestFramework): counter = 0 @@ -76,9 +75,9 @@ def setup_addr_msg(self, num): def send_addr_msg(self, source, msg, receivers): source.send_and_ping(msg) # pop m_next_addr_send timer - self.bump_mocktime(5 * 60) + self.bump_mocktime(10 * 60) for peer in receivers: - peer.sync_send_with_ping() + peer.sync_with_ping() def oversized_addr_test(self): self.log.info('Send an addr message that is too large') @@ -97,7 +96,7 @@ def relay_tests(self): num_receivers = 7 receivers = [] for _ in range(num_receivers): - receivers.append(self.nodes[0].add_p2p_connection(AddrReceiver())) + receivers.append(self.nodes[0].add_p2p_connection(AddrReceiver(test_addr_contents=True))) # Keep this with length <= 10. Addresses from larger messages are not # relayed. @@ -121,8 +120,8 @@ def relay_tests(self): self.nodes[0].disconnect_p2ps() self.log.info('Check relay of addresses received from outbound peers') - inbound_peer = self.nodes[0].add_p2p_connection(AddrReceiver()) - full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=0, connection_type="outbound-full-relay") + inbound_peer = self.nodes[0].add_p2p_connection(AddrReceiver(test_addr_contents=True)) + full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type="outbound-full-relay") msg = self.setup_addr_msg(2) self.send_addr_msg(full_outbound_peer, msg, [inbound_peer]) self.log.info('Check that the first addr message received from an outbound peer is not relayed') @@ -138,7 +137,7 @@ def relay_tests(self): assert_equal(inbound_peer.num_ipv4_received, 2) self.log.info('Check address relay to outbound peers') - block_relay_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=1, connection_type="block-relay-only") + block_relay_peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=1, connection_type="block-relay-only") msg3 = self.setup_addr_msg(2) self.send_addr_msg(inbound_peer, msg3, [full_outbound_peer, block_relay_peer]) @@ -152,17 +151,17 @@ def relay_tests(self): def getaddr_tests(self): self.log.info('Test getaddr behavior') self.log.info('Check that we send a getaddr message upon connecting to an outbound-full-relay peer') - full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=0, connection_type="outbound-full-relay") + full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type="outbound-full-relay") full_outbound_peer.sync_with_ping() - assert full_outbound_peer.getaddr_received + assert full_outbound_peer.getaddr_received() self.log.info('Check that we do not send a getaddr message upon connecting to a block-relay-only peer') - block_relay_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=1, connection_type="block-relay-only") + block_relay_peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=1, connection_type="block-relay-only") block_relay_peer.sync_with_ping() - assert_equal(block_relay_peer.getaddr_received, False) + assert_equal(block_relay_peer.getaddr_received(), False) self.log.info('Check that we answer getaddr messages only from inbound peers') - inbound_peer = self.nodes[0].add_p2p_connection(GetAddrStore()) + inbound_peer = self.nodes[0].add_p2p_connection(AddrReceiver()) inbound_peer.sync_with_ping() # Add some addresses to addrman @@ -177,7 +176,7 @@ def getaddr_tests(self): inbound_peer.send_and_ping(msg_getaddr()) self.bump_mocktime(5 * 60) - inbound_peer.wait_until(inbound_peer.addr_received) + inbound_peer.wait_until(lambda: inbound_peer.addr_received() is True) assert_equal(full_outbound_peer.num_ipv4_received, 0) assert_equal(block_relay_peer.num_ipv4_received, 0) @@ -190,14 +189,16 @@ def blocksonly_mode_tests(self): self.restart_node(0, ["-blocksonly"]) self.log.info('Check that we send getaddr messages') - full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=0, connection_type="outbound-full-relay") + full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type="outbound-full-relay") full_outbound_peer.sync_with_ping() - assert full_outbound_peer.getaddr_received + assert full_outbound_peer.getaddr_received() self.log.info('Check that we relay address messages') addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) msg = self.setup_addr_msg(2) - self.send_addr_msg(addr_source, msg, [full_outbound_peer]) + addr_source.send_and_ping(msg) + self.bump_mocktime(5 * 60) + full_outbound_peer.sync_with_ping() assert_equal(full_outbound_peer.num_ipv4_received, 2) self.nodes[0].disconnect_p2ps() From fe66202c059e3a2df43c838c8d782de1420ac20e Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 4 Apr 2024 17:13:03 +0000 Subject: [PATCH 04/13] merge bitcoin#22211: relay I2P addresses even if not reachable (by us) --- src/netaddress.h | 2 +- test/functional/p2p_addrv2_relay.py | 30 ++++++++++++--------- test/functional/test_framework/messages.py | 31 +++++++++++++++++----- 3 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/netaddress.h b/src/netaddress.h index e0ff362dcfdee..f50c925158946 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -232,7 +232,7 @@ class CNetAddr */ bool IsRelayable() const { - return IsIPv4() || IsIPv6() || IsTor(); + return IsIPv4() || IsIPv6() || IsTor() || IsI2P(); } /** diff --git a/test/functional/p2p_addrv2_relay.py b/test/functional/p2p_addrv2_relay.py index 6b4740c265db7..95c5bf52470cf 100755 --- a/test/functional/p2p_addrv2_relay.py +++ b/test/functional/p2p_addrv2_relay.py @@ -15,6 +15,10 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal +I2P_ADDR = "c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p" + +ADDRS = [] + class AddrReceiver(P2PInterface): addrv2_received_and_checked = False @@ -23,11 +27,8 @@ def __init__(self): super().__init__(support_addrv2 = True) def on_addrv2(self, message): - for addr in message.addrs: - assert_equal(addr.nServices, 1) - assert addr.ip.startswith('123.123.123.') - assert 8333 <= addr.port < 8343 - self.addrv2_received_and_checked = True + if ADDRS == message.addrs: + self.addrv2_received_and_checked = True def wait_for_addrv2(self): self.wait_until(lambda: "addrv2" in self.last_message) @@ -39,18 +40,21 @@ def set_test_params(self): self.num_nodes = 1 def run_test(self): - ADDRS = [] for i in range(10): addr = CAddress() addr.time = int(self.mocktime) + i addr.nServices = NODE_NETWORK - addr.ip = "123.123.123.{}".format(i % 256) + # Add one I2P address at an arbitrary position. + if i == 5: + addr.net = addr.NET_I2P + addr.ip = I2P_ADDR + else: + addr.ip = f"123.123.123.{i % 256}" addr.port = 8333 + i ADDRS.append(addr) self.log.info('Create connection that sends addrv2 messages') addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) - msg = msg_addrv2() self.log.info('Send too-large addrv2 message') @@ -63,22 +67,24 @@ def run_test(self): self.log.info('Check that addrv2 message content is relayed and added to addrman') addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) addr_receiver = 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', - 'received: addrv2 (131 bytes) peer=1', + # The I2P address is not added to node's own addrman because it has no + # I2P reachability (thus 10 - 1 = 9). + 'Added 9 addresses from 127.0.0.1: 0 tried', + 'received: addrv2 (159 bytes) peer=1', ]): addr_source.send_and_ping(msg) # Wait until "Added ..." before bumping mocktime to make sure addv2 is (almost) fully processed with self.nodes[0].assert_debug_log([ - 'sending addrv2 (131 bytes) peer=2', + 'sending addrv2 (159 bytes) peer=2', ]): self.bump_mocktime(30 * 60) addr_receiver.wait_for_addrv2() assert addr_receiver.addrv2_received_and_checked + assert_equal(len(self.nodes[0].getnodeaddresses(count=0, network="i2p")), 0) self.nodes[0].disconnect_p2ps() diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 17164eb55ab6d..c6ed32ebeab74 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -16,7 +16,7 @@ Classes use __slots__ to ensure extraneous attributes aren't accidentally added by tests, compromising their intended effect. """ - +from base64 import b32decode, b32encode import copy from collections import namedtuple import hashlib @@ -251,15 +251,20 @@ class CAddress: # see https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki NET_IPV4 = 1 + NET_I2P = 5 ADDRV2_NET_NAME = { - NET_IPV4: "IPv4" + NET_IPV4: "IPv4", + NET_I2P: "I2P" } ADDRV2_ADDRESS_LENGTH = { - NET_IPV4: 4 + NET_IPV4: 4, + NET_I2P: 32 } + I2P_PAD = "====" + def __init__(self): self.time = 0 self.nServices = 1 @@ -267,6 +272,9 @@ def __init__(self): self.ip = "0.0.0.0" self.port = 0 + def __eq__(self, other): + return self.net == other.net and self.ip == other.ip and self.nServices == other.nServices and self.port == other.port and self.time == other.time + def deserialize(self, f, *, with_time=True): """Deserialize from addrv1 format (pre-BIP155)""" if with_time: @@ -299,24 +307,33 @@ def deserialize_v2(self, f): self.nServices = deser_compact_size(f) self.net = struct.unpack("B", f.read(1))[0] - assert self.net == self.NET_IPV4 + assert self.net in (self.NET_IPV4, self.NET_I2P) address_length = deser_compact_size(f) assert address_length == self.ADDRV2_ADDRESS_LENGTH[self.net] - self.ip = socket.inet_ntoa(f.read(4)) + addr_bytes = f.read(address_length) + if self.net == self.NET_IPV4: + self.ip = socket.inet_ntoa(addr_bytes) + else: + self.ip = b32encode(addr_bytes)[0:-len(self.I2P_PAD)].decode("ascii").lower() + ".b32.i2p" self.port = struct.unpack(">H", f.read(2))[0] def serialize_v2(self): """Serialize in addrv2 format (BIP155)""" - assert self.net == self.NET_IPV4 + assert self.net in (self.NET_IPV4, self.NET_I2P) r = b"" r += struct.pack("H", self.port) return r From 602d13d2a23ee63bc083bb125b0784d6c02cb8b9 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 12 Apr 2024 16:40:58 +0000 Subject: [PATCH 05/13] merge bitcoin#22387: Rate limit the processing of rumoured addresses --- src/net_permissions.h | 3 +- src/net_processing.cpp | 54 ++++++++++++++ src/net_processing.h | 2 + src/rpc/net.cpp | 2 + test/functional/p2p_addr_relay.py | 95 +++++++++++++++++++++++-- test/functional/p2p_addrv2_relay.py | 5 +- test/functional/p2p_invalid_messages.py | 1 + 7 files changed, 155 insertions(+), 7 deletions(-) diff --git a/src/net_permissions.h b/src/net_permissions.h index ed75562738d76..0a29cc2216fff 100644 --- a/src/net_permissions.h +++ b/src/net_permissions.h @@ -30,7 +30,8 @@ enum class NetPermissionFlags : uint32_t { NoBan = (1U << 4) | Download, // Can query the mempool Mempool = (1U << 5), - // Can request addrs without hitting a privacy-preserving cache + // Can request addrs without hitting a privacy-preserving cache, and send us + // unlimited amounts of addrs. Addr = (1U << 7), // True if the user did not specifically set fine grained permissions diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 23cd76cc7e93d..5b3ba67797f18 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -176,6 +176,13 @@ static constexpr uint32_t MAX_GETCFHEADERS_SIZE = 2000; static constexpr size_t MAX_PCT_ADDR_TO_SEND = 23; /** The maximum number of address records permitted in an ADDR message. */ static constexpr size_t MAX_ADDR_TO_SEND{1000}; +/** The maximum rate of address records we're willing to process on average. Can be bypassed using + * the NetPermissionFlags::Addr permission. */ +static constexpr double MAX_ADDR_RATE_PER_SECOND{0.1}; +/** The soft limit of the address processing token bucket (the regular MAX_ADDR_RATE_PER_SECOND + * based increments won't go above this, but the MAX_ADDR_TO_SEND increment following GETADDR + * is exempt from this limit. */ +static constexpr size_t MAX_ADDR_PROCESSING_TOKEN_BUCKET{MAX_ADDR_TO_SEND}; struct COrphanTx { // When modifying, adapt the copy of this definition in tests/DoS_tests. @@ -273,6 +280,15 @@ struct Peer { std::atomic_bool m_wants_addrv2{false}; /** Whether this peer has already sent us a getaddr message. */ bool m_getaddr_recvd{false}; + /** Number of addr messages that can be processed from this peer. Start at 1 to + * permit self-announcement. */ + double m_addr_token_bucket{1.0}; + /** When m_addr_token_bucket was last updated */ + std::chrono::microseconds m_addr_token_timestamp{GetTime()}; + /** Total number of addresses that were dropped due to rate limiting. */ + std::atomic m_addr_rate_limited{0}; + /** Total number of addresses that were processed (excludes rate limited ones). */ + std::atomic m_addr_processed{0}; /** Set of txids to reconsider once their parent transactions have been accepted **/ std::set m_orphan_work_set GUARDED_BY(g_cs_orphans); @@ -1447,6 +1463,8 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats) } stats.m_ping_wait = ping_wait; + stats.m_addr_processed = peer->m_addr_processed.load(); + stats.m_addr_rate_limited = peer->m_addr_rate_limited.load(); return true; } @@ -3219,6 +3237,9 @@ void PeerManagerImpl::ProcessMessage( // Get recent addresses m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::GETADDR)); peer->m_getaddr_sent = true; + // When requesting a getaddr, accept an additional MAX_ADDR_TO_SEND addresses in response + // (bypassing the MAX_ADDR_PROCESSING_TOKEN_BUCKET limit). + peer->m_addr_token_bucket += MAX_ADDR_TO_SEND; } if (!pfrom.IsInboundConn()) { @@ -3387,11 +3408,34 @@ void PeerManagerImpl::ProcessMessage( std::vector vAddrOk; int64_t nNow = GetAdjustedTime(); int64_t nSince = nNow - 10 * 60; + + // Update/increment addr rate limiting bucket. + const auto current_time = GetTime(); + if (peer->m_addr_token_bucket < MAX_ADDR_PROCESSING_TOKEN_BUCKET) { + // Don't increment bucket if it's already full + const auto time_diff = std::max(current_time - peer->m_addr_token_timestamp, 0us); + const double increment = CountSecondsDouble(time_diff) * MAX_ADDR_RATE_PER_SECOND; + peer->m_addr_token_bucket = std::min(peer->m_addr_token_bucket + increment, MAX_ADDR_PROCESSING_TOKEN_BUCKET); + } + peer->m_addr_token_timestamp = current_time; + + const bool rate_limited = !pfrom.HasPermission(NetPermissionFlags::Addr); + uint64_t num_proc = 0; + uint64_t num_rate_limit = 0; + Shuffle(vAddr.begin(), vAddr.end(), FastRandomContext()); for (CAddress& addr : vAddr) { if (interruptMsgProc) return; + // Apply rate limiting. + if (rate_limited) { + if (peer->m_addr_token_bucket < 1.0) { + ++num_rate_limit; + continue; + } + peer->m_addr_token_bucket -= 1.0; + } // We only bother storing full nodes, though this may include // things which we would not make an outbound connection to, in // part because we may make feeler connections to them. @@ -3405,6 +3449,7 @@ void PeerManagerImpl::ProcessMessage( // Do not process banned/discouraged addresses beyond remembering we received them continue; } + ++num_proc; bool fReachable = IsReachable(addr); if (addr.nTime > nSince && !peer->m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable()) { // Relay to a limited number of other nodes @@ -3414,6 +3459,15 @@ void PeerManagerImpl::ProcessMessage( if (fReachable) vAddrOk.push_back(addr); } + peer->m_addr_processed += num_proc; + peer->m_addr_rate_limited += num_rate_limit; + LogPrint(BCLog::NET, "Received addr: %u addresses (%u processed, %u rate-limited) from peer=%d%s\n", + vAddr.size(), + num_proc, + num_rate_limit, + pfrom.GetId(), + fLogIPs ? ", peeraddr=" + pfrom.addr.ToString() : ""); + m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60); if (vAddr.size() < 1000) peer->m_getaddr_sent = false; if (pfrom.IsAddrFetchConn()) { diff --git a/src/net_processing.h b/src/net_processing.h index 680dd2cae56b9..d403ec4052239 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -41,6 +41,8 @@ struct CNodeStateStats { int m_starting_height = -1; std::chrono::microseconds m_ping_wait; std::vector vHeightInFlight; + uint64_t m_addr_processed = 0; + uint64_t m_addr_rate_limited = 0; }; class PeerManager : public CValidationInterface, public NetEventsInterface diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index eb6fda687fd33..04f2ae6f1d905 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -243,6 +243,8 @@ static RPCHelpMan getpeerinfo() heights.push_back(height); } obj.pushKV("inflight", heights); + obj.pushKV("addr_processed", statestats.m_addr_processed); + obj.pushKV("addr_rate_limited", statestats.m_addr_rate_limited); } obj.pushKV("whitelisted", stats.m_legacyWhitelisted); UniValue permissions(UniValue::VARR); diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index 1746d7dc81ee5..306e16b55d41c 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -12,16 +12,19 @@ msg_addr, msg_getaddr ) -from test_framework.p2p import P2PInterface -from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import ( - assert_equal, +from test_framework.p2p import ( + P2PInterface, + p2p_lock, ) +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal +import random class AddrReceiver(P2PInterface): num_ipv4_received = 0 test_addr_contents = False + _tokens = 1 def __init__(self, test_addr_contents=False): super().__init__() @@ -38,6 +41,20 @@ def on_addr(self, message): raise AssertionError("Invalid addr.port of {} (8333-8342 expected)".format(addr.port)) assert addr.ip.startswith('123.123.123.') + def on_getaddr(self, message): + # When the node sends us a getaddr, it increments the addr relay tokens for the connection by 1000 + self._tokens += 1000 + + @property + def tokens(self): + with p2p_lock: + return self._tokens + + def increment_tokens(self, n): + # When we move mocktime forward, the node increments the addr relay tokens for its peers + with p2p_lock: + self._tokens += n + def addr_received(self): return self.num_ipv4_received != 0 @@ -50,12 +67,14 @@ class AddrTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 + self.extra_args = [["-whitelist=addr@127.0.0.1"]] def run_test(self): self.oversized_addr_test() self.relay_tests() self.getaddr_tests() self.blocksonly_mode_tests() + self.rate_limit_tests() def setup_addr_msg(self, num): addrs = [] @@ -72,6 +91,19 @@ def setup_addr_msg(self, num): msg.addrs = addrs return msg + def setup_rand_addr_msg(self, num): + addrs = [] + for i in range(num): + addr = CAddress() + addr.time = self.mocktime + i + addr.nServices = NODE_NETWORK + addr.ip = f"{random.randrange(128,169)}.{random.randrange(1,255)}.{random.randrange(1,255)}.{random.randrange(1,255)}" + addr.port = 8333 + addrs.append(addr) + msg = msg_addr() + msg.addrs = addrs + return msg + def send_addr_msg(self, source, msg, receivers): source.send_and_ping(msg) # pop m_next_addr_send timer @@ -186,7 +218,7 @@ def getaddr_tests(self): def blocksonly_mode_tests(self): self.log.info('Test addr relay in -blocksonly mode') - self.restart_node(0, ["-blocksonly"]) + self.restart_node(0, ["-blocksonly", "-whitelist=addr@127.0.0.1"]) self.log.info('Check that we send getaddr messages') full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type="outbound-full-relay") @@ -203,6 +235,59 @@ def blocksonly_mode_tests(self): self.nodes[0].disconnect_p2ps() + def send_addrs_and_test_rate_limiting(self, peer, no_relay, new_addrs, total_addrs): + """Send an addr message and check that the number of addresses processed and rate-limited is as expected""" + + peer.send_and_ping(self.setup_rand_addr_msg(new_addrs)) + + peerinfo = self.nodes[0].getpeerinfo()[0] + addrs_processed = peerinfo['addr_processed'] + addrs_rate_limited = peerinfo['addr_rate_limited'] + self.log.debug(f"addrs_processed = {addrs_processed}, addrs_rate_limited = {addrs_rate_limited}") + + if no_relay: + assert_equal(addrs_processed, 0) + assert_equal(addrs_rate_limited, 0) + else: + assert_equal(addrs_processed, min(total_addrs, peer.tokens)) + assert_equal(addrs_rate_limited, max(0, total_addrs - peer.tokens)) + + def rate_limit_tests(self): + + self.restart_node(0, []) + + for contype, no_relay in [("outbound-full-relay", False), ("block-relay-only", True), ("inbound", False)]: + self.log.info(f'Test rate limiting of addr processing for {contype} peers') + if contype == "inbound": + peer = self.nodes[0].add_p2p_connection(AddrReceiver()) + else: + peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type=contype) + + # Send 600 addresses. For all but the block-relay-only peer this should result in addresses being processed. + self.send_addrs_and_test_rate_limiting(peer, no_relay, 600, 600) + + # Send 600 more addresses. For the outbound-full-relay peer (which we send a GETADDR, and thus will + # process up to 1001 incoming addresses), this means more addresses will be processed. + self.send_addrs_and_test_rate_limiting(peer, no_relay, 600, 1200) + + # Send 10 more. As we reached the processing limit for all nodes, no more addresses should be procesesd. + self.send_addrs_and_test_rate_limiting(peer, no_relay, 10, 1210) + + # Advance the time by 100 seconds, permitting the processing of 10 more addresses. + # Send 200 and verify that 10 are processed. + self.bump_mocktime(100) + peer.increment_tokens(10) + + self.send_addrs_and_test_rate_limiting(peer, no_relay, 200, 1410) + + # Advance the time by 1000 seconds, permitting the processing of 100 more addresses. + # Send 200 and verify that 100 are processed. + self.bump_mocktime(1000) + peer.increment_tokens(100) + + self.send_addrs_and_test_rate_limiting(peer, no_relay, 200, 1610) + + self.nodes[0].disconnect_p2ps() if __name__ == '__main__': AddrTest().main() diff --git a/test/functional/p2p_addrv2_relay.py b/test/functional/p2p_addrv2_relay.py index 95c5bf52470cf..df083dbcca3eb 100755 --- a/test/functional/p2p_addrv2_relay.py +++ b/test/functional/p2p_addrv2_relay.py @@ -27,7 +27,9 @@ def __init__(self): super().__init__(support_addrv2 = True) def on_addrv2(self, message): - if ADDRS == message.addrs: + expected_set = set((addr.ip, addr.port) for addr in ADDRS) + received_set = set((addr.ip, addr.port) for addr in message.addrs) + if expected_set == received_set: self.addrv2_received_and_checked = True def wait_for_addrv2(self): @@ -38,6 +40,7 @@ class AddrTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 1 + self.extra_args = [["-whitelist=addr@127.0.0.1"]] def run_test(self): for i in range(10): diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 8082ee4f44928..3dc55fa0c6f21 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -46,6 +46,7 @@ class InvalidMessagesTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 self.setup_clean_chain = True + self.extra_args = [["-whitelist=addr@127.0.0.1"]] def run_test(self): self.test_buffer() From c1874c6615e020f7530dc3979cca01c0acf7d72f Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 12 Apr 2024 16:43:21 +0000 Subject: [PATCH 06/13] net_processing: gate `m_tx_relay` access behind `!IsBlockOnlyConn()` In bitcoin#21528, the value of `m_addr_relay_enabled` isn't determined until the first ADDR/ADDRV2/GETADDR call. Until then, it'll always default to `false`. This creates a false-negative where a term equivalent to "not a block connection" no longer reliably means that. Therefore we need to switch to directly querying "not a block-only connection". --- src/net_processing.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5b3ba67797f18..3bf2041789e74 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2230,7 +2230,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } else if (inv.IsMsgFilteredBlk()) { bool sendMerkleBlock = false; CMerkleBlock merkleBlock; - if (RelayAddrsWithPeer(peer)) { + if (!pfrom.IsBlockOnlyConn()) { LOCK(pfrom.m_tx_relay->cs_filter); if (pfrom.m_tx_relay->pfilter) { sendMerkleBlock = true; @@ -2333,7 +2333,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic const std::chrono::seconds now = GetTime(); // Get last mempool request time - const std::chrono::seconds mempool_req = RelayAddrsWithPeer(peer) ? pfrom.m_tx_relay->m_last_mempool_req.load() + const std::chrono::seconds mempool_req = !pfrom.IsBlockOnlyConn() ? pfrom.m_tx_relay->m_last_mempool_req.load() : std::chrono::seconds::min(); // Process as many TX items from the front of the getdata queue as @@ -3197,7 +3197,7 @@ void PeerManagerImpl::ProcessMessage( // set nodes not capable of serving the complete blockchain history as "limited nodes" pfrom.m_limited_node = (!(nServices & NODE_NETWORK) && (nServices & NODE_NETWORK_LIMITED)); - if (RelayAddrsWithPeer(*peer)) { + if (!pfrom.IsBlockOnlyConn()) { LOCK(pfrom.m_tx_relay->cs_filter); pfrom.m_tx_relay->fRelayTxes = fRelay; // set to true after we get the first filter* message } @@ -4414,7 +4414,7 @@ void PeerManagerImpl::ProcessMessage( return; } - if (RelayAddrsWithPeer(*peer)) { + if (!pfrom.IsBlockOnlyConn()) { LOCK(pfrom.m_tx_relay->cs_tx_inventory); pfrom.m_tx_relay->fSendMempool = true; } @@ -4508,7 +4508,7 @@ void PeerManagerImpl::ProcessMessage( // There is no excuse for sending a too-large filter Misbehaving(pfrom.GetId(), 100, "too-large bloom filter"); } - else if (RelayAddrsWithPeer(*peer)) + else if (!pfrom.IsBlockOnlyConn()) { LOCK(pfrom.m_tx_relay->cs_filter); pfrom.m_tx_relay->pfilter.reset(new CBloomFilter(filter)); @@ -4531,7 +4531,7 @@ void PeerManagerImpl::ProcessMessage( bool bad = false; if (vData.size() > MAX_SCRIPT_ELEMENT_SIZE) { bad = true; - } else if (RelayAddrsWithPeer(*peer)) { + } else if (!pfrom.IsBlockOnlyConn()) { LOCK(pfrom.m_tx_relay->cs_filter); if (pfrom.m_tx_relay->pfilter) { pfrom.m_tx_relay->pfilter->insert(vData); @@ -4551,7 +4551,7 @@ void PeerManagerImpl::ProcessMessage( pfrom.fDisconnect = true; return; } - if (!RelayAddrsWithPeer(*peer)) { + if (pfrom.IsBlockOnlyConn()) { return; } LOCK(pfrom.m_tx_relay->cs_filter); @@ -5336,7 +5336,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) LOCK2(m_mempool.cs, peer->m_block_inv_mutex); size_t reserve = INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000; - if (RelayAddrsWithPeer(*peer)) { + if (!pto->IsBlockOnlyConn()) { LOCK(pto->m_tx_relay->cs_tx_inventory); reserve = std::min(pto->m_tx_relay->setInventoryTxToSend.size(), reserve); } @@ -5367,7 +5367,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) } }; - if (RelayAddrsWithPeer(*peer)) { + if (!pto->IsBlockOnlyConn()) { LOCK(pto->m_tx_relay->cs_tx_inventory); // Check whether periodic sends should happen // Note: If this node is running in a Masternode mode, it makes no sense to delay outgoing txes From 18fe76598814726996f135635aafa8d8ed663007 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 12 Apr 2024 16:34:32 +0000 Subject: [PATCH 07/13] merge bitcoin#21528: Reduce addr blackholes --- src/net.h | 3 +- src/net_processing.cpp | 86 ++++++++++++++++++++------- src/net_processing.h | 1 + src/rpc/net.cpp | 2 + test/functional/p2p_addr_relay.py | 81 +++++++++++++++++++++++-- test/functional/test_framework/p2p.py | 1 + 6 files changed, 147 insertions(+), 27 deletions(-) diff --git a/src/net.h b/src/net.h index dde7a0e7277a0..6dc77b830b4f6 100644 --- a/src/net.h +++ b/src/net.h @@ -603,7 +603,8 @@ class CNode }; // in bitcoin: m_tx_relay == nullptr if we're not relaying transactions with this peer - // in dash: m_tx_relay should never be nullptr, use `!IsBlockOnlyConn() == false` instead + // in dash: m_tx_relay should never be nullptr, we don't relay transactions if + // `IsBlockOnlyConn() == true` is instead std::unique_ptr m_tx_relay{std::make_unique()}; /** UNIX epoch time of the last block received from this peer that we had diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3bf2041789e74..45e94b9c6847d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -264,9 +264,31 @@ struct Peer { /** A vector of addresses to send to the peer, limited to MAX_ADDR_TO_SEND. */ std::vector m_addrs_to_send; - /** Probabilistic filter of addresses that this peer already knows. - * Used to avoid relaying addresses to this peer more than once. */ - const std::unique_ptr m_addr_known; + /** Probabilistic filter to track recent addr messages relayed with this + * peer. Used to avoid relaying redundant addresses to this peer. + * + * We initialize this filter for outbound peers (other than + * block-relay-only connections) or when an inbound peer sends us an + * address related message (ADDR, ADDRV2, GETADDR). + * + * Presence of this filter must correlate with m_addr_relay_enabled. + **/ + std::unique_ptr m_addr_known; + /** Whether we are participating in address relay with this connection. + * + * We set this bool to true for outbound peers (other than + * block-relay-only connections), or when an inbound peer sends us an + * address related message (ADDR, ADDRV2, GETADDR). + * + * We use this bool to decide whether a peer is eligible for gossiping + * addr messages. This avoids relaying to peers that are unlikely to + * forward them, effectively blackholing self announcements. Reasons + * peers might support addr relay on the link include that they connected + * to us as a block-relay-only peer or they are a light client. + * + * This field must correlate with whether m_addr_known has been + * initialized.*/ + std::atomic_bool m_addr_relay_enabled{false}; /** Whether a getaddr request to this peer is outstanding. */ bool m_getaddr_sent{false}; /** Guards address sending timers. */ @@ -298,9 +320,8 @@ struct Peer { /** Work queue of items requested by this peer **/ std::deque m_getdata_requests GUARDED_BY(m_getdata_requests_mutex); - explicit Peer(NodeId id, bool addr_relay) + explicit Peer(NodeId id) : m_id(id) - , m_addr_known{addr_relay ? std::make_unique(5000, 0.001) : nullptr} {} }; @@ -523,6 +544,14 @@ class PeerManagerImpl final : public PeerManager */ void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv); + /** Checks if address relay is permitted with peer. If needed, initializes + * the m_addr_known bloom filter and sets m_addr_relay_enabled to true. + * + * @return True if address relay is enabled with peer + * False if address relay is disallowed + */ + bool SetupAddressRelay(CNode& node, Peer& peer); + /** Number of nodes with fSyncStarted. */ int nSyncStarted GUARDED_BY(cs_main) = 0; @@ -852,11 +881,6 @@ static CNodeState *State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return &it->second; } -static bool RelayAddrsWithPeer(const Peer& peer) -{ - return peer.m_addr_known != nullptr; -} - /** * Whether the peer supports the address. For example, a peer that does not * implement BIP155 cannot receive Tor v3 addresses because it requires @@ -1330,9 +1354,7 @@ void PeerManagerImpl::InitializeNode(CNode *pnode) { mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, pnode->IsInboundConn())); } { - // Addr relay is disabled for outbound block-relay-only peers to - // prevent adversaries from inferring these links from addr traffic. - PeerRef peer = std::make_shared(nodeid, /* addr_relay = */ !pnode->IsBlockOnlyConn()); + PeerRef peer = std::make_shared(nodeid); LOCK(m_peer_mutex); m_peer_map.emplace_hint(m_peer_map.end(), nodeid, std::move(peer)); } @@ -1465,6 +1487,7 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats) stats.m_ping_wait = ping_wait; stats.m_addr_processed = peer->m_addr_processed.load(); stats.m_addr_rate_limited = peer->m_addr_rate_limited.load(); + stats.m_addr_relay_enabled = peer->m_addr_relay_enabled.load(); return true; } @@ -1654,7 +1677,7 @@ bool PeerManagerImpl::CanRelayAddrs(NodeId pnode) const PeerRef peer = GetPeerRef(pnode); if (peer == nullptr) return false; - return RelayAddrsWithPeer(*peer); + return peer->m_addr_relay_enabled; } bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state, @@ -2128,7 +2151,7 @@ void PeerManagerImpl::RelayAddress(NodeId originator, LOCK(m_peer_mutex); for (auto& [id, peer] : m_peer_map) { - if (RelayAddrsWithPeer(*peer) && id != originator && IsAddrCompatible(*peer, addr)) { + if (peer->m_addr_relay_enabled && id != originator && IsAddrCompatible(*peer, addr)) { uint64_t hashKey = CSipHasher(hasher).Write(id).Finalize(); for (unsigned int i = 0; i < nRelayNodes; i++) { if (hashKey > best[i].first) { @@ -2354,7 +2377,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic } ++it; - if (!RelayAddrsWithPeer(peer) && NetMessageViolatesBlocksOnly(inv.GetCommand())) { + if (!peer.m_addr_relay_enabled && NetMessageViolatesBlocksOnly(inv.GetCommand())) { // Note that if we receive a getdata for non-block messages // from a block-relay-only outbound peer that violate the policy, // we skip such getdata messages from this peer @@ -3208,7 +3231,8 @@ void PeerManagerImpl::ProcessMessage( UpdatePreferredDownload(pfrom, State(pfrom.GetId())); } - if (!pfrom.IsInboundConn() && !pfrom.IsBlockOnlyConn()) { + // Self advertisement & GETADDR logic + if (!pfrom.IsInboundConn() && SetupAddressRelay(pfrom, *peer)) { // For outbound peers, we try to relay our address (so that other // nodes can try to find us more quickly, as we have no guarantee // that an outbound peer is even aware of how to reach us) and do a @@ -3217,8 +3241,9 @@ void PeerManagerImpl::ProcessMessage( // empty and no one will know who we are, so these mechanisms are // important to help us connect to the network. // - // We skip this for block-relay-only peers to avoid potentially leaking - // information about our block-relay-only connections via address relay. + // We skip this for block-relay-only peers. We want to avoid + // potentially leaking addr information and we do not want to + // indicate to the peer that we will participate in addr relay. if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) { CAddress addr = GetLocalAddress(&pfrom.addr, pfrom.GetLocalServices()); @@ -3394,10 +3419,11 @@ void PeerManagerImpl::ProcessMessage( s >> vAddr; - if (!RelayAddrsWithPeer(*peer)) { + if (!SetupAddressRelay(pfrom, *peer)) { LogPrint(BCLog::NET, "ignoring %s message from %s peer=%d\n", msg_type, pfrom.ConnectionTypeAsString(), pfrom.GetId()); return; } + if (vAddr.size() > MAX_ADDR_TO_SEND) { Misbehaving(pfrom.GetId(), 20, strprintf("%s message size = %u", msg_type, vAddr.size())); @@ -4371,6 +4397,8 @@ void PeerManagerImpl::ProcessMessage( return; } + SetupAddressRelay(pfrom, *peer); + // Only send one GetAddr response per connection to reduce resource waste // and discourage addr stamping of INV announcements. if (peer->m_getaddr_recvd) { @@ -5025,7 +5053,7 @@ void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::mic void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::microseconds current_time) { // Nothing to do for non-address-relay peers - if (!RelayAddrsWithPeer(peer)) return; + if (!peer.m_addr_relay_enabled) return; LOCK(peer.m_addr_send_times_mutex); // Periodically advertise our local address to the peer. @@ -5108,6 +5136,22 @@ class CompareInvMempoolOrder }; } +bool PeerManagerImpl::SetupAddressRelay(CNode& node, Peer& peer) +{ + // We don't participate in addr relay with outbound block-relay-only + // connections to prevent providing adversaries with the additional + // information of addr traffic to infer the link. + if (node.IsBlockOnlyConn()) return false; + + if (!peer.m_addr_relay_enabled.exchange(true)) { + // First addr message we have received from the peer, initialize + // m_addr_known + peer.m_addr_known = std::make_unique(5000, 0.001); + } + + return true; +} + bool PeerManagerImpl::SendMessages(CNode* pto) { assert(m_llmq_ctx); diff --git a/src/net_processing.h b/src/net_processing.h index d403ec4052239..34e1a7d2cdb7d 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -43,6 +43,7 @@ struct CNodeStateStats { std::vector vHeightInFlight; uint64_t m_addr_processed = 0; uint64_t m_addr_rate_limited = 0; + bool m_addr_relay_enabled{false}; }; class PeerManager : public CValidationInterface, public NetEventsInterface diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 04f2ae6f1d905..772fe12901204 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -104,6 +104,7 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::STR, "addr", "(host:port) The IP address and port of the peer"}, {RPCResult::Type::STR, "addrbind", "(ip:port) Bind address of the connection to the peer"}, {RPCResult::Type::STR, "addrlocal", "(ip:port) Local address as reported by the peer"}, + {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"}, {RPCResult::Type::STR, "network", "Network (" + Join(GetNetworkNames(/* append_unroutable */ true), ", ") + ")"}, {RPCResult::Type::STR, "mapped_as", "The AS in the BGP route to the peer used for diversifying peer selection"}, {RPCResult::Type::STR_HEX, "services", "The services offered"}, @@ -192,6 +193,7 @@ static RPCHelpMan getpeerinfo() if (!(stats.addrLocal.empty())) { obj.pushKV("addrlocal", stats.addrLocal); } + obj.pushKV("addr_relay_enabled", statestats.m_addr_relay_enabled); obj.pushKV("network", GetNetworkName(stats.m_network)); if (stats.m_mapped_as != 0) { obj.pushKV("mapped_as", uint64_t(stats.m_mapped_as)); diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index 306e16b55d41c..e34086f743aaf 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -10,14 +10,15 @@ CAddress, NODE_NETWORK, msg_addr, - msg_getaddr + msg_getaddr, + msg_verack ) from test_framework.p2p import ( P2PInterface, p2p_lock, ) from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal +from test_framework.util import assert_equal, assert_greater_than import random @@ -25,10 +26,12 @@ class AddrReceiver(P2PInterface): num_ipv4_received = 0 test_addr_contents = False _tokens = 1 + send_getaddr = True - def __init__(self, test_addr_contents=False): + def __init__(self, test_addr_contents=False, send_getaddr=True): super().__init__() self.test_addr_contents = test_addr_contents + self.send_getaddr = send_getaddr def on_addr(self, message): for addr in message.addrs: @@ -58,6 +61,11 @@ def increment_tokens(self, n): def addr_received(self): return self.num_ipv4_received != 0 + def on_version(self, message): + self.send_message(msg_verack()) + if (self.send_getaddr): + self.send_message(msg_getaddr()) + def getaddr_received(self): return self.message_count['getaddr'] > 0 @@ -72,6 +80,10 @@ def set_test_params(self): def run_test(self): self.oversized_addr_test() self.relay_tests() + self.inbound_blackhole_tests() + + # This test populates the addrman, which can impact the node's behavior + # in subsequent tests self.getaddr_tests() self.blocksonly_mode_tests() self.rate_limit_tests() @@ -152,7 +164,7 @@ def relay_tests(self): self.nodes[0].disconnect_p2ps() self.log.info('Check relay of addresses received from outbound peers') - inbound_peer = self.nodes[0].add_p2p_connection(AddrReceiver(test_addr_contents=True)) + inbound_peer = self.nodes[0].add_p2p_connection(AddrReceiver(test_addr_contents=True, send_getaddr=False)) full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type="outbound-full-relay") msg = self.setup_addr_msg(2) self.send_addr_msg(full_outbound_peer, msg, [inbound_peer]) @@ -163,6 +175,9 @@ def relay_tests(self): # of the outbound peer which is often sent before the GETADDR response. assert_equal(inbound_peer.num_ipv4_received, 0) + # Send an empty ADDR message to intialize address relay on this connection. + inbound_peer.send_and_ping(msg_addr()) + self.log.info('Check that subsequent addr messages sent from an outbound peer are relayed') msg2 = self.setup_addr_msg(2) self.send_addr_msg(full_outbound_peer, msg2, [inbound_peer]) @@ -180,7 +195,63 @@ def relay_tests(self): self.nodes[0].disconnect_p2ps() + def sum_addr_messages(self, msgs_dict): + return sum(bytes_received for (msg, bytes_received) in msgs_dict.items() if msg in ['addr', 'addrv2', 'getaddr']) + + def inbound_blackhole_tests(self): + self.log.info('Check that we only relay addresses to inbound peers who have previously sent us addr related messages') + + addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) + receiver_peer = self.nodes[0].add_p2p_connection(AddrReceiver()) + blackhole_peer = self.nodes[0].add_p2p_connection(AddrReceiver(send_getaddr=False)) + initial_addrs_received = receiver_peer.num_ipv4_received + + peerinfo = self.nodes[0].getpeerinfo() + assert_equal(peerinfo[0]['addr_relay_enabled'], True) # addr_source + assert_equal(peerinfo[1]['addr_relay_enabled'], True) # receiver_peer + assert_equal(peerinfo[2]['addr_relay_enabled'], False) # blackhole_peer + + # addr_source sends 2 addresses to node0 + msg = self.setup_addr_msg(2) + addr_source.send_and_ping(msg) + self.bump_mocktime(30 * 60) + receiver_peer.sync_with_ping() + blackhole_peer.sync_with_ping() + + peerinfo = self.nodes[0].getpeerinfo() + + # Confirm node received addr-related messages from receiver peer + assert_greater_than(self.sum_addr_messages(peerinfo[1]['bytesrecv_per_msg']), 0) + # And that peer received addresses + assert_equal(receiver_peer.num_ipv4_received - initial_addrs_received, 2) + + # Confirm node has not received addr-related messages from blackhole peer + assert_equal(self.sum_addr_messages(peerinfo[2]['bytesrecv_per_msg']), 0) + # And that peer did not receive addresses + assert_equal(blackhole_peer.num_ipv4_received, 0) + + self.log.info("After blackhole peer sends addr message, it becomes eligible for addr gossip") + blackhole_peer.send_and_ping(msg_addr()) + + # Confirm node has now received addr-related messages from blackhole peer + assert_greater_than(self.sum_addr_messages(peerinfo[1]['bytesrecv_per_msg']), 0) + assert_equal(self.nodes[0].getpeerinfo()[2]['addr_relay_enabled'], True) + + msg = self.setup_addr_msg(2) + self.send_addr_msg(addr_source, msg, [receiver_peer, blackhole_peer]) + + # And that peer received addresses + assert_equal(blackhole_peer.num_ipv4_received, 2) + + self.nodes[0].disconnect_p2ps() + def getaddr_tests(self): + # In the previous tests, the node answered GETADDR requests with an + # empty addrman. Due to GETADDR response caching (see + # CConnman::GetAddresses), the node would continue to provide 0 addrs + # in response until enough time has passed or the node is restarted. + self.restart_node(0) + self.log.info('Test getaddr behavior') self.log.info('Check that we send a getaddr message upon connecting to an outbound-full-relay peer') full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type="outbound-full-relay") @@ -193,7 +264,7 @@ def getaddr_tests(self): assert_equal(block_relay_peer.getaddr_received(), False) self.log.info('Check that we answer getaddr messages only from inbound peers') - inbound_peer = self.nodes[0].add_p2p_connection(AddrReceiver()) + inbound_peer = self.nodes[0].add_p2p_connection(AddrReceiver(send_getaddr=False)) inbound_peer.sync_with_ping() # Add some addresses to addrman diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index 4e94149a48706..92232d42a4884 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -474,6 +474,7 @@ def on_version(self, message): self.send_message(msg_sendaddrv2()) self.send_message(msg_verack()) self.nServices = message.nServices + self.send_message(msg_getaddr()) # Connection helper methods From 8b8fbc5226893b08d87a69a4fa53b5c4e209614d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 3 Aug 2021 09:25:38 -0700 Subject: [PATCH 08/13] merge bitcoin#22618: Small follow-ups to 21528 --- doc/release-notes-5978.md | 7 +++++++ src/net_processing.cpp | 4 +++- test/functional/p2p_addr_relay.py | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/doc/release-notes-5978.md b/doc/release-notes-5978.md index da532820511cf..aafd5f0818a7e 100644 --- a/doc/release-notes-5978.md +++ b/doc/release-notes-5978.md @@ -6,3 +6,10 @@ RPC changes - `getnodeaddresses` now also accepts a "network" argument (ipv4, ipv6, onion, or i2p) to return only addresses of the specified network. + +P2P and network changes +----------------------- + +- A dashd node will no longer rumour addresses to inbound peers by default. + They will become eligible for address gossip after sending an ADDR, ADDRV2, + or GETADDR message. diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 45e94b9c6847d..99229202773af 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4397,7 +4397,9 @@ void PeerManagerImpl::ProcessMessage( return; } - SetupAddressRelay(pfrom, *peer); + // Since this must be an inbound connection, SetupAddressRelay will + // never fail. + Assume(SetupAddressRelay(pfrom, *peer)); // Only send one GetAddr response per connection to reduce resource waste // and discourage addr stamping of INV announcements. diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index e34086f743aaf..aedb5532ef46d 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -175,7 +175,7 @@ def relay_tests(self): # of the outbound peer which is often sent before the GETADDR response. assert_equal(inbound_peer.num_ipv4_received, 0) - # Send an empty ADDR message to intialize address relay on this connection. + # Send an empty ADDR message to initialize address relay on this connection. inbound_peer.send_and_ping(msg_addr()) self.log.info('Check that subsequent addr messages sent from an outbound peer are relayed') From 60b3e08ed14301ba11c40bfb26482a7ed4fd6085 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 25 Mar 2024 16:04:17 +0000 Subject: [PATCH 09/13] merge bitcoin#22616: address relay fixups --- src/net_processing.cpp | 4 ++-- src/rpc/net.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 99229202773af..a72c07cd584f9 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -550,7 +550,7 @@ class PeerManagerImpl final : public PeerManager * @return True if address relay is enabled with peer * False if address relay is disallowed */ - bool SetupAddressRelay(CNode& node, Peer& peer); + bool SetupAddressRelay(const CNode& node, Peer& peer); /** Number of nodes with fSyncStarted. */ int nSyncStarted GUARDED_BY(cs_main) = 0; @@ -5138,7 +5138,7 @@ class CompareInvMempoolOrder }; } -bool PeerManagerImpl::SetupAddressRelay(CNode& node, Peer& peer) +bool PeerManagerImpl::SetupAddressRelay(const CNode& node, Peer& peer) { // We don't participate in addr relay with outbound block-relay-only // connections to prevent providing adversaries with the additional diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 772fe12901204..053ccd7c540ee 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -104,7 +104,6 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::STR, "addr", "(host:port) The IP address and port of the peer"}, {RPCResult::Type::STR, "addrbind", "(ip:port) Bind address of the connection to the peer"}, {RPCResult::Type::STR, "addrlocal", "(ip:port) Local address as reported by the peer"}, - {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"}, {RPCResult::Type::STR, "network", "Network (" + Join(GetNetworkNames(/* append_unroutable */ true), ", ") + ")"}, {RPCResult::Type::STR, "mapped_as", "The AS in the BGP route to the peer used for diversifying peer selection"}, {RPCResult::Type::STR_HEX, "services", "The services offered"}, @@ -146,6 +145,7 @@ static RPCHelpMan getpeerinfo() { {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"}, }}, + {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"}, {RPCResult::Type::BOOL, "whitelisted", "Whether the peer is whitelisted"}, {RPCResult::Type::ARR, "permissions", "Any special permissions that have been granted to this peer", { @@ -193,7 +193,6 @@ static RPCHelpMan getpeerinfo() if (!(stats.addrLocal.empty())) { obj.pushKV("addrlocal", stats.addrLocal); } - obj.pushKV("addr_relay_enabled", statestats.m_addr_relay_enabled); obj.pushKV("network", GetNetworkName(stats.m_network)); if (stats.m_mapped_as != 0) { obj.pushKV("mapped_as", uint64_t(stats.m_mapped_as)); @@ -245,6 +244,7 @@ static RPCHelpMan getpeerinfo() heights.push_back(height); } obj.pushKV("inflight", heights); + obj.pushKV("addr_relay_enabled", statestats.m_addr_relay_enabled); obj.pushKV("addr_processed", statestats.m_addr_processed); obj.pushKV("addr_rate_limited", statestats.m_addr_rate_limited); } From 06e909b7376cbb731e09d875d7a525d00a37ad99 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 31 Jul 2021 21:56:59 +0200 Subject: [PATCH 10/13] merge bitcoin#22604: address rate-limiting follow-ups --- src/net_processing.cpp | 19 ++++++++----------- src/rpc/net.cpp | 2 ++ test/functional/p2p_addr_relay.py | 22 +++++++++++----------- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a72c07cd584f9..3b9260e3da8c7 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -181,7 +181,7 @@ static constexpr size_t MAX_ADDR_TO_SEND{1000}; static constexpr double MAX_ADDR_RATE_PER_SECOND{0.1}; /** The soft limit of the address processing token bucket (the regular MAX_ADDR_RATE_PER_SECOND * based increments won't go above this, but the MAX_ADDR_TO_SEND increment following GETADDR - * is exempt from this limit. */ + * is exempt from this limit). */ static constexpr size_t MAX_ADDR_PROCESSING_TOKEN_BUCKET{MAX_ADDR_TO_SEND}; struct COrphanTx { @@ -302,14 +302,14 @@ struct Peer { std::atomic_bool m_wants_addrv2{false}; /** Whether this peer has already sent us a getaddr message. */ bool m_getaddr_recvd{false}; - /** Number of addr messages that can be processed from this peer. Start at 1 to + /** Number of addresses that can be processed from this peer. Start at 1 to * permit self-announcement. */ double m_addr_token_bucket{1.0}; /** When m_addr_token_bucket was last updated */ std::chrono::microseconds m_addr_token_timestamp{GetTime()}; /** Total number of addresses that were dropped due to rate limiting. */ std::atomic m_addr_rate_limited{0}; - /** Total number of addresses that were processed (excludes rate limited ones). */ + /** Total number of addresses that were processed (excludes rate-limited ones). */ std::atomic m_addr_processed{0}; /** Set of txids to reconsider once their parent transactions have been accepted **/ @@ -3455,11 +3455,12 @@ void PeerManagerImpl::ProcessMessage( return; // Apply rate limiting. - if (rate_limited) { - if (peer->m_addr_token_bucket < 1.0) { + if (peer->m_addr_token_bucket < 1.0) { + if (rate_limited) { ++num_rate_limit; continue; } + } else { peer->m_addr_token_bucket -= 1.0; } // We only bother storing full nodes, though this may include @@ -3487,12 +3488,8 @@ void PeerManagerImpl::ProcessMessage( } peer->m_addr_processed += num_proc; peer->m_addr_rate_limited += num_rate_limit; - LogPrint(BCLog::NET, "Received addr: %u addresses (%u processed, %u rate-limited) from peer=%d%s\n", - vAddr.size(), - num_proc, - num_rate_limit, - pfrom.GetId(), - fLogIPs ? ", peeraddr=" + pfrom.addr.ToString() : ""); + LogPrint(BCLog::NET, "Received addr: %u addresses (%u processed, %u rate-limited) from peer=%d\n", + vAddr.size(), num_proc, num_rate_limit, pfrom.GetId()); m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60); if (vAddr.size() < 1000) peer->m_getaddr_sent = false; diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 053ccd7c540ee..da3a720f10eb5 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -146,6 +146,8 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"}, }}, {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"}, + {RPCResult::Type::NUM, "addr_processed", "The total number of addresses processed, excluding those dropped due to rate limiting"}, + {RPCResult::Type::NUM, "addr_rate_limited", "The total number of addresses dropped due to rate limiting"}, {RPCResult::Type::BOOL, "whitelisted", "Whether the peer is whitelisted"}, {RPCResult::Type::ARR, "permissions", "Any special permissions that have been granted to this peer", { diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index aedb5532ef46d..bd8d08b78cfe0 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -306,7 +306,7 @@ def blocksonly_mode_tests(self): self.nodes[0].disconnect_p2ps() - def send_addrs_and_test_rate_limiting(self, peer, no_relay, new_addrs, total_addrs): + def send_addrs_and_test_rate_limiting(self, peer, no_relay, *, new_addrs, total_addrs): """Send an addr message and check that the number of addresses processed and rate-limited is as expected""" peer.send_and_ping(self.setup_rand_addr_msg(new_addrs)) @@ -324,41 +324,41 @@ def send_addrs_and_test_rate_limiting(self, peer, no_relay, new_addrs, total_add assert_equal(addrs_rate_limited, max(0, total_addrs - peer.tokens)) def rate_limit_tests(self): - self.restart_node(0, []) - for contype, no_relay in [("outbound-full-relay", False), ("block-relay-only", True), ("inbound", False)]: - self.log.info(f'Test rate limiting of addr processing for {contype} peers') - if contype == "inbound": + for conn_type, no_relay in [("outbound-full-relay", False), ("block-relay-only", True), ("inbound", False)]: + self.log.info(f'Test rate limiting of addr processing for {conn_type} peers') + if conn_type == "inbound": peer = self.nodes[0].add_p2p_connection(AddrReceiver()) else: - peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type=contype) + peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type=conn_type) # Send 600 addresses. For all but the block-relay-only peer this should result in addresses being processed. - self.send_addrs_and_test_rate_limiting(peer, no_relay, 600, 600) + self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=600, total_addrs=600) # Send 600 more addresses. For the outbound-full-relay peer (which we send a GETADDR, and thus will # process up to 1001 incoming addresses), this means more addresses will be processed. - self.send_addrs_and_test_rate_limiting(peer, no_relay, 600, 1200) + self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=600, total_addrs=1200) # Send 10 more. As we reached the processing limit for all nodes, no more addresses should be procesesd. - self.send_addrs_and_test_rate_limiting(peer, no_relay, 10, 1210) + self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=10, total_addrs=1210) # Advance the time by 100 seconds, permitting the processing of 10 more addresses. # Send 200 and verify that 10 are processed. self.bump_mocktime(100) peer.increment_tokens(10) - self.send_addrs_and_test_rate_limiting(peer, no_relay, 200, 1410) + self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=200, total_addrs=1410) # Advance the time by 1000 seconds, permitting the processing of 100 more addresses. # Send 200 and verify that 100 are processed. self.bump_mocktime(1000) peer.increment_tokens(100) - self.send_addrs_and_test_rate_limiting(peer, no_relay, 200, 1610) + self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=200, total_addrs=1610) self.nodes[0].disconnect_p2ps() + if __name__ == '__main__': AddrTest().main() From 45d9e58023f4f0022a4fd59d84bedea1a7606b70 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 25 Mar 2024 11:05:21 +0000 Subject: [PATCH 11/13] merge bitcoin#22960: Set peertimeout in write_config --- test/functional/feature_bip68_sequence.py | 5 +---- test/functional/feature_maxuploadtarget.py | 1 - test/functional/test_framework/util.py | 5 +++++ 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/test/functional/feature_bip68_sequence.py b/test/functional/feature_bip68_sequence.py index b6d28afee4faa..079c9b787d47a 100755 --- a/test/functional/feature_bip68_sequence.py +++ b/test/functional/feature_bip68_sequence.py @@ -38,10 +38,7 @@ class BIP68Test(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 2 self.extra_args = [ - [ - "-acceptnonstdtxn=1", - "-peertimeout=9999", # bump because mocktime might cause a disconnect otherwise - ], + ["-acceptnonstdtxn=1"], ["-acceptnonstdtxn=0"], ] diff --git a/test/functional/feature_maxuploadtarget.py b/test/functional/feature_maxuploadtarget.py index e4aff0fcbdf1b..eef54463e52d9 100755 --- a/test/functional/feature_maxuploadtarget.py +++ b/test/functional/feature_maxuploadtarget.py @@ -38,7 +38,6 @@ def set_test_params(self): self.extra_args = [[ "-maxuploadtarget=200", "-blockmaxsize=999000", - "-peertimeout=9999", # bump because mocktime might cause a disconnect otherwise "-maxtipage="+str(2*60*60*24*7), "-acceptnonstdtxn=1" ]] diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index d435eecf1ca72..f36450ba8dd00 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -373,6 +373,11 @@ def write_config(config_path, *, n, chain, extra_config=""): f.write("dnsseed=0\n") f.write("fixedseeds=0\n") f.write("listenonion=0\n") + # Increase peertimeout to avoid disconnects while using mocktime. + # peertimeout is measured in wall clock time, so setting it to the + # duration of the longest test is sufficient. It can be overriden in + # tests. + f.write("peertimeout=999999\n") f.write("printtoconsole=0\n") f.write("upnp=0\n") f.write("natpmp=0\n") From 022b76f20b0f57c2993a00f6e95f91f84f611f14 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 25 Mar 2024 10:50:27 +0000 Subject: [PATCH 12/13] merge bitcoin#23218: Use mocktime for ping timeout --- src/net.cpp | 3 +-- src/net.h | 2 +- src/net_processing.cpp | 5 ++++- src/test/denialofservice_tests.cpp | 2 ++ src/test/util/net.h | 6 ++++++ test/functional/p2p_ping.py | 14 +++++++++++--- 6 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 59e15169e70f8..78b28003eda47 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1491,9 +1491,8 @@ void CConnman::CalculateNumConnectionsChangedStats() statsClient.gauge("peers.torConnections", torNodes, 1.0f); } -bool CConnman::ShouldRunInactivityChecks(const CNode& node, std::optional now_in) const +bool CConnman::ShouldRunInactivityChecks(const CNode& node, int64_t now) const { - const int64_t now = now_in ? now_in.value() : GetTimeSeconds(); return node.nTimeConnected + m_peer_connect_timeout < now; } diff --git a/src/net.h b/src/net.h index 6dc77b830b4f6..acbc3c3ad4b7f 100644 --- a/src/net.h +++ b/src/net.h @@ -1237,7 +1237,7 @@ friend class CNode; void SetAsmap(std::vector asmap) { addrman.m_asmap = std::move(asmap); } /** Return true if we should disconnect the peer for failing an inactivity check. */ - bool ShouldRunInactivityChecks(const CNode& node, std::optional now=std::nullopt) const; + bool ShouldRunInactivityChecks(const CNode& node, int64_t secs_now) const; private: struct ListenSocket { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3b9260e3da8c7..41cd78c329247 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5017,8 +5017,11 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers() void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now) { - if (m_connman.ShouldRunInactivityChecks(node_to) && peer.m_ping_nonce_sent && + if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast(now).count()) && + peer.m_ping_nonce_sent && now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) { + // The ping timeout is using mocktime. To disable the check during + // testing, increase -peertimeout. LogPrint(BCLog::NET, "ping timeout: %fs peer=%d\n", 0.000001 * count_microseconds(now - peer.m_ping_start.load()), peer.m_id); node_to.fDisconnect = true; return; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 0ae1ad5c316cd..1fc2330cf8d0f 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -64,6 +64,8 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) { const CChainParams& chainparams = Params(); auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); + // Disable inactivity checks for this test to avoid interference + static_cast(connman.get())->SetPeerConnectTimeout(99999); auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr, *m_node.scheduler, *m_node.chainman, *m_node.mempool, *m_node.govman, *m_node.sporkman, ::deterministicMNManager, m_node.cj_ctx, m_node.llmq_ctx, false); diff --git a/src/test/util/net.h b/src/test/util/net.h index c6cebed8def1e..74fd43970f40c 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -15,6 +15,12 @@ struct ConnmanTestMsg : public CConnman { using CConnman::CConnman; + + void SetPeerConnectTimeout(int64_t timeout) + { + m_peer_connect_timeout = timeout; + } + void AddTestNode(CNode& node) { LOCK(cs_vNodes); diff --git a/test/functional/p2p_ping.py b/test/functional/p2p_ping.py index 73ac7807415b6..b43f316faafd0 100755 --- a/test/functional/p2p_ping.py +++ b/test/functional/p2p_ping.py @@ -30,11 +30,16 @@ def on_ping(self, message): pass +TIMEOUT_INTERVAL = 20 * 60 + + class PingPongTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 1 - self.extra_args = [['-peertimeout=3']] + # Set the peer connection timeout low. It does not matter for this + # test, as long as it is less than TIMEOUT_INTERVAL. + self.extra_args = [['-peertimeout=1']] def check_peer_info(self, *, pingtime, minping, pingwait): stats = self.nodes[0].getpeerinfo()[0] @@ -114,8 +119,11 @@ def run_test(self): self.nodes[0].ping() no_pong_node.wait_until(lambda: 'ping' in no_pong_node.last_message) with self.nodes[0].assert_debug_log(['ping timeout: 1201.000000s']): - self.mock_forward(20 * 60 + 1) - time.sleep(4) # peertimeout + 1 + self.mock_forward(TIMEOUT_INTERVAL // 2) + # Check that sending a ping does not prevent the disconnect + no_pong_node.sync_with_ping() + self.mock_forward(TIMEOUT_INTERVAL // 2 + 1) + no_pong_node.wait_for_disconnect() if __name__ == '__main__': From 1fedf470cd821f23efd6ea896ebbfab6a3e4b951 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 11 Apr 2024 17:49:35 +0000 Subject: [PATCH 13/13] test: add type annotation for `ADDRS` in `p2p_addrv2_relay` Required to avoid unhappy python linter[1] result. Have to use annotation instead of re-aligning with upstream (where ADDRS is populated in the global state) due to reliance on `self.mocktime`, without which, the test fails[2] [1] - https://gitlab.com/dashpay/dash/-/jobs/6594035886 [2] - https://gitlab.com/dashpay/dash/-/jobs/6597322548 --- test/functional/p2p_addrv2_relay.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/functional/p2p_addrv2_relay.py b/test/functional/p2p_addrv2_relay.py index df083dbcca3eb..0e5530259eb98 100755 --- a/test/functional/p2p_addrv2_relay.py +++ b/test/functional/p2p_addrv2_relay.py @@ -6,6 +6,8 @@ Test addrv2 relay """ +from typing import List + from test_framework.messages import ( CAddress, msg_addrv2, @@ -17,7 +19,7 @@ I2P_ADDR = "c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p" -ADDRS = [] +ADDRS: List[CAddress] = [] class AddrReceiver(P2PInterface):