[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

dongcarl · 2020-12-22T18:25:20Z

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

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

maflcko · 2020-12-22T18:53:33Z

src/init.cpp

@@ -699,7 +699,7 @@ static void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImp
            if (!file)
                break; // This error is logged in OpenBlockFile
            LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
-            LoadExternalBlockFile(chainparams, file, &pos);
+            ::ChainstateActive().LoadExternalBlockFile(chainparams, file, &pos);


I know I've asked the same question already, but I didn't get why this can't use chainman.ActiveChainstate().Load....

This should simplify review because it will touch the same lines of code less often.

Also, it shouldn't make review harder, because everyone knows that there is only one chainman (whether it is called g_chainman or chainman shouldn't matter). On top you added a runtime assertion, which validates this for all executed code paths in our tests:

assert(std::addressof(::ChainstateActive()) == std::addressof(ActiveChainstate()));

Anyway, no big deal if you want to keep it that way. Will review regardless.

DrahtBot · 2020-12-22T20:41:01Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

validation: Guard chainman chainstates with cs_main #21025 (validation: Guard the active_chainstate with cs_main by dongcarl)
net-processing refactoring -- lose globals, move implementation details from .h to .cpp #20758 (net-processing refactoring -- lose globals, move implementation details from .h to .cpp by ajtowns)
[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 by dongcarl)
allow -loadblock blocks to be unsorted #20331 (allow -loadblock blocks to be unsorted by LarryRuane)
tree-wide: De-globalize ChainstateManager #20158 (tree-wide: De-globalize ChainstateManager by dongcarl)
validation: UTXO snapshot activation #19806 (validation: UTXO snapshot activation by jamesob)
refactor: Make mapBlocksUnknownParent local, and rename it #19594 (refactor: Make mapBlocksUnknownParent local, and rename it by hebasto)
Introduce deploymentstatus #19438 (Introduce deploymentstatus by ajtowns)
Improve runtime performance of --reindex #16981 (Improve runtime performance of --reindex by LarryRuane)
test: Set BIP34Height = 2 for regtest #16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)

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.

Sjors

ACK e8dedbb

Regarding 98f2375's commit comment:

However, it seems like the original
intention was for FindForkInGlobalIndex to work with any chain, not
just the current active chain. Let me know if this should be changed.

This was introduced in 2014 by @jtimon in #4796, while refactoring chainActive.FindFork(locator) to findForkInGlobalIndex(chainActive, locator);. Unless @jamesob sees a use for it for snapshot loading, I think dropping the chain parameter should be fine.

src/validation.cpp

src/validation.h

jnewbery · 2020-12-30T14:25:35Z

Concept ACK!

laanwj · 2021-01-08T21:07:30Z

Concept ACK

jnewbery · 2021-01-11T21:08:14Z

I've reviewed everything up to 94bbdd7 validation: Remove global LookupBlockIndex and it looks good so far.

Does it make sense to remove the assert(std::addressof(thing) == std::addressof(thing)); lines as the last commit of this PR? Is there value in keeping those in place until the series of PRs is finished?

ryanofsky

Code review ACK 67c9a83. Changes since last review:

new first commit adding guarded by for active chain state, and a few minimal (recursive?) locks to satisfy the annotation. Hopefully these can be replaced later by annotations and appropriately scoped locking
removing redundant getspendheight lock
include/comment tweaks

It would be good to see suggestion from #20749 (comment) followed up at some point: for ProcessNewBlock or functions like it to avoid calling ActiveChainstate() repeatedly without locking, and instead assign the chainstate they want to use to a variable and use it consistently. (This might be done already in one of the followup PRs)

jnewbery · 2021-02-01T09:34:26Z

If @laanwj reACKs this, then I think it'll be ready for merge

laanwj · 2021-02-01T12:04:15Z

re-ACK 67c9a83

hebasto · 2021-02-01T12:43:34Z

Warnings are fixed in #21051.

…:Lookup… …BlockIndex

b6aadcd build: Add -Werror=mismatched-tags (Hennadii Stepanov) 1485124 Fix -Wmismatched-tags warnings (Hennadii Stepanov) Pull request description: Warnings were introduced in #20749: ``` ./validation.h:43:1: warning: class 'CCheckpointData' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags] class CCheckpointData; ^ ./chainparams.h:24:8: note: previous use is here struct CCheckpointData { ^ ./validation.h:43:1: note: did you mean struct here? class CCheckpointData; ^~~~~ struct 1 warning generated. ``` This change fixes AppVeyor build: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37547435 ACKs for top commit: glozow: utACK b6aadcd 🚗 practicalswift: cr ACK b6aadcd: patch looks correct Tree-SHA512: 3ac887ebdbf9a1ae33c1fd5381b3b8d83388ad557ddeb55013acd42bb9752a5bd009e3a0eed52644a023a7a0dda1c159277981af82f58fb0abfe60b84e01bf29

b6aadcd build: Add -Werror=mismatched-tags (Hennadii Stepanov) 1485124 Fix -Wmismatched-tags warnings (Hennadii Stepanov) Pull request description: Warnings were introduced in bitcoin#20749: ``` ./validation.h:43:1: warning: class 'CCheckpointData' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags] class CCheckpointData; ^ ./chainparams.h:24:8: note: previous use is here struct CCheckpointData { ^ ./validation.h:43:1: note: did you mean struct here? class CCheckpointData; ^~~~~ struct 1 warning generated. ``` This change fixes AppVeyor build: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37547435 ACKs for top commit: glozow: utACK bitcoin@b6aadcd 🚗 practicalswift: cr ACK b6aadcd: patch looks correct Tree-SHA512: 3ac887ebdbf9a1ae33c1fd5381b3b8d83388ad557ddeb55013acd42bb9752a5bd009e3a0eed52644a023a7a0dda1c159277981af82f58fb0abfe60b84e01bf29

20677ff validation: Guard all chainstates with cs_main (Carl Dong) Pull request description: ``` This avoids a potential race-condition where a thread is reading the ChainstateManager::m_active_chainstate pointer while another one is writing to it. There is no portable guarantee that reading/writing the pointer is thread-safe. This is also done in way that mimics ::ChainstateActive(), so the transition from that function to this method is easy. More discussion: 1. #20749 (comment) 2. #19806 (comment) 3. #19806 (comment) 4. #19806 (comment) ``` Basically this PR removes the loaded-but-unfired footgun, which: - Is multiplied (but still unshot) in the chainman deglobalization PRs (#20158) - Is shot in the test framework in the au.activate PR (#19806) ACKs for top commit: jnewbery: code review ACK 20677ff. I've verified by eye that neither of these members are accessed without cs_main. ryanofsky: Code review ACK 20677ff. It is safer to have these new `GUARDED_BY` annotations and locks than not to have them, but in the longer run I think every `LOCK(cs_main)` added here and added earlier in f92dc65 from #20749 should be removed and replaced with `EXCLUSIVE_LOCKS_REQUIRED(cs_main)` on the accessor methods instead. `cs_main` is a high level lock that should be explicitly acquired at a high level to prevent the chain state from changing. It shouldn't be acquired recursively in low-level methods just to read pointer values atomically. Tree-SHA512: 68a3a46d79a407b774eab77e1d682a97e95f1672db0a5fcb877572e188bec09f3a7b47c5d0cc1f2769ea276896dcbe97cb35c861acf7d8e3e513e955dc773f89

See related discussion: bitcoin#20749 (comment)

20677ff validation: Guard all chainstates with cs_main (Carl Dong) Pull request description: ``` This avoids a potential race-condition where a thread is reading the ChainstateManager::m_active_chainstate pointer while another one is writing to it. There is no portable guarantee that reading/writing the pointer is thread-safe. This is also done in way that mimics ::ChainstateActive(), so the transition from that function to this method is easy. More discussion: 1. bitcoin#20749 (comment) 2. bitcoin#19806 (comment) 3. bitcoin#19806 (comment) 4. bitcoin#19806 (comment) ``` Basically this PR removes the loaded-but-unfired footgun, which: - Is multiplied (but still unshot) in the chainman deglobalization PRs (bitcoin#20158) - Is shot in the test framework in the au.activate PR (bitcoin#19806) ACKs for top commit: jnewbery: code review ACK 20677ff. I've verified by eye that neither of these members are accessed without cs_main. ryanofsky: Code review ACK 20677ff. It is safer to have these new `GUARDED_BY` annotations and locks than not to have them, but in the longer run I think every `LOCK(cs_main)` added here and added earlier in f92dc65 from bitcoin#20749 should be removed and replaced with `EXCLUSIVE_LOCKS_REQUIRED(cs_main)` on the accessor methods instead. `cs_main` is a high level lock that should be explicitly acquired at a high level to prevent the chain state from changing. It shouldn't be acquired recursively in low-level methods just to read pointer values atomically. Tree-SHA512: 68a3a46d79a407b774eab77e1d682a97e95f1672db0a5fcb877572e188bec09f3a7b47c5d0cc1f2769ea276896dcbe97cb35c861acf7d8e3e513e955dc773f89

Summary: This avoids a potential race-condition where a thread is reading the `ChainstateManager::m_active_chainstate` pointer while another one is writing to it. There is no portable guarantee that reading/writing the pointer is thread-safe. This is also done in way that mimics `::ChainstateActive()`, so the transition from that function to this method is easy. This is a backport of [[bitcoin/bitcoin#20749 | core#20749]] [1/17] bitcoin/bitcoin@f92dc65 Depends on D11171 Test Plan: With debug: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11172

Summary: LookupBlockIndex only acts on BlockManager. This is a backport of [[bitcoin/bitcoin#20749 | core#20749]] [2, 3 & 4/17] bitcoin/bitcoin@15d20f4 bitcoin/bitcoin@eae54e6 bitcoin/bitcoin@3664a15 Depends on D11172 Test Plan: With debug: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11173

Summary: [META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. `FindForkInGlobalIndex` only acts on BlockManager. Note to reviewers: Since` FindForkInGlobalIndex` is always called with `::ChainActive()` as its first parameter, it is possible to move `FindForkInGlobalIndex` to `CChainState` and remove this `const CChain&` parameter to instead use `m_chain`. However, it seems like the original intention was for `FindForkInGlobalIndex` to work with _any_ chain, not just the current active chain. Let me know if this should be changed. This is a backport of [[bitcoin/bitcoin#20749 | core#20749]] [5/17] bitcoin/bitcoin@b026e31 Depends on D11173 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11174

Summary: [META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. GetSpendHeight only acts on BlockManager. This is a backport of [[bitcoin/bitcoin#20749 | core#20749]] [6/17] bitcoin/bitcoin@e4b95ee Depends on D11174 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11175

Summary: [META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. GetLastCheckPoint mainly acts on BlockManager. This is a partial backport of [[bitcoin/bitcoin#20749 | core#20749]] [7/17] and [[bitcoin/bitcoin#15655 | core#15655]] bitcoin/bitcoin@f11d116 bitcoin/bitcoin@418d323 Depends on D11175 Notes: - moving code from `checkpoints.cpp` to `validation.cpp` (which is huge already) may not seem great from a code quality perspective, but it will eventually be moved to it separate `blockmanager.cpp` file anyway. - we cannot remove `checkpoint.{h,cpp}` because of D1308 which resurrected `Checkpoints::CheckBlock` Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11176

Summary: [META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. This is a backport of [[bitcoin/bitcoin#20749 | core#20749]] [8/17] bitcoin/bitcoin@d363d06 Depends on D11176 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11177

Summary: This is a backport of [[bitcoin/bitcoin#20749 | core#20749]] [9/17] bitcoin/bitcoin@0e17c83 Depends on D11177 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11180

Summary: [META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. This is a backport of [[bitcoin/bitcoin#20749 | core#20749]] [10/17] bitcoin/bitcoin@9c300cc Depends on D11180 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11182

Summary: [META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. This is a backport of [[bitcoin/bitcoin#20749 | core#20749]] [11/17] bitcoin/bitcoin@2a69647 Depends on D11182 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11183

Summary: Instead use CChainState::ActivateBestChain, which is what the global one calls anyway. This is a backport of [[bitcoin/bitcoin#20749 | core#20749]] [12/17] bitcoin/bitcoin@5f8cd7b Depends on D11183 Note that the change in default argument from a null `CBlock` shared pointer to a `nullptr` does not change behavior, as both cases lead to the same code path of `ActivateBestChainStep()` being called with a null `Cblock` pointer. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11184

Summary: [META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. LoadExternalBlockFile mainly acts on CChainState. This is a backport of [[bitcoin/bitcoin#20749 | [[bitcoin/bitcoin#20749 | core#20749]]]] [13/17] bitcoin/bitcoin@e0dc305 Depends on D11184 Test Plan: `ninja all check-all bitcoin-fuzzers` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11185

…lockHeaders Summary: [META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. This is a backport of [[bitcoin/bitcoin#20749 | core#20749]] [14 & 15/17] bitcoin/bitcoin@ea4fed9 bitcoin/bitcoin@0cdad75 Depends on D11185 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11186

Summary: This concludes backport of [[bitcoin/bitcoin#20749 | core#20749]] [16/16] bitcoin/bitcoin@67c9a83 Test Plan: NA. Trivial documentation change. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11187

DrahtBot added Mempool Mining P2P RPC/REST/ZMQ UTXO Db and Indexes Validation labels Dec 22, 2020

maflcko reviewed Dec 22, 2020

View reviewed changes

maflcko added Refactoring and removed Mempool Mining P2P RPC/REST/ZMQ UTXO Db and Indexes labels Dec 22, 2020

dongcarl mentioned this pull request Dec 22, 2020

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

Merged

Sjors approved these changes Dec 30, 2020

View reviewed changes

src/validation.cpp Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

This was referenced Jan 3, 2021

rpc/validation: enable packages through testmempoolaccept #20833

Merged

net-processing refactoring -- lose globals, move implementation details from .h to .cpp #20758

Closed

locks and docs in ATMP and CheckInputsFromMempoolAndCache #20834

Merged

ryanofsky approved these changes Jan 29, 2021

View reviewed changes

This was referenced Jan 29, 2021

validation: Guard chainman chainstates with cs_main #21025

Merged

tree-wide: De-globalize ChainstateManager #20158

Closed

laanwj merged commit 44f4bcd into bitcoin:master Feb 1, 2021

hebasto mentioned this pull request Feb 1, 2021

Fix -Wmismatched-tags warnings #21051

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 2, 2021

Merge bitcoin#20749: [Bundle 1/n] Prune g_chainman usage related to :…

60a5781

…:Lookup… …BlockIndex

jamesob added a commit to jamesob/bitcoin that referenced this pull request Feb 4, 2021

validation: cover the active chainstate with lock annotations

74c8167

See related discussion: bitcoin#20749 (comment)

ryanofsky mentioned this pull request Apr 13, 2021

ChainstateManager locking improvements #21620

Closed

kanoi mentioned this pull request Jun 11, 2021

Optional late, block submission, difficulty test #22119

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

Uh oh!

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

Uh oh!

dongcarl commented Dec 22, 2020

Uh oh!

maflcko Dec 22, 2020 •

edited

Loading

Uh oh!

maflcko Dec 22, 2020

Uh oh!

DrahtBot commented Dec 22, 2020 •

edited

Loading

Uh oh!

Sjors left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Dec 30, 2020

Uh oh!

laanwj commented Jan 8, 2021

Uh oh!

jnewbery commented Jan 11, 2021

Uh oh!

ryanofsky left a comment

Uh oh!

jnewbery commented Feb 1, 2021

Uh oh!

laanwj commented Feb 1, 2021

Uh oh!

hebasto commented Feb 1, 2021

Uh oh!

Uh oh!

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749

Uh oh!

Conversation

dongcarl commented Dec 22, 2020

Uh oh!

maflcko Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Dec 30, 2020

Uh oh!

laanwj commented Jan 8, 2021

Uh oh!

jnewbery commented Jan 11, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Feb 1, 2021

Uh oh!

laanwj commented Feb 1, 2021

Uh oh!

hebasto commented Feb 1, 2021

Uh oh!

Uh oh!

maflcko Dec 22, 2020 •

edited

Loading

DrahtBot commented Dec 22, 2020 •

edited

Loading