validation: Make translations of fatal errors consistent #29672

1. 
   TheCharlatan commented at 8:59 am on March 18, 2024: contributor

   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.

2. 
   DrahtBot commented at 8:59 am on March 18, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	stickies-v, hebasto, maflcko, achow101
   Concept ACK	BrandonOdiwuor

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
   • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
   • #28280 (Don’t empty dbcache on prune flushes: >30% faster IBD by andrewtoth)
   • #28233 (validation: don’t clear cache on periodic flush by andrewtoth)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. DrahtBot added the label Validation on Mar 18, 2024
4. TheCharlatan marked this as ready for review on Mar 18, 2024
5. 
   hebasto commented at 1:39 pm on March 18, 2024: member

   Concept ACK.
6. in src/node/abort.cpp:22 in 75ffa4fc45 outdated

   21 {
   22-    SetMiscWarning(Untranslated(debug_message));
   23-    LogPrintf("*** %s\n", debug_message);
   24-    InitError(user_message.empty() ? _("A fatal internal error occurred, see debug.log for details") : user_message);
   25+    SetMiscWarning(message);
   26+    LogPrintf("*** %s\n", message.original);
   

   ---

   ---

   

   stickies-v commented at 12:05 pm on March 19, 2024:

   0    LogError("%s\n", message.original);
   

   And potentially the same a few lines down, even though it’s currently not touched.

7. in src/node/abort.h:17 in 75ffa4fc45 outdated

   14@@ -15,7 +15,7 @@ class SignalInterrupt;
   15 } // namespace util
   16 
   17 namespace node {
   18-void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const std::string& debug_message, const bilingual_str& user_message = {});
   19+void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message);
   

   ---

   ---

   

   stickies-v commented at 12:07 pm on March 19, 2024:

   nit: no more need for #include <string>
8. in src/validation.cpp:4302 in 75ffa4fc45 outdated

   4298@@ -4299,7 +4299,7 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& pblock,
   4299         }
   4300         ReceivedBlockTransactions(block, pindex, blockPos);
   4301     } catch (const std::runtime_error& e) {
   4302-        return FatalError(GetNotifications(), state, std::string("System error: ") + e.what());
   4303+        return FatalError(GetNotifications(), state, strprintf(_("System error: %s"), e.what()));
   

   ---

   ---

   

   stickies-v commented at 12:32 pm on March 19, 2024:

   nit: while touching, would be useful to consistently add context (+here)

   0        return FatalError(GetNotifications(), state, strprintf(_("System error while writing block to history file: %s"), e.what()));
   

9. 
   stickies-v commented at 12:37 pm on March 19, 2024: contributor

   Approach ACK 75ffa4fc456d898b308cf51385e20c2faf11aa42
10. TheCharlatan force-pushed on Mar 19, 2024
11. 
    TheCharlatan commented at 4:17 pm on March 19, 2024: contributor

    Thank you for the review @stickies-v,

    75ffa4fc456d898b308cf51385e20c2faf11aa42 -> 9a32540c0328268fb1cfc3105aaee596be4b6f6a (fatallogtranslate_0 -> fatallogtranslate_1, compare)

    • Addressed @stickies-v’s comment, replacing LogPrintf with LogError.
    • Addressed @stickies-v’s comment, removing leftover string include.
    • Addressed @stickies-v comment, added brief context description to “System errors”.
12. in src/node/abort.cpp:26 in 9a32540c03 outdated

    26+    LogError("*** %s\n", message.original);
    27+    InitError(_("A fatal internal error occurred, see debug.log for details: ") + message);
    28     exit_status.store(EXIT_FAILURE);
    29     if (shutdown && !(*shutdown)()) {
    30-        LogPrintf("Error: failed to send shutdown signal\n");
    31+        LogError("Error: failed to send shutdown signal\n");
    

    ---

    ---

    

    stickies-v commented at 5:15 pm on March 19, 2024:

    nit: by using LogError, we’re already adding an [error] prefix so I think “Error: " could be removed, as could the “***” a few lines up imo

    ---

    

    TheCharlatan commented at 7:53 pm on March 19, 2024:

    Good suggestion, applied.
13. stickies-v approved
14. 
    stickies-v commented at 6:08 pm on March 19, 2024: contributor

    ACK 9a32540c0328268fb1cfc3105aaee596be4b6f6a
15. DrahtBot requested review from hebasto on Mar 19, 2024
16. TheCharlatan force-pushed on Mar 19, 2024
17. 
    TheCharlatan commented at 7:53 pm on March 19, 2024: contributor

    Updated 9a32540c0328268fb1cfc3105aaee596be4b6f6a -> 918efe63a685f9da4a941dd3c757ce60639e52b6 (fatallogtranslate_1 -> fatallogtranslate_2, compare)

    • Addressed @stickies-v’s comment, making the abort error messages a bit more legible.
18. 
    stickies-v commented at 11:53 pm on March 19, 2024: contributor

    re-ACK 918efe63a685f9da4a941dd3c757ce60639e52b6
19. DrahtBot added the label Needs rebase on Mar 20, 2024
20. TheCharlatan force-pushed on Mar 20, 2024
21. 
    TheCharlatan commented at 6:43 pm on March 20, 2024: contributor

    Rebased 918efe63a685f9da4a941dd3c757ce60639e52b6 -> 98d0dd92ad384990d3b63618c2473ee83a23754d (fatallogtranslate_2 -> fatallogtranslate_3, compare)
22. DrahtBot removed the label Needs rebase on Mar 20, 2024
23. 
    stickies-v commented at 11:31 pm on March 20, 2024: contributor

    re-ACK 98d0dd92ad384990d3b63618c2473ee83a23754d - no differences but to resolve merge conflict with #27039
24. 
    BrandonOdiwuor commented at 7:47 am on March 21, 2024: contributor

    Concept ACK
25. in src/node/abort.cpp:22 in 98d0dd92ad outdated

    22-    SetMiscWarning(Untranslated(debug_message));
    23-    LogPrintf("*** %s\n", debug_message);
    24-    InitError(user_message.empty() ? _("A fatal internal error occurred, see debug.log for details") : user_message);
    25+    SetMiscWarning(message);
    26+    LogError("%s\n", message.original);
    27+    InitError(_("A fatal internal error occurred, see debug.log for details: ") + message);
    

    ---

    ---

    

    hebasto commented at 12:15 pm on March 21, 2024:

    While this code is touched, the LogError call seems redundant as the following InitError logs the message unconditionally:

    02024-03-21T12:08:54Z [error] Flushing block file to disk failed. This is likely the result of an I/O error.
    12024-03-21T12:08:54Z Error: A fatal internal error occurred, see debug.log for details: Flushing block file to disk failed. This is likely the result of an I/O error.
    

    ---

    

    TheCharlatan commented at 12:37 pm on March 21, 2024:

    It’s the uiInterface that logs it. I think if you run bitcoin-qt, there should be no redundant log line.

    ---

    

    hebasto commented at 1:03 pm on March 21, 2024:

    It’s the uiInterface that logs it. I think if you run bitcoin-qt, there should be no redundant log line.

    When running the GUI, the log line redundancy remains :)

    ---

    

    TheCharlatan commented at 1:37 pm on March 21, 2024:

    Oh, not sure why I got that impression then. This should be fixed.
26. hebasto approved
27. 
    hebasto commented at 12:15 pm on March 21, 2024: member

    ACK 98d0dd92ad384990d3b63618c2473ee83a23754d.
28. DrahtBot requested review from BrandonOdiwuor on Mar 21, 2024
29. 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.

    ddc7872c08
30. node: Use log levels in noui_ThreadSafeMessageBox 824f47294a
31. TheCharlatan force-pushed on Mar 21, 2024
32. 
    TheCharlatan commented at 3:49 pm on March 21, 2024: contributor

    Sorry for invalidating those ACKs, but I feel like this is easy enough to look at again.

    98d0dd92ad384990d3b63618c2473ee83a23754d -> 824f47294a309ba8e58ba8d1da0af15d8d828f43 (fatallogtranslate_3 -> fatallogtranslate_4, compare)

    • Addressed @hebasto’s comment, removing duplicate line.
    • Added another commit using the new log functions in noui_ThreadSafeMessageBox. Added this because it seemed weird to go back to the old style of logging the error after the previous change.
33. 
    stickies-v commented at 7:40 pm on March 21, 2024: contributor

    re-ACK 824f47294a309ba8e58ba8d1da0af15d8d828f43
34. DrahtBot requested review from hebasto on Mar 21, 2024
35. hebasto approved
36. 
    hebasto commented at 10:27 am on March 22, 2024: member

    re-ACK 824f47294a309ba8e58ba8d1da0af15d8d828f43.

    A side note: After 6 years, it seems worth reconsidering #14655 again.

37. 
    maflcko commented at 1:09 pm on March 22, 2024: member

    ACK 824f47294a309ba8e58ba8d1da0af15d8d828f43 🔎

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK 824f47294a309ba8e58ba8d1da0af15d8d828f43 🔎
    3/HtPXKA7yWdeOi1vZ7iHZlvCPniJNvpwshDC2/y6X3+cd7VXmisahmSugJEk3y43Euz4FSp5ro/x+dBjvx4HDw==
    

38. 
    achow101 commented at 6:43 pm on March 22, 2024: member

    ACK 824f47294a309ba8e58ba8d1da0af15d8d828f43
39. achow101 merged this on Mar 22, 2024
40. achow101 closed this on Mar 22, 2024

    ---

41. 
    ASISBusiness commented at 10:26 pm on March 23, 2024: none

    Working

Contributors
TheCharlatan DrahtBot hebasto stickies-v BrandonOdiwuor maflcko achow101 ASISBusiness

Review Requested
BrandonOdiwuor

Labels
Validation