refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager`

dongcarl commented at 7:58 pm on July 27, 2021: member

This is a small part of the work to accomplish what I described in 972c5166ee685447a6d4bf5e501b07a0871fba85:

0Over time, we should probably move these mutable global state variables
1into ChainstateManager or CChainState so it's easier to reason about
2their lifecycles.

::UnloadBlockIndex manually resets a subset of our mutable globals in addition to unloading the ChainstateManager and clearing the mempool. The need for this manual reset (AFAICT) arises out of the fact that many of these globals are closely related to the block index (hence ::UnloadBlockIndex), and need to be reset with it.

I’ve shot this “manual reset” gun at my foot several times while doing the de-globalize chainman work.

Thankfully, now that we have a BlockManager class that owns the block index, these globals should be moved under that class so that they can live and die with the block index. These moves, along with making the block index non-heap-based, eliminates:

https://github.com/bitcoin/bitcoin/commit/3585b521392c5b2c855c3ba6dc9b7d2a171b3710 The need to reason about when we need to manually call ::UnloadBlockIndex (this decision can at times seem almost arbitrary)
https://github.com/bitcoin/bitcoin/commit/f741623c25455c20bff9eb1ddd10a4ac84dc5655 The need to have an ::UnloadBlockIndex or explicit ~ChainstateManager at all

dongcarl commented at 8:02 pm on July 27, 2021: member

Ping @jnewbery, since #21866 (comment) :relaxed:

DrahtBot added the label Block storage on Jul 27, 2021

DrahtBot added the label Consensus on Jul 27, 2021

DrahtBot added the label Mining on Jul 27, 2021

DrahtBot added the label P2P on Jul 27, 2021

DrahtBot added the label RPC/REST/ZMQ on Jul 27, 2021

DrahtBot added the label Validation on Jul 27, 2021

DrahtBot added the label Wallet on Jul 27, 2021

jnewbery commented at 9:48 pm on July 27, 2021: member

Concept ACK. Globals must fall!

MarcoFalke commented at 8:49 am on July 28, 2021: member

Concept ACK

DrahtBot commented at 10:06 am on July 28, 2021: 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:

#24595 (deploymentstatus: move g_versionbitscache global to ChainstateManager by ajtowns)
#24232 (assumeutxo: add init and completion logic by jamesob)
#24008 (assumeutxo: net_processing changes by jamesob)
#23443 (p2p: Erlay support signaling by naumenkogs)
#20030 (validation: Remove useless call to mempool->clear() by MarcoFalke)
#19888 (rpc, test: Improve getblockstats for unspendables by fjahr)

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.

in src/deploymentstatus.h:20 in 4d00171fc5 outdated

19     assert(Consensus::ValidDeployment(dep));
20     return (pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1) >= params.DeploymentHeight(dep);
21 }
22 
23-inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)
24+inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep, VersionBitsCache& versionbitscache)

ajtowns commented at 12:23 pm on August 1, 2021:

Hard NACK – the signatures for the two versions of DeploymentActiveAfter (and ..At) should only differ by whether they accept a DeploymentPos or a BuriedDeployment, otherwise we’re back to having to edit bunches of call sites when burying deployments.

BUT… I think you can get the result you’re trying for by making DeploymentActiveAfter (and ..At) inline member functions of BlockManager. Proof of concept at https://github.com/ajtowns/bitcoin/commits/202108-deplotmentstatus-blockmanager

Would it be feasible to move BlockManager to its own module to reduce the amount of stuff validation.cpp does? Giving BlockManager a CChainParams member reference and having CChainState and ChainstateManager reference it via their BlockManager references, and also removing the Consensus::Params and CChainParams params from bunches of functions (including the DeploymentActive.. ones) seems like it would be a win.

MarcoFalke commented at 2:24 pm on August 1, 2021:

Wouldn’t another class in validation be better suited to host the deployment status? My goal was to move BlockManger to node/storage and have it deal with storage only, not validation.

ajtowns commented at 2:52 pm on August 1, 2021:

@MarcoFalke Maybe? I look at deployment status as an expansion of Consensus::Params – so more as input to validation than a result of it. If we switch to using the coinbase witness nonce for deployment signalling we’d need to change the way we store blocks so that it’s easy to access that info without having to load every signalling block from disk, so it’s kind-of storage related in that sense?

dongcarl commented at 7:27 pm on August 3, 2021:

the signatures for the two versions of DeploymentActiveAfter (and ..At) should only differ by whether they accept a DeploymentPos or a BuriedDeployment, otherwise we’re back to having to edit bunches of call sites when burying deployments.

Oh I did not realize that this was the case! Note to self: add a code comment documenting this.

BUT… I think you can get the result you’re trying for by making DeploymentActiveAfter (and ..At) inline member functions of BlockManager. Proof of concept at https://github.com/ajtowns/bitcoin/commits/202108-deplotmentstatus-blockmanager

This looks good to me, and it seems like they would eventually end up having to interact with BlockManager anyway according to the following?

If we switch to using the coinbase witness nonce for deployment signalling we’d need to change the way we store blocks so that it’s easy to access that info without having to load every signalling block from disk, so it’s kind-of storage related in that sense?

Would it be feasible to move BlockManager to its own module to reduce the amount of stuff validation.cpp does? Giving BlockManager a CChainParams member reference and having CChainState and ChainstateManager reference it via their BlockManager references, and also removing the Consensus::Params and CChainParams params from bunches of functions (including the DeploymentActive.. ones) seems like it would be a win.

Yes I think as part of some other work I’ll have to pull validation apart anyway, I will look into this!

ajtowns commented at 5:02 am on September 15, 2021:

This looks good to me, and it seems like they would eventually end up having to interact with BlockManager anyway according to the following?

Well, using the cb wit nonce for activation signalling might never happen, and even if it does, I think it’s a fair way off (since it would need getblocktemplate changes or stratumv2 or similar to make it even possible), so I wouldn’t put too much weight on it, but it seems plausible.

ajtowns commented at 2:54 pm on March 17, 2022:

I think this is dealt with by #24595

in src/validation.cpp:4154 in 4d00171fc5 outdated

4106@@ -4106,7 +4107,6 @@ void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman)
4107     LOCK(cs_main);
4108     chainman.Unload();
4109     if (mempool) mempool->clear();
4110-    g_versionbitscache.Clear();

ajtowns commented at 1:06 pm on August 1, 2021:

Note that there shouldn’t be any logical need for this to be cleared here – the cached information doesn’t become invalid when we load up different blocks, only if the consensus parameters are changed.

dongcarl commented at 7:29 pm on August 24, 2021:

Is this the same case for warningcache as well? If so, perhaps we’ll just move the clearing logic to SelectParams and not move these two structures into BlockManager?

ajtowns commented at 3:04 am on September 15, 2021:

Yes, warningcache only relies on the chain history, consensus params, and versionbitscache state. Moving the clearing logic to SelectParams would introduce a circular dependency though, I think?

ajtowns commented at 7:00 am on January 10, 2022:

Here’s a rebase of this that replaces moving g_versionbits with just removing g_versionbitscache.Clear() from UnloadBlockIndex – https://github.com/ajtowns/bitcoin/commits/202201-vb-unloadblockindex … What do you think about doing that instead? I’ve got some updates for the versionbits code I’d like to PR, and would prefer to minimise conflicts with this one…

dongcarl commented at 7:36 pm on January 10, 2022:

If warningcache and g_versionbitscache are both only dependent on consensus parameters, then perhaps (perhaps not in this PR) we should move them to ChainstateManager or NodeContext? At the very least we would need to clear them in ~ChainTestingSetup so that tests with different consensus params won’t get confused.

ajtowns commented at 5:14 am on January 11, 2022:

Moving them somewhere rather than having them be globals would be good eventually; I had a look at moving them into ChainstateManager but it felt a bit too invasive when what I was hoping to do was avoid/defer introducing conflicts…

The patch above clears g_versionbitscache in the BasicTestingSetup constructor instead (where SelectParams is invoked), which I think should be good enough? It’s also already cleared explicitly in versionbits_tests.cpp which manually changes the consensus params being used.

ajtowns commented at 9:04 pm on April 14, 2022:

#24595 moves g_versionbitscache into ChainstateManager and builds on this PR, so hopefully resolves this thread

in src/validation.cpp:4110 in 6b0a3bc7bd outdated

4106@@ -4107,8 +4107,8 @@ void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman)
4107     LOCK(cs_main);
4108     chainman.Unload();
4109     if (mempool) mempool->clear();
4110-    for (int b = 0; b < VERSIONBITS_NUM_BITS; b++) {
4111-        warningcache[b].clear();
4112+    for (auto i : warningcache) {

ajtowns commented at 1:44 pm on August 1, 2021:

Should be auto& i : warningcache ??

in src/validation.h:111 in 6a897c0601 outdated

106@@ -107,7 +107,12 @@ enum class SynchronizationState {
107 };
108 
109 extern RecursiveMutex cs_main;
110-typedef std::unordered_map<uint256, CBlockIndex*, BlockHasher> BlockMap;
111+
112+// If we ever switch to another associative container, we need to either use a

ryanofsky commented at 7:46 pm on August 23, 2021:

In commit “No more heap BlockIndices” (6a897c0601dff4f116b1ab223595c5c2fac0e7a3)

Comment seems to lack a little context, maybe prefix with “Because validation code takes pointers to the map’s CBlockIndex objects, if we ever switch…”

ryanofsky commented at 7:51 pm on August 23, 2021: member

This PR generally looks pretty good and it would be nice to see an update.

re: #22564#issue-698150124

In particular, I’d love to get some feedback on 6a897c0, since it changes the key type of BlockMap from CBlockIndex* to CBlockIndex

This seems like an improvement in every respect: removing a level of indirection, removing need for manual deletions, avoiding pointer bugs. No downsides are apparent to me at least.

dongcarl force-pushed on Aug 24, 2021

dongcarl commented at 5:57 pm on August 24, 2021: member

Pushed f741623c25 -> 6e6472e6dc

Address #22564 (review)
Address #22564 (review)

dongcarl force-pushed on Aug 24, 2021

dongcarl commented at 6:45 pm on August 24, 2021: member

Pushed 6e6472e6dc -> 6bb9ed3f62:

Rebased on master

in src/validation.h:429 in 9fdf6825bc outdated

426@@ -430,6 +427,11 @@ class BlockManager
427       */
428     std::set<CBlockIndex*> m_failed_blocks;
429 
430+    CBlockIndex* pindexBestInvalid = nullptr;

ryanofsky commented at 2:42 pm on August 25, 2021:

In commit “Move pindexBest{Header,Invalid} to BlockManager” (9fdf6825bc6a3a27b23b7a1015afc33b3a62effd)

Is there a reason these are moved to BlockManager class instead of ChainstateManager class? I don’t see anywhere where the blockmanager class itself is accessing these members, and I don’t see any references to this through any blockmanager reference except the ChainStateManager::m_blockman one. These are also set from chain state not block state, so it seems like these are more related to chain state functionality than in block manager. Maybe ideally it would be a would be a private ChainStateManager member?

I’m not sure about this and maybe it doesn’t matter, but it would be good to at least hint at what the thinking is behind the move destination is in the commit description.

dongcarl commented at 4:59 pm on August 25, 2021:

I believe they are accessed by BlockManager here: https://github.com/bitcoin/bitcoin/blob/21438d55d553ae5bf3be7c0d4431aaf136db6c6b/src/validation.cpp#L3732-L3737

Additionally (and I’d like feedback on if this makes sense), it seems to me that variables whose type involve CBlockIndex* should likely live an die with the BlockManager since it owns the BlockMap entries to which these pointers are pointing.

ryanofsky commented at 5:41 pm on August 25, 2021:

I believe they are accessed by BlockManager here:

https://github.com/bitcoin/bitcoin/blob/21438d55d553ae5bf3be7c0d4431aaf136db6c6b/src/validation.cpp#L3732-L3737

Additionally (and I’d like feedback on if this makes sense), it seems to me that variables whose type involve CBlockIndex* should likely live an die with the BlockManager since it owns the BlockMap entries to which these pointers are pointing.

Regardless of what should happen with these two variables I would definitely push back against this logic. CBlockIndex pointers are used in all kinds of code that has nothing to do with the actual block index, just as convenient and easy way to refer to blocks. If you tried to move all CBlockIndex type variables into BlockManager, you would be moving validation logic and indexing logic (and even wallet logic prior to #10973) into the BlockManager class where it pretty clearly does not belong.

I think class state should generally be located in the class accessing and updating that state. It doesn’t usually make sense for one class to store state on behalf on another class that it never accesses or uses itself. And probably ideally, all of these classes would have private state that would not be directly manipulated by other classes at all.

ryanofsky commented at 5:58 pm on August 25, 2021:

Looking at your link more it does make some sense to me to keep these variables here if they can be derived directly from block index data like that.

The boundaries are definitely a little blurry here and in longer run I’d be inclined to want to move this logic out of the Block Manager class so it is just responsible storing the index data and not interpreting it so much. In short run I would add additional CBlockIndex*&best_header and CBlockIndex*&best_invalid_header output parameters so the LoadBlockIndex can keep the same logic and just return the information.

But now that I see what motivated moving these variables here, I don’t think it’s crazy to keep the variables here either. So you should do what seems best to you, and thanks for clarifying!

jamesob commented at 11:06 pm on September 20, 2021:

Not at all blocking and could be done in a separate commit, but might be nice to add a docstring describing purpose and usage of this member while all this is fresh in your head.

dongcarl commented at 1:40 am on October 7, 2021:

Good idea, added in e78b6e5105

in src/node/blockstorage.h:76 in feb62ff2a0 outdated

72@@ -73,9 +73,9 @@ bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, c
73 bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start);
74 
75 bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex);
76-bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams);
77+// bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams);

ryanofsky commented at 2:59 pm on August 25, 2021:

In commit “Move setDirty{BlockIndex,FileInfo} to BlockManager” (feb62ff2a0af6fedfaa3078b1a512416cdf71264)

Should just delete these old function declarations (now that they are member functions)

dongcarl commented at 6:07 pm on September 14, 2021:

Deleted as of d849178b9c36de80cded34a5305236282d7b24d6

in src/validation.cpp:3748 in feb62ff2a0 outdated

3743@@ -3746,6 +3744,9 @@ void BlockManager::Unload() {
3744 
3745     pindexBestInvalid = nullptr;
3746     pindexBestHeader = nullptr;
3747+
3748+    setDirtyBlockIndex.clear();

ryanofsky commented at 3:05 pm on August 25, 2021:

In commit “Move setDirty{BlockIndex,FileInfo} to BlockManager” (feb62ff2a0af6fedfaa3078b1a512416cdf71264)

Can these clears which are moved from UnloadBlockIndex to BlockManager::Unload be done in a different commit before all the other changes in this commit? It’s confusing because UnloadBlockIndex isn’t even calling BlockManager::Unload directly, but indirectly through the ChainstateManager::Unload method. Another reason it would be nice to have the change in a standalone commit would be to describe what is motivating the move.

Additionally, maybe separately it would be good to flatten the existing call chain here, because it doesn’t seem to make much sense outwardly and seems unnecessarily complex. It seems like ChainstateManager::Unload is only called one place and could be removed and inlined. Also CChainState::UnloadBlockIndex looks like it’s only called one place and could be removed and inlined. Another reason it would be great to drop that one is the confusing name clash between the member function and the global function.

in src/validation.h:435 in feb62ff2a0 outdated

431@@ -432,6 +432,12 @@ class BlockManager
432     /** Best header we've seen so far (used for getheaders queries' starting points). */
433     CBlockIndex *pindexBestHeader = nullptr;
434 
435+    /** Dirty block index entries. */
436+    std::set<CBlockIndex*> setDirtyBlockIndex;

ryanofsky commented at 3:32 pm on August 25, 2021:

In commit “Move setDirty{BlockIndex,FileInfo} to BlockManager” (feb62ff2a0af6fedfaa3078b1a512416cdf71264)

Moving these variables to BlockManager makes more sense to me than moving pIndexBest variables in the prior commit, but it would helpful to say why this move destination makes sense in the commit description. Maybe “Move these variables to block manager because they are used to update the block index on disk. In the future it might make sense move more logic responsible for updating and writing the block index to the block manager class and make these variables private instead of public.”

dongcarl commented at 7:41 pm on September 14, 2021:

Description added to fa455c0339561ca19a3edd6b249be962ae87194d

ryanofsky commented at 3:51 pm on August 25, 2021: member

Started review (will update list below with progress).

9fdf6825bc6a3a27b23b7a1015afc33b3a62effd Move pindexBest{Header,Invalid} to BlockManager (1/12)
feb62ff2a0af6fedfaa3078b1a512416cdf71264 Move setDirty{BlockIndex,FileInfo} to BlockManager (2/12)
ccd6ec5e620dbb3ae251b3511d00aaf3614e97da validation: Add missing cs_LastBlockFile locks in PruneAndFlush() and UnloadBlockIndex(). Add missing locking annotation for nLastBlockFile and fCheckForPruning. (3/12)
7c3f21a5c16e83ef6baacbcd600d15a470ff1e5b validation: Add missing cs_nBlockSequenceId lock in UnloadBlockIndex(). Add missing locking annotation for nBlockSequenceId. (4/12)
ca77cd53abb455847094cecdc11b217c64166de8 Move cs_LastBlockFile guarded objects to BlockManager (5/12)
b40f35d7f0507fcfb71d1c950c5158d6ff5fdb5e Move fHavePruned to BlockManager (6/12)
3eb20479f23f0280576c374c76527c06c587cd86 Move versionbitscache to BlockManager (7/12)
711399647e52dfb7354d9e3ec79989fefe1925dc warningcache: C-array to std::array (8/12)
b267bc14594c63123dc7e8c5c4cce5a26b313f10 Move warningcache to BlockManager (9/12)
1806d68d0bb62582457d3936c218d29a95e53f66 No more heap BlockIndices (10/12)
8a30b3dd248bee78bca6f8ae0a7902f34e0cd58e No more heap, no mo weirdneess (11/12)
6bb9ed3f62b99f14730cc469e9fb651d0739448d Remove UnloadBlockIndex, ~ChainstateManager (12/12)

in src/node/blockstorage.cpp:30 in ccd6ec5e62 outdated

27@@ -28,12 +28,12 @@ uint64_t nPruneTarget = 0;
28 // TODO make namespace {
29 RecursiveMutex cs_LastBlockFile;
30 std::vector<CBlockFileInfo> vinfoBlockFile;

ryanofsky commented at 5:13 pm on August 26, 2021:

In commit “validation: Add missing cs_LastBlockFile locks in PruneAndFlush() and UnloadBlockIndex(). Add missing locking annotation for nLastBlockFile and fCheckForPruning.” (ccd6ec5e620dbb3ae251b3511d00aaf3614e97da)

I think GUARDED_BY(cs_LastBlockFile) should be added here to the vinfoBlockFile variable as well. The “last block” mutex name is confusing, but I believe the naming only ended up that way because the vinfo variable was added in a later commit ed6d1a2c7be9d046d5e78330d5e163023a6b302a than the last block variables 5382bcf8cd23c36a435c29080770a79b5e28af42. From the code it looks pretty clearly like the mutex is meant to cover all the variables, and it seems to be always locked when accessing them (except during unloading/loading where other locks have been elided as well).

dongcarl commented at 7:27 pm on September 14, 2021:

vinfo guarded in: 952482e8598c9bd38a7a4e382c0c791acda11ff4 cs_LastBlockFile renamed in: 71bacd9265f66f4ffc94f551dcdb9fc64e673fb9

in src/validation.h:576 in 7c3f21a5c1 outdated

572@@ -573,7 +573,7 @@ class CChainState
573      */
574     RecursiveMutex cs_nBlockSequenceId;
575     /** Blocks loaded from disk are assigned id 0, so start the counter at 1. */
576-    int32_t nBlockSequenceId = 1;
577+    int32_t nBlockSequenceId GUARDED_BY(cs_nBlockSequenceId) = 1;

ryanofsky commented at 4:35 pm on August 27, 2021:

In commit “validation: Add missing cs_nBlockSequenceId lock in UnloadBlockIndex(). Add missing locking annotation for nBlockSequenceId.” (7c3f21a5c16e83ef6baacbcd600d15a470ff1e5b)

Two notes for future improvements:

Looking at actual nBlockSequenceId uses, it is really just used as an atomic, and could be replaced with an atomic and the mutex could be eliminated with no change in behavior.
nBlockReverseSequenceId below should have GUARDED_BY(cs_main)

dongcarl commented at 6:09 pm on September 14, 2021:

First note done in master, second note done by: b903d3b682bc1a779f924fb9da778ec295ab017b

in src/node/blockstorage.h:59 in ca77cd53ab outdated

54@@ -55,11 +55,11 @@ FILE* OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false);
55 /** Translation to a filesystem path */
56 fs::path GetBlockPosFilename(const FlatFilePos& pos);
57 
58-/** Get block file info entry for one block file */
59-CBlockFileInfo* GetBlockFileInfo(size_t n);
60+// /** Get block file info entry for one block file */
61+// CBlockFileInfo* GetBlockFileInfo(size_t n);

ryanofsky commented at 4:50 pm on August 27, 2021:

In commit “Move cs_LastBlockFile guarded objects to BlockManager” (ca77cd53abb455847094cecdc11b217c64166de8)

Should fully remove this declaration and the one below

dongcarl commented at 6:09 pm on September 14, 2021:

Removed as of d849178b9c36de80cded34a5305236282d7b24d6

dongcarl commented at 6:09 pm on September 14, 2021:

Removed as of d849178b9c36de80cded34a5305236282d7b24d6

in src/test/validation_chainstate_tests.cpp:48 in ca77cd53ab outdated

37@@ -39,11 +38,14 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)
38     CChainState& c1 = WITH_LOCK(cs_main, return manager.InitializeChainstate(&mempool));
39     c1.InitCoinsDB(
40         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
41-    WITH_LOCK(::cs_main, c1.InitCoinsCache(1 << 23));
42 
43-    // Add a coin to the in-memory cache, upsize once, then downsize.

ryanofsky commented at 4:55 pm on August 27, 2021:

In commit “Move cs_LastBlockFile guarded objects to BlockManager” (ca77cd53abb455847094cecdc11b217c64166de8)

It would seem better to keep this comment which is still summarizing behavior of this block of code.

dongcarl commented at 6:10 pm on September 14, 2021:

Fixed in 8fd345b98f8b8cb81a2c5c9e227405364d7b46a2

in src/wallet/test/wallet_tests.cpp:89 in ca77cd53ab outdated

83@@ -84,7 +84,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
84 {
85     // Cap last block file size, and mine new block in a new block file.
86     CBlockIndex* oldTip = m_node.chainman->ActiveChain().Tip();
87-    GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE;
88+    (*WITH_LOCK(::cs_main, return &m_node.chainman->m_blockman)).GetBlockFileInfo(oldTip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE;

ryanofsky commented at 5:07 pm on August 27, 2021:

In commit “Move cs_LastBlockFile guarded objects to BlockManager” (ca77cd53abb455847094cecdc11b217c64166de8)

I guess this is a preexisting thing better to address in a different PR, but it seems wrong if cs_main is required to get a pointer to blockman from the chainman. I didn’t look but ideally the blockman pointer would be set once in the chainman constructor and could just be const. Or even if it could not be const for some reason, it could be atomic and not require a validation lock.

in src/bench/rpc_blockchain.cpp:43 in b40f35d7f0 outdated

39@@ -40,7 +40,7 @@ static void BlockToJsonVerbose(benchmark::Bench& bench)
40 {
41     TestBlockAndIndex data;
42     bench.run([&] {
43-        auto univalue = blockToJSON(data.block, &data.blockindex, &data.blockindex, /*verbose*/ true);
44+        auto univalue = blockToJSON(WITH_LOCK(::cs_main, return std::ref(data.testing_setup->m_node.chainman->m_blockman)), data.block, &data.blockindex, &data.blockindex, /*verbose*/ true);

ryanofsky commented at 5:08 pm on August 30, 2021:

In commit “Move fHavePruned to BlockManager” (b40f35d7f0507fcfb71d1c950c5158d6ff5fdb5e)

This is another place like #22564 (review) where it looks like m_blockman is improperly annotated to require cs_main. In general, if you have a variable

0T variable GUARDED_BY(mutex);

And you are writing WITH_LOCK(mutex, return &variable); you’re just bypassing the annotation, and either the annotation is wrong and shouldn’t be there, or it’s right and you’re introducing a possible race condition when the variable is accessed through the pointer later without the lock held.

It would be nice to drop the m_blockman annotation in this PR because it seems overly broad. Or if it’s easier a comment could be added just to say why the annotation is bypassed here.

dongcarl force-pushed on Sep 13, 2021

dongcarl force-pushed on Sep 14, 2021

jamesob commented at 7:50 pm on September 14, 2021: member

Concept ACK, will review in next few days

in src/node/blockstorage.cpp:219 in fa455c0339 outdated

226@@ -233,7 +227,7 @@ fs::path GetBlockPosFilename(const FlatFilePos& pos)
227     return BlockFileSeq().FileName(pos);
228 }
229 
230-bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, CChain& active_chain, uint64_t nTime, bool fKnown = false)
231+bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, CChain& active_chain, uint64_t nTime, bool fKnown = false)

jamesob commented at 11:59 pm on September 20, 2021:

https://github.com/bitcoin/bitcoin/pull/22564/commits/fa455c0339561ca19a3edd6b249be962ae87194d

Might mention these changes somewhere in the commit message; I was surprised to see them here after reading it.

dongcarl commented at 1:41 am on October 7, 2021:

True! Fixed in f99156b575!

in src/versionbits.h:54 in 17f227f6d4 outdated

50@@ -51,6 +51,8 @@ struct BIP9Stats {
51     bool possible;
52 };
53 
54+class VersionBitsCache;

jamesob commented at 6:34 pm on September 27, 2021:

17f227f6d44bb417d0c6540e081d4d374bece6a7

I think this forward declaration is probably outdated and doesn’t seem necessary anymore - I was able to compile without it.

dongcarl commented at 1:41 am on October 7, 2021:

Done! f5ad3509dc

in src/validation.cpp:2806 in b48d7c010f outdated

2799@@ -2800,20 +2800,21 @@ CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block)
2800     uint256 hash = block.GetHash();
2801     BlockMap::iterator it = m_block_index.find(hash);
2802     if (it != m_block_index.end())
2803-        return it->second;
2804+        return &it->second;
2805 
2806     // Construct new block index object
2807-    CBlockIndex* pindexNew = new CBlockIndex(block);
2808+    CBlockIndex neue(block);

jamesob commented at 7:37 pm on September 27, 2021:

https://github.com/bitcoin/bitcoin/pull/22564/commits/b48d7c010f644ed01e310ea4ab590580640a16b7

Took me awhile to parse this name. I’m not opposed, it’s kind of cute, but is it something we’ve used before? Maybe newIndex?

dongcarl commented at 1:41 am on October 7, 2021:

Hehe that’s an artifact from me experimenting. Changed in 5bab523f31!

in src/validation.cpp:3616 in b48d7c010f outdated

3577@@ -3576,9 +3578,9 @@ bool BlockManager::LoadBlockIndex(
3578     // Calculate nChainWork
3579     std::vector<std::pair<int, CBlockIndex*> > vSortedByHeight;
3580     vSortedByHeight.reserve(m_block_index.size());
3581-    for (const std::pair<const uint256, CBlockIndex*>& item : m_block_index)
3582+    for (std::pair<const uint256, CBlockIndex>& item : m_block_index)

jamesob commented at 7:54 pm on September 27, 2021:

https://github.com/bitcoin/bitcoin/pull/22564/commits/b48d7c010f644ed01e310ea4ab590580640a16b7

I’m not so worried about this function in particular since it’s really only executed once on startup, but I’ve got to imagine there may be some performance implications if we’re potentially introducing a lot of CBlockIndex value copying (vs. the pointers).

This does strike me as a pretty big change, especially for a PR that’s nominally about deglobalization. I’m not opposed to including it here, but I just want to make sure that we’ve thought through all the implications and have made sure that this change is really only as significant as inclusion in a semi-related PR merits.

Does anyone have any historical notion of why we didn’t have the block index hold values in the first place? For what it’s worth, I’m definitely concept ACK on reducing CBlockIndex* usage.

dongcarl commented at 7:48 pm on October 6, 2021:

we’re potentially introducing a lot of CBlockIndex value copying (vs. the pointers)

I don’t think we’re introducing value copying because we’re iterating with std::pair<...>&, but perhaps there’s something I’m missing? When I asked him about it, ryanofsky also pointed out that the const reference was actually more dangerous since it could have created copies through implicit conversion.

in src/init.cpp:1215 in 3de7842f50 outdated

1210@@ -1211,114 +1211,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1211     assert(!node.connman);
1212     node.connman = std::make_unique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), *node.addrman, args.GetBoolArg("-networkactive", true));
1213 
1214+    // ********************************************************* Step 7: load block chain

jamesob commented at 2:19 pm on September 28, 2021:

https://github.com/bitcoin/bitcoin/pull/22564/commits/3de7842f50c44e6bed675048737e19a1d8ca5fa4

This is most easily reviewed with --color-move=dimmed_zebra.

Given that init reordering is notoriously tricky, we should probably be sure to test invocation of bitcoind with each flag that was moved here.

dongcarl commented at 7:48 pm on October 6, 2021:

Yes, I think I’m going to work a bit more on this commit to see if I can make it more easily reviewable.

jamesob commented at 2:37 pm on September 28, 2021: member

Took a read through this locally and built it. In general the change looks great; it should make reusing init logic from within unittests more tractable. Given the precarious and complicated nature of the init stuff, that’s a very good thing.

This changeset is largely in good shape. The commits need to be cleaned up slightly and I think we need to be sure the heap-to-stack block index change is as harmless a change as it seems. I’d also like to do more comprehensive testing on the init changes. But in general I think this is in good shape and should be moved out of draft soon.

3ea18162a4 Move pindexBest{Header,Invalid} to BlockManager
fa455c0339 Move setDirty{BlockIndex,FileInfo} to BlockManager
a5d50a092b validation: Add missing cs_LastBlockFile locks in PruneAndFlush() and UnloadBlockIndex(). Add missing locking annotation for nLastBlockFile and fCheckForPruning.
952482e859 Guard vinfoBlockFile with cs_LastBlockFile as well
- You may have already intended to do this, but you can probably absorb this into the previous commit.
81f0828ade Move cs_LastBlockFile guarded objects to BlockManager
c2ffc0c660 validation: Guard nBlockReverseSequenceId with cs_main
024f19f8ef Move fHavePruned to BlockManager
17f227f6d4 Move versionbitscache to BlockManager
318d8ffdf1 warningcache: C-array to std::array
9ab4a88eda Move warningcache to BlockManager
- Can probably be squashed into prev commit.
b48d7c010f No more heap BlockIndices
f4e61fac35 No more heap, no mo weirdneess
3de7842f50 init: Reset mempool and chainman via reconstruction
913b47cb42 validation: Prune UnloadBlockIndex and callees
- Love this commit.
71bacd9265 scripted-diff: Rename cs_LastBlockFile to cs_blockfiles

jamesob commented at 5:20 pm on September 29, 2021: member

For what it’s worth, I benched this and there don’t seem to be any significant performance implications to moving block index entries from CBlockIndex pointers to values.

bench name	command
ibd.local.range.500000.540000	`bitcoind -dbcache=10000 -debug=coindb -debug=bench -listen=0 -connect=0 -addnode=127.0.0.1:8888 -prune=9999999 -printtoconsole=0 -assumevalid=000000000000000000176c192f42ad13ab159fdb20198b87e7ba3c001e47b876`

#22564 vs. $mergebase (absolute)

bench name	x	#22564	$mergebase
ibd.local.range.500000.540000.total_secs	2	3122.0459 (± 10.1604)	3127.1493 (± 4.1510)
ibd.local.range.500000.540000.peak_rss_KiB	2	6345706.0000 (± 3578.0000)	6356022.0000 (± 2906.0000)
ibd.local.range.500000.540000.cpu_kernel_secs	2	264.0400 (± 0.7700)	266.6600 (± 1.8000)
ibd.local.range.500000.540000.cpu_user_secs	2	13185.5400 (± 4.5500)	13150.5350 (± 14.1950)

#22564 vs. $mergebase (relative)

bench name	x	#22564	$mergebase
ibd.local.range.500000.540000.total_secs	2	1.00	1.002
ibd.local.range.500000.540000.peak_rss_KiB	2	1.00	1.002
ibd.local.range.500000.540000.cpu_kernel_secs	2	1.00	1.010
ibd.local.range.500000.540000.cpu_user_secs	2	1.00	1.000

dongcarl force-pushed on Oct 7, 2021

dongcarl commented at 1:52 am on October 7, 2021: member

Pushed 71bacd9 -> e78b6e5:

Addressed #22564 (review)
Addressed #22564 (review)
Addressed #22564 (review)
Addressed #22564 (review)

dongcarl force-pushed on Oct 7, 2021

dongcarl commented at 1:59 am on October 7, 2021: member

Pushed e78b6e5 -> 5b72333:

Rebased over master

dongcarl force-pushed on Oct 8, 2021

dongcarl commented at 10:16 pm on October 8, 2021: member

Pushed 5b72333 -> 148bf14

Added CBlockUndo forward declaration
Made intermediate commits comply with lock annotations by adding GUARDED_BY annotations to extern declarations and fixing up code

dongcarl force-pushed on Oct 9, 2021

dongcarl commented at 0:39 am on October 9, 2021: member

Pushed 148bf148e198027b25894df429dd13b9ca9f60a1 -> 0898078ed59453cc850ebe08010e68e6dea51dbd:

Only move the PeerManager construction in “init: Reset mempool and chainman via reconstruction” so that it’s more easily reviewable.

DrahtBot added the label Needs rebase on Dec 13, 2021

dongcarl commented at 5:06 pm on January 5, 2022: member

Going to update this soon, @MarcoFalke would it make sense to rebase this on top of your #23974?

MarcoFalke commented at 6:37 pm on January 5, 2022: member

Oh, apologies I missed that you (partly?) already did #23974 in here. I don’t have a preference on how to proceed. Let me know if I should close my pull or something else.

ryanofsky commented at 7:14 pm on January 5, 2022: member

Oh, apologies I missed that you (partly?) already did #23974 in here. I don’t have a preference on how to proceed. Let me know if I should close my pull or something else.

Oh, I knew this code looked familiar and couldn’t remember where I made these comments before. IMO, #23974 is really easy to understand, and already up to date, and this PR would ultimately be simpler rebased on top of #23974. But I’m not sure if it would significant extra work to do that, so I don’t really have an informed opinion about merge order. Will ACK #23974 in any case

fanquake commented at 3:19 am on January 7, 2022: member

#23974 has been merged.

dongcarl force-pushed on Jan 7, 2022

dongcarl commented at 7:59 pm on January 7, 2022: member

Rebased over #23974, more changes to come.

While rebasing, I noticed a few things:

Recent changes has made it such that CChainState holds a reference to ChainstateManager as m_chainman, and that BlockManager::LoadBlockIndex takes in a ChainstateManager parameter. It seems that there’s a lot of reaching up and down the ownership hierarchy, and I’m wondering if I’m missing some underlying logic/structure behind all of this.
The recent introduction of the chainstatemanager_loadblockindex and its call to CChainState::UnloadBlockIndex has made it such that I cannot prune that method. If we examine CChainState::UnloadBlockIndex closely, we can also observe that it does not affect the block index at all, but only CChainState’s members which relate to the block index. Perhaps something should be done here, but I’m not exactly sure what yet.

DrahtBot removed the label Needs rebase on Jan 7, 2022

dongcarl force-pushed on Jan 7, 2022

MarcoFalke commented at 8:18 am on January 8, 2022: member

Recent changes has made it such that CChainState holds a reference to ChainstateManager as m_chainman, and that BlockManager::LoadBlockIndex takes in a ChainstateManager parameter. It seems that there’s a lot of reaching up and down the ownership hierarchy, and I’m wondering if I’m missing some underlying logic/structure behind all of this.

I haven’t made the CChainState::m_chainman, but I think there is a comment saying that the “clean” alternative required more code changes.

BlockManager::LoadBlockIndex takes in a ChainstateManager parameter to set the “best invalid” pointer. An alternative could be to pass in the mutable pointer only. It is also doing a check on chainstates, which I believe can be replaced by passing in a const bool: #23174 (review)

in src/node/chainstate.cpp:15 in fd2cdbc54e outdated

11@@ -12,6 +12,7 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
12                                                      ChainstateManager& chainman,
13                                                      CTxMemPool* mempool,
14                                                      bool fPruneMode,
15+

MarcoFalke commented at 8:21 am on January 8, 2022:

in commit fd2cdbc54efa02f51577c833288961254d5b5053: :eye:

DrahtBot added the label Needs rebase on Jan 8, 2022

MarcoFalke commented at 7:28 am on January 10, 2022: member

nit: s/BlockManager/ChainstateManger/ in the first commit title

MarcoFalke commented at 9:27 am on January 10, 2022: member

At some point it could also make sense to move functions to BlockManager that don’t read any member vars of it, but logically belong to it. For example, CleanupBlockRevFiles…

MarcoFalke commented at 10:25 am on January 11, 2022: member

I am more than happy to work on #22564 (comment) myself. Just wanted to ping here first to see if you worry about conflicts.

Also, I think this pull can be split up. 14 commits is a bit large, especially given that some are “basically ready” for months now and others (versionbits) are still up for debate/details.

dongcarl commented at 6:31 pm on January 12, 2022: member

Good idea, I will split it up and PR separately!

dongcarl force-pushed on Feb 23, 2022

dongcarl renamed this:
~~[WIP] refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager`~~
refactor: Move mutable globals cleared in `::UnloadBlockIndex` to `BlockManager`
on Feb 23, 2022

dongcarl commented at 8:56 pm on February 23, 2022: member

Updated to be based on #24050

Edit: Just realized I forgot the re-add my rebase of #15191, will do.

dongcarl force-pushed on Feb 23, 2022

DrahtBot removed the label Needs rebase on Feb 23, 2022

MarcoFalke referenced this in commit 5e49b2a252 on Mar 7, 2022

DrahtBot added the label Needs rebase on Mar 7, 2022

dongcarl force-pushed on Mar 7, 2022

dongcarl marked this as ready for review on Mar 7, 2022

dongcarl commented at 2:14 pm on March 7, 2022: member

Pushed 40305c228f1582d31530437f77db22a4c163ff69 -> dc57ee310615d0d63e23e5f391593705141ccc29

Rebased over master
Forgot to mention in a previous update: I split up the move-mostly commits and the commits where I move the clearing of variables so that it’s easier for reviewers

in src/node/blockstorage.h:169 in 0e4b6541d2 outdated

164@@ -165,6 +165,9 @@ class BlockManager
165     //! Returns last CBlockIndex* that is a checkpoint
166     const CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
167 
168+    /** Best header we've seen so far (used for getheaders queries' starting points). */
169+    CBlockIndex* pindexBestHeader = nullptr;

MarcoFalke commented at 2:24 pm on March 7, 2022:

Is there any reason to move this to blockstorage? It is never used there

My suggestion would be to move it to chainman. Also, there could be a follow-up scripted diff to rename the variable after the move to m_....

dongcarl commented at 2:31 pm on March 7, 2022:

I think it’s used in BlockManager::AddToBlockIndex and BlockManager::LoadBlockIndex, see previous comment here: #22564 (review)

Resolve the blurry boundary seems somewhat non-trivial and I don’t think would be within the scope of this PR, but we can certainly add a TODO comment so that this doesn’t get lost to time? Let me know what you think is best!

MarcoFalke commented at 2:40 pm on March 7, 2022:

LoadBlockIndex already has a ChainstateManager reference, so this shouldn’t cause any issues. See also fab6d6b2d154893ab422dda87f3535d42c3e06f4

How hard would it be to do the same for AddToBlockIndex?

dongcarl commented at 2:49 am on March 8, 2022:

I’m not sure that reaching up to ChainstateManager when loading the block index is the best here… Preferably we can split the “load block index itself” logic from “derive Chainstate variables from loaded block index” logic. Doing so would allow us to keep pindexBestHeader in ChainstateManager as you suggested (and I think is the right place for it to be in the long run).

Here’s a commit that does this: https://github.com/dongcarl/bitcoin/commit/41b61f245f1f115de4625fda40d8898cb6b2d3ba

Branch: https://github.com/dongcarl/bitcoin/commits/2022-03-kirby-p2.5

Lmk what you think!

MarcoFalke commented at 9:02 am on March 8, 2022:

Yeah, looks fine long-term. Might want to chat with @jamesob to discuss conflicts.

I just prefer a (temporary) ChainMan reference to be passed down and have the member in ChainMan, instead of (temporarily) putting it in BlockMan. Though, as both are temporary, either is probably fine.

jamesob commented at 5:25 pm on March 9, 2022:

No trouble doing this move in terms of planned assumeutxo changes, and certainly having it under blockman is better than prior to this changeset.

I agree some of the “deep-dot” accesses are clunky (probably the necessary result of script-diffs) and there are a few instances where pindexBestHeader is accessed like chainstate.chainman.blockman.pindexbestheader when it could be chainstate.blockman.pindexbestheader, again probably just due to scripted-diffs. If we wanted to ease some of the other uses, we could just have a ChainstateManager::bestIndex method that returned from its blockmanager.

dongcarl commented at 5:54 pm on March 9, 2022:

I think what we’re thinking is doing the changes in https://github.com/dongcarl/bitcoin/commits/2022-03-kirby-p2.5, and then making pindexBestHeader a ChainstateManager member instead of a BlockManager one (this current PR makes it a BlockManager member). Lmk if that sounds alright @jamesob

jamesob commented at 6:46 pm on March 9, 2022:

@dongcarl sounds good to me!

MarcoFalke approved

MarcoFalke commented at 2:33 pm on March 7, 2022: member

crACK

DrahtBot removed the label Needs rebase on Mar 7, 2022

MarcoFalke removed the label Wallet on Mar 7, 2022

MarcoFalke removed the label RPC/REST/ZMQ on Mar 7, 2022

MarcoFalke removed the label P2P on Mar 7, 2022

MarcoFalke removed the label Mining on Mar 7, 2022

MarcoFalke removed the label Validation on Mar 7, 2022

MarcoFalke removed the label Consensus on Mar 7, 2022

MarcoFalke removed the label Block storage on Mar 7, 2022

MarcoFalke added the label Refactoring on Mar 7, 2022

dongcarl force-pushed on Mar 9, 2022

dongcarl commented at 11:25 pm on March 9, 2022: member

Pushed dc57ee310615d0d63e23e5f391593705141ccc29 -> 0cc5b02bd3b3e9a179b894fad60af639fd370d87

This PR is now based over #24515
Moved pindexBestHeader to ChainstateManager instead

I gotta say that I’m not entirely happy that having pindexBestHeader in ChainstateManager means:

We gotta pass in the pindexBestHeader in BlockManager::AddBlockToIndex here: https://github.com/bitcoin/bitcoin/pull/22564/commits/4f80756c338970fb7d323459e2a4a40850f77f3e#diff-114c2880ec1ff2c5293ac65ceda0637bf92c05745b74b58410585a549464a33fR49
We gotta add an additional place where we reach up from CChainState to ChainstateManager to get the pindexBestHeader here: https://github.com/bitcoin/bitcoin/pull/22564/commits/4f80756c338970fb7d323459e2a4a40850f77f3e#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R4193

But I don’t think we can fix this without some clearer definition of what belongs where and some CChainState vs ChainstateManager disambiguation, so I think we’re gucci for now.

dongcarl marked this as a draft on Mar 9, 2022

MarcoFalke commented at 10:28 am on March 10, 2022: member

We gotta pass in the pindexBestHeader in BlockManager::AddBlockToIndex here: https://github.com/bitcoin/bitcoin/commit/4f80756c338970fb7d323459e2a4a40850f77f3e#diff-114c2880ec1ff2c5293ac65ceda0637bf92c05745b74b58410585a549464a33fR49

An alternative would be to pass in a post-process lambda that is only run when a block was inserted

DrahtBot added the label Needs rebase on Mar 17, 2022

dongcarl force-pushed on Mar 17, 2022

DrahtBot removed the label Needs rebase on Mar 17, 2022

dongcarl commented at 10:26 pm on March 17, 2022: member

Pushed 0cc5b02bd3b3e9a179b894fad60af639fd370d87 -> 3227a67e2d987953f71b3a2c9dae508fcc1f1a44

Rebased after merge of #24515

dongcarl marked this as ready for review on Mar 17, 2022

in src/validation.cpp:4143 in a9552a094d outdated

4138@@ -4140,6 +4139,8 @@ bool ChainstateManager::LoadBlockIndex()
4139             if (pindex->nStatus & BLOCK_FAILED_MASK && (!m_best_invalid || pindex->nChainWork > m_best_invalid->nChainWork)) {
4140                 m_best_invalid = pindex;
4141             }
4142+            if (pindex->IsValid(BLOCK_VALID_TREE) && (pindexBestHeader == nullptr || CBlockIndexWorkComparator()(pindexBestHeader, pindex)))
4143+                pindexBestHeader = pindex;

MarcoFalke commented at 8:38 am on March 18, 2022:

first commit: Looks like this hunk can be moved earlier in a separate commit? Seems mostly unrelated to changing the namespace of pindexBestHeader

This would also simplify review.

dongcarl commented at 4:48 pm on March 18, 2022:

Done 568181625c52568f0af2a2b3c46d636480fc6a67

in src/rpc/blockchain.cpp:1210 in a9552a094d outdated

1206@@ -1207,7 +1207,7 @@ RPCHelpMan getblockchaininfo()
1207     UniValue obj(UniValue::VOBJ);
1208     obj.pushKV("chain",                 Params().NetworkIDString());
1209     obj.pushKV("blocks",                height);
1210-    obj.pushKV("headers",               pindexBestHeader ? pindexBestHeader->nHeight : -1);
1211+    obj.pushKV("headers",               chainman.pindexBestHeader ? chainman.pindexBestHeader->nHeight : -1);

MarcoFalke commented at 9:08 am on March 18, 2022:

first commit: Any reason to violate clang-format on this line? I think the excessive and useless whitespace can be removed while touching this line. In line with CalculateCurrentUsage below.

(feel free to ignore, ofc)

dongcarl commented at 4:48 pm on March 18, 2022:

Done: a4d9c6fd64e1d3222e47b8aae162d472957f2d47, 65ac133803386fb19498d3c0fc5f8b27051665af

in src/node/interfaces.cpp:215 in a9552a094d outdated

211@@ -212,9 +212,10 @@ class NodeImpl : public Node
212     bool getHeaderTip(int& height, int64_t& block_time) override
213     {
214         LOCK(::cs_main);
215-        if (::pindexBestHeader) {
216-            height = ::pindexBestHeader->nHeight;
217-            block_time = ::pindexBestHeader->GetBlockTime();
218+        auto best_header = chainman().pindexBestHeader;

MarcoFalke commented at 9:10 am on March 18, 2022:

first commit: You are touching all lines where pindexBestHeader is used. It seems a scripted-diff can be appended to rename it to m_best_header without causing additional merge conflicts.

dongcarl commented at 4:48 pm on March 18, 2022:

Done ddd6bafc16e75e1fb93fd0177f5c5c7c64579095

in src/init.cpp:1400 in b7e44255af outdated

1391@@ -1403,8 +1392,16 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1392     LogPrintf("* Using %.1f MiB for chain state database\n", cache_sizes.coins_db * (1.0 / 1024 / 1024));
1393     LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), nMempoolSizeMax * (1.0 / 1024 / 1024));
1394 
1395+    assert(!node.mempool);
1396+    assert(!node.chainman);
1397     bool fLoaded = false;
1398     while (!fLoaded && !ShutdownRequested()) {
1399+        node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), check_ratio);
1400+        CTxMemPool& mempool = *Assert(node.mempool);

MarcoFalke commented at 9:19 am on March 18, 2022:

in b7e44255af0fbd11f33b5425af9fba8809c2e2d9: Any reason to make mempool a reference? This conflicts with #22850.

It seems probably best to remove this line and leave the other line untouched. This will reduce the diff here and conflicts/code-churn elsewhere.

dongcarl commented at 4:49 pm on March 18, 2022:

Done 276a07f92925c9be82ecd2fe0504323ded1051a1

in src/validation.cpp:5138 in fc13e841aa outdated

5133@@ -5138,3 +5134,15 @@ void ChainstateManager::MaybeRebalanceCaches()
5134         }
5135     }
5136 }
5137+
5138+ChainstateManager::~ChainstateManager() {

MarcoFalke commented at 9:21 am on March 18, 2022:

nit in fc13e841aa2937dfeb067a36f699c7343947cd6d: According to clang-format missing newline?

(feel free to ignore, ofc)

dongcarl commented at 4:49 pm on March 18, 2022:

Done 607d73a8a21bb3b3848b45cbb9844d58e9934c42

MarcoFalke approved

MarcoFalke commented at 9:24 am on March 18, 2022: member

Concept ACK. I have not reviewed the changes, but I like them very much.

Concept ACK 3227a67e2d987953f71b3a2c9dae508fcc1f1a44 🐫

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Concept ACK 3227a67e2d987953f71b3a2c9dae508fcc1f1a44 🐫
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUg/KQv+JXufDJCzVVKUhNwu9b9RCcnUZbc8BmPB/5e5Dm2nIAFFxXRO60omDSnW
 8WWwAXdXtx82bt797pKK7aAsC6HzX65DLLKgbOcxoqSA0CpM895DMN1+cx7qpA8bk
 9HO4YaKd8GliDgJ9Tj/8FeplZnhvbj2XkFhRVwIZBbLfGbBvPU6HsQvaP/YNKbpWf
10QVpaoKv1lyjngn8zqgBXP386tLi8K1Sw6f2ojze3M3DqBxCO8kCuaUaB1s0AjmT4
119VED73T/LREn9YT6qKnTj0Yf1Mnmg1eRbITbH9b7HjNsBF1Yfa1ZJkJCfOzZ8VTx
123m+HwoHprUSSYGuF3SiMmI+fKvUAjZvxBhTw3t1YBjrZ55RmBcVbjqz4RIxc4Etz
136ij8L4Ch8zseLXorZ10vqTfYLqBYr5t4y7fpf501XoP7tKRiTYq57kECyT99w6jV
142f5PZARJf725ZcVNCBanAdpdhJLfAO7CLGr/2kjP5Vx6mKqnGRCCL3UBA0v6lHYZ
156HLBgpqo
16=M7cZ
17-----END PGP SIGNATURE-----

dongcarl force-pushed on Mar 18, 2022

dongcarl commented at 4:47 pm on March 18, 2022: member

Pushed 3227a67e2d987953f71b3a2c9dae508fcc1f1a44 -> ddd6bafc16e75e1fb93fd0177f5c5c7c64579095

Addressed all review comments

in src/validation.cpp:4144 in 568181625c outdated

4139@@ -4140,6 +4140,8 @@ bool ChainstateManager::LoadBlockIndex()
4140             if (pindex->nStatus & BLOCK_FAILED_MASK && (!m_best_invalid || pindex->nChainWork > m_best_invalid->nChainWork)) {
4141                 m_best_invalid = pindex;
4142             }
4143+            if (pindex->IsValid(BLOCK_VALID_TREE) && (pindexBestHeader == nullptr || CBlockIndexWorkComparator()(pindexBestHeader, pindex)))
4144+                pindexBestHeader = pindex;

ryanofsky commented at 9:09 pm on March 29, 2022:

In commit “validation: Load pindexBestHeader in ChainMan” (568181625c52568f0af2a2b3c46d636480fc6a67)

Note: Apparently relationship between these methods is that ChainstateManager::LoadBlockIndex() calls BlockManager::LoadBlockIndexDB() which calls BlockManager::LoadBlockIndex().

Code is moving from inner to outer function, and there is only one call to each of inner functions, so no behavior is changing.

MarcoFalke commented at 10:05 am on April 15, 2022:

Apart from checking that the moved code is called the same number of times, it is also important to check that nothing executed in between the moved code interacts with the moved code.

in src/validation.cpp:5109 in b0b7d756ba outdated

5105@@ -5107,6 +5106,7 @@ void ChainstateManager::Unload()
5106 
5107     m_failed_blocks.clear();
5108     m_blockman.Unload();
5109+    pindexBestHeader = nullptr;

ryanofsky commented at 8:58 pm on April 5, 2022:

In commit “Clear pindexBestHeader in ChainstateManager::Unload()” (b0b7d756ba6f943f3292734f4fa35a16b8cbabde)

Note: This seems safe because because the pindexBestHeader = nullptr call is moving from UnloadBlockIndex function to the ChainstateManager::Unload() method, which is called by UnloadBlockIndex and no other callers.

ryanofsky commented at 9:00 pm on April 5, 2022: member

Started review (will update list below with progress).

568181625c52568f0af2a2b3c46d636480fc6a67 validation: Load pindexBestHeader in ChainMan (1/14)
a4d9c6fd64e1d3222e47b8aae162d472957f2d47 move-mostly: Make pindexBestHeader a ChainMan member (2/14)
65ac133803386fb19498d3c0fc5f8b27051665af style-only: Miscellaneous whitespace changes (3/14)
b0b7d756ba6f943f3292734f4fa35a16b8cbabde Clear pindexBestHeader in ChainstateManager::Unload() (4/14)
7123f3d8af4e592212439968f4eaaec912ec8d02 move-mostly: Make fHavePruned a BlockMan member (5/14)
18d68910bdd4f18eadbf72fa7ec0c0299def3666 Clear fHavePruned in BlockManager::Unload() (6/14)
f1eaec567c289d2188f3bcdcbc002fe011d4224a refactor: Convert warningcache to std::array (7/14)
276a07f92925c9be82ecd2fe0504323ded1051a1 init: Reset mempool and chainman via reconstruction (8/14)
d7373af0599b65c0787c8259ff999b2b8d20a156 style-only: Use for instead of when loading Chainstate (9/14)
c9437752896c15acd20d3809a12bd127e7016747 style-only: Use std::clamp for check_ratio, rename (10/14)
607d73a8a21bb3b3848b45cbb9844d58e9934c42 Clear {versionbits,warning}cache in ~Chainstatemanager (11/14)
0ff795e6c920d0a052b42f08d6ff6442c6b5b33a validation: No mempool clearing in UnloadBlockIndex (12/14)
becf5e94195e51f6ed62caf8b6ddbcd2012dce72 validation: Prune UnloadBlockIndex and callees (13/14)
ddd6bafc16e75e1fb93fd0177f5c5c7c64579095 scripted-diff: Rename pindexBestHeader, fHavePruned (14/14)

DrahtBot added the label Needs rebase on Apr 6, 2022

in src/rpc/blockchain.cpp:182 in 7123f3d8af outdated

178@@ -180,7 +179,7 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIn
179         case TxVerbosity::SHOW_DETAILS:
180         case TxVerbosity::SHOW_DETAILS_AND_PREVOUT:
181             CBlockUndo blockUndo;
182-            const bool have_undo{WITH_LOCK(::cs_main, return !IsBlockPruned(blockindex) && UndoReadFromDisk(blockUndo, blockindex))};
183+            const bool have_undo{WITH_LOCK(::cs_main, return !blockman.IsBlockPruned(blockindex)) && UndoReadFromDisk(blockUndo, blockindex)};

ryanofsky commented at 8:35 pm on April 6, 2022:

In commit “move-mostly: Make fHavePruned a BlockMan member” (7123f3d8af4e592212439968f4eaaec912ec8d02)

Possible bug or unrelated change? Before this locked cs_main while calling UndoReadFromDisk, now it now longer does. It does seems good to not lock cs_main while calling UndoReadFromDisk, but it doesn’t really make sense as part of this commit.

dongcarl commented at 0:17 am on April 12, 2022:

Ah! Yes this was unintentional 😅 Fixed in 85b8b25239f6a7ce1f6877eec52d786352c90872

in src/node/blockstorage.cpp:303 in 18d68910bd outdated

298@@ -299,6 +299,8 @@ void BlockManager::Unload()
299     m_last_blockfile = 0;
300     m_dirty_blockindex.clear();
301     m_dirty_fileinfo.clear();
302+
303+    fHavePruned = false;

ryanofsky commented at 8:42 pm on April 6, 2022:

In commit “Clear fHavePruned in BlockManager::Unload()” (18d68910bdd4f18eadbf72fa7ec0c0299def3666)

Note: BlockManager::Unload() is only called by ChainstateManager::Unload() and the BlockManager destructor. ChainstateManager::Unload() is only called by UnloadBlockIndex. So there is not really a change moving this line from UnloadBlockIndex

dongcarl force-pushed on Apr 12, 2022

dongcarl commented at 0:16 am on April 12, 2022: member

Pushed ddd6bafc16e75e1fb93fd0177f5c5c7c64579095 -> 26779f2df1f04e5b5ff8b7edf19cd5f1280074d9

Rebased over master
Addressed #22564 (review)

dongcarl commented at 0:18 am on April 12, 2022: member

@ryanofsky I love these reviewer notes! Do you think I should put these notes into the commit messages or leaving it on GitHub is fine?

ryanofsky commented at 0:33 am on April 12, 2022: member

@ryanofsky I love these reviewer notes! Do you think I should put these notes into the commit messages or leaving it on GitHub is fine?

I’m just writing these notes as breadcrumbs to remind myself how these commits work in case I look at them again. They would make sense in commit messages, though, so feel free to take anything that seems useful.

DrahtBot removed the label Needs rebase on Apr 12, 2022

dongcarl force-pushed on Apr 12, 2022

dongcarl commented at 6:43 pm on April 12, 2022: member

Pushed 26779f2df1f04e5b5ff8b7edf19cd5f1280074d9 -> c6bce70edfe3857b63414f314394cf7b717ffffe:

No code changes
Added helpful reviewer comments by ryanofsky

MarcoFalke commented at 10:03 am on April 15, 2022: member

Have you thought about moving the last commit (scripted-diff) right after the prune commit (4032fc9cf943e512319217d21349f2bb3be55129) and then split that part off from this pull? It may be that the move parts are causing the most conflicts, but they take the least time to review in comparison to the other commits. So maybe a split-off can reduce the rebases needed for the later commits in this pull.

MarcoFalke added this to the milestone 24.0 on Apr 15, 2022

in src/node/chainstate.cpp:35 in f8afc08153 outdated

27@@ -28,11 +28,8 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
28     };
29 
30     LOCK(cs_main);
31-    chainman.InitializeChainstate(mempool);
32-    chainman.m_total_coinstip_cache = nCoinCacheUsage;
33-    chainman.m_total_coinsdb_cache = nCoinDBCache;
34 
35-    UnloadBlockIndex(mempool, chainman);

ajtowns commented at 9:51 pm on April 15, 2022:

Dropping this looks like it leaves chainman.m_best_invalid uninitialized

dongcarl force-pushed on Apr 18, 2022

dongcarl commented at 6:53 pm on April 18, 2022: member

Pushed fbc58b4cc5274371ff7f883c71a21febc4cc508a -> 018f97c77f90c950c2e633c9a63be43a71f431db

Split off #24909 per Marco’s suggestion: #22564 (comment)
Cherry-picked aj’s fix for uninitialized error due to no longer calling UnloadBlockIndex: #24857 (comment)

MarcoFalke referenced this in commit dbdc83ae01 on Apr 20, 2022

DrahtBot added the label Needs rebase on Apr 20, 2022

dongcarl force-pushed on Apr 20, 2022

dongcarl commented at 3:06 pm on April 20, 2022: member

Pushed 018f97c77f90c950c2e633c9a63be43a71f431db -> b1ab56316ab8cad391676be2e9ca749215603deb

Rebased over master after merge of #24909

DrahtBot removed the label Needs rebase on Apr 20, 2022

in src/validation.cpp:2553 in 4373351ead outdated

2532@@ -2533,7 +2533,7 @@ void CChainState::UpdateTip(const CBlockIndex* pindexNew)
2533         const CBlockIndex* pindex = pindexNew;
2534         for (int bit = 0; bit < VERSIONBITS_NUM_BITS; bit++) {
2535             WarningBitsConditionChecker checker(bit);
2536-            ThresholdState state = checker.GetStateFor(pindex, m_params.GetConsensus(), warningcache[bit]);
2537+            ThresholdState state = checker.GetStateFor(pindex, m_params.GetConsensus(), warningcache.at(bit));

MarcoFalke commented at 3:03 pm on April 25, 2022:

Unrelated.

Sometimes I wonder how safe it is to throw in random places in the consensus code and then continue the program. Too bad there is no asserting at().

Screenshot from 2022-04-25 17-03-14

ajtowns commented at 4:59 am on April 27, 2022:

There is a compile-time erroring at() – std::get<N>(my_array) won’t compile if N is outside the range. I’ve got a patchset that switches versionbits to working that way, fwiw; needs a lot of tidying up yet though, and builds on these patches.

MarcoFalke commented at 6:33 am on April 28, 2022:

Nice. TIL https://en.cppreference.com/w/cpp/container/array/get

in src/validation.h:836 in 4be05be37a outdated

830@@ -831,9 +831,9 @@ class ChainstateManager
831 
832     //! If true, the assumed-valid chainstate has been fully validated
833     //! by the background validation chainstate.
834-    bool m_snapshot_validated{false};
835+    bool m_snapshot_validated GUARDED_BY(::cs_main) {false};
836 
837-    CBlockIndex* m_best_invalid;
838+    CBlockIndex* m_best_invalid GUARDED_BY(::cs_main) {nullptr};

MarcoFalke commented at 3:05 pm on April 25, 2022:

nit in the second commit:

clang format:

 0diff --git a/src/validation.h b/src/validation.h
 1index caca7f2531..adefc706fd 100644
 2--- a/src/validation.h
 3+++ b/src/validation.h
 4@@ -831,9 +831,9 @@ private:
 5 
 6     //! If true, the assumed-valid chainstate has been fully validated
 7     //! by the background validation chainstate.
 8-    bool m_snapshot_validated GUARDED_BY(::cs_main) {false};
 9+    bool m_snapshot_validated GUARDED_BY(::cs_main){false};
10 
11-    CBlockIndex* m_best_invalid GUARDED_BY(::cs_main) {nullptr};
12+    CBlockIndex* m_best_invalid GUARDED_BY(::cs_main){nullptr};
13     friend bool node::BlockManager::LoadBlockIndex(const Consensus::Params&);
14 
15     //! Internal helper for ActivateSnapshot().

dongcarl commented at 4:38 pm on April 25, 2022:

Done in fd6e94252e0860831527293fe9aa743044ca635f

in src/init.cpp:1272 in d04d3c4c1f outdated

1268@@ -1269,18 +1269,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1269     // as they would never get updated.
1270     if (!ignores_incoming_txs) node.fee_estimator = std::make_unique<CBlockPolicyEstimator>();
1271 
1272-    assert(!node.mempool);
1273     int check_ratio = std::min<int>(std::max<int>(args.GetIntArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000);

MarcoFalke commented at 3:15 pm on April 25, 2022:

In the third commit:

I think this should be moved as well, otherwise it is easy to confuse it with some other check_ratio

Edit: I see it is renamed later, but I still think it could make sense to move as well.

MarcoFalke commented at 3:30 pm on April 25, 2022:

Also, would it be better to split the peerman move into a separate commit?

Also, it would be best to do a rebase first to pull in the intermediate changes to init.cpp?

dongcarl commented at 4:39 pm on April 25, 2022:

I think this should be moved as well, otherwise it is easy to confuse it with some other check_ratio

Done in 976679b251e417d3aa7329807f3d934944ab5252

Also, would it be better to split the peerman move into a separate commit?

Don’t think so, the diff itself isn’t that large, and the peerman move only makes sense in the context of moving the mempool and chainman

Also, it would be best to do a rebase first to pull in the intermediate changes to init.cpp?

Done as of the 133d98f446217f0fa37aa5b067fb74d3ac4e3b01 push

MarcoFalke approved

MarcoFalke commented at 3:46 pm on April 25, 2022: member

ACK b1ab56316ab8cad391676be2e9ca749215603de 🏐

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK b1ab56316ab8cad391676be2e9ca749215603de 🏐
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgrhAwAlBimHVBdqGT+/XIgR4/6KDDdTo7KHYTiKNoCwjOtFk3qOleBfXyhPgUr
 88mmwG4RZ/UyAWut71omZs6yQI+7SyhcWZGSOihPk+k08QJFHbT0Ss9Kvb1xuujvs
 9Y+arP/Zv6zPWfgkbEJbYg6yPRLNRfESBnoRGmTY4JzUV3Y7wkgjMw40PcJyO6DBb
10i2JFI1Pap5wqOYBFMXO/ElITTka+nqAPgPIRLhH6Ld4CJjd4bLW6xbPQLCHf4naj
11yy5HvV30NCFeB9mZyVW6qnXxGMmWLsI9TzFXPSOE+v/YDD0Hb4FLg8a7X80sZs+F
12E11avAyixd61w5+xcAUpbRUbEA+6FRgByodLVeHBi5xyOibHXO8zlKqfyA86u8Yx
13Te4ARRWifX2bsz88dnuaeQZy/rpwJbxA+E6jSZDTAwsaS9l7DbtNk2SK7YnB2COq
14WctKXdBzXzZRRi+c0Q7nG2heNlhR9KQFkvsfiw7Am4yqU61fy8L8orkAvHUrGMya
15A9Kg72e8
16=ZuTN
17-----END PGP SIGNATURE-----

dongcarl force-pushed on Apr 25, 2022

dongcarl commented at 4:40 pm on April 25, 2022: member

Pushed b1ab56316ab8cad391676be2e9ca749215603deb -> 133d98f446217f0fa37aa5b067fb74d3ac4e3b01

Rebased over master
Addressed Marco’s review comments (thanks Marco!)

DrahtBot added the label Needs rebase on Apr 26, 2022

refactor: Convert warningcache to std::array 98f4bdae81

validation: default initialize and guard chainman members 6e747e80e7

dongcarl force-pushed on Apr 26, 2022

DrahtBot removed the label Needs rebase on Apr 26, 2022

in src/init.cpp:1428 in 09574d7ad7 outdated

1423     bool fLoaded = false;
1424     while (!fLoaded && !ShutdownRequested()) {
1425+        node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), check_ratio);
1426+
1427+        node.chainman = std::make_unique<ChainstateManager>();
1428+        ChainstateManager& chainman = *Assert(node.chainman);

ajtowns commented at 5:15 am on April 27, 2022:

super nitty: make_unique throws if it can’t allocate, so the Assert here shouldn’t be needed?

dongcarl commented at 3:15 pm on April 27, 2022:

Fixed as of 7ab07e033237d6ea179a6a2c76575ed6bd01a670

in src/node/chainstate.cpp:33 in 09574d7ad7 outdated

27@@ -28,11 +28,8 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
28     };
29 
30     LOCK(cs_main);
31-    chainman.InitializeChainstate(mempool);
32-    chainman.m_total_coinstip_cache = nCoinCacheUsage;
33-    chainman.m_total_coinsdb_cache = nCoinDBCache;

ajtowns commented at 5:22 am on April 27, 2022:

Why are m_total_coinstip_cache and m_total_coinsdb_cache not initialized anymore? As it stands, bitcoind compiles with both those members marked as static constexpr which doesn’t seem right…

dongcarl commented at 3:15 pm on April 27, 2022:

Fixed as of 7ab07e033237d6ea179a6a2c76575ed6bd01a670

in src/init.cpp:1422 in 66201eab01 outdated

1418@@ -1418,10 +1419,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1419 
1420     assert(!node.mempool);
1421     assert(!node.chainman);
1422-    int check_ratio = std::min<int>(std::max<int>(args.GetIntArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000);
1423+    int mempool_check_ratio = std::clamp<int>(args.GetIntArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0, 1000000);

ajtowns commented at 5:24 am on April 27, 2022:

If retouching, maybe add a const

dongcarl commented at 3:15 pm on April 27, 2022:

Fixed as of 7ab07e033237d6ea179a6a2c76575ed6bd01a670

in src/bitcoin-chainstate.cpp:259 in 678f5c5a62 outdated

255@@ -256,7 +256,5 @@ int main(int argc, char* argv[])
256     }
257     GetMainSignals().UnregisterBackgroundSignalScheduler();
258 
259-    WITH_LOCK(::cs_main, UnloadBlockIndex(nullptr, chainman));

ajtowns commented at 5:31 am on April 27, 2022:

The previous commit should have dropped the nullptr from this call.

dongcarl commented at 3:15 pm on April 27, 2022:

Fixed as of 7ab07e033237d6ea179a6a2c76575ed6bd01a670

ajtowns commented at 5:39 am on April 27, 2022: member

The m_total_coins*_cache changes seem wrong to me, otherwise looks good.

Commit 678f5c5a623771f52d5e270cc79c76b87db3e688

init: Reset mempool and chainman via reconstruction

Fixes https://github.com/bitcoin/bitcoin/issues/22964

Previously, we used UnloadBlockIndex() in order to reset node.mempool
and node.chainman. However, that has proven to be fragile (see
https://github.com/bitcoin/bitcoin/issues/22964), and requires
UnloadBlockIndex and its callees to be updated manually for each member
that's introduced to the mempool and chainman classes.

In this commit, we stop using the UnloadBlockIndex function and we
simply reconstruct node.mempool and node.chainman.

Since PeerManager needs a valid reference to both node.mempool and
node.chainman, we also move PeerManager's construction via `::make` to
after the chainstate activation sequence is complete.

There are no more callers to UnloadBlockIndex after this commit, so it
and its sole callees can be pruned.

5921b863e3

style-only: Use for instead of when loading Chainstate

It's a bit clearer and restricts the scope of fLoaded

fe96a2e4bd

style-only: Use std::clamp for check_ratio, rename eca4ca4d60

Clear {versionbits,warning}cache in ~Chainstatemanager

Also add TODO item to deglobalize the {versionbits,warning}cache, which
should really only need to be cleared if we change the chainparams.

572d831927

validation: No mempool clearing in UnloadBlockIndex

The only caller that uses this is ~ChainTestingSetup() where we
immediately destroy the mempool afterwards.

7d99d725cd

validation: Prune UnloadBlockIndex and callees

In previous commits in this patchset, we've made sure that every
Unload/UnloadBlockIndex member function resets its own members, and does
not reach out to globals.

This means that their corresponding classes' default destructors can now
replace them, and do an even more thorough job without the need to be
updated for every new member variable.

Therefore, we can remove them, and also remove UnloadBlockIndex since
that's not used anymore.

Unfortunately, chainstatemanager_loadblockindex relies on
CChainState::UnloadBlockIndex, so that needs to stay for now.

7ab07e0332

in src/validation.cpp:1924 in 98f4bdae81 outdated

1920@@ -1921,7 +1921,7 @@ class WarningBitsConditionChecker : public AbstractThresholdConditionChecker
1921     }
1922 };
1923 
1924-static ThresholdConditionCache warningcache[VERSIONBITS_NUM_BITS] GUARDED_BY(cs_main);
1925+static std::array<ThresholdConditionCache, VERSIONBITS_NUM_BITS> warningcache GUARDED_BY(cs_main);

ryanofsky commented at 2:59 pm on April 27, 2022:

In commit “refactor: Convert warningcache to std::array” (98f4bdae81804de17f125bd7c2cd8a48e850a6d2)

This commit seems ok but it’s not clear why it’s good or what the connection is to the PR. Would be good to mention in commit message.

MarcoFalke commented at 6:31 am on April 28, 2022:

I think it is just “updating the code”, since it is already touched for other reasons in this PR

dongcarl force-pushed on Apr 27, 2022

dongcarl commented at 3:17 pm on April 27, 2022: member

Pushed 678f5c5a623771f52d5e270cc79c76b87db3e688 -> 7ab07e033237d6ea179a6a2c76575ed6bd01a670

Addressed outstanding reviews (Thanks AJ!)

in src/init.cpp:1566 in 5921b863e3 outdated

1557@@ -1562,6 +1558,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1558         return false;
1559     }
1560 
1561+    ChainstateManager& chainman = *Assert(node.chainman);
1562+
1563+    assert(!node.peerman);
1564+    node.peerman = PeerManager::make(chainparams, *node.connman, *node.addrman, node.banman.get(),
1565+                                     chainman, *node.mempool, ignores_incoming_txs);
1566+    RegisterValidationInterface(node.peerman.get());

ryanofsky commented at 3:25 pm on April 27, 2022:

In commit “init: Reset mempool and chainman via reconstruction” (976679b251e417d3aa7329807f3d934944ab5252)

Is it ok for peerman to register for validation notification events later than before? Maybe because none will be sent above, or because peerman doesn’t care about ones that would be sent? (would be helpful info for commit message)

MarcoFalke commented at 6:48 am on April 28, 2022:

My understanding was that no events are sent, since blocks and the mempool are only imported later on

MarcoFalke commented at 7:15 am on April 28, 2022:

And it looks like (from testing) with existing blocks and without import/reindex/empty datadir no events are sent at all during startup. I can only see events when the block import thread is running.

in src/node/chainstate.cpp:35 in 5921b863e3 outdated

31@@ -32,8 +32,6 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
32     chainman.m_total_coinstip_cache = nCoinCacheUsage;
33     chainman.m_total_coinsdb_cache = nCoinDBCache;
34 
35-    UnloadBlockIndex(mempool, chainman);

ryanofsky commented at 3:26 pm on April 27, 2022:

In commit “init: Reset mempool and chainman via reconstruction” (5921b863e39e5c3997895ffee1c87159e37a5d6f)

UnloadBlockIndex is still clearing g_versionbitscache and warningcache at this point. Should this commit come after the later commit “Clear {versionbits,warning}cache in ~Chainstatemanager” (572d8319272ae84a81d6bfd53dd9685585697f65) to avoid changing behavior?

MarcoFalke commented at 10:12 am on April 28, 2022:

Have you seen #22564 (review) ?

in src/validation.cpp:5241 in 572d831927 outdated

5236+    // TODO: The version bits cache and warning cache should probably become
5237+    // non-globals
5238+    g_versionbitscache.Clear();
5239+    for (auto& i : warningcache) {
5240+        i.clear();
5241+    }

ryanofsky commented at 4:03 pm on April 27, 2022:

In commit “Clear {versionbits,warning}cache in ~Chainstatemanager” (572d8319272ae84a81d6bfd53dd9685585697f65)

It would seem more appropriate to clear these variables in the constructor not the destructor. Using the constructor to initialize things a class needs is more straightforward than doing things in the destructor to prepare for a future instance being created.

MarcoFalke commented at 10:13 am on April 28, 2022:

Maybe this can be discussed in the versionbits follow-up?

ryanofsky approved

ryanofsky commented at 5:17 pm on April 27, 2022: member

Code review ACK 7ab07e033237d6ea179a6a2c76575ed6bd01a670. This all looks good and simplifies things nicely. I left some minor suggestions below but feel free to ignore.

MarcoFalke commented at 7:39 am on April 28, 2022: member

ACK 7ab07e033237d6ea179a6a2c76575ed6bd01a670 👘

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 7ab07e033237d6ea179a6a2c76575ed6bd01a670 👘
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhluAwAl/H737rZZOleCgVaxyEu2ymxIHFJINMjj3ivDb0m/PhKo/ebMuzRiHe+
 84tLOLlbIAWDh53hh6GBcdJRqKSWPUpED45VaEAF/icNYwUhWaaXavfqfHPwcZ6ud
 9V436tvffYSKlKoEtMBUupJQ4Ah951vAPwTDFVVr8nNexs5EpmAHei+WdB3hJjht3
10UZO2KaLcPrFVfgSJaHEB2Ea2zdpp3NXBAqfLO/QMPaUYcEdUcW4JjVj4uYSaQsv/
11we+/rcmw6f5hGPSvUENkkXa9LaEfGaLrNwp/oB5pa4o+4wIVbI6RQluNtSe1HyJz
121hijjwuV3x/ozky4zB/py1ETlJ1tW4G+UIYFf7b65lV+wymt/Cfx8TM3ijXh47Ha
13F5VBksvCvgbmAmLr2Vel7xZHq+jdxbuiU2t+uS2iBY+zNZ+2THEizCEV7hxVOQnB
14vAGVKwxMc23sbBUOOnp1jC10xHJer7BbbFdksfrx/f/Evmwa8XKaX5ObpZMx2quu
154u0iV6Pf
16=Lld+
17-----END PGP SIGNATURE-----

ajtowns commented at 10:06 am on April 28, 2022: member

ACK 7ab07e033237d6ea179a6a2c76575ed6bd01a670

MarcoFalke merged this on Apr 28, 2022

MarcoFalke closed this on Apr 28, 2022

sidhujag referenced this in commit 76ed6923be on Apr 29, 2022

Fabcien referenced this in commit ef3c0ac424 on Feb 2, 2023

Fabcien referenced this in commit 8ddcf128ce on Feb 2, 2023

Fabcien referenced this in commit 4b236a1f34 on Feb 2, 2023

Fabcien referenced this in commit 61adf21f6f on Feb 2, 2023

Fabcien referenced this in commit 9b392c5f87 on Feb 2, 2023

Fabcien referenced this in commit 9ba9cf2f69 on Feb 3, 2023

DrahtBot locked this on Apr 28, 2023

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

Conflicts

#22564 vs. $mergebase (absolute)

#22564 vs. $mergebase (relative)

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