Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fail on unknown config file options #15021

Open
maflcko opened this issue Dec 21, 2018 · 6 comments
Open

Fail on unknown config file options #15021

maflcko opened this issue Dec 21, 2018 · 6 comments
Labels
Utils/log/libs

Comments

@maflcko
Copy link
Member

maflcko commented Dec 21, 2018

A 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) or the wallet. (#13799 (comment))
@maflcko maflcko mentioned this issue Jun 18, 2019
Merged
maflcko pushed a commit to maflcko/bitcoin-core that referenced this issue Jun 20, 2019
Merge bitcoin#16234: test: Add test for unknown args
ea45967 
fa7dd88 test: Add test for unknown args (MarcoFalke)

Pull request description:

  Currently uncovered.

  Further reading:

  * https://marcofalke.github.io/btc_cov/total.coverage/src/util/system.cpp.gcov.html
  *  Fail on unknown config file options bitcoin#15021

ACKs for commit fa7dd8:
  promag:
    ACK fa7dd88, tests looks good to me.
  hebasto:
    ACK fa7dd88, I have tested the code.

Tree-SHA512: 86ab370ce8e85925f945a52e81457b5678d71bbabcef01205a97782b780003f363552e0bad1ff678bccc784f82c6b511c3b88de3f8f25f62b0b713c387950564
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Jun 20, 2019
@sidhujag
Merge bitcoin#16234: test: Add test for unknown args
63f310d 
fa7dd88 test: Add test for unknown args (MarcoFalke)

Pull request description:

  Currently uncovered.

  Further reading:

  * https://marcofalke.github.io/btc_cov/total.coverage/src/util/system.cpp.gcov.html
  *  Fail on unknown config file options bitcoin#15021

ACKs for commit fa7dd8:
  promag:
    ACK fa7dd88, tests looks good to me.
  hebasto:
    ACK fa7dd88, I have tested the code.

Tree-SHA512: 86ab370ce8e85925f945a52e81457b5678d71bbabcef01205a97782b780003f363552e0bad1ff678bccc784f82c6b511c3b88de3f8f25f62b0b713c387950564
@maflcko maflcko mentioned this issue Dec 3, 2020
Closed
@luke-jr
Copy link
Member

luke-jr commented Dec 3, 2020

Concept NACK, this creates unnecessary problems switching between versions for no reason

@harding
Copy link
Contributor

harding commented Dec 3, 2020

Concept ACK. A good reason for a policy of failing on unknown settings would be eliminating the annoyance of having to figure out that your configuration is broken and then having to restart the node---which is something that can take a long time (e.g. if you have a high db cache or are using an underpowered computer) and which inexperienced users might not know how to do (or might do wrong, e.g. sending SIGKILL rather than SIGTERM, which could result in a need to reindex).

Based on my own misadventures, wrongly configuring a node is a much more common problem than wanting to use a future/spinoff configuration file with an old/baseline node, so I think we should optimize for helping users fix misconfigurations as fast and painlessly as possible.

@promag
Copy link
Member

promag commented Dec 3, 2020

I still think that a -strict option (on by default on future releases) makes everyone happy.

@promag
Copy link
Member

promag commented Dec 3, 2020 •
edited

Concept NACK, this creates unnecessary problems switching between versions for no reason

@luke-jr do you agree that very few do this?

@maflcko
Copy link
Member Author

maflcko commented Dec 3, 2020 •
edited

Switching versions happens on literally every upgrade/downgrade. Ignoring unknown config values is dangerous and may lead to loss of funds.

@luke-jr
Copy link
Member

luke-jr commented Dec 3, 2020

@promag As @MarcoFalke points out, pretty much everyone does it...

linuxsh2 pushed a commit to linuxsh2/dash that referenced this issue Aug 11, 2021
@linuxsh2
Merge bitcoin#16234: test: Add test for unknown args
0bcf027 
fa7dd88 test: Add test for unknown args (MarcoFalke)

Pull request description:

  Currently uncovered.

  Further reading:

  * https://marcofalke.github.io/btc_cov/total.coverage/src/util/system.cpp.gcov.html
  *  Fail on unknown config file options bitcoin#15021

ACKs for commit fa7dd8:
  promag:
    ACK fa7dd88, tests looks good to me.
  hebasto:
    ACK fa7dd88, I have tested the code.

Tree-SHA512: 86ab370ce8e85925f945a52e81457b5678d71bbabcef01205a97782b780003f363552e0bad1ff678bccc784f82c6b511c3b88de3f8f25f62b0b713c387950564
@maflcko maflcko mentioned this issue Jun 15, 2023
Closed
@Gitsarry Gitsarry mentioned this issue Nov 4, 2023
Closed
@maflcko maflcko mentioned this issue Apr 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@harding @luke-jr @promag @maflcko