If we first check for datadir existence and then read the config file we bail out in situations where the default datadir is invalid, e.g. because of an non-existing HOME, but the datadir specified in config files is valid. We should not do that, as the user may specified a valid datadir in the config file and expects the setting to take effect.
See for example #12255 (comment) where users hit this.
Doesn't the location of the default config file depend on the datadir?
Doesn't the location of the default config file depend on the datadir?
Yes, BITCOIN_CONF_FILENAME, which is set to bitcoin.conf is feed into AbsPathForConfigVal() which uses the datadir setting. However, if no datadir is explicitly set it defaults to "". I think that this means that bitcoin.conf is opened relative to the processe's working directory.
It appears that you have a specific problem in mind, which I don't see. Care to elaborate?
See https://dev.visucore.com/bitcoin/doxygen/class_args_manager.html#aec848fd54ace0df3df48dac446feb7d2 which calls GetDataDir
Now the second if clause seems meaningless.
Because it already checked in ReadConfigFiles.
Right, the check can get probably removed.
<!--e57a25ab6845829454e8d69fc972939a-->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.
As the real datadir may be set in the configuration file, this would
case the default datadir to get created.
If datadir is not a directory, do not fallback to empty path as this would shadow the original path set in the config file. Consider
bitcoin.conf:
datadir=/does/not/exist
and bitcoind started with "-conf=bitcoin.conf", which would previusly yield
Error: Specified data directory "" does not exist.
when it should state
Error: Specified data directory "/does/not/exist" does not exist.
If we first check for datadir existence and then read the config file
we bail out in situations where the default datadir is invalid,
e.g. because of an non-existing HOME, but the datadir specified in
config files is valid. We should not do that, as the user specified a
valid datadir and expects the setting to take effect.
Also remove datadir check in ReadConfigFiles() which now became
superfluous: We check for the datadir in AppInit() already, there is
no need to check also in ReadConfigFiles().
Furthermore re-enable the disabled tests in feature_config_args.py,
which where disabled in fabe28a0cdc ("qa: Temporarily disable test
that reads the default datadir location"), as those tests do not touch
"$HOME/.bitcoin" any more.Rebased. Also fixed the cause and re-enabled the tests which were disabled in #13687 with fabe28a0cdcfa13e0e595a0905e3642a960d3077.
I had a hard time working on the argument and configuration file parsing code. It is a little bit spagettish and some parts are messy.
I used default argument values in order to optionally change GetDataDir() to not create the data directory, which, I assumed, made the change less intrusive. But I am happy to change it towards a GetDataDir() / GetDataDirAndCreateIfNecessary(). I am undecied which of those two approaches would be better.
127 | - } 128 | if (!gArgs.ReadConfigFiles(error, true)) { 129 | fprintf(stderr, "Error reading configuration file: %s\n", error.c_str()); 130 | return EXIT_FAILURE; 131 | } 132 | + if (!fs::is_directory(GetDataDir(false))) {
It seems never fail unless user specify unpermitted path.
It fails if the specified path is not a directory. I think that is sensible and was already previous behavior. Or do have a specific change request?
If I specify a file, boost::filesystem::create_directory throw an error.
If I specify a non-exist path, a new directory is created and no error happens.
I think you just missed second argument; GetDataDir(false, false) might be what you want.
Sorry for my poor English. Please feel free to ask me if my English is not clear.
Nice work :) But this PR seems to contains lots of changes for me such as
I think you should leave only first one, and move rests to another PR, and then discuss about them.
I think you should leave only first one
Well, those commits depend on each other: For example I wouldn't be able to re-enable the relevant tests of "Check for datadir after the config files were read" without the previous commits.
I don't feel like that this patch series is worth splitting up, as it is pretty small, but I am happy create single PRs for each commit one after another, if there is a consensus that this would be favorable.
I don't feel like that this patch series is worth splitting up, as it is pretty small
I see. So please don't care about that. That was just newbie's nonsense comment.
Pinging @MarcoFalke since this PR re-enables tests he disabled in fabe28a0cdcfa13e0e595a0905e3642a960d3077 (#13687). Would be great to hear your thoughts on this.
I haven't looked at the most recent code, but I still think that the location of the default config file depends on the datadir? See https://dev.visucore.com/bitcoin/doxygen/class_args_manager.html#aec848fd54ace0df3df48dac446feb7d2 which calls GetDataDir.
So you are changing how and what config files are read. Not sure if we want this behavior change without discussion first.
Also, you modified how bitcoind and bitcoin-cli work, but not the gui. Wouldn't that lead to issues when the gui is used as rpc server via the bitcoin-cli?
So if you really want to go ahead with these changes, they need tests that fail on master but pass with your fixes for bitcoind as well as the bitcoin-qt and bitcoin-cli.
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
Closing "up for grabs".