Merge bitcoin/bitcoin#29672: validation: Make translations of fatal errors consistent

824f472 node: Use log levels in noui_ThreadSafeMessageBox (TheCharlatan)
ddc7872 node: Make translations of fatal errors consistent (TheCharlatan)

Pull request description:

  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.

ACKs for top commit:
  stickies-v:
    re-ACK 824f472
  maflcko:
    ACK 824f472 🔎
  achow101:
    ACK 824f472
  hebasto:
    re-ACK 824f472.

Tree-SHA512: 2868ee7b045fe7f3ac582ce5039141b398480b7627734976201dafaaef7544b8461635a7292fee4a7f32ff1bfc26f9bd4d0c292dca424ba42fb7fc4483d7ce8d

Author: achow101

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<KernelNotifications>();

src/index/base.cpp

@@ -31,7 +31,7 @@ template <typename... Args>
 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)

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

src/kernel/notifications_interface.h

@@ -5,14 +5,12 @@
 #ifndef BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H
 #define BITCOIN_KERNEL_NOTIFICATIONS_INTERFACE_H
 
-#include <util/translation.h>
-
 #include <cstdint>
-#include <string>
 #include <variant>
 
 class CBlockIndex;
 enum class SynchronizationState;
+struct bilingual_str;
 
 namespace kernel {
 
@@ -48,7 +46,7 @@ class Notifications
     //! 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 @@ class Notifications
     //! 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
 

src/node/abort.cpp

@@ -16,14 +16,13 @@
 
 namespace node {
 
-void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const std::string& debug_message, const bilingual_str& user_message)
+void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& 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

src/node/abort.h

@@ -5,17 +5,16 @@
 #ifndef BITCOIN_NODE_ABORT_H
 #define BITCOIN_NODE_ABORT_H
 
-#include <util/translation.h>
-
 #include <atomic>
-#include <string>
+
+struct bilingual_str;
 
 namespace util {
 class SignalInterrupt;
 } // namespace util
 
 namespace node {
-void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const std::string& debug_message, const bilingual_str& user_message = {});
+void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message);
 } // namespace node
 
 #endif // BITCOIN_NODE_ABORT_H

src/node/blockstorage.cpp

@@ -404,7 +404,7 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
     if (snapshot_blockhash) {
         const std::optional<AssumeutxoData> 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<fs::path> 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;
             }
         }

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)

src/node/kernel_notifications.h

@@ -9,7 +9,6 @@
 
 #include <atomic>
 #include <cstdint>
-#include <string>
 
 class ArgsManager;
 class CBlockIndex;
@@ -37,9 +36,9 @@ class KernelNotifications : public kernel::Notifications
 
     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};

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;
 }

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<CBlock> pblockNew = std::make_shared<CBlock>();
         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;
@@ -4347,7 +4347,7 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& 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
@@ -5031,7 +5031,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<std::chrono::milliseconds>(SteadyClock::now() - start));
 }
@@ -5541,8 +5541,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;
@@ -5881,7 +5881,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) {
@@ -6222,9 +6222,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)));
     };
 

src/validation.h

@@ -93,7 +93,7 @@ extern const std::vector<std::string> 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);