validation: Reduce direct g

dongcarl commented at 5:08 pm on September 9, 2020: member

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

dongcarl added the label Refactoring on Sep 9, 2020

dongcarl added the label Validation on Sep 9, 2020

in src/init.cpp:1562 in bc872e44e2 outdated

1558@@ -1559,7 +1559,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1559                 chainman.m_total_coinstip_cache = nCoinCacheUsage;
1560                 chainman.m_total_coinsdb_cache = nCoinDBCache;
1561 
1562-                UnloadBlockIndex(node.mempool.get());
1563+                UnloadBlockIndex(node.mempool.get(), &chainman);

MarcoFalke commented at 7:36 pm on September 9, 2020:

in commit bc872e44e29f7108720072a7d37ce06fb069ba4f: Doesn’t it make more sense to pass it as this (i.e. make it a member function)?

I’ve done that in #19909 (commit facd88a77189f6953d28a6743e0c68175bc18f9e)

Conceptually it makes little sense for chainman to be optional. Why would you want to run a node with validation disabled? So, if you don’t like to make it a member function, I’d say to at least pass it as reference.

dongcarl commented at 4:38 pm on September 10, 2020:

I think in this particular instance, I prefer the latter solution of passing chainman as a reference, simply because UnloadBlockIndex touches so many validation globals that are not owned by chainman. Perhaps in the future when we move those globals into chainman or some other validation-container class, it would make more sense to do so.

I was worried about callers (to UnloadBlockIndex) who only have a pointer to chainman, but the only caller I see that only has a pointer to chainman is ~TestingSetup, and it already looks like it makes the assumption that m_node.chainman is not null (see the m_node.chainman->Reset() line in ~TestingSetup). Will make the change and take a look at your PR too.

jnewbery commented at 11:50 am on September 15, 2020:

I agree conceptually that UnloadBlockIndex should eventually be a member function of chainman, but also agree that we don’t need to do everything in one PR.

MarcoFalke commented at 7:36 pm on September 9, 2020: member

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

jamesob commented at 7:57 pm on September 9, 2020: member

Concept ACK; helps reduce globals, supports fuzzing.

fanquake commented at 0:55 am on September 10, 2020: member

Concept ACK

in src/validation.h:413 in 86cb85e77f outdated

406@@ -407,6 +407,12 @@ class BlockManager {
407     /** Create a new block index entry for a given block hash */
408     CBlockIndex* InsertBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
409 
410+    //! Mark one block file as pruned (modify associated database entries)
411+    void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
412+
413+    // See definition for documentation

jnewbery commented at 5:42 pm on September 13, 2020:

Move the doxygen comments here (this comment made sense when it was next to a forward declaration, but now that the declaration is in the header file, it should have the doxygen comment)

dongcarl commented at 3:15 pm on September 14, 2020:

Resolved in cdd2063bbe8b8df4db42c81ee7913e2dc0b2387a

in src/validation.h:921 in 86cb85e77f outdated

916@@ -914,6 +917,15 @@ class ChainstateManager
917     //! Check to see if caches are out of balance and if so, call
918     //! ResizeCoinsCaches() as needed.
919     void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
920+
921+    ~ChainstateManager() {

jnewbery commented at 5:44 pm on September 13, 2020:

Did you consider moving this logic to the BlockManager dtor instead of here? There’s only ever one BlockManager instance (inside ChainstateManager), so destruction will happen at the same time. All the logic in this function is on BlockManager data.

jnewbery commented at 9:02 pm on September 13, 2020:

Actually, can’t you just call BlockManager::Unload() in the BlockManager dtor?

dongcarl commented at 3:15 pm on September 14, 2020:

Resolved in 4668ded6d6ea4299d998abbb57543f37519812e2

in src/validation.h:922 in 86cb85e77f outdated

916@@ -914,6 +917,15 @@ class ChainstateManager
917     //! Check to see if caches are out of balance and if so, call
918     //! ResizeCoinsCaches() as needed.
919     void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
920+
921+    ~ChainstateManager() {
922+        // block headers

jnewbery commented at 5:44 pm on September 13, 2020:

Remove useless comment.

in src/validation.h:924 in 86cb85e77f outdated

916@@ -914,6 +917,15 @@ class ChainstateManager
917     //! Check to see if caches are out of balance and if so, call
918     //! ResizeCoinsCaches() as needed.
919     void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
920+
921+    ~ChainstateManager() {
922+        // block headers
923+        BlockMap::iterator it1 = BlockIndex().begin();
924+        for (; it1 != BlockIndex().end(); it1++) {

jnewbery commented at 5:45 pm on September 13, 2020:

0        for (auto it = BlockIndex().begin; it != BlockIndex().end(); ++it) {

jnewbery commented at 5:47 pm on September 13, 2020: member

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.

Empact commented at 10:57 pm on September 13, 2020: member

I get this warning:

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

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

validation: Move ~CMainCleanup logic to ~BlockManager

~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

4668ded6d6

dongcarl force-pushed on Sep 14, 2020

dongcarl commented at 3:22 pm on September 14, 2020: member

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.

in src/validation.cpp:3965 in cdd2063bbe outdated

4002     LOCK2(cs_main, cs_LastBlockFile);
4003-    if (::ChainActive().Tip() == nullptr)
4004+    if (chain_tip_height < 0)
4005         return;
4006 
4007+

jnewbery commented at 11:42 am on September 15, 2020:

style: no need for extra line break.

dongcarl commented at 6:23 pm on September 15, 2020:

Resolved in fe07ba16511bd119b6ed744e68adfb3210a84254

in src/validation.cpp:4001 in cdd2063bbe outdated

3999 {
4000     assert(fPruneMode && nManualPruneHeight > 0);
4001 
4002     LOCK2(cs_main, cs_LastBlockFile);
4003-    if (::ChainActive().Tip() == nullptr)
4004+    if (chain_tip_height < 0)

jnewbery commented at 11:43 am on September 15, 2020:

style: when touching lines, fix code to match style guide (here, join the line below with this or add braces)

dongcarl commented at 6:24 pm on September 15, 2020:

Resolved in 597ec8a853df20fc68892b5f1131d27a1f2cf0e5

jnewbery commented at 11:49 am on September 15, 2020: member

utACK cdd2063bbe8b8df4db42c81ee7913e2dc0b2387a

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

in src/qt/test/apptests.cpp:89 in 577ec184bd outdated

83@@ -84,8 +84,11 @@ void AppTests::appTests()
84     // Reset global state to avoid interfering with later tests.
85     LogInstance().DisconnectTestLogger();
86     AbortShutdown();
87-    UnloadBlockIndex(/* mempool */ nullptr);
88-    WITH_LOCK(::cs_main, g_chainman.Reset());
89+    {
90+        LOCK(cs_main);
91+        UnloadBlockIndex(/* mempool */ nullptr, g_chainman);

ryanofsky commented at 2:59 pm on September 15, 2020:

In commit “validation: Pass in chainman to UnloadBlockIndex” (577ec184bd47b3e88a1d3e60b4d2045bd51328b4)

re: [META] This is a pure refactor commit.

Trivial: I’d call this “mostly pure refactor” instead of “pure refactor” because cs_main is no longer released between UnloadBlockIndex and chainstate reset (which I think is nontrivial, closing various coin views)

Am also curious about origin of [META] tags. They seems funny because I’d expect a meta comment in the commit description to be a comment about the commit description (:exploding_head:), not a comment about the commit. Anyway, I like the tags and they seem good for describing changes in a uniform way.

dongcarl commented at 6:27 pm on September 15, 2020:

Resolved in 74f73c783d46b012f375d819e2cd09c792820cd5 (I just removed that comment since “mostly pure refactor” isn’t too meaningful :smile:

For the [META] tags, I literally just came up with it randomly, mostly to describe high-level things that apply to the entire commit or things that reference other commits.

in src/validation.cpp:2296 in 84d983656f outdated

2291+            //
2292+            // This comment and the following assert will be removed in a
2293+            // subsequent commit, as they're just meant to demonstrate
2294+            // correctness (you can run tests against it and see that nothing
2295+            // exit unexpectedly).
2296+            assert(std::addressof(::ChainActive()) == std::addressof(m_chain));

ryanofsky commented at 3:10 pm on September 15, 2020:

In commit “validation: Move FindFilesToPrune{,Manual} to BlockManager” (84d983656f1927955e516a94366b7c216bacded3)

Nice way of verifying, and didn’t know about std::addressof!

ryanofsky approved

ryanofsky commented at 3:17 pm on September 15, 2020: member

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

validation: Pass in chainman to UnloadBlockIndex 74f73c783d

validation: Move PruneOneBlockFile to BlockManager

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

f8d4975ab3

dongcarl force-pushed on Sep 15, 2020

dongcarl commented at 6:30 pm on September 15, 2020: member

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 commented at 8:20 am on September 16, 2020: member

code review ACK 75142a7bd7890f2e14920dbe9701ada2cbc2326f

ryanofsky approved

ryanofsky commented at 4:00 pm on September 16, 2020: member

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 commented at 1:42 pm on September 19, 2020: 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:

#19909 (txmempool: Remove unused clear() member function by MarcoFalke)
#18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
#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.

MarcoFalke commented at 8:08 am on September 20, 2020: member

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.

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

MarcoFalke commented at 8:11 am on September 20, 2020: member

cr ACK 75142a7bd7890f2e14920dbe9701ada2cbc2326f 😺

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3cr ACK 75142a7bd7890f2e14920dbe9701ada2cbc2326f 😺
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhyqwv/eNXZQ6DzY+d3CX83lRRCFbjD3zfjQ0o0dT2SBGnP4oaG6gI+fwzUQacl
 8ZdtAZ3JEhJU6RDFs+gMun19ldVP7SG+RhJAGHRZgNuWzT0VvC/I1USyK/QZ7Rb53
 9G2Rz7oSIMTZ9X88kVBuBVYPphSalSVKDTwbPoV5tYUn96Z753aG768FS1pop6Lvx
10LwFLLVWjN8hpA1UAinh57qIAy9Jr21jA1bGcUG5M8AydqTlwA1pTig80Yktn5cvb
11Me9FWSLHqifUHR2Js8TUjwykChul18d/8C9C4kmOynDeH4b+n7nmYFP1Ynrh458X
12P71pSb0vcVFK1fXnFFVh3z493Is2vpGxxuZino4RPvK4+53G7fJ41xlL78r+2cRk
13gKrBAukJNMM02RRwdQ1W+4jSpcZcf8PHLUNSg7BVOP5AFLcCekvhExVLIJJO9LiS
14Q5jNu4OQSVEuoZhjbmoeHQhgPe+lrd3LYWvPtpJveos24+gTNSS3KWFD1IB3QvzK
15HIpk9O5/
16=izRp
17-----END PGP SIGNATURE-----

Timestamp of file with hash 3230012495f1048f1411855fc06225652a1358399085dc6b400e304c7f82dadb -

validation: Move FindFilesToPrune{,Manual} to BlockManager

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

3f5b5f3f6d

style: Make FindFilesToPrune{,Manual} match style guide

[META] This is a pure style commit.

485899a93c

docs: Move FindFilesToPrune{,Manual} doxygen comment

[META] This is a pure comment commit.

They belong in the member declarations in the header file.

3756853b15

validation: Remove review-only comments + assertions

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

72a1d5c6f3

dongcarl force-pushed on Sep 21, 2020

dongcarl commented at 5:35 pm on September 21, 2020: member

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

Previous code reviewers: this change can be reviewed with:

0git diff 75142a7 72a1d5c

in src/validation.h:364 in 72a1d5c6f3

360+class BlockManager
361+{
362+    friend CChainState;
363+
364+private:
365+    /* Calculate the block/rev files to delete based on height specified by user with RPC command pruneblockchain */

jnewbery commented at 10:26 am on September 23, 2020:

(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 :slightly_smiling_face:

jnewbery commented at 10:26 am on September 23, 2020: member

utACK 72a1d5c6f3834e206719ee5121df7727aed5b786

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

MarcoFalke commented at 6:34 pm on September 23, 2020: member

re-ACK 72a1d5c6f3 👚

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 72a1d5c6f3 👚
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhTrAv+Ky3GNUn0kO04zX+7dsU23mPYIvAUJ/SRXBtSQID9lNph+93gmf29iMWM
 83eOnDT1KhSINP5mHGRX9j4loYTh2VxUE9Hwqzc/em4bfh+QcE/Fyjxx6uArGVZBQ
 9cYCEwsoQ51nbp9/Yftt2UFQzjjtHXtEGn7KO++u2WDzpOtL8vdxOZTK+e0Xb6Gbd
10XJSDGf2D6YeAhElyMETc6TJQ3/E7nHTWurNjXVR/C6EbeeVzrTlaLmGVGUBqWcAf
11qcQjVHB3qrQHrXfr+bK6XlSOJ4/60T+Q801mX2AeUK4ygpgcsVpWquaviC1DqzfM
12kdrdssVPW9b5yuUM8ciaPxLTiXBqyzKVaUA8Q3CADsQClStA7MOfUItmo9HUs/Lz
13VWughvafPFfI+cbHkR3rvvvMcPza12/9sYYfCi+xHfw1CaMnjs5lvDD0Cw4ShGMH
14h/vJzV/MFnDquVrg/T1Cam1zkXf3bxGsc7K/mJh3wO99Kgf9X9nlS7Y77eQIeWx2
15bzQoIo/G
16=9glt
17-----END PGP SIGNATURE-----

Timestamp of file with hash bcdea3595c56b92fa07e06e3a4cdb3a9a7439cb78f42e7fcdf109c35f3d1dd79 -

MarcoFalke merged this on Sep 23, 2020

MarcoFalke closed this on Sep 23, 2020

sidhujag referenced this in commit be4781cdfb on Sep 23, 2020

jamesob referenced this in commit ed479bb9b6 on Apr 6, 2021

jamesob referenced this in commit 361dd79c27 on Apr 6, 2021

deadalnix referenced this in commit 37142f8783 on Jul 21, 2021

deadalnix referenced this in commit 0082bd0457 on Jul 21, 2021

deadalnix referenced this in commit 046b04cb79 on Jul 21, 2021

Fabcien referenced this in commit a9ecaa69dc on Jul 21, 2021

deadalnix referenced this in commit e551bfacbf on Jul 21, 2021

DrahtBot locked this on Feb 15, 2022

validation: Reduce direct g_chainman usage #19927

Conflicts