index: avoid “failed to commit” errors on initialization #29671
pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:2024-03-pr26903-reopen changing 1 files +14 −14-
fjahr commented at 7:39 pm on March 17, 2024: contributorIn 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: memberIsn’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 anullptr
, 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 updatingpindex
if we are synced already (no nextpindex
), and we would want to keep doing that at the beginning of the loop even if we updatepindex
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 thepprev
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 ofcurrent_time
along with the logging. This waycurrent_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 approvedryanofsky commented at 2:11 pm on March 19, 2024: contributorCode 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
whenm_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, 2024fjahr renamed this:
index: Skip commit during init when nothing can be committed
index: avoid "failed to commit" errors on initialization
on Mar 19, 2024fjahr force-pushed on Mar 19, 2024index: 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>
fjahr force-pushed on Mar 19, 2024ryanofsky approvedryanofsky commented at 7:03 pm on March 19, 2024: contributorCode 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, 2024in 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, 2024furszy commented at 7:08 pm on March 19, 2024: memberACK f65b0f6401091e4a4ca4c9f4db1cf388f0336badTheCharlatan approvedTheCharlatan commented at 3:04 pm on March 20, 2024: contributorACK f65b0f6401091e4a4ca4c9f4db1cf388f0336badachow101 commented at 8:21 pm on March 20, 2024: memberACK f65b0f6401091e4a4ca4c9f4db1cf388f0336badachow101 merged this on Mar 20, 2024achow101 closed this on Mar 20, 2024
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 03:12 UTC
More mirrored repositories can be found on mirror.b10c.me