index: avoid "failed to commit" errors on initialization

fjahr commented at 7:39 pm on March 17, 2024: contributor

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 https://github.com/bitcoin/bitcoin/commit/7878f97bf15b6e7c9b47d1c0d96419b97e1bdcbd from #25494 and was reported by pstratem in #26903 with an alternate fix.

DrahtBot commented at 7:39 pm on March 17, 2024: contributor

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.

DrahtBot renamed this:
~~index: Skip commit when nothing can be committed~~
index: Skip commit when nothing can be committed
on Mar 17, 2024

DrahtBot added the label UTXO Db and Indexes on Mar 17, 2024

furszy commented at 10:01 pm on March 17, 2024: member

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

fjahr commented at 3:18 pm on March 18, 2024: contributor

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 :)

fjahr renamed this:
~~index: Skip commit when nothing can be committed~~
index: Skip commit during init when nothing can be committed
on Mar 18, 2024

furszy commented at 6:28 pm on March 18, 2024: member

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

Why this is not possible?

 0diff --git a/src/index/base.cpp b/src/index/base.cpp
 1--- a/src/index/base.cpp	(revision aefa9a8fca467ffb26466741f0b6b90289d92e20)
 2+++ b/src/index/base.cpp	(date 1710785794630)
 3@@ -184,13 +184,6 @@
 4                 last_log_time = current_time;
 5             }
 6 
 7-            if (pindex->pprev && last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) {
 8-                SetBestBlockIndex(pindex->pprev);
 9-                last_locator_write_time = current_time;
10-                // No need to handle errors in Commit. See rationale above.
11-                Commit();
12-            }
13-
14             CBlock block;
15             interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex);
16             if (!m_chainstate->m_blockman.ReadBlockFromDisk(block, *pindex)) {
17@@ -205,6 +198,13 @@
18                            __func__, pindex->GetBlockHash().ToString());
19                 return;
20             }
21+
22+            if (last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) {
23+                SetBestBlockIndex(pindex);
24+                last_locator_write_time = current_time;
25+                // No need to handle errors in Commit. See rationale above.
26+                Commit();
27+            }
28         }
29     }
30

fjahr force-pushed on Mar 18, 2024

fjahr commented at 9:23 pm on March 18, 2024: contributor

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 commented at 0:07 am on March 19, 2024: member

self ACK 94e36d9

Could update the PR title and description.

in src/index/base.cpp:202 in 94e36d9e58 outdated

197@@ -205,6 +198,13 @@ void BaseIndex::ThreadSync()
198                            __func__, pindex->GetBlockHash().ToString());
199                 return;
200             }
201+
202+            if (last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) {

ryanofsky commented at 1:50 pm on March 19, 2024:

In commit “index: Move last_locator_write_time to end of threadsync loop” (94e36d9e584dc20dd36a784f37cf30a5c32ec549)

Would be good to add current_time = std::chrono::steady_clock::now(); right before this, so timing is more accurate, because time will have elapsed since reading the block data and reindexing the block.

fjahr commented at 5:18 pm on March 19, 2024:

I would suggest to move down the initialization of current_time along with the logging. This way current_time is still accurate (as much as before) and we don’t have to get the clock time twice. I have added this now, let me know if that works for you as well.

ryanofsky approved

ryanofsky commented at 2:11 pm on March 19, 2024: contributor

Code review ACK 94e36d9e584dc20dd36a784f37cf30a5c32ec549 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 7878f97bf15b6e7c9b47d1c0d96419b97e1bdcbd from #25494 and was reported by pstratem in #26903 with an alternate fix.

DrahtBot requested review from ryanofsky on Mar 19, 2024

fjahr renamed this:
~~index: Skip commit during init when nothing can be committed~~
index: avoid "failed to commit" errors on initialization
on Mar 19, 2024

fjahr force-pushed on Mar 19, 2024

index: Move last_locator_write_time and logging to end of threadsync loop

This avoids having commit print a needless error message during init.

Co-authored-by: furszy <mfurszy@protonmail.com>

f65b0f6401

fjahr force-pushed on Mar 19, 2024

ryanofsky approved

ryanofsky commented at 7:03 pm on March 19, 2024: contributor

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

DrahtBot requested review from furszy on Mar 19, 2024

in src/index/base.cpp:197 in f65b0f6401

190@@ -205,6 +191,20 @@ void BaseIndex::ThreadSync()
191                            __func__, pindex->GetBlockHash().ToString());
192                 return;
193             }
194+
195+            auto current_time{std::chrono::steady_clock::now()};
196+            if (last_log_time + SYNC_LOG_INTERVAL < current_time) {
197+                LogPrintf("Syncing %s with block chain from height %d\n",

furszy commented at 7:07 pm on March 19, 2024:

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.

DrahtBot requested review from furszy on Mar 19, 2024

furszy commented at 7:08 pm on March 19, 2024: member

ACK f65b0f6401091e4a4ca4c9f4db1cf388f0336bad

TheCharlatan approved

TheCharlatan commented at 3:04 pm on March 20, 2024: contributor

ACK f65b0f6401091e4a4ca4c9f4db1cf388f0336bad

achow101 commented at 8:21 pm on March 20, 2024: member

ACK f65b0f6401091e4a4ca4c9f4db1cf388f0336bad

achow101 merged this on Mar 20, 2024

achow101 closed this on Mar 20, 2024

index: avoid “failed to commit” errors on initialization #29671

Code Coverage

Reviews

Conflicts