validation: Reduce direct g_chainman usage by dongcarl · Pull Request #19927 · bitcoin/bitcoin

dongcarl · 2020-09-09T17:08:05Z

This PR paves the way for de-globalizing g_chainman entirely by removing the usage of g_chainman in the following functions/methods:

~CMainCleanup
CChainState::FlushStateToDisk
UnloadBlockIndex

The remaining direct uses of g_chainman are as follows:

In initialization codepaths:
- AppTests
- AppInitMain
- TestingSetup::TestingSetup
::ChainstateActive
LookupBlockIndex
- Note: LookupBlockIndex is used extensively throughout the codebase and require a much larger set of changes, therefore I've left it out of this initial PR

maflcko

Concept ACK. I did similar changes or was planning to do them.

src/init.cpp

jamesob · 2020-09-09T19:57:43Z

Concept ACK; helps reduce globals, supports fuzzing.

fanquake · 2020-09-10T00:55:40Z

Concept ACK

jnewbery

Concept ACK.

I think you may as well clean up the code as you're moving it. No need to retain comments that are no longer useful, for example.

src/validation.h

Empact · 2020-09-13T22:57:55Z

I get this warning:

qt/test/apptests.cpp:87:45: warning: passing variable 'g_chainman' by reference requires holding mutex 'cs_main' [-Wthread-safety-reference]
    UnloadBlockIndex(/* mempool */ nullptr, g_chainman);
                                            ^

Another proposed change: pass as a ref instead (I see this was discussed above).

~CMainCleanup: 1. Is vestigial 2. References the g_chainman global (we should minimize g_chainman refs) 3. Only acts on g_chainman.m_blockman 4. Does the same thing as BlockManager::Unload

dongcarl · 2020-09-14T15:22:08Z

Thank you MarcoFalke, jnewbery, and Empact for the code review!

Marco: ChainstateManager is now passed in as a reference in 577ec184bd47b3e88a1d3e60b4d2045bd51328b4
jnewbery: Thanks for the detailed review! See the review comments for resolutions
Empact: Good find. I'm turning on -Werror=thread-safety by default now and that particular problem is fixed in 577ec184bd47b3e88a1d3e60b4d2045bd51328b4

Note: I will push a commit to remove the comments and assertions in 84d983656f1927955e516a94366b7c216bacded3 once I get the first Code Review ACK.

jnewbery

utACK cdd2063bbe8b8df4db42c81ee7913e2dc0b2387a

Just a couple of style suggestions inline. Feel free to ignore.

src/validation.cpp

ryanofsky

Code review ACK cdd2063bbe8b8df4db42c81ee7913e2dc0b2387a. Seems good and can re-ack if an extra assert-cleanup commit will be added.

src/qt/test/apptests.cpp

src/validation.cpp

[META] This is a pure refactor commit. Move PruneBlockFile to BlockManager because: 1. PruneOneBlockFile only acts on BlockManager 2. Eliminates the need for callers (FindFilesToPrune{,Manual}) to have a reference to the larger ChainstateManager, just a reference to BlockManager is enough. See following commits.

dongcarl · 2020-09-15T18:30:03Z

Got a code review ACK, just pushed a version which:

Has a last commit which removes the review-only comments + assertions
Adds a style-only commit which makes the code I moved conform to our style guide

This is now ready for final review and (hopefully) merge.

jnewbery · 2020-09-16T08:20:31Z

code review ACK 75142a7bd7890f2e14920dbe9701ada2cbc2326f

ryanofsky

Code review ACK 75142a7bd7890f2e14920dbe9701ada2cbc2326f. Since last review just new commit removing asserts, new commit cleaning up whitespace, and a newline fix in earlier commit

DrahtBot · 2020-09-19T13:42:13Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Remove unused CTxMemPool::clear() helper #19909 (txmempool: Remove unused clear() member function by MarcoFalke)
refactor: Make CCheckQueue RAII-styled #18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
Add local thread pool to CCheckQueue #18710 (Add local thread pool to CCheckQueue by hebasto)

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.

maflcko · 2020-09-20T08:08:41Z

Previously those methods were private (static in a cpp file), so I am a bit worried about exposing them as public member functions to all of Bitcoin Core (rpc, net_processing, ...).

What do you think about turning them private again?

E.g.

diff --git a/src/validation.h b/src/validation.h
index cc5e36acfc..bc9b136039 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -357,7 +357,12 @@ struct CBlockIndexWorkComparator
  * This data is used mostly in `CChainState` - information about, e.g.,
  * candidate tips is not maintained here.
  */
-class BlockManager {
+class BlockManager
+{
+    friend CChainState;
+    void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight, int chain_tip_height);
+    void FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPruneAfterHeight, int chain_tip_height, bool is_ibd);
+
 public:
     BlockMap m_block_index GUARDED_BY(cs_main);
 
@@ -411,24 +416,6 @@ public:
     //! Mark one block file as pruned (modify associated database entries)
     void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
-    /* Calculate the block/rev files to delete based on height specified by user with RPC command pruneblockchain */
-    void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight, int chain_tip_height);
-    /**
-     * Prune block and undo files (blk???.dat and undo???.dat) so that the disk space used is less than a user-defined target.
-     * The user sets the target (in MB) on the command line or in config file.  This will be run on startup and whenever new
-     * space is allocated in a block or undo file, staying below the target. Changing back to unpruned requires a reindex
-     * (which in this case means the blockchain must be re-downloaded.)
-     *
-     * Pruning functions are called from FlushStateToDisk when the global fCheckForPruning flag has been set.
-     * Block and undo files are deleted in lock-step (when blk00003.dat is deleted, so is rev00003.dat.)
-     * Pruning cannot take place until the longest chain is at least a certain length (100000 on mainnet, 1000 on testnet, 1000 on regtest).
-     * Pruning will never delete a block within a defined distance (currently 288) from the active chain's tip.
-     * The block index is updated by unsetting HAVE_DATA and HAVE_UNDO for any blocks that were stored in the deleted files.
-     * A db flag records the fact that at least some block files have been pruned.
-     *
-     * @param[out]   setFilesToPrune   The set of file indices that can be unlinked will be returned
-     */
-    void FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPruneAfterHeight, int chain_tip_height, bool is_ibd);
     /**
      * If a block header hasn't already been seen, call CheckBlockHeader on it, ensure
      * that it doesn't descend from an invalid block, and then add it to m_block_index.

maflcko · 2020-09-20T08:11:46Z

cr ACK 75142a7bd7890f2e14920dbe9701ada2cbc2326f 😺

Show signature and timestamp

Signature:

Timestamp of file with hash 3230012495f1048f1411855fc06225652a1358399085dc6b400e304c7f82dadb -

[META] No behaviour change is intended in this commit. [META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. Also stop FindFilesToPrune{,Manual} from unnecessary reaching for ::ChainActive() by passing in the necessary information.

[META] This is a pure style commit.

[META] This is a pure comment commit. They belong in the member declarations in the header file.

[META] This is a followup to "validation: Move FindFilesToPrune{,Manual} to BlockManager" removing comments and assertions meant only to show that the change is correct.

dongcarl · 2020-09-21T17:35:31Z

Made FindFilesToPrune{,Manual} private per Marco's comment.

Previous code reviewers: this change can be reviewed with:

git diff 75142a7 72a1d5c

jnewbery

utACK 72a1d5c

One style suggestion inline. Only consider it if you retouch the branch.

jnewbery · 2020-09-23T10:26:07Z

src/validation.h

+    friend CChainState;
+
+private:
+    /* Calculate the block/rev files to delete based on height specified by user with RPC command pruneblockchain */


(Only if you retouch the branch again) Make this a doxygen comment (start with /**). Also line-wrap this comment and the one below at something sensible 🙂

maflcko · 2020-09-23T18:34:34Z

re-ACK 72a1d5c 👚

Show signature and timestamp

Signature:

Timestamp of file with hash bcdea3595c56b92fa07e06e3a4cdb3a9a7439cb78f42e7fcdf109c35f3d1dd79 -

72a1d5c validation: Remove review-only comments + assertions (Carl Dong) 3756853 docs: Move FindFilesToPrune{,Manual} doxygen comment (Carl Dong) 485899a style: Make FindFilesToPrune{,Manual} match style guide (Carl Dong) 3f5b5f3 validation: Move FindFilesToPrune{,Manual} to BlockManager (Carl Dong) f8d4975 validation: Move PruneOneBlockFile to BlockManager (Carl Dong) 74f73c7 validation: Pass in chainman to UnloadBlockIndex (Carl Dong) 4668ded validation: Move ~CMainCleanup logic to ~BlockManager (Carl Dong) Pull request description: This PR paves the way for de-globalizing `g_chainman` entirely by removing the usage of `g_chainman` in the following functions/methods: - `~CMainCleanup` - `CChainState::FlushStateToDisk` - `UnloadBlockIndex` The remaining direct uses of `g_chainman` are as follows: 1. In initialization codepaths: - `AppTests` - `AppInitMain` - `TestingSetup::TestingSetup` 2. `::ChainstateActive` 3. `LookupBlockIndex` - Note: `LookupBlockIndex` is used extensively throughout the codebase and require a much larger set of changes, therefore I've left it out of this initial PR ACKs for top commit: MarcoFalke: re-ACK 72a1d5c 👚 jnewbery: utACK 72a1d5c Tree-SHA512: 944a4fa8405eecf39706ff944375d6824373aaeea849d11473f08181eff26b12f70043a8348a5b08e6e9021b243b481842fbdfbc7c3140ca795fce3688b7f5c3

In preparation for removing `cs_main` requirements for ChainstateActive()/ActiveChainstate(), relax lock requirements for g_chainman. I have verified that all usages of `g_chainman` that are concurrency-sensitive are usages of its members (e.g. `m_blockman`), which are already annotated. The references to `g_chainman.m_active_chainstate` will be protected by use of `std::atomic` in later commits. Here are all non-`BlockManager`-related usages: ``` % rg --pcre2 --trim 'g_chainman(?!.m_blockman)' src/net_processing.cpp 1271:assert(std::addressof(g_chainman) == std::addressof(m_chainman)); src/validation.h 994:extern ChainstateManager g_chainman; src/init.cpp 1423:node.chainman = &g_chainman; doc/release-notes/release-notes-0.21.0.md 577:- bitcoin#19927 Reduce direct `g_chainman` usage (dongcarl) src/validation.cpp 105:ChainstateManager g_chainman; 109:assert(g_chainman.m_active_chainstate); 110:return *g_chainman.m_active_chainstate; 174:assert(std::addressof(g_chainman.BlockIndex()) == std::addressof(m_block_index)); src/node/interfaces.cpp 479:assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman)); 489:assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman)); 502:assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman)); 514:assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman)); 516:assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman)); 525:assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman)); 527:assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman)); src/test/util/setup_common.cpp 143:m_node.chainman = &::g_chainman; src/qt/test/apptests.cpp 89:UnloadBlockIndex(/* mempool */ nullptr, g_chainman); 90:g_chainman.Reset(); ```

Summary: PR description: > This PR paves the way for de-globalizing g_chainman entirely by removing the usage of g_chainman in the following functions/methods: > > - ~CMainCleanup > - CChainState::FlushStateToDisk > - UnloadBlockIndex > > The remaining direct uses of g_chainman are as follows: > > - In initialization codepaths: > - AppTests > - AppInitMain > - TestingSetup::TestingSetup > - ::ChainstateActive > - LookupBlockIndex > Note: LookupBlockIndex is used extensively throughout the codebase and require a much larger set of changes, therefore I've left it out of this initial PR ~CMainCleanup: 1. Is vestigial 2. References the g_chainman global (we should minimize g_chainman refs) 3. Only acts on g_chainman.m_blockman 4. Does the same thing as BlockManager::Unload This is a backport of [[bitcoin/bitcoin#19927 | core#19927]] [1/6] bitcoin/bitcoin@4668ded Backport note: there were 7 commits in the original PR, but one of those commits is only touching style and is not relevant to us as the linter already fixed the style. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9820

Summary: This is a backport of [[bitcoin/bitcoin#19927 | core#19927]] [2/6] bitcoin/bitcoin@74f73c7 Depends on D9820 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9821

Summary: [META] This is a pure refactor commit. Move PruneBlockFile to BlockManager because: 1. PruneOneBlockFile only acts on BlockManager 2. Eliminates the need for callers (FindFilesToPrune{,Manual}) to have a reference to the larger ChainstateManager, just a reference to BlockManager is enough. See following commits. This is a backport of [[bitcoin/bitcoin#19927 | core#19927]] [3/6] bitcoin/bitcoin@f8d4975 Depends on D9821 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9822

Summary: [META] This is a pure comment commit. They belong in the member declarations in the header file. This is a backport of [[bitcoin/bitcoin#19927 | core#19927]] [5/6] bitcoin/bitcoin@3756853 Depends on D9823 Test Plan: NA Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9824

Summary: [META] This is a followup to "validation: Move FindFilesToPrune{,Manual} to BlockManager" removing comments and assertions meant only to show that the change is correct. This is a backport of [[bitcoin/bitcoin#19927 | core#19927]] [6/6] bitcoin/bitcoin@72a1d5c Depends on D9824 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9825

dongcarl added Refactoring Validation labels Sep 9, 2020

maflcko reviewed Sep 9, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Sep 13, 2020

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

validation: Move ~CMainCleanup logic to ~BlockManager

4668ded

~CMainCleanup: 1. Is vestigial 2. References the g_chainman global (we should minimize g_chainman refs) 3. Only acts on g_chainman.m_blockman 4. Does the same thing as BlockManager::Unload

dongcarl force-pushed the 2020-09-reduce-g_chainman-usage branch from 86cb85e to cdd2063 Compare September 14, 2020 15:13

jnewbery reviewed Sep 15, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Sep 15, 2020

View reviewed changes

src/qt/test/apptests.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

dongcarl added 2 commits September 15, 2020 14:11

validation: Pass in chainman to UnloadBlockIndex

74f73c7

dongcarl force-pushed the 2020-09-reduce-g_chainman-usage branch from cdd2063 to 75142a7 Compare September 15, 2020 18:23

ryanofsky approved these changes Sep 16, 2020

View reviewed changes

This was referenced Sep 19, 2020

refactor: Remove unused CTxMemPool::clear() helper #19909

Merged

Prune locks #19463

Closed

refactor: Make CCheckQueue RAII-styled #18731

Closed

Add local thread pool to CCheckQueue #18710

Merged

dongcarl added 4 commits September 21, 2020 13:27

style: Make FindFilesToPrune{,Manual} match style guide

485899a

[META] This is a pure style commit.

docs: Move FindFilesToPrune{,Manual} doxygen comment

3756853

[META] This is a pure comment commit. They belong in the member declarations in the header file.

validation: Remove review-only comments + assertions

72a1d5c

[META] This is a followup to "validation: Move FindFilesToPrune{,Manual} to BlockManager" removing comments and assertions meant only to show that the change is correct.

dongcarl force-pushed the 2020-09-reduce-g_chainman-usage branch from 75142a7 to 72a1d5c Compare September 21, 2020 17:33

jnewbery reviewed Sep 23, 2020

View reviewed changes

maflcko merged commit 1b313ca into bitcoin:master Sep 23, 2020

This was referenced Sep 30, 2020

De-globalizing ChainstateManager #20049

Closed

validation: Prune (in)direct g_chainman usage related to ::LookupBlockIndex (bundle 1) #20050

Closed

dongcarl mentioned this pull request Oct 15, 2020

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

dongcarl commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented Sep 9, 2020

Uh oh!

fanquake commented Sep 10, 2020

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Empact commented Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongcarl commented Sep 14, 2020

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dongcarl commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Sep 16, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Sep 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Sep 20, 2020

Uh oh!

maflcko commented Sep 20, 2020

Uh oh!

dongcarl commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dongcarl commented Sep 9, 2020 •

edited

Loading

Empact commented Sep 13, 2020 •

edited

Loading

dongcarl commented Sep 15, 2020 •

edited

Loading

DrahtBot commented Sep 19, 2020 •

edited

Loading

dongcarl commented Sep 21, 2020 •

edited

Loading