Skip to content

Commit 1258af4

Browse files
committed
Merge bitcoin/bitcoin#27073: Convert ArgsManager::GetDataDir to a read-only function
64c1054 util: make GetDataDir read-only & create datadir.. (willcl-ark) 56e370f util: add ArgsManager datadir helper functions (willcl-ark) Pull request description: Fixes #20070 Currently `ArgsManager::GetDataDir()` ensures it will always return a datadir by creating one if necessary. The function is shared between `bitcoind` `bitcoin-qt` and `bitcoin-cli` which results in the undesirable behaviour described in #20070. This PR splits out the part of the function which creates directories and adds it as a standalone function, only called as part of `bitcoind` and `bitcoin-qt` init, but not `bitcoin-cli`. `ReadConfigFiles`' behavior is changed to use the absolute path of the config file in error and warning messages instead of a relative path. This was inadvertantly the form being tested [here](https://github.com/bitcoin/bitcoin/blob/73966f75f67fb797163f0a766292a79d4b2c1b70/test/functional/feature_config_args.py#L287), whilst we were _not_ testing that a relative path was returned by the message even though we passed a relative path in as argument. ACKs for top commit: achow101: ACK 64c1054 hebasto: re-ACK 64c1054, only comments have been adjusted as requsted since my previous [review](bitcoin/bitcoin#27073 (review)). TheCharlatan: Re-ACK 64c1054 ryanofsky: Code review ACK 64c1054. Only comment changes since last review Tree-SHA512: b129501346071ad62551c9714492b21536d0558a94117d97218e255ef4e948d00df899a4bc2788faea27d3b1f20fc6136ef9d03e6a08498d926d9ad8688d6c96
2 parents c033720 + 64c1054 commit 1258af4

File tree

3 files changed

+42
-12
lines changed

3 files changed

+42
-12
lines changed

src/qt/bitcoin.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ static void initTranslations(QTranslator &qtTranslatorBase, QTranslator &qtTrans
167167

168168
static bool InitSettings()
169169
{
170+
gArgs.EnsureDataDir();
170171
if (!gArgs.GetSettingsPath()) {
171172
return true; // Do nothing if settings file disabled.
172173
}

src/util/system.cpp

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -417,8 +417,7 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const
417417
LOCK(cs_args);
418418
fs::path& path = net_specific ? m_cached_network_datadir_path : m_cached_datadir_path;
419419

420-
// Cache the path to avoid calling fs::create_directories on every call of
421-
// this function
420+
// Used cached path if available
422421
if (!path.empty()) return path;
423422

424423
const fs::path datadir{GetPathArg("-datadir")};
@@ -432,20 +431,34 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const
432431
path = GetDefaultDataDir();
433432
}
434433

435-
if (!fs::exists(path)) {
436-
fs::create_directories(path / "wallets");
437-
}
438-
439434
if (net_specific && !BaseParams().DataDir().empty()) {
440435
path /= fs::PathFromString(BaseParams().DataDir());
441-
if (!fs::exists(path)) {
442-
fs::create_directories(path / "wallets");
443-
}
444436
}
445437

446438
return path;
447439
}
448440

441+
void ArgsManager::EnsureDataDir() const
442+
{
443+
/**
444+
* "/wallets" subdirectories are created in all **new**
445+
* datadirs, because wallet code will create new wallets in the "wallets"
446+
* subdirectory only if exists already, otherwise it will create them in
447+
* the top-level datadir where they could interfere with other files.
448+
* Wallet init code currently avoids creating "wallets" directories itself
449+
* for backwards compatibility, but this be changed in the future and
450+
* wallet code here could go away (#16220).
451+
*/
452+
auto path{GetDataDir(false)};
453+
if (!fs::exists(path)) {
454+
fs::create_directories(path / "wallets");
455+
}
456+
path = GetDataDir(true);
457+
if (!fs::exists(path)) {
458+
fs::create_directories(path / "wallets");
459+
}
460+
}
461+
449462
void ArgsManager::ClearPathCache()
450463
{
451464
LOCK(cs_args);
@@ -491,6 +504,7 @@ bool ArgsManager::IsArgSet(const std::string& strArg) const
491504

492505
bool ArgsManager::InitSettings(std::string& error)
493506
{
507+
EnsureDataDir();
494508
if (!GetSettingsPath()) {
495509
return true; // Do nothing if settings file disabled.
496510
}
@@ -965,6 +979,11 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file
965979
return true;
966980
}
967981

982+
fs::path ArgsManager::GetConfigFilePath() const
983+
{
984+
return GetConfigFile(GetPathArg("-conf", BITCOIN_CONF_FILENAME));
985+
}
986+
968987
bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
969988
{
970989
{
@@ -973,8 +992,8 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
973992
m_config_sections.clear();
974993
}
975994

976-
const fs::path conf_path = GetPathArg("-conf", BITCOIN_CONF_FILENAME);
977-
std::ifstream stream{GetConfigFile(conf_path)};
995+
const auto conf_path{GetConfigFilePath()};
996+
std::ifstream stream{conf_path};
978997

979998
// not ok to have a config file specified that cannot be opened
980999
if (IsArgSet("-conf") && !stream.good()) {

src/util/system.h

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,11 @@ class ArgsManager
242242
void SelectConfigNetwork(const std::string& network);
243243

244244
[[nodiscard]] bool ParseParameters(int argc, const char* const argv[], std::string& error);
245+
246+
/**
247+
* Return config file path (read-only)
248+
*/
249+
fs::path GetConfigFilePath() const;
245250
[[nodiscard]] bool ReadConfigFiles(std::string& error, bool ignore_invalid_keys = false);
246251

247252
/**
@@ -475,13 +480,18 @@ class ArgsManager
475480
*/
476481
void LogArgs() const;
477482

483+
/**
484+
* If datadir does not exist, create it along with wallets/
485+
* subdirectory(s).
486+
*/
487+
void EnsureDataDir() const;
488+
478489
private:
479490
/**
480491
* Get data directory path
481492
*
482493
* @param net_specific Append network identifier to the returned path
483494
* @return Absolute path on success, otherwise an empty path when a non-directory path would be returned
484-
* @post Returned directory path is created unless it is empty
485495
*/
486496
const fs::path& GetDataDir(bool net_specific) const;
487497

0 commit comments

Comments
 (0)