rpc: Properly parse -rpcworkqueue/-rpcthreads #34582

pull maflcko wants to merge 3 commits into bitcoin:master from maflcko:2602-int-arg changing 5 files +70 −24
  1. maflcko commented at 2:40 pm on February 13, 2026: member

    The integral arg parsing has many issues:

    • There is no way to parse an unsigned integral type at all
    • There is no way to parse an integral type of less width than int64_t
    • As a result, calling code splatters confusing c-style casts just to let the code compile. However, usually there are no range checks and proper range handling.

    For example, when someone (maybe for testing) wants to set the rpc work queue to the maximum possible number, there is no easy way to do so without reading the source code and manually crafting the exact integer value. Using the “9999 hack” will silently set it to -1 (!)

    To test:

    /bld-cmake/bin/bitcoin-qt -datadir=/tmp -regtest -rpcworkqueue=99999999999999999999999999 -printtoconsole=1 -server=1 -debug=http | grep 'set work queue of depth'

    Before:

    0[http] set work queue of depth -1
    

    After:

    0[http] set work queue of depth 2147483647
    
  2. DrahtBot renamed this:
    rpc: Properly parse -rpcworkqueue/-rpcthreads
    rpc: Properly parse -rpcworkqueue/-rpcthreads
    on Feb 13, 2026
  3. DrahtBot added the label RPC/REST/ZMQ on Feb 13, 2026
  4. DrahtBot commented at 2:40 pm on February 13, 2026: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK stickies-v
    Concept ACK pinheadmz, furszy

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
    • #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
    • #16545 (refactor: Implement missing error checking for ArgsManager flags 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.

  5. pinheadmz commented at 4:22 pm on February 13, 2026: member
    concept ACK, I noticed this while testing #33689 and trying to break it with things like -rpcthreads=-1 and -rpcthreads=llama both of which didn’t throw errors
  6. maflcko commented at 4:31 pm on February 13, 2026: member
    Heh, this one does not throw any errors either. (That will likely be a more involved change). The change here just removes the confusion of silently mapping clearly positive integer values to negative ones.
  7. in src/httpserver.cpp:413 in fa155f01f2
    409@@ -410,8 +410,8 @@ bool InitHTTPServer(const util::SignalInterrupt& interrupt)
    410     }
    411 
    412     LogDebug(BCLog::HTTP, "Initialized HTTP server\n");
    413-    g_max_queue_depth = std::max((long)gArgs.GetIntArg("-rpcworkqueue", DEFAULT_HTTP_WORKQUEUE), 1L);
    414-    LogDebug(BCLog::HTTP, "set work queue of depth %d\n", g_max_queue_depth);
    415+    g_max_queue_depth = std::max(gArgs.GetArg<decltype(g_max_queue_depth)>("-rpcworkqueue", DEFAULT_HTTP_WORKQUEUE), 1);
    


    furszy commented at 6:43 pm on February 13, 2026:
    Isn’t the return type automatically detected from the type of the default value? Meaning, you shouldn’t need the decltype(g_max_queue_depth)

    maflcko commented at 8:56 am on February 16, 2026:
    Thx, right, it can be assumed that the DEFAULT_... value is of the same type. Dropped decltype twice
  8. furszy commented at 7:12 pm on February 13, 2026: member
    also, adding some coverage wouldn’t hurt. The -1 hack seems like a quick good candidate. Otherwise ACK.
  9. util: Add SettingTo<Int>() and GetArg<Int>()
    Redirect:
    * SettingToInt to SettingTo<int64_t>, and
    * GetIntArg to GetArg<int64_t>
    faee36f63b
  10. rpc: Properly parse -rpcworkqueue/-rpcthreads
    Also, remove the trailing unnecessary \n from the two logs.
    fac3ecaf69
  11. in src/common/args.h:100 in fa155f01f2 outdated
     97+
     98+template <std::integral Int>
     99+std::optional<Int> SettingTo(const common::SettingsValue&);
    100+
    101+inline int64_t SettingToInt(const common::SettingsValue& value, int64_t nDefault) { return SettingTo<int64_t>(value, nDefault); }
    102+inline std::optional<int64_t> SettingToInt(const common::SettingsValue& value) { return SettingTo<int64_t>(value); }
    


    stickies-v commented at 8:08 am on February 16, 2026:

    These are now only used in 1 other file, I think we should just remove them and have the callsites specify which integral type they want?

     0diff --git a/src/common/args.h b/src/common/args.h
     1index 477f2cbf6c..ea4e173bf7 100644
     2--- a/src/common/args.h
     3+++ b/src/common/args.h
     4@@ -96,9 +96,6 @@ Int SettingTo(const common::SettingsValue&, Int);
     5 template <std::integral Int>
     6 std::optional<Int> SettingTo(const common::SettingsValue&);
     7 
     8-inline int64_t SettingToInt(const common::SettingsValue& value, int64_t nDefault) { return SettingTo<int64_t>(value, nDefault); }
     9-inline std::optional<int64_t> SettingToInt(const common::SettingsValue& value) { return SettingTo<int64_t>(value); }
    10-
    11 bool SettingToBool(const common::SettingsValue&, bool);
    12 std::optional<bool> SettingToBool(const common::SettingsValue&);
    13 
    14diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp
    15index 3d876a8f63..cfbc350f09 100644
    16--- a/src/qt/optionsmodel.cpp
    17+++ b/src/qt/optionsmodel.cpp
    18@@ -88,14 +88,14 @@ static common::SettingsValue PruneSetting(bool prune_enabled, int prune_size_gb)
    19 static bool PruneEnabled(const common::SettingsValue& prune_setting)
    20 {
    21     // -prune=1 setting is manual pruning mode, so disabled for purposes of the gui
    22-    return SettingToInt(prune_setting, 0) > 1;
    23+    return SettingTo<int64_t>(prune_setting, 0) > 1;
    24 }
    25 
    26 //! Get pruning size value to show in GUI from bitcoin -prune setting. If
    27 //! pruning is not enabled, just show default recommended pruning size (2GB).
    28 static int PruneSizeGB(const common::SettingsValue& prune_setting)
    29 {
    30-    int value = SettingToInt(prune_setting, 0);
    31+    int value = SettingTo<int64_t>(prune_setting, 0);
    32     return value > 1 ? PruneMiBtoGB(value) : DEFAULT_PRUNE_TARGET_GB;
    33 }
    34 
    35@@ -469,9 +469,9 @@ QVariant OptionsModel::getOption(OptionID option, const std::string& suffix) con
    36                suffix.empty()          ? getOption(option, "-prev") :
    37                                          DEFAULT_PRUNE_TARGET_GB;
    38     case DatabaseCache:
    39-        return qlonglong(SettingToInt(setting(), DEFAULT_DB_CACHE >> 20));
    40+        return qlonglong(SettingTo<int64_t>(setting(), DEFAULT_DB_CACHE >> 20));
    41     case ThreadsScriptVerif:
    42-        return qlonglong(SettingToInt(setting(), DEFAULT_SCRIPTCHECK_THREADS));
    43+        return qlonglong(SettingTo<int64_t>(setting(), DEFAULT_SCRIPTCHECK_THREADS));
    44     case Listen:
    45         return SettingToBool(setting(), DEFAULT_LISTEN);
    46     case Server:
    

    maflcko commented at 3:25 pm on February 16, 2026:
    thx, done
  12. maflcko force-pushed on Feb 16, 2026
  13. maflcko commented at 8:58 am on February 16, 2026: member

    also, adding some coverage wouldn’t hurt. The -1 hack seems like a quick good candidate.

    There isn’t really any test to check arg parsing, and it seems a bit odd to test some, but not all. So maybe a follow-up can think about this. For now, I’ve added unit test coverage in the first commit.

  14. maflcko closed this on Feb 16, 2026

  15. maflcko reopened this on Feb 16, 2026

  16. DrahtBot added the label CI failed on Feb 16, 2026
  17. DrahtBot removed the label CI failed on Feb 16, 2026
  18. stickies-v commented at 2:42 pm on February 16, 2026: contributor
    Concept ACK
  19. refactor: [gui] Use SettingTo<int64_t> over deprecated SettingToInt
    This refactor does not change any behavior.
    
    Also, remove the now-unused deprecated aliases.
    fa5672dcaf
  20. stickies-v approved
  21. stickies-v commented at 4:43 am on February 17, 2026: contributor

    ACK fa5672dcafa154dff7409eaaf762febe1d76aad7

    Removes some httpserver platform-specific overflow behaviour by properly saturating instead, and the new templated functions make for a nice and safe arg/settings getting interface.

  22. DrahtBot requested review from furszy on Feb 17, 2026
  23. DrahtBot requested review from pinheadmz on Feb 17, 2026

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-02-17 06:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me