From 05438605ba95df14705bf2f57924455f3b2e40e1 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 20 Mar 2024 17:12:09 -0400 Subject: [PATCH] refactor, validation: Return more errors from VerifyDB Return ConnectBlock errors from VerifyDB. --- src/node/chainstate.cpp | 39 ++++++++++++----------- src/node/chainstate.h | 2 +- src/rpc/blockchain.cpp | 5 +-- src/validation.cpp | 69 ++++++++++++++++++++++++++--------------- src/validation.h | 13 +++----- 5 files changed, 72 insertions(+), 56 deletions(-) diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index a82499e0b35fd..6254210da4a7b 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -285,7 +286,7 @@ FlushResult LoadChainstate(ChainstateManag return result; } -util::Result VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options) +FlushResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options) { auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { return options.wipe_chainstate_db || chainstate->CoinsTip().GetBestBlock().IsNull(); @@ -293,7 +294,7 @@ util::Result VerifyLoadedChainstate(Chains LOCK(cs_main); - util::Result result; + FlushResult result; for (Chainstate* chainstate : chainman.GetAll()) { if (!is_coinsview_empty(chainstate)) { const CBlockIndex* tip = chainstate->m_chain.Tip(); @@ -305,27 +306,25 @@ util::Result VerifyLoadedChainstate(Chains return result; } - VerifyDBResult verify_result = CVerifyDB(chainman.GetNotifications()).VerifyDB( + auto verify_result{CVerifyDB(chainman.GetNotifications()).VerifyDB( *chainstate, chainman.GetConsensus(), chainstate->CoinsDB(), options.check_level, - options.check_blocks); - switch (verify_result) { - case VerifyDBResult::SUCCESS: - case VerifyDBResult::SKIPPED_MISSING_BLOCKS: - break; - case VerifyDBResult::INTERRUPTED: - result.Update(Interrupted{}); - return result; - case VerifyDBResult::CORRUPTED_BLOCK_DB: + options.check_blocks) >> result}; + if (verify_result) { + std::visit(util::Overloaded{ + [&](VerifySuccess) {}, + [&](SkippedMissingBlocks) {}, + [&](SkippedL3Checks) { + if (options.require_full_verification) { + result.Update({util::Error{_("Insufficient dbcache for block verification")}, ChainstateLoadError::FAILURE_INSUFFICIENT_DBCACHE}); + } + }, + [&](kernel::Interrupted) { + result.Update(Interrupted{}); + }}, *verify_result); + } else { result.Update({util::Error{_("Corrupted block database detected")}, ChainstateLoadError::FAILURE}); - return result; - case VerifyDBResult::SKIPPED_L3_CHECKS: - if (options.require_full_verification) { - result.Update({util::Error{_("Insufficient dbcache for block verification")}, ChainstateLoadError::FAILURE_INSUFFICIENT_DBCACHE}); - return result; - } - break; - } // no default case, so the compiler can warn about missing cases + } } } diff --git a/src/node/chainstate.h b/src/node/chainstate.h index 38e47192f6a24..b1030c8cbd49d 100644 --- a/src/node/chainstate.h +++ b/src/node/chainstate.h @@ -55,7 +55,7 @@ enum class ChainstateLoadError { kernel::FlushResult LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes, const ChainstateLoadOptions& options); -util::Result VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options); +kernel::FlushResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options); } // namespace node #endif // BITCOIN_NODE_CHAINSTATE_H diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index ebd8800c50210..f6f5844e0fe68 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1189,8 +1189,9 @@ static RPCHelpMan verifychain() LOCK(cs_main); Chainstate& active_chainstate = chainman.ActiveChainstate(); - return CVerifyDB(chainman.GetNotifications()).VerifyDB( - active_chainstate, chainman.GetParams().GetConsensus(), active_chainstate.CoinsTip(), check_level, check_depth) == VerifyDBResult::SUCCESS; + auto verify_result{CVerifyDB(chainman.GetNotifications()).VerifyDB( + active_chainstate, chainman.GetParams().GetConsensus(), active_chainstate.CoinsTip(), check_level, check_depth)}; + return verify_result && std::holds_alternative(*verify_result); }, }; } diff --git a/src/validation.cpp b/src/validation.cpp index 0b43cedbb2267..26ad0b57e5c5c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4813,17 +4813,17 @@ CVerifyDB::~CVerifyDB() m_notifications.progress(bilingual_str{}, 100, false); } -VerifyDBResult CVerifyDB::VerifyDB( +FlushResult CVerifyDB::VerifyDB( Chainstate& chainstate, const Consensus::Params& consensus_params, CCoinsView& coinsview, int nCheckLevel, int nCheckDepth) { AssertLockHeld(cs_main); - FlushResult<> result; // TODO Return this result! + FlushResult result; if (chainstate.m_chain.Tip() == nullptr || chainstate.m_chain.Tip()->pprev == nullptr) { - return VerifyDBResult::SUCCESS; + return result; } // Verify blocks in the best chain @@ -4865,22 +4865,28 @@ VerifyDBResult CVerifyDB::VerifyDB( CBlock block; // check level 0: read from disk if (!chainstate.m_blockman.ReadBlockFromDisk(block, *pindex)) { - LogPrintf("Verification error: ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); - return VerifyDBResult::CORRUPTED_BLOCK_DB; + auto error{Untranslated(strprintf("Verification error: ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()))}; + LogPrintf("%s\n", error.original); + result.Update(util::Error{std::move(error)}); + return result; } // check level 1: verify block validity if (nCheckLevel >= 1 && !CheckBlock(block, state, consensus_params)) { - LogPrintf("Verification error: found bad block at %d, hash=%s (%s)\n", - pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); - return VerifyDBResult::CORRUPTED_BLOCK_DB; + auto error{Untranslated(strprintf("Verification error: found bad block at %d, hash=%s (%s)", + pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()))}; + LogPrintf("%s\n", error.original); + result.Update(util::Error{std::move(error)}); + return result; } // check level 2: verify undo validity if (nCheckLevel >= 2 && pindex) { CBlockUndo undo; if (!pindex->GetUndoPos().IsNull()) { if (!chainstate.m_blockman.UndoReadFromDisk(undo, *pindex)) { - LogPrintf("Verification error: found bad undo data at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); - return VerifyDBResult::CORRUPTED_BLOCK_DB; + auto error{Untranslated(strprintf("Verification error: found bad undo data at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()))}; + LogPrintf("%s\n", error.original); + result.Update(util::Error{std::move(error)}); + return result; } } } @@ -4892,8 +4898,10 @@ VerifyDBResult CVerifyDB::VerifyDB( assert(coins.GetBestBlock() == pindex->GetBlockHash()); DisconnectResult res = chainstate.DisconnectBlock(block, pindex, coins); if (res == DISCONNECT_FAILED) { - LogPrintf("Verification error: irrecoverable inconsistency in block data at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); - return VerifyDBResult::CORRUPTED_BLOCK_DB; + auto error{Untranslated(strprintf("Verification error: irrecoverable inconsistency in block data at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()))}; + LogPrintf("%s\n", error.original); + result.Update(util::Error{std::move(error)}); + return result; } if (res == DISCONNECT_UNCLEAN) { nGoodTransactions = 0; @@ -4905,11 +4913,16 @@ VerifyDBResult CVerifyDB::VerifyDB( skipped_l3_checks = true; } } - if (chainstate.m_chainman.m_interrupt) return VerifyDBResult::INTERRUPTED; + if (chainstate.m_chainman.m_interrupt) { + result.Update(Interrupted{}); + return result; + } } if (pindexFailure) { - LogPrintf("Verification error: coin database inconsistencies found (last %i blocks, %i good transactions before that)\n", chainstate.m_chain.Height() - pindexFailure->nHeight + 1, nGoodTransactions); - return VerifyDBResult::CORRUPTED_BLOCK_DB; + auto error{Untranslated(strprintf("Verification error: coin database inconsistencies found (last %i blocks, %i good transactions before that)", chainstate.m_chain.Height() - pindexFailure->nHeight + 1, nGoodTransactions))}; + LogPrintf("%s\n", error.original); + result.Update(util::Error{std::move(error)}); + return result; } if (skipped_l3_checks) { LogPrintf("Skipped verification of level >=3 (insufficient database cache size). Consider increasing -dbcache.\n"); @@ -4931,26 +4944,32 @@ VerifyDBResult CVerifyDB::VerifyDB( pindex = chainstate.m_chain.Next(pindex); CBlock block; if (!chainstate.m_blockman.ReadBlockFromDisk(block, *pindex)) { - LogPrintf("Verification error: ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); - return VerifyDBResult::CORRUPTED_BLOCK_DB; + auto error{Untranslated(strprintf("Verification error: ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()))}; + LogPrintf("%s\n", error.original); + result.Update(util::Error{std::move(error)}); + return result; } if (!(chainstate.ConnectBlock(block, state, pindex, coins) >> result)) { - LogPrintf("Verification error: found unconnectable block at %d, hash=%s (%s)\n", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); - return VerifyDBResult::CORRUPTED_BLOCK_DB; + auto error{Untranslated(strprintf("Verification error: found unconnectable block at %d, hash=%s (%s)", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()))}; + LogPrintf("%s\n", error.original); + result.Update(util::Error{std::move(error)}); + return result; + } + if (chainstate.m_chainman.m_interrupt) { + result.Update(Interrupted{}); + return result; } - if (chainstate.m_chainman.m_interrupt) return VerifyDBResult::INTERRUPTED; } } LogPrintf("Verification: No coin database inconsistencies in last %i blocks (%i transactions)\n", block_count, nGoodTransactions); if (skipped_l3_checks) { - return VerifyDBResult::SKIPPED_L3_CHECKS; + result.Update(SkippedL3Checks{}); + } else if (skipped_no_block_data) { + result.Update(SkippedMissingBlocks{}); } - if (skipped_no_block_data) { - return VerifyDBResult::SKIPPED_MISSING_BLOCKS; - } - return VerifyDBResult::SUCCESS; + return result; } /** Apply the effects of a block on the utxo cache, ignoring that it may already have been applied. */ diff --git a/src/validation.h b/src/validation.h index 4a90798cc6d85..ee35a8105db98 100644 --- a/src/validation.h +++ b/src/validation.h @@ -405,13 +405,10 @@ bool IsBlockMutated(const CBlock& block, bool check_witness_root); /** Return the sum of the claimed work on a given set of headers. No verification of PoW is done. */ arith_uint256 CalculateClaimedHeadersWork(std::span headers); -enum class VerifyDBResult { - SUCCESS, - CORRUPTED_BLOCK_DB, - INTERRUPTED, - SKIPPED_L3_CHECKS, - SKIPPED_MISSING_BLOCKS, -}; +struct VerifySuccess{}; +struct SkippedL3Checks{}; +struct SkippedMissingBlocks{}; +using VerifyDBResult = std::variant; /** RAII wrapper for VerifyDB: Verify consistency of the block and coin databases */ class CVerifyDB @@ -422,7 +419,7 @@ class CVerifyDB public: explicit CVerifyDB(kernel::Notifications& notifications); ~CVerifyDB(); - [[nodiscard]] VerifyDBResult VerifyDB( + [[nodiscard]] kernel::FlushResult VerifyDB( Chainstate& chainstate, const Consensus::Params& consensus_params, CCoinsView& coinsview,