diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 76fd09843ac7..be6e45e4f477 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -745,39 +745,44 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid return true; } -bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams) +std::optional ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams) { block.SetNull(); // Open history file to read CAutoFile filein(OpenBlockFile(pos, true), SER_DISK, CLIENT_VERSION); if (filein.IsNull()) { - return error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString()); + error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString()); + return std::nullopt; } // Read block try { filein >> block; } catch (const std::exception& e) { - return error("%s: Deserialize or I/O error - %s at %s", __func__, e.what(), pos.ToString()); + error("%s: Deserialize or I/O error - %s at %s", __func__, e.what(), pos.ToString()); + return std::nullopt; } // Check the header - if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams)) { - return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString()); + const uint256 hash{block.GetHash()}; + if (!CheckProofOfWork(hash, block.nBits, consensusParams)) { + error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString()); + return std::nullopt; } - return true; + return hash; } bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams) { const FlatFilePos block_pos{WITH_LOCK(cs_main, return pindex->GetBlockPos())}; - if (!ReadBlockFromDisk(block, block_pos, consensusParams)) { + const auto hash{ReadBlockFromDisk(block, block_pos, consensusParams)}; + if (!hash) { return false; } - if (block.GetHash() != pindex->GetBlockHash()) { + if (*hash != pindex->GetBlockHash()) { return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s", pindex->ToString(), block_pos.ToString()); } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 2cfae856c2cc..8b741b0b2ff8 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -221,7 +222,7 @@ fs::path GetBlockPosFilename(const FlatFilePos& pos); void UnlinkPrunedFiles(const std::set& setFilesToPrune); /** Functions for disk access for blocks */ -bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams); +std::optional ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams); bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams); bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex); diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index 84daeb098541..88867be2e75d 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -202,6 +202,39 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) BOOST_CHECK_EQUAL(sub->m_expected_tip, m_node.chainman->ActiveChain().Tip()->GetBlockHash()); } +/** + * Test that AcceptBlock works correctly when a pre-computed known_hash is + * supplied, exercising the reindex optimization path through AcceptBlockHeader + * and CheckBlock. + */ +BOOST_AUTO_TEST_CASE(checkblock_accept_known_hash) +{ + bool ignored; + BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock( + std::make_shared(Params().GenesisBlock()), true, &ignored)); + + auto good = GoodBlock(Params().GenesisBlock().GetHash()); + const uint256 hash{good->GetHash()}; + + // AcceptBlock with correct known_hash should succeed. + // Do not call CheckBlock beforehand: that would set fChecked=true, + // causing AcceptBlock's internal CheckBlock to short-circuit and skip + // the known_hash path. Keeping fChecked=false mirrors the reindex flow. + { + LOCK(::cs_main); + BlockValidationState state; + CBlockIndex* pindex = nullptr; + bool newblock = false; + BOOST_REQUIRE(m_node.chainman->ActiveChainstate().AcceptBlock( + good, state, &pindex, /*fRequested=*/true, + /*dbp=*/nullptr, &newblock, &hash)); + BOOST_REQUIRE(state.IsValid()); + BOOST_REQUIRE(newblock); + BOOST_REQUIRE(pindex != nullptr); + BOOST_CHECK_EQUAL(pindex->GetBlockHash(), hash); + } +} + /** * Test that mempool updates happen atomically with reorgs. * diff --git a/src/validation.cpp b/src/validation.cpp index d10eac6f8eac..a82a27269197 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3971,18 +3971,21 @@ static bool CheckBlockHeader(const CBlockHeader& block, const uint256& hash, Blo return true; } -bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot) +bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot, const uint256* known_hash) { // These are checks that are independent of context. auto start = Now(); + ASSERT_IF_DEBUG(!known_hash || *known_hash == block.GetHash()); + if (block.fChecked) return true; // Check that the header is valid (particularly PoW). This is mostly // redundant with the call in AcceptBlockHeader. - if (!CheckBlockHeader(block, block.GetHash(), state, consensusParams, fCheckPOW)) + const uint256 hash{known_hash ? *known_hash : block.GetHash()}; + if (!CheckBlockHeader(block, hash, state, consensusParams, fCheckPOW)) return false; // Check the merkle root. @@ -4182,11 +4185,11 @@ static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& stat return true; } -bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationState& state, CBlockIndex** ppindex) +bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationState& state, CBlockIndex** ppindex, const uint256& hash) { AssertLockHeld(cs_main); - // Check for duplicate - uint256 hash = block.GetHash(); + + ASSERT_IF_DEBUG(hash == block.GetHash()); // TODO : ENABLE BLOCK CACHE IN SPECIFIC CASES BlockMap::iterator miSelf{m_blockman.m_block_index.find(hash)}; @@ -4320,7 +4323,7 @@ bool ChainstateManager::ProcessNewBlockHeaders(const std::vector& LOCK(cs_main); for (const CBlockHeader& header : headers) { CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast - bool accepted{AcceptBlockHeader(header, state, &pindex)}; + bool accepted{AcceptBlockHeader(header, state, &pindex, header.GetHash())}; ActiveChainstate().CheckBlockIndex(); if (!accepted) { @@ -4343,7 +4346,7 @@ bool ChainstateManager::ProcessNewBlockHeaders(const std::vector& } /** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */ -bool CChainState::AcceptBlock(const std::shared_ptr& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock) +bool CChainState::AcceptBlock(const std::shared_ptr& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock, const uint256* known_hash) { auto start = Now(); @@ -4355,7 +4358,13 @@ bool CChainState::AcceptBlock(const std::shared_ptr& pblock, Block CBlockIndex *pindexDummy = nullptr; CBlockIndex *&pindex = ppindex ? *ppindex : pindexDummy; - bool accepted_header{m_chainman.AcceptBlockHeader(block, state, &pindex)}; + // If the caller supplies a pre-computed hash, verify it in debug builds. + // In release builds a wrong hash is still caught: AcceptBlockHeader calls + // CheckBlockHeader which runs CheckProofOfWork against the header's nBits. + ASSERT_IF_DEBUG(!known_hash || *known_hash == block.GetHash()); + + const uint256 hash{known_hash ? *known_hash : block.GetHash()}; + bool accepted_header{m_chainman.AcceptBlockHeader(block, state, &pindex, hash)}; CheckBlockIndex(); if (!accepted_header) @@ -4394,7 +4403,7 @@ bool CChainState::AcceptBlock(const std::shared_ptr& pblock, Block if (pindex->nChainWork < nMinimumChainWork) return true; } - if (!CheckBlock(block, state, m_params.GetConsensus()) || + if (!CheckBlock(block, state, m_params.GetConsensus(), true, true, &hash) || !ContextualCheckBlock(block, state, m_chainman, pindex->pprev)) { if (state.IsInvalid() && state.GetResult() != BlockValidationResult::BLOCK_MUTATED) { pindex->nStatus |= BLOCK_FAILED_VALID; @@ -5146,7 +5155,7 @@ void CChainState::LoadExternalBlockFile( nRewind = blkdat.GetPos(); BlockValidationState state; - if (AcceptBlock(pblock, state, nullptr, true, dbp, nullptr)) { + if (AcceptBlock(pblock, state, nullptr, true, dbp, nullptr, &hash)) { nLoaded++; } if (state.IsError()) { @@ -5179,14 +5188,15 @@ void CChainState::LoadExternalBlockFile( while (range.first != range.second) { std::multimap::iterator it = range.first; std::shared_ptr pblockrecursive = std::make_shared(); - if (ReadBlockFromDisk(*pblockrecursive, it->second, m_params.GetConsensus())) { - LogPrint(BCLog::REINDEX, "%s: Processing out of order child %s of %s\n", __func__, pblockrecursive->GetHash().ToString(), + if (auto opt_hash{ReadBlockFromDisk(*pblockrecursive, it->second, m_params.GetConsensus())}) { + const uint256& blockhash = *opt_hash; + LogPrint(BCLog::REINDEX, "%s: Processing out of order child %s of %s\n", __func__, blockhash.ToString(), head.ToString()); LOCK(cs_main); BlockValidationState dummy; - if (AcceptBlock(pblockrecursive, dummy, nullptr, true, &it->second, nullptr)) { + if (AcceptBlock(pblockrecursive, dummy, nullptr, true, &it->second, nullptr, &blockhash)) { nLoaded++; - queue.push_back(pblockrecursive->GetHash()); + queue.push_back(blockhash); } } range.first++; diff --git a/src/validation.h b/src/validation.h index 56bedba032ab..5a9badab22d6 100644 --- a/src/validation.h +++ b/src/validation.h @@ -371,7 +371,7 @@ void InitScriptExecutionCache(); /** Functions for validating blocks and updating the block tree */ /** Context-independent validity checks */ -bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true); +bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true, const uint256* known_hash = nullptr); /** Check a block is completely valid from start to finish (only works on top of our current best block) */ bool TestBlockValidity(BlockValidationState& state, @@ -705,7 +705,7 @@ class CChainState EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) LOCKS_EXCLUDED(::cs_main); - bool AcceptBlock(const std::shared_ptr& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool AcceptBlock(const std::shared_ptr& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock, const uint256* known_hash = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); // Block (dis)connection on a given view: DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -915,7 +915,8 @@ class ChainstateManager bool AcceptBlockHeader( const CBlockHeader& block, BlockValidationState& state, - CBlockIndex** ppindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + CBlockIndex** ppindex, + const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); friend CChainState; public: