scripted-diff: Type-safe settings retrieval by ryanofsky · Pull Request #31260 · bitcoin/bitcoin

ryanofsky · 2024-11-08T22:34:46Z

This PR changes the way settings are registered and retrieved to provide more compile-time safety. Currently settings are registered and retrieved like:

// Register setting
argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

// Retrieve setting
args.GetPathArg("-pid", BITCOIN_PID_FILENAME)

But this is not ideal because nothing checks that setting names are spelled correctly, or that default values (BITCOIN_PID_FILENAME) are used consistently in the help string and retrieval sites, or that settings are retrieved with consistent types (bool, int, string, path, or list). This PR addresses these issues by adding setting declarations which allow settings to be registered and retrieved like:

// Declare setting
using PidSetting = common::Setting<
    "-pid=<file>", fs::path, {.legacy = true},
    "Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)">
    ::DefaultFn<[] { return BITCOIN_PID_FILENAME; }>;

// Register setting
PidSetting::Register(argsman);

// Retrieve setting
PidSetting::Get(args)

Suggestions for review

All the real changes in this PR are in the last scripted-diff commit:

scripted-diff: Replace AddArgs / GetArgs calls with Setting Register / Get calls

If you take a few minutes to look at the changes applied in this scripted-diff commit, you'll understand everything this PR is doing. A good place to start looking around in this commit is the generated src/init_settings.h file which declares the settings that get registered in src/init.cpp. Then you can look at the surrounding diffs and see they are just replacing AddArg and GetArg calls.

The other notable commit is second commit, which implements the Setting class:

common: Add Setting class to support typed Settings

The other commits do minor things like moving code or updating linters. The python script that implements the scripted diff is a temporary artifact that gets deleted. The python script is complicated because it does things like parsing c++ code to extract help strings, and figuring out the right types to declare settings with so code compiles. But the entire scope of the script is to (1) generate Setting type definitions, (2) add #includes, and (3) replace AddArg() calls with Register() calls and GetArg() calls with Get() calls. So there is not much the script can actually do wrong without triggering build and test failures.

Extensions

This PR only adds the ability to declare individual settings with built-in types. It doesn't provide any new runtime behavior, but 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.

This change is mostly orthogonal to #16545. #16545 only provides runtime type checking while this PR only provides compile-time checking with no new runtime behavior. But this change does allow a nicer way of declaring types in #16545, using c++ types like int instead of flags like ALLOW_INT, orstd::vector<std::string> instead of ALLOW_STRING | ALLOW_LIST.

DrahtBot · 2024-11-08T22:34:49Z

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/31260.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	l0rinc, hodlinator

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/762 (Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog by pablomartin4btc)
#35713 (Remove boost as a unit test runner by rustaceanrob)
#35675 (mining: add block template manager by ismaelsadeeq)
#35592 (http: check rpcallowip immediately after accepting connection by pinheadmz)
#35586 (doc: note -blocknotify is not run during IBD/reindex in help text by fernandguil)
#35581 (node: add block template manager and track waitNext fee inflow by ismaelsadeeq)
#35578 (net: don’t self advertise tor exit node ip addresses in outbound connections by stratospher)
#35429 (wallet: avoid global access in external signer SPKM by w0xlt)
#35387 (logging: make trace logging easily usable by ryanofsky)
#35322 (logging: streamline Logger state and drop redundant methods by ryanofsky)
#35205 (kernel,node: add dbcache setter and clarify defaults by l0rinc)
#35076 (doc: clarify pruning impact on wallet sync by MemeticMoney)
#35037 (ipc: support per-address max-connections options on -ipcbind by enirox001)
#34995 (iwyu: Fix warnings in src/common and treat them as errors by hebasto)
#34978 (init: reserve file descriptors for IPC connections by enirox001)
#34812 (net: advertise CJDNS addresses when -externalip disables discovery by w0xlt)
#34681 (wallet: move rescan logic into ChainScanner and wallet/scan by Eunovo)
#34628 (p2p: Replace per-peer transaction rate-limiting with global rate limits by ajtowns)
#34603 (wallet: Fix detection of symlinks on Windows by achow101)
#34534 (rpc: Manual prune lock management (Take 2) by fjahr)
#34486 (net: Reduce local network activity when networkactive=0 by willcl-ark)
#34400 (wallet: parallel fast rescan (approx 5x speed up with 8 threads) by Eunovo)
#34132 (coins: drop error catcher, centralize fatal read handling by l0rinc)
#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
#34038 (logging: replace -loglevel with -trace, expose trace logging via RPC by ajtowns)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#33540 (argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments) by pablomartin4btc)
#33324 (blocks: add -reobfuscate-blocks argument to enable (de)obfuscating existing blocks by l0rinc)
#30951 (net: option to disallow v1 connection on ipv4 and ipv6 peers by stratospher)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
#28690 (build: Introduce internal kernel library by sedited)
#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#17783 (common: Disallow calling IsArgSet() on ALLOW_LIST options by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

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:

it just allows allows new runtime safety checks... -> it just allows new runtime safety checks... [duplicate word]
It accept a list of constexpr values... -> It accepts a list of constexpr values... [subject-verb agreement]

Possible places where comparison-specific test macros should replace generic comparisons:

[src/test/getarg_tests.cpp] BOOST_CHECK_THROW(FooSettingBool::Get(args, true), std::runtime_error); (and the other repeated BOOST_CHECK_THROW(..., std::runtime_error) cases) -> consider BOOST_CHECK_EXCEPTION(..., std::runtime_error, HasReason("...")) so the failure message is verified, not just the exception type.

^{2026-07-14 14:57:53}

ryanofsky · 2024-11-08T23:36:25Z

Current status of this PR is that bitcoind and test_bitcoin binaries work and functional and unit tests pass, but there are compile errors in the other binaries that need to be fixed, and this also needs to be rebased. The PR is complete with all functionality described above implemented, but it probably needs more documentation. I also would like to add more commits replacing last remaining GetArg / GetIntArg / GetBoolArg / GetArgs / IsArgSet / IsArgNegated method uses with Setting::Get and dropping all those methods.

ryanofsky · 2024-11-13T00:36:03Z

Updated 416860f -> 47e30b4 (pr/scripty.1 -> pr/scripty.2, compare) getting the remaining binaries (not just test_bitcoin and bitcoind) to build, simplifying the way optional and default values are used, making many other cleanups and fixes.
Rebased 47e30b4 -> d8a4a0a (pr/scripty.2 -> pr/scripty.3, compare) due to conflicts
Updated d8a4a0a -> ad32a27 (pr/scripty.3 -> pr/scripty.4, compare) with a fix for the feature_logging.py test that got broken by the previous optional/default changes. Also includes cmake changes to fix some CI build errors.
Updated ad32a27 -> 534f971 (pr/scripty.4 -> pr/scripty.5, compare) replacing IsArgSet calls with Setting::Value().isNull() instead of using std::optional and Setting::Get() to avoid changing any behavior since Get() fully parses the setting and can throw exceptions, and to reduce uses of std::optional which complicated code. Also add basic unit tests and make various script improvements.
Rebased 534f971 -> 7e57237 (pr/scripty.5 -> pr/scripty.6, compare) with a number of changes intended to fix CI errors, and with a fix for silent merge conflict with #31174
Rebased 7e57237 -> 1f24346 (pr/scripty.6 -> pr/scripty.7, compare) due to conflict #31287 and many workarounds for various compiler issues in CI and fixes for more linters

DrahtBot · 2024-11-13T01:21:21Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/32896439725}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot · 2026-06-24T15:23:50Z

🚧 At least one of the CI tasks failed.
_{Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/28106613355/job/83222475522}
_{LLM reason (✨ experimental): IWYU failed (auto-fix created a diff in src/bench/bench_bitcoin.cpp, causing the job to exit with “Failure generated from IWYU”).}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Avoid string concatenation and add missing namespace names so AddArg calls can be moved to header files in an upcoming scripted-diff.

Move constant declarations referenced in AddArg calls to headers so AddArg calls be moved to header files in an upcoming scripted-diff.

Move node constants referenced in GetArg calls to node/settings header so they can be referenced including settings will not pull in node headers like validation.h txmempool.h that depend on boost. After bitcoin#34495 and bitcoin#34496, boost headers can't be included from files like src/chainparams.cpp that reference settings.

Drop hidden_args vector so all AddHiddenArgs calls can be replaced in upcoming scripted diff.

Linter changes to deal with _settings.h headers added in the next commit. In circular-dependencies.py, prevent _settings.h headers from being included transitively. Add a new check that makes it an error to include _settings.h from another header file, rather than a .cpp file. Use this fact to avoid errors about .cpp files including _settings.h files being circularly dependent based on each other. In check-doc.py update test to look for Setting::Get and Setting::Register instead of GetArg and AddArg.

Add univalue include directory to libraries that need it it because they will include it indirectly through setting.h files, and add libevent include directory to libraries that need it because they will include it indirectly through init_settings.h which includes torcontrol.h which includes event2/util.h. The missing cmake library dependencies trigger CI errors that may not happen on all systems depending in how package include paths are laid out. In particular the torcontrol / libevent errors only seem to happen on macos with homebrew. It would probably be good to streamline headers to avoid these dependencies in the future. It would be good if the torcontrol header did not have a pulic dependency on libevent or, if it was just split up and the DEFAULT_TOR_CONTROL and DEFAULT_TOR_CONTROL_PORT constants needed by init_settings.h to a different file. Errors look like: D:\a\bitcoin\bitcoin\src\common\setting.h(10,1): error C1083: Cannot open include file: 'univalue.h': No such file or directory [D:\a\bitcoin\bitcoin\build\src\bitcoin.vcxproj] In file included from /home/admin/actions-runner/_work/_temp/src/bitcoin.cpp:7: In file included from /home/admin/actions-runner/_work/_temp/src/bitcoin_settings.h:4: /home/admin/actions-runner/_work/_temp/src/common/setting.h:10:10: fatal error: 'univalue.h' file not found 10 | #include <univalue.h> | ^~~~~~~~~~~~ 1 error generated. D:\a\bitcoin\bitcoin\src\txmempool.h(27,1): error C1083: Cannot open include file: 'boost/multi_index/hashed_index.hpp': No such file or directory [D:\a\bitcoin\bitcoin\build\src\bitcoin.vcxproj] In file included from /home/admin/actions-runner/_work/_temp/src/validation.h:29, from /home/admin/actions-runner/_work/_temp/src/node/chainstatemanager_args.h:9, from /home/admin/actions-runner/_work/_temp/src/init_settings.h:21, from /home/admin/actions-runner/_work/_temp/src/bitcoin.cpp:10: /home/admin/actions-runner/_work/_temp/src/txmempool.h:27:10: fatal error: boost/multi_index/hashed_index.hpp: No such file or directory 27 | #include <boost/multi_index/hashed_index.hpp> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /Users/runner/work/bitcoin/bitcoin/src/qt/bitcoin.cpp:14: In file included from /Users/runner/work/bitcoin/bitcoin/src/init_settings.h:30: /Users/runner/work/bitcoin/bitcoin/src/torcontrol.h:14:10: fatal error: 'event2/util.h' file not found 14 | #include <event2/util.h> | ^~~~~~~~~~~~~~~ 1 error generated. ninja: build stopped: subcommand failed. In file included from /Users/runner/work/bitcoin/bitcoin/src/test/util/setup_common.cpp:17: In file included from /Users/runner/work/bitcoin/bitcoin/src/init_settings.h:30: /Users/runner/work/bitcoin/bitcoin/src/torcontrol.h:14:10: fatal error: 'event2/util.h' file not found 14 | #include <event2/util.h> | ^~~~~~~~~~~~~~~ 1 error generated. In file included from /Users/runner/work/bitcoin/bitcoin/src/common/config.cpp:6: In file included from /Users/runner/work/bitcoin/bitcoin/src/init_settings.h:30: /Users/runner/work/bitcoin/bitcoin/src/torcontrol.h:14:10: fatal error: 'event2/util.h' file not found 14 | #include <event2/util.h> | ^~~~~~~~~~~~~~~ 1 error generated. In file included from /Users/runner/work/bitcoin/bitcoin/src/bitcoin.cpp:10: In file included from /Users/runner/work/bitcoin/bitcoin/src/init_settings.h:30: /Users/runner/work/bitcoin/bitcoin/src/torcontrol.h:14:10: fatal error: 'event2/util.h' file not found 14 | #include <event2/util.h> | ^~~~~~~~~~~~~~~ 1 error generated. In file included from /Users/runner/work/bitcoin/bitcoin/src/bitcoind.cpp:15: In file included from /Users/runner/work/bitcoin/bitcoin/src/init_settings.h:30: /Users/runner/work/bitcoin/bitcoin/src/torcontrol.h:14:10: fatal error: 'event2/util.h' file not found 14 | #include <event2/util.h> | ^~~~~~~~~~~~~~~ 1 error generated.

…egister / Get calls Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

…/ Get calls This commit is a pure refactoring and does not change behavior in any way. -BEGIN VERIFY SCRIPT- python contrib/devtools/reg-settings.py git add -N src/bench/bench_bitcoin_settings.h src/bitcoin_settings.h src/bitcoin-tx_settings.h src/bitcoin-util_settings.h src/bitcoin-wallet_settings.h src/chainparamsbase_settings.h src/common/args_settings.h src/init/common_settings.h src/init_settings.h src/qt/bitcoin_settings.h src/test/argsman_tests_settings.h src/test/logging_tests_settings.h src/wallet/init_settings.h src/dummywallet_settings.h src/qt/test/optiontests_settings.h src/test/getarg_tests_settings.h git rm contrib/devtools/reg-settings.py -END VERIFY SCRIPT-

DrahtBot · 2026-07-14T01:43:53Z

🚧 At least one of the CI tasks failed.
_{Task Alpine (musl): https://github.com/bitcoin/bitcoin/actions/runs/29296593802/job/86971366411}
_{LLM reason (✨ experimental): CI failed because the build stopped with a C++ compile error: MAX_PREVOUTFETCH_THREADS and DEFAULT_PREVOUTFETCH_THREADS were not declared in init_settings.h (used by chainparams.cpp).}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

ryanofsky marked this pull request as draft November 8, 2024 22:34

DrahtBot added the Needs rebase label Nov 8, 2024

ryanofsky force-pushed the pr/scripty branch 2 times, most recently from 47e30b4 to d8a4a0a Compare November 13, 2024 00:31

DrahtBot added the CI failed label Nov 13, 2024

DrahtBot removed the Needs rebase label Nov 13, 2024

ryanofsky force-pushed the pr/scripty branch from d8a4a0a to ad32a27 Compare November 13, 2024 03:21

ryanofsky force-pushed the pr/scripty branch from ad32a27 to 534f971 Compare November 14, 2024 00:40

This was referenced Nov 14, 2024

kernel, refactor: return error status on all fatal errors #29700

Draft

scripted-diff: Use LogInfo over LogPrintf #29641

Merged

DrahtBot added the Needs rebase label Nov 14, 2024

This was referenced Nov 14, 2024

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Merged

Extend signetchallenge to set target block spacing #29365

Closed

Wallet: Add maxfeerate wallet startup option #29278

Open

This was referenced May 30, 2026

mining: add getMemoryLoad() and track template non-mempool memory footprint #33922

Draft

cli: added -rpcmaxresponsesize to cap HTTP response size #35420

Closed

wallet: avoid global access in external signer SPKM #35429

Open

DrahtBot mentioned this pull request Jun 8, 2026

node: move index ownership to NodeContext #35474

Open

ryanofsky and others added 11 commits July 13, 2026 15:29

common: expose SettingToPath function

8279be5

common: Add Setting class to support typed Settings

f0fd678

test: Add test for common::Setting class

f0ee850

init, refactor: Prepare AddArg calls for scripted-diff

7172157

Avoid string concatenation and add missing namespace names so AddArg calls can be moved to header files in an upcoming scripted-diff.

move-only: move AddArg default values to headers

3429229

Move constant declarations referenced in AddArg calls to headers so AddArg calls be moved to header files in an upcoming scripted-diff.

refactor: Prepare AddHiddenArgs call for scripted-diff

019d2d3

Drop hidden_args vector so all AddHiddenArgs calls can be replaced in upcoming scripted diff.

contrib: Add script to replace AddArgs / GetArgs calls with Setting R…

9fc7b98

…egister / Get calls Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

scripted-diff: Type-safe settings retrieval#31260

scripted-diff: Type-safe settings retrieval#31260
ryanofsky wants to merge 11 commits into
bitcoin:masterfrom
ryanofsky:pr/scripty

ryanofsky commented Nov 8, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Nov 8, 2024 •

edited

Loading

Uh oh!

ryanofsky commented Nov 8, 2024

Uh oh!

ryanofsky commented Nov 13, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Nov 13, 2024

Uh oh!

DrahtBot commented Jun 24, 2026

Uh oh!

DrahtBot commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

ryanofsky commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Suggestions for review

Extensions

Uh oh!

DrahtBot commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

ryanofsky commented Nov 8, 2024

Uh oh!

ryanofsky commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 13, 2024

Uh oh!

DrahtBot commented Jun 24, 2026

Uh oh!

DrahtBot commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ryanofsky commented Nov 8, 2024 •

edited

Loading

DrahtBot commented Nov 8, 2024 •

edited

Loading

ryanofsky commented Nov 13, 2024 •

edited

Loading