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, pinheadmz, sedited
    Concept ACK 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
  24. in src/common/args.h:310 in faee36f63b
    307+
    308+    template <std::integral Int>
    309+    std::optional<Int> GetArg(const std::string& strArg) const;
    310+
    311+    int64_t GetIntArg(const std::string& strArg, int64_t nDefault) const { return GetArg<int64_t>(strArg, nDefault); }
    312+    std::optional<int64_t> GetIntArg(const std::string& strArg) const { return GetArg<int64_t>(strArg); }
    


    pinheadmz commented at 7:05 pm on February 17, 2026:

    faee36f63b5fde886458d0415778719ea2233d14

    nit: shouldn’t GetIntArg stay adjacent to the doxygen comment?


    maflcko commented at 8:09 pm on February 17, 2026:
    I think the legacy alias can be considered for removal in the future
  25. pinheadmz approved
  26. pinheadmz commented at 8:01 pm on February 17, 2026: member

    ACK fa5672dcafa154dff7409eaaf762febe1d76aad7

    Built and tested on macos/arm64. Compared -rpcthreads behavior against latest release and confirmed the issue. Reviewed each commit, only one style question below.

    Most default argument values are declared with int, and those defaults’ types now determine the template of GetArgInt(). This reduces the range of these arguments from their 64 bit width on master to 32. I spent a little time looking for any configuration arguments where this would be a problem and there aren’t any.

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA256
     2
     3ACK fa5672dcafa154dff7409eaaf762febe1d76aad7
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmmUwt8ACgkQ5+KYS2KJ
     7yTo/3RAAwFwkQssd8PiABI5ScwQqiKuvAxX2oO3yWHvHlIMYvi2sv/QuroKr5Ja8
     8rUX3rrCcuZbGsikMq1nCAajRyWPLAmXdXDTQPhuduaM7R5AF+L0b/JUVO1PtIUmn
     9PCX2Zwu3euNLV3K2x9OmzuI12XYbVCcnyYmnhyQ6YT9Bv9JKfbvp1e6YNgLk6jZ4
    10NwCuFSkMqWmZ2NfAiWnAK6tTrHHTeU8WJpD4AbWJVR0Tfu7RFrqUl0Ax93AF7flR
    11U5B7dL7jqQgrIEa7jbUQnTlE2Idxsll/vy3BTCtjdQ8/cJvJ52Mi1Xvc0gLYrz2Y
    12SmT6zmr30oEsq6iTlRK5UVxWT0GWvQwFHuwhKZaB8ry6MYAndUpu9eDP9o5KLMjX
    13MfB3k/AC+BZzc/NW0xcijPNtVmU7SNe+duUd9H3Gt5kRmKWhCmHSvJolO5rsvqIJ
    149F6CZIhBOgvODIOaoxUzh/ZeJ4l7v7wgTY1yfcvQ0+1G64ht48uPExhOAy1ZB/KE
    15arKbiVMGQbodY1MJ68OUvqnq8cDp2Nl+z4SRaWdnpZRdi8r/6EcLw+8SNCJX3xou
    166Adoa5uDahjlLLHnncM6uJMEqmxoDuCFWYUCBaWnTufX6gDedGWJ2T+1Kj+HXkzT
    173W/EM+mMh80iMZuPaPXm6HOCoaC9FyoMFTZoyFXwdnBp1vVfxlo=
    18=xVdv
    19-----END PGP SIGNATURE-----
    

    pinheadmz’s public key is on openpgp.org

  27. maflcko commented at 8:09 pm on February 17, 2026: member

    Most default argument values are declared with int, and those defaults’ types now determine the template of GetArgInt(). This reduces the range of these arguments from their 64 bit width on master to 32. I spent a little time looking for any configuration arguments where this would be a problem and there aren’t any.

    I don’t think this is true. GetIntArg is hard-coded to

    0    int64_t GetIntArg(const std::string& strArg, int64_t nDefault) const { return GetArg<int64_t>(strArg, nDefault); }
    

    , which is the exact signature and behavior that was used in the past. So there is no change in behavior here. I see that I forgot to put “This refactor does not change any behavior.” in the first commit. Happy to do that on the next push.

  28. pinheadmz commented at 8:18 pm on February 17, 2026: member

    I don’t think this is true

    Right thanks, my mistake. Vision got blurry on the templated GetArg vs GetIntArg

  29. sedited approved
  30. sedited commented at 3:20 pm on February 19, 2026: contributor
    ACK fa5672dcafa154dff7409eaaf762febe1d76aad7
  31. sedited merged this on Feb 19, 2026
  32. sedited closed this on Feb 19, 2026

  33. maflcko deleted the branch on Feb 19, 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-03-15 09:13 UTC

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