Make blockstorage globals private members of BlockManager #23974

pull MarcoFalke wants to merge 7 commits into bitcoin:master from MarcoFalke:2201-LessGlobals changing 6 files +142 −135

MarcoFalke commented at 1:36 PM on January 4, 2022: member

Globals aren't too nice because they hide dependencies, also they make testing harder.

Fix that by removing some.
fanquake added the label Block storage on Jan 4, 2022
MarcoFalke cross-referenced this on Jan 4, 2022 from issue Add `src/node/` and `src/wallet/` code to `node::` and `wallet::` namespaces by ryanofsky
DrahtBot commented at 6:32 AM on January 5, 2022: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #23497 (Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces by ryanofsky)
- #22932 (Guard CBlockIndex::nStatus by cs_main, require GetBlockPos/GetUndoPos to hold cs_main by jonatack)
- #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.
Move functions to BlockManager
Needed for a later commit
fa88cfd3f9

move-only: Create WriteBlockIndexDB helper

Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

fa467f3913

MarcoFalke force-pushed on Jan 5, 2022
in src/node/blockstorage.cpp:338 in fad81b7a0f outdated
```
 330 | @@ -348,6 +331,11 @@ void BlockManager::Unload()
 331 |      }
 332 |  
 333 |      m_block_index.clear();
 334 | +
 335 | +    vinfoBlockFile.clear();
 336 | +    nLastBlockFile = 0;
 337 | +    setDirtyBlockIndex.clear();
 338 | +    setDirtyFileInfo.clear();
```
ryanofsky commented at 2:19 PM on January 5, 2022:

In commit "Make blockstorage globals private members of BlockManager" (fad81b7a0f8ea300708321c3fcae7f75cade14ba)

Is this a pure refactor, or is there some change in behavior here? It seems like these lines are being moved from UnloadBlockIndex to BlockManager::Unload but neither function is directly calling the other.

It might be good to move this change to a separate commit before the "make private members" commit with an explanation about why it's safe. Not saying it's unsafe. It's just not obviously safe.

MarcoFalke commented at 2:23 PM on January 5, 2022:

UnloadBlockIndex calls chainman.Unload(), which again should call BlockManager::Unload. So only change should be the destruction order

ryanofsky commented at 2:39 PM on January 5, 2022:

UnloadBlockIndex calls chainman.Unload(), which again should call BlockManager::Unload. So only change should be the destruction order

So there is no code in some other place calling BlockManager::Unload and now having extra state cleared?

If you put this change in a separate commit and explicitly say whether it's supposed to change behavior and maybe say how it doesn't change behavior, it can make it easier for future reviewers and future regression bugfixers, who I think would logically be asking these questions.

MarcoFalke commented at 3:29 PM on January 5, 2022:

Thanks Ryan. Reworked the pull into more commits with descriptions in the body.

ryanofsky commented at 8:21 PM on January 5, 2022:

Thanks Ryan.

Ok, you finally convinced me

Move blockstorage-related unload to BlockManager::Unload

This is a refactor and safe to do because:
* UnloadBlockIndex calls ChainstateManager::Unload, which calls
  BlockManager::Unload
* Only unit tests call Unload directly

fab262174b

test: Load genesis block to allow flush

This is needed to turn globals into member variables. Otherwise, this
will lead to issues:

runtime error: reference binding to null pointer of type 'CBlockFileInfo'
    #0 in std::vector<CBlockFileInfo, std::allocator<CBlockFileInfo> >::operator[](unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:1046:2
    #1 in BlockManager::FlushBlockFile(bool, bool) src/node/blockstorage.cpp:540:47
    #2 in CChainState::FlushStateToDisk(BlockValidationState&, FlushStateMode, int) src/validation.cpp:2262:28
    #3 in CChainState::ResizeCoinsCaches(unsigned long, unsigned long) src/validation.cpp:4414:15
    #4 in validation_chainstate_tests::validation_chainstate_resize_caches::test_method() src/test/validation_chainstate_tests.cpp:66:12

fad381b2f8

doc: Clarify nPruneAfterHeight for signet faa8c2d7d7
Make blockstorage globals private members of BlockManager facd3df21f

scripted-diff: Rename touched member variables

-BEGIN VERIFY SCRIPT-

 ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; }

 ren vinfoBlockFile     m_blockfile_info
 ren nLastBlockFile     m_last_blockfile
 ren fCheckForPruning   m_check_for_pruning
 ren setDirtyBlockIndex m_dirty_blockindex
 ren setDirtyFileInfo   m_dirty_fileinfo

-END VERIFY SCRIPT-

fa68a6c2fc

MarcoFalke force-pushed on Jan 5, 2022
MarcoFalke added the label Refactoring on Jan 5, 2022
dongcarl cross-referenced this on Jan 5, 2022 from issue refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager` by dongcarl
ryanofsky approved
ryanofsky commented at 8:17 PM on January 5, 2022: contributor

Code review ACK fa68a6c2fc6754c160e0f98007785602201b3c47. Nice changes!
DrahtBot cross-referenced this on Jan 6, 2022 from issue Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main by jonatack
DrahtBot cross-referenced this on Jan 6, 2022 from issue assumeutxo by jamesob
Sjors commented at 4:03 PM on January 6, 2022: member

ACK fa68a6c2fc6754c160e0f98007785602201b3c47
fanquake merged this on Jan 7, 2022
fanquake closed this on Jan 7, 2022
in src/node/blockstorage.cpp:342 in fa68a6c2fc
```
 337 | +    m_dirty_blockindex.clear();
 338 | +    m_dirty_fileinfo.clear();
 339 | +}
 340 | +
 341 | +bool BlockManager::WriteBlockIndexDB()
 342 | +{
```
jonatack commented at 12:17 PM on January 7, 2022:

fa467f3913918701 Missing AssertLockHeld(::cs_main) in the definition of this new helper. Added in #24002.

in src/node/blockstorage.h:145 in fa68a6c2fc

 141 | @@ -120,6 +142,16 @@ class BlockManager
 142 |  
 143 |      CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 144 |  
 145 | +    /** Get block file info entry for one block file */

jonatack commented at 1:38 PM on January 7, 2022:

Would be it worth mentioning that this function is only used by the unit tests? If yes, can add to #24002.

-    /** Get block file info entry for one block file */
+    /** Get block file info entry for one block file. Currently only used for unit tests. */

MarcoFalke commented at 8:40 AM on January 8, 2022:

It might be better to move the function to the test, if it is only used there.

jonatack cross-referenced this on Jan 7, 2022 from issue refactor: add thread safety lock assertion to WriteBlockIndexDB() by jonatack
sidhujag referenced this in commit a523ac9e01 on Jan 7, 2022
psgreco cross-referenced this on Jan 8, 2022 from issue Guix builds failing after #23778 by psgreco
MarcoFalke referenced this in commit 6182e5086f on Jan 8, 2022
MarcoFalke deleted the branch on Jan 8, 2022
sidhujag referenced this in commit 191608dff8 on Jan 9, 2022
Fabcien referenced this in commit d965a961b8 on Nov 17, 2022
Fabcien referenced this in commit 64321b46c0 on Nov 17, 2022
Fabcien referenced this in commit d218c9c030 on Nov 17, 2022
Fabcien referenced this in commit e9a3a974e4 on Nov 17, 2022
Fabcien referenced this in commit 6476bba7b7 on Nov 17, 2022
Fabcien referenced this in commit ae6e4269f6 on Nov 17, 2022
Fabcien referenced this in commit 15a33f9e06 on Nov 17, 2022
bitcoin locked this on Jan 8, 2023

Contributors

Labels

Refactoring Block storage

Linked (view graph)

#22564 refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager`#23497 Add `src/node/` and `src/wallet/` code to `node::` and `wallet::` namespaces #24002 refactor: add thread safety lock assertion to WriteBlockIndexDB()#24010 Guix builds failing after #23778

Make blockstorage globals private members of BlockManager #23974

Conflicts