#16545 refactor: Implement missing error checking for ArgsManager flags
https://github.com/bitcoin/bitcoin/pull/16545 · · +851/-90 in 9 files, 6 commits · labels: Utils/log/libs · draft
Goal
- Prevent startup argument errors and confusing configuration bugs caused by untyped strings
- Establish validation logic to detect malformed options and incompatible configuration flags
Implements validation and parsing logic for ArgsManager flags (ALLOW_BOOL, ALLOW_INT, ALLOW_STRING, ALLOW_LIST) to detect malformed arguments at startup. It disallows incompatible flag combinations, raises logic errors when GetArg helper functions are called with mismatched types, and adjusts fuzz and unit tests to exercise the new flags. Existing node options are not yet converted to these typed flags, making this patch behavior-neutral for production startup until follow-up PRs adopt them.
Problem: Bitcoin Core's configuration and argument parsing treats most options as untyped strings, allowing confusing bugs such as double negatives (-nosetting=0), silently ignored duplicate assignments, or accidental misinterpretations of empty values.
Category: Utilities (logging, arguments, libraries) (#13 of 66)
P3 · cleanup
- P3 because direct user benefit is deferred until existing options adopt the typed flags
- P3 because it provides cleanup and leverage for follow-up fixes to configuration quirks
Strong internal refactoring that establishes type and sanity checks for configuration parsing. The direct user benefit is deferred because existing arguments are not converted within this PR, but it provides leverage for follow-up fixes to longstanding argument parsing quirks.
Membership: Modifies ArgsManager parsing and validation logic in src/common/args.{h,cpp} and src/common/config.cpp.
Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 2
Reviewability: Paused: Review #31260 first
- Review #31260 first
- Author suggests prioritizing #31260 because its C++ type declarations may alter or clarify this PR
The author explicitly noted that reviewers should look at #31260 first because it introduces C++ type declarations for settings that make the validation semantics clearer and may alter how this PR is rebased.
Author status: Active, keeps branch rebased and clean, but recommended prioritizing #31260 first.
Open concerns:
- Author suggested reviewing #31260 ahead of this PR because defining argument types via C++ types rather than bit flags may supersede or clarify this flag mechanism.
- Reviewers raised concerns about whether flag combinations are too broad and whether changes should be split by individual types (e.g. bool only).
Resolved concerns:
- Dropped support for complex flag combinations in the main PR branch to simplify review.
- Added extensive ExampleOptions test coverage and doxygen documentation to clarify flag behaviors.
- Addressed fuzzing exceptions by tightening logic_error catches in system fuzz target.
Agreement: Positive
- General support for enforcing stronger, consistent argument error checking
- Concept approval for incremental strict typing of startup options (laanwj, hodlinator)
- Approach approval with suggested refactorings incorporated (l0rinc)
- Objection on flag complexity addressed by removing multi-flag combinations (maflcko)
- Dormant concern regarding flag ergonomics and default-reset behavior (ajtowns)
Positive with Concept and Approach ACKs, though past discussions contested flag semantics and scope.
Reviewers have given Concept and Approach ACKs for stronger argument checking. Historical approach objections from ajtowns regarding default-reset behavior went dormant after the author answered, and maflcko's request to restrict flag combinations was implemented.
- laanwj gave Concept ACK noting consistent argument error checking is desirable.
- hodlinator gave Concept ACK praising the incremental approach toward strict argument typing.
- l0rinc gave Approach ACK and suggested several refactorings that the author incorporated.
- ajtowns questioned the ergonomics of flag combinations and default value resetting in 2020; author responded and ajtowns did not follow up.
- maflcko asked to avoid multi-flag combinations and suggested smaller steps; author removed flag combinations in response.
Review verdicts (DrahtBot): 0 (+1) -1
- Stale ACK: hebasto
- Approach ACK: l0rinc
- Concept ACK: laanwj, hodlinator
- Approach NACK: ajtowns
Dependencies
Files
564 lines under test/bench/ci.
- src/test/argsman_tests.cpp +425/-9
- src/common/args.cpp +161/-49
- src/common/args.h +144/-19
- src/test/getarg_tests.cpp +61/-3
- src/test/fuzz/system.cpp +48/-7
- test/lint/check-doc.py +3/-2
- src/common/config.cpp +4/-0
- src/test/util/common.h +4/-0
- test/functional/feature_config_args.py +1/-1
Card
This PR adds error checking and runtime type enforcement to ArgsManager flags (ALLOW_BOOL, ALLOW_INT, ALLOW_STRING, ALLOW_LIST) to reject invalid configuration inputs at startup. It does not alter existing options directly, serving instead as foundational infrastructure for a stack of follow-ups that clean up configuration handling footguns. The PR has Concept and Approach ACKs from several contributors, but the author recommended reviewing #31260 first to settle C++ type-safe settings retrieval before landing this flag-based PR.