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

eefe56967b bugfix: Fix incorrect debug.log config file path (Ryan Ofsky)
3746f00be1 init: Error if ignored bitcoin.conf file is found (Ryan Ofsky)
398c3719b0 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 eefe56967b
  willcl-ark:
    re-ACK eefe56967b
  TheCharlatan:
    ACK eefe56967b

Tree-SHA512: 939a98a4b271b5263d64a2df3054c56fcde94784edf6f010d78693a371c38aa03138ae9cebb026b6164bbd898d8fd0845a61a454fd996e328fd7bcf51c580c2b

doc/release-notes-27302.md

@@ -0,0 +1,4 @@
+Configuration
+---
+
+- `bitcoind` and `bitcoin-qt` will now raise an error on startup if a datadir that is being used contains a bitcoin.conf file that will be ignored, which can happen when a datadir= line is used in a bitcoin.conf file. The error message is just a diagnostic intended to prevent accidental misconfiguration, and it can be disabled to restore the previous behavior of using the datadir while ignoring the bitcoin.conf contained in it.

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

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

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;

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;

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

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.

test/functional/feature_config_args.py

@@ -5,9 +5,14 @@
 """Test various command line arguments and configuration file parameters."""
 
 import os
+import pathlib
+import re
+import sys
+import tempfile
 import time
 
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.test_node import ErrorMatch
 from test_framework import util
 
 
@@ -74,7 +79,7 @@ class ConfArgsTest(BitcoinTestFramework):
         util.write_config(main_conf_file_path, n=0, chain='', extra_config=f'includeconf={inc_conf_file_path}\n')
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
             conf.write('acceptnonstdtxn=1\n')
-        self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={main_conf_file_path}"], expected_msg='Error: acceptnonstdtxn is not currently supported for main chain')
+        self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={main_conf_file_path}", "-allowignoredconf"], expected_msg='Error: acceptnonstdtxn is not currently supported for main chain')
 
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
             conf.write('nono\n')
@@ -108,6 +113,41 @@ class ConfArgsTest(BitcoinTestFramework):
         with open(inc_conf_file2_path, 'w', encoding='utf-8') as conf:
             conf.write('')  # clear
 
+    def test_config_file_log(self):
+        # Disable this test for windows currently because trying to override
+        # the default datadir through the environment does not seem to work.
+        if sys.platform == "win32":
+            return
+
+        self.log.info('Test that correct configuration path is changed when configuration file changes the datadir')
+
+        # Create a temporary directory that will be treated as the default data
+        # directory by bitcoind.
+        env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "test_config_file_log"))
+        default_datadir.mkdir(parents=True)
+
+        # Write a bitcoin.conf file in the default data directory containing a
+        # datadir= line pointing at the node datadir.
+        node = self.nodes[0]
+        conf_text = pathlib.Path(node.bitcoinconf).read_text()
+        conf_path = default_datadir / "bitcoin.conf"
+        conf_path.write_text(f"datadir={node.datadir}\n{conf_text}")
+
+        # Drop the node -datadir= argument during this test, because if it is
+        # specified it would take precedence over the datadir setting in the
+        # config file.
+        node_args = node.args
+        node.args = [arg for arg in node.args if not arg.startswith("-datadir=")]
+
+        # Check that correct configuration file path is actually logged
+        # (conf_path, not node.bitcoinconf)
+        with self.nodes[0].assert_debug_log(expected_msgs=[f"Config file: {conf_path}"]):
+            self.start_node(0, ["-allowignoredconf"], env=env)
+            self.stop_node(0)
+
+        # Restore node arguments after the test
+        node.args = node_args
+
     def test_invalid_command_line_options(self):
         self.nodes[0].assert_start_raises_init_error(
             expected_msg='Error: Error parsing command line arguments: Can not set -proxy with no value. Please specify value with -proxy=value.',
@@ -282,6 +322,55 @@ class ConfArgsTest(BitcoinTestFramework):
                     unexpected_msgs=seednode_ignored):
                 self.restart_node(0, extra_args=[connect_arg, '-seednode=fakeaddress2'])
 
+    def test_ignored_conf(self):
+        self.log.info('Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored '
+                      'because a conflicting -conf file argument is passed.')
+        node = self.nodes[0]
+        with tempfile.NamedTemporaryFile(dir=self.options.tmpdir, mode="wt", delete=False) as temp_conf:
+            temp_conf.write(f"datadir={node.datadir}\n")
+        node.assert_start_raises_init_error([f"-conf={temp_conf.name}"], re.escape(
+            f'Error: Data directory "{node.datadir}" contains a "bitcoin.conf" file which is ignored, because a '
+            f'different configuration file "{temp_conf.name}" from command line argument "-conf={temp_conf.name}" '
+            f'is being used instead.') + r"[\s\S]*", match=ErrorMatch.FULL_REGEX)
+
+        # Test that passing a redundant -conf command line argument pointing to
+        # the same bitcoin.conf that would be loaded anyway does not trigger an
+        # error.
+        self.start_node(0, [f'-conf={node.datadir}/bitcoin.conf'])
+        self.stop_node(0)
+
+    def test_ignored_default_conf(self):
+        # Disable this test for windows currently because trying to override
+        # the default datadir through the environment does not seem to work.
+        if sys.platform == "win32":
+            return
+
+        self.log.info('Test error is triggered when bitcoin.conf in the default data directory sets another datadir '
+                      'and it contains a different bitcoin.conf file that would be ignored')
+
+        # Create a temporary directory that will be treated as the default data
+        # directory by bitcoind.
+        env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "home"))
+        default_datadir.mkdir(parents=True)
+
+        # Write a bitcoin.conf file in the default data directory containing a
+        # datadir= line pointing at the node datadir. This will trigger a
+        # startup error because the node datadir contains a different
+        # bitcoin.conf that would be ignored.
+        node = self.nodes[0]
+        (default_datadir / "bitcoin.conf").write_text(f"datadir={node.datadir}\n")
+
+        # Drop the node -datadir= argument during this test, because if it is
+        # specified it would take precedence over the datadir setting in the
+        # config file.
+        node_args = node.args
+        node.args = [arg for arg in node.args if not arg.startswith("-datadir=")]
+        node.assert_start_raises_init_error([], re.escape(
+            f'Error: Data directory "{node.datadir}" contains a "bitcoin.conf" file which is ignored, because a '
+            f'different configuration file "{default_datadir}/bitcoin.conf" from data directory "{default_datadir}" '
+            f'is being used instead.') + r"[\s\S]*", env=env, match=ErrorMatch.FULL_REGEX)
+        node.args = node_args
+
     def run_test(self):
         self.test_log_buffer()
         self.test_args_log()
@@ -290,7 +379,10 @@ class ConfArgsTest(BitcoinTestFramework):
         self.test_connect_with_seednode()
 
         self.test_config_file_parser()
+        self.test_config_file_log()
         self.test_invalid_command_line_options()
+        self.test_ignored_conf()
+        self.test_ignored_default_conf()
 
         # Remove the -datadir argument so it doesn't override the config file
         self.nodes[0].args = [arg for arg in self.nodes[0].args if not arg.startswith("-datadir")]

test/functional/test_framework/test_node.py

@@ -190,7 +190,7 @@ class TestNode():
             assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
             return getattr(RPCOverloadWrapper(self.rpc, descriptors=self.descriptors), name)
 
-    def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, **kwargs):
+    def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None, **kwargs):
         """Start the node."""
         if extra_args is None:
             extra_args = self.extra_args
@@ -213,6 +213,8 @@ class TestNode():
 
         # add environment variable LIBC_FATAL_STDERR_=1 so that libc errors are written to stderr and not the terminal
         subp_env = dict(os.environ, LIBC_FATAL_STDERR_="1")
+        if env is not None:
+            subp_env.update(env)
 
         self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs)
 

test/functional/test_framework/util.py

@@ -12,13 +12,15 @@ import inspect
 import json
 import logging
 import os
+import pathlib
 import random
 import re
+import sys
 import time
 
 from . import coverage
 from .authproxy import AuthServiceProxy, JSONRPCException
-from typing import Callable, Optional
+from typing import Callable, Optional, Tuple
 
 logger = logging.getLogger("TestFramework.utils")
 
@@ -419,6 +421,22 @@ def get_datadir_path(dirname, n):
     return os.path.join(dirname, "node" + str(n))
 
 
+def get_temp_default_datadir(temp_dir: pathlib.Path) -> Tuple[dict, pathlib.Path]:
+    """Return os-specific environment variables that can be set to make the
+    GetDefaultDataDir() function return a datadir path under the provided
+    temp_dir, as well as the complete path it would return."""
+    if sys.platform == "win32":
+        env = dict(APPDATA=str(temp_dir))
+        datadir = temp_dir / "Bitcoin"
+    else:
+        env = dict(HOME=str(temp_dir))
+        if sys.platform == "darwin":
+            datadir = temp_dir / "Library/Application Support/Bitcoin"
+        else:
+            datadir = temp_dir / ".bitcoin"
+    return env, datadir
+
+
 def append_config(datadir, options):
     with open(os.path.join(datadir, "bitcoin.conf"), 'a', encoding='utf8') as f:
         for option in options:

test/lint/run-lint-format-strings.py

@@ -241,20 +241,32 @@ def count_format_specifiers(format_string):
     3
     >>> count_format_specifiers("foo %d bar %i foo %% foo %*d foo")
     4
+    >>> count_format_specifiers("foo %5$d")
+    5
+    >>> count_format_specifiers("foo %5$*7$d")
+    7
     """
     assert type(format_string) is str
     format_string = format_string.replace('%%', 'X')
-    n = 0
-    in_specifier = False
-    for i, char in enumerate(format_string):
-        if char == "%":
-            in_specifier = True
+    n = max_pos = 0
+    for m in re.finditer("%(.*?)[aAcdeEfFgGinopsuxX]", format_string, re.DOTALL):
+        # Increase the max position if the argument has a position number like
+        # "5$", otherwise increment the argument count.
+        pos_num, = re.match(r"(?:(^\d+)\$)?", m.group(1)).groups()
+        if pos_num is not None:
+            max_pos = max(max_pos, int(pos_num))
+        else:
             n += 1
-        elif char in "aAcdeEfFgGinopsuxX":
-            in_specifier = False
-        elif in_specifier and char == "*":
+
+        # Increase the max position if there is a "*" width argument with a
+        # position like "*7$", and increment the argument count if there is a
+        # "*" width argument with no position.
+        star, star_pos_num = re.match(r"(?:.*?(\*(?:(\d+)\$)?)|)", m.group(1)).groups()
+        if star_pos_num is not None:
+            max_pos = max(max_pos, int(star_pos_num))
+        elif star is not None:
             n += 1
-    return n
+    return max(n, max_pos)
 
 
 def main():