#31260 scripted-diff: Type-safe settings retrieval
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #31260 scripted-diff: Type-safe settings retrieval | Ready | 0 | Mild | 2,845 + 882 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
This pull request replaces string-based argument registration and lookup in ArgsManager with compile-time type-safe declarations using a new `common::Setting` template class. Argument name, expected type (bool, int, string, path, or list), description, and default values are centralized into static setting declarations across dedicated `*_settings.h` headers. A scripted diff migrates setting registrations and lookups repository-wide from `AddArg` and `Get*Arg` to typed `Setting::Register` and `Setting::Get` calls.
Problem: Command-line and configuration settings are referenced by raw string literals without compile-time checking. Typos in argument names are not caught at compile time, retrieval sites can parse conflicting types, and default values can silently drift between user help text and code fallback logic.
Evidence of importance:
- PR description: 'Currently settings are registered and retrieved like: argsman.AddArg("-pid=<file>", ...); args.GetPathArg("-pid", BITCOIN_PID_FILENAME)... But this is not ideal because nothing checks that setting names are spelled correctly, or that default values are used consistently in the help string and retrieval sites, or that settings are retrieved with consistent types.'
- PR description: 'a branch in issue #22978 extends the Setting class implemented here to support runtime setting validation, additional types like std::variant, additional conversion options, custom types, custom validation, and groups of settings declared as options structs.'
Scope: Very wide mechanical change touching 100 files (+3727/-1122). Almost all call-site changes are generated in a single scripted-diff commit via a temporary Python script (`contrib/devtools/reg-settings.py`), creating 15 new `*_settings.h` headers. A circular dependency linter rule is added to prevent `*_settings.h` headers from being included inside other headers.
Risk of a mistake: medium (rpc_api, wallet, mempool_policy, build, tests). A typo or incorrect type mapping in the generated settings headers could misparse critical runtime options, wallet configurations, or RPC parameters at startup.
Changed since the previous assessment: Rebased over Boost dependency cleanups (#34495/#34496), moved node constants to `src/node/settings.h` to avoid exposing Boost headers to settings consumers, added CMake include paths for UniValue/Boost/libevent to fix platform build failures, and refreshed IWYU includes in `init.cpp`.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: response to objections and questions from jeanpablojp and suggestion from hodlinator 2026-08-20
Agreement
Mild
objection that needs an answer has none yet (jeanpablojp)
Thread notes: Author updated the rebase log comment c:2699103082 to record subsequent rebases and conflict resolutions, including resolving conflicts with merged PR #36295, but has not yet replied to open objections from jeanpablojp or confirmed removal of GetPtr().
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| hodlinator (member) | objection, maintenance | non-constexpr static variables duplicated across compilation units causing redundant allocations | fixed · fix pushed · author switched to constexpr string literal types | answered | 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?' Status from: 2024-11-22: ryanofsky: 'Good catch. I wanted to avoid changing these to keep the commit move-only as much as possible but it doesn't make sense' |
| jeanpablojp (contributor) | objection, correctness | help text sentence for BlockFilterIndexSetting is omitted in the generated help | open | answered | no | 2026-08-20: 'This sentence disappears from the generated help, BlockFilterIndexSetting keeps only the strprintf format string. It still describes what the code does.' |
| jeanpablojp (contributor) | objection, correctness | null elements in settings.json arrays are silently dropped instead of triggering a type error as on master | open | answered | 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.' |
| jeanpablojp (contributor) | question, scope | whether the parser regex should match templated GetArg<T> calls to migrate the five remaining settings | open | answered | no | 2026-08-20: 'This one is still read through args.GetArg<int32_t>("-prevoutfetchthreads"), but was generated as common::Unset. The script's regex matches GetArg( but not GetArg<T>(. Five in total... Nothing breaks,' |
| jeanpablojp (contributor) | objection, correctness | check-doc linter misses ProxySettingStr::Get and ::Value calls, allowing mismatched setting names to pass unnoticed | open | answered | no | 2026-08-20: 'This only matches ::Get on identifiers ending in Setting, so it misses ProxySettingStr::Get and ::Value calls like CJdnsReachableSetting::Value in init.cpp.' |
| hodlinator (member) | suggestion, maintenance | remove SettingTo/SettingToString/SettingToBool function overloads to eliminate the need for GetPtr() | accepted · hodlinator moved this cleanup to separate PR #36295 | answered | by others | 2026-08-26: 'preferable to remove the overloads and thereby remove the need for GetPtr()' Status from: 2026-09-18: hodlinator: 'Published: #36295' |
| hodlinator (member) | suggestion, maintenance | remove GetPtr() on next rebase now that #36295 has merged | open | answered | no | 2026-10-20: '#36295 has been merged so `GetPtr()` can be removed from this PR on the next rebase.' |
Support:
- l0rinc (member) (Concept ACK, 2024-12-10) [read]: it's a lot better to have the compiler do the job for us
- hodlinator (member) (Concept ACK, 2025-07-02) [read]: No (intended) changes in behavior when executing the code, just installing compile time guardrails
- jeanpablojp (contributor) (Concept ACK, 2026-08-20) [tested]: Ran the script from the scripted diff, then built and ran the tests
Participants:
- hodlinator (member, support, 49 statements 2024-11-20 to 2026-10-20): Concept ACK; reviewed template usage, compilation impact, and suggested various code and script cleanups
- l0rinc (member, support, 42 statements 2024-12-10 to 2024-12-12): Concept ACK for compile-time enforcement of settings
- Pitan1993 (none, neutral, 1 statements 2024-12-31 to 2024-12-31): changes-requested review quoting a single CI log line without explanation
- maflcko (member, neutral, 7 statements 2025-02-18 to 2026-09-17): suggested improvements to DefaultFn, HelpFn extensibility, and removing IsArgSet uses
- yancyribbens (contributor, neutral, 1 statements 2025-02-21 to 2025-02-21): pointed out a typo in a comment
- jeanpablojp (contributor, objection, 6 statements 2026-08-17 to 2026-08-20): Concept ACK after testing, but raised objections regarding missing help text, silent behavior changes with null values, and linter gaps
Dependencies
Files
1232 lines under test/bench/ci.
- src/init_settings.h +910/-0
- src/init.cpp +292/-356
- 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 +153/-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 +101/-0
- src/init/common_settings.h +87/-0
- src/wallet/init.cpp +30/-36
- src/init/common.cpp +32/-31
- src/bitcoin-tx.cpp +25/-36
- src/chainparamsbase_settings.h +59/-0
- src/test/getarg_tests_settings.h +52/-0
- src/dummywallet.cpp +24/-25
- src/bench/bench_bitcoin_settings.h +47/-0
- src/node/settings.h +45/-0
- src/qt/bitcoin_settings.h +45/-0
- src/wallet/wallet.cpp +21/-20
- src/bitcoin-wallet_settings.h +36/-0
- contrib/devtools/circular-dependencies.py +31/-2
- src/bench/bench_bitcoin.cpp +15/-18
- src/common/args.cpp +19/-11
- src/node/mempool_args.cpp +15/-14
- src/qt/bitcoin.cpp +15/-13
- src/test/logging_tests_settings.h +24/-0
- src/chainparamsbase.cpp +11/-11
- 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/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
The short text the ranking pass compares across PRs.
This PR replaces string-based configuration argument handling in ArgsManager with compile-time type-safe declarations using a new common::Setting template class. Currently, settings are registered and queried via unchecked string literals, allowing typos, conflicting type queries, and default-value mismatches to compile without warnings. A scripted-diff commit converts setting registrations and lookups across 100 files to typed Setting::Register and Setting::Get calls. The change establishes compile-time safety and serves as the foundation for runtime type checking and validation in #16545 and #22978.
Data
thread read through 2026-10-20 (166 statements), head 086c22104d
labels: Refactoring
Categories
Utilities (logging, arguments, libraries)
P2 · cleanup
Provides a comprehensive compile-time type-safety overhaul for repository-wide configuration settings. The author notes that 'nothing checks that setting names are spelled correctly, or that default values are used consistently in the help string and retrieval sites', eliminating a wide class of silent configuration bugs and unblocking runtime setting validation in #22978.
Membership: Introduces common::Setting and refactors ArgsManager registration and lookup across src/common/args* and src/init*.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 2