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

dongcarl · 2021-02-22T17:26:14Z

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

Based on:

[Bundle 3/n] Prune remaining g_chainman usage in validation functions #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

DrahtBot · 2021-02-23T00:55:49Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

🐙 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".}

ryanofsky

Code review ACK last 11 commits only a0741fa. Changes since last review in #20158 (review): rebase, and a few more functions moved in move static functions commit

jnewbery · 2021-03-06T18:01:38Z

Concept ACK. Will review after rebase.

dongcarl · 2021-03-08T20:36:52Z

Pushed a0741fa -> 38c325b

Rebased over master

Leftover from last bundle.

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

REQUIRES ATTENTION

- BlockRequestAllowed - AlreadyHaveBlock - ProcessGetBlockData - PrepareBlockFilterRequest - ProcessGetCFilters - ProcessGetCFHeaders - ProcessGetCFCheckPt Moved out of anonymous namespace: - ProcessBlockAvailability - UpdateBlockAvailability - CanDirectFetch

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

dongcarl · 2021-03-08T20:57:11Z

Pushed 38c325b -> a67983c

Addressed [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270 (comment) in prepended commit
Fixed namespace brace syntax mistake in "net_processing: Move some static functions to PeerManager"

ryanofsky

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

glozow

code review ACK a67983c

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&

glozow · 2021-03-11T00:53:25Z

src/miner.cpp

 {
    CMutableTransaction tx{*block.vtx.at(0)};
    tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));
    block.vtx.at(0) = MakeTransactionRef(tx);

-    GenerateCoinbaseCommitment(block, WITH_LOCK(cs_main, return g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());
+    GenerateCoinbaseCommitment(block, WITH_LOCK(::cs_main, assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman)); return blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());


I think this line would give jnewbery a heart attack...

Suggested change

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

GenerateCoinbaseCommitment(block,

WITH_LOCK(::cs_main,

assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman));

return blockman.LookupBlockIndex(block.hashPrevBlock)),

Params().GetConsensus());

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)

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

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:

Suggested change

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

CBlockIndex* index;

{

LOCK(::cs_main);

assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman));

index = blockman.LookupBlockIndex(block.hashPrevBlock);

}

GenerateCoinbaseCommitment(block, index, Params().GetConsensus());

laanwj · 2021-03-11T10:46:21Z

Code review ACK a67983c

jnewbery · 2021-03-11T13:33:04Z

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

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.

jnewbery · 2021-03-11T11:36:15Z

src/miner.cpp

@@ -39,13 +39,13 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
    return nNewTime - nOldTime;
 }

-void RegenerateCommitments(CBlock& block)
+void RegenerateCommitments(CBlock& block, BlockManager& blockman)


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.

src/node/coinstats.h

jnewbery · 2021-03-11T11:45:42Z

src/node/coinstats.h

@@ -36,6 +37,6 @@ struct CCoinsStats
 };

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


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

jnewbery · 2021-03-11T11:51:38Z

src/node/interfaces.cpp

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


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.

jnewbery · 2021-03-11T11:55:35Z

src/node/interfaces.cpp

-        const CBlockIndex* ancestor = g_chainman.m_blockman.LookupBlockIndex(ancestor_hash);
+        assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman));
+        const CBlockIndex* block = m_node.chainman->m_blockman.LookupBlockIndex(block_hash);
+        assert(std::addressof(g_chainman) == std::addressof(*m_node.chainman));


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

jnewbery · 2021-03-11T11:56:23Z

src/node/interfaces.cpp

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


Same comment as above about holding cs_main for this assert.

src/net_processing.cpp

jnewbery · 2021-03-11T12:00:40Z

src/net_processing.cpp

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


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

jnewbery · 2021-03-11T13:25:57Z

src/net_processing.cpp

@@ -1920,27 +1939,27 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer,
        // because it is set in UpdateBlockAvailability. Some nullptr checks
        // are still present, however, as belt-and-suspenders.

-        if (received_new_header && pindexLast->nChainWork > ::ChainActive().Tip()->nChainWork) {
+        if (received_new_header && pindexLast->nChainWork > m_chainman.ActiveChain().Tip()->nChainWork) {


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.

…n-adjac… …ent modules

maflcko · 2021-03-12T11:14:30Z

src/miner.cpp

@@ -99,7 +99,7 @@ void BlockAssembler::resetBlock()
 Optional<int64_t> BlockAssembler::m_last_block_num_txs{nullopt};
 Optional<int64_t> BlockAssembler::m_last_block_weight{nullopt};

-std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
+std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(CChainState& chainstate, const CScript& scriptPubKeyIn)


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.

src/net_processing.cpp

maflcko · 2021-03-12T11:15:57Z

src/node/coin.cpp

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


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

maflcko · 2021-03-12T11:17:27Z

src/node/interfaces.cpp

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


maflcko · 2021-03-12T11:22:21Z

src/node/transaction.cpp

    // If the transaction is already confirmed in the chain, don't do anything
    // and return early.
-    CCoinsViewCache &view = ::ChainstateActive().CoinsTip();
+    CCoinsViewCache &view = node.chainman->ActiveChainstate().CoinsTip();


dongcarl · 2021-03-12T22:12:30Z

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!

693414d node/ifaces: ChainImpl: Use an accessor for ChainMan (Carl Dong) 98c4e25 node/ifaces: NodeImpl: Use an accessor for ChainMan (Carl Dong) 7e8b5ee validation: Make BlockManager::LookupBlockIndex const (Carl Dong) 88aead2 node: Avoid potential UB by asserting assumptions (Carl Dong) 1dd8ed7 net_processing: Move comments to declarations (Carl Dong) 07156eb node/coinstats: Replace #include with fwd-declaration (Carl Dong) 7b8e976 miner: Add chainstate member to BlockAssembler (Carl Dong) e62067e Revert "miner: Pass in chainstate to BlockAssembler::CreateNewBlock" (Carl Dong) eede064 Revert "scripted-diff: Invoke CreateNewBlock with chainstate" (Carl Dong) 0c1b2bc Revert "miner: Remove old CreateNewBlock w/o chainstate param" (Carl Dong) Pull request description: Chronological history of this changeset: 1. Bundle 4 (#21270) got merged 2. Posthumous reviews were posted 3. These changes were prepended in bundle 5 4. More reviews were added in bundle 5 5. Someone suggested that we split the prepended changes up to another PR 6. This is that PR In the future, I will just do posthumous review changes in another PR instead. I apologize for the confusion. Addresses posthumous reviews on bundle 4: - From jnewbery: - bitcoin/bitcoin#21270 (comment) - I didn't fix this one, but I added a `TODO` comment so that we don't lost track of it - bitcoin/bitcoin#21270 (comment) - bitcoin/bitcoin#21270 (comment) - bitcoin/bitcoin#21270 (comment) - bitcoin/bitcoin#21270 (comment) - From MarcoFalke: - bitcoin/bitcoin#21270 (comment) - bitcoin/bitcoin#21270 (comment) - bitcoin/bitcoin#21270 (comment) - bitcoin/bitcoin#21270 (comment) Addresses reviews on bundle 5: - Checking chainman existence before locking cs_main - MarcoFalke - bitcoin/bitcoin#21391 (comment) - bitcoin/bitcoin#21391 (comment) - Appropriate locking, usage of chainman, and control flow in `src/node/interfaces.cpp` - MarcoFalke - bitcoin/bitcoin#21391 (comment) - jnewbery - bitcoin/bitcoin#21391 (comment) - bitcoin/bitcoin#21391 (comment) - ryanofsky - bitcoin/bitcoin#21391 (comment) - Style/comment formatting changes - jnewbery - bitcoin/bitcoin#21391 (comment) - bitcoin/bitcoin#21391 (comment) - Making LookupBlockIndex const - jnewbery - bitcoin/bitcoin#21391 (comment) ACKs for top commit: MarcoFalke: review ACK 693414d 🛐 ryanofsky: Code review ACK 693414d. I reviewed this previously as part of #21391. I am a fan of the increasingly complicated bundle numbering, and kind of hope there in the next round there is some way we can get bundles 5.333333 and 5.666667! jamesob: ACK 693414d ([`jamesob/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f`](https://github.com/jamesob/bitcoin/tree/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f)) Tree-SHA512: 9bdc199f70400d01764e1bd03c25bdb6cff26dcef60e4ca3b649baf8d017a2dfc1f058099067962b4b6ccd32d078002b1389d733039f4c337558cb70324c0ee3

586190f rpc/rest: Take and reuse local Chain/ChainState obj (Carl Dong) bc3bd36 rpc: style: Improve BuriedForkDescPushBack signature (Carl Dong) f999139 rpc: Remove unnecessary casting of block height (Carl Dong) 6a3d192 rpc: Tidy up local references (see commit message) (Carl Dong) 038854f rest/rpc: Remove now-unused old Ensure functions (Carl Dong) 6fb65b4 scripted-diff: rest/rpc: Use renamed EnsureAny*() (Carl Dong) 1570c7e rpc: Add renamed EnsureAny*() functions (Carl Dong) 306b1cd rpc: Add alt Ensure* functions acepting NodeContext (Carl Dong) d7824ac rest: Use existing NodeContext (Carl Dong) 3f08934 rest: Pass in NodeContext to rest_block (Carl Dong) 7be0671 rpc/rawtx: Use existing NodeContext (Carl Dong) 60dc05a rpc/mining: Use existing NodeContext (Carl Dong) d485e81 rpc/blockchain: Use existing NodeContext (Carl Dong) d0abf0b rpc/*,rest: Add review-only assertion to EnsureChainman (Carl Dong) cced0f4 miner: Pass in previous CBlockIndex to RegenerateCommitments (Carl Dong) Pull request description: Overall PR: #20158 (tree-wide: De-globalize ChainstateManager) Based on: - [x] #21270 | [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules - [x] #21525 | [Bundle 4.5/n] Followup fixups to bundle 4 Note to reviewers: 1. 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](https://github.com/bitcoin/bitcoin/pull/20158/commits) 2. 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](https://github.com/bitcoin/bitcoin/pull/20158/commits) 3. 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` ACKs for top commit: ryanofsky: Code review ACK 586190f. 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! jnewbery: utACK 586190f MarcoFalke: review ACK 586190f 🍯 Tree-SHA512: 64b677fb50141805b55c3f1afe68fcd298f9a071a359bdcd63256d52e334f83e462f31fb3ebee9b630da8f1d912a03a128cfc38179e7aaec29a055744a98478c

Summary: This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [2/16] and [[bitcoin/bitcoin#21270 | core#21270]] [1/12] bitcoin/bitcoin@4927c9e bitcoin/bitcoin@a04aac4 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11206

Summary: This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [2, 3 & 4/9] bitcoin/bitcoin@d0de61b bitcoin/bitcoin@46b7f29 bitcoin/bitcoin@2afcf24 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11223

Summary: This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [6/12] and [[bitcoin/bitcoin#21525 | core#21525]][5/10] bitcoin/bitcoin@106bcd4 bitcoin/bitcoin@07156eb Depends on D11420 Test Plan: `ninja all check-all ` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11422

Summary: This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [7/12] and [[bitcoin/bitcoin#21525 | core#21525]] [7/10] bitcoin/bitcoin@4cde4a7 bitcoin/bitcoin@88aead2 Depends on D11422 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11423

Summary: This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [8/12] and [[bitcoin/bitcoin#21525 | core#21525]] [9/10] bitcoin/bitcoin@8a1d580 bitcoin/bitcoin@98c4e25 Depends on D11423 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11424

Summary: This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [9/12] and [[bitcoin/bitcoin#21525 | core#21525]] [10/10] bitcoin/bitcoin@91c5b68 bitcoin/bitcoin@693414d Depends on D11424 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11425

Summary: - BlockRequestAllowed - AlreadyHaveBlock - AlreadyHaveProof - PrepareBlockFilterRequest - ProcessGetBlockData - ProcessGetCFilters - ProcessGetCFHeaders - ProcessGetCFCheckPt Moved out of anonymous namespace: - ProcessBlockAvailability - UpdateBlockAvailability - CanDirectFetch This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [10/12] bitcoin/bitcoin@021a04a Depends on D11493 Test Plan: With clangh and debug (for thread safety analysis) `ninja all check-all` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D11495

Summary: ``` -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 arc lint -END VERIFY SCRIPT- ``` This is a backport of [[bitcoin/bitcoin#21270 | core#21270]] [11/12] bitcoin/bitcoin@272d993 Depends on D11495 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D11497

dongcarl added Refactoring Validation labels Feb 22, 2021

dongcarl mentioned this pull request Feb 22, 2021

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

DrahtBot mentioned this pull request Mar 2, 2021

[Bundle 3/n] Prune remaining g_chainman usage in validation functions #21055

Merged

2 tasks

DrahtBot added the Needs rebase label Mar 4, 2021

ryanofsky approved these changes Mar 6, 2021

View reviewed changes

dongcarl force-pushed the 2020-09-libbitcoinruntime-v6 branch from a0741fa to 38c325b Compare March 8, 2021 20:35

dongcarl added 7 commits March 8, 2021 15:54

validation: Remove extraneous LoadGenesisBlock function prototype

a04aac4

Leftover from last bundle.

miner: Pass in chainstate to BlockAssembler::CreateNewBlock

d0de61b

scripted-diff: Invoke CreateNewBlock with chainstate

46b7f29

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

miner: Remove old CreateNewBlock w/o chainstate param

2afcf24

miner: Pass in blockman to ::RegenerateCommitments

2c3ba00

REQUIRES ATTENTION

node/coinstats: Pass in BlockManager to GetUTXOStats

106bcd4

node: Use existing NodeContext

4cde4a7

dongcarl added 3 commits March 8, 2021 15:54

scripted-diff: net_processing: Use existing chainman

272d993

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

net_processing: Add review-only assertion to PeerManager

a67983c

dongcarl force-pushed the 2020-09-libbitcoinruntime-v6 branch from 38c325b to a67983c Compare March 8, 2021 20:55

dongcarl mentioned this pull request Mar 8, 2021

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

Merged

2 tasks

dongcarl removed the Needs rebase label Mar 8, 2021

fanquake referenced this pull request Mar 9, 2021

validation: Remove global ::LoadGenesisBlock

4927c9e

ryanofsky approved these changes Mar 9, 2021

View reviewed changes

glozow reviewed Mar 11, 2021

View reviewed changes

laanwj merged commit 767bb7d into bitcoin:master Mar 11, 2021

jnewbery reviewed Mar 11, 2021

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 11, 2021

Merge bitcoin#21270: [Bundle 4/n] Prune g_chainman usage in validatio…

86a818d

…n-adjac… …ent modules

maflcko reviewed Mar 12, 2021

View reviewed changes

dongcarl mentioned this pull request Mar 24, 2021

[Bundle 4.5/n] Followup fixups to bundle 4 #21525

Merged

dongcarl mentioned this pull request Apr 26, 2021

followups: Various potential improvements arising out of chainman-deglobalizing review #21766

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dongcarl commented Feb 22, 2021 •

edited

Loading

DrahtBot commented Feb 23, 2021 •

edited

Loading

DrahtBot commented Mar 4, 2021

ryanofsky left a comment

jnewbery commented Mar 6, 2021

dongcarl commented Mar 8, 2021

dongcarl commented Mar 8, 2021

ryanofsky left a comment

glozow left a comment

glozow Mar 11, 2021

laanwj Mar 11, 2021 •

edited

Loading

maflcko Mar 11, 2021

laanwj Mar 11, 2021

ghostofjnewbery Mar 11, 2021

laanwj commented Mar 11, 2021

jnewbery commented Mar 11, 2021

jnewbery left a comment

jnewbery Mar 11, 2021

jnewbery Mar 11, 2021

jnewbery Mar 11, 2021

jnewbery Mar 11, 2021

jnewbery Mar 11, 2021

jnewbery Mar 11, 2021

jnewbery Mar 11, 2021

maflcko Mar 12, 2021

maflcko Mar 12, 2021

maflcko Mar 12, 2021

maflcko Mar 12, 2021

dongcarl commented Mar 12, 2021

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

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

Conversation

dongcarl commented Feb 22, 2021 • edited Loading

DrahtBot commented Feb 23, 2021 • edited Loading

Conflicts

DrahtBot commented Mar 4, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

jnewbery commented Mar 6, 2021

dongcarl commented Mar 8, 2021

dongcarl commented Mar 8, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Mar 11, 2021

jnewbery commented Mar 11, 2021

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongcarl commented Mar 12, 2021

dongcarl commented Feb 22, 2021 •

edited

Loading

DrahtBot commented Feb 23, 2021 •

edited

Loading

laanwj Mar 11, 2021 •

edited

Loading