Fix #15240, see: #15240 (comment)
Fix #15745
Fix broken feature_config_args.py tests (disabled by MarcoFalke@fabe28a0cdcfa13e0e595a0905e3642a960d3077). All test are enabled now.
This PR is alternative to #13621.
User’s $HOME directory is not touched unnecessarily now.
To make reviewing easier only bitcoind code is modified (neither bitcoin-cli nor bitcoin-qt).
Refs:
788@@ -784,7 +789,8 @@ void ClearDatadirCache()
789
790 fs::path GetConfigFile(const std::string& confPath)
791 {
792- return AbsPathForConfigVal(fs::path(confPath), false);
793+ fs::path path = fs::path(confPath);
794+ return path.is_absolute() ? path : AbsPathForConfigVal(path, false);
fs::absolute already does this in AbsPathForConfigVal.
It seems like a better fix (simpler and more general) would be to add:
0if (path.is_absolute()) return path;
as the first line of AbsPathForConfigVal.
772@@ -773,6 +773,11 @@ const fs::path &GetDataDir(bool fNetSpecific)
773 return path;
774 }
775
776+bool CheckDataDirOption()
777+{
778+ return gArgs.IsArgSet("-datadir") ? fs::is_directory(fs::system_complete(gArgs.GetArg("-datadir", ""))) : true;
You are right. This behavior is intentional.
Would be helpful to say what the function is supposed to do in the header. Maybe
0//! Return true if -datadir value points to a valid directory or was not specified.
It might be also be less confusing to write this as:
0return !gArgs.IsArgSet("-datadir") || fs::is_directory(fs::system_complete(gArgs.GetArg("-datadir", "")))
In commit “Add CheckDataDirOption() function” (a45f692366d7bb4814af83e3b645c9e5a5a734e6)
I don’t think IsArgSet is the right function to call here. If somebody has a datadir option specified in the config file, but wants to unset it on the command line by passing -nodatadir or -datadir="", it seems like this should be allowed. Would suggest:
0std::string datadir = gArgs.GetArg("-datadir", "");
1return datadir.empty() || fs::is_directory(fs::system_complete(datadir));
And a corresponding change in GetDataDir()
I don’t think IsArgSet is the right function to call here. If somebody has a datadir option specified in the config file, but wants to unset it on the command line by passing -nodatadir or -datadir="", it seems like this should be allowed.
Good catch! Agree.
On my system:
0# create a file in the default data dir
1touch "/Users/joao/Library/Application Support/Bitcoin"
2
3# launch with default data dir
4src/bitcoind
5
6
7************************
8EXCEPTION: N5boost10filesystem16filesystem_errorE
9boost::filesystem::create_directory: File exists: "/Users/joao/Library/Application Support/Bitcoin"
10bitcoin in AppInit()
11
12Assertion failed: (globalChainBaseParams), function BaseParams, file chainparamsbase.cpp, line 30.
13[1] 53221 abort src/bitcoind
On my system:
0# create a file in the default data dir 1touch "/Users/joao/Library/Application Support/Bitcoin" 2 3# launch with default data dir 4src/bitcoind 5 6 7************************ 8EXCEPTION: N5boost10filesystem16filesystem_errorE 9boost::filesystem::create_directory: File exists: "/Users/joao/Library/Application Support/Bitcoin" 10bitcoin in AppInit() 11 12Assertion failed: (globalChainBaseParams), function BaseParams, file chainparamsbase.cpp, line 30. 13[1] 53221 abort src/bitcoind
The same behavior is observed on master as well. This PR does not change it.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
This comment doesn’t seem to be true: #15864#issue-272240173
To make reviewing easier only bitcoind code is modified (neither bitcoin-cli nor bitcoin-qt).
Since GetConfigFile is called all three places. It would also seem worrying if this were true, since it would mean the different binaries wouldn’t behave consistently.
@ryanofsky
Thank you for your review. Your comment has been addressed.
Also commits for bitcoin-qt and tools (bitcoin-cli, bitcoin-wallet) have been added.
Would you mind re-reviewing?
1225@@ -1226,6 +1226,9 @@ int64_t GetStartupTime()
1226
1227 fs::path AbsPathForConfigVal(const fs::path& path, bool net_specific)
1228 {
1229+ if (path.is_absolute()) {
In commit “Return absolute path early in AbsPathForConfigVal” (f46eec4f80117ce242ed8c86b6216ece3eb20fba)
It would be helpful if the commit message said why this change was being made. Is this an optimization or is it preventing an error or does it have some other effect on behavior?
94@@ -95,8 +95,7 @@ static bool AppInit(int argc, char* argv[])
95
96 try
97 {
98- if (!fs::is_directory(GetDataDir(false)))
99- {
100+ if (!CheckDataDirOption()) {
In commit “Fix datadir handling in bitcoind” (160470f76f22edd1f9ef6448584381fc5626b9c4)
I can’t figure out what this commit is doing. Is it fixing #15745 or #15864? Is the idea to prevent the datadir from being cached here? Can you write in the commit description what problem this is fixing and how it works?
utACK c4ab7420ef5dc4e6925519f8571a936f38720862. It’s not really clear to me which bugs are being fixed by these changes, and how exactly the fixes work, but the basic idea seems to be to avoid calling GetDataDir() various places early on startup when it isn’t necessary, to avoid caching bad values.
The changes seem worth making as optimizations or for clarity regardless of the bugs. But it’d also make sense as followup to look into:
Both of these seem questionable to begin with.
It’s not really clear to me which bugs are being fixed by these changes, and how exactly the fixes work, but the basic idea seems to be to avoid calling GetDataDir() various places early on startup when it isn’t necessary, to avoid caching bad values.
The root of the reported bugs (#15240, #15745) is GetDataDir() tries to create data directory. This step is premature when config file has not been read yet.
GetDataDir() - probably on init.
752@@ -753,8 +753,9 @@ const fs::path &GetDataDir(bool fNetSpecific)
753 if (!path.empty())
754 return path;
755
756- if (gArgs.IsArgSet("-datadir")) {
757- path = fs::system_complete(gArgs.GetArg("-datadir", ""));
758+ std::string datadir = gArgs.GetArg("-datadir", "");
In commit “Add CheckDataDirOption() function” (5df1581b9ee6f13593b33fa9629ea9fbadcad732)
Ideally commit message might also mention that it changes the behavior of the GetDataDir function. The effect I think is to now allow -nodatadir or -datadir='' options on the command line to override any datadir specified in the config file and reset it to default. In theory this could even be mentioned in release notes, but it’s probably too niche.
The effect I think is to now allow
-nodatadiror-datadir=''options on the command line to override any datadir specified in the config file and reset it to default.
Unfortunately, neither -nodatadir nor on the command line cannot unset datadir specified in the config file or reset it to default. See: #16097.-datadir=''
The initial idea was:
I don’t think
IsArgSetis the right function to call here.
If we recall: https://github.com/bitcoin/bitcoin/blob/8c69fae94410f54bad13be0f34d54370fddbf4b3/src/util/system.cpp#L470-L474
using IsArgNegated is (logically) inappropriate for -datadir option, IMO.
Also (but minor), the suggested code uses only one function call (GetArg) instead of two calls (IsArgSet and GetArg).
The effect I think is to now allow
-nodatadiror-datadir=''options on the command line to override any datadir specified in the config file and reset it to default.
Actually, with this PR -datadir='' (with empty string) or -datadir will reset it to default. So, going to close #16416 in favor of this PR.
utACK 9bddf12763e1bcc41c176f8b7f0b883efed49823. Changes since last review are adding commit descriptions, documenting new CheckDataDirOption() method, and handling -nodatadir and -datadir="" better.
This PR should be tagged as needing release notes if release notes aren’t added before it is merged in a doc/release-notes-15864.md file. I think you could take the description from #15864 (comment) and basically turn it into a release note, describing the class of bugs that are fixed and not mentioning specific function names.
1198@@ -1199,6 +1199,9 @@ int64_t GetStartupTime()
1199
1200 fs::path AbsPathForConfigVal(const fs::path& path, bool net_specific)
1201 {
1202+ if (path.is_absolute()) {
1203+ return path;
1204+ }
Do you mean to combine the “Enable all tests in feature_config_args.py " commit (5124b1cef24a9b30876ddedf922dfdb555bd4ace) into the “Return absolute path early in AbsPathForConfigVal” commit (c21150fd6f598196fae93cf4bddc6e7625a43660)?
Master branch plus this combined commit will fail the test. And the test will be passed on top of the 584621f9a81b6cc547c8f1365f1b358866e9a335 and b921f6ee951f0ad6d87eeeb372136a7314af0b0e commits.
Did I understand your intention correctly?
This prevents premature GetDataDir() calls, e.g., when config file is
not read yet.
This prevents premature tries to access or create the default datadir.
This is useful when the -datadir option is specified and the default
datadir is unreachable.
This prevents premature tries to access or create the default datadir.
This is useful when the -datadir option is specified and the default
datadir is unreachable.
This prevents premature tries to access or create the default datadir.
This is useful when the -datadir option is specified and the default
datadir is unreachable.
All other clients and tools use CheckDataDirOption() rather
fs::is_directory(GetDataDir(false)) for the first datadir check.
Rebased. @MarcoFalke
The 09a3071 commit would have to be in the test commit or before it, otherwise the test running with the gui might fail on the test commit?
Commits have been rearranged. The test commit is the last one now.
Label “Needs backport 0.18” ?