From 5d235d50d6dd0cc23175a1484e8ebb6cdc6e2183 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Wed, 28 May 2025 12:55:46 +0200 Subject: [PATCH 1/3] net: assert block hash in `ProcessGetBlockData` and `ProcessMessage` The non-recent-block code path in `ProcessGetBlockData` already has `inv.hash` available (equaling `pindex->GetBlockHash()`). 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 (equaling `pindex->GetBlockHash()`). Pass this hash to `ReadBlock()` for verification and assert that the index lookup matches. Co-authored-by: TheCharlatan Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com> --- src/net_processing.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ead69f086df..8649f564f7c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2298,7 +2298,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } std::shared_ptr 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 pblockRead = std::make_shared(); - 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); From 2371b9f4ee0b108ebbb8afedc47d73ce0f97d272 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Wed, 28 May 2025 14:11:32 +0200 Subject: [PATCH 2/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. --- src/bench/readwriteblock.cpp | 8 +++++--- src/test/util/blockfilter.cpp | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/bench/readwriteblock.cpp b/src/bench/readwriteblock.cpp index 10434b15d20..c0537b1fa69 100644 --- a/src/bench/readwriteblock.cpp +++ b/src/bench/readwriteblock.cpp @@ -42,10 +42,12 @@ static void ReadBlockBench(benchmark::Bench& bench) { const auto testing_setup{MakeNoLogFileContext(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); }); } diff --git a/src/test/util/blockfilter.cpp b/src/test/util/blockfilter.cpp index 8a17f7f4f51..8716af9fdaa 100644 --- a/src/test/util/blockfilter.cpp +++ b/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; } From 9341b5333ad54ccdb7c16802ff06c51b956948e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Wed, 28 May 2025 14:12:04 +0200 Subject: [PATCH 3/3] 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. --- src/node/blockstorage.h | 2 +- src/test/blockmanager_tests.cpp | 6 +++--- src/validation.cpp | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 9405f68c6cf..0e4c13debbc 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -411,7 +411,7 @@ public: void UnlinkPrunedFiles(const std::set& setFilesToPrune) const; /** Functions for disk access for blocks */ - bool ReadBlock(CBlock& block, const FlatFilePos& pos, const std::optional& expected_hash = {}) const; + bool ReadBlock(CBlock& block, const FlatFilePos& pos, const std::optional& expected_hash) const; bool ReadBlock(CBlock& block, const CBlockIndex& index) const; bool ReadRawBlock(std::vector& block, const FlatFilePos& pos) const; diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index d1c64e786dc..d3b58143719 100644 --- a/src/test/blockmanager_tests.cpp +++ b/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); } diff --git a/src/validation.cpp b/src/validation.cpp index 18c775a9db9..35f66271c45 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5168,14 +5168,14 @@ void ChainstateManager::LoadExternalBlockFile( while (range.first != range.second) { std::multimap::iterator it = range.first; std::shared_ptr pblockrecursive = std::make_shared(); - 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++;