assumeutxo: add init and completion logic #24232

any reason this is a reference. memepool at the call site is already a pointer to allow setting it to nullptr

Any reason this is a pointer? This can't be null, neither at the call site nor inside the function.

any reason to add the assert?

Should probably de-activate the existing snapshot chainstate as in the if (snapshot_invalid) conditional.

Same as above. These failure modes also need tests.

We should either deactivate the snapshot chainstate or shutdown.

Looks like #24299 was merged, so you might have to inline the needed resets here (or create a helper function for it).

In commit "init: add utxo snapshot detection" (becdbd6da4c6d7c9c658ffc38b1c9da1c89571a1)

Log message seems vague about what the result of this warning is. I think it would be clearer if log message said the directory would be ignored like "[snapshot] WARNING: Multiple snapshot directories were not expected. Ignoring unexpected snapshot directory {it->path}."

Also, as long as the code is not capable of handling multiple snapshots it seems like a better design for this might just be to call the directory <datadir>/chainstate_snapshot instead of <datadir>/chainstate_{blockhash} so there would be no ambiguity. Blockhash could be dumped to a <datadir>/chainstate_snapshot/block text file, if the idea is that it should be human readable (or maybe a chainstate_snapshot/snapshot.json file to be more fancy.)

In any case, I don't know what happens to the ignored snapshot directories after the background download completes? Will it it finish validating one snapshot and then go onto validate the next snapshot next time bitcoind is restarted?

In commit "init: add utxo snapshot detection" (becdbd6da4c6d7c9c658ffc38b1c9da1c89571a1)

re: https://github.com/bitcoin/bitcoin/pull/24232/#discussion_r832762880

Also, as long as the code is not capable of handling multiple snapshots it seems like a better design for this might just be to call the directory <datadir>/chainstate_snapshot instead of <datadir>/chainstate_{blockhash} so there would be no ambiguity.

I take back this part of my suggestion. I think current design of iterating all the chainstate directories is good, but the code should be improved to be more robust (in the future, not necessarily here). I think the best thing for bitcoin to do at startup would be to list all the chainstate* directories and use whichever one has the most work at its tip as the active chainstate. Then, if that active chainstate comes from a snapshot that needs to be validated, additionally load up the "ibd" chainstate and sync it up in the background to validate the starting snapshot.

This makes the right thing happen regardless of what directories are present. Snapshots will be used if they are useful, ignored if they are not, and if they are manually added or removed it causes no problems.

I think the best thing for bitcoin to do at startup would be to list all the chainstate* directories and use whichever one has the most work at its tip as the active chainstate.

I agree this would be a nice feature, but the process of unpacking snapshots and loading them to the point of being able to evaluate "most work" entails some big changes, so I'll defer this one for later.

I do like your suggestion of calling it <datadir>/chainstate_snapshot and will incorporate that.

I've changed the code to write the snapshot chainstate leveldb to chainstate_snapshot. A separate file within that directory called base_blockhash contains a serialized uint256 of what used to be in the directory name.

In commit "validation: tear down snapshot datadirs upon validation failure" (fe09582ec34ff4d19bb2aa490ffd6ccca95e8480)

Two issues this line:

• Should not put code intended to produce side effects in an assert because it will be skipped if asserts are disabled. I know the bitcoin project does not "support" building with asserts disabled, but I don't think it should go out of its way to be broken either. Also, people who used to reading conventional c++ code may skim past asserts not expecting to see real behavior hidden inside them. Would suggest if (remove_all() <= 0) { LogPrint("Warning failed to remove {snapshot}"); } since this is handling an error condition inside an error condition, and asserts should be used to check code correctness, not handle i/o errors anyway.

• IMO, it is usually not appropriate to call removal_all in production code. One stray mv or mount bind command combined with this could result in deleting data that doesn't belong to us, and potentially very large data losses. Would be safer to use leveldb::DestroyDB on the database and fs::remove calls individually on any of our own files or directories.

Both good points, both fixed. I'm now using leveldb::DestroyDB() to remove chainstate data.

In commit "validation: tear down snapshot datadirs upon validation failure" (fe09582ec34ff4d19bb2aa490ffd6ccca95e8480)

Here and other places, may be better to replace "tear down" with "remove". I'm not exactly sure what tearing down implies, but it sounds different than removing.

In commit "validation: tear down snapshot datadirs upon validation failure" (fe09582ec34ff4d19bb2aa490ffd6ccca95e8480)

Return value is not checked here. It seems like right now this could say:

bool removed = snapshot_chainstate->TeardownSnapshotDatadir();
assert(removed);

since logically it should be impossible for any of the conditions where TeardownSnapshotDatadir returns false to trigger currently. However, this is pretty fragile, and if you're not planning to call TeardownSnapshotDatadir from other places in the future, I think it would be better to make TeardownSnapshotDatadir return void instead of bool, and turn the checks inside into asserts to make them function preconditions.

I don't mean to nitpick but I think anyplace errors are not checked it increases chances that bugs will slip in and go undetected.

In commit "blockmanager: avoid undefined behavior during FlushBlockFile" (1aeca9ae4d9938a9ae5a06dbe16d8b1cd40b9960)

Would add assert(m_blockfile_info.size() > m_last_blockfile) to catch the related UB condition.

Added, thanks.

In commit "validation: add CChainState::m_stop_use and ChainMan::isUsable" (c703836160f3eca8073c22a1531bb98ad9666999)

Would s/this chainstate/the background chainstate/. Which chainstate is "this chainstate" was not immediately obvious.

Fixed

In commit "validation: add CChainState::m_stop_use and ChainMan::isUsable" (c703836160f3eca8073c22a1531bb98ad9666999)

Would change "This might be false, for, say" to "This will be false for". IMO wishy washy comments are hard to understand and don't provide any real future proofing (A veritably false comment is still less confusing than a technically true but mostly misleading comment)

Wishywashy comment no more.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (328aa05c28fe9e5fff5d90e9b23bbbb5f6da1846)

I think it would simplify things and be less confusing to drop this checkForUncleanShutdown() method and just call maybeCompleteSnapshotValidation() here unconditionally.

I think it overcomplicates things to think of this as special "unclean" case. I think if shutdown is requested, a node should stop what it is doing and shut down. If it was in the middle of doing work, and it has to pick up and redo some of that work on the restart, there is nothing "unclean" about that, it's just what you do when you are trying to write a robust and responsive application.

Yep, good idea. Done. I'm now just calling maybeCompleteSnapshotValidation() from within LoadChainstate() (during init). This dovetails with the broader change of doing background validation cleanup on initialization (instead of shutdown) which I'll describe in other comments.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (328aa05c28fe9e5fff5d90e9b23bbbb5f6da1846)

I think this error should trigger more than logprints and a silent shutdown. Especially since after this a GUI user or systemd service script is likely to just bring the node up again and continue syncing like nothing happened. Probably would be best to call AbortNode() here to trigger a more noticeable error, and not just close the GUI with no indication of why it is closing.

Also text of this error doesn't seem to give an indication of what is causing it or how it should be dealt with. IIUC there are 3 possible causes for this error:

1. Bitcoin sources were modified to include the hash of an invalid snapshot
2. Bitcoin validation code is buggy
3. Local data or memory corruption

I'm assuming (3) is the most likely cause and the error message could reflect this. Maybe would suggest:

{PACKAGE_NAME} failed to validate the assumeutxo snapshot state. This indicates a hardware problem, or a bug in the software, or an bad software modification that allowed an invalid snapshot to be loaded. As a result of this, the node will shut down and delete any state that was built on the snapshot, resetting the chain height from {snapshot_tip_height} to {snapshot_base_height}. On the next restart, the node will resume syncing from {snapshot_base_height} not using any snapshot data.

Also to minimize potential damage, maybe help to debug or recover, and just avoid wiping traces of a bad thing that happened, i think it would probably be better to just rename the snapshot directory with an -invalid suffix instead of trying to delete it.

I think the log print can be even more proactive, if the causes of this error is 1., the log print could invite the node operator to report the snapshot source to the project, e.g opening an issue. That would help to identify and correct wrongdoing in the snapshot distribution circuit.

Now using AbortNode() as well as the nice log message you specify; I'm also including PACKAGE_BUGREPORT which lists the github issues URL. Thanks for the great suggestion.

In commit "test: add reset_chainstate parameter for snapshot unittests" (9916bb825f6ef69f6c906dd4154309df07aa9d03)

Would avoid using statement in header file because it's not limited to this file, but applies to any cpp file including it.

Done, thanks.

In commit "test: add reset_chainstate parameter for snapshot unittests" (9916bb825f6ef69f6c906dd4154309df07aa9d03)

Can the comment say what in particular is hacky and what would make it less hacky? This just seems like it is unitiializing and reinitializing the chainstate, not doing anything too egregrious. Is the hacky aspect of this just that its is duplicating a lot of code. Would it be easy to deduplicate in a followup?

Removed the mention of hackiness. Perhaps this could live as a separate function if it was found to be useful elsewhere, but I think for now it's fine here. The code itself is sort of single-purpose; I don't think there's another case where we want to do such an unceremonious unload of all the chainstate data.

In commit "validation: tear down snapshot datadirs upon validation failure" (fe09582ec34ff4d19bb2aa490ffd6ccca95e8480)

It seems fragile that in order to remove a particular snapshot chainstate's data files, we have to call a function that iterates all snapshot chainstate directories, and hope that it returns the same directory, and then do extra checks below to ensure that it did actually return the same one.

It would be nice to introduce a fs::path ChainstatePath(std::optional<uint256> from_snapshot_blockhash) function and just say fs::path snapshot_datadir = ChainstatePath(m_from_snapshot_blockhash) here. (ChainstatePath would just do the inverse of PathToSnapshotHash and probably would be useful to call other places).

EDIT: Or maybe this could just call the StoragePath() method added later to get the chainstate directory path from *this?

I've simplified all this with the rename to chainstate_snapshot.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (328aa05c28fe9e5fff5d90e9b23bbbb5f6da1846)

SnapshotBlockhash() not being nullopt seems like another thing that could be asserted above so this impossible value_or() case can go away

Done.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (328aa05c28fe9e5fff5d90e9b23bbbb5f6da1846)

Here and below I think it would be make code more readable to stop using the generic chainstate variable and instead use the unambiguous m_ibd_chainstate variable, since at this point point to the same chainstate.

Done, thanks.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (328aa05c28fe9e5fff5d90e9b23bbbb5f6da1846)

This seems like an error that actually needs to be checked for, not something that can asserted. If the snapshot blockhash comes from the directory name, an outside process renaming the directory could trigger this?

Good point, done.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (328aa05c28fe9e5fff5d90e9b23bbbb5f6da1846)

Why obfuscated?

Removed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (328aa05c28fe9e5fff5d90e9b23bbbb5f6da1846)

I don't think I understand what this comment is saying. Is using the chainstate variable future-proofing for some upcoming change? What could it mean for the caller to "act specifically based on this value"?

I must've been in a haze when I typed this - I have no idea what it means either. Removed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (328aa05c28fe9e5fff5d90e9b23bbbb5f6da1846)

This isn't really specific to shutdown. Renaming a directory before deleting it is just good practice because renaming is atomic and deleting a directory isn't.

Removed comment

In commit "add ChainstateManager.getSnapshot{Height,BaseBlock}()" (d8f32839cfbc8cbfe194d190bdc19fb3c4d6e32a)

Would rename getSnapshotHeight to getSnapshotBaseHeight for two reasons:

• To be consistent with getSnapshotBaseBlock
• To disambiguate. "Snapshot" could refer to snapshot chainstate rather than snapshot itself, and chainstate may have a height higher than the initial snapshot

This is a tangent, but I'm increasingly disliking the terms "ibd chainstate" and "snapshot chainstate." If had a magic wand I would abolish them everywhere. I think ideally a chainstate would consist of a UTXO set, a tip pointer, and "validated starting from" block pointer and "validated starting from" UTXO hash. ChainstateManager would have a list of chainstates, and one of them would be active with rest inactive. Network code would download blocks to validate chainstates, prioritizing blocks for the active chainstate. ChainstateManager would connect the incoming blocks, and if the tip of any chainstate crossed the "validated starting from" block of a later chainstate, and UTXO hash matched, the later "validated_starting_from" pointer would moved back to the earlier one, and earlier chainstate (optionally) discarded. Probably having explicit tip and "validated starting from" pointers would also allow CBlockIndex validation flags to be simplified so more code doesn't need to care about assumeutxo. This could all be cleanup though. More important to get the existing code that's already merged functional and not change things at this point.

Renamed. Agree with your thoughts on the better design, and also agree that the path from here to there is not so long. I'd just prefer to wait until we actually need to revamp the design (e.g. multiple parallel background chainstates) to do all the code changes.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (328aa05c28fe9e5fff5d90e9b23bbbb5f6da1846)

I think I'd NACK the approach for trying to rename and delete things on shutdown. I don't see why it's not possible to rename and delete the snapshot when it is validated. I understand that maybe you want the pointers to be stable so can't delete the CChainstate object, but that doesn't seem like a reason why you can't close/destroy databases and rename directories.

If I'm missing something and it really is not possible to rename the chainstate directories while the node is running, the best time to rename them would be startup time, not shutdown time. Shutdown is the time to stop work, not begin it, and it's especially precarious if bitcoin is running on a constantly rebooting windows machine or as a systemd service, where the system may ask nicely for a shutdown but quickly run out of patience and kill the process.

Good points, willfix.

I'm now doing the "cleanup" (i.e. deletion of background chainstate data) on startup, and not shutdown. I'd like to experiment with doing deletion inline in a follow-up PR.

At least one improvement of the log message could be to advice the node operator on what they should do, e.g deleting the oldest snapshot one or whatever could be appropriate.

Same comment, unclear what a node operator should do with an invalid, name-different snapshot datadir.

nit: s/ahs/has/g

Fixed, thanks

        this, NoMalleation, /*reset_chainstate=*/true));

Anywhere you are adding / touching named args, please use the style from the developer docs.

Done, thanks.

nit: maybeCompleteSnapshotValidation

Fixed, thanks.

Nice to add comments about error purposes.

Done

nit: s/proablem/problem/g and s/use/user/g

Done, thanks.

From reading ChainstateManager docs, there are two notions of IBD chainstates, the ibd chainstate and the background ibd chainstate. However, the ibd chainstate is only used here, after it has been fully validated and won't be used further as node operation is shutting down.

So what's the purpose of using the notion of an ibd_chainstate, couldn't m_ibd_chainstate be called m_background_ibd_chainstate ?

m_ibd_chainstate is the active if a snapshot is not being used, so calling it m_background_... wouldn't be appropriate.

Perhaps it isn't actually necessary to hold cs_main throughout this if I set m_stop_use immediately, and then ensure all chainstate accesses are going through isUsable() checks...

Edit: anyway, that's an optional optimization that can be looked at in a future PR.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (00b319a35cfb691a2fb831e84f63512412cae068)

Perhaps it isn't actually necessary to hold cs_main throughout this if I set m_stop_use immediately, and then ensure all chainstate accesses are going through isUsable() checks...

Edit: anyway, that's an optional optimization that can be looked at in a future PR.

Maybe change the TODO comment below to mention this idea. It seems like a more direct solution than using different locks.

Done

In commit "db: add StoragePath, Options to CDBWrapper/CCoinsViewDB" (98fc81fbaaa02792b94264680cc0062ccfde9698)

Minor: Might be a little nicer to combine two members these into a std::optional<std::path> member to avoid work when StoragePath is called, and to prevent possibility of class being in a confusing m_is_memory && !m_path.empty() state

While I agree, there are a few users of CDBWrapper outside the scope of this PR (BaseIndex::DB, CBlockTreeDB) - they seem to construct with non-empty paths even when being used in memory, so making this change will involve a few out-of-scope modifications. I agree with you that this is the right way to go, and would be a nice follow-on.

In commit "validation: add CChainState::destroyCoinsDB" (f427976e6a3e024564fa824cc0578ee9b1285198)

Is it necessary for this function to be passed a database path instead of getting the path from this->CoinsDB->StoragePath()? If so, would be helpful to add a comment saying when db_path might not the coinsdb path. If not, would be nice to eliminate the db_path argument to simplify and remove the possibility of bugs from passing the wrong path.

I think the new changes should take care of this, let me know if not.

In commit "validation: rename snapshot chainstate dir" (da4aaba0073108092f0dd0e02753eea253395c4e)

Just IMO, but snapshot_block_hash might be a more descriptive name than base_blockhash for someone just looking at directory contents.

I'm going to stick with base_blockhash just because it makes grepping for {m_,}base_blockhash easier, which is the equivalent data in SnapshotMetadata.

In commit "validation: remove snapshot datadirs upon validation failure" (5e8879c402b23f8c3b71287a1f4393caa73dfcbd)

This function was added in previous commit and is moving this commit. Would be good to just add the function to the final location in previous commit ("init: add utxo snapshot detection", 94a484340be246e38ba40644f4e2ba1913d22240) and avoid the move.

Done, thanks.

In commit "validation: remove snapshot datadirs upon validation failure" (5e8879c402b23f8c3b71287a1f4393caa73dfcbd)

Maybe this doesn't apply because our project uses assets in a nonstandard way, but I would only ever use C asserts to catch internal logic bugs which are supposed to be impossible. Would not use asserts to handle operating system errors, problems with I/O, or external libraries.

So I'd replace this assert and other assert(removed) in the function below to call AbortNode or throw runtime_error instead. Not sure if this required, though.

I would only ever use C asserts to catch internal logic bugs which are supposed to be impossible. Would not use asserts to handle operating system errors, problems with I/O, or external libraries.

This is a really good point, thanks for putting it in those terms. I've fixed up all asserts that fit this description, and I'm not wrapped each filesystem operation in a try/catch to be sure I'm catching any fs::filesystem_errors that are thrown.

In commit "validation: add CChainState::m_stop_use and ChainMan::isUsable" (70455311b695e65b847031a90fa870ab50519b48)

It seems like a good thing to drop the IsSnapshotValidated call in this commit so the GetAll() method implementation is simpler and not tied to details of snapshot validation. But just to make sure new code is equivalent to previous code, is it true that isUsable(m_ibd_chainstate) implies IsSnapshotValidated()? Like if you added a assert(!isUsable(m_ibd_chainstate) || IsSnapshotValidated()) here it would pass? Not suggesting to add this, just wondering.

I've rephrased the definition of IsSnapshotValidated() to be based on m_stop_use (i.e. IsUsable()), and so the two falling out of sync is no longer a concern here. Turns out that the m_snapshot_validated state is unnecessary.

In commit "validation: add CChainState::m_stop_use and ChainMan::isUsable" (70455311b695e65b847031a90fa870ab50519b48)

IMO, this would be a little clearer as:

    if (!ibd_usable) {
        // Snapshot must have been validated if ibd chain is out of use.
        assert(snapshot_usable);
        assert(m_snapshot_validated);
    }

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

Would s/move/stop using/. The word "move" seems less clear here, and the actual useful details about the rename are logged later.

Fixed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

I believe should log .original, not .translated

Fixed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

Curious why the code doesn't just assert index_new.nHeight <= snapshot_base_height. Is it possible for chainstates that would trigger this to even be loaded? If so, might make be better to check chainstate heights early when they are loaded than to handle it later as part of validation.

Fixed this to be clearer.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

Would be good to log that the rename fails, if it fails. Not critical, but could avoid confusion figuring out what is happening from the logs if two snapshots fail to validate.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

I'm suprised you can just rename directories while leveldb is still using them. If this works, I guess it's ok, but I think more ideally CChainstate objects would have Destroy() and Rename(new_name) methods and would do these operations in a safe way without ChainStateManager being involved at such a low level.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

Would it make sense to assert(!chainman.IsSnapshotValidated()) here?

I think it may make sense to have that assertion at the end of the function, after the call to maybeCompleteSnapshotValidation() (which I will do), but at this point if we are init'ing after having shut down after the background validation chainstate has hit the snapshot base block, we are initializing a snapshot chainstate that has been validated but needs to be removed.

That said, I'm thinking it may be possible to get rid of m_snapshot_validated/IsSnapshotValidated() altogether (in favor of the m_stop_use/isUsable() pair)... going to see if this is doable.

That said, I'm thinking it may be possible to get rid of m_snapshot_validated/IsSnapshotValidated() altogether (in favor of the m_stop_use/isUsable() pair)... going to see if this is doable.

(This was implemented in cf5a6d3cfb1c77b5689ceb7a4bfa468e79774fb0)

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

Stray comment?

Fixed, thanks.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

Was this check ever necessary to begin with when it was added in commit 94a484340be246e38ba40644f4e2ba1913d22240? It seems like IsSnapshotValidated would have always been false this whole time. Would probably be good to backport this change to the earlier commit.

Good point, backported.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

This is a little hard to follow, and maybe it's the simplest approach, but I wonder if LoadChainstate could just handle snapshot validated case internally. Treating SNAPSHOT_VALIDATION_COMPLETE like an error code when it doesn't really indicate a problem, and using continue to break out of a switch statement inside of a while loop seem like messy complications that could be nice to avoid, if there's a more straightforward way.

Yeah, definitely agree. I'll take a look.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

Maybe this is the simplest approach for now, but could you add a comment about what will cause this case to happen and what needs to be done to handle it. It seems like it would be an unusual for background validation to complete on startup. Would it basically only happen if the node was shut down after the background chainstate was validated but before it was deleted?

Also wondering why it is it even necessary to call maybeCompleteSnapshotValidation here when there is already a call in connectTip()

Also, is this approach as efficient as it could be, or is it doing unnecessary work like calling UnloadBlockIndex / LoadBlockIndex. SNAPSHOT_VALIDATION_COMPLETE causes LoadChainstate to be called again, but it's unclear which parts of LoadChainstate actually need to re-run

I think it would be good to add a code comment to explain this case more and answer some of these questions.

Would it basically only happen if the node was shut down after the background chainstate was validated but before it was deleted?

Yes, and this is now the default mode. I'm not removing any leveldb contents during runtime because re-initializing chainstates mid-run is just too delicate a change to intermingle with all this other stuff. Maybe in a future PR we can look at actually doing the "snapshot cleanup" (i.e. leveldb deletion and rename) inline when the bg validation chainstate connects up with the snapshot base block, but until then I'm just going to handle that messy stuff during init (in LoadChainstate()).

Also wondering why it is it even necessary to call maybeCompleteSnapshotValidation here when there is already a call in connectTip()

We have to rehash the contents of the bg validation's UTXO set to ensure that they match the expected value, since that hash isn't persisted anywhere after the first snapshot validation.

Also, is this approach as efficient as it could be, or is it doing unnecessary work like calling UnloadBlockIndex / LoadBlockIndex. SNAPSHOT_VALIDATION_COMPLETE causes LoadChainstate to be called again, but it's unclear which parts of LoadChainstate actually need to re-run

Good call, I've cleaned this up to avoid the confusing continue flow-control; now all chainstate re-initialization is confined to LoadChainstate(). As an added bonus, I'm now able to unittest (or, more accurately, "cpp test") this whole process!

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (2efe7c7ec82e7885e07140e1deb5b5e9d6869f1d)

Just a suggestion, but maybe s/ABCStep/ActivateBestChainStep/ to help with grepping.

Done, thank you.

In commit "move-only-ish: init: factor out chainstate initialization" (1eb127e55e1fabe1c5addccd9be0feb421149645)

Name CompleteChainstateInitialization is a bit vague. Maybe something like OpenChainstateDBs could give a better idea what the function is actually doing

Based on your other feedback of consolidating more logic into this function (which I've taken), I think the suggested name here is probably too narrow.

In commit "validation: rename snapshot chainstate dir" (60d8fabe1bd7b24257193884bb542cef55fb4252)

Could change SNAPSHOT_BLOCKHASH_FILENAME type from std::string to fs::path so you don't need any u8path call. (Also applies to read function below)

Fixed.

In commit "validation: rename snapshot chainstate dir" (60d8fabe1bd7b24257193884bb542cef55fb4252)

Preexisting problem should probably be dealt with separately, but it doesn't seem great CAutoFile ignores errors calling fclose. Might be a place to improve error reporting in the future.

Fixed.

In commit "validation: rename snapshot chainstate dir" (60d8fabe1bd7b24257193884bb542cef55fb4252)

You could consider dropping both of these fs::exists checks because they should already be covered by "failed to open base blockhash file for reading" check below. Would make code shorter and I think that error message provides more useful context anyway.

I'm going to push back on this because I do think the specificity could be helpful, even if the possibility of those errors is pretty slim. Why not be specific about errors when possible at the cost of a few lines?

In commit "validation: rename snapshot chainstate dir" (60d8fabe1bd7b24257193884bb542cef55fb4252)

Could log warnings on unexpected conditions.

--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4937,6 +4937,11 @@ static std::optional<uint256> ReadSnapshotBaseBlockhash(fs::path datadir)
         return std::nullopt;
     }
     afile >> base_blockhash;
+    if (std::fgetc(afile.Get()) != EOF) {
+        LogPrintf("[snapshot] warning: unexpected trailing data in %s\n", fs::PathToString(p));
+    } else if (std::ferror(afile.Get())) {
+        LogPrintf("[snapshot] warning: i/o error reading %s\n", fs::PathToString(p));
+    }
     return base_blockhash;
 }

I'm assuming failure would be pretty quick if blockhash was not valid (block would not be found) so it wouldn't improve anything practically to add a checksum or more validation here.

Fixed, thanks.

In commit "validation: rename snapshot chainstate dir" (60d8fabe1bd7b24257193884bb542cef55fb4252)

Throughout this commit would be good to s/datadir/chainstate_dir/ or similar. Datadir suggests top-level -datadir, I think

Fixed.

In commit "init: add utxo snapshot detection" (084c8e37494c7423ea53d165cc04354117b4938c)

I think it would clarify things to only call InitializeChainstate one place and eliminate chainstate juggling inside like:

<details><summary>Diff</summary> <p>

diff --git a/src/validation.cpp b/src/validation.cpp
index f417f82f975..38df4e94d44 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4848,28 +4848,13 @@ std::vector<CChainState*> ChainstateManager::GetAll()
     return out;
 }
 
-CChainState& ChainstateManager::InitializeChainstate(
-    CTxMemPool* mempool, const std::optional<uint256>& snapshot_blockhash)
+void ChainstateManager::InitializeChainstate(CTxMemPool* mempool)
 {
     AssertLockHeld(::cs_main);
-    bool is_snapshot = snapshot_blockhash.has_value();
-    std::unique_ptr<CChainState>& to_modify =
-        is_snapshot ? m_snapshot_chainstate : m_ibd_chainstate;
-
-    if (to_modify) {
-        throw std::logic_error("should not be overwriting a chainstate");
-    }
-    to_modify.reset(new CChainState(mempool, m_blockman, *this, snapshot_blockhash));
-
-    // Snapshot chainstates and initial IBD chaintates always become active.
-    if (is_snapshot || (!is_snapshot && !m_active_chainstate)) {
-        LogPrintf("Switching active chainstate to %s\n", to_modify->ToString());
-        m_active_chainstate = to_modify.get();
-    } else {
-        throw std::logic_error("unexpected chainstate activation");
-    }
-
-    return *to_modify;
+    assert(!m_ibd_chainstate);
+    assert(!m_active_chainstate);
+    m_ibd_chainstate = std::make_unique<CChainState>(mempool, m_blockman, *this);
+    m_active_chainstate = m_ibd_chainstate.get();
 }
 
 const AssumeutxoData* ExpectedAssumeutxo(
@@ -5308,6 +5293,7 @@ ChainstateManager::~ChainstateManager()
 
 bool ChainstateManager::DetectSnapshotChainstate(CTxMemPool* mempool)
 {
+    assert(!m_snapshot_chainstate);
     std::optional<fs::path> path = FindSnapshotChainstateDatadir();
     if (!path) {
         return false;
@@ -5318,6 +5304,8 @@ bool ChainstateManager::DetectSnapshotChainstate(CTxMemPool* mempool)
     if (!base_blockhash) {
         return false;
     }
-    this->InitializeChainstate(mempool, /*snapshot_blockhash=*/ *base_blockhash);
+    m_snapshot_chainstate = std::make_unique<CChainState>(mempool, m_blockman, *this, base_blockhash);
+    LogPrintf("Switching active chainstate to %s\n", m_snapshot_chainstate->ToString());
+    m_active_chainstate = m_snapshot_chainstate.get();
     return true;
 }
diff --git a/src/validation.h b/src/validation.h
index f8bdda160e5..70fb9c6d324 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -903,17 +903,10 @@ public:
     //! coins databases. This will be split somehow across chainstates.
     int64_t m_total_coinsdb_cache{0};
 
-    //! Instantiate a new chainstate and assign it based upon whether it is
-    //! from a snapshot.
+    //! Instantiate a new chainstate.
     //!
-    //! [@param](/bitcoin-bitcoin/contributor/param/)[in] mempool              The mempool to pass to the chainstate
-    //                                  constructor
-    //! [@param](/bitcoin-bitcoin/contributor/param/)[in] snapshot_blockhash   If given, signify that this chainstate
-    //!                                 is based on a snapshot.
-    CChainState& InitializeChainstate(
-        CTxMemPool* mempool,
-        const std::optional<uint256>& snapshot_blockhash = std::nullopt)
-        LIFETIMEBOUND EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    //! [@param](/bitcoin-bitcoin/contributor/param/)[in] mempool The mempool to pass to the chainstate constructor
+    void InitializeChainstate(CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     //! Get all chainstates currently being used.
     std::vector<CChainState*> GetAll();

Fixed, thanks.

In commit "validation: remove snapshot datadirs upon validation failure" (6e88c2cbea05b42d7ef387b83ae4147c79c26383)

AbortNode can't shut down immediately, so this assert could still trigger. It seems like it should be fine to just drop the assert since only caller to this function appears to be loadtxoutset and it is handling the false return value. Or if there is a concern about the node doing something bad with the snapshot directory still in the way, could throw an exception or call std::exit or std::abort instead of asserting

Fixed.

In commit "validation: add CChainState::m_stop_use and ChainMan::isUsable" (cf5a6d3cfb1c77b5689ceb7a4bfa468e79774fb0)

Should this be guarded by cs_main like m_snapshot_validated was? I'm guessing it can be accessed by more than one thread. If not, maybe add a comment about how it's only accessed synchronously.

Fixed.

In commit "move-only-ish: init: factor out chainstate initialization" (1eb127e55e1fabe1c5addccd9be0feb421149645)

This used to be nCoinCacheUsage instead of chainman.m_total_coinstip_cache * init_cache_fraction. Is it an intentional change, and could it be explained in the commit message? Should it be moved to the earlier commit introducing init_cache_fraction?

Fixed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (fa004947262b4180ac41b14ede7ba19ee24eaad0)

Would maybe mention that this is what normally happens, is not some unusual case that is just being cleaned up after, like I originally thought reviewing this PR. Could s/run, detect/run, the background chainstate won't be deleted until the next restart, so detect/

Fixed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (fa004947262b4180ac41b14ede7ba19ee24eaad0)

Perhaps make a method that recomputes setBlockIndexCandidates without reloading? Seems like that would cleanup BlockIndex code and simplify this code as well.

That'd be a substantial portion of LoadBlockIndex yanked out into its own function; a change like that would also require some thought as to whether the two operations can truly be independent, which I'm not sure about offhand. In the interest of keeping this PR within its already-pretty-broad scope, I'm going to save this as a follow-up for someone else.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (fa004947262b4180ac41b14ede7ba19ee24eaad0)

Seems like if these two checks dealing with the genesis block were moved into the CompleteChainstateInitialization function, the code wouldn't need to be repeated here and above

Fixed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (fa004947262b4180ac41b14ede7ba19ee24eaad0)

Should translate the format string not formatted string

--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5383,7 +5383,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation(
     assert(index_new.nHeight == snapshot_base_height);
 
     auto handle_invalid_snapshot = [&]() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
-        bilingual_str user_error = _(strprintf(
+        bilingual_str user_error = strprintf(_(
             "%s failed to validate the -assumeutxo snapshot state. "
             "This indicates a hardware problem, or a bug in the software, or a "
             "bad software modification that allowed an invalid snapshot to be "
@@ -5392,9 +5392,9 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation(
             "from %d to %d. On the next "
             "restart, the node will resume syncing from %d "
             "without using any snapshot data. "
-            "Please report this incident to %s, including how you obtained the snapshot.",
+            "Please report this incident to %s, including how you obtained the snapshot."),
             PACKAGE_NAME, snapshot_tip_height, snapshot_base_height, snapshot_base_height, PACKAGE_BUGREPORT
-        ).c_str());
+        );
 
         LogPrintf("[snapshot] !!! %s\n", user_error.original);
         LogPrintf("[snapshot] deleting snapshot, reverting to validated chain, and stopping node\n");

Fixed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (fa004947262b4180ac41b14ede7ba19ee24eaad0)

I think it would be better to use the datadir path that is was already loaded this->CoinsDB().StoragePath() instead of going outside the CChainState object and looking for a chainstate directory in the filesystem.

Fixed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (fa004947262b4180ac41b14ede7ba19ee24eaad0)

I don't think this should be an assert. It could just as easily be a filesystem error or outside change as an internal bug in the code.

Assert isn't necessary anyway, since rename will fail if directory doesn't exist. Just handling filesystem errors and not trying to anticipate them is usually simpler and more reliable unless the existence check provides additional information.

EDIT: Same comment could apply to other assert(fs::exists()) calls below

Fixed.

In commit "add utilities for deleting on-disk leveldb data" (3ad9487ad9474a3912c65832daa56a9723495407)

I don't think it is actually valid to use the previous options structs after destroying m_coins_view. For example the structs have info_log and block_cache pointers that will no longer be valid.

I'd guess it should be sufficient to drop this method, have callers just delete the whole CChainState object then call DestroyDB(path, /*options=*/{}) with a default options parameter. If not could add a dbwrapper::DestroyDB(path) wrapper that doesn't need options and fills them out itself.

Fixed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (fa004947262b4180ac41b14ede7ba19ee24eaad0)

You may want something like:

auto breakpoint_fnc = [] { if (ShutdownRequested()) throw StopHashing{}; };

try {
    ComputeUTXOStats(breakpoint_fnc);
} except (const StopHashing&) {
    return false;
}

Fixed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (fa004947262b4180ac41b14ede7ba19ee24eaad0)

I think log message is a little confusing. It will always print the same hash because the two hashes were were already compared on line 5378. Maybe move it log message above line 5378 or just print the hash once.

Fixed.

In commit "validation: add ChainMan logic for completing UTXO snapshot validation" (fa004947262b4180ac41b14ede7ba19ee24eaad0)

s/msg.translated/msg.original/ since that's what's logged

Fixed.

In commit "test: move-only-ish: factor out LoadVerifyActivateChainstate()" (1918b546a59d75abcdac80ede563e3eb552d3968)

Fixup belongs in previous commit, I think

Fixed.

In commit "docs: update assumeutxo.md" (d988631b50b8b10a8dadae00fe61ebd32058a9d1)

s/until subsequent initialization/the software is restarted/ would be clearer I think.

could also s/on subsequent inits/on software startup/ above

Fixed.

In commit "blockmanager: avoid undefined behavior during FlushBlockFile" (ae7c79103851b9157c382fab9cb8433b172e7eed)

Could consider adding a unit test to hit this code path.

I believe this is hit on every initialization, per the comment above (which is also how I realized it was a necessary change :).

In commit "blockmanager: avoid undefined behavior during FlushBlockFile" (5254307117d7bacf275a85707f29dcbdd3d24dc5)

Is commit message out of date? It says "This case becomes more than academic in the next commit, when we call MaybeRebalanceCaches() during chainstate init before the block index has been loaded." But I think this is actually happening in an earlier commit b1dbc76037c1f2ff893113057afd3f2eac581dbd, not the next commit? In any case, it would be helpful to know specifically what "during chainstate init" means in the code comment here and maybe be more specific about related commit in the commit message.

Fixed.

In commit "test: add reset_chainstate parameter for snapshot unittests" (9fb0eab4faf4b2aaabe29504ebb2a93a1c36a0e5)

This test setup code seems fragile duplicative. Maybe comment can say that it is a stripped down copy of the LoadChainstate function. Would be nice if a future followup could call LoadChainstate directly or split up the block part and the chain part of LoadChainstate and just call the chain part.

Yeah, agree - though reusing LoadChainstate in this case would require building in the option to preserve the existing block index, which (to date anyway) would be a test-specific feature. Not sure which is worse: test-specific parameter or duplicating a few lines of code.

I'm adding a note to the comment as you suggest though.

In commit "test: add testcases for snapshot completion" (2e30177903c3df456ed288a61fa05efb117d2e1c)

Maybe expose FindSnapshotChainstateDatadir function and call that, to reduce repetition here and add coverage there.

This would also be a good place to call ReadSnapshotBaseBlockhash function and test that.

Fixed; moved both of those functions to the new src/node/utxo_snapshot.cpp unit.

In commit "validation: rename snapshot chainstate dir" (60d8fabe1bd7b24257193884bb542cef55fb4252)

I get =Werror compiler warning/errors this commit because these functions are static and not called anywhere. IMO, these functions seem useful to call in unit tests and I would just make them normal non-hidden functions.

You could also move them to src/node/snapshot.cpp or something like that so they are not expanding validation.cpp which is already bursting at the seams.

Fixed.

In commit "init: add utxo snapshot detection" (084c8e37494c7423ea53d165cc04354117b4938c)

Should init_cache_fraction also be used in chainstate->InitCoinsCache call below? (It is changing later in a move-only commit, and maybe that change was supposed to happen this commit)

Fixed.

In commit "validation: remove snapshot datadirs upon validation failure" (6e88c2cbea05b42d7ef387b83ae4147c79c26383)

Seems like there is no unit test for this code path? I think it would be important to add because this seems like code that would run very rarely and could easily become broken without anybody knowing about it.

This is covered in the unittests; you can verify by observing the logline

2022-06-21T19:59:23.617500Z (mocktime: 2020-08-31T15:34:12Z) [test] [validation.cpp:4919] [DeleteCoinsDBFromDisk] Removing leveldb dir at /tmp/test_common_Bitcoin Core/c0e34f63b527f04735c4b2ae434e9b81110d7f5d39eedf5c919e4c3ccd6fabdb/regtest/chainstate_snapshot

after running

% ./src/test/test_bitcoin --log_level=message '--run_test=*/chainstatemanager_snapshot_completion' -- -printtoconsole=1 -debug=0

In commit "add ChainstateManager.GetSnapshot{BaseHeight,BaseBlock}()" (dde02d3d6af14ab94c6910b5695e9bbd6262fe55)

Seems like these would be good to call in the new unit tests

These are private methods so I think that's out. But it isn't so bad - they're already incidentally tested by ActivateSnapshot and MaybeCompleteSnapshotValidation.

Note to reviewers: this is necessary because of the simplification of InitializeChainstate(); we now only log when switching the active to a snapshot chainstate.

Minor comment, using the "in-memory" wording (and in the whole cascade of callers) to qualify the state of chainstate sounds confusing. It's could be interpreted as if the chainstate is not loaded in your DRAM, or not already persisted on your disk, or "fresh" from a validation standpoint...

While I agree that "in-memory" is a little confusing in a vacuum, that term has had a very specific meaning for awhile in the context of chainstates and coinsdb - see e.g. the arguments in_memory on CChainState::InitCoinsDB, CoinsViews::CoinsViews, and CCoinsViewDB::m_is_memory.

I think it could ease debugging and feature correctness monitoring if the assumeutxo logic gets its own LogFlags. It would be nice if one can observe the chainstate dance operating nicely, or spotting any weirdness.

It's more a global note on the assumeutxo patchet, it can be addressed latter on.

Agree, maybe will do in a later change.

Maybe log deserves its "[snapshot]" prefix.

Done.

At this PR, DeleteCoinsDBFromDisk is called only twice so it sounds there is no clear default argument so it could be better to give them explicitly. Default arguments have already been source of bugs in the past..

nit: could document when an optional not containing value is returned.

Done.

Side-note, i think it can be interesting to document PopulateAndValidateSnapshot() with what exactly this function is achieving, the sequence of checks and operations and the potential failures. It's a ~200 LoC function in the middle of the validation code.

Yeah, good point. I'll do a follow-up on this since it's out of scope for the already large change.

nit: could be documented like the writer.

Fixed.

I think this call could be modified a bit to avoid holding cs_main during the function lifetime. In ActivateSnapshot() you might have a WITH_LOCK to get the StoragePath() and give it as a argument here.

I think conceptually this should hold cs_main, since we're writing to a chainstate directory. Splitting cs_main out from per-chainstate locks should be a larger follow-on effort, and I think we can delay reworking this call until then.

All good to me.

to improve readability of this function, could be read_from.

Fixed.

By reinitializing the snapshot, you mean removing the snapshot chainstate dir and calling back loadtxoutset?

Fixed, thanks.

whitespace here

Not sure what you mean - trailing whitespace?

Well I don't have it anymore on the latest push of #25667 so all good.

I think it would be good to document the reasoning behind picking up 0.2 and to which proportion of the coin cache is allocated to each chainstate iirc the purpose of cache rebalancing.

Fixed.

I would say, as the return value of DetectSnapshotChainstate is not considered in LoadChainstate the log print could be only located only when the detection sequence is fully valid, i.e after the read of the base blockhash.

Good point, fixed.

whitespace

I think this is an okay linebreak to prevent long lines.

Is that an undocumented design assumption of our writers that they should always take cs_main lock even if there is no modification of a protected value like here afaict ? Wonder if the consistency guarantees of the files are not guaranteed at the kernel level anyway.

I assumed that it was commonly accepted that any destructive modification to a chainstate directory would require cs_main, but you make a good point that I don't think that's documented anywhere...

nit: could be documented

Fixed.

The snapshot invalid case mentioned by IsUsable could be also reflected here.

Done.

Maybe you could update GetAll documentation to "Get all chainstates currently IsUsable() to make it clear some chainstates might be filtered out.

I think the "currently being used" in the doc covers this; talking about IsUsable() would be leaking details since that's a private method.

could be good to document the ChainstateLoadingError, that would ease the review of the code paths where errors are returned and the adequateness of them.

These are apparently going away in #25308 anyway, so I'll leave this be.

What's the purpose of moving those checks from LoadChainstate() to CompleteChainstateInitialization() ? I'm not sure the comment about ThreadImport is still accurate.

It's to consolidate the calls so that we don't have to repeat them in the if (snapshot_completion.completed) case.

I believe the ThreadImport comment is still accurate: https://github.com/jamesob/bitcoin/blob/master/src/node/blockstorage.cpp#L894

Right, that's accurate.