#17783 common: Disallow calling IsArgSet() on ALLOW_LIST options
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #17783 common: Disallow calling IsArgSet() on ALLOW_LIST options | Review #16545 first | 0 | Positive | 392 + 793 tests | |||
|
|
|
| ||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Disallows calling ArgsManager::IsArgSet() on configuration arguments flagged with ALLOW_LIST, throwing a std::logic_error when attempted. Cleans up existing call sites across node initialization to check GetArgs().empty() or inspect settings directly instead of calling IsArgSet() on list options. Also normalizes node initialization behavior when -noonlynet is passed alongside -privatebroadcast and -listenonion.
Problem: When a list option is negated on the command line or config file (such as -norpcwhitelist or -noonlynet), IsArgSet() returns true even though GetArgs() returns an empty list. Callers checking IsArgSet() can mistakenly treat the option as active with empty parameters rather than disabled, creating subtle configuration and networking bugs.
Evidence of importance:
- PR description: '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'
- Commit efe0fea4ec notes that specifying -noonlynet, -privatebroadcast, and -listenonion together improperly caused an init error claiming no networks are reachable because of an IsArgSet("-onlynet") check.
Scope: The PR diff includes 17 commits, but the first 14 commits are stacked from prerequisite PRs #16545 and #17580; only the final 3 commits are unique to this PR.
Risk of a mistake: medium (other). A missed call site or improper argument classification could throw a logic error and cause bitcoind to crash on startup under specific CLI or config combinations.
Changed since the previous assessment: Rebased on updated base branches and adapted to new list options introduced in recent commits, specifically removing IsArgSet calls on -testactivationheight and -vbparams introduced in #35335.
Reviewability
Ready · Review #16545 first
Stacked on #16545; the base PR's changes are included in this diff.
Waiting on (from the thread read):
- nothing
Agreement
Positive
support without stated reasons, no open objection (promag)
Thread notes: Updated to current schema. No claims raised; promag provided an unsubstantiated Concept ACK. / Brought record up to current schema. Only a brief Concept ACK from promag is on record; PR has seen continuous rebases by the author.
Objections, suggestions, and questions: none recorded.
Support:
- promag (contributor) (Concept ACK, 2019-12-22): Concept ACK. [not substantive]
Participants:
- promag (contributor, support, 1 statements 2019-12-22 to 2019-12-22): Concept ACK with no rationale
Dependencies
Files
882 lines under test/bench/ci.
- src/test/argsman_tests.cpp +661/-71
- src/common/args.cpp +168/-31
- src/common/args.h +162/-18
- src/init.cpp +48/-39
- src/test/fuzz/system.cpp +60/-9
- src/test/getarg_tests.cpp +61/-3
- src/chainparamsbase.cpp +6/-5
- 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
- src/wallet/init.cpp +1/-1
- test/functional/feature_config_args.py +1/-1
Card
The short text the ranking pass compares across PRs.
This pull request disallows calling ArgsManager::IsArgSet() on options flagged with ALLOW_LIST, throwing a logic error if invoked. It prevents a class of configuration bugs where negated list arguments (like -noonlynet or -norpcwhitelist) cause IsArgSet() to return true while GetArgs() returns empty, leading callers to treat empty lists as enabled features. It eliminates call sites on list options across node startup and normalizes -noonlynet handling with Tor/I2P broadcasts. The PR is stacked on top of open PRs #16545 and #17580, with only three commits unique to this PR.
Data
thread read through 2020-09-28 (2 statements), head 084f3a8bce
labels: Utils/log/libs · draft
Categories
Utilities (logging, arguments, libraries)
P2 · bug fix
Eliminating IsArgSet() on ALLOW_LIST options removes an error-prone API trap that causes subtle configuration and initialization bugs. Calling IsArgSet() on negated options returned true while GetArgs() was empty, leading to improper node initialization errors such as when -noonlynet was specified alongside -privatebroadcast and -listenonion.
Membership: Carries the Utils/log/libs label and directly modifies configuration and argument handling in src/common/args.cpp and src/common/args.h.
Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 1