init: Error if ignored bitcoin.conf file is found

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
  bitcoin/bitcoin#27246 (comment)
  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 specified 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.

Author: ryanofsky

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/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 = args.GetConfigFilePath();
+
         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

@@ -441,6 +441,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);

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 @@ def test_config_file_parser(self):
         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')
@@ -282,6 +287,55 @@ def test_connect_with_seednode(self):
                     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()
@@ -291,6 +345,8 @@ def run_test(self):
 
         self.test_config_file_parser()
         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 @@ def __getattr__(self, name):
             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 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, **kwargs
 
         # 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,14 +12,16 @@
 import json
 import logging
 import os
+import pathlib
 import random
 import re
+import sys
 import time
 import unittest
 
 from . import coverage
 from .authproxy import AuthServiceProxy, JSONRPCException
-from typing import Callable, Optional
+from typing import Callable, Optional, Tuple
 
 logger = logging.getLogger("TestFramework.utils")
 
@@ -420,6 +422,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: