diff options
author | Andrew Chow <github@achow101.com> | 2023-02-23 16:29:49 -0500 |
---|---|---|
committer | Andrew Chow <github@achow101.com> | 2023-02-23 16:41:14 -0500 |
commit | 1258af40c0d396dd922cc8dc1b4c1719b39a4026 (patch) | |
tree | 42e15fb83567f3af6a723219fb2c336e9f543ea8 | |
parent | c033720b2bc7aabf703a686e0fc024c2d25681c6 (diff) | |
parent | 64c105442ce8c10900ea6fbecdbcfebe42f2d387 (diff) | |
download | bitcoin-1258af40c0d396dd922cc8dc1b4c1719b39a4026.tar.xz |
Merge bitcoin/bitcoin#27073: Convert ArgsManager::GetDataDir to a read-only function
64c105442ce8c10900ea6fbecdbcfebe42f2d387 util: make GetDataDir read-only & create datadir.. (willcl-ark)
56e370fbb9413260723c598048392219b1055ad0 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 64c105442ce8c10900ea6fbecdbcfebe42f2d387
hebasto:
re-ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387, only comments have been adjusted as requsted since my previous [review](https://github.com/bitcoin/bitcoin/pull/27073#pullrequestreview-1307435890).
TheCharlatan:
Re-ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387
ryanofsky:
Code review ACK 64c105442ce8c10900ea6fbecdbcfebe42f2d387. Only comment changes since last review
Tree-SHA512: b129501346071ad62551c9714492b21536d0558a94117d97218e255ef4e948d00df899a4bc2788faea27d3b1f20fc6136ef9d03e6a08498d926d9ad8688d6c96
-rw-r--r-- | src/qt/bitcoin.cpp | 1 | ||||
-rw-r--r-- | src/util/system.cpp | 41 | ||||
-rw-r--r-- | src/util/system.h | 12 |
3 files changed, 42 insertions, 12 deletions
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 59f433749d..c383c8bd58 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -167,6 +167,7 @@ static void initTranslations(QTranslator &qtTranslatorBase, QTranslator &qtTrans static bool InitSettings() { + gArgs.EnsureDataDir(); if (!gArgs.GetSettingsPath()) { return true; // Do nothing if settings file disabled. } diff --git a/src/util/system.cpp b/src/util/system.cpp index e72c970157..77b659df7e 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -417,8 +417,7 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const LOCK(cs_args); fs::path& path = net_specific ? m_cached_network_datadir_path : m_cached_datadir_path; - // Cache the path to avoid calling fs::create_directories on every call of - // this function + // Used cached path if available if (!path.empty()) return path; const fs::path datadir{GetPathArg("-datadir")}; @@ -432,20 +431,34 @@ const fs::path& ArgsManager::GetDataDir(bool net_specific) const path = GetDefaultDataDir(); } - if (!fs::exists(path)) { - fs::create_directories(path / "wallets"); - } - if (net_specific && !BaseParams().DataDir().empty()) { path /= fs::PathFromString(BaseParams().DataDir()); - if (!fs::exists(path)) { - fs::create_directories(path / "wallets"); - } } return path; } +void ArgsManager::EnsureDataDir() const +{ + /** + * "/wallets" subdirectories are created in all **new** + * datadirs, because wallet code will create new wallets in the "wallets" + * subdirectory only if exists already, otherwise it will create them in + * the top-level datadir where they could interfere with other files. + * Wallet init code currently avoids creating "wallets" directories itself + * for backwards compatibility, but this be changed in the future and + * wallet code here could go away (#16220). + */ + auto path{GetDataDir(false)}; + if (!fs::exists(path)) { + fs::create_directories(path / "wallets"); + } + path = GetDataDir(true); + if (!fs::exists(path)) { + fs::create_directories(path / "wallets"); + } +} + void ArgsManager::ClearPathCache() { LOCK(cs_args); @@ -491,6 +504,7 @@ bool ArgsManager::IsArgSet(const std::string& strArg) const bool ArgsManager::InitSettings(std::string& error) { + EnsureDataDir(); if (!GetSettingsPath()) { return true; // Do nothing if settings file disabled. } @@ -965,6 +979,11 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file return true; } +fs::path ArgsManager::GetConfigFilePath() const +{ + return GetConfigFile(GetPathArg("-conf", BITCOIN_CONF_FILENAME)); +} + bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) { { @@ -973,8 +992,8 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) m_config_sections.clear(); } - const fs::path conf_path = GetPathArg("-conf", BITCOIN_CONF_FILENAME); - std::ifstream stream{GetConfigFile(conf_path)}; + const auto conf_path{GetConfigFilePath()}; + std::ifstream stream{conf_path}; // not ok to have a config file specified that cannot be opened if (IsArgSet("-conf") && !stream.good()) { diff --git a/src/util/system.h b/src/util/system.h index c053adf8c3..14f093501a 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -242,6 +242,11 @@ protected: void SelectConfigNetwork(const std::string& network); [[nodiscard]] bool ParseParameters(int argc, const char* const argv[], std::string& error); + + /** + * Return config file path (read-only) + */ + fs::path GetConfigFilePath() const; [[nodiscard]] bool ReadConfigFiles(std::string& error, bool ignore_invalid_keys = false); /** @@ -475,13 +480,18 @@ protected: */ void LogArgs() const; + /** + * If datadir does not exist, create it along with wallets/ + * subdirectory(s). + */ + void EnsureDataDir() const; + private: /** * Get data directory path * * @param net_specific Append network identifier to the returned path * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned - * @post Returned directory path is created unless it is empty */ const fs::path& GetDataDir(bool net_specific) const; |