Merge bitcoin/bitcoin#34582: rpc: Properly parse -rpcworkqueue/-rpcthreads

fa5672dcaf refactor: [gui] Use SettingTo<int64_t> over deprecated SettingToInt (MarcoFalke)
fac3ecaf69 rpc: Properly parse -rpcworkqueue/-rpcthreads (MarcoFalke)
faee36f63b util: Add SettingTo<Int>() and GetArg<Int>() (MarcoFalke)

Pull request description:

  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:

  ```
  [http] set work queue of depth -1
  ```

  After:

  ```
  [http] set work queue of depth 2147483647

ACKs for top commit:
  stickies-v:
    ACK fa5672dcaf
  pinheadmz:
    ACK fa5672dcaf
  sedited:
    ACK fa5672dcaf

Tree-SHA512: e5060453a0aa1c4e27080e928b0ae2d1015fe487246e4059866eef415f301bc7712ce306d95076ce5b66a5e57c620715b33998192c0ff06b0384085a0390c714

src/common/args.cpp

@@ -483,29 +483,33 @@ std::string SettingToString(const common::SettingsValue& value, const std::strin
     return SettingToString(value).value_or(strDefault);
 }
 
-int64_t ArgsManager::GetIntArg(const std::string& strArg, int64_t nDefault) const
+template <std::integral Int>
+Int ArgsManager::GetArg(const std::string& strArg, Int nDefault) const
 {
-    return GetIntArg(strArg).value_or(nDefault);
+    return GetArg<Int>(strArg).value_or(nDefault);
 }
 
-std::optional<int64_t> ArgsManager::GetIntArg(const std::string& strArg) const
+template <std::integral Int>
+std::optional<Int> ArgsManager::GetArg(const std::string& strArg) const
 {
     const common::SettingsValue value = GetSetting(strArg);
-    return SettingToInt(value);
+    return SettingTo<Int>(value);
 }
 
-std::optional<int64_t> SettingToInt(const common::SettingsValue& value)
+template <std::integral Int>
+std::optional<Int> SettingTo(const common::SettingsValue& value)
 {
     if (value.isNull()) return std::nullopt;
     if (value.isFalse()) return 0;
     if (value.isTrue()) return 1;
-    if (value.isNum()) return value.getInt<int64_t>();
+    if (value.isNum()) return value.getInt<Int>();
-    return LocaleIndependentAtoi<int64_t>(value.get_str());
+    return LocaleIndependentAtoi<Int>(value.get_str());
 }
 
-int64_t SettingToInt(const common::SettingsValue& value, int64_t nDefault)
+template <std::integral Int>
+Int SettingTo(const common::SettingsValue& value, Int nDefault)
 {
-    return SettingToInt(value).value_or(nDefault);
+    return SettingTo<Int>(value).value_or(nDefault);
 }
 
 bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const
@@ -531,6 +535,23 @@ bool SettingToBool(const common::SettingsValue& value, bool fDefault)
     return SettingToBool(value).value_or(fDefault);
 }
 
+#define INSTANTIATE_INT_TYPE(Type)                                                    \
+    template Type ArgsManager::GetArg<Type>(const std::string&, Type) const;          \
+    template std::optional<Type> ArgsManager::GetArg<Type>(const std::string&) const; \
+    template Type SettingTo<Type>(const common::SettingsValue&, Type);                \
+    template std::optional<Type> SettingTo<Type>(const common::SettingsValue&)
+
+INSTANTIATE_INT_TYPE(int8_t);
+INSTANTIATE_INT_TYPE(uint8_t);
+INSTANTIATE_INT_TYPE(int16_t);
+INSTANTIATE_INT_TYPE(uint16_t);
+INSTANTIATE_INT_TYPE(int32_t);
+INSTANTIATE_INT_TYPE(uint32_t);
+INSTANTIATE_INT_TYPE(int64_t);
+INSTANTIATE_INT_TYPE(uint64_t);
+
+#undef INSTANTIATE_INT_TYPE
+
 bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strValue)
 {
     LOCK(cs_args);

src/common/args.h

@@ -11,6 +11,7 @@
 #include <util/chaintype.h>
 #include <util/fs.h>
 
+#include <concepts>
 #include <cstdint>
 #include <iosfwd>
 #include <list>
@@ -89,8 +90,11 @@ struct SectionInfo {
 std::string SettingToString(const common::SettingsValue&, const std::string&);
 std::optional<std::string> SettingToString(const common::SettingsValue&);
 
-int64_t SettingToInt(const common::SettingsValue&, int64_t);
+template <std::integral Int>
-std::optional<int64_t> SettingToInt(const common::SettingsValue&);
+Int SettingTo(const common::SettingsValue&, Int);
+
+template <std::integral Int>
+std::optional<Int> SettingTo(const common::SettingsValue&);
 
 bool SettingToBool(const common::SettingsValue&, bool);
 std::optional<bool> SettingToBool(const common::SettingsValue&);
@@ -293,8 +297,14 @@ protected:
      * @param nDefault (e.g. 1)
      * @return command-line argument (0 if invalid number) or default value
      */
-    int64_t GetIntArg(const std::string& strArg, int64_t nDefault) const;
+    template <std::integral Int>
-    std::optional<int64_t> GetIntArg(const std::string& strArg) const;
+    Int GetArg(const std::string& strArg, Int nDefault) const;
+
+    template <std::integral Int>
+    std::optional<Int> GetArg(const std::string& strArg) const;
+
+    int64_t GetIntArg(const std::string& strArg, int64_t nDefault) const { return GetArg<int64_t>(strArg, nDefault); }
+    std::optional<int64_t> GetIntArg(const std::string& strArg) const { return GetArg<int64_t>(strArg); }
 
     /**
      * Return boolean argument or default value

src/httpserver.cpp

@@ -410,8 +410,8 @@ bool InitHTTPServer(const util::SignalInterrupt& interrupt)
     }
 
     LogDebug(BCLog::HTTP, "Initialized HTTP server\n");
-    g_max_queue_depth = std::max((long)gArgs.GetIntArg("-rpcworkqueue", DEFAULT_HTTP_WORKQUEUE), 1L);
+    g_max_queue_depth = std::max(gArgs.GetArg("-rpcworkqueue", DEFAULT_HTTP_WORKQUEUE), 1);
-    LogDebug(BCLog::HTTP, "set work queue of depth %d\n", g_max_queue_depth);
+    LogDebug(BCLog::HTTP, "set work queue of depth %d", g_max_queue_depth);
 
     // transfer ownership to eventBase/HTTP via .release()
     eventBase = base_ctr.release();
@@ -431,8 +431,8 @@ static std::thread g_thread_http;
 
 void StartHTTPServer()
 {
-    int rpcThreads = std::max((long)gArgs.GetIntArg("-rpcthreads", DEFAULT_HTTP_THREADS), 1L);
+    int rpcThreads = std::max(gArgs.GetArg("-rpcthreads", DEFAULT_HTTP_THREADS), 1);
-    LogInfo("Starting HTTP server with %d worker threads\n", rpcThreads);
+    LogInfo("Starting HTTP server with %d worker threads", rpcThreads);
     g_threadpool_http.Start(rpcThreads);
     g_thread_http = std::thread(ThreadHTTP, eventBase);
 }

src/qt/optionsmodel.cpp

@@ -88,14 +88,14 @@ static common::SettingsValue PruneSetting(bool prune_enabled, int prune_size_gb)
 static bool PruneEnabled(const common::SettingsValue& prune_setting)
 {
     // -prune=1 setting is manual pruning mode, so disabled for purposes of the gui
-    return SettingToInt(prune_setting, 0) > 1;
+    return SettingTo<int64_t>(prune_setting, 0) > 1;
 }
 
 //! Get pruning size value to show in GUI from bitcoin -prune setting. If
 //! pruning is not enabled, just show default recommended pruning size (2GB).
 static int PruneSizeGB(const common::SettingsValue& prune_setting)
 {
-    int value = SettingToInt(prune_setting, 0);
+    int value = SettingTo<int64_t>(prune_setting, 0);
     return value > 1 ? PruneMiBtoGB(value) : DEFAULT_PRUNE_TARGET_GB;
 }
 
@@ -469,9 +469,9 @@ QVariant OptionsModel::getOption(OptionID option, const std::string& suffix) con
                suffix.empty()          ? getOption(option, "-prev") :
                                          DEFAULT_PRUNE_TARGET_GB;
     case DatabaseCache:
-        return qlonglong(SettingToInt(setting(), DEFAULT_DB_CACHE >> 20));
+        return qlonglong(SettingTo<int64_t>(setting(), DEFAULT_DB_CACHE >> 20));
     case ThreadsScriptVerif:
-        return qlonglong(SettingToInt(setting(), DEFAULT_SCRIPTCHECK_THREADS));
+        return qlonglong(SettingTo<int64_t>(setting(), DEFAULT_SCRIPTCHECK_THREADS));
     case Listen:
         return SettingToBool(setting(), DEFAULT_LISTEN);
     case Server:

src/test/getarg_tests.cpp

@@ -247,26 +247,41 @@ BOOST_AUTO_TEST_CASE(intarg)
     const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY);
     const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY);
     SetupArgs(local_args, {foo, bar});
+
     ResetArgs(local_args, "");
+    BOOST_CHECK(!local_args.GetArg<int64_t>("-foo").has_value());
+    BOOST_CHECK(!local_args.GetArg<uint8_t>("-bar").has_value());
     BOOST_CHECK_EQUAL(local_args.GetIntArg("-foo", 11), 11);
     BOOST_CHECK_EQUAL(local_args.GetIntArg("-foo", 0), 0);
+    BOOST_CHECK_EQUAL(local_args.GetArg("-bar", uint8_t{222}), 222);
+    BOOST_CHECK_EQUAL(local_args.GetArg("-bar", uint8_t{0}), 0);
 
     ResetArgs(local_args, "-foo -bar");
+    BOOST_CHECK_EQUAL(local_args.GetArg<int64_t>("-foo"), 0);
+    BOOST_CHECK_EQUAL(local_args.GetArg<uint8_t>("-bar"), 0);
     BOOST_CHECK_EQUAL(local_args.GetIntArg("-foo", 11), 0);
-    BOOST_CHECK_EQUAL(local_args.GetIntArg("-bar", 11), 0);
+    BOOST_CHECK_EQUAL(local_args.GetArg("-bar", uint8_t{222}), 0);
 
     // Check under-/overflow behavior.
     ResetArgs(local_args, "-foo=-9223372036854775809 -bar=9223372036854775808");
+    BOOST_CHECK_EQUAL(local_args.GetArg<int64_t>("-foo"), std::numeric_limits<int64_t>::min());
+    BOOST_CHECK_EQUAL(local_args.GetArg<uint8_t>("-bar"), std::numeric_limits<uint8_t>::max());
     BOOST_CHECK_EQUAL(local_args.GetIntArg("-foo", 0), std::numeric_limits<int64_t>::min());
     BOOST_CHECK_EQUAL(local_args.GetIntArg("-bar", 0), std::numeric_limits<int64_t>::max());
+    BOOST_CHECK_EQUAL(local_args.GetArg("-foo", uint8_t{0}), std::numeric_limits<uint8_t>::min());
+    BOOST_CHECK_EQUAL(local_args.GetArg("-bar", uint8_t{0}), std::numeric_limits<uint8_t>::max());
 
     ResetArgs(local_args, "-foo=11 -bar=12");
+    BOOST_CHECK_EQUAL(local_args.GetArg<int64_t>("-foo"), 11);
+    BOOST_CHECK_EQUAL(local_args.GetArg<uint8_t>("-bar"), 12);
     BOOST_CHECK_EQUAL(local_args.GetIntArg("-foo", 0), 11);
-    BOOST_CHECK_EQUAL(local_args.GetIntArg("-bar", 11), 12);
+    BOOST_CHECK_EQUAL(local_args.GetArg("-bar", uint8_t{11}), 12);
 
     ResetArgs(local_args, "-foo=NaN -bar=NotANumber");
+    BOOST_CHECK_EQUAL(local_args.GetArg<int64_t>("-foo"), 0);
+    BOOST_CHECK_EQUAL(local_args.GetArg<uint8_t>("-bar"), 0);
     BOOST_CHECK_EQUAL(local_args.GetIntArg("-foo", 1), 0);
-    BOOST_CHECK_EQUAL(local_args.GetIntArg("-bar", 11), 0);
+    BOOST_CHECK_EQUAL(local_args.GetArg("-bar", uint8_t{11}), 0);
 }
 
 BOOST_AUTO_TEST_CASE(patharg)