Skip to content

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Nov 24, 2019

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


This has no effect on behavior because as of #17493 it's not possible to specify multiple values for single value settings in the config file.

This change implements one of the settings simplifications listed in #17508

@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/17581.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

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)
  • #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)
  • #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 “simple string option that canoot be negated”]
  • “a where negation should be disallowed” → “where negation should be disallowed” [extra “a” makes the phrase awkward and unclear]

drahtbot_id_4_m

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.
Enable error "Multiple values specified for -setting in same section of config
file.", for ALLOW_ANY settings that don't specify ALLOW_LIST.

Instead of silently ignoring settings, this change makes it an error to provide
an ambiguous config file that provides assigns multiple values to a
single-value setting. Change include release notes.
…ied" errors

Also skip calling TestArgString in these cases since exact behavior retrieving
values is irrelevant when parsing values fails.

Also s/TestArgString/GetArg/ to be more efficient and direct, now that it's no
longer necessary to call with separation of ALLOW_LIST and non ALLOW_LIST
cases.
This commit has no effect on behavior because as of
bitcoin#17493 it's not possible to specify
multiple values for single value settings in the config file.
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