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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32606.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | w0xlt |
| Concept ACK | jonatack, mzumsande, hodlinator, instagibbs, ajtowns, Crypt-iQ |
| Stale ACK | polespinasa |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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 | +
I'll delete this on next rebase.
Concept ACK. BIP152 is marked as Final but perhaps could be clarified on this point.
Concept ACK. LLM linter's punctuation advice seems right to me. :)
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.
Rebased to address @ajtowns and @mzumsande feedback.
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?
+ 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)?
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index dad2c4ce6d..d46c9b003b 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4413,6 +4413,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
}
if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
+ if (m_opts.ignore_incoming_txs) {
+ BlockTransactionsRequest req;
+ for (size_t i = 0; i < cmpctblock.BlockTxCount(); i++) {
+ req.indexes.push_back(i);
+ }
+ req.blockhash = pindex->GetBlockHash();
+ MakeAndPushMessage(pfrom, NetMsgType::GETBLOCKTXN, vInv);
+ return;
+ }
LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s, not marked as high-bandwidth, sent us an unsolicited compact block!", pfrom.GetId(), pfrom.LogIP(fLogIPs));
return;
}
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:
fRevertToHeaderProcessing = 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...
My main concern here is if this disables a FIBRE future; FIBRE apparently used unsolicited compact blocks (see: #28272 (comment)). If we are not concerned about that, then Concept ACK.
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):
LogDebug(BCLog::NET, "Peer %d%s, not marked as high-bandwidth, sent us an unsolicited compact block!", pfrom.GetId(), pfrom.LogIP(fLogIPs));
Thanks for this suggestion, I've taken it.
This doesn't seem to have been taken (or has been lost in a rebase?) Should this be a NET log message or a CMPCTBLOCK log message?
👆
@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.
$ git rev-parse HEAD
5caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8
$ grep -n not.marked net_processing.cpp
4416: LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());
@ajtowns Sorry, my mistake, see my edit above.
I should've counted a few more Mississippi's before commenting 😁
Concept ACK 1fc95e080040a9d038a8291eeecf645e0413a5c8
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'Force-pushed to address merge conflict and @hodlinator's feedback to log IP's.
concept ACK
Concept ACK here too; haven't looked at the code yet
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.
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;
remark: Wondered whether it would be fair to mark as Misbehaving() or disconnect them, but there's a tiny chance they may have recently been an HB peer (as remarked in #32606 (comment)).
+1 to the way @instagibbs pointed out for honest peers sending unsolicited CMPCTBLOCK's making this a bad idea. And if we did this, any way now or in the future that you could trick a node into sending an unsolicited CMPCTBLOCK becomes an attack vector. Given that there is little an attacker could accomplish by sending us an unsolicited compact block if we drop unsolicited compact blocks on the ground, I think we don't gain much by marking them as misbehaving or disconnecting them.
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:
Ah yes, you're right. Should have tested.
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:
self.assert_highbandwidth_states(self.nodes[0], idx=0, hb_to=True, hb_from=True)
Since 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.
+1 for separate test cases for solicited / unsolicited. I agree the test can be improved in a dedicated 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.
<details> <summary>Could check which block..</summary>
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -904,7 +904,7 @@ class CompactBlocksTest(BitcoinTestFramework):
node = self.nodes[0]
assert len(self.utxos)
- def announce_cmpct_block(node, peer, txn_count):
+ def announce_cmpct_block(node, peer, txn_count, expect_success):
utxo = self.utxos.pop(0)
block = self.build_block_with_transactions(node, utxo, txn_count)
@@ -914,12 +914,18 @@ class CompactBlocksTest(BitcoinTestFramework):
peer.send_and_ping(msg)
with p2p_lock:
assert "getblocktxn" in peer.last_message
+ if expect_success:
+ assert peer.last_message["getblocktxn"].block_txn_request.blockhash == block.hash_int, \
+ f'Mismatching blocks - message: {peer.last_message["getblocktxn"].block_txn_request.blockhash:x}, expected: {block.hash_hex}'
+ else:
+ assert peer.last_message["getblocktxn"].block_txn_request.blockhash != block.hash_int
+
return block, cmpct_block
for name, peer in [("delivery", delivery_peer), ("inbound", inbound_peer), ("outbound", outbound_peer)]:
self.log.info(f"Setting {name} as high bandwidth peer")
self.make_peer_hb_to_candidate(node, peer)
- block, cmpct_block = announce_cmpct_block(node, peer, 1)
+ block, cmpct_block = announce_cmpct_block(node, peer, 1, expect_success=True)
msg = msg_blocktxn()
msg.block_transactions.blockhash = block.hash_int
msg.block_transactions.transactions = block.vtx[1:]
@@ -933,12 +939,13 @@ class CompactBlocksTest(BitcoinTestFramework):
# Remaining low-bandwidth peer is stalling_peer, who announces first
assert_equal([peer['bip152_hb_to'] for peer in node.getpeerinfo()], [False, True, True, True])
- block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing)
+ block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing, expect_success=False)
delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
with p2p_lock:
# The second peer to announce should still get a getblocktxn
assert "getblocktxn" in delivery_peer.last_message
+ assert delivery_peer.last_message["getblocktxn"].block_txn_request.blockhash == cmpct_block.header.hash_int
assert_not_equal(node.getbestblockhash(), block.hash_hex)
inbound_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
@@ -951,6 +958,7 @@ class CompactBlocksTest(BitcoinTestFramework):
with p2p_lock:
# The third peer to announce should get a getblocktxn if outbound
assert "getblocktxn" in outbound_peer.last_message
+ assert outbound_peer.last_message["getblocktxn"].block_txn_request.blockhash == cmpct_block.header.hash_int
assert_not_equal(node.getbestblockhash(), block.hash_hex)
# Second peer completes the compact block first
</details>
<details> <summary>Another variation is also to clear `getblocktxn` before checking for it instead of after, which seems more robust. (And which you do in your new `test_unsolicited_compact_blocks_ignored()` towards the end).</summary>
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -904,36 +904,42 @@ class CompactBlocksTest(BitcoinTestFramework):
node = self.nodes[0]
assert len(self.utxos)
- def announce_cmpct_block(node, peer, txn_count):
+ def announce_cmpct_block(node, peer, txn_count, expect_success):
utxo = self.utxos.pop(0)
block = self.build_block_with_transactions(node, utxo, txn_count)
+ peer.clear_getblocktxn()
cmpct_block = HeaderAndShortIDs()
cmpct_block.initialize_from_block(block)
msg = msg_cmpctblock(cmpct_block.to_p2p())
peer.send_and_ping(msg)
with p2p_lock:
- assert "getblocktxn" in peer.last_message
+ if expect_success:
+ assert "getblocktxn" in peer.last_message
+ else:
+ assert "getblocktxn" not in peer.last_message
return block, cmpct_block
for name, peer in [("delivery", delivery_peer), ("inbound", inbound_peer), ("outbound", outbound_peer)]:
self.log.info(f"Setting {name} as high bandwidth peer")
self.make_peer_hb_to_candidate(node, peer)
- block, cmpct_block = announce_cmpct_block(node, peer, 1)
+ block, cmpct_block = announce_cmpct_block(node, peer, 1, expect_success=True)
msg = msg_blocktxn()
msg.block_transactions.blockhash = block.hash_int
msg.block_transactions.transactions = block.vtx[1:]
peer.send_and_ping(msg)
assert_equal(node.getbestblockhash(), block.hash_hex)
- peer.clear_getblocktxn()
# Test the simple parallel download case...
for num_missing in [1, 5, 20]:
+ delivery_peer.clear_getblocktxn()
+ inbound_peer.clear_getblocktxn()
+ outbound_peer.clear_getblocktxn()
# Remaining low-bandwidth peer is stalling_peer, who announces first
assert_equal([peer['bip152_hb_to'] for peer in node.getpeerinfo()], [False, True, True, True])
- block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing)
+ block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing, expect_success=False)
delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
with p2p_lock:
@@ -964,10 +970,6 @@ class CompactBlocksTest(BitcoinTestFramework):
stalling_peer.send_and_ping(msg)
self.utxos.append([block.vtx[-1].txid_int, 0, block.vtx[-1].vout[0].nValue])
- delivery_peer.clear_getblocktxn()
- inbound_peer.clear_getblocktxn()
- outbound_peer.clear_getblocktxn()
-
def test_unsolicited_compact_blocks_ignored(self):
node = self.nodes[0]
# create new p2p connection for a fresh state w/o any prior sendcmpct messages sent
</details>
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.
Taken, thanks.
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:
# 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)
Duplicated from other test in same file, could be made class-local instead? (Last arg is always left at default in this new outer test function).
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.
Doesn't it always get cleared here in 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.
This seems possible if the malicious peer knows a block that nobody else has, they could keep sending the header and get disconnected at the 10m mark, and repeat. If the block is known by others, there's always the chance that a HB connection can give the block, or that after the attacker is disconnected for not serving the block, due to the shuffling of nodes, an honest node can have the block requested from before another attacking peer can send the header.
Yeah in order to perform the attack on nodes that have >=1 honest HB peer, the malicious peer has to be able to produce valid PoW block headers for blocks no one has. Maybe this is not irrelevant since someone doing selfish mining (and not trying to hide it) could probably slightly extend their advantage by broadcasting headers of unpublished blocks and then stalling.
@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:
# Note: since we never fulfill the GETBLOCKTXN, this will never change
- # the HB status of a connection.
+ # the test node's HB status for the connection.
Yeah, would have been much easier to discover with #31723, I will try to start rebasing on top of it in my future functional test debugging sessions. I think the language in the existing comment is clear.
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.
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.
Reviewed 5caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8
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)
These two lines can be removed if self.segwit_node is marked as HB above?
Good catch, in the branch I'll push soon I address this by checking both a solicited cmpctblock and unsolicited one.
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)
nit: can remove this line, stalling_peer is already HB
Thanks for catching this, I'll remove the line, but like I said above, ideally this test wouldn't be stateful at all.
I agree with @dergoegge that it's easy to become a HB peer if you're already the first to send unsolicited compact blocks to try and fingerprint a node's mempool. I think the benefit with this change is being more strict with what we'll accept and following the protocol closer (and I think there's even more improvements we can make). Left some test-related nits.
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?
@Crypt-iQ 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?
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
while (range_flight.first != range_flight.second) {
if (range_flight.first->second.first == pfrom.GetId()) {
requested_block_from_this_peer = true;
+ const auto& qb_list_it = range_flight.first->second.second;
+ if (qb_list_it->partialBlock != nullptr) {
+ LogDebug(BCLog::CMPCTBLOCK, "Ignoring duplicate CMPCTBLOCK message for block %s received from peer=%d%s", pindex->GetBlockHash().ToString(), pfrom.GetId(), pfrom.LogIP(fLogIPs));
+ return;
+ }
break;
}
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?
--- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, [...]
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:
sequenceDiagram
participant A
Us ->> A: SENDCMPCT 0
Us ->> B: SENDCMPCT 1 (high-bandwidth)
A ->> Us: t=0 HEADERS
Us ->> A: GETDATA(CMPCT)
B ->> Us: t=25ms CMPCTBLOCK
Us ->> B: GETBLOCKTXN
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
<details><summary>Full diff, including some previously missing clear_getblocktxn(), and deferring clearing to the last possible place.</summary>
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -414,6 +414,7 @@ class CompactBlocksTest(BitcoinTestFramework):
[k0, k1] = comp_block.get_siphash_keys()
coinbase_hash = block.vtx[0].wtxid_int
comp_block.shortids = [calculate_shortid(k0, k1, coinbase_hash)]
+ test_node.clear_getblocktxn()
test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
assert_equal(int(node.getbestblockhash(), 16), block.hashPrevBlock)
# Expect a getblocktxn message.
@@ -451,6 +452,7 @@ class CompactBlocksTest(BitcoinTestFramework):
node = self.nodes[0]
def test_getblocktxn_response(compact_block, peer, expected_result):
+ peer.clear_getblocktxn()
msg = msg_cmpctblock(compact_block.to_p2p())
peer.send_and_ping(msg)
with p2p_lock:
@@ -517,8 +519,7 @@ class CompactBlocksTest(BitcoinTestFramework):
assert tx.txid_hex in mempool
# Clear out last request.
- with p2p_lock:
- test_node.last_message.pop("getblocktxn", None)
+ test_node.clear_getblocktxn()
# Send compact block
comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
@@ -547,6 +548,7 @@ class CompactBlocksTest(BitcoinTestFramework):
# Send compact block
comp_block = HeaderAndShortIDs()
comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
+ test_node.clear_getblocktxn()
test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
absolute_indexes = []
with p2p_lock:
@@ -592,6 +594,7 @@ class CompactBlocksTest(BitcoinTestFramework):
# Send compact block
comp_block = HeaderAndShortIDs()
comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
+ test_node.clear_getblocktxn()
test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
absolute_indexes = []
with p2p_lock:
@@ -844,6 +847,7 @@ class CompactBlocksTest(BitcoinTestFramework):
cmpct_block = HeaderAndShortIDs()
cmpct_block.initialize_from_block(block)
+ peer.clear_getblocktxn()
msg = msg_cmpctblock(cmpct_block.to_p2p())
peer.send_and_ping(msg)
with p2p_lock:
@@ -924,14 +928,13 @@ class CompactBlocksTest(BitcoinTestFramework):
assert len(self.utxos)
def announce_cmpct_block(node, peer, txn_count, expect_getblocktxn):
- peer.clear_getblocktxn()
-
utxo = self.utxos.pop(0)
block = self.build_block_with_transactions(node, utxo, txn_count)
cmpct_block = HeaderAndShortIDs()
cmpct_block.initialize_from_block(block)
msg = msg_cmpctblock(cmpct_block.to_p2p())
+ peer.clear_getblocktxn()
peer.send_and_ping(msg)
with p2p_lock:
if expect_getblocktxn:
@@ -1027,10 +1030,10 @@ class CompactBlocksTest(BitcoinTestFramework):
block, msg = build_compact_block()
unsolicited_peer.send_without_ping(msg_headers([block]))
unsolicited_peer.wait_for_getdata([block.hash_int], timeout=30)
+ unsolicited_peer.clear_getblocktxn()
unsolicited_peer.send_and_ping(msg)
with p2p_lock:
assert "getblocktxn" in unsolicited_peer.last_message
- unsolicited_peer.clear_getblocktxn()
# The node will ask for transactions from an unsolicited compact block
# it receives from a high bandwidth peer, we need to use one set up earlier,
</details>
Isn't it a matter of taste to some extent if we clear after or before? I guess you can be confident about the state locally if you clear before, but if you clear after you have to trust that the last test case to run cleared it properly; fair enough. To avoid blowing up the diff I'll refrain from changing the existing instances in this PR, but I'll update
Actually sorry, I see that I have a commit already changing lots of instances of this co-authored by you ;) , I'll add to that..
Okay, taken, thanks for the diff (again)
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[...]).
too_high_index = 1
prefilled_txn = PrefilledTransaction(too_high_index, block.vtx[0])
Taken, thanks.
Reviewed 32bfd6136134e7194ea0b56ec08498f66829fc40
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.
Commit message of 42e281dba0f0807cc08d8c50a9d19906e27e2129 can change to say "non-HB peers sending unsolicited invalid"
Thanks for catching this, fixed.
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)
Since 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.
Subtle, thanks for catching this, I've reworked this so that in the first commit where we drop CMPCTBLOCK messages that are unsolicited I change the test to send a headers, and in the later commit where I tighten down the getblocktxn stuff, I dropped this 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:
pfrom->m_bip152_highbandwidth_to = true;
cmake --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:
2026-02-26T08:35:23.352817Z TestFramework (INFO): Setting delivery as high bandwidth peer
2026-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
peer.send_without_ping(msg_headers([block]))
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?
// Accept unsolicited cmpctblock *only* from peers we selected as high-bandwidth
// "to" (hb_to). Peers that merely requested HB *from* us (hb_from) don't qualify.
// This matches BIP152's "unsolicited in high-bandwidth mode" intent.
if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
I don't think a comment would add much more than the log message here.
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
} 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):
- if expected == False:
+ if not expected:
Fixed, thanks.
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:
- making it only possible for peers selected as high bandwidth to discover a node's mempool.
+ 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:
Yes, there was nowhere before where the default value was being used anyways, since all of the existing tests were testing that nodes behave as expected given certain version numbers, the only version that we currently expect is 2, so this makes sense as a default value for tests that are actually just trying to perform the SENDCMPCT handshake correctly as part of set-up, not trying to test the version number behavior.
Added a note to the commit message, thanks
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:
for peerinfo in node.getpeerinfo():
if int(peerinfo['addrbind'].split(':')[-1]) == peer.dstport:
assert_equal(peerinfo['bip152_hb_to'], True)
return
raise AssertionError(f"Could not find peer with port {peer.dstport}")
don't think it's necessarily worth including.
Yeah, the awkward part here is determining the peer index, this feels like something that should exist in the test framework somewhere, but I can't think of a clean way of doing this yet.
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.
I kept it outside since I think it's generally useful for future tests or refactoring existing tests, although I'm not using it in that way now.
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:
self.log.info("Test that a node does NOT ignore unsolicited CMPCTBLOCK messages from HB peers.")
Thanks for catching, fixed
The word is added in the later accd8d34de340f32b125267f820ff85c4cf36471 while it seems like it should already be part of the original fb407a915da08fe9165aa62ceb4f8d57d311db15 which introduces the line.
Yeah I think it should be in this commit if not too much trouble just for commit hygiene
Oops, thanks for keeping me honest, fixed.
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>:
assert not ignores_compact_block(unsolicited_peer, solicited=True)
This would make it:
--- a/test/functional/p2p_compactblocks_blocksonly.py
+++ b/test/functional/p2p_compactblocks_blocksonly.py
@@ -40,7 +40,7 @@ class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
block = from_hex(CBlock(), block_hex)
return block
- def can_receive_compactblock(self, node_id, conn, expected=False, solicited=False):
+ def received_compactblock(self, node_id, conn, solicited=False):
# Briefly connect to mining node, sync, and disconnect, just to make
# sure the node is on the same tip as the miner so that compact block
# reconstruction works.
@@ -57,10 +57,11 @@ class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
msg = msg_cmpctblock(cmpct_block.to_p2p())
conn.send_and_ping(msg)
- assert_equal(expected, self.nodes[node_id].getbestblockhash() == cmpct_block.header.hash_hex)
- if expected == False:
+ if cmpct_ignored := self.nodes[node_id].getbestblockhash() != cmpct_block.header.hash_hex:
+ # Compact block was ignored, send the full block to keep in sync.
conn.send_and_ping(msg_block(block))
+ return not cmpct_ignored
def run_test(self):
# Nodes will only request hb compact blocks mode when they're out of IBD
@@ -156,16 +157,16 @@ class P2PCompactBlocksBlocksOnly(BitcoinTestFramework):
# This is redundant with other tests, and is here as a test-of-the-test
self.log.info("Test that normal nodes don't ignore CMPCTBLOCK messages from HB peers")
- self.can_receive_compactblock(1, p2p_conn_high_bw, expected=True, solicited=False)
+ assert self.received_compactblock(1, p2p_conn_high_bw, solicited=False)
self.log.info("Test that -blocksonly nodes ignore CMPCTBLOCK messages")
- self.can_receive_compactblock(0, p2p_conn_blocksonly, expected=False, solicited=False)
+ assert not self.received_compactblock(0, p2p_conn_blocksonly, solicited=False)
self.log.info("Test that low bandwidth nodes listen to CMPCTBLOCK messages when the block is requested")
- self.can_receive_compactblock(3, p2p_conn_low_bw, expected=True, solicited=True)
+ assert self.received_compactblock(3, p2p_conn_low_bw, solicited=True)
self.log.info("Test that -blocksonly nodes ignore CMPCTBLOCK messages even when the block is requested")
- self.can_receive_compactblock(0, p2p_conn_blocksonly, expected=False, solicited=True)
+ assert not self.received_compactblock(0, p2p_conn_blocksonly, solicited=True)
if __name__ == '__main__':
P2PCompactBlocksBlocksOnly(__file__).main()
I agree, thanks for the suggestion, I did a version of this that's syntactically identical but more closely fits the semantics of the test. I also think we should avoid doing != 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
CI Failures seem unrelated to me.
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)
Thanks for catching, fixed.
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);
nit: Can drop trailing \n.
Fixed, thanks.
Reviewed b11753f6a6a985ad6b5d2890f28323d4d5d5582d
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()
nit: could add a log message before this line similar to the other tests?
Thanks, done
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.
I'm not sure that I agree, I think when it comes to something that is expensive to produce like a high-PoW header, and that could be an escape valve for an e.g. eclipse attack (a high-PoW chaintip) I think we want to be pretty aggressive in pursuing it.
But that is just a kind of rhetorical / intuitive argument, I'm not sure how to be more thorough in comparing the potential harm of processing a protocol-violating CMPCTBLOCK message to check for a high-PoW header vs the benefit of potentially learning about a high-PoW chain.
This doesn't make sense to me because it's protocol-invalid. The -blocksonly node should never accept cmpctblocks at all, it never requests them in the first place. What are the odds that we're eclipsed by not having the header (it's also possible to be eclipsed by having the header and not the block which this wouldn't help) and we have a protocol-violating peer who sends us a compact block to bail us out? I think the code should be very strict in what we accept, not doing this leads to edge cases like addrfetch connections being marked HB or processing compact blocks when sendcmpct was never sent. Anecdotally, I've found bugs in other projects where the root cause is being loose in what we accept.
Tend to agree with @Crypt-iQ on avoiding deserialization of CBlockHeaderAndShortTxIDs and further work before reaching this point. If we want to rescue hypothetical nodes from eclipse attacks it seems better to announce regular headers to them than send compact blocks.
I agree about erring on the side of being strict about what we process, but I think checking block headers for PoW might be an exception. But, I concede that this does raise the general risk level of this logic, and might leave an existing bug exposed or allow a bug to be introduced in the future.
I feel like I can argue for doing either, so I asked about this on IRC: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2026-03-11#1773250654-1773250664;
Worth bringing up in the IRC meeting?
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
nice addition 👍
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) {
I think this should also be moved to where the LoadingBlocks() check is
Let's discuss in the related comment above: #32606 (review)
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())
This line can be removed since the attacker never sends a cmpctblock
Initially thought of this as belt-and-suspenders that the mutated block is rejected for the reason we are testing, but the test is pretty explicit:
Thanks, fixed.
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.
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.
908 | @@ -894,13 +909,17 @@ def announce_cmpct_block(node, peer, txn_count): 909 | cmpct_block = HeaderAndShortIDs() 910 | cmpct_block.initialize_from_block(block) 911 | msg = msg_cmpctblock(cmpct_block.to_p2p()) 912 | + peer.send_without_ping(msg_headers([block])) 913 | + peer.wait_for_getdata([block.hash_int], timeout=30) 914 | +
e42a906 p2p: Drop unsolicited CMPCTBLOCK from non-HB peer:
As in the resolved #32606 (review) I insist that these lines are both introduced unnecessarily early, and in later commits are only necessary for one use of the function once it's extracted to the parent scope.
Branch only focusing on this aspect: https://github.com/hodlinator/bitcoin/tree/pr/32606_suggestions2
Reviewed 82c3a9f6b05a6cd578001da147355fc07d585332
reACK 82c3a9f6b05a6cd578001da147355fc07d585332