validation: Make translations of fatal errors consistent #29672

TheCharlatan · 2024-03-18T08:59:08Z

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.

DrahtBot · 2024-03-18T08:59:11Z

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.

hebasto · 2024-03-18T13:39:51Z

Concept ACK.

stickies-v

Approach ACK 75ffa4f

src/node/abort.cpp

src/node/abort.h

stickies-v · 2024-03-19T12:32:59Z

src/validation.cpp

@@ -4299,7 +4299,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: %s"), e.what()));


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

Suggested change

return FatalError(GetNotifications(), state, strprintf(_("System error: %s"), e.what()));

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

TheCharlatan · 2024-03-19T16:17:48Z

Thank you for the review @stickies-v,

75ffa4f -> 9a32540 (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".

stickies-v

ACK 9a32540

src/node/abort.cpp

TheCharlatan · 2024-03-19T19:53:39Z

Updated 9a32540 -> 918efe6 (fatallogtranslate_1 -> fatallogtranslate_2, compare)

Addressed @stickies-v's comment, making the abort error messages a bit more legible.

stickies-v · 2024-03-19T23:53:12Z

re-ACK 918efe6

TheCharlatan · 2024-03-20T18:43:27Z

Rebased 918efe6 -> 98d0dd9 (fatallogtranslate_2 -> fatallogtranslate_3, compare)

stickies-v · 2024-03-20T23:31:13Z

re-ACK 98d0dd9 - no differences but to resolve merge conflict with #27039

BrandonOdiwuor

Concept ACK

hebasto

ACK 98d0dd9.

src/node/abort.cpp

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.

TheCharlatan · 2024-03-21T15:49:07Z

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

98d0dd9 -> 824f472 (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.

stickies-v · 2024-03-21T19:40:28Z

re-ACK 824f472

hebasto

re-ACK 824f472.

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

maflcko · 2024-03-22T13:09:06Z

ACK 824f472 🔎

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 824f47294a309ba8e58ba8d1da0af15d8d828f43 🔎
/HtPXKA7yWdeOi1vZ7iHZlvCPniJNvpwshDC2/y6X3+cd7VXmisahmSugJEk3y43Euz4FSp5ro/x+dBjvx4HDw==

achow101 · 2024-03-22T18:43:01Z

ACK 824f472

ASISBusiness

Working

DrahtBot added the Validation label Mar 18, 2024

TheCharlatan marked this pull request as ready for review March 18, 2024 11:44

This was referenced Mar 18, 2024

kernel: Handle fatal errors through return values #29642

Draft

scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] #29641

Draft

This was referenced Mar 18, 2024

Don't empty dbcache on prune flushes: >30% faster IBD #28280

Merged

validation: don't clear cache on periodic flush: >2x block connection speed #28233

Merged

blockstorage: do not flush block to disk if it is already there #27039

Merged

stickies-v reviewed Mar 19, 2024

View reviewed changes

TheCharlatan force-pushed the fatallogtranslate branch from 75ffa4f to 9a32540 Compare March 19, 2024 16:17

stickies-v approved these changes Mar 19, 2024

View reviewed changes

src/node/abort.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from hebasto March 19, 2024 18:08

TheCharlatan force-pushed the fatallogtranslate branch from 9a32540 to 918efe6 Compare March 19, 2024 19:52

DrahtBot added the Needs rebase label Mar 20, 2024

TheCharlatan force-pushed the fatallogtranslate branch from 918efe6 to 98d0dd9 Compare March 20, 2024 18:43

DrahtBot removed the Needs rebase label Mar 20, 2024

BrandonOdiwuor reviewed Mar 21, 2024

View reviewed changes

hebasto approved these changes Mar 21, 2024

View reviewed changes

src/node/abort.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from BrandonOdiwuor March 21, 2024 12:15

TheCharlatan added 2 commits March 21, 2024 16:40

node: Use log levels in noui_ThreadSafeMessageBox

824f472

TheCharlatan force-pushed the fatallogtranslate branch from 98d0dd9 to 824f472 Compare March 21, 2024 15:48

DrahtBot requested a review from hebasto March 21, 2024 19:40

DrahtBot mentioned this pull request Mar 22, 2024

kernel, refactor: return error status on all fatal errors #29700

Draft

hebasto approved these changes Mar 22, 2024

View reviewed changes

achow101 merged commit c122318 into bitcoin:master Mar 22, 2024

ASISBusiness reviewed Mar 23, 2024

View reviewed changes

bitcoin locked and limited conversation to collaborators Mar 23, 2025

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

validation: Make translations of fatal errors consistent #29672

validation: Make translations of fatal errors consistent #29672

Uh oh!

Conversation

TheCharlatan commented Mar 18, 2024

Uh oh!

DrahtBot commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

hebasto commented Mar 18, 2024

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stickies-v Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

TheCharlatan commented Mar 19, 2024

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheCharlatan commented Mar 19, 2024

Uh oh!

stickies-v commented Mar 19, 2024

Uh oh!

TheCharlatan commented Mar 20, 2024

Uh oh!

stickies-v commented Mar 20, 2024

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheCharlatan commented Mar 21, 2024

Uh oh!

stickies-v commented Mar 21, 2024

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 22, 2024

Uh oh!

achow101 commented Mar 22, 2024

Uh oh!

ASISBusiness left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Mar 18, 2024 •

edited

Loading