Check for datadir after the config files were read #13621

Flowdalic · 2018-07-10T06:34:51Z

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.

maflcko · 2018-07-11T16:05:20Z

Doesn't the location of the default config file depend on the datadir?

Flowdalic · 2018-07-11T17:37:14Z

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?

maflcko · 2018-07-11T18:41:03Z

See https://dev.visucore.com/bitcoin/doxygen/class_args_manager.html#aec848fd54ace0df3df48dac446feb7d2 which calls GetDataDir

AtsukiTak · 2018-07-12T03:53:22Z

Now the second if clause seems meaningless.

https://github.com/Flowdalic/bitcoin/blob/94a038b8694229c0153ede48a53701424ca51e38/src/bitcoind.cpp#L98-L102

Because it already checked in ReadConfigFiles.

bitcoin/src/util.cpp

Lines 909 to 912 in 4a7e64f

    
           if (!fs::is_directory(GetDataDir(false))) { 
        
               error = strprintf("specified data directory \"%s\" does not exist.", gArgs.GetArg("-datadir", "").c_str()); 
        
               return false; 
        
           }

Flowdalic · 2018-07-12T05:54:35Z

Right, the check can get probably removed.

DrahtBot · 2018-07-17T16:17:54Z

Reviewers, this pull request conflicts with the following ones:

utils and libraries: Make 'blocksdir' always net specific #14409 (utils and libraries: Make 'blocksdir' always net specific by hebasto)
qa: Run more tests with wallet disabled #14324 (qa: Run more tests with wallet disabled by MarcoFalke)
utils: Add fstream wrapper to allow to pass unicode filename on Windows #13878 (utils: Add fstream wrapper to allow to pass unicode filename on Windows by ken2812221)
-masterdatadir for datadir bootstrapping #13746 (-masterdatadir for datadir bootstrapping by kallewoof)

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 fabe28a ("qa: Temporarily disable test that reads the default datadir location"), as those tests do not touch "$HOME/.bitcoin" any more.

Flowdalic · 2018-07-19T19:43:31Z

Rebased. Also fixed the cause and re-enabled the tests which were disabled in #13687 with fabe28a.

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.

AtsukiTak · 2018-07-20T17:15:10Z

src/bitcoin-cli.cpp

    if (!gArgs.ReadConfigFiles(error, true)) {
        fprintf(stderr, "Error reading configuration file: %s\n", error.c_str());
        return EXIT_FAILURE;
    }
+    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.

AtsukiTak · 2018-07-20T17:28:35Z

Nice work :)
But this PR seems to contains lots of changes for me such as

Check for datadir after the config files were read
Create directory if user specify non exist directory
Do not fallback to empy path in GetDataDir()

I think you should leave only first one, and move rests to another PR, and then discuss about them.

AtsukiTak · 2018-07-21T18:43:56Z

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.

leishman · 2018-08-24T23:11:07Z

I don't think our PRs conflict, but just wanted to make you aware of a minor change I'm putting in the logging behavior: #14057

I'm also working on logic to create a bitcoin.conf.example file: #13761

…eadconf

Flowdalic · 2018-09-07T11:10:57Z

Pinging @MarcoFalke since this PR re-enables tests he disabled in fabe28a (#13687). Would be great to hear your thoughts on this.

maflcko · 2018-09-07T12:16:21Z

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.

maflcko · 2018-09-07T12:17:32Z

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.

DrahtBot · 2018-10-08T06:07:36Z

Needs rebase

fanquake · 2019-01-05T06:06:56Z

Closing "up for grabs".

ffea41f Enable all tests in feature_config_args.py (Hennadii Stepanov) 66f5c17 Use CheckDataDirOption() for code uniformity (Hennadii Stepanov) 7e33a18 Fix datadir handling in bitcoin-cli (Hennadii Stepanov) b28dada Fix datadir handling in bitcoin-qt (Hennadii Stepanov) 5082409 Fix datadir handling in bitcoind (Hennadii Stepanov) 740d41c Add CheckDataDirOption() function (Hennadii Stepanov) c1f3251 Return absolute path early in AbsPathForConfigVal (Hennadii Stepanov) Pull request description: 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: - #15745 (comment) by **laanwj** - #16220 Top commit has no ACKs. Tree-SHA512: 4a4cda10e0b67c8f374da0c9567003d2b566d948e7f8550fe246868b5794c15010e88ea206009480b9cd2f737f310a15e984f920730448f99a895893bed351df

ffea41f Enable all tests in feature_config_args.py (Hennadii Stepanov) 66f5c17 Use CheckDataDirOption() for code uniformity (Hennadii Stepanov) 7e33a18 Fix datadir handling in bitcoin-cli (Hennadii Stepanov) b28dada Fix datadir handling in bitcoin-qt (Hennadii Stepanov) 5082409 Fix datadir handling in bitcoind (Hennadii Stepanov) 740d41c Add CheckDataDirOption() function (Hennadii Stepanov) c1f3251 Return absolute path early in AbsPathForConfigVal (Hennadii Stepanov) Pull request description: Fix bitcoin#15240, see: bitcoin#15240 (comment) Fix bitcoin#15745 Fix broken `feature_config_args.py` tests (disabled by MarcoFalke@fabe28a0cdcfa13e0e595a0905e3642a960d3077). All test are enabled now. This PR is alternative to bitcoin#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: - bitcoin#15745 (comment) by **laanwj** - bitcoin#16220 Top commit has no ACKs. Tree-SHA512: 4a4cda10e0b67c8f374da0c9567003d2b566d948e7f8550fe246868b5794c15010e88ea206009480b9cd2f737f310a15e984f920730448f99a895893bed351df

Flowdalic mentioned this pull request Jul 10, 2018

Update bitcoin.service to conform to init.md #12255

Merged

fanquake added the Refactoring label Jul 10, 2018

fanquake changed the title ~~Check for datadir after the config files where read~~ Check for datadir after the config files were read Jul 10, 2018

Flowdalic force-pushed the init-swap-datadir-readconf branch 2 times, most recently from 6bcdbd3 to 94a038b Compare July 10, 2018 07:30

maflcko added Utils/log/libs and removed Refactoring labels Jul 10, 2018

DrahtBot mentioned this pull request Jul 17, 2018

travis: Check that ~/.bitcoin is never created #13687

Merged

DrahtBot added the Needs rebase label Jul 18, 2018

Flowdalic force-pushed the init-swap-datadir-readconf branch 6 times, most recently from be2ba85 to 9b1987f Compare July 19, 2018 15:47

Flowdalic added 4 commits July 19, 2018 20:33

Do not create datadir when calling GetConfigFile

dd906c1

As the real datadir may be set in the configuration file, this would case the default datadir to get created.

Show the datadir that was checked in error message

b6a43df

Flowdalic force-pushed the init-swap-datadir-readconf branch from 0717367 to b6a43df Compare July 19, 2018 19:31

DrahtBot removed the Needs rebase label Jul 20, 2018

AtsukiTak reviewed Jul 20, 2018

View reviewed changes

DrahtBot mentioned this pull request Jul 30, 2018

Ignore unknown config file options; warn instead of error #13799

Merged

DrahtBot added the Needs rebase label Jul 31, 2018

Merge branch 'master' into init-swap-datadir-readconf

e8ca79d

DrahtBot removed the Needs rebase label Aug 3, 2018

DrahtBot mentioned this pull request Aug 4, 2018

utils: Add fstream wrapper to allow to pass unicode filename on Windows #13878

Merged

Merge branch 'master' into init-swap-datadir-readconf

bbd458c

Merge remote-tracking branch 'origin/master' into init-swap-datadir-r…

0053ea2

…eadconf

DrahtBot mentioned this pull request Sep 26, 2018

qa: Run more tests with wallet disabled #14324

Merged

DrahtBot mentioned this pull request Oct 5, 2018

utils and libraries: Make 'blocksdir' always net specific #14409

Merged

DrahtBot added the Needs rebase label Oct 8, 2018

fanquake added the Up for grabs label Jan 5, 2019

fanquake closed this Jan 5, 2019

hebasto mentioned this pull request Apr 21, 2019

Fix datadir handling #15864

Merged

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for datadir after the config files were read #13621

Check for datadir after the config files were read #13621

Flowdalic commented Jul 10, 2018 •

edited

maflcko commented Jul 11, 2018

Flowdalic commented Jul 11, 2018 •

edited

maflcko commented Jul 11, 2018

AtsukiTak commented Jul 12, 2018

Flowdalic commented Jul 12, 2018

DrahtBot commented Jul 17, 2018 •

edited

Flowdalic commented Jul 19, 2018

AtsukiTak Jul 20, 2018

Flowdalic Jul 21, 2018

AtsukiTak Jul 21, 2018

AtsukiTak Jul 21, 2018

AtsukiTak commented Jul 20, 2018

AtsukiTak commented Jul 21, 2018

leishman commented Aug 24, 2018

Flowdalic commented Sep 7, 2018 •

edited

maflcko commented Sep 7, 2018

maflcko commented Sep 7, 2018 •

edited

DrahtBot commented Oct 8, 2018

fanquake commented Jan 5, 2019

Check for datadir after the config files were read #13621

Check for datadir after the config files were read #13621

Conversation

Flowdalic commented Jul 10, 2018 • edited

maflcko commented Jul 11, 2018

Flowdalic commented Jul 11, 2018 • edited

maflcko commented Jul 11, 2018

AtsukiTak commented Jul 12, 2018

Flowdalic commented Jul 12, 2018

DrahtBot commented Jul 17, 2018 • edited

Flowdalic commented Jul 19, 2018

AtsukiTak Jul 20, 2018

Choose a reason for hiding this comment

Flowdalic Jul 21, 2018

Choose a reason for hiding this comment

AtsukiTak Jul 21, 2018

Choose a reason for hiding this comment

AtsukiTak Jul 21, 2018

Choose a reason for hiding this comment

AtsukiTak commented Jul 20, 2018

AtsukiTak commented Jul 21, 2018

leishman commented Aug 24, 2018

Flowdalic commented Sep 7, 2018 • edited

maflcko commented Sep 7, 2018

maflcko commented Sep 7, 2018 • edited

DrahtBot commented Oct 8, 2018

fanquake commented Jan 5, 2019

Flowdalic commented Jul 10, 2018 •

edited

Flowdalic commented Jul 11, 2018 •

edited

DrahtBot commented Jul 17, 2018 •

edited

Flowdalic commented Sep 7, 2018 •

edited

maflcko commented Sep 7, 2018 •

edited