Remove `GetDataDir(net_specific)` function #21850

kiminuo · 2021-05-04T12:06:26Z

This PR is a follow up PR to #21244. The PR attempts to move us an inch towards the goal 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" (#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: #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 but not fully applied in Move GetDataDir to ArgsManager #21244. (m_cached_datadir_path and m_cached_network_datadir_path were marked as mutable in Move GetDataDir to ArgsManager #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 Move GetDataDir to ArgsManager #21244 but it was left 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.

DrahtBot · 2021-05-05T00:20:34Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

banman: save the banlist in a JSON format on disk #20966 (banman: save the banlist in a JSON format on disk by vasild)
rpc: allow dumptxoutset to dump human-readable data #18689 (rpc: allow dumptxoutset to dump human-readable data by pierreN)

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.

laanwj · 2021-05-10T12:51:03Z

Code review and concept ACK 2ca79de
Getting the data directory seems common enough (as well as shared between all commands) to have on gArgs directly.
Agree that ideally, the arguments would be passed in some way instead of part of a global, but this is at least a step forward toward that.

ryanofsky

Code review ACK 2ca79de.

Just my opinion, but I do think since this is switching code to a new ArgsManager interface, it would be better for the interface to have explicit Base and Net methods instead of using the previous bool net_specific=true default argument. E.g. something like:

public:
    const fs::path& GetDataDirBasePath() const { return GetDataDirPath(false); }
    const fs::path& GetDataDirNetPath() const { return GetDataDirPath(true); }
private:
    const fs::path& GetDataDirPath(bool net_specific) const;

GetDataDirNetPath() would seem more readable than GetDataDirPath(true) at call sites, and harder to confuse when writing code. And since call sites are being updated in this PR anyway, implementing this would just require tweaking the scripted diffs.

kiminuo · 2021-05-10T14:43:15Z

Just my opinion, but I do think since this is switching code to a new ArgsManager interface, it would be better for the interface to have explicit Base and Net methods instead of using the previous bool net_specific=true default argument. E.g. something like:

I find this to be a very good suggestion. If this PR needs to be touched, I would do it in this PR. Otherwise, I would reserve that for a follow-up PR.

maflcko · 2021-05-10T14:57:49Z

Is there a reason to suffix the return type to the function name? Wouldn't GetDataDirBase and GetDataDirNet be sufficient?

laanwj · 2021-05-17T12:28:39Z

GetDataDirNetPath() would seem more readable than GetDataDirPath(true) at call sites,

Agree. Boolean arguments tend to be bad for readability.

kiminuo · 2021-05-21T09:41:22Z

Code review ACK 2ca79de.

Just my opinion, but I do think since this is switching code to a new ArgsManager interface, it would be better for the interface to have explicit Base and Net methods instead of using the previous bool net_specific=true default argument. E.g. something like:
public:
    const fs::path& GetDataDirBasePath() const { return GetDataDirPath(false); }
    const fs::path& GetDataDirNetPath() const { return GetDataDirPath(true); }
private:
    const fs::path& GetDataDirPath(bool net_specific) const;
GetDataDirNetPath() would seem more readable than GetDataDirPath(true) at call sites, and harder to confuse when writing code. And since call sites are being updated in this PR anyway, implementing this would just require tweaking the scripted diffs.

I have attempted to do the change you propose. Please let me know if you find it better.

(I'm sorry about the needless rebase. I got confused by a test result.)

ryanofsky

Code review ACK c8616c5

src/test/util/setup_common.cpp

src/util/system.h

…ath()` const.

maflcko

review ACK 849ea30 🍇

Show signature and timestamp

Signature:

Timestamp of file with hash 73e12c06b9e21bd03f9dc8f12311837a91583686ee46b861540ec6de544476d4 -

src/util/system.h

src/util/system.cpp

src/util/system.h

hebasto

Approach ACK 457515a.

src/util/system.h

maflcko · 2021-05-24T06:18:31Z

ACK 457515a 🚊

Show signature and timestamp

Signature:

Timestamp of file with hash cbcc6143812dabddaf1ceda66348759c62c6dfc96fa21f1fab33a29027c960f4 -

…as an intended replacement for `ArgsManager.GetDataDirPath(net_identifier)`

…GetDataDirBase()` in tests -BEGIN VERIFY SCRIPT- git ls-files src/test/*_tests.cpp src/test/util/setup_common.cpp | xargs sed -i 's/.GetDataDirPath()/.GetDataDirBase()/g'; -END VERIFY SCRIPT-

…()` calls -BEGIN VERIFY SCRIPT- git ls-files -- 'src' ':(exclude)src/util/system.h' ':(exclude)src/util/system.cpp' | xargs sed -i 's/GetDataDir()/gArgs.GetDataDirNet()/g'; -END VERIFY SCRIPT-

…rNet()` calls -BEGIN VERIFY SCRIPT- git ls-files -- 'src' ':(exclude)src/util/system.h' ':(exclude)src/util/system.cpp' | xargs sed -i 's/GetDataDir(true)/gArgs.GetDataDirNet()/g'; -END VERIFY SCRIPT-

hebasto

ACK aca0e5d

maflcko

re-ACK aca0e5d 👃

Show signature and timestamp

Signature:

Timestamp of file with hash f424bad1e3eed91a429f67534ae92bee38c1b1ae4c61eae9451b352c20c8a36b -

Copied GetMainchainAuthCookieFile logic from GetAuthCookieFile, since the former no longer compiled.

DrahtBot added GUI P2P RPC/REST/ZMQ TX fees and policy Utils/log/libs UTXO Db and Indexes Validation Wallet labels May 4, 2021

laanwj removed GUI P2P RPC/REST/ZMQ TX fees and policy UTXO Db and Indexes Validation Wallet labels May 4, 2021

This was referenced May 5, 2021

banman: save the banlist in a JSON format on disk #20966

Merged

rpc: allow dumptxoutset to dump human-readable data #18689

Closed

ryanofsky approved these changes May 10, 2021

View reviewed changes

maflcko added the Refactoring label May 10, 2021

kiminuo force-pushed the feature/2021-05-get-data-dir-step-2 branch 4 times, most recently from e75c34e to c8616c5 Compare May 21, 2021 08:28

ryanofsky approved these changes May 21, 2021

View reviewed changes

src/test/util/setup_common.cpp Show resolved Hide resolved

maflcko suggested changes May 22, 2021

View reviewed changes

src/util/system.h Show resolved Hide resolved

kiminuo force-pushed the feature/2021-05-get-data-dir-step-2 branch from c8616c5 to 17c4128 Compare May 22, 2021 13:20

Make m_cached_blocks_path mutable. Make `ArgsManager::GetBlocksDirP…

716de29

…ath()` const.

kiminuo force-pushed the feature/2021-05-get-data-dir-step-2 branch 2 times, most recently from a1831d5 to dfb94ae Compare May 22, 2021 13:58

maflcko approved these changes May 22, 2021

View reviewed changes

src/util/system.h Outdated Show resolved Hide resolved

src/util/system.cpp Show resolved Hide resolved

src/util/system.h Outdated Show resolved Hide resolved

kiminuo force-pushed the feature/2021-05-get-data-dir-step-2 branch from dfb94ae to 457515a Compare May 23, 2021 07:18

hebasto reviewed May 23, 2021

View reviewed changes

src/util/system.h Outdated Show resolved Hide resolved

kiminuo added 6 commits May 24, 2021 10:29

Add ArgsManager.GetDataDirBase() and ArgsManager.GetDataDirNet() …

0f53df4

…as an intended replacement for `ArgsManager.GetDataDirPath(net_identifier)`

scripted-diff: Change ArgsManager.GetDataDirPath() to `ArgsManager.…

4d8189f

…GetDataDirBase()` in tests -BEGIN VERIFY SCRIPT- git ls-files src/test/*_tests.cpp src/test/util/setup_common.cpp | xargs sed -i 's/.GetDataDirPath()/.GetDataDirBase()/g'; -END VERIFY SCRIPT-

Make ArgsManager.GetDataDirPath private and drop needless suffix

13bd8bb

scripted-diff: Replace GetDataDir() calls with `gArgs.GetDataDirNet…

4c3a5dc

…()` calls -BEGIN VERIFY SCRIPT- git ls-files -- 'src' ':(exclude)src/util/system.h' ':(exclude)src/util/system.cpp' | xargs sed -i 's/GetDataDir()/gArgs.GetDataDirNet()/g'; -END VERIFY SCRIPT-

scripted-diff: Replace GetDataDir(true) calls with `gArgs.GetDataDi…

b3e67f2

…rNet()` calls -BEGIN VERIFY SCRIPT- git ls-files -- 'src' ':(exclude)src/util/system.h' ':(exclude)src/util/system.cpp' | xargs sed -i 's/GetDataDir(true)/gArgs.GetDataDirNet()/g'; -END VERIFY SCRIPT-

Remove GetDataDir(bool fNetSpecific = true) function

aca0e5d

kiminuo force-pushed the feature/2021-05-get-data-dir-step-2 branch from 457515a to aca0e5d Compare May 24, 2021 08:31

hebasto approved these changes May 24, 2021

View reviewed changes

maflcko approved these changes May 24, 2021

View reviewed changes

maflcko merged commit 5990009 into bitcoin:master May 24, 2021

kiminuo deleted the feature/2021-05-get-data-dir-step-2 branch May 24, 2021 09:11

ryanofsky mentioned this pull request May 24, 2021

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 25, 2021

Merge bitcoin#21850: Remove GetDataDir(net_specific) function

d1ca320

This was referenced May 25, 2021

Add fee_est tool for debugging fee estimation code #10443

Closed

interfaces: Expose settings.json methods to GUI #15936

Merged

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

Merge 5990009 into merged_master (Bitcoin PR bitcoin/bitcoin#21850)

29a8292

Copied GetMainchainAuthCookieFile logic from GetAuthCookieFile, since the former no longer compiled.

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

Remove GetDataDir(net_specific) function #21850

Remove GetDataDir(net_specific) function #21850

Uh oh!

Conversation

kiminuo commented May 4, 2021

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

laanwj commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment • 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!

kiminuo commented May 10, 2021

Uh oh!

maflcko commented May 10, 2021

Uh oh!

laanwj commented May 17, 2021

Uh oh!

kiminuo commented May 21, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented May 24, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Remove `GetDataDir(net_specific)` function #21850

Remove `GetDataDir(net_specific)` function #21850

DrahtBot commented May 5, 2021 •

edited

Loading

laanwj commented May 10, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading