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 85c3eaf08d8..fe0b6213402 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 c38352ee386..e9421606efe 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 1205da869ee..3f4a423531c 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 f11b7b54f04..e74e0a6f08f 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 e918e841848..fb8029cb650 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 d70017d7344..185cad5baf6 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 c119bee96ff..0e51575c5a5 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 00000000000..c551ba8044c --- /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 00000000000..ca02feda91c --- /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 ccc053f576b..0b79b382763 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 432bdc8e33b..da42372a6a3 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 e9421606efe..ac81ebab820 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 71f3aac3664..d060e45af30 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 223b0e6a17c..a27a7a54663 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 a1ebb0df0a8..36b8aa48061 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 f094766886c..631f85908e1 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 93a60db8322..514df73922d 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 b797de46af6..f918ed6f958 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 b2095bd4a30..e5b5e0b8a12 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 04d0ecb39f2..6405437be93 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 6836498a640..fc8d292d6a1 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 8bc8842c54d..5412cb1d48a 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 ec23cc1247c..99c33d53ba4 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 f54126e0235..43addab2f3d 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 d1896dba840..2da72f07026 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 da42372a6a3..cb391e30de5 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 ac81ebab820..a1b210f2014 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 8f26422f72c..0251bbb10ab 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 48248e9aa0b..e0098104735 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 a27a7a54663..729ac8850a2 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 926b157f3b1..7c7db26e9b9 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 3e665bbf14e..fa80af11205 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 631f85908e1..3e8cbb218d4 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 514df73922d..629b0cdcb8d 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 f918ed6f958..00d33114dd3 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 fe0b6213402..4e9c161c57d 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 cb391e30de5..c36233207e8 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 99c33d53ba4..cf07cae2864 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 231f36b6053..8affee90f86 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 a1b210f2014..988976028dd 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 e0098104735..e596a144a88 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 00000000000..a554126b86b --- /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 00000000000..d6bb0c14d53 --- /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 729ac8850a2..191b6dc2f5f 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 7c7db26e9b9..0be7d51afbe 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 fa80af11205..d35b6ecca5c 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 185cad5baf6..fc18ccd2076 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 0e51575c5a5..2d6ace8d936 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 3e8cbb218d4..2ab2fa55f0a 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 629b0cdcb8d..3e2f0ab88d5 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 00d33114dd3..8f46d546217 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 fc8d292d6a1..be6afbd3208 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 5412cb1d48a..66bc036e668 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 2da72f07026..ed98b1b2f81 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)"),