Issue Details

Number

30348

Title

RFC: Misused LogError and LogWarning macros

Description

As a programmer, when I see `LogError` and `LogWarning` macros, I assume `LogError` should be used to log information about a failure that happened, and `LogWarning` should be used to log information about an unusual condition which happened that might indicate a real problem depending on other factors and intent. But [developer notes](https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/doc/developer-notes.md#logging) suggest very different meanings for these macros. They say `LogError` should be used for "severe problems that require the node (or a subsystem) to shut down entirely (e.g., insufficient storage space)" and that `LogWarning` should be used "for severe problems that the node admin should address, but are not severe enough to warrant shutting down the node (e.g., system time appears to be wrong, unknown soft fork appears to have activated)." I think the distinction the developer notes makes and the levels it defines are useful, but the current names make it likely that the macros will be used incorrectly. So while the macros are new, I think it would be good to give them clearer names and I would suggest: - `LogFatal` for severe problems that require the node or a subsystem to shut down entirely, and - `LogCritical` for severe problems that the node admin should address, but do not warrant shutting down Looking at the code I found 58 cases where `LogError` and `Level::Error` appeared to be use correctly, in conditions that would lead to full or partial shutdowns, and 71 cases where it appeared to be used incorrectly to report errors that would not cause shutdowns and were potentially transient. Current `LogWarning` and `Level::Warn` usages were better but more rare, with 8 correct uses and 4 incorrect ones. I think before more code is written and ported to use `LogWarning` and `LogError` macros incorrectly, we should update documentation and/or code so macro names accurately reflect the way they are intended to be used. If we don't care about log levels distinguishing between fatal and nonfatal errors, #30361 is the most direct fix for this issue and is just a documentation change. If we think using levels distinguish fatal errors from other errors is valuable, #30347 fixes this issue by clarifying documentation and introducing LogFatal and LogCritical levels suggested above.

URL

https://github.com/bitcoin/bitcoin/issue/30348

Closed by

#30361

Notes

Attention of

Kill Factor (1-10)

Save

Back to List