p2p: avoid block disk reads on unnecessary requests #35832

furszy commented at 8:32 PM on July 28, 2026: member

Reject requests that make the node read blocks from disk unnecessarily:

getblocktxn is meant to request the txs a peer is missing. When a peer sends a getblocktxn with an empty index vector, it isn't missing anything, so it shouldn't have sent the message in the first place.
The peer should not request a filtered block when the node does not advertise the NODE_BLOOM service. Filtered blocks are built from the bloom filter, which can be loaded only when the NODE_BLOOM service is offered.

Both are treated as misbehavior now.

Note: can be split in two PRs if preferred.

refactor: split p2p_getdata.py in sub-cases 817aa8ac91

p2p: reject filtered block inv early when bloom is disabled

A peer should not request filtered blocks from a node that
does not advertise NODE_BLOOM. Perform this check before
looking up the block to avoid an unnecessary disk read.

Note: currently, the request is ignored only after the
block has been read from disk, in the bloom filter
existence check.

90f52a5c03

p2p: reject empty getblocktxn requests

A getblocktxn msg is only needed when at least one tx is
missing from a compact block. If no txs are missing, the
block can be reconstructed without sending the request.

This avoids reading the requested block from disk
unnecessarily.

4f099df4e6

DrahtBot added the label P2P on Jul 28, 2026

DrahtBot commented at 8:32 PM on July 28, 2026: 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/35832.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	pinheadmz
Concept ACK	l0rinc, 151henry151

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

jut -> just [typo in the comment “Get block within MAX_BLOCKTXN_DEPTH window, jut not the cached tip”]

in test/functional/p2p_getdata.py:46 in 817aa8ac91

  41 | @@ -42,6 +42,12 @@ def run_test(self):
  42 |          good_getdata.inv.append(CInv(t=2, h=best_block))
  43 |          p2p_block_store.send_and_ping(good_getdata)
  44 |          p2p_block_store.wait_until(lambda: p2p_block_store.blocks[best_block] == 1)
  45 | +        p2p_block_store.peer_disconnect()
  46 | +        p2p_block_store.wait_for_disconnect()

l0rinc commented at 12:18 AM on July 29, 2026:

817aa8a refactor: split p2p_getdata.py in sub-cases:

nit: isn't this a small change in behavior? Can you maybe adjust the commit message https://github.com/bitcoin/bitcoin/blob/31abaa264c35d1779a34d8c110a0c91e62c08ee0/CONTRIBUTING.md?plain=1#L141

in test/functional/p2p_getdata.py:49 in 90f52a5c03

  45 | @@ -45,9 +46,20 @@ def test_invalid_getdata(self):
  46 |          p2p_block_store.peer_disconnect()
  47 |          p2p_block_store.wait_for_disconnect()
  48 |  
  49 | +    def test_filtered_block_disconnects_when_bloom_disabled(self):

l0rinc commented at 2:31 AM on July 29, 2026:

90f52a5 p2p: reject filtered block inv early when bloom is disabled:

The filtered-block request exercises the same no-bloom disconnect policy as the messages already tested in p2p_nobloomfilter_messages.py.

Could we test it there instead and leave p2p_getdata.py unchanged?

<details><summary>keep no-bloom tests together</summary>

diff --git a/test/functional/p2p_nobloomfilter_messages.py b/test/functional/p2p_nobloomfilter_messages.py
index 2d5d7addc7..77618742a7 100755
--- a/test/functional/p2p_nobloomfilter_messages.py
+++ b/test/functional/p2p_nobloomfilter_messages.py
@@ -11,7 +11,7 @@ Test that, when bloom filters are not enabled, peers are disconnected if:
 4. They send a p2p filterclear message
 """
 
-from test_framework.messages import msg_mempool, msg_filteradd, msg_filterload, msg_filterclear
+from test_framework.messages import msg_mempool, msg_filteradd, msg_filterload, msg_filterclear, CInv, MSG_FILTERED_BLOCK, msg_getdata
 from test_framework.p2p import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
@@ -43,6 +43,10 @@ class P2PNoBloomFilterMessages(BitcoinTestFramework):
         self.log.info("Test that peer is disconnected if it sends a filterclear message")
         self.test_message_causes_disconnect(msg_filterclear())
 
+        self.log.info("Test that peer is disconnected if it requests a filtered block")
+        with self.nodes[0].assert_debug_log(['filtered block request received when NODE_BLOOM service disabled']):
+            self.test_message_causes_disconnect(msg_getdata([CInv(MSG_FILTERED_BLOCK, int(self.nodes[0].getbestblockhash(), 16))]))
+
 
 if __name__ == '__main__':
     P2PNoBloomFilterMessages(__file__).main()

</details>

l0rinc commented at 2:52 AM on July 29, 2026: contributor

Concept ACK

Could we characterize both affected behaviors first in preceding tests, to clarify current behavior? After that the fix commits would focus on the behavior change and would only flip the relevant expectations, see #35260. It could help reviewers assess the current behavior and the extent of how the fix changes those assumptions.

151henry151 commented at 4:12 AM on July 29, 2026: contributor

Concept ACK

Suggest a plain disconnect (as in the FILTERLOAD handler) instead of Misbehaving() for both checks: discouragement is address-wide, so one buggy SPV client behind a shared NAT takes its neighbors with it. 3a10d935ac deliberately downgraded the filter* checks on the same principle — "Increasing the banscore and/or banning is too harsh, just disconnecting is enough" — and BIP 111 only prescribes disconnection.

The filtered-block check could also go further: with NODE_BLOOM offered but no filter loaded, the block is still read from disk and discarded (sendMerkleBlock == false). Checking GetTxRelay()->m_bloom_filter here too would cover the remaining wasted read.

The empty-getblocktxn check runs before the m_most_recent_block lookup, so it punishes exactly where BIP 152 says we "MUST respond with either an appropriate blocktxn message, or a full block message" (an empty indexes isn't malformed per spec) — and where master answers from memory without touching disk. Ignoring the request seems more proportionate there.

Tested the behaviors above against both master and this branch with the functional tests below.

<details><summary>test on this branch: Misbehaving/discouragement paths, no-filter disk read</summary>

#!/usr/bin/env python3
"""Run against this branch."""
from test_framework.messages import (
    BlockTransactionsRequest,
    CInv,
    MSG_FILTERED_BLOCK,
    msg_filterload,
    msg_getblocktxn,
    msg_getdata,
)
from test_framework.p2p import P2PInterface
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_equal


class Review35832Demo(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1

    def test_severity_inconsistency(self):
        self.log.info("1a) filterload on non-bloom node: plain disconnect, no Misbehaving")
        node = self.nodes[0]
        self.restart_node(0, ["-peerbloomfilters=0"])

        peer = node.add_p2p_connection(P2PInterface())
        with node.assert_debug_log(expected_msgs=["filterload received despite not offering bloom services"],
                                   unexpected_msgs=["Misbehaving"]):
            peer.send_without_ping(msg_filterload(data=b'\x00' * 10, nHashFuncs=1))
            peer.wait_for_disconnect()

        self.log.info("1b) getdata(MSG_FILTERED_BLOCK) on non-bloom node: Misbehaving -> discouragement path")
        block_hash = int(node.getblockhash(2), 16)
        peer = node.add_p2p_connection(P2PInterface())
        with node.assert_debug_log(expected_msgs=[
                "Misbehaving: peer=",
                "filtered block request received when NODE_BLOOM service disabled",
                # Discouragement engaged; skipped only because the peer address
                # is local. A remote address would hit banman->Discourage().
                "disconnecting but not discouraging local peer"]):
            peer.send_without_ping(msg_getdata([CInv(MSG_FILTERED_BLOCK, block_hash)]))
            peer.wait_for_disconnect()

    def test_empty_getblocktxn_no_disk_read_case(self):
        self.log.info("2) empty getblocktxn for the most-recent block (served from memory on master) is discouraged")
        node = self.nodes[0]
        self.restart_node(0, [])
        # Mine one block so m_most_recent_block is populated.
        self.generate(node, 1)
        tip = int(node.getbestblockhash(), 16)

        peer = node.add_p2p_connection(P2PInterface())
        msg = msg_getblocktxn()
        msg.block_txn_request = BlockTransactionsRequest(blockhash=tip, indexes=[])
        with node.assert_debug_log(expected_msgs=[
                "Misbehaving: peer=",
                "getblocktxn received with no transaction indexes",
                "disconnecting but not discouraging local peer"]):
            peer.send_without_ping(msg)
            peer.wait_for_disconnect()

    def test_bloom_enabled_no_filter_still_reads_disk(self):
        self.log.info("3) NODE_BLOOM enabled, no filter loaded: block still read from disk, no response")
        node = self.nodes[0]
        self.restart_node(0, ["-peerbloomfilters=1"])

        # Deep-ish block so the a_recent_block fast path can't serve it.
        block_hash = int(node.getblockhash(node.getblockcount() - 5), 16)
        peer = node.add_p2p_connection(P2PInterface())
        peer.send_and_ping(msg_getdata([CInv(MSG_FILTERED_BLOCK, block_hash)]))
        # Still connected, no merkleblock and no disconnect: wasted disk read.
        assert_equal(node.getconnectioncount(), 1)
        assert "merkleblock" not in peer.last_message
        assert "block" not in peer.last_message
        peer.peer_disconnect()

    def run_test(self):
        self.test_severity_inconsistency()
        self.test_empty_getblocktxn_no_disk_read_case()
        self.test_bloom_enabled_no_filter_still_reads_disk()


if __name__ == '__main__':
    Review35832Demo(__file__).main()

</details>

<details><summary>baseline on master: silent ignore after wasted read, empty getblocktxn answered from memory</summary>

#!/usr/bin/env python3
"""Run against master."""
from test_framework.messages import (
    BlockTransactionsRequest,
    CInv,
    MSG_FILTERED_BLOCK,
    msg_getblocktxn,
    msg_getdata,
)
from test_framework.p2p import P2PInterface
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_equal


class Review35832MasterBaseline(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1

    def run_test(self):
        node = self.nodes[0]

        self.log.info("1) master: filtered-block getdata on non-bloom node silently ignored (block read from disk for nothing)")
        self.restart_node(0, ["-peerbloomfilters=0"])
        block_hash = int(node.getblockhash(2), 16)
        peer = node.add_p2p_connection(P2PInterface())
        peer.send_and_ping(msg_getdata([CInv(MSG_FILTERED_BLOCK, block_hash)]))
        assert_equal(node.getconnectioncount(), 1)
        assert "merkleblock" not in peer.last_message
        peer.peer_disconnect()

        self.log.info("2) master: empty getblocktxn for tip answered from memory with empty blocktxn")
        self.restart_node(0, [])
        self.generate(node, 1)
        tip = int(node.getbestblockhash(), 16)
        peer = node.add_p2p_connection(P2PInterface())
        msg = msg_getblocktxn()
        msg.block_txn_request = BlockTransactionsRequest(blockhash=tip, indexes=[])
        peer.send_and_ping(msg)
        peer.wait_until(lambda: "blocktxn" in peer.last_message)
        assert_equal(len(peer.last_message["blocktxn"].block_transactions.transactions), 0)
        assert_equal(node.getconnectioncount(), 1)


if __name__ == '__main__':
    Review35832MasterBaseline(__file__).main()

</details>

in test/functional/p2p_compactblocks.py:849 in 4f099df4e6

 842 | @@ -843,6 +843,18 @@ def test_invalid_tx_in_compactblock(self, test_node):
 843 |          msg = msg_cmpctblock(comp_block.to_p2p())
 844 |          test_node.send_await_disconnect(msg)
 845 |  
 846 | +    def test_empty_getblocktxn_disconnects(self):
 847 | +        self.log.info("Testing empty getblocktxn disconnects the peer...")
 848 | +        node = self.nodes[0]
 849 | +        # Get block within MAX_BLOCKTXN_DEPTH window, jut not the cached tip

pinheadmz commented at 7:42 PM on July 29, 2026:

4f099df4e6584e2c178be380eac4f3948845fdf6

typo nit "just".

Also -- why not? What happens if you request the tip?

pinheadmz approved

pinheadmz commented at 7:46 PM on July 29, 2026: member

ACK 4f099df4e6584e2c178be380eac4f3948845fdf6

Built and tested on macos/arm64. Reviewed code changes, two very simple checks at the start of long message processing functions. Tests make sense, fail when malleated and fail on master.

<details><summary>Show Signature</summary>

pinheadmz's public key is on openpgp.org

</details>

DrahtBot requested review from l0rinc on Jul 29, 2026

furszy commented at 1:50 AM on July 30, 2026: member

Could we characterize both affected behaviors first in preceding tests, to clarify current behavior? It could help reviewers assess the current behavior and the extent of how the fix changes those assumptions.

I pondered it before submitting the PR. There is currently no mechanism for a functional test to determine whether a block was read from disk by the node. And, for the first case, checking that a connection remains open for a few seconds would not test anything meaningful, since it could still be closed immediately after whatever delay is chosen. The second case could be characterized with the empty blocktxn response, that shouldn't be a problem.

I do not think these changes justify decoupling net code or introducing complexity to characterize the current first case behavior. Both cases should be clear from the explanation, code and the added tests, which verify the expected behavior.

furszy commented at 3:16 AM on July 30, 2026: member

Suggest a plain disconnect (as in the FILTERLOAD handler) instead of Misbehaving() for both checks: discouragement is address-wide, so one buggy SPV client behind a shared NAT takes its neighbors with it. https://github.com/bitcoin/bitcoin/commit/3a10d935ac8ebabdfd336569d943f042ff84b13e deliberately downgraded the filter* checks on the same principle — "Increasing the banscore and/or banning is too harsh, just disconnecting is enough" — and BIP 111 only prescribes disconnection.

Interesting point. My reasoning was that the peer has received our advertised services without NODE_BLOOM, skips sending filterload, which would immediately disconnect it, and requests a filtered block that cannot be produced without a loaded filter. We can still disconnect instead.

The filtered-block check could also go further: with NODE_BLOOM offered but no filter loaded, the block is still read from disk and discarded (sendMerkleBlock == false). Checking GetTxRelay()->m_bloom_filter here too would cover the remaining wasted read.

I did not introduced this one on purpose. The scenario only exists when NODE_BLOOM is explicitly enabled, opting into BIP37 and its known I/O and CPU asymmetry. I think further hardening of that path should be handled separately. Extra disk reads there are negligible compared to how one can DoS a NODE_BLOOM enabled peer.

The empty-getblocktxn check runs before the m_most_recent_block lookup, so it punishes exactly where BIP152 says we "MUST respond with either an appropriate blocktxn message, or a full block message" (an empty indexes isn't malformed per spec) — and where master answers from memory without touching disk. Ignoring the request seems more proportionate there.

There are separate topics here. Ignoring the request would leave the peer waiting for a response that will never arrive, which is undesirable. Also, an empty indexes getblocktxn seems more like an omission in BIP152 than intentionally supported behavior. The msg is meant to request missing txs, and an empty list requests none.

Since BIP152 does not specify the scenario at the moment, we should either disconnect or forward the full block. Let's think about it.