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 detailed information about the code coverage, see the test coverage report.
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.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
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)
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.