When InitData returns READ_STATUS_INVALID (the peer sent malformed data) during optimistic compact block reconstruction. Call Misbehaving instead of silently ignoring it, consistent with the primary compact block handling path.
Removes the TODO added in 7017298eb2.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | w0xlt, instagibbs, davidgumberg |
| Concept ACK | sedited |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept ACK This is consistent with how the happy path invalid compact blocks are treated, and consistent with how Misbehaving() is used in general, although for me it does open the question of what the rationale for Misbehaving() is and how and where it should be used, I asked this question on IRC: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2026-05-23#1779494915-1779495170; I don't think that question is blocking with this PR.
Note, there is a silent merge conflict with this PR and #32606.
312 | + invalid_cmpct.prefilled_txn_length = 1 313 | + invalid_cmpct.prefilled_txn = [PrefilledTransaction(1, block2.vtx[0])] 314 | + bad_peer = self.nodes[0].add_p2p_connection(TestP2PConn()) 315 | + bad_peer.send_await_disconnect(msg_cmpctblock(invalid_cmpct)) 316 | + assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.hashPrevBlock) 317 | +
I think it would be better if all of the peers were ephemeral p2p conns like stall1, but this requires a little bit of refactoring which has been done in #32606, but isn't available yet, see e.g.:
https://github.com/bitcoin/bitcoin/pull/32606/changes/856deb0d431d98253ca9ad968aa808b86061838e
I think this is fine as-is, but if #32606 is merged first, this should incorporate similar changes:
make_hb_to_candidate() stall1, stall2, and badpeerstall3 does a solicited announcement, stall1 and 2 then do an unsolicited announcementbadpeer does its thing
I agree, it's kinda awkward to read this way but at the same time I didn't want to refact it just to add this new test to cover this specific case. I'd happily update with your new changes in case your PR is merged first :)
Now that #36206 has been merged, maybe this could be refactored?
Yes! I will take a look and refact it.
Refactored in b9788a5a10bd7edf87072c9d719b81ebbaea9fc1 following your suggestion, except the final slot uses outbound_node instead of a third fresh peer, since three fresh inbound peers can't all stay HB. Details in this comment.
4642 | @@ -4643,7 +4643,7 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string 4643 | PartiallyDownloadedBlock tempBlock(&m_mempool); 4644 | ReadStatus status = tempBlock.InitData(cmpctblock, vExtraTxnForCompact); 4645 | if (status != READ_STATUS_OK) { 4646 | - // TODO: don't ignore failures 4647 | + if (status == READ_STATUS_INVALID) Misbehaving(peer, "invalid compact block");
nit: comment could be added clarifying that only malformed compact blocks are considered misbehavior.
+ // Malformed compact blocks are peer misbehavior; reconstruction failures are not.
if (status == READ_STATUS_INVALID) Misbehaving(peer, "invalid compact block");
I agree, this makes the reconstruction failures case clearer. Done 4fe4b2a5fbf09ff4b3dbe622674ddbde6e4a8cfc
281 | + # message for a block already in-flight from MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK peers 282 | + # must also be disconnected. 283 | + block2 = self.build_block_on_tip(self.nodes[0]) 284 | + 285 | + # Encode coinbase as shortid (never in mempool) to force getblocktxn. 286 | + valid_cmpct = HeaderAndShortIDs()
nit: valid_cmpct can be renamed to inflight_cmpct to describe its role better.
- valid_cmpct = HeaderAndShortIDs()
- valid_cmpct.header = CBlockHeader(block2)
- valid_cmpct.nonce = 0
- [k0, k1] = valid_cmpct.get_siphash_keys()
- valid_cmpct.shortids = [calculate_shortid(k0, k1, block2.vtx[0].wtxid_int)]
+ inflight_cmpct = HeaderAndShortIDs()
+ inflight_cmpct.header = CBlockHeader(block2)
+ inflight_cmpct.nonce = 0
+ [k0, k1] = inflight_cmpct.get_siphash_keys()
+ inflight_cmpct.shortids = [calculate_shortid(k0, k1, block2.vtx[0].wtxid_int)]
I agree, this matches the MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK constant name. Done 0fc9dc02763e44f693920f02a21ed025aebc89a1
308 | + # sending an invalid cmpctblock message for the same block falls into the optimistic 309 | + # reconstruction path and must be disconnected. 310 | + invalid_cmpct = P2PHeaderAndShortIDs() 311 | + invalid_cmpct.header = CBlockHeader(block2) 312 | + invalid_cmpct.prefilled_txn_length = 1 313 | + invalid_cmpct.prefilled_txn = [PrefilledTransaction(1, block2.vtx[0])]
nit: comment could be added explaining why PrefilledTransaction(1, block2.vtx[0]) is invalid.
+ # This prefilled transaction index is too high for a compact block with no shortids.
invalid_cmpct.prefilled_txn = [PrefilledTransaction(1, block2.vtx[0])]
I agree that a comment is welcome here. I added just # This index will be too high to match the previous comment on the same function. Done 0fc9dc02763e44f693920f02a21ed025aebc89a1
Good catch ! ACK f24db575d33cdd6ebaac8cd02233bf9d266b7d42 with some non-blocking nits.
Looking back at the code I just noticed:
/** Maximum number of outstanding CMPCTBLOCK requests for the same block. */
static const unsigned int MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK = 3;
This seems imprecise:
The comment states "outstanding CMPCTBLOCK requests for the same block" but the context where MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK is used the CMPCTBLOCK is a message peers send to us (inbound), not something we request. What we actually request are the missing transactions (via GETBLOCKTXN).
MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK is how many peers we're simultaneously doing compact block reconstruction with for the same block, but the name implies CMPCTBLOCKS inflight requests which is not what's happening.
But this is out of scope of this PR, just making an observation
I asked this question on IRC: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2026-05-23#1779494915-1779495170;
It's a good question. It does seem to only punish software that is buggy / not strictly protocol conforming that are not causing harm. I thought about it and a reason could be to not waste connections on non protocol conforming peers, but still seems that Misbehave on first non comforming message may be too harsh.
Forced push 0fc9dc0. Addressed @w0xlt feedback (forgot to explain the force-push earlier)
@davidgumberg can you take another look here?
Gah, github swalled this notification for me, sorry
crACK 0fc9dc02763e44f693920f02a21ed025aebc89a1
I would prefer if the test were refactored as suggested in https://github.com/bitcoin/bitcoin/pull/35321/changes/0fc9dc02763e44f693920f02a21ed025aebc89a1#r3307751026, but also fine as-is.
reACK 0fc9dc02763e44f693920f02a21ed025aebc89a1
Cc @instagibbs
313 | + # This index will be too high 314 | + invalid_cmpct.prefilled_txn = [PrefilledTransaction(1, block2.vtx[0])] 315 | + bad_peer = self.nodes[0].add_p2p_connection(TestP2PConn()) 316 | + bad_peer.send_await_disconnect(msg_cmpctblock(invalid_cmpct)) 317 | + assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.hashPrevBlock) 318 | +
agreed that this test is a bit of a mess, currently it leaves stalled in-flight requests. Would be willing to look at a refactored test again
diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -316,6 +316,12 @@ class CompactBlocksTest(BitcoinTestFramework):
bad_peer = self.nodes[0].add_p2p_connection(TestP2PConn())
bad_peer.send_await_disconnect(msg_cmpctblock(invalid_cmpct))
assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.hashPrevBlock)
+ # Deliver the block so the stalled in-flight requests don't leak into later tests.
+ msg = msg_blocktxn()
+ msg.block_transactions = BlockTransactions(block2.hash_int, [block2.vtx[0]])
+ stall1.send_and_ping(msg)
+ assert_equal(self.nodes[0].getbestblockhash(), block2.hash_hex)
+
# Compare the generated shortids to what we expect based on BIP 152, given
# bitcoind's choice of nonce.
def test_compactblock_construction(self, test_node):
Thanks! Refactored the test and added this cleanup patch on b9788a5a10bd7edf87072c9d719b81ebbaea9fc1
Thanks for the reviews. Force-pushed b9788a5a10bd7edf87072c9d719b81ebbaea9fc1 refactoring the test.
I tried the setup suggested, but outbound_node is already HB from an earlier test, and the last outbound HB peer is never evicted. With three fresh inbound peers, one loses HB, so bad_peer never reaches the optimistic path and the test passes even without this PR's change. So the peers used were:
stall1: non-HB, solicited, first slotstall2: HB, unsolicited, second slotoutbound_node: HB, unsolicited, final slotbad_peer: HB, sends the invalid cmpctblock
reACK b9788a5a10bd7edf87072c9d719b81ebbaea9fc1
ACK b9788a5a10bd7edf87072c9d719b81ebbaea9fc1
cc @mzumsande