The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/17581.
See the guideline for information on the review process. A summary of reviews will appear here.
Reviewers, 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.
Implement cleanup suggestions from l0rinc:
https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773221823
https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773291395
https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775897382
This commit just adds documentation for the type flags. The flags are actually
implemented in the following two commits.
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.
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 -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.
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.
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
https://github.com/bitcoin/bitcoin/pull/17493 it's not possible to specify
multiple values for single value settings in the config file.
⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
Labels
Refactoring
Needs rebase