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-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35321.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | davidgumberg, w0xlt |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--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 :)
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)