Skip to content

common: Disallow calling IsArgSet() on ALLOW_LIST options#17783

Draft
ryanofsky wants to merge 18 commits into
bitcoin:masterfrom
ryanofsky:pr/wdnolist
Draft

common: Disallow calling IsArgSet() on ALLOW_LIST options#17783
ryanofsky wants to merge 18 commits into
bitcoin:masterfrom
ryanofsky:pr/wdnolist

Conversation

@ryanofsky

@ryanofsky ryanofsky commented Dec 20, 2019

Copy link
Copy Markdown
Contributor

This is based on #16545 + #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

@DrahtBot

DrahtBot commented Dec 20, 2019

Copy link
Copy Markdown
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/17783.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK promag

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #35713 (Remove boost as a unit test runner by rustaceanrob)
  • #35387 (logging: make trace logging easily usable by ryanofsky)
  • #35322 (logging: streamline Logger state and drop redundant methods by ryanofsky)
  • #35205 (kernel,node: add dbcache setter and clarify defaults by l0rinc)
  • #35037 (ipc: support per-address max-connections options on -ipcbind by enirox001)
  • #34978 (init: reserve file descriptors for IPC connections by enirox001)
  • #34812 (net: advertise CJDNS addresses when -externalip disables discovery by w0xlt)
  • #34038 (logging: replace -loglevel with -trace, expose trace logging via RPC by ajtowns)
  • #33540 (argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments) by pablomartin4btc)
  • #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)

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:

  • "-includeconf=<file>" ... "only useable from configuration file..." -> usable [“useable” is a misspelling; “usable” is the standard form]

Possible places where comparison-specific test macros should replace generic comparisons:

  • [src/test/argsman_tests.cpp] BOOST_CHECK_THROW(test.GetArg("-value", "zzzzz"), std::logic_error); (and the related GetIntArg / GetBoolArg / GetArgs throw checks) -> use BOOST_CHECK_EXCEPTION(..., std::logic_error, HasReason{...}) so the test verifies the actual failure message, not just the exception type.
  • [src/test/argsman_tests.cpp] BOOST_CHECK_THROW(test.AddArg("-arg1", "name", M::ALLOW_BOOL | M::ALLOW_ANY, OptionsCategory::OPTIONS), std::logic_error); (and the other two bad-flag AddArg checks) -> use BOOST_CHECK_EXCEPTION(..., std::logic_error, HasReason{...}) to assert the specific incompatibility error text.

2026-06-24 15:27:29

@promag

promag commented Dec 22, 2019

Copy link
Copy Markdown
Contributor

Concept ACK.

@ryanofsky

ryanofsky commented Sep 28, 2020

Copy link
Copy Markdown
Contributor Author

Rebased d0abe64 -> cedf83e (pr/wdnolist.1 -> pr/wdnolist.2, compare) due to conflict with #19561, #19638, #19709 on top of #17580 pr/wdlist.4
Rebased cedf83e -> b650727 (pr/wdnolist.2 -> pr/wdnolist.3, compare) after conflict with #18267, adding changes originally in #17580 on top of #17580 pr/wdlist.6
Rebased b650727 -> 4223415 (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 4223415 -> fba5a00 (pr/wdnolist.4 -> pr/wdnolist.5, compare) to try to fix fuzz test bug https://cirrus-ci.com/task/6395856740941824?logs=ci#L4435
Updated fba5a00 -> 6b8276e (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 6b8276e -> 048a4ee (pr/wdnolist.6 -> pr/wdnolist.7, compare) due to conflict with #21732
Rebased 048a4ee -> 98ebf0b (pr/wdnolist.7 -> pr/wdnolist.8, compare) on top of #17580 pr/wdlist.12
Rebased 98ebf0b -> 2212f18 (pr/wdnolist.8 -> pr/wdnolist.9, compare) on top of #17580 pr/wdlist.14
Rebased 2212f18 -> e75b01c (pr/wdnolist.9 -> pr/wdnolist.10, compare) on top of #17580 pr/wdlist.15--


Rebased 11d0a89 -> 6321fdb (pr/wdnolist.20 -> pr/wdnolist.21, compare)

Rebased 6321fdb -> 38a9cd5 (pr/wdnolist.21 -> pr/wdnolist.22, compare)

Rebased 38a9cd5 -> 2f44966 (pr/wdnolist.22 -> pr/wdnolist.23, compare) on top of #17580 pr/wdlist.31

Updated 2f44966 -> bf609a9 (pr/wdnolist.23 -> pr/wdnolist.24, compare) to fix CI error caused by silent conflict with #29415 https://github.com/bitcoin/bitcoin/actions/runs/21119016159/job/60728873971?pr=17783

Rebased bf609a9 -> 811a7a9 (pr/wdnolist.24 -> pr/wdnolist.25, compare) on top of #17580 pr/wdlist.33

Rebased 811a7a9 -> 1d00d2a (pr/wdnolist.25 -> pr/wdnolist.26, compare) on top of #17580 pr/wdlist.34, fixing silent merge conflict with #35335 which called IsArgSet on -testactivationheight and -vbparams list options

Rebased 1d00d2a -> bc5e499 (pr/wdnolist.26 -> pr/wdnolist.27, compare) due to conflict with #35441

ryanofsky and others added 12 commits June 23, 2026 16:19
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants