scripted-diff: Unify error and warning log formatting

maflcko commented at 9:55 am on December 9, 2025: member

Errors and warnings should normally not happen. However, if they do happen, it is easier to spot them, if they are all logged in the same format via LogError or LogWarning.

So do that with a scripted-diff.

This is a minimal behavior change and unifies the log output from:

[net:error] Something bad happened
[net:warning] Something problematic happened

to either

[error] Something bad happened
[warning] Something problematic happened

or, when -loglevelalways=1 is enabled:

[all:error] Something bad happened
[all:warning] Something problematic happened

Such a behavior change is desired, because all warning and error logs are written in the same style in the source code and they are logged in the same format for log consumers.

Removing the category should be harmless, because warning and error messages should be descriptive and unique anyway.

scripted-diff: LogPrintLevel(*,BCLog::Level::Debug,*) -> LogDebug()

This refactor does not change behavior.

-BEGIN VERIFY SCRIPT-

 sed --regexp-extended --in-place \
   's/LogPrintLevel\((BCLog::[^,]*), BCLog::Level::Debug,/LogDebug(\1,/g' \
   $( git grep -l LogPrintLevel ':(exclude)src/test/logging_tests.cpp' )

-END VERIFY SCRIPT-

fa6c7a1954

scripted-diff: LogPrintLevel(*,BCLog::Level::*,*) -> LogError()/LogWarning()

This is a minimal behavior change and changes log output from:

  [net:error] Something bad happened
  [net:warning] Something problematic happened

to either

  [error] Something bad happened
  [warning] Something problematic happened

or, when -loglevelalways=1 is enabled:

  [all:error] Something bad happened
  [all:warning] Something problematic happened

Such a behavior change is desired, because all warning and error logs
are written in the same style in the source code and they are logged in
the same format for log consumers.

-BEGIN VERIFY SCRIPT-

 sed --regexp-extended --in-place \
   's/LogPrintLevel\((BCLog::[^,]*), BCLog::Level::(Error|Warning), */Log\2(/g' \
   $( git grep -l LogPrintLevel ':(exclude)src/test/logging_tests.cpp' )

-END VERIFY SCRIPT-

fa89f60e31

DrahtBot added the label Refactoring on Dec 9, 2025

DrahtBot commented at 9:56 am on December 9, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34033.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, rkrux, ajtowns
Concept ACK	glozow

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
#30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)

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.

in src/node/blockstorage.cpp:866 in fa89f60e31

862@@ -863,7 +863,7 @@ FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int n
863         // a reindex. A flush error might also leave some of the data files
864         // untrimmed.
865         if (!FlushBlockFile(last_blockfile, /*fFinalize=*/true, finalize_undo)) {
866-            LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning,
867+            LogWarning(

stickies-v commented at 12:36 pm on December 9, 2025:

nit: if you decide to add manual commits, a few statements could be tidied up a bit now:

 0diff --git a/src/netbase.cpp b/src/netbase.cpp
 1index 7c170eeff3..487a62c517 100644
 2--- a/src/netbase.cpp
 3+++ b/src/netbase.cpp
 4@@ -480,8 +480,8 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
 5         }
 6         if (pchRet2[1] != SOCKS5Reply::SUCCEEDED) {
 7             // Failures to connect to a peer that are not proxy errors
 8-            LogDebug(BCLog::NET,
 9-                          "Socks5() connect to %s:%d failed: %s\n", strDest, port, Socks5ErrorString(pchRet2[1]));
10+            LogDebug(BCLog::NET, "Socks5() connect to %s:%d failed: %s",
11+                strDest, port, Socks5ErrorString(pchRet2[1]));
12             return false;
13         }
14         if (pchRet2[2] != 0x00) { // Reserved field must be 0
15diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
16index a5984df617..4e22731b2c 100644
17--- a/src/node/blockstorage.cpp
18+++ b/src/node/blockstorage.cpp
19@@ -864,8 +864,9 @@ FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int n
20         // untrimmed.
21         if (!FlushBlockFile(last_blockfile, /*fFinalize=*/true, finalize_undo)) {
22             LogWarning(
23-                          "Failed to flush previous block file %05i (finalize=1, finalize_undo=%i) before opening new block file %05i\n",
24-                          last_blockfile, finalize_undo, nFile);
25+                "Failed to flush previous block file %05i (finalize=1, finalize_undo=%i) before "
26+                "opening new block file %05i",
27+                last_blockfile, finalize_undo, nFile);
28         }
29         // No undo data yet in the new file, so reset our undo-height tracking.
30         m_blockfile_cursors[chain_type] = BlockfileCursor{nFile};

maflcko commented at 1:53 pm on December 9, 2025:

I am thinking that the trailing, harmless, and unused newline can be removed when (and if) the switch to std::format and {} happens. So I’ll leave the lines untouched for now and leave this a scripted-diff only for now.

stickies-v approved

stickies-v commented at 12:48 pm on December 9, 2025: contributor

ACK fa89f60e31d18b6c17d372f1cec26a4440e3d8b3

There are 2 places where we still have diverging error and warning messages. Doesn’t have to be in this PR (would make it not entirely scripted-diff), but something like https://github.com/stickies-v/bitcoin/commits/2025-12/separate-log-print-level/ seems worth doing. As a bonus, it prevents issues such as in #34008 where debug logs can lead to unconditional logs being ratelimited.

maflcko commented at 1:50 pm on December 9, 2025: member

thx, for the suggestion. I want to focus only on error/warning logs here. I can pick up other logs in a follow-up, once and if there is one.

rkrux commented at 5:53 pm on December 9, 2025: contributor

lgtm code review ACK fa89f60e31d18b6c17d372f1cec26a4440e3d8b3

The uniformity at the code level is appealing.

glozow commented at 7:23 pm on December 9, 2025: member

concept ACK

ajtowns commented at 0:11 am on December 10, 2025: contributor

ACK fa89f60e31d18b6c17d372f1cec26a4440e3d8b3

This is doing Debug in the first commit (which has no behaviour change), and Warning and Error in the second, but missing a couple of cases of Info in i2p.cpp and mapport.cpp. I’m assuming that’s intentional.

DrahtBot requested review from glozow on Dec 10, 2025

scripted-diff: Unify error and warning log formatting #34033

Code Coverage & Benchmarks

Reviews

Conflicts