Merge bitcoin/bitcoin#32638: blocks: force hash validations on disk read

9341b53 blockstorage: make block read hash checks explicit (Lőrinc)
2371b9f test/bench: verify hash in `ComputeFilter` reads (Lőrinc)
5d235d5 net: assert block hash in `ProcessGetBlockData` and `ProcessMessage` (Lőrinc)

Pull request description:

  A follow-up to bitcoin/bitcoin#32487 (comment), 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 (but no longer has a default value)

  * 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.

ACKs for top commit:
  maflcko:
    review ACK 9341b53 🕙
  achow101:
    ACK 9341b53
  hodlinator:
    ACK 9341b53
  janb84:
    re ACK 9341b53

Tree-SHA512: cf1d4fff4c15e3f8898ec284929cb83d7e747125d4ee759e77d369f1716728e843ef98030be32c8d608956a96ae2fbefa0e801200c333b9eefd6c086ec032e1f

Author: achow101

src/bench/readwriteblock.cpp

@@ -42,10 +42,12 @@ static void ReadBlockBench(benchmark::Bench& bench)
 {
     const auto testing_setup{MakeNoLogFileContext<const TestingSetup>(ChainType::MAIN)};
     auto& blockman{testing_setup->m_node.chainman->m_blockman};
-    const auto pos{blockman.WriteBlock(CreateTestBlock(), 413'567)};
-    CBlock block;
+    const auto& test_block{CreateTestBlock()};
+    const auto& expected_hash{test_block.GetHash()};
+    const auto& pos{blockman.WriteBlock(test_block, 413'567)};
     bench.run([&] {
-        const auto success{blockman.ReadBlock(block, pos)};
+        CBlock block;
+        const auto success{blockman.ReadBlock(block, pos, expected_hash)};
         assert(success);
     });
 }

src/net_processing.cpp

@@ -2298,7 +2298,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
     }
 
     std::shared_ptr<const CBlock> pblock;
-    if (a_recent_block && a_recent_block->GetHash() == pindex->GetBlockHash()) {
+    if (a_recent_block && a_recent_block->GetHash() == inv.hash) {
         pblock = a_recent_block;
     } else if (inv.IsMsgWitnessBlk()) {
         // Fast-path: in this case it is possible to serve the block directly from disk,
@@ -2318,7 +2318,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
     } else {
         // Send block from disk
         std::shared_ptr<CBlock> pblockRead = std::make_shared<CBlock>();
-        if (!m_chainman.m_blockman.ReadBlock(*pblockRead, block_pos)) {
+        if (!m_chainman.m_blockman.ReadBlock(*pblockRead, block_pos, inv.hash)) {
             if (WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.IsBlockPruned(*pindex))) {
                 LogDebug(BCLog::NET, "Block was pruned before it could be read, %s\n", pfrom.DisconnectMsg(fLogIPs));
             } else {
@@ -2364,7 +2364,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
             // and we don't feel like constructing the object for them, so
             // instead we respond with the full, non-compact block.
             if (can_direct_fetch && pindex->nHeight >= tip->nHeight - MAX_CMPCTBLOCK_DEPTH) {
-                if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) {
+                if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == inv.hash) {
                     MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, *a_recent_compact_block);
                 } else {
                     CBlockHeaderAndShortTxIDs cmpctblock{*pblock, m_rng.rand64()};
@@ -4173,7 +4173,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         if (!block_pos.IsNull()) {
             CBlock block;
-            const bool ret{m_chainman.m_blockman.ReadBlock(block, block_pos)};
+            const bool ret{m_chainman.m_blockman.ReadBlock(block, block_pos, req.blockhash)};
             // If height is above MAX_BLOCKTXN_DEPTH then this block cannot get
             // pruned after we release cs_main above, so this read should never fail.
             assert(ret);

src/node/blockstorage.h

@@ -411,7 +411,7 @@ class BlockManager
     void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune) const;
 
     /** Functions for disk access for blocks */
-    bool ReadBlock(CBlock& block, const FlatFilePos& pos, const std::optional<uint256>& expected_hash = {}) const;
+    bool ReadBlock(CBlock& block, const FlatFilePos& pos, const std::optional<uint256>& expected_hash) const;
     bool ReadBlock(CBlock& block, const CBlockIndex& index) const;
     bool ReadRawBlock(std::vector<std::byte>& block, const FlatFilePos& pos) const;
 

src/test/blockmanager_tests.cpp

@@ -190,12 +190,12 @@ BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file)
     BOOST_CHECK_EQUAL(read_block.nVersion, 0);
     {
         ASSERT_DEBUG_LOG("Errors in block header");
-        BOOST_CHECK(!blockman.ReadBlock(read_block, pos1));
+        BOOST_CHECK(!blockman.ReadBlock(read_block, pos1, {}));
         BOOST_CHECK_EQUAL(read_block.nVersion, 1);
     }
     {
         ASSERT_DEBUG_LOG("Errors in block header");
-        BOOST_CHECK(!blockman.ReadBlock(read_block, pos2));
+        BOOST_CHECK(!blockman.ReadBlock(read_block, pos2, {}));
         BOOST_CHECK_EQUAL(read_block.nVersion, 2);
     }
 
@@ -212,7 +212,7 @@ BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file)
     BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), (TEST_BLOCK_SIZE + STORAGE_HEADER_BYTES) * 2);
 
     // Block 2 was not overwritten:
-    blockman.ReadBlock(read_block, pos2);
+    BOOST_CHECK(!blockman.ReadBlock(read_block, pos2, {}));
     BOOST_CHECK_EQUAL(read_block.nVersion, 2);
 }
 

src/test/util/blockfilter.cpp

@@ -17,7 +17,7 @@ bool ComputeFilter(BlockFilterType filter_type, const CBlockIndex& block_index,
     LOCK(::cs_main);
 
     CBlock block;
-    if (!blockman.ReadBlock(block, block_index.GetBlockPos())) {
+    if (!blockman.ReadBlock(block, block_index)) {
         return false;
     }
 

src/validation.cpp

@@ -5204,14 +5204,14 @@ void ChainstateManager::LoadExternalBlockFile(
                     while (range.first != range.second) {
                         std::multimap<uint256, FlatFilePos>::iterator it = range.first;
                         std::shared_ptr<CBlock> pblockrecursive = std::make_shared<CBlock>();
-                        if (m_blockman.ReadBlock(*pblockrecursive, it->second)) {
-                            LogDebug(BCLog::REINDEX, "%s: Processing out of order child %s of %s\n", __func__, pblockrecursive->GetHash().ToString(),
-                                    head.ToString());
+                        if (m_blockman.ReadBlock(*pblockrecursive, it->second, {})) {
+                            const auto& block_hash{pblockrecursive->GetHash()};
+                            LogDebug(BCLog::REINDEX, "%s: Processing out of order child %s of %s", __func__, block_hash.ToString(), head.ToString());
                             LOCK(cs_main);
                             BlockValidationState dummy;
                             if (AcceptBlock(pblockrecursive, dummy, nullptr, true, &it->second, nullptr, true)) {
                                 nLoaded++;
-                                queue.push_back(pblockrecursive->GetHash());
+                                queue.push_back(block_hash);
                             }
                         }
                         range.first++;