Remove almost all blockstorage globals #25781

pull maflcko wants to merge 5 commits into bitcoin:master from maflcko:2208-block-globals-🙅 changing 16 files +135 −64
  1. maflcko commented at 3:02 PM on August 4, 2022: member

    It seems preferable to assign globals to a class (in this case BlockManager), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.

  2. maflcko force-pushed on Aug 4, 2022
  3. maflcko force-pushed on Aug 4, 2022
  4. maflcko force-pushed on Aug 4, 2022
  5. maflcko renamed this:
    Remove all blockstorage globals
    Remove almost all blockstorage globals
    on Aug 4, 2022
  6. maflcko force-pushed on Aug 4, 2022
  7. maflcko marked this as ready for review on Aug 4, 2022
  8. maflcko cross-referenced this on Aug 4, 2022 from issue [kernel 3e/n] Decouple `CDBWrapper` and its kernel users from `ArgsManager` by dongcarl
  9. maflcko force-pushed on Aug 4, 2022
  10. DrahtBot commented at 4:13 AM on August 5, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan, dergoegge, achow101
    Concept ACK fanquake
    Stale ACK w0xlt

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27254 (refactor: Extract util/fs from util/system by TheCharlatan)
    • #27125 (refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan)
    • #27050 (p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode by dergoegge)
    • #25386 (refactor: Extract MIB_BYTES constant for init.cpp by Empact)
    • #25193 (indexes: Read the locator's top block during init, allow interaction with reindex-chainstate by mzumsande)
    • #15606 (assumeutxo by jamesob)

    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.

  11. DrahtBot cross-referenced this on Aug 5, 2022 from issue refactor: Use util::Result class for wallet loading by ryanofsky
  12. DrahtBot cross-referenced this on Aug 5, 2022 from issue refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky
  13. DrahtBot cross-referenced this on Aug 5, 2022 from issue refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge
  14. DrahtBot cross-referenced this on Aug 5, 2022 from issue indexes: Read the locator's top block during init, allow interaction with reindex-chainstate by mzumsande
  15. DrahtBot cross-referenced this on Aug 5, 2022 from issue incorrect blk file size calculation during reindex results in recoverable blk file corruption by mruddy
  16. DrahtBot cross-referenced this on Aug 5, 2022 from issue indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky
  17. DrahtBot cross-referenced this on Aug 5, 2022 from issue refactor: Make adjusted time type safe by maflcko
  18. DrahtBot cross-referenced this on Aug 5, 2022 from issue refactor: Remove almost all validation option globals by maflcko
  19. DrahtBot cross-referenced this on Aug 6, 2022 from issue During IBD, prune as much as possible until we get close to where we will eventually keep blocks by luke-jr
  20. DrahtBot cross-referenced this on Aug 8, 2022 from issue refactor: Extract MIB_BYTES constant for init.cpp by Empact
  21. maflcko cross-referenced this on Aug 10, 2022 from issue test: Use existing {Chainstate,Block}Man by maflcko
  22. maflcko referenced this in commit e5d8b65423 on Aug 11, 2022
  23. maflcko force-pushed on Aug 11, 2022
  24. DrahtBot cross-referenced this on Aug 17, 2022 from issue refactor, kernel: Remove gArgs accesses from dbwrapper and txdb by ryanofsky
  25. DrahtBot cross-referenced this on Aug 19, 2022 from issue p2p: Erlay support signaling by naumenkogs
  26. DrahtBot added the label Needs rebase on Aug 22, 2022
  27. maflcko force-pushed on Aug 22, 2022
  28. DrahtBot removed the label Needs rebase on Aug 22, 2022
  29. w0xlt approved
  31. DrahtBot cross-referenced this on Aug 23, 2022 from issue refactor: Move ChainstateManager options into m_options struct by ryanofsky
  32. DrahtBot added the label Needs rebase on Aug 25, 2022
  33. TheCharlatan approved
  34. TheCharlatan commented at 1:47 PM on September 18, 2022: contributor

    Light code review ACK faf14c6305dcde11337cb1ed4f035f2e6b3bfc04

    I focused on the removal of the global params and for unwanted side effects during initialization. Delaying the pruning related errors a bit, but still reporting them before loading the chain data, seems acceptable to me.

  35. dergoegge commented at 5:20 PM on October 19, 2022: member

    Concept ACK

  36. maflcko force-pushed on Dec 7, 2022
  37. fanquake commented at 2:29 PM on December 7, 2022: member

    Concept ACK

  38. maflcko force-pushed on Dec 7, 2022
  39. maflcko force-pushed on Dec 7, 2022
  40. DrahtBot removed the label Needs rebase on Dec 7, 2022
  41. maflcko force-pushed on Dec 7, 2022
  42. maflcko force-pushed on Dec 7, 2022
  43. DrahtBot cross-referenced this on Dec 9, 2022 from issue validation: Improve error handling when VerifyDB dosn't finish successfully by mzumsande
  44. DrahtBot cross-referenced this on Dec 9, 2022 from issue Prune locks by luke-jr
  45. DrahtBot cross-referenced this on Dec 15, 2022 from issue assumeutxo: background validation completion by jamesob
  46. DrahtBot cross-referenced this on Dec 20, 2022 from issue prune: scan and unlink already pruned block files on startup by andrewtoth
  47. maflcko force-pushed on Jan 3, 2023
  48. maflcko force-pushed on Jan 3, 2023
  49. DrahtBot cross-referenced this on Jan 3, 2023 from issue bugfix: Make `CCheckQueue` RAII-styled (attempt 2) by hebasto
  50. in src/node/blockstorage.h:152 in fa20626495 outdated 
     147 | +    std::atomic<bool> m_importing{false};
 148 | +    /** Pruning-related variables and constants */
 149 | +    /** True if we're running in -prune mode. */
 150 | +    const bool m_prune_mode;
 151 | +    /** Number of bytes of block files that we're trying to stay below. */
 152 | +    const uint64_t m_prune_target;
    dergoegge commented at 10:10 AM on January 4, 2023:

    I would prefer if these were private and only accessed through methods on BlockManager.

    maflcko commented at 9:28 AM on January 30, 2023:

    done, I think

  51. in src/node/blockstorage.h:188 in fa20626495 outdated 
     184 | @@ -177,6 +185,11 @@ class BlockManager
 185 |      /** Store block on disk. If dbp is not nullptr, then it provides the known position of the block within a block file on disk. */
 186 |      FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp);
 187 |  
 188 | +    [[nodiscard]] bool LoadingBlocks()
    dergoegge commented at 10:10 AM on January 4, 2023: 
        [[nodiscard]] bool LoadingBlocks() const
    fanquake commented at 11:50 AM on January 4, 2023:

    See also #26664.

    maflcko commented at 9:28 AM on January 30, 2023:

    already done in master

  52. dergoegge changes_requested
  53. DrahtBot cross-referenced this on Jan 5, 2023 from issue refactor: add kernel/cs_main.h by fanquake
  54. DrahtBot added the label Needs rebase on Jan 16, 2023
  55. maflcko cross-referenced this on Jan 16, 2023 from issue refactor: Add BlockManager getters by maflcko
  56. maflcko force-pushed on Jan 17, 2023
  57. DrahtBot removed the label Needs rebase on Jan 17, 2023
  58. DrahtBot cross-referenced this on Jan 18, 2023 from issue Remove reindex special case from the progress bar label by maflcko
  59. DrahtBot cross-referenced this on Jan 19, 2023 from issue refactor: Make m_mempool optional in PeerManager by maflcko
  60. DrahtBot cross-referenced this on Jan 21, 2023 from issue refactor: Replace `std::optional<bilingual_str>` with `util::Result` by ryanofsky
  61. DrahtBot cross-referenced this on Jan 26, 2023 from issue net, refactor: net_processing, add `ProcessCompactBlockTxns` by brunoerg
  62. maflcko referenced this in commit 9a288430df on Jan 27, 2023
  63. maflcko force-pushed on Jan 27, 2023
  64. maflcko force-pushed on Jan 27, 2023
  65. TheCharlatan approved
  66. TheCharlatan commented at 9:16 AM on January 30, 2023: contributor

    re-code review ACK fac85d8a93be71e919e01511f2bff7e684932b6d

  67. in src/validation.cpp:5309 in fa710f1aa3 outdated 
    5305 | @@ -5306,7 +5306,7 @@ static ChainstateManager::Options&& Flatten(ChainstateManager::Options&& opts)
5306 |      return std::move(opts);
5307 |  }
5308 |  
5309 | -ChainstateManager::ChainstateManager(Options options) : m_options{Flatten(std::move(options))} {}
5310 | +ChainstateManager::ChainstateManager(Options options) : m_options{Flatten(std::move(options))}, m_blockman{options.blockman_opts} {}
    maflcko commented at 9:30 AM on January 30, 2023:

    in fa710f1aa3280532279ddaa510826e6bd2d3d1c8: I wonder if this is a use-after-move and why clang-tidy does not see it

    dergoegge commented at 11:11 AM on January 30, 2023:

    From https://en.cppreference.com/w/cpp/language/constructor#Initialization_order:

    non-static data member are initialized in order of declaration in the class definition.

    So according to that I'd say yes, this is a use after move (as m_options is declared before m_blockman).

    (also: https://github.com/llvm/llvm-project/issues/56195)

    TheCharlatan commented at 4:00 PM on February 15, 2023:

    In this case why not decouple the two options and pass and construct them independently? I've been using this branch for further work on getting the gArgs out of blockstorage.cpp and got the impression like there is little benefit in nesting the options.

    maflcko commented at 5:52 PM on March 14, 2023:

    there is little benefit ...

    Thanks, done.

  68. fanquake requested review from john-moffett on Feb 6, 2023
  69. DrahtBot added the label Needs rebase on Feb 7, 2023
  70. DrahtBot requested review from w0xlt on Feb 15, 2023
  71. TheCharlatan cross-referenced this on Feb 19, 2023 from issue Remove blockstorage args by TheCharlatan
  72. TheCharlatan cross-referenced this on Feb 19, 2023 from issue refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan
  73. maflcko force-pushed on Mar 14, 2023
  74. maflcko force-pushed on Mar 14, 2023
  75. DrahtBot removed the label Needs rebase on Mar 14, 2023
  76. fanquake removed review request from w0xlt on Mar 14, 2023
  77. fanquake requested review from TheCharlatan on Mar 14, 2023
  78. in src/node/blockmanager_args.cpp:14 in ffffd70359 outdated 
       9 | +
  10 | +namespace node {
  11 | +std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Options& opts)
  12 | +{
  13 | +    // block pruning; get the amount of disk space (in MiB) to allot for block & undo files
  14 | +    int64_t nPruneArg = args.GetIntArg("-prune", 0);
    TheCharlatan commented at 7:35 PM on March 14, 2023:

    I'm taking a look at this with some fresh knowledge now. If I understand this correctly, this always overrides what is passed in the opts with 0 if the prune arg is not set. The way I understand the current patterns elsewhere in the code for reading the arguments, the passed in opts are preserved if no argument is found, for example as done in chainstatemanager_args / chainstatemanager_opts. I'm not sure which pattern is preferable to be honest, but I think it should be consistent.

    This also similarly applies to fPruneMode.

    maflcko commented at 2:48 PM on March 15, 2023:

    Yes, makes sense for consistency. Thanks, done.

    This also similarly applies to fPruneMode.

    I don't think it does. I've reworked the second commit.

    TheCharlatan commented at 4:15 PM on March 15, 2023:

    Right on, can be marked as resolved.

  79. DrahtBot cross-referenced this on Mar 15, 2023 from issue p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode by dergoegge
  80. DrahtBot cross-referenced this on Mar 15, 2023 from issue assumeutxo by jamesob
  81. DrahtBot cross-referenced this on Mar 15, 2023 from issue refactor: Extract util/fs from util/system by TheCharlatan
  82. Move ::nPruneTarget into BlockManager fa721f1cab
  83. Move ::fPruneMode into BlockManager fa177d7b6b
  84. Pass fImporting to ImportingNow helper class fa442b1377
  85. Move ::fImporting to BlockManager fa9bd7be47
  86. maflcko force-pushed on Mar 15, 2023
  87. refactor: Add and use PRUNE_TARGET_MANUAL constexpr fadf8b8182
  88. maflcko commented at 3:04 PM on March 15, 2023: member

    I've added a new commit, because all of this was reworked anyway.

  89. TheCharlatan commented at 4:51 PM on March 15, 2023: contributor

    Code review ACK fadf8b818226dc60adf88e927160f9c9680473a4

  90. DrahtBot requested review from w0xlt on Mar 15, 2023
  91. in src/node/blockstorage.h:150 in fadf8b8182 
     145 | +
 146 | +    explicit BlockManager(Options opts)
 147 | +        : m_prune_mode{opts.prune_target > 0},
 148 | +          m_opts{std::move(opts)} {};
 149 | +
 150 | +    std::atomic<bool> m_importing{false};
    dergoegge commented at 5:04 PM on March 15, 2023:

    nit: I think it would be nice if access to this was wrapped in a getter/setter, e.g. Importing(), {Start,Stop}Importing().

    My thinking is that we might (in the future) want to mock the BlockManager to avoid expensive i/o in the fuzz tests for example. Having a clear interface defined would make that easier. A simple member like this is not a huge blocker for that so feel free to ignore.

    maflcko commented at 8:31 AM on March 16, 2023:

    This shouldn't be exposed at all, so I think making it private and ImportingNow a friend or so might be better?

  92. dergoegge commented at 5:05 PM on March 15, 2023: member

    Code review ACK fadf8b818226dc60adf88e927160f9c9680473a4

  93. DrahtBot removed review request from w0xlt on Mar 15, 2023
  94. DrahtBot requested review from w0xlt on Mar 15, 2023
  95. DrahtBot removed review request from w0xlt on Mar 15, 2023
  96. DrahtBot requested review from w0xlt on Mar 15, 2023
  97. achow101 commented at 10:38 PM on March 15, 2023: member

    ACK fadf8b818226dc60adf88e927160f9c9680473a4

  98. DrahtBot removed review request from w0xlt on Mar 15, 2023
  99. DrahtBot requested review from w0xlt on Mar 15, 2023
  100. achow101 merged this on Mar 15, 2023
  101. achow101 closed this on Mar 15, 2023
  102. sidhujag referenced this in commit 1eb1360d5e on Mar 16, 2023
  103. sidhujag referenced this in commit 5ed0b8bb8c on Mar 16, 2023
  104. maflcko deleted the branch on Mar 16, 2023
  105. hebasto cross-referenced this on Mar 31, 2023 from issue Rebased `cmake-staging` branch (post PR#10) by hebasto
  106. fanquake referenced this in commit c2f2abd0a4 on May 11, 2023
  107. bitcoin locked this on Mar 22, 2024
Contributors
maflckoDrahtBotw0xltTheCharlatandergoeggefanquakeachow101


w0xltjohn-moffett

Linked (view graph)
#25623 [kernel 3e/n] Decouple `CDBWrapper` and its kernel users from `ArgsManager`#25815 test: Use existing {Chainstate,Block}Man#26664 refactor: make some BlockManager members const#26900 refactor: Add BlockManager getters#27125 refactor, kernel: Decouple ArgsManager from blockstorage
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: 2026-05-19 06:53 UTC