refactor, validation: Return more errors from VerifyDB

Return ConnectBlock errors from VerifyDB.

src/node/chainstate.cpp

@@ -19,6 +19,7 @@
 #include <util/fs.h>
 #include <util/signalinterrupt.h>
 #include <util/time.h>
+#include <util/overloaded.h>
 #include <util/translation.h>
 #include <validation.h>
 
@@ -253,7 +254,7 @@ FlushResult<InterruptResult, ChainstateLoadError> LoadChainstate(ChainstateManag
     return result;
 }
 
-util::Result<InterruptResult, ChainstateLoadError> VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options)
+FlushResult<InterruptResult, ChainstateLoadError> 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();
@@ -261,7 +262,7 @@ util::Result<InterruptResult, ChainstateLoadError> VerifyLoadedChainstate(Chains
 
     LOCK(cs_main);
 
-    util::Result<InterruptResult, ChainstateLoadError> result;
+    FlushResult<InterruptResult, ChainstateLoadError> result;
     for (Chainstate* chainstate : chainman.GetAll()) {
         if (!is_coinsview_empty(chainstate)) {
             const CBlockIndex* tip = chainstate->m_chain.Tip();
@@ -273,27 +274,25 @@ util::Result<InterruptResult, ChainstateLoadError> 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);
+                options.check_blocks) >> result};
-            switch (verify_result) {
+            if (verify_result) {
-            case VerifyDBResult::SUCCESS:
+                std::visit(util::Overloaded{
-            case VerifyDBResult::SKIPPED_MISSING_BLOCKS:
+                    [&](VerifySuccess) {},
-                break;
+                    [&](SkippedMissingBlocks) {},
-            case VerifyDBResult::INTERRUPTED:
+                    [&](SkippedL3Checks) {
-                result.Update(Interrupted{});
-                return result;
-            case VerifyDBResult::CORRUPTED_BLOCK_DB:
-                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
+                    [&](kernel::Interrupted) {
+                       result.Update(Interrupted{});
+                    }}, *verify_result);
+            } else {
+                result.Update({util::Error{_("Corrupted block database detected")}, ChainstateLoadError::FAILURE});
+            }
         }
     }
 

src/node/chainstate.h

@@ -52,7 +52,7 @@ enum class ChainstateLoadError {
 
 kernel::FlushResult<kernel::InterruptResult, ChainstateLoadError> LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes,
                                                                                  const ChainstateLoadOptions& options);
-util::Result<kernel::InterruptResult, ChainstateLoadError> VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options);
+kernel::FlushResult<kernel::InterruptResult, ChainstateLoadError> VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options);
 } // namespace node
 
 #endif // BITCOIN_NODE_CHAINSTATE_H

src/rpc/blockchain.cpp

@@ -1194,8 +1194,9 @@ static RPCHelpMan verifychain()
     LOCK(cs_main);
 
     Chainstate& active_chainstate = chainman.ActiveChainstate();
-    return CVerifyDB(chainman.GetNotifications()).VerifyDB(
+    auto verify_result{CVerifyDB(chainman.GetNotifications()).VerifyDB(
-               active_chainstate, chainman.GetParams().GetConsensus(), active_chainstate.CoinsTip(), check_level, check_depth) == VerifyDBResult::SUCCESS;
+        active_chainstate, chainman.GetParams().GetConsensus(), active_chainstate.CoinsTip(), check_level, check_depth)};
+    return verify_result && std::holds_alternative<VerifySuccess>(*verify_result);
 },
     };
 }

src/validation.cpp

@@ -4848,17 +4848,17 @@ CVerifyDB::~CVerifyDB()
     m_notifications.progress(bilingual_str{}, 100, false);
 }
 
-VerifyDBResult CVerifyDB::VerifyDB(
+FlushResult<VerifyDBResult> 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<VerifyDBResult> result;
 
     if (chainstate.m_chain.Tip() == nullptr || chainstate.m_chain.Tip()->pprev == nullptr) {
-        return VerifyDBResult::SUCCESS;
+        return result;
     }
 
     // Verify blocks in the best chain
@@ -4900,22 +4900,28 @@ VerifyDBResult CVerifyDB::VerifyDB(
         CBlock block;
         // check level 0: read from disk
         if (!chainstate.m_blockman.ReadBlock(block, *pindex)) {
-            LogPrintf("Verification error: ReadBlock failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString());
+            auto error{Untranslated(strprintf("Verification error: ReadBlock failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()))};
-            return VerifyDBResult::CORRUPTED_BLOCK_DB;
+            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",
+            auto error{Untranslated(strprintf("Verification error: found bad block at %d, hash=%s (%s)",
-                      pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString());
+                      pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()))};
-            return VerifyDBResult::CORRUPTED_BLOCK_DB;
+            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.ReadBlockUndo(undo, *pindex)) {
-                    LogPrintf("Verification error: found bad undo data at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString());
+                    auto error{Untranslated(strprintf("Verification error: found bad undo data at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()))};
-                    return VerifyDBResult::CORRUPTED_BLOCK_DB;
+                    LogPrintf("%s\n", error.original);
+                    result.Update(util::Error{std::move(error)});
+                    return result;
                 }
             }
         }
@@ -4927,8 +4933,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());
+                    auto error{Untranslated(strprintf("Verification error: irrecoverable inconsistency in block data at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()))};
-                    return VerifyDBResult::CORRUPTED_BLOCK_DB;
+                    LogPrintf("%s\n", error.original);
+                    result.Update(util::Error{std::move(error)});
+                    return result;
                 }
                 if (res == DISCONNECT_UNCLEAN) {
                     nGoodTransactions = 0;
@@ -4940,11 +4948,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);
+        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))};
-        return VerifyDBResult::CORRUPTED_BLOCK_DB;
+        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");
@@ -4966,26 +4979,32 @@ VerifyDBResult CVerifyDB::VerifyDB(
             pindex = chainstate.m_chain.Next(pindex);
             CBlock block;
             if (!chainstate.m_blockman.ReadBlock(block, *pindex)) {
-                LogPrintf("Verification error: ReadBlock failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString());
+                auto error{Untranslated(strprintf("Verification error: ReadBlock failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()))};
-                return VerifyDBResult::CORRUPTED_BLOCK_DB;
+                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());
+                auto error{Untranslated(strprintf("Verification error: found unconnectable block at %d, hash=%s (%s)", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()))};
-                return VerifyDBResult::CORRUPTED_BLOCK_DB;
+                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 result;
-        return VerifyDBResult::SKIPPED_MISSING_BLOCKS;
-    }
-    return VerifyDBResult::SUCCESS;
 }
 
 /** Apply the effects of a block on the utxo cache, ignoring that it may already have been applied. */

src/validation.h

@@ -402,13 +402,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<const CBlockHeader> headers);
 
-enum class VerifyDBResult {
+struct VerifySuccess{};
-    SUCCESS,
+struct SkippedL3Checks{};
-    CORRUPTED_BLOCK_DB,
+struct SkippedMissingBlocks{};
-    INTERRUPTED,
+using VerifyDBResult = std::variant<VerifySuccess, kernel::Interrupted, SkippedL3Checks, SkippedMissingBlocks>;
-    SKIPPED_L3_CHECKS,
-    SKIPPED_MISSING_BLOCKS,
-};
 
 /** RAII wrapper for VerifyDB: Verify consistency of the block and coin databases */
 class CVerifyDB
@@ -419,7 +416,7 @@ private:
 public:
     explicit CVerifyDB(kernel::Notifications& notifications);
     ~CVerifyDB();
-    [[nodiscard]] VerifyDBResult VerifyDB(
+    [[nodiscard]] kernel::FlushResult<VerifyDBResult> VerifyDB(
         Chainstate& chainstate,
         const Consensus::Params& consensus_params,
         CCoinsView& coinsview,