Skip to content

Conversation

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Nov 24, 2019

This is based on #16545. The non-base commits are:


Except for two rpcauth and blockfilterindex fixes (update: rpcauth fix was merged separately in #30401) this PR does not change any behavior outside of tests. It is just supposed to enforce internal consistency and prevent bugs by ensuring that list arguments are always retrieved with GetArgs() and non-list arguments are always retrieved with GetArg(). Followup PRs could use the ALLOW_LIST flags for better documentation and error checking in the future. For example, #17493 builds on this to disallow conflicting config values.

This change was originally made as part of #17493

@hebasto
Copy link
Member

hebasto commented Nov 24, 2019

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 24, 2019

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/17580.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK hebasto, promag, ajtowns

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:

  • #34267 (net: avoid unconditional privatebroadcast logging (+ warn for debug logs) by l0rinc)
  • #34038 (logging: API improvements by ajtowns)
  • #33343 (help: enrich help text for -loadblock by HowHsu)
  • #32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
  • #32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)
  • #31974 (Drop testnet3 by Sjors)
  • #28792 (build: Embedded ASMap [3/3]: Build binary dump header file by fjahr)

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:

  • "useable" -> "usable" [common spelling: "usable" is the standard form in this context]
  • "retrieved by called GetSetting()" -> "retrieved by calling GetSetting()" [grammatical error: "by called" is incorrect; "by calling" expresses the intended meaning]

2025-12-16

@promag
Copy link
Contributor

promag commented Dec 22, 2019

Wrong commit bcbbc48? nevermind.

Concept ACK.

Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, shame all these checks are happening at runtime rather than compile time though

Arg* arg;
unsigned int prev_flags = arg ? arg->m_flags : 0;
};
//! Call GetArgs(), temporarily enabling ALLOW_LIST so call can succeed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to restructure the tests than have these special functions that tweak the flags. Maybe something like:

str_config =
   "a=\n"
   "b=1\n"
   "c=foo\n"
   "noaaa=1\n"
   "r=0\n"
   "r=1\n"

(with a section as well to check overrides/repeats etc work correctly) and then try setting up all the args as ALLOW_ANY and check for appropriate errors, then all the args as ALLOW_BOOL and check for appropriate results/errors, and all the args as ALLOW_LIST and check for appropriate errors?

(Might be nicer to separate out the test infrastructure changes first, so that it's easy to see all the changes to test results in the commit that changes the functionality, but YMMV)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "refactor: Always enforce ALLOW_LIST in CheckArgFlags" (4f3e5e9)

I think it might be better to restructure the tests than have these special functions that tweak the flags.

I'm not too inclined to want to restructure the tests, though I'd be happy to review a PR that did. I also wouldn't want to do it in this PR because I'd like the test changes here to reflect only reflect behavior that's changing, and not have other differences.

src/httprpc.cpp Outdated
strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
}
if (gArgs.GetArg("-rpcauth","") != "")
if (!gArgs.GetArgs("-rpcauth").empty())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gArgs.IsArgSet("-rpcauth") ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "refactor: Fix more ALLOW_LIST arguments" (4bd1c3b)

gArgs.IsArgSet("-rpcauth") ?

That would do the wrong thing if the argument is negated. IsArgSet is generally broken and misused for list settings (and a lot of non-list settings), see #17783

Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

shame all these checks are happening at runtime rather than compile time though

Agree, but I think adding checks and types like this PR and #17783 are doing is really the hard part. Switching enforcement from runtime to compile time after constraints are in already place would be easier by comparison

src/httprpc.cpp Outdated
strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
}
if (gArgs.GetArg("-rpcauth","") != "")
if (!gArgs.GetArgs("-rpcauth").empty())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "refactor: Fix more ALLOW_LIST arguments" (4bd1c3b)

gArgs.IsArgSet("-rpcauth") ?

That would do the wrong thing if the argument is negated. IsArgSet is generally broken and misused for list settings (and a lot of non-list settings), see #17783

Arg* arg;
unsigned int prev_flags = arg ? arg->m_flags : 0;
};
//! Call GetArgs(), temporarily enabling ALLOW_LIST so call can succeed.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "refactor: Always enforce ALLOW_LIST in CheckArgFlags" (4f3e5e9)

I think it might be better to restructure the tests than have these special functions that tweak the flags.

I'm not too inclined to want to restructure the tests, though I'd be happy to review a PR that did. I also wouldn't want to do it in this PR because I'd like the test changes here to reflect only reflect behavior that's changing, and not have other differences.

Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 4f3e5e9 -> e42d223 (pr/wdlist.2 -> pr/wdlist.3, compare) with suggested checkargflags simplification
Rebased e42d223 -> 9621fae (pr/wdlist.3 -> pr/wdlist.4, compare) due to conflicts with #18594, #15935, #19561, #19709 on top of #16545 pr/argcheck.18
Rebased 9621fae -> 1fe816e (pr/wdlist.4 -> pr/wdlist.5, compare) due to conflict with #18267 on top of #16545 pr/argcheck.18
Updated 1fe816e -> 3271ee8 (pr/wdlist.5 -> pr/wdlist.6, compare) moving unrelated changes to #17783 on top of #16545 pr/argcheck.18
Rebased 3271ee8 -> 678d20b (pr/wdlist.6 -> pr/wdlist.7, compare) due to conflicts with #18267, #18309, and #19991 on top of #16545 pr/argcheck.18
Rebased 678d20b -> f78c85b (pr/wdlist.7 -> pr/wdlist.8, compare) due to conflicts with #19884, #20685, #21060 on top of #16545 pr/argcheck.21
Updated f78c85b -> a562638 (pr/wdlist.8 -> pr/wdlist.9, compare) fixing silent conflicts and CI errors https://cirrus-ci.com/task/5867084006555648 https://cirrus-ci.com/task/4565262239268864
Rebased a562638 -> b83d0e0 (pr/wdlist.9 -> pr/wdlist.10, compare) on top of #16545 pr/argcheck.22 due to conflicts with #21377, #21710, and #21752
Rebased b83d0e0 -> 601fe2d (pr/wdlist.10 -> pr/wdlist.11, compare) on top of #16545 pr/argcheck.25
Rebased 601fe2d -> 71815bc (pr/wdlist.11 -> pr/wdlist.12, compare) on top of #16545 pr/argcheck.28
Rebased 71815bc -> f98d2b0 (pr/wdlist.12 -> pr/wdlist.13, compare) on top of #16545 pr/argcheck.29 and adding new commit to fix new problem with -blockfilterindex arguments
Rebased f98d2b0 -> 38db444 (pr/wdlist.13 -> pr/wdlist.14, compare) on top of #16545 pr/argcheck.30 to fix signed integer conversion https://cirrus-ci.com/task/5371023720710144
Rebased 38db444 -> 27685d5 (pr/wdlist.14 -> pr/wdlist.15, compare) due to conflicts with #22087 and #26489

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Task macOS native, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/20166547137/job/57891257159
LLM reason (✨ experimental): Fuzzing failed due to a std::logic_error in GetBoolArg for -help-debug, causing the container test to exit non-zero.

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
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.

ryanofsky and others added 13 commits December 15, 2025 11:42
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).
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 <[email protected]>
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.
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.

6 participants