Before this change the block tree db was needlessly re-opened during startup when loading a completed snapshot. Improve this by letting the block manager open it on construction. This also simplifies the test code a bit.
The change was initially motivated to make it easier for users of the kernel library to instantiate a BlockManager that may be used to read data from disk without loading the block index into a cache.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30965.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
122@@ -119,14 +123,12 @@ int main(int argc, char* argv[])
123 .chainparams = chainman_opts.chainparams,
124 .blocks_dir = abs_datadir / "blocks",
125 .notifications = chainman_opts.notifications,
126+ .block_tree_db_dir = abs_datadir / "blocks" / "index",
127+ .block_tree_db_cache_size = 2 << 20,
cache_sizes.block_tree_db to set block_tree_db_cache_size on L127? Otherwise it’s not clear to me why those values were moved.
Sorry, I don’t think my last comment on this was very clear. Lines L109-L112 are moved up from below, and cache_sizes.block_tree_db is set. So I think L127 should be:
0 .block_tree_db_cache_size = cache_sizes.block_tree_db,
unless I’m missing something?
1536+ .blocks_dir = args.GetBlocksDirPath(),
1537+ .notifications = chainman_opts.notifications,
1538+ .block_tree_db_dir = args.GetDataDirNet() / "blocks" / "index",
1539+ .block_tree_db_cache_size = cache_sizes.block_tree_db,
1540+ };
1541+ Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction
CacheSizes is set on L1521, and then the value cache_sizes.block_tree_db is used when creating the BlockManager::Options. As a nice side effect, it seems better to have the logs for cache configuration first, and then proceed with setting and creating the BlockManager.
1562@@ -1559,8 +1563,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1563 }
1564 LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024));
1565
1566+ blockman_opts.wipe_block_tree_db = do_reindex;
1567+
1568 try {
1569 node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown), chainman_opts, blockman_opts);
1570+ } catch (dbwrapper_error&) {
1571+ return InitError(Untranslated("Error opening block database."));
dbwrapper_error yet so not sure if this is possible, but perhaps we could bubble up the actual db name that failed to open instead of assuming it’s always the block database?
This changes the error handling if the block tree db is corrupted as a side effect. Before, we’d get the advice “Please restart with -reindex or -reindex-chainstate to recover.” on the console, and, if using the GUI, the option to reindex by clicking a button. Now we just get an “Error opening block database” error on both without any advice how to fix it.
I’m not sure if the GUI one-click reindex option is totally essential, but in my opinion we should at least keep the advice to reindex, and also show it in the GUI.
I think this error handling makes more sense post #30968.
I rebased this commit on top of #30968 and was able to get the same error handling behavior as before: https://github.com/josibake/bitcoin/commit/85895b870d669175f42d0730d58930bea169e221, was quite happy with the result!
27@@ -27,6 +28,14 @@ struct BlockManagerOpts {
28 bool fast_prune{false};
29 const fs::path blocks_dir;
30 Notifications& notifications;
31+ const fs::path block_tree_db_dir;
32+ // Whether to wipe the block tree database when loading it. If set, this
33+ // will also set a reindexing flag so any existing block data files will be
34+ // scanned and added to the database.
35+ bool wipe_block_tree_db{false};
src/chainstate.h
41@@ -42,7 +42,6 @@ struct ChainstateManagerOpts {
42 std::optional<uint256> assumed_valid_block{};
43 //! If the tip is older than this, the node is considered to be in initial block download.
44 std::chrono::seconds max_tip_age{DEFAULT_MAX_TIP_AGE};
45- DBOptions block_tree_db{};
BlockManagerOptions
36@@ -37,26 +37,6 @@ static ChainstateLoadResult CompleteChainstateInitialization(
37 const CacheSizes& cache_sizes,
38 const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
39 {
40- auto& pblocktree{chainman.m_blockman.m_block_tree_db};
41- // new BlockTreeDB tries to delete the existing file, which
42- // fails if it's still open from the previous loop. Close it first:
43- pblocktree.reset();
BlockManager. Will need to look at the old code more closely to understand what “previous loop” this comment is referring to.
Concept ACK
Conceptually, this makes a lot more sense and makes the code much easier to understand, IMO. Left some comments while doing an initial pass, more questions than concrete suggestions. I did find it a bit hard to parse what was going with a lot of the code being move-only, but from across different files. Not sure if much can be done here, but it would make this easier to follow if you could separate as much of the move-only code changes into their own commit(s)? Specifically, I’m thinking of two instances where the option setting code is moved up a few lines so the cache size option can be passed through.
Rebased 63dfc6067bd7fe06e5a440c229cca030bba6d53a -> 63dfc6067bd7fe06e5a440c229cca030bba6d53a (blockmanDB -> blockmanDB, compare)
Updated 63dfc6067bd7fe06e5a440c229cca030bba6d53a -> 60ade81516d42d275c1143f49f47e142d32c45fc (blockmanDB_1 -> blockmanDB_2, compare)
crACK https://github.com/bitcoin/bitcoin/commit/60ade81516d42d275c1143f49f47e142d32c45fc
Verified that the change is mostly move-only. Makes the code much more clear and cleanly separates initialising / opening the database from loading the database contents into the in memory cache.
The change was initially motivated to make it easier for users of the kernel library to instantiate a BlockManager that may be used to read data from disk without loading the block index into a cache.
Will test this and report back if I find anything unexpected!
Conceptual question:
I think that just the goal of not loading the blocktreedb twice could have been achieved easier by simply moving the code from CompleteChainstateInitialization into LoadChainstate().
You also list other motivations, but I don’t completely get these:
It seems a little bit non-intuitive to me to have things like Cleanup of blk / rev files (CleanupBlockRevFiles) done in the constructor, instead of having a separate call for this.
It also doesn’t feel consistent if the blocktreedb is loaded automatically when creating a ChainstateManager, but the coinsb of the chainstates aren’t - I would have expected both or neither.
So can you explain a bit more in detail how this helps kernel?
Re #30965#pullrequestreview-2352892509
Thank you for the questions @mzumsande!
So can you explain a bit more in detail how this helps kernel?
I will try to give a thought-dump of what I am attempting to achieve here. As said in the title description, the kernel-motivated goal of this PR is to allow users of the library to instantiate the block tree db in the block manager in the same way we would currently in the chainstate load routines without having to load everything into the cache or having to load a chainstate. This allows a potential user to immediately use the block tree db, for example to recurse through it and read ordered blocks from disk.
As an alternative to the approach here, opening the database could happen in a separate routine, outside of the BlockManager constructor, but from my current mental model, this would then require our current code to either check if this routine was called before, or force the user to close it again before doing other tasks. I would consider this a less elegant solution.
It seems a little bit non-intuitive to me to have things like Cleanup of blk / rev files (CleanupBlockRevFiles) done in the constructor, instead of having a separate call for this.
My thinking here is that since this is always the correct thing to do if the corresponding options are set, having a separate call is not really useful. If certain options are set and I instantiate a BlockManager I would expect it to be in the state I configured it with, without having the call additional functions.
It also doesn’t feel consistent if the blocktreedb is loaded automatically when creating a ChainstateManager, but the coinsb of the chainstates aren’t - I would have expected both or neither.
Yes, my eventual goal was to move calling InitializeChainstate for the fully validated chainstate into the ChainstateManager constructor as well as calling InitCoinsDB for it. I wanted to do this in a future pull request if this change is accepted.
Rebased 60ade81516d42d275c1143f49f47e142d32c45fc -> 1c1df66cebbe825a24cb2f66a24aab78709db7db (blockmanDB_2 -> blockmanDB_3, compare)
1209+ .wipe_block_tree_db = do_reindex,
1210+ .block_tree_db_cache_size = cache_sizes.block_tree_db,
1211 };
1212 Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction
1213 try {
1214 node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown_signal), chainman_opts, blockman_opts);
36@@ -37,33 +37,6 @@ static ChainstateLoadResult CompleteChainstateInitialization(
37 const CacheSizes& cache_sizes,
cache_sizes is no longer used and can be removed from CompleteChainstateInitialization()
Thanks - your explanation makes sense - I did a bit of research on how much logic and I/O in constructors are considered to be “good style”, and there doesn’t really seem to be a clear consensus to me.
For me the main downside would be that some of the things the ctor does may be unexpected, hence below suggestion to add some documentation.
Thank you for the review @mzumsande,
Updated f55d36a19c61e0ec8c06a6e5a0bbae455c8a1d06 -> 182945e946ac6a92daad012d6579e5441b9641cc (blockmanDB_4 -> blockmanDB_5, compare)
cache_sizes param in CompleteChainstateInitialization.
Re #30965#pullrequestreview-2521245377
I did a bit of research on how much logic and I/O in constructors are considered to be “good style”, and there doesn’t really seem to be a clear consensus to me.
I wish there would be better consensus on what the best practices are. If somebody has reasoning for doing things differently, do share!
60- if (options.prune) {
61- chainman.m_blockman.CleanupBlockRevFiles();
62- }
63- }
64-
65- if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
In commit “kernel: Move block tree db open to block manager” (182945e946ac6a92daad012d6579e5441b9641cc)
I don’t think this line should be dropped. It seems like potentially slow loading steps do happen before this CompleteChainstateInitialization call, so it’s good to check if a shutdown was requested before proceeding.
123@@ -152,7 +124,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
124 }
125 }
126
127- if (!options.wipe_block_tree_db) {
128+ if (!chainman.m_blockman.m_opts.wipe_block_tree_db) {
In commit “kernel: Move block tree db open to block manager” (182945e946ac6a92daad012d6579e5441b9641cc)
I don’t actually see why this check (introduced in #21009) is necessary, and think it would be better to drop. Looking at NeedsRedownload() definition it seems like it will already return false if chainstates are empty, so it should be ok to skip.
If this check can be dropped it looks like a number of other simplifications are possible: BlockManager::m_opts would not need to be public, so its declaration would not have to move and m_block_file_seq / m_undo_file_seq initializations would not need to change because m_opts would not be moving.
I agree that it should be possible to drop the check.
This is a off-topic here, but maybe the entire NeedsRedownload() check could be removed at some point, or simplified to just check block one after segwit, instead over iterating over 400k indexes each startup? Segwit activation is now almost 8 years in the past after all…
33+ // will also set a reindexing flag so any existing block data files will be
34+ // scanned and added to the database.
35+ bool wipe_block_tree_db{false};
36+ bool block_tree_db_in_memory{false};
37+ DBOptions block_tree_db_options{};
38+ int64_t block_tree_db_cache_size;
In commit “kernel: Move block tree db open to block manager” (182945e946ac6a92daad012d6579e5441b9641cc)
All of these new fields are just duplicating the fields of the existing DBParams struct, except for adding a block_tree to the name. Would be nice to replace them all with a single DBParams block_db_params field.
122@@ -119,14 +123,12 @@ int main(int argc, char* argv[])
123 .chainparams = chainman_opts.chainparams,
124 .blocks_dir = abs_datadir / "blocks",
125 .notifications = chainman_opts.notifications,
126+ .block_tree_db_dir = abs_datadir / "blocks" / "index",
127+ .block_tree_db_cache_size = cache_sizes.block_tree_db,
In commit “kernel: Move block tree db open to block manager” (182945e946ac6a92daad012d6579e5441b9641cc)
It seems far from ideal for bitcoin-chainstate and other kernel library users (not to mention our own test code) to need to hardcode the “blocks/index” path and to manually copy the cache size from one struct to another. I think a better API would allow these option fields to be unset and internally use a Flatten method similar to the ones in validation.cpp and txmempool.cpp to set appropriate default values when the fields are unset.
DBParams field an optional, so it’s not easily possible to set only the cache size, but not the path. It is also annoying that the current behaviour with the path is that it is dependent on the data dir path, not the blocks dir path.
Code review ACK 182945e946ac6a92daad012d6579e5441b9641cc. I think a number of improvements are possible here (see comments below) but it does make sense to move block database initialization out of src/node/chainstate.cpp and into the BlockManager class, and move its options out of ChainstateManager::Options and into BlockManager::Options.
On constructor initialization question I do think it’s often useful if lightweight construction with delayed initialization or lazy initialization is possible, but it should be very easy to support these things if needed by just splitting the constructor up and overloading it.
Updated 182945e946ac6a92daad012d6579e5441b9641cc -> 384c73d4fcda4af7c2b4bfb945a248cb93dba47d (blockmanDB_5 -> blockmanDB_6, compare)
1220+ // The coinsdb is opened at a later point on LoadChainstate.
1221 try {
1222 node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown_signal), chainman_opts, blockman_opts);
1223+ } catch (dbwrapper_error& e) {
1224+ LogError("%s\n", e.what());
1225+ return {ChainstateLoadStatus::FAILURE, _("Error opening block database")};
In commit “refactor: Remove redundant reindex check” (fd590d5ca1cfb6c2525b7755edb6a3a2cf16c856)
Curious about something: In #30965 (comment) you wrote,
Yes, my eventual goal was to move calling
InitializeChainstatefor the fully validated chainstate into theChainstateManagerconstructor as well as callingInitCoinsDBfor it. I wanted to do this in a future pull request if this change is accepted.
So if coins db were intialized in the chainstate constructor, then this dbwrapper_error exception could mean there is an error either from the coins db or the blocks db, not just the blocks db as stated in the current error message.
For this reason, and in any case, it seems fragile for this code to have to know what ChainstateManager constructor is doing internally and what exceptions that code might throw. I think it might be better if instead of letting dbwrapper_error be thrown from the ChainstateManager constructor, code in that constructor or code in the BlockStorage constructor would catch that exception and throw a more generic exception that this code could handle without needing to know about ChainstateManager internals.
Maybe a good candidate for a generic exception type could be the existing ChainstateLoadResult type since it can hold an error message and also indicate whether the error is fatal or not (i.e. whether a reindex should be attempted or not).
Good thinking ahead! Yeah, my idea was to use something along those lines too. Maybe we could start annotating some of the functions here to make it easier for the developer to see which exceptions it throws? Though my understanding is that such annotations have been deliberately left out of the code so far (https://stackoverflow.com/questions/1055387/throw-keyword-in-functions-signature).
On the other hand it would be nice if we could agree on more coherent error handling for constructors, be it with factory functions, error out params, or exceptions. I am starting to like exceptions for these cases though, and combining them with ChainstateLoadResult would make responding to them fairly straight forward in our init code.
61- chainman.m_blockman.CleanupBlockRevFiles();
62- }
63- }
64-
65- if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
66+ if (chainman.m_interrupt) return {ChainstateLoadStatus::INTERRUPTED, {}};
In commit “kernel: Move block tree db open to block manager” (384c73d4fcda4af7c2b4bfb945a248cb93dba47d)
Looks like indentation unintentionally changed here
m_block_tree_db_in_memory / opts.block_tree_db_in_memory in tests, and seem more correct now.
Updated 384c73d4fcda4af7c2b4bfb945a248cb93dba47d -> b776e40554c8a315d832f3996d14ff2e3ae7b8cb (blockmanDB_6 -> blockmanDB_7, compare)
The check for whether the block tree db has been wiped before calling
NeedsRedownload() is confusing. The boolean is set in case of a reindex.
It was originally introduced to guard NeedsRedownload in case of a
reindex in #21009. However NeedsRedownload already returns early if the
chain's tip is not loaded. Since that is the case during a reindex, the
pre-check is redundant.
Rebased b776e40554c8a315d832f3996d14ff2e3ae7b8cb -> 7f484b319bdc9e646e270b6d6d9a1c7e52296dc0 (blockmanDB_7 -> blockmanDB_8, compare)
1208 .chainparams = chainman_opts.chainparams,
1209 .blocks_dir = args.GetBlocksDirPath(),
1210 .notifications = chainman_opts.notifications,
1211+ .block_tree_db_params = DBParams{
1212+ .path = args.GetDataDirNet() / "blocks" / "index",
1213+ .cache_bytes = static_cast<size_t>(cache_sizes.block_tree_db),
Updated 7f484b319bdc9e646e270b6d6d9a1c7e52296dc0 -> f3cfdf2b511776211a0399510ced735e556d510d (blockmanDB_8 -> blockmanDB_9, compare)
255+ .memory_only = opts.block_tree_db_in_memory,
256+ .wipe_data = m_args.GetBoolArg("-reindex", false),
257+ },
258 };
259 m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown_signal), chainman_opts, blockman_opts);
260 LOCK(m_node.chainman->GetMutex());
m_blockman is supposed to be under the chainman mutex?), but this is now a no-op, as the scope ends in the next line.
1220+ // the blocks tree db, and wipes existing block files in case of a reindex.
1221+ // The coinsdb is opened at a later point on LoadChainstate.
1222 try {
1223 node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown_signal), chainman_opts, blockman_opts);
1224+ } catch (dbwrapper_error& e) {
1225+ LogError("%s\n", e.what());
0 LogError("%s", e.what());
style-nit: Newline not needed. Alternatively, if you want to keep it move-only, I’d suggest to keep the name of err as err, instead of e.
Personally, I found this a bit easier to review by having the stuff that simply moves the fields from one struct to another in one commit and the remaining changes in another commit. This means the second commit would contain the following diff (and the third commit would contain whatever remains to restore an empty diff to the HEAD of this pull):
0diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
1index 51c482607a..e657f01871 100644
2--- a/src/bitcoin-chainstate.cpp
3+++ b/src/bitcoin-chainstate.cpp
4@@ -106,6 +106,7 @@ int main(int argc, char* argv[])
5 };
6 auto notifications = std::make_unique<KernelNotifications>();
7
8+ kernel::CacheSizes cache_sizes{DEFAULT_KERNEL_CACHE};
9
10 // SETUP: Chainstate
11 auto chainparams = CChainParams::Main();
12@@ -119,11 +120,14 @@ int main(int argc, char* argv[])
13 .chainparams = chainman_opts.chainparams,
14 .blocks_dir = abs_datadir / "blocks",
15 .notifications = chainman_opts.notifications,
16+ .block_tree_db_params = DBParams{
17+ .path = abs_datadir / "blocks" / "index",
18+ .cache_bytes = cache_sizes.block_tree_db,
19+ },
20 };
21 util::SignalInterrupt interrupt;
22 ChainstateManager chainman{interrupt, chainman_opts, blockman_opts};
23
24- kernel::CacheSizes cache_sizes{DEFAULT_KERNEL_CACHE};
25 node::ChainstateLoadOptions options;
26 auto [status, error] = node::LoadChainstate(chainman, cache_sizes, options);
27 if (status != node::ChainstateLoadStatus::SUCCESS) {
28diff --git a/src/init.cpp b/src/init.cpp
29index 116823c36f..a306449dbd 100644
30--- a/src/init.cpp
31+++ b/src/init.cpp
32@@ -1057,6 +1057,10 @@ bool AppInitParameterInteraction(const ArgsManager& args)
33 .chainparams = chainman_opts_dummy.chainparams,
34 .blocks_dir = args.GetBlocksDirPath(),
35 .notifications = chainman_opts_dummy.notifications,
36+ .block_tree_db_params = DBParams{
37+ .path = args.GetDataDirNet() / "blocks" / "index",
38+ .cache_bytes = 0,
39+ },
40 };
41 auto blockman_result{ApplyArgsManOptions(args, blockman_opts_dummy)};
42 if (!blockman_result) {
43@@ -1199,10 +1203,16 @@ static ChainstateLoadResult InitAndLoadChainstate(
44 .signals = node.validation_signals.get(),
45 };
46 Assert(ApplyArgsManOptions(args, chainman_opts)); // no error can happen, already checked in AppInitParameterInteraction
47+
48 BlockManager::Options blockman_opts{
49 .chainparams = chainman_opts.chainparams,
50 .blocks_dir = args.GetBlocksDirPath(),
51 .notifications = chainman_opts.notifications,
52+ .block_tree_db_params = DBParams{
53+ .path = args.GetDataDirNet() / "blocks" / "index",
54+ .cache_bytes = cache_sizes.block_tree_db,
55+ .wipe_data = do_reindex,
56+ },
57 };
58 Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction
59 try {
60@@ -1233,7 +1243,6 @@ static ChainstateLoadResult InitAndLoadChainstate(
61 };
62 node::ChainstateLoadOptions options;
63 options.mempool = Assert(node.mempool.get());
64- options.wipe_block_tree_db = do_reindex;
65 options.wipe_chainstate_db = do_reindex || do_reindex_chainstate;
66 options.prune = chainman.m_blockman.IsPruneMode();
67 options.check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
68diff --git a/src/kernel/blockmanager_opts.h b/src/kernel/blockmanager_opts.h
69index 261ec3be58..5c7b24d717 100644
70--- a/src/kernel/blockmanager_opts.h
71+++ b/src/kernel/blockmanager_opts.h
72@@ -5,6 +5,7 @@
73 #ifndef BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H
74 #define BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H
75
76+#include <dbwrapper.h>
77 #include <kernel/notifications_interface.h>
78 #include <util/fs.h>
79
80@@ -27,6 +28,7 @@ struct BlockManagerOpts {
81 bool fast_prune{false};
82 const fs::path blocks_dir;
83 Notifications& notifications;
84+ DBParams block_tree_db_params;
85 };
86
87 } // namespace kernel
88diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h
89index 1b605f3d55..15a8fbec61 100644
90--- a/src/kernel/chainstatemanager_opts.h
91+++ b/src/kernel/chainstatemanager_opts.h
92@@ -42,7 +42,6 @@ struct ChainstateManagerOpts {
93 std::optional<uint256> assumed_valid_block{};
94 //! If the tip is older than this, the node is considered to be in initial block download.
95 std::chrono::seconds max_tip_age{DEFAULT_MAX_TIP_AGE};
96- DBOptions block_tree_db{};
97 DBOptions coins_db{};
98 CoinsViewOptions coins_view{};
99 Notifications& notifications;
100diff --git a/src/node/blockmanager_args.cpp b/src/node/blockmanager_args.cpp
101index 0fc4e1646a..5186b65546 100644
102--- a/src/node/blockmanager_args.cpp
103+++ b/src/node/blockmanager_args.cpp
104@@ -6,6 +6,7 @@
105
106 #include <common/args.h>
107 #include <node/blockstorage.h>
108+#include <node/database_args.h>
109 #include <tinyformat.h>
110 #include <util/result.h>
111 #include <util/translation.h>
112@@ -34,6 +35,8 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Op
113
114 if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;
115
116+ ReadDatabaseArgs(args, opts.block_tree_db_params.options);
117+
118 return {};
119 }
120 } // namespace node
121diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
122index ac6db0558d..9db80c26c5 100644
123--- a/src/node/blockstorage.h
124+++ b/src/node/blockstorage.h
125@@ -268,6 +268,8 @@ public:
126 using Options = kernel::BlockManagerOpts;
127
128 explicit BlockManager(const util::SignalInterrupt& interrupt, Options opts);
129+ auto MakeBlockTreeDb() { return std::make_unique<BlockTreeDB>(m_opts.block_tree_db_params); }
130+ auto OptsWipeBlockTreeDb() { return m_opts.block_tree_db_params.wipe_data; }
131
132 const util::SignalInterrupt& m_interrupt;
133 std::atomic<bool> m_importing{false};
134diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
135index 10b6e52c78..4fc7a851cd 100644
136--- a/src/node/chainstate.cpp
137+++ b/src/node/chainstate.cpp
138@@ -36,7 +36,6 @@ namespace node {
139 // to ChainstateManager::InitializeChainstate().
140 static ChainstateLoadResult CompleteChainstateInitialization(
141 ChainstateManager& chainman,
142- const CacheSizes& cache_sizes,
143 const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
144 {
145 auto& pblocktree{chainman.m_blockman.m_block_tree_db};
146@@ -44,18 +43,13 @@ static ChainstateLoadResult CompleteChainstateInitialization(
147 // fails if it's still open from the previous loop. Close it first:
148 pblocktree.reset();
149 try {
150- pblocktree = std::make_unique<BlockTreeDB>(DBParams{
151- .path = chainman.m_options.datadir / "blocks" / "index",
152- .cache_bytes = cache_sizes.block_tree_db,
153- .memory_only = options.block_tree_db_in_memory,
154- .wipe_data = options.wipe_block_tree_db,
155- .options = chainman.m_options.block_tree_db});
156+ pblocktree = chainman.m_blockman.MakeBlockTreeDb();
157 } catch (dbwrapper_error& err) {
158 LogError("%s\n", err.what());
159 return {ChainstateLoadStatus::FAILURE, _("Error opening block database")};
160 }
161
162- if (options.wipe_block_tree_db) {
163+ if (chainman.m_blockman.OptsWipeBlockTreeDb()) {
164 pblocktree->WriteReindexing(true);
165 chainman.m_blockman.m_blockfiles_indexed = false;
166 //If we're reindexing in prune mode, wipe away unusable block files and all undo data files
167@@ -206,7 +200,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
168 }
169 }
170
171- auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options);
172+ auto [init_status, init_error] = CompleteChainstateInitialization(chainman, options);
173 if (init_status != ChainstateLoadStatus::SUCCESS) {
174 return {init_status, init_error};
175 }
176@@ -242,7 +236,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
177 // for the fully validated chainstate.
178 chainman.ActiveChainstate().ClearBlockIndexCandidates();
179
180- auto [init_status, init_error] = CompleteChainstateInitialization(chainman, cache_sizes, options);
181+ auto [init_status, init_error] = CompleteChainstateInitialization(chainman, options);
182 if (init_status != ChainstateLoadStatus::SUCCESS) {
183 return {init_status, init_error};
184 }
185diff --git a/src/node/chainstate.h b/src/node/chainstate.h
186index ce414fa043..843e8e09a6 100644
187--- a/src/node/chainstate.h
188+++ b/src/node/chainstate.h
189@@ -22,12 +22,7 @@ namespace node {
190
191 struct ChainstateLoadOptions {
192 CTxMemPool* mempool{nullptr};
193- bool block_tree_db_in_memory{false};
194 bool coins_db_in_memory{false};
195- // Whether to wipe the block tree database when loading it. If set, this
196- // will also set a reindexing flag so any existing block data files will be
197- // scanned and added to the database.
198- bool wipe_block_tree_db{false};
199 // Whether to wipe the chainstate database when loading it. If set, this
200 // will cause the chainstate database to be rebuilt starting from genesis.
201 bool wipe_chainstate_db{false};
202diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
203index 0ac96c5514..db36d03fd5 100644
204--- a/src/node/chainstatemanager_args.cpp
205+++ b/src/node/chainstatemanager_args.cpp
206@@ -49,7 +49,6 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
207
208 if (auto value{args.GetIntArg("-maxtipage")}) opts.max_tip_age = std::chrono::seconds{*value};
209
210- ReadDatabaseArgs(args, opts.block_tree_db);
211 ReadDatabaseArgs(args, opts.coins_db);
212 ReadCoinsViewArgs(args, opts.coins_view);
213
214diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
215index c2b95dd861..ec91f2b276 100644
216--- a/src/test/blockmanager_tests.cpp
217+++ b/src/test/blockmanager_tests.cpp
218@@ -33,6 +33,10 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
219 .chainparams = *params,
220 .blocks_dir = m_args.GetBlocksDirPath(),
221 .notifications = notifications,
222+ .block_tree_db_params = DBParams{
223+ .path = m_args.GetDataDirNet() / "blocks" / "index",
224+ .cache_bytes = 0,
225+ },
226 };
227 BlockManager blockman{*Assert(m_node.shutdown_signal), blockman_opts};
228 // simulate adding a genesis block normally
229@@ -140,6 +144,10 @@ BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file)
230 .chainparams = Params(),
231 .blocks_dir = m_args.GetBlocksDirPath(),
232 .notifications = notifications,
233+ .block_tree_db_params = DBParams{
234+ .path = m_args.GetDataDirNet() / "blocks" / "index",
235+ .cache_bytes = 0,
236+ },
237 };
238 BlockManager blockman{*Assert(m_node.shutdown_signal), blockman_opts};
239
240diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
241index 2cf25c4a2a..83b78d7b5a 100644
242--- a/src/test/util/setup_common.cpp
243+++ b/src/test/util/setup_common.cpp
244@@ -62,7 +62,6 @@
245 #include <stdexcept>
246
247 using namespace util::hex_literals;
248-using kernel::BlockTreeDB;
249 using node::ApplyArgsManOptions;
250 using node::BlockAssembler;
251 using node::BlockManager;
252@@ -250,14 +249,16 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts)
253 .chainparams = chainman_opts.chainparams,
254 .blocks_dir = m_args.GetBlocksDirPath(),
255 .notifications = chainman_opts.notifications,
256+ .block_tree_db_params = DBParams{
257+ .path = m_args.GetDataDirNet() / "blocks" / "index",
258+ .cache_bytes = m_kernel_cache_sizes.block_tree_db,
259+ .memory_only = opts.block_tree_db_in_memory,
260+ .wipe_data = m_args.GetBoolArg("-reindex", false),
261+ },
262 };
263 m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown_signal), chainman_opts, blockman_opts);
264 LOCK(m_node.chainman->GetMutex());
265- m_node.chainman->m_blockman.m_block_tree_db = std::make_unique<BlockTreeDB>(DBParams{
266- .path = m_args.GetDataDirNet() / "blocks" / "index",
267- .cache_bytes = m_kernel_cache_sizes.block_tree_db,
268- .memory_only = true,
269- });
270+ m_node.chainman->m_blockman.m_block_tree_db = m_node.chainman->m_blockman.MakeBlockTreeDb();
271 };
272 m_make_chainman();
273 }
274@@ -283,9 +284,7 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
275 auto& chainman{*Assert(m_node.chainman)};
276 node::ChainstateLoadOptions options;
277 options.mempool = Assert(m_node.mempool.get());
278- options.block_tree_db_in_memory = m_block_tree_db_in_memory;
279 options.coins_db_in_memory = m_coins_db_in_memory;
280- options.wipe_block_tree_db = m_args.GetBoolArg("-reindex", false);
281 options.wipe_chainstate_db = m_args.GetBoolArg("-reindex", false) || m_args.GetBoolArg("-reindex-chainstate", false);
282 options.prune = chainman.m_blockman.IsPruneMode();
283 options.check_blocks = m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS);
284diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
285index 6c2a825e64..bf440ca639 100644
286--- a/src/test/validation_chainstatemanager_tests.cpp
287+++ b/src/test/validation_chainstatemanager_tests.cpp
288@@ -393,6 +393,11 @@ struct SnapshotTestSetup : TestChain100Setup {
289 .chainparams = chainman_opts.chainparams,
290 .blocks_dir = m_args.GetBlocksDirPath(),
291 .notifications = chainman_opts.notifications,
292+ .block_tree_db_params = DBParams{
293+ .path = chainman.m_options.datadir / "blocks" / "index",
294+ .cache_bytes = m_kernel_cache_sizes.block_tree_db,
295+ .memory_only = m_block_tree_db_in_memory,
296+ },
297 };
298 // For robustness, ensure the old manager is destroyed before creating a
299 // new one.
But just my personal impression, no strong opinion.
left some style-nits (only if you re-touch)
review ACK f3cfdf2b511776211a0399510ced735e556d510d 🥃
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK f3cfdf2b511776211a0399510ced735e556d510d 🥃
3B0REdECuAQLOEWL1/F7/sHyOzZiaGn5sptzUNU7w1dEMjVCfDkqW5G60UcFwChqEIxDmcB0qfNeBXz1nC0DlDw==
This commit is done in preparation for the next commit. Here, the block
tree options are moved to the blockmanager options and the block tree is
instantiated through a helper method of the BlockManager, which is
removed again in the next commit.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Make the block db open RAII style by calling it in the BlockManager
constructor.
Before this change the block tree db was needlessly re-opened during
startup when loading a completed snapshot. Improve this by letting the
block manager open it on construction. This also simplifies the test
code a bit.
The change was initially motivated to make it easier for users of the
kernel library to instantiate a BlockManager that may be used to read
data from disk without loading the block index into a cache.
Updated f3cfdf2b511776211a0399510ced735e556d510d -> 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d (blockmanDB_9 -> blockmanDB_10, compare)
Only changes are the ones mentioned already.
Also, some moved code wasn’t covered by tests, so I went ahead and added them in #31696.
re-ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d 🏪
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d 🏪
3vZ0ZIXtCMMLTmeZAUXHGTkpkkYDgbHyBBh0s+63idosqNjx5EsxhncUD7/0s/n5It0kLJWr2C67fU8bD8B+DCQ==
