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

dongcarl · 2021-02-01T17:09:24Z

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

Based on:

[Bundle 2/n] Prune g_chainman usage in mempool-related validation functions #20750 | [Bundle 2/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

Note to self:

Address: [Bundle 2/n] Prune g_chainman usage in mempool-related validation functions #20750 (comment)

DrahtBot · 2021-02-01T23:42:19Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

[Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270 ([Bundle 4/n] Prune g_chainman usage in validation-adjacent modules by dongcarl)
Default to NODE_WITNESS in nLocalServices #21090 (Default to NODE_WITNESS in nLocalServices by dhruv)
Remove RewindBlockIndex logic #21009 (Remove RewindBlockIndex logic by dhruv)
rpc/validation: enable packages through testmempoolaccept #20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
allow -loadblock blocks to be unsorted #20331 (allow -loadblock blocks to be unsorted by LarryRuane)
Replace unused BIP 9 logic with draft BIP 8 #19573 (Replace unused BIP 9 logic with draft BIP 8 by luke-jr)
Introduce deploymentstatus #19438 (Introduce deploymentstatus by ajtowns)

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

Code review ACK f5ee2f3

ryanofsky · 2021-02-02T21:19:39Z

src/validation.cpp

@@ -4112,11 +4112,12 @@ void BlockManager::Unload() {
    m_block_index.clear();
 }

-bool static LoadBlockIndexDB(ChainstateManager& chainman, const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+bool CChainState::LoadBlockIndexDB(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main)


In commit "validation: Move LoadBlockIndexDB to CChainState" (fa730f7)

It seems chainman variable wasn't really needed here, just the chainman.m_blockman member, so this might make more sense as a BlockManager method instead of CChainState method

Unfortunately we need setBlockIndexCandidates which is a member off CChainState... If you think LoadBlockIndexDB makes more sense logically as a method of BlockManager rather than CChainState we can pass in the setBlockIndexCandidates. Let me know!

re: #21055 (comment)

Unfortunately we need setBlockIndexCandidates which is a member off CChainState... If you think LoadBlockIndexDB makes more sense logically as a method of BlockManager rather than CChainState we can pass in the setBlockIndexCandidates. Let me know!

Thanks, I didn't recognize the setBlockIndexCandidates access. Proposed change sounds reasonable, but I don't actually know too much about it and it would seem good to tackle as a followup not changing here.

src/net_processing.cpp

ariard

See about the maybe missing assert otherwise code review ACK f5ee2f3

ariard · 2021-02-08T13:06:43Z

src/validation.cpp

@@ -4973,24 +4973,6 @@ CBlockFileInfo* GetBlockFileInfo(size_t n)
    return &vinfoBlockFile.at(n);
 }

-ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::DeploymentPos pos)
-{
-    LOCK(cs_main);


This global was introduced by d23f6c6. At that commit, the globalVersionBitsTipState's unique callerBIP9SoftForkDesc didn't require cs_main locking.

Those methods were refactored by 3862e47, where the new BIP9SoftForkDescPushBack did require the lock. I think since then this lock has been useless and could have been substituted by an annotation.

So not a concern to get rid of them.

src/net_processing.cpp

dongcarl · 2021-02-18T20:09:09Z

Pushed f5ee2f3 -> eb8f8e4

Rebased on master

Note: Have not yet addressed review.

dongcarl · 2021-02-18T20:48:47Z

Pushed eb8f8e4 -> 914e9d3

Addressed: [Bundle 3/n] Prune remaining g_chainman usage in validation functions #21055 (comment)

jnewbery · 2021-02-20T10:47:01Z

Concept ACK. This may need rebase now that #20750 is merged.

...instead of recursively locking unconditionally

dongcarl · 2021-03-03T19:58:01Z

Pushed ef83bb8 -> e11b649

Applied [Bundle 3/n] Prune remaining g_chainman usage in validation functions #21055 (comment)
Addressed [Bundle 3/n] Prune remaining g_chainman usage in validation functions #21055 (comment)

Thanks jnewbery for your diligent review!

laanwj · 2021-03-04T13:50:44Z

Code review ACK e11b649

… validation functions

ryanofsky

Code review ACK e11b649. Changes since last review: rebase, two new commits adding null assert and lock assert, replacing mempool check coins and spendheight arguments

…ent modules a67983c net_processing: Add review-only assertion to PeerManager (Carl Dong) 272d993 scripted-diff: net_processing: Use existing chainman (Carl Dong) 021a04a net_processing: Move some static functions to PeerManager (Carl Dong) 91c5b68 node/ifaces: ChainImpl: Use existing NodeContext member (Carl Dong) 8a1d580 node/ifaces: NodeImpl: Use existing NodeContext member (Carl Dong) 4cde4a7 node: Use existing NodeContext (Carl Dong) 106bcd4 node/coinstats: Pass in BlockManager to GetUTXOStats (Carl Dong) 2c3ba00 miner: Pass in blockman to ::RegenerateCommitments (Carl Dong) 2afcf24 miner: Remove old CreateNewBlock w/o chainstate param (Carl Dong) 46b7f29 scripted-diff: Invoke CreateNewBlock with chainstate (Carl Dong) d0de61b miner: Pass in chainstate to BlockAssembler::CreateNewBlock (Carl Dong) a04aac4 validation: Remove extraneous LoadGenesisBlock function prototype (Carl Dong) Pull request description: Overall PR: #20158 (tree-wide: De-globalize ChainstateManager) Based on: - [x] #21055 | [Bundle 3/n] Prune g_chainman usage in mempool-related validation functions 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: laanwj: Code review ACK a67983c ryanofsky: Code review ACK a67983c. Only change since last review new first commit fixing header declaration, and rebase glozow: code review ACK a67983c Tree-SHA512: dce182a18b88be80cbf50978d4ba8fa6ab0f01e861d09bae0ae9364051bb78f9334859d164b185b07f1d70a583e739557fab6d820cac8c37b3855b85c2a6771b

Summary: ...also update comments to remove mention of ::ChainActive() From: bitcoin/bitcoin#20750 (comment) > Also, what about passing a const reference instead of a pointer? I > know this is only theoretical, but previously if the tip was nullptr, > then Height() evaluated to -1, now it evaluates to UB Note: in Bitcoin ABC, there was already a check for `active_chain_tip == nullptr`, but only for `nMedianPastTime`. A few lines above we accessed the pointer to get the `nHeight` without checking first (potential UB introduced in D11191). This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [1/16] bitcoin/bitcoin@9da106b Test Plan: ``` cmake -GNinja .. \ -DCMAKE_BUILD_TYPE=Debug \ -DENABLE_SANITIZERS=undefined ninja check check-functional ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11205

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: Note: `FinalizeBlock` was already addressed in D6861 This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [3/16] bitcoin/bitcoin@a3ba08b Depends on D11206 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11207

Summary: Note: I used this opportunity to reorder the function parameters and match the ones used by Core. This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [4/16] bitcoin/bitcoin@4bada76 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11209

Summary: This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [5/16] bitcoin/bitcoin@63e4c73 Depends on D11209 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11210

Summary: Tip: versionbitscache is currently a global so we didn't need to pass it in to any of ::VersionBitsTip*'s callers Notes: - I had to restore `versionbitscache` in validation.h, removed in D2783. - `VersionBitsBlockState` was not used anywhere. It was introduced and used in D5282, then `minerfund.cpp` was deleted then added again but without using that function. This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [6/16] bitcoin/bitcoin@31eac50 Depends on D11210 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11211

Summary: This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [7/16] bitcoin/bitcoin@2bdf37f Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11212

Summary: Notes: - `InvalidChainFound` was already moved to `CChainState` in D7573 - `CheckForkWarningConditionsOnNewFork` was removed by core in [[bitcoin/bitcoin#19905 | core#19905]] because it doesn't work, but in Bitcoin ABC it was fixed in D1950. This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [8/16] bitcoin/bitcoin@8b99efb Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11213

Summary: CChainState needed cuz setBlockIndexCandidates This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [9/16] bitcoin/bitcoin@8cdb2f7 Depends on D11213 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11214

Summary: This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [10/16] bitcoin/bitcoin@1fb7b2c Depends on D11214 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11215

Summary: This is the only instance where validation reaches for something outside of it. This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [11/16] bitcoin/bitcoin@4744efc Depends on D11215 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11216

Summary: This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [12/16] bitcoin/bitcoin@a9d28bc Depends on D11216 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11219

Summary: This is a backport of [[bitcoin/bitcoin#21055 | core#21055]] [13/16] bitcoin/bitcoin@fee7334 Depends on D11219 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11220

Summary: instead of recursively locking unconditionally This concludes backport of [[bitcoin/bitcoin#21055 | core#21055]] [16/16] bitcoin/bitcoin@e11b649 Depends on D11221 Test Plan: With debug and clang: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11222

9376a6d refactor: make active_chain_tip a reference (Aurèle Oulès) Pull request description: This PR fixes a TODO introduced in bitcoin#21055. Makes `active_chain_tip` argument in `CheckFinalTxAtTip` function a reference instead of a pointer. ACKs for top commit: dongcarl: ACK 9376a6d Tree-SHA512: c36d1769e0b9598b7f79334704b26b73e958d54caa3bd7e4eff954f3964fcf3f5e3a44a5a760497afad51b76e1614c86314fe035e4083c855e3574a620de7f4d

dongcarl mentioned this pull request Feb 1, 2021

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

DrahtBot added Consensus Mempool P2P RPC/REST/ZMQ Validation labels Feb 1, 2021

This was referenced Feb 2, 2021

Remove RewindBlockIndex logic #21009

Merged

test: Move MakeNoLogFileContext to libtest_util, and use it in bench #21003

Merged

rpc/validation: enable packages through testmempoolaccept #20833

Merged

dongcarl force-pushed the 2020-09-reduce-validation-ccsactiveglobal-usage branch from adb0c77 to f5ee2f3 Compare February 2, 2021 02:50

This was referenced Feb 2, 2021

fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) #19259

Merged

refactor: return MempoolAcceptResult from ATMP #21062

Merged

ryanofsky approved these changes Feb 2, 2021

View reviewed changes

This was referenced Feb 2, 2021

Introduce deploymentstatus #19438

Merged

Replace unused BIP 9 logic with draft BIP 8 #19573

Closed

allow -loadblock blocks to be unsorted #20331

Closed

Default to NODE_WITNESS in nLocalServices #21090

Merged

ariard reviewed Feb 8, 2021

View reviewed changes

This was referenced Feb 9, 2021

[Bundle 2/n] Prune g_chainman usage in mempool-related validation functions #20750

Merged

fuzz: Add tx_pool fuzz target #21142

Merged

validation/coins: limit MemPoolAccept access to coins cache + make it responsible for uncaching #21146

Closed

DrahtBot added the Needs rebase label Feb 11, 2021

dongcarl force-pushed the 2020-09-reduce-validation-ccsactiveglobal-usage branch from f5ee2f3 to eb8f8e4 Compare February 18, 2021 20:07

DrahtBot removed the Needs rebase label Feb 18, 2021

dongcarl force-pushed the 2020-09-reduce-validation-ccsactiveglobal-usage branch from eb8f8e4 to 914e9d3 Compare February 18, 2021 20:48

DrahtBot mentioned this pull request Feb 19, 2021

Split orphan handling from net_processing into txorphanage #21148

Merged

dongcarl added 4 commits March 3, 2021 14:49

validation: Pass in chain to FindBlockPos+SaveBlockToDisk

fee7334

validation: Use existing chain member in CChainState::AcceptBlock

5e4af77

validation: Use existing chain member in CChainState::LoadGenesisBlock

03f75c4

validation: CVerifyDB::VerifyDB: Use locking annotation

e11b649

...instead of recursively locking unconditionally

dongcarl force-pushed the 2020-09-reduce-validation-ccsactiveglobal-usage branch from ef83bb8 to e11b649 Compare March 3, 2021 19:56

laanwj merged commit 702cfc8 into bitcoin:master Mar 4, 2021

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

Merge bitcoin#21055: [Bundle 3/n] Prune remaining g_chainman usage in…

5a7dfaa

… validation functions

ryanofsky reviewed Mar 6, 2021

View reviewed changes

klementtan mentioned this pull request Jun 19, 2021

refactor: CheckFinalTx pass by reference instead of pointer #22282

Closed

aureleoules mentioned this pull request Jul 22, 2022

refactor: make active_chain_tip a reference #25677

Merged

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

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

dongcarl commented Feb 1, 2021 •

edited

Loading

DrahtBot commented Feb 1, 2021 •

edited

Loading

ryanofsky left a comment

ryanofsky Feb 2, 2021

dongcarl Feb 18, 2021

ryanofsky Mar 5, 2021

ariard left a comment

ariard Feb 8, 2021

dongcarl commented Feb 18, 2021

dongcarl commented Feb 18, 2021

jnewbery commented Feb 20, 2021

dongcarl commented Mar 3, 2021

laanwj commented Mar 4, 2021

ryanofsky left a comment

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

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

Conversation

dongcarl commented Feb 1, 2021 • edited Loading

DrahtBot commented Feb 1, 2021 • edited Loading

Conflicts

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Feb 2, 2021

Choose a reason for hiding this comment

dongcarl Feb 18, 2021

Choose a reason for hiding this comment

ryanofsky Mar 5, 2021

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

ariard Feb 8, 2021

Choose a reason for hiding this comment

dongcarl commented Feb 18, 2021

dongcarl commented Feb 18, 2021

jnewbery commented Feb 20, 2021

dongcarl commented Mar 3, 2021

laanwj commented Mar 4, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

dongcarl commented Feb 1, 2021 •

edited

Loading

DrahtBot commented Feb 1, 2021 •

edited

Loading