[Bundle 4/n] Prune g_chainman usage in validation-adjacent modules

dongcarl commented at 5:26 pm on February 22, 2021: member

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

Based on:

#21055 | [Bundle 3/n] Prune g_chainman usage in mempool-related validation functions

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

dongcarl added the label Refactoring on Feb 22, 2021

dongcarl added the label Validation on Feb 22, 2021

DrahtBot commented at 0:55 am on February 23, 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:

#21236 (Net processing: Extract addr send functionality into MaybeSendAddr() by jnewbery)
#21235 (p2p: Clarify disconnect log message in ProcessGetBlockData, remove send bool by MarcoFalke)
#21186 (Net/Net Processing: Move addr data into net_processing by jnewbery)
#21061 ([p2p] Introduce node rebroadcast module by amitiuttarwar)
#21009 (Remove RewindBlockIndex logic by dhruv)
#19521 (Coinstats Index 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.

DrahtBot commented at 10:01 am on March 4, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Mar 4, 2021

ryanofsky approved

ryanofsky commented at 3:07 pm on March 6, 2021: member

Code review ACK last 11 commits only a0741fa4d5acb567122d0d43c725f8f64fd75eaf. Changes since last review in #20158#pullrequestreview-581753681: rebase, and a few more functions moved in move static functions commit

c8382b98a493635ffe57ad5daa5e03fdecdb3b77 miner: Pass in chainstate to BlockAssembler::CreateNewBlock (17/27)
c625552140ce8b9845f657a04963573a98daaba5 scripted-diff: Invoke CreateNewBlock with chainstate (18/27)
b186b2f2129dcfac52fb2dea9230c6cf29a952c2 miner: Remove old CreateNewBlock w/o chainstate param (19/27)
588a0c75d7b139dc572866dd92de7177adfb5136 miner: Pass in blockman to ::RegenerateCommitments (20/27)
c46ff650d3b55f2e5a1de5a28a4946aedc357368 node/coinstats: Pass in BlockManager to GetUTXOStats (21/27)
62ab369eacb4d39fe74df12697b8a410636116c9 node: Use existing NodeContext (22/27)
c975f64b7e742105ab3cfb10596fdbaaba8bec7a node/ifaces: NodeImpl: Use existing NodeContext member (23/27)
6723fe829672cd6b5487e690852aff180d8a4106 node/ifaces: ChainImpl: Use existing NodeContext member (24/27)
15276edbbee67941d1483504eace147dcfbf25ec net_processing: Move some static functions to PeerManager (25/27)
0ac24f921ebe33027466ec9aaf75201fef5622db scripted-diff: net_processing: Use existing chainman (26/27)
a0741fa4d5acb567122d0d43c725f8f64fd75eaf net_processing: Add review-only assertion to PeerManager (27/27)

jnewbery commented at 6:01 pm on March 6, 2021: member

Concept ACK. Will review after rebase.

dongcarl force-pushed on Mar 8, 2021

dongcarl commented at 8:36 pm on March 8, 2021: member

Pushed a0741fa4d5acb567122d0d43c725f8f64fd75eaf -> 38c325b1df093b2ff380df93c3a518286c531f32

Rebased over master

validation: Remove extraneous LoadGenesisBlock function prototype

Leftover from last bundle.

a04aac493f

miner: Pass in chainstate to BlockAssembler::CreateNewBlock d0de61b764

scripted-diff: Invoke CreateNewBlock with chainstate

-BEGIN VERIFY SCRIPT-
find_regex='(\.|->)CreateNewBlock\(' \
    && git grep -l -E "$find_regex" -- src \
        | grep -v '^src/miner\.\(cpp\|h\)$' \
        | xargs sed -i -E 's@'"$find_regex"'@\0::ChainstateActive(), @g'
-END VERIFY SCRIPT-

46b7f29340

miner: Remove old CreateNewBlock w/o chainstate param 2afcf24408

miner: Pass in blockman to ::RegenerateCommitments

REQUIRES ATTENTION

2c3ba00693

node/coinstats: Pass in BlockManager to GetUTXOStats 106bcd4f39

node: Use existing NodeContext 4cde4a701b

node/ifaces: NodeImpl: Use existing NodeContext member 8a1d580b21

node/ifaces: ChainImpl: Use existing NodeContext member 91c5b68acd

net_processing: Move some static functions to PeerManager

- BlockRequestAllowed
- AlreadyHaveBlock
- ProcessGetBlockData
- PrepareBlockFilterRequest
- ProcessGetCFilters
- ProcessGetCFHeaders
- ProcessGetCFCheckPt

Moved out of anonymous namespace:
- ProcessBlockAvailability
- UpdateBlockAvailability
- CanDirectFetch

021a04a469

scripted-diff: net_processing: Use existing chainman

-BEGIN VERIFY SCRIPT-
sed -i -E \
    -e 's/g_chainman/m_chainman/g' \
    -e 's@([^:])(Chain(state|)Active)@\1::\2@g' \
    -e 's@::Chain(state|)Active\(\)@m_chainman.ActiveChain\1()@g' \
    -- src/net_processing.cpp
-END VERIFY SCRIPT-

272d993e75

net_processing: Add review-only assertion to PeerManager a67983cd6d

dongcarl force-pushed on Mar 8, 2021

dongcarl commented at 8:57 pm on March 8, 2021: member

Pushed 38c325b1df093b2ff380df93c3a518286c531f32 -> a67983cd6d8e61565da4e03f3ba401d0148fe195

Addressed #21270#commitcomment-47985104 in prepended commit
Fixed namespace brace syntax mistake in “net_processing: Move some static functions to PeerManager”

dongcarl removed the label Needs rebase on Mar 8, 2021

fanquake referenced this in commit 4927c9e699 on Mar 9, 2021

ryanofsky approved

ryanofsky commented at 1:53 pm on March 9, 2021: member

Code review ACK a67983cd6d8e61565da4e03f3ba401d0148fe195. Only change since last review new first commit fixing header declaration, and rebase

in src/miner.cpp:48 in a67983cd6d

45     CMutableTransaction tx{*block.vtx.at(0)};
46     tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));
47     block.vtx.at(0) = MakeTransactionRef(tx);
48 
49-    GenerateCoinbaseCommitment(block, WITH_LOCK(cs_main, return g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());
50+    GenerateCoinbaseCommitment(block, WITH_LOCK(::cs_main, assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman)); return blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());

glozow commented at 0:53 am on March 11, 2021:

I think this line would give jnewbery a heart attack…

0    GenerateCoinbaseCommitment(block,
1                               WITH_LOCK(::cs_main,
2                                         assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman));
3                                         return blockman.LookupBlockIndex(block.hashPrevBlock)),
4                               Params().GetConsensus());

laanwj commented at 8:04 am on March 11, 2021:

This is a really strange C++ construction to me, to have assertions and return statements inside a function argument. It is likely out of scope of this PR to refactor it though. I’m not sure splitting it up over multiple lines makes it much more insightful. (the change here didn’t make it much worse)

MarcoFalke commented at 8:56 am on March 11, 2021:

The assert will go away eventually, if I understood correctly, so the line is only temporarily longer than it previously was

laanwj commented at 10:49 am on March 11, 2021:

Right :smile:

ghostofjnewbery commented at 11:15 am on March 11, 2021:

I had a heart attack, but I’ve returned from beyond the grave to relay this warning from the underworld. Do not make 150 column function calls containing asserts and return statements, lest one day you should have to read and understand that code.

Consider doing this instead. It’s a lot less spooky:

0    CBlockIndex* index;
1    {
2        LOCK(::cs_main);
3        assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman));
4        index = blockman.LookupBlockIndex(block.hashPrevBlock);
5    }
6    GenerateCoinbaseCommitment(block, index, Params().GetConsensus());

glozow commented at 1:49 am on March 11, 2021: member

code review ACK a67983cd6d8e61565da4e03f3ba401d0148fe195

Reviewed commits individually. It helped to git show 021a04a --color-moved=dimmed-zebra on the net_processing: Move some static functions to PeerManager commit.

Question: I forget if this was already mentioned somewhere, but why isn’t LookupBlockIndex a const member function? Otherwise could be passing const BlockManager&

laanwj commented at 10:46 am on March 11, 2021: member

Code review ACK a67983cd6d8e61565da4e03f3ba401d0148fe195

laanwj merged this on Mar 11, 2021

laanwj closed this on Mar 11, 2021

in src/miner.cpp:42 in a67983cd6d

38@@ -39,13 +39,13 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
39     return nNewTime - nOldTime;
40 }
41 
42-void RegenerateCommitments(CBlock& block)
43+void RegenerateCommitments(CBlock& block, BlockManager& blockman)

jnewbery commented at 11:36 am on March 11, 2021:

Did you consider changing the function signature here to accept a CBlockIndex*, which you fetch from blockman before calling the function? That seems like a more natural interface, and would avoid some of the difficult to read WITH_LOCK() in function calls.

in src/node/coinstats.h:11 in a67983cd6d

 7@@ -8,6 +8,7 @@
 8 
 9 #include <amount.h>
10 #include <uint256.h>
11+#include <validation.h>

jnewbery commented at 11:42 am on March 11, 2021:

No need for this include. You can just forward declare BlockManager

fanquake commented at 5:40 pm on January 18, 2023:

Done in #21525.

in src/node/coinstats.h:40 in a67983cd6d

36@@ -36,6 +37,6 @@ struct CCoinsStats
37 };
38 
39 //! Calculate statistics about the unspent transaction output set
40-bool GetUTXOStats(CCoinsView* view, CCoinsStats& stats, const CoinStatsHashType hash_type, const std::function<void()>& interruption_point = {});
41+bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, const CoinStatsHashType hash_type, const std::function<void()>& interruption_point = {});

jnewbery commented at 11:45 am on March 11, 2021:

Could this be changed to take just a CChainstate&, and get the CCoinsView and BlockManager from there?

in src/node/interfaces.cpp:214 in a67983cd6d

211         }
212         return GuessVerificationProgress(Params().TxData(), tip);
213     }
214-    bool isInitialBlockDownload() override { return ::ChainstateActive().IsInitialBlockDownload(); }
215+    bool isInitialBlockDownload() override {
216+        assert(std::addressof(::ChainstateActive()) == std::addressof(m_context->chainman->ActiveChainstate()));

jnewbery commented at 11:51 am on March 11, 2021:

Should this assert happen while holding cs_main? Both ChainstateActive() and ActiveChainstate() take cs_main, but there’s a race condition where it changes between those locks.

in src/node/interfaces.cpp:496 in a67983cd6d

490@@ -479,17 +491,21 @@ class ChainImpl : public Chain
491     {
492         WAIT_LOCK(cs_main, lock);
493         const CChain& active = Assert(m_node.chainman)->ActiveChain();
494-        const CBlockIndex* block = g_chainman.m_blockman.LookupBlockIndex(block_hash);
495-        const CBlockIndex* ancestor = g_chainman.m_blockman.LookupBlockIndex(ancestor_hash);
496+        assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman));
497+        const CBlockIndex* block = m_node.chainman->m_blockman.LookupBlockIndex(block_hash);
498+        assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman));

jnewbery commented at 11:55 am on March 11, 2021:

This second assert seems unnecessary. cs_main is held throughout this function. Same below for findCommonAncestor.

in src/node/interfaces.cpp:617 in a67983cd6d

612@@ -595,7 +613,10 @@ class ChainImpl : public Chain
613         return ::fHavePruned;
614     }
615     bool isReadyToBroadcast() override { return !::fImporting && !::fReindex && !isInitialBlockDownload(); }
616-    bool isInitialBlockDownload() override { return ::ChainstateActive().IsInitialBlockDownload(); }
617+    bool isInitialBlockDownload() override {
618+        assert(std::addressof(::ChainstateActive()) == std::addressof(m_node.chainman->ActiveChainstate()));

jnewbery commented at 11:56 am on March 11, 2021:

Same comment as above about holding cs_main for this assert.

in src/net_processing.cpp:687 in a67983cd6d

701@@ -684,41 +702,6 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, co
702     return true;
703 }
704 
705-/** Check whether the last unknown block a peer advertised is not yet known. */

jnewbery commented at 11:59 am on March 11, 2021:

Can you move the function comments to the declarations now that they’re no longer static?

fanquake commented at 5:42 pm on January 18, 2023:

Done in #21525

in src/net_processing.cpp:1221 in a67983cd6d

1217@@ -1200,10 +1218,10 @@ bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat
1218 // active chain if they are no more than a month older (both in time, and in
1219 // best equivalent proof of work) than the best header chain we know about and
1220 // we fully-validated them at some point.
1221-static bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1222+bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams)

jnewbery commented at 12:00 pm on March 11, 2021:

None of these functions need to take Consensus::Params& now that they’re members of PeerManagerImpl. They can just use PeerManagerImpl.m_chainparams.consensus.

in src/net_processing.cpp:1942 in a67983cd6d

1938@@ -1920,27 +1939,27 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer,
1939         // because it is set in UpdateBlockAvailability. Some nullptr checks
1940         // are still present, however, as belt-and-suspenders.
1941 
1942-        if (received_new_header && pindexLast->nChainWork > ::ChainActive().Tip()->nChainWork) {
1943+        if (received_new_header && pindexLast->nChainWork > m_chainman.ActiveChain().Tip()->nChainWork) {

jnewbery commented at 1:25 pm on March 11, 2021:

This sequence of m_chainman.ActiveChain() function calls is repeatedly taking a cs_main lock and releasing it again (while holding cs_main the whole time). That’s fine, and correct, but perhaps it’d be better to just take a local const CChainstate& variable once to avoid the multiple function calls.

jnewbery commented at 1:33 pm on March 11, 2021: member

Question: I forget if this was already mentioned somewhere, but why isn’t LookupBlockIndex a const member function? Otherwise could be passing const BlockManager&

I can’t see any reason why LookupBlockIndex shouldn’t be a const function. Changing all the places that BlockManager& is passed to pass const BlockManager& seems like it’d be a big improvement and indicate to the reader that the function won’t mutate the BlockManager.

jnewbery commented at 1:38 pm on March 11, 2021: member

Looks good to me. A few comments inline for your consideration for follow-up PRs.

It seems a shame that net_processing and other components need to make so many calls into ActiveChainstate() and ActiveChain(), simply to call IsInitialBlockDownload(), ActivateBestChain(), or another global function, or pass the received pointer back to a validation function. It suggests to me that the interface we have between net_processing and validation isn’t quite right here. net_processing really shouldn’t have to know which chainstate is active for most of its purposes.

sidhujag referenced this in commit 86a818d103 on Mar 11, 2021

in src/miner.cpp:102 in a67983cd6d

 98@@ -99,7 +99,7 @@ void BlockAssembler::resetBlock()
 99 Optional<int64_t> BlockAssembler::m_last_block_num_txs{nullopt};
100 Optional<int64_t> BlockAssembler::m_last_block_weight{nullopt};
101 
102-std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
103+std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(CChainState& chainstate, const CScript& scriptPubKeyIn)

MarcoFalke commented at 11:14 am on March 12, 2021:

This should really be a member variable, like the mempool. I’d doubt it would make sense to pass a different chainstate reference than the one the mempool is based on.

in src/net_processing.cpp:478 in a67983cd6d

471@@ -472,6 +472,24 @@ class PeerManagerImpl final : public PeerManager
472     std::vector<std::pair<uint256, CTransactionRef>> vExtraTxnForCompact GUARDED_BY(g_cs_orphans);
473     /** Offset into vExtraTxnForCompact to insert the next tx */
474     size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0;
475+
476+    void ProcessBlockAvailability(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
477+    void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
478+    bool CanDirectFetch(const Consensus::Params &consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

MarcoFalke commented at 11:15 am on March 12, 2021:

Isn’t params a member? Seems odd to pass twice

MarcoFalke commented at 12:09 pm on March 12, 2021:

#21425

in src/node/coin.cpp:16 in a67983cd6d

11@@ -12,7 +12,8 @@ void FindCoins(const NodeContext& node, std::map<COutPoint, Coin>& coins)
12 {
13     assert(node.mempool);
14     LOCK2(cs_main, node.mempool->cs);
15-    CCoinsViewCache& chain_view = ::ChainstateActive().CoinsTip();
16+    assert(std::addressof(::ChainstateActive()) == std::addressof(node.chainman->ActiveChainstate()));
17+    CCoinsViewCache& chain_view = node.chainman->ActiveChainstate().CoinsTip();

MarcoFalke commented at 11:15 am on March 12, 2021:

Any reason to prefer UB over a clean assert, like it is done for node.mempool above?

in src/node/interfaces.cpp:186 in a67983cd6d

181@@ -182,18 +182,21 @@ class NodeImpl : public Node
182     int getNumBlocks() override
183     {
184         LOCK(::cs_main);
185-        return ::ChainActive().Height();
186+        assert(std::addressof(::ChainActive()) == std::addressof(m_context->chainman->ActiveChain()));
187+        return m_context->chainman->ActiveChain().Height();

MarcoFalke commented at 11:17 am on March 12, 2021:

same

in src/node/transaction.cpp:45 in a67983cd6d

38@@ -39,9 +39,10 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
39 
40     { // cs_main scope
41     LOCK(cs_main);
42+    assert(std::addressof(::ChainstateActive()) == std::addressof(node.chainman->ActiveChainstate()));
43     // If the transaction is already confirmed in the chain, don't do anything
44     // and return early.
45-    CCoinsViewCache &view = ::ChainstateActive().CoinsTip();
46+    CCoinsViewCache &view = node.chainman->ActiveChainstate().CoinsTip();

MarcoFalke commented at 11:22 am on March 12, 2021:

same

dongcarl commented at 10:12 pm on March 12, 2021: member

Thanks everyone for the diligent reviews! I will prepend commits in Bundle 5 when I get the chance and link back to the convo here, we can continue the convos over on the Bundle 5 thread!

MarcoFalke referenced this in commit 80a699fda9 on Apr 1, 2021

MarcoFalke referenced this in commit 0dd7b23489 on Apr 17, 2021

Fabcien referenced this in commit 2e4442d716 on Mar 17, 2022

Fabcien referenced this in commit d1b32162fe on Mar 21, 2022

Fabcien referenced this in commit 9813976d76 on May 9, 2022

Fabcien referenced this in commit a701d5196e on May 9, 2022

Fabcien referenced this in commit 01a7dd7dc3 on May 9, 2022

Fabcien referenced this in commit bea681fd0d on May 9, 2022

Fabcien referenced this in commit 4dbe0b84bf on May 19, 2022

Fabcien referenced this in commit 2af932fc29 on May 19, 2022

DrahtBot locked this on Aug 16, 2022

[Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270

Conflicts