index: reset indexes when running reindex-chainstate

mzumsande commented at 6:55 pm on March 21, 2022: member

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.

mzumsande force-pushed on Mar 21, 2022

index: reset indexes on reindex-chainstate

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.

cf531ba531

mzumsande force-pushed on Mar 21, 2022

DrahtBot commented at 7:25 pm on March 21, 2022: member

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.

DrahtBot added the label Tests on Mar 21, 2022

luke-jr commented at 3:03 am on March 25, 2022: member

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 commented at 11:42 am on March 25, 2022: member

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 commented at 12:35 pm on March 25, 2022: member

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.

MarcoFalke commented at 12:50 pm on March 25, 2022: member

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

mzumsande commented at 2:23 pm on March 25, 2022: member

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.

MarcoFalke commented at 2:42 pm on March 25, 2022: member

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 commented at 3:37 am on March 26, 2022: member

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 commented at 10:41 pm on March 26, 2022: member

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 commented at 7:18 pm on March 29, 2022: member

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 commented at 8:28 pm on March 29, 2022: member

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.

mzumsande marked this as a draft on Mar 29, 2022

ryanofsky commented at 9:00 pm on March 29, 2022: member

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:

https://github.com/bitcoin/bitcoin/blob/ecf692b466860f44334a1da967fc2559da913bec/src/node/chainstate.cpp#L41

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

https://github.com/bitcoin/bitcoin/blob/ecf692b466860f44334a1da967fc2559da913bec/src/node/chainstate.cpp#L84-L87

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 commented at 3:57 pm on April 6, 2022: member

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 approved

luke-jr commented at 8:07 pm on April 13, 2022: member

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3utACK cf531ba531ceb78a94b911fdb8f8d5bfea455380
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQQzBAEBCAAdFiEE5GOpP18xF+7ebHMWvQKUJCH0iJ8FAmJXLXAACgkQvQKUJCH0
 7iJ+cMSAAvRiY+px/GUzY65GoVHoJFjKdNRl4xI1t1DR/SxlnddA5ql66XjtNERya
 8NbQSVn59yG2mUZl2CF1sFh5sbU1fZiBoHFDy00TpmMLz8xHNGi51ZE0SiXj/0x3j
 9KsKhpmqMgqtARocbVMVdkj1QxUOA7KDqcTjtlzLzLtZJzLq5Dbp65bqLfJliE3R+
10gKgItkyllOr97XCdwpELMzVCBxb8B/PMea4nxNCHaLB26Pq590FVMRveR+s0JGZ8
11JfcPr2kTIp7LpVSHHS6nU1oHVVBmIZGrBnZYogm2Z0hPcLiHIKDiYHbNAqq/REiF
12KGlglsDUctvIddewWmm0nag8NytrLjaAdPk0wU1oFrc1qt4ymt/r1X0LAQaf8Vgo
13DxKV8Ao7E7JOp0fgioErlcBkISaZt5u3Dtn6Fs523yH8eq2byDAsCNqU9O8ptPg1
14gzY0YWV/mGLe4xe7b/Qd0k2aV4c422DXLdq2pY+Aqonfhq9CtDIuUDX78+tHeB0m
15gdSQZCmbETaS1csDvpCdq0s1R4IYi23g0UF+7AeZmNEQnNhOCXPCM3+mJ84dXYti
16wn4LBEn1AoMnPGiBDVLU7djoKqfv1DkuCpHXrR0usVFls3JkTQOWbzyVq4B1yFX8
17qWA/krZYd9smmGXMDWPQ5LEA0DWvpa+pQ0p1Qm9IwvDJr+NHr5sXLQWYh801xoSF
18/2yoHV58JS2ge5uAWY9jtGXfitGzD7/PHOicce0JY+sLI1B5lMLYCK5ydZCUpatV
19wEZ0sNk6NQs4RvUKfNR09AQ/NMaN7Vy4frQebMs44FZN4lYHxZc+WVjhrfhiah3j
20CVcQhyX18/c4GnTrgAZuhko8TDtpp+Ji3i2Uko29z3PZcRKpX0zwP8iNY0e/pfWv
21BaITWz/qUJQ2Q5SSWO2wNDjOQpDB5XTVX0lfirmH0Z0iJBpKCEcFh2i9TBDypuQZ
22SFkCkIGwcwRxDvnpp7Si9Lt+QqGOxR7AajfyjSmG98x30/xHPuUW604k4PZ/QBsE
23YYU+FM0Dut+Sp85kbgiMltstdatq+DzvIlc2zPHeBRVR3+f+EZdb1jIzbeB4CccI
24p321OcKbVNPR5UktLVmUiVupc1/qlIzi09Xkz9cES46sTIe6OUQzwylb8MoiEt5/
25wKU2VEz/OmdU9OY5cZkAOv8KDAx8xjhi94ptWfeD8NyzD3Rw6fPclW2Uvq+S+G0Y
26SCPRWrlhYVm05+E6ZoG8h+FuBwDtiwVkq+bFY6dwPzTm5u+Ofwj0gL6vFk3glErI
27xvDDiWWSt4OBadTCx7IOFegJ8SoxMqS1pHcrTdDc1D9ty/Ew/41crCLNsagEuKBY
28RrdoRwbywN4/RVToVPfhSkDnu9AFfQ==
29=/qjs
30-----END PGP SIGNATURE-----

ryanofsky commented at 1:13 am on April 20, 2022: member

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 (cf531ba531ceb78a94b911fdb8f8d5bfea455380), 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 commented at 2:16 pm on April 21, 2022: member

Closed in favor of #24789

mzumsande closed this on Apr 21, 2022

fanquake referenced this in commit 269dcad16e on Apr 26, 2022

luke-jr referenced this in commit c37ab289c4 on May 21, 2022

DrahtBot locked this on Apr 21, 2023

index: reset indexes when running reindex-chainstate #24630

Conflicts