Fix BaseIndex::Commit false error

pstratem commented at 10:10 am on January 17, 2023: contributor

Previously an ERROR was logged every time an index was initialized. Now we only record an error on actual error.

Fix BaseIndex::Commit false error.

Previously an ERROR was logged every time an index was initialized.
Now we only record an error on actual error.

1c67b495cb

DrahtBot commented at 10:10 am on January 17, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	mzumsande

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.

fanquake added the label UTXO Db and Indexes on Jan 17, 2023

maflcko renamed this:
~~Fix BaseIndex::Commit false error.~~
log: Fix BaseIndex::Commit false error
on Jan 17, 2023

pstratem commented at 12:15 pm on January 17, 2023: contributor

@MarcoFalke this does also change the behavior slightly

maflcko renamed this:
~~log: Fix BaseIndex::Commit false error~~
Fix BaseIndex::Commit false error
on Jan 17, 2023

maflcko commented at 12:20 pm on January 17, 2023: member

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.

pstratem commented at 1:35 pm on January 17, 2023: contributor

@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.

pstratem commented at 5:30 am on January 23, 2023: contributor

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

pstratem commented at 5:35 am on January 23, 2023: contributor

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)

mzumsande commented at 4:40 pm on January 25, 2023: contributor

Concept ACK

fanquake requested review from ryanofsky on Feb 27, 2023

ryanofsky commented at 10:17 pm on March 1, 2023: contributor

I agree 1c67b495cb2de76723c404f42d0bee219cc765c3 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:

 0--- a/src/index/base.cpp
 1+++ b/src/index/base.cpp
 2@@ -189,9 +189,9 @@ void BaseIndex::ThreadSync()
 3                           GetName(), pindex->nHeight);
 4                 last_log_time = current_time;
 5             }
 6 
 7-            if (last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) {
 8+            if (pindex->pprev && last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) {
 9                 SetBestBlockIndex(pindex->pprev);
10                 last_locator_write_time = current_time;
11                 // No need to handle errors in Commit. See rationale above.
12                 Commit();

Note on history of this bug: The m_best_block_index == nullptr check in the Commit() function was first added in 0243907faee0aa6af09974131d9a46a7f9c3ef38 from #24117 to avoid writing the index locator and try to prevent data corruption. It was later extended in 7878f97bf15b6e7c9b47d1c0d96419b97e1bdcbd 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 (review), but I didn’t realize how commonly the error would happen, and we didn’t do anything about it then.

achow101 commented at 4:29 pm on September 20, 2023: member

Are you still working on this?

achow101 added the label Up for grabs on Sep 20, 2023

fanquake commented at 3:41 pm on September 26, 2023: member

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

fanquake closed this on Sep 26, 2023

mzumsande commented at 9:00 pm on September 27, 2023: contributor

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?

achow101 referenced this in commit bf1b6383db on Mar 20, 2024

bitcoin locked this on Sep 26, 2024

Fix BaseIndex::Commit false error #26903

Reviews

Conflicts