refactor: Remove settings merge reverse precedence code #17581

pull ryanofsky wants to merge 18 commits into bitcoin:master from ryanofsky:pr/wdrev changing 17 files +1172 −212
  1. ryanofsky commented at 8:17 pm on November 24, 2019: contributor

    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

  2. fanquake added the label Refactoring on Nov 24, 2019
  3. DrahtBot commented at 11:52 pm on November 24, 2019: 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/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

  4. DrahtBot added the label Needs rebase on Dec 19, 2019
  5. ryanofsky force-pushed on Dec 20, 2019
  6. DrahtBot removed the label Needs rebase on Dec 20, 2019
  7. DrahtBot added the label Needs rebase on Jan 30, 2020
  8. ryanofsky force-pushed on Sep 28, 2020
  9. DrahtBot removed the label Needs rebase on Sep 28, 2020
  10. DrahtBot added the label Needs rebase on Sep 29, 2020
  11. ryanofsky force-pushed on Apr 13, 2021
  12. DrahtBot removed the label Needs rebase on Apr 13, 2021
  13. ryanofsky force-pushed on Apr 14, 2021
  14. DrahtBot added the label Needs rebase on Apr 15, 2021
  15. ryanofsky force-pushed on Jun 17, 2021
  16. DrahtBot removed the label Needs rebase on Jun 18, 2021
  17. DrahtBot added the label Needs rebase on Aug 22, 2021
  18. ryanofsky force-pushed on Dec 30, 2021
  19. DrahtBot removed the label Needs rebase on Dec 30, 2021
  20. DrahtBot added the label Needs rebase on Feb 9, 2022
  21. ryanofsky force-pushed on Sep 27, 2022
  22. DrahtBot removed the label Needs rebase on Sep 27, 2022
  23. DrahtBot added the label Needs rebase on Nov 15, 2022
  24. ryanofsky force-pushed on Feb 15, 2023
  25. DrahtBot removed the label Needs rebase on Feb 15, 2023
  26. DrahtBot added the label Needs rebase on Mar 20, 2023
  27. achow101 marked this as a draft on Apr 25, 2023
  28. ryanofsky force-pushed on May 3, 2023
  29. DrahtBot removed the label Needs rebase on May 3, 2023
  30. DrahtBot added the label Needs rebase on May 9, 2023
  31. ryanofsky force-pushed on Jul 19, 2024
  32. DrahtBot removed the label Needs rebase on Jul 20, 2024
  33. DrahtBot added the label Needs rebase on Aug 5, 2024
  34. ryanofsky force-pushed on Aug 6, 2024
  35. DrahtBot removed the label Needs rebase on Aug 6, 2024
  36. DrahtBot added the label Needs rebase on Aug 7, 2024
  37. common: Grammar / formatting tweaks
    Implement cleanup suggestions from l0rinc:
    
    https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773221823
    https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773291395
    https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775897382
    20a9986751
  38. doc: Add detailed ArgsManager type flag documention
    This commit just adds documentation for the type flags. The flags are actually
    implemented in the following two commits.
    30385f2976
  39. common: Implement ArgsManager flags to parse and validate settings on 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.
    a7a35ed080
  40. common: Update ArgManager GetArg helper methods to work better with ALLOW 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).
    a4e8ed267a
  41. test: Add tests to demonstrate usage of ArgsManager flags
    Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    59c449e034
  42. test: Add test for settings.json parsing with type flags
    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 <172445034+hodlinator@users.noreply.github.com>
    4090dcf6cf
  43. ryanofsky force-pushed on Dec 6, 2024
  44. DrahtBot removed the label Needs rebase on Dec 6, 2024
  45. DrahtBot added the label Needs rebase on Dec 14, 2024
  46. ryanofsky force-pushed on Mar 12, 2025
  47. DrahtBot removed the label Needs rebase on Mar 12, 2025
  48. DrahtBot added the label Needs rebase on Mar 14, 2025
  49. maflcko removed the label Refactoring on Jun 11, 2025
  50. maflcko removed the label Needs rebase on Jun 11, 2025
  51. DrahtBot added the label Refactoring on Jun 11, 2025
  52. maflcko added the label Needs rebase on Jun 11, 2025
  53. Merge remote-tracking branch 'origin/pull/16545/head' f2a3fa480e
  54. common: Add support for combining ArgsManager flags
    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.)
    b36eab7822
  55. scripted-diff: Add ALLOW_LIST flag to arguments retrieved with GetArgs
    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-
    b2e3cdbbbb
  56. refactor: Fix more ALLOW_LIST arguments
    - 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.
    9cb58a804c
  57. Always reject empty -blockfilterindex="" arguments
    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.
    a6d6208e10
  58. refactor: Stop calling GetArg/IsArgSet on ALLOW_LIST options
    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.
    ca956f3f66
  59. refactor: Always enforce ALLOW_LIST in CheckArgFlags
    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.
    6e874dee28
  60. Merge remote-tracking branch 'origin/pull/17580/head' 6eea42e07c
  61. util: Forbid ambiguous multiple assignments in config file
    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.
    4ff9769b82
  62. test: Extend util_ArgsMerge test to check for "Multiple values specified" 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.
    98e2bba705
  63. Merge remote-tracking branch 'origin/pull/17493/head' 4395a23e00
  64. refactor: Remove settings merge reverse precedence code
    This commit has no effect on behavior because as of
    https://github.com/bitcoin/bitcoin/pull/17493 it's not possible to specify
    multiple values for single value settings in the config file.
    eb89b174b1
  65. ryanofsky force-pushed on Aug 4, 2025
  66. DrahtBot removed the label Needs rebase on Aug 4, 2025

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-23 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me