Merge bitcoin/bitcoin#27302: init: Error if ignored bitcoin.conf file is found

eefe56967b4eb4b5144325cde4f40fc1cbde3e65 bugfix: Fix incorrect debug.log config file path (Ryan Ofsky)
3746f00be1b732a04976fc70cbb0661f97bbbd99 init: Error if ignored bitcoin.conf file is found (Ryan Ofsky)
398c3719b02197ad92fded20f6ff83b364747297 lint: Fix lint-format-strings false positives when format specifiers have argument positions (Ryan Ofsky)

Pull request description:

  Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen:

  - One case reported in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043) happens when a `bitcoin.conf` file in the default datadir (e.g. `$HOME/.bitcoin/bitcoin.conf`) has a `datadir=/path` line that sets different datadir containing a second `bitcoin.conf` file. Currently the second `bitcoin.conf` file is ignored with no warning.

  - Another way this could happen is if a `-conf=` command line argument points to a configuration file with a `datadir=/path` line and that path contains a `bitcoin.conf` file, which is currently ignored.

  This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant `-datadir` or `-conf` settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored.

ACKs for top commit:
  pinheadmz:
    re-ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
  willcl-ark:
    re-ACK eefe56967b
  TheCharlatan:
    ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65

Tree-SHA512: 939a98a4b271b5263d64a2df3054c56fcde94784edf6f010d78693a371c38aa03138ae9cebb026b6164bbd898d8fd0845a61a454fd996e328fd7bcf51c580c2b

6 files changed, 49 insertions, 8 deletions

diff --git a/src/common/args.cpp b/src/common/args.cpp
index 93f2005931..c9af2d7f5e 100644
--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -716,7 +716,8 @@ bool CheckDataDirOption(const ArgsManager& args)
 
 fs::path ArgsManager::GetConfigFilePath() const
 {
-    return GetConfigFile(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME));
+    LOCK(cs_args);
+    return *Assert(m_config_path);
 }
 
 ChainType ArgsManager::GetChainType() const
diff --git a/src/common/args.h b/src/common/args.h
index 537a64fcfd..7569297a74 100644
--- a/src/common/args.h
+++ b/src/common/args.h
@@ -28,7 +28,6 @@ extern const char * const BITCOIN_SETTINGS_FILENAME;
 
 // Return true if -datadir option points to a valid directory or is not specified.
 bool CheckDataDirOption(const ArgsManager& args);
-fs::path GetConfigFile(const ArgsManager& args, const fs::path& configuration_file_path);
 
 /**
  * Most paths passed as configuration arguments are treated as relative to
@@ -138,6 +137,7 @@ protected:
     std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);
     bool m_accept_any_command GUARDED_BY(cs_args){true};
     std::list<SectionInfo> m_config_sections GUARDED_BY(cs_args);
+    std::optional<fs::path> m_config_path GUARDED_BY(cs_args);
     mutable fs::path m_cached_blocks_path GUARDED_BY(cs_args);
     mutable fs::path m_cached_datadir_path GUARDED_BY(cs_args);
     mutable fs::path m_cached_network_datadir_path GUARDED_BY(cs_args);
diff --git a/src/common/config.cpp b/src/common/config.cpp
index 5efb5efb67..e25b4fe2df 100644
--- a/src/common/config.cpp
+++ b/src/common/config.cpp
@@ -27,11 +27,6 @@
 #include <utility>
 #include <vector>
 
-fs::path GetConfigFile(const ArgsManager& args, const fs::path& configuration_file_path)
-{
-    return AbsPathForConfigVal(args, configuration_file_path, /*net_specific=*/false);
-}
-
 static bool GetConfigOptions(std::istream& stream, const std::string& filepath, std::string& error, std::vector<std::pair<std::string, std::string>>& options, std::list<SectionInfo>& sections)
 {
     std::string str, prefix;
@@ -126,6 +121,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
         LOCK(cs_args);
         m_settings.ro_config.clear();
         m_config_sections.clear();
+        m_config_path = AbsPathForConfigVal(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false);
     }
 
     const auto conf_path{GetConfigFilePath()};
@@ -176,7 +172,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
             const size_t default_includes = add_includes({});
 
             for (const std::string& conf_file_name : conf_file_names) {
-                std::ifstream conf_file_stream{GetConfigFile(*this, fs::PathFromString(conf_file_name))};
+                std::ifstream conf_file_stream{AbsPathForConfigVal(*this, fs::PathFromString(conf_file_name), /*net_specific=*/false)};
                 if (conf_file_stream.good()) {
                     if (!ReadConfigStream(conf_file_stream, conf_file_name, error, ignore_invalid_keys)) {
                         return false;
diff --git a/src/common/init.cpp b/src/common/init.cpp
index 8933d59c27..412d73aec7 100644
--- a/src/common/init.cpp
+++ b/src/common/init.cpp
@@ -5,6 +5,7 @@
 #include <chainparams.h>
 #include <common/args.h>
 #include <common/init.h>
+#include <logging.h>
 #include <tinyformat.h>
 #include <util/fs.h>
 #include <util/translation.h>
@@ -20,6 +21,19 @@ std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn setting
         if (!CheckDataDirOption(args)) {
             return ConfigError{ConfigStatus::FAILED, strprintf(_("Specified data directory \"%s\" does not exist."), args.GetArg("-datadir", ""))};
         }
+
+        // Record original datadir and config paths before parsing the config
+        // file. It is possible for the config file to contain a datadir= line
+        // that changes the datadir path after it is parsed. This is useful for
+        // CLI tools to let them use a different data storage location without
+        // needing to pass it every time on the command line. (It is not
+        // possible for the config file to cause another configuration to be
+        // used, though. Specifying a conf= option in the config file causes a
+        // parse error, and specifying a datadir= location containing another
+        // bitcoin.conf file just ignores the other file.)
+        const fs::path orig_datadir_path{args.GetDataDirBase()};
+        const fs::path orig_config_path{AbsPathForConfigVal(args, args.GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false)};
+
         std::string error;
         if (!args.ReadConfigFiles(error, true)) {
             return ConfigError{ConfigStatus::FAILED, strprintf(_("Error reading configuration file: %s"), error)};
@@ -48,6 +62,32 @@ std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn setting
             fs::create_directories(net_path / "wallets");
         }
 
+        // Show an error or warning if there is a bitcoin.conf file in the
+        // datadir that is being ignored.
+        const fs::path base_config_path = base_path / BITCOIN_CONF_FILENAME;
+        if (fs::exists(base_config_path) && !fs::equivalent(orig_config_path, base_config_path)) {
+            const std::string cli_config_path = args.GetArg("-conf", "");
+            const std::string config_source = cli_config_path.empty()
+                ? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path)))
+                : strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path));
+            const std::string error = strprintf(
+                "Data directory %1$s contains a %2$s file which is ignored, because a different configuration file "
+                "%3$s from %4$s is being used instead. Possible ways to address this would be to:\n"
+                "- Delete or rename the %2$s file in data directory %1$s.\n"
+                "- Change datadir= or conf= options to specify one configuration file, not two, and use "
+                "includeconf= to include any other configuration files.\n"
+                "- Set allowignoredconf=1 option to treat this condition as a warning, not an error.",
+                fs::quoted(fs::PathToString(base_path)),
+                fs::quoted(BITCOIN_CONF_FILENAME),
+                fs::quoted(fs::PathToString(orig_config_path)),
+                config_source);
+            if (args.GetBoolArg("-allowignoredconf", false)) {
+                LogPrintf("Warning: %s\n", error);
+            } else {
+                return ConfigError{ConfigStatus::FAILED, Untranslated(error)};
+            }
+        }
+
         // Create settings.json if -nosettings was not specified.
         if (args.GetSettingsPath()) {
             std::vector<std::string> details;
diff --git a/src/init.cpp b/src/init.cpp
index 18f91164a3..1ae21ec8d2 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -444,6 +444,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
     argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-includeconf=<file>", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-allowignoredconf", strprintf("For backwards compatibility, treat an unused %s file in the datadir as a warning, not an error.", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-loadblock=<file>", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-maxmempool=<n>", strprintf("Keep the transaction memory pool below <n> megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE_MB), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-maxorphantx=<n>", strprintf("Keep at most <n> unconnectable transactions in memory (default: %u)", DEFAULT_MAX_ORPHAN_TRANSACTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp
index eaadbd7f7a..e45fc1ced8 100644
--- a/src/qt/test/test_main.cpp
+++ b/src/qt/test/test_main.cpp
@@ -71,6 +71,9 @@ int main(int argc, char* argv[])
     gArgs.ForceSetArg("-upnp", "0");
     gArgs.ForceSetArg("-natpmp", "0");
 
+    std::string error;
+    if (!gArgs.ReadConfigFiles(error, true)) QWARN(error.c_str());
+
     // Prefer the "minimal" platform for the test instead of the normal default
     // platform ("xcb", "windows", or "cocoa") so tests can't unintentionally
     // interfere with any background GUIs and don't require extra resources.