Make blockstorage globals private members of BlockManager by maflcko · Pull Request #23974 · bitcoin/bitcoin

maflcko · 2022-01-04T13:36:14Z

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

Fix that by removing some.

DrahtBot · 2022-01-05T06:32:59Z

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.

Needed for a later commit

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

src/node/blockstorage.cpp

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

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 bitcoin#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

-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-

ryanofsky

Code review ACK fa68a6c. Nice changes!

Sjors · 2022-01-06T16:03:07Z

ACK fa68a6c

jonatack · 2022-01-07T12:17:06Z

src/node/blockstorage.cpp

+}
+
+bool BlockManager::WriteBlockIndexDB()
+{


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

jonatack · 2022-01-07T13:38:16Z

src/node/blockstorage.h


    CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);

+    /** Get block file info entry for one block file */


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. */

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

…ckManager

…kIndexDB() 1823766 refactor: add thread safety lock assertion to WriteBlockIndexDB() (Jon Atack) Pull request description: New helper function `BlockManager::WriteBlockIndexDB()` added in #23974 has a thread safety lock annotation in its declaration but is missing the corresponding run-time lock assertion in its definition. Per doc/developer-notes.md: "Combine annotations in function declarations with run-time asserts in function definitions." ACKs for top commit: MarcoFalke: cr ACK 1823766 Tree-SHA512: b915e6b105c38b8bbe04ad810aefa68e940a13b8dd265e79563a2aaefc93ffa031d56a7f3c481a5ada90de7c2ddd3b419dcfa46c22fa26c22f95eda15cd243bc

…iteBlockIndexDB() 1823766 refactor: add thread safety lock assertion to WriteBlockIndexDB() (Jon Atack) Pull request description: New helper function `BlockManager::WriteBlockIndexDB()` added in bitcoin#23974 has a thread safety lock annotation in its declaration but is missing the corresponding run-time lock assertion in its definition. Per doc/developer-notes.md: "Combine annotations in function declarations with run-time asserts in function definitions." ACKs for top commit: MarcoFalke: cr ACK 1823766 Tree-SHA512: b915e6b105c38b8bbe04ad810aefa68e940a13b8dd265e79563a2aaefc93ffa031d56a7f3c481a5ada90de7c2ddd3b419dcfa46c22fa26c22f95eda15cd243bc

Summary: Needed for a later commit This is a partial backport of [[bitcoin/bitcoin#23974 | core#23974]] bitcoin/bitcoin@fa88cfd Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12510

Summary: Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space This is a partial backport of [[bitcoin/bitcoin#23974 | core#23974]] bitcoin/bitcoin@fa467f3 Depends on D12510 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12511

Summary: This is a refactor and safe to do because: * UnloadBlockIndex calls ChainstateManager::Unload, which calls BlockManager::Unload * Only unit tests call Unload directly This is a partial backport of [[bitcoin/bitcoin#23974 | core#23974]] bitcoin/bitcoin@fab2621 Depends on D12511 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12512

Summary: 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 ``` This is a partial backport of [[bitcoin/bitcoin#23974 | core#23974]] bitcoin/bitcoin@fad381b Depends on D12512 Test Plan: `ninja check` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12513

Summary: This is a partial backport of [[bitcoin/bitcoin#23974 | core#23974]] bitcoin/bitcoin@faa8c2d Depends on D12513 Test Plan: proofreading Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12514

Summary: This is a partial backport of [[bitcoin/bitcoin#23974 | core#23974]] bitcoin/bitcoin@facd3df Depends on D12514 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12517

Summary: ``` -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- ``` This concludes backport of [[bitcoin/bitcoin#23974 | core#23974]] bitcoin/bitcoin@fa68a6c Depends on D12517 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12518

fanquake added the Block storage label Jan 4, 2022

maflcko mentioned this pull request Jan 4, 2022

Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces #23497

Merged

MarcoFalke added 2 commits January 5, 2022 15:07

Move functions to BlockManager

fa88cfd

Needed for a later commit

move-only: Create WriteBlockIndexDB helper

fa467f3

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

maflcko force-pushed the 2201-LessGlobals branch from faaecfe to fad81b7 Compare January 5, 2022 14:09

ryanofsky reviewed Jan 5, 2022

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

MarcoFalke added 5 commits January 5, 2022 16:15

Move blockstorage-related unload to BlockManager::Unload

fab2621

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

doc: Clarify nPruneAfterHeight for signet

faa8c2d

Make blockstorage globals private members of BlockManager

facd3df

maflcko force-pushed the 2201-LessGlobals branch from e339654 to fa68a6c Compare January 5, 2022 15:19

maflcko added the Refactoring label Jan 5, 2022

dongcarl mentioned this pull request Jan 5, 2022

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

ryanofsky approved these changes Jan 5, 2022

View reviewed changes

This was referenced Jan 6, 2022

Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main #22932

Merged

assumeutxo #15606

Closed

fanquake merged commit 4ada742 into bitcoin:master Jan 7, 2022

jonatack reviewed Jan 7, 2022

View reviewed changes

jonatack mentioned this pull request Jan 7, 2022

refactor: add thread safety lock assertion to WriteBlockIndexDB() #24002

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 7, 2022

Merge bitcoin#23974: Make blockstorage globals private members of Blo…

a523ac9

…ckManager

psgreco mentioned this pull request Jan 8, 2022

Guix builds failing after #23778 #24010

Closed

maflcko deleted the 2201-LessGlobals branch January 8, 2022 08:40

bitcoin locked and limited conversation to collaborators Jan 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make blockstorage globals private members of BlockManager#23974

Make blockstorage globals private members of BlockManager#23974
fanquake merged 7 commits intobitcoin:masterfrom
maflcko:2201-LessGlobals

maflcko commented Jan 4, 2022

Uh oh!

DrahtBot commented Jan 5, 2022 •

edited

Loading

Uh oh!

Uh oh!

ryanofsky left a comment

Uh oh!

Sjors commented Jan 6, 2022 •

edited

Loading

Uh oh!

jonatack Jan 7, 2022

Uh oh!

jonatack Jan 7, 2022

Uh oh!

maflcko Jan 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants


		CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);

		/** Get block file info entry for one block file */

Conversation

maflcko commented Jan 4, 2022

Uh oh!

DrahtBot commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 8, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Jan 5, 2022 •

edited

Loading

Sjors commented Jan 6, 2022 •

edited

Loading