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
cmpctblockblocks can be sent unsolicitedly in high-bandwidth mode”
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32606.
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.
775@@ -763,6 +776,7 @@ def announce_cmpct_block(node, peer):
776 assert "getblocktxn" in peer.last_message
777 return block, cmpct_block
778
779+
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
[#28272](https://github.com/bitcoin/bitcoin/issues/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 #28272.
4434@@ -4435,6 +4435,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4435 range_flight.first++;
4436 }
4437
4438+ 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?
0+ 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: https://github.com/bitcoin/bitcoin/blob/6e1a49b66e77c2e6d158c51f51a19ec43cd74707/src/net_processing.cpp#L2780-L2788
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).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.
4434@@ -4435,6 +4435,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4435 range_flight.first++;
4436 }
4437
4438+ if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
4439+ 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):
0 LogDebug(BCLog::NET, "Peer %d%s, not marked as high-bandwidth, sent us an unsolicited compact block!", pfrom.GetId(), pfrom.LogIP(fLogIPs));