From 6360b5302d2675788de5c4a28ea77d823f6d809e Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Thu, 6 Oct 2022 15:51:09 -0400 Subject: [PATCH 1/5] validation: Change return value of VerifyDB to enum type This does not change behavior. It is in preparation for special handling of the case where VerifyDB doesn't finish for various reasons, but doesn't fail. --- src/node/chainstate.cpp | 14 +++++++++----- src/rpc/blockchain.cpp | 2 +- src/validation.cpp | 39 ++++++++++++++++++++++++--------------- src/validation.h | 7 ++++++- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index ba1024d22e1..245dfee14fc 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -187,12 +187,16 @@ ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const C "Only rebuild the block database if you are sure that your computer's date and time are correct")}; } - if (!CVerifyDB().VerifyDB( - *chainstate, chainman.GetConsensus(), chainstate->CoinsDB(), - options.check_level, - options.check_blocks)) { + VerifyDBResult result = CVerifyDB().VerifyDB( + *chainstate, chainman.GetConsensus(), chainstate->CoinsDB(), + options.check_level, + options.check_blocks); + switch (result) { + case VerifyDBResult::SUCCESS: + break; + case VerifyDBResult::CORRUPTED_BLOCK_DB: return {ChainstateLoadStatus::FAILURE, _("Corrupted block database detected")}; - } + } // no default case, so the compiler can warn about missing cases } } diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 8bee066ab80..e2a1efea55a 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1125,7 +1125,7 @@ static RPCHelpMan verifychain() Chainstate& active_chainstate = chainman.ActiveChainstate(); return CVerifyDB().VerifyDB( - active_chainstate, chainman.GetParams().GetConsensus(), active_chainstate.CoinsTip(), check_level, check_depth); + active_chainstate, chainman.GetParams().GetConsensus(), active_chainstate.CoinsTip(), check_level, check_depth) == VerifyDBResult::SUCCESS; }, }; } diff --git a/src/validation.cpp b/src/validation.cpp index f0ffb748dd6..29299a8eb3a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4057,7 +4057,7 @@ CVerifyDB::~CVerifyDB() uiInterface.ShowProgress("", 100, false); } -bool CVerifyDB::VerifyDB( +VerifyDBResult CVerifyDB::VerifyDB( Chainstate& chainstate, const Consensus::Params& consensus_params, CCoinsView& coinsview, @@ -4066,7 +4066,7 @@ bool CVerifyDB::VerifyDB( AssertLockHeld(cs_main); if (chainstate.m_chain.Tip() == nullptr || chainstate.m_chain.Tip()->pprev == nullptr) { - return true; + return VerifyDBResult::SUCCESS; } // Verify blocks in the best chain @@ -4106,19 +4106,22 @@ bool CVerifyDB::VerifyDB( CBlock block; // check level 0: read from disk if (!ReadBlockFromDisk(block, pindex, consensus_params)) { - return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); + LogPrintf("Verification error: ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + return VerifyDBResult::CORRUPTED_BLOCK_DB; } // check level 1: verify block validity if (nCheckLevel >= 1 && !CheckBlock(block, state, consensus_params)) { - return error("%s: *** found bad block at %d, hash=%s (%s)\n", __func__, - pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); + LogPrintf("Verification error: found bad block at %d, hash=%s (%s)\n", + pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); + return VerifyDBResult::CORRUPTED_BLOCK_DB; } // check level 2: verify undo validity if (nCheckLevel >= 2 && pindex) { CBlockUndo undo; if (!pindex->GetUndoPos().IsNull()) { if (!UndoReadFromDisk(undo, pindex)) { - return error("VerifyDB(): *** found bad undo data at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + LogPrintf("Verification error: found bad undo data at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + return VerifyDBResult::CORRUPTED_BLOCK_DB; } } } @@ -4130,7 +4133,8 @@ bool CVerifyDB::VerifyDB( assert(coins.GetBestBlock() == pindex->GetBlockHash()); DisconnectResult res = chainstate.DisconnectBlock(block, pindex, coins); if (res == DISCONNECT_FAILED) { - return error("VerifyDB(): *** irrecoverable inconsistency in block data at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); + LogPrintf("Verification error: irrecoverable inconsistency in block data at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + return VerifyDBResult::CORRUPTED_BLOCK_DB; } if (res == DISCONNECT_UNCLEAN) { nGoodTransactions = 0; @@ -4142,14 +4146,16 @@ bool CVerifyDB::VerifyDB( skipped_l3_checks = true; } } - if (ShutdownRequested()) return true; + if (ShutdownRequested()) return VerifyDBResult::SUCCESS; } if (pindexFailure) { - return error("VerifyDB(): *** coin database inconsistencies found (last %i blocks, %i good transactions before that)\n", chainstate.m_chain.Height() - pindexFailure->nHeight + 1, nGoodTransactions); + 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; } if (skipped_l3_checks) { LogPrintf("Skipped verification of level >=3 (insufficient database cache size). Consider increasing -dbcache.\n"); } + // store block count as we move pindex at check level >= 4 int block_count = chainstate.m_chain.Height() - pindex->nHeight; @@ -4165,18 +4171,21 @@ bool CVerifyDB::VerifyDB( uiInterface.ShowProgress(_("Verifying blocks…").translated, percentageDone, false); pindex = chainstate.m_chain.Next(pindex); CBlock block; - if (!ReadBlockFromDisk(block, pindex, consensus_params)) - return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString()); - if (!chainstate.ConnectBlock(block, state, pindex, coins)) { - return error("VerifyDB(): *** found unconnectable block at %d, hash=%s (%s)", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); + if (!ReadBlockFromDisk(block, pindex, consensus_params)) { + LogPrintf("Verification error: ReadBlockFromDisk failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); + return VerifyDBResult::CORRUPTED_BLOCK_DB; } - if (ShutdownRequested()) return true; + if (!chainstate.ConnectBlock(block, state, pindex, coins)) { + LogPrintf("Verification error: found unconnectable block at %d, hash=%s (%s)\n", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); + return VerifyDBResult::CORRUPTED_BLOCK_DB; + } + if (ShutdownRequested()) return VerifyDBResult::SUCCESS; } } LogPrintf("Verification: No coin database inconsistencies in last %i blocks (%i transactions)\n", block_count, nGoodTransactions); - return true; + return VerifyDBResult::SUCCESS; } /** 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 7170467b006..7e30fc95de4 100644 --- a/src/validation.h +++ b/src/validation.h @@ -349,12 +349,17 @@ bool HasValidProofOfWork(const std::vector& headers, const Consens /** Return the sum of the work on a given set of headers */ arith_uint256 CalculateHeadersWork(const std::vector& headers); +enum class VerifyDBResult { + SUCCESS, + CORRUPTED_BLOCK_DB, +}; + /** RAII wrapper for VerifyDB: Verify consistency of the block and coin databases */ class CVerifyDB { public: CVerifyDB(); ~CVerifyDB(); - bool VerifyDB( + [[nodiscard]] VerifyDBResult VerifyDB( Chainstate& chainstate, const Consensus::Params& consensus_params, CCoinsView& coinsview, From d6f781f1cfcbc2c2ad5ee289a0642ed00386d013 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Thu, 16 Feb 2023 16:54:26 -0500 Subject: [PATCH 2/5] validation: return VerifyDBResult::INTERRUPTED if verification was interrupted This means that the -verifydb RPC will now return false if it cannot finish due to the node being shutdown. --- src/node/chainstate.cpp | 1 + src/validation.cpp | 4 ++-- src/validation.h | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 245dfee14fc..7d6e0f3bb0e 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -193,6 +193,7 @@ ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const C options.check_blocks); switch (result) { case VerifyDBResult::SUCCESS: + case VerifyDBResult::INTERRUPTED: break; case VerifyDBResult::CORRUPTED_BLOCK_DB: return {ChainstateLoadStatus::FAILURE, _("Corrupted block database detected")}; diff --git a/src/validation.cpp b/src/validation.cpp index 29299a8eb3a..5a6db2674d8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4146,7 +4146,7 @@ VerifyDBResult CVerifyDB::VerifyDB( skipped_l3_checks = true; } } - if (ShutdownRequested()) return VerifyDBResult::SUCCESS; + if (ShutdownRequested()) return VerifyDBResult::INTERRUPTED; } 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); @@ -4179,7 +4179,7 @@ VerifyDBResult CVerifyDB::VerifyDB( LogPrintf("Verification error: found unconnectable block at %d, hash=%s (%s)\n", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); return VerifyDBResult::CORRUPTED_BLOCK_DB; } - if (ShutdownRequested()) return VerifyDBResult::SUCCESS; + if (ShutdownRequested()) return VerifyDBResult::INTERRUPTED; } } diff --git a/src/validation.h b/src/validation.h index 7e30fc95de4..a9977e76e38 100644 --- a/src/validation.h +++ b/src/validation.h @@ -352,6 +352,7 @@ arith_uint256 CalculateHeadersWork(const std::vector& headers); enum class VerifyDBResult { SUCCESS, CORRUPTED_BLOCK_DB, + INTERRUPTED, }; /** RAII wrapper for VerifyDB: Verify consistency of the block and coin databases */ From 0c7785bb2540b69564104767d38342704230cbc2 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Thu, 6 Oct 2022 17:11:02 -0400 Subject: [PATCH 3/5] init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache The rpc command verifychain now fails if the dbcache was not sufficient to complete the verification at the specified level and depth. In the same situation, the VerifyDB check during Init will now fail (and lead to an early shutdown) if the user has explicitly specified -checkblocks or -checklevel but the check couldn't be executed because of the limited cache. If the user didn't change any of the two and is using the defaults, log a warning but don't prevent the node from starting up. --- src/init.cpp | 3 ++- src/node/chainstate.cpp | 5 +++++ src/node/chainstate.h | 9 ++++++++- src/rpc/blockchain.cpp | 2 +- src/test/util/setup_common.cpp | 1 + src/validation.cpp | 3 +++ src/validation.h | 1 + 7 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 5b486854e09..b24ae8606ad 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1502,6 +1502,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) options.prune = chainman.m_blockman.IsPruneMode(); options.check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS); options.check_level = args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL); + options.require_full_verification = args.IsArgSet("-checkblocks") || args.IsArgSet("-checklevel"); options.check_interrupt = ShutdownRequested; options.coins_error_cb = [] { uiInterface.ThreadSafeMessageBox( @@ -1533,7 +1534,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } } - if (status == node::ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB) { + if (status == node::ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB || status == node::ChainstateLoadStatus::FAILURE_INSUFFICIENT_DBCACHE) { return InitError(error); } diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 7d6e0f3bb0e..c521b20b2a5 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -197,6 +197,11 @@ ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const C break; case VerifyDBResult::CORRUPTED_BLOCK_DB: return {ChainstateLoadStatus::FAILURE, _("Corrupted block database detected")}; + case VerifyDBResult::SKIPPED_L3_CHECKS: + if (options.require_full_verification) { + return {ChainstateLoadStatus::FAILURE_INSUFFICIENT_DBCACHE, _("Insufficient dbcache for block verification")}; + } + 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 d3c7656bf2b..7838a62d0c6 100644 --- a/src/node/chainstate.h +++ b/src/node/chainstate.h @@ -25,6 +25,7 @@ struct ChainstateLoadOptions { bool reindex{false}; bool reindex_chainstate{false}; bool prune{false}; + bool require_full_verification{true}; int64_t check_blocks{DEFAULT_CHECKBLOCKS}; int64_t check_level{DEFAULT_CHECKLEVEL}; std::function check_interrupt; @@ -35,7 +36,13 @@ struct ChainstateLoadOptions { //! case, and treat other cases as errors. More complex applications may want to //! try reindexing in the generic failure case, and pass an interrupt callback //! and exit cleanly in the interrupted case. -enum class ChainstateLoadStatus { SUCCESS, FAILURE, FAILURE_INCOMPATIBLE_DB, INTERRUPTED }; +enum class ChainstateLoadStatus { + SUCCESS, + FAILURE, + FAILURE_INCOMPATIBLE_DB, + FAILURE_INSUFFICIENT_DBCACHE, + INTERRUPTED, +}; //! Chainstate load status code and optional error string. using ChainstateLoadResult = std::tuple; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index e2a1efea55a..28a619fe541 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1110,7 +1110,7 @@ static RPCHelpMan verifychain() {"nblocks", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%d, 0=all", DEFAULT_CHECKBLOCKS)}, "The number of blocks to check."}, }, RPCResult{ - RPCResult::Type::BOOL, "", "Verified or not"}, + RPCResult::Type::BOOL, "", "Verification finished successfully. If false, check debug.log for reason."}, RPCExamples{ HelpExampleCli("verifychain", "") + HelpExampleRpc("verifychain", "") diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 6e72f699682..0b347d3141a 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -218,6 +218,7 @@ void TestingSetup::LoadVerifyActivateChainstate() options.prune = chainman.m_blockman.IsPruneMode(); options.check_blocks = m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS); options.check_level = m_args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL); + options.require_full_verification = m_args.IsArgSet("-checkblocks") || m_args.IsArgSet("-checklevel"); auto [status, error] = LoadChainstate(chainman, m_cache_sizes, options); assert(status == node::ChainstateLoadStatus::SUCCESS); diff --git a/src/validation.cpp b/src/validation.cpp index 5a6db2674d8..4da52b7d8b3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4185,6 +4185,9 @@ VerifyDBResult CVerifyDB::VerifyDB( 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; + } return VerifyDBResult::SUCCESS; } diff --git a/src/validation.h b/src/validation.h index a9977e76e38..044b4ef8230 100644 --- a/src/validation.h +++ b/src/validation.h @@ -353,6 +353,7 @@ enum class VerifyDBResult { SUCCESS, CORRUPTED_BLOCK_DB, INTERRUPTED, + SKIPPED_L3_CHECKS, }; /** RAII wrapper for VerifyDB: Verify consistency of the block and coin databases */ From 57ef2a4812f443b2d734f43cebf3ef5038da83f2 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 6 Feb 2023 15:05:33 -0500 Subject: [PATCH 4/5] validation: report if pruning prevents completion of verification Now the verifychain RPC returns false if the checks didn't finish because the blocks requested to be queried have been pruned. --- src/node/chainstate.cpp | 1 + src/validation.cpp | 7 ++++++- src/validation.h | 1 + test/functional/feature_pruning.py | 4 ++-- 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index c521b20b2a5..11898ac0b39 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -194,6 +194,7 @@ ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const C switch (result) { case VerifyDBResult::SUCCESS: case VerifyDBResult::INTERRUPTED: + case VerifyDBResult::SKIPPED_MISSING_BLOCKS: break; case VerifyDBResult::CORRUPTED_BLOCK_DB: return {ChainstateLoadStatus::FAILURE, _("Corrupted block database detected")}; diff --git a/src/validation.cpp b/src/validation.cpp index 4da52b7d8b3..de1a4ce91fa 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4081,6 +4081,7 @@ VerifyDBResult CVerifyDB::VerifyDB( int nGoodTransactions = 0; BlockValidationState state; int reportDone = 0; + bool skipped_no_block_data{false}; bool skipped_l3_checks{false}; LogPrintf("Verification progress: 0%%\n"); @@ -4100,7 +4101,8 @@ VerifyDBResult CVerifyDB::VerifyDB( if ((chainstate.m_blockman.IsPruneMode() || is_snapshot_cs) && !(pindex->nStatus & BLOCK_HAVE_DATA)) { // If pruning or running under an assumeutxo snapshot, only go // back as far as we have data. - LogPrintf("VerifyDB(): block verification stopping at height %d (pruning, no data)\n", pindex->nHeight); + LogPrintf("VerifyDB(): block verification stopping at height %d (no data). This could be due to pruning or use of an assumeutxo snapshot.\n", pindex->nHeight); + skipped_no_block_data = true; break; } CBlock block; @@ -4188,6 +4190,9 @@ VerifyDBResult CVerifyDB::VerifyDB( if (skipped_l3_checks) { return VerifyDBResult::SKIPPED_L3_CHECKS; } + if (skipped_no_block_data) { + return VerifyDBResult::SKIPPED_MISSING_BLOCKS; + } return VerifyDBResult::SUCCESS; } diff --git a/src/validation.h b/src/validation.h index 044b4ef8230..65f59ab2c23 100644 --- a/src/validation.h +++ b/src/validation.h @@ -354,6 +354,7 @@ enum class VerifyDBResult { CORRUPTED_BLOCK_DB, INTERRUPTED, SKIPPED_L3_CHECKS, + SKIPPED_MISSING_BLOCKS, }; /** RAII wrapper for VerifyDB: Verify consistency of the block and coin databases */ diff --git a/test/functional/feature_pruning.py b/test/functional/feature_pruning.py index 664ed779db2..519877ac5b5 100755 --- a/test/functional/feature_pruning.py +++ b/test/functional/feature_pruning.py @@ -223,8 +223,8 @@ class PruneTest(BitcoinTestFramework): def reorg_back(self): # Verify that a block on the old main chain fork has been pruned away assert_raises_rpc_error(-1, "Block not available (pruned data)", self.nodes[2].getblock, self.forkhash) - with self.nodes[2].assert_debug_log(expected_msgs=['block verification stopping at height', '(pruning, no data)']): - self.nodes[2].verifychain(checklevel=4, nblocks=0) + with self.nodes[2].assert_debug_log(expected_msgs=['block verification stopping at height', '(no data)']): + assert not self.nodes[2].verifychain(checklevel=4, nblocks=0) self.log.info(f"Will need to redownload block {self.forkheight}") # Verify that we have enough history to reorg back to the fork point From 0af16e7134459e0820ab95d751093876c1ec4c6d Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Thu, 19 Jan 2023 15:33:15 -0500 Subject: [PATCH 5/5] doc: add release note for #25574 --- doc/release-notes-25574.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 doc/release-notes-25574.md diff --git a/doc/release-notes-25574.md b/doc/release-notes-25574.md new file mode 100644 index 00000000000..312a99d95ba --- /dev/null +++ b/doc/release-notes-25574.md @@ -0,0 +1,13 @@ +Updated settings +---------------- + +If the `-checkblocks` or `-checklevel` options are explicitly provided by the +user, but the verification checks cannot be completed due to an insufficient +dbcache, Bitcoin Core will now return an error at startup. (#25574) + +RPC +--- +The `-verifychain` RPC will now return `false` if the checks didn't fail, +but couldn't be completed at the desired depth and level. This could be due +to missing data while pruning, due to an insufficient dbcache or due to +the node being shutdown before the call could finish. (#25574)