From 20a9986751785d3047cfd87fa82c4b05a2e82b41 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 25 Sep 2024 14:42:12 -0400 Subject: [PATCH 01/14] 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 02/14] 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 03/14] 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 04/14] 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 05/14] 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 06/14] 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 Date: Mon, 19 Aug 2024 15:42:00 -0400 Subject: [PATCH 07/14] common: Add support for combining ArgsManager flags Let ALLOW_STRING and ALLOW_INT flags be combined with ALLOW_BOOL so string and int options can be specified without explicit values. This is useful for imperative settings that trigger new behavior when specified and can accept optional string or integer values, but do not require them. (For examples, see the example_options unit test modified in this commit.) --- src/common/args.cpp | 4 -- src/common/args.h | 40 ++++++++----- src/test/argsman_tests.cpp | 113 ++++++++++++++++++++++++++++++++++++- src/test/fuzz/system.cpp | 1 - 4 files changed, 138 insertions(+), 20 deletions(-) diff --git a/src/common/args.cpp b/src/common/args.cpp index 3fb4100154b..00b556bca92 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -689,10 +689,6 @@ void ArgsManager::AddArg(const std::string& name, const std::string& help, unsig 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 42fb8ec6b68..689621c5762 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -109,6 +109,11 @@ public: * - 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. * + * - If your setting causes a new action to be performed, and does not + * require a value, add | ALLOW_BOOL to the flags. Adding it just allows + * the setting to be specified alone on the command line without a value, + * as "-foo" instead of "-foo=value". + * * - 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. @@ -118,16 +123,16 @@ public: * 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 | + * | Syntax | STRING | INT | BOOL | STRING\|BOOL | INT\|BOOL | + * | -------- | :----: | :-: | :--: | :----------: | :-------: | + * | -foo=abc | X | | | X | | + * | -foo=123 | X | X | | X | X | + * | -foo=0 | X | X | X | X | X | + * | -foo=1 | X | X | X | X | X | + * | -foo | | | X | X | X | + * | -foo= | X | X | X | X | X | + * | -nofoo | X | X | X | X | X | + * | -nofoo=1 | X | X | X | X | X | * * Once validated, settings can be retrieved by called GetSetting(), * GetArg(), GetIntArg(), and GetBoolArg(). GetSetting() is the most general @@ -257,9 +262,18 @@ protected: * {"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. + * when they would be unambiguous. This makes it possible to distinguish + * values by type by checking for narrow types first. For example, to + * handle boolean settings.json or command line values (-foo -nofoo) + * differently than string values (-foo=abc), you can write: + * + * if (auto foo{args.GetBoolArg("-foo")}) { + * // handle -foo or -nofoo bool in foo.value() + * } else if (auto foo{args.GetArg("-foo")}) { + * // handle -foo=abc string in foo.value() + * } else { + * // handle unset setting + * } * * More examples of GetArg function usage can be found in the * @ref example_options::ReadOptions() function in diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp index a65ba1c41c3..965be3834fe 100644 --- a/src/test/argsman_tests.cpp +++ b/src/test/argsman_tests.cpp @@ -69,6 +69,15 @@ struct Options { //! Example of a simple string option that canoot be negated ChainType chain{ChainType::MAIN}; + //! Whether rescan wallets starting from an optional height. + //! Example of an imperative integer setting that uses ALLOW_BOOL flag. + std::optional wallet_rescan; + + //! Whether to bind to an IPC socket. False to not bind, true to bind to the + //! default path, and string to bind to the specified path. + //! Example of an imperative string setting that uses ALLOW_BOOL flag. + std::variant ipc_bind{false}; + //! Paths of block files to load before starting. //! Example of a simple string list setting. std::vector load_block; @@ -89,6 +98,8 @@ void RegisterArgs(ArgsManager& args) args.AddArg("-assumevalid", "", ArgsManager::ALLOW_STRING, {}); args.AddArg("-logfile", "", ArgsManager::ALLOW_STRING, {}); args.AddArg("-chain", "", ArgsManager::ALLOW_STRING | ArgsManager::DISALLOW_NEGATION, {}); + args.AddArg("-rescan", "", ArgsManager::ALLOW_INT | ArgsManager::ALLOW_BOOL, {}); + args.AddArg("-ipcbind", "", ArgsManager::ALLOW_STRING | ArgsManager::ALLOW_BOOL, {}); args.AddArg("-loadblock", "", ArgsManager::ALLOW_STRING | ArgsManager::ALLOW_LIST, {}); args.AddArg("-listen", "", ArgsManager::ALLOW_STRING | ArgsManager::ALLOW_LIST, {}); } @@ -131,6 +142,25 @@ void ReadOptions(const ArgsManager& args, Options& options) } } + if (auto value{args.GetBoolArg("-rescan")}) { + // If -rescan was passed with no height, enable wallet rescan with + // default options. If -norescan was passed, do nothing. + if (*value) options.wallet_rescan.emplace(); + } else if (auto value = args.GetIntArg("-rescan")) { + // If -rescan= command was passed enable wallet rescan from the + // specified height. + options.wallet_rescan = RescanOptions{.start_height=*value}; + } + + if (auto value{args.GetBoolArg("-ipcbind")}) { + // If -ipcbind was passed with no address, set ipc_bind = true, or if + // -noipcbind was passed, set ipc_bind = false. + options.ipc_bind = *value; + } else if (auto value = args.GetArg("-ipcbind")) { + // If -ipcbind=
was passed, set ipc_bind =
+ options.ipc_bind = *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)); @@ -155,6 +185,19 @@ void ReadOptions(const ArgsManager& args, Options& options) } } +//! Return Options::ipc_bind as a human readable string for easier testing. +std::string IpcBindStr(const Options& options) +{ + if (auto address{std::get_if(&options.ipc_bind)}) { + return *address; + } else if (auto enabled{std::get_if(&options.ipc_bind)}; enabled && *enabled) { + // Default address in this example when -ipcbind is specified without an + // address is "node.sock". + return "node.sock"; + } + return ""; +} + //! Return Options::load_block as a human readable string for easier testing. std::string LoadBlockStr(const Options& options) { @@ -308,6 +351,34 @@ BOOST_FIXTURE_TEST_CASE(ExampleOptions, example_options::TestSetup) 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 rescan value is unset. + BOOST_CHECK(!ParseOptions({}).wallet_rescan); + // Check passing -rescan makes it rescan from default height + BOOST_CHECK_EQUAL(ParseOptions({"-rescan"}).wallet_rescan.value().start_height, std::nullopt); + // Check passing -rescan=500000 makes it scan from specified height. + BOOST_CHECK_EQUAL(ParseOptions({"-rescan=500000"}).wallet_rescan.value().start_height, 500000); + // Check passing -norescan makes it not rescan. + BOOST_CHECK(!ParseOptions({"-norescan"}).wallet_rescan); + // Check passing -rescan=0 does not simply set the bool but treats it as a height. + BOOST_CHECK_EQUAL(ParseOptions({"-rescan=0"}).wallet_rescan.value().start_height, 0); + // Check passing -rescan=1 does not simply set the bool but treats it as a height. + BOOST_CHECK_EQUAL(ParseOptions({"-rescan=1"}).wallet_rescan.value().start_height, 1); + // Check adding -rescan= sets it back to default. + BOOST_CHECK(!ParseOptions({"-rescan=500000", "-rescan="}).wallet_rescan); + // Check passing invalid value. + BOOST_CHECK_EXCEPTION(ParseOptions({"-rescan=yes"}), std::exception, HasReason{"Cannot set -rescan value to 'yes'. It must be set to an integer."}); + + // Check default ipcbind value is disabled. + BOOST_CHECK_EQUAL(IpcBindStr(ParseOptions({})), ""); + // Check passing -ipcbind enables it at default address. + BOOST_CHECK_EQUAL(IpcBindStr(ParseOptions({"-ipcbind"})), "node.sock"); + // Check passing -noipcbind makes it disabled. + BOOST_CHECK_EQUAL(IpcBindStr(ParseOptions({"-noipcbind"})), ""); + // Check passing -ipcbind=address sets an address. + BOOST_CHECK_EQUAL(IpcBindStr(ParseOptions({"-ipcbind=address"})), "address"); + // Check adding -ipcbind= sets it back to default. + BOOST_CHECK_EQUAL(IpcBindStr(ParseOptions({"-ipcbind=address", "-ipcbind="})), ""); + // Check default loadblock value is empty. BOOST_CHECK_EQUAL(LoadBlockStr(ParseOptions({})), ""); // Check passing -loadblock can set multiple values. @@ -551,6 +622,34 @@ BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest) CheckValue(M::ALLOW_STRING, "-value=2", Expect{"2"}.String("2")); CheckValue(M::ALLOW_STRING, "-value=abc", Expect{"abc"}.String("abc")); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, nullptr, Expect{{}}.DefaultInt().DefaultBool()); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue", Expect{false}.Int(0).Bool(false)); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('').")); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=0", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('0').")); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=1", Expect{false}.Int(0).Bool(false)); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=2", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('2').")); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-novalue=abc", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('abc').")); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value", Expect{true}.Int(1).Bool(true)); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=", Expect{""}.DefaultInt().DefaultBool()); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=0", Expect{0}.Int(0).DefaultBool()); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=1", Expect{1}.Int(1).DefaultBool()); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=2", Expect{2}.Int(2).DefaultBool()); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=abc", Expect{{}}.Error("Cannot set -value value to 'abc'. It must be set to an integer.")); + + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, nullptr, Expect{{}}.DefaultString().DefaultBool()); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue", Expect{false}.String("").Bool(false)); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('').")); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=0", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('0').")); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=1", Expect{false}.String("").Bool(false)); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=2", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('2').")); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-novalue=abc", Expect{{}}.Error("Cannot negate -value at the same time as setting a value ('abc').")); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value", Expect{true}.DefaultString().Bool(true)); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=", Expect{""}.DefaultString().DefaultBool()); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=0", Expect{"0"}.String("0").DefaultBool()); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=1", Expect{"1"}.String("1").DefaultBool()); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=2", Expect{"2"}.String("2").DefaultBool()); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=abc", Expect{"abc"}.String("abc").DefaultBool()); + 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 ('').")); @@ -579,6 +678,18 @@ BOOST_FIXTURE_TEST_CASE(util_CheckBoolStringsNotSpecial, CheckValueTest) 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.")); + + // Similarly, check "true" and "false" are not treated specially when + // ALLOW_BOOL is combined with ALLOW_INT and ALLOW_STRING. (The only + // difference ALLOW_BOOL makes for int and string arguments is that it + // enables "-foo" syntax with no equal sign assigning explicit int or string + // values. This is useful for arguments like "-upgradewallet" or "-listen" + // that primarily toggle features on and off, but also accept optional int + // or string values to influence behavior.) + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=true", Expect{{}}.Error("Cannot set -value value to 'true'. It must be set to an integer.")); + CheckValue(M::ALLOW_INT | M::ALLOW_BOOL, "-value=false", Expect{{}}.Error("Cannot set -value value to 'false'. It must be set to an integer.")); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=true", Expect{"true"}.String("true").DefaultBool()); + CheckValue(M::ALLOW_STRING | M::ALLOW_BOOL, "-value=false", Expect{"false"}.String("false").DefaultBool()); } BOOST_AUTO_TEST_CASE(util_CheckSingleValue) @@ -598,8 +709,6 @@ BOOST_AUTO_TEST_CASE(util_CheckBadFlagCombinations) 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 be343270c8a..31fd3875429 100644 --- a/src/test/fuzz/system.cpp +++ b/src/test/fuzz/system.cpp @@ -82,7 +82,6 @@ FUZZ_TARGET(system, .init = initialize_system) 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 62c6f53618f135011f54a22f83776e4a37dba31d Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 15 Nov 2019 11:12:18 -0500 Subject: [PATCH 08/14] scripted-diff: Add ALLOW_LIST flag to arguments retrieved with GetArgs This change has no effect on behavior, and is basically just a documentation change at this point. The ALLOW_LIST flag is currently ignored unless ALLOW_BOOL, ALLOW_INT, or ALLOW_STRING flags are also present, and these flags are not used yet. -BEGIN VERIFY SCRIPT- for f in `git grep -n 'GetArgs(' | grep -v _tests | sed -n 's/.*GetArgs("\([^"]\+\)".*/\1/p' | sort -u`; do git grep -l -- "$f" | xargs sed -i "/AddArg(\"$f[=\"]/ s/ArgsManager::ALLOW_ANY/& | ArgsManager::ALLOW_LIST/g" done -END VERIFY SCRIPT- --- src/bitcoin-wallet.cpp | 4 ++-- src/chainparamsbase.cpp | 8 ++++---- src/init.cpp | 32 ++++++++++++++++---------------- src/init/common.cpp | 2 +- src/test/logging_tests.cpp | 6 +++--- src/wallet/init.cpp | 2 +- 6 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index c37f9d29958..51eefafc39e 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -35,9 +35,9 @@ static void SetupWalletToolArgs(ArgsManager& argsman) argsman.AddArg("-version", "Print version and exit", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-datadir=", "Specify data directory", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); - argsman.AddArg("-wallet=", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS); + argsman.AddArg("-wallet=", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS); argsman.AddArg("-dumpfile=", "When used with 'dump', writes out the records to this file. When used with 'createfromdump', loads the records into a new wallet.", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS); - argsman.AddArg("-debug=", "Output debugging information (default: 0).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-debug=", "Output debugging information (default: 0).", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST, OptionsCategory::DEBUG_TEST); argsman.AddArg("-descriptors", "Create descriptors wallet. Only for 'create'", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-legacy", "Create legacy wallet. Only for 'create'", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-format=", "The format of the wallet file to create. Either \"bdb\" or \"sqlite\". Only used with 'createfromdump'", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); diff --git a/src/chainparamsbase.cpp b/src/chainparamsbase.cpp index 060d519d920..54d03e3017e 100644 --- a/src/chainparamsbase.cpp +++ b/src/chainparamsbase.cpp @@ -16,13 +16,13 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman) argsman.AddArg("-chain=", "Use the chain (default: main). Allowed values: " LIST_CHAIN_NAMES, ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-regtest", "Enter regression test mode, which uses a special chain in which blocks can be solved instantly. " "This is intended for regression testing tools and app development. Equivalent to -chain=regtest.", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS); - argsman.AddArg("-testactivationheight=name@height.", "Set the activation height of 'name' (segwit, bip34, dersig, cltv, csv). (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-testactivationheight=name@height.", "Set the activation height of 'name' (segwit, bip34, dersig, cltv, csv). (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-testnet", "Use the testnet3 chain. Equivalent to -chain=test. Support for testnet3 is deprecated and will be removed in an upcoming release. Consider moving to testnet4 now by using -testnet4.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-testnet4", "Use the testnet4 chain. Equivalent to -chain=testnet4.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); - argsman.AddArg("-vbparams=deployment:start:end[:min_activation_height]", "Use given start/end times and min_activation_height for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS); + argsman.AddArg("-vbparams=deployment:start:end[:min_activation_height]", "Use given start/end times and min_activation_height for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS); argsman.AddArg("-signet", "Use the signet chain. Equivalent to -chain=signet. Note that the network is defined by the -signetchallenge parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS); - argsman.AddArg("-signetchallenge", "Blocks must satisfy the given script to be considered valid (only for signet networks; defaults to the global default signet test network challenge)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS); - argsman.AddArg("-signetseednode", "Specify a seed node for the signet network, in the hostname[:port] format, e.g. sig.net:1234 (may be used multiple times to specify multiple seed nodes; defaults to the global default signet test network seed node(s))", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS); + argsman.AddArg("-signetchallenge", "Blocks must satisfy the given script to be considered valid (only for signet networks; defaults to the global default signet test network challenge)", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS); + argsman.AddArg("-signetseednode", "Specify a seed node for the signet network, in the hostname[:port] format, e.g. sig.net:1234 (may be used multiple times to specify multiple seed nodes; defaults to the global default signet test network seed node(s))", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS); } static std::unique_ptr globalChainBaseParams; diff --git a/src/init.cpp b/src/init.cpp index 7fdbf75dc66..ea863b8c92f 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -491,7 +491,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-dbcache=", strprintf("Maximum database cache size MiB (minimum %d, default: %d). Make sure you have enough RAM. In addition, unused memory allocated to the mempool is shared with this cache (see -maxmempool).", MIN_DB_CACHE >> 20, DEFAULT_DB_CACHE >> 20), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-includeconf=", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-allowignoredconf", strprintf("For backwards compatibility, treat an unused %s file in the datadir as a warning, not an error.", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-loadblock=", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-loadblock=", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST, OptionsCategory::OPTIONS); argsman.AddArg("-maxmempool=", strprintf("Keep the transaction memory pool below megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE_MB), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxorphantx=", strprintf("Keep at most unconnectable transactions in memory (default: %u)", DEFAULT_MAX_ORPHAN_TRANSACTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-mempoolexpiry=", strprintf("Do not keep transactions in the mempool longer than hours (default: %u)", DEFAULT_MEMPOOL_EXPIRY_HOURS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); @@ -513,7 +513,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-settings=", strprintf("Specify path to dynamic settings data file. Can be disabled with -nosettings. File is written at runtime and not meant to be edited by users (use %s instead for custom settings). Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME, BITCOIN_SETTINGS_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); #if HAVE_SYSTEM argsman.AddArg("-startupnotify=", "Execute command on startup.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-shutdownnotify=", "Execute command immediately before beginning shutdown. The need for shutdown may be urgent, so be careful not to delay it long (if the command doesn't require interaction with the server, consider having it fork into the background).", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-shutdownnotify=", "Execute command immediately before beginning shutdown. The need for shutdown may be urgent, so be careful not to delay it long (if the command doesn't require interaction with the server, consider having it fork into the background).", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST, OptionsCategory::OPTIONS); #endif argsman.AddArg("-txindex", strprintf("Maintain a full transaction index, used by the getrawtransaction rpc call (default: %u)", DEFAULT_TXINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-blockfilterindex=", @@ -521,16 +521,16 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) " If is not supplied or if = 1, indexes for all known types are enabled.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-addnode=", strprintf("Add a node to connect to and attempt to keep the connection open (see the addnode RPC help for more info). This option can be specified multiple times to add multiple nodes; connections are limited to %u at a time and are counted separately from the -maxconnections limit.", MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); + argsman.AddArg("-addnode=", strprintf("Add a node to connect to and attempt to keep the connection open (see the addnode RPC help for more info). This option can be specified multiple times to add multiple nodes; connections are limited to %u at a time and are counted separately from the -maxconnections limit.", MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); argsman.AddArg("-asmap=", strprintf("Specify asn mapping used for bucketing of the peers (default: %s). Relative paths will be prefixed by the net-specific datadir location.", DEFAULT_ASMAP_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-bantime=", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - argsman.AddArg("-bind=[:][=onion]", strprintf("Bind to given address and always listen on it (default: 0.0.0.0). Use [host]:port notation for IPv6. Append =onion to tag any incoming connections to that address and port as incoming Tor connections (default: 127.0.0.1:%u=onion, testnet3: 127.0.0.1:%u=onion, testnet4: 127.0.0.1:%u=onion, signet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultChainParams->GetDefaultPort() + 1, testnetChainParams->GetDefaultPort() + 1, testnet4ChainParams->GetDefaultPort() + 1, signetChainParams->GetDefaultPort() + 1, regtestChainParams->GetDefaultPort() + 1), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); + argsman.AddArg("-bind=[:][=onion]", strprintf("Bind to given address and always listen on it (default: 0.0.0.0). Use [host]:port notation for IPv6. Append =onion to tag any incoming connections to that address and port as incoming Tor connections (default: 127.0.0.1:%u=onion, testnet3: 127.0.0.1:%u=onion, testnet4: 127.0.0.1:%u=onion, signet: 127.0.0.1:%u=onion, regtest: 127.0.0.1:%u=onion)", defaultChainParams->GetDefaultPort() + 1, testnetChainParams->GetDefaultPort() + 1, testnet4ChainParams->GetDefaultPort() + 1, signetChainParams->GetDefaultPort() + 1, regtestChainParams->GetDefaultPort() + 1), ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); argsman.AddArg("-cjdnsreachable", "If set, then this host is configured for CJDNS (connecting to fc00::/8 addresses would lead us to the CJDNS network, see doc/cjdns.md) (default: 0)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - argsman.AddArg("-connect=", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); + argsman.AddArg("-connect=", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); argsman.AddArg("-discover", "Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-dns", strprintf("Allow DNS lookups for -addnode, -seednode and -connect (default: %u)", DEFAULT_NAME_LOOKUP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-dnsseed", strprintf("Query for peer addresses via DNS lookup, if low on addresses (default: %u unless -connect used or -maxconnections=0)", DEFAULT_DNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - argsman.AddArg("-externalip=", "Specify your own public address", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + argsman.AddArg("-externalip=", "Specify your own public address", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST, OptionsCategory::CONNECTION); argsman.AddArg("-fixedseeds", strprintf("Allow fixed seeds if DNS seeds don't provide peers (default: %u)", DEFAULT_FIXEDSEEDS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-forcednsseed", strprintf("Always query for peer addresses via DNS lookup (default: %u)", DEFAULT_FORCEDNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-listen", strprintf("Accept connections from outside (default: %u if no -proxy, -connect or -maxconnections=0)", DEFAULT_LISTEN), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); @@ -546,7 +546,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) #endif argsman.AddArg("-i2psam=", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-i2pacceptincoming", strprintf("Whether to accept inbound I2P connections (default: %i). Ignored if -i2psam is not set. Listening for inbound I2P connections is done through the SAM proxy, not by binding to a local address and port.", DEFAULT_I2P_ACCEPT_INCOMING), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - argsman.AddArg("-onlynet=", "Make automatic outbound connections only to network (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + argsman.AddArg("-onlynet=", "Make automatic outbound connections only to network (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST, OptionsCategory::CONNECTION); argsman.AddArg("-v2transport", strprintf("Support v2 transport (default: %u)", DEFAULT_V2_TRANSPORT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); @@ -558,7 +558,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-proxy=", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_ELISION, OptionsCategory::CONNECTION); #endif argsman.AddArg("-proxyrandomize", strprintf("Randomize credentials for every proxy connection. This enables Tor stream isolation (default: %u)", DEFAULT_PROXYRANDOMIZE), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - argsman.AddArg("-seednode=", "Connect to a node to retrieve peer addresses, and disconnect. This option can be specified multiple times to connect to multiple nodes. During startup, seednodes will be tried before dnsseeds.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + argsman.AddArg("-seednode=", "Connect to a node to retrieve peer addresses, and disconnect. This option can be specified multiple times to connect to multiple nodes. During startup, seednodes will be tried before dnsseeds.", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST, OptionsCategory::CONNECTION); argsman.AddArg("-networkactive", "Enable all P2P network activity (default: 1). Can be changed by the setnetworkactive RPC command", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-timeout=", strprintf("Specify socket connection timeout in milliseconds. If an initial attempt to connect is unsuccessful after this amount of time, drop it (minimum: 1, default: %d)", DEFAULT_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-peertimeout=", strprintf("Specify a p2p connection timeout delay in seconds. After connecting to a peer, wait this amount of time before considering disconnection based on inactivity (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION); @@ -609,14 +609,14 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-checkaddrman=", strprintf("Run addrman consistency checks every operations. Use 0 to disable. (default: %u)", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkmempool=", strprintf("Run mempool consistency checks every transactions. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-deprecatedrpc=", "Allows deprecated RPC method(s) to be used", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-deprecatedrpc=", "Allows deprecated RPC method(s) to be used", ArgsManager::ALLOW_ANY | ArgsManager::ALLOW_LIST | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", DEFAULT_STOPAFTERBLOCKIMPORT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-stopatheight", strprintf("Stop running after reaching the given height in the main chain (default: %u)", DEFAULT_STOPATHEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-limitancestorcount=", strprintf("Do not accept transactions if number of in-mempool ancestors is or more (default: %u)", DEFAULT_ANCESTOR_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-limitancestorsize=", strprintf("Do not accept transactions whose size with all in-mempool ancestors exceeds kilobytes (default: %u)", DEFAULT_ANCESTOR_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-limitdescendantcount=", strprintf("Do not accept transactions if any ancestor would have or more in-mempool descendants (default: %u)", DEFAULT_DESCENDANT_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-limitdescendantsize=", strprintf("Do not accept transactions if any ancestor would have more than kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-test=