Ignore unknown config file options; warn instead of error #13799

pull sipa wants to merge 2 commits into bitcoin:master from sipa:201807_warnunknown changing 4 files +14 −8

sipa commented at 7:31 AM on July 30, 2018: member

As reported by @satwo on IRC a few days ago, the current mechanism of treating unknown config file options as errors is problematic for options like -rpcclienttimeout which aren't defined for bitcoind.

A full solution would be to either make all binaries be aware of each other's options, or to permit config file options that only apply to specific binaries (bitcoind, bitcoin-qt, bitcoin-cli). Both of these seem too invasive to introduce for 0.17.

As a compromise, this PR makes it ignores those options, but still warn about it in the log file.
sipa force-pushed on Jul 30, 2018
Report when unknown config file options are ignored 04ce0d88ca
sipa force-pushed on Jul 30, 2018
ken2812221 commented at 7:38 AM on July 30, 2018: contributor

Related #13441
DrahtBot commented at 9:12 AM on July 30, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #13621 (Check for datadir after the config files were read by Flowdalic)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/interfaces/node.cpp:56 in 5901ca411b outdated

  52 | @@ -53,7 +53,7 @@ class NodeImpl : public Node
  53 |      {
  54 |          return gArgs.ParseParameters(argc, argv, error);
  55 |      }
  56 | -    bool readConfigFiles(std::string& error) override { return gArgs.ReadConfigFiles(error); }
  57 | +    bool readConfigFiles(std::string& error) override { return gArgs.ReadConfigFiles(error, true); }

MarcoFalke commented at 11:39 AM on July 30, 2018:

Please use named arguments for literal primitive types. (/* ignore_invalid_keys */ true)

laanwj commented at 12:48 PM on July 30, 2018: member
Tested ACK, we've discussed this on IRC and I think this is a good work-around for now.

Without:
```
$ src/bitcoind -rpcclienttimeout=1
Error parsing command line arguments: Invalid parameter -rpcclienttimeout
$ src/bitcoind -conf=/tmp/test.conf
Error reading configuration file: Invalid configuration value rpcclienttimeout
```
With:
```
$ src/bitcoind -rpcclienttimeout=1
Error parsing command line arguments: Invalid parameter -rpcclienttimeout
$ src/bitcoind -conf=/tmp/test.conf
[...]
```
(where /tmp/test.conf contains rpcclienttimeout=1)

It corrrectly rejects unknown direct arguments, but not those in the specified config file.

A full solution would be to either make all binaries be aware of each other's options

I strongly dislike this solution, it destroys all semblance of decoupling, modularity.

or to permit config file options that only apply to specific binaries (bitcoind, bitcoin-qt, bitcoin-cli).

This would be better, command-specific config sections would improve readability of configuration as well.
MarcoFalke added this to the milestone 0.17.0 on Jul 30, 2018
MarcoFalke added the label Utils/log/libs on Jul 30, 2018
MarcoFalke commented at 12:56 PM on July 30, 2018: member

utACK 5901ca411b420fe097e688425d624c7a8d9f1a30. Needs tests fixed/disabled.
promag commented at 1:11 PM on July 30, 2018: member

utACK 5901ca4.

How about hardcode the exceptions for now and keep the error behavior?
laanwj commented at 1:15 PM on July 30, 2018: member

How about hardcode the exceptions for now and keep the error behavior?

No, let's not do that. This solution is good. The implementation of errors for unknown arguments was primarily introduced for command-line arguments anyhow—see #1044—at least I never predicted the implications for configuration options, but it's a separate concern.
satwo commented at 2:06 PM on July 30, 2018: contributor

Tested ACK 5901ca411b420fe097e688425d624c7a8d9f1a30.
in src/util.cpp:867 in 5901ca411b outdated
```
 865 | +        if (!IsArgKnown(strKey)) {
 866 | +            if (!ignore_invalid_keys) {
 867 | +                error = strprintf("Invalid configuration value %s", option.first.c_str());
 868 | +                return false;
 869 | +            } else {
 870 | +                LogPrintf("Ignoring unknown configuration value %s\n", option.first);
```
achow101 commented at 6:52 PM on July 30, 2018:

This doesn't work because logging is not setup at this point in startup. The warning does not appear in the debug.log.

sipa commented at 6:59 PM on July 30, 2018:

Hmm, I thought we cached early log messages and printed them afterwards. Any suggestions for how to deal with that?

sipa commented at 7:01 PM on July 30, 2018:

Ah, #13088.
achow101 commented at 6:53 PM on July 30, 2018: member

The test feature_includeconf.py has a test case for invalid options in the conf file. That will need to be removed or be updated.
achow101 commented at 8:29 PM on July 30, 2018: member

Right now, to get around this problem, we already have a bunch of hidden arguments for the arguments from other binaries. I think it would be reasonable, at least for 0.17, to just extend that list to include the bitcoin-cli arguments. Then later we can add more invasive changes to actually fix the problem.
Ignore unknown config file options for now 247d5740d2
sipa force-pushed on Jul 31, 2018
laanwj commented at 3:25 PM on July 31, 2018: member

re-utACK 247d5740d29752c35861136a2fc561831c7e9832
laanwj commented at 3:26 PM on July 31, 2018: member

I think it would be reasonable, at least for 0.17, to just extend that list to include the bitcoin-cli arguments.

No, I don't think that's reasonable. This solution is much better (I feel like I'm repeating myself...).
MarcoFalke merged this on Jul 31, 2018
MarcoFalke closed this on Jul 31, 2018
MarcoFalke referenced this in commit 230652cafc on Jul 31, 2018
deadalnix referenced this in commit ef63e35a7c on Mar 26, 2020
NickIAm referenced this in commit 788672c1a9 on Apr 18, 2021
UdjinM6 referenced this in commit b2027ac236 on Jun 29, 2021
UdjinM6 referenced this in commit 9f4e48228b on Jun 29, 2021
UdjinM6 referenced this in commit edb52a7170 on Jul 1, 2021
UdjinM6 referenced this in commit cbb0db6936 on Jul 2, 2021
UdjinM6 referenced this in commit 8f745e57b7 on Jul 2, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.17.0

Linked (view graph)

#13821 qa: Re-enable test for unknown arg in conf file #15021 Fail on unknown config file options