This is based on #16545 + #30529 + #17580. The non-base commits are:
Disallow calling IsArgSet() function on ALLOW_LIST options. Code that uses IsArgSet() with list options is confusing and leads to bugs when IsArgSet() returns true, but GetArgs() returns an empty list, so the option is considered enabled even though it is empty. This led to a number of bugs which are fixed in #30529
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | promag |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
pr/wdnolist.1 -> pr/wdnolist.2, compare) due to conflict with #19561, #19638, #19709 on top of #17580 pr/wdlist.4
Rebased cedf83ee857f717027f7367232682ebc27da4017 -> b65072798cdf8a9aca87f05fb2e6f39414f40844 (pr/wdnolist.2 -> pr/wdnolist.3, compare) after conflict with #18267, adding changes originally in #17580 on top of #17580 pr/wdlist.6
Rebased b65072798cdf8a9aca87f05fb2e6f39414f40844 -> 4223415ff6de22724ca1b85cd65119771dbdf0e9 (pr/wdnolist.3 -> pr/wdnolist.4, compare) on top of #17580 pr/wdlist.9 due to conflicts with #21415 and #20048, also fixing fuzz error https://travis-ci.org/github/bitcoin/bitcoin/jobs/730931049
Updated 4223415ff6de22724ca1b85cd65119771dbdf0e9 -> fba5a00126b647225dd13b4aa18406aae82c76af (pr/wdnolist.4 -> pr/wdnolist.5, compare) to try to fix fuzz test bug https://cirrus-ci.com/task/6395856740941824?logs=ci#L4435
Updated fba5a00126b647225dd13b4aa18406aae82c76af -> 6b8276e8f595c3dc212f06eb121ca6a5bbaeb58a (pr/wdnolist.5 -> pr/wdnolist.6, compare) to fix same fuzz test bug https://cirrus-ci.com/task/6640989080125440 previous push didn’t fix
Rebased 6b8276e8f595c3dc212f06eb121ca6a5bbaeb58a -> 048a4ee606a370d51620e39aecc8f6d4a132c31c (pr/wdnolist.6 -> pr/wdnolist.7, compare) due to conflict with #21732
Rebased 048a4ee606a370d51620e39aecc8f6d4a132c31c -> 98ebf0b32a6deb6f14b919286e2c3195ab68ab6d (pr/wdnolist.7 -> pr/wdnolist.8, compare) on top of #17580 pr/wdlist.12
Rebased 98ebf0b32a6deb6f14b919286e2c3195ab68ab6d -> 2212f18e344d53c5c23af5253ae4f21abe3dae7f (pr/wdnolist.8 -> pr/wdnolist.9, compare) on top of #17580 pr/wdlist.14
Rebased 2212f18e344d53c5c23af5253ae4f21abe3dae7f -> e75b01cddd5f454c51071c707305f609d8950bad (pr/wdnolist.9 -> pr/wdnolist.10, compare) on top of #17580 pr/wdlist.15
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27686787039
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Make sure -noconnect has same effect as -connect for disabling DNS seeding and
listening by default, and warning about -dnsseed being ignored with the -proxy
setting.
Initial implementation of https://github.com/bitcoin/bitcoin/pull/30529
accidentally broke this behavior, so having coverage may be useful to make sure
it does not break again.
Handle -noconnect setting explicity with IsArgNegated() function instead of
implicitly with IsArgSet() and add comments so it is clearer what the code is
trying to do when -noconnect is specified.
This commit is a refactoring does not change behavior. Test coverage for this
behavior was added in the previous commit.
Not sure if it really makes sense not to warn about seednode being ignored if
-noconnect is specified, and only to warn about -dnsseed being ignored when
-proxy is specified, but these behaviors are not changed from before.
Drop unnecessary IsArgSet calls for -debug, -loglevel, and -vbparams options
and fix inaccurate comment about -nodebug.
This commit is a refactoring and does not change behavior. It is not necessary
to check IsArgSet before calling GetArgs, because if IsArgSet returns false
GetArgs just returns an empty vector.
This change fixes some corner cases handling negated list options:
-norpcwhitelist, -norpcallowip, -norpcbind, -nobind, -nowhitebind,
-noexternalip, -noonlynet, -noseednode, -nosignetchallenge, -nosignetseednode,
and -notest.
Negating list options on the command line is a useful way of resetting options
that may have been set earlier in the command line or config file. But before
this change, negating these options wouldn't fully reset them, and would cause
side effects interacting with other parameters (listed below). Now, negating
these options behaves the same as not setting them at all.
The code change in this commit is just to avoid using IsArgSet() and GetArgs()
together on the same options. Using IsArgSet() and GetArgs() together
frequently leads to bugs because it overlooks the case where an argument is
negated and IsArgSet() returns true while GetArgs() returns an empty list. It
almost always makes sense to call GetArgs("-option").empty() instead
IsArgSet("-option") for list options that are allowed to be called multiple
times.
This change includes release notes, but the release notes don't go into details
about specific options. For reference this change:
- Treats specifying -norpcwhitelist exactly the same as not specifying any
-rpcwhitelist, instead of behaving almost the same but flipping the default
-rpcwhitelistdefault value.
- Treats specifying -norpcallowip and -norpcbind exactly the same as not
specifying -rpcallowip or -rpcbind, instead of failing to bind to localhost
and failing to show warnings when one value is set without the other.
- Treats specifying -nobind, and -nowhitebind exactly the same as not
specifying -bind and -whitebind values instead of causing them to soft-set
-listen=1.
- Treats specifying -noexternalip exactly the same as not specifying any
-externalip, instead of treating it almost the same but interacting with the
-discover value.
- Treats specifying -noonlynet exactly the same as not specifying -onlynet
instead of marking all networks unreachable.
- Treats specifying -noseednode exactly the same as not specifying any
-seednode value, instead of enabling seed node timeout and log messages
- Treats specifying -nosignetchallenge exactly the same as not specifying
-signetchallenge instead of throwing strange error "-signetchallenge cannot
be multiple values"
- Treats specifying -notest exactly the same as not specifying any
-test value, instead of complaining that it must be used with -regtest.
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.
This restores previous behavior before 743077544b5420246ef29e0b708c90e3a8dfeeb6
from https://github.com/bitcoin/bitcoin/pull/18594, which inadvertently changed
it.
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>
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 nonsensical:
- If an empty -rpcauth="" argument was specified as the last command
line argument, it would cause all other -rpcauth arguments to be
ignored.
- If an empty -rpcauth="" argument was specified on the command line
followed by any nonempty -rpcauth argument, it would cause an error.
- If an empty "rpcauth=" line was specified after non-empty rpcauth line
it would cause an error.
- If an empty "rpcauth=" line in a config file was the entry in the
config file it would cause all rpcauth entries to be ignored, unless
the last command line argument was a nonempty -rpcauth argument, in
which case it would be ignored.
New behavior is simple:
- If an empty "rpcauth=" config line or -rpcauth="" command line
argument is used it will cause an error
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.
Disallow calling IsArgSet() function on ALLOW_LIST options. Code that uses
IsArgSet() with list options is confusing and leads to mistakes due to the easy
to overlook case where an argument is negated and IsArgSet() returns true, but
GetArgs() returns an empty list.
🐙 This pull request conflicts with the target branch and needs rebase.
⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?