net: serve `MSG_BLOCK` by stripping witnesses in place #36548

Problem: Serving blocks to legacy nodes currently decodes and reserializes them just to remove witnesses. MSG_WITNESS_BLOCK already avoids this work by serving the stored bytes directly. Faster witnessless IBD, as proposed in #36476, also needs peers to serve these blocks efficiently.

Fix: Read disk-backed, non-Signet MSG_BLOCK replies into an owned byte buffer, check the header's proof of work and expected hash, and strip witness ranges from that buffer in place.

Extract a shared helper so decoding transactions and skipping their bytes follow the same encoding rules. Signet keeps ReadBlock for its solution check. Header checks now precede transaction parsing, so records corrupted in both places can log a different error.

Performance:

Time to read a block from disk and build its non-witness message:

The synthetic fixture adds a 512-byte witness item and an empty item to every input.

Related work: This extends the raw witness-block serving added in #13151. The #13011 discussion and the closed #13098 identify unnecessary transaction hashing when serving historical blocks. The implementation also builds on the bulk block reads introduced in #31551.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

External sites

• Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36548
• Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36548.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36548#author-ralph

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #36808 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36808.svg">} (iwyu: Fix warnings in src/test/fuzz and treat them as errors by hebasto)
• #36739 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36739.svg">} (refactor: pass BlockManager to ChainstateManager by pzafonte)
• #36521 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36521.svg">} (Keep witness stacks in serialized form in CScriptWitness by sipa)
• #36074 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36074.svg">} (scripted-diff: [test] Add util/check.h includes for assertions by maflcko)
• #36000 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36000.svg">} (validation: prefetch blocks while connecting by l0rinc)
• #35731 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35731.svg">} (Indexes: Harden the flush-error notification invariant by arejula27)
• #35003 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35003.svg">} (validation: improve block data I/O error handling in P2P paths by furszy)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

• test/functional/p2p_getdata.py: assert len(from_binary(CBlock, full).serialize(with_witness=False)) < len(full) → Prefer assert_greater_than(len(full), len(from_binary(CBlock, full).serialize(with_witness=False))).

^{2026-11-11 08:25:28}

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/38833479928/job/116757258223} _{LLM reason (✨ experimental): CI failed because IWYU modified includes in src/bench/readwriteblock.cpp, and the formatting check detected uncommitted changes.}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

tACK 0b01ce05e162ee61fb0e3c0d8413e0e1af4c1976

I checked that the new MSG_BLOCK reply is byte for byte the old one. On 214 blocks, the head's ReadBlockWithoutWitness matches its ReadBlock plus TX_NO_WITNESS, that old path matches the base's, and btclib, a library I maintain, agrees. The blocks are mainnet 481824, 709632, 774628, 920000 and the bench's 413567, plus 209 regtest blocks with segwit v0 and v1 spends, 1,000 witness items, empty stacks and items, a 3,990,000-byte annex, 300 inputs, 316 outputs, 260 transactions, or no witness. Over P2P, a head node and a base node give the same MSG_BLOCK and MSG_WITNESS_BLOCK payloads for all 209: 208 from disk, and the two successive tips from the recent-block cache.

Each of seven mutants of StripBlockWitness and ReadBlockWithoutWitness fails blockmanager_tests. They range from keeping the marker or the witnesses to skipping the proof-of-work or hash check. The new p2p_getdata case also passes against a base node, so it guards the payload format rather than the new path.

Building the reply got 7.2x to 9.0x faster than on the base for those five mainnet blocks and the two largest regtest blocks (median of three runs, Release, Apple M5). Peak heap fell from 2.9 to 2.0 MB for 481824 and from 7.9 to 4.0 MB for 774628. I measured with a Boost test that writes each block with WriteBlock and reads it back both ways, and a functional test running a head and a base node; I can share both as a gist.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/41212933561/job/124571513411} _{LLM reason (✨ experimental): CI failed because IWYU modified src/bench/readwriteblock.cpp to add missing includes, and the subsequent diff check detected uncommitted changes.}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

tACK d36943b49ce321cd622a8ae5a885227c7b3189cf

The PR's own changes are the ones I tested on 0b01ce05: the range-diff only moves three bench includes into the earlier bench commits and places the new tests before master's new blockmanager_tests case, and the net diff is unchanged line for line. The new base widens block file positions to 64 bits, which the new read path goes through, so I re-ran the byte-for-byte check rather than carry the old result over.

On this head, ReadBlockWithoutWitness matches ReadBlock plus TX_NO_WITNESS for the same five mainnet blocks and for 263 new regtest blocks, and btclib agrees. The regtest blocks include taproot annexes of up to 3,990,001 bytes, witness stacks of 1,001 items, 260 transactions, 300 inputs and 316 outputs, and three blocks with no witness. Over P2P, MSG_BLOCK equals the test framework's non-witness serialization for all 263 blocks read from disk and for 22 of them while they were the tip. MSG_WITNESS_BLOCK equals getblock. blockmanager_tests, p2p_getdata, p2p_segwit and p2p_compactblocks pass. I did not re-run the mutants or the timings. Scripts and output: https://gist.github.com/fametrano/740bc1f88f3d5967023c920824bafd70/29865a5e048c740b1ac50fbc311e2f1607bfaa3d

Rebased + applied some minor Fable 5.2 simplification suggestions.

reACK 52081127364481ed14249e38bd460e725e4b0a40

Since d36943b4, the rebase and the simplifications l0rinc applied leave the non-test changes identical line for line; only the unit tests, the functional test and the bench changed. The new base widens the block file sizes and constants to 64 bits, with the same values.

I re-ran the earlier checks on this head with the same scripts (https://gist.github.com/fametrano/740bc1f88f3d5967023c920824bafd70/29865a5e048c740b1ac50fbc311e2f1607bfaa3d). ReadBlockWithoutWitness still matches ReadBlock plus TX_NO_WITNESS for the five mainnet and 263 regtest blocks, MSG_BLOCK still matches the non-witness serialization for all 263 regtest blocks, and blockmanager_tests, p2p_getdata, p2p_segwit and p2p_compactblocks pass.