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

ryanofsky · 2023-03-22T17:38:03Z

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 · 2023-03-22T17:38:06Z

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.

ryanofsky · 2023-03-23T15:11:06Z

CI's hate this one weird PR.

Updated c661be4 -> 780c696 (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 · 2023-03-23T15:42:42Z

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 · 2023-03-23T16:25:09Z

Updated 780c696 -> 53d9955 (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

willcl-ark · 2023-03-23T16:38:56Z

src/common/init.cpp

+                "- 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 "


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?

re: #27302 (comment)

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.

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.

re: #27302 (comment)

Another name could be allowignoredconf if that is better

allowignoredconf

yeah

re: #27302 (comment)

allowignoredconf

yeah

Renamed warnignoredconf to allowignoredconf

ryanofsky · 2023-03-23T18:34:14Z

Updated 53d9955 -> 9723357 (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 · 2023-03-23T21:04:24Z

Updated 9723357 -> d00762c (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 · 2023-03-23T23:44:33Z

Updated d00762c -> 37ffeca (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

pinheadmz

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.

pinheadmz · 2023-03-24T15:08:42Z

src/common/init.cpp

+                "- 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 "


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.

src/common/init.cpp

pinheadmz · 2023-03-24T15:25:52Z

test/functional/feature_config_args.py

@@ -332,5 +390,21 @@ def run_test(self):
        assert os.path.exists(os.path.join(new_data_dir_2, self.chain, 'blocks'))


+def get_temp_default_datadir(temp_dir: pathlib.Path) -> Tuple[dict, pathlib.Path]:


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?

re: #27302 (comment)

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.

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.

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

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

bitcoin/test/functional/test_framework/util.py

Lines 417 to 418 in b759cef

def get_datadir_path(dirname, n):

return os.path.join(dirname, "node" + str(n))

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

re: #27302 (comment)

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

Sure, moved there now

src/common/init.cpp

ryanofsky

Thanks for the review

src/common/init.cpp

ryanofsky · 2023-03-24T16:01:44Z

test/functional/feature_config_args.py

@@ -332,5 +390,21 @@ def run_test(self):
        assert os.path.exists(os.path.join(new_data_dir_2, self.chain, 'blocks'))


+def get_temp_default_datadir(temp_dir: pathlib.Path) -> Tuple[dict, pathlib.Path]:


re: #27302 (comment)

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.

ryanofsky · 2023-03-24T16:03:01Z

src/common/init.cpp

+                "- 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 "


re: #27302 (comment)

Another name could be allowignoredconf if that is better

TheCharlatan

Concept ACK

src/common/init.cpp

ghost · 2023-03-27T15:21:11Z

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 · 2023-03-27T15:40:53Z

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

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

src/common/init.cpp

ryanofsky · 2023-03-27T17:18:08Z

src/common/init.cpp

+                "- 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 "


re: #27302 (comment)

allowignoredconf

yeah

Renamed warnignoredconf to allowignoredconf

ghost · 2023-03-27T17:55:01Z

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

ACK ac9fee6

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.

Show Signature

pinheadmz's public key is on keybase

pinheadmz · 2023-03-27T18:26:00Z

test/functional/feature_config_args.py

@@ -332,5 +390,21 @@ def run_test(self):
        assert os.path.exists(os.path.join(new_data_dir_2, self.chain, 'blocks'))


+def get_temp_default_datadir(temp_dir: pathlib.Path) -> Tuple[dict, pathlib.Path]:


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

ryanofsky · 2023-03-27T18:34:43Z

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 · 2023-03-27T18:59:07Z

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.

src/common/init.cpp

ryanofsky · 2023-03-28T01:35:22Z

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

TheCharlatan

ACK 0319de5

pinheadmz

ACK 0319de5

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)

Show Signature

pinheadmz's public key is on keybase

pinheadmz · 2023-05-15T15:55:48Z

doc/release-notes-27302.md

+Configuration
+---
+
+- `bitcoind` and `bitcoin-qt` will now raise an error on startup if a datadir that is being used contains a bitcoin.conf file that 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.


Suggested change

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

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

re: #27302 (comment)

Thanks! Applied change

pinheadmz · 2023-05-15T17:19:01Z

test/functional/test_framework/util.py

+    else:
+      env = dict(HOME=str(temp_dir))
+      if sys.platform == "darwin":
+          datadir = temp_dir / "Library/Application Support/Bitcoin"
+      else:
+          datadir = temp_dir / ".bitcoin"


nit: indent should be +2 more spaces

re: #27302 (comment)

nit: indent should be +2 more spaces

Thanks! Fixed this

src/common/init.cpp

hebasto

Approach ACK 0319de5.

src/common/init.cpp

ryanofsky

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

Thanks for the reviews!

ryanofsky · 2023-04-04T21:10:50Z

test/functional/feature_config_args.py

@@ -282,6 +289,55 @@ def test_connect_with_seednode(self):
                    unexpected_msgs=seednode_ignored):
                self.restart_node(0, extra_args=[connect_arg, '-seednode=fakeaddress2'])

+    def test_ignored_conf(self):
+        self.log.info('Test error is triggered when datadir contains a bitcoin.conf file that would be ignored '


re: #27302 (comment)

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

test/functional/feature_config_args.py

ryanofsky · 2023-04-04T23:40:02Z

test/functional/feature_config_args.py

@@ -332,5 +390,21 @@ def run_test(self):
        assert os.path.exists(os.path.join(new_data_dir_2, self.chain, 'blocks'))


+def get_temp_default_datadir(temp_dir: pathlib.Path) -> Tuple[dict, pathlib.Path]:


re: #27302 (comment)

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

Sure, moved there now

ryanofsky · 2023-05-23T14:35:51Z

doc/release-notes-27302.md

+Configuration
+---
+
+- `bitcoind` and `bitcoin-qt` will now raise an error on startup if a datadir that is being used contains a bitcoin.conf file that 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.


re: #27302 (comment)

Thanks! Applied change

src/common/init.cpp

ryanofsky · 2023-05-23T14:36:59Z

test/functional/test_framework/util.py

+    else:
+      env = dict(HOME=str(temp_dir))
+      if sys.platform == "darwin":
+          datadir = temp_dir / "Library/Application Support/Bitcoin"
+      else:
+          datadir = temp_dir / ".bitcoin"


re: #27302 (comment)

nit: indent should be +2 more spaces

Thanks! Fixed this

pinheadmz

re-ACK eefe569

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

Show Signature

pinheadmz's public key is on keybase

willcl-ark

re-ACK eefe569

TheCharlatan

ACK eefe569

pinheadmz · 2023-06-02T15:03:32Z

@ryanofsky does this close #27246 ?

ryanofsky · 2023-06-02T15:51:17Z

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

…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 Github-Pull: bitcoin#27302 Rebased-From: 398c371

Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen: - One case reported in bitcoin#27246 (comment) happens when a bitcoin.conf file in the default datadir (e.g. $HOME/.bitcoin/bitcoin.conf) has a "datadir=/path" line that sets different datadir containing a second bitcoin.conf file. Currently the second bitcoin.conf file is ignored with no warning. - Another way this could happen is if a -conf= command line argument points to a configuration file with a "datadir=/path" line and that specified path contains a bitcoin.conf file, which is currently ignored. This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant -datadir or -conf settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored. Github-Pull: bitcoin#27302 Rebased-From: 3746f00

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 bitcoin#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> Github-Pull: bitcoin#27302 Rebased-From: eefe569

AdamISZ · 2023-12-16T21:00:12Z

@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 😄

ryanofsky · 2023-12-18T15:58:07Z

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 · 2023-12-18T16:17:58Z

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 · 2023-12-18T17:02:01Z

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 mentioned this pull request Mar 22, 2023

Bitcoin ignores datadir and blocksdir parameter in .conf #27246

Closed

1 task

pinheadmz mentioned this pull request Mar 22, 2023

system: cache config file path before potentially updating datadir #27303

Closed

fanquake requested a review from pinheadmz March 23, 2023 11:35

ryanofsky force-pushed the pr/ignoredconf branch from c661be4 to 780c696 Compare March 23, 2023 15:05

ryanofsky force-pushed the pr/ignoredconf branch from 780c696 to 53d9955 Compare March 23, 2023 16:23

willcl-ark reviewed Mar 23, 2023

View reviewed changes

ryanofsky force-pushed the pr/ignoredconf branch from 53d9955 to 9723357 Compare March 23, 2023 18:33

ryanofsky force-pushed the pr/ignoredconf branch from 9723357 to d00762c Compare March 23, 2023 21:01

ryanofsky force-pushed the pr/ignoredconf branch from d00762c to 37ffeca Compare March 23, 2023 23:42

pinheadmz reviewed Mar 24, 2023

View reviewed changes

ryanofsky commented Mar 24, 2023

View reviewed changes

TheCharlatan reviewed Mar 27, 2023

View reviewed changes

src/common/init.cpp Show resolved Hide resolved

ryanofsky force-pushed the pr/ignoredconf branch from 37ffeca to ac9fee6 Compare March 27, 2023 17:43

ryanofsky commented Mar 27, 2023

View reviewed changes

pinheadmz approved these changes Mar 27, 2023

View reviewed changes

willcl-ark reviewed Mar 27, 2023

View reviewed changes

src/common/init.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Mar 27, 2023

refactor: Extract util/fs from util/system #27254

Merged

willcl-ark mentioned this pull request Apr 26, 2023

Relative paths named in the -conf parameter reset when parsing datadir in named config #19990

Closed

TheCharlatan approved these changes May 15, 2023

View reviewed changes

DrahtBot requested review from pinheadmz and willcl-ark May 15, 2023 10:35

pinheadmz approved these changes May 15, 2023

View reviewed changes

hebasto reviewed May 22, 2023

View reviewed changes

src/common/init.cpp Show resolved Hide resolved

src/common/init.cpp Show resolved Hide resolved

ryanofsky force-pushed the pr/ignoredconf branch from 0319de5 to eefe569 Compare May 23, 2023 15:35

ryanofsky commented May 23, 2023

View reviewed changes

DrahtBot added the CI failed label May 23, 2023

pinheadmz approved these changes May 24, 2023

View reviewed changes

DrahtBot requested a review from TheCharlatan May 25, 2023 06:06

willcl-ark approved these changes May 25, 2023

View reviewed changes

TheCharlatan approved these changes May 26, 2023

View reviewed changes

fanquake merged commit 66b08e7 into bitcoin:master May 26, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 26, 2023

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

9fc4472

bitcoin locked and limited conversation to collaborators Dec 17, 2024

		@@ -332,5 +390,21 @@ def run_test(self):
		assert os.path.exists(os.path.join(new_data_dir_2, self.chain, 'blocks'))


		def get_temp_default_datadir(temp_dir: pathlib.Path) -> Tuple[dict, pathlib.Path]:

	def get_datadir_path(dirname, n):
	return os.path.join(dirname, "node" + str(n))

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

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

Uh oh!

Conversation

ryanofsky commented Mar 22, 2023

Uh oh!

DrahtBot commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

ryanofsky commented Mar 23, 2023

Uh oh!

hebasto commented Mar 23, 2023

Uh oh!

ryanofsky commented Mar 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Mar 23, 2023

Uh oh!

ryanofsky commented Mar 23, 2023

Uh oh!

ryanofsky commented Mar 23, 2023

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Mar 27, 2023

Uh oh!

ryanofsky commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 27, 2023

DrahtBot commented Mar 22, 2023 •

edited

Loading

ryanofsky commented Mar 27, 2023 •

edited

Loading