Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tree-wide: De-globalize ChainstateManager #20158

Closed
wants to merge 70 commits into from

Conversation

dongcarl
Copy link
Contributor

@dongcarl dongcarl commented Oct 15, 2020

Prelude

Note to reviewers: Currently looking Code Review on Sub-PRs (see below)

Originally: #20049

Sub-PRs

Last updated May 9th, 2020

Todo List

Last updated Feb 1st, 2020

Motivation

Modularizing our consensus engine

Excerpt from #20049

From my reading of past conversations and from a few offline chats, it seems that modularizing our consensus engine is a worthwhile first step towards a more complete isolation of said engine from non-consensus code.

Modularizing our consensus engine means that:

  1. We get clearer visibility into what currently lies in consensus codepaths and what depends on our consensus engine
  2. We can coalesce duplicate consensus initialization codepaths, mitigating against bugs that arise out of test/non-test initialization inconsistencies

De-globalizing g_chainman

Excerpt from #20049

In order to modularize our consensus engine, we need to first de-globalize the global ChainstateManager -- namely g_chainman -- as it and its dependencies are what makes up the bulk of our consensus engine. A few direct references to g_chainman have already been removed in #19927, however, its indirect uses (mainly via ::Chain(state|)Active()) are numerous in our codebase and often used to cheat avoid obtaining a ChainstateManager reference.

Description

This changeset moves the global ChainstateManger to NodeContext and removes ::Chain{state,}Active() as a first step towards better modularization of our consensus engine.

The commits are ordered as such:

  1. Fixes to our existing codebase crucial to subsequent changes in this changeset
    master...dongcarl:2020-10-chainman-fixes
  2. Remove all references to g_chainman, ::Chain{state,}Active()
    1. In src/validation.{cpp,h}
      1. In a bundle of functions related to ::LookupBlockIndex in the call graph
        dongcarl/bitcoin@2020-10-chainman-fixes...dongcarl:2020-09-libbitcoinruntime-v4
      2. In a bundle of functions that are mempool-related
        dongcarl/bitcoin@dongcarl:2020-09-libbitcoinruntime-v4...2020-09-reduce-validation-mempool-ccsactiveglobal-usage
      3. In a bundle of functions which do not belong in previous bundles
        dongcarl/bitcoin@2020-09-reduce-validation-mempool-ccsactiveglobal-usage...dongcarl:2020-09-reduce-validation-ccsactiveglobal-usage
    2. In "validation-adjacent" modules of the codebase
      dongcarl/bitcoin@dongcarl:2020-09-reduce-validation-ccsactiveglobal-usage...2020-09-libbitcoinruntime-v6
      1. In src/txmempool.cpp
      2. In src/miner.cpp
      3. In src/node
      4. In src/net_processing.cpp
    3. In RPC modules of the codebase
      dongcarl/bitcoin@2020-09-libbitcoinruntime-v6...dongcarl:2020-10-libbitcoinruntime-v7
      1. In src/rpc
      2. In src/rest.cpp
    4. In auxiliary modules of the codebase
      dongcarl/bitcoin@dongcarl:2020-10-libbitcoinruntime-v7...2020-10-libbitcoinruntime-v8
      1. In src/bench
      2. In src/index
    5. In initialization codepaths and tests
      dongcarl/bitcoin@2020-10-libbitcoinruntime-v8...dongcarl:2020-06-libbitcoinruntime
      1. In src/init.cpp
      2. In src/test
      3. In src/wallet/test
      4. In src/qt/test

A few things to note:

  • The above ordering is constructed according to the overall call graph of our codebase such that we avoid touching the same function/module twice.
  • Due to the length of this overall change, each commit aims to be trivially-reviewable and only requires the reviewer to reason about the correctness of one function/module at a time.
  • There are a lot of review-only assertions which can be used to check for correctness. They are removed in 2236237.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 15, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

MarcoFalke added a commit to MarcoFalke/bitcoin-core that referenced this pull request Feb 20, 2021
…elated validation functions

e8ae1db style-only: Make AcceptToMemoryPool signature readable (Carl Dong)
8f5c100 style-only: Make CheckSequenceLock signature readable (Carl Dong)
8c82481 validation: Use *this in CChainState::LoadMempool (Carl Dong)
0a9a24d validation: Pass in chainstate to UpdateMempoolForReorg (Carl Dong)
7142018 validation: Pass in chainstate to CTxMemPool::removeForReorg (Carl Dong)
71734c6 validation: Pass in chain to ::TestLockPointValidity (Carl Dong)
120aaba tree-wide: Fix erroneous AcceptToMemoryPool replacements (Carl Dong)
417dafc validation: Remove old AcceptToMemoryPool w/o chainstate param (Carl Dong)
3704433 scripted-diff: Invoke ::AcceptToMemoryPool with chainstate (Carl Dong)
229bc37 validation: Pass in chainstate to ::AcceptToMemoryPool (Carl Dong)
d0da7ea validation: Pass in chainstate to ::LoadMempool (Carl Dong)
3a205c4 validation: Pass in chainstate to AcceptToMemoryPoolWithTime (Carl Dong)
d8a8163 validation: Add chainstate member to MemPoolAccept (Carl Dong)
4c15942 validation: Pass in chainstate to ::CheckSequenceLocks (Carl Dong)
577b774 validation: Remove old CheckFinalTx w/o chain tip param (Carl Dong)
7031cf8 scripted-diff: Invoke ::CheckFinalTx with chain tip (Carl Dong)
d015eaa validation: Pass in chain tip to ::CheckFinalTx (Carl Dong)
252b489 validation: Pass in coins tip to CheckInputsFromMempoolAndCache (Carl Dong)
73a6d2b validation: Pass in chainstate to IsCurrentForFeeEstimation (Carl Dong)
d1f932b validation: Pass in coins cache to ::LimitMempoolSize (Carl Dong)

Pull request description:

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

  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:
  glozow:
    reACK bitcoin@e8ae1db via `git range-diff 15f0042...e8ae1db`, only change is fixing ATMP call from conflict
  MarcoFalke:
    ACK e8ae1db 📣

Tree-SHA512: 6af50f04940a69c5c3d3796a24f32f963fa02503cdc1155cc11fff832a99172b407cd163a19793080a5af98580f051b48195b62ec4a797ba2763b4883174153d
Copy link

@alizeyni alizeyni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A

@bitcoin bitcoin deleted a comment from alizeyni Feb 27, 2021
laanwj added a commit that referenced this pull request Mar 4, 2021
…tion functions

e11b649 validation: CVerifyDB::VerifyDB: Use locking annotation (Carl Dong)
03f75c4 validation: Use existing chain member in CChainState::LoadGenesisBlock (Carl Dong)
5e4af77 validation: Use existing chain member in CChainState::AcceptBlock (Carl Dong)
fee7334 validation: Pass in chain to FindBlockPos+SaveBlockToDisk (Carl Dong)
a9d28bc validation: Use *this in CChainState::ActivateBestChainStep (Carl Dong)
4744efc validation: Pass in chainstate to CTxMemPool::check (Carl Dong)
1fb7b2c validation: Use *this in CChainState::InvalidateBlock (Carl Dong)
8cdb2f7 validation: Move LoadBlockIndexDB to CChainState (Carl Dong)
8b99efb validation: Move invalid block handling to CChainState (Carl Dong)
2bdf37f validation: Pass in chainstate to CVerifyDB::VerifyDB (Carl Dong)
31eac50 validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics} (Carl Dong)
63e4c73 validation: Pass in chainstate to ::PruneBlockFilesManual (Carl Dong)
4bada76 validation: Pass in chainstate to UpdateTip (Carl Dong)
a3ba08b validation: Remove global ::{{Precious,Invalidate}Block,ResetBlockFailureFlags} (Carl Dong)
4927c9e validation: Remove global ::LoadGenesisBlock (Carl Dong)
9da106b validation: Check chain tip is non-null in CheckFinalTx (Carl Dong)

Pull request description:

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

  Based on:
  - [x] #20750 | [Bundle 2/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`

  Note to self:
  - [x] Address: #20750 (comment)

ACKs for top commit:
  laanwj:
    Code review ACK e11b649

Tree-SHA512: 205a451a741e32f17d5966de289f2f5a3f0817738c0087b70ff4755ddd217b53d01050ed396669bda2b1d216a88d927b9778777f9ff95ab1fe20e59c5f341776
laanwj added a commit that referenced this pull request Mar 11, 2021
…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
MarcoFalke added a commit that referenced this pull request Apr 17, 2021
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
MarcoFalke added a commit to MarcoFalke/bitcoin-core that referenced this pull request May 24, 2021
aca0e5d Remove `GetDataDir(bool fNetSpecific = true)` function (Kiminuo)
b3e67f2 scripted-diff: Replace `GetDataDir(true)` calls with `gArgs.GetDataDirNet()` calls (Kiminuo)
4c3a5dc scripted-diff: Replace `GetDataDir()` calls with `gArgs.GetDataDirNet()` calls (Kiminuo)
13bd8bb Make `ArgsManager.GetDataDirPath` private and drop needless suffix (Kiminuo)
4d8189f scripted-diff: Change `ArgsManager.GetDataDirPath()` to `ArgsManager.GetDataDirBase()` in tests (Kiminuo)
0f53df4 Add `ArgsManager.GetDataDirBase()` and `ArgsManager.GetDataDirNet()` as an intended replacement for `ArgsManager.GetDataDirPath(net_identifier)` (Kiminuo)
716de29 Make `m_cached_blocks_path` mutable. Make `ArgsManager::GetBlocksDirPath()` const. (Kiminuo)

Pull request description:

  This PR is a follow up PR to bitcoin#21244. The PR attempts to move us an inch towards the [goal](bitcoin#21244 (comment)) by removing `GetDataDir(net_specific)` and replacing it by `gArgs.GetDataDir(net_specific)` calls.

  The approach of this PR attempts to be similar to the one chosen in "De-globalize ChainstateManager" (bitcoin#20158). The goal is to pass `ArgsManager` to functions (or ideally to have `ArgsManager` as a member of a class where needed; inspiration from here: bitcoin#21789) instead of having it as a global variable (i.e. `gArgs`).

  **Notes:**
  * First commit makes `m_cached_blocks_path` `mutable` as was suggested [here](bitcoin#21244 (comment)) but not fully applied in bitcoin#21244. (`m_cached_datadir_path` and `m_cached_network_datadir_path` were marked as `mutable` in bitcoin#21244) This commit can be in a separate PR too.
  * Other commits deal with removing of `GetDataDir(net_specific)` function.
      * This was originally part of bitcoin#21244 but it was [left]((bitcoin#21244 (review))) for a follow up PR.
  * I think that the proposed changes show nicely where there is reliance on `gArgs` which is IMO a good thing.

  If you know about a better approach how to do this, please share it here.

ACKs for top commit:
  hebasto:
    ACK aca0e5d
  MarcoFalke:
    re-ACK aca0e5d 👃

Tree-SHA512: deec4d88edb32d7f4c818c3a74ffbb64709685819b88242dcf5dbaa1fb611f3ce2b29d2576ddb9e0dc5e75288e43538968224008c0a80e7149fc81c309f7c9da
@jamesob jamesob mentioned this pull request May 27, 2021
17 tasks
MarcoFalke added a commit to MarcoFalke/bitcoin-core that referenced this pull request Jun 1, 2021
… modules

7a799c9 index: refactor-only: Reuse CChain ref (Carl Dong)
db33cde index: Add chainstate member to BaseIndex (Carl Dong)
f4a47a1 bench: Use existing chainman in AssembleBlock (Carl Dong)
91226eb bench: Use existing NodeContext in DuplicateInputs (Carl Dong)
e6b4aa6 miner: Pass in chainman to RegenerateCommitments (Carl Dong)
9ecade1 rest: Add GetChainman function and use it (Carl Dong)
fc1c282 rpc/blockchain: Use existing blockman in gettxoutsetinfo (Carl Dong)

Pull request description:

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

  The first 2 commits are fixups addressing review for the last bundle: bitcoin#21391

  NEW note:
  1. I have opened bitcoin#21766 which keeps track of potential improvements where the flaws already existed before the de-globalization work, please post on that issue about these improvements, thanks!

  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:
  jarolrod:
    ACK  7a799c9
  ariard:
    Code Review ACK 7a799c9
  fjahr:
    re-ACK 7a799c9
  MarcoFalke:
    review ACK 7a799c9 🌠
  ryanofsky:
    Code review ACK 7a799c9. Basically no change since last review except fixed rebase conflicts and a new comment about REST Ensure()
  jamesob:
    conditional ACK 7a799c9 ([`jamesob/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai`](https://github.com/jamesob/bitcoin/tree/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai))

Tree-SHA512: 531c00ddcb318817457db2812d9a9d930bc664e58e6f7f1c746350732b031dd624270bfa6b9f49d8056aeb6321d973f0e38e4ff914acd6768edd8602c017d10e
fanquake added a commit that referenced this pull request Jun 12, 2021
6f99488 validation: Farewell, global Chainstate! (Carl Dong)
972c516 qt/test: Reset chainman in ~ChainstateManager instead (Carl Dong)
6c3b5dc scripted-diff: tree-wide: Remove all review-only assertions (Carl Dong)
3e82abb tree-wide: Remove stray review-only assertion (Carl Dong)
f323248 qt/test: Use existing chainman in ::TestGUI (can be scripted-diff) (Carl Dong)
6c15de1 scripted-diff: wallet/test: Use existing chainman (Carl Dong)
ee0ab1e fuzz: Initialize a TestingSetup for test_one_input (Carl Dong)
0d61634 scripted-diff: test: Use existing chainman in unit tests (Carl Dong)
e197076 test: Pass in CoinsTip to ValidateCheckInputsForAllFlags (Carl Dong)
4d99b61 test/miner_tests: Pass in chain tip to CreateBlockIndex (Carl Dong)
f0dd5e6 test/util: Use existing chainman in ::PrepareBlock (Carl Dong)
464c313 init: Use existing chainman (Carl Dong)

Pull request description:

  Based on:  #21767

  à la Mr. Sandman
  ```
  Mr. Chainman, bring me a tip (bung, bung, bung, bung)
  Make it the most work that I've ever seen (bung, bung, bung, bung)
  Rewind old tip till we're at the fork point (bung, bung, bung, bung)
  Then tell it that it's time to call Con-nectTip

  Chainman, I'm so alone (bung, bung, bung, bung)
  No local objects to call my own (bung, bung, bung, bung)
  Please make sure I have a ref
  Mr. Chainman, bring me a tip!
  ```

  This is the last bundle in the #20158 series. Thanks everyone for their diligent review.
  I would like to call attention to #21766, where a few leftover improvements were collated.

  - Remove globals:
    - `ChainstateManager g_chainman`
    - `CChainState& ChainstateActive()`
    - `CChain& ChainActive()`
  - Remove all review-only assertions.

ACKs for top commit:
  jamesob:
    reACK 6f99488 based on the contents of
  ariard:
    Code Review ACK 6f99488.
  jnewbery:
    utACK 6f99488
  achow101:
    Code Review ACK 6f99488
  ryanofsky:
    Code review ACK 6f99488.

Tree-SHA512: 4052ea79360cf0efd81ad0ee3f982e1d93aab1837dcec75f875a56ceda085de078bb3099a2137935d7cc2222004ad88da94b605ef5efef35cb6bc733725debe6
@dongcarl
Copy link
Contributor Author

Thanks everyone for the reviews, seems like the last bundle is merged. I would like to call attention to #21766, where a few leftover improvements were collated.

@dongcarl dongcarl closed this Jun 16, 2021
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 19, 2022
Summary:
Currently, CTxMemPool::check locks CTxMemPool's own cs member, then
calls GetSpendHeight which locks cs_main. This can potentially cause an
undesirable lock invesion since CTxMemPool's cs is supposed to be locked
after cs_main.

This does not cause us any problems right now because all callers of
CTxMemPool already lock cs_main before calling CTxMemPool::check, which
means that the LOCK(cs_main) in GetSpendHeight becomes benign.

However, it is currently possible for new code to be added which calls
CTxMemPool::check without locking cs_main (which would be dangerous).
Therefore we should make it explicit that cs_main needs to be held
before calling CTxMemPool::check.

NOTE: After all review-only assertions are removed in "[[bitcoin/bitcoin#20158 | core#20158]] |
      tree-wide: De-globalize ChainstateManager", and assuming that we
      keep the changes in "validation: Pass in spendheight to
      CTxMemPool::check", we can re-evaluate to see if this annotation
      is still necessary.

This is a backport of [[bitcoin/bitcoin#20972 | core#20972]]

Depends on D10842

Test Plan:
With TSAN:
`ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10844
MarcoFalke added a commit that referenced this pull request Mar 24, 2022
39b1763 Replace use of `ArgsManager` with `DatabaseOptions` (Kiminuo)

Pull request description:

  Contributes to #21005.

  The goal of this PR is to remove `gArgs` from database classes (i.e. `bdb.h` and `sqlite.h`) so that they can be tested without relying on `gArgs` in tests.

  Notes:

  * My goal is to enable unit-testing without relying on `gArgs` as much as possible. Global variables are hard to reason about which in turn makes it slightly harder to contribute to this codebase. When the compiler does the heavy lifting for us and allows us only to construct an object (or call a method) with valid parameters, we may also save some time in code reviews. The cost for this is passing an argument which is not for free but the cost is very miniscule compared to benefits, I think.
      * GUI code is an exception because it seems fine to have `gArgs` there so I don't plan to make changes in `src/qt` folder, for example.
  * My approach to removal of `gArgs` uses is moving from lower levels to upper ones and pass `ArgsManager` as an argument as needed. The approach is very similar to what #20158.

ACKs for top commit:
  achow101:
    ACK 39b1763
  ryanofsky:
    Code review ACK 39b1763. Just the two small ReadDatabaseArgs and Berkeley open changes that were discussed since the last review

Tree-SHA512: aa066b314db593e46c18698fe8cdd500f558b405dc04e4a9a3ff57b52b5b3a81a6cb090e0e661785d1d02c1bf18958c1f4cd715ff233aab63381e3f80960622d
@bitcoin bitcoin locked and limited conversation to collaborators Nov 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet