Ignore unknown config file options; warn instead of error by sipa · Pull Request #13799 · bitcoin/bitcoin

sipa · 2018-07-30T07:31:35Z

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.

ken2812221 · 2018-07-30T07:38:20Z

Related #13441

DrahtBot · 2018-07-30T09:12:59Z

Note to reviewers: This pull request conflicts with the following ones:

Check for datadir after the config files were read #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.

maflcko · 2018-07-30T11:39:54Z

src/interfaces/node.cpp

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

laanwj · 2018-07-30T12:48:50Z

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.

maflcko · 2018-07-30T12:56:09Z

utACK 5901ca411b420fe097e688425d624c7a8d9f1a30. Needs tests fixed/disabled.

promag · 2018-07-30T13:11:23Z

utACK 5901ca4.

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

laanwj · 2018-07-30T13:15:39Z

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.

ghost · 2018-07-30T14:06:23Z

Tested ACK 5901ca411b420fe097e688425d624c7a8d9f1a30.

achow101

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 · 2018-07-30T18:52:40Z

src/util.cpp

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

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

Ah, #13088.

achow101 · 2018-07-30T20:29:10Z

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.

laanwj · 2018-07-31T15:25:32Z

re-utACK 247d574

laanwj · 2018-07-31T15:26:09Z

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

247d574 Ignore unknown config file options for now (Pieter Wuille) 04ce0d8 Report when unknown config file options are ignored (Pieter Wuille) Pull request description: 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. Tree-SHA512: dfddc771b91df3031a9c98d9f3292f8f4fcd1b97ebb7317b2f457e12d9f205dc63f42721302e7258dbb53f273d7cc041a65a0a9120972769555784e1f1cc9aef

Summary: ``` 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. ``` Backport of core [[bitcoin/bitcoin#13799 | PR13799]]. Depends on D5553 (it is buggy otherwise). Test Plan: ninja all check ./test/functional/test_runner.py feature_includeconf Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D5554

… of error 247d574 Ignore unknown config file options for now (Pieter Wuille) 04ce0d8 Report when unknown config file options are ignored (Pieter Wuille) Pull request description: 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. Tree-SHA512: dfddc771b91df3031a9c98d9f3292f8f4fcd1b97ebb7317b2f457e12d9f205dc63f42721302e7258dbb53f273d7cc041a65a0a9120972769555784e1f1cc9aef

sipa force-pushed the 201807_warnunknown branch from b8a02f6 to a17ca7a Compare July 30, 2018 07:32

Report when unknown config file options are ignored

04ce0d8

sipa force-pushed the 201807_warnunknown branch from a17ca7a to 5901ca4 Compare July 30, 2018 07:36

DrahtBot mentioned this pull request Jul 30, 2018

Check for datadir after the config files were read #13621

Closed

maflcko reviewed Jul 30, 2018

View reviewed changes

src/interfaces/node.cpp Outdated

Copy link

Member

maflcko Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

maflcko added this to the 0.17.0 milestone Jul 30, 2018

maflcko added the Utils/log/libs label Jul 30, 2018

achow101 reviewed Jul 30, 2018

View reviewed changes

Ignore unknown config file options for now

247d574

sipa force-pushed the 201807_warnunknown branch from 5901ca4 to 247d574 Compare July 31, 2018 01:01

maflcko merged commit 247d574 into bitcoin:master Jul 31, 2018

maflcko mentioned this pull request Jul 31, 2018

qa: Re-enable test for unknown arg in conf file #13821

Closed

Bushstar mentioned this pull request Aug 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#358

Merged

jlopp mentioned this pull request Oct 28, 2018

Don't silently ignore invalid arguments in monerod.conf monero-project/monero#4749

Closed

maflcko mentioned this pull request Dec 21, 2018

Fail on unknown config file options #15021

Open

AkioNak mentioned this pull request Jan 10, 2019

Error if rpcpassword contains hash in conf sections #15087

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

sipa commented Jul 30, 2018

Uh oh!

ken2812221 commented Jul 30, 2018

Uh oh!

DrahtBot commented Jul 30, 2018

Uh oh!

maflcko Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jul 30, 2018

Uh oh!

maflcko commented Jul 30, 2018

Uh oh!

promag commented Jul 30, 2018

Uh oh!

laanwj commented Jul 30, 2018

Uh oh!

ghost commented Jul 30, 2018

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

sipa Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

sipa Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jul 31, 2018

Uh oh!

laanwj commented Jul 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

achow101 commented Jul 30, 2018 •

edited

Loading