From e2d680a32d757de0ef8eb836047a0daa1d82e3c4 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Thu, 1 Jun 2023 16:53:33 -0400 Subject: [PATCH 1/5] util: Add SignalInterrupt class and use in shutdown.cpp This change helps generalize shutdown code so an interrupt can be provided to libbitcoinkernel callers. This may also be useful to eventually de-globalize all of the shutdown code. Co-authored-by: Russell Yanofsky Co-authored-by: TheCharlatan --- src/Makefile.am | 3 ++ src/init.cpp | 4 +- src/kernel/context.cpp | 5 +++ src/kernel/context.h | 14 +++++++ src/qt/test/apptests.cpp | 1 - src/shutdown.cpp | 69 +++++++-------------------------- src/shutdown.h | 2 +- src/util/signalinterrupt.cpp | 74 ++++++++++++++++++++++++++++++++++++ src/util/signalinterrupt.h | 52 +++++++++++++++++++++++++ src/util/threadinterrupt.h | 16 +++++--- 10 files changed, 175 insertions(+), 65 deletions(-) create mode 100644 src/util/signalinterrupt.cpp create mode 100644 src/util/signalinterrupt.h diff --git a/src/Makefile.am b/src/Makefile.am index 85c3eaf08d..fe0b621340 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -310,6 +310,7 @@ BITCOIN_CORE_H = \ util/readwritefile.h \ util/result.h \ util/serfloat.h \ + util/signalinterrupt.h \ util/sock.h \ util/spanparsing.h \ util/string.h \ @@ -733,6 +734,7 @@ libbitcoin_util_a_SOURCES = \ util/moneystr.cpp \ util/rbf.cpp \ util/readwritefile.cpp \ + util/signalinterrupt.cpp \ util/thread.cpp \ util/threadinterrupt.cpp \ util/threadnames.cpp \ @@ -972,6 +974,7 @@ libbitcoinkernel_la_SOURCES = \ util/moneystr.cpp \ util/rbf.cpp \ util/serfloat.cpp \ + util/signalinterrupt.cpp \ util/strencodings.cpp \ util/string.cpp \ util/syserror.cpp \ diff --git a/src/init.cpp b/src/init.cpp index c38352ee38..e9421606ef 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -812,9 +812,7 @@ bool AppInitBasicSetup(const ArgsManager& args, std::atomic& exit_status) // Enable heap terminate-on-corruption HeapSetInformation(nullptr, HeapEnableTerminationOnCorruption, nullptr, 0); #endif - if (!InitShutdownState(exit_status)) { - return InitError(Untranslated("Initializing wait-for-shutdown state failed.")); - } + InitShutdownState(exit_status); if (!SetupNetworking()) { return InitError(Untranslated("Initializing networking failed.")); diff --git a/src/kernel/context.cpp b/src/kernel/context.cpp index 1205da869e..3f4a423531 100644 --- a/src/kernel/context.cpp +++ b/src/kernel/context.cpp @@ -14,9 +14,12 @@ namespace kernel { +Context* g_context; Context::Context() { + assert(!g_context); + g_context = this; std::string sha256_algo = SHA256AutoDetect(); LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo); RandomInit(); @@ -26,6 +29,8 @@ Context::Context() Context::~Context() { ECC_Stop(); + assert(g_context); + g_context = nullptr; } } // namespace kernel diff --git a/src/kernel/context.h b/src/kernel/context.h index f11b7b54f0..e74e0a6f08 100644 --- a/src/kernel/context.h +++ b/src/kernel/context.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_KERNEL_CONTEXT_H #define BITCOIN_KERNEL_CONTEXT_H +#include + #include namespace kernel { @@ -16,12 +18,24 @@ namespace kernel { //! State stored directly in this struct should be simple. More complex state //! should be stored to std::unique_ptr members pointing to opaque types. struct Context { + //! Interrupt object that can be used to stop long-running kernel operations. + util::SignalInterrupt interrupt; + //! Declare default constructor and destructor that are not inline, so code //! instantiating the kernel::Context struct doesn't need to #include class //! definitions for all the unique_ptr members. Context(); ~Context(); }; + +//! Global pointer to kernel::Context for legacy code. New code should avoid +//! using this, and require state it needs to be passed to it directly. +//! +//! Having this pointer is useful because it allows state be moved out of global +//! variables into the kernel::Context struct before all global references to +//! that state are removed. This allows the global references to be removed +//! incrementally, instead of all at once. +extern Context* g_context; } // namespace kernel #endif // BITCOIN_KERNEL_CONTEXT_H diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp index e918e84184..fb8029cb65 100644 --- a/src/qt/test/apptests.cpp +++ b/src/qt/test/apptests.cpp @@ -86,7 +86,6 @@ void AppTests::appTests() // Reset global state to avoid interfering with later tests. LogInstance().DisconnectTestLogger(); - AbortShutdown(); } //! Entry point for BitcoinGUI tests. diff --git a/src/shutdown.cpp b/src/shutdown.cpp index d70017d734..185cad5baf 100644 --- a/src/shutdown.cpp +++ b/src/shutdown.cpp @@ -9,17 +9,15 @@ #include #endif +#include #include #include #include -#include +#include #include -#include #include -#ifdef WIN32 -#include -#endif +#include static std::atomic* g_exit_status{nullptr}; @@ -36,76 +34,37 @@ bool AbortNode(const std::string& strMessage, bilingual_str user_message) return false; } -static std::atomic fRequestShutdown(false); -#ifdef WIN32 -/** On windows it is possible to simply use a condition variable. */ -std::mutex g_shutdown_mutex; -std::condition_variable g_shutdown_cv; -#else -/** On UNIX-like operating systems use the self-pipe trick. - */ -static TokenPipeEnd g_shutdown_r; -static TokenPipeEnd g_shutdown_w; -#endif - -bool InitShutdownState(std::atomic& exit_status) +void InitShutdownState(std::atomic& exit_status) { g_exit_status = &exit_status; -#ifndef WIN32 - std::optional pipe = TokenPipe::Make(); - if (!pipe) return false; - g_shutdown_r = pipe->TakeReadEnd(); - g_shutdown_w = pipe->TakeWriteEnd(); -#endif - return true; } void StartShutdown() { -#ifdef WIN32 - std::unique_lock lk(g_shutdown_mutex); - fRequestShutdown = true; - g_shutdown_cv.notify_one(); -#else - // This must be reentrant and safe for calling in a signal handler, so using a condition variable is not safe. - // Make sure that the token is only written once even if multiple threads call this concurrently or in - // case of a reentrant signal. - if (!fRequestShutdown.exchange(true)) { - // Write an arbitrary byte to the write end of the shutdown pipe. - int res = g_shutdown_w.TokenWrite('x'); - if (res != 0) { - LogPrintf("Sending shutdown token failed\n"); - assert(0); - } + try { + Assert(kernel::g_context)->interrupt(); + } catch (const std::system_error&) { + LogPrintf("Sending shutdown token failed\n"); + assert(0); } -#endif } void AbortShutdown() { - if (fRequestShutdown) { - // Cancel existing shutdown by waiting for it, this will reset condition flags and remove - // the shutdown token from the pipe. - WaitForShutdown(); - } - fRequestShutdown = false; + Assert(kernel::g_context)->interrupt.reset(); } bool ShutdownRequested() { - return fRequestShutdown; + return bool{Assert(kernel::g_context)->interrupt}; } void WaitForShutdown() { -#ifdef WIN32 - std::unique_lock lk(g_shutdown_mutex); - g_shutdown_cv.wait(lk, [] { return fRequestShutdown.load(); }); -#else - int res = g_shutdown_r.TokenRead(); - if (res != 'x') { + try { + Assert(kernel::g_context)->interrupt.wait(); + } catch (const std::system_error&) { LogPrintf("Reading shutdown token failed\n"); assert(0); } -#endif } diff --git a/src/shutdown.h b/src/shutdown.h index c119bee96f..0e51575c5a 100644 --- a/src/shutdown.h +++ b/src/shutdown.h @@ -16,7 +16,7 @@ bool AbortNode(const std::string& strMessage, bilingual_str user_message = bilin /** Initialize shutdown state. This must be called before using either StartShutdown(), * AbortShutdown() or WaitForShutdown(). Calling ShutdownRequested() is always safe. */ -bool InitShutdownState(std::atomic& exit_status); +void InitShutdownState(std::atomic& exit_status); /** Request shutdown of the application. */ void StartShutdown(); diff --git a/src/util/signalinterrupt.cpp b/src/util/signalinterrupt.cpp new file mode 100644 index 0000000000..c551ba8044 --- /dev/null +++ b/src/util/signalinterrupt.cpp @@ -0,0 +1,74 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#ifdef WIN32 +#include +#else +#include +#endif + +#include +#include + +namespace util { + +SignalInterrupt::SignalInterrupt() : m_flag{false} +{ +#ifndef WIN32 + std::optional pipe = TokenPipe::Make(); + if (!pipe) throw std::ios_base::failure("Could not create TokenPipe"); + m_pipe_r = pipe->TakeReadEnd(); + m_pipe_w = pipe->TakeWriteEnd(); +#endif +} + +SignalInterrupt::operator bool() const +{ + return m_flag; +} + +void SignalInterrupt::reset() +{ + // Cancel existing interrupt by waiting for it, this will reset condition flags and remove + // the token from the pipe. + if (*this) wait(); + m_flag = false; +} + +void SignalInterrupt::operator()() +{ +#ifdef WIN32 + std::unique_lock lk(m_mutex); + m_flag = true; + m_cv.notify_one(); +#else + // This must be reentrant and safe for calling in a signal handler, so using a condition variable is not safe. + // Make sure that the token is only written once even if multiple threads call this concurrently or in + // case of a reentrant signal. + if (!m_flag.exchange(true)) { + // Write an arbitrary byte to the write end of the pipe. + int res = m_pipe_w.TokenWrite('x'); + if (res != 0) { + throw std::ios_base::failure("Could not write interrupt token"); + } + } +#endif +} + +void SignalInterrupt::wait() +{ +#ifdef WIN32 + std::unique_lock lk(m_mutex); + m_cv.wait(lk, [this] { return m_flag.load(); }); +#else + int res = m_pipe_r.TokenRead(); + if (res != 'x') { + throw std::ios_base::failure("Did not read expected interrupt token"); + } +#endif +} + +} // namespace util diff --git a/src/util/signalinterrupt.h b/src/util/signalinterrupt.h new file mode 100644 index 0000000000..ca02feda91 --- /dev/null +++ b/src/util/signalinterrupt.h @@ -0,0 +1,52 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_UTIL_SIGNALINTERRUPT_H +#define BITCOIN_UTIL_SIGNALINTERRUPT_H + +#ifdef WIN32 +#include +#include +#else +#include +#endif + +#include +#include + +namespace util { +/** + * Helper class that manages an interrupt flag, and allows a thread or + * signal to interrupt another thread. + * + * This class is safe to be used in a signal handler. If sending an interrupt + * from a signal handler is not necessary, the more lightweight \ref + * CThreadInterrupt class can be used instead. + */ + +class SignalInterrupt +{ +public: + SignalInterrupt(); + explicit operator bool() const; + void operator()(); + void reset(); + void wait(); + +private: + std::atomic m_flag; + +#ifndef WIN32 + // On UNIX-like operating systems use the self-pipe trick. + TokenPipeEnd m_pipe_r; + TokenPipeEnd m_pipe_w; +#else + // On windows use a condition variable, since we don't have any signals there + std::mutex m_mutex; + std::condition_variable m_cv; +#endif +}; +} // namespace util + +#endif // BITCOIN_UTIL_SIGNALINTERRUPT_H diff --git a/src/util/threadinterrupt.h b/src/util/threadinterrupt.h index ccc053f576..0b79b38276 100644 --- a/src/util/threadinterrupt.h +++ b/src/util/threadinterrupt.h @@ -12,11 +12,17 @@ #include #include -/* - A helper class for interruptible sleeps. Calling operator() will interrupt - any current sleep, and after that point operator bool() will return true - until reset. -*/ +/** + * A helper class for interruptible sleeps. Calling operator() will interrupt + * any current sleep, and after that point operator bool() will return true + * until reset. + * + * This class should not be used in a signal handler. It uses thread + * synchronization primitives that are not safe to use with signals. If sending + * an interrupt from a signal handler is necessary, the \ref SignalInterrupt + * class can be used instead. + */ + class CThreadInterrupt { public: From edb55e2777063dfeba0a52bbd0b92af8b4688501 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 17 May 2023 12:43:23 +0200 Subject: [PATCH 2/5] kernel: Pass interrupt reference to chainman This and the following commit seek to decouple the libbitcoinkernel library from the shutdown code. As a library, it should it should have its own flexible interrupt infrastructure without relying on node-wide globals. The commit takes the first step towards this goal by de-globalising `ShutdownRequested` calls in kernel code. Co-authored-by: Russell Yanofsky Co-authored-by: TheCharlatan --- src/bitcoin-chainstate.cpp | 2 +- src/init.cpp | 2 +- src/kernel/mempool_persist.cpp | 4 +- src/node/blockstorage.cpp | 15 +++---- src/node/blockstorage.h | 9 ++++- src/test/blockmanager_tests.cpp | 2 +- src/test/util/setup_common.cpp | 2 +- .../validation_chainstatemanager_tests.cpp | 2 +- src/txdb.cpp | 6 +-- src/txdb.h | 5 ++- src/validation.cpp | 40 ++++++++++--------- src/validation.h | 6 ++- 12 files changed, 55 insertions(+), 40 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 432bdc8e33..da42372a6a 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -113,7 +113,7 @@ int main(int argc, char* argv[]) .chainparams = chainman_opts.chainparams, .blocks_dir = abs_datadir / "blocks", }; - ChainstateManager chainman{chainman_opts, blockman_opts}; + ChainstateManager chainman{kernel_context.interrupt, chainman_opts, blockman_opts}; node::CacheSizes cache_sizes; cache_sizes.block_tree_db = 2 << 20; diff --git a/src/init.cpp b/src/init.cpp index e9421606ef..ac81ebab82 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1462,7 +1462,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) { node.mempool = std::make_unique(mempool_opts); - node.chainman = std::make_unique(chainman_opts, blockman_opts); + node.chainman = std::make_unique(node.kernel->interrupt, chainman_opts, blockman_opts); ChainstateManager& chainman = *node.chainman; node::ChainstateLoadOptions options; diff --git a/src/kernel/mempool_persist.cpp b/src/kernel/mempool_persist.cpp index 71f3aac366..d060e45af3 100644 --- a/src/kernel/mempool_persist.cpp +++ b/src/kernel/mempool_persist.cpp @@ -9,13 +9,13 @@ #include #include #include -#include #include #include #include #include #include #include +#include #include #include @@ -95,7 +95,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active } else { ++expired; } - if (ShutdownRequested()) + if (active_chainstate.m_chainman.m_interrupt) return false; } std::map mapDeltas; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 223b0e6a17..a27a7a5466 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -13,12 +13,12 @@ #include #include #include -#include #include #include #include #include #include +#include #include #include @@ -250,7 +250,8 @@ CBlockIndex* BlockManager::InsertBlockIndex(const uint256& hash) bool BlockManager::LoadBlockIndex() { - if (!m_block_tree_db->LoadBlockIndexGuts(GetConsensus(), [this](const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return this->InsertBlockIndex(hash); })) { + if (!m_block_tree_db->LoadBlockIndexGuts( + GetConsensus(), [this](const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return this->InsertBlockIndex(hash); }, m_interrupt)) { return false; } @@ -260,7 +261,7 @@ bool BlockManager::LoadBlockIndex() CBlockIndexHeightOnlyComparator()); for (CBlockIndex* pindex : vSortedByHeight) { - if (ShutdownRequested()) return false; + if (m_interrupt) return false; pindex->nChainWork = (pindex->pprev ? pindex->pprev->nChainWork : 0) + GetBlockProof(*pindex); pindex->nTimeMax = (pindex->pprev ? std::max(pindex->pprev->nTimeMax, pindex->nTime) : pindex->nTime); @@ -890,8 +891,8 @@ void ThreadImport(ChainstateManager& chainman, std::vector vImportFile } LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile); chainman.ActiveChainstate().LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent); - if (ShutdownRequested()) { - LogPrintf("Shutdown requested. Exit %s\n", __func__); + if (chainman.m_interrupt) { + LogPrintf("Interrupt requested. Exit %s\n", __func__); return; } nFile++; @@ -909,8 +910,8 @@ void ThreadImport(ChainstateManager& chainman, std::vector vImportFile if (file) { LogPrintf("Importing blocks file %s...\n", fs::PathToString(path)); chainman.ActiveChainstate().LoadExternalBlockFile(file); - if (ShutdownRequested()) { - LogPrintf("Shutdown requested. Exit %s\n", __func__); + if (chainman.m_interrupt) { + LogPrintf("Interrupt requested. Exit %s\n", __func__); return; } } else { diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index a1ebb0df0a..36b8aa4806 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -33,6 +33,9 @@ struct FlatFilePos; namespace Consensus { struct Params; } +namespace util { +class SignalInterrupt; +} // namespace util namespace node { @@ -153,10 +156,12 @@ private: public: using Options = kernel::BlockManagerOpts; - explicit BlockManager(Options opts) + explicit BlockManager(const util::SignalInterrupt& interrupt, Options opts) : m_prune_mode{opts.prune_target > 0}, - m_opts{std::move(opts)} {}; + m_opts{std::move(opts)}, + m_interrupt{interrupt} {}; + const util::SignalInterrupt& m_interrupt; std::atomic m_importing{false}; BlockMap m_block_index GUARDED_BY(cs_main); diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index f094766886..631f85908e 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -25,7 +25,7 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) .chainparams = *params, .blocks_dir = m_args.GetBlocksDirPath(), }; - BlockManager blockman{blockman_opts}; + BlockManager blockman{m_node.kernel->interrupt, blockman_opts}; CChain chain {}; // simulate adding a genesis block normally BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, chain, nullptr).nPos, BLOCK_SERIALIZATION_HEADER_SIZE); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 93a60db832..514df73922 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -197,7 +197,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto .chainparams = chainman_opts.chainparams, .blocks_dir = m_args.GetBlocksDirPath(), }; - m_node.chainman = std::make_unique(chainman_opts, blockman_opts); + m_node.chainman = std::make_unique(m_node.kernel->interrupt, chainman_opts, blockman_opts); m_node.chainman->m_blockman.m_block_tree_db = std::make_unique(DBParams{ .path = m_args.GetDataDirNet() / "blocks" / "index", .cache_bytes = static_cast(m_cache_sizes.block_tree_db), diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index b797de46af..f918ed6f95 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -393,7 +393,7 @@ struct SnapshotTestSetup : TestChain100Setup { // For robustness, ensure the old manager is destroyed before creating a // new one. m_node.chainman.reset(); - m_node.chainman = std::make_unique(chainman_opts, blockman_opts); + m_node.chainman = std::make_unique(m_node.kernel->interrupt, chainman_opts, blockman_opts); } return *Assert(m_node.chainman); } diff --git a/src/txdb.cpp b/src/txdb.cpp index b2095bd4a3..e5b5e0b8a1 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -9,8 +9,8 @@ #include #include #include -#include #include +#include #include #include @@ -291,7 +291,7 @@ bool CBlockTreeDB::ReadFlag(const std::string &name, bool &fValue) { return true; } -bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function insertBlockIndex) +bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function insertBlockIndex, const util::SignalInterrupt& interrupt) { AssertLockHeld(::cs_main); std::unique_ptr pcursor(NewIterator()); @@ -299,7 +299,7 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams, // Load m_block_index while (pcursor->Valid()) { - if (ShutdownRequested()) return false; + if (interrupt) return false; std::pair key; if (pcursor->GetKey(key) && key.first == DB_BLOCK_INDEX) { CDiskBlockIndex diskindex; diff --git a/src/txdb.h b/src/txdb.h index 04d0ecb39f..6405437be9 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -29,6 +29,9 @@ class uint256; namespace Consensus { struct Params; }; +namespace util { +class SignalInterrupt; +} // namespace util //! -dbcache default (MiB) static const int64_t nDefaultDbCache = 450; @@ -98,7 +101,7 @@ public: void ReadReindexing(bool &fReindexing); bool WriteFlag(const std::string &name, bool fValue); bool ReadFlag(const std::string &name, bool &fValue); - bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function insertBlockIndex) + bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function insertBlockIndex, const util::SignalInterrupt& interrupt) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); }; diff --git a/src/validation.cpp b/src/validation.cpp index 6836498a64..fc8d292d6a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -50,6 +50,7 @@ #include #include #include +#include #include #include #include @@ -3183,11 +3184,11 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< break; } - // We check shutdown only after giving ActivateBestChainStep a chance to run once so that we - // never shutdown before connecting the genesis block during LoadChainTip(). Previously this - // caused an assert() failure during shutdown in such cases as the UTXO DB flushing checks + // We check interrupt only after giving ActivateBestChainStep a chance to run once so that we + // never interrupt before connecting the genesis block during LoadChainTip(). Previously this + // caused an assert() failure during interrupt in such cases as the UTXO DB flushing checks // that the best block hash is non-null. - if (ShutdownRequested()) break; + if (m_chainman.m_interrupt) break; } while (pindexNewTip != pindexMostWork); CheckBlockIndex(); @@ -3277,7 +3278,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // Disconnect (descendants of) pindex, and mark them invalid. while (true) { - if (ShutdownRequested()) break; + if (m_chainman.m_interrupt) break; // Make sure the queue of validation callbacks doesn't grow unboundedly. LimitValidationInterfaceQueue(); @@ -4079,7 +4080,7 @@ void Chainstate::LoadMempool(const fs::path& load_path, FopenFn mockable_fopen_f { if (!m_mempool) return; ::LoadMempool(*m_mempool, load_path, *this, mockable_fopen_function); - m_mempool->SetLoadTried(!ShutdownRequested()); + m_mempool->SetLoadTried(!m_chainman.m_interrupt); } bool Chainstate::LoadChainTip() @@ -4212,7 +4213,7 @@ VerifyDBResult CVerifyDB::VerifyDB( skipped_l3_checks = true; } } - if (ShutdownRequested()) return VerifyDBResult::INTERRUPTED; + if (chainstate.m_chainman.m_interrupt) 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); @@ -4245,7 +4246,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::INTERRUPTED; + if (chainstate.m_chainman.m_interrupt) return VerifyDBResult::INTERRUPTED; } } @@ -4413,7 +4414,7 @@ bool ChainstateManager::LoadBlockIndex() } for (CBlockIndex* pindex : vSortedByHeight) { - if (ShutdownRequested()) return false; + if (m_interrupt) return false; if (pindex->IsAssumedValid() || (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->HaveTxsDownloaded() || pindex->pprev == nullptr))) { @@ -4519,7 +4520,7 @@ void Chainstate::LoadExternalBlockFile( // such as a block fails to deserialize. uint64_t nRewind = blkdat.GetPos(); while (!blkdat.eof()) { - if (ShutdownRequested()) return; + if (m_chainman.m_interrupt) return; blkdat.SetPos(nRewind); nRewind++; // start one byte further next time, in case of failure @@ -5152,13 +5153,13 @@ struct StopHashingException : public std::exception { const char* what() const throw() override { - return "ComputeUTXOStats interrupted by shutdown."; + return "ComputeUTXOStats interrupted."; } }; -static void SnapshotUTXOHashBreakpoint() +static void SnapshotUTXOHashBreakpoint(const util::SignalInterrupt& interrupt) { - if (ShutdownRequested()) throw StopHashingException(); + if (interrupt) throw StopHashingException(); } bool ChainstateManager::PopulateAndValidateSnapshot( @@ -5235,7 +5236,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot( // If our average Coin size is roughly 41 bytes, checking every 120,000 coins // means <5MB of memory imprecision. if (coins_processed % 120000 == 0) { - if (ShutdownRequested()) { + if (m_interrupt) { return false; } @@ -5292,7 +5293,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot( try { maybe_stats = ComputeUTXOStats( - CoinStatsHashType::HASH_SERIALIZED, snapshot_coinsdb, m_blockman, SnapshotUTXOHashBreakpoint); + CoinStatsHashType::HASH_SERIALIZED, snapshot_coinsdb, m_blockman, [&interrupt = m_interrupt] { SnapshotUTXOHashBreakpoint(interrupt); }); } catch (StopHashingException const&) { return false; } @@ -5470,7 +5471,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( CoinStatsHashType::HASH_SERIALIZED, &ibd_coins_db, m_blockman, - SnapshotUTXOHashBreakpoint); + [&interrupt = m_interrupt] { SnapshotUTXOHashBreakpoint(interrupt); }); } catch (StopHashingException const&) { return SnapshotCompletionResult::STATS_FAILED; } @@ -5579,9 +5580,10 @@ static ChainstateManager::Options&& Flatten(ChainstateManager::Options&& opts) return std::move(opts); } -ChainstateManager::ChainstateManager(Options options, node::BlockManager::Options blockman_options) - : m_options{Flatten(std::move(options))}, - m_blockman{std::move(blockman_options)} {} +ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Options options, node::BlockManager::Options blockman_options) + : m_interrupt{interrupt}, + m_options{Flatten(std::move(options))}, + m_blockman{interrupt, std::move(blockman_options)} {} ChainstateManager::~ChainstateManager() { diff --git a/src/validation.h b/src/validation.h index 8bc8842c54..5412cb1d48 100644 --- a/src/validation.h +++ b/src/validation.h @@ -62,6 +62,9 @@ class SnapshotMetadata; namespace Consensus { struct Params; } // namespace Consensus +namespace util { +class SignalInterrupt; +} // namespace util /** Maximum number of dedicated script-checking threads allowed */ static const int MAX_SCRIPTCHECK_THREADS = 15; @@ -959,7 +962,7 @@ private: public: using Options = kernel::ChainstateManagerOpts; - explicit ChainstateManager(Options options, node::BlockManager::Options blockman_options); + explicit ChainstateManager(const util::SignalInterrupt& interrupt, Options options, node::BlockManager::Options blockman_options); const CChainParams& GetParams() const { return m_options.chainparams; } const Consensus::Params& GetConsensus() const { return m_options.chainparams.GetConsensus(); } @@ -982,6 +985,7 @@ public: */ RecursiveMutex& GetMutex() const LOCK_RETURNED(::cs_main) { return ::cs_main; } + const util::SignalInterrupt& m_interrupt; const Options m_options; std::thread m_load_block; //! A single BlockManager instance is shared across each constructed From 3fa9094b92c5d37f486b0f8265062d3456796a50 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Sun, 14 May 2023 19:21:50 +0200 Subject: [PATCH 3/5] scripted-diff: Rename FatalError to FatalErrorf This is done in preparation for the next commit where a new FatalError function is introduced. FatalErrorf follows common convention to append 'f' for functions accepting format arguments. -BEGIN VERIFY SCRIPT- sed -i 's/FatalError/FatalErrorf/g' $( git grep -l 'FatalError') -END VERIFY SCRIPT- --- src/index/base.cpp | 16 ++++++++-------- test/lint/lint-format-strings.py | 2 +- test/lint/run-lint-format-strings.py | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index ec23cc1247..99c33d53ba 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -30,7 +30,7 @@ constexpr auto SYNC_LOG_INTERVAL{30s}; constexpr auto SYNC_LOCATOR_WRITE_INTERVAL{30s}; template -static void FatalError(const char* fmt, const Args&... args) +static void FatalErrorf(const char* fmt, const Args&... args) { AbortNode(tfm::format(fmt, args...)); } @@ -197,7 +197,7 @@ void BaseIndex::ThreadSync() break; } if (pindex_next->pprev != pindex && !Rewind(pindex, pindex_next->pprev)) { - FatalError("%s: Failed to rewind index %s to a previous chain tip", + FatalErrorf("%s: Failed to rewind index %s to a previous chain tip", __func__, GetName()); return; } @@ -221,14 +221,14 @@ void BaseIndex::ThreadSync() CBlock block; interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex); if (!m_chainstate->m_blockman.ReadBlockFromDisk(block, *pindex)) { - FatalError("%s: Failed to read block %s from disk", + FatalErrorf("%s: Failed to read block %s from disk", __func__, pindex->GetBlockHash().ToString()); return; } else { block_info.data = █ } if (!CustomAppend(block_info)) { - FatalError("%s: Failed to write block %s to index database", + FatalErrorf("%s: Failed to write block %s to index database", __func__, pindex->GetBlockHash().ToString()); return; } @@ -294,7 +294,7 @@ void BaseIndex::BlockConnected(const std::shared_ptr& block, const const CBlockIndex* best_block_index = m_best_block_index.load(); if (!best_block_index) { if (pindex->nHeight != 0) { - FatalError("%s: First block connected is not the genesis block (height=%d)", + FatalErrorf("%s: First block connected is not the genesis block (height=%d)", __func__, pindex->nHeight); return; } @@ -312,7 +312,7 @@ void BaseIndex::BlockConnected(const std::shared_ptr& block, const return; } if (best_block_index != pindex->pprev && !Rewind(best_block_index, pindex->pprev)) { - FatalError("%s: Failed to rewind index %s to a previous chain tip", + FatalErrorf("%s: Failed to rewind index %s to a previous chain tip", __func__, GetName()); return; } @@ -325,7 +325,7 @@ void BaseIndex::BlockConnected(const std::shared_ptr& block, const // processed, and the index object being safe to delete. SetBestBlockIndex(pindex); } else { - FatalError("%s: Failed to write block %s to index", + FatalErrorf("%s: Failed to write block %s to index", __func__, pindex->GetBlockHash().ToString()); return; } @@ -345,7 +345,7 @@ void BaseIndex::ChainStateFlushed(const CBlockLocator& locator) } if (!locator_tip_index) { - FatalError("%s: First block (hash=%s) in locator was not found", + FatalErrorf("%s: First block (hash=%s) in locator was not found", __func__, locator_tip_hash.ToString()); return; } diff --git a/test/lint/lint-format-strings.py b/test/lint/lint-format-strings.py index f54126e023..43addab2f3 100755 --- a/test/lint/lint-format-strings.py +++ b/test/lint/lint-format-strings.py @@ -16,7 +16,7 @@ import re import sys FUNCTION_NAMES_AND_NUMBER_OF_LEADING_ARGUMENTS = [ - 'FatalError,0', + 'FatalErrorf,0', 'fprintf,1', 'tfm::format,1', # Assuming tfm::::format(std::ostream&, ... 'LogConnectFailure,1', diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py index d1896dba84..2da72f0702 100755 --- a/test/lint/run-lint-format-strings.py +++ b/test/lint/run-lint-format-strings.py @@ -14,7 +14,7 @@ import sys FALSE_POSITIVES = [ ("src/dbwrapper.cpp", "vsnprintf(p, limit - p, format, backup_ap)"), - ("src/index/base.cpp", "FatalError(const char* fmt, const Args&... args)"), + ("src/index/base.cpp", "FatalErrorf(const char* fmt, const Args&... args)"), ("src/netbase.cpp", "LogConnectFailure(bool manual_connection, const char* fmt, const Args&... args)"), ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"), ("src/test/translation_tests.cpp", "strprintf(format, arg)"), From 7320db96f8d2aeff0bc5dc67d8b7b37f5f808990 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Thu, 15 Jun 2023 23:09:37 +0200 Subject: [PATCH 4/5] kernel: Add flushError method to notifications This is done in addition with the following commit. Both have the goal of getting rid of direct calls to AbortNode from kernel code. This extra flushError method is added to notify specifically about errors that arrise when flushing (syncing) block data to disk. Unlike other instances, the current calls to AbortNode in the blockstorage flush functions do not report an error to their callers. This commit is part of the libbitcoinkernel project and further removes the shutdown's and, more generally, the kernel library's dependency on interface_ui with a kernel notification method. By removing interface_ui from the kernel library, its dependency on boost is reduced to just boost::multi_index. At the same time it also takes a step towards de-globalising the interrupt infrastructure. --- src/bitcoin-chainstate.cpp | 6 ++++++ src/init.cpp | 2 ++ src/kernel/blockmanager_opts.h | 2 ++ src/kernel/notifications_interface.h | 8 ++++++++ src/node/blockstorage.cpp | 4 ++-- src/node/kernel_notifications.cpp | 6 ++++++ src/node/kernel_notifications.h | 2 ++ src/test/blockmanager_tests.cpp | 6 +++++- src/test/util/setup_common.cpp | 1 + src/test/validation_chainstatemanager_tests.cpp | 1 + 10 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index da42372a6a..cb391e30de 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -97,6 +97,11 @@ int main(int argc, char* argv[]) { std::cout << "Warning: " << warning.original << std::endl; } + void flushError(const std::string& debug_message) override + { + std::cerr << "Error flushing block data to disk: " << debug_message << std::endl; + } + }; auto notifications = std::make_unique(); @@ -112,6 +117,7 @@ int main(int argc, char* argv[]) const node::BlockManager::Options blockman_opts{ .chainparams = chainman_opts.chainparams, .blocks_dir = abs_datadir / "blocks", + .notifications = chainman_opts.notifications, }; ChainstateManager chainman{kernel_context.interrupt, chainman_opts, blockman_opts}; diff --git a/src/init.cpp b/src/init.cpp index ac81ebab82..a1b210f201 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -999,6 +999,7 @@ bool AppInitParameterInteraction(const ArgsManager& args) BlockManager::Options blockman_opts_dummy{ .chainparams = chainman_opts_dummy.chainparams, .blocks_dir = args.GetBlocksDirPath(), + .notifications = chainman_opts_dummy.notifications, }; auto blockman_result{ApplyArgsManOptions(args, blockman_opts_dummy)}; if (!blockman_result) { @@ -1423,6 +1424,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) BlockManager::Options blockman_opts{ .chainparams = chainman_opts.chainparams, .blocks_dir = args.GetBlocksDirPath(), + .notifications = chainman_opts.notifications, }; Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction diff --git a/src/kernel/blockmanager_opts.h b/src/kernel/blockmanager_opts.h index 8f26422f72..0251bbb10a 100644 --- a/src/kernel/blockmanager_opts.h +++ b/src/kernel/blockmanager_opts.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H #define BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H +#include #include #include @@ -25,6 +26,7 @@ struct BlockManagerOpts { bool fast_prune{false}; bool stop_after_block_import{DEFAULT_STOPAFTERBLOCKIMPORT}; const fs::path blocks_dir; + Notifications& notifications; }; } // namespace kernel diff --git a/src/kernel/notifications_interface.h b/src/kernel/notifications_interface.h index 48248e9aa0..e009810473 100644 --- a/src/kernel/notifications_interface.h +++ b/src/kernel/notifications_interface.h @@ -27,6 +27,14 @@ public: virtual void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {} virtual void progress(const bilingual_str& title, int progress_percent, bool resume_possible) {} virtual void warning(const bilingual_str& warning) {} + + //! The flush error notification is sent to notify the user that an error + //! occurred while flushing block data to disk. Kernel code may ignore flush + //! errors that don't affect the immediate operation it is trying to + //! perform. Applications can choose to handle the flush error notification + //! by logging the error, or notifying the user, or triggering an early + //! shutdown as a precaution against causing more errors. + virtual void flushError(const std::string& debug_message) {} }; } // namespace kernel diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index a27a7a5466..729ac8850a 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -528,7 +528,7 @@ void BlockManager::FlushUndoFile(int block_file, bool finalize) { FlatFilePos undo_pos_old(block_file, m_blockfile_info[block_file].nUndoSize); if (!UndoFileSeq().Flush(undo_pos_old, finalize)) { - AbortNode("Flushing undo file to disk failed. This is likely the result of an I/O error."); + m_opts.notifications.flushError("Flushing undo file to disk failed. This is likely the result of an I/O error."); } } @@ -547,7 +547,7 @@ void BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo) FlatFilePos block_pos_old(m_last_blockfile, m_blockfile_info[m_last_blockfile].nSize); if (!BlockFileSeq().Flush(block_pos_old, fFinalize)) { - AbortNode("Flushing block file to disk failed. This is likely the result of an I/O error."); + m_opts.notifications.flushError("Flushing block file to disk failed. This is likely the result of an I/O error."); } // we do not always flush the undo file, as the chain tip may be lagging behind the incoming blocks, // e.g. during IBD or a sync after a node going offline diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp index 926b157f3b..7c7db26e9b 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -72,4 +73,9 @@ void KernelNotifications::warning(const bilingual_str& warning) DoWarning(warning); } +void KernelNotifications::flushError(const std::string& debug_message) +{ + AbortNode(debug_message); +} + } // namespace node diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index 3e665bbf14..fa80af1120 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -25,6 +25,8 @@ public: void progress(const bilingual_str& title, int progress_percent, bool resume_possible) override; void warning(const bilingual_str& warning) override; + + void flushError(const std::string& debug_message) override; }; } // namespace node diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index 631f85908e..3e8cbb218d 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -5,14 +5,16 @@ #include #include #include +#include #include #include #include #include -using node::BlockManager; using node::BLOCK_SERIALIZATION_HEADER_SIZE; +using node::BlockManager; +using node::KernelNotifications; using node::MAX_BLOCKFILE_SIZE; // use BasicTestingSetup here for the data directory configuration, setup, and cleanup @@ -21,9 +23,11 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) { const auto params {CreateChainParams(ArgsManager{}, ChainType::MAIN)}; + KernelNotifications notifications{}; const BlockManager::Options blockman_opts{ .chainparams = *params, .blocks_dir = m_args.GetBlocksDirPath(), + .notifications = notifications, }; BlockManager blockman{m_node.kernel->interrupt, blockman_opts}; CChain chain {}; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 514df73922..629b0cdcb8 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -196,6 +196,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto const BlockManager::Options blockman_opts{ .chainparams = chainman_opts.chainparams, .blocks_dir = m_args.GetBlocksDirPath(), + .notifications = chainman_opts.notifications, }; m_node.chainman = std::make_unique(m_node.kernel->interrupt, chainman_opts, blockman_opts); m_node.chainman->m_blockman.m_block_tree_db = std::make_unique(DBParams{ diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index f918ed6f95..00d33114dd 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -389,6 +389,7 @@ struct SnapshotTestSetup : TestChain100Setup { const BlockManager::Options blockman_opts{ .chainparams = chainman_opts.chainparams, .blocks_dir = m_args.GetBlocksDirPath(), + .notifications = chainman_opts.notifications, }; // For robustness, ensure the old manager is destroyed before creating a // new one. From 6eb33bd0c21b3e075fbab596351cacafdc947472 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Tue, 9 May 2023 11:15:46 +0200 Subject: [PATCH 5/5] kernel: Add fatalError method to notifications FatalError replaces what previously was the AbortNode function in shutdown.cpp. This commit is part of the libbitcoinkernel project and further removes the shutdown's and, more generally, the kernel library's dependency on interface_ui with a kernel notification method. By removing interface_ui from the kernel library, its dependency on boost is reduced to just boost::multi_index. At the same time it also takes a step towards de-globalising the interrupt infrastructure. Co-authored-by: Russell Yanofsky Co-authored-by: TheCharlatan --- src/Makefile.am | 3 +- src/bitcoin-chainstate.cpp | 6 ++- src/index/base.cpp | 6 ++- src/index/base.h | 3 ++ src/init.cpp | 6 +-- src/kernel/notifications_interface.h | 12 +++++- src/node/abort.cpp | 27 ++++++++++++++ src/node/abort.h | 17 +++++++++ src/node/blockstorage.cpp | 11 +++--- src/node/kernel_notifications.cpp | 11 +++++- src/node/kernel_notifications.h | 10 +++++ src/shutdown.cpp | 30 +-------------- src/shutdown.h | 12 ------ src/test/blockmanager_tests.cpp | 2 +- src/test/util/setup_common.cpp | 2 +- .../validation_chainstatemanager_tests.cpp | 15 +++----- src/validation.cpp | 37 +++++++++---------- src/validation.h | 7 +--- test/lint/run-lint-format-strings.py | 1 + 19 files changed, 128 insertions(+), 90 deletions(-) create mode 100644 src/node/abort.cpp create mode 100644 src/node/abort.h diff --git a/src/Makefile.am b/src/Makefile.am index fe0b621340..4e9c161c57 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -205,6 +205,7 @@ BITCOIN_CORE_H = \ netbase.h \ netgroup.h \ netmessagemaker.h \ + node/abort.h \ node/blockmanager_args.h \ node/blockstorage.h \ node/caches.h \ @@ -400,6 +401,7 @@ libbitcoin_node_a_SOURCES = \ net.cpp \ net_processing.cpp \ netgroup.cpp \ + node/abort.cpp \ node/blockmanager_args.cpp \ node/blockstorage.cpp \ node/caches.cpp \ @@ -935,7 +937,6 @@ libbitcoinkernel_la_SOURCES = \ logging.cpp \ node/blockstorage.cpp \ node/chainstate.cpp \ - node/interface_ui.cpp \ node/utxo_snapshot.cpp \ policy/feerate.cpp \ policy/fees.cpp \ diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index cb391e30de..c36233207e 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -101,7 +101,11 @@ int main(int argc, char* argv[]) { std::cerr << "Error flushing block data to disk: " << debug_message << std::endl; } - + void fatalError(const std::string& debug_message, const bilingual_str& user_message) override + { + std::cerr << "Error: " << debug_message << std::endl; + std::cerr << (user_message.empty() ? "A fatal internal error occurred." : user_message.original) << std::endl; + } }; auto notifications = std::make_unique(); diff --git a/src/index/base.cpp b/src/index/base.cpp index 99c33d53ba..cf07cae286 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -30,9 +31,10 @@ constexpr auto SYNC_LOG_INTERVAL{30s}; constexpr auto SYNC_LOCATOR_WRITE_INTERVAL{30s}; template -static void FatalErrorf(const char* fmt, const Args&... args) +void BaseIndex::FatalErrorf(const char* fmt, const Args&... args) { - AbortNode(tfm::format(fmt, args...)); + auto message = tfm::format(fmt, args...); + node::AbortNode(m_chain->context()->exit_status, message); } CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash) diff --git a/src/index/base.h b/src/index/base.h index 231f36b605..8affee90f8 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -94,6 +94,9 @@ private: virtual bool AllowPrune() const = 0; + template + void FatalErrorf(const char* fmt, const Args&... args); + protected: std::unique_ptr m_chain; Chainstate* m_chainstate{nullptr}; diff --git a/src/init.cpp b/src/init.cpp index a1b210f201..988976028d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -812,8 +812,6 @@ bool AppInitBasicSetup(const ArgsManager& args, std::atomic& exit_status) // Enable heap terminate-on-corruption HeapSetInformation(nullptr, HeapEnableTerminationOnCorruption, nullptr, 0); #endif - InitShutdownState(exit_status); - if (!SetupNetworking()) { return InitError(Untranslated("Initializing networking failed.")); } @@ -986,7 +984,7 @@ bool AppInitParameterInteraction(const ArgsManager& args) // Also report errors from parsing before daemonization { - KernelNotifications notifications{}; + kernel::Notifications notifications{}; ChainstateManager::Options chainman_opts_dummy{ .chainparams = chainparams, .datadir = args.GetDataDirNet(), @@ -1410,7 +1408,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // ********************************************************* Step 7: load block chain - node.notifications = std::make_unique(); + node.notifications = std::make_unique(node.exit_status); fReindex = args.GetBoolArg("-reindex", false); bool fReindexChainState = args.GetBoolArg("-reindex-chainstate", false); ChainstateManager::Options chainman_opts{ diff --git a/src/kernel/notifications_interface.h b/src/kernel/notifications_interface.h index e009810473..e596a144a8 100644 --- a/src/kernel/notifications_interface.h +++ b/src/kernel/notifications_interface.h @@ -5,12 +5,13 @@ #ifndef BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H #define BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H +#include + #include #include class CBlockIndex; enum class SynchronizationState; -struct bilingual_str; namespace kernel { @@ -35,6 +36,15 @@ public: //! by logging the error, or notifying the user, or triggering an early //! shutdown as a precaution against causing more errors. virtual void flushError(const std::string& debug_message) {} + + //! The fatal error notification is sent to notify the user when an error + //! occurs in kernel code that can't be recovered from. After this + //! notification is sent, whatever function triggered the error should also + //! return an error code or raise an exception. Applications can choose to + //! handle the fatal error notification by logging the error, or notifying + //! the user, or triggering an early shutdown as a precaution against + //! causing more errors. + virtual void fatalError(const std::string& debug_message, const bilingual_str& user_message = {}) {} }; } // namespace kernel diff --git a/src/node/abort.cpp b/src/node/abort.cpp new file mode 100644 index 0000000000..a554126b86 --- /dev/null +++ b/src/node/abort.cpp @@ -0,0 +1,27 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace node { + +void AbortNode(std::atomic& exit_status, const std::string& debug_message, const bilingual_str& user_message, bool shutdown) +{ + SetMiscWarning(Untranslated(debug_message)); + LogPrintf("*** %s\n", debug_message); + InitError(user_message.empty() ? _("A fatal internal error occurred, see debug.log for details") : user_message); + exit_status.store(EXIT_FAILURE); + if (shutdown) StartShutdown(); +} +} // namespace node diff --git a/src/node/abort.h b/src/node/abort.h new file mode 100644 index 0000000000..d6bb0c14d5 --- /dev/null +++ b/src/node/abort.h @@ -0,0 +1,17 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_NODE_ABORT_H +#define BITCOIN_NODE_ABORT_H + +#include + +#include +#include + +namespace node { +void AbortNode(std::atomic& exit_status, const std::string& debug_message, const bilingual_str& user_message = {}, bool shutdown = true); +} // namespace node + +#endif // BITCOIN_NODE_ABORT_H diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 729ac8850a..191b6dc2f5 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -659,7 +659,8 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne bool out_of_space; size_t bytes_allocated = BlockFileSeq().Allocate(pos, nAddSize, out_of_space); if (out_of_space) { - return AbortNode("Disk space is too low!", _("Disk space is too low!")); + m_opts.notifications.fatalError("Disk space is too low!", _("Disk space is too low!")); + return false; } if (bytes_allocated != 0 && IsPruneMode()) { m_check_for_pruning = true; @@ -683,7 +684,7 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP bool out_of_space; size_t bytes_allocated = UndoFileSeq().Allocate(pos, nAddSize, out_of_space); if (out_of_space) { - return AbortNode(state, "Disk space is too low!", _("Disk space is too low!")); + return FatalError(m_opts.notifications, state, "Disk space is too low!", _("Disk space is too low!")); } if (bytes_allocated != 0 && IsPruneMode()) { m_check_for_pruning = true; @@ -725,7 +726,7 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid return error("ConnectBlock(): FindUndoPos failed"); } if (!UndoWriteToDisk(blockundo, _pos, block.pprev->GetBlockHash(), GetParams().MessageStart())) { - return AbortNode(state, "Failed to write undo data"); + return FatalError(m_opts.notifications, state, "Failed to write undo data"); } // rev files are written in block height order, whereas blk files are written as blocks come in (often out of order) // we want to flush the rev (undo) file once we've written the last block, which is indicated by the last height @@ -843,7 +844,7 @@ FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CCha } if (!position_known) { if (!WriteBlockToDisk(block, blockPos, GetParams().MessageStart())) { - AbortNode("Failed to write block"); + m_opts.notifications.fatalError("Failed to write block"); return FlatFilePos(); } } @@ -927,7 +928,7 @@ void ThreadImport(ChainstateManager& chainman, std::vector vImportFile for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) { BlockValidationState state; if (!chainstate->ActivateBestChain(state, nullptr)) { - AbortNode(strprintf("Failed to connect best block (%s)", state.ToString())); + chainman.GetNotifications().fatalError(strprintf("Failed to connect best block (%s)", state.ToString())); return; } } diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp index 7c7db26e9b..0be7d51afb 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -10,8 +10,12 @@ #include #include +#include +#include +#include #include #include +#include #include #include #include @@ -75,7 +79,12 @@ void KernelNotifications::warning(const bilingual_str& warning) void KernelNotifications::flushError(const std::string& debug_message) { - AbortNode(debug_message); + AbortNode(m_exit_status, debug_message); +} + +void KernelNotifications::fatalError(const std::string& debug_message, const bilingual_str& user_message) +{ + node::AbortNode(m_exit_status, debug_message, user_message, m_shutdown_on_fatal_error); } } // namespace node diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index fa80af1120..d35b6ecca5 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -7,6 +7,7 @@ #include +#include #include #include @@ -18,6 +19,8 @@ namespace node { class KernelNotifications : public kernel::Notifications { public: + KernelNotifications(std::atomic& exit_status) : m_exit_status{exit_status} {} + void blockTip(SynchronizationState state, CBlockIndex& index) override; void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) override; @@ -27,6 +30,13 @@ public: void warning(const bilingual_str& warning) override; void flushError(const std::string& debug_message) override; + + void fatalError(const std::string& debug_message, const bilingual_str& user_message = {}) override; + + //! Useful for tests, can be set to false to avoid shutdown on fatal error. + bool m_shutdown_on_fatal_error{true}; +private: + std::atomic& m_exit_status; }; } // namespace node diff --git a/src/shutdown.cpp b/src/shutdown.cpp index 185cad5baf..fc18ccd207 100644 --- a/src/shutdown.cpp +++ b/src/shutdown.cpp @@ -5,39 +5,13 @@ #include -#if defined(HAVE_CONFIG_H) -#include -#endif - #include #include -#include #include #include -#include -#include -#include - -static std::atomic* g_exit_status{nullptr}; - -bool AbortNode(const std::string& strMessage, bilingual_str user_message) -{ - SetMiscWarning(Untranslated(strMessage)); - LogPrintf("*** %s\n", strMessage); - if (user_message.empty()) { - user_message = _("A fatal internal error occurred, see debug.log for details"); - } - InitError(user_message); - Assert(g_exit_status)->store(EXIT_FAILURE); - StartShutdown(); - return false; -} - -void InitShutdownState(std::atomic& exit_status) -{ - g_exit_status = &exit_status; -} +#include +#include void StartShutdown() { diff --git a/src/shutdown.h b/src/shutdown.h index 0e51575c5a..2d6ace8d93 100644 --- a/src/shutdown.h +++ b/src/shutdown.h @@ -6,18 +6,6 @@ #ifndef BITCOIN_SHUTDOWN_H #define BITCOIN_SHUTDOWN_H -#include // For bilingual_str - -#include - -/** Abort with a message */ -bool AbortNode(const std::string& strMessage, bilingual_str user_message = bilingual_str{}); - -/** Initialize shutdown state. This must be called before using either StartShutdown(), - * AbortShutdown() or WaitForShutdown(). Calling ShutdownRequested() is always safe. - */ -void InitShutdownState(std::atomic& exit_status); - /** Request shutdown of the application. */ void StartShutdown(); diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index 3e8cbb218d..2ab2fa55f0 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -23,7 +23,7 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos) { const auto params {CreateChainParams(ArgsManager{}, ChainType::MAIN)}; - KernelNotifications notifications{}; + KernelNotifications notifications{m_node.exit_status}; const BlockManager::Options blockman_opts{ .chainparams = *params, .blocks_dir = m_args.GetBlocksDirPath(), diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 629b0cdcb8..3e2f0ab88d 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -184,7 +184,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto m_cache_sizes = CalculateCacheSizes(m_args); - m_node.notifications = std::make_unique(); + m_node.notifications = std::make_unique(m_node.exit_status); const ChainstateManager::Options chainman_opts{ .chainparams = chainparams, diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 00d33114dd..8f46d54621 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -379,7 +379,7 @@ struct SnapshotTestSetup : TestChain100Setup { LOCK(::cs_main); chainman.ResetChainstates(); BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0); - m_node.notifications = std::make_unique(); + m_node.notifications = std::make_unique(m_node.exit_status); const ChainstateManager::Options chainman_opts{ .chainparams = ::Params(), .datadir = chainman.m_options.datadir, @@ -564,7 +564,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup auto db_cache_before_complete = active_cs.m_coinsdb_cache_size_bytes; SnapshotCompletionResult res; - auto mock_shutdown = [](bilingual_str msg) {}; + m_node.notifications->m_shutdown_on_fatal_error = false; fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir(chainman.m_options.datadir); BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); @@ -574,8 +574,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup const uint256 snapshot_tip_hash = WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip()->GetBlockHash()); - res = WITH_LOCK(::cs_main, - return chainman.MaybeCompleteSnapshotValidation(mock_shutdown)); + res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation()); BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SUCCESS); WITH_LOCK(::cs_main, BOOST_CHECK(chainman.IsSnapshotValidated())); @@ -591,8 +590,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup BOOST_CHECK_EQUAL(all_chainstates[0], &active_cs); // Trying completion again should return false. - res = WITH_LOCK(::cs_main, - return chainman.MaybeCompleteSnapshotValidation(mock_shutdown)); + res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation()); BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SKIPPED); // The invalid snapshot path should not have been used. @@ -645,7 +643,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna Chainstate& validation_chainstate = *std::get<0>(chainstates); ChainstateManager& chainman = *Assert(m_node.chainman); SnapshotCompletionResult res; - auto mock_shutdown = [](bilingual_str msg) {}; + m_node.notifications->m_shutdown_on_fatal_error = false; // Test tampering with the IBD UTXO set with an extra coin to ensure it causes // snapshot completion to fail. @@ -661,8 +659,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna fs::path snapshot_chainstate_dir = gArgs.GetDataDirNet() / "chainstate_snapshot"; BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); - res = WITH_LOCK(::cs_main, - return chainman.MaybeCompleteSnapshotValidation(mock_shutdown)); + res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation()); BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::HASH_MISMATCH); auto all_chainstates = chainman.GetAll(); diff --git a/src/validation.cpp b/src/validation.cpp index fc8d292d6a..be6afbd320 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1842,9 +1842,9 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, return true; } -bool AbortNode(BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage) +bool FatalError(Notifications& notifications, BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage) { - AbortNode(strMessage, userMessage); + notifications.fatalError(strMessage, userMessage); return state.Error(strMessage); } @@ -2079,7 +2079,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // We don't write down blocks to disk if they may have been // corrupted, so this should be impossible unless we're having hardware // problems. - return AbortNode(state, "Corrupt block found indicating potential hardware failure; shutting down"); + return FatalError(m_chainman.GetNotifications(), state, "Corrupt block found indicating potential hardware failure; shutting down"); } return error("%s: Consensus::CheckBlock: %s", __func__, state.ToString()); } @@ -2499,7 +2499,7 @@ bool Chainstate::FlushStateToDisk( if (fDoFullFlush || fPeriodicWrite) { // Ensure we can write block index if (!CheckDiskSpace(m_blockman.m_opts.blocks_dir)) { - return AbortNode(state, "Disk space is too low!", _("Disk space is too low!")); + return FatalError(m_chainman.GetNotifications(), state, "Disk space is too low!", _("Disk space is too low!")); } { LOG_TIME_MILLIS_WITH_CATEGORY("write block and undo data to disk", BCLog::BENCH); @@ -2513,7 +2513,7 @@ bool Chainstate::FlushStateToDisk( LOG_TIME_MILLIS_WITH_CATEGORY("write block index to disk", BCLog::BENCH); if (!m_blockman.WriteBlockIndexDB()) { - return AbortNode(state, "Failed to write to block index database"); + return FatalError(m_chainman.GetNotifications(), state, "Failed to write to block index database"); } } // Finally remove any pruned files @@ -2535,11 +2535,11 @@ bool Chainstate::FlushStateToDisk( // an overestimation, as most will delete an existing entry or // overwrite one. Still, use a conservative safety factor of 2. if (!CheckDiskSpace(m_chainman.m_options.datadir, 48 * 2 * 2 * CoinsTip().GetCacheSize())) { - return AbortNode(state, "Disk space is too low!", _("Disk space is too low!")); + return FatalError(m_chainman.GetNotifications(), state, "Disk space is too low!", _("Disk space is too low!")); } // Flush the chainstate (which may refer to block index entries). if (!CoinsTip().Flush()) - return AbortNode(state, "Failed to write to coin database"); + return FatalError(m_chainman.GetNotifications(), state, "Failed to write to coin database"); m_last_flush = nNow; full_flush_completed = true; TRACE5(utxocache, flush, @@ -2555,7 +2555,7 @@ bool Chainstate::FlushStateToDisk( GetMainSignals().ChainStateFlushed(m_chain.GetLocator()); } } catch (const std::runtime_error& e) { - return AbortNode(state, std::string("System error while flushing: ") + e.what()); + return FatalError(m_chainman.GetNotifications(), state, std::string("System error while flushing: ") + e.what()); } return true; } @@ -2791,7 +2791,7 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, if (!pblock) { std::shared_ptr pblockNew = std::make_shared(); if (!m_blockman.ReadBlockFromDisk(*pblockNew, *pindexNew)) { - return AbortNode(state, "Failed to read block"); + return FatalError(m_chainman.GetNotifications(), state, "Failed to read block"); } pthisBlock = pblockNew; } else { @@ -2975,7 +2975,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* // If we're unable to disconnect a block during normal operation, // then that is a failure of our local system -- we should abort // rather than stay on a less work chain. - AbortNode(state, "Failed to disconnect block; see debug.log for details"); + FatalError(m_chainman.GetNotifications(), state, "Failed to disconnect block; see debug.log for details"); return false; } fBlocksDisconnected = true; @@ -3970,7 +3970,7 @@ bool Chainstate::AcceptBlock(const std::shared_ptr& pblock, BlockV } ReceivedBlockTransactions(block, pindex, blockPos); } catch (const std::runtime_error& e) { - return AbortNode(state, std::string("System error: ") + e.what()); + return FatalError(m_chainman.GetNotifications(), state, std::string("System error: ") + e.what()); } FlushStateToDisk(state, FlushStateMode::NONE); @@ -4661,7 +4661,7 @@ void Chainstate::LoadExternalBlockFile( } } } catch (const std::runtime_error& e) { - AbortNode(std::string("System error: ") + e.what()); + m_chainman.GetNotifications().fatalError(std::string("System error: ") + e.what()); } LogPrintf("Loaded %i blocks from external file in %dms\n", nLoaded, Ticks(SteadyClock::now() - start)); } @@ -5113,7 +5113,7 @@ bool ChainstateManager::ActivateSnapshot( snapshot_chainstate.reset(); bool removed = DeleteCoinsDBFromDisk(*snapshot_datadir, /*is_snapshot=*/true); if (!removed) { - AbortNode(strprintf("Failed to remove snapshot chainstate dir (%s). " + GetNotifications().fatalError(strprintf("Failed to remove snapshot chainstate dir (%s). " "Manually remove it before restarting.\n", fs::PathToString(*snapshot_datadir))); } } @@ -5378,8 +5378,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot( // through IsUsable() checks, or // // (ii) giving each chainstate its own lock instead of using cs_main for everything. -SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( - std::function shutdown_fnc) +SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() { AssertLockHeld(cs_main); if (m_ibd_chainstate.get() == &this->ActiveChainstate() || @@ -5431,7 +5430,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( user_error = strprintf(Untranslated("%s\n%s"), user_error, util::ErrorString(rename_result)); } - shutdown_fnc(user_error); + GetNotifications().fatalError(user_error.original, user_error); }; if (index_new.GetBlockHash() != snapshot_blockhash) { @@ -5728,13 +5727,13 @@ bool ChainstateManager::ValidatedSnapshotCleanup() fs::path tmp_old{ibd_chainstate_path + "_todelete"}; - auto rename_failed_abort = []( + auto rename_failed_abort = [this]( fs::path p_old, fs::path p_new, const fs::filesystem_error& err) { LogPrintf("%s: error renaming file (%s): %s\n", __func__, fs::PathToString(p_old), err.what()); - AbortNode(strprintf( + GetNotifications().fatalError(strprintf( "Rename of '%s' -> '%s' failed. " "Cannot clean up the background chainstate leveldb directory.", fs::PathToString(p_old), fs::PathToString(p_new))); @@ -5759,7 +5758,7 @@ bool ChainstateManager::ValidatedSnapshotCleanup() } if (!DeleteCoinsDBFromDisk(tmp_old, /*is_snapshot=*/false)) { - // No need to AbortNode because once the unneeded bg chainstate data is + // No need to FatalError because once the unneeded bg chainstate data is // moved, it will not interfere with subsequent initialization. LogPrintf("Deletion of %s failed. Please remove it manually, as the " /* Continued */ "directory is now unnecessary.\n", diff --git a/src/validation.h b/src/validation.h index 5412cb1d48..66bc036e66 100644 --- a/src/validation.h +++ b/src/validation.h @@ -106,7 +106,7 @@ void StopScriptCheckWorkerThreads(); CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams); -bool AbortNode(BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage = bilingual_str{}); +bool FatalError(kernel::Notifications& notifications, BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage = {}); /** Guess verification progress (as a fraction between 0.0=genesis and 1.0=current tip). */ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex* pindex); @@ -1056,10 +1056,7 @@ public: //! If the coins match (expected), then mark the validation chainstate for //! deletion and continue using the snapshot chainstate as active. //! Otherwise, revert to using the ibd chainstate and shutdown. - SnapshotCompletionResult MaybeCompleteSnapshotValidation( - std::function shutdown_fnc = - [](bilingual_str msg) { AbortNode(msg.original, msg); }) - EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + SnapshotCompletionResult MaybeCompleteSnapshotValidation() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! The most-work chain. Chainstate& ActiveChainstate() const; diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py index 2da72f0702..ed98b1b2f8 100755 --- a/test/lint/run-lint-format-strings.py +++ b/test/lint/run-lint-format-strings.py @@ -15,6 +15,7 @@ import sys FALSE_POSITIVES = [ ("src/dbwrapper.cpp", "vsnprintf(p, limit - p, format, backup_ap)"), ("src/index/base.cpp", "FatalErrorf(const char* fmt, const Args&... args)"), + ("src/index/base.h", "FatalErrorf(const char* fmt, const Args&... args)"), ("src/netbase.cpp", "LogConnectFailure(bool manual_connection, const char* fmt, const Args&... args)"), ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"), ("src/test/translation_tests.cpp", "strprintf(format, arg)"),