refactor: cleanup index logging #32948

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.

Type	Reviewers
ACK	l0rinc, maflcko

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:

#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#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:

            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:

            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:

        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:

            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:

            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

l0rinc commented at 1:04 PM on July 12, 2025: contributor

Lightweight code review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5

maflcko commented at 3:16 PM on July 14, 2025: member

review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5 🚣

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5 🚣
HVfKE6iYNJQgqhL99MmsFQIj2G47AGelwdrYUcE5dd/bXGM5iW1Ozo6jXoO90pDfqNSY2VTu1mLyTa+UdkZqAg==

</details>

fanquake merged this on Jul 14, 2025

fanquake closed this on Jul 14, 2025