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

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/ignoredconf changing 11 files +189 −20
  1. 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.

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

  3. fanquake requested review from pinheadmz on Mar 23, 2023
  4. ryanofsky force-pushed on Mar 23, 2023
  5. 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

    0tuple[] 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

    0common/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

    0C:\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]
1          with
2          [
3              _Alloc=std::allocator<char>
4          ]
5C:\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

    0src/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

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

  7. ryanofsky force-pushed on Mar 23, 2023
  8. 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:

     0Traceback (most recent call last):
 1  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
 2    self.run_test()
 3  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 345, in run_test
 4    self.test_ignored_conf()
 5  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 297, in test_ignored_conf
 6    pathlib.Path(temp_conf.name).write_text(f"datadir={node.datadir}\n")
 7  File "C:\Python39\lib\pathlib.py", line 1275, in write_text
 8    with self.open(mode='w', encoding=encoding, errors=errors) as f:
 9  File "C:\Python39\lib\pathlib.py", line 1242, in open
10    return io.open(self, mode, buffering, encoding, errors, newline,
11  File "C:\Python39\lib\pathlib.py", line 1110, in _opener
12    return self._accessor.open(self, flags, mode)
13PermissionError: [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

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

  10. ryanofsky force-pushed on Mar 23, 2023
  11. 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:

     0TestFramework (ERROR): Assertion failed
 1Traceback (most recent call last):
 2  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
 3    self.run_test()
 4  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 346, in run_test
 5    self.test_ignored_conf()
 6  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 299, in test_ignored_conf
 7    node.assert_start_raises_init_error([f"-conf={temp_conf.name}"], re.escape(
 8  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
 9    self._raise_assertion_error(
10  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_node.py", line 173, in _raise_assertion_error
11    raise AssertionError(self._node_msg(msg))
12AssertionError: [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:
13"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

  12. ryanofsky force-pushed on Mar 23, 2023
  13. 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:

     0Traceback (most recent call last):
 1  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
 2    self.run_test()
 3  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 346, in run_test
 4    self.test_ignored_default_conf()
 5  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\feature_config_args.py", line 330, in test_ignored_default_conf
 6    node.assert_start_raises_init_error([], re.escape(
 7  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
 8    self._raise_assertion_error(assert_msg)
 9  File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_node.py", line 173, in _raise_assertion_error
10    raise AssertionError(self._node_msg(msg))
11AssertionError: [node 0] bitcoind should have exited within 480s with expected error
  14. ryanofsky force-pushed on Mar 23, 2023
  15. 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
  16. 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.

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

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

  19. 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.
  20. ryanofsky commented at 4:05 pm on March 24, 2023: contributor
    Thanks for the review
  21. 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.

  22. TheCharlatan commented at 1:36 pm on March 27, 2023: contributor
    Concept ACK
  23. 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

    1. Why is it ignored?
    2. Any downsides for using bitcoin.conf from datadir if conf argument isn’t used?
  24. 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

  25. ryanofsky force-pushed on Mar 27, 2023
  26. 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
  27. 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

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

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

  28. pinheadmz approved
  29. 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.

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK ac9fee615a4f0c4d1bbed0d69486c54be4860dcb
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmQh374ACgkQ5+KYS2KJ
 7yTqaMRAAzyu11syUK7aDjZBg4ofpo0Juk6BQXjg5MZ8/b3ThNoTNt2oqHv2UXI6S
 8mcZk+4pBfZoM9p0qG1LknMpzjuaISjH6PbuUHa2eGofkPGf0kuhBYBdL2hPwdD8g
 9VhV8nG4B96dIVwRx24JRR+lpK2Cgklo+c1Qk6MQKhProK1SuvKoJ3lk+CWKjyEuV
10zuicbRt73D+0vgfrqKMFrVQQ60qtfRbct2f2HSP911wBl0CaNOCxARpOfVk7phLf
11ebtqYNPuhPipFAKxMTzJfw1E6tGSVauAHieWFJXKIUTbflQCrcnq0aN/5ruJs2UL
12kDuHBH2At2FwjJvJuwuT0yJPhPdp8iqpwMgEia/4uubrvokxIra0EoRKuvfzmr9H
133YmaeYgk+ud9NupXmxxiDOT4+BZIabLTMswDQ9DNP9laSKiweSZuNB5HTj+MIbEV
14ZvT6s7xay1cSIbODy8j8uYonAMifa1yV/iNfztFPnJXvnQfRZz2+xJ0Lhxil/UvA
15OZi5dppCAHt0KXteMLeQ70nu8zfasKk+aA6c4OLXeGlOXqxeZji4IWmvRwCrzoWg
164+HKz2iXC5N+ZlL9oM9TMcwM7i1EILKbeXGGMM5F10S1F3vtEPwUBeMdLePONXGU
17IfkoysTVFSDUe5tNbewM081axGf0Wp43kozNCXTX2pkz3al4BXc=
18=n8np
19-----END PGP SIGNATURE-----

    pinheadmz’s public key is on keybase

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

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

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

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

    02023-03-27T21:22:09Z Default data directory /home/will/.bitcoin
12023-03-27T21:22:09Z Using data directory /tmp/bitcoin_27302/regtest
22023-03-27T21:22:09Z Config file: /tmp/bitcoin_27302/bitcoin.conf
32023-03-27T21:22:09Z Config file arg: datadir="/tmp/bitcoin_27302/datadir2"
42023-03-27T21:22:09Z Config file arg: regtest="1"
52023-03-27T21:22:09Z Command-line arg: datadir="/tmp/bitcoin_27302/"
62023-03-27T21:22:09Z Command-line arg: port="18556"
72023-03-27T21:22:09Z Command-line arg: rpcport="18555"
82023-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:

     0will@ubuntu in /tmp/bitcoin_27302
 1₿ tree
 2.
 3├── bitcoin.conf
 4├── datadir2
 5│   └── bitcoin.conf
 6└── regtest
 7    ├── banlist.json
 8    ├── bitcoind.pid
 9    ├── blocks
10    │   ├── blk00000.dat
11    │   ├── index
12    │   │   ├── 000015.log
13    │   │   ├── 000016.ldb
14    │   │   ├── CURRENT
15    │   │   ├── LOCK
16    │   │   └── MANIFEST-000013
17    │   └── rev00000.dat
18    ├── chainstate
19    │   ├── 000005.ldb
20    │   ├── 000025.ldb
21    │   ├── 000027.log
22    │   ├── CURRENT
23    │   ├── LOCK
24    │   └── MANIFEST-000026
25    ├── debug.log
26    ├── fee_estimates.dat
27    ├── mempool.dat
28    ├── onion_v3_private_key
29    ├── peers.dat
30    ├── settings.json
31    └── wallets
32
336 directories, 23 files
34
35will@ubuntu in /tmp/bitcoin_27302
36₿ cat bitcoin.conf
37regtest=1
38datadir=/tmp/bitcoin_27302/datadir2
39
40will@ubuntu in /tmp/bitcoin_27302
41₿ cat datadir2/bitcoin.conf
42port=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:

    0₿ ./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:

    0₿ ./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.
  33. 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.

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

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

  36. willcl-ark approved
  37. willcl-ark commented at 12:57 pm on March 28, 2023: contributor

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

  38. 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.
  39. RandyMcMillan commented at 1:58 pm on March 28, 2023: contributor
    Concept ACK
  40. 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.

  41. DrahtBot added the label Needs rebase on Apr 3, 2023
  42. ryanofsky force-pushed on Apr 5, 2023
  43. ryanofsky commented at 0: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
  44. DrahtBot removed the label Needs rebase on Apr 5, 2023
  45. ryanofsky force-pushed on Apr 5, 2023
  46. DrahtBot added the label Needs rebase on Apr 21, 2023
  47. 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
  48. 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
  49. 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
  50. ryanofsky force-pushed on Apr 21, 2023
  51. DrahtBot removed the label Needs rebase on Apr 21, 2023
  52. DrahtBot added the label CI failed on Apr 21, 2023
  53. ryanofsky force-pushed on Apr 25, 2023
  54. DrahtBot removed the label CI failed on Apr 25, 2023
  55. TheCharlatan approved
  56. TheCharlatan commented at 10:35 am on May 15, 2023: contributor
    ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef
  57. DrahtBot requested review from pinheadmz on May 15, 2023
  58. DrahtBot requested review from willcl-ark on May 15, 2023
  59. 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: 
    0- `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

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

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

  62. pinheadmz approved
  63. 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)

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRifEUACgkQ5+KYS2KJ
 7yToIABAArY2mwOFAHYDCg/86hxjzZtiKV7CzsAN2G5639KH/HhMYASg+Lo+RU9jU
 8okayFjZpunwnKaKoePSFAVMUhIMSmRQTfq0gYaYJoamft/2jb4ZK4Ea8f/ICMrfA
 9y+rNReXmG3ViMrzXUFq67A208VT96/jGn1KxVQLahCIeT3bN8zYlhgHk95CERa9B
106s5P9ScHbq/UJwMLuQC7xn49blh2IiLdE9LezaTDiGVSBRgh8baqGKYGW33RWmDO
111ObKjVe7VxHQ0fAfA+tDk0q9KQT80Meh78m1DnNe+4a/TU7KDtRMjqk76FUXKwuI
12DrTHWmrgfjwmhgHoiQHBOc1sUTgH4FwGuubkgqayBMFga6lm+PAHtVeg3LmC5CVl
13KCkNqst3kdhunbnwkkbUAM1tedc+CmUra0HsdIWXmoEM+yVJpJK6Mzzx5YBgBUeZ
14l/vG+if4QQaazny1b1sFCRCyN5ARnhBWQ/scHBH7dV2T1EHh/+wFGG3rTySxPA/D
15M21QRkBGJ7j0vA5UMUkPtKP43bKbV/QtyCgagSxtdfcWYtts78vavwo6MkiS1LY6
16RJWV2S9IoIZ9giBW3a9H1xDakgc8JxdDB7jAO6UL5QKmOwBNSFHDjjoVNyJafOLr
17P7065vXz/NcdMJOq3wqApvebZgb0MbedSZYVWUk/xrSaXVnL//c=
18=dV5U
19-----END PGP SIGNATURE-----

    pinheadmz’s public key is on keybase

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

    0$ ./src/bitcoind -regtest -allowignoredconf=1
12023-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:
2- Delete or rename the "bitcoin.conf" file in data directory "/home/hebasto/TEST".
3- Change datadir= or conf= options to specify one configuration file, not two, and use includeconf= to include any other configuration files.
4- Set allowignoredconf=1 option to treat this condition as a warning, not an error.
5...
    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.

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

  66. hebasto commented at 2:11 pm on May 22, 2023: member
    Approach ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef.
  67. ryanofsky force-pushed on May 23, 2023
  68. 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!

  69. DrahtBot added the label CI failed on May 23, 2023
  70. pinheadmz approved
  71. 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.

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRucOIACgkQ5+KYS2KJ
 7yTpqZg//YoQc3ivctelv3QQLCGcGruDO9twOQMm8TRx+8mZRtIV2UqPom2pwLjXM
 8BnZ2oWvd5k/8bX12q7p5kWRdWuAu77pZ/OltrJp/+PstFh+G9J/uWaS7y7bxPICb
 9zfeslfAhywVQzHsPPLlo1KF0WJY2TfdFRZwmcL+qJ9Jhy1iQLxrMUb3cdbUeLwsa
10km9UlJWmRBNF60kD79YY+pPiLNET45CSfvFlJIMk4cb+vXdVtwYqq+XOxLPLLJ0R
11bUHclTDTgoRJTzMGdsiwoTq3wCVx2WMzkzEyX+3KwS4MJZjke58wqwVP1UOQ7pFv
12OMGAOk4kZWOqQ9JSKTu4Abbq+K64FXXsCZuDFRz31dUiCoSPBbppTIP7dQi711Fz
13YT2ICOvpI8CXcWUdlaBKJWC+6JuNyjfNb16GD5jq77ylzUNMsZLL9Glg293zwGHk
14T48wE/MHxPmp1qyU/EDdg1mM/o0AoXrtMdi2smYROw/tQwsT81AFpoADdSJFkbog
155nUMNR5bNBiUnT/UyZ73WTkZ5/kjRgc1kDbfCaPvQhJYU7gVo9xGZ8sLBemmgDS6
16tREcNf5nHno1vc8ctrE0eOK3h+ndiXOp8LwVmS92UX/tCB35xphONfbejmhlCaVn
17AiejqtCmuBnG3yPcdIXxPV9EA0iSI3PJzIjuTfJLT9AD2BBWbLg=
18=e+a2
19-----END PGP SIGNATURE-----

    pinheadmz’s public key is on keybase

  72. DrahtBot requested review from TheCharlatan on May 25, 2023
  73. willcl-ark approved
  74. willcl-ark commented at 9:53 am on May 25, 2023: contributor
    re-ACK eefe56967b
  75. TheCharlatan approved
  76. TheCharlatan commented at 8:39 am on May 26, 2023: contributor
    ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
  77. fanquake merged this on May 26, 2023
  78. fanquake closed this on May 26, 2023
  79. sidhujag referenced this in commit 9fc4472e0e on May 26, 2023
  80. pinheadmz commented at 3:03 pm on June 2, 2023: member
    @ryanofsky does this close #27246 ?
  81. 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
  82. luke-jr referenced this in commit a7afcf229c on Aug 16, 2023
  83. luke-jr referenced this in commit d0000b6c55 on Aug 16, 2023
  84. luke-jr referenced this in commit b64ca49659 on Aug 16, 2023
  85. 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:

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

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

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


ryanofsky DrahtBot hebasto willcl-ark pinheadmz TheCharlatan ghost RandyMcMillan AdamISZ

Labels
CI failed

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me