Repository: bitcoin/bitcoin (language: cpp)

Issue:
bitcoin-cli needlessly creates empty ~/.bitcoin/wallets directory
When `bitcoin-cli` is run, **it, not bitcoind** creates an empty `~/.bitcoin/wallets` direcctory with `~` bring home of currently running user. This is weird and leads to user confusion because it happens even in cases when `bitcoin-cli` connects to a `bitcoind` running under a different user or probably on other machine.

**Expected behavior**

`bitcoin-cli` never modifies filesystem, especially not things that the daemon should take care of.

**Actual behavior**

`bitcoin-cli creates empty directory`

**To reproduce**

0. make sure `~/.bitcoin` does **not** exist under user A
1. install `bitcoind` and run it under a user B
2. run `bitcoin-cli getblockchaininfo` under user A
3. `ls ~/.bitcoin` under user A

For quick setup, you can try to use [my project](https://deb.ln-ask.me) in a Debian VM or using provided Docker file. Just `apt install bitcoin-regtest bitcoin-cli`, it's already pre-configured to run under a different user, so then just running `sudo bitcoin-cli getblockchaininfo && sudo ls /root/.bitcoin` should reproduce the issue.

**System information**

0.20 official release downloaded from `https://bitcoin.org/bin/bitcoin-core-...`, verified GPG signature from Wladimir.

OS: Linux Debian Buster
CPU: Intel x86_64
Disk: NVMe SSD
FS: ext4

I don't think configuration is relevant since `bitcoin-cli` is **not** informed where to find it (via parameters or otherwise) and thus it can **not** access it. I also added `-rpcport` and `-rpccookiefile` parameters explicitly but I don't think it's relevant.

Loosely related to #16220, but not a duplicate.


Discussion / hints:
Was able to reproduce and track it down to https://github.com/bitcoin/bitcoin/pull/11466, especially https://github.com/bitcoin/bitcoin/pull/11466/files#r150251905 (currently in `master` in https://github.com/bitcoin/bitcoin/blob/master/src/util/system.cpp#L722) :v:
Good find. My understanding is that the function is shared between daemon and client, that's why it happens. Not surprised, if the function is called `GetDatadir` but it doesn't just get the datadir, but also creates it. :)
>  I also added -rpcport and -rpccookiefile parameters explicitly but I don't think it's relevant.

As a temporary workaround, you can skip passing them and instead pass in the datadir to bitcoin-cli. It will then read those values from the conf file (or cookie file), but also skip creating the wallet dir (because it already exists).
Good idea. I can't skip them because of other reasons but adding `-datadir=/tmp/` disables creating the directory.

Fix patch:
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 59f433749d8a7..c383c8bd58faa 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 e72c9701571c1..77b659df7e8ee 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 c053adf8c3ef9..14f093501aafc 100644
--- a/src/util/system.h
+++ b/src/util/system.h
@@ -242,6 +242,11 @@ class ArgsManager
     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 @@ class ArgsManager
      */
     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;
 


Test patch: