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

Reviews

See the guideline for information on the review process.

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:

• #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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.

@MarcoFalke this does also change the behavior slightly

In Rewind? I guess it might be good to list all behaviour changes in the commit or pull description to give reviewers a more accessible entry point. Also, for new code, we use {} in multi-line if.

@MarcoFalke anything that checks the return value will see a different result on the first call.

I'm not sure that matters but it is different.

@MarcoFalke each of the if statements is a single line single expression block

yes the only thing checking the result from Commit is Rewind, the behavior isn't changed here since Rewind won't be called before m_best_block_index has been set (it asserts that the best block index is the current tip)

Concept ACK

I agree 1c67b49 would fix the problem, but I think it suppresses errors too broadly. The Commit() function is called many places, so changing it to silently skip the CustomCommit() call and return true when it doesn't do anything could make it harder debug other problems in the future, or even know about them.

I think it would be better to apply a more targeted fix of just not calling Commit() when there is nothing to commit in the sync thread:

--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -189,9 +189,9 @@ void BaseIndex::ThreadSync()
                           GetName(), pindex->nHeight);
                 last_log_time = current_time;
             }
 
-            if (last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < 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();

Note on history of this bug: The m_best_block_index == nullptr check in the Commit() function was first added in 0243907 from #24117 to avoid writing the index locator and try to prevent data corruption. It was later extended in 7878f97 from #25494 to avoid writing custom index data as well as the index locator, and to print an error message. That change unfortunately caused the issue here by spuriously printing the error message on startup when a new index was enabled. @mzumsande actually pointed out the spurious error message at the time in a review comment #25494 (comment), but I didn't realize how commonly the error would happen, and we didn't do anything about it then.

Are you still working on this?

@mzumsande Maybe you'll want to pick this up?

I think this worth fixing and could pick it up (might take a week or two), but @ryanofsky since you suggested a specific fix above do you want to PR it?

Removed "Up for Grabs", as this looks like it was fixed in #29671.