Many of the normal, and expected init logs, which are run once after startup use the deprecated alias of LogInfo.
Fix that by using LogInfo directly, which is a refactor, except for a few log lines that also have __func__ removed.
(Also remove the unused trailing \n char while touching those logs)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32967.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, fanquake |
| Stale ACK | l0rinc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Possible typos and grammar issues:
drahtbot_id_4_m
152+ LogInfo("Validating signatures for all blocks.");
153 }
154- LogPrintf("Setting nMinimumChainWork=%s\n", chainman.MinimumChainWork().GetHex());
155+ LogInfo("Setting nMinimumChainWork=%s", chainman.MinimumChainWork().GetHex());
156 if (chainman.MinimumChainWork() < UintToArith256(chainman.GetConsensus().nMinimumChainWork)) {
157 LogPrintf("Warning: nMinimumChainWork set below default value of %s\n", chainman.GetConsensus().nMinimumChainWork.GetHex());
1289 for (auto* index : node.indexes) {
1290 index->Interrupt();
1291 index->Stop();
1292 if (!(index->Init() && index->StartBackgroundSync())) {
1293- LogPrintf("[snapshot] WARNING failed to restart index %s on snapshot chain\n", index->GetName());
1294+ LogInfo("[snapshot] WARNING failed to restart index %s on snapshot chain", index->GetName());
LogPrintf call here, removed the alias and the dedicated test, and do the info/warn/error differentiation in a separate PR.
I know that means we’d be touching the same lines multiple times, but in that case this PR could simply be a scripted diff with alias removal (+ line ending and __func__ and spacing fixes in separate commits) instead of having to check if we consider the text a warning or an info as well (since there are many of remaining LogPrintf calls that I would still consider info logs but weren’t migrated here for some reason.
instead of having to check if we consider the text a warning or an info as well (since there are many of remaining
LogPrintfcalls that I would still consider info logs but weren’t migrated here for some reason.
The whole point of having log levels is that they are used correctly. Otherwise, if we don’t care, we should just close this pull (and any related ones). If we do care, we should just review it (it isn’t that hard tbh).
As for the scope of this pull: It is only about init logs (those that happen during startup and shutdown). Missing some is harmless, because they can just be done later.
The whole point of having log levels is that they are used correctly
I wasn’t recommending any incorrect usage, was just wondering if it would simplify things if we migrated the warning/error logs before this PR, so that we can automate migration of all remaining ones and remove the LogInfo alias here. I’m also fine with it as is.
If we do care, we should just review it
I’m not against this change, I just don’t fully understand why some of the LogPrintf weren’t migrated, e.g.
Missing some is harmless, because they can just be done later
Got it, I’m fine with that as well.
I’m not against this change, I just don’t fully understand why some of the
LogPrintfweren’t migrated, e.g.
thx, done httpserver. The others you mention are not init logs (those that happen only once during startup and shutdown), because they are called in a loop for as long as the program runs.
95 return true;
96 }
97
98 if (gArgs.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY) && gArgs.SoftSetBoolArg("-walletbroadcast", false)) {
99- LogPrintf("%s: parameter interaction: -blocksonly=1 -> setting -walletbroadcast=0\n", __func__);
100+ LogInfo("Parameter interaction: -blocksonly=1 -> setting -walletbroadcast=0");
LogWarning could be appropriate here for both interactions?
I disagree, because:
LogInfo("parameter interaction: -blocksonly=1 -> setting -maxmempool=%d\n", DEFAULT_BLOCKSONLY_MAX_MEMPOOL_SIZE_MB); in init cpp)-blocksonly docs already explain the interaction, so if someone ignores the official docs, hiding a warning in the debug log likely isn’t going to affect them
All parameter interaction is (and was) info level
Fair enough, that’s at least a good reason to not do it here but in a separate pull.
A warning is for the node operator to investigate and possibly fix.
That’s exactly why I was thinking it should be warning: a node operator “misconfigured” their node. It may be fine, but the node may also be behaving differently than what they expect, so best is for the node operator to investigate and fix the interaction.
Anyway, can be marked as resolved here. Will probably open a pull later.
edit: I was wrong, see below
LogInfo is absolutely appropriate.
114@@ -115,9 +115,9 @@ bool StartLogging(const ArgsManager& args)
115 }
116
117 if (!LogInstance().m_log_timestamps)
ACK fa65716a878dbc0daabf1b31e832db5b230c285c
Mostly refactor operations, with a couple of minor logging statement updates (e.g. removing function names, adding detail on which operation is started/exited, …)
1231@@ -1232,17 +1232,17 @@ void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> import_
1232 if (file.IsNull()) {
1233 break; // This error is logged in OpenBlockFile
1234 }
1235- LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
1236+ LogInfo("Reindexing block file blk%05u.dat...", (unsigned int)nFile);
1237 chainman.LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent);
1238 if (chainman.m_interrupt) {
1239- LogPrintf("Interrupt requested. Exit %s\n", __func__);
1240+ LogInfo("Interrupt requested. Exit reindexing.");
LogPrintf inlining from the __func__ and rewording changes into focused commits?
Concept ACK, thanks for the cleanup.
I personally would prefer doing every single LogPrintf -> LogInfo inlining here in a scripted diff (so we can get rid of the alias), with follow-up commits fixing the line endings and formatting and stuff.
And I agree that the warn/error split should be done in a separate PR where we can focus on the context (we could probably do a scripted diff for most of the lines that contain warning/error to cover most of those as well)
87@@ -88,14 +88,14 @@ bool WalletInit::ParameterInteraction() const
88 {
89 if (gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) {
90 for (const std::string& wallet : gArgs.GetArgs("-wallet")) {
91- LogPrintf("%s: parameter interaction: -disablewallet -> ignoring -wallet=%s\n", __func__, wallet);
92+ LogInfo("Parameter interaction: -disablewallet -> ignoring -wallet=%s", wallet);
Slightly unrelated question: given that WalletLogPrintf calls LogInfo now
(which was changed in https://github.com/bitcoin/bitcoin/commit/facbcd4cef8890ae18976fb53b67ea56b3c04454#diff-bf36b7865eb69d4b0a3576d830004933c82a795d3fd73c8073c11fc2e637a5c4R259-R261), I was wondering if this is the right time to rename the method as well. I don’t mind if you think this isn’t the place.
114@@ -115,9 +115,9 @@ bool StartLogging(const ArgsManager& args)
115 }
116
117 if (!LogInstance().m_log_timestamps)
118- LogPrintf("Startup time: %s\n", FormatISO8601DateTime(GetTime()));
119- LogPrintf("Default data directory %s\n", fs::PathToString(GetDefaultDataDir()));
120- LogPrintf("Using data directory %s\n", fs::PathToString(gArgs.GetDataDirNet()));
121+ LogInfo("Startup time: %s", FormatISO8601DateTime(GetTime()));
155+ LogInfo("Setting nMinimumChainWork=%s", chainman.MinimumChainWork().GetHex());
156 if (chainman.MinimumChainWork() < UintToArith256(chainman.GetConsensus().nMinimumChainWork)) {
157 LogPrintf("Warning: nMinimumChainWork set below default value of %s\n", chainman.GetConsensus().nMinimumChainWork.GetHex());
158 }
159 if (chainman.m_blockman.GetPruneTarget() == BlockManager::PRUNE_TARGET_MANUAL) {
160- LogPrintf("Block pruning enabled. Use RPC call pruneblockchain(height) to manually prune block and undo files.\n");
This refactor does not change behavior, but we’re removing a redundant space here, which could theoretically invalidate previous log parsers. It seems fine to me to do that, but we might want to mention it in the commit message.
It is redundant with -logsourcelocations and the log messages are
clearer without it.
Also, remove a double-space.
Also, add braces around `if` touched in the next commit.
This tiny behavior change requires a test fixup.
This refactor does not change behavior.
Post-merge ACK
It seems the post-merge build was partially cancelled for some reason, but the follow-ups passed fully 👍