Remove `GetDataDir(net_specific)` function

kiminuo commented at 12:06 pm on May 4, 2021: contributor

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 #21244. (m_cached_datadir_path and m_cached_network_datadir_path were marked as mutable in #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 #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 added the label GUI on May 4, 2021

DrahtBot added the label P2P on May 4, 2021

DrahtBot added the label RPC/REST/ZMQ on May 4, 2021

DrahtBot added the label TX fees and policy on May 4, 2021

DrahtBot added the label Utils/log/libs on May 4, 2021

DrahtBot added the label UTXO Db and Indexes on May 4, 2021

DrahtBot added the label Validation on May 4, 2021

DrahtBot added the label Wallet on May 4, 2021

laanwj removed the label GUI on May 4, 2021

laanwj removed the label P2P on May 4, 2021

laanwj removed the label RPC/REST/ZMQ on May 4, 2021

laanwj removed the label TX fees and policy on May 4, 2021

laanwj removed the label UTXO Db and Indexes on May 4, 2021

laanwj removed the label Validation on May 4, 2021

laanwj removed the label Wallet on May 4, 2021

DrahtBot commented at 0:20 am on May 5, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20966 (banman: save the banlist in a JSON format on disk by vasild)
#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 commented at 12:51 pm on May 10, 2021: member

Code review and concept ACK 2ca79de5a92be6f5179c47a4e28bcb5109a3670b 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 approved

ryanofsky commented at 1:42 pm on May 10, 2021: member

Code review ACK 2ca79de5a92be6f5179c47a4e28bcb5109a3670b.

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:

0public:
1    const fs::path& GetDataDirBasePath() const { return GetDataDirPath(false); }
2    const fs::path& GetDataDirNetPath() const { return GetDataDirPath(true); }
3private:
4    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 commented at 2:43 pm on May 10, 2021: contributor

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.

MarcoFalke added the label Refactoring on May 10, 2021

MarcoFalke commented at 2:57 pm on May 10, 2021: member

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

laanwj commented at 12:28 pm on May 17, 2021: member

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

Agree. Boolean arguments tend to be bad for readability.

kiminuo force-pushed on May 20, 2021

kiminuo force-pushed on May 21, 2021

kiminuo commented at 9:41 am on May 21, 2021: contributor

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:
0public:
1    const fs::path& GetDataDirBasePath() const { return GetDataDirPath(false); }
2    const fs::path& GetDataDirNetPath() const { return GetDataDirPath(true); }
3private:
4    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.)

in src/test/util/setup_common.cpp:198 in 80dec17172 outdated

194@@ -195,7 +195,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
195     }
196 
197     m_node.addrman = std::make_unique<CAddrMan>();
198-    m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirPath() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
199+    m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);

ryanofsky commented at 5:36 pm on May 21, 2021:

In commit “Use ArgsManager.GetDataDirBasePath() and ArgsManager.GetDataDirNetPath() instead of ArgsManager.GetDataDirPath(net_identifier)” (80dec1717258fd74ae32274affed71daf2949e46)

IMO it would be a little better to keep the GetDataDirPath method and argumentless GetDataDirPath() calls unchanged in this commit, so the commit is smaller. Then the subsequent scripted-diff commit could do mechanical work of replacing m_args.GetDataDirPath() with m_args.GetDataDirNet() and GetDataDirPath with GetDataDir.

Also, it doesn’t matter since it only affects tests, but replacing GetDataDirPath() with GetDataDirNet() instead of GetDataDirBase() would be a more obvious choice to keep behavior unchanged.

This is all fine, though. Just extra suggestions in case there’s a reason to update.

kiminuo commented at 1:22 pm on May 22, 2021:

I have moved the tests changes to a separate commit (https://github.com/bitcoin/bitcoin/pull/21850/commits/7f5774c09affebc8837a4d117aae198d6fd6157a).

The PR differs in one trailing newline as an effect. So there is no code change, only git commit changes.

edit: Hm, merge conflict apparently, will sort out later. edit: Hopefully, done.

ryanofsky approved

ryanofsky commented at 5:47 pm on May 21, 2021: member

Code review ACK c8616c5c291c3c180460cf01ef3fcacd9194a2e4

in src/util/system.h:276 in 80dec17172 outdated

270@@ -271,11 +271,18 @@ class ArgsManager
271     /**
272      * Get data directory path
273      *
274-     * @param net_specific Append network identifier to the returned path
275      * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned
276      * @post Returned directory path is created unless it is empty
277      */
278-    const fs::path& GetDataDirPath(bool net_specific = true) const;
279+    const fs::path& GetDataDirBase() const { return GetDataDir(false); }

MarcoFalke commented at 9:39 am on May 22, 2021:

80dec1717258fd74ae32274affed71daf2949e46: The goal of this pull is to remove the GetDataDir function, but here you introduce more call sites?

Also, it could make sense to have this in a separate commit and move the other mechanical changes to a scripted diff

kiminuo commented at 12:31 pm on May 22, 2021:

80dec17: The goal of this pull is to remove the GetDataDir function, but here you introduce more call sites?

GetDataDir(false) actually calls const fs::path& ArgsManager::GetDataDir(bool net_specific) const (i.e. https://github.com/bitcoin/bitcoin/commit/80dec1717258fd74ae32274affed71daf2949e46#diff-a5795f7946088222aa59a662e15f5b5aee627bfe2311459e789d2a46783e37cbR455). But I can see that it is actually very confusing.

I’ll try to go with #21850 (review) now and hopefully it will be nicer.

MarcoFalke changes_requested

kiminuo force-pushed on May 22, 2021

Make `m_cached_blocks_path` mutable. Make `ArgsManager::GetBlocksDirPath()` const. 716de29dd8

kiminuo force-pushed on May 22, 2021

in src/util/system.h:286 in d5f1458ea3 outdated

281+     *
282+     * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned
283+     * @post Returned directory path is created unless it is empty
284+     */
285+    const fs::path& GetDataDirNet() const { return GetDataDirPath(true); }
286+

MarcoFalke commented at 4:06 pm on May 22, 2021:

d5f1458ea3821cdf87954f012bd1606659cf818f: This is adding trailing whitespace. Also the commit title is wrong. Path should be removed.

in src/util/system.cpp:1359 in 162b12cbc1 outdated

1360@@ -1361,7 +1361,7 @@ fs::path AbsPathForConfigVal(const fs::path& path, bool net_specific)
1361     if (path.is_absolute()) {
1362         return path;
1363     }
1364-    return fsbridge::AbsPathJoin(GetDataDir(net_specific), path);
1365+    return fsbridge::AbsPathJoin(net_specific ? gArgs.GetDataDirNet() : gArgs.GetDataDirBase(), path);

MarcoFalke commented at 4:18 pm on May 22, 2021:

162b12cbc1051dbb8a25f271d0def1985f7cd73e: This can be squashed into d5f1458ea3821cdf87954f012bd1606659cf818f?

kiminuo commented at 6:40 am on May 23, 2021:

Yes, it should. I can do it if another touch is needed.

MarcoFalke commented at 6:47 am on May 23, 2021:

My ACK is the only one. I am happy to re-ACK if you decide to push.

in src/util/system.h:447 in 849ea30efe outdated

443@@ -453,6 +444,16 @@ class ArgsManager
444     void LogArgs() const;
445 
446 private:
447+

MarcoFalke commented at 4:21 pm on May 22, 2021:

849ea30efe96b29ec3023d10b077ffb4f7fa0f86: According to clang-format this newline should be removed

kiminuo commented at 8:42 pm on May 22, 2021:

Would it make sense to add a clang-format linter to Cirrus or maybe to lint-all.sh to enforce proper code style?

MarcoFalke commented at 6:03 am on May 23, 2021:

There are cases where clang-format is “wrong”, so we can’t enforce this. Maybe suggestions could make sense, but they might be noisy.

MarcoFalke approved

MarcoFalke commented at 4:21 pm on May 22, 2021: member

review ACK 849ea30efe96b29ec3023d10b077ffb4f7fa0f86 🍇

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 849ea30efe96b29ec3023d10b077ffb4f7fa0f86 🍇
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgybwv/bU7GLhuy9uVt7h3E2faUAPQ2zkOGL9yViqsbXcZyqOUd5XYp8NQFt0Od
 8ADu7KAStRDEkqMCf0XMI9+Sx1loYVnzXACPvbf8b41+5yAerLOnmig08tDAIoLMc
 9YMuHK5rHjzrcLiwtrANmsW/jUR7l1pQY7QC5K0wWzSUX3XC69XZ0ZLHnJf2xJv+W
10lr+xSFzbDJKcDE0PJQ3veswVGTHIkRw70QdkVj1Us3cX8i7cWADDNCsev9+DolBk
11sojoZWDE58Gh/RKB5+dKyi1NpmsMjMJ4KN3l0Cgq6MU1Z8Q1ZiJ1UjwmQNIiGKqC
12NNe352Op3nqbhIvXmciUgpTQcYveHmi8OAOv4hYwX2JMCmzM3PdPQ+TDgHuAY8El
13PKH6mYTXfdqHow9x9QOpno67/Ei3ubzqmWsFnNTmG825lZEmb5YhviMbUvaEXWSy
146oum6G5mof5Mou4YI72d3cJbrMe0YCgA1P+SzK/jOEx2Hsp6rDw2+LIreBhtzffI
15hmJY6Ucg
16=7cLP
17-----END PGP SIGNATURE-----

Timestamp of file with hash 73e12c06b9e21bd03f9dc8f12311837a91583686ee46b861540ec6de544476d4 -

kiminuo force-pushed on May 23, 2021

in src/util/system.h:121 in 457515aab1 outdated

117@@ -119,7 +118,7 @@ UniValue RunCommandParseJSON(const std::string& str_command, const std::string&
118  * the datadir if they are not absolute.
119  *
120  * @param path The path to be conditionally prefixed with datadir.
121- * @param net_specific Forwarded to GetDataDir().
122+ * @param net_specific Forwarded to gArgs.GetDataDirNet().

hebasto commented at 10:36 am on May 23, 2021:

This comment is not true now. Also suggesting to move this comment update into the “Add ArgsManager.GetDataDirBase() and ArgsManager.GetDataDirNet() as an intended replacement for ArgsManager.GetDataDirPath(net_identifier)” commit (babd5ed0204b01793c8c96d079d5304efdcc36e5).

kiminuo commented at 6:06 am on May 24, 2021:

Right. I’m not sure about wording though.

Maybe any of the following variants:

* [@param](/bitcoin-bitcoin/contributor/param/) net_specific Whether to append network identifier to base datadir first or
* [@param](/bitcoin-bitcoin/contributor/param/) net_specific Use network specific datadir variant or
* [@param](/bitcoin-bitcoin/contributor/param/) net_specific Forwarded either to ArgsManager.GetDataDirNet() or ArgsManager.GetDataDirBase()

MarcoFalke commented at 6:20 am on May 24, 2021:

The AbsPathForConfigVal function should probably be made a method on ArgsManager anyway. The comment can be fixed up in the follow-up.

Happy to re-ACK if you push, though.

hebasto commented at 6:23 am on May 24, 2021:

The second one lgtm.

kiminuo commented at 8:32 am on May 24, 2021:

The AbsPathForConfigVal function should probably be made a method on ArgsManager anyway. The comment can be fixed up in the follow-up.

Yes, it seems so.

The second one lgtm.

Done. Thanks.

hebasto commented at 10:37 am on May 23, 2021: member

Approach ACK 457515aab1042d842ee2ac6752ebf577ae6dee2b.

MarcoFalke commented at 6:18 am on May 24, 2021: member

ACK 457515aab1042d842ee2ac6752ebf577ae6dee2b 🚊

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 457515aab1042d842ee2ac6752ebf577ae6dee2b 🚊
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUibDgwAqnNrcU52+PTrpofeW2/FPyAMU3psifPGg3tyNYmI379eF9mbddfGk+Nv
 8heuP8JcCDj3JY1MPjqGsj6X5Bhof4zldeGHEpP/Zx+eW+IazU39RsdlCEv21X32n
 9+bCL7cnV4+6PJENrtKpJqxyET1Y6zguEjU+0O1kUeZdvYeLPP6K4SGgXKoYDxZl6
10J3coLN7s0839Pn+qCNXFs/GEAjZR4j/RXXfbIUxx/L1bdBK5y/gAau5br/FMY+WS
11ld465ZH1kOFdg80AyCxV4dY1B2QuIUw+zwE+8q7U7JVATAGOqQGkOBt/DKkd8+6U
122u7bbwkHieOba4XNfMXKvoApUmcD0pJTroSYo2P8Z9kSwVm7YCxGxBxP5Z2gC2qa
13yKXQJkv07LUgMpXCIrfjky+hx4AVclLzlMYi800/8EAfrEDFxxPidUUEjOUFp8e2
14cyNG8fkTd32Be41h0MDNidKQ+uLLv5dDin4cPDvHfNO7Q7qYuf/d7hSM3wnGeVwo
15oVWPT/P3
16=joGU
17-----END PGP SIGNATURE-----

Timestamp of file with hash cbcc6143812dabddaf1ceda66348759c62c6dfc96fa21f1fab33a29027c960f4 -

Add `ArgsManager.GetDataDirBase()` and `ArgsManager.GetDataDirNet()` as an intended replacement for `ArgsManager.GetDataDirPath(net_identifier)` 0f53df47d5

scripted-diff: Change `ArgsManager.GetDataDirPath()` to `ArgsManager.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-

4d8189f620

Make `ArgsManager.GetDataDirPath` private and drop needless suffix 13bd8bb053

scripted-diff: Replace `GetDataDir()` calls with `gArgs.GetDataDirNet()` 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-

4c3a5dcbfc

scripted-diff: Replace `GetDataDir(true)` calls with `gArgs.GetDataDirNet()` 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-

b3e67f20a0

Remove `GetDataDir(bool fNetSpecific = true)` function aca0e5dcdb

kiminuo force-pushed on May 24, 2021

hebasto approved

hebasto commented at 8:43 am on May 24, 2021: member

ACK aca0e5dcdb174ef7e88b76910d6fffd633688235

MarcoFalke approved

MarcoFalke commented at 9:05 am on May 24, 2021: member

re-ACK aca0e5dcdb174ef7e88b76910d6fffd633688235 👃

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK aca0e5dcdb174ef7e88b76910d6fffd633688235 👃
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiN6Qv/RgP9uS/QKuYzESznN0Tb9TySTikTypK4lmJTJloWTVxqVMWSRaeey4wP
 8lgnhjfPjicCgooYTHZWb9T1qrQyFrjlRuyBkDANMY0zLvuLikxfTVEc+txoxvIib
 9cqYHxrf8sfKrv50JJZNpspRNBxgNrgl/0vdDNHisX3+rwm7/oRb6o5WXsWPa3xmv
103UKTnWZ75Z9XTRE5pqQKDtwGsidFay8GVCJPrq0NuB8o50nQBAByfUCD1thOdE5g
11nNfunXEPM2hRwgy7/T8AL6fd69STFPoOuiSCWXX+5ASbWNUB8Mwl48o93DxZdIlK
12/Sk3jk+q+zTl8nc6aOT3sKR3HK9P5XLUtY5tcmDH6kx9S89/IsCots2gEHL9R0Le
13Wxu5hKBcttxMLH6dN/qLa8mK/Q5foDbFEft7Yv0XYYocI9Tc5APA5xkUyYng7Pb7
141FDXJC6Xal/+4aXHNwRLeXIaJgJ6LxQ4mq7xxlIGx5zUH53vIr88rL8B8uRBTn+5
15DGxzYjRs
16=z/lk
17-----END PGP SIGNATURE-----

Timestamp of file with hash f424bad1e3eed91a429f67534ae92bee38c1b1ae4c61eae9451b352c20c8a36b -

MarcoFalke merged this on May 24, 2021

MarcoFalke closed this on May 24, 2021

kiminuo deleted the branch on May 24, 2021

sidhujag referenced this in commit d1ca32072e on May 25, 2021

gwillen referenced this in commit 29a8292858 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

Remove GetDataDir(net_specific) function #21850

Conflicts

Remove `GetDataDir(net_specific)` function #21850