index: reset indexes when running reindex-chainstate #24630

mzumsande · 2022-03-21T18:55:29Z

This resets the index dbs when running -reindex-chainstate as was previously done only for -reindex.

It fixes two bugs with -reindex-chainstate in conjunction with the indices:

coinstatsindex gets corrupted:
reindex-chainstate leads to BlockConnected() signals to the index for each block that is reprocessed:
If the index db is not reset, the running hash DB_MUHASH is not reset either and it will include data from each block twice after the reindexing, so the MuHash will be incorrect. I added a test for this that fails on master.
blockfilterindex duplicates the flatfile data:
It has a variable DB_FILTER_POS for the current position which would not get reset. As a result, the filterdata would be written twice to disk in succession.

txindex has no running variable like the other two indexes, but I think that conceptually it makes sense to reset the index db here as well.

This resets the index db for -reindex-chainstate as it is done for -reindex. Before, a reindex-chainstate would have led to a index corruption of coinstatsindex, and to a duplication of the flatfile data in blockfilterindex.

DrahtBot · 2022-03-21T19:25:16Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24789 (init, index: disallow indexes when running reindex-chainstate by mzumsande)
#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.

luke-jr · 2022-03-25T03:03:05Z

Unsure of this solution. Might be better to fix the indexes to tolerate it? (Or perhaps the calling code to pay more attention to where the indexes are at.)

(On another note, it might be nice to have a -reset-index=XYZ or such)

jnewbery · 2022-03-25T11:42:46Z

Unsure of this solution. Might be better to fix the indexes to tolerate it?

I agree that this seems like a more robust solution (as long as it doesn't result in lots of code to cover edge-cases in the indexes). I could imagine --reindex-chainstate being replaced by a command in a standalone bitcoin-chainstate application (#24304) in the future. It'd be nice if there weren't unnecessary couplings between --reindex-chainstate and the indexes.

mzumsande · 2022-03-25T12:35:25Z

Might be better to fix the indexes to tolerate it?

I can see the point that a corruption in the chainstate db shouldn't make it necessary to rebuild a block-based index.
So I think we'd need to prevent that a block that is already indexed is processed a second time on BlockConnected() signals. That could mean querying the index database for an existing entry, and if it's identical to the new one (e.g. by comparing height and hash), stop the processing.

Do you think that this should be specific to -reindex-chainstate or would all of this also apply to -reindex? I'm not sure if corruption in the blockindex db would require us to rebuild an external index, as is currently done.

maflcko · 2022-03-25T12:50:25Z

I think -reindex rewrites the blocks on disk, so at least txindex will need to be rewritten as well?

mzumsande · 2022-03-25T14:23:31Z

I think -reindex rewrites the blocks on disk, so at least txindex will need to be rewritten as well?

Does it really? I always thought it just rebuilds the blocks/index database plus the *rev files, but does not alter the *blk files.
But yes, if we can't reindex up to the index's bestblock for some reason, and have to switch to IBD at some point, in that case txindex will definitely need to be rewritten.

maflcko · 2022-03-25T14:42:08Z

You wouldn't run -reindex unless there is disk corruption (or a missing file, etc) in which case at least one file will need to be touched and may have blocks written to it in a different order (or stale blocks removed)?

luke-jr · 2022-03-26T03:37:44Z

Reviewing the base index code, I observe:

BlockConnected is written to prevent issues of this sort. It should rewind back to genesis already.
Since its reaction is to rewind to genesis, resetting the indexes seems reasonable.

So IMO, we should move forward with this PR, but also figure out why the existing rewind logic is malfunctioning...

mzumsande · 2022-03-26T22:41:24Z

So IMO, we should move forward with this PR, but also figure out why the existing rewind logic is malfunctioning...

I think it's because of the order in init:

LoadChainstate() wipes the chainstate
Init() in index/base reads the index info including the DB_BEST_BLOCK locator corresponding to the previous index height, but will then set the best block index to nullptr because we have no chain (FindForkInGlobalIndex() returns nullptr).
When BlockConnected notifications arrive Rewind() is not executed because there is no previous height to rewind from.

ryanofsky · 2022-03-29T19:18:55Z

It's good to do something about this, but it seems like an extreme workaround to wipe index data when the "-reindex-chainstate" option is used. Nothing about "-reindex-chainstate" name or documentation suggests that it will wipe other indexes. Other more mild workarounds might be:

Just warn that indexes can't currently be enabled when the "-reindex-chainstate" is in use and don't start them.
Delay starting indexes when "-reindex-chainstate" is used until the new chainstate tip catches up to the previous tip
Add a new "-reindex-everything" option that does what this PR does and wipes all indexes. If the "-reindex-chainstate" option is used when indexes are enabled it could fail and suggest temporary disabling the indexes or using "-reindex-everything" as alternatives.

Probably an ideal fix could be entirely contained in the BaseIndex class. It could just avoid doing any work and not consider any any index to be synced until the first BlockConnected notification arrives which is not an ancestor of the index's known best block. This behavior could be conditional on "-reindex-chainstate" or it could just be generic behavior.

Indexes already do have some current behavior that is a little lazy like this. For example, indexes currently don't handle BlockDisconnected events, but instead wait for the next BlockConnected event before they rewind.

mzumsande · 2022-03-29T20:28:50Z

Nothing about "-reindex-chainstate" name or documentation suggests that it will wipe other indexes.

Good point, actually it could also be mentioned in the "-reindex" help that the indexes data is wiped: ("Rebuild chain state and block index from the blk*.dat files on disk"). So "-reindex" is currently "-reindex-everything" (including the Block Index database).

Option 1) should be trivial to implement, but the warning could be missed by the user who then wonders why the index isn't running.

Delay starting indexes when "-reindex-chainstate" is used until the new chainstate tip catches up to the previous tip
Probably an ideal fix could be entirely contained in the BaseIndex class. It could just avoid doing any work and not consider any any index to be synced until the first BlockConnected notification arrives which is not an ancestor of the index's known best block.

The challenge I see here, as well as for option 2), is that the reindexing happens in the loadblk thread which can run well until after the node has started. So we don't only need to ignore BlockConnected notifications until then, we'd also have to delay the Init part of the index, because we can't derive what the best block is from the saved DB_BEST_BLOCK locator without a chain.

I'll try to make a milder option work, putting the PR into draft until then.

ryanofsky · 2022-03-29T21:00:54Z

we'd also have to delay the Init part of the index, because we can't derive what the best block is from the saved DB_BEST_BLOCK locator without a chain.

I think we just need CBlockTreeDB index in blocks/index/ which is not wiped when fReindexChainState is true:

bitcoin/src/node/chainstate.cpp

Line 41 in ecf692b

    
           pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, block_tree_db_in_memory, fReset));

which is different than the CoinsDB index in chainstate/ which is wiped:

bitcoin/src/node/chainstate.cpp

Lines 84 to 87 in ecf692b

    
           chainstate->InitCoinsDB( 
        
               /* cache_size_bytes */ nCoinDBCache, 
        
               /* in_memory */ coins_db_in_memory, 
        
               /* should_wipe */ fReset || fReindexChainState);

to determine whether blockConnected block is ancestor of the index locator or previous coins tip.

But these things are more for long term fixes. For a short term fix I think just disabling other indexes when -reindex-chainstate is used or just showing a startup error that says indexes can't be enabled while -reindex-chainstate is enabled would already be improvements over status quo if status quo is to corrupt the indexes.

(Would suggest opening a new PR if you implement a new approach, too, so older comments here can be easily understood, and so different approaches can be compared)

mzumsande · 2022-04-06T15:57:12Z

But these things are more for long term fixes. For a short term fix I think just disabling other indexes when -reindex-chainstate is used or just showing a startup error that says indexes can't be enabled while -reindex-chainstate is enabled would already be improvements over status quo if status quo is to corrupt the indexes.

(Would suggest opening a new PR if you implement a new approach, too, so older comments here can be easily understood, and so different approaches can be compared)

See #24789 for an alternative that just disallows the interaction.

luke-jr

ryanofsky · 2022-04-20T01:13:14Z

See #24789 for an alternative that just disallows the interaction.

I do think #24789 is a better alternative to this PR in it's current form (cf531ba), because it currently it clears and rebuilds all the indexes if -reindex-chainstate is used. Clearing the indexes is better than corrupting them, but still seems like a surprising and potentially undesirable behavior. Code and tests in this PR seem like a useful start for getting the indexing options to work better together, though.

mzumsande · 2022-04-21T14:16:37Z

Closed in favor of #24789

…nstate dac44fc init: disallow reindex-chainstate with optional indexes (Martin Zumsande) 62e1428 doc: Add note that -reindex will rebuild optional indexes (Martin Zumsande) Pull request description: When started together with `-reindex-chainstate`, currently coinstatsindex gets corrupted and the blockfilterindex flatfiles duplicated. See the OP of #24630 for more a more detailed explanation on why this happens. This is an alternative to #24630 which does not wipe and rebuild the indexes but returns an `InitError` when they are activated, thus requiring the user to deactivate them temporarily until the `-reindex-chainstate` run is finished. This also disallows `-reindex-chainstate` in combination with `-txindex`, which is not leading to corruption, but currently still rebuilds the index unnecessarily and unexpectedly. As a long-term goal, it would be desirable to have the indexes tolerate `reindex-chainstate` by ignoring their `BlockConnected` notifications (there is discussion in #24630 about this) or possibly move `reindex-chainstate` option into a `bitcoin-chainstate` executable, which could also solve the problem. But these would be larger projects - until then, it might be better to disallow the interaction than having corrupted indexes. The first commit adjusts the `-reindex` doc to mention that this option does rebuild all active indexes. ACKs for top commit: ryanofsky: Code review ACK dac44fc. Just fixed IsArgSet call and edited error messages since last review Tree-SHA512: c1abf7d350648ae227c3fd6c95d9a54c3bac9de70915275dea1c87cca6d9a76a056c0e306d95ef8cfe4df1f8525b418e0e7a4f52ded3be464041c0dc297f8930

This resets the index db for -reindex-chainstate as it is done for -reindex. Before, a reindex-chainstate would have led to a index corruption of coinstatsindex, and to a duplication of the flatfile data in blockfilterindex. Github-Pull: bitcoin#24630 Rebased-From: cf531ba

mzumsande force-pushed the 202203_index_reindex_cs branch 2 times, most recently from 69251bf to 35e669b Compare March 21, 2022 19:13

index: reset indexes on reindex-chainstate

cf531ba

This resets the index db for -reindex-chainstate as it is done for -reindex. Before, a reindex-chainstate would have led to a index corruption of coinstatsindex, and to a duplication of the flatfile data in blockfilterindex.

mzumsande force-pushed the 202203_index_reindex_cs branch from 35e669b to cf531ba Compare March 21, 2022 19:23

DrahtBot added the Tests label Mar 21, 2022

DrahtBot mentioned this pull request Mar 22, 2022

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

Draft

mzumsande marked this pull request as draft March 29, 2022 20:28

mzumsande mentioned this pull request Apr 6, 2022

init, index: disallow indexes when running reindex-chainstate #24789

Merged

luke-jr approved these changes Apr 13, 2022

View reviewed changes

mzumsande closed this Apr 21, 2022

bitcoin locked and limited conversation to collaborators Apr 21, 2023

index: reset indexes when running reindex-chainstate #24630

index: reset indexes when running reindex-chainstate #24630

Uh oh!

Conversation

mzumsande commented Mar 21, 2022

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

luke-jr commented Mar 25, 2022

Uh oh!

jnewbery commented Mar 25, 2022

Uh oh!

mzumsande commented Mar 25, 2022

Uh oh!

maflcko commented Mar 25, 2022

Uh oh!

mzumsande commented Mar 25, 2022

Uh oh!

maflcko commented Mar 25, 2022

Uh oh!

luke-jr commented Mar 26, 2022

Uh oh!

mzumsande commented Mar 26, 2022

Uh oh!

ryanofsky commented Mar 29, 2022

Uh oh!

mzumsande commented Mar 29, 2022

Uh oh!

ryanofsky commented Mar 29, 2022

Uh oh!

mzumsande commented Apr 6, 2022

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Apr 20, 2022

Uh oh!

mzumsande commented Apr 21, 2022

Uh oh!

Uh oh!

DrahtBot commented Mar 21, 2022 •

edited

Loading