common: Disallow calling IsArgSet() on ALLOW_LIST options#17783
Draft
ryanofsky wants to merge 18 commits into
Draft
common: Disallow calling IsArgSet() on ALLOW_LIST options#17783ryanofsky wants to merge 18 commits into
ryanofsky wants to merge 18 commits into
Conversation
Contributor
|
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/17783. 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. LLM Linter (✨ experimental)Possible typos and grammar issues:
Possible places where comparison-specific test macros should replace generic comparisons:
2026-06-24 15:27:29 |
Contributor
|
Concept ACK. |
d0abe64 to
cedf83e
Compare
This was referenced Sep 3, 2020
cedf83e to
b650727
Compare
Contributor
Author
This was referenced Sep 28, 2020
This was referenced Apr 12, 2021
This was referenced Apr 8, 2026
This was referenced May 4, 2026
This was referenced May 19, 2026
This was referenced Jun 8, 2026
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.
… -listenonion If -noonlynet -privatebroadcast -listenonion are specified together the "none of Tor or I2P networks is reachable" init error should not be shown because the -listenonion option should make the tor network reachable. This change is a followup to earlier commit ecd590d which normalized `-noonlynet` behavior in other places, replacing `!args.IsArgSet("-onlynet")` with `onlynets.empty()` exactly like this change. The other more general reason for dropping the `args.IsArgSet("-onlynet")` call is that the `-onlynet` option is a list option and list options are treated as both empty and set when negated, which is confusing. A followup commit disallows using `IsArgSet` with list options to avoid this.
…bparams IsArgSet calls on -testactivationheight and -vbparams list options were added in 4995c00 from bitcoin#35335 but this is inconsistent and potentially confusing because negating list arguments is supposed to be equivalent to leaving them unset. Previous PR bitcoin#30529 fixed a variety of cases where calling IsArgSet on list arguments led to problems, so this commit just fixes another case for consistency. The only difference in behavior here is that negated arguments no long trigger "option may not be used on mainnet" errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Needed due to base PR, can be dropped before merge
This was referenced Jul 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is based on #16545 + #17580. The non-base commits are:
398cbb070cdNormalize inconsistent -noonlynet behavior with -privatebroadcast and -listenonion784daf2c2e3init: Stop calling IsArgSet on list args -testactivationheight and -vbparamsbc5e4996596common: Disallow calling IsArgSet() on ALLOW_LIST optionsDisallow 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