[Bundle 5/n] Prune g_chainman usage in RPC modules

dongcarl commented at 9:18 pm on March 8, 2021: member

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

Based on:

#21270 | [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules
#21525 | [Bundle 4.5/n] Followup fixups to bundle 4

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 Mining on Mar 8, 2021

DrahtBot added the label P2P on Mar 8, 2021

DrahtBot added the label RPC/REST/ZMQ on Mar 8, 2021

DrahtBot added the label Validation on Mar 8, 2021

DrahtBot commented at 4:25 am on March 9, 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:

#21526 (validation: UpdateTip/CheckBlockIndex assumeutxo support by jamesob)
#21523 (validation: run VerifyDB on all chainstates by jamesob)
#21422 (Add feerate histogram to getmempoolinfo by kiminuo)
#20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
#20017 (rpc: Add RPCContext by promag)
#19521 (Coinstats Index by fjahr)
#19438 (Introduce deploymentstatus by ajtowns)
#19391 (RPC/Mining: Clean out pre-Segwit miner compatibility code by luke-jr)

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.

ryanofsky approved

ryanofsky commented at 2:14 pm on March 9, 2021: member

Code review ACK last 6 commits only a041676d3905002758a9c1827b410ea87d3329cf. Many chainmans.

384c56d16e724344f092192d31ef01a529f1bfec rpc/*,rest: Add review-only assertion to EnsureChainman (14/19)
b6d384784f9cf272f1a56600a770faf8d2d8f05b rpc/blockchain: Use existing NodeContext (15/19)
2b0fb0e2219f3ffe5dee1e45ad519ff427fa01b6 rpc/mining: Use existing NodeContext (16/19)
8e3d4558a1d6ea55279642e4a9279d4b4c31ed69 rpc/rawtx: Use existing NodeContext (17/19)
cbe4f7ba2165518800ffd69757b5ec79fac9a7c9 rest: Pass in NodeContext to rest_block (18/19)
a041676d3905002758a9c1827b410ea87d3329cf rest: Use existing NodeContext (19/19)

dongcarl force-pushed on Mar 9, 2021

dongcarl commented at 5:40 pm on March 9, 2021: member

Pushed a041676d3905002758a9c1827b410ea87d3329cf -> 6b89f537bad5fc3761ef3ada4cc5c9ac213c0ae6

Fixed assert linter complaint by using CHECK_NONFATAL instead of assert

DrahtBot commented at 4:49 pm on March 15, 2021: member

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

MarcoFalke removed the label Mining on Mar 15, 2021

MarcoFalke removed the label P2P on Mar 15, 2021

MarcoFalke removed the label RPC/REST/ZMQ on Mar 15, 2021

MarcoFalke added the label Refactoring on Mar 15, 2021

MarcoFalke commented at 6:27 pm on March 15, 2021: member

(Needs rebase)

dongcarl force-pushed on Mar 17, 2021

dongcarl commented at 9:24 pm on March 17, 2021: member

Pushed 6b89f537bad5fc3761ef3ada4cc5c9ac213c0ae6 -> b57dc39eae25294071959b1eec42bd7dd5563357

Rebased over master
Prepended fixes to last bundle

Note to reviewers:

I’ve made some changes in 5de3e3609ab591d88281558182efdec39b065541 that requires some attention, mostly I had to make decisions about default values of getNumBlocks, isInitialBlockDownload, getUnspentOutput, and checkFinalTx when our context doesn’t contain a chainman
The commits before b7ed70dba191881d79fe71a746a2a826f40de884 should address the following posthumous reviews:
- From jnewbery:
  - #21270 (comment)
    - I didn’t fix this one, but I added a TODO comment so that we don’t lost track of it
  - #21270 (review)
  - #21270 (review)
  - #21270 (review)
  - #21270 (review)
- From MarcoFalke:

I have also went through all my changes in the last bundle and ensured that we didn’t miss any potential UB-introductions.

A few explanations:

#21270 (review) Unfortunately GetUTXOStats is fuzzed by constructing a CCoinsView* as an input, so this signature needs to remain.
#21270 (review) 100% agree that it’s unnecessary, but going to leave it be for now since it’ll be removed in the end if that’s okay with folks.
#21270 (review) Right, it would most likely be better to take a local const CChainstate& once in places where we’re already taking cs_main, let’s do this in a followup!

jnewbery commented at 10:47 pm on March 17, 2021: member

Concept ACK. Thanks for addressing review comments from the last bundle!

in src/miner.h:149 in b57dc39eae outdated

145@@ -146,6 +146,7 @@ class BlockAssembler
146     int64_t nLockTimeCutoff;
147     const CChainParams& chainparams;
148     const CTxMemPool& m_mempool;
149+    CChainState& m_chainstate;

MarcoFalke commented at 7:05 am on March 18, 2021:

nit: Can this be const?

dongcarl commented at 5:10 pm on March 24, 2021:

Unfortunately not… We pass m_chainstate to TestBlockValidity in BlockAssembler::CreateNewBlock, and TestBlockValidity requires its CChainState param to be non-const

in src/node/interfaces.cpp:655 in b57dc39eae outdated

650@@ -621,8 +651,13 @@ class ChainImpl : public Chain
651     }
652     bool isReadyToBroadcast() override { return !::fImporting && !::fReindex && !isInitialBlockDownload(); }
653     bool isInitialBlockDownload() override {
654-        assert(std::addressof(::ChainstateActive()) == std::addressof(m_node.chainman->ActiveChainstate()));
655-        return m_node.chainman->ActiveChainstate().IsInitialBlockDownload();
656+        const CChainState* active_chainstate;
657+        if (m_node.chainman) {

MarcoFalke commented at 7:13 am on March 18, 2021:

does it make sense to run a node without a chainman? It might make more sense to use Assert, like it is already done in waitForNotificationsIfTipChanged.

Maybe the assert can even be wrapped into a private member:

0private:
1 ChainstateManager& chainman() { return *Assert(m_node.chainman); }

in src/node/transaction.cpp:41 in b57dc39eae outdated

38@@ -39,6 +39,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
39 
40     { // cs_main scope
41     LOCK(cs_main);
42+    assert(node.chainman);

MarcoFalke commented at 7:13 am on March 18, 2021:

would be nice to assert before cs_main, because cs_main “belongs” to the chainman

in src/node/coin.cpp:14 in b57dc39eae outdated

11@@ -12,6 +12,7 @@ void FindCoins(const NodeContext& node, std::map<COutPoint, Coin>& coins)
12 {
13     assert(node.mempool);
14     LOCK2(cs_main, node.mempool->cs);
15+    assert(node.chainman);

MarcoFalke commented at 7:14 am on March 18, 2021:

would be nice to assert before cs_main, because cs_main “belongs” to the chainman

MarcoFalke commented at 7:16 am on March 18, 2021: member

left some questions

DrahtBot added the label Needs rebase on Mar 18, 2021

in src/net_processing.cpp:486 in b57dc39eae outdated

481     bool CanDirectFetch(const Consensus::Params &consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
482+
483+    //////////////////////////////////////////////////////////////////////////////
484+    //
485+    // blockchain -> download logic notification
486+    //

jnewbery commented at 4:12 pm on March 18, 2021:

Just remove this section marker.

in src/net_processing.cpp:491 in b57dc39eae outdated

486+    //
487+
488+    // To prevent fingerprinting attacks, only send blocks/headers outside of the
489+    // active chain if they are no more than a month older (both in time, and in
490+    // best equivalent proof of work) than the best header chain we know about and
491+    // we fully-validated them at some point.

jnewbery commented at 4:13 pm on March 18, 2021:

Perhaps make this a doxygen comment (ie start with /**)

in src/net_processing.cpp:1245 in b57dc39eae outdated

1242@@ -1185,15 +1243,6 @@ bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat
1243 }
1244 
1245

jnewbery commented at 4:13 pm on March 18, 2021:

Remove this extra blank line? :grimacing:

in src/node/interfaces.cpp:185 in b57dc39eae outdated

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

jnewbery commented at 4:15 pm on March 18, 2021:

I think it’d be just as clear (and a smaller diff) to invert the conditional here:

0        if (!m_context->chainman) return 0;
1        // Logic that uses chainman ...

in src/node/interfaces.cpp:195 in b57dc39eae outdated

195     uint256 getBestBlockHash() override
196     {
197-        assert(std::addressof(::ChainActive()) == std::addressof(m_context->chainman->ActiveChain()));
198-        const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_context->chainman->ActiveChain().Tip());
199+        const CBlockIndex* tip;
200+        if (m_context->chainman) {

jnewbery commented at 4:15 pm on March 18, 2021:

Same here. You don’t need this local variable if you exit early when chainman doesn’t exist.

ryanofsky commented at 7:13 pm on March 18, 2021:

In commit “node/interfaces: Meaningful locking and avoid potential UB” (5de3e3609ab591d88281558182efdec39b065541)

Same here. You don’t need this local variable if you exit early when chainman doesn’t exist.

I think John’s suggestions are good, but Marco’s suggestion to use an accessor that asserts #21391 (review) would be even better. That way we don’t need to waste brain cells thinking about what values to return if chainman is null. Like if getBestBlockHash returns the genesis block hash in that case should getNumBlocks return 1 instead of 0 to be consistent? Easier to take everything down and assert.

jnewbery commented at 10:02 am on March 19, 2021:

Seems reasonable to me!

In general, I like the concept of making all components optional, since it enforces decoupling. However, it’s hard to imagine what a bitcoind without chainman would look like.

in src/validation.h:460 in b57dc39eae outdated

456@@ -457,6 +457,7 @@ class BlockManager
457         const CChainParams& chainparams,
458         CBlockIndex** ppindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
459 
460+    // TODO: Change this to a const function

jnewbery commented at 4:19 pm on March 18, 2021:

Any reason not to just do this? The function is obviously const since it just does a lookup and returns a pointer.

in src/rpc/blockchain.cpp:403 in b57dc39eae outdated

398@@ -398,7 +399,8 @@ static RPCHelpMan getdifficulty()
399         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
400 {
401     LOCK(cs_main);
402-    return GetDifficulty(::ChainActive().Tip());
403+    CChain& active_chain = EnsureChainman(request.context).ActiveChain();
404+    return GetDifficulty(active_chain.Tip());

jnewbery commented at 4:22 pm on March 18, 2021:

Maybe join these lines?

in src/rpc/blockchain.cpp:768 in b57dc39eae outdated

764@@ -763,12 +765,13 @@ static RPCHelpMan getblockhash()
765         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
766 {
767     LOCK(cs_main);
768+    CChain& active_chain = EnsureChainman(request.context).ActiveChain();

jnewbery commented at 4:23 pm on March 18, 2021:

0    const CChain& active_chain = EnsureChainman(request.context).ActiveChain();

(same goes for other CChain&s)

in src/rpc/blockchain.cpp:1083 in b57dc39eae outdated

1081 
1082     const CoinStatsHashType hash_type{request.params[0].isNull() ? CoinStatsHashType::HASH_SERIALIZED : ParseHashType(request.params[0].get_str())};
1083 
1084-    CCoinsView* coins_view = WITH_LOCK(::cs_main, return &::ChainstateActive().CoinsDB());
1085+    CCoinsView* coins_view = WITH_LOCK(::cs_main, return &active_chainstate.CoinsDB());
1086+    BlockManager& blockman = WITH_LOCK(::cs_main, return std::ref(active_chainstate.m_blockman));

jnewbery commented at 4:54 pm on March 18, 2021:

It would be nice to avoid so many repeated locks and releases of cs_main. In this function we take it at least 5 times:

in EnsureChainman()
in ActiveChainstate()
in ForceFlushStateToDisk()
in WITH_LOCK(::cs_main, return &active_chainstate.CoinsDB());
in WITH_LOCK(::cs_main, return std::ref(active_chainstate.m_blockman));

this gets rid of one unnecessary lock/release:

 0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
 1index aa1164ae29..941b2bf780 100644
 2--- a/src/rpc/blockchain.cpp
 3+++ b/src/rpc/blockchain.cpp
 4@@ -1080,10 +1080,15 @@ static RPCHelpMan gettxoutsetinfo()
 5 
 6     const CoinStatsHashType hash_type{request.params[0].isNull() ? CoinStatsHashType::HASH_SERIALIZED : ParseHashType(request.params[0].get_str())};
 7 
 8-    CCoinsView* coins_view = WITH_LOCK(::cs_main, return &active_chainstate.CoinsDB());
 9-    BlockManager& blockman = WITH_LOCK(::cs_main, return std::ref(active_chainstate.m_blockman));
10+    CCoinsView* coins_view;
11+    BlockManager* blockman;
12+    {
13+        LOCK(::cs_main);
14+        coins_view = &active_chainstate.CoinsDB();
15+        blockman = &active_chainstate.m_blockman;
16+    }
17     NodeContext& node = EnsureNodeContext(request.context);
18-    if (GetUTXOStats(coins_view, blockman, stats, hash_type, node.rpc_interruption_point)) {
19+    if (GetUTXOStats(coins_view, *blockman, stats, hash_type, node.rpc_interruption_point)) {
20         ret.pushKV("height", (int64_t)stats.nHeight);
21         ret.pushKV("bestblock", stats.hashBlock.GetHex());
22         ret.pushKV("transactions", (int64_t)stats.nTransactions);

in src/rpc/blockchain.cpp:1372 in b57dc39eae outdated

1368@@ -1358,14 +1369,15 @@ RPCHelpMan getblockchaininfo()
1369     }
1370 
1371     const Consensus::Params& consensusParams = Params().GetConsensus();
1372+    int active_tip_nheight = tip->nHeight;

jnewbery commented at 4:57 pm on March 18, 2021:

Can you declare this further up and use it instead of (int)chainman.ActiveChain().Height()) for "blocks"?

jnewbery commented at 5:31 pm on March 18, 2021:

I think just height here probably suffices:

0    const int height = tip->nHeight;

in src/rpc/mining.cpp:48 in b57dc39eae outdated

43@@ -44,11 +44,12 @@
44  * or from the last difficulty change if 'lookup' is nonpositive.
45  * If 'height' is nonnegative, compute the estimate at the time when a given block was found.
46  */
47-static UniValue GetNetworkHashPS(int lookup, int height) {
48-    CBlockIndex *pb = ::ChainActive().Tip();
49+static UniValue GetNetworkHashPS(int lookup, int height, CChain& active_chain) {
50+    CBlockIndex *pb = active_chain.Tip();

jnewbery commented at 5:40 pm on March 18, 2021:

0    CBlockIndex* pb = active_chain.Tip();

(could also make this const if you update line 65 to make pb0 const too)

jnewbery commented at 6:27 pm on March 18, 2021: member

Looks good. I’ve left a few style comments inline.

This is quite a large PR. I wonder if this should perhaps be split into two PRs - one to address the comments from bundle 4 and another for bundle 5. I don’t think they overlap very much so you can have them open in parallel without much conflict.

in src/node/interfaces.cpp:198 in 5de3e3609a outdated

196     {
197-        assert(std::addressof(::ChainActive()) == std::addressof(m_context->chainman->ActiveChain()));
198-        const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_context->chainman->ActiveChain().Tip());
199+        const CBlockIndex* tip;
200+        if (m_context->chainman) {
201+            LOCK(cs_main);

ryanofsky commented at 7:07 pm on March 18, 2021:

In commit “node/interfaces: Meaningful locking and avoid potential UB” (5de3e3609ab591d88281558182efdec39b065541)

It seems like this method and the two IBD methods are the only places that are changing locks or adding “meaningful locking”. Since locking changes can have unexpected pitfalls I think it’d be good to split the locking parts of this commit into a separate commit. Also would be good to expand the commit message to say what’s motivating the lock changes. I’m assuming it’s to satisfy some lock annotations later?

ryanofsky approved

ryanofsky commented at 7:35 pm on March 18, 2021: member

Code review ACK b57dc39eae25294071959b1eec42bd7dd5563357. Looks good. I just rereviewed from scratch instead of seeing what changed since the last review. Reviewing with -U0 --word-diff-regex=. made it easy to check that the bulk of this PR is just switching from one chainman reference to another, and to focus more closely on the parts making other changes. -w --color-moved was also useful for reviewing the “Move comments to declarations” commit

dongcarl force-pushed on Mar 24, 2021

dongcarl commented at 5:58 pm on March 24, 2021: member

Pushed b57dc39eae25294071959b1eec42bd7dd5563357 -> 877c8a01fc1137e0c8187b0b6990a99658077152

Addressed some review suggestions
Note: this is not yet rebased

dongcarl force-pushed on Mar 24, 2021

dongcarl commented at 8:30 pm on March 24, 2021: member

Pushed 877c8a01fc1137e0c8187b0b6990a99658077152 -> b7833034a554b571b3d3e3e55a0639421770dfb9

Rebased on top of #21525
Addressed all reviews

dongcarl force-pushed on Mar 24, 2021

dongcarl commented at 8:39 pm on March 24, 2021: member

Pushed b7833034a554b571b3d3e3e55a0639421770dfb9 -> 677b836c4847315bf24a9d222bbb2eb23a2b9a2c

Use CHECK_NONFATAL instead of assert

DrahtBot removed the label Needs rebase on Mar 24, 2021

in src/node/interfaces.cpp:199 in 3460a07d88 outdated

195-        assert(std::addressof(::ChainActive()) == std::addressof(m_context->chainman->ActiveChain()));
196-        const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_context->chainman->ActiveChain().Tip());
197+        const CBlockIndex* tip;
198+        {
199+            LOCK(cs_main);
200+            assert(std::addressof(::ChainActive()) == std::addressof(chainman().ActiveChain()));

ryanofsky commented at 3:27 pm on March 30, 2021:

In commits “node/ifaces: NodeImpl: Use an accessor for ChainMan” (3460a07d88d841c009e7b7efc0be2ae89bb4a63e) and “node/ifaces: ChainImpl: Use an accessor for ChainMan” (3b75ec491a68eb6fb137c2f829e7dde93b3c3189)

Not important unless there will be a lot more of these, but I think it would be better not to add nested scopes for temporary asserts that we are going to want to remove and collapse later. The asserts seem good in other places where they can be removed with scripted diffs just deleting the assert lines. But places like this should be easier to clean up later using a helper like:

0//! Temporary accessor that can be removed after
1//! [#20158](/bitcoin-bitcoin/20158/), but helps check correctness
2//! of refactored code in the meantime.
3static inline CChain& ActiveChain(ChainstateManager& chainman) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
4{
5    assert(std::addressof(::ChainActive()) == std::addressof(chainman.ActiveChain()));
6    return chainman.ActiveChain();
7}

that could substituted out with a scripted-diff without needing to change surrounding code. Could also be appealing if you think usage looks nicer:

0-        assert(std::addressof(::ChainActive()) == std::addressof(chainman().ActiveChain()));
1-        return chainman().ActiveChain().Height();
2+        return ActiveChain(chainman()).Height();

in src/net_processing.cpp:500 in 067d0ba36c outdated

495+     * Validation logic for compact filters request handling.
496+     *
497+     * May disconnect from the peer in the case of a bad request.
498+     *
499+     * @param[in]   peer            The peer that we received the request from
500+     * @param[in]   chain_params    Chain parameters

ryanofsky commented at 3:38 pm on March 30, 2021:

In commit “net_processing: Move comments to declarations” (067d0ba36cb701adff530f76ef9b1f574c966c6f)

Is no more chain parameters.

ryanofsky approved

ryanofsky commented at 3:54 pm on March 30, 2021: member

Code review ACK 677b836c4847315bf24a9d222bbb2eb23a2b9a2c

Since last review there were many suggested review changes, notably replacing interfaces update commit 5de3e3609ab591d88281558182efdec39b065541 with more limited 3460a07d88d841c009e7b7efc0be2ae89bb4a63e 3b75ec491a68eb6fb137c2f829e7dde93b3c3189 commits for #21391 (review)

Cirrus android failure https://cirrus-ci.com/task/5300781294288896 is spurious and should be fixed by #21541

Left some new suggestions but they aren’t important and could be followed up later or not at all.

dongcarl force-pushed on Mar 30, 2021

dongcarl commented at 5:56 pm on March 30, 2021: member

Pushed 677b836c4847315bf24a9d222bbb2eb23a2b9a2c -> 188bb5e224d757d8d0603bb05a911f816776d295

Rebased on updated #21525, update summary reproduced below:

Pushed 3b75ec491a68eb6fb137c2f829e7dde93b3c3189 -> 693414d27181cf967f787a2ca72344e52c58c7f0

Addressed #21391#pullrequestreview-624405217

ryanofsky approved

ryanofsky commented at 6:19 pm on March 30, 2021: member

Code review ACK 188bb5e224d757d8d0603bb05a911f816776d295. Only changes since last review were comment changes following up the last review (thanks!)

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

MarcoFalke commented at 9:51 am on April 1, 2021: member

Needs rebase?

dongcarl force-pushed on Apr 2, 2021

dongcarl commented at 9:05 pm on April 2, 2021: member

Pushed 188bb5e224d757d8d0603bb05a911f816776d295 -> 7045f5f557f910827f13501e303ad617a8f0aaa6

Rebased over master after merge of #21525, fixed silent conflict in src/rest.cpp

dongcarl commented at 9:09 pm on April 2, 2021: member

@MarcoFalke re: https://github.com/bitcoin/bitcoin/pull/21525/files/e62067e7bcad5a559899afff2e4a8e8b7e9f4301..7b8e976cd5ac78a22f1be2b2fed8562c693af5d9#diff-cd34d7ba23b9b4f3f61cd06e9289c4981a5a6bcb17b61b5d010f3dc01c77787e

This is more or less what I’m thinking, let me know if I’m missing something!

 0diff --git a/src/miner.cpp b/src/miner.cpp
 1index 8a9406f810..3bc7fdd458 100644
 2--- a/src/miner.cpp
 3+++ b/src/miner.cpp
 4@@ -39,13 +39,14 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
 5     return nNewTime - nOldTime;
 6 }
 7 
 8-void RegenerateCommitments(CBlock& block, BlockManager& blockman)
 9+void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block)
10 {
11     CMutableTransaction tx{*block.vtx.at(0)};
12     tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));
13     block.vtx.at(0) = MakeTransactionRef(tx);
14 
15-    GenerateCoinbaseCommitment(block, WITH_LOCK(::cs_main, assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman)); return blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());
16+    WITH_LOCK(::cs_main, assert(g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock) == prev_block));
17+    GenerateCoinbaseCommitment(block, prev_block, Params().GetConsensus());
18 
19     block.hashMerkleRoot = BlockMerkleRoot(block);
20 }
21diff --git a/src/miner.h b/src/miner.h
22index c400c90f6c..becf362b79 100644
23--- a/src/miner.h
24+++ b/src/miner.h
25@@ -202,8 +202,7 @@ private:
26 void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce);
27 int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
28 
29-// TODO just accept a CBlockIndex*
30 /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */
31-void RegenerateCommitments(CBlock& block, BlockManager& blockman);
32+void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block);
33 
34 #endif // BITCOIN_MINER_H
35diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
36index 8b54d07b50..5136a4e3de 100644
37--- a/src/rpc/mining.cpp
38+++ b/src/rpc/mining.cpp
39@@ -374,8 +374,8 @@ static RPCHelpMan generateblock()
40 
41     // Add transactions
42     block.vtx.insert(block.vtx.end(), txs.begin(), txs.end());
43-    WITH_LOCK(::cs_main, CHECK_NONFATAL(std::addressof(g_chainman.m_blockman) == std::addressof(chainman.m_blockman)));
44-    RegenerateCommitments(block, WITH_LOCK(::cs_main, return std::ref(chainman.m_blockman)));
45+    CBlockIndex* prev_block = WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock));
46+    RegenerateCommitments(block, prev_block);
47 
48     {
49         LOCK(cs_main);
50diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
51index f7800aefca..46c7b781b0 100644
52--- a/src/test/util/setup_common.cpp
53+++ b/src/test/util/setup_common.cpp
54@@ -253,7 +253,8 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
55     for (const CMutableTransaction& tx : txns) {
56         block.vtx.push_back(MakeTransactionRef(tx));
57     }
58-    RegenerateCommitments(block, WITH_LOCK(::cs_main, return std::ref(g_chainman.m_blockman)));
59+    CBlockIndex* prev_block = WITH_LOCK(::cs_main, return g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock));
60+    RegenerateCommitments(block, prev_block);
61 
62     while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;
63

in src/rest.cpp:183 in 7045f5f557 outdated

180@@ -181,13 +181,14 @@ static bool rest_headers(const std::any& context,
181     headers.reserve(count);
182     {
183         LOCK(cs_main);
184-        tip = ::ChainActive().Tip();
185-        const CBlockIndex* pindex = g_chainman.m_blockman.LookupBlockIndex(hash);
186-        while (pindex != nullptr && ::ChainActive().Contains(pindex)) {
187+        ChainstateManager& chainman = EnsureChainman(context);

MarcoFalke commented at 6:20 am on April 3, 2021:

nit: Would be nice to do this before locking cs_main, because I think cs_main should be made a (private) member of chainman in the future, if possible.

in src/rest.cpp:253 in 7045f5f557 outdated

250@@ -249,8 +251,9 @@ static bool rest_block(HTTPRequest* req,
251     CBlockIndex* tip = nullptr;
252     {
253         LOCK(cs_main);
254-        tip = ::ChainActive().Tip();
255-        pblockindex = g_chainman.m_blockman.LookupBlockIndex(hash);
256+        ChainstateManager& chainman = EnsureChainman(context);

MarcoFalke commented at 6:20 am on April 3, 2021:

same

in src/rpc/blockchain.cpp:842 in 7045f5f557 outdated

823@@ -822,8 +824,9 @@ static RPCHelpMan getblockheader()
824     const CBlockIndex* tip;
825     {
826         LOCK(cs_main);
827-        pblockindex = g_chainman.m_blockman.LookupBlockIndex(hash);
828-        tip = ::ChainActive().Tip();
829+        ChainstateManager& chainman = EnsureChainman(request.context);

MarcoFalke commented at 6:23 am on April 3, 2021:

same (etc …)

MarcoFalke approved

MarcoFalke commented at 6:28 am on April 3, 2021: member

As you are modifying those lines, it would be nice if the chainman reference was created before taking cs_main, as that would simplify future work to remove cs_main (or reduce the scope of it).

miner: Pass in previous CBlockIndex to RegenerateCommitments cced0f46c9

rpc/*,rest: Add review-only assertion to EnsureChainman d0abf0bf42

rpc/blockchain: Use existing NodeContext

Also pass in appropriate object to:
- BIP9SoftForkDescPushBack
- BuriedForkDescPushBack

d485e815e2

rpc/mining: Use existing NodeContext

Also pass in appropriate object to:
- GetNetworkHashPS
- [gG]enerateBlock{,s}

Also:
- Misc style/constness changes

60dc05afc6

rpc/rawtx: Use existing NodeContext

Also pass in appropriate object to:
- TxToJSON

7be0671b95

rest: Pass in NodeContext to rest_block 3f08934799

rest: Use existing NodeContext d7824acdb9

dongcarl commented at 3:19 pm on April 5, 2021: member

As you are modifying those lines, it would be nice if the chainman reference was created before taking cs_main, as that would simplify future work to remove cs_main (or reduce the scope of it).

These changes seem simple enough, so I’d be happy to make it. I do want to understand: how do you see these small changes helping with “future work to remove cs_main (or reduce the scope of it)”?

MarcoFalke commented at 3:38 pm on April 5, 2021: member

cs_main is a global and used to lock stuff that has nothing to do with validation (e.g. the p2p thread, disk access, orphan handling, …). There is ongoing effort to clean those up: #19398, https://github.com/bitcoin/bitcoin/pull/21527/files#r601283910, #21598 (comment), …

If cs_main is a member, the code might look like this:

0Chainstatemanager& cm = ...;
1LOCK(cm.main);

So simply getting a reference to the chainstatemanager before taking the lock will help in that regard, so that the two lines don’t have to be switched in the future.

in src/rpc/blockchain.cpp:202 in 7045f5f557 outdated

198@@ -198,7 +199,7 @@ static RPCHelpMan getblockcount()
199         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
200 {
201     LOCK(cs_main);
202-    return ::ChainActive().Height();
203+    return EnsureChainman(request.context).ActiveChain().Height();

jnewbery commented at 4:22 pm on April 5, 2021:

Doesn’t need cs_main to be locked first. You can also just call ActiveHeight().

jamesob commented at 6:34 pm on April 5, 2021:

Agree about ActiveHeight() and similar comments below, but may be fine to leave the LOCK(cs_main) line as-is since I suspect we’ll move to lock annotations for ActiveChainstate() in the near future (vs. the nested acquisition in the function definition).

jnewbery commented at 7:36 am on April 6, 2021:

I think that’d be a move in the wrong direction. Ideally we’d remove the need to lock cs_main completely from the Active*() methods. I’m concerned that adding so many additional cs_main locks may cause performance regressions - for example, the optimization in #8007 is no longer possible since cs_main will almost always be locked before calling IsInitialBlockDownload() @jamesob: Did you consider changing the active pointer to being an atomic to remove this requirement as suggested here: #19806 (comment)?

jamesob commented at 1:37 pm on April 6, 2021:

I think you may be forgetting that long before ChainstateManager was introduced, chainActive was protected by cs_main. So unless we dramatically rework the chainstate locking scheme, the Active{Height,Tip,..}() methods will always require holding cs_main one way or another. I think ultimately annotations are a cleaner way of doing this because they give callers the flexibility to reuse existing cs_main acquisitions, and avoid recursive locking.

Your suggestion to move chainman’s active pointer to an atomic may be a good one, but I think it’s largely incidental in terms of performance since we usually need to acquire cs_main anyway when referencing chainstate data. Though my comment above is unrelated to that, since I’m making reference to the chain’s protection by cs_main and not ancillary ChainstateManager layout.

Recent bitcoinperf benchmarks don’t indicate any regressions in recent revisions, but I will be doing more thorough benchmarking in a bit.

in src/rpc/blockchain.cpp:221 in 7045f5f557 outdated

217@@ -217,7 +218,7 @@ static RPCHelpMan getbestblockhash()
218         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
219 {
220     LOCK(cs_main);
221-    return ::ChainActive().Tip()->GetBlockHash().GetHex();
222+    return EnsureChainman(request.context).ActiveChain().Tip()->GetBlockHash().GetHex();

jnewbery commented at 4:23 pm on April 5, 2021:

Doesn’t need cs_main to be locked first. You can also just call ActiveTip().

in src/rpc/blockchain.cpp:402 in 7045f5f557 outdated

398@@ -398,7 +399,7 @@ static RPCHelpMan getdifficulty()
399         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
400 {
401     LOCK(cs_main);
402-    return GetDifficulty(::ChainActive().Tip());
403+    return GetDifficulty(EnsureChainman(request.context).ActiveChain().Tip());

jnewbery commented at 4:24 pm on April 5, 2021:

Doesn’t need cs_main to be locked first. You can also just call ActiveTip().

in src/rpc/blockchain.cpp:970 in 7045f5f557 outdated

948@@ -946,8 +949,9 @@ static RPCHelpMan getblock()
949     const CBlockIndex* tip;
950     {
951         LOCK(cs_main);
952-        pblockindex = g_chainman.m_blockman.LookupBlockIndex(hash);
953-        tip = ::ChainActive().Tip();
954+        ChainstateManager& chainman = EnsureChainman(request.context);
955+        pblockindex = chainman.m_blockman.LookupBlockIndex(hash);
956+        tip = chainman.ActiveChain().Tip();

jnewbery commented at 4:38 pm on April 5, 2021:

ActiveTip()

in src/rpc/blockchain.cpp:1029 in 7045f5f557 outdated

1010         heightParam = pindex->nHeight;
1011     }
1012 
1013     unsigned int height = (unsigned int) heightParam;
1014-    unsigned int chainHeight = (unsigned int) ::ChainActive().Height();
1015+    unsigned int chainHeight = (unsigned int) chainman.ActiveChain().Height();

jnewbery commented at 4:38 pm on April 5, 2021:

ActiveHeight()

in src/rpc/blockchain.cpp:1040 in 7045f5f557 outdated

1019@@ -1015,8 +1020,8 @@ static RPCHelpMan pruneblockchain()
1020         height = chainHeight - MIN_BLOCKS_TO_KEEP;
1021     }
1022 
1023-    PruneBlockFilesManual(::ChainstateActive(), height);
1024-    const CBlockIndex* block = ::ChainActive().Tip();
1025+    PruneBlockFilesManual(chainman.ActiveChainstate(), height);
1026+    const CBlockIndex* block = chainman.ActiveChain().Tip();

jnewbery commented at 4:38 pm on April 5, 2021:

ActiveTip()

in src/rpc/blockchain.cpp:1367 in 7045f5f557 outdated

1341@@ -1328,17 +1342,20 @@ RPCHelpMan getblockchaininfo()
1342         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1343 {
1344     LOCK(cs_main);
1345+    ChainstateManager& chainman = EnsureChainman(request.context);
1346 
1347-    const CBlockIndex* tip = ::ChainActive().Tip();
1348+    const CBlockIndex* tip = chainman.ActiveChain().Tip();

jnewbery commented at 5:43 pm on April 5, 2021:

0    const CBlockIndex* tip = chainman.ActiveTip();

in src/rpc/blockchain.cpp:1352 in 7045f5f557 outdated

1349+    CHECK_NONFATAL(tip);
1350+    const int height = tip->nHeight;
1351     UniValue obj(UniValue::VOBJ);
1352     obj.pushKV("chain",                 Params().NetworkIDString());
1353-    obj.pushKV("blocks",                (int)::ChainActive().Height());
1354+    obj.pushKV("blocks",                (int)height);

jnewbery commented at 5:47 pm on April 5, 2021:

This is casting an int to an int.

0    obj.pushKV("blocks",                height);

in src/rpc/blockchain.cpp:1223 in 7045f5f557 outdated

1220 },
1221     };
1222 }
1223 
1224-static void BuriedForkDescPushBack(UniValue& softforks, const std::string &name, int height) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1225+static void BuriedForkDescPushBack(UniValue& softforks, const std::string &name, int height, int active_tip_nheight) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

jnewbery commented at 5:49 pm on April 5, 2021:

Maybe just:

0static void BuriedForkDescPushBack(UniValue& softforks, const std::string &name, int softfork_height, int tip_height) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

nheight is hungarian, which we don’t use any more.

in src/rpc/blockchain.cpp:1732 in 7045f5f557 outdated

1704     int blockcount = 30 * 24 * 60 * 60 / Params().GetConsensus().nPowTargetSpacing; // By default: 1 month
1705 
1706     if (request.params[1].isNull()) {
1707         LOCK(cs_main);
1708-        pindex = ::ChainActive().Tip();
1709+        pindex = chainman.ActiveChain().Tip();

jnewbery commented at 5:50 pm on April 5, 2021:

0        pindex = chainman.ActiveTip();

in src/rpc/blockchain.cpp:1910 in 7045f5f557 outdated

1886 
1887     CBlockIndex* pindex;
1888     if (request.params[0].isNum()) {
1889         const int height = request.params[0].get_int();
1890-        const int current_tip = ::ChainActive().Height();
1891+        const int current_tip = chainman.ActiveChain().Height();

jnewbery commented at 5:51 pm on April 5, 2021:

0        const int current_tip = chainman.ActiveHeight();

in src/rpc/blockchain.cpp:2313 in 7045f5f557 outdated

2290+            ChainstateManager& chainman = EnsureChainman(request.context);
2291+            chainman.ActiveChainstate().ForceFlushStateToDisk();
2292+            pcursor = std::unique_ptr<CCoinsViewCursor>(chainman.ActiveChainstate().CoinsDB().Cursor());
2293             CHECK_NONFATAL(pcursor);
2294-            tip = ::ChainActive().Tip();
2295+            tip = chainman.ActiveChain().Tip();

jnewbery commented at 5:52 pm on April 5, 2021:

0            tip = chainman.ActiveTip();

in src/rpc/mining.cpp:117 in 7045f5f557 outdated

112@@ -111,7 +113,8 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t&
113 
114     {
115         LOCK(cs_main);
116-        IncrementExtraNonce(&block, ::ChainActive().Tip(), extra_nonce);
117+        CHECK_NONFATAL(std::addressof(::ChainActive()) == std::addressof(chainman.ActiveChain()));
118+        IncrementExtraNonce(&block, chainman.ActiveChain().Tip(), extra_nonce);

jnewbery commented at 5:53 pm on April 5, 2021:

0        IncrementExtraNonce(&block, chainman.ActiveTip(), extra_nonce);

jnewbery commented at 10:54 am on April 15, 2021:

For a follow-up: IncrementExtraNonce() doesn’t require cs_main. In fact, we could just get a height using chainman.ActiveHeight() and pass that to IncrementExtraNonce().

in src/rpc/mining.cpp:150 in 7045f5f557 outdated

145@@ -143,7 +146,8 @@ static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& me
146 
147     {   // Don't keep cs_main locked
148         LOCK(cs_main);
149-        nHeight = ::ChainActive().Height();
150+        CHECK_NONFATAL(std::addressof(::ChainActive()) == std::addressof(chainman.ActiveChain()));
151+        nHeight = chainman.ActiveChain().Height();

jnewbery commented at 5:54 pm on April 5, 2021:

0        nHeight = chainman.ActiveHeight();

in src/rpc/mining.cpp:645 in 7045f5f557 outdated

635@@ -628,12 +636,12 @@ static RPCHelpMan getblocktemplate()
636                 return "duplicate-inconclusive";
637             }
638 
639-            CBlockIndex* const pindexPrev = ::ChainActive().Tip();
640+            CBlockIndex* const pindexPrev = chainman.ActiveChain().Tip();

jnewbery commented at 5:55 pm on April 5, 2021:

0            CBlockIndex* const pindexPrev = chainman.ActiveTip();

in src/rpc/mining.cpp:706 in 7045f5f557 outdated

697@@ -690,7 +698,7 @@ static RPCHelpMan getblocktemplate()
698         else
699         {
700             // NOTE: Spec does not specify behaviour for non-string longpollid, but this makes testing easier
701-            hashWatchedChain = ::ChainActive().Tip()->GetBlockHash();
702+            hashWatchedChain = chainman.ActiveChain().Tip()->GetBlockHash();

jnewbery commented at 5:55 pm on April 5, 2021:

0            hashWatchedChain = chainman.ActiveTip()->GetBlockHash();

in src/rpc/mining.cpp:751 in 7045f5f557 outdated

742@@ -735,20 +743,20 @@ static RPCHelpMan getblocktemplate()
743     static CBlockIndex* pindexPrev;
744     static int64_t nStart;
745     static std::unique_ptr<CBlockTemplate> pblocktemplate;
746-    if (pindexPrev != ::ChainActive().Tip() ||
747+    if (pindexPrev != chainman.ActiveChain().Tip() ||

jnewbery commented at 5:56 pm on April 5, 2021:

Maybe just get ActiveChainstate once and then get the m_tip from there?

in src/rpc/mining.cpp:900 in 7045f5f557 outdated

891@@ -884,7 +892,7 @@ static RPCHelpMan getblocktemplate()
892     result.pushKV("transactions", transactions);
893     result.pushKV("coinbaseaux", aux);
894     result.pushKV("coinbasevalue", (int64_t)pblock->vtx[0]->vout[0].nValue);
895-    result.pushKV("longpollid", ::ChainActive().Tip()->GetBlockHash().GetHex() + ToString(nTransactionsUpdatedLast));
896+    result.pushKV("longpollid", chainman.ActiveChain().Tip()->GetBlockHash().GetHex() + ToString(nTransactionsUpdatedLast));

jnewbery commented at 5:57 pm on April 5, 2021:

0    result.pushKV("longpollid", chainman.ActiveTip()->GetBlockHash().GetHex() + ToString(nTransactionsUpdatedLast));

in src/rest.cpp:191 in 7045f5f557 outdated

190+        while (pindex != nullptr && chainman.ActiveChain().Contains(pindex)) {
191             headers.push_back(pindex);
192             if (headers.size() == (unsigned long)count)
193                 break;
194-            pindex = ::ChainActive().Next(pindex);
195+            pindex = chainman.ActiveChain().Next(pindex);

jnewbery commented at 6:04 pm on April 5, 2021:

Maybe just grab chainman.ActiveChain() once above and then use it for each iteration.

in src/rest.cpp:256 in 7045f5f557 outdated

250@@ -249,8 +251,9 @@ static bool rest_block(HTTPRequest* req,
251     CBlockIndex* tip = nullptr;
252     {
253         LOCK(cs_main);
254-        tip = ::ChainActive().Tip();
255-        pblockindex = g_chainman.m_blockman.LookupBlockIndex(hash);
256+        ChainstateManager& chainman = EnsureChainman(context);
257+        tip = chainman.ActiveChain().Tip();

jnewbery commented at 6:05 pm on April 5, 2021:

0        tip = chainman.ActiveTip();

in src/rest.cpp:558 in 7045f5f557 outdated

553@@ -550,12 +554,12 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
554             if (!mempool) return false;
555             // use db+mempool as cache backend in case user likes to query mempool
556             LOCK2(cs_main, mempool->cs);
557-            CCoinsViewCache& viewChain = ::ChainstateActive().CoinsTip();
558+            CCoinsViewCache& viewChain = chainman.ActiveChainstate().CoinsTip();

jnewbery commented at 6:07 pm on April 5, 2021:

Perhaps just call chainman.ActiveChainstate() once in this function.

in src/rpc/blockchain.cpp:845 in 9d67fd7229 outdated

823@@ -823,8 +824,9 @@ static RPCHelpMan getblockheader()
824     const CBlockIndex* tip;
825     {
826         LOCK(cs_main);
827-        pblockindex = g_chainman.m_blockman.LookupBlockIndex(hash);
828-        tip = ::ChainActive().Tip();
829+        ChainstateManager& chainman = EnsureChainman(request.context);
830+        pblockindex = chainman.m_blockman.LookupBlockIndex(hash);
831+        tip = chainman.ActiveChain().Tip();

jamesob commented at 7:22 pm on April 5, 2021:

ActiveTip()

in src/rpc/mining.cpp:432 in 626aa73ae4 outdated

425@@ -420,12 +426,13 @@ static RPCHelpMan getmininginfo()
426 {
427     LOCK(cs_main);
428     const CTxMemPool& mempool = EnsureMemPool(request.context);
429+    const CChain& active_chain = EnsureChainman(request.context).ActiveChain();
430 
431     UniValue obj(UniValue::VOBJ);
432-    obj.pushKV("blocks",           (int)::ChainActive().Height());
433+    obj.pushKV("blocks",           (int)active_chain.Height());

jamesob commented at 7:40 pm on April 5, 2021:

https://github.com/bitcoin/bitcoin/pull/21391/commits/626aa73ae48f6b5a50e88fc3d03e8c7842f75b1b

Can remove unnecessary cast.

in src/rest.cpp:185 in 7045f5f557 outdated

180@@ -181,13 +181,14 @@ static bool rest_headers(const std::any& context,
181     headers.reserve(count);
182     {
183         LOCK(cs_main);
184-        tip = ::ChainActive().Tip();
185-        const CBlockIndex* pindex = g_chainman.m_blockman.LookupBlockIndex(hash);
186-        while (pindex != nullptr && ::ChainActive().Contains(pindex)) {
187+        ChainstateManager& chainman = EnsureChainman(context);
188+        tip = chainman.ActiveChain().Tip();

jamesob commented at 7:50 pm on April 5, 2021:

ActiveTip()

jamesob commented at 7:53 pm on April 5, 2021: member

Looks good! Would be good to fix up the Active*() method usages. I think given there is no scripted-diff usage, it may also be worth squashing the last two commits together.

ryanofsky approved

ryanofsky commented at 1:52 am on April 6, 2021: member

Code review ACK 7045f5f557f910827f13501e303ad617a8f0aaa6. No changes since last review other than rebase. Cleanup suggestions from Marco/John/James all seem reasonable too.

dongcarl commented at 5:26 pm on April 6, 2021: member

Question: Is there an actual difference between chainman.ActiveTip/Height() and chainman.ActiveChain().Tip/height() or is it just shorter?

jnewbery commented at 5:28 pm on April 6, 2021: member

Is there an actual difference between chainman.ActiveTip/Height() and chainman.ActiveChain().Tip/height() or is it just shorter?

Just shorter. Feel free to ignore those comments if you disagree that it’s clearer.

rpc: Add alt Ensure* functions acepting NodeContext 306b1cd3ee

dongcarl force-pushed on Apr 13, 2021

dongcarl commented at 1:49 am on April 13, 2021: member

Pushed 7045f5f557f910827f13501e303ad617a8f0aaa6 -> 1c483e8e73c9f02674895b73b017dc0d13a030b0

Addressed MarcoFalke’s cs_main vs ChainstateManager code ordering comments
Addressed jnewbery and jamesob’s height int casting comments
Addressed jnewbery’s BuriedForkDescPushBack signature comments

w/re getting rid of multiple calls to ActiveChain{,State}(), I really didn’t consider it part of the PR scope, but am happy to do it if it’s very simple and can be easily convinced that it’s safe+correct. I had some questions for jamesob here: #21620 (comment)

dongcarl force-pushed on Apr 13, 2021

dongcarl commented at 2:42 am on April 13, 2021: member

Pushed 1c483e8e73c9f02674895b73b017dc0d13a030b0 -> 6d923137f6eb31ace6f2d38480708af039a21437

Once more, with feeling…
I renamed params in last commit but forgot to rename references… oops

in src/rpc/blockchain.h:65 in 306b1cd3ee outdated

56@@ -57,8 +57,11 @@ void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fInclud
57 void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, bool include_hex = true, int serialize_flags = 0, const CTxUndo* txundo = nullptr);
58 
59 NodeContext& EnsureNodeContext(const std::any& context);
60+CTxMemPool& EnsureMemPool(const NodeContext& node);
61 CTxMemPool& EnsureMemPool(const std::any& context);
62+ChainstateManager& EnsureChainman(const NodeContext& node);
63 ChainstateManager& EnsureChainman(const std::any& context);
64+CBlockPolicyEstimator& EnsureFeeEstimator(const NodeContext& node);
65 CBlockPolicyEstimator& EnsureFeeEstimator(const std::any& context);

ryanofsky commented at 3:22 pm on April 13, 2021:

In commit “rpc: Add alt Ensure* functions acepting NodeContext” (306b1cd3eeb2502904ed4698646d2c86d028aad2)

Caramba. I think this commit is ok, but it is a footgun to have overloaded functions where one of the functions takes an untyped std::any argument, because someone could easily try to call the typed overload, and fail because the type is a little off (const instead of nonconst, pointer instead of reference, or just accidentally passing the wrong variable), and the compiler will never catch it because it will fall back to the std::any overload.

I think the new functions are good, but the existing std::any functions should be renamed to EnsureAnyMemPool, EnsureAnyChainman, etc to be more distinct. Or maybe the std::any functions should take JSONRPCRequest arguments instead. Or maybe they can be removed after a lot of usages are removed in the next commit.

Anyway no need to worry about now. Save for bundle √26

ryanofsky approved

ryanofsky commented at 3:52 pm on April 13, 2021: member

Code review ACK 6d923137f6eb31ace6f2d38480708af039a21437. No changes to existing commits, just new commits doing suggested cleanups, particularly making RPC code more consistent about how it accesses chainmem & mempool globals. I like the new commit message that says “(see commit message)”. Seems worth a try, people never read commit messages!

rpc: Add renamed EnsureAny*() functions

- The original Ensure*(const std::any& context) functions are kept and
  the parameter renamed to ctx so that the scripted-diff in the
  subsequent commit will work as expected

- The renaming avoids overloading mistakes arising out of the untyped
  std::any argument.

1570c7ee98

scripted-diff: rest/rpc: Use renamed EnsureAny*()

-BEGIN VERIFY SCRIPT-
sed -i -E 's@Ensure([^(]+)(\((request\.|)context\))@EnsureAny\1\2@g' \
    -- src/rest.cpp src/rpc/*.cpp
-END VERIFY SCRIPT-

6fb65b49f4

rest/rpc: Remove now-unused old Ensure functions

The scripted-diff in the previous commit should have removed all calls
to functions like: Ensure(?!Any)\(const std::any& (context|ctx)\), so we
can remove them now.

038854f31e

rpc: Tidy up local references (see commit message)

Organize local variables/references such that:

1. There is always a `ChainstateManager` reference before any `LOCK(cs_main)`.
2. NodeContext references are used with Ensure*() functions introduced in
   previous commit where appropriate to avoid duplicate assertions.

6a3d192020

rpc: Remove unnecessary casting of block height f99913969f

rpc: style: Improve BuriedForkDescPushBack signature bc3bd36902

rpc/rest: Take and reuse local Chain/ChainState obj

In all rest/rpc-related modules, if there are multiple calls to
ActiveChain{,State}(), and the calls fall under the same ::cs_main lock,
we can simply take a local reference and use/reuse it instead of calling
ActiveChain{,State}() again and again.

586190f0b4

dongcarl force-pushed on Apr 14, 2021

dongcarl commented at 3:20 pm on April 14, 2021: member

Pushed 6d923137f6eb31ace6f2d38480708af039a21437 -> 586190f0b4740457cb86cba632e3d64e6dfe9b0c

Addressed #21391 (review) to keep irrational numbers at bay
Addressed jnewbery’s reviews relating to taking a reference once and reusing it

ryanofsky approved

ryanofsky commented at 7:39 pm on April 14, 2021: member

Code review ACK 586190f0b4740457cb86cba632e3d64e6dfe9b0c. Since last review, no changes to existing commits, just some simple new commits added: three new commits renaming std::any Ensure functions (scripted diff commit and manual pre/post commits), and one new commit factoring out a repeated ActiveChain() call made in a loop. Thanks for the updates!

in src/rpc/blockchain.cpp:2313 in 586190f0b4

2307@@ -2261,15 +2308,17 @@ static RPCHelpMan scantxoutset()
2308         int64_t count = 0;
2309         std::unique_ptr<CCoinsViewCursor> pcursor;
2310         CBlockIndex* tip;
2311+        NodeContext& node = EnsureAnyNodeContext(request.context);
2312         {
2313+            ChainstateManager& chainman = EnsureChainman(node);

jnewbery commented at 10:13 am on April 15, 2021:

I think it’d be better to move this EnsureChainman() to the top of the function. If chainman doesn’t exist, then it’s better to immediately exit from the function.

in src/rpc/blockchain.cpp:2317 in 586190f0b4

2314             LOCK(cs_main);
2315-            ::ChainstateActive().ForceFlushStateToDisk();
2316-            pcursor = std::unique_ptr<CCoinsViewCursor>(::ChainstateActive().CoinsDB().Cursor());
2317+            CChainState& active_chainstate = chainman.ActiveChainstate();
2318+            active_chainstate.ForceFlushStateToDisk();
2319+            pcursor = std::unique_ptr<CCoinsViewCursor>(active_chainstate.CoinsDB().Cursor());

jnewbery commented at 10:15 am on April 15, 2021:

While you’re touching this, perhaps change it to use std::make_unique. That’s the standard way to set unique pointers now that we’re on c++17.

MarcoFalke commented at 9:26 am on April 16, 2021:

Shouldn’t Cursor return a unique_ptr?

jnewbery commented at 9:42 am on April 16, 2021:

Yes, you’re right. Cursor() is passing ownership to the caller.

jnewbery commented at 9:49 am on April 16, 2021:

I’m going to mark this comment as resolved since this PR shouldn’t make any changes here. I think Cursor()’s return type should be changed in a separate PR.

MarcoFalke commented at 3:40 pm on April 17, 2021:

(unresolved comment to make it easier to see all that needs to be addressed in follow-ups)

in src/rpc/blockchain.cpp:2394 in 586190f0b4

2390@@ -2342,8 +2391,9 @@ static RPCHelpMan getblockfilter()
2391     const CBlockIndex* block_index;
2392     bool block_was_connected;
2393     {
2394+        ChainstateManager& chainman = EnsureAnyChainman(request.context);

jnewbery commented at 10:17 am on April 15, 2021:

Again, chainman is a requirement for this function. I think it makes sense to place the assumption at the top of the function.

in src/rpc/mining.cpp:47 in 586190f0b4

43@@ -44,11 +44,12 @@
44  * or from the last difficulty change if 'lookup' is nonpositive.
45  * If 'height' is nonnegative, compute the estimate at the time when a given block was found.
46  */
47-static UniValue GetNetworkHashPS(int lookup, int height) {
48-    CBlockIndex *pb = ::ChainActive().Tip();
49+static UniValue GetNetworkHashPS(int lookup, int height, const CChain& active_chain) {

jnewbery commented at 10:28 am on April 15, 2021:

Even though it’s not annotated as such, I think that holding a reference to m_chain and calling methods on it requires holding cs_main. Perhaps it would be a good idea to either:

annotate this function and add an assertion that cs_main is held; or
pass in a &Chainman, then inside the function lock cs_main, get the CChain&, and call the methods on that CChain&.

I think (2) is nicer - best to limit scope of locks as much as possible.

in src/rpc/mining.cpp:105 in 586190f0b4

 99@@ -99,8 +100,9 @@ static RPCHelpMan getnetworkhashps()
100                 },
101         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
102 {
103+    ChainstateManager& chainman = EnsureAnyChainman(request.context);
104     LOCK(cs_main);
105-    return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].get_int() : 120, !request.params[1].isNull() ? request.params[1].get_int() : -1);
106+    return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].get_int() : 120, !request.params[1].isNull() ? request.params[1].get_int() : -1, chainman.ActiveChain());

jnewbery commented at 10:31 am on April 15, 2021:

This line is long and difficult to read. Consider something like this:

 0diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
 1index 0cef310c50..b60783761d 100644
 2--- a/src/rpc/mining.cpp
 3+++ b/src/rpc/mining.cpp
 4@@ -102,7 +102,9 @@ static RPCHelpMan getnetworkhashps()
 5 {
 6     ChainstateManager& chainman = EnsureAnyChainman(request.context);
 7     LOCK(cs_main);
 8-    return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].get_int() : 120, !request.params[1].isNull() ? request.params[1].get_int() : -1, chainman.ActiveChain());
 9+    const int blocks{!request.params[0].isNull() ? request.params[0].get_int() : 120};
10+    const int height{!request.params[1].isNull() ? request.params[1].get_int() : -1};
11+    return GetNetworkHashPS(blocks, height, chainman.ActiveChain());
12 },
13     };
14 }

It almost certainly compiles to the same thing, is much easier to read and reduces the risk of precedence bugs.

in src/rpc/rawtransaction.cpp:1611 in 586190f0b4

1605@@ -1598,9 +1606,11 @@ static RPCHelpMan utxoupdatepsbt()
1606     CCoinsView viewDummy;
1607     CCoinsViewCache view(&viewDummy);
1608     {
1609-        const CTxMemPool& mempool = EnsureMemPool(request.context);
1610+        NodeContext& node = EnsureAnyNodeContext(request.context);
1611+        const CTxMemPool& mempool = EnsureMemPool(node);
1612+        ChainstateManager& chainman = EnsureChainman(node);

jnewbery commented at 11:38 am on April 15, 2021:

I haven’t commented on all of these examples, but I really do think readability is helped by placing the Ensure*() assumptions at the top of the rpc methods.

jnewbery commented at 12:00 pm on April 15, 2021: member

utACK 586190f0b4740457cb86cba632e3d64e6dfe9b0c

It’s not a blocker, but I don’t like the proliferation of code outside validation holding m_chain. In general, I don’t think there’s any reason for code outside validation to take a reference to a CChain - it should be able to call a higher abstraction function on chainman to read whatever data it needs.

in src/miner.cpp:48 in cced0f46c9 outdated

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, assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman)); return blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());
50+    WITH_LOCK(::cs_main, assert(g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock) == prev_block));

MarcoFalke commented at 10:22 am on April 17, 2021:

cced0f46c9:

I don’t understand this commit. Is the goal to remove this assert? If yes, how are new callers protected from accidentally passing the wrong pointer? If no, you’ll need to pass blockman again, making this commit useless.

Also, this seems to put more code burden on the caller, duplicating the LookupBlockIndex at each call site.

It might help to specify a goal of the commit here. Is the goal to avoid locking cs_main twice instead of only once? If yes, a simpler solution would be to just pass chainman (from the rpc context) instead of the blockman reference (which needs cs_main to be taken from chainman).

in src/rest.cpp:254 in d7824acdb9 outdated

250@@ -250,8 +251,9 @@ static bool rest_block(const std::any& context,
251     CBlockIndex* tip = nullptr;
252     {
253         LOCK(cs_main);
254-        tip = ::ChainActive().Tip();
255-        pblockindex = g_chainman.m_blockman.LookupBlockIndex(hash);
256+        ChainstateManager& chainman = EnsureChainman(context);

MarcoFalke commented at 11:24 am on April 17, 2021:

This may crash the node if the chainman is not available.

0terminate called after throwing an instance of 'UniValue'

MarcoFalke commented at 11:25 am on April 17, 2021: member

I looked at d7824acdb9 🐃

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3I looked at d7824acdb9 🐃
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjV+AwAt/NhM4OI3plsZuJBMc1+PcsEelcHiTdtzMCG+qIWOyxE7zU11aZPYkJ2
 8xhT+gSEwHQxSWENkLb7hf5eU4l23JXvA9JD7WwO3yAptxMQwpmTPzOzyefAONC/z
 92Up7r09SMieAwKKTLuHiAXTSuoA9vqx3dw361XsFhcnM1Pu06zqkYzYumduL9yP6
10fSj22cFffihAaZEIg3leD2LE1CXWcaq80so3kWAv741hXZRx+vPssKUIdNDW2oNr
1150pbZTaiKc7T1wU16KwxeY9xBj1x12pg4WrMhYRDAXIUzwP4gk18gmhhth9EvD6q
12v0bfig+bk4bXt07HvEftTT7yWpMu381mNGZGrbljgGS4NomCRbBKfuvDNqsX17LB
13z8dn54TrW5Pr3//Q46xWZLxhH9f04qH5D6nuhxoghdRXP52Zh1h7A/tRrm2tFJ1t
14TNw7dlDWAUZs8NUlY1Jq6nZlm9DpueU+5/0tubX9fmqYO1k8IFI3T77btrsYC/0L
150gSquAFL
16=bVUN
17-----END PGP SIGNATURE-----

Timestamp of file with hash d97e47afcf29e46d1bcd8c3ce2c34513261879f42b7516ce21deecae7598dee6 -

in src/rest.cpp:181 in 6fb65b49f4 outdated

180@@ -181,7 +181,7 @@ static bool rest_headers(const std::any& context,
181     headers.reserve(count);

MarcoFalke commented at 3:21 pm on April 17, 2021:

6fb65b49f4: the scripted diff will modify files that are not tracked by git. Could use ... -- $(git grep -l Ensure)

MarcoFalke commented at 3:34 pm on April 17, 2021: member

review ACK 586190f0b4740457cb86cba632e3d64e6dfe9b0c 🍯

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 586190f0b4740457cb86cba632e3d64e6dfe9b0c 🍯
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjIYgv/YeuLpuAboBxi/xa/PWLK/q/57E44wyfEF9cZHpOCgFzkR+gfJI1ouI1C
 8bBUBnk0CkOqLfFwi8a1Vzel2zXYvTOtYJ6OPpeau+bxJVhlaI8ysFRuI8oomn4Tx
 9ypQdNYi0bNMIIiuwezoYX+dPMkBRA8Ox8JuMZ/SUuO1ZH8F66s2U9EstshDhFiS5
10cN9L9kujU0ZovHAnutbEF5zSFDqR/brrmuhCg0ACE5rCla87AAQqe/29SIcKvbls
11TiV9EEHWj5V8eRy/CLg2ch+bGOFp4vISWJBITYdho7hnwMLRMTHC/K5RvsfR703R
12t3reCFwd4whXkcBGN6z43f16GM3fvRNJVKKT7rQ0FB6n0oVjkFE3Gqxln5Ke209N
13wDAbn99WerhV8tGhOhgYTcG1N9awQBTH4aZEuOt2MrtGxYkUEXw7+mchbrQ4iPPs
1475AoZ51CaEWESzIYWRV82IBtNCtD0UUvGUOuo3E3fXCqrVKBmU7JnSuMDQ3UaPR5
15QVKk2RIm
16=kNPA
17-----END PGP SIGNATURE-----

Timestamp of file with hash 3e1b6d60dc76018210f94a83c678a1e0f9c63606e06fb1b3d34881ee98abc0d8 -

MarcoFalke merged this on Apr 17, 2021

MarcoFalke closed this on Apr 17, 2021

sidhujag referenced this in commit 4372e26b0d on Apr 17, 2021

in src/rpc/blockchain.cpp:1107 in 586190f0b4

1106+    CCoinsView* coins_view;
1107+    BlockManager* blockman;
1108+    {
1109+        LOCK(::cs_main);
1110+        coins_view = &active_chainstate.CoinsDB();
1111+        blockman = &active_chainstate.m_blockman;

jnewbery commented at 3:18 pm on May 9, 2021:

This doesn’t require cs_main.

in src/rpc/blockchain.cpp:1103 in 586190f0b4

1102 
1103-    CCoinsView* coins_view = WITH_LOCK(::cs_main, return &::ChainstateActive().CoinsDB());
1104-    NodeContext& node = EnsureNodeContext(request.context);
1105-    if (GetUTXOStats(coins_view, WITH_LOCK(::cs_main, return std::ref(g_chainman.m_blockman)), stats, hash_type, node.rpc_interruption_point)) {
1106+    CCoinsView* coins_view;
1107+    BlockManager* blockman;

jnewbery commented at 3:20 pm on May 9, 2021:

It’s slightly confusing to take the references from CoinsDB() and m_blockman and take their addresses to make pointers, and then just dereference those pointers. Better just to use references as the local variables.

MarcoFalke referenced this in commit f63fc53c2a on Jun 1, 2021

Fabcien referenced this in commit 053eb6c9cd on May 23, 2022

Fabcien referenced this in commit 3ff0431d42 on May 23, 2022

Fabcien referenced this in commit 816bdfb899 on May 23, 2022

Fabcien referenced this in commit c3bd9bc2d4 on Jun 9, 2022

Fabcien referenced this in commit 041fad036e on Jun 9, 2022

Fabcien referenced this in commit 77cb6d1931 on Jun 9, 2022

Fabcien referenced this in commit 3d100ae1b7 on Jun 9, 2022

Fabcien referenced this in commit 0aa5c2bddb on Jun 9, 2022

DrahtBot locked this on Aug 18, 2022

[Bundle 5/n] Prune g_chainman usage in RPC modules #21391

Conflicts