From 20a9986751785d3047cfd87fa82c4b05a2e82b41 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 25 Sep 2024 14:42:12 -0400 Subject: [PATCH 1/6] common: Grammar / formatting tweaks Implement cleanup suggestions from l0rinc: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773221823 https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1773291395 https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775897382 --- src/common/args.cpp | 6 +++--- test/functional/feature_config_args.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index f59d2b8f0fc..43bf7175a20 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -101,11 +101,11 @@ KeyInfo InterpretKey(std::string key) * @param[in] flags ArgsManager registered argument flags * @param[out] error Error description if settings value is not valid * - * @return parsed settings value if it is valid, otherwise nullopt accompanied + * @return parsed settings value if it is valid, otherwise `nullopt` accompanied * by a descriptive error string */ std::optional InterpretValue(const KeyInfo& key, const std::string* value, - unsigned int flags, std::string& error) + unsigned int flags, std::string& error) { // Return negated settings as false values. if (key.negated) { @@ -121,7 +121,7 @@ std::optional InterpretValue(const KeyInfo& key, const st return false; } if (!value && (flags & ArgsManager::DISALLOW_ELISION)) { - error = strprintf("Can not set -%s with no value. Please specify value with -%s=value.", key.name, key.name); + error = strprintf("Cannot set -%s with no value. Please specify value with -%s=value.", key.name, key.name); return std::nullopt; } return value ? *value : ""; diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index 44c7edf9626..74e685eae0e 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -150,7 +150,7 @@ class ConfArgsTest(BitcoinTestFramework): def test_invalid_command_line_options(self): self.nodes[0].assert_start_raises_init_error( - expected_msg='Error: Error parsing command line arguments: Can not set -proxy with no value. Please specify value with -proxy=value.', + expected_msg='Error: Error parsing command line arguments: Cannot set -proxy with no value. Please specify value with -proxy=value.', extra_args=['-proxy'], ) # Provide a value different from 1 to the -wallet negated option From 30385f2976e70f31f45a0e4cd2e43e3171d2998a Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 30 Jul 2024 12:57:30 -0400 Subject: [PATCH 2/6] doc: Add detailed ArgsManager type flag documention This commit just adds documentation for the type flags. The flags are actually implemented in the following two commits. --- src/common/args.h | 150 ++++++++++++++++++++++++++++++++++++----- test/lint/check-doc.py | 5 +- 2 files changed, 137 insertions(+), 18 deletions(-) diff --git a/src/common/args.h b/src/common/args.h index 8d9daf5f65d..1b1b9f79ee8 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -99,15 +99,87 @@ class ArgsManager { public: /** - * Flags controlling how config and command line arguments are validated and - * interpreted. + * Flags controlling how config and command line arguments are parsed. + * + * The flags below provide very basic type checking, designed to catch + * obvious configuration mistakes and provide helpful error messages. + * Specifying these flags is not a substitute for actually validating + * setting values that are parsed and making sure they are legitimate. + * + * Summary of recommended flags: + * + * - For most settings, just use standalone ALLOW_BOOL, ALLOW_INT, or + * ALLOW_STRING flags. + * + * - If your setting accepts multiple values and you want to read all the + * values, not just the last value, add | ALLOW_LIST to the flags. + * + * - Only use the DISALLOW_NEGATION flag if your setting really cannot + * function without a value, so the command line interface will generally + * support negation and be more consistent. + * + * Detailed description of flags: + * + * The ALLOW_STRING, ALLOW_INT, and ALLOW_BOOL flags control what syntaxes are + * accepted, according to the following chart: + * + * | Syntax | STRING | INT | BOOL | + * | -------- | :----: | :-: | :--: | + * | -foo=abc | X | | | + * | -foo=123 | X | X | | + * | -foo=0 | X | X | X | + * | -foo=1 | X | X | X | + * | -foo | | | X | + * | -foo= | X | X | X | + * | -nofoo | X | X | X | + * | -nofoo=1 | X | X | X | + * + * Once validated, settings can be retrieved by called GetSetting(), + * GetArg(), GetIntArg(), and GetBoolArg(). GetSetting() is the most general + * way to access settings, returning them as JSON values. The other + * functions just wrap GetSetting() for convenience. + * + * As can be seen in the chart, the default behavior of the flags is not + * very restrictive, although it can be restricted further. It tries to + * accommodate parsing command lines and configuration files written by + * human beings, not just machines, understanding that users may have + * different configuration styles and debugging needs. So the flags do not + * mandate one way to configure things or try to prevent every possible + * error, but instead catch the most common and blatant errors, and allow + * application code to impose additional restrictions, since application + * code needs to parse settings and reject invalid values anyway. + * + * Specifically, by default: + * + * - All settings can be specified multiple times, not just ALLOW_LIST + * settings. This allows users to override the config file from the + * command line, and override earlier command line settings with later + * ones. Application code can disable this behavior by calling the + * GetArgs() function and raising an error if more than one value is + * returned. + * + * - All settings can be negated. This provides a consistent command line + * interface where settings support -nofoo syntax when meaningful. + * GetSetting() returns a false JSON value for negated settings, and + * GetArg(), GetIntArg(), and GetBoolArg() return "", 0, and false + * respectively. Application code can disallow negation by specifying the + * DISALLOW_NEGATION flag, or just handling "", 0, and false values and + * rejecting them if they do not make sense. + * + * - All settings can be empty. Since all settings are optional, it is + * useful to have a way to set them, and a way to unset them. It is also + * unambiguous in most cases to treat empty -foo= syntax as not setting a + * value, so by default this syntax is allowed and causes GetSetting() to + * return JSON "", and GetArg(), GetIntArg() and GetBoolArg() to return + * std::nullopt. Application code can override this behavior by rejecting + * these values. */ enum Flags : uint32_t { - ALLOW_ANY = 0x01, //!< disable validation - // ALLOW_BOOL = 0x02, //!< unimplemented, draft implementation in #16545 - // ALLOW_INT = 0x04, //!< unimplemented, draft implementation in #16545 - // ALLOW_STRING = 0x08, //!< unimplemented, draft implementation in #16545 - // ALLOW_LIST = 0x10, //!< unimplemented, draft implementation in #16545 + ALLOW_ANY = 0x01, //!< allow any argument value (no type checking) + ALLOW_BOOL = 0x02, //!< allow -foo=1, -foo=0, -foo, -nofoo, -nofoo=1, and -foo= + ALLOW_INT = 0x04, //!< allow -foo=123, -nofoo, -nofoo=1, and -foo= + ALLOW_STRING = 0x08, //!< allow -foo=abc, -nofoo, -nofoo=1, and -foo= + ALLOW_LIST = 0x10, //!< allow multiple -foo=bar -foo=baz values DISALLOW_NEGATION = 0x20, //!< disallow -nofoo syntax DISALLOW_ELISION = 0x40, //!< disallow -foo syntax that doesn't assign any value @@ -154,12 +226,64 @@ protected: bool UseDefaultSection(const std::string& arg) const EXCLUSIVE_LOCKS_REQUIRED(cs_args); public: + ArgsManager(); + ~ArgsManager(); + + /** + * @name GetArg Functions + * + * GetArg functions are an easy way to access most settings. They are + * wrappers around the lower-level GetSetting() function that provide + * greater convenience. + * + * Examples: + * + * GetArg("-foo") // returns "abc" if -foo=abc was specified, or nullopt if unset + * GetIntArg("-foo") // returns 123 if -foo=123 was specified, or nullopt if unset + * GetBoolArg("-foo") // returns true if -foo was specified, or nullopt if unset + * GetBoolArg("-foo") // returns false if -nofoo was specified, or nullopt if unset + * + * If no type flags (ALLOW_STRING, ALLOW_INT, or ALLOW_BOOL) are set, GetArg + * functions do many type coercions and can have surprising behaviors which + * legacy code relies on, like parsing -nofoo as string "0" or -foo=true as + * boolean false. + * + * If any type flags are set, then: + * + * - Only GetArg functions with types matching the flags can be called. For + * example, it is an error to call GetIntArg() if ALLOW_INT is not set. + * + * - GetArg functions act like std::get_if(), returning null if the + * requested type is not available or the setting is unspecified or empty. + * + * - "Widening" type conversions from smaller to bigger types are done if + * unambiguous (bool -> int -> string). For example, if settings.json + * contains {"foo":123}, GetArg("-foo") will return "123". If it contains + * {"foo":true}, GetIntArg("-foo") will return 1. + * + * - "Narrowing" type conversions in the other direction are not done even + * when they would be unambiguous. For example, if settings.json contains + * {"foo":"abc"} or {"foo":"123"} GetIntArg("-foo") will return nullopt in + * both cases. + * + * More examples of GetArg function usage can be found in the + * @ref example_options::ReadOptions() function in + * @ref argsman_tests.cpp + *@{*/ + std::optional GetArg(const std::string& strArg) const; + std::optional GetIntArg(const std::string& strArg) const; + std::optional GetBoolArg(const std::string& strArg) const; + /**@}*/ + /** * Get setting value. * - * Result will be null if setting was unset, true if "-setting" argument was passed - * false if "-nosetting" argument was passed, and a string if a "-setting=value" - * argument was passed. + * Result will be null if setting was unspecified, true if `-setting` + * argument was passed, false if `-nosetting` argument was passed, and will + * be a string, integer, or boolean if a `-setting=value` argument was + * passed (which of the three depends on ALLOW_STRING, ALLOW_INT, and + * ALLOW_BOOL flags). See \ref InterpretValue for a full description of how + * command line and configuration strings map to JSON values. */ common::SettingsValue GetSetting(const std::string& arg) const; @@ -168,9 +292,6 @@ protected: */ std::vector GetSettingsList(const std::string& arg) const; - ArgsManager(); - ~ArgsManager(); - /** * Select the network in use */ @@ -271,7 +392,6 @@ protected: * @return command-line argument or default value */ std::string GetArg(const std::string& strArg, const std::string& strDefault) const; - std::optional GetArg(const std::string& strArg) const; /** * Return path argument or default value @@ -293,7 +413,6 @@ protected: * @return command-line argument (0 if invalid number) or default value */ int64_t GetIntArg(const std::string& strArg, int64_t nDefault) const; - std::optional GetIntArg(const std::string& strArg) const; /** * Return boolean argument or default value @@ -303,7 +422,6 @@ protected: * @return command-line argument or default value */ bool GetBoolArg(const std::string& strArg, bool fDefault) const; - std::optional GetBoolArg(const std::string& strArg) const; /** * Set an argument if it doesn't already have a value diff --git a/test/lint/check-doc.py b/test/lint/check-doc.py index f55d0f8cb7b..9a0389b7f71 100755 --- a/test/lint/check-doc.py +++ b/test/lint/check-doc.py @@ -14,11 +14,12 @@ from subprocess import check_output import re FOLDER_GREP = 'src' -FOLDER_TEST = 'src/test/' +EXCLUDE_PATHS = ['src/test/', 'src/common/args.h'] +EXCLUDE_ARGS = ' '.join(f"':(exclude){path}'" for path in EXCLUDE_PATHS) REGEX_ARG = r'\b(?:GetArg|GetArgs|GetBoolArg|GetIntArg|GetPathArg|IsArgSet|get_net)\("(-[^"]+)"' REGEX_DOC = r'AddArg\("(-[^"=]+?)(?:=|")' CMD_ROOT_DIR = '$(git rev-parse --show-toplevel)/{}'.format(FOLDER_GREP) -CMD_GREP_ARGS = r"git grep --perl-regexp '{}' -- {} ':(exclude){}'".format(REGEX_ARG, CMD_ROOT_DIR, FOLDER_TEST) +CMD_GREP_ARGS = f"git grep --perl-regexp '{REGEX_ARG}' -- {CMD_ROOT_DIR} {EXCLUDE_ARGS}" CMD_GREP_WALLET_ARGS = r"git grep --function-context 'void WalletInit::AddWalletOptions' -- {} | grep AddArg".format(CMD_ROOT_DIR) CMD_GREP_WALLET_HIDDEN_ARGS = r"git grep --function-context 'void DummyWalletInit::AddWalletOptions' -- {}".format(CMD_ROOT_DIR) CMD_GREP_DOCS = r"git grep --perl-regexp '{}' {}".format(REGEX_DOC, CMD_ROOT_DIR) From a7a35ed080893498ed6de6174518367e3a93fabe Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Sun, 4 Aug 2019 09:36:04 -0400 Subject: [PATCH 3/6] 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. --- src/common/args.cpp | 92 ++++++++++++++++++++++++++++++--- src/common/args.h | 7 +++ src/common/config.cpp | 4 ++ src/test/argsman_tests.cpp | 102 +++++++++++++++++++++++++++++++++---- src/test/fuzz/system.cpp | 5 ++ 5 files changed, 194 insertions(+), 16 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index 43bf7175a20..4ee9181cda8 100644 --- 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 InterpretValue(const KeyInfo& key, const std::string* value, unsigned int flags, std::string& error) @@ -113,6 +145,16 @@ std::optional 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); @@ -120,11 +162,29 @@ std::optional InterpretValue(const KeyInfo& key, const st } 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(*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& names) diff --git a/src/common/args.h b/src/common/args.h index 1b1b9f79ee8..d2d241fd191 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -564,6 +564,13 @@ private: const std::map>& 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 index 98223fc3e3a..489f8cf0957 100644 --- 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 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 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 index 297595a9cf0..e47f20d6527 100644 --- a/src/test/argsman_tests.cpp +++ b/src/test/argsman_tests.cpp @@ -127,7 +127,7 @@ public: 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 @@ public: 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 @@ public: } 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 index 2ab5b7ed39a..9e3ec8fb5eb 100644 --- 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() & ~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); }, [&] { From a4e8ed267ae5790691eb71a9839ed82f57683b5e Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 21 Sep 2022 15:14:40 -0400 Subject: [PATCH 4/6] common: Update ArgManager GetArg helper methods to work better with ALLOW flags Update GetArg, GetArgs, GetBoolArg, and GetIntArg helper methods to work conveniently with ALLOW_BOOL, ALLOW_INT, and ALLOW_STRING flags. The GetArg methods are convenience wrappers around the GetSetting method. The GetSetting method returns the originally parsed settings values in their declared bool/int/string types, while the GetArg wrappers provide extra type-coercion and default-value fallback features as additional conveniences for callers. This commit makes two changes to GetArg, GetArgs, GetBoolArg, and GetIntArg helper methods when BOOL/INT/STRING flags are used: 1. GetArg methods will now raise errors if they are called with inconsistent flags. For example, GetArgs will raise a logic_error if it is called on a non-LIST setting, GetIntArg will raise a logic_error if it is called on a non-INT setting. 2. GetArg methods will now avoid various type coersion footguns when they are called on new BOOL/INT/STRING settings. Existing ALLOW_ANY settings are unaffected. For example, negated settings will return "" empty strings instead of "0" strings (in the past the "0" strings caused strangeness like "-nowallet" options creating wallet files named "0"). The new behaviors are fully specified and checked by the `CheckValueTest` unit test. The ergonomics of the GetArg helper methods are subjective and the behaviors they implement can be nitpicked and debated endlessly. But behavior of these helper methods does not dictate application behavior, and they can be bypassed by calling GetSetting and GetSettingList methods instead. If it's necessary, behavior of these helper methods can also be changed again in the future. The changes have no effect on current application behavior because the new flags are only used in unit tests. The `setting_args` unit test and ALLOW_ANY checks in the `CheckValueTest` unit test are unchanged and confirm that `GetArg` methods behave the same as before for ALLOW_ANY flags (returning the same values and throwing the same exceptions). --- src/common/args.cpp | 107 ++++++++++++++++++++++++------------- src/common/args.h | 7 +-- src/test/argsman_tests.cpp | 84 ++++++++++++++++++----------- src/test/fuzz/system.cpp | 38 ++++++++++--- 4 files changed, 159 insertions(+), 77 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index 4ee9181cda8..baa659ad473 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -187,6 +187,40 @@ std::optional InterpretValue(const KeyInfo& key, const st return std::nullopt; } +//! Return string if setting is a nonempty string or number (-setting=abc, +//! -setting=123), "" if setting is false (-nosetting), otherwise return +//! nullopt. For legacy untyped args, coerce bool settings to strings as well. +static inline std::optional ConvertToString(const common::SettingsValue& value, bool typed_arg) +{ + if (value.isStr() && !value.get_str().empty()) return value.get_str(); + if (value.isNum()) return value.getValStr(); + if (typed_arg && value.isFalse()) return ""; + if (!typed_arg && !value.isNull()) { + if (value.isBool()) return value.get_bool() ? "1" : "0"; + return value.get_str(); + } + return {}; +} + +//! Return int64 if setting is a number or bool, otherwise return nullopt. For +//! legacy untyped args, coerce string settings as well. +static inline std::optional ConvertToInt(const common::SettingsValue& value, bool typed_arg) +{ + if (value.isNum()) return value.getInt(); + if (value.isBool()) return value.get_bool(); + if (!typed_arg && !value.isNull()) return LocaleIndependentAtoi(value.get_str()); + return {}; +} + +//! Return bool if setting is a bool, otherwise return nullopt. For legacy +//! untyped args, coerce string settings as well. +static inline std::optional ConvertToBool(const common::SettingsValue& value, bool typed_arg) +{ + if (value.isBool()) return value.get_bool(); + if (!typed_arg && !value.isNull()) return InterpretBool(value.get_str()); + return {}; +} + // Define default constructor and destructor that are not inline, so code instantiating this class doesn't need to // #include class definitions for all members. // For example, m_settings has an internal dependency on univalue. @@ -329,6 +363,29 @@ std::optional ArgsManager::GetArgFlags(const std::string& name) co return std::nullopt; } +/** + * Check that arg has the right flags for use in a given context. Raises + * logic_error if this isn't the case, indicating the argument was registered + * with bad AddArg flags. + * + * Returns true if the arg is registered and has type checking enabled. Returns + * false if the arg was never registered or is untyped. + */ +bool ArgsManager::CheckArgFlags(const std::string& name, + uint32_t require, + uint32_t forbid, + const char* context) const +{ + std::optional flags = GetArgFlags(name); + if (!flags || !IsTypedArg(*flags)) return false; + if ((*flags & require) != require || (*flags & forbid) != 0) { + throw std::logic_error( + strprintf("Bug: Can't call %s on arg %s registered with flags 0x%08x (requires 0x%x, disallows 0x%x)", + context, name, *flags, require, forbid)); + } + return true; +} + fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value) const { if (IsArgNegated(arg)) return fs::path{}; @@ -421,9 +478,10 @@ std::optional ArgsManager::GetCommand() const std::vector ArgsManager::GetArgs(const std::string& strArg) const { + bool typed_arg = CheckArgFlags(strArg, /*require=*/ ALLOW_STRING | ALLOW_LIST, /*forbid=*/ 0, __func__); std::vector result; for (const common::SettingsValue& value : GetSettingsList(strArg)) { - result.push_back(value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str()); + result.push_back(ConvertToString(value, typed_arg).value_or("")); } return result; } @@ -521,22 +579,13 @@ std::string ArgsManager::GetArg(const std::string& strArg, const std::string& st std::optional ArgsManager::GetArg(const std::string& strArg) const { - const common::SettingsValue value = GetSetting(strArg); - return SettingToString(value); -} - -std::optional SettingToString(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.getValStr(); - return value.get_str(); + bool typed_arg = CheckArgFlags(strArg, /*require=*/ ALLOW_STRING, /*forbid=*/ ALLOW_LIST, __func__); + return ConvertToString(GetSetting(strArg), typed_arg); } std::string SettingToString(const common::SettingsValue& value, const std::string& strDefault) { - return SettingToString(value).value_or(strDefault); + return ConvertToString(value, /*typed_arg=*/false).value_or(strDefault); } int64_t ArgsManager::GetIntArg(const std::string& strArg, int64_t nDefault) const @@ -546,22 +595,13 @@ int64_t ArgsManager::GetIntArg(const std::string& strArg, int64_t nDefault) cons std::optional ArgsManager::GetIntArg(const std::string& strArg) const { - const common::SettingsValue value = GetSetting(strArg); - return SettingToInt(value); -} - -std::optional SettingToInt(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(); - return LocaleIndependentAtoi(value.get_str()); + bool typed_arg = CheckArgFlags(strArg, /*require=*/ ALLOW_INT, /*forbid=*/ ALLOW_LIST, __func__); + return ConvertToInt(GetSetting(strArg), typed_arg); } int64_t SettingToInt(const common::SettingsValue& value, int64_t nDefault) { - return SettingToInt(value).value_or(nDefault); + return ConvertToInt(value, /*typed_arg=*/false).value_or(nDefault); } bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const @@ -571,20 +611,13 @@ bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const std::optional ArgsManager::GetBoolArg(const std::string& strArg) const { - const common::SettingsValue value = GetSetting(strArg); - return SettingToBool(value); -} - -std::optional SettingToBool(const common::SettingsValue& value) -{ - if (value.isNull()) return std::nullopt; - if (value.isBool()) return value.get_bool(); - return InterpretBool(value.get_str()); + bool typed_arg = CheckArgFlags(strArg, /*require=*/ ALLOW_BOOL, /*forbid=*/ ALLOW_LIST, __func__); + return ConvertToBool(GetSetting(strArg), typed_arg); } bool SettingToBool(const common::SettingsValue& value, bool fDefault) { - return SettingToBool(value).value_or(fDefault); + return ConvertToBool(value, /*typed_arg=*/false).value_or(fDefault); } bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strValue) @@ -598,6 +631,7 @@ bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strVa bool ArgsManager::SoftSetBoolArg(const std::string& strArg, bool fValue) { LOCK(cs_args); + CheckArgFlags(strArg, /*require=*/ ALLOW_BOOL, /*forbid=*/ ALLOW_LIST, __func__); if (IsArgSet(strArg)) return false; m_settings.forced_settings[SettingName(strArg)] = fValue; return true; @@ -606,6 +640,7 @@ bool ArgsManager::SoftSetBoolArg(const std::string& strArg, bool fValue) void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strValue) { LOCK(cs_args); + CheckArgFlags(strArg, /*require=*/ ALLOW_STRING, /*forbid=*/ 0, __func__); m_settings.forced_settings[SettingName(strArg)] = strValue; } @@ -870,7 +905,7 @@ std::variant ArgsManager::GetChainArg() const /* ignore_default_section_config= */ false, /*ignore_nonpersistent=*/false, /* get_chain_type= */ true); - return value.isNull() ? false : value.isBool() ? value.get_bool() : InterpretBool(value.get_str()); + return ConvertToBool(value, /*typed_arg=*/false).value_or(false); }; const bool fRegTest = get_net("-regtest"); diff --git a/src/common/args.h b/src/common/args.h index d2d241fd191..42fb8ec6b68 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -87,13 +87,8 @@ struct SectionInfo { }; std::string SettingToString(const common::SettingsValue&, const std::string&); -std::optional SettingToString(const common::SettingsValue&); - int64_t SettingToInt(const common::SettingsValue&, int64_t); -std::optional SettingToInt(const common::SettingsValue&); - bool SettingToBool(const common::SettingsValue&, bool); -std::optional SettingToBool(const common::SettingsValue&); class ArgsManager { @@ -541,6 +536,8 @@ protected: void LogArgs() const; private: + bool CheckArgFlags(const std::string& name, uint32_t require, uint32_t forbid, const char* context) const; + /** * Get data directory path * diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp index e47f20d6527..9a5feef5737 100644 --- a/src/test/argsman_tests.cpp +++ b/src/test/argsman_tests.cpp @@ -137,12 +137,26 @@ public: BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), "zzzzz"); } else if (expect.string_value) { BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), expect.string_value); + } else if (success) { + // Extra check to ensure complete test coverage. Assert that if + // caller did not call Expect::DefaultString() or Expect::String(), + // then this test case must be one where ParseParameters() fails, or + // one where GetArg() throws logic_error because ALLOW_STRING is not + // specified. + BOOST_CHECK_THROW(test.GetArg("-value", "zzzzz"), std::logic_error); } 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 if (success) { + // Extra check to ensure complete test coverage. Assert that if + // caller did not call Expect::DefaultInt() or Expect::Int(), then + // this test case must be one where ParseParameters() fails, or one + // where GetArg() throws logic_error because ALLOW_INT is not + // specified. + BOOST_CHECK_THROW(test.GetIntArg("-value", 99999), std::logic_error); } if (expect.default_bool) { @@ -151,11 +165,21 @@ public: } 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 if (success) { + // Extra check to ensure complete test coverage. Assert that if + // caller did not call Expect::DefaultBool() or Expect::Bool(), then + // this test case must be one where ParseParameters() fails, or one + // where GetArg() throws logic_error because ALLOW_BOOL is not + // specified. + BOOST_CHECK_THROW(test.GetBoolArg("-value", false), std::logic_error); + BOOST_CHECK_THROW(test.GetBoolArg("-value", true), std::logic_error); } 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 if (success) { + BOOST_CHECK_THROW(test.GetArgs("-value"), std::logic_error); } } }; @@ -178,61 +202,61 @@ BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest) 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, nullptr, Expect{{}}.DefaultBool()); + CheckValue(M::ALLOW_BOOL, "-novalue", Expect{false}.Bool(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=1", Expect{false}.Bool(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", Expect{true}.Bool(true)); + CheckValue(M::ALLOW_BOOL, "-value=", Expect{""}.DefaultBool()); + CheckValue(M::ALLOW_BOOL, "-value=0", Expect{false}.Bool(false)); + CheckValue(M::ALLOW_BOOL, "-value=1", Expect{true}.Bool(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, nullptr, Expect{{}}.DefaultInt()); + CheckValue(M::ALLOW_INT, "-novalue", Expect{false}.Int(0)); 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=1", Expect{false}.Int(0)); 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=", Expect{""}.DefaultInt()); + CheckValue(M::ALLOW_INT, "-value=0", Expect{0}.Int(0)); + CheckValue(M::ALLOW_INT, "-value=1", Expect{1}.Int(1)); + CheckValue(M::ALLOW_INT, "-value=2", Expect{2}.Int(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, nullptr, Expect{{}}.DefaultString()); + CheckValue(M::ALLOW_STRING, "-novalue", Expect{false}.String("")); 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=1", Expect{false}.String("")); 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, "-value=", Expect{""}.DefaultString()); + CheckValue(M::ALLOW_STRING, "-value=0", Expect{"0"}.String("0")); + CheckValue(M::ALLOW_STRING, "-value=1", Expect{"1"}.String("1")); + CheckValue(M::ALLOW_STRING, "-value=2", Expect{"2"}.String("2")); + CheckValue(M::ALLOW_STRING, "-value=abc", Expect{"abc"}.String("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, nullptr, Expect{{}}.List({})); + CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-novalue", Expect{false}.List({})); 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=1", Expect{false}.List({})); 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"}); + CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=", Expect{""}.List({""})); + CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=0", Expect{"0"}.List({"0"})); + CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=1", Expect{"1"}.List({"1"})); + CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=2", Expect{"2"}.List({"2"})); + CheckValue(M::ALLOW_STRING | M::ALLOW_LIST, "-value=abc", Expect{"abc"}.List({"abc"})); } BOOST_FIXTURE_TEST_CASE(util_CheckBoolStringsNotSpecial, CheckValueTest) diff --git a/src/test/fuzz/system.cpp b/src/test/fuzz/system.cpp index 9e3ec8fb5eb..be343270c8a 100644 --- a/src/test/fuzz/system.cpp +++ b/src/test/fuzz/system.cpp @@ -46,7 +46,12 @@ FUZZ_TARGET(system, .init = initialize_system) [&] { auto str_arg = fuzzed_data_provider.ConsumeRandomLengthString(16); auto str_value = fuzzed_data_provider.ConsumeRandomLengthString(16); - args_manager.SoftSetArg(str_arg, str_value); + // Avoid Can't call SoftSetArg on arg registered with flags 0x8d8d8d00 (requires 0x2, disallows 0x10) + try { + args_manager.SoftSetArg(str_arg, str_value); + } catch (const std::logic_error& e) { + if (std::string_view(e.what()).find("Can't call ForceSetArg on arg") == std::string_view::npos) throw; + } }, [&] { auto str_arg = fuzzed_data_provider.ConsumeRandomLengthString(16); @@ -56,7 +61,12 @@ FUZZ_TARGET(system, .init = initialize_system) [&] { auto str_arg = fuzzed_data_provider.ConsumeRandomLengthString(16); auto f_value = fuzzed_data_provider.ConsumeBool(); - args_manager.SoftSetBoolArg(str_arg, f_value); + // Avoid Can't call SoftSetBoolArg on arg registered with flags 0x8d8d8d00 (requires 0x2, disallows 0x10) + try { + args_manager.SoftSetBoolArg(str_arg, f_value); + } catch (const std::logic_error& e) { + if (std::string_view(e.what()).find("Can't call SoftSetBoolArg on arg") == std::string_view::npos) throw; + } }, [&] { const OptionsCategory options_category = fuzzed_data_provider.PickValueInArray({OptionsCategory::OPTIONS, OptionsCategory::CONNECTION, OptionsCategory::WALLET, OptionsCategory::WALLET_DEBUG_TEST, OptionsCategory::ZMQ, OptionsCategory::DEBUG_TEST, OptionsCategory::CHAINPARAMS, OptionsCategory::NODE_RELAY, OptionsCategory::BLOCK_CREATION, OptionsCategory::RPC, OptionsCategory::GUI, OptionsCategory::COMMANDS, OptionsCategory::REGISTER_COMMANDS, OptionsCategory::CLI_COMMANDS, OptionsCategory::IPC, OptionsCategory::HIDDEN}); @@ -115,11 +125,27 @@ FUZZ_TARGET(system, .init = initialize_system) const int64_t i64 = fuzzed_data_provider.ConsumeIntegral(); const bool b = fuzzed_data_provider.ConsumeBool(); - (void)args_manager.GetIntArg(s1, i64); - (void)args_manager.GetArg(s1, s2); + try { + (void)args_manager.GetIntArg(s1, i64); + } catch (const std::logic_error& e) { + if (std::string_view(e.what()).find("Can't call GetIntArg on arg") == std::string_view::npos) throw; + } + try { + (void)args_manager.GetArg(s1, s2); + } catch (const std::logic_error& e) { + if (std::string_view(e.what()).find("Can't call GetArg on arg") == std::string_view::npos) throw; + } (void)args_manager.GetArgFlags(s1); - (void)args_manager.GetArgs(s1); - (void)args_manager.GetBoolArg(s1, b); + try { + (void)args_manager.GetArgs(s1); + } catch (const std::logic_error& e) { + if (std::string_view(e.what()).find("Can't call GetArgs on arg") == std::string_view::npos) throw; + } + try { + (void)args_manager.GetBoolArg(s1, b); + } catch (const std::logic_error& e) { + if (std::string_view(e.what()).find("Can't call GetBoolArg on arg") == std::string_view::npos) throw; + } try { (void)args_manager.GetChainTypeString(); } catch (const std::runtime_error&) { From 59c449e0348014e7ec59bec542189da37f551f05 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 29 Jul 2024 23:26:27 -0400 Subject: [PATCH 5/6] test: Add tests to demonstrate usage of ArgsManager flags Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com> --- src/test/argsman_tests.cpp | 307 +++++++++++++++++++++++++++++++++++ src/test/util/setup_common.h | 4 + 2 files changed, 311 insertions(+) diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp index 9a5feef5737..a65ba1c41c3 100644 --- a/src/test/argsman_tests.cpp +++ b/src/test/argsman_tests.cpp @@ -24,6 +24,313 @@ using util::ToString; BOOST_FIXTURE_TEST_SUITE(argsman_tests, BasicTestingSetup) +//! Example code showing how to declare and parse options using ArgsManager flags. +namespace example_options { +struct Address { + std::string host; + uint16_t port; +}; + +struct RescanOptions { + std::optional start_height; +}; + +struct Options { + //! Whether to use UPnP to map the listening port. + //! Example of a boolean option defaulting to false. + bool enable_upnp{false}; + + //! Whether to listen for RPC commands. + //! Example of a boolean option defaulting to true. + bool enable_rpc_server{true}; + + //! Whether to look for peers with DNS lookup. + //! Example of a boolean option without a default value. (If unspecified, + //! default behavior depends on other options.) + std::optional enable_dns_seed; + + //! Amount of time to ban peers + //! Example of a simple integer setting. + std::chrono::seconds bantime{86400}; + + //! Equivalent bytes per sigop. + //! Example of a where negation should be disallowed. + int bytes_per_sigop{20}; + + //! Hash of block to assume valid and skip script verification. + //! Example of a simple string option. + std::optional assumevalid; + + //! Path to log file + //! Example of a simple string option with a default value. + fs::path log_file{"debug.log"}; + + //! Chain name. + //! Example of a simple string option that canoot be negated + ChainType chain{ChainType::MAIN}; + + //! Paths of block files to load before starting. + //! Example of a simple string list setting. + std::vector load_block; + + //! Addresses to listen on. + //! Example of a list setting where negating the setting is different than + //! not specifying it. + std::optional> listen_addresses; +}; + +void RegisterArgs(ArgsManager& args) +{ + args.AddArg("-upnp", "", ArgsManager::ALLOW_BOOL, {}); + args.AddArg("-rpcserver", "", ArgsManager::ALLOW_BOOL, {}); + args.AddArg("-dnsseed", "", ArgsManager::ALLOW_BOOL, {}); + args.AddArg("-bantime", "", ArgsManager::ALLOW_INT, {}); + args.AddArg("-bytespersigop", "", ArgsManager::ALLOW_INT | ArgsManager::DISALLOW_NEGATION, {}); + args.AddArg("-assumevalid", "", ArgsManager::ALLOW_STRING, {}); + args.AddArg("-logfile", "", ArgsManager::ALLOW_STRING, {}); + args.AddArg("-chain", "", ArgsManager::ALLOW_STRING | ArgsManager::DISALLOW_NEGATION, {}); + args.AddArg("-loadblock", "", ArgsManager::ALLOW_STRING | ArgsManager::ALLOW_LIST, {}); + args.AddArg("-listen", "", ArgsManager::ALLOW_STRING | ArgsManager::ALLOW_LIST, {}); +} + +void ReadOptions(const ArgsManager& args, Options& options) +{ + if (auto value = args.GetBoolArg("-upnp")) options.enable_upnp = *value; + + if (auto value = args.GetBoolArg("-rpcserver")) options.enable_rpc_server = *value; + + if (auto value = args.GetBoolArg("-dnsseed")) options.enable_dns_seed = *value; + + if (auto value = args.GetIntArg("-bantime")) { + if (*value < 0) throw std::runtime_error(strprintf("-bantime value %i is negative", *value)); + options.bantime = std::chrono::seconds{*value}; + } + + if (auto value = args.GetIntArg("-bytespersigop")) { + if (*value < 1) throw std::runtime_error(strprintf("-bytespersigop value %i is less than 1", *value)); + options.bytes_per_sigop = *value; + } + + if (auto value = args.GetArg("-assumevalid"); value && !value->empty()) { + if (auto hash{uint256::FromHex(*value)}) { + options.assumevalid = *hash; + } else { + throw std::runtime_error(strprintf("-assumevalid value '%s' is not a valid hash", *value)); + } + } + + if (auto value = args.GetArg("-logfile")) { + options.log_file = fs::PathFromString(*value); + } + + if (auto value = args.GetArg("-chain")) { + if (auto chain_type{ChainTypeFromString(*value)}) { + options.chain = *chain_type; + } else { + throw std::runtime_error(strprintf("Invalid chain type '%s'", *value)); + } + } + + for (const std::string& value : args.GetArgs("-loadblock")) { + if (value.empty()) throw std::runtime_error(strprintf("-loadblock value '%s' is not a valid file path", value)); + options.load_block.push_back(fs::PathFromString(value)); + } + + if (args.IsArgNegated("-listen")) { + // If -nolisten was passed, disable listening by assigning an empty list + // of listening addresses. + options.listen_addresses.emplace(); + } else if (auto addresses{args.GetArgs("-listen")}; !addresses.empty()) { + // If -listen= options were passed, explicitly add these as + // listening addresses, otherwise leave listening option unset to enable + // default listening behavior. + options.listen_addresses.emplace(); + for (const std::string& value : addresses) { + Address addr{"", 8333}; + if (!SplitHostPort(value, addr.port, addr.host) || addr.host.empty()) { + throw std::runtime_error(strprintf("-listen address '%s' is not a valid host[:port]", value)); + } + options.listen_addresses->emplace_back(std::move(addr)); + } + } +} + +//! Return Options::load_block as a human readable string for easier testing. +std::string LoadBlockStr(const Options& options) +{ + std::string ret; + for (const auto& block : options.load_block) { + if (!ret.empty()) ret += " "; + ret += fs::PathToString(block); + } + return ret; +} + +//! Return Options::listen_addresses as a human readable string for easier +//! testing. +std::string ListenStr(const Options& options) +{ + if (!options.listen_addresses) { + // Default listening behavior in this example is just to listen on port + // 8333. In reality, it could be arbitrarily complicated and depend on + // other settings. + return "0.0.0.0:8333"; + } else { + std::string ret; + for (const auto& addr : *options.listen_addresses) { + if (!ret.empty()) ret += " "; + ret += strprintf("%s:%d", addr.host, addr.port); + } + return ret; + } +} + +struct TestSetup : public BasicTestingSetup +{ + Options ParseOptions(const std::vector& opts) + { + ArgsManager args; + RegisterArgs(args); + std::vector argv{"ignored"}; + for (const auto& opt : opts) { + argv.push_back(opt.c_str()); + } + std::string error; + if (!args.ParseParameters(argv.size(), argv.data(), error)) { + throw std::runtime_error(error); + } + BOOST_CHECK_EQUAL(error, ""); + Options options; + ReadOptions(args, options); + return options; + } +}; +} // namespace example_options + +BOOST_FIXTURE_TEST_CASE(ExampleOptions, example_options::TestSetup) +{ + // Check default upnp value is false + BOOST_CHECK_EQUAL(ParseOptions({}).enable_upnp, false); + // Check passing -upnp makes it true. + BOOST_CHECK_EQUAL(ParseOptions({"-upnp"}).enable_upnp, true); + // Check passing -upnp=1 makes it true. + BOOST_CHECK_EQUAL(ParseOptions({"-upnp=1"}).enable_upnp, true); + // Check adding -upnp= sets it back to default. + BOOST_CHECK_EQUAL(ParseOptions({"-upnp=1", "-upnp="}).enable_upnp, false); + // Check passing invalid value. + BOOST_CHECK_EXCEPTION(ParseOptions({"-upnp=yes"}), std::exception, HasReason{"Cannot set -upnp value to 'yes'. It must be set to 0 or 1."}); + + // Check default rpcserver value is true. + BOOST_CHECK_EQUAL(ParseOptions({}).enable_rpc_server, true); + // Check passing -norpcserver makes it false. + BOOST_CHECK_EQUAL(ParseOptions({"-norpcserver"}).enable_rpc_server, false); + // Check passing -rpcserver=0 makes it false. + BOOST_CHECK_EQUAL(ParseOptions({"-rpcserver=0"}).enable_rpc_server, false); + // Check adding -rpcserver= sets it back to default. + BOOST_CHECK_EQUAL(ParseOptions({"-rpcserver=0", "-rpcserver="}).enable_rpc_server, true); + // Check passing invalid value. + BOOST_CHECK_EXCEPTION(ParseOptions({"-rpcserver=yes"}), std::exception, HasReason{"Cannot set -rpcserver value to 'yes'. It must be set to 0 or 1."}); + + // Check default dnsseed value is unset. + BOOST_CHECK_EQUAL(ParseOptions({}).enable_dns_seed, std::nullopt); + // Check passing -dnsseed makes it true. + BOOST_CHECK_EQUAL(ParseOptions({"-dnsseed"}).enable_dns_seed, true); + // Check passing -dnsseed=1 makes it true. + BOOST_CHECK_EQUAL(ParseOptions({"-dnsseed=1"}).enable_dns_seed, true); + // Check passing -nodnsseed makes it false. + BOOST_CHECK_EQUAL(ParseOptions({"-nodnsseed"}).enable_dns_seed, false); + // Check passing -dnsseed=0 makes it false. + BOOST_CHECK_EQUAL(ParseOptions({"-dnsseed=0"}).enable_dns_seed, false); + // Check adding -dnsseed= sets it back to default. + BOOST_CHECK_EQUAL(ParseOptions({"-dnsseed=1", "-dnsseed="}).enable_dns_seed, std::nullopt); + // Check passing invalid value. + BOOST_CHECK_EXCEPTION(ParseOptions({"-dnsseed=yes"}), std::exception, HasReason{"Cannot set -dnsseed value to 'yes'. It must be set to 0 or 1."}); + + // Check default bantime value is unset. + BOOST_CHECK_EQUAL(ParseOptions({}).bantime.count(), 86400); + // Check passing -bantime=3600 overrides it. + BOOST_CHECK_EQUAL(ParseOptions({"-bantime=3600"}).bantime.count(), 3600); + // Check passing -nobantime makes it 0. + BOOST_CHECK_EQUAL(ParseOptions({"-nobantime"}).bantime.count(), 0); + // Check passing -bantime=0 makes it 0. + BOOST_CHECK_EQUAL(ParseOptions({"-bantime=0"}).bantime.count(), 0); + // Check adding -bantime= sets it back to default. + BOOST_CHECK_EQUAL(ParseOptions({"-bantime=3600", "-bantime="}).bantime.count(), 86400); + // Check passing invalid values. + BOOST_CHECK_EXCEPTION(ParseOptions({"-bantime"}), std::exception, HasReason{"Cannot set -bantime with no value. Please specify value with -bantime=value. It must be set to an integer."}); + BOOST_CHECK_EXCEPTION(ParseOptions({"-bantime=abc"}), std::exception, HasReason{"Cannot set -bantime value to 'abc'. It must be set to an integer."}); + BOOST_CHECK_EXCEPTION(ParseOptions({"-bantime=-1000"}), std::exception, HasReason{"-bantime value -1000 is negative"}); + + // Check default bytespersigop value. + BOOST_CHECK_EQUAL(ParseOptions({}).bytes_per_sigop, 20); + // Check passing -bytespersigop=30 overrides it. + BOOST_CHECK_EQUAL(ParseOptions({"-bytespersigop=30"}).bytes_per_sigop, 30); + // Check adding -bytespersigop= sets it back to default. + BOOST_CHECK_EQUAL(ParseOptions({"-bytespersigop=30", "-bytespersigop="}).bytes_per_sigop, 20); + // Check passing invalid values. + BOOST_CHECK_EXCEPTION(ParseOptions({"-bytespersigop"}), std::exception, HasReason{"Cannot set -bytespersigop with no value. Please specify value with -bytespersigop=value. It must be set to an integer."}); + BOOST_CHECK_EXCEPTION(ParseOptions({"-nobytespersigop"}), std::exception, HasReason{"Negating of -bytespersigop is meaningless and therefore forbidden"}); + BOOST_CHECK_EXCEPTION(ParseOptions({"-bytespersigop=0"}), std::exception, HasReason{"-bytespersigop value 0 is less than 1"}); + BOOST_CHECK_EXCEPTION(ParseOptions({"-bytespersigop=abc"}), std::exception, HasReason{"Cannot set -bytespersigop value to 'abc'. It must be set to an integer."}); + + // Check default assumevalid value is unset. + BOOST_CHECK_EQUAL(ParseOptions({}).assumevalid, std::nullopt); + // Check passing -assumevalid= makes it set that hash. + BOOST_CHECK_EQUAL(ParseOptions({"-assumevalid=0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"}).assumevalid, uint256{"0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"}); + // Check passing -noassumevalid makes it not assumevalid. + BOOST_CHECK_EQUAL(ParseOptions({"-noassumevalid"}).assumevalid, std::nullopt); + // Check adding -assumevalid= sets it back to default. + BOOST_CHECK_EQUAL(ParseOptions({"-assumevalid=0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff", "-assumevalid="}).assumevalid, std::nullopt); + // Check passing invalid values. + BOOST_CHECK_EXCEPTION(ParseOptions({"-assumevalid"}), std::exception, HasReason{"Cannot set -assumevalid with no value. Please specify value with -assumevalid=value. It must be set to a string."}); + BOOST_CHECK_EXCEPTION(ParseOptions({"-assumevalid=1"}), std::exception, HasReason{"-assumevalid value '1' is not a valid hash"}); + + // Check default logfile value. + BOOST_CHECK_EQUAL(ParseOptions({}).log_file, fs::path{"debug.log"}); + // Check passing -logfile=custom.log overrides it. + BOOST_CHECK_EQUAL(ParseOptions({"-logfile=custom.log"}).log_file, fs::path{"custom.log"}); + // Check passing -nologfile makes it disables logging. + BOOST_CHECK_EQUAL(ParseOptions({"-nologfile"}).log_file, fs::path{}); + // Check adding -logfile= sets it back to default. + BOOST_CHECK_EQUAL(ParseOptions({"-logfile=custom.log", "-logfile="}).log_file, fs::path{"debug.log"}); + BOOST_CHECK_EQUAL(ParseOptions({"-nologfile", "-logfile="}).log_file, fs::path{"debug.log"}); + // Check passing invalid values. + BOOST_CHECK_EXCEPTION(ParseOptions({"-logfile"}), std::exception, HasReason{"Cannot set -logfile with no value. Please specify value with -logfile=value. It must be set to a string."}); + + // Check default chain value. + BOOST_CHECK_EQUAL(ParseOptions({}).chain, ChainType::MAIN); + // Check passing -chain=regtest overrides it. + BOOST_CHECK_EQUAL(ParseOptions({"-chain=regtest"}).chain, ChainType::REGTEST); + // Check adding -chain= sets it back to default. + BOOST_CHECK_EQUAL(ParseOptions({"-chain=regtest", "-chain="}).chain, ChainType::MAIN); + // Check passing invalid values. + BOOST_CHECK_EXCEPTION(ParseOptions({"-chain"}), std::exception, HasReason{"Cannot set -chain with no value. Please specify value with -chain=value. It must be set to a string."}); + BOOST_CHECK_EXCEPTION(ParseOptions({"-chain=abc"}), std::exception, HasReason{"Invalid chain type 'abc'"}); + BOOST_CHECK_EXCEPTION(ParseOptions({"-nochain"}), std::exception, HasReason{"Negating of -chain is meaningless and therefore forbidden"}); + + // Check default loadblock value is empty. + BOOST_CHECK_EQUAL(LoadBlockStr(ParseOptions({})), ""); + // Check passing -loadblock can set multiple values. + BOOST_CHECK_EQUAL(LoadBlockStr(ParseOptions({"-loadblock=a", "-loadblock=b"})), "a b"); + // Check passing -noloadblock clears previous values. + BOOST_CHECK_EQUAL(LoadBlockStr(ParseOptions({"-loadblock=a", "-noloadblock", "-loadblock=b", "-loadblock=c"})), "b c"); + // Check passing invalid values. + BOOST_CHECK_EXCEPTION(ParseOptions({"-loadblock"}), std::exception, HasReason{"Cannot set -loadblock with no value. Please specify value with -loadblock=value. It must be set to a string."}); + BOOST_CHECK_EXCEPTION(ParseOptions({"-loadblock="}), std::exception, HasReason{"-loadblock value '' is not a valid file path"}); + + // Check default listen value. + BOOST_CHECK_EQUAL(ListenStr(ParseOptions({})), "0.0.0.0:8333"); + // Check passing -listen can set multiple values. + BOOST_CHECK_EQUAL(ListenStr(ParseOptions({"-listen=a", "-listen=b"})), "a:8333 b:8333"); + // Check passing -nolisten clears previous values. + BOOST_CHECK_EQUAL(ListenStr(ParseOptions({"-listen=a", "-nolisten", "-listen=b", "-listen=c"})), "b:8333 c:8333"); + // Check final -nolisten disables listening. + BOOST_CHECK_EQUAL(ListenStr(ParseOptions({"-listen=a", "-nolisten"})), ""); + // Check passing invalid values. + BOOST_CHECK_EXCEPTION(ParseOptions({"-listen"}), std::exception, HasReason{"Cannot set -listen with no value. Please specify value with -listen=value. It must be set to a string."}); + BOOST_CHECK_EXCEPTION(ParseOptions({"-listen="}), std::exception, HasReason{"-listen address '' is not a valid host[:port]"}); +} + BOOST_AUTO_TEST_CASE(util_datadir) { // Use local args variable instead of m_args to avoid making assumptions about test setup diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 30d4280fa53..ba03800218c 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -259,6 +259,10 @@ inline std::ostream& operator<<(std::ostream& os, const std::optional& v) return v ? os << *v : os << "std::nullopt"; } +inline std::ostream& operator<<(std::ostream& os, const std::nullopt_t) +{ + return os << "std::nullopt"; +} } // namespace std std::ostream& operator<<(std::ostream& os, const arith_uint256& num); From 4090dcf6cfda2e21b7a7323de0e0499092ea900c Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 5 Jun 2024 14:48:51 -0400 Subject: [PATCH 6/6] test: Add test for settings.json parsing with type flags The type flags aren't currently used to validate or convert settings in the settings.json file, but they should be in the future. Add test to check current behavior that can be extended when flags are applied. Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com> --- src/test/getarg_tests.cpp | 64 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 3 deletions(-) diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp index 8734735fd5b..2a08e8ce4ad 100644 --- a/src/test/getarg_tests.cpp +++ b/src/test/getarg_tests.cpp @@ -52,9 +52,11 @@ void SetupArgs(ArgsManager& local_args, const std::vector