index: avoid "failed to commit" errors on initialization

[fjahr]

In the index sync thread, when initializing an index for the first time, stop callng BaseIndex::Commit when m_best_block_index is null, to avoid a spurious "failed to commit" error from that function. This error started happening in commit 7878f97 from #25494 and was reported by pstratem in #26903 with an alternate fix.

[DrahtBot]

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.

Type	Reviewers
ACK	ryanofsky, furszy, TheCharlatan, achow101

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/LogDebug over LogPrintf/LogPrint by maflcko)

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.

[furszy]

Isn't more natural to move the last locator update to the end of the loop?

[fjahr]

Isn't more natural to move the last locator update to the end of the loop?

I know what you mean by "more natural" but it requires a bigger change because 1. pindex is initially a nullptr, so either we have to set it before the loop when we do this change or add even more checks in the loop. And 2. we are also checking at the same time as updating pindex if we are synced already (no next pindex), and we would want to keep doing that at the beginning of the loop even if we update pindex later.

Maybe there is a simpler way to do this refactoring but I am not seeing it right now :)

[furszy]

Maybe there is a simpler way to do this refactoring but I am not seeing it right now :)

Why this is not possible?

diff --git a/src/index/base.cpp b/src/index/base.cpp
--- a/src/index/base.cpp	(revision aefa9a8fca467ffb26466741f0b6b90289d92e20)
+++ b/src/index/base.cpp	(date 1710785794630)
@@ -184,13 +184,6 @@
                 last_log_time = current_time;
             }
 
-            if (pindex->pprev && last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) {
-                SetBestBlockIndex(pindex->pprev);
-                last_locator_write_time = current_time;
-                // No need to handle errors in Commit. See rationale above.
-                Commit();
-            }
-
             CBlock block;
             interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex);
             if (!m_chainstate->m_blockman.ReadBlockFromDisk(block, *pindex)) {
@@ -205,6 +198,13 @@
                            __func__, pindex->GetBlockHash().ToString());
                 return;
             }
+
+            if (last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) {
+                SetBestBlockIndex(pindex);
+                last_locator_write_time = current_time;
+                // No need to handle errors in Commit. See rationale above.
+                Commit();
+            }
         }
     }
 

[fjahr]

Why this is not possible?

That works. I misunderstood your comment before, I thought by locator you meant pindex and wanted me to move that to the end to avoid the pprev stuff. All good now.

[furszy]

self ACK 94e36d9

Could update the PR title and description.

[ryanofsky]

Code review ACK 94e36d9 but I found the PR title and description confusing because "Skip commit during init when nothing can be committed" makes it sounds like this is trying to skip committing when there have been no changes since the last commit, when actually it is trying to skip committing when m_best_block_index pointer is null and the index is completely empty. Also the title makes this sound like an efficiency improvement, when the actual goal of the PR is to fix is misleading error message. Would suggest title and description more like:

• index: avoid "failed to commit" errors on initialization
• In the index sync thread, when initializing an index for the first time, stop callng BaseIndex::Commit when m_best_block_index is null, to avoid a spurious "failed to commit" error from that function. This error started happening in commit 7878f97 from #25494 and was reported by pstratem in #26903 with an alternate fix.

[ryanofsky]

Code review ACK f65b0f6. Just moved log "Syncing" log line since last commit to avoid having to call now() twice.

[furszy]

I agree on the change with a small nuance: now the "syncing index from height " is not entirely accurate. When the log is triggered, the block presented here was already scanned.

[furszy]

ACK f65b0f6

[TheCharlatan]

ACK f65b0f6

[achow101]

ACK f65b0f6