#17783 common: Disallow calling IsArgSet() on ALLOW_LIST options

ReviewabilityReviewsAgreementSize
#17783 ryanofsky common: Disallow calling IsArgSet() on ALLOW_LIST optionsReview #16545 first0Positive392 + 793 tests
  • Prevent subtle configuration and networking bugs caused by negated list options
  • Ensure disabled list arguments are not mistakenly treated as active during node startup
  • Concept approval without stated reasons (promag)
  • No objections raised
  • 392 lines added or modified outside tests
  • 793 lines added or modified in tests
  • 186 lines removed in total
  • 14 files, 17 commits
Utilities (logging, arguments, libraries)#11 of 59
P2 · bug fix
  • P2 because it eliminates an error-prone argument check that causes subtle node initialization bugs
  • Fixes improper startup failures when combining negated networking flags with other options
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

Depends on: #16545, #17580

Based on (shares commits with): #16545, #17580

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

dossier JSON · extract JSON · model ledger, generated 2026-11-01T01:16, confidence high, input hash 47987d45fc1969eb
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