bitcoin
util: Forbid ambiguous multiple assignments in config file
This is based on #16545 + #17580. The non-base commits are:
928aa85db517util: Forbid ambiguous multiple assignments in config fileec1698a0a370test: Extend util_ArgsMerge test to check for "Multiple values specified" errors
Enable error "Multiple values specified for -setting in same section of config file.", for ALLOW_ANY settings that don't specify ALLOW_LIST.
Instead of silently ignoring settings, this change makes it an error to provide an ambiguous config file that provides assigns multiple values to a single-value setting. Change includes release notes.
Part of the motivation for this change is to improve usability and prevent settings that look valid from being silently ignored. Another motivation is to be able to remove confusing "reverse precedence" logic in #17581
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage & Benchmarks
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/17493.
Reviews
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | ajtowns |
| Concept ACK | JeremyRubin |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #33920 (Export embedded ASMap RPC by fjahr)
- #33892 (policy: Remove individual transaction <minrelay restriction by instagibbs)
- #33770 (init: Require explicit -asmap filename by ryanofsky)
- #33631 (init: Split file path handling out of -asmap option by fjahr)
- #33629 (Cluster mempool by sdaftuar)
- #33343 (help: enrich help text for
-loadblockby HowHsu) - #32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
- #32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)
- #31974 (Drop testnet3 by Sjors)
- #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
- #28792 (build: Embedded ASMap [3/3]: Build binary dump header file by fjahr)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
LLM Linter (✨ experimental)
Possible typos and grammar issues:
- canoot -> cannot [typo in “canoot be negated” making the phrase invalid]
drahtbot_id_4_m
Weak concept NACK on this, precedence in config files might be confusing but it's not ambiguous and specifying setting=1 \n setting=2 is something you might plausibly see in the wild, so this change has the potential to cause systems to not restart correctly... Maybe having a warning rather than an error for a release would be better? Still, better to make the change once for everything than gradually as things get switched away from ALLOW_ANY to something more specific.
so this change has the potential to cause systems to not restart correctly...
This description sounds a little misleading to me. It is true that if someone had an ambiguous config file, and they updated to a new bitcoin version and then restarted without testing, they would see a "Multiple values specified for -foo in same section of config file" init error. But there wouldn't be a question of correctness, or risk that the new version would start up using different settings than the old version.
Personally, I don't think a debug log warning would be as good as an error here, given experience with issues like #15629 where warnings have seemed pretty easy to miss. I understand your concern about the case where someone upgrades their version of bitcoin and sees an error about an insignificant ambiguity in their config file. But I wouldn't want to forget about cases where someone could upgrade bitcoin be prompted to fix config errors that actually affected their privacy or security.
I wonder what you think about Jeremy's suggestion to prevent errors in more spurious cases where a setting is repeated but the value doesn't change: https://github.com/bitcoin/bitcoin/pull/17493#pullrequestreview-318636254. It'd be pretty easy to tweak the PR to avoid triggering errors in these cases.
This description sounds a little misleading to me.
Sure; I meant "restart correctly" as in "keep working the way things are supposed to rather than gratuitously break and cause the sysadmin headaches" :) But yeah, I agree a warning everyone will miss isn't that helpful, and there aren't any better approaches coming to mind; really that's what release notes and being careful about upgrades are for.
I'd probably wait to see someone using it in real life before adding special cases for duplicate settings with the same param/value.
Moved this to draft for now, given it's based on multiple other PRs, some of which are also drafts.
