From f17e403b4bee6b7eecb96b74a5c7f5c56cfec98c Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Mon, 21 Jun 2021 08:16:01 +0200 Subject: [PATCH] Merge bitcoin/bitcoin#22147: p2p: Protect last outbound HB compact block peer 30aee2dfe671b347438c1c327c6f79edfacff1ce tests: Add test for compact block HB selection (Pieter Wuille) 6efbcec4ded6116a42d2783c96c60ef0f255a1b2 Protect last outbound HB compact block peer (Suhas Daftuar) Pull request description: 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. ACKs for top commit: achow101: ACK 30aee2dfe671b347438c1c327c6f79edfacff1ce ariard: Code ACK 30aee2dfe jonatack: ACK 30aee2dfe671b347438c1c327c6f79edfacff1ce Tree-SHA512: 5c6c9326e3667b97e0864c371ae2174d2be9054dad479f4366127b9cd3ac60ffa01ec9707b16ef29cac122db6916cf56fd9985733390017134ace483278921d5 --- src/net_processing.cpp | 15 ++++ test/functional/p2p_compactblocks_hb.py | 97 +++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 3 files changed, 113 insertions(+) create mode 100755 test/functional/p2p_compactblocks_hb.py diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 190bb0bf8a23c9..f5b8d53f443249 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -808,12 +808,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){ AssertLockHeld(cs_main); diff --git a/test/functional/p2p_compactblocks_hb.py b/test/functional/p2p_compactblocks_hb.py new file mode 100755 index 00000000000000..a3d30a6f0423f6 --- /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 6f7ff90acd491c..811a0dfbafe61f 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -167,6 +167,7 @@ 'rpc_signrawtransaction.py', 'p2p_addrv2_relay.py', 'wallet_groups.py', + 'p2p_compactblocks_hb.py', 'p2p_disconnect_ban.py', 'feature_addressindex.py', 'feature_timestampindex.py',