net: check for empty header before calling FillBlock

Crypt-iQ commented at 5:54 pm on September 3, 2025: contributor

This avoids an Assume crash if multiple blocktxn messages are received. The first call to FillBlock would make the header empty via SetNull and the call right before the second FillBlock would crash here since LookupBlockIndex won’t find anything. Fix that by checking for an empty header before the Assume.

DrahtBot added the label P2P on Sep 3, 2025

DrahtBot commented at 5:54 pm on September 3, 2025: contributor

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/33296.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	instagibbs, mzumsande

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

in src/net_processing.cpp:3332 in eb8bc005e0 outdated

3328@@ -3329,6 +3329,11 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
3329 
3330         PartiallyDownloadedBlock& partialBlock = *range_flight.first->second.second->partialBlock;
3331 
3332+        if (partialBlock.header.IsNull()) {

instagibbs commented at 5:59 pm on September 3, 2025:

might note that the header is blanked in FillBlock, but the partial block never deleted, which is why this would catch the behavior

Crypt-iQ commented at 6:05 pm on September 3, 2025:

Yup good point, will do.

Crypt-iQ commented at 9:40 pm on September 3, 2025:

Done in 0df02ac0e870bc34c90f95d2a429e6dcc9217dfa

dergoegge commented at 9:06 am on September 4, 2025:

Wondering if we should always wipe the partialBlock (i.e. downgrade the request to a regular block request) when FillBlock fails. Then it would no longer be possible to process two blocktxn messages for the same block.

Crypt-iQ commented at 12:35 pm on September 4, 2025:

@instagibbs suggested this as well offline. I looked into it and the main issue is that it allows a peer to stall block download:

malicious peer sends compact block, reconstruction is required
malicious peer sends blocktxn and reconstruction fails, fallback to regular block request with this peer (and wiping partialBlock)
malicious peer goes back to step 1, preventing honest non-compact-block-relay peers from giving the block. This does not work in current master because of this else statement.

I think there is probably a solution here where the malicious peer couldn’t keep doing this, but it might require some additional state? I think it would be better to not keep the partialBlock around.

mzumsande commented at 10:27 pm on September 4, 2025:

allows a peer to stall block download

I guess this would only be a serious problem in the case where all of our other peers don’t support compact block (because otherwise parallel compact blocks would allow us to download the block).

In any case, this was non-obvious to me. Maybe add a comment in the spot where we fall back to GETDATA explaining that we don’t wipe the partialBlock there on purpose?

Crypt-iQ commented at 12:38 pm on September 5, 2025:

where all of our other peers don’t support compact block

This, or a malicious peer somehow controls all three slots (harder since one slot is outbound), or we are in -blocksonly mode.

In any case, this was non-obvious to me. Maybe add a comment in the spot where we fall back to GETDATA explaining that we don’t wipe the partialBlock there on purpose?

Good idea, I will add a comment. I am not sure if the existing logic is on purpose, but the way it is written in master makes it impossible to stall block download indefinitely since the first_in_flight peer needs to make forward progress after the getdata fallback after 10 minutes or get disconnected.

Crypt-iQ commented at 12:56 pm on September 5, 2025:

It may be possible to call reset() on partialBlock to wipe it. I was assuming that wiping would mean calling RemoveBlockRequest (resets m_downloading_since which allows block download stalling) followed by BlockRequested. I will have to think more on this because I am not sure if it introduces a footgun.

Crypt-iQ commented at 2:01 pm on September 5, 2025:

Comment added in 7c1b388c7ab2059b562c89b1e7c7dd5b641a9cb8

Crypt-iQ force-pushed on Sep 3, 2025

Crypt-iQ marked this as ready for review on Sep 3, 2025

fanquake added this to the milestone 30.0 on Sep 4, 2025

instagibbs commented at 4:11 pm on September 4, 2025: member

concept ACK, as per offline conversations. It would be really nice to make the state machine better, for now we can at least avoid the Assume in debug builds when a peer does this.

We could also disconnect the peer for this behavior, I think, but not a priority

in src/net_processing.cpp:3339 in 0df02ac0e8 outdated

3328@@ -3329,6 +3329,14 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
3329 
3330         PartiallyDownloadedBlock& partialBlock = *range_flight.first->second.second->partialBlock;
3331 
3332+        if (partialBlock.header.IsNull()) {
3333+            // It is possible for the header to be empty if a previous call to FillBlock wiped the header, but left
3334+            // the PartiallyDownloadedBlock pointer around (i.e. did not call RemoveBlockRequest). In this case, we
3335+            // should not call LookupBlockIndex below.
3336+            LogDebug(BCLog::NET, "Peer %d sent compact block transactions multiple times\n", pfrom.GetId());
3337+            return;

mzumsande commented at 10:36 pm on September 4, 2025:

I think this will change behavior in non-debug builds such that repeated blocktxn messages will now be ignored (whereas they would result in READ_STATUS_INVALID / disconnection before). Shouldn’t we rather keep on disconnecting the peer?

Crypt-iQ commented at 12:39 pm on September 5, 2025:

Good point, I will preserve the logic by disconnecting the peer.

Crypt-iQ commented at 2:01 pm on September 5, 2025:

Added this in 7c1b388c7ab2059b562c89b1e7c7dd5b641a9cb8

mzumsande commented at 10:36 pm on September 4, 2025: contributor

Concept ACK

Crypt-iQ force-pushed on Sep 5, 2025

in src/net_processing.cpp:3336 in 7c1b388c7a outdated

3328@@ -3329,6 +3329,16 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
3329 
3330         PartiallyDownloadedBlock& partialBlock = *range_flight.first->second.second->partialBlock;
3331 
3332+        if (partialBlock.header.IsNull()) {
3333+            // It is possible for the header to be empty if a previous call to FillBlock wiped the header, but left
3334+            // the PartiallyDownloadedBlock pointer around (i.e. did not call RemoveBlockRequest). In this case, we
3335+            // should not call LookupBlockIndex below.
3336+            RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId());

instagibbs commented at 2:08 pm on September 5, 2025:

IIUC block requests are cleaned up during disconnect, so this would allow noban/manual connections to re-start trying to give us blocks vs freeze them out?

Crypt-iQ commented at 2:31 pm on September 5, 2025:

Yup, like @mzumsande pointed out, this shouldn’t be a change in behavior since they would previously hit the READ_STATUS_INVALID case.

I didn’t consider noban/manual connections for stalling block download as it seems like this is already possible by triggering either of the READ_STATUS_INVALID cases (whether here or here) and then having SendMessages request it again here.

I think it shouldn’t be allowed for a noban/manual connection to do this, but since this is pre-existing I didn’t really consider it.

net: check for empty header before calling FillBlock

Previously in debug builds, this would cause an Assume crash if
FillBlock had been called previously. This could happen when multiple
blocktxn messages were received.

Co-Authored-By: Greg Sanders <gsanders87@gmail.com>

be0fb2cecb

test: send duplicate blocktxn message in p2p_compactblocks.py

Add test_multiple_blocktxn_response that checks that the peer is
disconnected.

2d31c5ae72

in src/net_processing.cpp:3353 in 7c1b388c7a outdated

3349@@ -3340,6 +3350,11 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
3350         } else if (status == READ_STATUS_FAILED) {
3351             if (first_in_flight) {
3352                 // Might have collided, fall back to getdata now :(
3353+                // If we wipe the partialBlock here via RemoveBlockRequest followed by BlockRequested instead of leaving

instagibbs commented at 2:20 pm on September 5, 2025:

How about something like:

// We keep the failed partialBlock to disallow processing another compact block announcement from the same // peer for the same block. We let the full block download below continue under the same m_downloading_since timer.

Crypt-iQ commented at 2:42 pm on September 5, 2025:

Thanks, this is less verbose. I’ve given commit attribution since I copied the text verbatim.

instagibbs commented at 2:43 pm on September 5, 2025:

thanks, as a future reader I always get suspicious the longer the comment is :)

Crypt-iQ force-pushed on Sep 5, 2025

net: check for empty header before calling FillBlock #33296

Code Coverage & Benchmarks

Reviews