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

3 files changed, 113 insertions, 0 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index b2112cfd2e..9bb3dfdcf3 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -836,12 +836,27 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid)
         return;
     }
     if (nodestate->fProvidesHeaderAndIDs) {
+        int num_outbound_hb_peers = 0;
         for (std::list<NodeId>::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);
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 76347b052d..ad1acd2e2f 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -174,6 +174,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',