-
Notifications
You must be signed in to change notification settings - Fork 38.7k
util: Forbid ambiguous multiple assignments in config file #17493
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/17493. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
JeremyRubin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK.
Curious if you think this has a high likelihood of breaking a lot of configs in the wild -- but I suppose it's worth it, as anything which could be intentionally repeated gets the new flag.
One interesting point: if something is not allow list, but receives a new param that is identical to the last set one, we could allow it? Something like this could be useful for certain bool settings if config files are being merged.
Noting for reference tracking that #12763 will require an update after this is merged.
src/util/system.cpp
Outdated
| if (flags) { | ||
| if (!CheckValid(key, value, flags, error)) { | ||
| if (!(*flags & ALLOW_LIST) && m_settings.ro_config[section].count(key)) { | ||
| error = strprintf("Multiple values specified for -%s in same section of config file.", key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps only err if the value differs?
This commit has no effect on behavior because as of bitcoin#17493 it's not possible to specify multiple values for single value settings in the config file.
This commit has no effect on behavior because as of bitcoin#17493 it's not possible to specify multiple values for single value settings in the config file.
|
Weak concept NACK on this, precedence in config files might be confusing but it's not ambiguous and specifying |
This description sounds a little misleading to me. It is true that if someone had an ambiguous config file, and they updated to a new bitcoin version and then restarted without testing, they would see a "Multiple values specified for -foo in same section of config file" init error. But there wouldn't be a question of correctness, or risk that the new version would start up using different settings than the old version. Personally, I don't think a debug log warning would be as good as an error here, given experience with issues like #15629 where warnings have seemed pretty easy to miss. I understand your concern about the case where someone upgrades their version of bitcoin and sees an error about an insignificant ambiguity in their config file. But I wouldn't want to forget about cases where someone could upgrade bitcoin be prompted to fix config errors that actually affected their privacy or security. I wonder what you think about Jeremy's suggestion to prevent errors in more spurious cases where a setting is repeated but the value doesn't change: #17493 (review). It'd be pretty easy to tweak the PR to avoid triggering errors in these cases.-- Rebased 98e2bba -> 68d1b62 ( Rebased 68d1b62 -> 20119c3 ( |
Sure; I meant "restart correctly" as in "keep working the way things are supposed to rather than gratuitously break and cause the sysadmin headaches" :) But yeah, I agree a warning everyone will miss isn't that helpful, and there aren't any better approaches coming to mind; really that's what release notes and being careful about upgrades are for. I'd probably wait to see someone using it in real life before adding special cases for duplicate settings with the same param/value. |
39207ee to
77623d1
Compare
This commit has no effect on behavior because as of bitcoin#17493 it's not possible to specify multiple values for single value settings in the config file.
This commit has no effect on behavior because as of bitcoin#17493 it's not possible to specify multiple values for single value settings in the config file.
This commit has no effect on behavior because as of bitcoin#17493 it's not possible to specify multiple values for single value settings in the config file.
This commit has no effect on behavior because as of bitcoin#17493 it's not possible to specify multiple values for single value settings in the config file.
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Implement cleanup suggestions from l0rinc: bitcoin#16545 (comment) bitcoin#16545 (comment) bitcoin#16545 (comment)
This commit just adds documentation for the type flags. The flags are actually implemented in the following two commits.
… startup This commit implements support for new ALLOW_BOOL, ALLOW_INT, ALLOW_STRING, and ALLOW_LIST flags by validating settings with these flags earlier on startup and providing detailed error messages to users. The new flags implement stricter error checking than ALLOW_ANY. For example, a double negated option like -nosetting=0 is treated like an error instead of true, and an unrecognized bool value like -setting=true is treated like an error instead of false. And if a non-list setting is assigned multiple times in the same section of a configuration file, the later assignments trigger errors instead of being silently ignored. The new flags also provide type information that allows ArgsManager GetSettings() and GetSettingsList() methods to return typed integer and boolean values instead of unparsed strings. The changes in this commit have no effect on current application behavior because the new flags are only used in unit tests. The existing ALLOW_ANY checks in the argsman_tests/CheckValueTest confirm that no behavior is changing for current settings, which use ALLOW_ANY.
…LLOW flags Update GetArg, GetArgs, GetBoolArg, and GetIntArg helper methods to work conveniently with ALLOW_BOOL, ALLOW_INT, and ALLOW_STRING flags. The GetArg methods are convenience wrappers around the GetSetting method. The GetSetting method returns the originally parsed settings values in their declared bool/int/string types, while the GetArg wrappers provide extra type-coercion and default-value fallback features as additional conveniences for callers. This commit makes two changes to GetArg, GetArgs, GetBoolArg, and GetIntArg helper methods when BOOL/INT/STRING flags are used: 1. GetArg methods will now raise errors if they are called with inconsistent flags. For example, GetArgs will raise a logic_error if it is called on a non-LIST setting, GetIntArg will raise a logic_error if it is called on a non-INT setting. 2. GetArg methods will now avoid various type coersion footguns when they are called on new BOOL/INT/STRING settings. Existing ALLOW_ANY settings are unaffected. For example, negated settings will return "" empty strings instead of "0" strings (in the past the "0" strings caused strangeness like "-nowallet" options creating wallet files named "0"). The new behaviors are fully specified and checked by the `CheckValueTest` unit test. The ergonomics of the GetArg helper methods are subjective and the behaviors they implement can be nitpicked and debated endlessly. But behavior of these helper methods does not dictate application behavior, and they can be bypassed by calling GetSetting and GetSettingList methods instead. If it's necessary, behavior of these helper methods can also be changed again in the future. The changes have no effect on current application behavior because the new flags are only used in unit tests. The `setting_args` unit test and ALLOW_ANY checks in the `CheckValueTest` unit test are unchanged and confirm that `GetArg` methods behave the same as before for ALLOW_ANY flags (returning the same values and throwing the same exceptions).
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
The type flags aren't currently used to validate or convert settings in the settings.json file, but they should be in the future. Add test to check current behavior that can be extended when flags are applied. Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Let ALLOW_STRING and ALLOW_INT flags be combined with ALLOW_BOOL so string and int options can be specified without explicit values. This is useful for imperative settings that trigger new behavior when specified and can accept optional string or integer values, but do not require them. (For examples, see the example_options unit test modified in this commit.)
This change has no effect on behavior, and is basically just a documentation
change at this point. The ALLOW_LIST flag is currently ignored unless
ALLOW_BOOL, ALLOW_INT, or ALLOW_STRING flags are also present, and these flags
are not used yet.
-BEGIN VERIFY SCRIPT-
for f in `git grep -n 'GetArgs(' | grep -v _tests | sed -n 's/.*GetArgs("\([^"]\+\)".*/\1/p' | sort -u`; do
git grep -l -- "$f" | xargs sed -i "/AddArg(\"$f[=\"]/ s/ArgsManager::ALLOW_ANY/& | ArgsManager::ALLOW_LIST/g"
done
-END VERIFY SCRIPT-
- Remove ALLOW_LIST flag from bitcoin-wallet -wallet and -debug arguments. They are list arguments for bitcoind, but single arguments for bitcoin-wallet. - Add ALLOW_LIST flag to -includeconf arg (missed by scripted diff since it's not accessed through GetArgs) - Add ALLOW_LIST flag to -proxy, -debug, -loglevel, -whitebind, and -whitelist args (missed by scripted diff due to line breaks in AddArgs calls) - Add ALLOW_LIST flag to -zmq args (missed by scripted diff due to programmatic GetArgs calls) This change has no effect on behavior, and is basically just a documentation change at this point. The ALLOW_LIST flag is currently ignored unless ALLOW_BOOL, ALLOW_INT, or ALLOW_STRING flags are also present, and these flags are not used yet.
Previous behavior was inconsistent: if -blockfilterindex or -blockfilterindex="" arguments were specified they would normally enable all block filter indexes, but could also trigger "Unknown -blockfilterindex value" errors if followed by later -blockfilterindex arguments. It was confusing that the same -blockfilterindex options could sometime trigger errors and sometimes not depending on option position. It was also confusing that an empty -blockfilterindex="" setting could enable all indexes even though indexes are disabled by default. New behavior is more straightforward: - -blockfilterindex and -blockfilterindex=1 always enable indexes - -noblockfilterindex and -blockfilterindex=0 always disable indexes - -blockfilterindex="" is always an unknown value error The meaning of these options no longer changes based on option position.
Upcoming commits will make it an error to call GetArg and IsArgSet methods on list options since these usages are error prone. For example GetArg will return last command line value but first config value in the list, and IsArgSet will return true even if the list is empty if the list was negated. This change is just a refactoring replacing problematic ArgsManager calls with equivalent calls to avoid changing any behavior. Current behavior could probably be improved in these cases, but this change should make new problems less likely to be introduced.
Prevent GetArg() from being called on ALLOW_LIST arguments, and GetArgs() from being called on non-list arguments. This checking was previously skipped unless typed INT/BOOL/STRING flags were present, but now it's always done. This change has no effect on external behavior. It is just supposed to enforce internal consistency and prevent bugs caused by using the wrong GetArg method to retrieve settings.
Enable error "Multiple values specified for -setting in same section of config file.", for ALLOW_ANY settings that don't specify ALLOW_LIST. Instead of silently ignoring settings, this change makes it an error to provide an ambiguous config file that provides assigns multiple values to a single-value setting. Change include release notes.
…ied" errors Also skip calling TestArgString in these cases since exact behavior retrieving values is irrelevant when parsing values fails. Also s/TestArgString/GetArg/ to be more efficient and direct, now that it's no longer necessary to call with separation of ALLOW_LIST and non ALLOW_LIST cases.
This commit has no effect on behavior because as of bitcoin#17493 it's not possible to specify multiple values for single value settings in the config file.
This commit has no effect on behavior because as of bitcoin#17493 it's not possible to specify multiple values for single value settings in the config file.
This is based on #16545 + #17580. The non-base commits are:
92a38cc44c1futil: Forbid ambiguous multiple assignments in config file20119c310f4etest: Extend util_ArgsMerge test to check for "Multiple values specified" errorsEnable error "Multiple values specified for -setting in same section of config file.", for ALLOW_ANY settings that don't specify ALLOW_LIST.
Instead of silently ignoring settings, this change makes it an error to provide an ambiguous config file that provides assigns multiple values to a single-value setting. Change includes release notes.
Part of the motivation for this change is to improve usability and prevent settings that look valid from being silently ignored. Another motivation is to be able to remove confusing "reverse precedence" logic in #17581