#17580 refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags
https://github.com/bitcoin/bitcoin/pull/17580 · · +1144/-197 in 15 files, 14 commits · labels: Refactoring · draft
Goal
- Prevent silent configuration bugs when reading multi-value options across the codebase
- Ensure developers retrieve all values consistently instead of accidentally taking only the first or last
Adds the ALLOW_LIST flag to ArgsManager options and enforces in CheckArgFlags that multi-value options are retrieved via GetArgs() rather than GetArg() or IsArgSet(). It also resolves inconsistent behavior when parsing empty -blockfilterindex arguments and adds unit tests for typed argument handling.
Problem: Using single-argument methods like GetArg() or IsArgSet() on list options can lead to silent bugs, such as returning only the first or last specified value or treating negated lists as present. Developers need the configuration system to enforce correct access patterns internally.
Category: Utilities (logging, arguments, libraries) (#14 of 66)
P3 · cleanup
- P3 because it improves shared configuration safety across the codebase by avoiding subtle option bugs
- Unblocks follow-up work forbidding duplicate conflicting configuration options
P3 because it improves the safety and clarity of the shared configuration manager across the codebase. Enforcing GetArgs on list options prevents subtle bugs and enables follow-up work such as forbidding duplicate conflicting settings in #17493.
Membership: Directly modifies ArgsManager flag definitions and parsing enforcement in src/common/args.cpp and src/common/args.h.
Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 2
Reviewability: Ready: Review #16545 first
The code is rebased and passing CI, but it sits on top of open PR #16545, so reviewers may prefer reviewing the base first.
Author status: active, keeps the branch regularly rebased and updated against master and the base PR
Resolved concerns:
- ajtowns suggested simplifying CheckArgFlags conditions and expressed a desire for compile-time rather than runtime checking; ryanofsky simplified the check and noted runtime validation is a pragmatic prerequisite for future compile-time guarantees
- ajtowns raised a question regarding IsArgSet usage on -rpcauth; ryanofsky clarified IsArgSet is flawed on list options and addressed it in the follow-up stack
Agreement: Positive
- Concept approval without stated reasons (hebasto, promag)
- Concept approval with technical feedback on validation checks addressed by author (ajtowns)
- Marked draft while waiting on the underlying PR (fanquake)
Concept ACKs from multiple maintainers and contributors (hebasto, promag, ajtowns) with addressed feedback.
Reviewers supported the concept of stricter ArgsManager flags in early review, and technical comments regarding flag simplification and test coverage were incorporated by the author.
- hebasto and promag gave Concept ACKs
- ajtowns gave a Concept ACK with inline design questions that were addressed
- fanquake marked the PR as draft in 2023 due to its dependency on draft PR #16545
Review verdicts (DrahtBot): 0
Dependencies
Files
849 lines under test/bench/ci.
- src/test/argsman_tests.cpp +648/-66
- src/common/args.cpp +158/-49
- src/common/args.h +162/-21
- src/init.cpp +44/-36
- src/test/getarg_tests.cpp +61/-3
- src/test/fuzz/system.cpp +47/-7
- src/chainparamsbase.cpp +4/-4
- src/init/common.cpp +3/-3
- src/test/logging_tests.cpp +3/-3
- test/lint/check-doc.py +3/-2
- src/common/config.cpp +4/-0
- src/test/util/common.h +4/-0
- .github/workflows/ci.yml +1/-1
- src/wallet/init.cpp +1/-1
- test/functional/feature_config_args.py +1/-1
Card
Adds the ALLOW_LIST flag to ArgsManager and enforces at runtime that multi-value options are accessed via GetArgs() rather than GetArg(). This cleans up latent developer footguns around option access and resolves inconsistent handling of empty -blockfilterindex arguments. It also unblocks follow-up work in #17493 to reject duplicate config assignments. The PR is technically clean and has concept support, but is stacked on unmerged base PR #16545.