Only load BlockMan in BlockMan member functions #24515

pull dongcarl wants to merge 7 commits into bitcoin:master from dongcarl:2022-03-kirby-p2.5 changing 10 files +155 −130
  1. dongcarl commented at 11:24 PM on March 9, 2022: contributor

    The only important commit is "Only load BlockMan in BlockMan member functions", everything else is all just small style changes.

    Here's the commit message, reproduced:

    This commit effectively splits the "load block index itself" logic from
"derive Chainstate variables from loaded block index" logic.

This means that BlockManager::LoadBlockIndex{,DB} will only load what's
relevant to the BlockManager.
  2. refactor: more const annotations for uses of CBlockIndex* 5be9ee3c54
  3. style-only: Various blockstorage.cpp cleanups 3bbb6fea05
  4. dongcarl cross-referenced this on Mar 9, 2022 from issue refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager` by dongcarl
  5. DrahtBot added the label Block storage on Mar 10, 2022
  6. DrahtBot added the label RPC/REST/ZMQ on Mar 10, 2022
  7. DrahtBot added the label UTXO Db and Indexes on Mar 10, 2022
  8. DrahtBot added the label Validation on Mar 10, 2022
  9. MarcoFalke removed the label UTXO Db and Indexes on Mar 10, 2022
  10. MarcoFalke removed the label RPC/REST/ZMQ on Mar 10, 2022
  11. MarcoFalke removed the label Validation on Mar 10, 2022
  12. MarcoFalke removed the label Block storage on Mar 10, 2022
  13. MarcoFalke added the label Refactoring on Mar 10, 2022
  14. MarcoFalke added the label Block storage on Mar 10, 2022
  15. in src/node/blockstorage.cpp:228 in 6741a6957a outdated 
     226 |      for (auto& [_, block_index] : m_block_index) {
 227 | -        vSortedByHeight.push_back(std::make_pair(block_index.nHeight, &block_index));
 228 | +        vSortedByHeight.push_back(&block_index);
 229 |      }
 230 | -    sort(vSortedByHeight.begin(), vSortedByHeight.end());
 231 | +    sort(vSortedByHeight.begin(), vSortedByHeight.end(),
    MarcoFalke commented at 7:06 AM on March 10, 2022:

    is this std::sort? If yes, maybe switch to that while touching

    dongcarl commented at 11:38 PM on March 15, 2022:

    Fixed in 28ba0313eac37e4a900b7e97af7169ce999c4024

  16. in src/node/blockstorage.cpp:236 in 6741a6957a outdated 
     236 |      // Find start of assumed-valid region.
 237 |      int first_assumed_valid_height = std::numeric_limits<int>::max();
 238 |  
 239 | -    for (const auto& [height, block] : vSortedByHeight) {
 240 | -        if (block->IsAssumedValid()) {
 241 | +    for (CBlockIndex* pindex : vSortedByHeight) {
    MarcoFalke commented at 7:09 AM on March 10, 2022:

    maybe add the const right here in this commit to avoid a separate commit for a single keyword? Also, could keep the symbol name block to reduce the diff by one line :sweat_smile:

    ryanofsky commented at 7:07 PM on March 14, 2022:

    In commit "style-only: No need for std::pair for vSortedByHeight" (6741a6957a240e9a0cdbd73df95f247b449e7b72)

    This is fine but I don't really get it. Previous commit is renaming a pindex to block_index, this one is renaming a block variable to pindex. If these cleanups were consolidated into one commit, it'd be easier to see if the changes were consistent. (Also of the names I like simple block the best).

    dongcarl commented at 11:48 PM on March 15, 2022:

    Absorbed into 42e56d9b188f97c077ed2269e24acc0be35ece17

  17. in src/validation.cpp:4094 in f5a20039a4 outdated 
    4090 | +        std::vector<CBlockIndex*> vSortedByHeight;
4091 | +        vSortedByHeight.reserve(m_blockman.m_block_index.size());
4092 | +        for (auto& [_, block_index] : m_blockman.m_block_index) {
4093 | +            vSortedByHeight.push_back(&block_index);
4094 | +        }
4095 | +        sort(vSortedByHeight.begin(), vSortedByHeight.end(),
    MarcoFalke commented at 7:10 AM on March 10, 2022:

    same

    dongcarl commented at 11:47 PM on March 15, 2022:

    Fixed in 28ba0313eac37e4a900b7e97af7169ce999c4024

  18. MarcoFalke approved
  19. MarcoFalke commented at 10:22 AM on March 10, 2022: member

    LGTM

  20. ajtowns commented at 3:26 PM on March 10, 2022: contributor

    (Ditto what @MarcoFalke said)

  21. DrahtBot commented at 6:36 AM on March 11, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24582 (Move txoutproof RPCs to txoutproof.cpp by MarcoFalke)
    • #24456 (blockman: Properly guard blockfile members by dongcarl)
    • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
    • #22564 (refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)
    • #21726 (Improve Indices on pruned nodes via prune blockers by fjahr)

    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.

  22. DrahtBot cross-referenced this on Mar 11, 2022 from issue CChainState -> Chainstate by jamesob
  23. DrahtBot cross-referenced this on Mar 11, 2022 from issue blockman: Properly guard blockfile members by dongcarl
  24. DrahtBot cross-referenced this on Mar 11, 2022 from issue indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky
  25. ryanofsky commented at 5:05 PM on March 14, 2022: contributor

    The main thing this PR seems to be doing is moving chainstate->setBlockIndexCandidates filling code out of block-manager to chainstate-manager. This seems like it makes sense because you would expect block manager just to deal with low level blocks and not know anything about chain states or validation. Would be good to have @jamesob concept ACK since he added most of the code that's moving

  26. in src/validation.cpp:4097 in f5a20039a4 outdated 
    4093 | +            vSortedByHeight.push_back(&block_index);
4094 | +        }
4095 | +        sort(vSortedByHeight.begin(), vSortedByHeight.end(),
4096 | +             [](const CBlockIndex* pa, const CBlockIndex* pb) {
4097 | +                 return pa->nHeight < pb->nHeight;
4098 | +             });
    ryanofsky commented at 6:34 PM on March 14, 2022:

    In commit "Only load BlockMan in BlockMan member functions" (f5a20039a4a140679981943fa2e645ded687d14e)

    IMO, it would be better to add a std::vector<CBlockIndex*> GetSortedByHeight(BlockManager&) or similar helper function instead of duplicating this code.

    dongcarl commented at 11:48 PM on March 15, 2022:

    See 28ba0313eac37e4a900b7e97af7169ce999c4024 and f865cf8ded2b2fbc82a6fbc41226d991909a6880

  27. ryanofsky approved
  28. ryanofsky commented at 7:10 PM on March 14, 2022: contributor

    Code review ACK af44e85b920055be4c3f724000a17f9d8d31d5ae. Ignore minor comments unless you feel like addressing them

  29. dongcarl force-pushed on Mar 15, 2022
  30. style-only: No need for std::pair for vSortedByHeight
    ...since the height information in already in CBlockIndex* and we can
use an easy custom sorter.
    42e56d9b18
  31. Only load BlockMan in BlockMan member functions
    This commit effectively splits the "load block index itself" logic from
"derive Chainstate variables from loaded block index" logic.

This means that BlockManager::LoadBlockIndex{,DB} will only load what's
relevant to the BlockManager.

I strongly recommend reviewing with the following git-diff flags:
  --color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change
    c600ee3816
  32. move-only: Move CBlockIndexWorkComparator to blockstorage
    ...it's declared in blockstorage.h
    12eb05df63
  33. Add and use CBlockIndexHeightOnlyComparator
    ...also use std::sort for clarity
    28ba0313ea
  34. Add and use BlockManager::GetAllBlockIndices f865cf8ded
  35. dongcarl force-pushed on Mar 15, 2022
  36. dongcarl commented at 11:49 PM on March 15, 2022: contributor

    Pushed af44e85b920055be4c3f724000a17f9d8d31d5ae to f865cf8ded2b2fbc82a6fbc41226d991909a6880

    • Addressed all outstanding minor comments
  37. MarcoFalke approved
  38. MarcoFalke commented at 10:07 AM on March 16, 2022: member

    review ACK f865cf8ded2b2fbc82a6fbc41226d991909a6880 🗂

    <details><summary>Show signature</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK f865cf8ded2b2fbc82a6fbc41226d991909a6880 🗂
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg0DAv/VZj7G6Rp0qriCmTSDHrk21I938vrPa39hBPwQ9GU3DpaKPoIth5mj2Vl
jMREh59ZzqKKorCPa0RdcVaRyiVPTRup0EOSi0b9qwIQREr1pumfJb1Mir9HJtCY
WbLA/sj1YGuXpIUvWAEUkKE56vpLVahWnSx5jd9QFO0DBWbfQ3iHWj/Soc5EBxEV
5pquK8ceeWyZxN7NKHWwKZegfjMmuPvmrr1HVfFB9qwjzYIsJk7Hv83PXbTHNzMa
OBBxlGy6o/anN1w+iS6tflGCzmTgxXXWKROlm2pCDAgOJZi4FyM2vj/V/9XM4E4F
RghIzqLltFmVJwvRV7x/o0Ahf3QHsez5TIl8JvFxVjBAwPP6WW2cbUQnXJRGrpho
oIUVnBf/r5iZCImOGzCkslm5oBDmWHCGi2jscPcdIQAcQNrusIN2jy4OrWxVAYFw
fjORBHiWF1TnQElKrfBurviwt+LO8SZFcjtTm7ZN4BGFFknCGgUBD9hqP9GB6mzt
Tu78aGDd
=BBgB
-----END PGP SIGNATURE-----

    </details>

  39. DrahtBot cross-referenced this on Mar 16, 2022 from issue Move txoutproof RPCs to txoutproof.cpp by MarcoFalke
  40. ajtowns commented at 8:54 PM on March 16, 2022: contributor

    ACK f865cf8ded2b2fbc82a6fbc41226d991909a6880 ; code review only

  41. ajtowns cross-referenced this on Mar 17, 2022 from issue deploymentstatus: move g_versionbitscache global to ChainstateManager by ajtowns
  42. MarcoFalke merged this on Mar 17, 2022
  43. MarcoFalke closed this on Mar 17, 2022
  44. sidhujag referenced this in commit 003384d78e on Mar 18, 2022
  45. Fabcien referenced this in commit 5c7e567e1d on Jan 23, 2023
  46. Fabcien referenced this in commit cecbcba200 on Jan 23, 2023
  47. Fabcien referenced this in commit 3d50222404 on Jan 23, 2023
  48. Fabcien referenced this in commit 2aa2529f4a on Jan 23, 2023
  49. Fabcien referenced this in commit e225f7080c on Jan 23, 2023
  50. bitcoin locked this on Mar 17, 2023
Contributors
dongcarlMarcoFalkeryanofskyajtownsDrahtBot
Labels
RefactoringBlock storage
Linked (view graph)
#22564 refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager`#24595 deploymentstatus: move g_versionbitscache global to ChainstateManager
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-20 06:53 UTC