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

ryanofsky commented at 5:38 PM on March 22, 2023: contributor

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

DrahtBot commented at 5:38 PM on March 22, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pinheadmz, willcl-ark, TheCharlatan
Concept NACK	ghost
Concept ACK	RandyMcMillan
Approach ACK	hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

fanquake requested review from pinheadmz on Mar 23, 2023

ryanofsky force-pushed on Mar 23, 2023

ryanofsky commented at 3:11 PM on March 23, 2023: contributor

CI's hate this one weird PR.

Updated c661be4e7fd9abe5bbec0a9b123dc66d04ffc1bf -> 780c696fc310a6df8464b40983b23f8b0a3074f0 (pr/ignoredconf.3 -> pr/ignoredconf.4, compare) to fix CI errors

tuple[] TypeError: 'type' object is not subscriptable`

https://cirrus-ci.com/task/6162407618248704?logs=ci#L10177 https://cirrus-ci.com/task/4598191232909312?logs=ci#L3943 https://cirrus-ci.com/task/4807809292828672?logs=ci#L3537 https://cirrus-ci.com/task/5599457664827392?logs=ci#L4306

common/init.cpp:70:61: error: invalid initialization of reference of type ‘const string&’ {aka ‘const std::__cxx11::basic_string<char>&’} from expression of type ‘const fs::path’

https://cirrus-ci.com/task/5036507711406080?logs=ci#L3761

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\common\init.cpp(70,78): error C2664: 'std::_Quote_out<char,std::char_traits<char>,std::_Default_allocator_traits<_Alloc>::size_type> fs::quoted(const std::string &)': cannot convert argument 1 from 'const fs::path' to 'const std::string &' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
          with
          [
              _Alloc=std::allocator<char>
          ]
C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\common\init.cpp(70,78): message : Reason: cannot convert from 'const fs::path' to 'const std::string'

https://cirrus-ci.com/task/5317982688116736?logs=build#L1748

src/common/init.cpp: Expected 8 argument(s) after format string but found 4 argument(s): 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 resolve 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 warnignoredconf=1 option to ignore the %2$s file in data directory %1$s with a " "warning instead

https://cirrus-ci.com/task/4755032734695424?logs=lint#L221

hebasto commented at 3:42 PM on March 23, 2023: member

Show an error on startup if a bitcoin datadir that is being used contains a bitcoin.conf file that is ignored.

Concept ACK on that.

ryanofsky force-pushed on Mar 23, 2023

ryanofsky commented at 4:25 PM on March 23, 2023: contributor

Updated 780c696fc310a6df8464b40983b23f8b0a3074f0 -> 53d99551e913bfb85769a2b34fed73898779ff0f (pr/ignoredconf.4 -> pr/ignoredconf.5, compare) to try to fix another CI error on win64:

Traceback (most recent call last):
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
    self.run_test()
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 345, in run_test
    self.test_ignored_conf()
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 297, in test_ignored_conf
    pathlib.Path(temp_conf.name).write_text(f"datadir={node.datadir}\n")
  File "C:\Python39\lib\pathlib.py", line 1275, in write_text
    with self.open(mode='w', encoding=encoding, errors=errors) as f:
  File "C:\Python39\lib\pathlib.py", line 1242, in open
    return io.open(self, mode, buffering, encoding, errors, newline,
  File "C:\Python39\lib\pathlib.py", line 1110, in _opener
    return self._accessor.open(self, flags, mode)
PermissionError: [Errno 13] Permission denied: 'C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\test_runner_₿_🏃_20230323_152950\\feature_config_args_10\\tmpc3nh946l'

https://cirrus-ci.com/task/5753433081249792?logs=functional_tests#L82

in src/common/init.cpp:78 in 53d99551e9 outdated

  73 | +                "Data directory %1$s contains a %2$s file which is ignored, because a different configuration file "
  74 | +                "%3$s from %4$s is being used instead. Possible ways to resolve this would be to:\n"
  75 | +                "- Delete or rename the %2$s file in data directory %1$s.\n"
  76 | +                "- Change datadir= or conf= options to specify one configuration file, not two, and use "
  77 | +                "includeconf= to include any other configuration files.\n"
  78 | +                "- Set warnignoredconf=1 option to ignore the %2$s file in data directory %1$s with a "

willcl-ark commented at 4:38 PM on March 23, 2023:

Whilst I appreciate that this is a simple and elegant way to remain backwards-compatible, I can't help but wonder if it's something we actually want to support with a new option; what is the use-case here? The operator either has to modify their current config with this new flag, or... fix their config?

If the operator is going to fix something up, then I think it should be the latter!

Perhaps I am missing a use-case though where they would want this old config to be purposefully ignored?

ryanofsky commented at 6:03 PM on March 23, 2023:

re: #27302 (review)

Whilst I appreciate that this is a simple and elegant way to remain backwards-compatible, I can't help but wonder if it's something we actually want to support with a new option; what is the use-case here? The operator either has to modify their current config with this new flag, or... fix their config?

There may be other use cases, but the backwards compatibility use-case I had in mind is where someone has a bitcoin datadir on a portable drive that they use with more than one PC, and the PC's are not configured the exact same way. I'd be reluctant to add a hard error where new a new version of bitcoin refuses to work with a datadir that worked previously and is otherwise fine except for having an extra config file.

Separately, the option also turned out to be useful in the python -acceptnonstdtxn python test, so the test could bypass the bitcoin.conf file created by the test framework without deleting it.

If other reviewers are more confident that adding a hard error won't cause problems, though, I'd be happy to remove the escape hatch.

pinheadmz commented at 3:08 PM on March 24, 2023:

Good thinking about the portable datdir use case, I think it is ok to allow a bypass. It's a bit funny calling it "warn...=1" though. I understand you mean "warn instead of throw an error" but I almost think something like -ignoreignoredconf or -ignoreextraconf makes more literal sense.

ryanofsky commented at 4:03 PM on March 24, 2023:

re: #27302 (review)

Another name could be allowignoredconf if that is better

pinheadmz commented at 5:03 PM on March 24, 2023:

allowignoredconf

yeah

ryanofsky commented at 5:18 PM on March 27, 2023:

re: #27302 (review)

allowignoredconf

yeah

Renamed warnignoredconf to allowignoredconf

ryanofsky force-pushed on Mar 23, 2023

ryanofsky commented at 6:34 PM on March 23, 2023: contributor

Updated 53d99551e913bfb85769a2b34fed73898779ff0f -> 972335762aeaab557dbb2e44b149b18005987f8b (pr/ignoredconf.5 -> pr/ignoredconf.6, compare) to try to fix another CI error on win64:

TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
    self.run_test()
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 346, in run_test
    self.test_ignored_conf()
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 299, in test_ignored_conf
    node.assert_start_raises_init_error([f"-conf={temp_conf.name}"], re.escape(
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_node.py", line 590, in assert_start_raises_init_error
    self._raise_assertion_error(
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_node.py", line 173, in _raise_assertion_error
    raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] Expected message "Error:\ Data\ directory\ "C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\test_runner_₿_🏃_20230323_164852\\feature_config_args_10\\node0"\ contains\ a\ "bitcoin\.conf"\ file\ which\ is\ ignored,\ because\ a\ different\ configuration\ file\ "C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\test_runner_₿_🏃_20230323_164852\\feature_config_args_10\\tmpuywrf6s4"\ from\ command\ line\ argument\ "\-conf=C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\test_runner_₿_🏃_20230323_164852\\feature_config_args_10\\tmpuywrf6s4"\ is\ being\ used\ instead\.[\s\S]*" does not fully match stderr:
"Error: Error reading configuration file: specified config file "C:\Users\ContainerAdministrator\AppData\Local\Temp\test_runner_₿_🏃_20230323_164852\feature_config_args_10\tmpuywrf6s4" could not be opened."

https://cirrus-ci.com/task/5315879898972160?logs=functional_tests#L2770

ryanofsky force-pushed on Mar 23, 2023

ryanofsky commented at 9:04 PM on March 23, 2023: contributor

Updated 972335762aeaab557dbb2e44b149b18005987f8b -> d00762c75a1b9ad16e0dadf25886a7baefa84a12 (pr/ignoredconf.6 -> pr/ignoredconf.7, compare) to try to fix a new win64 CI error in test_ignored_default_conf:

Traceback (most recent call last):
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
    self.run_test()
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 346, in run_test
    self.test_ignored_default_conf()
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 330, in test_ignored_default_conf
    node.assert_start_raises_init_error([], re.escape(
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_node.py", line 605, in assert_start_raises_init_error
    self._raise_assertion_error(assert_msg)
  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_node.py", line 173, in _raise_assertion_error
    raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] bitcoind should have exited within 480s with expected error

ryanofsky force-pushed on Mar 23, 2023

ryanofsky commented at 11:44 PM on March 23, 2023: contributor

Updated d00762c75a1b9ad16e0dadf25886a7baefa84a12 -> 37ffeca9e0363781f8168114faad004d5543260f (pr/ignoredconf.7 -> pr/ignoredconf.8, compare) disabling one of the new tests on win64 since it continues to fail https://cirrus-ci.com/task/5859482064912384

in src/common/init.cpp:34 in 37ffeca9e0 outdated

  29 | +        // possible for the config file to cause another configuration to be
  30 | +        // used, though. Specifying a conf= option in the config file causes a
  31 | +        // parse error, and specifying a datadir= location containing another
  32 | +        // bitcoin.conf file just ignores the other file.)
  33 | +        const fs::path orig_datadir_path{args.GetDataDirBase()};
  34 | +        const fs::path orig_config_path = args.GetConfigFilePath();

pinheadmz commented at 3:11 PM on March 24, 2023:

Is it possible for you to cache the config file path at the system level like I did here? That would also fix the confusing log statement produced by the same datadir/conf configuration.

ryanofsky commented at 3:51 PM on March 24, 2023:

re: #27302 (review)

Is it possible for you to cache the config file path at the system level like I did here? That would also fix the confusing log statement produced by the same datadir/conf configuration.

Yes, I was planning to review #27303 today, but I did look at it previously and if #27303 was merged first and this was merged on top, this PR this line could go away. I don't think the PRs should be combined though. Better for them to stay targeted and simple. If there are any conflicts they should be trivial.

ryanofsky commented at 5:17 PM on March 27, 2023:

re: #27302 (comment)

I don't think the PRs should be combined though. Better for them to stay targeted and simple. If there are any conflicts they should be trivial.

It turns out it is not possible to actually write a meaningful test for #27303 without some of the test code added in this PR. This PR adds test coverage for starting bitcoind without -conf or -datadir arguments, just with a bitcoin.conf file in the default directory, and it turns out the only way to reproduce the bug reported in #27303 is by doing the same thing.

So I've added a new commit ac9fee615a4f0c4d1bbed0d69486c54be4860dcb to this PR which includes a fix similar to the one from #27303 as well as a new test.

in test/functional/feature_config_args.py:429 in 37ffeca9e0 outdated

 389 | @@ -332,5 +390,21 @@ def run_test(self):
 390 |          assert os.path.exists(os.path.join(new_data_dir_2, self.chain, 'blocks'))
 391 |  
 392 |  
 393 | +def get_temp_default_datadir(temp_dir: pathlib.Path) -> Tuple[dict, pathlib.Path]:

pinheadmz commented at 3:25 PM on March 24, 2023:

Why do you need this os-specific path generator instead of just adding a single subdir to the direcotry where the test framework is already working? On my system at least, it doesn't actually return the "real" default path:

/tmp/bitcoin_func_test_yg47qoyn/home/Library/Application Support/Bitcoin

so why not just add home/ on every platform and use that?

ryanofsky commented at 4:01 PM on March 24, 2023:

re: #27302 (review)

Why do you need this os-specific path generator instead of just adding a single subdir to the direcotry where the test framework is already working? On my system at least, it doesn't actually return the "real" default path:

/tmp/bitcoin_func_test_yg47qoyn/home/Library/Application Support/Bitcoin

IIUC, that path is literally the path this function is generating and returning. The point of this function is to let python code write a bitcoin.conf file that bitcoind will use, but if there's another suggestion about how to achieve this I'm happy to adopt it.

pinheadmz commented at 5:02 PM on March 24, 2023:

Ok I understand now, I tried removing it but now I see the environment variables are important for setting the initial "where's the conf" directory without spoiling -datadir. I also notice get_temp_default_datadir() has a windows branch but windows will skip this test anyway! Not a bad idea to leave it in there anyway though I guess.

pinheadmz commented at 6:26 PM on March 27, 2023:

Not a blocker for me, but curious if this kind of function should go in a broader utility package like test_node.py ?

willcl-ark commented at 12:44 PM on March 28, 2023:

There is get_datadir_path() in test_framework/util.py:

https://github.com/bitcoin/bitcoin/blob/b759cefe936ed3991633acff215ea1dcec5ece28/test/functional/test_framework/util.py#L417-L418

So perhaps could go next to that in a future PR?

ryanofsky commented at 11:40 PM on April 4, 2023:

re: #27302 (review)

So perhaps could go next to that in a future PR?

Sure, moved there now

in src/common/init.cpp:72 in 37ffeca9e0 outdated

  66 | +        const fs::path base_config_path = base_path / BITCOIN_CONF_FILENAME;
  67 | +        if (fs::exists(base_config_path) && !fs::equivalent(orig_config_path, base_config_path)) {
  68 | +            const std::string cli_config_path = args.GetArg("-conf", "");
  69 | +            const std::string config_source = cli_config_path.empty()
  70 | +                ? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path)))
  71 | +                : strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path));

pinheadmz commented at 3:30 PM on March 24, 2023:

clang-format wants this to be a single line but I prefer this style too, especially for these longer return values 🤷‍♂️

ryanofsky commented at 3:54 PM on March 24, 2023:

re: #27302 (review)

clang-format wants this to be a single line but I prefer this style too, especially for these longer return values man_shrugging

Right this was done manually, but you can set a width preference which will cause clang to wrap in the same way, I believe. Have done that for previous PRs.

pinheadmz commented at 3:38 PM on March 24, 2023: member

concept ACK Reviewed code, I have a few questions about the implementation and test below. I reverted the linter commit to reproduce the error and confirm the fix. Also set up the data directories and conf files to reproduce the use case and tested with bitcoind on regtest with and without the warning flag.

ryanofsky commented at 4:05 PM on March 24, 2023: contributor

Thanks for the review

in src/common/init.cpp:85 in 37ffeca9e0 outdated

  80 | +                fs::quoted(fs::PathToString(base_path)),
  81 | +                fs::quoted(BITCOIN_CONF_FILENAME),
  82 | +                fs::quoted(fs::PathToString(orig_config_path)),
  83 | +                config_source);
  84 | +            if (args.GetBoolArg("-warnignoredconf", false)) {
  85 | +                LogPrintf("Warning: %s\n", error);

TheCharlatan commented at 11:16 AM on March 27, 2023:

Nit: This is currently printing the line instructing the user to set warningnoredconf=1 even when it is already set.

ryanofsky commented at 3:48 PM on March 27, 2023:

re: #27302 (review)

Nit: This is currently printing the line instructing the user to set warningnoredconf=1 even when it is already set.

Thanks, changed "resolve" to "address" and reworded the text to make it clear a warning will still occur.

TheCharlatan commented at 1:36 PM on March 27, 2023: contributor

Concept ACK

ghost commented at 3:21 PM on March 27, 2023: none

Show an error on startup if a bitcoin datadir that is being used contains a bitcoin.conf file that is ignored

Why is it ignored?
Any downsides for using bitcoin.conf from datadir if conf argument isn't used?

ryanofsky commented at 3:40 PM on March 27, 2023: contributor

Show an error on startup if a bitcoin datadir that is being used contains a bitcoin.conf file that is ignored
1. Why is it ignored?

It's debatable whether it should be ignored, but the reason it is ignored is that one configuration file has already been parsed and trying to merge in a second configuration file in another datadir would be new and potentially confusing behavior. Merging in another configuration file could lead to unexpected conflicts between settings in the two files, or weird scenarios like a datadir containing a configuration file pointing to another datadir containing another configuration file, potentially in an infinite loop.

So the simplest improvement we can make is just to show a warning or error if multiple configuration files present and it is not clear which one is supposed to be used.

2. Any downsides for using bitcoin.conf from datadir if conf argument isn't used?

it would be a change in behavior that could break existing configurations in non-obvious ways. We did discuss different options for doing it in #27246 (comment), but there also didn't seem to be a good use case that wouldn't be better addressed by using -includeconf

ryanofsky force-pushed on Mar 27, 2023

ryanofsky commented at 5:45 PM on March 27, 2023: contributor

Updated 37ffeca9e0363781f8168114faad004d5543260f -> ac9fee615a4f0c4d1bbed0d69486c54be4860dcb (pr/ignoredconf.8 -> pr/ignoredconf.9, compare) with some changes to wording and adding a new commit ac9fee615a4f0c4d1bbed0d69486c54be4860dcb which can be an alternative to #27303

ghost commented at 5:55 PM on March 27, 2023: none

Show an error on startup if a bitcoin datadir that is being used contains a bitcoin.conf file that is ignored
1. Why is it ignored?
It's debatable whether it should be ignored, but the reason it is ignored is that one configuration file has already been parsed and trying to merge in a second configuration file in another datadir would be new and potentially confusing behavior. Merging in another configuration file could lead to unexpected conflicts between settings in the two files, or weird scenarios like a datadir containing a configuration file pointing to another datadir containing another configuration file, potentially in an infinite loop.

So the simplest improvement we can make is just to show a warning or error if multiple configuration files present and it is not clear which one is supposed to be used.
2. Any downsides for using bitcoin.conf from datadir if conf argument isn't used?
it would be a change in behavior that could break existing configurations in non-obvious ways. We did discuss different options for doing it in #27246 (comment), but there also didn't seem to be a good use case that wouldn't be better addressed by using -includeconf

Not convinced. Sorry.

NACK

pinheadmz approved

pinheadmz commented at 6:27 PM on March 27, 2023: member

ACK ac9fee615a4f0c4d1bbed0d69486c54be4860dcb

Reviewed code and ran tests. Confirmed tests fail on master, not on branch. I believe this PR also closes #19990 as a nice added bonus.

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

ryanofsky commented at 6:34 PM on March 27, 2023: contributor

NACK

I guess you want multiple bitcoin.conf files if detected to be merged together. But I'm not sure you should NACK this PR because they aren't merged together currently. This PR just warns the user about multiple files being present, and should make it easier and safer to merge them later if someone wants to implement that.

ghost commented at 6:59 PM on March 27, 2023: none

NACK

I guess you want multiple bitcoin.conf files if detected to be merged together. But I'm not sure you should NACK this PR because they aren't merged together currently. This PR just warns the user about multiple files being present, and should make it easier and safer to merge them later if someone wants to implement that.

This PR confirms we are okay with buggy behaviour defined in #27302 (comment)

I am not okay with that. It's okay if others want to do commits that don't really fix anything.

in src/common/init.cpp:66 in 94505e0a13 outdated

  60 | @@ -48,6 +61,32 @@ std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn setting
  61 |              fs::create_directories(net_path / "wallets");
  62 |          }
  63 |  
  64 | +        // Show an error or warning if there is a bitcoin.conf file in the
  65 | +        // datadir that is being ignored.

willcl-ark commented at 9:31 PM on March 27, 2023:

I don't know if I'm doing something supported here, but this doesn't seem to work for me:

./src/bitcoind -datadir=/tmp/bitcoin_27302/ -rpcport=18555 -port=18556

2023-03-27T21:22:09Z Default data directory /home/will/.bitcoin
2023-03-27T21:22:09Z Using data directory /tmp/bitcoin_27302/regtest
2023-03-27T21:22:09Z Config file: /tmp/bitcoin_27302/bitcoin.conf
2023-03-27T21:22:09Z Config file arg: datadir="/tmp/bitcoin_27302/datadir2"
2023-03-27T21:22:09Z Config file arg: regtest="1"
2023-03-27T21:22:09Z Command-line arg: datadir="/tmp/bitcoin_27302/"
2023-03-27T21:22:09Z Command-line arg: port="18556"
2023-03-27T21:22:09Z Command-line arg: rpcport="18555"
2023-03-27T21:22:09Z Using at most 125 automatic connections (1024 file descriptors available)

I am specifying -datadir= on the command line (to start with a non-default datadir). This first datadir contains a bitcoin.conf file which contains a second datadir= option, where the second datadir also contains a second bitcoin.conf but I don't see the warning?

The port= option in datadir2/bitcoin.conf is being ignored:

will@ubuntu in /tmp/bitcoin_27302
₿ tree
.
├── bitcoin.conf
├── datadir2
│   └── bitcoin.conf
└── regtest
    ├── banlist.json
    ├── bitcoind.pid
    ├── blocks
    │   ├── blk00000.dat
    │   ├── index
    │   │   ├── 000015.log
    │   │   ├── 000016.ldb
    │   │   ├── CURRENT
    │   │   ├── LOCK
    │   │   └── MANIFEST-000013
    │   └── rev00000.dat
    ├── chainstate
    │   ├── 000005.ldb
    │   ├── 000025.ldb
    │   ├── 000027.log
    │   ├── CURRENT
    │   ├── LOCK
    │   └── MANIFEST-000026
    ├── debug.log
    ├── fee_estimates.dat
    ├── mempool.dat
    ├── onion_v3_private_key
    ├── peers.dat
    ├── settings.json
    └── wallets

6 directories, 23 files

will@ubuntu in /tmp/bitcoin_27302
₿ cat bitcoin.conf
regtest=1
datadir=/tmp/bitcoin_27302/datadir2

will@ubuntu in /tmp/bitcoin_27302
₿ cat datadir2/bitcoin.conf
port=18557

I don't see yet where the error is here, but suspect I might have short circuited it by providing a -datadir command line option intially?

willcl-ark commented at 9:53 PM on March 27, 2023:

I also do not get the error with:

₿ ./src/bitcoind -datadir=/tmp/bitcoin_27302/ -conf=/tmp/bitcoin_27302/bitcoin.conf -rpcport=18555 -port=18556

But I do see the error (as described in the commit bullet point 2) with:

₿ ./src/bitcoind -conf=/tmp/bitcoin_27302/bitcoin.conf -rpcport=18555 -port=18556

In my opinion:

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

...is not fully satisfied by this PR, as using a non-default datadir can bypass it?

willcl-ark commented at 12:58 PM on March 28, 2023:

^ this can be resolved.

ryanofsky commented at 1:35 AM on March 28, 2023: contributor

@willcl-ark this PR is not trying to error when one datadir setting takes precedence over another one. As the PR description states it adds an error for cases where "a bitcoin datadir that is being used contains a bitcoin.conf file that is ignored" (emphasis added). It does not show an error in cases where a bitcoin datadir that not being used contains a bitcoin.conf file that is ignored, because the whole directory is ignored, so there is nothing special about one file in it.

But we could open a separate PR or issue there should be a warning about a command line -datadir= argument overriding a bitcoin.conf datadir= option. I don't think it should be an error because it is important for parameters on the command line to be able to override options in the configuration file (and for later command line options to be able to override earlier ones) as a tool for debugging and deployment. But it wouldn't hurt for the debug log to be more explicit in general about which settings were applied and if any were overridden.

in test/functional/feature_config_args.py:293 in 94505e0a13 outdated

 288 | @@ -282,6 +289,55 @@ def test_connect_with_seednode(self):
 289 |                      unexpected_msgs=seednode_ignored):
 290 |                  self.restart_node(0, extra_args=[connect_arg, '-seednode=fakeaddress2'])
 291 |  
 292 | +    def test_ignored_conf(self):
 293 | +        self.log.info('Test error is triggered when datadir contains a bitcoin.conf file that would be ignored '

willcl-ark commented at 9:39 AM on March 28, 2023:

If you re-touch, perhaps this could read:

'Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored '

ryanofsky commented at 9:10 PM on April 4, 2023:

re: #27302 (review)

If you re-touch, perhaps this could read:

'Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored '

Thanks applied suggestion

in test/functional/feature_config_args.py:332 in 94505e0a13 outdated

 294 | +                      'because a conflicting -conf file argument is passed.')
 295 | +        node = self.nodes[0]
 296 | +        with tempfile.NamedTemporaryFile(dir=self.options.tmpdir, mode="wt", delete=False) as temp_conf:
 297 | +            temp_conf.write(f"datadir={node.datadir}\n")
 298 | +        node.assert_start_raises_init_error([f"-conf={temp_conf.name}"], re.escape(
 299 | +            f'Error: Data directory "{node.datadir}" contains a "bitcoin.conf" file which is ignored, because a '

willcl-ark commented at 9:39 AM on March 28, 2023:

On a re-touch similar re-wording could be done here?

ryanofsky commented at 9:24 PM on April 4, 2023:

re: #27302 (review)

On a re-touch similar re-wording could be done here?

I don't think I would add "used" here or imply anything about the datadir here since the message is supposed to be about the config file not the datadir. The gist of the message is supposed to be: "you have two configuration files, and one is being ignored, so please merge them or get rid of the one you don't want."

Extra notes about the datadir I think are helpful for setting up the test and triggering the bug intentionally, but not really for resolving the problem once it happens. I'm happy to make edits though in case I'm misunderstanding the suggestion.

willcl-ark approved

willcl-ark commented at 12:57 PM on March 28, 2023: member

ACK ac9fee615 @ryanofsky Thanks for the extra clarification. I agree (and have now tested) that this PR does achieve that correctly.

I left a few tiny nits that don't need to be adressed unless you retouch (and want to address them).

TheCharlatan commented at 1:08 PM on March 28, 2023: contributor

Should this get its own release note? There are bound to be some node operators that will be caught off-guard by this change.

RandyMcMillan commented at 1:58 PM on March 28, 2023: contributor

Concept ACK

RandyMcMillan commented at 2:08 PM on March 28, 2023: contributor

A note on the merging idea:

There seems to be a lot of potential issues when "merging" configs that use blocknotify/walletnotify/alertnotify - i would lean toward a clean override - mitigating user over sight of any conflicting automation.

DrahtBot added the label Needs rebase on Apr 3, 2023

ryanofsky force-pushed on Apr 5, 2023

ryanofsky commented at 12:13 AM on April 5, 2023: contributor

Rebased ac9fee615a4f0c4d1bbed0d69486c54be4860dcb -> d31816eb5a0518c80f6cc4166fb4246acfc4decd (pr/ignoredconf.9 -> pr/ignoredconf.10, compare) due to conflict with #27254. Also added release note and applied review suggestions

DrahtBot removed the label Needs rebase on Apr 5, 2023

ryanofsky force-pushed on Apr 5, 2023

DrahtBot added the label Needs rebase on Apr 21, 2023

lint: Fix lint-format-strings false positives when format specifiers have argument positions

Do not error on valid format specifications like strprintf("arg2=%2$s arg1=%1$s arg2=%2$s", arg1, arg2);

Needed to avoid lint error in upcoming commit: https://cirrus-ci.com/task/4755032734695424?logs=lint#L221

Additionally tested with python -m doctest test/lint/run-lint-format-strings.py

398c3719b0

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
  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 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.

3746f00be1

bugfix: Fix incorrect debug.log config file path

Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.

The bad log print was reported and originally fixed in
https://github.com/bitcoin/bitcoin/pull/27303 by
Matthew Zipkin <pinheadmz@gmail.com>

This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>

eefe56967b

ryanofsky force-pushed on Apr 21, 2023

DrahtBot removed the label Needs rebase on Apr 21, 2023

DrahtBot added the label CI failed on Apr 21, 2023

ryanofsky force-pushed on Apr 25, 2023

DrahtBot removed the label CI failed on Apr 25, 2023

TheCharlatan approved

TheCharlatan commented at 10:35 AM on May 15, 2023: contributor

ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef

DrahtBot requested review from pinheadmz on May 15, 2023

DrahtBot requested review from willcl-ark on May 15, 2023

in doc/release-notes-27302.md:4 in 0319de5cbe outdated

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

pinheadmz commented at 3:55 PM on May 15, 2023:

- `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.

ryanofsky commented at 2:35 PM on May 23, 2023:

re: #27302 (review)

Thanks! Applied change

in test/functional/test_framework/util.py:437 in 0319de5cbe outdated

 432 | +    else:
 433 | +      env = dict(HOME=str(temp_dir))
 434 | +      if sys.platform == "darwin":
 435 | +          datadir = temp_dir / "Library/Application Support/Bitcoin"
 436 | +      else:
 437 | +          datadir = temp_dir / ".bitcoin"

pinheadmz commented at 5:19 PM on May 15, 2023:

nit: indent should be +2 more spaces

ryanofsky commented at 2:36 PM on May 23, 2023:

re: #27302 (review)

nit: indent should be +2 more spaces

Thanks! Fixed this

in src/common/init.cpp:75 in 0319de5cbe outdated

  70 | +            const std::string config_source = cli_config_path.empty()
  71 | +                ? strprintf("data directory %s", fs::quoted(fs::PathToString(orig_datadir_path)))
  72 | +                : strprintf("command line argument %s", fs::quoted("-conf=" + cli_config_path));
  73 | +            const std::string error = strprintf(
  74 | +                "Data directory %1$s contains a %2$s file which is ignored, because a different configuration file "
  75 | +                "%3$s from %4$s is being used instead. Possible ways to address this would be to:\n"

pinheadmz commented at 5:37 PM on May 15, 2023:

I think this will unnecessarily repeat the entire path, could maybe drop from %4$s, I got this message just now:

Data directory "/Users/matthewzipkin/Library/Application Support/Bitcoin/whoa" contains a "bitcoin.conf" file which is ignored, because a different configuration file "/Users/matthewzipkin/Library/Application Support/Bitcoin/bitcoin.conf" from data directory "/Users/matthewzipkin/Library/Application Support/Bitcoin" is being used instead.

ryanofsky commented at 2:36 PM on May 23, 2023:

re: #27302 (review)

I think this will unnecessarily repeat the entire path, could maybe drop from %4$s, I got this message just now:

Feel free to suggest the code change that you would like to see, but I think saying configuration file {initial_datadir}/bitcoin.conf from data directory {initial_datadir}" is more helpful than saying "configuration file {initial_datdir}/bitcoin.conf" because the latter tells you which configuration file it was trying to load but not why it was trying to load that configuration file.

I could have alternately made the message "from the initial data directory" instead of "data directory {initial_datadir}", but that assumes someone would know what "initial data directory" refers to. Just literally printing the datadir path that caused the configuration file to be loaded seemed like it would provide the most explicit information without assuming users understand config system minutia

pinheadmz approved

pinheadmz commented at 6:41 PM on May 15, 2023: member

ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef

Code review and run bitcoind and -qt with various conf file configurations to see errors and warnings. A few style nits below, can be ignored (especially the release note since that will get reviewed again upon actual release)

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

in src/common/init.cpp:79 in 0319de5cbe outdated

  74 | +                "Data directory %1$s contains a %2$s file which is ignored, because a different configuration file "
  75 | +                "%3$s from %4$s is being used instead. Possible ways to address this would be to:\n"
  76 | +                "- Delete or rename the %2$s file in data directory %1$s.\n"
  77 | +                "- Change datadir= or conf= options to specify one configuration file, not two, and use "
  78 | +                "includeconf= to include any other configuration files.\n"
  79 | +                "- Set allowignoredconf=1 option to treat this condition as a warning, not an error.",

hebasto commented at 2:09 PM on May 22, 2023:

This line seems redundant when allowignoredconf=1 has been provided actually:

$ ./src/bitcoind -regtest -allowignoredconf=1
2023-05-22T14:06:27Z Warning: Data directory "/home/hebasto/TEST" contains a "bitcoin.conf" file which is ignored, because a different configuration file "/home/hebasto/.bitcoin/bitcoin.conf" from data directory "/home/hebasto/.bitcoin" is being used instead. Possible ways to address this would be to:
- Delete or rename the "bitcoin.conf" file in data directory "/home/hebasto/TEST".
- Change datadir= or conf= options to specify one configuration file, not two, and use includeconf= to include any other configuration files.
- Set allowignoredconf=1 option to treat this condition as a warning, not an error.
...

ryanofsky commented at 2:36 PM on May 23, 2023:

re: #27302 (review)

This line seems redundant when allowignoredconf=1 has been provided actually:

Feel free to suggest a code change that you would like to see here.

IMO, the -allowignoredconf option is meant to provide an escape hatch for backwards compatibility when your bitcoin configuration is complicated and confusing and you don't have time to clean it up. I think implementation of the option should be as simple as possible and not complicate message formatting. If the log message repeats some redundant information it seems like there is not much harm in that and it might even be helpful.

in src/common/init.cpp:87 in 0319de5cbe outdated

  82 | +                fs::quoted(fs::PathToString(orig_config_path)),
  83 | +                config_source);
  84 | +            if (args.GetBoolArg("-allowignoredconf", false)) {
  85 | +                LogPrintf("Warning: %s\n", error);
  86 | +            } else {
  87 | +                return ConfigError{ConfigStatus::FAILED, Untranslated(error)};

hebasto commented at 2:10 PM on May 22, 2023:

Any possibility to make the error message translatable, as for others ConfigError calls?

ryanofsky commented at 2:36 PM on May 23, 2023:

re: #27302 (review)

Any possibility to make the error message translatable, as for others ConfigError calls?

Are you suggesting actually translating this message, or just making a code change to make it easier to translations the future?

Obviously any message is translatable, but I didn't want this particular message translated because it I think it should only show up in niche cases where users have manually created multiple datadirs and configuration files, and because the message has a lot of details that could easily be mistranslated

hebasto commented at 2:11 PM on May 22, 2023: member

Approach ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef.

ryanofsky force-pushed on May 23, 2023

ryanofsky commented at 3:37 PM on May 23, 2023: contributor

Updated 0319de5cbedd1a8f8766cfec61151c58b3fb27ef -> eefe56967b4eb4b5144325cde4f40fc1cbde3e65 (pr/ignoredconf.13 -> pr/ignoredconf.14, compare) with suggestions

Thanks for the reviews!

DrahtBot added the label CI failed on May 23, 2023

pinheadmz approved

pinheadmz commented at 8:18 PM on May 24, 2023: member

re-ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65

Confirmed nits addressed since last ACK, minimal diff. All tests pass locally for me, CI failures are unrelated to code changes.

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

DrahtBot requested review from TheCharlatan on May 25, 2023

willcl-ark approved

willcl-ark commented at 9:53 AM on May 25, 2023: member

re-ACK eefe56967b

TheCharlatan approved

TheCharlatan commented at 8:39 AM on May 26, 2023: contributor

ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65

fanquake merged this on May 26, 2023

fanquake closed this on May 26, 2023

sidhujag referenced this in commit 9fc4472e0e on May 26, 2023

pinheadmz commented at 3:03 PM on June 2, 2023: member

@ryanofsky does this close #27246 ?

ryanofsky commented at 3:51 PM on June 2, 2023: contributor

No I still need to figure out what the last message #27246 (comment) means, and finish #27409 to fix the "Unfortunately the behavior for bitcoin-qt is stupider than bitcoind..." issue

luke-jr referenced this in commit a7afcf229c on Aug 16, 2023

luke-jr referenced this in commit d0000b6c55 on Aug 16, 2023

luke-jr referenced this in commit b64ca49659 on Aug 16, 2023

AdamISZ commented at 9:00 PM on December 16, 2023: none

@ryanofsky I just wanted to give feedback that this kind of bit me [*], in running Joinmarket tests - the pytest invocation just started hanging when using Bitcoin Core 26.0 and it took some investigating to see why. Not disastrous of course, but I was left bewildered - why would Core be complaining that there existed another bitcoin.conf in ~/.bitcoin when I had explicitly set -conf=/somewhere/else/bitcoin.conf? I've skimmed the thread but I don't really get the logic. Is it something to do with wanting to support the aggregation of multiple conf files? ... that could make sense. It just seems unnatural that a manual setting doesn't function as an override (and an override is certainly what I wanted, and I'd expect most other users would too?).

[*] My fault for not reading the release notes :smile:

ryanofsky commented at 3:58 PM on December 18, 2023: contributor

Thanks @AdamISZ, this is great feedback. In your case it sounds like you have a bitcoin.conf file in the datadir, but you want it to be ignored, so I hope the error message was clear enough to explain why that configuration is ambiguous and how the ambiguity could be resolved by specifying allowignoredconf=1. I did want the error message to be self contained and tell you how to fix it without needing to refer to release notes. So if this wasn't the case, it sounds like some improvements could be needed.

Hopefully it should be clear why there is an ambiguity. If a data directory is being used, and data directory contains a bitcoin.conf file, I think the only reasonable expectation someone could have is that the bitcoin.conf file will be parsed and used just like the other files in the data directory. It's reasonable to expect that command line options would override settings in the bitcoin.conf file, but surprising for the file to be ignored completely, so the idea is just that you should specify allowignoredconf=1 when you do want that to happen.

AdamISZ commented at 4:17 PM on December 18, 2023: none

If a data directory is being used, and data directory contains a bitcoin.conf file, I think the only reasonable expectation someone could have is that the bitcoin.conf file will be parsed and used just like the other files in the data directory.

and

but surprising for the file to be ignored completely

That would be unarguable if we weren't talking about a manual setting of a different bitcoin.conf file. But we are, and I'd argue it's the opposite that's surprising [*].

If it's possible to use more than one .conf file at the same time, then I retract my confusion, this all makes a lot more sense.

[*] I know we tend, a lot of the time, to automate things with shell scripts, but ultimately command line invocation exists as a way to make a concrete decision now, "I am running this in this specific way, this time" - which is why having command line flags as distinct from always setting things in configuration files, is useful. And in that context only override makes sense (caveat - if it's a "single slot", i.e. only one value is possible at once), which is why I think software basically always works that way. Which is why this completely flummoxed me.

ryanofsky commented at 5:02 PM on December 18, 2023: contributor

That makes sense, so I'd change "the only reasonable expectation" to "a reasonable expectation" in my reply above.

I also think your "single slot" model of the -conf option is accurate, and we shouldn't try to merge a configuration file specified with -conf on the command line with a different bitcoin.conf file residing in the active data directory. But if there are two config files like that, and one is being ignored, it does seem like there is a high enough likelihood of misconfiguration that calling out the ambiguity is warranted.

I'm very open to making changes though if this doesn't work well in your use-case, or if we just need to do something to make the situation clearer.

bitcoin locked this on Dec 17, 2024