In some cases after running the client for a bit, the chainstate seems to have blocks with a lower validity level than VALID_SCRIPTS. This causes a crash on launch in the PruneBlockIndexCandidates function, where the setBlockIndexCandidates is empty after filtering by m_chain.
Could not figure out root cause as to why, so for now just added an alert so the user is not deadended.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33127.
See the guideline for information on the review process. A summary of reviews will appear here.
No conflicts as of last run.
Could not figure out root cause as to why, so for now just added an alert so the user is not deadended.
Could you share steps to reproduce it?
Could not figure out root cause as to why, so for now just added an alert so the user is not deadended.
Could you share steps to reproduce it?
Couldn’t really reproduce it myself, just a had a data dir in a bad state
Which, if this assert is true, should imply we are in a bad data dir state where the chainstate is not included in the setBlockIndexCandidates and all setBlockIndexCandidates are less work.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/47254107937
LLM reason (✨ experimental): The CI failure is caused by lint errors related to trailing whitespace.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
3303+ if (setBlockIndexCandidates.empty()) {
3304+ m_chainman.GetNotifications().fatalError(_("Seems like your data is corrupt. Try running -reindex-chainstate or -reindex. Shutting down..."));
3305+ }
3306+
3307 // Either the current tip or a successor of it we're working towards is left in setBlockIndexCandidates.
3308 assert(!setBlockIndexCandidates.empty());
3298@@ -3299,6 +3299,11 @@ void Chainstate::PruneBlockIndexCandidates() {
3299 while (it != setBlockIndexCandidates.end() && setBlockIndexCandidates.value_comp()(*it, m_chain.Tip())) {
3300 setBlockIndexCandidates.erase(it++);
3301 }
3302+
3303+ if (setBlockIndexCandidates.empty()) {
3304+ m_chainman.GetNotifications().fatalError(_("Seems like your data is corrupt. Try running -reindex-chainstate or -reindex. Shutting down..."));
I agree this is a bad experience for non technical users.
But it seems like this is the standard right now in the code, to give this kind of alert on the code base (ie tell the the user to reindex instead of a direct action button)
If there is somewhere that actually has a button that reindexes, show me, I’ll use it. If not, adding this functionality is work for another pull request.
There already is, in src/init.cpp (“Do you want to rebuild the databases now?”)
(But I’m not sure we can detect this condition that early)
Looking at the relevant code, this feels like a symptom of a problem we could/should detect earlier somehow (which may also be early enough to follow the reindex GUI path)
Yeah agreed overall. However i cant repro and don’t have the data dir. That being said this deadend the user to a crash with no explanation or logs. And the crash is repeated on every launch. This would just a lot of node runners to give up and move on. Atleast this way they have some way out. It’s quite a soft change.
Yeah agreed overall. However i cant repro and don’t have the data dir. That being said this deadend the user to a crash with no explanation or logs. And the crash is repeated on every launch. This would just a lot of node runners to give up and move on. Atleast this way they have some way out. It’s quite a soft change.
The problem is that this is added to such a central spot that you could imagine other ways leading to this assert failing, in particular ways that have nothing to do with db corruption - in the worst case a logic bug in validation that we would want to be reported instead of telling the user to reindex, assume data corruption / blame LevelDB or the own hardware and forget about it.
Therefore it’s probably best to recognize index corruption better in BlockManager::LoadBlockIndex() or similar - for which an explanation how this occurred, or a way to reproduce would be helpful. Just telling the user to reindex on a hunch seems not great.
I agree with @mzumsande here. At a second glance this alert is too deep into the flow when the bad state can easily be detected somewhere between LoadBlockIndex() and LoadChainTip() which more directly would imply a db corruption state.
Unfortunately I lost access to the datadir to reproduce and test this. If it comes up again will fix by moving the alert up, closing for now.