common: Implement ArgsManager flags to parse and validate settings on…

… startup This commit implements support for new ALLOW_BOOL, ALLOW_INT, ALLOW_STRING, and ALLOW_LIST flags by validating settings with these flags earlier on startup and providing detailed error messages to users. The new flags implement stricter error checking than ALLOW_ANY. For example, a double negated option like -nosetting=0 is treated like an error instead of true, and an unrecognized bool value like -setting=true is treated like an error instead of false. And if a non-list setting is assigned multiple times in the same section of a configuration file, the later assignments trigger errors instead of being silently ignored. The new flags also provide type information that allows ArgsManager GetSettings() and GetSettingsList() methods to return typed integer and boolean values instead of unparsed strings. The changes in this commit have no effect on current application behavior because the new flags are only used in unit tests. The existing ALLOW_ANY checks in the argsman_tests/CheckValueTest confirm that no behavior is changing for current settings, which use ALLOW_ANY.
bitcoin · ryanofsky · Sep 25, 2024 · Jul 30, 2024 · Aug 4, 2019 · Sep 21, 2022
commit a7a35ed080893498ed6de6174518367e3a93fabe
diff --git a/src/common/args.cpp b/src/common/args.cpp
@@ -103,6 +103,38 @@ KeyInfo InterpretKey(std::string key)
  *
  * @return parsed settings value if it is valid, otherwise `nullopt` accompanied
  * by a descriptive error string
+ *
+ * @note By design, the \ref InterpretValue function does mostly lossless
+ * conversions of command line arguments and configuration file values to JSON
+ * `common::SettingsValue` values, so higher level application code and GetArg
+ * helper methods can unambiguously determine original configuration strings
+ * from the JSON values, and flexibly interpret settings and provide good error
+ * feedback. Specifically:
+ * \n
+ * - JSON `null` value is never returned and is reserved for settings that were
+ *   not configured at all.
+ *
+ * - JSON `false` value is returned for negated settings like `-nosetting` or
+ *   `-nosetting=1`. `false` is also returned for boolean-only settings that
+ *   have the ALLOW_BOOL flag and false values like `setting=0`.
+ *
+ * - JSON `true` value is returned for settings that have the ALLOW_BOOL flag
+ *   and are specified on the command line without a value like `-setting`.
+ *   `true` is also returned for boolean-only settings that have the ALLOW_BOOL
+ *   flag and true values like `setting=1`. `true` is also returned for untyped
+ *   legacy settings (see \ref IsTypedArg) that use double negation like
+ *   `-nosetting=0`.
+ *
+ * - JSON `""` empty string value is returned for settings like `-setting=`
+ *   that specify empty values. `""` is also returned for untyped legacy
+ *   settings (see \ref IsTypedArg) that are specified on the command line
+ *   without a value like `-setting`.
+ *
+ * - JSON strings like `"abc"` are returned for settings like `-setting=abc` if
+ *   the setting has the ALLOW_STRING flag or is an untyped legacy setting.
+ *
+ * - JSON numbers like `123` are returned for settings like `-setting=123` if
+ *   the setting enables integer parsing with the ALLOW_INT flag.
  */
 std::optional<common::SettingsValue> InterpretValue(const KeyInfo& key, const std::string* value,
                                                     unsigned int flags, std::string& error)
@@ -113,18 +145,46 @@ std::optional<common::SettingsValue> InterpretValue(const KeyInfo& key, const st
             error = strprintf("Negating of -%s is meaningless and therefore forbidden", key.name);
             return std::nullopt;
         }
+        if (IsTypedArg(flags)) {
+            // If argument is typed, only allow negation with no value or with
+            // literal "1" value. Avoid calling InterpretBool and accepting
+            // other values which could be ambiguous.
+            if (value && *value != "1") {
+                error = strprintf("Cannot negate -%s at the same time as setting a value ('%s').", key.name, *value);
+                return std::nullopt;
+            }
+            return false;
+        }
         // Double negatives like -nofoo=0 are supported (but discouraged)
         if (value && !InterpretBool(*value)) {
             LogPrintf("Warning: parsed potentially confusing double-negative -%s=%s\n", key.name, *value);
             return true;
         }
         return false;
     }
-    if (!value && (flags & ArgsManager::DISALLOW_ELISION)) {
+    if (value) {
+        if ((flags & ArgsManager::ALLOW_STRING) || !IsTypedArg(flags) || value->empty()) return *value;
+        if (flags & ArgsManager::ALLOW_INT) {
+            if (auto parsed_int = ToIntegral<int64_t>(*value)) return *parsed_int;
+        }
+        if (flags & ArgsManager::ALLOW_BOOL) {
+            if (*value == "0") return false;
+            if (*value == "1") return true;
+        }
+        error = strprintf("Cannot set -%s value to '%s'.", key.name, *value);
+    } else {
+        if (flags & ArgsManager::ALLOW_BOOL) return true;
+        if (!(flags & ArgsManager::DISALLOW_ELISION) && !IsTypedArg(flags)) return "";
         error = strprintf("Cannot set -%s with no value. Please specify value with -%s=value.", key.name, key.name);
-        return std::nullopt;
     }
-    return value ? *value : "";
+    if (flags & ArgsManager::ALLOW_STRING) {
+        error = strprintf("%s %s", error, "It must be set to a string.");
+    } else if (flags & ArgsManager::ALLOW_INT) {
+        error = strprintf("%s %s", error, "It must be set to an integer.");
+    } else if (flags & ArgsManager::ALLOW_BOOL) {
+        error = strprintf("%s %s", error, "It must be set to 0 or 1.");
+    }
+    return std::nullopt;
 }
 
 // Define default constructor and destructor that are not inline, so code instantiating this class doesn't need to
@@ -537,10 +597,10 @@ bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strVa
 
 bool ArgsManager::SoftSetBoolArg(const std::string& strArg, bool fValue)
 {
-    if (fValue)
-        return SoftSetArg(strArg, std::string("1"));
-    else
-        return SoftSetArg(strArg, std::string("0"));
+    LOCK(cs_args);
+    if (IsArgSet(strArg)) return false;
+    m_settings.forced_settings[SettingName(strArg)] = fValue;
+    return true;
 }
 
 void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strValue)
@@ -580,6 +640,24 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, unsig
     if (flags & ArgsManager::NETWORK_ONLY) {
         m_network_only_args.emplace(arg_name);
     }
+
+    // Disallow flag combinations that would result in nonsensical behavior or a bad UX.
+    if ((flags & ALLOW_ANY) && (flags & (ALLOW_BOOL | ALLOW_INT | ALLOW_STRING))) {
+        throw std::logic_error(strprintf("Bug: bad %s flags. ALLOW_{BOOL|INT|STRING} flags are incompatible with "
+                                         "ALLOW_ANY (typed arguments need to be type checked)", arg_name));
+    }
+    if ((flags & ALLOW_BOOL) && (flags & DISALLOW_ELISION)) {
+        throw std::logic_error(strprintf("Bug: bad %s flags. ALLOW_BOOL flag is incompatible with DISALLOW_ELISION "
+                                         "(boolean arguments should not require argument values)", arg_name));
+    }
+    if ((flags & ALLOW_INT) && (flags & ALLOW_STRING)) {
+        throw std::logic_error(strprintf("Bug: bad %s flags. ALLOW_INT flag is incompatible with ALLOW_STRING "
+                                         "(any valid integer is also a valid string)", arg_name));
+    }
+    if ((flags & ALLOW_BOOL) && (flags & (ALLOW_INT | ALLOW_STRING))) {
+        throw std::logic_error(strprintf("Bug: bad %s flags. ALLOW_BOOL flag may not currently be specified with ALLOW_INT or ALLOW_STRING "
+                                         "(integer and string argument values cannot currently be omitted)", arg_name));
+    }
 }
 
 void ArgsManager::AddHiddenArgs(const std::vector<std::string>& names)

diff --git a/src/common/args.h b/src/common/args.h
@@ -564,6 +564,13 @@ class ArgsManager
         const std::map<std::string, std::vector<common::SettingsValue>>& args) const;
 };
 
+//! Whether the type of the argument has been specified and extra validation
+//! rules should apply.
+inline bool IsTypedArg(uint32_t flags)
+{
+    return flags & (ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT |  ArgsManager::ALLOW_STRING);
+}
+
 extern ArgsManager gArgs;
 
 /**

diff --git a/src/common/config.cpp b/src/common/config.cpp
@@ -101,6 +101,10 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file
         std::optional<unsigned int> flags = GetArgFlags('-' + key.name);
         if (!IsConfSupported(key, error)) return false;
         if (flags) {
+            if (IsTypedArg(*flags) && !(*flags & ALLOW_LIST) && m_settings.ro_config[key.section].count(key.name)) {
+                error = strprintf("Multiple values specified for -%s in same section of config file.", key.name);
+                return false;
+            }
             std::optional<common::SettingsValue> value = InterpretValue(key, &option.second, *flags, error);
             if (!value) {
                 return false;

diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
@@ -127,7 +127,7 @@ class CheckValueTest : public TestChain100Setup
 
         if (expect.error) {
             BOOST_CHECK(!success);
-            BOOST_CHECK_NE(error.find(expect.error), std::string::npos);
+            BOOST_CHECK_EQUAL(error, expect.error);
         } else {
             BOOST_CHECK(success);
             BOOST_CHECK_EQUAL(error, "");
@@ -137,16 +137,12 @@ class CheckValueTest : public TestChain100Setup
             BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), "zzzzz");
         } else if (expect.string_value) {
             BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), expect.string_value);
-        } else {
-            BOOST_CHECK(!success);
         }
 
         if (expect.default_int) {
             BOOST_CHECK_EQUAL(test.GetIntArg("-value", 99999), 99999);
         } else if (expect.int_value) {
             BOOST_CHECK_EQUAL(test.GetIntArg("-value", 99999), *expect.int_value);
-        } else {
-            BOOST_CHECK(!success);
         }
 
         if (expect.default_bool) {
@@ -155,15 +151,11 @@ class CheckValueTest : public TestChain100Setup
         } else if (expect.bool_value) {
             BOOST_CHECK_EQUAL(test.GetBoolArg("-value", false), *expect.bool_value);
             BOOST_CHECK_EQUAL(test.GetBoolArg("-value", true), *expect.bool_value);
-        } else {
-            BOOST_CHECK(!success);
         }
 
         if (expect.list_value) {
             auto l = test.GetArgs("-value");
             BOOST_CHECK_EQUAL_COLLECTIONS(l.begin(), l.end(), expect.list_value->begin(), expect.list_value->end());
-        } else {
-            BOOST_CHECK(!success);
         }
     }
 };
@@ -185,6 +177,98 @@ BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest)
     CheckValue(M::ALLOW_ANY, "-value=1", Expect{"1"}.String("1").Int(1).Bool(true).List({"1"}));
     CheckValue(M::ALLOW_ANY, "-value=2", Expect{"2"}.String("2").Int(2).Bool(true).List({"2"}));
     CheckValue(M::ALLOW_ANY, "-value=abc", Expect{"abc"}.String("abc").Int(0).Bool(false).List({"abc"}));
+
+    CheckValue(M::ALLOW_BOOL, nullptr, Expect{{}});
+    CheckValue(M::ALLOW_BOOL, "-novalue", Expect{false});
+    CheckValue(M::ALLOW_BOOL, "-novalue=", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('')."));
+    CheckValue(M::ALLOW_BOOL, "-novalue=0", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('0')."));
+    CheckValue(M::ALLOW_BOOL, "-novalue=1", Expect{false});
+    CheckValue(M::ALLOW_BOOL, "-novalue=2", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('2')."));
+    CheckValue(M::ALLOW_BOOL, "-novalue=abc", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('abc')."));
+    CheckValue(M::ALLOW_BOOL, "-value", Expect{true});
+    CheckValue(M::ALLOW_BOOL, "-value=", Expect{""});
+    CheckValue(M::ALLOW_BOOL, "-value=0", Expect{false});
+    CheckValue(M::ALLOW_BOOL, "-value=1", Expect{true});
+    CheckValue(M::ALLOW_BOOL, "-value=2", Expect{{}}.Error("Cannot set -value value to '2'. It must be set to 0 or 1."));
+    CheckValue(M::ALLOW_BOOL, "-value=abc", Expect{{}}.Error("Cannot set -value value to 'abc'. It must be set to 0 or 1."));
+
+    CheckValue(M::ALLOW_INT, nullptr, Expect{{}});
+    CheckValue(M::ALLOW_INT, "-novalue", Expect{false});
+    CheckValue(M::ALLOW_INT, "-novalue=", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('')."));
+    CheckValue(M::ALLOW_INT, "-novalue=0", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('0')."));
+    CheckValue(M::ALLOW_INT, "-novalue=1", Expect{false});
+    CheckValue(M::ALLOW_INT, "-novalue=2", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('2')."));
+    CheckValue(M::ALLOW_INT, "-novalue=abc", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('abc')."));
+    CheckValue(M::ALLOW_INT, "-value", Expect{{}}.Error("Cannot set -value with no value. Please specify value with -value=value. It must be set to an integer."));
+    CheckValue(M::ALLOW_INT, "-value=", Expect{""});
+    CheckValue(M::ALLOW_INT, "-value=0", Expect{0});
+    CheckValue(M::ALLOW_INT, "-value=1", Expect{1});
+    CheckValue(M::ALLOW_INT, "-value=2", Expect{2});
+    CheckValue(M::ALLOW_INT, "-value=abc", Expect{{}}.Error("Cannot set -value value to 'abc'. It must be set to an integer."));
+
+    CheckValue(M::ALLOW_STRING, nullptr, Expect{{}});
+    CheckValue(M::ALLOW_STRING, "-novalue", Expect{false});
+    CheckValue(M::ALLOW_STRING, "-novalue=", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('')."));
+    CheckValue(M::ALLOW_STRING, "-novalue=0", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('0')."));
+    CheckValue(M::ALLOW_STRING, "-novalue=1", Expect{false});
+    CheckValue(M::ALLOW_STRING, "-novalue=2", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('2')."));
+    CheckValue(M::ALLOW_STRING, "-novalue=abc", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('abc')."));
+    CheckValue(M::ALLOW_STRING, "-value", Expect{{}}.Error("Cannot set -value with no value. Please specify value with -value=value. It must be set to a string."));
+    CheckValue(M::ALLOW_STRING, "-value=", Expect{""});
+    CheckValue(M::ALLOW_STRING, "-value=0", Expect{"0"});
+    CheckValue(M::ALLOW_STRING, "-value=1", Expect{"1"});
+    CheckValue(M::ALLOW_STRING, "-value=2", Expect{"2"});
+    CheckValue(M::ALLOW_STRING, "-value=abc", Expect{"abc"});
+
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, nullptr, Expect{{}});
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue", Expect{false});
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('')."));
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=0", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('0')."));
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=1", Expect{false});
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=2", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('2')."));
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue=abc", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('abc')."));
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value", Expect{{}}.Error("Cannot set -value with no value. Please specify value with -value=value. It must be set to a string."));
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=", Expect{""});
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=0", Expect{"0"});
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=1", Expect{"1"});
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=2", Expect{"2"});
+    CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=abc", Expect{"abc"});
+}
+
+BOOST_FIXTURE_TEST_CASE(util_CheckBoolStringsNotSpecial, CheckValueTest)
+{
+    // Check that "true" and "false" strings are rejected for ALLOW_BOOL
+    // arguments. We might want to change this behavior in the future and
+    // interpret strings like "true" as true, and strings like "false" as false.
+    // But because it would be confusing to interpret "true" as true for
+    // ALLOW_BOOL arguments but false for ALLOW_ANY arguments (because
+    // atoi("true")==0), for now it is safer to just disallow strings like
+    // "true" and "false" for ALLOW_BOOL arguments as long as there are still
+    // other boolean arguments interpreted with ALLOW_ANY.
+    using M = ArgsManager;
+    CheckValue(M::ALLOW_BOOL, "-value=true", Expect{{}}.Error("Cannot set -value value to 'true'. It must be set to 0 or 1."));
+    CheckValue(M::ALLOW_BOOL, "-value=false", Expect{{}}.Error("Cannot set -value value to 'false'. It must be set to 0 or 1."));
+}
+
+BOOST_AUTO_TEST_CASE(util_CheckSingleValue)
+{
+    TestArgsManager test;
+    test.SetupArgs({{"-single", ArgsManager::ALLOW_INT}});
+    std::istringstream stream("single=1\nsingle=2\n");
+    std::string error;
+    BOOST_CHECK(!test.ReadConfigStream(stream, "file.conf", error));
+    BOOST_CHECK_EQUAL(error, "Multiple values specified for -single in same section of config file.");
+}
+
+BOOST_AUTO_TEST_CASE(util_CheckBadFlagCombinations)
+{
+    TestArgsManager test;
+    using M = ArgsManager;
+    BOOST_CHECK_THROW(test.AddArg("-arg1", "name", M::ALLOW_BOOL | M::ALLOW_ANY, OptionsCategory::OPTIONS), std::logic_error);
+    BOOST_CHECK_THROW(test.AddArg("-arg2", "name", M::ALLOW_BOOL | M::DISALLOW_ELISION, OptionsCategory::OPTIONS), std::logic_error);
+    BOOST_CHECK_THROW(test.AddArg("-arg3", "name", M::ALLOW_INT | M::ALLOW_STRING, OptionsCategory::OPTIONS), std::logic_error);
+    BOOST_CHECK_THROW(test.AddArg("-arg4", "name", M::ALLOW_INT | M::ALLOW_BOOL, OptionsCategory::OPTIONS), std::logic_error);
+    BOOST_CHECK_THROW(test.AddArg("-arg5", "name", M::ALLOW_STRING | M::ALLOW_BOOL, OptionsCategory::OPTIONS), std::logic_error);
 }
 
 struct NoIncludeConfTest {

diff --git a/src/test/fuzz/system.cpp b/src/test/fuzz/system.cpp
@@ -68,6 +68,11 @@ FUZZ_TARGET(system, .init = initialize_system)
                 }
                 auto help = fuzzed_data_provider.ConsumeRandomLengthString(16);
                 auto flags = fuzzed_data_provider.ConsumeIntegral<unsigned int>() & ~ArgsManager::COMMAND;
+                // Avoid hitting "ALLOW_INT flag is incompatible with ALLOW_STRING", etc exceptions
+                if (flags & ArgsManager::ALLOW_ANY) flags &= ~(ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT | ArgsManager::ALLOW_STRING);
+                if (flags & ArgsManager::ALLOW_BOOL) flags &= ~ArgsManager::DISALLOW_ELISION;
+                if (flags & ArgsManager::ALLOW_STRING) flags &= ~ArgsManager::ALLOW_INT;
+                if (flags & ArgsManager::ALLOW_BOOL) flags &= ~(ArgsManager::ALLOW_INT | ArgsManager::ALLOW_STRING);
                 args_manager.AddArg(argument_name, help, flags, options_category);
             },
             [&] {