From 6efbcec4ded6116a42d2783c96c60ef0f255a1b2 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 3 Jun 2021 16:38:59 -0400 Subject: [PATCH 1/2] Protect last outbound HB compact block peer If all our high-bandwidth compact block serving peers (BIP 152) stall block download, then we can be denied a block for (potentially) a long time. As inbound connections are much more likely to be adversarial than outbound connections, mitigate this risk by never removing our last outbound HB peer if it would be replaced by an inbound. --- src/net_processing.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 65224b4259..2f2614a11a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -825,12 +825,27 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) return; } if (nodestate->fProvidesHeaderAndIDs) { + int num_outbound_hb_peers = 0; for (std::list::iterator it = lNodesAnnouncingHeaderAndIDs.begin(); it != lNodesAnnouncingHeaderAndIDs.end(); it++) { if (*it == nodeid) { lNodesAnnouncingHeaderAndIDs.erase(it); lNodesAnnouncingHeaderAndIDs.push_back(nodeid); return; } + CNodeState *state = State(*it); + if (state != nullptr && !state->m_is_inbound) ++num_outbound_hb_peers; + } + if (nodestate->m_is_inbound) { + // If we're adding an inbound HB peer, make sure we're not removing + // our last outbound HB peer in the process. + if (lNodesAnnouncingHeaderAndIDs.size() >= 3 && num_outbound_hb_peers == 1) { + CNodeState *remove_node = State(lNodesAnnouncingHeaderAndIDs.front()); + if (remove_node != nullptr && !remove_node->m_is_inbound) { + // Put the HB outbound peer in the second slot, so that it + // doesn't get removed. + std::swap(lNodesAnnouncingHeaderAndIDs.front(), *std::next(lNodesAnnouncingHeaderAndIDs.begin())); + } + } } m_connman.ForNode(nodeid, [this](CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); From 30aee2dfe671b347438c1c327c6f79edfacff1ce Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 8 Jun 2021 18:03:46 -0700 Subject: [PATCH 2/2] tests: Add test for compact block HB selection --- test/functional/p2p_compactblocks_hb.py | 97 +++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 98 insertions(+) create mode 100755 test/functional/p2p_compactblocks_hb.py diff --git a/test/functional/p2p_compactblocks_hb.py b/test/functional/p2p_compactblocks_hb.py new file mode 100755 index 0000000000..a3d30a6f04 --- /dev/null +++ b/test/functional/p2p_compactblocks_hb.py @@ -0,0 +1,97 @@ +#!/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 compact blocks HB selection logic.""" + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal + + +class CompactBlocksConnectionTest(BitcoinTestFramework): + """Test class for verifying selection of HB peer connections.""" + + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 6 + + def peer_info(self, from_node, to_node): + """Query from_node for its getpeerinfo about to_node.""" + for peerinfo in self.nodes[from_node].getpeerinfo(): + if "(testnode%i)" % to_node in peerinfo['subver']: + return peerinfo + return None + + def setup_network(self): + self.setup_nodes() + # Start network with everyone disconnected + self.sync_all() + + def relay_block_through(self, peer): + """Relay a new block through peer peer, and return HB status between 1 and [2,3,4,5].""" + self.connect_nodes(peer, 0) + self.nodes[0].generate(1) + self.sync_blocks() + self.disconnect_nodes(peer, 0) + status_to = [self.peer_info(1, i)['bip152_hb_to'] for i in range(2, 6)] + status_from = [self.peer_info(i, 1)['bip152_hb_from'] for i in range(2, 6)] + assert_equal(status_to, status_from) + return status_to + + def run_test(self): + self.log.info("Testing reserved high-bandwidth mode slot for outbound peer...") + + # Connect everyone to node 0, and mine some blocks to get all nodes out of IBD. + for i in range(1, 6): + self.connect_nodes(i, 0) + self.nodes[0].generate(2) + self.sync_blocks() + for i in range(1, 6): + self.disconnect_nodes(i, 0) + + # Construct network topology: + # - Node 0 is the block producer + # - Node 1 is the "target" node being tested + # - Nodes 2-5 are intermediaries. + # - Node 1 has an outbound connection to node 2 + # - Node 1 has inbound connections from nodes 3-5 + self.connect_nodes(3, 1) + self.connect_nodes(4, 1) + self.connect_nodes(5, 1) + self.connect_nodes(1, 2) + + # Mine blocks subsequently relaying through nodes 3,4,5 (inbound to node 1) + for nodeid in range(3, 6): + status = self.relay_block_through(nodeid) + assert_equal(status, [False, nodeid >= 3, nodeid >= 4, nodeid >= 5]) + + # And again through each. This should not change HB status. + for nodeid in range(3, 6): + status = self.relay_block_through(nodeid) + assert_equal(status, [False, True, True, True]) + + # Now relay one block through peer 2 (outbound from node 1), so it should take HB status + # from one of the inbounds. + status = self.relay_block_through(2) + assert_equal(status[0], True) + assert_equal(sum(status), 3) + + # Now relay again through nodes 3,4,5. Since 2 is outbound, it should remain HB. + for nodeid in range(3, 6): + status = self.relay_block_through(nodeid) + assert status[0] + assert status[nodeid - 2] + assert_equal(sum(status), 3) + + # Reconnect peer 2, and retry. Now the three inbounds should be HB again. + self.disconnect_nodes(1, 2) + self.connect_nodes(1, 2) + for nodeid in range(3, 6): + status = self.relay_block_through(nodeid) + assert not status[0] + assert status[nodeid - 2] + assert_equal(status, [False, True, True, True]) + + +if __name__ == '__main__': + CompactBlocksConnectionTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index cb0c83069f..d009223f12 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -173,6 +173,7 @@ BASE_SCRIPTS = [ 'wallet_groups.py --legacy-wallet', 'p2p_addrv2_relay.py', 'wallet_groups.py --descriptors', + 'p2p_compactblocks_hb.py', 'p2p_disconnect_ban.py', 'rpc_decodescript.py', 'rpc_blockchain.py',