Fix datadir handling #15864

hebasto · 2019-04-21T20:44:10Z

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:

promag · 2019-04-21T22:45:49Z

src/util/system.cpp

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.

promag · 2019-04-21T22:49:01Z

src/util/system.cpp

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.

hebasto · 2019-04-22T04:19:44Z

@promag Thank you for your review.
To be clear, the premature address to the default datadir, until possible -datadir option is read from the config file(s), is the root of all issues fixed by this PR.

promag · 2019-04-22T10:01:07Z

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

hebasto · 2019-04-22T17:38:56Z

@promag

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.

DrahtBot · 2019-04-28T11:47:10Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16224 (gui: Bilingual GUI error messages by hebasto)

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.

ryanofsky

This comment doesn't seem to be true: #15864 (comment)

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 · 2019-04-29T15:06:04Z

src/util/system.cpp

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.

hebasto · 2019-04-29T20:58:58Z

@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?

ryanofsky

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:

Whether caching datadir is a useful optimization
Whether it makes sense to interpret a non-absolute "-conf" argument relative to the datadir

Both of these seem questionable to begin with.

src/util/system.cpp

ryanofsky · 2019-05-07T18:50:25Z

src/util/system.cpp

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", "")))

ryanofsky · 2019-05-07T19:01:21Z

src/util/system.cpp

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()

src/bitcoind.cpp

hebasto · 2019-05-09T15:30:04Z

@ryanofsky

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.

promag · 2019-05-09T16:35:33Z

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.

hebasto · 2019-05-09T18:23:43Z

@promag

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.

Is it a bit out of the scope of this PR?

hebasto · 2019-05-09T18:24:57Z

@ryanofsky
All your comments have been addressed.

ryanofsky

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.

ryanofsky · 2019-05-09T19:37:14Z

src/util/system.cpp

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 -nodatadir or -datadir='' options on the command line to override any datadir specified in the config file and reset it to default.

Unfortunately, ~~neither~~ -nodatadir ~~nor -datadir=''~~ on the command line cannot unset datadir specified in the config file or reset it to default. See: #16097.

Then, why is this changed?

The initial idea was:

I don't think IsArgSet is the right function to call here.

If we recall:

bitcoin/src/util/system.cpp

Lines 470 to 474 in 8c69fae

bool ArgsManager::IsArgSet(const std::string& strArg) const

{

if (IsArgNegated(strArg)) return true; // special case

return ArgsManagerHelper::GetArg(*this, strArg).first;

}

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).

@ryanofsky @MarcoFalke

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.

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.

hebasto · 2019-06-16T22:57:45Z

@MarcoFalke would you mind reviewing this PR?

hebasto · 2019-06-17T15:56:07Z

Rebased after #16205 merged.

maflcko · 2019-06-17T17:18:57Z

Concept ACK

ryanofsky

utACK 4f0de6214ae591bfb37ef745140f45fcc97aec33. No changes since last review except rebase and replacing fprintf with tfm::format.

hebasto · 2019-06-28T19:36:28Z

Rebased on top of #16300.

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.

All other clients and tools use CheckDataDirOption() rather fs::is_directory(GetDataDir(false)) for the first datadir check.

hebasto · 2019-07-24T16:11:30Z

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.

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

hebasto · 2019-10-17T13:52:16Z

@MarcoFalke #15240 (comment)

Label "Needs backport 0.18" ?

Summary: **Pull Request#16366** fa6f402bde146f92ed131e0c9c8e15a55e723307 Call node->initError instead of InitError from GUI code (Russell Yanofsky) fad2502240a1c440ef03ac3f880475702e418275 init: Use InitError for all errors in bitcoind/qt (MarcoFalke) Description: Using the same InitError for startup error in the daemon and the gui makes it possible to run the tests with the gui again: (N.f.B: Depends on D5794) ```sh BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args ``` --- **Pull Request#15864** ffea41f5301d5582665cf10ba5c2b9547a1443de Enable all tests in feature_config_args.py (Hennadii Stepanov) Description: Fix broken `feature_config_args.py` tests (disabled by MarcoFalke@fabe28a0cdcfa13e0e595a0905e3642a960d3077). All test are enabled now. --- This is a backport of Core [[bitcoin/bitcoin#16366 | PR16366]] with a partial backport of core [[bitcoin/bitcoin#15864 | PR15864]] Test Plan: ninja check check-functional Reviewers: deadalnix, #bitcoin_abc Reviewed By: deadalnix, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D5807

Summary: Merge #15864: Fix datadir handling ~~ffea41f5301d5582665cf10ba5c2b9547a1443de Enable all tests in feature_config_args.py (Hennadii Stepanov)~~ (N.f.B. backported in D5807) 66f5c17f8a3fe06fc65191e379ffc04e43cbbc86 Use CheckDataDirOption() for code uniformity (Hennadii Stepanov) 7e33a18a34b1a9b0f115076c142661d6d30c0585 Fix datadir handling in bitcoin-cli (Hennadii Stepanov) b28dada37465c0a773cf08b0e6766f0081bcb943 Fix datadir handling in bitcoin-qt (Hennadii Stepanov) 50824093bb2d68fe1393dfd636fab5f8795faa5d Fix datadir handling in bitcoind (Hennadii Stepanov) 740d41ce9f7fdf209366e930bd0fdcc6b1bc6b79 Add CheckDataDirOption() function (Hennadii Stepanov) c1f325126cf51d28dce8da74bfdf5cd05ab237ea Return absolute path early in AbsPathForConfigVal (Hennadii Stepanov) Pull request description: Fix #15240, see: bitcoin/bitcoin#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: - bitcoin/bitcoin#15745 (comment) by **laanwj** - #16220 --- Depends on D5807 This is a partial backport of Core [[bitcoin/bitcoin#15864 | PR15864]] Test Plan: ninja check check-functional Reviewers: deadalnix, #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5812

6358d70 Fix datadir handling in pivx-cli (Hennadii Stepanov) 8fbbd16 Fix datadir handling in pivx-qt (Hennadii Stepanov) aa397eb Fix datadir handling in pivxd (Hennadii Stepanov) 2862a75 Add CheckDataDirOption() function (Hennadii Stepanov) 3303376 Make PID file creating errors fatal (furszy) Pull request description: Backporting: * bitcoin#15278. * bitcoin#15864. ACKs for top commit: Fuzzbawls: ACK 6358d70 random-zebra: utACK 6358d70 Tree-SHA512: ad906fa2ae859d489b62a5a79830186fa6f252cc5fb88b1d628768d985fb93bd1d3c3eda38c83f77bb37f08a52df3278a78a4990de9e0d1dc29a6b77eb9209eb

hebasto mentioned this pull request Apr 21, 2019

bitcoind wont start with datadir specified in conf file and no home dir #15240

Closed

fanquake added the Utils/log/libs label Apr 21, 2019

promag reviewed Apr 21, 2019

View reviewed changes

hebasto mentioned this pull request Apr 22, 2019

unexpected behavior when specify datadir in config file #15745

Closed

hebasto mentioned this pull request Apr 23, 2019

Handle exception when creating default -datadir #15871

Closed

ryanofsky reviewed Apr 29, 2019

View reviewed changes

hebasto force-pushed the 20190421-datadir-handling branch from e7bf289 to c4ab742 Compare April 29, 2019 20:54

ryanofsky approved these changes May 7, 2019

View reviewed changes

hebasto force-pushed the 20190421-datadir-handling branch from c4ab742 to f8c9ef0 Compare May 9, 2019 15:47

hebasto force-pushed the 20190421-datadir-handling branch from f8c9ef0 to 9bddf12 Compare May 9, 2019 17:17

ryanofsky approved these changes May 9, 2019

View reviewed changes

hebasto mentioned this pull request Jun 16, 2019

odd behaviour of GetDataDir creating wallets/ subdirectory #16220

Closed

hebasto force-pushed the 20190421-datadir-handling branch from 9bddf12 to 4f0de62 Compare June 17, 2019 15:48

maflcko mentioned this pull request Jun 27, 2019

util: Remove code to cache datadir #16255

Closed

ryanofsky approved these changes Jun 27, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jun 28, 2019

hebasto force-pushed the 20190421-datadir-handling branch from 4f0de62 to 5f55360 Compare June 28, 2019 19:35

hebasto force-pushed the 20190421-datadir-handling branch 2 times, most recently from 1fcc102 to f346a65 Compare July 24, 2019 00:30

DrahtBot added the Needs rebase label Jul 24, 2019

hebasto changed the title ~~Fix datadir handling~~ WIP: Fix datadir handling Jul 24, 2019

hebasto added 3 commits July 24, 2019 18:42

Return absolute path early in AbsPathForConfigVal

c1f3251

This prevents premature GetDataDir() calls, e.g., when config file is not read yet.

Add CheckDataDirOption() function

740d41c

Fix datadir handling in bitcoind

5082409

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.

hebasto force-pushed the 20190421-datadir-handling branch from f346a65 to 5082409 Compare July 24, 2019 15:45

hebasto changed the title ~~WIP: Fix datadir handling~~ Fix datadir handling Jul 24, 2019

hebasto added 3 commits July 24, 2019 18:54

Fix datadir handling in bitcoin-qt

b28dada

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.

Fix datadir handling in bitcoin-cli

7e33a18

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.

Use CheckDataDirOption() for code uniformity

66f5c17

All other clients and tools use CheckDataDirOption() rather fs::is_directory(GetDataDir(false)) for the first datadir check.

Enable all tests in feature_config_args.py

ffea41f

hebasto force-pushed the 20190421-datadir-handling branch from c508635 to ffea41f Compare July 24, 2019 16:15

DrahtBot removed the Needs rebase label Jul 24, 2019

maflcko added this to the 0.19.0 milestone Aug 16, 2019

maflcko merged commit ffea41f into bitcoin:master Aug 19, 2019

hebasto deleted the 20190421-datadir-handling branch August 19, 2019 15:14

fanquake added the Needs backport (0.18) label Oct 17, 2019

ryanofsky mentioned this pull request Jan 9, 2020

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

fanquake removed the Needs backport (0.18) label Mar 13, 2020

furszy mentioned this pull request Jan 17, 2022

Improve PID file error and datadir handling PIVX-Project/PIVX#2726

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	bool ArgsManager::IsArgSet(const std::string& strArg) const
	{
	if (IsArgNegated(strArg)) return true; // special case
	return ArgsManagerHelper::GetArg(*this, strArg).first;
	}

Fix datadir handling #15864

Fix datadir handling #15864

Uh oh!

Conversation

hebasto commented Apr 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 22, 2019

Uh oh!

promag commented Apr 22, 2019

Uh oh!

hebasto commented Apr 22, 2019

Uh oh!

DrahtBot commented Apr 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 29, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented May 9, 2019

Uh oh!

hebasto commented May 9, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto May 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 16, 2019

Uh oh!

hebasto commented Apr 21, 2019 •

edited

Loading

hebasto Apr 22, 2019 •

edited

Loading

DrahtBot commented Apr 28, 2019 •

edited

Loading

hebasto commented May 9, 2019 •

edited

Loading

promag commented May 9, 2019 •

edited

Loading

hebasto May 26, 2019 •

edited

Loading

hebasto Jul 24, 2019 •

edited

Loading