validation: In case of a continued reindex, only activate chain in the end #31439
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31439. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
This simplifies the code. The only reason to call ActivateBestChain() here is to allow the main init thread to finish startup in a case of -reindex. In this situation no second chainstate can exist anyway because -reindex would have deleted any snapshot chainstate earlier.
mzumsande
force-pushed
the
202412_reindex_interrupt
branch
from
b91a191 to
0ab9cbe
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
If a reindex was interrupted while it was iterating through the block files, genesis will already be connected when the reindex resumes at the next startup. In this case, a call to ActivateBestChainState() is not only unnecessary, but it would connect multiple blocks without applying -assumevalid, which is much slower. This is because assumevalid requires us to have a header above the minimum chainwork, but that header is unknown to us if it's in a later blockfile not indexed yet.
mzumsande
force-pushed
the
202412_reindex_interrupt
branch
from
0ab9cbe to
570af72
Compare
|
@mzumsande, do you happen to know why setting Edit: Detailsi.e. during synchronization the UpdateTip already starts and fScriptChecks gets enabled until the correct block is loaded, which will disable it - weird |
|
I have tested 570af72, did an IBD to a clean |
I can't reproduce this. Assuming it's not related to reindex, on an empty datadir after header sync, whether I start with
Did you do this on mainnet? That would be because This is a general issue if you do reindex with a datadir that doesn't have blocks above the minchainwork threshold in it - this PR doesn't solve this. |
There was a problem hiding this comment.
Concept ACK - I was somehow under the impression that this was done on purpose to guarantee the validity of the already done work before continuing, but could never come up with a really good reason of why this should be the case - and I guess there really isn't one!
Reading through this code again recently makes me wish we'd have better handling for the genesis block in general. It feels weird that it is only activated in the first place during a fresh startup once ImportBlocks is called. It is also inconvenient for kernel users, since they need to manually call ActivateBestChain on startup (like done in bitcoin-chainstate). I wonder if it would be better to move loading it into load chainstate, for example like I did here (though that patch is pretty hacky with hardcoding the flat file position and making the registered validation interfaces miss the first notification and has some other implications).
|
Concept ACK. These changes seem clearly better than current code, though the fix here seems like a workaround for a workaround for a workaround. IIUC, the original problem leading to this complexity was a crash in some code that assumed the genesis block existed, so a wait was added in #5243 to prevent that crash (first workaround instead of just making code handle the state and not crash). Then this wait apparently prevented gui from showing reindex progress so an ActivateBestChain call was added in #7917 to connect the genesis block and bypass the wait (second workaround, which can connect too many blocks after reindexing is interrupted or when -loadblock is used, leading to this bug). Now a third workaround is being implemented to prevent connecting too many blocks in the typical case when reindexing is interrupted and blocks are written in expected order with genesis first. But this is still a workaround, not a solid fix because other blocks besides the genesis block can still be connected in other cases. It seems to me like things would be simpler and more reliable if we could delete the wait and delete the ActivateBestChain call, but as long as they need to exist, this fix seems strictly better than the current code. Am wondering if there is some way to write a functional test for this PR. It'd be hard to write a test that interrupts reindexing at the right time, but maybe there could be a test that uses two -loadblock arguments to load the same blocks multiple times and makes sure all the blocks except the genesis blocks are only connected together at the very end. |
|
Thanks for digging up the history!
I don't completely understand this - do you mean -loadblock with specially crafted blockfiles in which block appear in a certain order?
Do you know any concrete examples for the latter? I was thinking that this was more of a solid fix because
So, if we call ABC directly after calling That being said, I agree it would be nice if we could remove this complexity, but I think it can only be done together with removing the wait-for-genesis in |
|
So it looks me like there are several ideas for further improvements in this general area:
|
If a user interrupts a reindex while it is iterating over the block files, it will continue to reindex with the next node start (if the
-reindexarg is dropped, otherwise it will start reindexing from scratch).However, due to an early call to
ActivateBestChainState()that only exists to connect the genesis block duringthe original
-reindex, it wil start connecting blocks immediately before having iterated through all block files.Because later headers above the minchainwork threshold won't be loaded in this case,
-assumevalidwill notbe applied and the process is much slower due to script validation being done.
Fix this by only calling
ActivateBestChainState()here if Genesis is not connected yet (equivalent toActiveHeight() == -1).Also simplify this spot by only doing this for the active chainstate instead of looping over all chainstates (first commit).
This issue was discussed in the thread below #31346 (comment), the impact on assumevalid was found by l0rinc.
The fix can be tested by manually aborting a
-reindexe.g. on signet and observing in the debug log the order in which blockfiles are indexed / blocks are connected with this branch vs master.