index: avoid "failed to commit" errors on initialization #29671

fjahr · 2024-03-17T19:39:00Z

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 · 2024-03-17T19:39:03Z

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 · 2024-03-18T15:18:19Z

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 · 2024-03-18T18:28:25Z

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 · 2024-03-18T21:23:29Z

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.

src/index/base.cpp

…loop This avoids having commit print a needless error message during init. Co-authored-by: furszy <mfurszy@protonmail.com>

ryanofsky

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

furszy · 2024-03-19T19:07:57Z

src/index/base.cpp

+
+            auto current_time{std::chrono::steady_clock::now()};
+            if (last_log_time + SYNC_LOG_INTERVAL < current_time) {
+                LogPrintf("Syncing %s with block chain from height %d\n",


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 · 2024-03-20T20:21:07Z

ACK f65b0f6

…lization f65b0f6 index: Move last_locator_write_time and logging to end of threadsync loop (Fabian Jahr) Pull request description: 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 bitcoin@7878f97 from bitcoin#25494 and was reported by pstratem in bitcoin#26903 with an alternate fix. ACKs for top commit: achow101: ACK f65b0f6 ryanofsky: Code review ACK f65b0f6. Just moved log "Syncing" log line since last commit to avoid having to call now() twice. furszy: ACK f65b0f6 TheCharlatan: ACK f65b0f6 Tree-SHA512: afa8f05786318d36346d167ff53ea0b3bc8abdb0ad04465d199dc3eb91e9f837369e24fcb7e24b5757b02d698ec504e61da6ac365eaf006c874fc07a424a7e20

fbc0bce Merge bitcoin#29665: build, depends: Fix `libmultiprocess` cross-compilation (fanquake) 59a2145 Merge bitcoin#29747: depends: fix mingw-w64 Qt DEBUG=1 build (fanquake) bdb2fa6 Merge bitcoin#29671: index: avoid "failed to commit" errors on initialization (Ava Chow) 65377ea Merge bitcoin#29192: Weaken serfloat tests (fanquake) 4122404 Merge bitcoin#28891: test: fix `AddNode` unit test failure on OpenBSD (fanquake) 812ef53 Merge bitcoin#25677: refactor: make active_chain_tip a reference (MacroFake) 34e12fa Merge bitcoin#24564: doc: Clarify that CheckSequenceLocksAtTip is a validation function (glozow) d529751 Merge bitcoin#24788: doc: Add gpg key import instructions for Windows (fanquake) 61bae78 Merge bitcoin#24784: refactor: deduplicate integer serialization in RollingBloom benchmark (MarcoFalke) Pull request description: ## Issue being fixed or feature implemented Trivial backports ## What was done? ## How Has This Been Tested? built locally ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK fbc0bce Tree-SHA512: 731f70b747712810d4f74fe64a90139b02ddb62e9ac260705fa2588595feb19bd6e5ffed521fd878bacaab0015683e582fed19ed1855c3e955f93cd223862a17

…loop Summary: This avoids having commit print a needless error message during init. Co-authored-by: furszy <mfurszy@protonmail.com> This is a backport of [[bitcoin/bitcoin#29671 | core#29671]] Depends on D17703 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, roqqit Reviewed By: #bitcoin_abc, roqqit Differential Revision: https://reviews.bitcoinabc.org/D17704

DrahtBot changed the title ~~index: Skip commit when nothing can be committed~~ index: Skip commit when nothing can be committed Mar 17, 2024

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

furszy reviewed Mar 17, 2024

View reviewed changes

DrahtBot mentioned this pull request Mar 18, 2024

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

fjahr changed the title ~~index: Skip commit when nothing can be committed~~ index: Skip commit during init when nothing can be committed Mar 18, 2024

fjahr force-pushed the 2024-03-pr26903-reopen branch from aefa9a8 to 94e36d9 Compare March 18, 2024 21:21

furszy reviewed Mar 19, 2024

View reviewed changes

ryanofsky approved these changes Mar 19, 2024

View reviewed changes

src/index/base.cpp Show resolved Hide resolved

DrahtBot requested a review from ryanofsky March 19, 2024 14:11

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

fjahr force-pushed the 2024-03-pr26903-reopen branch from 94e36d9 to 87fe47f Compare March 19, 2024 17:19

index: Move last_locator_write_time and logging to end of threadsync …

f65b0f6

…loop This avoids having commit print a needless error message during init. Co-authored-by: furszy <mfurszy@protonmail.com>

fjahr force-pushed the 2024-03-pr26903-reopen branch from 87fe47f to f65b0f6 Compare March 19, 2024 17:20

ryanofsky approved these changes Mar 19, 2024

View reviewed changes

DrahtBot requested a review from furszy March 19, 2024 19:03

furszy reviewed Mar 19, 2024

View reviewed changes

DrahtBot requested a review from furszy March 19, 2024 19:08

furszy reviewed Mar 19, 2024

View reviewed changes

DrahtBot mentioned this pull request Mar 20, 2024

scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] #29641

Draft

TheCharlatan approved these changes Mar 20, 2024

View reviewed changes

achow101 merged commit bf1b638 into bitcoin:master Mar 20, 2024

mzumsande mentioned this pull request Sep 6, 2024

Misleading index error message #30836

Closed

bitcoin locked and limited conversation to collaborators Mar 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

index: avoid "failed to commit" errors on initialization #29671

index: avoid "failed to commit" errors on initialization #29671

Uh oh!

fjahr commented Mar 17, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Mar 17, 2024 •

edited

Loading

Uh oh!

furszy left a comment

Uh oh!

fjahr commented Mar 18, 2024

Uh oh!

furszy commented Mar 18, 2024

Uh oh!

fjahr commented Mar 18, 2024

Uh oh!

furszy left a comment

Uh oh!

ryanofsky left a comment •

edited

Loading

Uh oh!

Uh oh!

ryanofsky left a comment

Uh oh!

furszy Mar 19, 2024

Uh oh!

furszy left a comment

Uh oh!

TheCharlatan left a comment

Uh oh!

achow101 commented Mar 20, 2024

Uh oh!

Uh oh!

index: avoid "failed to commit" errors on initialization #29671

index: avoid "failed to commit" errors on initialization #29671

Uh oh!

Conversation

fjahr commented Mar 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Mar 18, 2024

Uh oh!

furszy commented Mar 18, 2024

Uh oh!

fjahr commented Mar 18, 2024

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Mar 20, 2024

Uh oh!

Uh oh!

fjahr commented Mar 17, 2024 •

edited

Loading

DrahtBot commented Mar 17, 2024 •

edited

Loading

ryanofsky left a comment •

edited

Loading