assumeutxo: snapshot initialization

jamesob commented at 5:18 pm on July 21, 2022: member

This is part of the assumeutxo project (parent PR: #15606)

Half of the replacement for #24232. The original PR grew larger than expected throughout the review process.

This change adds the ability to initialize a snapshot-based chainstate during init if one is detected on disk. This is of course unused as of now (aside from in unittests) given that we haven’t yet enabled actually loading snapshots.

Don’t be scared! There are some big move-only commits in here.

Accompanying changes include:

moving the snapshot coinsdb directory from being called chainstate_[base blockhash] to chainstate_snapshot, since we only support one snapshot in use at a time. This simplifies some logic, but it necessitates writing that base blockhash out to a file within the coinsdb dir. See discussion here.
adding a simple fix in FlushBlockFile() that avoids a crash when attemping to flush to disk before LoadBlockIndexDB() is called, which happens when calling MaybeRebalanceCaches() during multiple chainstate init.
improving the unittest to allow testing with on-disk chainstates - necessary to test a simulated restart and re-initialization.

jamesob force-pushed on Jul 21, 2022

DrahtBot commented at 6:21 pm on July 21, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26283 (p2p: Fill reconciliation sets and request reconciliation (Erlay) by naumenkogs)
#25977 (refactor: Replace std::optional<bilingual_str> with util::Result by ryanofsky)
#25862 (refactor, kernel: Remove gArgs accesses from dbwrapper and txdb by ryanofsky)
#25704 (refactor: Remove almost all validation option globals by MarcoFalke)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
#23443 (p2p: Erlay support signaling by naumenkogs)
#22663 (dbwrapper: properly destroy objects in case CDBWrapper ctor throws by Crypt-iQ)
#19888 (rpc, test: Improve getblockstats for unspendables by fjahr)
#17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

laanwj added the label UTXO Db and Indexes on Jul 21, 2022

laanwj added the label Validation on Jul 21, 2022

jamesob force-pushed on Jul 21, 2022

in src/test/validation_chainstatemanager_tests.cpp:382 in 15da925f29 outdated

373+            BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0);
374+            const ChainstateManager::Options chainman_opts{
375+                ::Params(),
376+                static_cast<int64_t(*)()>(::GetTime),
377+            };
378+            m_node.chainman.reset(new ChainstateManager(chainman_opts));

ryanofsky commented at 2:11 pm on July 22, 2022:

In commit “test: add testcases for snapshot initialization” (15da925f2951eaadf81aea9d5b67ec1646581d30)

Might be a little more robust to destroy the old chainman before creating a new one. I.e. add another blank chainman.reset() call before this.

jamesob commented at 7:03 pm on September 6, 2022:

Fixed, thanks.

ryanofsky approved

ryanofsky commented at 2:15 pm on July 22, 2022: contributor

Code review ACK 15da925f2951eaadf81aea9d5b67ec1646581d30. All commits here except the last two were directly cherry-picked from previously reviewed #24232.

in src/node/chainstate.cpp:58 in 15da925f29 outdated

37 
38+    // Load the fully validated chainstate.
39+    chainman.InitializeChainstate(options.mempool);
40+
41+    // Load a chain created from a UTXO snapshot, if any exist.
42+    chainman.DetectSnapshotChainstate(options.mempool);

ariard commented at 3:55 pm on August 1, 2022:

Am I correct this is not documented in design/assumeutxo.md ? I think this is the case where the user already loaded a snapshot UTXO through loadtxoutset, a chainstate_[SHA256 blockhash of snapshot base block] has been created and the bitcoind process is restarted.

(can be done in a follow-up)

jamesob commented at 7:05 pm on September 6, 2022:

This wasn’t documented, but I’ve just added some related doc in this PR. There are more doc updates, of course, in the follow-on PR (#25740).

in src/validation.cpp:4770 in 15da925f29 outdated

4707@@ -4720,6 +4708,46 @@ const AssumeutxoData* ExpectedAssumeutxo(
4708     return nullptr;
4709 }
4710 
4711+static bool DeleteCoinsDBFromDisk(const fs::path db_path, bool is_snapshot)

ariard commented at 4:27 pm on August 1, 2022:

note for reviewers: for now is_snapshot is always true as there is a single usage. This is not the case anymore with the remaining commits of the #24232 patchset.

in src/validation.h:1058 in 15da925f29 outdated

988+    //! snapshot that is in the process of being validated.
989+    bool DetectSnapshotChainstate(CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
990+
991+    void ResetChainstates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
992+
993+    CChainState& ActivateExistingSnapshot(CTxMemPool* mempool, uint256 base_blockhash)

ariard commented at 4:38 pm on August 1, 2022:

even if name is explicit, could be documented “Switch active chainstate to snapshot one” in case of future refactor to check function correctness (and DetectSnapshotChainstate could say it activates the snapshot if any)

jamesob commented at 7:05 pm on September 6, 2022:

Done, thanks.

in src/node/utxo_snapshot.h:48 in 15da925f29 outdated

38@@ -33,6 +39,30 @@ class SnapshotMetadata
39 
40     SERIALIZE_METHODS(SnapshotMetadata, obj) { READWRITE(obj.m_base_blockhash, obj.m_coins_count); }
41 };
42+
43+//! The file in the snapshot chainstate dir which stores the base blockhash. This
44+//! is needed to reconstruct snapshot chainstates on init.
45+const fs::path SNAPSHOT_BLOCKHASH_FILENAME{"base_blockhash"};

ariard commented at 4:47 pm on August 1, 2022:

comment can be extended “There should be a single snapshot at any given time”

jamesob commented at 7:05 pm on September 6, 2022:

Done.

in src/validation.h:1054 in 15da925f29 outdated

983@@ -990,6 +984,15 @@ class ChainstateManager
984     /** Produce the necessary coinbase commitment for a block (modifies the hash, don't call for mined blocks). */
985     std::vector<unsigned char> GenerateCoinbaseCommitment(CBlock& block, const CBlockIndex* pindexPrev) const;
986 
987+    //! When starting up, search the datadir for a chainstate based on a UTXO
988+    //! snapshot that is in the process of being validated.
989+    bool DetectSnapshotChainstate(CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
990+
991+    void ResetChainstates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

ariard commented at 4:52 pm on August 1, 2022:

note to reviewers: this method is used in the remaining #24232 patchset.

ariard commented at 4:55 pm on August 1, 2022: member

Code Review ACK 15da925f, my comments on #24232 have been addressed.

ariard commented at 5:00 pm on August 1, 2022: member

@dergoegge I think you could be interested to review the assumeutxo work. In case of future hypothetical integration of libutreexo in Core, I believe there would be intersecting code paths and data structure modified. It might also prepare for future synergies between utreexo and assumeutxo.

dongcarl commented at 7:51 pm on August 2, 2022: contributor

Planning on diving deeply into this, thanks for splitting it up @jamesob!

Questions from going over the commits:

Q1: Is the intended layout:

<datadir>
- chainstate
  - *.ldb
  - *.log
  - CURRENT
  - LOCK
  - MANIFEST-*
- chainstate_snapshot
  - base_blockhash
  - *.ldb
  - *.log
  - CURRENT
  - LOCK
  - MANIFEST-*

Q2: The last 6 commits seem to be focused on unit tests, but it seems to me that the functionality being added here is better tested with functional tests. Is there something we need to test here that a functional test won’t be able to do?

in src/validation.cpp:4795 in aa26b508fb outdated

4731+    std::string path_str = fs::PathToString(db_path);
4732+    LogPrintf("Removing leveldb dir at %s\n", path_str);
4733+
4734+    // We have to destruct before this call leveldb::DB in order to release the db
4735+    // lock, otherwise `DestroyDB` will fail. See `leveldb::~DBImpl()`.
4736+    const bool destroyed = dbwrapper::DestroyDB(path_str, {}).ok();

dongcarl commented at 7:56 pm on August 2, 2022:

Note to other reviewers: the call to DestroyDB will actually remove the entire directory, including the blockhash file if it still existed since we place the blockhash file inside what leveldb considers to be its directory, but it’s probably better (more polite?) to remove it ourselves just so we can print nice errors.

https://github.com/bitcoin/bitcoin/blob/de3c46c93818c6e4000175bcdbf7dd332f54768d/src/leveldb/db/db_impl.cc#L1550

DrahtBot added the label Needs rebase on Aug 3, 2022

in src/txdb.h:12 in 48354fba63 outdated

 8@@ -9,6 +9,7 @@
 9 #include <coins.h>
10 #include <dbwrapper.h>
11 #include <sync.h>
12+#include <fs.h>

fjahr commented at 6:38 pm on August 7, 2022:

nit: Can be swapped with sync.h above to have alphabetical order

ariard commented at 1:47 am on August 23, 2022: member

This PR needs rebase.

jamesob force-pushed on Sep 6, 2022

jamesob commented at 7:09 pm on September 6, 2022: member

Hi all, sorry for the delay. My spirit has been weak the last few months, and I took two weeks off to try and shake the burnout.

I’ve addressed all feedback listed here and rebased. @dongcarl asks:

Q1: Is the intended layout:

Yep, it is.

Q2: The last 6 commits seem to be focused on unit tests, but it seems to me that the functionality being added here is better tested with functional tests. Is there something we need to test here that a functional test won’t be able to do?

We can’t have functional tests to exercise this behavior until we add loadtxoutset to the RPC interface, which will effectively reveal assumeutxo to end users before all the necessary changes have been made to make it usable. Plus, I think the unittests allow us to more easily express granular testcases and assertions.

DrahtBot removed the label Needs rebase on Sep 6, 2022

jamesob force-pushed on Sep 6, 2022

in doc/design/assumeutxo.md:80 in f73ca14c2f outdated

76@@ -77,7 +77,9 @@ original chainstate remains in use as active.
77 Once the snapshot chainstate is loaded and validated, it is promoted to active
78 chainstate and a sync to tip begins. A new chainstate directory is created in the
79 datadir for the snapshot chainstate called
80-`chainstate_[SHA256 blockhash of snapshot base block]`.
81+`chainstate_[SHA256 blockhash of snapshot base block]`. When this directory is present

naumenkogs commented at 9:55 am on September 7, 2022:

isn’t this outdated now after d59db4ff4b585d2b86b68ea0db206ec1c0cc5022?

jamesob commented at 4:34 pm on September 7, 2022:

Oops, good catch.

jamesob force-pushed on Sep 7, 2022

naumenkogs commented at 4:59 pm on September 7, 2022: member

utACK 1430518eeced979449c3b031bea02fd707b26d1a

in src/dbwrapper.h:231 in 75bb0c54b3 outdated

222@@ -219,6 +223,12 @@ class CDBWrapper
223 
224     std::vector<unsigned char> CreateObfuscateKey() const;
225 
226+    //! path to filesystem storage
227+    const fs::path m_path;
228+
229+    //! whether or not the database resides in memory
230+    bool m_is_memory;
231+

maflcko commented at 2:37 pm on September 13, 2022:

nit in 75bb0c54b383e434695bf6bc1d60ef5bcd83cd70:

Can combine both into

0    //! path to filesystem storage or std::nullopt if database resides in memory
1    std::optional<fs::path> m_path;

jamesob commented at 5:36 pm on September 13, 2022:

I think @ryanofsky brought this up earlier; while I agree it would be a good change, it will broaden the scope of this PR quite a bit because all uses of CDBWrapper will have to change, since path is currently a required argument. PR is already large, so this could be a follow-up.

in src/node/utxo_snapshot.cpp:28 in d59db4ff4b outdated

23+    const std::optional<fs::path> chaindir = snapshot_chainstate.CoinsDB().StoragePath();
24+    assert(chaindir); // Sanity check that chainstate isn't in-memory.
25+    const fs::path write_to = *chaindir / node::SNAPSHOT_BLOCKHASH_FILENAME;
26+
27+    FILE* file{fsbridge::fopen(write_to, "wb")};
28+    CAutoFile afile{file, SER_DISK, CLIENT_VERSION};

maflcko commented at 2:45 pm on September 13, 2022:

nit in d59db4ff4b585d2b86b68ea0db206ec1c0cc5022:

 0diff --git a/src/node/utxo_snapshot.cpp b/src/node/utxo_snapshot.cpp
 1index 9c9947df95..2c5885198b 100644
 2--- a/src/node/utxo_snapshot.cpp
 3+++ b/src/node/utxo_snapshot.cpp
 4@@ -25,7 +25,7 @@ bool WriteSnapshotBaseBlockhash(CChainState& snapshot_chainstate)
 5     const fs::path write_to = *chaindir / node::SNAPSHOT_BLOCKHASH_FILENAME;
 6 
 7     FILE* file{fsbridge::fopen(write_to, "wb")};
 8-    CAutoFile afile{file, SER_DISK, CLIENT_VERSION};
 9+    AutoFile afile{file};
10     if (afile.IsNull()) {
11         LogPrintf("[snapshot] failed to open base blockhash file for writing: %s\n",
12                   fs::PathToString(write_to));
13@@ -60,7 +60,7 @@ std::optional<uint256> ReadSnapshotBaseBlockhash(fs::path chaindir)
14 
15     uint256 base_blockhash;
16     FILE* file{fsbridge::fopen(read_from, "rb")};
17-    CAutoFile afile{file, SER_DISK, CLIENT_VERSION};
18+    AutoFile afile{file};
19     if (afile.IsNull()) {
20         LogPrintf("[snapshot] failed to open base blockhash file for reading: %s\n",
21             read_from_str);

ariard commented at 0:24 am on September 16, 2022:

I think it could be valuable to document CAutoFile in streams.h, there is no document to explain the diff compared to AutoFile. From my understanding, it’s a wrapper to handle file pointer, without having to bother with serialization type and versioning.

in src/node/utxo_snapshot.cpp:26 in d59db4ff4b outdated

20+    AssertLockHeld(::cs_main);
21+    assert(snapshot_chainstate.m_from_snapshot_blockhash);
22+
23+    const std::optional<fs::path> chaindir = snapshot_chainstate.CoinsDB().StoragePath();
24+    assert(chaindir); // Sanity check that chainstate isn't in-memory.
25+    const fs::path write_to = *chaindir / node::SNAPSHOT_BLOCKHASH_FILENAME;

maflcko commented at 2:47 pm on September 13, 2022:

question in https://github.com/bitcoin/bitcoin/commit/d59db4ff4b585d2b86b68ea0db206ec1c0cc5022: why can’t the blockhash be read from the metadata in the dump?

jamesob commented at 4:31 pm on September 13, 2022:

By “dump” do you mean the UTXO snapshot? If so, I don’t think we can expect users to keep the snapshot on-hand after they’ve loaded it; it’s essentially unnecessary at that point, and multiple GBs.

maflcko commented at 4:39 pm on September 13, 2022:

Thanks, makes sense.

in src/test/util/chainstate.h:79 in 1430518eec outdated

75+            LOCK(::cs_main);
76+            uint256 gen_hash = node.chainman->ActiveChainstate().m_chain[0]->GetBlockHash();
77+            node.chainman->ResetChainstates();
78+            node.chainman->InitializeChainstate(node.mempool.get());
79+            CChainState& chain = node.chainman->ActiveChainstate();
80+            BOOST_CHECK(chain.LoadGenesisBlock());

maflcko commented at 2:50 pm on September 13, 2022:

0            Assert(chain.LoadGenesisBlock());

might be better to not continue execution when this fails?

in src/test/util/chainstate.h:92 in 1430518eec outdated

88+        }
89+        BlockValidationState state;
90+        if (!node.chainman->ActiveChainstate().ActivateBestChain(state)) {
91+            throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
92+        }
93+        BOOST_CHECK_EQUAL(

maflcko commented at 2:51 pm on September 13, 2022:

0        Assert(

maflcko commented at 2:51 pm on September 13, 2022: member

left a question. Also, this no longer compiles because boost was removed from libtest_util

DrahtBot added the label Needs rebase on Sep 13, 2022

db: add StoragePath to CDBWrapper/CCoinsViewDB

This is used in subsequent commits. It allows us to clean up UTXO
snapshot chainstate after background validation completes.

d14bebf100

validation: rename snapshot chainstate dir

This changes the snapshot's leveldb chainstate dir name from
`chainstate_[blockhash]` to `chainstate_snapshot`. This simplifies
later logic that loads snapshot data, and enforces the limitation
of a single snapshot at any given time.

Since we still need to persis the blockhash of the base block, we
write that out to a file (`chainstate_snapshot/base_blockhash`) for
later use during initialization, so that we can reinitialize the
snapshot chainstate.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

f9f1735f13

init: add utxo snapshot detection

Add functionality for activating a snapshot-based chainstate if one is
detected on-disk.

Also cautiously initialize chainstate cache usages so that we don't
somehow blow past our cache allowances during initialization, then
rebalance at the end of init.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

252abd1e8b

add utilities for deleting on-disk leveldb data

Used in later commits to remove leveldb directories for
- invalid snapshot chainstates, and
- background-vaildation chainstates that have finished serving their
  purpose.

34d1590331

validation: remove snapshot datadirs upon validation failure

If a UTXO snapshot fails to validate, don't leave the resulting datadir
on disk as this will confuse initialization on next startup and we'll
get an assertion error.

ad67ff377c

blockmanager: avoid undefined behavior during FlushBlockFile

If we call FlushBlockFile() without having intitialized the block index
with LoadBlockIndexDB(), we may be indexing into an empty vector.

Specifically this is an issue when we call MaybeRebalanceCaches() during
chainstate init before the block index has been loaded, which calls
FlushBlockFile().

Also add an assert to avoid undefined behavior.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

8153bd9247

move-only: test: make snapshot chainstate setup reusable

For use in next commit.

Most easily reviewed with
`--color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change`.

3a29dfbfb2

validation: add ResetChainstates()

Necessary for the following test commit.

00b357c215

test: add reset_chainstate parameter for snapshot unittests

This CreateAndActivateUTXOSnapshot parameter is necessary once we
perform snapshot completion within ABC, since the existing UpdateTip
test will fail because the IBD chain that has generated the snapshot
will exceed the base of the snapshot.

Being able to test snapshots being loaded into a mostly-uninitialized
datadir allows for more realistic unittest scenarios.

3c361391b8

test: allow on-disk coins and block tree dbs in tests

Used when testing cleanup of on-disk chainstate data for snapshot
testcases. Also necessary for simulating node restart in .cpp tests.

51fc9241c0

test: move-only-ish: factor out LoadVerifyActivateChainstate()

in TestingSetup(). This is used in the following commit to test
reinitializing chainstates after snapshot validation and cleanup.

Best reviewed with `git diff --color-moved=dimmed-zebra`.

cced4e7336

test: add testcases for snapshot initialization e4d7995286

doc: add note about snapshot chainstate init bf95976061

jamesob force-pushed on Sep 13, 2022

jamesob commented at 5:36 pm on September 13, 2022: member

Thanks for the feedback @MarcoFalke, all addressed.

DrahtBot removed the label Needs rebase on Sep 13, 2022

naumenkogs commented at 7:39 am on September 14, 2022: member

utACK bf9597606166323158bbf631137b82d41f39334f

ariard approved

ariard commented at 0:42 am on September 16, 2022: member

Code Review ACK bf9597606

Changes since last ACK:

CChainState -> Chainstate, after #24513
replacing CAutoFile usage in utxo_snapshot.cpp by AutoFile
commenting SNAPSHOT_BLOCKHASH_FILENAME
a new informative log L116 in src/node/chainstate.cpp
replacing InitializeChainstate by ActivateExistingSnapshot in tests
dropping the optional snapshot_blockash argument from InitializeChainstate and adding asserts
commenting ActivateExistingSnapshot
comment, lock simplification and minor code changes in unit tests

ryanofsky approved

ryanofsky commented at 5:36 pm on September 21, 2022: contributor

Code review ACK bf9597606166323158bbf631137b82d41f39334f. Changes since last review: rebasing, switching from CAutoFile to AutoFile, adding comments, switching from BOOST_CHECK to Assert in test util, using chainman.GetMutex() in tests, destroying one ChainstateManager before creating a new one in tests

in doc/design/assumeutxo.md:79 in bf95976061

75@@ -76,8 +76,9 @@ original chainstate remains in use as active.
76 
77 Once the snapshot chainstate is loaded and validated, it is promoted to active
78 chainstate and a sync to tip begins. A new chainstate directory is created in the
79-datadir for the snapshot chainstate called
80-`chainstate_[SHA256 blockhash of snapshot base block]`.
81+datadir for the snapshot chainstate called `chainstate_snapshot`. When this directory

fjahr commented at 6:23 pm on September 25, 2022:

line 117 in this file still mentions chainstate_[hash]

fjahr commented at 8:39 pm on September 25, 2022: contributor

utACK bf9597606166323158bbf631137b82d41f39334f

jamesob commented at 5:40 pm on October 4, 2022: member

I’m counting 4 ACKs here - what more is needed to merge?

in src/dbwrapper.h:284 in d14bebf100 outdated

277@@ -268,6 +278,14 @@ class CDBWrapper
278         return WriteBatch(batch, fSync);
279     }
280 
281+    //! @returns filesystem path to the on-disk data.
282+    std::optional<fs::path> StoragePath() {
283+        if (m_is_memory) {
284+            return {};

aureleoules commented at 10:41 am on October 5, 2022:

d14bebf100aaaa25c7558eeed8b5c536da99885f also missing #include <optional>

0            return std::nullopt;

in src/node/blockstorage.cpp:528 in 8153bd9247 outdated

523@@ -524,6 +524,16 @@ void BlockManager::FlushUndoFile(int block_file, bool finalize)
524 void BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo)
525 {
526     LOCK(cs_LastBlockFile);
527+
528+    if (m_blockfile_info.size() < 1) {

aureleoules commented at 11:37 am on October 5, 2022:

8153bd9247dad3982d54488bcdb3960470315290 nit: easier to read and guaranteed constant time

0    if (m_blockfile_info.empty()) {

in src/node/utxo_snapshot.cpp:45 in bf95976061

40+        return false;
41+    }
42+    return true;
43+}
44+
45+std::optional<uint256> ReadSnapshotBaseBlockhash(fs::path chaindir)

aureleoules commented at 11:47 am on October 5, 2022:

f9f1735f139b6a1f1c7fea50717ff90dc4ba2bce nit

0std::optional<uint256> ReadSnapshotBaseBlockhash(const fs::path& chaindir)

in src/test/util/setup_common.cpp:282 in bf95976061

279+TestChain100Setup::TestChain100Setup(
280+        const std::string& chain_name,
281+        const std::vector<const char*>& extra_args,
282+        const bool coins_db_in_memory,
283+        const bool block_tree_db_in_memory)
284+    : TestingSetup{CBaseChainParams::REGTEST, extra_args, coins_db_in_memory, block_tree_db_in_memory}

aureleoules commented at 11:50 am on October 5, 2022:

51fc9241c08a00f1f407f1534853a5cddbbc0a23 I believe it should be like this?

0    : TestingSetup{chain_name, extra_args, coins_db_in_memory, block_tree_db_in_memory}

in src/test/validation_chainstatemanager_tests.cpp:519 in bf95976061

514+        LOCK(chainman_restarted.GetMutex());
515+        BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 220);
516+
517+        // Background chainstate should be unaware of new blocks on the snapshot
518+        // chainstate.
519+        for (Chainstate* cs : chainman_restarted.GetAll()) {

aureleoules commented at 11:55 am on October 5, 2022:

e4d799528696c5ede38c257afaffd367917e0de8 nit

0        for (const Chainstate* cs : chainman_restarted.GetAll()) {

in src/validation.cpp:4770 in bf95976061

4766@@ -4780,6 +4767,46 @@ const AssumeutxoData* ExpectedAssumeutxo(
4767     return nullptr;
4768 }
4769 
4770+static bool DeleteCoinsDBFromDisk(const fs::path db_path, bool is_snapshot)

aureleoules commented at 11:56 am on October 5, 2022:

34d159033106cc595cfa852695610bfe419c989c nit

0static bool DeleteCoinsDBFromDisk(const fs::path& db_path, bool is_snapshot)

aureleoules commented at 1:04 pm on October 5, 2022: member

LGTM, I also verified that all commits compile and pass unit tests. Left some minor/nit comments.

jamesob commented at 7:30 pm on October 7, 2022: member

I’m going to leave the nits as-is to avoid ACK churn unless maints feel they are blocking merge. I’ll be very quick to review any followups that @aureleoules or @fjahr would like to do towards addressing them.

aureleoules approved

aureleoules commented at 8:30 am on October 10, 2022: member

ACK bf9597606166323158bbf631137b82d41f39334f

achow101 merged this on Oct 13, 2022

achow101 closed this on Oct 13, 2022

sidhujag referenced this in commit 6a24659c52 on Oct 23, 2022

in src/validation.cpp:4781 in 34d1590331 outdated

4776+        fs::path base_blockhash_path = db_path / node::SNAPSHOT_BLOCKHASH_FILENAME;
4777+
4778+        if (fs::exists(base_blockhash_path)) {
4779+            bool removed = fs::remove(base_blockhash_path);
4780+            if (!removed) {
4781+                LogPrintf("[snapshot] failed to remove file %s\n",

andrewtoth commented at 8:43 pm on December 23, 2022:

I don’t think this code is the intended behavior. This is redundant to the previous check 3 lines up. If fs::remove without a second parameter returns false it is only because the file does not exist. If it actually fails to remove an existing file it will throw an exception (see https://en.cppreference.com/w/cpp/filesystem/remove).

If intended behavior is to only log error and continue fs::remove should be wrapped in a try/catch block or passed a std::error_code.

See https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L326-L332.

Fabcien referenced this in commit dabda24d19 on Oct 26, 2023

Fabcien referenced this in commit 728d8df239 on Oct 26, 2023

Fabcien referenced this in commit 88394e92b4 on Oct 26, 2023

Fabcien referenced this in commit 26b3b6bfd4 on Oct 26, 2023

Fabcien referenced this in commit d7c38f7de5 on Oct 26, 2023

Fabcien referenced this in commit 209c477a7c on Oct 26, 2023

Fabcien referenced this in commit f58299dfff on Oct 26, 2023

bitcoin locked this on Dec 23, 2023

assumeutxo: snapshot initialization #25667

Conflicts