The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32606.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #33738 (log: avoid collecting GetSerializeSize data when compact block logging is disabled by l0rinc)

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.

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. :)

Rebased to address @ajtowns and @mzumsande feedback.

• Looking at 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).
• 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.

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.

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.

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

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:

1. The most recent block has resulted in us adding an HB peer and rotating one out.
2. A second block is found in:
   A. <=~1/2 round-trip-time to the old HB peer that got unselected. The CB message that arrives is a waste of bandwidth.
   B. <=~1/2 RTT to the new HB-peer that got selected. We will be short-handed to 2 HB peers for this second block.

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.

I'm not sure this makes sense, per #28272 (comment) . Perhaps it would be better to:

• 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
• 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)

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 (comment)

Future improvements to fully "lock down" this code path could be to:

• check m_opts.ignore_incoming_txs
• check m_provides_cmpctblocks is 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:

1. Ooh, this peer is telling me about a possibly new block header!
2. Ooh, it would be a new tip and I don't have the block yet! And I don't have it in-flight yet, I should reque...
3. Ooh, it's already a CMPCTBLOCK and I don't need to request it! Great, let's analyse it, see if it reconstructs!
4. Oh, it doesn't, better request the transactions

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?