refactor: Remove ::Params() global from CChainState by maflcko · Pull Request #21789 · bitcoin/bitcoin

maflcko · 2021-04-27T18:51:20Z

The ::Params() global is verbose and confusing. Also it makes tests a bit harder to write because they'd have to mock a global.

Fix all issues by simply using a member variable that points to the right params.

(Can be reviewed with --word-diff-regex=.)

jamesob · 2021-04-27T19:24:28Z

Concept ACK, will review soon

practicalswift · 2021-04-27T20:42:44Z

Concept ACK

fanquake · 2021-04-28T03:00:03Z

Concept ACK

DrahtBot · 2021-04-28T06:14:29Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) #20487 by practicalswift
allow -loadblock blocks to be unsorted #20331 by LarryRuane
Introduce deploymentstatus #19438 by ajtowns
consensus: Simplify ConnectTrace #18685 by jnewbery
Improve runtime performance of --reindex #16981 by LarryRuane
assumeutxo #15606 by jamesob

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.

kiminuo · 2021-04-28T06:25:05Z

Concept ACK.

It looks like a very nice simplification which makes it harder to make a mistake.

DrahtBot · 2021-05-03T09:31:33Z

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

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

kiminuo · 2021-06-03T07:32:15Z

The main change in the first commit (fa216cc7) seems to be: "refactor: Add CChainState member to CChainState class" rather than "refactor: Remove ::Params() global from CChainState".

Also, the changes like this fa216cc#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R2284 in the first commit (fa216cc7) can probably be moved to the second commit (fa06aaacf3b9c35d9e30c883bcd7780dcf0bceda) so that the first commit would only introduce m_params.

Changes in fa216cc79267ba25fa3709982626def61f4abe2a & fa06aaacf3b9c35d9e30c883bcd7780dcf0bceda LGTM.

jnewbery · 2021-06-09T10:58:43Z

Concept ACK

maflcko · 2021-06-10T09:49:46Z

I think the two commits are nicely split up. The first one removes it from inside the functions, the second removes the arg of the functions.

jnewbery · 2021-06-10T10:24:26Z

ACK 2222da832066d06be3ff8752e49008fef71eb668

…ctions It is confusing and verbose to repeatedly access the global when a member variable can simply refer to it.

Passing this is confusing and redundant with the m_params member.

maflcko · 2021-06-14T07:22:20Z

Rebased

jnewbery · 2021-06-14T08:40:15Z

ACK fa0d921

kiminuo · 2021-06-14T09:40:32Z

utACK fa0d921

kiminuo · 2021-06-14T09:56:25Z

    for (int i = 0; i < 10; ++i) {
        BlockValidationState state;
-        m_node.chainman->ActiveChainstate().InvalidateBlock(state, Params(), active.Tip());
+        m_node.chainman->ActiveChainstate().InvalidateBlock(state, active.Tip());


Is #include <chainparams.h> still needed?

Thanks, will try to remove if I have to force push

theStack

ACK fa0d921 🍉
Nice refactoring idea, glad to see this simplification of the CChainState interface.

maflcko force-pushed the 2104-ChainstateParamsRefactor branch from fa5915e to fad0c2a Compare April 27, 2021 18:51

DrahtBot added P2P Refactoring RPC/REST/ZMQ Validation labels Apr 27, 2021

maflcko removed P2P RPC/REST/ZMQ labels Apr 28, 2021

maflcko force-pushed the 2104-ChainstateParamsRefactor branch from fad0c2a to fa4b981 Compare April 28, 2021 05:08

kiminuo reviewed Apr 28, 2021

View reviewed changes

Comment thread src/validation.h Outdated

maflcko force-pushed the 2104-ChainstateParamsRefactor branch from fae8528 to fa49430 Compare April 28, 2021 07:02

This was referenced Apr 30, 2021

Introduce deploymentstatus #19438

Merged

[docs] Update references to RewindBlockIndex() #21816

Closed

DrahtBot mentioned this pull request May 2, 2021

scripted-diff: Replace three dots with ellipsis in the UI strings #21836

Merged

kiminuo mentioned this pull request May 4, 2021

Remove GetDataDir(net_specific) function #21850

Merged

maflcko force-pushed the 2104-ChainstateParamsRefactor branch from fa49430 to fad6615 Compare May 5, 2021 15:23

DrahtBot mentioned this pull request May 6, 2021

[Bundle 7/7] validation: Farewell, global Chainstate! #21866

Merged

DrahtBot added the Needs rebase label May 10, 2021

maflcko force-pushed the 2104-ChainstateParamsRefactor branch from fad6615 to eeee9c6 Compare May 10, 2021 16:03

This was referenced May 28, 2021

assumeutxo #15606

Closed

refactor: address ProcessNewBlock comments from #21713 #22106

Merged

DrahtBot added the Needs rebase label Jun 2, 2021

maflcko force-pushed the 2104-ChainstateParamsRefactor branch from eeee9c6 to faa72ee Compare June 2, 2021 07:29

DrahtBot removed the Needs rebase label Jun 2, 2021

kiminuo reviewed Jun 2, 2021

View reviewed changes

Comment thread src/chain.h Outdated

maflcko added the Waiting for author label Jun 3, 2021

maflcko removed the Waiting for author label Jun 10, 2021

maflcko force-pushed the 2104-ChainstateParamsRefactor branch from faa72ee to 2222da8 Compare June 10, 2021 09:49

jnewbery reviewed Jun 10, 2021

View reviewed changes

Comment thread src/test/util/setup_common.cpp Outdated

DrahtBot added the Needs rebase label Jun 12, 2021

MarcoFalke added 2 commits June 13, 2021 09:39

refactor: Remove ::Params() global from inside CChainState member fun…

fa38947

…ctions It is confusing and verbose to repeatedly access the global when a member variable can simply refer to it.

refactor: Remove chainparams arg from CChainState member functions

fa0d921

Passing this is confusing and redundant with the m_params member.

maflcko force-pushed the 2104-ChainstateParamsRefactor branch from 2222da8 to fa0d921 Compare June 13, 2021 07:44

DrahtBot removed the Needs rebase label Jun 13, 2021

kiminuo reviewed Jun 14, 2021

View reviewed changes

DrahtBot mentioned this pull request Jun 14, 2021

Move CBlockTreeDB to node/blockstorage #22242

Closed

theStack approved these changes Jun 28, 2021

View reviewed changes

fanquake merged commit 8071ec1 into bitcoin:master Jun 29, 2021

maflcko deleted the 2104-ChainstateParamsRefactor branch June 29, 2021 06:39

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 8071ec1 into merged_master (Bitcoin PR bitcoin/bitcoin#21789)

cac878c

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

Uh oh!

Conversation

maflcko commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob commented Apr 27, 2021

Uh oh!

practicalswift commented Apr 27, 2021

Uh oh!

fanquake commented Apr 28, 2021

Uh oh!

DrahtBot commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

kiminuo commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 3, 2021

Uh oh!

Uh oh!

kiminuo commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Jun 9, 2021

Uh oh!

maflcko commented Jun 10, 2021

Uh oh!

jnewbery commented Jun 10, 2021

Uh oh!

Uh oh!

maflcko commented Jun 14, 2021

Uh oh!

jnewbery commented Jun 14, 2021

Uh oh!

kiminuo commented Jun 14, 2021

Uh oh!

kiminuo Jun 14, 2021 • 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 Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

maflcko commented Apr 27, 2021 •

edited

Loading

DrahtBot commented Apr 28, 2021 •

edited

Loading

kiminuo commented Apr 28, 2021 •

edited

Loading

kiminuo commented Jun 3, 2021 •

edited

Loading

kiminuo Jun 14, 2021 •

edited

Loading