p2p: Protect last outbound HB compact block peer #22147

sdaftuar · 2021-06-04T12:39:49Z

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.

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.

jonatack

Concept and code review ACK 6efbcec

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?

jonatack · 2021-06-04T18:41:23Z

src/net_processing.cpp

+        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) {


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

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

Crypt-iQ · 2021-06-06T18:04:53Z

ACK 6efbcec

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

jnewbery · 2021-06-07T10:48:27Z

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 · 2021-06-07T13:00:17Z

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 · 2021-06-09T01:05:18Z

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

sdaftuar · 2021-06-09T11:27:48Z

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

test/functional/p2p_compactblocks_hb.py

jnewbery · 2021-06-11T16:58:36Z

test/functional/p2p_compactblocks_hb.py

+    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']:


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)

jonatack

ACK 30aee2d

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:

master

[True,  True, False, True ]
[True,  True, True,  False]
[False, True, True,  True ]   <--- assert fails at this point

branch

[True, True,  False, True ]
[True, True,  True,  False]
[True, False, True,  True ]

A few suggestions for the test, feel free to ignore

@@ -18,7 +18,7 @@ class CompactBlocksConnectionTest(BitcoinTestFramework):
     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']:
+            if f"(testnode{to_node})" in peerinfo['subver']:
                 return peerinfo
         return None
 
@@ -73,14 +73,13 @@ class CompactBlocksConnectionTest(BitcoinTestFramework):
         # 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)
+        assert_equal(status, [True, False, True, True])
 
         # 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(status[0], True)
+            assert_equal(status[nodeid - 2], True)
             assert_equal(sum(status), 3)
 
         # Reconnect peer 2, and retry. Now the three inbounds should be HB again.
@@ -88,8 +87,8 @@ class CompactBlocksConnectionTest(BitcoinTestFramework):
         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[0], False)
+            assert_equal(status[nodeid - 2], True)
         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 · 2021-06-15T17:08:58Z

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

jonatack · 2021-06-15T17:14:46Z

@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 · 2021-06-15T18:21:47Z

ACK 30aee2d

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

ariard · 2021-06-16T15:23:33Z

Code ACK 30aee2d

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 · 2021-06-18T09:47:40Z

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

30aee2d tests: Add test for compact block HB selection (Pieter Wuille) 6efbcec 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 30aee2d ariard: Code ACK 30aee2d jonatack: ACK 30aee2d Tree-SHA512: 5c6c9326e3667b97e0864c371ae2174d2be9054dad479f4366127b9cd3ac60ffa01ec9707b16ef29cac122db6916cf56fd9985733390017134ace483278921d5

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. Github-Pull: bitcoin#22147 Rebased-From: 6efbcec

Github-Pull: bitcoin#22147 Rebased-From: 30aee2d

fanquake added the P2P label Jun 4, 2021

jonatack reviewed Jun 4, 2021

View reviewed changes

jnewbery reviewed Jun 9, 2021

View reviewed changes

test/functional/p2p_compactblocks_hb.py Outdated Show resolved Hide resolved

tests: Add test for compact block HB selection

30aee2d

sdaftuar force-pushed the 2021-06-reserve-outbound-hb branch from 3aba631 to 30aee2d Compare June 10, 2021 14:23

jnewbery reviewed Jun 11, 2021

View reviewed changes

sipa added this to the 22.0 milestone Jun 14, 2021

jonatack reviewed Jun 15, 2021

View reviewed changes

laanwj merged commit f6a25be into bitcoin:master Jun 21, 2021

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

tests: Add test for compact block HB selection

b692afc

Github-Pull: bitcoin#22147 Rebased-From: 30aee2d

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge f6a25be into merged_master (Bitcoin PR bitcoin/bitcoin#22147)

53951df

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

p2p: Protect last outbound HB compact block peer #22147

p2p: Protect last outbound HB compact block peer #22147

Uh oh!

Conversation

sdaftuar commented Jun 4, 2021

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ commented Jun 6, 2021

Uh oh!

jnewbery commented Jun 7, 2021

Uh oh!

sdaftuar commented Jun 7, 2021

Uh oh!

sipa commented Jun 9, 2021

Uh oh!

sdaftuar commented Jun 9, 2021

Uh oh!

Uh oh!

jnewbery Jun 11, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 15, 2021

Uh oh!

jonatack commented Jun 15, 2021

Uh oh!

achow101 commented Jun 15, 2021

Uh oh!

ariard commented Jun 16, 2021

Uh oh!

naumenkogs commented Jun 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jonatack left a comment •

edited

Loading

jonatack Jun 4, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading