p2p: Protect last outbound HB compact block peer

sdaftuar commented at 12:39 pm on June 4, 2021: member

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.

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.

6efbcec4de

fanquake added the label P2P on Jun 4, 2021

in src/net_processing.cpp:841 in 6efbcec4de outdated

836+            if (state != nullptr && !state->m_is_inbound) ++num_outbound_hb_peers;
837+        }
838+        if (nodestate->m_is_inbound) {
839+            // If we're adding an inbound HB peer, make sure we're not removing
840+            // our last outbound HB peer in the process.
841+            if (lNodesAnnouncingHeaderAndIDs.size() >= 3 && num_outbound_hb_peers == 1) {

jonatack commented at 6:41 pm on June 4, 2021:

Optional, if retouch perhaps hoist the 3 here and in line 854 to a constexpr and maybe the following mini-wishlist:

 0         int num_outbound_hb_peers = 0;
 1         for (std::list<NodeId>::iterator it = lNodesAnnouncingHeaderAndIDs.begin(); it != lNodesAnnouncingHeaderAndIDs.end(); it++) {
 2             if (*it == nodeid) {
 3+                // Move this nodeid to the end of the list.
 4                 lNodesAnnouncingHeaderAndIDs.erase(it);
 5                 lNodesAnnouncingHeaderAndIDs.push_back(nodeid);
 6                 return;
 7@@ -1556,7 +1557,7 @@ void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationSta
 8              !m_chainman.ActiveChainstate().IsInitialBlockDownload() &&
 9              mapBlocksInFlight.count(hash) == mapBlocksInFlight.size()) {
10         if (it != mapBlockSource.end()) {
11-            MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first);
12+            MaybeSetPeerAsAnnouncingHeaderAndIDs(/*nodeid=*/ it->second.first);
13         }

jonatack commented at 6:48 pm on June 4, 2021: member

Concept and code review ACK 6efbcec4ded6116a42d2783c96c60ef0f255a1b2

Running a node with this patch.

A test in p2p_compactblocks.py would be nice.

If this is implemented, should BIP152 “Implementation Notes” be updated with a note?

Crypt-iQ commented at 6:04 pm on June 6, 2021: contributor

ACK 6efbcec

As jonatack mentioned, I think extending the existing functional test would be good.

jnewbery commented at 10:48 am on June 7, 2021: member

Concept ACK. It seems reasonable to protect one outbound peer as an HB compact block source.

I agree with @jonatack and @Crypt-iQ that it’d be good to see a test for this new behaviour.

I’ve always found the MaybeSetPeerAsAnnouncingHeaderAndIDs() logic to be strangely convoluted and difficult to follow - is there a good reason to recursively call into ForNode() instead of just calling it serially to first switch off hb for one node and then enable it for the other? If we did that, then this logic would be a lot easier to read - we wouldn’t need to std::swap elements in a list only to later pop off the front of that list.

sdaftuar commented at 1:00 pm on June 7, 2021: member

I agree a test would be nice – I’m not planning to write one myself, but if someone contributes one I’d be happy to include it here.

I’ve always found the MaybeSetPeerAsAnnouncingHeaderAndIDs() logic to be strangely convoluted and difficult to follow - is there a good reason to recursively call into ForNode() instead of just calling it serially to first switch off hb for one node and then enable it for the other? If we did that, then this logic would be a lot easier to read - we wouldn’t need to std::swap elements in a list only to later pop off the front of that list.

I don’t know for sure, but my guess is that it’s easiest to reason about the logic if the substitution of going from one HB peer to another all happens within one lock acquisition? At any rate, I think the real problem with readability is having ForNode() at all; its existence implies that our model for how net and net_processing interact is broken (see eg #11604). I think the goal is to eventually refactor all the code like this so that we don’t have ForNode() or ForEachNode() in our code anymore, so I don’t think there’s much value in refactoring the existing logic here just to have it rewritten in the future.

sipa commented at 1:05 am on June 9, 2021: member

Here is an RPC-based test for compact blocks HB selection: https://github.com/sipa/bitcoin/commits/sdaftuar-2021-06-reserve-outbound-hb

sdaftuar commented at 11:27 am on June 9, 2021: member

@sipa Thanks, I’ve cherry-picked that commit onto this branch.

tests: Add test for compact block HB selection 30aee2dfe6

in test/functional/p2p_compactblocks_hb.py:17 in 3aba63158a outdated

12+    """Test class for verifying selection of HB peer connections."""
13+
14+    def set_test_params(self):
15+        self.setup_clean_chain = True
16+        self.num_nodes = 6
17+        self.connection_ids = {}

jnewbery commented at 12:26 pm on June 9, 2021:

This member variable is unused.

sipa commented at 3:30 pm on June 9, 2021:

Oh, indeed. Leftover from an earlier iteration.

sipa commented at 5:31 pm on June 9, 2021:

Made a few style improvements to the commit in https://github.com/sipa/bitcoin/commits/sdaftuar-2021-06-reserve-outbound-hb

sdaftuar commented at 2:23 pm on June 10, 2021:

Thanks, cherry-picked the latest commit on that branch.

sdaftuar force-pushed on Jun 10, 2021

in test/functional/p2p_compactblocks_hb.py:21 in 30aee2dfe6

16+        self.num_nodes = 6
17+
18+    def peer_info(self, from_node, to_node):
19+        """Query from_node for its getpeerinfo about to_node."""
20+        for peerinfo in self.nodes[from_node].getpeerinfo():
21+            if "(testnode%i)" % to_node in peerinfo['subver']:

jnewbery commented at 4:58 pm on June 11, 2021:

If you touch this branch again, consider using f-strings for string formatting (https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines)

sipa added this to the milestone 22.0 on Jun 14, 2021

jonatack commented at 4:58 pm on June 15, 2021: member

ACK 30aee2dfe671b347438c1c327c6f79edfacff1ce

Verified the test fails on master at line 81 (and at no other point) and passes on this branch; pprinting the value of status at that point shows the following differences between master and this change:

 0master
 1
 2[True,  True, False, True ]
 3[True,  True, True,  False]
 4[False, True, True,  True ]   <--- assert fails at this point
 5
 6branch
 7
 8[True, True,  False, True ]
 9[True, True,  True,  False]
10[True, False, True,  True ]

 0@@ -18,7 +18,7 @@ class CompactBlocksConnectionTest(BitcoinTestFramework):
 1     def peer_info(self, from_node, to_node):
 2         """Query from_node for its getpeerinfo about to_node."""
 3         for peerinfo in self.nodes[from_node].getpeerinfo():
 4-            if "(testnode%i)" % to_node in peerinfo['subver']:
 5+            if f"(testnode{to_node})" in peerinfo['subver']:
 6                 return peerinfo
 7         return None
 8 
 9@@ -73,14 +73,13 @@ class CompactBlocksConnectionTest(BitcoinTestFramework):
10         # Now relay one block through peer 2 (outbound from node 1), so it should take HB status
11         # from one of the inbounds.
12         status = self.relay_block_through(2)
13-        assert_equal(status[0], True)
14-        assert_equal(sum(status), 3)
15+        assert_equal(status, [True, False, True, True])
16 
17         # Now relay again through nodes 3,4,5. Since 2 is outbound, it should remain HB.
18         for nodeid in range(3, 6):
19             status = self.relay_block_through(nodeid)
20-            assert status[0]
21-            assert status[nodeid - 2]
22+            assert_equal(status[0], True)
23+            assert_equal(status[nodeid - 2], True)
24             assert_equal(sum(status), 3)
25 
26         # Reconnect peer 2, and retry. Now the three inbounds should be HB again.
27@@ -88,8 +87,8 @@ class CompactBlocksConnectionTest(BitcoinTestFramework):
28         self.connect_nodes(1, 2)
29         for nodeid in range(3, 6):
30             status = self.relay_block_through(nodeid)
31-            assert not status[0]
32-            assert status[nodeid - 2]
33+            assert_equal(status[0], False)
34+            assert_equal(status[nodeid - 2], True)
35         assert_equal(status, [False, True, True, True])

(Very nice test.)

(Out of curiousity, do people see inbounds selected as HB peers? Perhaps my peer topology has much better outbounds than inbounds, or not enough inbounds, but it’s rare for even one inbound to provide a block and be selected for HB mode, much less three. Outbounds get the nod.)

sipa commented at 5:08 pm on June 15, 2021: member

My node at bitcoin.sipa.be currently has all 3 HB connections on inbounds (it has 250 connections).

jonatack commented at 5:14 pm on June 15, 2021: member

@sipa thanks – good to hear about different peer topologies. Am probably restarting too often (for testing pulls) to see anything close to that many inbounds.

achow101 commented at 6:21 pm on June 15, 2021: member

ACK 30aee2dfe671b347438c1c327c6f79edfacff1ce

Reviewed code and checked that the test fails on master but passes with this PR.

ariard commented at 3:23 pm on June 16, 2021: member

Code ACK 30aee2dfe

Our last oubound HB compact block peer could still be evicted under our CHAIN_SYNC_TIMEOUT/EXTRA_PEER_CHECK_INTERVAL logics if it appears slow on block-delivery. Hitting this eviction on our side would require from an attacker to control our peer’s block-relay, assuming this peer’s implementation has the same mitigations in place against adversarial peers, it sounds hard to achieve.

naumenkogs commented at 9:47 am on June 18, 2021: member

Concept ACK. I agree with protecting last outbound HB peer.

laanwj merged this on Jun 21, 2021

laanwj closed this on Jun 21, 2021

sidhujag referenced this in commit 664de143c7 on Jun 24, 2021

luke-jr referenced this in commit fdc18ae1bc on Jun 27, 2021

luke-jr referenced this in commit b692afc591 on Jun 27, 2021

gwillen referenced this in commit 53951df7ef on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

p2p: Protect last outbound HB compact block peer #22147