Move GetDataDir to ArgsManager

kiminuo commented at 11:03 am on February 20, 2021: contributor

This PR attempts to contribute to “Remove gArgs” (#21005).

Main changes:

GetDataDir() function is moved to ArgsManager.GetDataDirPath().
GetBlocksDir() function is moved to ArgsManager.GetBlocksDirPath().

kiminuo force-pushed on Feb 20, 2021

DrahtBot added the label Utils/log/libs on Feb 20, 2021

kiminuo force-pushed on Feb 20, 2021

DrahtBot commented at 2:25 pm on February 20, 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:

#21727 (refactor: Move more stuff to blockstorage by MarcoFalke)
#20966 (banman: save the banlist in a JSON format on disk by vasild)
#19213 (util: Get rid of RecursiveMutex in Get{Blocks,Data}Dir by hebasto)
#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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 force-pushed on Feb 21, 2021

kiminuo force-pushed on Feb 22, 2021

in src/util/system.cpp:770 in 3544cd27b4 outdated

767@@ -767,34 +768,40 @@ const fs::path &GetBlocksDir()
768     return path;
769 }
770 
771-const fs::path &GetDataDir(bool fNetSpecific)
772+void GetDataDir(ArgsManager& argsManager, bool fNetSpecific, fs::path* path)

kiminuo commented at 11:30 am on February 23, 2021:

note: std::string datadir can be passed instead of ArgsManager instance.

in src/util/system.cpp:774 in 3544cd27b4 outdated

784     if (!datadir.empty()) {
785-        path = fs::system_complete(datadir);
786-        if (!fs::is_directory(path)) {
787-            path = "";
788-            return path;
789+        *path = fs::system_complete(datadir);

kiminuo commented at 11:31 am on February 23, 2021:

Should I get rid of pointers?

in src/util/system.h:100 in 3544cd27b4 outdated

 94@@ -94,6 +95,15 @@ fs::path GetDefaultDataDir();
 95 // The blocks directory is always net specific.
 96 const fs::path &GetBlocksDir();
 97 const fs::path &GetDataDir(bool fNetSpecific = true);
 98+
 99+/**
100+ * Get data directory path.

kiminuo commented at 11:32 am on February 23, 2021:

note: Maybe mention that empty datadir leads to default datadir?

in src/util/system.h:106 in 3544cd27b4 outdated

101+ *
102+ * @param      argsManager Arguments manager instance.
103+ * @param      fnetSpecific Append network identifier to the path.
104+ * @param[out] data Directory path.
105+ */
106+void GetDataDir(ArgsManager& argsManager, bool fNetSpecific, fs::path* path);

kiminuo commented at 11:34 am on February 23, 2021:

Note: Is signature in form: void GetDataDir(fs::path* path, ArgsManager& argsManager, bool fNetSpecific = true); preferred?

in src/test/util_tests.cpp:52 in 3544cd27b4 outdated

47@@ -48,24 +48,40 @@ BOOST_FIXTURE_TEST_SUITE(util_tests, BasicTestingSetup)
48 
49 BOOST_AUTO_TEST_CASE(util_datadir)
50 {
51-    ClearDatadirCache();
52-    const fs::path dd_norm = GetDataDir();
53+    FastRandomContext insecure_rand_ctx;
54+    const fs::path datadir_path = fs::temp_directory_path() / "test_common_" PACKAGE_NAME / insecure_rand_ctx.rand256().ToString();

kiminuo commented at 11:35 am on February 23, 2021:

note: Would be nice to wrap this in a helper function.

in src/test/util_tests.cpp:52 in 3544cd27b4 outdated

47@@ -48,24 +48,40 @@ BOOST_FIXTURE_TEST_SUITE(util_tests, BasicTestingSetup)
48 
49 BOOST_AUTO_TEST_CASE(util_datadir)
50 {
51-    ClearDatadirCache();

kiminuo commented at 11:35 am on February 23, 2021:

Should the original test be preserved?

laanwj commented at 1:37 pm on February 23, 2021: member

Concept ACK

kiminuo force-pushed on Feb 23, 2021

hebasto commented at 9:21 pm on February 23, 2021: member

Concept ACK.

kiminuo commented at 6:46 am on February 24, 2021: contributor

@sipa Yes, you are right. I have modified that.

kiminuo force-pushed on Feb 25, 2021

in src/util/system.h:98 in 82b8119733 outdated

94@@ -94,6 +95,7 @@ fs::path GetDefaultDataDir();
95 // The blocks directory is always net specific.
96 const fs::path &GetBlocksDir();
97 const fs::path &GetDataDir(bool fNetSpecific = true);
98+

kiminuo commented at 8:32 am on February 26, 2021:

Will remove.

in src/test/util_tests.cpp:1149 in 82b8119733 outdated

1144@@ -1139,21 +1145,23 @@ BOOST_AUTO_TEST_CASE(util_ReadWriteSettings)
1145 {
1146     // Test writing setting.
1147     TestArgsManager args1;
1148-    args1.LockSettings([&](util::Settings& settings) { settings.rw_settings["name"] = "value"; });
1149+    args1.ForceSetArg("-datadir", m_path_root.string());
1150+    args1.LockSettings([&](util::Settings& settings) { settings.rw_settings["name"] = "value"; });

kiminuo commented at 8:33 am on February 26, 2021:

0    args1.LockSettings([&](util::Settings& settings) { settings.rw_settings["name"] = "value"; });

kiminuo commented at 8:33 am on February 26, 2021:

Will remove to fix linter.

kiminuo force-pushed on Feb 26, 2021

in src/util/system.cpp:820 in 3aa33265a2 outdated

816@@ -806,10 +817,7 @@ bool CheckDataDirOption()
817 
818 void ClearDatadirCache()
819 {
820-    LOCK(csPathCached);
821-
822-    pathCached = fs::path();
823-    pathCachedNetSpecific = fs::path();
824+    gArgs.ClearDatadirPathCache();

kiminuo commented at 11:34 am on February 26, 2021:

bug: LOCK(csPathCached); should be here.

kiminuo force-pushed on Feb 27, 2021

kiminuo marked this as ready for review on Feb 27, 2021

kiminuo renamed this:
~~GetDataDir: Accept ArgsManager as a parameter.~~
Move GetDataDir to ArgsManager as a parameter
on Feb 27, 2021

kiminuo renamed this:
~~Move GetDataDir to ArgsManager as a parameter~~
Move GetDataDir to ArgsManager
on Feb 27, 2021

kiminuo force-pushed on Feb 28, 2021

kiminuo commented at 10:34 am on March 2, 2021: contributor

@laanwj @hebasto @MarcoFalke @jonatack @jnewbery Friendly ping: Could anyone please tell me if you find the approach to be correct or not?

MarcoFalke commented at 10:56 am on March 2, 2021: member

It seems fine to remove a global from a function that should be a pure utility helper

kiminuo commented at 9:29 pm on March 8, 2021: contributor

@MarcoFalke So are you saying that my actual approach is good or not? I honestly can’t say.

MarcoFalke commented at 7:07 am on March 9, 2021: member

Concept ACK, haven’t reviewed in detail

DrahtBot added the label Needs rebase on Mar 12, 2021

kiminuo force-pushed on Mar 12, 2021

DrahtBot removed the label Needs rebase on Mar 12, 2021

DrahtBot commented at 4:49 pm on March 15, 2021: member

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

DrahtBot added the label Needs rebase on Mar 15, 2021

kiminuo force-pushed on Mar 15, 2021

DrahtBot removed the label Needs rebase on Mar 15, 2021

DrahtBot added the label Needs rebase on Mar 30, 2021

kiminuo force-pushed on Mar 30, 2021

DrahtBot removed the label Needs rebase on Mar 30, 2021

DrahtBot added the label Needs rebase on Apr 6, 2021

kiminuo force-pushed on Apr 6, 2021

DrahtBot removed the label Needs rebase on Apr 6, 2021

in src/util/system.h:204 in 839324993a outdated

199@@ -200,6 +200,8 @@ class ArgsManager
200     std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);
201     bool m_accept_any_command GUARDED_BY(cs_args){true};
202     std::list<SectionInfo> m_config_sections GUARDED_BY(cs_args);
203+    fs::path datadirPathCached GUARDED_BY(cs_args);
204+    fs::path datadirPathCachedNetSpecific GUARDED_BY(cs_args);

ryanofsky commented at 4:56 pm on April 8, 2021:

In commit “Move GetDataDir(fNetSpecific) implementation to ArgsManager.” (839324993aeb8448af5a811b7323fa460ac9eac7)

See symbol naming conventions https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md, but would suggest m_cached_datadir_path, m_cached_network_datadir_path

kiminuo commented at 6:10 am on April 10, 2021:

Applied, thanks.

in src/test/util_tests.cpp:52 in ec7769315f outdated

48@@ -49,24 +49,27 @@ BOOST_FIXTURE_TEST_SUITE(util_tests, BasicTestingSetup)
49 
50 BOOST_AUTO_TEST_CASE(util_datadir)
51 {
52-    ClearDatadirCache();
53-    const fs::path dd_norm = GetDataDir();
54+    ArgsManager argsManager;

ryanofsky commented at 5:00 pm on April 8, 2021:

In commit “Modify “util_datadir” unit test to not use gArgs.” (ec7769315f5ed62e6145492977226925e122292c)

It seems unnecessary to add another argsmanager for this test, with 3 side-by-side argsmanagers:

gArgs
BasicTestingSetup::m_args_manager added in d4f86a3f608c98365b537d78d7ef1693463d7b9b
This local argsManager variable.

Maybe it could just use the basictesting member. If not though, it’d be helpful to have a comment explaining its purpose.

Also see naming in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md but camelCase should not be used for variable names. If there needs to be a variable would suggest just calling it args which is convention for other ArgsManager instances.

kiminuo commented at 8:34 pm on April 8, 2021:

The main reason I did it this way was/is that I persuades me that it does what one expects and there is no possible confusion. When I put gArgs (or BasicTestingSetup::m_args_manager) there, it forces to me wonder: How is gArgs actually initialized and where it is? Can some later refactoring affect the test silently?

Maybe I see it this way because I don’t read Bitcoin Core source code for years as other people here. Anyway, I’m open to suggestions, if you still find it better to change the code, I can do it.

Note: An alternative would be to move the test somewhere where there is no BasicTestingSetup and no gArgs but I’m not sure if there are such tests.

ryanofsky commented at 9:37 pm on April 8, 2021:

That’s reasonable, I would maybe add a commment like /* Use local args variable instead of m_args to avoid making assumptions about test setup. */, but feel free to keep the variable.

kiminuo commented at 6:05 am on April 10, 2021:

Added comment, thanks.

in src/util/system.h:198 in fa0b2291aa outdated

194@@ -195,6 +195,7 @@ class ArgsManager
195     std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);
196     bool m_accept_any_command GUARDED_BY(cs_args){true};
197     std::list<SectionInfo> m_config_sections GUARDED_BY(cs_args);
198+    fs::path blocksPathCachedNetSpecific GUARDED_BY(cs_args);

ryanofsky commented at 5:06 pm on April 8, 2021:

In commit “Change GetBlocksDir() to ArgsManager.GetBlocksDirPath().” (fa0b2291aa84fe372bb37a9fcbf1ad3165f7daea)

Again should follow naming convention, would suggest m_cached_blocks_path. See naming in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

kiminuo commented at 6:05 am on April 10, 2021:

Applied, thanks.

in src/util/system.cpp:741 in c44993ad96 outdated

749-}
750-} // namespace
751-
752 static fs::path g_blocks_path_cache_net_specific;
753-static fs::path pathCached;
754-static fs::path pathCachedNetSpecific;

ryanofsky commented at 5:15 pm on April 8, 2021:

In commit “Move StripRedundantLastElementsOfPath before ArgsManager class.” (c44993ad96d649be48c94d4ae38d354d609a7282)

This commit seems not to compile because these variables are removed

 0  CXX      libbitcoin_server_a-net.o
 1util/system.cpp:771:37: error: use of undeclared identifier 'pathCachedNetSpecific'
 2    fs::path &path = fNetSpecific ? pathCachedNetSpecific : pathCached;
 3                                    ^
 4util/system.cpp:771:61: error: use of undeclared identifier 'pathCached'
 5    fs::path &path = fNetSpecific ? pathCachedNetSpecific : pathCached;
 6                                                            ^
 7util/system.cpp:809:5: error: use of undeclared identifier 'pathCached'; did you mean 'csPathCached'?
 8    pathCached = fs::path();
 9    ^~~~~~~~~~
10    csPathCached
11util/system.cpp:740:23: note: 'csPathCached' declared here
12static RecursiveMutex csPathCached;
13                      ^
14util/system.cpp:809:16: error: no viable overloaded '='
15    pathCached = fs::path();
16    ~~~~~~~~~~ ^ ~~~~~~~~~~
17./sync.h:89:16: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'fs::path' to 'const AnnotatedMixin<std::recursive_mutex>' for 1st argument
18class LOCKABLE AnnotatedMixin : public PARENT
19               ^
20util/system.cpp:810:5: error: use of undeclared identifier 'pathCachedNetSpecific'
21    pathCachedNetSpecific = fs::path();
22    ^

kiminuo commented at 6:10 am on April 10, 2021:

Fixed, thanks.

in src/util/system.cpp:422 in 839324993a outdated

417+
418+    path = StripRedundantLastElementsOfPath(path);
419+    return path;
420+}
421+
422+void ArgsManager::ClearDatadirPathCache()

ryanofsky commented at 5:18 pm on April 8, 2021:

In commit “Move GetDataDir(fNetSpecific) implementation to ArgsManager.” (839324993aeb8448af5a811b7323fa460ac9eac7)

This commit seems not to compile

 0util/system.cpp:422:19: error: out-of-line definition of 'ClearDatadirPathCache' does not match any declaration in 'ArgsManager'; did you mean 'ClearPathCache'?
 1void ArgsManager::ClearDatadirPathCache()
 2                  ^~~~~~~~~~~~~~~~~~~~~
 3                  ClearPathCache                                                                                                                                          
 4./util/system.h:280:10: note: 'ClearPathCache' declared here
 5    void ClearPathCache();
 6         ^
 7util/system.cpp:820:11: error: no member named 'ClearDatadirPathCache' in 'ArgsManager'
 8    gArgs.ClearDatadirPathCache();
 9    ~~~~~ ^
102 errors generated.

kiminuo commented at 6:09 am on April 10, 2021:

This is fixed on my system now. Thanks.

in src/test/util/setup_common.h:84 in d4f86a3f60 outdated

80@@ -80,6 +81,7 @@ struct BasicTestingSetup {
81     ~BasicTestingSetup();
82 
83     const fs::path m_path_root;
84+    ArgsManager m_args_manager;

ryanofsky commented at 5:25 pm on April 8, 2021:

In commit “BasicTestingSetup: Add ArgsManager.” (d4f86a3f608c98365b537d78d7ef1693463d7b9b)

Convention other places is to call ArgsManager instances args, suggest naming this m_args

kiminuo commented at 6:03 am on April 10, 2021:

Applied, thanks.

ryanofsky commented at 5:46 pm on April 8, 2021: member

This basically looks good. Other reviewers might be hesitant about this PR adding many new gArgs references when it claims to help “Remove gArgs”. I think this is fine, because these are just places where gArgs was being used implicitly. An alternative to avoid churn would be to pass ArgsManager& references functions to places needing it in advance, but if the options for removing gArgs are:

One big PR removing gArgs
Smaller PRs removing gArgs with less churn, but less obvious changes.
Smaller PRs removing gArgs with more churn, but more obvious changes.

This PR is choosing option 3.

I added some review comments below. Suggestions are:

Fix compile errors in intermediate commits
For consistency and brevity call ArgsManager instances args or m_args not argsManager or m_args_manager
Fix other camelCase names

ryanofsky commented at 6:01 pm on April 8, 2021: member

The new BasicTestingSetup args manager instance doesn’t seem to be used very much right now. I’d think here or at some point in the future it’d make sense for BasicTestingSetup::m_node.args to be a pointer to the local test args manager instead of to gArgs. Maybe a change like the following would make sense:

 0diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
 1index 80ab69c131e..6fddb469e56 100644
 2--- a/src/bitcoind.cpp
 3+++ b/src/bitcoind.cpp
 4@@ -113,8 +113,8 @@ static bool AppInit(int argc, char* argv[])
 5     util::ThreadSetInternalName("init");
 6 
 7     // If Qt is used, parameters/bitcoin.conf are parsed in qt/bitcoin.cpp's main()
 8-    SetupServerArgs(node);
 9     ArgsManager& args = *Assert(node.args);
10+    SetupServerArgs(args);
11     std::string error;
12     if (!args.ParseParameters(argc, argv, error)) {
13         return InitError(Untranslated(strprintf("Error parsing command line arguments: %s\n", error)));
14diff --git a/src/init.cpp b/src/init.cpp
15index 0d44b75b727..7b024e14686 100644
16--- a/src/init.cpp
17+++ b/src/init.cpp
18@@ -358,12 +358,8 @@ static void OnRPCStopped()
19     LogPrint(BCLog::RPC, "RPC stopped.\n");
20 }
21 
22-void SetupServerArgs(NodeContext& node)
23+void SetupServerArgs(ArgsManager& argsman)
24 {
25-    assert(!node.args);
26-    node.args = &gArgs;
27-    ArgsManager& argsman = *node.args;
28-
29     SetupHelpOptions(argsman);
30     argsman.AddArg("-help-debug", "Print help message with debugging options and exit", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); // server-only for now
31 
32diff --git a/src/init.h b/src/init.h
33index 5d01d4c1ac3..98b83f0cf7f 100644
34--- a/src/init.h
35+++ b/src/init.h
36@@ -69,7 +69,7 @@ bool AppInitMain(const std::any& context, NodeContext& node, interfaces::BlockAn
37 /**
38  * Register all arguments with the ArgsManager
39  */
40-void SetupServerArgs(NodeContext& node);
41+void SetupServerArgs(ArgsManager& argsman);
42 
43 /** Returns licensing information (for -version) */
44 std::string LicenseInfo();
45diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
46index fc6d0febc2d..7eeeb3c1155 100644
47--- a/src/qt/bitcoin.cpp
48+++ b/src/qt/bitcoin.cpp
49@@ -477,7 +477,7 @@ int GuiMain(int argc, char* argv[])
50 
51     /// 2. Parse command-line options. We do this after qt in order to show an error if there are problems parsing these
52     // Command-line options take precedence:
53-    SetupServerArgs(node_context);
54+    SetupServerArgs(gArgs);
55     SetupUIArgs(gArgs);
56     std::string error;
57     if (!gArgs.ParseParameters(argc, argv, error)) {
58diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
59index 1c961be419d..a460eab906d 100644
60--- a/src/test/util/setup_common.cpp
61+++ b/src/test/util/setup_common.cpp
62@@ -74,6 +74,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
63     : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()},
64       m_args_manager{}
65 {
66+    m_node.args = &m_args_manager;
67     const std::vector<const char*> arguments = Cat(
68         {
69             "dummy",
70@@ -92,7 +93,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
71     gArgs.ForceSetArg("-datadir", m_path_root.string());
72     gArgs.ClearPathCache();
73     {
74-        SetupServerArgs(m_node);
75+        SetupServerArgs(*m_node.args);
76         std::string error;
77         const bool success{m_node.args->ParseParameters(arguments.size(), arguments.data(), error)};
78         assert(success);

kiminuo commented at 8:35 pm on April 8, 2021: contributor

@ryanofsky Thank you for the review! I will try to address it soon.

Other reviewers might be hesitant about this PR adding many new gArgs references when it claims to help “Remove gArgs”.

I was not sure about the scope of the PR, I started small but it did not feel compelling enough. My inspiration was #20158.

kiminuo force-pushed on Apr 9, 2021

kiminuo commented at 11:21 am on April 9, 2021: contributor

The new BasicTestingSetup args manager instance doesn’t seem to be used very much right now. I’d think here or at some point in the future it’d make sense for BasicTestingSetup::m_node.args to be a pointer to the local test args manager instead of to gArgs. Maybe a change like the following would make sense:

I would like to reserve this for a follow-up PR. Other than that, I have attempted to address all your suggestions. Notably, I tried to run make check for each commit of this PR. I can try that again in the evening with clear head.

kiminuo force-pushed on Apr 9, 2021

kiminuo commented at 6:14 am on April 10, 2021: contributor

“Cirrus CI / [depends, sanitizers: thread (TSan), no gui] [focal]” seems to be flaky lately.

jonatack commented at 7:31 am on April 10, 2021: member

“Cirrus CI / [depends, sanitizers: thread (TSan), no gui] [focal]” seems to be flaky lately.

If you rebase on latest master I think that solves it.

kiminuo force-pushed on Apr 10, 2021

kiminuo commented at 8:29 am on April 10, 2021: contributor

If you rebase on latest master I think that solves it.

Let’s try it :)

edit: It helped!

in src/util/system.cpp:741 in fac964698f outdated

775@@ -737,8 +776,6 @@ fs::path GetDefaultDataDir()
776 }
777 
778 static fs::path g_blocks_path_cache_net_specific;
779-static fs::path pathCached;
780-static fs::path pathCachedNetSpecific;

ryanofsky commented at 5:09 pm on April 12, 2021:

In commit “Move GetDataDir(fNetSpecific) implementation to ArgsManager.” (fac964698fab1b269d4414dc54560b29a3eccd18)

Note (no change suggested): I was initially confused why pathCached and pathCachedNetSpecific were being removed in this commit without also removing g_blocks_path_cache_net_specific. But it’s removed in a later commit, and the reason for treating it differently just seems to be that it’s needed many fewer places, so the ::GetBlocksDir wrapper function can be dropped right away, unlike ::GetDataDir

kiminuo commented at 1:38 pm on April 13, 2021:

My primary objective was to tackle ::GetDataDir but then it led me to handle ::GetBlocksDir too. I did not want to modify those two at the same time.

in src/test/util_tests.cpp:68 in 8584666393 outdated

67-    gArgs.ForceSetArg("-datadir", dd_norm.string() + "/./");
68-    ClearDatadirCache();
69-    BOOST_CHECK_EQUAL(dd_norm, GetDataDir());
70-
71-    gArgs.ForceSetArg("-datadir", dd_norm.string() + "/.//");
72-    ClearDatadirCache();

ryanofsky commented at 5:23 pm on April 12, 2021:

In commit “Modify “util_datadir” unit test to not use gArgs.” (8584666393255a7ce62d741ddd8d994c486423c6)

It seems like the checks in this test might be meaningless now that the ClearDatadirCache calls are dropped, because the repeated BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirPath()); checks will keep passing no matter what value -datadir is force set to.

I think it would be best for this commit to either keep the Clear calls so the test is changed as little as possible. Or the test could be rewritten to use different expected values or different args objects.

kiminuo commented at 1:35 pm on April 13, 2021:

You are right. I have put those commands back.

ryanofsky commented at 5:49 pm on April 12, 2021: member

Code review almost-ACK 92b1890c9eeeecf9cf7325ef3dc1d2c44a588b87, modulo the util_datadir commit 8584666393255a7ce62d741ddd8d994c486423c6, which I think breaks that test in a way where it still passes but no longer meaningfully checks things (see suggestion below).

I would also consider dropping the 6e7e67f855282ae4baa3756f2e95aee12de13f91 commit from this PR because I think it makes the PR a lot bigger, causes code churn, and doesn’t add as much value as the other changes here. 6e7e67f855282ae4baa3756f2e95aee12de13f91 could be its own separate followup PR. (@dongcarl could be good person to ask about it, too.)

Otherwise everything looks great.

kiminuo force-pushed on Apr 12, 2021

kiminuo commented at 8:15 am on April 13, 2021: contributor

Code review almost-ACK 92b1890, modulo the util_datadir commit 8584666, which I think breaks that test in a way where it still passes but no longer meaningfully checks things (see suggestion below).

I have put ClearDatadirCache calls back. This was an unintended change I missed.

I would also consider dropping the 6e7e67f commit from this PR because I think it makes the PR a lot bigger, causes code churn, and doesn’t add as much value as the other changes here. 6e7e67f could be its own separate followup PR. (@dongcarl could be good person to ask about it, too.)

You are right, the commit makes the PR too large probably. I have dropped the 6e7e67f (Change GetDataDir() calls to gArgs.GetDataDirPath() calls.) commit.

ryanofsky approved

ryanofsky commented at 5:22 pm on April 13, 2021: member

Code review ACK 834b971bff371951e3e63617d0a3fb9b3ad79c62. Thanks for fast updates! Only change last review was restoring util_datadir test, and dropping a commit so the PR is now limited to src/util/system (and tests) and doesn’t affect outside code.

ryanofsky commented at 1:32 pm on April 16, 2021: member

Noticed this in IRC:

<Kiminuo> Hi guys, a quick question: I have this PR #21244 and it got one ACK. Now, should I just wait patiently until somebody else will review the PR? Or am I supposed to do something else to increase the chance of merging it? I’m not in hurry. I would just like to understand the review process better.

This is an easy review that mostly affects tests so I’d encourage anybody who cares about c++ unit tests to look at this PR.

Also for Kiminuo, I think a good way to get other people to review your PRs is to review their PRs. Progress on bitcoin is limited more by lack of review than lack of code contributions, and reviews that are substantive and show thought & understanding are encouraged even if you don’t have prior experience reviewing bitcoin PRs (https://bitcoincore.reviews/ is a good resource for this).

jonatack commented at 1:43 pm on April 16, 2021: member

I think a good way to get other people to review your PRs is to review their PRs. Progress on bitcoin is limited more by lack of review than lack of code contributions, and reviews that are substantive and show thought & understanding are encouraged even if you don’t have prior experience reviewing bitcoin PRs (https://bitcoincore.reviews/ is a good resource for this).

Agree! suggested a similar approach to Kiminuo yesterday via IRC DM.

in src/util/system.h:201 in 834b971bff outdated

195@@ -200,6 +196,9 @@ class ArgsManager
196     std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);
197     bool m_accept_any_command GUARDED_BY(cs_args){true};
198     std::list<SectionInfo> m_config_sections GUARDED_BY(cs_args);
199+    fs::path m_cached_blocks_path GUARDED_BY(cs_args);
200+    fs::path m_cached_datadir_path GUARDED_BY(cs_args);
201+    fs::path m_cached_network_datadir_path GUARDED_BY(cs_args);

hebasto commented at 4:25 pm on April 17, 2021:

As these data members are cache, why do not declare them mutable? It will allow to keep constness of the GetSettingsPath and WriteSettingsFile.

kiminuo commented at 6:34 pm on April 17, 2021:

I have modified this.

in src/util/system.h:284 in 834b971bff outdated

279+    const fs::path &GetDataDirPath(bool fNetSpecific = true);
280+
281+    /**
282+     * For testing
283+     */
284+    void ClearPathCache();

hebasto commented at 4:37 pm on April 17, 2021:

The description of this function is not true: https://github.com/bitcoin/bitcoin/blob/834b971bff371951e3e63617d0a3fb9b3ad79c62/src/util/system.cpp#L974

kiminuo commented at 5:33 pm on April 17, 2021:

Do you have a suggestion for the description?

I think the comment is mostly unhelpful as it is too generic anyway. I would write something like that it’s useful to propagate changes in args. I’m not sure about proper English wording though.

hebasto commented at 5:42 pm on April 17, 2021:

Obviously, I’m not an Official Namer Of Stuff :)

Maybe “Clear cached directory paths”?

kiminuo commented at 10:07 am on April 18, 2021:

Will be addressed in the next push.

in src/util/system.h:279 in 834b971bff outdated

274+     *
275+     * @param fNetSpecific Append network identifier to the returned path
276+     * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned
277+     * @post Returned directory path is created unless it is empty
278+     */
279+    const fs::path &GetDataDirPath(bool fNetSpecific = true);

hebasto commented at 4:38 pm on April 17, 2021:

0    const fs::path& GetDataDirPath(bool net_specific = true);

kiminuo commented at 5:33 pm on April 17, 2021:

Yes, thank you.

in src/util/system.cpp:420 in 834b971bff outdated

415+}
416+
417+const fs::path& ArgsManager::GetDataDirPath(bool fNetSpecific)
418+{
419+    LOCK(cs_args);
420+    fs::path &path = fNetSpecific ? m_cached_network_datadir_path : m_cached_datadir_path;

hebasto commented at 4:40 pm on April 17, 2021:

style nit:

0    fs::path& path = fNetSpecific ? m_cached_network_datadir_path : m_cached_datadir_path;

kiminuo commented at 5:33 pm on April 17, 2021:

Yes, thank you.

in src/util/system.cpp:394 in 834b971bff outdated

387@@ -375,6 +388,72 @@ std::optional<unsigned int> ArgsManager::GetArgFlags(const std::string& name) co
388     return std::nullopt;
389 }
390 
391+const fs::path& ArgsManager::GetBlocksDirPath()
392+{
393+    LOCK(cs_args);
394+    fs::path &path = m_cached_blocks_path;

hebasto commented at 4:41 pm on April 17, 2021:

style nit:

0    fs::path& path = m_cached_blocks_path;

hebasto commented at 4:54 pm on April 17, 2021: member

Approach ACK 834b971bff371951e3e63617d0a3fb9b3ad79c62.

I see no reasons to guard new data members m_cached_*_path with RecursiveMutex cs_args. It tangles recursive locking of cs_args more and more. We should move from RecursiveMutex to Mutex (#19303, #19213). To not deteriorate status quo, maybe just use a new specific mutex?

Move StripRedundantLastElementsOfPath before ArgsManager class. 70cdf679f8

Move GetDataDir(fNetSpecific) implementation to ArgsManager. 1add318704

kiminuo force-pushed on Apr 17, 2021

kiminuo commented at 6:39 pm on April 17, 2021: contributor

I see no reasons to guard new data members m_cached_*_path with RecursiveMutex cs_args. It tangles recursive locking of cs_args more and more. We should move from RecursiveMutex to Mutex (#19303, #19213). To not deteriorate status quo, maybe just use a new specific mutex?

Would it be better to do this in a follow-up PR?

hebasto commented at 6:55 pm on April 17, 2021: member

I see no reasons to guard new data members m_cached_*_path with RecursiveMutex cs_args. It tangles recursive locking of cs_args more and more. We should move from RecursiveMutex to Mutex (#19303, #19213). To not deteriorate status quo, maybe just use a new specific mutex?

Would it be better to do this in a follow-up PR?

Not sure if it is better, but it won’t stop me from ACKing :)

hebasto commented at 7:07 pm on April 17, 2021: member

70cdf679f8e665dbdc3301873a0267fe9faa72cd - “Move StripRedundantLastElementsOfPath before ArgsManager class.”
1add318704108faa98f5b1b8e9c96d960e9d23a8 - “Move GetDataDir(fNetSpecific) implementation to ArgsManager.”
5441bd3f54e675445f5a9c10462c24fbba721243 - “Modify “util_datadir” unit test to not use gArgs.”
ea70f2266b4329cce75ccb8db73e426055f345a8 - “BasicTestingSetup: Add ArgsManager.”
46c8546e4d42b4825b82adf3fd1ff42591850530 - “Modify unit tests to use the ArgsManager in the BasicTestingSetup class instead of implicitly relying on gArgs.”
3cf594e1dc1131f6e39911e8ddc5721f310e5aeb - “Change GetBlocksDir() to ArgsManager.GetBlocksDirPath().”
69b8c4ad40185862fe199005c81a75d582053bd4 - “Change ClearDataDirPathCache() to ArgsManager.ClearPathCache().”

in src/test/util_tests.cpp:56 in 69b8c4ad40 outdated

56+    args.ForceSetArg("-datadir", m_path_root.string());
57 
58-    gArgs.ForceSetArg("-datadir", dd_norm.string() + "/");
59-    ClearDatadirCache();
60-    BOOST_CHECK_EQUAL(dd_norm, GetDataDir());
61+    fs::path dd_norm = args.GetDataDirPath();

hebasto commented at 7:43 pm on April 17, 2021:

Why const removed?

kiminuo commented at 10:04 am on April 18, 2021:

Will be addressed in the next push. Thanks.

hebasto approved

hebasto commented at 7:47 pm on April 17, 2021: member

ACK 69b8c4ad40185862fe199005c81a75d582053bd4, I have reviewed the code and it looks OK, tested locally.

nit: It seems 46c8546e4d42b4825b82adf3fd1ff42591850530 could be a scripted-diff.

Please next time do not rebase without necessity :)

kiminuo commented at 7:53 pm on April 17, 2021: contributor

Please next time do not rebase without necessity :)

Interesting point, it did not occur to me that it’s better. Thank you for pointing it out.

in src/test/util/setup_common.cpp:91 in ea70f2266b outdated

87@@ -87,6 +88,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
88         extra_args);
89     util::ThreadRename("test");
90     fs::create_directories(m_path_root);
91+    m_args.ForceSetArg("-datadir", m_path_root.string());

MarcoFalke commented at 8:32 pm on April 17, 2021:

There is already m_node.args. Any reason to avoid using that?

ryanofsky commented at 8:42 pm on April 17, 2021:

There is already m_node.args. Any reason to avoid using that?

I suggested this for a followup (It would be a bigger change involving init and might also require updating more tests). Diff was here: #21244 (comment)

MarcoFalke commented at 8:57 pm on April 17, 2021:

Until then I’d prefer to use m_node.args-> (or gArgs. if that is too much typing). It will be a trivial scripted diff to change that to m_args.. But having three argsman in the tests (two of which are the same and one that is different) will just cause confusion and makes review harder/code more fragile. How would you protect against their state diverging?

ryanofsky commented at 9:40 pm on April 17, 2021:

I’m happy with any intermediate state. Here’s what I think the ultimate goal should be:

Tests should have an ArgsManager m_args member that is newly initialized before each test and completely destroyed after each test.
Where possible, tests should pass m_args references directly to functions that are being tested and need to read options.
Tests can pass m_args pointers indirectly through NodeContext when they are calling node RPC methods, or through WalletContext when they are calling wallet RPC methods.
Unless tests are calling RPC methods or init code, they probably don’t need NodeContext and WalletContext instances and it would be nice to drop NodeContext from the base test class.
gArgs should go away in node and wallet code. It might be useful to keep gArgs in test code to read test options (random seeds, timeouts, log options, debug options). It might make sense to keep gArgs in GUI code since GUI code relies on other global singletons like QApplications and QSettings and trying to get rid of it might just add extra complexity.

The current PR seems just like a good a first step in this direction to me since it adds local test ArgsManager instances and starts to use them minimally.

These are just my thoughts though. I think any approach you want to take here is probably reasonable.

MarcoFalke commented at 8:21 am on April 18, 2021:

Fair enough, any approach fine by me, too

kiminuo commented at 10:56 am on April 18, 2021:

I think Russell summarized it very nicely.

My approach with this PR is to use m_args wrt GetDataDir() functionality for tests. Yes, it is somewhat fragile at this point. If you are too concerned about this, I can revert that change in BasicTestingSetup and leave it for some follow-up PR.

note: There are tests that just need GetDataDir() function and not anything else from ArgsManager, these tests benefit from using m_args (instead of gArgs).

note 2: Originally, https://github.com/bitcoin/bitcoin/commit/6e7e67f855282ae4baa3756f2e95aee12de13f91 was a part of this PR and with this, one can see quite nicely where gArgs is used and that’s one step closer to remove it from those places and make some more tests to be true unit tests.

Modify "util_datadir" unit test to not use gArgs. 1cb52ba065

BasicTestingSetup: Add ArgsManager. 511ce3a26b

scripted-diff: Replace m_args with m_local_args in getarg_tests.cpp

-BEGIN VERIFY SCRIPT-
git ls-files src/test/getarg_tests.cpp | xargs sed -i "s/m_args/m_local_args/g";
-END VERIFY SCRIPT-

55c68e6f01

scripted-diff: Modify unit tests to use the ArgsManager in the BasicTestingSetup class instead of implicitly relying on gArgs.

-BEGIN VERIFY SCRIPT-
git ls-files src/test/dbwrapper_tests.cpp src/test/denialofservice_tests.cpp src/test/flatfile_tests.cpp src/test/fs_tests.cpp src/test/settings_tests.cpp src/test/util_tests.cpp | xargs sed -i 's/GetDataDir()/m_args.GetDataDirPath()/g';
-END VERIFY SCRIPT-

83292e2a70

Change GetBlocksDir() to ArgsManager.GetBlocksDirPath(). b4190eff72

Change ClearDataDirPathCache() to ArgsManager.ClearPathCache(). bb8d1c6e02

in src/test/getarg_tests.cpp:192 in ea70f2266b outdated

197 
198     ResetArgs("-nofoo -foo"); // foo always wins:
199-    BOOST_CHECK(m_args.GetBoolArg("-foo", true));
200-    BOOST_CHECK(m_args.GetBoolArg("-foo", false));
201+    BOOST_CHECK(m_local_args.GetBoolArg("-foo", true));
202+    BOOST_CHECK(m_local_args.GetBoolArg("-foo", false));

MarcoFalke commented at 8:20 am on April 18, 2021:

This diff could be a scripted-diff in a separate commit?

kiminuo commented at 10:04 am on April 18, 2021:

Yes, will be addressed in the next push.

kiminuo commented at 10:09 am on April 18, 2021: contributor

I addessed outstanding suggestions. Diff since 69b8c4ad is minimal.

kiminuo force-pushed on Apr 18, 2021

ryanofsky approved

ryanofsky commented at 1:46 pm on April 19, 2021: member

Code review ACK bb8d1c6e029a2fd01387599d2ff3bfc879a8ada5. Just minor const/naming changes and splitting/scripting commits since last review

MarcoFalke commented at 6:57 pm on April 19, 2021: member

review ACK bb8d1c6e029a2fd01387599d2ff3bfc879a8ada5 📓

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK bb8d1c6e029a2fd01387599d2ff3bfc879a8ada5 📓
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYEACgkQzit1aX5p
 7pUgg4wv/VxpdghXhej+GNGTyDSSqYsXme76aDsRmpsxCgFsPO1mpnR9KzmCQNYN7
 8M9MBJ8+1Slsa8an8P5pk9cFYmGciu0Iu26hVOrZ/lESZYZ4j9+qWbN5dN2VkSDEg
 9GEt1P1RnE5qyqLQZmrrafkk0EcGl8ErBc56kjlYyciugjKX8KNqdvDSJAAdnMCmB
10h+TIlGuCXrdG2TEngv0neHJ5nrp4JG0hdkD73u8QeVnszs3KuHbXzWenng5jJKwC
11CidMrDLjLtc/B/I31GlXeWD3pYwlOkxrtC5rAObaThRtU0NlQ5Pz3DsrcvWLuEPA
12vzyBa/ru6TUHxGPpq4IMTjdf0v3GVYyjJc0xbXiGGLQygJw6ff0YzJm8F0hqw7J+
13/6xvt8hVz+10z887VNIBQLKgasLiDB5R1us+XprxYcwGW3GljiDtKCFUcIBvSlbn
14D3yODkhg6jYIoiLUi2xp8B2e9AWqlCSx09dySUl3Gn+Q9srvkHIu5ZOfUg1vM6rx
15XhwN4wTz
16=q1yj
17-----END PGP SIGNATURE-----

Timestamp of file with hash 977786d72da2ca0c8d5b6be0fa8686d3e4ab5578eca910bb84af1e639ee23595 -

hebasto approved

hebasto commented at 9:22 pm on April 19, 2021: member

re-ACK bb8d1c6e029a2fd01387599d2ff3bfc879a8ada5, addressed comments, and two commits made scripted-diffs since my previous review.

fanquake merged this on Apr 20, 2021

fanquake closed this on Apr 20, 2021

sidhujag referenced this in commit 77c3838976 on Apr 20, 2021

kiminuo deleted the branch on Apr 20, 2021

MarcoFalke referenced this in commit 599000903e on May 24, 2021

MarcoFalke referenced this in commit cea38b491f on Aug 26, 2021

sidhujag referenced this in commit a9f957b9d7 on Aug 28, 2021

Fabcien referenced this in commit 5257d17bbd on Jan 4, 2022

Fabcien referenced this in commit c50dcf5b67 on Jan 4, 2022

Fabcien referenced this in commit 3a1f63eafd on Jan 4, 2022

Fabcien referenced this in commit 0dca7ef835 on Jan 4, 2022

Fabcien referenced this in commit a0d2976033 on Jan 4, 2022

Fabcien referenced this in commit 134c109300 on Jan 4, 2022

Fabcien referenced this in commit 94db4facf3 on Jan 4, 2022

Fabcien referenced this in commit f4f40659c4 on Jan 4, 2022

DrahtBot locked this on Aug 16, 2022

Move GetDataDir to ArgsManager #21244

Conflicts