assumeutxo: add init and completion logic #24232

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.

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.

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:

0bool removed = snapshot_chainstate->TeardownSnapshotDatadir();
1assert(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.

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.

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)

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.

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.

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.

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?

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?

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

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.

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?

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

Why obfuscated?

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”?

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.

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.

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.

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

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

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 ?

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.

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

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.

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.

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.

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.

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

IMO, this would be a little clearer as:

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

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.

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

I believe should log .original, not .translated

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.

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?

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.

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.

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.

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

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)

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.

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.

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

Could log warnings on unexpected conditions.

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

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.

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

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:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index f417f82f975..38df4e94d44 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -4848,28 +4848,13 @@ std::vector<CChainState*> ChainstateManager::GetAll()
 5     return out;
 6 }
 7 
 8-CChainState& ChainstateManager::InitializeChainstate(
 9-    CTxMemPool* mempool, const std::optional<uint256>& snapshot_blockhash)
10+void ChainstateManager::InitializeChainstate(CTxMemPool* mempool)
11 {
12     AssertLockHeld(::cs_main);
13-    bool is_snapshot = snapshot_blockhash.has_value();
14-    std::unique_ptr<CChainState>& to_modify =
15-        is_snapshot ? m_snapshot_chainstate : m_ibd_chainstate;
16-
17-    if (to_modify) {
18-        throw std::logic_error("should not be overwriting a chainstate");
19-    }
20-    to_modify.reset(new CChainState(mempool, m_blockman, *this, snapshot_blockhash));
21-
22-    // Snapshot chainstates and initial IBD chaintates always become active.
23-    if (is_snapshot || (!is_snapshot && !m_active_chainstate)) {
24-        LogPrintf("Switching active chainstate to %s\n", to_modify->ToString());
25-        m_active_chainstate = to_modify.get();
26-    } else {
27-        throw std::logic_error("unexpected chainstate activation");
28-    }
29-
30-    return *to_modify;
31+    assert(!m_ibd_chainstate);
32+    assert(!m_active_chainstate);
33+    m_ibd_chainstate = std::make_unique<CChainState>(mempool, m_blockman, *this);
34+    m_active_chainstate = m_ibd_chainstate.get();
35 }
36 
37 const AssumeutxoData* ExpectedAssumeutxo(
38@@ -5308,6 +5293,7 @@ ChainstateManager::~ChainstateManager()
39 
40 bool ChainstateManager::DetectSnapshotChainstate(CTxMemPool* mempool)
41 {
42+    assert(!m_snapshot_chainstate);
43     std::optional<fs::path> path = FindSnapshotChainstateDatadir();
44     if (!path) {
45         return false;
46@@ -5318,6 +5304,8 @@ bool ChainstateManager::DetectSnapshotChainstate(CTxMemPool* mempool)
47     if (!base_blockhash) {
48         return false;
49     }
50-    this->InitializeChainstate(mempool, /*snapshot_blockhash=*/ *base_blockhash);
51+    m_snapshot_chainstate = std::make_unique<CChainState>(mempool, m_blockman, *this, base_blockhash);
52+    LogPrintf("Switching active chainstate to %s\n", m_snapshot_chainstate->ToString());
53+    m_active_chainstate = m_snapshot_chainstate.get();
54     return true;
55 }
56diff --git a/src/validation.h b/src/validation.h
57index f8bdda160e5..70fb9c6d324 100644
58--- a/src/validation.h
59+++ b/src/validation.h
60@@ -903,17 +903,10 @@ public:
61     //! coins databases. This will be split somehow across chainstates.
62     int64_t m_total_coinsdb_cache{0};
63 
64-    //! Instantiate a new chainstate and assign it based upon whether it is
65-    //! from a snapshot.
66+    //! Instantiate a new chainstate.
67     //!
68-    //! [@param](/bitcoin-bitcoin/contributor/param/)[in] mempool              The mempool to pass to the chainstate
69-    //                                  constructor
70-    //! [@param](/bitcoin-bitcoin/contributor/param/)[in] snapshot_blockhash   If given, signify that this chainstate
71-    //!                                 is based on a snapshot.
72-    CChainState& InitializeChainstate(
73-        CTxMemPool* mempool,
74-        const std::optional<uint256>& snapshot_blockhash = std::nullopt)
75-        LIFETIMEBOUND EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
76+    //! [@param](/bitcoin-bitcoin/contributor/param/)[in] mempool The mempool to pass to the chainstate constructor
77+    void InitializeChainstate(CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
78 
79     //! Get all chainstates currently being used.
80     std::vector<CChainState*> GetAll();

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

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.

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?

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/

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.

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

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

Should translate the format string not formatted string

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

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.

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

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.

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

You may want something like:

0auto breakpoint_fnc = [] { if (ShutdownRequested()) throw StopHashing{}; };
1
2try {
3    ComputeUTXOStats(breakpoint_fnc);
4} except (const StopHashing&) {
5    return false;
6}

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.

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

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

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

Fixup belongs in previous commit, I think

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

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

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

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.