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);
Is this necessary? Looks like fs::absolute already does this in AbsPathForConfigVal.
Yes, it is needed to not touch default datadir unnecessarily, before all configs are read.
It seems like a better fix (simpler and more general) would be to add:
if (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;
This doesn't check the default data dir.
You are right. This behavior is intentional.
You are right. This behavior is intentional.
Would be helpful to say what the function is supposed to do in the header. Maybe
//! 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:
return !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:
std::string datadir = gArgs.GetArg("-datadir", "");
return 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:
# create a file in the default data dir
touch "/Users/joao/Library/Application Support/Bitcoin"
# launch with default data dir
src/bitcoind
************************
EXCEPTION: N5boost10filesystem16filesystem_errorE
boost::filesystem::create_directory: File exists: "/Users/joao/Library/Application Support/Bitcoin"
bitcoin in AppInit()
Assertion failed: (globalChainBaseParams), function BaseParams, file chainparamsbase.cpp, line 30.
[1] 53221 abort src/bitcoind
On my system:
# create a file in the default data dir touch "/Users/joao/Library/Application Support/Bitcoin" # launch with default data dir src/bitcoind ************************ EXCEPTION: N5boost10filesystem16filesystem_errorE boost::filesystem::create_directory: File exists: "/Users/joao/Library/Application Support/Bitcoin" bitcoin in AppInit() Assertion failed: (globalChainBaseParams), function BaseParams, file chainparamsbase.cpp, line 30. [1] 53221 abort src/bitcoind
The same behavior is observed on master as well. This PR does not change it.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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?
Fixed.
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?
Please see #15864 (comment)
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.
I think we could replace the directory creation with an assertion (that datadir already exists) and create the directory right before the first call to GetDataDir() - probably on init.
@ryanofsky All your comments have been addressed.
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=''
Then, why is this changed?
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.
@MarcoFalke would you mind reviewing this PR?
Concept ACK
utACK 4f0de6214ae591bfb37ef745140f45fcc97aec33. No changes since last review except rebase and replacing fprintf with tfm::format.
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 | + }
Would be nice if each commit had a test that failed before compilation and passes after. That might simplify review, no?
Do tests enabled in 5124b1cef24a9b30876ddedf922dfdb555bd4ace do this job?
Can they be enabled in this commit (the first commit)?
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?
utACK 5f55360ea36487131a14dbc0a2518ec3babc1580. No changes since last review other than rebase and comment tweak.
Rebased.
The 09a307183aca7b2f6e1a14caf4430a1fc546e704 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?
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" ?
Milestone
0.19.0