refactor: cleanup index logging

Sjors commented at 12:46 pm on July 11, 2025: member

This PR removes the use of __func__ from index logging, since we have -logsourcelocations.

It also improves readability by putting GetName() in a more logical place.

Before

coinstatsindex: best block of the index not found. Please rebuild the index.

After:

best block of coinstatsindex not found. Please rebuild the index.

I found myself maintaining this commit as part of https://github.com/Sjors/bitcoin/pull/86, but since that might never land here, it seemed better to split it into its own PR (or get rid of it).

DrahtBot added the label Refactoring on Jul 11, 2025

DrahtBot commented at 12:46 pm on July 11, 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/32948.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26966 (index: initial sync speedup, parallelize process by furszy)

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/index/base.cpp:350 in 8c76ee102b outdated

346@@ -347,15 +347,15 @@ void BaseIndex::BlockConnected(ChainstateRole role, const std::shared_ptr<const
347         // in the ValidationInterface queue backlog even after the sync thread has caught up to the
348         // new chain tip. In this unlikely event, log a warning and let the queue clear.
349         if (best_block_index->GetAncestor(pindex->nHeight - 1) != pindex->pprev) {
350-            LogPrintf("%s: WARNING: Block %s does not connect to an ancestor of "
351+            LogPrintf("WARNING: Block %s does not connect to an ancestor of "

fanquake commented at 12:52 pm on July 11, 2025:

If you’re touching these lines, they should be getting changed to LogInfo.

in src/index/coinstatsindex.cpp:337 in 8c76ee102b outdated

336     if (block) {
337         DBVal entry;
338         if (!LookUpOne(*m_db, *block, entry)) {
339-            LogError("%s: Cannot read current %s state; index may be corrupted\n",
340-                         __func__, GetName());
341+            LogError("Cannot read current %s state; index may be corrupted\n",

maflcko commented at 12:54 pm on July 11, 2025:

0            LogError("Cannot read current %s state; index may be corrupted",

nit: Can remove redundant newlines

Sjors force-pushed on Jul 11, 2025

Sjors commented at 12:59 pm on July 11, 2025: member

I missed a few __func__. Replaced LogPrintf with LogInfo and removed trailing \n’s

dergoegge commented at 1:03 pm on July 11, 2025: member

While we’re touching all of these, it might be worthwhile to change to LogWarn/LogError where appropriate.

Sjors commented at 1:05 pm on July 11, 2025: member

@dergoegge any suggestions? Despite the new guidance, it’s still not always obvious what log level to use.

in src/index/base.cpp:350 in bb53138095 outdated

346@@ -347,15 +347,15 @@ void BaseIndex::BlockConnected(ChainstateRole role, const std::shared_ptr<const
347         // in the ValidationInterface queue backlog even after the sync thread has caught up to the
348         // new chain tip. In this unlikely event, log a warning and let the queue clear.
349         if (best_block_index->GetAncestor(pindex->nHeight - 1) != pindex->pprev) {
350-            LogPrintf("%s: WARNING: Block %s does not connect to an ancestor of "
351-                      "known best chain (tip=%s); not updating index\n",
352-                      __func__, pindex->GetBlockHash().ToString(),
353+            LogInfo("WARNING: Block %s does not connect to an ancestor of "

dergoegge commented at 1:06 pm on July 11, 2025:

0            LogWarn("Block %s does not connect to an ancestor of "

in src/index/base.cpp:405 in bb53138095 outdated

401@@ -402,9 +402,9 @@ void BaseIndex::ChainStateFlushed(ChainstateRole role, const CBlockLocator& loca
402     // event, log a warning and let the queue clear.
403     const CBlockIndex* best_block_index = m_best_block_index.load();
404     if (best_block_index->GetAncestor(locator_tip_index->nHeight) != locator_tip_index) {
405-        LogPrintf("%s: WARNING: Locator contains block (hash=%s) not on known best "
406-                  "chain (tip=%s); not writing index locator\n",
407-                  __func__, locator_tip_hash.ToString(),
408+        LogInfo("WARNING: Locator contains block (hash=%s) not on known best "

dergoegge commented at 1:06 pm on July 11, 2025:

0        LogWarn("Locator contains block (hash=%s) not on known best "

in src/index/blockfilterindex.cpp:221 in bb53138095 outdated

219+            LogInfo("Failed to truncate filter file %d", pos.nFile);
220             return 0;
221         }
222         if (!last_file.Commit()) {
223-            LogPrintf("%s: Failed to commit filter file %d\n", __func__, pos.nFile);
224+            LogInfo("Failed to commit filter file %d", pos.nFile);

dergoegge commented at 1:07 pm on July 11, 2025:

These can all be LogError

in src/index/blockfilterindex.cpp:244 in bb53138095 outdated

241     }
242 
243     AutoFile fileout{m_filter_fileseq->Open(pos)};
244     if (fileout.IsNull()) {
245-        LogPrintf("%s: Failed to open filter file %d\n", __func__, pos.nFile);
246+        LogInfo("Failed to open filter file %d", pos.nFile);

dergoegge commented at 1:07 pm on July 11, 2025:

LogError

in src/index/coinstatsindex.cpp:129 in bb53138095 outdated

125@@ -126,12 +126,12 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
126 
127         uint256 expected_block_hash{*Assert(block.prev_hash)};
128         if (read_out.first != expected_block_hash) {
129-            LogPrintf("WARNING: previous block header belongs to unexpected block %s; expected %s\n",
130+            LogInfo("WARNING: previous block header belongs to unexpected block %s; expected %s",

dergoegge commented at 1:07 pm on July 11, 2025:

0            LogWarn("previous block header belongs to unexpected block %s; expected %s",

in src/index/coinstatsindex.cpp:400 in bb53138095 outdated

396@@ -397,12 +397,12 @@ bool CoinStatsIndex::ReverseBlock(const interfaces::BlockInfo& block)
397 
398         uint256 expected_block_hash{*block.prev_hash};
399         if (read_out.first != expected_block_hash) {
400-            LogPrintf("WARNING: previous block header belongs to unexpected block %s; expected %s\n",
401+            LogInfo("WARNING: previous block header belongs to unexpected block %s; expected %s",

dergoegge commented at 1:07 pm on July 11, 2025:

0            LogWarn("previous block header belongs to unexpected block %s; expected %s",

Sjors commented at 1:16 pm on July 11, 2025: member

Thanks for the suggestions @dergoegge.

I replaced:

LogInfo("WARNING: with LogWarning,
LogInfo("Failed with LogError

Sjors force-pushed on Jul 11, 2025

in src/index/blockfilterindex.cpp:238 in 65a52af48e outdated

234@@ -235,13 +235,13 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
235     bool out_of_space;
236     m_filter_fileseq->Allocate(pos, data_size, out_of_space);
237     if (out_of_space) {
238-        LogPrintf("%s: out of disk space\n", __func__);
239+        LogInfo("out of disk space");

dergoegge commented at 1:17 pm on July 11, 2025:

I think this is also worth LogError as it’ll require user action.

Sjors force-pushed on Jul 11, 2025

refactor: cleanup index logging

- don't log function name
- take into account that GetName() always ends with " index"
- replace deprecated LogPrintf with LogInfo
- remove trailing \n
- adjusted log level where needed

c18bf0bd9b

Sjors force-pushed on Jul 11, 2025

refactor: cleanup index logging #32948

Code Coverage & Benchmarks

Reviews

Conflicts