Skip to content

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Dec 20, 2019

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
Copy link
Contributor

DrahtBot commented Dec 20, 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/17783.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #33191 (net: Provide block templates to peers on request by ajtowns)
  • #32699 (docs: adds correct updated documentation links by Zeegaths)
  • #32541 (index: store per-block transaction locations for efficient lookups by romanz)
  • #32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
  • #32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)
  • #31974 (Drop testnet3 by Sjors)
  • #30059 (Add option dbfilesize to control LevelDB target ("max") file size by luke-jr)
  • #29365 (Extend signetchallenge to set target block spacing by starius)
  • #28792 (Embed default ASMap as binary dump header file by fjahr)
  • #28676 (Cluster mempool implementation by sdaftuar)
  • #26966 (index: initial sync speedup, parallelize process by furszy)

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:

  • canoot -> cannot [typo in “canoot be negated” makes the sentence unclear]
    No other incomprehensible typos were found.

drahtbot_id_4_m

@promag
Copy link
Contributor

promag commented Dec 22, 2019

Concept ACK.

@ryanofsky
Copy link
Contributor Author

ryanofsky commented Sep 28, 2020

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

achow101 added a commit that referenced this pull request Feb 14, 2025
…case behavior

a85e8c0 doc: Add some general documentation about negated options (Ryan Ofsky)
490c8fa doc: Add release notes summarizing negated option behavior changes. (Ryan Ofsky)
458ef0a refactor: Avoid using IsArgSet() on -connect list option (Ryan Ofsky)
752ab9c test: Add test to make sure -noconnect disables -dnsseed and -listen by default (Ryan Ofsky)
3c2920e refactor: Avoid using IsArgSet() on -signetseednode and -signetchallenge list options (Ryan Ofsky)
d056689 refactor: Avoid using IsArgSet() on -debug, -loglevel, and -vbparams list options (Ryan Ofsky)
3d1e8ca Normalize inconsistent -noexternalip behavior (Ryan Ofsky)
ecd590d Normalize inconsistent -noonlynet behavior (Ryan Ofsky)
5544a19 Fix nonsensical bitcoin-cli -norpcwallet behavior (Ryan Ofsky)
6e8e7f4 Fix nonsensical -noasmap behavior (Ryan Ofsky)
b6ab350 Fix nonsensical -notest behavior (Ryan Ofsky)
6768389 Fix nonsensical -norpcwhitelist behavior (Ryan Ofsky)
e03409c Fix nonsensical -norpcbind and -norpcallowip behavior (Ryan Ofsky)
40c4899 Fix nonsensical -nobind and -nowhitebind behavior (Ryan Ofsky)
5453e66 Fix nonsensical -noseednode behavior (Ryan Ofsky)

Pull request description:

  The PR changes behavior of negated `-noseednode`, `-nobind`, `-nowhitebind`, `-norpcbind`, `-norpcallowip`, `-norpcwhitelist`, `-notest`, `-noasmap`, `-norpcwallet`, `-noonlynet`, and `-noexternalip` options, so negating these options just clears previously specified values doesn't have other side effects.

  Negating options on the command line can be 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 have confusing and undocumented side effects (see commit descriptions for details). Now, negating these options just resets them and behaves the same as not specifying them.

  Motivation for this PR is to fix confusing behaviors and also to remove incorrect usages of the `IsArgSet()` function. Using `IsArgSet()` tends to lead to negated option bugs in general, but it especially causes bugs when used with list settings returned by `GetArgs()`, because when these settings are negated, `IsArgSet()` will return true but `GetArgs()` will return an empty list. This PR eliminates all uses of `IsArgSet()` and `GetArgs()` together, and followup PR #17783 makes it an error to use `IsArgSet()` on list settings, since calling `IsArgSet()` is never actually necessary. Most of the changes here were originally made in #17783 and then moved here to be easier to review and avoid a dependency on #16545.

ACKs for top commit:
  achow101:
    ACK a85e8c0
  danielabrozzoni:
    re-ACK a85e8c0
  hodlinator:
    re-ACK a85e8c0

Tree-SHA512: dd4b19faac923aeaa647b1c241d929609ce8242b43e3b7bc32523cc48ec92a83ac0dc5aee79f1eba8794535e0314b96cb151fd04ac973671a1ebb9b52dd16697
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.
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.
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