From 9047337d369d800e6eca4d3b686139073a8e8905 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 12 Jun 2023 11:48:27 -0400 Subject: [PATCH 1/2] validation: Stricter assumeutxo error handling in LoadChainstate Make LoadChainstate return an explicit error when snapshot validation succeeds, but there is an error trying to replace the background chainstate with the snapshot chainstate. Previously in this case LoadChainstate would trigger a shutdown and return INTERRUPTED, now it will return an actual error code. There's no real change to behavior other than error message being formatted a little differently. Motivation for this change is to replace error handling via callbacks with error handling via return value ahead of https://github.com/bitcoin/bitcoin/pull/27861 --- src/init.cpp | 2 +- src/node/chainstate.cpp | 2 +- src/node/chainstate.h | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 38e1dbb4a2c..234e3386407 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1530,7 +1530,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } } - if (status == node::ChainstateLoadStatus::FAILURE_INCOMPATIBLE_DB || status == node::ChainstateLoadStatus::FAILURE_INSUFFICIENT_DBCACHE) { + if (status == node::ChainstateLoadStatus::FAILURE_FATAL || 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 8f997b05947..3900d2e6200 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -207,7 +207,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize } else if (snapshot_completion == SnapshotCompletionResult::SUCCESS) { LogPrintf("[snapshot] cleaning up unneeded background chainstate, then reinitializing\n"); if (!chainman.ValidatedSnapshotCleanup()) { - AbortNode("Background chainstate cleanup failed unexpectedly."); + return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Background chainstate cleanup failed unexpectedly.")}; } // Because ValidatedSnapshotCleanup() has torn down chainstates with diff --git a/src/node/chainstate.h b/src/node/chainstate.h index 77240cafe93..2e35035c283 100644 --- a/src/node/chainstate.h +++ b/src/node/chainstate.h @@ -42,7 +42,8 @@ struct ChainstateLoadOptions { //! and exit cleanly in the interrupted case. enum class ChainstateLoadStatus { SUCCESS, - FAILURE, + FAILURE, //!< Generic failure which reindexing may fix + FAILURE_FATAL, //!< Fatal error which should not prompt to reindex FAILURE_INCOMPATIBLE_DB, FAILURE_INSUFFICIENT_DBCACHE, INTERRUPTED, From 1c7d08b9acd33aff343228ada7e058e606cb1062 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 12 Jun 2023 11:48:27 -0400 Subject: [PATCH 2/2] validation: Stricter assumeutxo error handling in InvalidateCoinsDBOnDisk Currently InvalidateCoinsDBOnDisk is calling AbortNode without an error to the caller if it fails. Change it to return just return util::Result, and update the caller to handle the error itself. This causes the secondary error to be shown below the main error instead of the other way around. --- src/validation.cpp | 16 ++++++++++------ src/validation.h | 3 ++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index d9a0fce34f2..a2e55add20e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5411,7 +5411,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( "restart, the node will resume syncing from %d " "without using any snapshot data. " "Please report this incident to %s, including how you obtained the snapshot. " - "The invalid snapshot chainstate has been left on disk in case it is " + "The invalid snapshot chainstate will be left on disk in case it is " "helpful in diagnosing the issue that caused this error."), PACKAGE_NAME, snapshot_tip_height, snapshot_base_height, snapshot_base_height, PACKAGE_BUGREPORT ); @@ -5424,7 +5424,10 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( assert(!this->IsUsable(m_snapshot_chainstate.get())); assert(this->IsUsable(m_ibd_chainstate.get())); - m_snapshot_chainstate->InvalidateCoinsDBOnDisk(); + auto rename_result = m_snapshot_chainstate->InvalidateCoinsDBOnDisk(); + if (!rename_result) { + user_error = strprintf(Untranslated("%s\n%s"), user_error, util::ErrorString(rename_result)); + } shutdown_fnc(user_error); }; @@ -5626,7 +5629,7 @@ bool IsBIP30Unspendable(const CBlockIndex& block_index) (block_index.nHeight==91812 && block_index.GetBlockHash() == uint256S("0x00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f")); } -void Chainstate::InvalidateCoinsDBOnDisk() +util::Result Chainstate::InvalidateCoinsDBOnDisk() { AssertLockHeld(::cs_main); // Should never be called on a non-snapshot chainstate. @@ -5655,13 +5658,14 @@ void Chainstate::InvalidateCoinsDBOnDisk() LogPrintf("%s: error renaming file '%s' -> '%s': %s\n", __func__, src_str, dest_str, e.what()); - AbortNode(strprintf( + return util::Error{strprintf(_( "Rename of '%s' -> '%s' failed. " "You should resolve this by manually moving or deleting the invalid " "snapshot directory %s, otherwise you will encounter the same error again " - "on the next startup.", - src_str, dest_str, src_str)); + "on the next startup."), + src_str, dest_str, src_str)}; } + return {}; } const CBlockIndex* ChainstateManager::GetSnapshotBaseBlock() const diff --git a/src/validation.h b/src/validation.h index cb5b291daba..7b215dea6b7 100644 --- a/src/validation.h +++ b/src/validation.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include @@ -810,7 +811,7 @@ private: * In case of an invalid snapshot, rename the coins leveldb directory so * that it can be examined for issue diagnosis. */ - void InvalidateCoinsDBOnDisk() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + [[nodiscard]] util::Result InvalidateCoinsDBOnDisk() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); friend ChainstateManager; };