Make blockstorage globals private members of BlockManager

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

DrahtBot commented at 6:32 am on January 5, 2022: member

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

ryanofsky approved

ryanofsky commented at 8:17 pm on January 5, 2022: member

Code review ACK fa68a6c2fc6754c160e0f98007785602201b3c47. Nice changes!

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.

0-    /** Get block file info entry for one block file */
1+    /** 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.

sidhujag referenced this in commit a523ac9e01 on Jan 7, 2022

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

DrahtBot locked this on Jan 8, 2023

Make blockstorage globals private members of BlockManager #23974

Conflicts