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

dongcarl · 2020-12-22T19:35:51Z

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

DrahtBot · 2020-12-22T20:40:05Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Split orphan handling from net_processing into txorphanage #21148 (Split orphan handling from net_processing into txorphanage by ajtowns)
validation/coins: limit MemPoolAccept access to coins cache + make it responsible for uncaching #21146 (validation/coins: limit MemPoolAccept access to coins cache + make it responsible for uncaching by glozow)
test: Move MakeNoLogFileContext to libtest_util, and use it in bench #21003 (test: Move MakeNoLogFileContext to libtest_util, and use it in bench by MarcoFalke)
rpc/validation: enable packages through testmempoolaccept #20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) #19259 (fuzz: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) by practicalswift)

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-01-13T11:46:12Z

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

jnewbery · 2021-02-01T12:17:08Z

#20749 is merged. Rebase please 🙂

dongcarl · 2021-02-01T16:57:27Z

Pushed 5803d30 -> 8d435f2

Rebased over master since "[Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex #20749 | [Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex" is now merged!

ariard

Approach ACK, did a first parse on each commit.

Side-question, what's your heuristic to divide your bundles ?

src/validation.cpp

src/test/validation_block_tests.cpp

src/txmempool.cpp

Several other parameters are now redundant since they can be safely obtained from the chainstate given that ::cs_main is locked. These are now removed.

dongcarl · 2021-02-18T19:57:01Z

Pushed 15f0042 -> e8ae1db

Rebased to fix silent conflict as mentioned in [Bundle 2/n] Prune g_chainman usage in mempool-related validation functions #20750 (comment)

maflcko

ACK e8ae1db 📣

Show signature and timestamp

Signature:

Timestamp of file with hash 4c3078abec10b46f834a4805961a34857896683a0701dfcccf98cc987f6a924e -

maflcko · 2021-02-19T18:46:06Z

src/validation.cpp

+    return CheckFinalTx(::ChainActive().Tip(), tx, flags);
+}
+
+bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, int flags)


nit in d015eaa: For new code it would be good to use the new style (& is left-attaching). 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. Also, in the comment in this function, you'll need to replace ::ChainActive().Height()+1 as well? Finally, the next two commits could just be squashed into this one?

Right, this makes sense. I'm going to put this nit off until the next bundle so that I don't invalidate ACKs if that's alright with you! I added it as a "Note to self" in the next bundle.

maflcko · 2021-02-19T18:58:38Z

src/txmempool.cpp

@@ -356,7 +356,7 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n)
 void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAncestors, bool validFeeEstimate)
 {
    // Add to memory pool without checking anything.
-    // Used by AcceptToMemoryPool(), which DOES do
+    // Used by AcceptToMemoryPool(::ChainstateActive(), ), which DOES do


nit: maybe exclude this file from the scripted diff? Also nit: git comes with an exclude syntax for files, so you don't have to use grep -v

Yeah the git syntax was confusing so I wanted to just use grep since it's more commonly used 😬. I will make this change if there are other substantial changes that requires a push!

glozow · 2021-02-19T20:56:19Z

reACK e8ae1db via git range-diff 15f0042...e8ae1db, only change is fixing ATMP call from conflict

…elated …

...also update comments to remove mention of ::ChainActive() From: 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

…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

...also update comments to remove mention of ::ChainActive() From: 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

Summary: PR title: Prune g_chainman usage in mempool-related validation functions (tree-wide: De-globalize ChainstateManager) This is a backport of [[bitcoin/bitcoin#20750 | core#20750]] [1/19] bitcoin/bitcoin@d1f932b Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11189

Summary: This is a backport of [[bitcoin/bitcoin#20750 | core#20750]] [2/19] bitcoin/bitcoin@252b489#i Depends on D11189 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11190

…ntBlock Summary: This is a backport of [[bitcoin/bitcoin#20750 | core#20750]] [3, 4 & 5/19] bitcoin/bitcoin@d015eaa bitcoin/bitcoin@7031cf8 bitcoin/bitcoin@577b774 Depends on D11190 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11191

Summary: This is a backport of [[bitcoin/bitcoin#20750 | core#20750]] [6/17] bitcoin/bitcoin@4c15942 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11192

Summary: This is a backport of [[bitcoin/bitcoin#20750 | core#20750]] [7/17] bitcoin/bitcoin@d8a8163 Depends on D11192 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11193

Summary: This is a backport of [[bitcoin/bitcoin#20750 | core#20750]] [8/17] bitcoin/bitcoin@3a205c4 Depends on D11193 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11194

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

Summary: This is a backport of [[bitcoin/bitcoin#20750 | core#20750]] [10, 11, 12 & 13/17] bitcoin/bitcoin@229bc37 bitcoin/bitcoin@3704433 bitcoin/bitcoin@417dafc bitcoin/bitcoin@120aaba Depends on D11195 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11196

Summary: This is a backport of [[bitcoin/bitcoin#20750 | core#20750]] [14/17] bitcoin/bitcoin@71734c6 Depends on D11196 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11201

Summary: Several other parameters are now redundant since they can be safely obtained from the chainstate given that ::cs_main is locked. These are now removed. This is a backport of [[bitcoin/bitcoin#20750 | core#20750]] [15/17] bitcoin/bitcoin@7142018 Depends on D11201 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11202

Summary: This is a backport of [[bitcoin/bitcoin#20750 | core#20750]] [16/17] bitcoin/bitcoin@0a9a24d Depends on D11202 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11203

Summary: This concludes backport of [[bitcoin/bitcoin#20750 | core#20750]] [17/17] bitcoin/bitcoin@8c82481 Depends on D11203 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11204

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

dongcarl added the Refactoring label Dec 22, 2020

This was referenced Jan 3, 2021

locks and docs in ATMP and CheckInputsFromMempoolAndCache #20834

Merged

rpc/validation: enable packages through testmempoolaccept #20833

Merged

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

Closed

DrahtBot added the Needs rebase label Jan 7, 2021

dongcarl force-pushed the 2020-09-reduce-validation-mempool-ccsactiveglobal-usage branch from f4bb7c5 to 5803d30 Compare January 27, 2021 00:07

DrahtBot mentioned this pull request Jan 27, 2021

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

Merged

DrahtBot removed the Needs rebase label Jan 27, 2021

DrahtBot mentioned this pull request Jan 29, 2021

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

Merged

jnewbery mentioned this pull request Feb 1, 2021

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

DrahtBot added the Needs rebase label Feb 1, 2021

dongcarl force-pushed the 2020-09-reduce-validation-mempool-ccsactiveglobal-usage branch from 5803d30 to 8d435f2 Compare February 1, 2021 16:55

dongcarl mentioned this pull request Feb 1, 2021

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

Merged

2 tasks

DrahtBot removed the Needs rebase label Feb 1, 2021

ariard reviewed Feb 1, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/test/validation_block_tests.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

dongcarl added 5 commits February 18, 2021 14:49

validation: Pass in chainstate to CTxMemPool::removeForReorg

7142018

Several other parameters are now redundant since they can be safely obtained from the chainstate given that ::cs_main is locked. These are now removed.

validation: Pass in chainstate to UpdateMempoolForReorg

0a9a24d

validation: Use *this in CChainState::LoadMempool

8c82481

style-only: Make CheckSequenceLock signature readable

8f5c100

style-only: Make AcceptToMemoryPool signature readable

e8ae1db

dongcarl force-pushed the 2020-09-reduce-validation-mempool-ccsactiveglobal-usage branch from 15f0042 to e8ae1db Compare February 18, 2021 19:55

maflcko reviewed Feb 19, 2021

View reviewed changes

maflcko merged commit 828bb77 into bitcoin:master Feb 20, 2021

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

Merge bitcoin#20750: [Bundle 2/n] Prune g_chainman usage in mempool-r…

3bdd88c

…elated …

klementtan mentioned this pull request Jun 19, 2021

refactor: CheckFinalTx pass by reference instead of pointer #22282

Closed

fanquake mentioned this pull request Aug 5, 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 2/n] Prune g_chainman usage in mempool-related validation functions #20750

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

dongcarl commented Dec 22, 2020 •

edited

DrahtBot commented Dec 22, 2020 •

edited

DrahtBot commented Jan 13, 2021

jnewbery commented Feb 1, 2021

dongcarl commented Feb 1, 2021

ariard left a comment

dongcarl commented Feb 18, 2021

maflcko left a comment

maflcko Feb 19, 2021

dongcarl Feb 19, 2021 •

edited

maflcko Feb 19, 2021

dongcarl Feb 19, 2021 •

edited

glozow commented Feb 19, 2021

[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 #20750

Conversation

dongcarl commented Dec 22, 2020 • edited

DrahtBot commented Dec 22, 2020 • edited

Conflicts

DrahtBot commented Jan 13, 2021

jnewbery commented Feb 1, 2021

dongcarl commented Feb 1, 2021

ariard left a comment

Choose a reason for hiding this comment

dongcarl commented Feb 18, 2021

maflcko left a comment

Choose a reason for hiding this comment

maflcko Feb 19, 2021

Choose a reason for hiding this comment

dongcarl Feb 19, 2021 • edited

Choose a reason for hiding this comment

maflcko Feb 19, 2021

Choose a reason for hiding this comment

dongcarl Feb 19, 2021 • edited

Choose a reason for hiding this comment

glozow commented Feb 19, 2021

dongcarl commented Dec 22, 2020 •

edited

DrahtBot commented Dec 22, 2020 •

edited

dongcarl Feb 19, 2021 •

edited

dongcarl Feb 19, 2021 •

edited