#17493 util: Forbid ambiguous multiple assignments in config file
https://github.com/bitcoin/bitcoin/pull/17493 · · +1196/-203 in 16 files, 17 commits · labels: Refactoring, Utils/log/libs · draft
Goal
- Fail startup with an explicit error when single-value settings are assigned multiple times in a config file
- Prevent node operators from mistakenly assuming a discarded duplicate setting is active
This PR causes `ArgsManager` to return an init error on startup when non-list settings (`ALLOW_ANY` without `ALLOW_LIST`) are assigned multiple times within the same section of a configuration file. It adds unit tests checking that ambiguous multi-assignments fail and includes release notes for the behavior change.
Problem: Currently, assigning multiple values to a single-value setting in a configuration file silently ignores all but one assignment (often subject to non-intuitive reverse-precedence rules). This can lead node operators to believe a setting is active when it is being silently discarded.
Category: Utilities (logging, arguments, libraries) (#15 of 66)
P3 · user request
- P3 because it eliminates silent misconfigurations from duplicate single-value options
- Protects node operators from invalid assumptions and unblocks cleanup in #17581
Improves configuration parsing reliability by throwing an explicit error when single-value settings are defined multiple times, preventing silently ignored settings. It also unblocks removing confusing reverse-precedence logic in #17581.
Membership: Directly modifies `ArgsManager` in `src/common/args.{h,cpp}` and `src/common/config.cpp`.
Factors: security/stability 1, bug 1, performance 0, user value 2, leverage 2
Reviewability: Ready: Review #16545 first
The PR is stacked on top of #16545 and #17580, with only two distinct commits of its own.
Author status: Active; regularly rebasing the PR on top of base PR updates.
Resolved concerns:
- ajtowns raised concern that nodes with existing duplicate config lines might fail to restart on upgrade; ajtowns subsequently agreed that warnings in logs are ineffective and release notes are the standard way to communicate upgrade breakage.
Agreement: Positive
- General support for catching duplicate configuration errors on startup
- Concept approval while weighing potential impact on existing configs (JeremyRubin)
- Concern over restart failures on upgrade resolved by using release notes (ajtowns)
Positive; ajtowns raised concern over breaking existing configs on upgrade but conceded release notes are sufficient.
JeremyRubin Concept ACKed the change. ajtowns expressed concern that it could break existing configurations on restart, but acknowledged that errors with release notes are better than ignored log warnings.
- JeremyRubin: 'Concept ACK. Curious if you think this has a high likelihood of breaking a lot of configs in the wild -- but I suppose it's worth it...'
- ajtowns: 'Weak concept NACK on this... potential to cause systems to not restart correctly...'
- ajtowns follow-up: 'But yeah, I agree a warning everyone will miss isn't that helpful... really that's what release notes and being careful about upgrades are for.'
Review verdicts (DrahtBot): 0 -1
- Concept ACK: JeremyRubin
- Concept NACK: ajtowns
Dependencies
Files
895 lines under test/bench/ci.
- src/test/argsman_tests.cpp +689/-71
- 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
- doc/release-notes-17493.md +8/-0
- src/chainparamsbase.cpp +4/-4
- src/common/config.cpp +7/-1
- src/init/common.cpp +3/-3
- src/test/logging_tests.cpp +3/-3
- test/lint/check-doc.py +3/-2
- 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
This PR modifies configuration file parsing to raise a startup error if a single-value setting is assigned multiple times within the same config section, rather than silently ignoring duplicate values. This prevents operator errors and unintended behavior from silently ignored options, while paving the way to clean up reverse-precedence config merging logic in #17581. Concept ACKed by JeremyRubin, with initial upgrade-breakage concerns by ajtowns resolved in favor of release note documentation. The PR is stacked on top of open PRs #16545 and #17580, which should be reviewed first.