kernel: Move block tree db open to block manager #30965

pull TheCharlatan wants to merge 1 commits into bitcoin:master from TheCharlatan:blockmanDB changing 14 files +61 −48
  1. TheCharlatan commented at 9:03 pm on September 24, 2024: contributor

    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.

  2. DrahtBot commented at 9:03 pm on September 24, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK josibake
    Concept ACK mzumsande

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31046 (init: Some small chainstate load improvements by TheCharlatan)
    • #30967 (refactor: Replace g_genesis_wait_cv with m_tip_block_cv by maflcko)

    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.

  3. DrahtBot added the label Validation on Sep 24, 2024
  4. in src/bitcoin-chainstate.cpp:127 in 40aa407a64 outdated
    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,
    


    josibake commented at 8:20 am on September 26, 2024:
    I’m guessing you moved the values up from below (L109-L112) so that you could set the value once and use 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.

    josibake commented at 9:45 am on October 7, 2024:

    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?

  5. in src/init.cpp:1541 in 40aa407a64 outdated
    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
    


    josibake commented at 8:29 am on September 26, 2024:
    Note to self (and other reviewers): this move wasn’t immediately obvious to me, until I noticed 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.
  6. in src/init.cpp:1571 in 40aa407a64 outdated
    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."));
    


    josibake commented at 8:35 am on September 26, 2024:
    Might be premature at this stage, but in theory we could be opening both the block database and txdb at this stage. I haven’t looked at 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?

    mzumsande commented at 8:35 pm on September 26, 2024:

    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.


    josibake commented at 1:20 pm on September 27, 2024:

    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!


    TheCharlatan commented at 8:24 am on October 7, 2024:
    Heh, we currently always inform the user with “Error opening block database.” even if there is an error with the chainstate. I think I am going to leave this as is here, so not to scope creep this PR and will address this in another PR.

    TheCharlatan commented at 8:44 am on October 7, 2024:
    Ugh, I should have drafted this PR once I opened #30968 , since I opened it after being fed up with the init logic in this change. The error handling introduced there forces you to do the right thing in this change now. Sorry for the churn!

    TheCharlatan commented at 1:12 pm on October 7, 2024:
    See #31046
  7. in src/kernel/blockmanager_opts.h:35 in 40aa407a64 outdated
    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};
    


    josibake commented at 8:38 am on September 26, 2024:
    Assuming this is just a comment explaining what was already happening and documenting a behaviour change introduced in this PR? (note to self to test this).

    josibake commented at 8:48 am on September 26, 2024:
    Nevermind, I can see this a move only from src/chainstate.h
  8. in src/kernel/chainstatemanager_opts.h:45 in 40aa407a64 outdated
    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{};
    


    josibake commented at 8:40 am on September 26, 2024:
    Nice! Makes a lot more sense to me to have block tree database options in the BlockManagerOptions
  9. in src/node/chainstate.cpp:43 in 40aa407a64 outdated
    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();
    


    josibake commented at 8:47 am on September 26, 2024:
    This code feels a bit smelly, so nice to see you are dropping it by moving the database initialisation into the BlockManager. Will need to look at the old code more closely to understand what “previous loop” this comment is referring to.

    TheCharlatan commented at 4:43 pm on September 26, 2024:
    The previous loop this refers to is the initialization retry loop that I am getting rid of in #30968.
  10. josibake commented at 8:53 am on September 26, 2024: member

    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.

  11. mzumsande commented at 8:36 pm on September 26, 2024: contributor
    Concept ACK
  12. DrahtBot added the label Needs rebase on Sep 30, 2024
  13. TheCharlatan force-pushed on Oct 7, 2024
  14. TheCharlatan commented at 8:44 am on October 7, 2024: contributor

    Rebased 63dfc6067bd7fe06e5a440c229cca030bba6d53a -> 63dfc6067bd7fe06e5a440c229cca030bba6d53a (blockmanDB -> blockmanDB, compare)

  15. DrahtBot removed the label Needs rebase on Oct 7, 2024
  16. kernel: Move block tree db open to block manager
    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.
    60ade81516
  17. TheCharlatan force-pushed on Oct 7, 2024
  18. TheCharlatan commented at 10:21 am on October 7, 2024: contributor

    Updated 63dfc6067bd7fe06e5a440c229cca030bba6d53a -> 60ade81516d42d275c1143f49f47e142d32c45fc (blockmanDB_1 -> blockmanDB_2, compare)

    • Addressed @josibake’s comment, reusing a option value in bitcoin-chainstate
    • Exchanged use of Untranslated with _, since the string in question is displayed to GUI users.
  19. josibake commented at 11:02 am on October 7, 2024: member

    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!

  20. DrahtBot requested review from mzumsande on Oct 7, 2024
  21. mzumsande commented at 8:39 pm on October 7, 2024: contributor

    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?

  22. DrahtBot requested review from mzumsande on Oct 7, 2024
  23. TheCharlatan commented at 9:22 pm on October 7, 2024: contributor

    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.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me