Processing unsolicited CMPCTBLOCK’s from a peer that has not been marked high bandwidth is not well-specified behavior in BIP-0152, in fact the BIP seems to imply that it is not permitted:
“[…] method is not useful for compact blocks because
cmpctblockblocks can be sent unsolicitedly in high-bandwidth mode”
This partially mitigates a mempool leak described in #28272, making it more difficult for peers not selected as high bandwidth to discover a node’s mempool.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32606.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | jonatack, mzumsande, hodlinator, instagibbs, ajtowns, Crypt-iQ |
| Stale ACK | polespinasa, w0xlt |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
No conflicts as of last run.
Possible typos and grammar issues:
Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):
Possible places where comparison-specific test macros should replace generic comparisons:
No other instances of bare “assert a == b” in the added lines were found.
2026-02-27 19:06:03
775@@ -763,6 +776,7 @@ def announce_cmpct_block(node, peer):
776 assert "getblocktxn" in peer.last_message
777 return block, cmpct_block
778
779+
Concept ACK
In the commit message: “making it only possible for peers selected as high bandwidth to discover a node’s mempool” is a bit too strong, since mempools are discoverable, just not transactions that were added to them after the last inv to the peer.
4434@@ -4435,6 +4435,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4435 range_flight.first++;
4436 }
4437
4438+ if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
I think this check is still permissive enough such that a NetMsgType::CMPCTBLOCK message can be sent to a node in -blocksonly mode and still get processed. This would require the -blocksonly node to request the block from the peer.
Maybe something like this instead?
0+ if ((!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) || m_opts.ignore_incoming_txs) {
Agreed - even if we are in blocks-only mode we might have requested_block_from_this_peer, but we won’t be able to reconstruct a block from our barren mempool unless the block is empty (or only contains transactions we originated).
One could special-case it to account for allowing empty block reconstruction on blocks-only nodes but it feels like dead weight in the code.
In case C of the diagram at https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#intended-protocol-flow, we might have explicitly asked for a compact block from a low bandwidth peer. But we appear to not be doing so in blocks-only mode, which makes sense: https://github.com/bitcoin/bitcoin/blob/6e1a49b66e77c2e6d158c51f51a19ec43cd74707/src/net_processing.cpp#L2780-L2788
One could special-case it to account for allowing empty block reconstruction on blocks-only nodes but it feels like dead weight in the code.
Yeah, I feel like the complexity isn’t worth it.
I think this check is still permissive enough such that a
NetMsgType::CMPCTBLOCKmessage can be sent to a node in -blocksonly mode and still get processed. T
Should we only send SENDCMPCT hb=false after VERACK if we’re not blocksonly? Making all the compact block stuff conditional on not being -blocksonly=1 would make sense to me, but probably isn’t needed for this PR.
Should we only send SENDCMPCT hb=false after VERACK if we’re not blocksonly?
I think this breaks -blocksonly nodes being able to send compact blocks since m_provides_cmpctblocks is never set (a peer won’t set them as HB)? I can PR the -blocksonly change I suggested above separately if it’s tangential.
Future improvements to fully “lock down” this code path could be to:
m_opts.ignore_incoming_txsm_provides_cmpctblocks is setMSG_CMPCT_BLOCK (requires state)
What if in the case of being in -blocksonly mode and receive a CMCTBLOCK message we instantly respond with a GETBLOCKTXN asking for all transactions (skipping all InitData logic)?
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index dad2c4ce6d..d46c9b003b 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -4413,6 +4413,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
5 }
6
7 if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
8+ if (m_opts.ignore_incoming_txs) {
9+ BlockTransactionsRequest req;
10+ for (size_t i = 0; i < cmpctblock.BlockTxCount(); i++) {
11+ req.indexes.push_back(i);
12+ }
13+ req.blockhash = pindex->GetBlockHash();
14+ MakeAndPushMessage(pfrom, NetMsgType::GETBLOCKTXN, vInv);
15+ return;
16+ }
17 LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s, not marked as high-bandwidth, sent us an unsolicited compact block!", pfrom.GetId(), pfrom.LogIP(fLogIPs));
18 return;
19 }
It doesn’t really ever make sense to process a CMPCTBLOCK message as blocks-only, the peer should just send the block, if we want to do anything with the message as a blocks-only, we should just process the header of the CMPCTBLOCK:
0fRevertToHeaderProcessing = true;
This will call ProcessHeadersMessage() and that can request the block.
Agree that it doesn’t make sense to process, we should drop it asap. Haven’t tested either of the below things:
ProcessHeadersMessage will trigger a GETDATA and if the block turns out to be invalid, our peer who sent us the CMPCTBLOCK won’t respond and will get disconnected for stalling. If our peer is modified to relay these in the first place to all peers, they could isolate themselves from -blocksonly nodes (assuming they aren’t a mining node and don’t validate what they send out). This is #13370.
A tangent, but I think a miner could trigger this statement in honest peer’s connections if they mined and withheld enough blocks (~4-5) and if the last CMPCTBLOCK is for an invalid block. I’m leaving out a few steps and can write out what I think on the linked issue if it’s worth addressing?
The approach I’m taking is to not use fRevertToHeaderProcessing, given that we already take the header and mark the peer as having the block at the top of the CMPCTBLOCK:
https://github.com/bitcoin/bitcoin/blob/96bec216ec34bbfa3c07e8dad7768d877ba6919b/src/net_processing.cpp#L4591 https://github.com/bitcoin/bitcoin/blob/96bec216ec34bbfa3c07e8dad7768d877ba6919b/src/net_processing.cpp#L4617
Maybe relevant: #34618
Given that, I believe that what you suggest @Crypt-iQ:
ProcessHeadersMessage will trigger a GETDATA and if the block turns out to be invalid, our peer who sent us the CMPCTBLOCK won’t respond and will get disconnected for stalling.
is already the case, since we’ll send a GETDATA on the next SendMessages tick.
Future improvements to fully “lock down” this code path could be to:
- check
m_opts.ignore_incoming_txs
Addressed in https://github.com/bitcoin/bitcoin/pull/32606/changes/6a2fa19d04bd1c2077db6b829b3fa1f7506a91e4
- check
m_provides_cmpctblocksis set
Addressed in https://github.com/bitcoin/bitcoin/pull/32606/changes/cb60b6e9ed85e2296c5bd792da4445aeff0f0b58
* check that if the block was requested, we used `MSG_CMPCT_BLOCK` (requires state)
I think if necessary, this can be addressed / discussed in a follow-up PR.
The approach I’m taking is to not use fRevertToHeaderProcessing, given that we already take the header and mark the peer as having the block at the top of the CMPCTBLOCK:
This works and lets the -blocksonly node use the header and is simpler. I just want to point out if for some reason a node is sending compact blocks to -blocksonly nodes, since the header is processed before returning in 6a2fa19d04bd1c2077db6b829b3fa1f7506a91e4, the buggy node still might get isolated because of invalid compact blocks timing out with the GETDATA fallback. A safer way would be to immediately (like right after LoadingBlocks) return early. The downside would be that the header is lost…
MaybeSetPeerAsAnnouncingHeaderAndIDs, becoming a node’s hb peer is pretty easy? I.e. be the first to supply the latest valid block at the tip and you’ll be selected as high bandwidth (we process unsolicited block messages, so this should be trivial).ProcessMessages call.
I.e. be the first to supply the latest valid block at the tip and you’ll be selected as high bandwidth (we process unsolicited block messages, so this should be trivial).
Yup I think this means the defense is ineffective.
Even if you’re low-bandwidth can’t you just send a headers announcement immediately followed by the compact block? As long as your headers message is the first announcement of the block, the compact block will be requested and since these messages are processed sequentially on the same thread, simply sending the compact directly after the headers (without waiting for the getdata) will cause it to be processed in the next ProcessMessages call.
This is interesting, but in this case I would still consider the compact block as “requested” as the peer has updated its internal state to request the compact block.
The fibre patchset is being revived and while I haven’t tested it so I don’t know if it breaks fibre, I would rather be cautious and only include the defense for -blocksonly nodes.
My main concern here is if this disables a FIBRE future; FIBRE apparently used unsolicited compact blocks
The fact that you’re enabling FIBRE on a connection can just be taken as implicitly soliciting compact blocks on that connection, so I don’t think this is a problem.
4434@@ -4435,6 +4435,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4435 range_flight.first++;
4436 }
4437
4438+ if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
4439+ LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());
Might as well include the IP of this suspicious peer (and drop newline):
0 LogDebug(BCLog::NET, "Peer %d%s, not marked as high-bandwidth, sent us an unsolicited compact block!", pfrom.GetId(), pfrom.LogIP(fLogIPs));
@ajtowns I’m not sure which suggestion you mean, I missed the suggestion to drop the newline, but in the current branch (5caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8) this is a CMPCTBLOCK message (which seems right to me) and IP’s are logged: https://github.com/bitcoin/bitcoin/blob/5caaefd/src/net_processing.cpp#L2484
Edit: My mistake, I was looking at the wrong log message 🤦, thanks for catching this.
0$ git rev-parse HEAD
15caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8
2$ grep -n not.marked net_processing.cpp
34416: LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());
The fact that you’re enabling FIBRE on a connection can just be taken as implicitly soliciting compact blocks on that connection, so I don’t think this is a problem.
Right, my earlier comment was wrong.
I think that dropping unsolicited cmpctblock in the non -blocksonly case is kind of ineffective as pointed out by others. But I also don’t have a strong opinion about it given that it doesn’t affect FIBRE.
266@@ -267,9 +267,13 @@ def check_announcement_of_new_block(node, peer, predicate):
267
268 # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
269 def test_invalid_cmpctblock_message(self):
270+ self.make_peer_hb_to_candidate(self.nodes[0], self.segwit_node)
271 self.generate(self.nodes[0], COINBASE_MATURITY + 1)
272 block = self.build_block_on_tip(self.nodes[0])
273
274+ self.segwit_node.send_header_for_blocks([block])
275+ self.segwit_node.wait_for_getdata([block.sha256], timeout=30)
AttributeError: 'CBlock' object has no attribute 'sha256'
2480@@ -2481,7 +2481,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
2481 tx_requested_size += resp.txn[i]->GetTotalSize();
2482 }
2483
2484- LogDebug(BCLog::CMPCTBLOCK, "Peer %d sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
2485+ LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), pfrom.LogIP(fLogIPs), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
nit: Avoid explicit \n.
0 LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)", pfrom.GetId(), pfrom.LogIP(fLogIPs), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
4411@@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4412 range_flight.first++;
4413 }
4414
4415+ if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
4416+ LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());
4417+ return;
Misbehaving() or disconnect them, but there’s a tiny chance they may have recently been an HB peer (as remarked in #32606 (comment)).
Haven’t tested. I think this will cause extra log messages if:
HEADERS from LB peer=0GETDATA to peer=0CMPCTBLOCK from HB peer=1, it reconstructs, clearing download state for all peers for this blockCMPCTBLOCK from LB peer=0Might confuse a user, but nothing is actually wrong here?
@Crypt-iQ In this case, I don’t think a message would be logged since we’ll return above after the check that we have data for the block:
264@@ -265,9 +265,13 @@ def check_announcement_of_new_block(node, peer, predicate):
265
266 # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
267 def test_invalid_cmpctblock_message(self):
268+ self.make_peer_hb_to_candidate(self.nodes[0], self.segwit_node)
This line is unnecessary, it can be replaced with something like:
0 self.assert_highbandwidth_states(self.nodes[0], idx=0, hb_to=True, hb_from=True)
self.segwit_node was disconnected in the prior test it’s not yet HB, plus it doesn’t look like it has sent over sendcmpct yet. I agree that we should assert that it is marked as HB though.
@hodlinator’s suggested change passes, because segwit node’s index is not 0 any more, it just got connected so it’s -1, 0 is some other node that’s already HB. And the test passes without making segwit_node HB since I’ve modified the test to actually be solicited, since the peer sends a header and waits for getdata. Should I add separate test cases for DC’ing invalid cmpctblock’s both unsolicited from HB’s and solicited from non-HB’s?
It also seems weird that we’ll accept a CMPCTBLOCK from someone that hasn’t given us a SENDCMPCT announcing support for it, but that’s probably fine, I’m thinking out loud but maybe there’s some way in which that’s an issue if later versions of the CMPCTBLOCK message are introduced in the future.
Edit: This was already pointed out by @Crypt-iQ below: #32606 (review)
In my opinion, the statefulness of p2p_compactblocks.py in general is pretty awful and would be worth fixing in a separate PR.
Thanks for using separate peers in 42e281dba0f0807cc08d8c50a9d19906e27e2129. Was totally not expecting the reassignment of segwit_node. :sleeping:
I like how you use hb_peer_idx in f24237ef3d8c94c01f4b9f86a7d28d0b5fcdc75b as both a parameter for assert_highbandwidth_states() and for indexing into self.nodes[0].p2ps.
This seems to be lacking precision now that we ignore some CMPCTBLOCK messages.
0--- a/test/functional/p2p_compactblocks.py
1+++ b/test/functional/p2p_compactblocks.py
2@@ -904,7 +904,7 @@ class CompactBlocksTest(BitcoinTestFramework):
3 node = self.nodes[0]
4 assert len(self.utxos)
5
6- def announce_cmpct_block(node, peer, txn_count):
7+ def announce_cmpct_block(node, peer, txn_count, expect_success):
8 utxo = self.utxos.pop(0)
9 block = self.build_block_with_transactions(node, utxo, txn_count)
10
11@@ -914,12 +914,18 @@ class CompactBlocksTest(BitcoinTestFramework):
12 peer.send_and_ping(msg)
13 with p2p_lock:
14 assert "getblocktxn" in peer.last_message
15+ if expect_success:
16+ assert peer.last_message["getblocktxn"].block_txn_request.blockhash == block.hash_int, \
17+ f'Mismatching blocks - message: {peer.last_message["getblocktxn"].block_txn_request.blockhash:x}, expected: {block.hash_hex}'
18+ else:
19+ assert peer.last_message["getblocktxn"].block_txn_request.blockhash != block.hash_int
20+
21 return block, cmpct_block
22
23 for name, peer in [("delivery", delivery_peer), ("inbound", inbound_peer), ("outbound", outbound_peer)]:
24 self.log.info(f"Setting {name} as high bandwidth peer")
25 self.make_peer_hb_to_candidate(node, peer)
26- block, cmpct_block = announce_cmpct_block(node, peer, 1)
27+ block, cmpct_block = announce_cmpct_block(node, peer, 1, expect_success=True)
28 msg = msg_blocktxn()
29 msg.block_transactions.blockhash = block.hash_int
30 msg.block_transactions.transactions = block.vtx[1:]
31@@ -933,12 +939,13 @@ class CompactBlocksTest(BitcoinTestFramework):
32 # Remaining low-bandwidth peer is stalling_peer, who announces first
33 assert_equal([peer['bip152_hb_to'] for peer in node.getpeerinfo()], [False, True, True, True])
34
35- block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing)
36+ block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing, expect_success=False)
37
38 delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
39 with p2p_lock:
40 # The second peer to announce should still get a getblocktxn
41 assert "getblocktxn" in delivery_peer.last_message
42+ assert delivery_peer.last_message["getblocktxn"].block_txn_request.blockhash == cmpct_block.header.hash_int
43 assert_not_equal(node.getbestblockhash(), block.hash_hex)
44
45 inbound_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
46@@ -951,6 +958,7 @@ class CompactBlocksTest(BitcoinTestFramework):
47 with p2p_lock:
48 # The third peer to announce should get a getblocktxn if outbound
49 assert "getblocktxn" in outbound_peer.last_message
50+ assert outbound_peer.last_message["getblocktxn"].block_txn_request.blockhash == cmpct_block.header.hash_int
51 assert_not_equal(node.getbestblockhash(), block.hash_hex)
52
53 # Second peer completes the compact block first
0--- a/test/functional/p2p_compactblocks.py
1+++ b/test/functional/p2p_compactblocks.py
2@@ -904,36 +904,42 @@ class CompactBlocksTest(BitcoinTestFramework):
3 node = self.nodes[0]
4 assert len(self.utxos)
5
6- def announce_cmpct_block(node, peer, txn_count):
7+ def announce_cmpct_block(node, peer, txn_count, expect_success):
8 utxo = self.utxos.pop(0)
9 block = self.build_block_with_transactions(node, utxo, txn_count)
10
11+ peer.clear_getblocktxn()
12 cmpct_block = HeaderAndShortIDs()
13 cmpct_block.initialize_from_block(block)
14 msg = msg_cmpctblock(cmpct_block.to_p2p())
15 peer.send_and_ping(msg)
16 with p2p_lock:
17- assert "getblocktxn" in peer.last_message
18+ if expect_success:
19+ assert "getblocktxn" in peer.last_message
20+ else:
21+ assert "getblocktxn" not in peer.last_message
22 return block, cmpct_block
23
24 for name, peer in [("delivery", delivery_peer), ("inbound", inbound_peer), ("outbound", outbound_peer)]:
25 self.log.info(f"Setting {name} as high bandwidth peer")
26 self.make_peer_hb_to_candidate(node, peer)
27- block, cmpct_block = announce_cmpct_block(node, peer, 1)
28+ block, cmpct_block = announce_cmpct_block(node, peer, 1, expect_success=True)
29 msg = msg_blocktxn()
30 msg.block_transactions.blockhash = block.hash_int
31 msg.block_transactions.transactions = block.vtx[1:]
32 peer.send_and_ping(msg)
33 assert_equal(node.getbestblockhash(), block.hash_hex)
34- peer.clear_getblocktxn()
35
36 # Test the simple parallel download case...
37 for num_missing in [1, 5, 20]:
38+ delivery_peer.clear_getblocktxn()
39+ inbound_peer.clear_getblocktxn()
40+ outbound_peer.clear_getblocktxn()
41
42 # Remaining low-bandwidth peer is stalling_peer, who announces first
43 assert_equal([peer['bip152_hb_to'] for peer in node.getpeerinfo()], [False, True, True, True])
44
45- block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing)
46+ block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing, expect_success=False)
47
48 delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
49 with p2p_lock:
50@@ -964,10 +970,6 @@ class CompactBlocksTest(BitcoinTestFramework):
51 stalling_peer.send_and_ping(msg)
52 self.utxos.append([block.vtx[-1].txid_int, 0, block.vtx[-1].vout[0].nValue])
53
54- delivery_peer.clear_getblocktxn()
55- inbound_peer.clear_getblocktxn()
56- outbound_peer.clear_getblocktxn()
57-
58 def test_unsolicited_compact_blocks_ignored(self):
59 node = self.nodes[0]
60 # create new p2p connection for a fresh state w/o any prior sendcmpct messages sent
nit: Would rather adjust the test to not share UTXO’s between test functions and remove assert len(self.utxos) in since pop() will raise (https://docs.python.org/3/tutorial/datastructures.html#more-on-lists):
See final commit in my suggestions branch.
972+ node = self.nodes[0]
973+ # create new p2p connection for a fresh state w/o any prior sendcmpct messages sent
974+ unsolicited_peer = self.nodes[0].add_p2p_connection(TestP2PConn())
975+
976+ # assert the RPC getpeerinfo boolean fields `bip152_hb_{to, from}`
977+ # match the given parameters for the last peer of a given node
nit:
0 # match the given parameters for the peer at idx of a given node
976+ # assert the RPC getpeerinfo boolean fields `bip152_hb_{to, from}`
977+ # match the given parameters for the last peer of a given node
978+ def assert_highbandwidth_states(node, idx, hb_to, hb_from=False):
979+ peerinfo = node.getpeerinfo()[idx]
980+ assert_equal(peerinfo['bip152_hb_to'], hb_to)
981+ assert_equal(peerinfo['bip152_hb_from'], hb_from)
1005+ # peer is still not selected as high-bandwidth
1006+ assert_highbandwidth_states(node, idx=-1, hb_to=False)
1007+
1008+ block, msg = build_compact_block()
1009+ unsolicited_peer.send_without_ping(msg_headers([block]))
1010+ unsolicited_peer.wait_for_getdata([block.hash_int], timeout=30)
Maybe use a comment to point out that since we only send the header and not the whole block, we are not marked as an HB peer?
And/or use assert_highbandwidth_states(node, idx=-1, hb_to=False) another time at the end of this block to verify/clarify.
unsolicited_peer also hasn’t sent sendcmpct so its m_provides_cmpctblocks is false, and the node still processes the compact block! I am ok with this as-is since imo it is useful to have coverage for this case.
Messing around with this I think there’s actually a bug, even if unsolicited_peer sends a sendcmpct, they don’t get a high bandwidth slot here even after they send the requisite BLOCKTXN, since they were the first to announce a block.
I believe this is because mapBlocksInFlight doesn’t get cleared when a node reconstructs from a GETBLOCKTXN resulting in MaybeSetPeerAsAnnouncing.* never getting called:
I will change this test so that a sendcmpct is sent, and introduce a fix for @Crypt-iQ’s suggestion that we shouldn’t process blocks from peers where m_provides_cmpctblocks is false, and try to fix this behavior in a follow-up.
ProcessBlock? https://github.com/bitcoin/bitcoin/blob/5a8a427610ed9672f485ebfefbb8e63b55282636/src/net_processing.cpp#L3400-L3415
A branch which shows the bad behavior: https://github.com/davidgumberg/bitcoin/tree/2026-02-19-inflight-hb-slot-bug / https://github.com/davidgumberg/bitcoin/commit/46f82c43b68f21ab75ec43e7a098f6743cec58d1
Doesn’t it always get cleared here in ProcessBlock?
Yes, my mistake!
I think what’s happening is that the heuristic of mapBlocksInFlight.count(hash) == mapBlocksInFlight.size() doesn’t work since another block is in-flight from earlier in the test. Seems like this is a very minor (but prohibitively expensive to exploit) DoS vector: if a malicious peer can perpetually maintain a block-in-flight slot on a node, they can prevent that node from ever having any HB peers.
@hodlinator In my latest push I’ve refactored some logic and added a comment that makes it clearer that since we don’t respond to the GETBLOCKTXN we never get an HB slot, although for the reasons discussed above, in this particular test, as is, we wouldn’t get a slot anyways because of the (potential) other in-flight blocks, but that’s not behavior I’m intending to test, it’s sufficient that by not responding I can be sure that the peer does not get a slot:
I am going to leave this marked unresolved so that the discussion about not allocating any HB slots when any block other than the one we’ve received is in-flight is visible, but I think I’ve addressed the OP feedback.
Quite the thread… nice find regarding the multiple blocks in flight leading to not being marked as HB-to.. agree with requiring sendcmpt(0 or 1) in order to process CMPCTBLOCK, seems consistent with the BIP152 diagram.
Not sure whether this would make things clearer in case you re-touch:
0 # Note: since we never fulfill the GETBLOCKTXN, this will never change
1- # the HB status of a connection.
2+ # the test node's HB status for the connection.
one race condition we might hit is a compact block being sent to our node before that peer receives the message un-selecting them as a hb peer.
In this case we probably are OK to drop, even though it wastes a little bandwidth, and for the time that our SENDCMPCT is on the wire to our new high-bandwidth peer, we are down to only 2 HB peers.
We’ll only unselect a peer as high bandwidth when selecting another which only happens when connecting a new block. It takes ~half a round-trip for our peer to find out they’ve gotten the boot, so this condition only occurs when:
If 1/2 RTT = 50ms, on average 0.008% ($$1 - e^{\frac{-50\text{ms}}{600000\text{ms}}}$$) of blocks or ~4.3 blocks/yr are found that quickly. and only if those happen to be the same blocks where we are selecting a new HB peer will we hit this condition. I’m not sure if that’s worth fixing.
264@@ -265,9 +265,13 @@ def check_announcement_of_new_block(node, peer, predicate):
265
266 # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
267 def test_invalid_cmpctblock_message(self):
268+ self.make_peer_hb_to_candidate(self.nodes[0], self.segwit_node)
269 self.generate(self.nodes[0], COINBASE_MATURITY + 1)
270 block = self.build_block_on_tip(self.nodes[0])
271
272+ self.segwit_node.send_header_for_blocks([block])
273+ self.segwit_node.wait_for_getdata([block.hash_int], timeout=30)
self.segwit_node is marked as HB above?
819@@ -806,6 +820,9 @@ def test_compactblock_reconstruction_stalling_peer(self, stalling_peer, delivery
820 node = self.nodes[0]
821 assert len(self.utxos)
822
823+ self.make_peer_hb_to_candidate(node, stalling_peer)
stalling_peer is already HB
I’m not sure this makes sense, per #28272 (comment) . Perhaps it would be better to:
That would perhaps be slightly wasteful of bandwidth if a block arrives that includes a tx that was broadcast just before the block was found, but would avoid this being an additional fingerprinting vector independently of how high-bandwidth a potential attacker was, as far as I can see.
Rebased to address reviewer feedback. @ajtowns I’m thinking of this now less as a mitigation for the fingerprinting vector, and more just defense-in-depth / closing off attack surface for compact blocks. E.g. CVE-2024-35202 and CVE-2024-52921 are more trivial to exploit because you don’t need an HB slot.
An HB slot might be a low bar for an attacker to overcome as @dergoegge points out above/ But, as I see it, there is very rarely a legitimate/useful reason today to process an unsolicited CMPCTBLOCK message, especially if it’s the case that getting an HB slot is trivial, and it does raise the cost of an attack, and it mitigates any DoS that require sending malformed CMPCTBLOCK’s persistently, any attacker is likely to get cycled out of their HB slot.
For example, once the first_in_flight slot is occupied, we won’t send GETBLOCKTXN’s to any non-HB peers, so if you have two connections to a node, you can have one occupy the first_in_flight slot for a real block with a fake cmpctblock that has nonsense shortid’s, never respond to GETBLOCKTXN, and the second peer can endlessly send CMPCTBLOCK’s with 2^16-1 shortid’s and the victim node will just keep calling InitData and looping the entire mempool until it gets the block from an honest peer.
Edit: Actually that DoS might be more effective with a single fake shortid.
second peer can endlessly send CMPCTBLOCK’s with 2^16-1 shortid’s and the victim node will just keep calling InitData and looping the entire mempool until it gets the block from an honest peer.
Kind of a tangent, but do you have any idea how fast/slow this is if the mempool has ~100k txns?
BlockEncoding.* benchmarks should cover this up to 50k. Quickly tested 100k, and it seems ~linear slowdown. (~4ms on my mid laptop)
the second peer can endlessly send CMPCTBLOCK’s
If we’re concerned about that, seems better to fix it directly?
0--- a/src/net_processing.cpp
1+++ b/src/net_processing.cpp
2@@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3 while (range_flight.first != range_flight.second) {
4 if (range_flight.first->second.first == pfrom.GetId()) {
5 requested_block_from_this_peer = true;
6+ const auto& qb_list_it = range_flight.first->second.second;
7+ if (qb_list_it->partialBlock != nullptr) {
8+ LogDebug(BCLog::CMPCTBLOCK, "Ignoring duplicate CMPCTBLOCK message for block %s received from peer=%d%s", pindex->GetBlockHash().ToString(), pfrom.GetId(), pfrom.LogIP(fLogIPs));
9+ return;
10+ }
11 break;
12 }
13 range_flight.first++;
But, as I see it, there is very #32606 (comment) a legitimate/useful reason today to process an unsolicited CMPCTBLOCK message
I could imagine someone restarting the FIBRE network, and having it interface to the p2p network with a custom node implementation that implements logic like “only relay txs with an internal node. drop connections to anyone with a ping time exceeding 50ms. for each block, did I get this block via FIBRE? if so, send a CMPCTBLOCK message to all peers because I’m probably the quickest source in this region. if not, behave normally.”
If we’re concerned about that, seems better to fix it directly?
0--- a/src/net_processing.cpp 1+++ b/src/net_processing.cpp 2@@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 3 [...]
The suggested diff doesn’t work because we remove the block request from mapBlocksInFlight if the txn’s are missing, the firstInFlight slot is already taken, and the block is not from an HB Peer:
To do something like what you’re suggesting I think we would need another map that tracks whether or not a peer has already had the block in flight at one point and we gave up on it.
I could imagine someone restarting the FIBRE network, and having it interface to the p2p network with a custom node implementation that implements logic like “only relay txs with an internal node. drop connections to anyone with a ping time exceeding 50ms. for each block, did I get this block via FIBRE? if so, send a CMPCTBLOCK message to all peers because I’m probably the quickest source in this region. if not, behave normally.”
If I’m not mistaken, at least the only effort I know of at restarting the FIBRE network does not want to reveal the identity of the FIBRE nodes to prevent targeted DoS, but maybe I am mistaken, pinging @w0xlt.
But, in general, I hear the broader stroke of your objection as: The “hole” in our implementation of compact block relay might actually be an opportunity we can leverage to improve block propagation, and we shouldn’t close it so hastily. And that’s fair enough, I kind of disagree about the FIBRE use case, but it is worth thinking about if, in an implementation like you suggested, where each node only processes one unsolicited block per peer per valid header, the price of InitData() * number of connections an attacker can reasonably get to a node is worth whatever the potential improvement we could get from nodes that definitely know they’ve got the block first like mining nodes, or are reasonably confident they’re the fastest like FIBRE nodes announcing unsolicited. It seems interesting, and I’ll try and think about it for a bit as well, but given the disproportionate number of CVE’s that are in this part of the code, without something concrete I would prefer to close down our implementation of the protocol to be as conservative as possible about what CMPCTBLOCK messages we process, e.g. this PR and the other improvements suggested by @Crypt-iQ above: #32606 (review)
Future improvements to fully “lock down” this code path could be to:
- check
m_opts.ignore_incoming_txs- check
m_provides_cmpctblocksis set- check that if the block was requested, we used
MSG_CMPCT_BLOCK(requires state)
The suggested diff doesn’t work because we remove the block request from mapBlocksInFlight if the txn’s are missing, the firstInFlight slot is already taken, and the block is not from an HB Peer:
Does that logic make sense? If we receive:
0sequenceDiagram
1 participant A
2 Us ->> A: SENDCMPCT 0
3 Us ->> B: SENDCMPCT 1 (high-bandwidth)
4 A ->> Us: t=0 HEADERS
5 Us ->> A: GETDATA(CMPCT)
6 B ->> Us: t=25ms CMPCTBLOCK
7 Us ->> B: GETBLOCKTXN
8 A ->> Us: t=50ms CMPCTBLOCK
does it really make sense to not request the missing txns from the peer who first announced the header to us? If the RTT latency between us and B is 80ms (versus us and A at 50ms), then we’d get a reply from A at t=100ms and the reply from B at t=105ms, eg.
Not requesting GETBLOCKTXN in response to a CMPCTBLOCK from a non-hb peer when we didn’t request the cb and we already have a request in flight still sounds sensible, though, so I don’t think this resolves the objection.
Finding a way to refactor this code so that the logic for how we do block relay is mostly in one place rather than scattered amongst message various different message parsing logic might be valuable… I guess if you treated the logic as:
then if you received an unsolicited CMPCTBLOCK message that you already had things in flight for that block, you’d stop at (2) and just treat the message as an overly verbose headers update, and not even try to process it against the mempool (after all, if you have requests in flight, that was probably from an honest peer and you probably are actually missing transactions, so the reconstruction would probably fail anyway). OTOH, if you didn’t have things in flight for it (or weren’t being as parallel as you’d like to be), you would continue, and would request (and eventually process) the missing transactions.
We add peers to mapBlocksInFlight whenever we send GETDATA’s in response to headers, so in the scenario you describe we would treat the peer that announced the header to us as first_in_flight().
The flow is ProcessHeadersMessage(), and if the header looks good, call HeadersDirectFetchBlocks(), and if everything looks good there and we decide to send a GETDATA for the block, we call BlockRequested() which adds the block to mapBlocksInFlight:
I think the first step of simplifying the logic would be to separate out “in-flight” as in I’ve GETDATA requested this block from this peer, and “in-flight” as in I’m waiting on a BLOCKTXN for this block from this peer.
The refactor you suggest sounds interesting, I don’t know exactly how it would look, but if I’m understanding you right a centralized block processing function where some additional data (e.g. a CMPCTBLOCK) might or might not be available seems a reasonable approach, but the complexity would be in all the possible ways to “re-enter” the function, e.g. process the first message, it’s a header we care about but no block or cmpctblock for us to process so send a GETDATA, get a cmpctblock message we’re missing tx’es so send a getblocktxn, get a blocktxn, would this all be in the same function?
code review ACK 32bfd6136134e7194ea0b56ec08498f66829fc40
I think it’s worth resolving the odd case for -blocksonly in this pull request. But I would be happy too with it being done in a follow-up PR.
treat a CMPCTBLOCK announcement as being fine at any time (eg, so that miners who get a block via RPC can just send CMPCTBLOCK messages directly), and the “high-bandwidth” as just a way for nodes to avoid wasting their bandwidth sending those messages unnecessarily
I think yes, except for -blocksonly nodes, where on receipt we wouldn’t punish or anything, but we would ideally drop the message?
change our behaviour when sending GETBLOCKTXN to also request any transactions that were in our mempool but had a sequence number higher than when we first saw the new block’s header (If you completely reconstructed the block, but used newer txns, you would still not send a GETBLOCKTXN)
I think it would be possible for an attacker to send HEADERS, receive GETDATA, wait for some time to allow new txns to accumulate in the node’s mempool, then send a CMPCTBLOCK with suspected new txns that wouldn’t have been announced yet. But, if the targeted node receives the valid block from any other peer, the download state gets wiped. So this is an edge-of-an-edge case. Since an attacker wants “tight timing information” as you pointed out on the linked issue and this is only able to be done ~10mins (and may not even work all the time if the attacker is not HB), it doesn’t seem urgent.
without something concrete I would prefer to close down our implementation of the protocol to be as conservative as possible about what CMPCTBLOCK messages we process
Any restrictive change here introduces a kind of implicit version for compact block relay even if it’s later lifted. I guess the unknown for me is whether this is something that miners (or others) want to do, are planning to do, or if this is a usecase we should support?
+1 to the refactor suggestion. I don’t have any suggestions off-hand. Maybe a new file could be introduced specifically for compact block relay (similar to what was done for blockencodings.cpp)? It would make writing tests a bit easier. Also, checking whether partialBlock is nullptr (and having to reset it in some cases), is awkward.
1031+ unsolicited_peer.send_without_ping(msg_headers([block]))
1032+ unsolicited_peer.wait_for_getdata([block.hash_int], timeout=30)
1033+ unsolicited_peer.send_and_ping(msg)
1034+ with p2p_lock:
1035+ assert "getblocktxn" in unsolicited_peer.last_message
1036+ unsolicited_peer.clear_getblocktxn()
This clearing should arguably happen before the assert, along the lines of #32606 (review) / 32bfd6136134e7194ea0b56ec08498f66829fc40
0--- a/test/functional/p2p_compactblocks.py
1+++ b/test/functional/p2p_compactblocks.py
2@@ -414,6 +414,7 @@ class CompactBlocksTest(BitcoinTestFramework):
3 [k0, k1] = comp_block.get_siphash_keys()
4 coinbase_hash = block.vtx[0].wtxid_int
5 comp_block.shortids = [calculate_shortid(k0, k1, coinbase_hash)]
6+ test_node.clear_getblocktxn()
7 test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
8 assert_equal(int(node.getbestblockhash(), 16), block.hashPrevBlock)
9 # Expect a getblocktxn message.
10@@ -451,6 +452,7 @@ class CompactBlocksTest(BitcoinTestFramework):
11 node = self.nodes[0]
12
13 def test_getblocktxn_response(compact_block, peer, expected_result):
14+ peer.clear_getblocktxn()
15 msg = msg_cmpctblock(compact_block.to_p2p())
16 peer.send_and_ping(msg)
17 with p2p_lock:
18@@ -517,8 +519,7 @@ class CompactBlocksTest(BitcoinTestFramework):
19 assert tx.txid_hex in mempool
20
21 # Clear out last request.
22- with p2p_lock:
23- test_node.last_message.pop("getblocktxn", None)
24+ test_node.clear_getblocktxn()
25
26 # Send compact block
27 comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
28@@ -547,6 +548,7 @@ class CompactBlocksTest(BitcoinTestFramework):
29 # Send compact block
30 comp_block = HeaderAndShortIDs()
31 comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
32+ test_node.clear_getblocktxn()
33 test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
34 absolute_indexes = []
35 with p2p_lock:
36@@ -592,6 +594,7 @@ class CompactBlocksTest(BitcoinTestFramework):
37 # Send compact block
38 comp_block = HeaderAndShortIDs()
39 comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
40+ test_node.clear_getblocktxn()
41 test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
42 absolute_indexes = []
43 with p2p_lock:
44@@ -844,6 +847,7 @@ class CompactBlocksTest(BitcoinTestFramework):
45
46 cmpct_block = HeaderAndShortIDs()
47 cmpct_block.initialize_from_block(block)
48+ peer.clear_getblocktxn()
49 msg = msg_cmpctblock(cmpct_block.to_p2p())
50 peer.send_and_ping(msg)
51 with p2p_lock:
52@@ -924,14 +928,13 @@ class CompactBlocksTest(BitcoinTestFramework):
53 assert len(self.utxos)
54
55 def announce_cmpct_block(node, peer, txn_count, expect_getblocktxn):
56- peer.clear_getblocktxn()
57-
58 utxo = self.utxos.pop(0)
59 block = self.build_block_with_transactions(node, utxo, txn_count)
60
61 cmpct_block = HeaderAndShortIDs()
62 cmpct_block.initialize_from_block(block)
63 msg = msg_cmpctblock(cmpct_block.to_p2p())
64+ peer.clear_getblocktxn()
65 peer.send_and_ping(msg)
66 with p2p_lock:
67 if expect_getblocktxn:
68@@ -1027,10 +1030,10 @@ class CompactBlocksTest(BitcoinTestFramework):
69 block, msg = build_compact_block()
70 unsolicited_peer.send_without_ping(msg_headers([block]))
71 unsolicited_peer.wait_for_getdata([block.hash_int], timeout=30)
72+ unsolicited_peer.clear_getblocktxn()
73 unsolicited_peer.send_and_ping(msg)
74 with p2p_lock:
75 assert "getblocktxn" in unsolicited_peer.last_message
76- unsolicited_peer.clear_getblocktxn()
77
78 # The node will ask for transactions from an unsolicited compact block
79 # it receives from a high bandwidth peer, we need to use one set up earlier,
283-
284 cmpct_block = P2PHeaderAndShortIDs()
285 cmpct_block.header = CBlockHeader(block)
286 cmpct_block.prefilled_txn_length = 1
287 # This index will be too high
288 prefilled_txn = PrefilledTransaction(1, block.vtx[0])
nit: Named variable feels clearer. (No risk of confusing with the index used for block.vtx[...]).
0 too_high_index = 1
1 prefilled_txn = PrefilledTransaction(too_high_index, block.vtx[0])
294+
295+ # Test an unsolicited invalid cmpctblock from an HB peer.
296+ hb_peer.send_await_disconnect(msg_cmpctblock(cmpct_block))
297+ assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
298+
299+ # Test a solicited invalid cmpctblock from a non-HB peer.
963
964 # Remaining low-bandwidth peer is stalling_peer, who announces first
965 assert_equal([peer['bip152_hb_to'] for peer in node.getpeerinfo()], [False, True, True, True])
966
967- block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing)
968+ block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing, expect_getblocktxn=False)
stalling_peer is low-bandwidth, the compact block is dropped and the full block is requested (taking up the first slot) in SendMessages since self.nodes[0] has processed the header. The test still passes, but I think what this is testing has changed slightly from the original. The original behavior can be maintained if stalling_peer sends the header first and then sends over the compact block.
expect change.
In 1e0611a3ab3f813998e2628fd57a63b0a3e54151 “p2p: Drop unsolicited CMPCTBLOCK from non-HB peer”:
This tale starts before I re-read this thread.
Checked out 1e0611a3ab3f813998e2628fd57a63b0a3e54151.
Reverted added lines at test/functional/p2p_compactblocks.py#912-914 in test_compactblock_reconstruction_parallel_reconstruction()’s announce_cmpct_block().
Noticed how the test succeeds anyway.
Rebased this PR on top of 31723. Used ccmake build to set ENABLE_WAIT_FOR_DEBUGGER=ON and CMAKE_BUILD_TYPE=Debug.
Checked out rebased commit “p2p: Drop unsolicited CMPCTBLOCK from non-HB peer” again and reverted same lines.
Set breakpoint on PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs()s:
0pfrom->m_bip152_highbandwidth_to = true;
0cmake --build build -t bitcoind -t bitcoin-cli && ./build/test/functional/p2p_compactblocks.py --debug_runs 0
Attached Sublime Text (with Debugger & LLDB plugins). (No PID needed since the test only runs one node with mocked peers).
Hit breakpoint but continued from some initial checks until we reach:
02026-02-26T08:35:23.352817Z TestFramework (INFO): Setting delivery as high bandwidth peer
12026-02-26T08:35:23.517486Z TestFramework (INFO): Setting inbound as high bandwidth peer
Realized we are calling the new make_peer_hb_to_candidate() right before announce_cmpct_block() so we don’t have to announce the block from the peer and wait for getdata in announce_cmpct_block() since the peer is already “HB-to”.
So yeah, probably best to avoid adding the lines until necessary (see suggestions branch).
Sorry for the marketing speak. I still think the current version of the PR introduces
0 peer.send_without_ping(msg_headers([block]))
1 peer.wait_for_getdata([block.hash_int], timeout=30)
into announce_cmpct_block() earlier than needed, and also should be guarded by a solicited argument which only rarely needs to be set, see suggestions branch #32606#pullrequestreview-3855999802. (I’m fine with if you want to keep more code-duplication than that branch, but sending unnecessary messages seems imprecise).
Reviewed 32bfd6136134e7194ea0b56ec08498f66829fc40 and left some comments. I think if the use-case brought up by @ajtowns is not a concern, then I can ACK.
Separately, I’ve been thinking about how a refactor would look. I’m still at a loss because unifying the headers and the compact blocks processing logic seems difficult because sometimes the compact block can be processed as a headers message (i.e. calls ProcessHeadersMessage) and there are compact blocks-specific checks that a headers message wouldn’t go through.
4411@@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4412 range_flight.first++;
4413 }
4414
4415+ if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
nit: Maybe add a comment for clarification?
0 // Accept unsolicited cmpctblock *only* from peers we selected as high-bandwidth
1 // "to" (hb_to). Peers that merely requested HB *from* us (hb_from) don't qualify.
2 // This matches BIP152's "unsolicited in high-bandwidth mode" intent.
3 if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
Sorry for the delay here, I’ve added changes to address feedback from @hodlinator and @Crypt-iQ, notably:
CMPCTBLOCK messages are dropped by blocksonly nodes. (https://github.com/bitcoin/bitcoin/pull/32606/changes/6a2fa19d04bd1c2077db6b829b3fa1f7506a91e4)CMPCTBLOCK messages from peers that have never sent SENDCMPCT are dropped. (https://github.com/bitcoin/bitcoin/pull/32606/changes/cb60b6e9ed85e2296c5bd792da4445aeff0f0b58)4602+ if (pindex->nStatus & BLOCK_HAVE_DATA) {
4603+ return;
4604+ } else if (m_opts.ignore_incoming_txs) {
4605+ LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s sent us a compact block even though we are blocksonly!", pfrom.GetId(), pfrom.LogIP(fLogIPs));
4606+ return;
4607+ } else if (nodestate->m_provides_cmpctblocks == false) {
nits: Please avoid x == true or x == false
0 } else if (!nodestate->m_provides_cmpctblocks) {
Similar case in /test/functional/p2p_compactblocks_blocksonly.py (unless you take the larger diff to that file in suggestions branch):
0- if expected == False:
1+ if not expected:
4622@@ -4642,6 +4623,11 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4623 range_flight.first++;
4624 }
4625
4626+ if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
PR description nit:
0- making it only possible for peers selected as high bandwidth to discover a node's mempool.
1+ making it significantly harder for peers not selected as high bandwidth to discover a node's mempool.
It’s still possible if you have a really good connection to be the first one to announce a block header to a node (that is not blocks-only) and then be able to respond with a compact block that it processes and asks back for missing transactions on, no?
1670@@ -1671,7 +1671,7 @@ class msg_sendcmpct:
1671 __slots__ = ("announce", "version")
1672 msgtype = b"sendcmpct"
1673
1674- def __init__(self, announce=False, version=1):
1675+ def __init__(self, announce=False, version=2):
Commit message could mention why we bump the version, seems to be due to:
SENDCMPCT handshake correctly as part of set-up, not trying to test the version number behavior.
820@@ -794,6 +821,12 @@ def test_invalid_tx_in_compactblock(self, test_node):
821 msg = msg_cmpctblock(comp_block.to_p2p())
822 test_node.send_await_disconnect(msg)
823
824+ # peer generates a block and sends it to node, which makes the peer a
825+ # candidate for high-bandwidth 'to' (up to 3 peers according to BIP 152)
826+ def make_peer_hb_to_candidate(self, node, peer):
827+ block = self.build_block_on_tip(node)
828+ peer.send_and_ping(msg_block(block))
remark: Played around with asserting that this function always works (spoiler: it does), there’s hopefully some easier way to express it:
0 for peerinfo in node.getpeerinfo():
1 if int(peerinfo['addrbind'].split(':')[-1]) == peer.dstport:
2 assert_equal(peerinfo['bip152_hb_to'], True)
3 return
4 raise AssertionError(f"Could not find peer with port {peer.dstport}")
don’t think it’s necessarily worth including.
988- inbound_peer.clear_getblocktxn()
989- outbound_peer.clear_getblocktxn()
990+ def test_compact_blocks_ignored(self):
991+ node = self.nodes[0]
992+
993+ def build_compact_block():
nit: Could define build_compact_block() inside ignores_compact_block() since it’s the only place using it.
Alternatively, one could use build_compact_block() inside announce_cmpct_block() as well, as in my suggestions branch.
1038+ self.assert_highbandwidth_states(node, idx=-1, hb_to=False, hb_from=False)
1039+
1040+ # The node will ask for transactions from an unsolicited compact block
1041+ # it receives from a high bandwidth peer, we need to use one set up earlier,
1042+ # since all the slots are full.
1043+ self.log.info("Test that a node does ignore unsolicited CMPCTBLOCK messages from HB peers.")
Correction - message does not match code below:
0 self.log.info("Test that a node does NOT ignore unsolicited CMPCTBLOCK messages from HB peers.")
39@@ -38,6 +40,28 @@ def build_block_on_tip(self):
40 block = from_hex(CBlock(), block_hex)
41 return block
42
43+ def can_receive_compactblock(self, node_id, conn, expected=False, solicited=False):
nit: I prefer the way you wrote it in p2p_compactblocks.py, test_compact_blocks_ignored(), instead of sending in expected=<x>:
0assert not ignores_compact_block(unsolicited_peer, solicited=True)
This would make it:
0--- a/test/functional/p2p_compactblocks_blocksonly.py
1+++ b/test/functional/p2p_compactblocks_blocksonly.py
2@@ -40,7 +40,7 @@ class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
3 block = from_hex(CBlock(), block_hex)
4 return block
5
6- def can_receive_compactblock(self, node_id, conn, expected=False, solicited=False):
7+ def received_compactblock(self, node_id, conn, solicited=False):
8 # Briefly connect to mining node, sync, and disconnect, just to make
9 # sure the node is on the same tip as the miner so that compact block
10 # reconstruction works.
11@@ -57,10 +57,11 @@ class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
12 msg = msg_cmpctblock(cmpct_block.to_p2p())
13 conn.send_and_ping(msg)
14
15- assert_equal(expected, self.nodes[node_id].getbestblockhash() == cmpct_block.header.hash_hex)
16
17- if expected == False:
18+ if cmpct_ignored := self.nodes[node_id].getbestblockhash() != cmpct_block.header.hash_hex:
19+ # Compact block was ignored, send the full block to keep in sync.
20 conn.send_and_ping(msg_block(block))
21+ return not cmpct_ignored
22
23 def run_test(self):
24 # Nodes will only request hb compact blocks mode when they're out of IBD
25@@ -156,16 +157,16 @@ class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
26
27 # This is redundant with other tests, and is here as a test-of-the-test
28 self.log.info("Test that normal nodes don't ignore CMPCTBLOCK messages from HB peers")
29- self.can_receive_compactblock(1, p2p_conn_high_bw, expected=True, solicited=False)
30+ assert self.received_compactblock(1, p2p_conn_high_bw, solicited=False)
31
32 self.log.info("Test that -blocksonly nodes ignore CMPCTBLOCK messages")
33- self.can_receive_compactblock(0, p2p_conn_blocksonly, expected=False, solicited=False)
34+ assert not self.received_compactblock(0, p2p_conn_blocksonly, solicited=False)
35
36 self.log.info("Test that low bandwidth nodes listen to CMPCTBLOCK messages when the block is requested")
37- self.can_receive_compactblock(3, p2p_conn_low_bw, expected=True, solicited=True)
38+ assert self.received_compactblock(3, p2p_conn_low_bw, solicited=True)
39
40 self.log.info("Test that -blocksonly nodes ignore CMPCTBLOCK messages even when the block is requested")
41- self.can_receive_compactblock(0, p2p_conn_blocksonly, expected=False, solicited=True)
42+ assert not self.received_compactblock(0, p2p_conn_blocksonly, solicited=True)
43
44 if __name__ == '__main__':
45 P2PCompactBlocksBlocksOnly(__file__).main()
!= with the hashes, because the strict inverse of that is not the same as ==, or ! != is not equivalent to ==
Reviewed cb60b6e9ed85e2296c5bd792da4445aeff0f0b58
Made a suggestions branch: https://github.com/hodlinator/bitcoin/tree/pr/32606_suggestions compare
989+ def test_compact_blocks_ignored(self):
990+ node = self.nodes[0]
991
992+ def build_compact_block():
993+ # generate a compact block to send that will force a GETBLOCKTXN
994+ assert len(self.utxos)
nit: Can be removed as per #32606 (review)
2583@@ -2611,7 +2584,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
2584 if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
2585 uint32_t tx_requested_size{0};
2586 for (const auto& tx : resp.txn) tx_requested_size += tx->ComputeTotalSize();
2587- LogDebug(BCLog::CMPCTBLOCK, "Peer %d sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
2588+ LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), pfrom.LogIP(fLogIPs), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
\n.
Processing unsolicited CMPCTBLOCK's from a peer that has not been marked
high bandwidth is not well-specified behavior in BIP-0152, in fact the
BIP seems to imply that it is not permitted:
"[...] method is not useful for compact blocks because `cmpctblock`
blocks can be sent unsolicitedly in high-bandwidth mode"
See https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#separate-version-for-segregated-witness
This partially mitigates a mempool leak described in
[#28272](https://github.com/bitcoin/bitcoin/issues/28272), but that
particular issue will persist for peers that have been selected as high
bandwidth.
This also mitigates potential DoS / bandwidth-wasting / abusive
behavior that is discussed in the comments of #28272.
This is move-only, and allows assert_highbandwidth_states to be used by
other tests.
Modifies the invalid_cmpctblock_message test to check that both HB peers
sending unsolicited and non-HB peers sending solicited invalid
cmpctblock's get the boot from us. Also refactors the test to make it
less stateful.
Clear the getblocktxn message so we're not checking existing messages,
and check that the hash in the getblocktxn match the cmpctblock being
announced.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
blocksonly nodes don't benefit from compact blocks, since they don't
have a mempool to aid in reconstruction, so they should not process
CMPCTBLOCK messages.
This is not just belt-and-suspenders, as a blocksonly node will
trivially reveal exactly which transactions in a block are its own in
the GETBLOCKTXN response to a CMPCTBLOCK. Since it will be missing every
transaction in the block, except for its own.
See discussion: https://github.com/bitcoin/bitcoin/issues/28272
This commit also changes the default sendcmpct version in the functional
test to `2`, since this is the version that nodes expect, prior to this
commit, nowhere in the functional test framework was the default version
value used:
git grep -P 'msg_sendcmpct\((?![^)]*version\s*=)' HEAD^
`version=2` is a sensible default, since this is the version nodes
currently expect in the SENDCMPCT handshake.
1094@@ -1045,6 +1095,8 @@ def run_test(self):
1095 self.log.info("Testing high-bandwidth mode states via getpeerinfo...")
1096 self.test_highbandwidth_mode_states_via_getpeerinfo()
1097
1098+ self.test_compact_blocks_ignored()
4596@@ -4597,8 +4597,13 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4597 nodestate->m_last_block_announcement = GetTime();
4598 }
4599
4600- if (pindex->nStatus & BLOCK_HAVE_DATA) // Nothing to do here
4601+ // Nothing to do here
4602+ if (pindex->nStatus & BLOCK_HAVE_DATA) {
4603 return;
4604+ } else if (m_opts.ignore_incoming_txs) {
This is bugging me. This does work, but in principle I think any defensive checks should fail early. I think this check should be at the very top where LoadingBlocks is. If some peer is buggy enough to send a blocksonly node a compact block, we can get the header in the code as-is, but I don’t think it’s even worth supporting that edge case.
Also, thanks for doing this in this PR.
155@@ -122,5 +156,18 @@ def test_for_cmpctblock(block):
156 self.nodes[0].submitblock(block2.serialize().hex())
157 p2p_conn_blocksonly.wait_until(lambda: test_for_cmpctblock(block2))
158
159+ # This is redundant with other tests, and is here as a test-of-the-test
4602@@ -4603,6 +4603,9 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4603 } else if (m_opts.ignore_incoming_txs) {
4604 LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s sent us a compact block even though we are blocksonly!", pfrom.GetId(), pfrom.LogIP(fLogIPs));
4605 return;
4606+ } else if (!nodestate->m_provides_cmpctblocks) {
LoadingBlocks() check is
43@@ -43,7 +44,9 @@ def run_test(self):
44 self.generate(self.wallet, COINBASE_MATURITY)
45
46 honest_relayer = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="outbound-full-relay")
47+ honest_relayer.send_and_ping(msg_sendcmpct())
48 attacker = self.nodes[0].add_p2p_connection(P2PInterface())
49+ attacker.send_and_ping(msg_sendcmpct())
I like the added tests. I think these tests could be expanded and also cleaned up in the future since I think reusing the peer connections between sub-tests is a little confusing, but that’s unrelated to this PR. My one critique is that the checks for -blocksonly and if the node has sent sendcmpct should be at the top of the scope imo. They are safe as-is, though I really do think any defensive checks should fail quickly. It prevents things like code accidentally being introduced between the top of the scope and the check, even if it’s unlikely that it happens. Other than that, I think this is good to go.
I’m not concerned about the earlier discussion about the use-case of unsolicited cmpctblocks being sent from non-HB peers. It doesn’t seem like anybody has plans to implement that, though I’m happy to be proven wrong.