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.
| Type | Reviewers |
|---|---|
| ACK | polespinasa |
| Concept ACK | jonatack, mzumsande, Crypt-iQ, hodlinator, instagibbs, ajtowns |
| Stale ACK | w0xlt |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
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.
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
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.
I think this check is still permissive enough such that a
NetMsgType::CMPCTBLOCKmessage can be sent to a node in -blocksonly mode and still get processed. T
Should we only send SENDCMPCT hb=false after VERACK if we’re not blocksonly? Making all the compact block stuff conditional on not being -blocksonly=1 would make sense to me, but probably isn’t needed for this PR.
Should we only send SENDCMPCT hb=false after VERACK if we’re not blocksonly?
I think this breaks -blocksonly nodes being able to send compact blocks since m_provides_cmpctblocks is never set (a peer won’t set them as HB)? I can PR the -blocksonly change I suggested above separately if it’s tangential.
Future improvements to fully “lock down” this code path could be to:
m_opts.ignore_incoming_txsm_provides_cmpctblocks is setMSG_CMPCT_BLOCK (requires state)
What if in the case of being in -blocksonly mode and receive a CMCTBLOCK message we instantly respond with a GETBLOCKTXN asking for all transactions (skipping all InitData logic)?
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index dad2c4ce6d..d46c9b003b 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -4413,6 +4413,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
5 }
6
7 if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
8+ if (m_opts.ignore_incoming_txs) {
9+ BlockTransactionsRequest req;
10+ for (size_t i = 0; i < cmpctblock.BlockTxCount(); i++) {
11+ req.indexes.push_back(i);
12+ }
13+ req.blockhash = pindex->GetBlockHash();
14+ MakeAndPushMessage(pfrom, NetMsgType::GETBLOCKTXN, vInv);
15+ return;
16+ }
17 LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s, not marked as high-bandwidth, sent us an unsolicited compact block!", pfrom.GetId(), pfrom.LogIP(fLogIPs));
18 return;
19 }
It doesn’t really ever make sense to process a CMPCTBLOCK message as blocks-only, the peer should just send the block, if we want to do anything with the message as a blocks-only, we should just process the header of the CMPCTBLOCK:
0fRevertToHeaderProcessing = true;
This will call ProcessHeadersMessage() and that can request the block.
Agree that it doesn’t make sense to process, we should drop it asap. Haven’t tested either of the below things:
ProcessHeadersMessage will trigger a GETDATA and if the block turns out to be invalid, our peer who sent us the CMPCTBLOCK won’t respond and will get disconnected for stalling. If our peer is modified to relay these in the first place to all peers, they could isolate themselves from -blocksonly nodes (assuming they aren’t a mining node and don’t validate what they send out). This is #13370.
A tangent, but I think a miner could trigger this statement in honest peer’s connections if they mined and withheld enough blocks (~4-5) and if the last CMPCTBLOCK is for an invalid block. I’m leaving out a few steps and can write out what I think on the linked issue if it’s worth addressing?
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));
@ajtowns I’m not sure which suggestion you mean, I missed the suggestion to drop the newline, but in the current branch (5caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8) this is a CMPCTBLOCK message (which seems right to me) and IP’s are logged: https://github.com/bitcoin/bitcoin/blob/5caaefd/src/net_processing.cpp#L2484
Edit: My mistake, I was looking at the wrong log message 🤦, thanks for catching this.
0$ git rev-parse HEAD
15caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8
2$ grep -n not.marked net_processing.cpp
34416: LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());
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.
266@@ -267,9 +267,13 @@ def check_announcement_of_new_block(node, peer, predicate):
267
268 # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
269 def test_invalid_cmpctblock_message(self):
270+ self.make_peer_hb_to_candidate(self.nodes[0], self.segwit_node)
271 self.generate(self.nodes[0], COINBASE_MATURITY + 1)
272 block = self.build_block_on_tip(self.nodes[0])
273
274+ self.segwit_node.send_header_for_blocks([block])
275+ self.segwit_node.wait_for_getdata([block.sha256], timeout=30)
AttributeError: 'CBlock' object has no attribute 'sha256'
2480@@ -2481,7 +2481,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
2481 tx_requested_size += resp.txn[i]->GetTotalSize();
2482 }
2483
2484- LogDebug(BCLog::CMPCTBLOCK, "Peer %d sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
2485+ LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), pfrom.LogIP(fLogIPs), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
nit: Avoid explicit \n.
0 LogDebug(BCLog::CMPCTBLOCK, "Peer %d%s sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)", pfrom.GetId(), pfrom.LogIP(fLogIPs), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
4411@@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4412 range_flight.first++;
4413 }
4414
4415+ if (!requested_block_from_this_peer && !pfrom.m_bip152_highbandwidth_to) {
4416+ LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());
4417+ return;
Misbehaving() or disconnect them, but there’s a tiny chance they may have recently been an HB peer (as remarked in #32606 (comment)).
Haven’t tested. I think this will cause extra log messages if:
HEADERS from LB peer=0GETDATA to peer=0CMPCTBLOCK from HB peer=1, it reconstructs, clearing download state for all peers for this blockCMPCTBLOCK from LB peer=0Might confuse a user, but nothing is actually wrong here?
264@@ -265,9 +265,13 @@ def check_announcement_of_new_block(node, peer, predicate):
265
266 # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
267 def test_invalid_cmpctblock_message(self):
268+ self.make_peer_hb_to_candidate(self.nodes[0], self.segwit_node)
This line is unnecessary, it can be replaced with something like:
0 self.assert_highbandwidth_states(self.nodes[0], idx=0, hb_to=True, hb_from=True)
self.segwit_node was disconnected in the prior test it’s not yet HB, plus it doesn’t look like it has sent over sendcmpct yet. I agree that we should assert that it is marked as HB though.
@hodlinator’s suggested change passes, because segwit node’s index is not 0 any more, it just got connected so it’s -1, 0 is some other node that’s already HB. And the test passes without making segwit_node HB since I’ve modified the test to actually be solicited, since the peer sends a header and waits for getdata. Should I add separate test cases for DC’ing invalid cmpctblock’s both unsolicited from HB’s and solicited from non-HB’s?
It also seems weird that we’ll accept a CMPCTBLOCK from someone that hasn’t given us a SENDCMPCT announcing support for it, but that’s probably fine, I’m thinking out loud but maybe there’s some way in which that’s an issue if later versions of the CMPCTBLOCK message are introduced in the future.
Edit: This was already pointed out by @Crypt-iQ below: #32606 (review)
In my opinion, the statefulness of p2p_compactblocks.py in general is pretty awful and would be worth fixing in a separate PR.
This seems to be lacking precision now that we ignore some CMPCTBLOCK messages.
0--- a/test/functional/p2p_compactblocks.py
1+++ b/test/functional/p2p_compactblocks.py
2@@ -904,7 +904,7 @@ class CompactBlocksTest(BitcoinTestFramework):
3 node = self.nodes[0]
4 assert len(self.utxos)
5
6- def announce_cmpct_block(node, peer, txn_count):
7+ def announce_cmpct_block(node, peer, txn_count, expect_success):
8 utxo = self.utxos.pop(0)
9 block = self.build_block_with_transactions(node, utxo, txn_count)
10
11@@ -914,12 +914,18 @@ class CompactBlocksTest(BitcoinTestFramework):
12 peer.send_and_ping(msg)
13 with p2p_lock:
14 assert "getblocktxn" in peer.last_message
15+ if expect_success:
16+ assert peer.last_message["getblocktxn"].block_txn_request.blockhash == block.hash_int, \
17+ f'Mismatching blocks - message: {peer.last_message["getblocktxn"].block_txn_request.blockhash:x}, expected: {block.hash_hex}'
18+ else:
19+ assert peer.last_message["getblocktxn"].block_txn_request.blockhash != block.hash_int
20+
21 return block, cmpct_block
22
23 for name, peer in [("delivery", delivery_peer), ("inbound", inbound_peer), ("outbound", outbound_peer)]:
24 self.log.info(f"Setting {name} as high bandwidth peer")
25 self.make_peer_hb_to_candidate(node, peer)
26- block, cmpct_block = announce_cmpct_block(node, peer, 1)
27+ block, cmpct_block = announce_cmpct_block(node, peer, 1, expect_success=True)
28 msg = msg_blocktxn()
29 msg.block_transactions.blockhash = block.hash_int
30 msg.block_transactions.transactions = block.vtx[1:]
31@@ -933,12 +939,13 @@ class CompactBlocksTest(BitcoinTestFramework):
32 # Remaining low-bandwidth peer is stalling_peer, who announces first
33 assert_equal([peer['bip152_hb_to'] for peer in node.getpeerinfo()], [False, True, True, True])
34
35- block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing)
36+ block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing, expect_success=False)
37
38 delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
39 with p2p_lock:
40 # The second peer to announce should still get a getblocktxn
41 assert "getblocktxn" in delivery_peer.last_message
42+ assert delivery_peer.last_message["getblocktxn"].block_txn_request.blockhash == cmpct_block.header.hash_int
43 assert_not_equal(node.getbestblockhash(), block.hash_hex)
44
45 inbound_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
46@@ -951,6 +958,7 @@ class CompactBlocksTest(BitcoinTestFramework):
47 with p2p_lock:
48 # The third peer to announce should get a getblocktxn if outbound
49 assert "getblocktxn" in outbound_peer.last_message
50+ assert outbound_peer.last_message["getblocktxn"].block_txn_request.blockhash == cmpct_block.header.hash_int
51 assert_not_equal(node.getbestblockhash(), block.hash_hex)
52
53 # Second peer completes the compact block first
0--- a/test/functional/p2p_compactblocks.py
1+++ b/test/functional/p2p_compactblocks.py
2@@ -904,36 +904,42 @@ class CompactBlocksTest(BitcoinTestFramework):
3 node = self.nodes[0]
4 assert len(self.utxos)
5
6- def announce_cmpct_block(node, peer, txn_count):
7+ def announce_cmpct_block(node, peer, txn_count, expect_success):
8 utxo = self.utxos.pop(0)
9 block = self.build_block_with_transactions(node, utxo, txn_count)
10
11+ peer.clear_getblocktxn()
12 cmpct_block = HeaderAndShortIDs()
13 cmpct_block.initialize_from_block(block)
14 msg = msg_cmpctblock(cmpct_block.to_p2p())
15 peer.send_and_ping(msg)
16 with p2p_lock:
17- assert "getblocktxn" in peer.last_message
18+ if expect_success:
19+ assert "getblocktxn" in peer.last_message
20+ else:
21+ assert "getblocktxn" not in peer.last_message
22 return block, cmpct_block
23
24 for name, peer in [("delivery", delivery_peer), ("inbound", inbound_peer), ("outbound", outbound_peer)]:
25 self.log.info(f"Setting {name} as high bandwidth peer")
26 self.make_peer_hb_to_candidate(node, peer)
27- block, cmpct_block = announce_cmpct_block(node, peer, 1)
28+ block, cmpct_block = announce_cmpct_block(node, peer, 1, expect_success=True)
29 msg = msg_blocktxn()
30 msg.block_transactions.blockhash = block.hash_int
31 msg.block_transactions.transactions = block.vtx[1:]
32 peer.send_and_ping(msg)
33 assert_equal(node.getbestblockhash(), block.hash_hex)
34- peer.clear_getblocktxn()
35
36 # Test the simple parallel download case...
37 for num_missing in [1, 5, 20]:
38+ delivery_peer.clear_getblocktxn()
39+ inbound_peer.clear_getblocktxn()
40+ outbound_peer.clear_getblocktxn()
41
42 # Remaining low-bandwidth peer is stalling_peer, who announces first
43 assert_equal([peer['bip152_hb_to'] for peer in node.getpeerinfo()], [False, True, True, True])
44
45- block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing)
46+ block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing, expect_success=False)
47
48 delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
49 with p2p_lock:
50@@ -964,10 +970,6 @@ class CompactBlocksTest(BitcoinTestFramework):
51 stalling_peer.send_and_ping(msg)
52 self.utxos.append([block.vtx[-1].txid_int, 0, block.vtx[-1].vout[0].nValue])
53
54- delivery_peer.clear_getblocktxn()
55- inbound_peer.clear_getblocktxn()
56- outbound_peer.clear_getblocktxn()
57-
58 def test_unsolicited_compact_blocks_ignored(self):
59 node = self.nodes[0]
60 # create new p2p connection for a fresh state w/o any prior sendcmpct messages sent
972+ node = self.nodes[0]
973+ # create new p2p connection for a fresh state w/o any prior sendcmpct messages sent
974+ unsolicited_peer = self.nodes[0].add_p2p_connection(TestP2PConn())
975+
976+ # assert the RPC getpeerinfo boolean fields `bip152_hb_{to, from}`
977+ # match the given parameters for the last peer of a given node
nit:
0 # match the given parameters for the peer at idx of a given node
976+ # assert the RPC getpeerinfo boolean fields `bip152_hb_{to, from}`
977+ # match the given parameters for the last peer of a given node
978+ def assert_highbandwidth_states(node, idx, hb_to, hb_from=False):
979+ peerinfo = node.getpeerinfo()[idx]
980+ assert_equal(peerinfo['bip152_hb_to'], hb_to)
981+ assert_equal(peerinfo['bip152_hb_from'], hb_from)
1005+ # peer is still not selected as high-bandwidth
1006+ assert_highbandwidth_states(node, idx=-1, hb_to=False)
1007+
1008+ block, msg = build_compact_block()
1009+ unsolicited_peer.send_without_ping(msg_headers([block]))
1010+ unsolicited_peer.wait_for_getdata([block.hash_int], timeout=30)
Maybe use a comment to point out that since we only send the header and not the whole block, we are not marked as an HB peer?
And/or use assert_highbandwidth_states(node, idx=-1, hb_to=False) another time at the end of this block to verify/clarify.
unsolicited_peer also hasn’t sent sendcmpct so its m_provides_cmpctblocks is false, and the node still processes the compact block! I am ok with this as-is since imo it is useful to have coverage for this case.
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:
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.
264@@ -265,9 +265,13 @@ def check_announcement_of_new_block(node, peer, predicate):
265
266 # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
267 def test_invalid_cmpctblock_message(self):
268+ self.make_peer_hb_to_candidate(self.nodes[0], self.segwit_node)
269 self.generate(self.nodes[0], COINBASE_MATURITY + 1)
270 block = self.build_block_on_tip(self.nodes[0])
271
272+ self.segwit_node.send_header_for_blocks([block])
273+ self.segwit_node.wait_for_getdata([block.hash_int], timeout=30)
self.segwit_node is marked as HB above?
819@@ -806,6 +820,9 @@ def test_compactblock_reconstruction_stalling_peer(self, stalling_peer, delivery
820 node = self.nodes[0]
821 assert len(self.utxos)
822
823+ self.make_peer_hb_to_candidate(node, stalling_peer)
stalling_peer is already HB
I’m not sure this makes sense, per #28272 (comment) . Perhaps it would be better to:
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.
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.
This is move-only, and allows assert_highbandwidth_states to be used by
other tests.
Modifies the invalid_cmpctblock_message to check that both HB peers
sending unsolicited and non-HB peers sending unsolicited invalid
cmpctblock's get the boot from us. Also refactors the test to make it
less stateful.
Clear the getblocktxn message so we're not checking existing messages,
and check that the hash in the getblocktxn match the cmpctblock being
announced.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
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?
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?
0--- a/src/net_processing.cpp
1+++ b/src/net_processing.cpp
2@@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3 while (range_flight.first != range_flight.second) {
4 if (range_flight.first->second.first == pfrom.GetId()) {
5 requested_block_from_this_peer = true;
6+ const auto& qb_list_it = range_flight.first->second.second;
7+ if (qb_list_it->partialBlock != nullptr) {
8+ LogDebug(BCLog::CMPCTBLOCK, "Ignoring duplicate CMPCTBLOCK message for block %s received from peer=%d%s", pindex->GetBlockHash().ToString(), pfrom.GetId(), pfrom.LogIP(fLogIPs));
9+ return;
10+ }
11 break;
12 }
13 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?
0--- a/src/net_processing.cpp 1+++ b/src/net_processing.cpp 2@@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 3 [...]
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 (review)
Future improvements to fully “lock down” this code path could be to:
- check
m_opts.ignore_incoming_txs- check
m_provides_cmpctblocksis 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:
0sequenceDiagram
1 participant A
2 Us ->> A: SENDCMPCT 0
3 Us ->> B: SENDCMPCT 1 (high-bandwidth)
4 A ->> Us: t=0 HEADERS
5 Us ->> A: GETDATA(CMPCT)
6 B ->> Us: t=25ms CMPCTBLOCK
7 Us ->> B: GETBLOCKTXN
8 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:
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?
code review ACK 32bfd6136134e7194ea0b56ec08498f66829fc40
I think it’s worth resolving the odd case for -blocksonly in this pull request. But I would be happy too with it being done in a follow-up PR.
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
I think yes, except for -blocksonly nodes, where on receipt we wouldn’t punish or anything, but we would ideally drop the message?
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)
I think it would be possible for an attacker to send HEADERS, receive GETDATA, wait for some time to allow new txns to accumulate in the node’s mempool, then send a CMPCTBLOCK with suspected new txns that wouldn’t have been announced yet. But, if the targeted node receives the valid block from any other peer, the download state gets wiped. So this is an edge-of-an-edge case. Since an attacker wants “tight timing information” as you pointed out on the linked issue and this is only able to be done ~10mins (and may not even work all the time if the attacker is not HB), it doesn’t seem urgent.
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
Any restrictive change here introduces a kind of implicit version for compact block relay even if it’s later lifted. I guess the unknown for me is whether this is something that miners (or others) want to do, are planning to do, or if this is a usecase we should support?
+1 to the refactor suggestion. I don’t have any suggestions off-hand. Maybe a new file could be introduced specifically for compact block relay (similar to what was done for blockencodings.cpp)? It would make writing tests a bit easier. Also, checking whether partialBlock is nullptr (and having to reset it in some cases), is awkward.