p2p: Drop unsolicited CMPCTBLOCK from non-HB peer #32606

davidgumberg · 2025-05-23T21:58:15Z

Processing unsolicited CMPCTBLOCK's from a peer that has not been marked high bandwidth is not well-specified behavior in BIP-0152, in fact the BIP seems to imply that it is not permitted:

"[...] method is not useful for compact blocks because cmpctblock blocks can be sent unsolicitedly in high-bandwidth mode"

See https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#separate-version-for-segregated-witness

This partially mitigates a mempool leak described in #28272, making it only possible for peers selected as high bandwidth to discover a node's mempool.

DrahtBot · 2025-05-23T21:58:17Z

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.

Type	Reviewers
ACK	w0xlt
Concept ACK	jonatack, ajtowns, mzumsande, Crypt-iQ, hodlinator

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:

#33083 (qa: test that we do not disconnect a peer for submitting an invalid compact block by darosior)

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.

test/functional/p2p_compactblocks.py

jonatack · 2025-05-23T22:16:22Z

Concept ACK. BIP152 is marked as Final but perhaps could be clarified on this point.

ajtowns · 2025-05-27T04:42:47Z

Concept ACK. LLM linter's punctuation advice seems right to me. :)

mzumsande

Concept ACK

In the commit message:
"making it only possible for peers selected as high bandwidth to discover a node's
mempool" is a bit too strong, since mempools are discoverable, just not transactions that were added to them after the last inv to the peer.

Processing unsolicited CMPCTBLOCK's from a peer that has not been marked high bandwidth is not well-specified behavior in BIP-0152, in fact the BIP seems to imply that it is not permitted: "[...] method is not useful for compact blocks because `cmpctblock` blocks can be sent unsolicitedly in high-bandwidth mode" See https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#separate-version-for-segregated-witness This partially mitigates a mempool leak described in [bitcoin#28272](bitcoin#28272), but that particular issue will persist for peers that have been selected as high bandwidth. This also mitigates potential DoS / bandwidth-wasting / abusive behavior that is discussed in the comments of bitcoin#28272.

davidgumberg · 2025-05-28T01:37:31Z

Rebased to address @ajtowns and @mzumsande feedback.

w0xlt

ACK 1fc95e0

Crypt-iQ

My main concern here is if this disables a FIBRE future; FIBRE apparently used unsolicited compact blocks (see: #28272 (comment)). If we are not concerned about that, then Concept ACK.

Crypt-iQ · 2025-05-28T20:06:38Z

src/net_processing.cpp

@@ -4435,6 +4435,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
            range_flight.first++;
        }

+        if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {


I think this check is still permissive enough such that a NetMsgType::CMPCTBLOCK message can be sent to a node in -blocksonly mode and still get processed. This would require the -blocksonly node to request the block from the peer.

Maybe something like this instead?

+ if ((!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) || m_opts.ignore_incoming_txs) {

Agreed - even if we are in blocks-only mode we might have requested_block_from_this_peer, but we won't be able to reconstruct a block from our barren mempool unless the block is empty (or only contains transactions we originated).

One could special-case it to account for allowing empty block reconstruction on blocks-only nodes but it feels like dead weight in the code.

In case C of the diagram at https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#intended-protocol-flow, we might have explicitly asked for a compact block from a low bandwidth peer. But we appear to not be doing so in blocks-only mode, which makes sense:

bitcoin/src/net_processing.cpp

Lines 2780 to 2788 in 6e1a49b

if (!m_opts.ignore_incoming_txs &&

nodestate->m_provides_cmpctblocks &&

vGetData.size() == 1 &&

mapBlocksInFlight.size() == 1 &&

last_header.pprev->IsValid(BLOCK_VALID_CHAIN)) {

// In any case, we want to download using a compact block, not a regular one

vGetData[0] = CInv(MSG_CMPCT_BLOCK, vGetData[0].hash);

}

MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vGetData);

One could special-case it to account for allowing empty block reconstruction on blocks-only nodes but it feels like dead weight in the code.

Yeah, I feel like the complexity isn't worth it.

dergoegge · 2025-05-29T10:23:33Z

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.

Crypt-iQ · 2025-05-30T17:48:28Z

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.

ajtowns · 2025-05-30T22:42:16Z

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.

hodlinator

Concept ACK 1fc95e0

hodlinator · 2025-06-11T13:13:13Z

src/net_processing.cpp

@@ -4435,6 +4435,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
            range_flight.first++;
        }

+        if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {


Agreed - even if we are in blocks-only mode we might have requested_block_from_this_peer, but we won't be able to reconstruct a block from our barren mempool unless the block is empty (or only contains transactions we originated).

One could special-case it to account for allowing empty block reconstruction on blocks-only nodes but it feels like dead weight in the code.

In case C of the diagram at https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#intended-protocol-flow, we might have explicitly asked for a compact block from a low bandwidth peer. But we appear to not be doing so in blocks-only mode, which makes sense:

bitcoin/src/net_processing.cpp

Lines 2780 to 2788 in 6e1a49b

if (!m_opts.ignore_incoming_txs &&

nodestate->m_provides_cmpctblocks &&

vGetData.size() == 1 &&

mapBlocksInFlight.size() == 1 &&

last_header.pprev->IsValid(BLOCK_VALID_CHAIN)) {

// In any case, we want to download using a compact block, not a regular one

vGetData[0] = CInv(MSG_CMPCT_BLOCK, vGetData[0].hash);

}

MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vGetData);

hodlinator · 2025-06-11T13:43:39Z

src/net_processing.cpp

@@ -4435,6 +4435,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
            range_flight.first++;
        }

+        if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
+            LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());


Might as well include the IP of this suspicious peer (and drop newline):

Suggested change

LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());

LogDebug(BCLog::NET, "Peer %d%s, not marked as high-bandwidth, sent us an unsolicited compact block!", pfrom.GetId(), pfrom.LogIP(fLogIPs));

Crypt-iQ · 2025-06-24T23:19:01Z

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.

DrahtBot · 2025-07-22T09:45:44Z

test/functional/p2p_compactblocks.py

        self.generate(self.nodes[0], COINBASE_MATURITY + 1)
        block = self.build_block_on_tip(self.nodes[0])

+        self.segwit_node.send_header_for_blocks([block])
+        self.segwit_node.wait_for_getdata([block.sha256], timeout=30)


AttributeError: 'CBlock' object has no attribute 'sha256'

DrahtBot · 2025-08-01T09:45:16Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the P2P label May 23, 2025

davidgumberg mentioned this pull request May 23, 2025

compact block fingerprinting #28272

Open

davidgumberg commented May 23, 2025

View reviewed changes

test/functional/p2p_compactblocks.py Outdated Show resolved Hide resolved

mzumsande reviewed May 27, 2025

View reviewed changes

davidgumberg added 2 commits May 27, 2025 18:29

test: p2p: Nodes ignore unsolicited CMPCTBLOCK's

1fc95e0

davidgumberg force-pushed the 5-23-25-ignore-unsolicited branch from 28086e7 to 1fc95e0 Compare May 28, 2025 01:36

w0xlt reviewed May 28, 2025

View reviewed changes

DrahtBot requested review from mzumsande, jonatack and ajtowns May 28, 2025 18:30

Crypt-iQ reviewed May 28, 2025

View reviewed changes

hodlinator reviewed Jun 12, 2025

View reviewed changes

DrahtBot added the CI failed label Jul 21, 2025

DrahtBot reviewed Jul 22, 2025

View reviewed changes

DrahtBot mentioned this pull request Jul 29, 2025

qa: test that we do not disconnect a peer for submitting an invalid compact block #33083

Merged

DrahtBot added the Needs rebase label Aug 1, 2025

	if (!m_opts.ignore_incoming_txs &&
	nodestate->m_provides_cmpctblocks &&
	vGetData.size() == 1 &&
	mapBlocksInFlight.size() == 1 &&
	last_header.pprev->IsValid(BLOCK_VALID_CHAIN)) {
	// In any case, we want to download using a compact block, not a regular one
	vGetData[0] = CInv(MSG_CMPCT_BLOCK, vGetData[0].hash);
	}
	MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vGetData);

	LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());
	LogDebug(BCLog::NET, "Peer %d%s, not marked as high-bandwidth, sent us an unsolicited compact block!", pfrom.GetId(), pfrom.LogIP(fLogIPs));

p2p: Drop unsolicited CMPCTBLOCK from non-HB peer #32606

Are you sure you want to change the base?

p2p: Drop unsolicited CMPCTBLOCK from non-HB peer #32606

Uh oh!

Conversation

davidgumberg commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

jonatack commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented May 27, 2025

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

davidgumberg commented May 28, 2025

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodlinator Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

dergoegge commented May 29, 2025

Uh oh!

Crypt-iQ commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented May 30, 2025

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

hodlinator Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

hodlinator Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ commented Jun 24, 2025

Uh oh!

DrahtBot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 1, 2025

Uh oh!

Uh oh!

davidgumberg commented May 23, 2025 •

edited

Loading

DrahtBot commented May 23, 2025 •

edited

Loading

jonatack commented May 23, 2025 •

edited

Loading

Crypt-iQ left a comment •

edited

Loading

Crypt-iQ May 28, 2025 •

edited

Loading

Crypt-iQ commented May 30, 2025 •

edited

Loading