validation: Make ReplayBlocks interruptible #30155
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/30155. 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. |
4bd626e to
26e78f2
Compare
| std::vector<uint256> old_heads = GetHeadBlocks(); | ||
| if (old_heads.size() == 2) { | ||
| if (old_heads[0] != hashBlock) { | ||
| LogPrintLevel(BCLog::COINDB, BCLog::Level::Error, "The coins database detected an inconsistent state, likely due to a previous crash or shutdown. You will need to restart bitcoind with the -reindex-chainstate or -reindex configuration option.\n"); |
There was a problem hiding this comment.
Is it a safe assumption that this cannot happen except an interrupted replay?
There was a problem hiding this comment.
Currently we abort with an assert in the next line after the log, so the expectation in master is that this shouldn't happen outside of db corruption. I couldn't find any reports on the internet of users that have run into this error / assert, so it probably doesn't happen very often in practice.
But theoretically speaking, if the coins db gets corrupted, I guess anything can happen depending on the specifics of the corruption?
While working on this PR, I initially went with the different approach of adding an explicit in_replay flag to BatchWrite(), and setting that flag only during the interrupt flush. I switched to the current approach of detecting this implicitly inside BatchWrite() because it seemed more elegant and led to less boilerplate code, but it's still an option.
There was a problem hiding this comment.
In commit "validation: Make ReplayBlocks interruptible" (26e78f2)
I switched to the current approach of detecting this implicitly inside
BatchWrite()because it seemed more elegant and led to less boilerplate code, but it's still an option.
I would definitely be curious to see the other approach. Not just because it might be better at detecting inconsistent states like Luke is suggesting, but also because it seems like it would make the code more clear.
I think it would be pretty hard to look at the code after this PR (without looking at the PR itself), and understand why old_heads[0] != hashBlock implies that this BatchWrite call is finalizing a replay that is currently being interrupted.
I was also confused by the name of the "unfinished_replay" variable below, because I thought "unfinished replay" was just referring to a replay of an previously unfinished flush. I did not realize it was referring specifically to an interrupted replay of an unfinished flush.
Also as a possible followup, for future maintainability of this code I think it could be a good idea to remove the replay logic entirely from BatchWrite(), since ReplayBlocks() is the only BatchWrite() caller that should need it. Other BatchWrite calls should be able to assume and assert that GetHeadBlocks() is empty, and this is just a normal write, not a replay.
There was a problem hiding this comment.
I would definitely be curious to see the other approach. Not just because it might be better at detecting inconsistent states like Luke is suggesting, but also because it seems like it would make the code more clear.
See mzumsande@db8232d - because of the inheritance the bool interrupted_replay needed to be added to quite a few spots, which I didn't love.
I think it would be pretty hard to look at the code after this PR (without looking at the PR itself), and understand why old_heads[0] != hashBlock implies that this BatchWrite call is finalizing a replay that is currently being interrupted.
I'll add more documentation (if I don't switch to the other approach).
I was also confused by the name of the "unfinished_replay" variable below,
Maybe interrupted_replay is better? But the issue remains that it's the replay that is being interrupted (or unfinished), and not the flush.
Also as a possible followup, for future maintainability of this code I think it could be a good idea to remove the replay logic entirely from BatchWrite(), since ReplayBlocks() is the only BatchWrite() caller that should need it. Other BatchWrite calls should be able to assume and assert that GetHeadBlocks() is empty, and this is just a normal write, not a replay.
Seems reasonable for a follow-up, though I haven't looked into details yet.
|
Concept ACK |
|
Concept ACK It appears to me that the size of the cache used in ReplayBlocks is never checked if it grows too large. Wouldn't this mean that if the node was sufficiently far behind, the rolling forward would fill up the cache to levels exceeding dbcache? This would result in a cycle of rolling forward and then OOM crashing, never being able to write any chainstate to disk. |
26e78f2 to
4d1342f
Compare
Good point - I think that is true, I guess users don't get the chance to change their preference in between the original run and the replay though, which is not ideal. Maybe the method introduced in this PR could also be used to do regular flushes. |
|
I have tested this with a recent interrupted reindex, it correctly interrupts and flushes and on next start continues from where we left off: and on a next run continues from where we left off: Needs a rebase, but otherwise Concept ACK |
Now, when an interrupt is received during ReplayBlocks the intermediate progress is flushed: In addition to updating the coins on disk, this flush adjusts the head blocks, so that with the next restart, the replay continues from where we stopped.
4d1342f to
d200cd5
Compare
Rebased for fresh CI (rebase seemed clean, didn't change anything) |
|
Closing as "up for grabs" - #11600 is already closed, and because we now flush the chainstate every hour replays in general shouldn't take as much time anymore, so there is less need to interrupt/continue them. That said, it could still be a nice-to-have if someone wants to pick it up (note #30155 (comment) in this case, maybe there is a more elegant way I didn't see). |
This addresses the problem from #11600 that the Rolling Forward process after a crash (
ReplayBlocks()) is uninterruptible.ReplayBlockscan take a long time to finish, and this can be especially annoying to GUI users who are taunted to "press q to shutdown" even though pressing "q" does nothing.Now, when an interrupt is received during
ReplayBlocks(), the intermediate progress is saved:In addition to writing the updated coins to disk, the flush adjusts the lower head block (saved in
DB_HEAD_BLOCKS), so that with the next restart, the replay continues from where it was stopped without losing progress.I tested this manually on signet: A situation where
ReplayBlocks()becomes necessary can be created by syncing with-dbcrashratio=1and stopping the node after ~70k blocks. Then, after the next restart, the replay process can be interrupted. I then used thedumptxoutsetoutput to check that the final utxo-set hash is unaffected by how many times the process is interrupted.