compact blocks in IBD resets m_stalling

Crypt-iQ commented at 1:53 am on February 28, 2025: contributor

During IBD it is possible that a malicious peer sends unsolicited compact-block messages to reset its m_stalling_since state. This requires two or more malicious peer connections and can happen with the following steps:

Malicious peer A sends a headers message that contains height h+1. The node requests block h+1 from peer A.
Malicious peer B sends a headers message that contains heights {h+1, h+2, ..., h+1024}. The node requests MAX_BLOCKS_IN_TRANSIT_PER_PEER blocks from B. Peer B does not need to reply with any of these blocks.
Honest peer C sends a headers message that contains heights {h+1, h+2, ..., h+1024}. The node will request blocks continuing from where it last requested from peer B.
Peer A sends a compact-block for h+2 with non-empty but bogus shorttxids.
- The target node will attempt to process the compact-block since it is within two blocks of h. The CanDirectFetch logic is also skipped here as the block is already in-flight with peer B.
- Since the shorttxids field is bogus, execution hits this branch and calls RemoveBlockRequest. This will reset m_stalling_since even though peer A hasn’t actually provided a block.
- This can be repeated by peer A to reset m_stalling_since before the timeout triggers. The peer should eventually be disconnected (unless it gives the block) after the larger block download timeout of ~10minutes+ depending on the number of downloading peers which could be numerous in IBD.

I think this scenario is a bit contrived since it requires two (or more?) malicious peers whom are requested subsequent blocks, but it illustrates that:

resetting m_stalling_since in this case is a bug
compact-blocks can be received unsolicited even in IBD. The block must be currently in-flight and must be one of the two blocks above the target node’s tip.

Here is a branch that demonstrates this with a functional test: https://github.com/Crypt-iQ/bitcoin/tree/ibd_cmpct

maflcko added the label P2P on Feb 28, 2025

mzumsande commented at 8:47 pm on March 3, 2025: contributor

I don’t think this would work with blocks below the minchainwork threshold, we should abort here - so large parts of IBD shouldn’t be affected. We will still be in IBD for a while after reaching that threshold though, depending on how up-to-date the bitcoind release is.

Also, the node schedules the blocks it downloads during IBD not as an answer to received headers messages, but on its own initiative in SendMessages. So it shouldn’t be possible to trigger this by sending header messages, but the two attacking peers could be be randomly selected next to each other.

Guessing that this could be connected to #27626 (fyi @instagibbs) because before we shouldn’t have processed a CMPCTBLOCK for a block that was already in flight?!

As for a fix, I wonder Is there a reason to have https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/src/net_processing.cpp#L4386-L4389 dependent on already_in_flight - could we unconditionally skip out here if CanDirectFetch() returns false? At least removing the condition doesn’t break any existing tests.

Crypt-iQ commented at 9:46 pm on March 3, 2025: contributor

Ah, I did not consider the minchainwork.

So it shouldn’t be possible to trigger this by sending header messages, but the two attacking peers could be be randomly selected next to each other.

I could have made that more clear in the description that the attacker needed two peers to be selected in sequence for the SendMessages call. That’s what I meant by “subsequent” but I think it was unclear.

dependent on already_in_flight - could we unconditionally skip out here if CanDirectFetch() returns false? At least removing the condition doesn’t break any existing tests.

That was what I was thinking, I can’t see a current use for the !already_in_flight check.

instagibbs commented at 9:57 pm on March 3, 2025: member

That code block appears to be in since the dawn of compact blocks: https://github.com/bitcoin/bitcoin/commit/d25cd3ec4e8#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R5312

I might be missing something of course.

At least removing the condition doesn’t break any existing tests.

Not very comforting :)

Crypt-iQ commented at 2:30 pm on October 1, 2025: contributor

I am (still) curious why the !already_in_flight && !CanDirectFetch() conditional was introduced and I wasn’t able to find an explanation in #8068 (because I don’t understand it, I’m not advocating for changing it). The check to see if we’ve requested the block doesn’t distinguish between if we requested it with MSG_CMPCT_BLOCK or MSG_BLOCK in the GETDATA. It also doesn’t tell us if we requested the block from the peer sending the compact block.

There are three places in the original PR (excluding full block fallback in compact block logic) where we’d request full blocks or compact blocks:

in response to a HEADERS here (now HeadersDirectFetchBlocks, full blocks or compact blocks)
in response to an INV here (now removed, full blocks or compact blocks)
FindNextBlocksToDownload (introduced in #4468 and referred to as parallel block download, full blocks only)

The first two cases call CanDirectFetch and if the peer supports compact blocks, we would request a compact block. The HEADERS case has an additional caveat that this be the only block in flight and the only block we’d request from the HEADERS message. Since we’re requesting the block in here and we’ve checked CanDirectFetch, I think this is unrelated to the conditional. If we hypothetically removed the already_in_flight check, since we’ve checked CanDirectFetch before request, we should still be able to process the compact block if we are close to tip (i.e. not almost 20 blocks away and our peer responds quickly).

In the original PR, there is only one place where an unsolicited compact block would be sent: when we have only one block to announce and the peer requested hb mode here. This is now in the SendMessages loop and still exists. An additional place was added in #9375 when NewPoWValidBlock was introduced. It then seems to me that the conditional is a belt-and-suspenders check to fallback to full block download if we’ve somehow fallen behind.

If the already_in_flight check was removed (which, again, I’m not advocating for), the block download state would need to also be wiped so that fallback to parallel block download could still work. Otherwise, the block would not be requested in SendMessages as it’s already in flight!

Lastly, the CanDirectFetch check is interesting. It will fail if our tip time is not within 3h20m of our local clock. It seems to me that if miners consistently created blocks with the lowest possible nTime that MTP would allow, they may be able to cause compact blocks to not be processed network-wide (because both requesting compact blocks relies on CanDirectFetch, and processing unrequested compact blocks relies on CanDirectFetch). I’m not sure if this is rational for them to do.

instagibbs commented at 11:36 am on October 3, 2025: member

Lastly, the CanDirectFetch check is interesting. It will fail if our tip time is not within 3h20m of our local clock.

Yeah this is a known weirdness @dergoegge . I don’t think there’s any real motivation to do it, defectors can “reset” the MTP forward IIUC. Extremely rare blocks or a big drop in hashing could cause it, but in the latter case lower block rate also makes races less likely.

I’ll spend some time next week hopefully trying to mentally tackle this.

compact blocks in IBD resets m_stalling_since #31962