From b6c5d1e450dde6a54bd785504c923adfb45c7060 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 28 May 2021 16:06:27 +0200 Subject: [PATCH 1/4] p2p: AddrFetch - don't disconnect on self-announcements Disconnecting an AddrFetch peer only after receiving an addr message of size >1 prevents dropping them before they had a chance to answer the getaddr request. --- src/net_processing.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0b83f756b3..51d628773c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2779,7 +2779,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60); if (vAddr.size() < 1000) peer->m_getaddr_sent = false; - if (pfrom.IsAddrFetchConn()) { + + // AddrFetch: Require multiple addresses to avoid disconnecting on self-announcements + if (pfrom.IsAddrFetchConn() && vAddr.size() > 1) { LogPrint(BCLog::NET, "addrfetch connection completed peer=%d; disconnecting\n", pfrom.GetId()); pfrom.fDisconnect = true; } From 533500d9072b7d5a36a6491784bdeb9247e91fb0 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Sat, 29 May 2021 23:58:37 +0200 Subject: [PATCH 2/4] p2p: Add timeout for AddrFetch peers If AddrFetch peers don't send us addresses, disconnect them after a while. --- src/net_processing.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 51d628773c..702a235993 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4364,6 +4364,12 @@ bool PeerManagerImpl::SendMessages(CNode* pto) const auto current_time = GetTime(); + if (pto->IsAddrFetchConn() && current_time - std::chrono::seconds(pto->nTimeConnected) > 10 * AVG_ADDRESS_BROADCAST_INTERVAL) { + LogPrint(BCLog::NET, "addrfetch connection timeout; disconnecting peer=%d\n", pto->GetId()); + pto->fDisconnect = true; + return true; + } + MaybeSendPing(*pto, *peer, current_time); // MaybeSendPing may have marked peer for disconnection From c34ad3309f93979b274a37de013502b05d25fad8 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 31 May 2021 22:49:42 +0200 Subject: [PATCH 3/4] net, rpc: Enable AddrFetch connections for functional testing Co-authored-by: Amiti Uttarwar --- src/net.cpp | 21 +++++++++++++++++---- src/net.h | 1 + src/rpc/net.cpp | 4 +++- test/functional/test_framework/test_node.py | 5 ++--- 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 1b6f04dead..a43fe9cdf6 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1166,16 +1166,29 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket, bool CConnman::AddConnection(const std::string& address, ConnectionType conn_type) { - if (conn_type != ConnectionType::OUTBOUND_FULL_RELAY && conn_type != ConnectionType::BLOCK_RELAY) return false; - - const int max_connections = conn_type == ConnectionType::OUTBOUND_FULL_RELAY ? m_max_outbound_full_relay : m_max_outbound_block_relay; + std::optional max_connections; + switch (conn_type) { + case ConnectionType::INBOUND: + case ConnectionType::MANUAL: + case ConnectionType::FEELER: + return false; + case ConnectionType::OUTBOUND_FULL_RELAY: + max_connections = m_max_outbound_full_relay; + break; + case ConnectionType::BLOCK_RELAY: + max_connections = m_max_outbound_block_relay; + break; + // no limit for ADDR_FETCH because -seednode has no limit either + case ConnectionType::ADDR_FETCH: + break; + } // no default case, so the compiler can warn about missing cases // Count existing connections int existing_connections = WITH_LOCK(cs_vNodes, return std::count_if(vNodes.begin(), vNodes.end(), [conn_type](CNode* node) { return node->m_conn_type == conn_type; });); // Max connections of specified type already exist - if (existing_connections >= max_connections) return false; + if (max_connections != std::nullopt && existing_connections >= max_connections) return false; // Max total outbound connections already exist CSemaphoreGrant grant(*semOutbound, true); diff --git a/src/net.h b/src/net.h index b43916c55e..3af23d2bd7 100644 --- a/src/net.h +++ b/src/net.h @@ -890,6 +890,7 @@ public: * * @param[in] address Address of node to try connecting to * @param[in] conn_type ConnectionType::OUTBOUND or ConnectionType::BLOCK_RELAY + * or ConnectionType::ADDR_FETCH * @return bool Returns false if there are no available * slots for this connection: * - conn_type not a supported ConnectionType diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index c4d6e3d546..50280915c3 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -337,7 +337,7 @@ static RPCHelpMan addconnection() "\nOpen an outbound connection to a specified node. This RPC is for testing only.\n", { {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP address and port to attempt connecting to."}, - {"connection_type", RPCArg::Type::STR, RPCArg::Optional::NO, "Type of connection to open, either \"outbound-full-relay\" or \"block-relay-only\"."}, + {"connection_type", RPCArg::Type::STR, RPCArg::Optional::NO, "Type of connection to open (\"outbound-full-relay\", \"block-relay-only\" or \"addr-fetch\")."}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -363,6 +363,8 @@ static RPCHelpMan addconnection() conn_type = ConnectionType::OUTBOUND_FULL_RELAY; } else if (conn_type_in == "block-relay-only") { conn_type = ConnectionType::BLOCK_RELAY; + } else if (conn_type_in == "addr-fetch") { + conn_type = ConnectionType::ADDR_FETCH; } else { throw JSONRPCError(RPC_INVALID_PARAMETER, self.ToString()); } diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index c17c16f797..71b8aeb20e 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -557,9 +557,8 @@ class TestNode(): return p2p_conn def add_outbound_p2p_connection(self, p2p_conn, *, p2p_idx, connection_type="outbound-full-relay", **kwargs): - """Add an outbound p2p connection from node. Either - full-relay("outbound-full-relay") or - block-relay-only("block-relay-only") connection. + """Add an outbound p2p connection from node. Must be an + "outbound-full-relay", "block-relay-only" or "addr-fetch" connection. This method adds the p2p connection to the self.p2ps list and returns the connection to the caller. From 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 31 May 2021 22:52:08 +0200 Subject: [PATCH 4/4] test: Add functional test for AddrFetch connections Co-authored-by: Amiti Uttarwar --- test/functional/p2p_addrfetch.py | 62 ++++++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 63 insertions(+) create mode 100755 test/functional/p2p_addrfetch.py diff --git a/test/functional/p2p_addrfetch.py b/test/functional/p2p_addrfetch.py new file mode 100755 index 0000000000..66ee1544a9 --- /dev/null +++ b/test/functional/p2p_addrfetch.py @@ -0,0 +1,62 @@ +#!/usr/bin/env python3 +# Copyright (c) 2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +""" +Test p2p addr-fetch connections +""" + +import time + +from test_framework.messages import msg_addr, CAddress, NODE_NETWORK, NODE_WITNESS +from test_framework.p2p import P2PInterface, p2p_lock +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal + +ADDR = CAddress() +ADDR.time = int(time.time()) +ADDR.nServices = NODE_NETWORK | NODE_WITNESS +ADDR.ip = "192.0.0.8" +ADDR.port = 18444 + + +class P2PAddrFetch(BitcoinTestFramework): + + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 1 + + def run_test(self): + node = self.nodes[0] + self.log.info("Connect to an addr-fetch peer") + peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="addr-fetch") + info = node.getpeerinfo() + assert_equal(len(info), 1) + assert_equal(info[0]['connection_type'], 'addr-fetch') + + self.log.info("Check that we send getaddr but don't try to sync headers with the addr-fetch peer") + peer.sync_send_with_ping() + with p2p_lock: + assert peer.message_count['getaddr'] == 1 + assert peer.message_count['getheaders'] == 0 + + self.log.info("Check that answering the getaddr with a single address does not lead to disconnect") + # This prevents disconnecting on self-announcements + msg = msg_addr() + msg.addrs = [ADDR] + peer.send_and_ping(msg) + assert_equal(len(node.getpeerinfo()), 1) + + self.log.info("Check that answering with larger addr messages leads to disconnect") + msg.addrs = [ADDR] * 2 + peer.send_message(msg) + peer.wait_for_disconnect(timeout=5) + + self.log.info("Check timeout for addr-fetch peer that does not send addrs") + peer = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, connection_type="addr-fetch") + node.setmocktime(int(time.time()) + 301) # Timeout: 5 minutes + peer.wait_for_disconnect(timeout=5) + + +if __name__ == '__main__': + P2PAddrFetch().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 49f269f8b4..4dcd92708c 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -182,6 +182,7 @@ BASE_SCRIPTS = [ 'p2p_addr_relay.py', 'p2p_getaddr_caching.py', 'p2p_getdata.py', + 'p2p_addrfetch.py', 'rpc_net.py', 'wallet_keypool.py --legacy-wallet', 'wallet_keypool.py --descriptors',