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.

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:

• #29975 (blockstorage: Separate reindexing from saving new blocks by mzumsande)
• #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
• #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
• #28830 ([refactor] Check CTxMemPool options in ctor by TheCharlatan)
• #28765 (p2p: Fill reconciliation sets (Erlay) by naumenkogs)
• #28676 ([WIP] Cluster mempool implementation by sdaftuar)
• #27826 (validation: log which peer sent us a header by Sjors)
• #26022 (Add util::ResultPtr class by ryanofsky)
• #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
• #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

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.

Depending on how many conflicts drahtbot detects, maybe this should be targeting a merge around 27.0's feature freeze?

Many of the log statements in init.cpp could be done at a better level, eg LogPrintf("Error: Out of memory. Terminating.\n"); should obviously be logging at the "error" level, not the "info" level. In order to keep this PR scripted-diff only, I won't fix that here; but it can fixed in an independent PR either before or after this PR is merged.

I don't think updating all the macros is a good idea until the API is finished.

Concept ACK - only 3 conflicts, and none look like priority projects.

happy to open a separate pull for it too?

Yeah, I think it is better to change a line of code only once, instead of twice, unless there is a reason to change it twice.

Suggested fixes in master...stickies-v:bitcoin:2024-01/fix-log-level-29231 - happy to open a separate pull for it too?

Added these commits

Rebased

Changes all uses of LogPrintLevel() where the level is hardcoded, and changes all LogPrintf and LogPrint uses in init.cpp.

Trying to understand the concept here. What problem does this solve?

re-ACK 8d46bd6750b47caaf2556f915800c6a22c997bc3 - no change since 1441ab74d6cd40e75f1dba80b68bc4b1791bf667 except for addressing merge conflicts

utACK 8d46bd6

Tangential to this PR:
Is LogPrintLevel in void libevent_log_cb(...) an example of:

"when [...] a category needs to be added for an info/warning/error log message"

 static void libevent_log_cb(int severity, const char *msg)
 {
-    BCLog::Level level;
     switch (severity) {
     case EVENT_LOG_DEBUG:
-        level = BCLog::Level::Debug;
+        LogDebug(BCLog::LIBEVENT, "%s\n", msg);
         break;
     case EVENT_LOG_MSG:
-        level = BCLog::Level::Info;
+        LogInfo("%s\n", msg);
         break;
     case EVENT_LOG_WARN:
-        level = BCLog::Level::Warning;
+        LogWarning("%s\n", msg);
         break;
     default: // EVENT_LOG_ERR and others are mapped to error
-        level = BCLog::Level::Error;
+        LogError("%s\n", msg);
         break;
     }
-    LogPrintLevel(BCLog::LIBEVENT, level, "%s\n", msg);
 }

Rebased over #28834, removed redundancy from LogWarning("Warning: ..")

@ajtowns Sorry, i didn't mean to derail this, i shouldn't have commented.