#31260 scripted-diff: Type-safe settings retrieval
https://github.com/bitcoin/bitcoin/pull/31260 · · +3713/-1113 in 100 files, 11 commits · labels: Refactoring, Needs rebase
Goal
- Prevent recurring bugs and silent errors from misspelled option names, invalid types, or mismatched defaults
- Ensure configuration and command-line settings are checked at compile time across the codebase
This PR introduces a `common::Setting` template class to provide compile-time type safety for command-line arguments and configuration settings. It migrates all existing `AddArg` and `GetArg*` calls across the codebase to `Setting::Register` and `Setting::Get` using an automated scripted diff.
Problem: Command-line arguments and configuration settings currently rely on string keys and scattered default values, allowing typos in setting names, mismatched defaults between help documentation and call sites, and inconsistent type interpretations to go undetected at compile time.
Category: Utilities (logging, arguments, libraries) (#7 of 66)
P2 · cleanup
- P2 because it enforces compile-time safety across node configuration retrieval and registration
- Prevents a long-standing source of subtle bugs from misspelled argument names and mismatched defaults
- Unblocks planned runtime range and type checking across the configuration manager
P2 because replacing stringly-typed configuration retrieval with static type definitions removes a persistent, recurring class of bugs across the entire node. Mismatched defaults and negation footguns have caused multiple regressions and cleanups (#30529, #31212), and establishing this infrastructure directly unblocks type and range checking for arguments (#22978).
Membership: Implements core configuration and argument management infrastructure in src/common/setting.h and src/common/args.cpp.
Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 2
Reviewability: Stale: Needs rebase
- Review is blocked by merge conflicts with master until the author rebases
The PR has merge conflicts with current master and requires a rebase. Additionally, the author previously indicated an intent to restructure and split the changes into smaller PRs.
Author status: active; frequently rebasing and keeping branch building across refactors, though yet to split the PR or reply to recent review comments
Open concerns:
- jeanpablojp noted that in `setting_internal.h`, null elements in settings.json arrays are dropped silently rather than throwing a type error, changing behavior from master
- jeanpablojp noted the scripted-diff parser regex misses templated `GetArg<T>` calls, leaving five settings unmigrated
- Author noted in April 2026 an intention to split the PR so the core `Setting` template class is introduced first with a few call sites, before applying the full tree-wide scripted diff
Resolved concerns:
- Static and constexpr variable linkage cleanups in generated headers resolved with hodlinator
- Extensibility and parameter handling of `HelpFn` updated based on feedback from maflcko
- Circular dependency and documentation linter updates adjusted after reviewer review
Agreement: Mild
- Strong concept support for compile-time guardrails on configuration settings
- Much better to let the compiler enforce settings safety (l0rinc)
- Praise for the safety improvements with testing and script refinements (hodlinator, jeanpablojp)
- Discussion remains open on handling edge cases and possibly splitting the repository-wide diff
Strong concept consensus, but an unintended behavior difference in settings.json null handling remains unaddressed
Reviewers strongly favor replacing string-based argument access with typed declarations. However, a nonblocking comment from jeanpablojp regarding an unintended behavior difference in settings.json array parsing has not yet been addressed by the author.
- hodlinator: 'Overdue Concept ACK... No (intended) changes in behavior when executing the code, just installing compile time guardrails.'
- l0rinc: 'This looks really cool, strong concept ack!'
- jeanpablojp: 'A null element gets dropped silently here, and numbers now get converted, where the old GetArgs() threw.'
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| jeanpablojp | correctness | A null element in a settings.json array gets dropped silently instead of throwing a type error as on master | open | no | no | 2026-08-20: 'A null element gets dropped silently here, and numbers now get converted, where the old GetArgs() threw... Raising it because the commit says behavior doesn't change.' |
| hodlinator | maintenance | Non-constexpr static variables duplicated across compilation units causing redundant allocations | resolved | no | yes | 2024-11-20: "these other non-constexpr statics seem like they end up being \"file-local\" static vars in every compilation unit that includes them. Seems off?" Settled: 2024-11-22: "they are switched to constexpr string literal types now" |
Support:
- hodlinator: Installs compile-time guardrails against settings typos and default mismatches without runtime performance degradation
- l0rinc: Strong Concept ACK for having the compiler enforce settings safety rather than relying on runtime conventions
- jeanpablojp: Concept ACK after verifying the scripted diff, building, and passing test suites
Participants: hodlinator (support), l0rinc (support), Pitan1993 (neutral), maflcko (support), yancyribbens (neutral), jeanpablojp (objection)
State derived from the lists: nonblocking objection open (jeanpablojp)
Review verdicts (DrahtBot): 0
- Concept ACK: l0rinc, hodlinator, jeanpablojp
Dependencies
Enables:
Files
1232 lines under test/bench/ci.
- src/init_settings.h +913/-0
- src/init.cpp +291/-350
- src/test/argsman_tests.cpp +153/-152
- src/test/getarg_tests.cpp +146/-145
- src/test/argsman_tests_settings.h +222/-0
- src/common/setting_internal.h +219/-0
- src/common/setting.h +155/-0
- src/wallet/init_settings.h +147/-0
- src/bitcoin-cli_settings.h +138/-0
- src/bitcoin-cli.cpp +52/-67
- src/test/setting_tests.cpp +109/-0
- src/bitcoin-tx_settings.h +107/-0
- src/dummywallet_settings.h +97/-0
- src/init/common_settings.h +87/-0
- src/wallet/init.cpp +29/-35
- src/init/common.cpp +32/-31
- src/bitcoin-tx.cpp +25/-36
- src/chainparamsbase_settings.h +56/-0
- src/test/getarg_tests_settings.h +52/-0
- src/bench/bench_bitcoin_settings.h +47/-0
- src/dummywallet.cpp +23/-24
- src/node/settings.h +45/-0
- src/qt/bitcoin_settings.h +45/-0
- src/wallet/wallet.cpp +20/-19
- src/bitcoin-wallet_settings.h +36/-0
- contrib/devtools/circular-dependencies.py +31/-2
- src/bench/bench_bitcoin.cpp +15/-18
- src/node/mempool_args.cpp +16/-15
- src/common/args.cpp +19/-11
- src/qt/bitcoin.cpp +15/-13
- src/test/logging_tests_settings.h +24/-0
- src/test/util/setup_common.cpp +11/-11
- src/test/util/setup_common_settings.h +22/-0
- src/bitcoin-wallet.cpp +11/-10
- src/common/args_settings.h +21/-0
- src/net.cpp +11/-10
- src/chainparamsbase.cpp +10/-10
- src/validation.h +0/-20
- src/CMakeLists.txt +17/-1
- src/httprpc.cpp +9/-8
- src/node/settings.cpp +17/-0
- src/chainparams.cpp +9/-7
- src/bitcoin_settings.h +15/-0
- src/httpserver.cpp +8/-7
- src/node/chainstatemanager_args.cpp +8/-7
- src/qt/test/optiontests_settings.h +15/-0
- src/bitcoin-util_settings.h +13/-0
- src/node/peerman_args.cpp +7/-6
- src/qt/test/optiontests.cpp +7/-6
- src/qt/intro.cpp +6/-4
- src/bitcoind.cpp +5/-4
- src/common/init.cpp +5/-4
- src/wallet/test/init_tests.cpp +5/-4
- src/test/util/common.h +6/-2
- src/validation.cpp +0/-8
- test/lint/check-doc.py +4/-4
- src/bitcoin-util.cpp +4/-3
- src/node/blockmanager_args.cpp +4/-3
- src/node/caches.cpp +4/-3
- src/test/logging_tests.cpp +4/-3
- src/torcontrol.cpp +4/-3
- src/wallet/load.cpp +4/-3
- src/bitcoin.cpp +4/-2
- src/init.h +6/-0
- src/node/mining_args.cpp +4/-2
- src/common/config.cpp +3/-2
- src/node/coins_view_args.cpp +3/-2
- src/node/kernel_notifications.cpp +3/-2
- src/rpc/util.cpp +3/-2
- src/wallet/dump.cpp +3/-2
- src/wallet/spend.cpp +3/-2
- src/wallet/wallettool.cpp +3/-2
- src/wallet/walletutil.cpp +3/-2
- src/qt/walletmodel.cpp +2/-2
- src/addrdb.cpp +2/-1
- src/kernel/chainstatemanager_opts.h +2/-1
- src/node/chainstatemanager_args.h +0/-3
- src/node/database_args.cpp +2/-1
- src/node/interfaces.cpp +2/-1
- src/node/mempool_persist_args.cpp +2/-1
- src/node/miner.cpp +2/-1
- src/qt/optionsmodel.cpp +2/-1
- src/rpc/external_signer.cpp +2/-1
- src/rpc/mempool.cpp +2/-1
- src/rpc/request.cpp +2/-1
- src/rpc/server.cpp +2/-1
- src/test/addrman_tests.cpp +2/-1
- src/test/fuzz/addrman.cpp +2/-1
- src/test/fuzz/connman.cpp +2/-1
- src/wallet/coincontrol.cpp +2/-1
- src/wallet/db.cpp +2/-1
- src/wallet/external_signer_scriptpubkeyman.cpp +2/-1
- src/node/mining_args.h +0/-2
- src/common/args.h +1/-0
- src/node/blockstorage.cpp +1/-0
- src/qt/CMakeLists.txt +1/-0
- src/rpc/blockchain.cpp +1/-0
- src/test/CMakeLists.txt +1/-0
- src/test/util/CMakeLists.txt +1/-0
- src/test/validation_chainstatemanager_tests.cpp +1/-0
Card
This PR refactors Bitcoin Core's settings management to use a compile-time type-safe Setting template class, converting all AddArg and GetArg call sites via a scripted diff. It solves the recurring problem of typos in option names, conflicting default values between documentation and call sites, and inconsistent type interpretations. Reviewers strongly endorse the concept, though the PR currently needs a rebase, has an unresolved minor behavioral observation regarding settings.json parsing, and the author previously noted an intention to split the template class from the large scripted diff.