blocks: force hash validations on disk read #32638

1. 
   l0rinc commented at 9:48 am on May 29, 2025: contributor

   A follow-up to #32487 (review), after which validating the hash of a read block from disk doesn’t incur the cost of calculating its hash anymore.

   Summary

   This PR adds explicit checks that the read block header’s hash matches the one we were expecting.

   Context

   After the previous PR, validating a block’s hash during read operations became essentially free. This PR leverages that by requiring callers to provide a block’s expected hash (or std::nullopt), preventing silent failures caused by corrupted or mismatched data. Most ReadBlock usages were updated with expected hashes and now fail on mismatch.

   Changes

   • added hash assertions in ProcessGetBlockData and ProcessMessage to validate that the block read from disk matches the expected hash;
   • updated tests and benchmark to pass the correct block hash to ReadBlock(), ensuring the hash validation is tested - or none if we already expect PoW failure;
   • removed the default value for expected_hash, requiring an explicit hash for all block reads.

   Why is the hash still optional

   • for header-error tests, where the goal is to trigger failures early in the parsing process;
   • for out-of-order orphan blocks, where the child hash isn’t available before the initial disk read.
2. net: assert block hash in `ProcessGetBlockData` and `ProcessMessage`

   The non-recent-block code path in `ProcessGetBlockData` already has `inv.hash` available.
   Pass it to `ReadBlock()` and assert that the on-disk header matches the requested hash.
   
   The `GETBLOCKTXN` message handler in `ProcessMessage` receives `req.blockhash` from the peer.
   Pass this hash to `ReadBlock()` for verification and assert that the index lookup matches.
   
   Co-authored-by: TheCharlatan <seb.kung@gmail.com>

   af5d30e8bb
3. test/bench: verify hash in `ComputeFilter` reads

   Switch to the index-aware `ReadBlock()` overload in `ComputeFilter` so that filter creation will abort if the stored block header hash doesn't match the expected one.
   
   In the `readwriteblock` benchmark, pass the expected hash to `ReadBlock()` to match the new signature without affecting benchmark performance.

   9ef7d230ee
4. blockstorage: make block read hash checks explicit

   Dropped the default expected_hash parameter from `ReadBlock()`.
   
   In `blockmanager_flush_block_file` tests, we pass {} since the tests would already fail at PoW validation for corrupted blocks.
   
   In `ChainstateManager::LoadExternalBlockFile`, we pass {} when processing child blocks because their hashes aren't known beforehand.

   efbe0e8681
5. 
   DrahtBot commented at 9:48 am on May 29, 2025: 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/32638.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	janb84

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

6. l0rinc renamed this:
   blocks: force hash validations of blocks read from disk explicit
   blocks: force hash validations of blocks read from disk
   on May 29, 2025
7. l0rinc marked this as ready for review on May 29, 2025
8. l0rinc renamed this:
   blocks: force hash validations of blocks read from disk
   blocks: force hash validations on disk read
   on May 29, 2025
9. 
   janb84 commented at 1:13 pm on June 2, 2025: contributor

   ACK efbe0e86810ccbe828472935eb221c2ddf295bf3

   Looks like a good followup PR to use the option to validate the hash as introduced in #32487

   • code reviewed ✅
   • compiled & run tests ✅
10. willcl-ark added the label Block storage on Jun 2, 2025

Contributors
l0rinc DrahtBot janb84

Labels
Block storage