[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex

dongcarl commented at 6:25 pm on December 22, 2020: member

Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)

Note to reviewers:

This bundle may apparently introduce usage of g_chainman or ::Chain(state|)Active() globals, but these are resolved later on in the overall PR. Commits of overall PR
There may be seemingly obvious local references to ChainstateManager or other validation objects which are not being used in callers of the current function in question, this is done intentionally to keep each commit centered around one function/method to ease review and to make the overall change systematic. We don’t assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. Commits of overall PR
When changing a function/method that has many callers (e.g. LookupBlockIndex with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add new_function, make old_function a wrapper of new_function, divert all calls to old_function to new_function in the local module only
2. Scripted-diff to divert all calls to old_function to new_function in the rest of the codebase
3. Remove old_function

DrahtBot added the label Mempool on Dec 22, 2020

DrahtBot added the label Mining on Dec 22, 2020

DrahtBot added the label P2P on Dec 22, 2020

DrahtBot added the label RPC/REST/ZMQ on Dec 22, 2020

DrahtBot added the label UTXO Db and Indexes on Dec 22, 2020

DrahtBot added the label Validation on Dec 22, 2020

in src/init.cpp:708 in e8dedbb064 outdated

698@@ -699,7 +699,7 @@ static void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImp
699             if (!file)
700                 break; // This error is logged in OpenBlockFile
701             LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
702-            LoadExternalBlockFile(chainparams, file, &pos);
703+            ::ChainstateActive().LoadExternalBlockFile(chainparams, file, &pos);

MarcoFalke commented at 6:53 pm on December 22, 2020:

I know I’ve asked the same question already, but I didn’t get why this can’t use chainman.ActiveChainstate().Load....

This should simplify review because it will touch the same lines of code less often.

Also, it shouldn’t make review harder, because everyone knows that there is only one chainman (whether it is called g_chainman or chainman shouldn’t matter). On top you added a runtime assertion, which validates this for all executed code paths in our tests:

0     assert(std::addressof(::ChainstateActive()) == std::addressof(ActiveChainstate()));

MarcoFalke commented at 6:58 pm on December 22, 2020:

Anyway, no big deal if you want to keep it that way. Will review regardless.

MarcoFalke removed the label Mempool on Dec 22, 2020

MarcoFalke removed the label Mining on Dec 22, 2020

MarcoFalke removed the label P2P on Dec 22, 2020

MarcoFalke removed the label RPC/REST/ZMQ on Dec 22, 2020

MarcoFalke removed the label UTXO Db and Indexes on Dec 22, 2020

MarcoFalke added the label Refactoring on Dec 22, 2020

DrahtBot commented at 8:41 pm on December 22, 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:

#21025 (validation: Guard the active_chainstate with cs_main by dongcarl)
#20758 (net-processing refactoring – lose globals, move implementation details from .h to .cpp by ajtowns)
#20750 ([Bundle 2/n] Prune g_chainman usage in mempool-related validation functions by dongcarl)
#20331 (allow -loadblock blocks to be unsorted by LarryRuane)
#20158 (tree-wide: De-globalize ChainstateManager by dongcarl)
#19806 (validation: UTXO snapshot activation by jamesob)
#19594 (refactor: Make mapBlocksUnknownParent local, and rename it by hebasto)
#19438 (Introduce deploymentstatus by ajtowns)
#16981 (Improve runtime performance of –reindex by LarryRuane)
#16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)

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/validation.cpp:3401 in 626b709edf outdated

3390@@ -3392,14 +3391,15 @@ std::vector<unsigned char> GenerateCoinbaseCommitment(CBlock& block, const CBloc
3391 }
3392 
3393 //! Returns last CBlockIndex* that is a checkpoint
3394-static CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
3395+CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data)

Sjors commented at 12:33 pm on December 30, 2020:

626b709edfe1bd71e6d485c096ff9a52f698a94c nit: move help text to header file

dongcarl commented at 3:16 pm on January 5, 2021:

Will do on next push!

in src/validation.cpp:4670 in a375714f24 outdated

4666@@ -4671,7 +4667,7 @@ void LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, FlatFi
4667                 // Activate the genesis block so normal node progress can continue
4668                 if (hash == chainparams.GetConsensus().hashGenesisBlock) {
4669                     BlockValidationState state;
4670-                    if (!ActivateBestChain(state, chainparams, nullptr)) {
4671+                    if (!::ChainstateActive().ActivateBestChain(state, chainparams, nullptr)) {

Sjors commented at 12:43 pm on December 30, 2020:

a375714f24b85b6f9ce7e1765a1137e3b85af009 nit: drop , nullptr?

in src/validation.h:653 in a375714f24 outdated

646@@ -654,6 +647,8 @@ class CChainState
647     void PruneAndFlush();
648 
649     /**
650+     * Find the best known block, and make it the tip of the block chain
651+     *

Sjors commented at 12:45 pm on December 30, 2020:

a375714f24b85b6f9ce7e1765a1137e3b85af009 Should this comment be preserved?

0* May not be called with cs_main held. May not be called in a		
1* validationinterface callback.

dongcarl commented at 3:15 pm on January 5, 2021:

Unsure, perhaps @jamesob can help expound on this point a little?

jnewbery commented at 9:04 pm on January 11, 2021:

It is preserved, at the bottom of this comment. It was just duplicated before.

Although I think it’s fine to remove “May not be called with cs_main held.” We have lock annotations to document/enforce that now, so the comment is redundant.

Sjors approved

Sjors commented at 12:52 pm on December 30, 2020: member

ACK e8dedbb064e90aaa8b2540c7524af54bbdd49d0a

Regarding 98f237506b454816599c58e7f4eb2a65bce32fc2’s commit comment:

However, it seems like the original intention was for FindForkInGlobalIndex to work with any chain, not just the current active chain. Let me know if this should be changed.

This was introduced in 2014 by @jtimon in #4796, while refactoring chainActive.FindFork(locator) to findForkInGlobalIndex(chainActive, locator);. Unless @jamesob sees a use for it for snapshot loading, I think dropping the chain parameter should be fine.

jnewbery commented at 2:25 pm on December 30, 2020: member

Concept ACK!

laanwj added this to the "Blockers" column in a project

laanwj commented at 9:07 pm on January 8, 2021: member

Concept ACK

jnewbery commented at 9:08 pm on January 11, 2021: member

I’ve reviewed everything up to 94bbdd7134 validation: Remove global LookupBlockIndex and it looks good so far.

Does it make sense to remove the assert(std::addressof(thing) == std::addressof(thing)); lines as the last commit of this PR? Is there value in keeping those in place until the series of PRs is finished?

in src/validation.h:554 in f28f8e3da5 outdated

554 
555 public:
556+    //! Reference to a BlockManager instance which itself is shared across all
557+    //! CChainState instances. Keeping a local reference allows us to test more
558+    //! easily as opposed to referencing a global.
559+    BlockManager& m_blockman GUARDED_BY(::cs_main);

ryanofsky commented at 8:08 pm on January 12, 2021:

In commit “validation: Make CChainState.m_blockman public” (f28f8e3da5a48be3cdc0207112d9aa8ec8f6e820)

Is it intentional to add GUARDED_BY here? Maybe add a comment saying what semantics are. I wouldn’t think it should be necessary to lock cs_main just to get a reference to the BlockManager instance. It does seem like cs_main is needed to use some BlockManager members, but these are already annotated.

dongcarl commented at 8:17 pm on January 13, 2021:

True, that doesn’t seem necessary!

dongcarl commented at 8:19 pm on January 12, 2021: member

I’ve reviewed everything up to 94bbdd7 validation: Remove global LookupBlockIndex and it looks good so far.

Yay! ☺️

Does it make sense to remove the assert(std::addressof(thing) == std::addressof(thing)); lines as the last commit of this PR? Is there value in keeping those in place until the series of PRs is finished?

I did that in a previous version of this PR, but I changed my mind in this version.

The main reason for this is that as we make further changes in the future (e.g. call LoadExternalBlockFile on the local chainman in ThreadImport in this commit) we still want to make sure that these assertions hold up for correctness.

Another advantage is that leaving these assertions in prevents new PRs from making the class of mistakes fixed in #20323 from passing tests while the de-globalize chainman bundles are being reviewed and merged.

ryanofsky approved

ryanofsky commented at 10:04 pm on January 12, 2021: member

Code review ACK e8dedbb064e90aaa8b2540c7524af54bbdd49d0a

DrahtBot commented at 11:46 am on January 13, 2021: member

🕵️ @sipa @fjahr have been requested to review this pull request as specified in the REVIEWERS file.

dongcarl force-pushed on Jan 13, 2021

dongcarl commented at 8:19 pm on January 13, 2021: member

Push e8dedbb064e90aaa8b2540c7524af54bbdd49d0a -> c948ff186b34693e762fa595ddc174cd88bfd89c

Addressed #20749 (review) and #20749 (review)

in src/validation.cpp:182 in c948ff186b outdated

180-CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator)
181+CBlockIndex* BlockManager::FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator)
182 {
183     AssertLockHeld(cs_main);
184 
185+    assert(std::addressof(g_chainman.m_blockman) == std::addressof(*this));

ariard commented at 2:02 am on January 15, 2021:

I think this assert can be removed after 98f2375, as FindForkInGlobalIndex is moved to BlockManager and thus you can now directly access the new local method BlockManager::LookupBlockIndex. Asserting g_chainmain.m_blockman equivalence is lapsed ?

Same for asserts in GetSpendHeight after d34c069 and in GetLastCheckpoint after 866834a

dongcarl commented at 6:21 pm on January 15, 2021:

I see your point, but I don’t think it hurts anything 😁 All of these will be removed in the end with a scripted-diff anyway

ariard commented at 2:03 am on January 15, 2021: member

Approach ACK in accordance to #20158

jonatack commented at 11:00 am on January 15, 2021: member

Concept ACK

in src/validation.h:435 in c948ff186b outdated

430+
431+    /** Find the last common block between the parameter chain and a locator. */
432+    CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
433+
434+    //! Returns last CBlockIndex* that is a checkpoint
435+    CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

jonatack commented at 11:34 am on January 15, 2021:

64d27fb0b950bcbd87bc7cd0b4d62f47f305ef88 naming and placement suggestions, feel free to ignore

as the verb is “look up” (not “lookup”, which is a noun or adjective), suggest naming the function LookUpBlockIndex
it may be nice to place the function next to its similar twins, per the following diff

 0+++ b/src/validation.h
 1@@ -417,6 +417,7 @@ public:
 2     CBlockIndex* AddToBlockIndex(const CBlockHeader& block) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 3     /** Create a new block index entry for a given block hash */
 4     CBlockIndex* InsertBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 5+    CBlockIndex* LookUpBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 6 
 7     //! Mark one block file as pruned (modify associated database entries)
 8     void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 9@@ -431,8 +432,6 @@ public:
10         const CChainParams& chainparams,
11         CBlockIndex** ppindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
12 
13-    CBlockIndex* LookupBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
14-
15     ~BlockManager() {
16         Unload();
17     }

jonatack commented at 11:47 am on January 15, 2021: member

Thanks for the care in making the changes clear and easier to review. Started by building and reviewing each of the first three commits 64d27fb / a0954ef / 94bbdd7 after rebasing the pull to master, and running the scripted diff in a0954ef

0$ test/lint/commit-script-check.sh f91587f..7644282
1Running script for: 76442824f66d50a3ceed844349f8d13ce8cf6c87
2find_regex='LookupBlockIndex' \
3    && git grep -l -E "$find_regex" -- src \
4        | grep -v '^src/validation\.\(cpp\|h\)$' \
5        | xargs sed -i -E "s@${find_regex}@g_chainman.m_blockman.LookupBlockIndex@g"
6OK

ryanofsky approved

ryanofsky commented at 4:56 pm on January 15, 2021: member

Code review ACK c948ff186b34693e762fa595ddc174cd88bfd89c. Just lock annotation tweak and added comment since last review

felipsoarez commented at 5:40 pm on January 15, 2021: none

Concept ACK

dongcarl commented at 9:06 pm on January 15, 2021: member

Note: If #20946 gets in before this, we won’t need this last commit I just pushed.

in src/txmempool.cpp:629 in 364fa81a2b outdated

624@@ -625,7 +625,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
625     uint64_t innerUsage = 0;
626 
627     CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(pcoins));
628-    const int64_t spendheight = GetSpendHeight(mempoolDuplicate);
629+    const int64_t spendheight = WITH_LOCK(::cs_main, return g_chainman.m_blockman.GetSpendHeight(mempoolDuplicate));

jnewbery commented at 9:30 am on January 18, 2021:

The lock ordering is to take cs_main then take mempool.cs so at first glance, this looks like a lock inversion since it’s locking cs_main while already holding mempool.cs. In fact, whenever mempool.check() is called, we’re already holding cs_main (see the two call sites in net_processing).

I think it’d be better to add an EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation to check() and remove the re-entrant lock-taking here.

ryanofsky commented at 1:57 pm on January 20, 2021:

In commit “validation: Move GetSpendHeight to BlockManager” (d34c0698ec34b45b3ce153dd7ef0b79650e6deb9)

re: #20749 (review)

The lock ordering is to take cs_main then take mempool.cs so at first glance, this looks like a lock inversion since it’s locking cs_main while already holding mempool.cs. In fact, whenever mempool.check() is called, we’re already holding cs_main (see the two call sites in net_processing).

I think it’d be better to add an EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation to check() and remove the re-entrant lock-taking here.

This is true and I commented on it previously #20158 (review), but since this commit is just a plain refactoring, I suspect it could complicate the PR in a bad way to try to tackle the lock ordering issue here. I would be happy to see a code comment pointing out the inconsistent lock ordering, or even to see it fixed here if your fix is not that complicated to review and implement. But I suspect this could be better to fix separately since it’s not directly related to changes in this PR

dongcarl commented at 8:48 pm on January 20, 2021:

I agree with all the points made, I wanted to keep it this way as it doesn’t change the existing (fragile) behaviour, but just makes it more obvious. However, I do think it’d be good to add the annotation.

I opened a new PR for this: https://github.com/bitcoin/bitcoin/pull/20972

glozow commented at 5:51 pm on January 26, 2021:

Er, why is WITH_LOCK(cs_main) needed if check() has already asserted that it’s held?

dongcarl commented at 8:13 pm on January 26, 2021:

Fixed!

in src/validation.h:54 in 364fa81a2b outdated

50@@ -50,6 +51,7 @@ struct ChainTxData;
51 struct DisconnectedBlockTransactions;
52 struct PrecomputedTransactionData;
53 struct LockPoints;
54+struct CCheckpointData;

jnewbery commented at 9:36 am on January 18, 2021:

No need for this forward declaration. You’re including chainparams.h, which is where this is declared.

dongcarl commented at 8:49 pm on January 20, 2021:

Good catch, fixed!

in src/validation.h:292 in 364fa81a2b outdated

288@@ -296,7 +289,7 @@ bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex);
289 bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
290 
291 /** Check a block is completely valid from start to finish (only works on top of our current best block) */
292-bool TestBlockValidity(BlockValidationState& state, const CChainParams& chainparams, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckPOW = true, bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
293+bool TestBlockValidity(BlockValidationState& state, const CChainParams& chainparams, CChainState& chainstate, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckPOW = true, bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

jnewbery commented at 9:48 am on January 18, 2021:

I find these very long lines almost impossible to read. If you touch this branch again, could you be convinced to split it up. Something like:

0bool TestBlockValidity(BlockValidationState& state, const CChainParams& chainparams,
1                       CChainState& chainstate, const CBlock& block, CBlockIndex* pindexPrev,
2                       bool fCheckPOW = true, bool fCheckMerkleRoot = true)
3    EXCLUSIVE_LOCKS_REQUIRED(cs_main);

dongcarl commented at 8:49 pm on January 20, 2021:

Fixed in: d8a5fe24af84c34492e2b23ed341bae70a3c8983

in src/validation.cpp:3800 in 364fa81a2b outdated

3799 
3800     return true;
3801 }
3802 
3803-bool TestBlockValidity(BlockValidationState& state, const CChainParams& chainparams, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckPOW, bool fCheckMerkleRoot)
3804+bool TestBlockValidity(BlockValidationState& state, const CChainParams& chainparams, CChainState& chainstate, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckPOW, bool fCheckMerkleRoot)

jnewbery commented at 9:49 am on January 18, 2021:

Same comment as for the declaration. Consider splitting this line.

dongcarl commented at 8:49 pm on January 20, 2021:

Fixed in: d8a5fe24af84c34492e2b23ed341bae70a3c8983

in src/rpc/mining.cpp:378 in 364fa81a2b outdated

374@@ -375,7 +375,7 @@ static RPCHelpMan generateblock()
375         LOCK(cs_main);
376 
377         BlockValidationState state;
378-        if (!TestBlockValidity(state, chainparams, block, LookupBlockIndex(block.hashPrevBlock), false, false)) {
379+        if (!TestBlockValidity(state, chainparams, ::ChainstateActive(), block, g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), false, false)) {

jnewbery commented at 9:55 am on January 18, 2021:

Any reason not to just jump straight to using EnsureChainman(request.context) in all the rpc methods that you’re touching here?

dongcarl commented at 9:31 pm on January 20, 2021:

I don’t think it makes too much of a difference, and I found it clearer to push that decision to a later commit when we’ve resolved everything under RPC and can see everything that depends on chainman. See: 02e3831e9cd10f78bfde2d67a99c47f08564f07b

jnewbery commented at 11:10 am on January 28, 2021:

ok!

jnewbery commented at 9:59 am on January 18, 2021: member

I’ve reviewed everything up to 4124fb0946 validation: Pass in chainstate to TestBlockValidity. Just a few additional comments so far.

in src/validation.h:652 in 364fa81a2b outdated

648@@ -643,6 +649,8 @@ class CChainState
649     void PruneAndFlush();
650 
651     /**
652+     * Find the best known block, and make it the tip of the block chain

jnewbery commented at 12:12 pm on January 18, 2021:

There’s still some redundant text in here. Consider:

 0diff --git a/src/validation.h b/src/validation.h
 1index 10943b6c24..2e1c441ba5 100644
 2--- a/src/validation.h
 3+++ b/src/validation.h
 4@@ -649,18 +649,16 @@ public:
 5     void PruneAndFlush();
 6 
 7     /**
 8-     * Find the best known block, and make it the tip of the block chain
 9-     *
10-     * Make the best chain active, in multiple steps. The result is either failure
11-     * or an activated best chain. pblock is either nullptr or a pointer to a block
12-     * that is already loaded (to avoid loading it again from disk).
13+     * Find the best known block, and make it the tip of the block chain. The
14+     * result is either failure or an activated best chain. pblock is either
15+     * nullptr or a pointer to a block that is already loaded (to avoid loading
16+     * it again from disk).
17      *
18      * ActivateBestChain is split into steps (see ActivateBestChainStep) so that
19      * we avoid holding cs_main for an extended period of time; the length of this
20      * call may be quite long during reindexing or a substantial reorg.
21      *
22-     * May not be called with cs_main held. May not be called in a
23-     * validationinterface callback.
24+     * May not be called in a validationinterface callback.
25      *
26      * [@returns](/bitcoin-bitcoin/contributor/returns/) true unless a system error occurred
27      */

dongcarl commented at 8:50 pm on January 20, 2021:

Fixed in: d0454bad30253e735254cf7be359212293d952af

in src/validation.cpp:3665 in 364fa81a2b outdated

3650@@ -3650,8 +3651,8 @@ bool ChainstateManager::ProcessNewBlockHeaders(const std::vector<CBlockHeader>&
3651             }
3652         }
3653     }
3654-    if (NotifyHeaderTip()) {
3655-        if (::ChainstateActive().IsInitialBlockDownload() && ppindex && *ppindex) {
3656+    if (NotifyHeaderTip(ActiveChainstate())) {

jnewbery commented at 12:49 pm on January 18, 2021:

ActiveChainstate() doesn’t take cs_main, whereas ::ChainstateActive() does. Is that a problem? I imagine we shouldn’t be reading m_active_chainstate without cs_main. The comment says:

0    //! Once this pointer is set to a corresponding chainstate, it will not
1    //! be reset until init.cpp:Shutdown(). This means it is safe to acquire
2    //! the contents of this pointer with ::cs_main held, release the lock,
3    //! and then use the reference without concern of it being deconstructed.

Should m_active_chainstate be GUARDED_BY cs_main and ActiveChainstate() have EXCLUSIVE_LOCKS_REQUIRED(::cs_main)?

ryanofsky commented at 1:58 pm on January 20, 2021:

In commit “validation: Use existing chainstate in ChainstateManager::ProcessNewBlockHeaders” (e0ab7ee8ded2eb44fe2791a081703452012228e8)

re: #20749 (review)

ActiveChainstate() doesn’t take cs_main, whereas ::ChainstateActive() does. Is that a problem? I imagine we shouldn’t be reading m_active_chainstate without cs_main. The comment says:
0    //! Once this pointer is set to a corresponding chainstate, it will not
1    //! be reset until init.cpp:Shutdown(). This means it is safe to acquire
2    //! the contents of this pointer with ::cs_main held, release the lock,
3    //! and then use the reference without concern of it being deconstructed.
Should m_active_chainstate be GUARDED_BY cs_main and ActiveChainstate() have EXCLUSIVE_LOCKS_REQUIRED(::cs_main)?

Wow, good catch! I assumed ActiveChainstate() either acquired cs_main or required it, and didn’t know it was accessing the m_active_chainstate pointer with no protection. This is probably safe for now because the pointer never changes after initialization, but it will not be safe after #19806 which adds an ActivateSnapshot method updating the pointer.

In this particular place (this line and next line), I think it would be best not to call either ActiveChainstate or ChainstateActive because both could have a race condition after #19806 if the active chain state changes after cs_main is released in the code immediately above. Best would be to have a CChainState* chain_state local variable that is initialized while cs_main is held above, and then used here.

If we can add a EXCLUSIVE_LOCKS_REQUIRED annotation to ActiveChainstate and GUARDED_BY annotation to m_active_chainstate that would be gravy too, but this could also happen in #19806. Suggested it #19806 (review)

dongcarl commented at 10:36 pm on January 20, 2021:

Question: if the concern is that after #19806 the active chainstate may change after “cs_main is released in the code immediately above”, then that concern would still exist even if we did not switch from ::ChainstateActive to ActiveChainstate, no? In other words, this is not a new problem that my PR introduced?

As far as I can tell, the behaviour change that switching from ::ChainstateActive to ActiveChainstate introduces is just a possible race condition in ActiveChainstate() whereby a race condition can occur if m_active_chainstate is modified between its assert and its return, which I’d be happy to fix if we think it’s necessary.

jamesob commented at 2:59 pm on January 26, 2021:

For what it’s worth, I’d figured that the GUARDED_BY on g_chainman would’ve cascaded through to many of the usages in question, but maybe I misunderstood.

jnewbery commented at 11:26 am on January 28, 2021:

if the concern is that after #19806 the active chainstate may change after “cs_main is released in the code immediately above”, then that concern would still exist even if we did not switch from ::ChainstateActive to ActiveChainstate, no? In other words, this is not a new problem that my PR introduced?

The concern is that reading/writing m_active_chainstate is not guaranteed to be atomic. If this thread reads the variable while another thread is writing to it, then it could read the old value, the new value, or anything else. We have no guarantees at all about what is read if the value is not atomic (or guarded by a mutex to prevent multiple threads accessing it at the same time).

This seems fairly simple to solve - just make m_active_chainstate atomic.

dongcarl commented at 7:08 pm on January 28, 2021:

Fixed by rebasing over #21025, thanks all for the patience and guidance!

jnewbery commented at 12:51 pm on January 18, 2021: member

I’ve reviewed up to c948ff186b validation: Use accessible chainstate in ChainstateManager::ProcessNewBlock.

I’m not very familiar with the fuzzing framework so I didn’t review that commit.

in src/test/fuzz/load_external_block_file.cpp:18 in 364fa81a2b outdated

14@@ -15,7 +15,7 @@
15 
16 void initialize_load_external_block_file()
17 {
18-    InitializeFuzzingContext();
19+    static TestingSetup setup{CBaseChainParams::REGTEST, {"-nodebuglogfile"}};

ryanofsky commented at 2:38 pm on January 20, 2021:

In commit “fuzz: Initialize a full TestingSetup instead of BasicTestingSetup” (364fa81a2b014bd6257e011e6a201a48c1b8853e)

This seems safe and fine, but I’m only casually familiar with the fuzzing code and can’t say I understand what the goal of the change and new code actually is. It might be good to have a code comment saying what the purpose of TestingSetup/REGTEST/nodebuglogfile is here, or a commit description saying what was bad about the previous InitializeFuzzingContext call

dongcarl commented at 8:52 pm on January 20, 2021:

Added some context in the commit message here: 4c4eb967016d80247cbf38e6811c8640761ada42 Lmk if that needs additional explanation!

ryanofsky approved

ryanofsky commented at 2:45 pm on January 20, 2021: member

Code review ACK 364fa81a2b014bd6257e011e6a201a48c1b8853e. Only change since last review is new fuzzing commit

laanwj commented at 5:53 pm on January 20, 2021: member

Code review ACK 364fa81a2b014bd6257e011e6a201a48c1b8853e Checked that this only moves methods around and doesn’t change any validation logic.

dongcarl force-pushed on Jan 20, 2021

fanquake deleted a comment on Jan 20, 2021

DrahtBot added the label Needs rebase on Jan 21, 2021

MarcoFalke referenced this in commit 1f45e85509 on Jan 21, 2021

dongcarl force-pushed on Jan 21, 2021

dongcarl commented at 3:54 pm on January 21, 2021: member

Pushed d0454bad30253e735254cf7be359212293d952af -> 6ea8e92fc1105cf827acf6a72572d16696e2051c

Rebased over master to include #20946, dropped last commit accordingly

DrahtBot removed the label Needs rebase on Jan 21, 2021

sidhujag referenced this in commit c0b3bb2d2a on Jan 21, 2021

jamesob referenced this in commit 385cb331bb on Jan 26, 2021

glozow commented at 6:12 pm on January 26, 2021: member

Code Review ACK https://github.com/bitcoin/bitcoin/commit/6ea8e92fc1105cf827acf6a72572d16696e2051c

I’m new to the de-globalize chainman effort but fwiw I reviewed each commit noting where things were moving and why they should go there. I can see how LookupBlockIndex, FindForkInGlobalIndex, GetSpendHeight, GetLastCheckpoint belong as BlockManager functions. It’s nice to get rid of some globals in validation.cpp, especially with all these very very similar sounding names 🤢

It makes sense to me to access BlockMap through g_chainman.blockman instead of g_chainman.BlockIndex() - assertions that they point to the same thing are really helpful - unsure why CChainstateManager and CChainState both have references to the BlockMap in the first place?

dongcarl force-pushed on Jan 26, 2021

dongcarl commented at 8:13 pm on January 26, 2021: member

Pushed 6ea8e92fc1105cf827acf6a72572d16696e2051c -> 2bda16f4e46bf361bcfa600a4e751e6303a63d0d

Rebased over master to include #20972, dropped cs_main locking according to #20749 (review)

glozow commented at 1:11 pm on January 27, 2021: member

reACK https://github.com/bitcoin/bitcoin/commit/2bda16f4e46bf361bcfa600a4e751e6303a63d0d, only change is dropping repeat-lock of cs_main

laanwj commented at 1:58 pm on January 27, 2021: member

re-ACK 2bda16f4e46bf361bcfa600a4e751e6303a63d0d

dongcarl commented at 0:50 am on January 28, 2021: member

If anyone was wondering about resolving the AssumeUTXO changes in #19806 with the deglobalizing-chainman changes, here’s an update: #19806 (comment).

The conclusion was that the PRs are mergeable in any order without much pain if the last commit of the AssumeUTXO changes in #19806 is slightly modified.

in src/validation.h:15 in 2bda16f4e4 outdated

11@@ -12,6 +12,7 @@
12 
13 #include <amount.h>
14 #include <coins.h>
15+#include <chainparams.h>

jnewbery commented at 10:32 am on January 28, 2021:

This isn’t required. You can just forward declare CCheckpointData and then include chainparams.h in validation.cpp (if a type is only used as a reference argument for a function declaration, you don’t need the full definition for that type).

dongcarl commented at 7:04 pm on January 28, 2021:

Fixed!

in src/validation.h:545 in 2bda16f4e4 outdated

545     std::unique_ptr<CoinsViews> m_coins_views;
546 
547 public:
548+    //! Reference to a BlockManager instance which itself is shared across all
549+    //! CChainState instances. Keeping a local reference allows us to test more
550+    //! easily as opposed to referencing a global.

jnewbery commented at 10:36 am on January 28, 2021:

“Keeping a local reference allows us to test more easily as opposed to referencing a global.” is outdated, since this reference is no longer used just for testing and is part of the public interface.

in src/validation.h:288 in 2bda16f4e4 outdated

282@@ -291,7 +283,13 @@ bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex);
283 bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
284 
285 /** Check a block is completely valid from start to finish (only works on top of our current best block) */
286-bool TestBlockValidity(BlockValidationState& state, const CChainParams& chainparams, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckPOW = true, bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
287+bool TestBlockValidity(BlockValidationState& state,
288+                       const CChainParams& chainparams,
289+                       CChainState& chainstate,

jnewbery commented at 10:39 am on January 28, 2021:

I was going to ask if you could const CChainState& to indicate that you’re not mutating the chainstate…

… but in fact TestBlockValidity() connects the passed-in block to the chain tip. What a terrible name for this function!

in src/validation.cpp:3802 in 2bda16f4e4 outdated

3798             return error("%s: AcceptBlock FAILED (%s)", __func__, state.ToString());
3799         }
3800     }
3801 
3802-    NotifyHeaderTip();
3803+    NotifyHeaderTip(ActiveChainstate());

jnewbery commented at 11:29 am on January 28, 2021:

Same problem as above: you’re grabbing m_active_chainstate without locking cs_main first. I think it’d be easiest to make a local CChainstate* variable and set it while holding cs_main.

dongcarl commented at 7:07 pm on January 28, 2021:

Fixed by rebasing over #21025

dongcarl referenced this in commit c1d4af79b3 on Jan 28, 2021

dongcarl referenced this in commit 5b8118e0fd on Jan 28, 2021

dongcarl force-pushed on Jan 28, 2021

dongcarl commented at 7:03 pm on January 28, 2021: member

Pushed 2bda16f4e46bf361bcfa600a4e751e6303a63d0d -> 246cb0f29b01e9b24cd13388dfd38c4b0729783c

Rebased on top of #21025 to fix locking
Addressed jnewbery’s review comments (thanks!)

validation: Guard the active_chainstate with cs_main

This avoids a potential race-condition where a thread is reading the
ChainstateManager::m_active_chainstate pointer while another one is
writing to it. There is no portable guarantee that reading/writing the
pointer is thread-safe.

This is also done in way that mimics ::ChainstateActive(), so the
transition from that function to this method is easy.

More discussion:
1. https://github.com/bitcoin/bitcoin/pull/20749#discussion_r559544027
2. https://github.com/bitcoin/bitcoin/pull/19806#discussion_r561023961
3. https://github.com/bitcoin/bitcoin/pull/19806#issuecomment-768946522
4. https://github.com/bitcoin/bitcoin/pull/19806#issuecomment-768955695

f92dc6557a

validation: Move LookupBlockIndex to BlockManager

[META] This commit should be followed up by a scripted-diff commit which
       fixes calls to LookupBlockIndex tree-wide.
[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

LookupBlockIndex only acts on BlockManager.

15d20f40e1

scripted-diff: Use BlockManager::LookupBlockIndex

[META] In a previous commit, we moved ::LookupBlockIndex to become a
       member function of BlockManager. This commit is split out from
       that one since it can be expressed nicely as a scripted-diff.

-BEGIN VERIFY SCRIPT-
find_regex='LookupBlockIndex' \
    && git grep -l -E "$find_regex" -- src \
        | grep -v '^src/validation\.\(cpp\|h\)$' \
        | xargs sed -i -E "s@${find_regex}@g_chainman.m_blockman.LookupBlockIndex@g"
-END VERIFY SCRIPT-

eae54e6e60

validation: Remove global LookupBlockIndex 3664a150ac

validation: Move FindForkInGlobalIndex to BlockManager

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

FindForkInGlobalIndex only acts on BlockManager.

Note to reviewers: Since FindForkInGlobalIndex is always called with
::ChainActive() as its first parameter, it is possible to move
FindForkInGlobalIndex to CChainState and remove this const CChain&
parameter to instead use m_chain. However, it seems like the original
intention was for FindForkInGlobalIndex to work with _any_ chain, not
just the current active chain. Let me know if this should be changed.

b026e318c3

validation: Move GetSpendHeight to BlockManager

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

GetSpendHeight only acts on BlockManager.

e4b95eefbc

validation: Move GetLastCheckpoint to BlockManager

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

GetLastCheckPoint mainly acts on BlockManager.

f11d11600d

validation: Pass in blockman to ContextualCheckBlockHeader

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

d363d06bf7

validation: Make CChainState.m_blockman public 0e17c833cd

validation: Pass in chainstate to TestBlockValidity

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

9c300cc8b3

validation: Pass in chainstate to ::NotifyHeaderTip

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

2a696472a1

validation: Remove global ::ActivateBestChain

Instead use CChainState::ActivateBestChain, which is what the global one
calls anyway.

5f8cd7b3a5

validation: Move LoadExternalBlockFile to CChainState

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

LoadExternalBlockFile mainly acts on CChainState.

e0dc305727

validation: Use existing chainstate in ChainstateManager::ProcessNewBlockHeaders

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

ea4fed9021

validation: Use accessible chainstate in ChainstateManager::ProcessNewBlock

[META] This commit should be followed up by removing the comments and
       assertions meant only to show that the change is correct.

0cdad75390

style-only: Make TestBlockValidity signature readable b8e95658d5

style-only: Remove redundant sentence in ActivateBestChain comment 67c9a83df1

dongcarl force-pushed on Jan 28, 2021

dongcarl commented at 7:16 pm on January 28, 2021: member

Pushed 246cb0f29b01e9b24cd13388dfd38c4b0729783c -> 67c9a83df19c6e2a2edb32336879204e7770b4a7

Rebased over new changes in #21025

jnewbery commented at 7:27 pm on January 28, 2021: member

utACK 67c9a83df19c6e2a2edb32336879204e7770b4a7

dongcarl referenced this in commit e6eb8d2bbb on Jan 28, 2021

ryanofsky approved

ryanofsky commented at 0:10 am on January 29, 2021: member

Code review ACK 67c9a83df19c6e2a2edb32336879204e7770b4a7. Changes since last review:

new first commit adding guarded by for active chain state, and a few minimal (recursive?) locks to satisfy the annotation. Hopefully these can be replaced later by annotations and appropriately scoped locking
removing redundant getspendheight lock
include/comment tweaks

It would be good to see suggestion from #20749 (review) followed up at some point: for ProcessNewBlock or functions like it to avoid calling ActiveChainstate() repeatedly without locking, and instead assign the chainstate they want to use to a variable and use it consistently. (This might be done already in one of the followup PRs)

jnewbery commented at 9:34 am on February 1, 2021: member

If @laanwj reACKs this, then I think it’ll be ready for merge

laanwj commented at 12:04 pm on February 1, 2021: member

re-ACK 67c9a83df19c6e2a2edb32336879204e7770b4a7

laanwj merged this on Feb 1, 2021

laanwj closed this on Feb 1, 2021

hebasto commented at 12:43 pm on February 1, 2021: member

Warnings are fixed in #21051.

sidhujag referenced this in commit 60a5781c0d on Feb 2, 2021

fanquake referenced this in commit f72d80b07a on Feb 2, 2021

sidhujag referenced this in commit 3a79b1874b on Feb 3, 2021

MarcoFalke referenced this in commit f1239b70d1 on Feb 4, 2021

jamesob referenced this in commit 74c8167b58 on Feb 4, 2021

sidhujag referenced this in commit 203242fcfc on Feb 4, 2021

laanwj removed this from the "Blockers" column in a project

Fabcien referenced this in commit 9ef337192e on Mar 16, 2022

Fabcien referenced this in commit 292bd98718 on Mar 16, 2022

Fabcien referenced this in commit 391e49e3db on Mar 16, 2022

Fabcien referenced this in commit 9f6029d777 on Mar 16, 2022

Fabcien referenced this in commit 8b9fd2199b on Mar 16, 2022

Fabcien referenced this in commit 09391f4324 on Mar 16, 2022

Fabcien referenced this in commit 9ab77c295f on Mar 16, 2022

Fabcien referenced this in commit 0044baa648 on Mar 16, 2022

Fabcien referenced this in commit ffd0454d64 on Mar 16, 2022

Fabcien referenced this in commit 4c17534749 on Mar 16, 2022

Fabcien referenced this in commit 92999ca3c9 on Mar 16, 2022

Fabcien referenced this in commit e9bcf5e92c on Mar 16, 2022

Fabcien referenced this in commit df881c7b93 on Mar 16, 2022

DrahtBot locked this on Aug 16, 2022

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

Conflicts