From ddc7872c08b7ddf9b1e83abdb97c21303f4a9172 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 15 Mar 2024 21:42:44 +0100 Subject: [PATCH 1/2] node: Make translations of fatal errors consistent The extra `bilingual_str` argument of the fatal error notifications and `node::AbortNode()` is often unused and when used usually contains the same string as the message argument. It also seems to be confusing, since it is not consistently used for errors requiring user action. For example some assumeutxo fatal errors require the user to do something, but are not translated. So simplify the fatal error and abort node interfaces by only passing a translated string. This slightly changes the fatal errors displayed to the user. Also de-duplicate the abort error log since it is repeated in noui.cpp. --- src/bitcoin-chainstate.cpp | 9 +++---- src/index/base.cpp | 2 +- src/init.cpp | 2 +- src/kernel/notifications_interface.h | 8 +++--- src/node/abort.cpp | 9 +++---- src/node/abort.h | 7 +++-- src/node/blockstorage.cpp | 16 ++++++------ src/node/kernel_notifications.cpp | 8 +++--- src/node/kernel_notifications.h | 5 ++-- src/validation.cpp | 36 +++++++++++++------------- src/validation.h | 2 +- test/functional/feature_abortnode.py | 2 +- test/functional/feature_assumeutxo.py | 2 +- test/functional/feature_index_prune.py | 2 +- 14 files changed, 52 insertions(+), 58 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 3eb64aa344b..642af06e82d 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -89,14 +89,13 @@ int main(int argc, char* argv[]) { std::cout << "Warning: " << warning.original << std::endl; } - void flushError(const std::string& debug_message) override + void flushError(const bilingual_str& message) override { - std::cerr << "Error flushing block data to disk: " << debug_message << std::endl; + std::cerr << "Error flushing block data to disk: " << message.original << std::endl; } - void fatalError(const std::string& debug_message, const bilingual_str& user_message) override + void fatalError(const bilingual_str& message) override { - std::cerr << "Error: " << debug_message << std::endl; - std::cerr << (user_message.empty() ? "A fatal internal error occurred." : user_message.original) << std::endl; + std::cerr << "Error: " << message.original << std::endl; } }; auto notifications = std::make_unique(); diff --git a/src/index/base.cpp b/src/index/base.cpp index b4bda2fca6a..5f2c3786c20 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -31,7 +31,7 @@ template void BaseIndex::FatalErrorf(const char* fmt, const Args&... args) { auto message = tfm::format(fmt, args...); - node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, message); + node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, Untranslated(message)); } CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash) diff --git a/src/init.cpp b/src/init.cpp index 0aa04755cbe..349d4ff1abc 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1757,7 +1757,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // Start indexes initial sync if (!StartIndexBackgroundSync(node)) { bilingual_str err_str = _("Failed to start indexes, shutting down.."); - chainman.GetNotifications().fatalError(err_str.original, err_str); + chainman.GetNotifications().fatalError(err_str); return; } // Load mempool from disk diff --git a/src/kernel/notifications_interface.h b/src/kernel/notifications_interface.h index c5e77b0df90..7283a88e862 100644 --- a/src/kernel/notifications_interface.h +++ b/src/kernel/notifications_interface.h @@ -5,14 +5,12 @@ #ifndef BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H #define BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H -#include - #include -#include #include class CBlockIndex; enum class SynchronizationState; +struct bilingual_str; namespace kernel { @@ -48,7 +46,7 @@ public: //! 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) {} + virtual void flushError(const bilingual_str& 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 @@ -57,7 +55,7 @@ public: //! 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 = {}) {} + virtual void fatalError(const bilingual_str& message) {} }; } // namespace kernel diff --git a/src/node/abort.cpp b/src/node/abort.cpp index 1bdc91670d7..b7276083842 100644 --- a/src/node/abort.cpp +++ b/src/node/abort.cpp @@ -16,14 +16,13 @@ namespace node { -void AbortNode(util::SignalInterrupt* shutdown, std::atomic& exit_status, const std::string& debug_message, const bilingual_str& user_message) +void AbortNode(util::SignalInterrupt* shutdown, std::atomic& exit_status, const bilingual_str& message) { - 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); + SetMiscWarning(message); + InitError(_("A fatal internal error occurred, see debug.log for details: ") + message); exit_status.store(EXIT_FAILURE); if (shutdown && !(*shutdown)()) { - LogPrintf("Error: failed to send shutdown signal\n"); + LogError("Failed to send shutdown signal\n"); }; } } // namespace node diff --git a/src/node/abort.h b/src/node/abort.h index 28d021cc787..10922791421 100644 --- a/src/node/abort.h +++ b/src/node/abort.h @@ -5,17 +5,16 @@ #ifndef BITCOIN_NODE_ABORT_H #define BITCOIN_NODE_ABORT_H -#include - #include -#include + +struct bilingual_str; namespace util { class SignalInterrupt; } // namespace util namespace node { -void AbortNode(util::SignalInterrupt* shutdown, std::atomic& exit_status, const std::string& debug_message, const bilingual_str& user_message = {}); +void AbortNode(util::SignalInterrupt* shutdown, std::atomic& exit_status, const bilingual_str& message); } // namespace node #endif // BITCOIN_NODE_ABORT_H diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index f78f33e3713..576c07a833e 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -404,7 +404,7 @@ bool BlockManager::LoadBlockIndex(const std::optional& snapshot_blockha if (snapshot_blockhash) { const std::optional maybe_au_data = GetParams().AssumeutxoForBlockhash(*snapshot_blockhash); if (!maybe_au_data) { - m_opts.notifications.fatalError(strprintf("Assumeutxo data not found for the given blockhash '%s'.", snapshot_blockhash->ToString())); + m_opts.notifications.fatalError(strprintf(_("Assumeutxo data not found for the given blockhash '%s'."), snapshot_blockhash->ToString())); return false; } const AssumeutxoData& au_data = *Assert(maybe_au_data); @@ -741,7 +741,7 @@ bool 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)) { - m_opts.notifications.flushError("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.")); return false; } return true; @@ -763,7 +763,7 @@ bool BlockManager::FlushBlockFile(int blockfile_num, bool fFinalize, bool finali FlatFilePos block_pos_old(blockfile_num, m_blockfile_info[blockfile_num].nSize); if (!BlockFileSeq().Flush(block_pos_old, fFinalize)) { - m_opts.notifications.flushError("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.")); success = false; } // we do not always flush the undo file, as the chain tip may be lagging behind the incoming blocks, @@ -935,7 +935,7 @@ 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) { - m_opts.notifications.fatalError("Disk space is too low!", _("Disk space is too low!")); + m_opts.notifications.fatalError(_("Disk space is too low!")); return false; } if (bytes_allocated != 0 && IsPruneMode()) { @@ -960,7 +960,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 FatalError(m_opts.notifications, state, "Disk space is too low!", _("Disk space is too low!")); + return FatalError(m_opts.notifications, state, _("Disk space is too low!")); } if (bytes_allocated != 0 && IsPruneMode()) { m_check_for_pruning = true; @@ -1008,7 +1008,7 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid return false; } if (!UndoWriteToDisk(blockundo, _pos, block.pprev->GetBlockHash())) { - return FatalError(m_opts.notifications, 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 @@ -1149,7 +1149,7 @@ FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, cons } if (!position_known) { if (!WriteBlockToDisk(block, blockPos)) { - m_opts.notifications.fatalError("Failed to write block"); + m_opts.notifications.fatalError(_("Failed to write block.")); return FlatFilePos(); } } @@ -1233,7 +1233,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector vImportFile for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) { BlockValidationState state; if (!chainstate->ActivateBestChain(state, nullptr)) { - chainman.GetNotifications().fatalError(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 1fd3bad2968..99f909ff75c 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -84,15 +84,15 @@ void KernelNotifications::warning(const bilingual_str& warning) DoWarning(warning); } -void KernelNotifications::flushError(const std::string& debug_message) +void KernelNotifications::flushError(const bilingual_str& message) { - AbortNode(&m_shutdown, m_exit_status, debug_message); + AbortNode(&m_shutdown, m_exit_status, message); } -void KernelNotifications::fatalError(const std::string& debug_message, const bilingual_str& user_message) +void KernelNotifications::fatalError(const bilingual_str& message) { node::AbortNode(m_shutdown_on_fatal_error ? &m_shutdown : nullptr, - m_exit_status, debug_message, user_message); + m_exit_status, message); } void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications) diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index 38d8600ac6c..f4d97a0fff2 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -9,7 +9,6 @@ #include #include -#include class ArgsManager; class CBlockIndex; @@ -37,9 +36,9 @@ public: void warning(const bilingual_str& warning) override; - void flushError(const std::string& debug_message) override; + void flushError(const bilingual_str& message) override; - void fatalError(const std::string& debug_message, const bilingual_str& user_message = {}) override; + void fatalError(const bilingual_str& message) override; //! Block height after which blockTip notification will return Interrupted{}, if >0. int m_stop_at_height{DEFAULT_STOPATHEIGHT}; diff --git a/src/validation.cpp b/src/validation.cpp index b6d0c38f391..85d8ee2fb54 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2051,10 +2051,10 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, return true; } -bool FatalError(Notifications& notifications, BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage) +bool FatalError(Notifications& notifications, BlockValidationState& state, const bilingual_str& message) { - notifications.fatalError(strMessage, userMessage); - return state.Error(strMessage); + notifications.fatalError(message); + return state.Error(message.original); } /** @@ -2276,7 +2276,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 FatalError(m_chainman.GetNotifications(), state, "Corrupt block found indicating potential hardware failure; shutting down"); + return FatalError(m_chainman.GetNotifications(), state, _("Corrupt block found indicating potential hardware failure.")); } LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString()); return false; @@ -2702,7 +2702,7 @@ bool Chainstate::FlushStateToDisk( if (fDoFullFlush || fPeriodicWrite) { // Ensure we can write block index if (!CheckDiskSpace(m_blockman.m_opts.blocks_dir)) { - return FatalError(m_chainman.GetNotifications(), state, "Disk space is too low!", _("Disk space is too low!")); + return FatalError(m_chainman.GetNotifications(), state, _("Disk space is too low!")); } { LOG_TIME_MILLIS_WITH_CATEGORY("write block and undo data to disk", BCLog::BENCH); @@ -2720,7 +2720,7 @@ bool Chainstate::FlushStateToDisk( LOG_TIME_MILLIS_WITH_CATEGORY("write block index to disk", BCLog::BENCH); if (!m_blockman.WriteBlockIndexDB()) { - return FatalError(m_chainman.GetNotifications(), 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 @@ -2742,11 +2742,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 FatalError(m_chainman.GetNotifications(), state, "Disk space is too low!", _("Disk space is too low!")); + return FatalError(m_chainman.GetNotifications(), state, _("Disk space is too low!")); } // Flush the chainstate (which may refer to block index entries). if (!CoinsTip().Flush()) - return FatalError(m_chainman.GetNotifications(), 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, @@ -2762,7 +2762,7 @@ bool Chainstate::FlushStateToDisk( m_chainman.m_options.signals->ChainStateFlushed(this->GetRole(), m_chain.GetLocator()); } } catch (const std::runtime_error& e) { - return FatalError(m_chainman.GetNotifications(), state, std::string("System error while flushing: ") + e.what()); + return FatalError(m_chainman.GetNotifications(), state, strprintf(_("System error while flushing: %s"), e.what())); } return true; } @@ -2998,7 +2998,7 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, if (!pblock) { std::shared_ptr pblockNew = std::make_shared(); if (!m_blockman.ReadBlockFromDisk(*pblockNew, *pindexNew)) { - return FatalError(m_chainman.GetNotifications(), state, "Failed to read block"); + return FatalError(m_chainman.GetNotifications(), state, _("Failed to read block.")); } pthisBlock = pblockNew; } else { @@ -3185,7 +3185,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. - FatalError(m_chainman.GetNotifications(), state, "Failed to disconnect block; see debug.log for details"); + FatalError(m_chainman.GetNotifications(), state, _("Failed to disconnect block.")); return false; } fBlocksDisconnected = true; @@ -4345,7 +4345,7 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr& pblock, } ReceivedBlockTransactions(block, pindex, blockPos); } catch (const std::runtime_error& e) { - return FatalError(GetNotifications(), state, std::string("System error: ") + e.what()); + return FatalError(GetNotifications(), state, strprintf(_("System error while saving block to disk: %s"), e.what())); } // TODO: FlushStateToDisk() handles flushing of both block and chainstate @@ -5029,7 +5029,7 @@ void ChainstateManager::LoadExternalBlockFile( } } } catch (const std::runtime_error& e) { - GetNotifications().fatalError(std::string("System error: ") + e.what()); + GetNotifications().fatalError(strprintf(_("System error while loading external block file: %s"), e.what())); } LogPrintf("Loaded %i blocks from external file in %dms\n", nLoaded, Ticks(SteadyClock::now() - start)); } @@ -5539,8 +5539,8 @@ bool ChainstateManager::ActivateSnapshot( snapshot_chainstate.reset(); bool removed = DeleteCoinsDBFromDisk(*snapshot_datadir, /*is_snapshot=*/true); if (!removed) { - GetNotifications().fatalError(strprintf("Failed to remove snapshot chainstate dir (%s). " - "Manually remove it before restarting.\n", fs::PathToString(*snapshot_datadir))); + GetNotifications().fatalError(strprintf(_("Failed to remove snapshot chainstate dir (%s). " + "Manually remove it before restarting.\n"), fs::PathToString(*snapshot_datadir))); } } return false; @@ -5879,7 +5879,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() user_error = strprintf(Untranslated("%s\n%s"), user_error, util::ErrorString(rename_result)); } - GetNotifications().fatalError(user_error.original, user_error); + GetNotifications().fatalError(user_error); }; if (index_new.GetBlockHash() != snapshot_blockhash) { @@ -6220,9 +6220,9 @@ bool ChainstateManager::ValidatedSnapshotCleanup() const fs::filesystem_error& err) { LogPrintf("Error renaming path (%s) -> (%s): %s\n", fs::PathToString(p_old), fs::PathToString(p_new), err.what()); - GetNotifications().fatalError(strprintf( + GetNotifications().fatalError(strprintf(_( "Rename of '%s' -> '%s' failed. " - "Cannot clean up the background chainstate leveldb directory.", + "Cannot clean up the background chainstate leveldb directory."), fs::PathToString(p_old), fs::PathToString(p_new))); }; diff --git a/src/validation.h b/src/validation.h index bcf153719af..0f00a48b9c7 100644 --- a/src/validation.h +++ b/src/validation.h @@ -93,7 +93,7 @@ extern const std::vector CHECKLEVEL_DOC; CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams); -bool FatalError(kernel::Notifications& notifications, BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage = {}); +bool FatalError(kernel::Notifications& notifications, BlockValidationState& state, const bilingual_str& message); /** Guess verification progress (as a fraction between 0.0=genesis and 1.0=current tip). */ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex* pindex); diff --git a/test/functional/feature_abortnode.py b/test/functional/feature_abortnode.py index 740d3b7f0ec..01ba2834c42 100755 --- a/test/functional/feature_abortnode.py +++ b/test/functional/feature_abortnode.py @@ -36,7 +36,7 @@ class AbortNodeTest(BitcoinTestFramework): # Check that node0 aborted self.log.info("Waiting for crash") - self.nodes[0].wait_until_stopped(timeout=5, expect_error=True, expected_stderr="Error: A fatal internal error occurred, see debug.log for details") + self.nodes[0].wait_until_stopped(timeout=5, expect_error=True, expected_stderr="Error: A fatal internal error occurred, see debug.log for details: Failed to disconnect block.") self.log.info("Node crashed - now verifying restart fails") self.nodes[0].assert_start_raises_init_error() diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py index eb9ea65c4f2..3e882f47b83 100755 --- a/test/functional/feature_assumeutxo.py +++ b/test/functional/feature_assumeutxo.py @@ -134,7 +134,7 @@ class AssumeutxoTest(BitcoinTestFramework): with self.nodes[0].assert_debug_log([log_msg]): self.nodes[0].assert_start_raises_init_error(expected_msg=error_msg) - expected_error_msg = f"Error: A fatal internal error occurred, see debug.log for details" + expected_error_msg = f"Error: A fatal internal error occurred, see debug.log for details: Assumeutxo data not found for the given blockhash '7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a'." error_details = f"Assumeutxo data not found for the given blockhash" expected_error(log_msg=error_details, error_msg=expected_error_msg) diff --git a/test/functional/feature_index_prune.py b/test/functional/feature_index_prune.py index d6e802b399e..b3bf35b5243 100755 --- a/test/functional/feature_index_prune.py +++ b/test/functional/feature_index_prune.py @@ -128,7 +128,7 @@ class FeatureIndexPruneTest(BitcoinTestFramework): self.log.info("make sure we get an init error when starting the nodes again with the indices") filter_msg = "Error: basic block filter index best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)" stats_msg = "Error: coinstatsindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)" - end_msg = f"{os.linesep}Error: Failed to start indexes, shutting down.." + end_msg = f"{os.linesep}Error: A fatal internal error occurred, see debug.log for details: Failed to start indexes, shutting down.." for i, msg in enumerate([filter_msg, stats_msg, filter_msg]): self.nodes[i].assert_start_raises_init_error(extra_args=self.extra_args[i], expected_msg=msg+end_msg) From 824f47294a309ba8e58ba8d1da0af15d8d828f43 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Thu, 21 Mar 2024 16:09:02 +0100 Subject: [PATCH 2/2] node: Use log levels in noui_ThreadSafeMessageBox --- src/noui.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/noui.cpp b/src/noui.cpp index af5a180ce36..23637dfa1f7 100644 --- a/src/noui.cpp +++ b/src/noui.cpp @@ -28,20 +28,21 @@ bool noui_ThreadSafeMessageBox(const bilingual_str& message, const std::string& switch (style) { case CClientUIInterface::MSG_ERROR: strCaption = "Error: "; + if (!fSecure) LogError("%s\n", message.original); break; case CClientUIInterface::MSG_WARNING: strCaption = "Warning: "; + if (!fSecure) LogWarning("%s\n", message.original); break; case CClientUIInterface::MSG_INFORMATION: strCaption = "Information: "; + if (!fSecure) LogInfo("%s\n", message.original); break; default: strCaption = caption + ": "; // Use supplied caption (can be empty) + if (!fSecure) LogInfo("%s%s\n", strCaption, message.original); } - if (!fSecure) { - LogPrintf("%s%s\n", strCaption, message.original); - } tfm::format(std::cerr, "%s%s\n", strCaption, message.original); return false; }