Skip to content

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Aug 4, 2019

Trigger startup errors if bitcoin is configured with bad setting values according to flags. Also raise internal errors if settings are registered and retrieved with inconsistent flags.

This change has no effect on behavior because the new ArgsManager flags added here are not used outside of tests yet.

It'll probably be easier to start applying type checking flags to new options than existing options. But for examples of how type checking flags can make sense applied to existing options, see the new example_options unit test added in this PR.


Followups:

  • Support for flag combinations is possible and is added in commit c919f51 (branch)
  • ALLOW_LIST flags are added and enforced in #17580
  • Bad IsArgSet usages with ALLOW_LIST are removed in #30529 and prevented in #17783
  • Confusing and ignored multiple assignments in config files are disallowed in #17493
  • Confusing reverse-precedence settings merging code is removed in #17581
  • Type flags added in this PR implement runtime part of a change to add more compile-time checking in #22978

@ryanofsky
Copy link
Contributor Author

FYI @hebasto, this adds error checking for the flags from #16097

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 4, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/16545.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK laanwj, hodlinator
Approach ACK l0rinc
Approach NACK ajtowns
Stale ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@hebasto
Copy link
Member

hebasto commented Aug 4, 2019

It seems that not providing a new InterpretNegated() function has some benefits:

  • no need for key_name local variables
  • diff gets much smaller

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, read the code and tried locally. Will review tests, but for now just a couple of comments.

Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviews!

re: #16545 (comment)

It seems that not providing a new InterpretNegated() function has some benefits:

  • no need for key_name local variables
  • diff gets much smaller

The function is only 9 lines long, so I don't see it having a very big impact on the size of the diff, but there are a few reasons why I think it's important for this to be separate and not part of FlagsOfKnownArg:

  • FlagsOfKnownArg is now called at argument retrieval time, not jut argument parsing time, so it's more efficient and easier to reason about if it is only doing a pure map lookup without string manipulation and parsing.

  • Having this be a separate function makes InterpretOption shorter and more understandable. Instead of parsing option names, parsing option values, and dealing with flags, it now only parses option values and no longer deals with flags or names at all.

  • Getting rid of the InterpretNegated function may reduce the size of this diff, but it would increase the diff in #15934, because it would leave more code for that PR to deduplicate and update. It's nice to take care of some work for #15934 when the changes make sense here as well.

@promag
Copy link
Contributor

promag commented Aug 7, 2019

Side note, I find FlagsOfKnownArg confusing because it also returns for unknown arguments and it doesn't tell if it's known or not. I'd reword to GetArgFlags.

Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Promag's suggestion to work on a commit using the flags
#16545 (comment) was really useful. Turns out after trying this out with wallet flags, the empty string check for TYPE_STRING is a lot less useful than I thought it would be, so I'm going to simplify the flags a little and get rid of it.

It does seem good to just start off with a minimal set of checks,, and then in the future when we think of checks that actually are useful in practice (maybe checks for ip addresses, timestamps, hex strings, value ranges) we can add them at that point.

I'm going to:

  • Make a little followup PR applying new types to some wallet args
  • Tweak flags a little in this PR to be less crazy about empty strings
  • Add example of using flags in the PR description as suggested and link to wallet flag PR
  • Improve comments, make /* Standard value types. */ comment more descriptive and add notes to IsArgSet and IsArgNegated methods explaining how there's no need to call these if the argument has a type declared.

Actual changes to this PR will be very small, so it still should be reviewable now.

Side note, I find FlagsOfKnownArg confusing because it also returns for unknown arguments and it doesn't tell if it's known or not. I'd reword to GetArgFlags.

Both seem good to me, but get GetArgFlags does seem a little more standard, so I'll rename if @hebasto also says it's better or just as good.

@hebasto
Copy link
Member

hebasto commented Aug 10, 2019

Concept ACK 01ca54a

@promag

Side note, I find FlagsOfKnownArg confusing because it also returns for unknown arguments and it doesn't tell if it's known or not. I'd reword to GetArgFlags.

For unknown arguments it returns ArgsManager::NONE.

@ryanofsky

Both seem good to me, but get GetArgFlags does seem a little more standard, so I'll rename if @hebasto also says it's better or just as good.

Naming is the hardest part of coding ;)
Agree with @promag's suggestion about renaming.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 9, 2019
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 9, 2019
Rename FlagsOfKnownArg to GetArgFlags

Discussed: bitcoin#16545 (comment)
Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated c7d018d -> b5e8b2a (pr/argcheck.5 -> pr/argcheck.6, compare) with changes described in #16545 (review).

Agree with @promag's suggestion about renaming.

Now renamed in 0779ce5.

@laanwj
Copy link
Member

laanwj commented Oct 23, 2019

Concept ACK, consistent argument error checking is good.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 14, 2019
Rename suggested by João Barbosa <joao.paulo.barbosa@gmail.com>
bitcoin#16545 (comment)

This also gets rid of ArgsManager::NONE constant, which was an implementation
detail not meant to be used by ArgsManager callers.

Finally this reverts a change from 7f40528
bitcoin#15934 adding "-" characters to argument
names. Better for GetArgFlags to require "-" prefixes for consistency with
other argument methods, and to be more efficient later when GetArg functions
need to call GetArgFlags.

This commit does not change behavior.
Copy link
Contributor

@l0rinc l0rinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C++ developers should be more comfortable with type declarations like std::variant<bool, std::string> than equivalent declarations with flags like ALLOW_BOOL | ALLOW_STRING

std::variant is basically a poor-man's-try-monad, until we can use std::expected in C++23.


I started reviewing the PR, but rereading your comment I'm not sure whether you want to modify this part twice - because I'm all for a more functional approach instead of playing with flags (did a similar refactor in #30906).

So, are you planning on migrating this PR away from flags, or should we review this, knowing that in a future PR you'll likely remove them?

// literal "1" value. Avoid calling InterpretBool and accepting
// other values which could be ambiguous.
if (value && *value != "1") {
error = strprintf("Can not negate -%s at the same time as setting a value ('%s').", key.name, *value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you're using both can not and cannot in the errors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

nit: you're using both can not and cannot in the errors

Seems like cannot is better so switched to that, thanks!

@@ -103,6 +103,38 @@ KeyInfo InterpretKey(std::string key)
*
* @return parsed settings value if it is valid, otherwise nullopt accompanied
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return parsed settings value if it is valid, otherwise nullopt accompanied
* @return parsed settings value if it is valid, otherwise `nullopt` accompanied
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

Thanks, applied suggestion

ryanofsky and others added 6 commits September 25, 2024 14:42
This commit just adds documentation for the type flags. The flags are actually
implemented in the following two commits.
… 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.
…LLOW 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).
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
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>
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 25, 2024
Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 87b6d4c -> 5a94560 (pr/argcheck.41 -> pr/argcheck.42, compare) with suggested formatting & grammar tweaks.

re: #16545 (review)

Thanks for the review. To be clear, I would be very happy to see this PR reviewed and merged as is, even though I am working on other related changes in #22978, and am also happy to proceed with them if this PR is stuck.

std::variant is basically a poor-man's-try-monad, until we can use std::expected in C++23.

It is true that std::variant is a poor substitute for std::expected in cases where std::expected is useful. But std::variant is more general than std::expected and provides a tagged union type / sum type that's useful for other things besides error handling, such as representing settings in different states.

I started reviewing the PR, but rereading your comment I'm not sure whether you want to modify this part twice

Changes here and in the experimental branch in #22978 should be basically orthogonal.

This PR is implementing new validation and parsing behavior so users can have better feedback about invalid settings, and so support for ALLOW_LIST can be added in #17580 and used to remove various confusing merge behaviors (see list of followups in the description.

The experimental branch in #22978 doesn't implement new behavior. It just hides (and will eliminate but doesn't currently eliminate) the ALLOW_BOOL ALLOW_INT ALLOW_STRING ALLOW_LIST flags, replacing them with equivalent C++ type expressions. If this PR were merged before the changes in that branch, behavior would not be changing twice, it would just be moving with superficial changes from the InterpretValue function here to a new ParseSetting function there.

If you are interested in the validation and parsing behavior implemented in this PR, or the followups listed in the description, it's probably worth reviewing this without waiting for the experimental branch to be ready. But if you are less interested in the way settings are validated and parsed, and more interested in being able to represent setting types as C++ type expressions like std::vector<std::string> instead of ALLOW_STRING | ALLOW_LIST, then the branch will be more interesting than this PR. The branch also provides more compile time checking and support for Options structs that should be convenient for developers.

So, are you planning on migrating this PR away from flags, or should we review this, knowing that in a future PR you'll likely remove them?

The branch will replace flags combinations with C++ type expressions as part of a larger migration from AddArgs() calls to Setting<> declarations that I hope to implement as a scripted diff. The parsing and validation implemented here which is controlled by the flags will not change. Replacing flags with C++ type expressions is a pretty superficial change, even though it does enable replacing runtime checks with compile-time ones.

@@ -103,6 +103,38 @@ KeyInfo InterpretKey(std::string key)
*
* @return parsed settings value if it is valid, otherwise nullopt accompanied
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

Thanks, applied suggestion

// literal "1" value. Avoid calling InterpretBool and accepting
// other values which could be ambiguous.
if (value && *value != "1") {
error = strprintf("Can not negate -%s at the same time as setting a value ('%s').", key.name, *value);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

nit: you're using both can not and cannot in the errors

Seems like cannot is better so switched to that, thanks!

Copy link
Contributor

@l0rinc l0rinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK

I did a first scan over the impl, left a few nits, some refactoring ideas, let me know what you think and I'll continue the reviews based on that.

Suggestions
Index: src/test/util/setup_common.h
<+>UTF-8
===================================================================
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
--- a/src/test/util/setup_common.h	(revision 5a945600451037693a032e6df94f99a666dd581f)
+++ b/src/test/util/setup_common.h	(date 1727301051076)
@@ -259,10 +259,6 @@
     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);
Index: src/common/args.cpp
<+>UTF-8
===================================================================
diff --git a/src/common/args.cpp b/src/common/args.cpp
--- a/src/common/args.cpp	(revision 5a945600451037693a032e6df94f99a666dd581f)
+++ b/src/common/args.cpp	(date 1727300363436)
@@ -56,11 +56,16 @@
  * For a more extensive discussion of this topic (and a wide range of opinions
  * on the Right Way to change this code), see PR12713.
  */
-static bool InterpretBool(const std::string& strValue)
+static bool InterpretBool(std::string_view strValue)
 {
-    if (strValue.empty())
-        return true;
-    return (LocaleIndependentAtoi<int>(strValue) != 0);
+    return strValue.empty() || LocaleIndependentAtoi<int>(strValue);
+}
+
+std::optional<int64_t> TryParseInt64(std::string_view str)
+{
+    int64_t result;
+    if (ParseInt64(str, &result)) return result;
+    return std::nullopt;
 }
 
 static std::string SettingName(const std::string& arg)
@@ -93,6 +98,56 @@
     return result;
 }
 
+std::optional<common::SettingsValue> HandleTypedArg(const KeyInfo& key, std::optional<std::string_view> value,
+                                                    unsigned int flags, std::string& error)
+{
+    if (key.negated) {
+        // 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;
+    }
+    if (value) {
+        if (IsStringArg(flags) || value->empty()) return *value;
+        if (IsIntArg(flags)) {
+            if (auto parsed = TryParseInt64(*value)) return *parsed;
+        }
+        if (IsBoolArg(flags)) {
+            if (value == "0") return false;
+            if (value == "1") return true;
+        }
+        error = strprintf("Cannot set -%s value to '%s'.", key.name, *value);
+    } else {
+        if (IsBoolArg(flags)) return true;
+        error = strprintf("Cannot set -%s with no value. Please specify value with -%s=value.", key.name, key.name);
+    }
+
+    auto type_str = IsStringArg(flags) ? "a string" : IsIntArg(flags) ? "an integer" : "0 or 1";
+    error += strprintf(" It must be set to %s.", type_str);
+    return std::nullopt;
+}
+
+std::optional<common::SettingsValue> HandleUntypedArg(const KeyInfo& key, std::optional<std::string_view> value,
+                                                      unsigned int flags, std::string& error)
+{
+    if (key.negated) {
+        // Double negatives like -nofoo=0 are supported (but discouraged)
+        if (value && !InterpretBool(*value)) {
+            LogPrintf("Warning: parsed potentially confusing double-negative -%s=%s\n", key.name, *value);
+            return true;
+        }
+        return false;
+    }
+    if (value) return *value;
+    if (!DisallowElision(flags)) return "";
+    error = strprintf("Cannot set -%s with no value. Please specify value with -%s=value.", key.name, key.name);
+    return std::nullopt;
+}
+
 /**
  * Interpret settings value based on registered flags.
  *
@@ -136,52 +191,18 @@
  * - JSON numbers like `123` are returned for settings like `-setting=123` if
  *   the setting enables integer parsing with the ALLOW_INT flag.
  */
-std::optional<common::SettingsValue> InterpretValue(const KeyInfo& key, const std::string* value,
-                                                  unsigned int flags, std::string& error)
+std::optional<common::SettingsValue> InterpretValue(const KeyInfo& key, std::optional<std::string_view> value,
+                                                    unsigned int flags, std::string& error)
 {
-    // Return negated settings as false values.
-    if (key.negated) {
-        if (flags & ArgsManager::DISALLOW_NEGATION) {
-            error = strprintf("Negating of -%s is meaningless and therefore forbidden", key.name);
-            return std::nullopt;
-        }
-        if (IsTypedArg(flags)) {
-            // If argument is typed, only allow negation with no value or with
-            // literal "1" value. Avoid calling InterpretBool and accepting
-            // other values which could be ambiguous.
-            if (value && *value != "1") {
-                error = strprintf("Cannot negate -%s at the same time as setting a value ('%s').", key.name, *value);
-                return std::nullopt;
-            }
-            return false;
-        }
-        // Double negatives like -nofoo=0 are supported (but discouraged)
-        if (value && !InterpretBool(*value)) {
-            LogPrintf("Warning: parsed potentially confusing double-negative -%s=%s\n", key.name, *value);
-            return true;
-        }
-        return false;
+    // Check for disallowed negation
+    if (key.negated && DisallowNegation(flags)) {
+        error = strprintf("Negating of -%s is meaningless and therefore forbidden", key.name);
+        return std::nullopt;
     }
-    if (value) {
-        int64_t parsed_int;
-        if ((flags & ArgsManager::ALLOW_STRING) || !IsTypedArg(flags) || value->empty()) return *value;
-        if ((flags & ArgsManager::ALLOW_INT) && ParseInt64(*value, &parsed_int)) return parsed_int;
-        if ((flags & ArgsManager::ALLOW_BOOL) && *value == "0") return false;
-        if ((flags & ArgsManager::ALLOW_BOOL) && *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);
-    }
-    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;
+
+    return IsTypedArg(flags)
+           ? HandleTypedArg(key, value, flags, error)
+           : HandleUntypedArg(key, value, flags, error);
 }
 
 //! Return string if setting is a nonempty string or number (-setting=abc,
@@ -284,10 +305,10 @@
 #endif
 
         if (key == "-") break; //bitcoin-tx using stdin
-        std::optional<std::string> val;
+        std::optional<std::string_view> val;
         size_t is_index = key.find('=');
         if (is_index != std::string::npos) {
-            val = key.substr(is_index + 1);
+            val = std::string_view{key}.substr(is_index + 1);
             key.erase(is_index);
         }
 #ifdef WIN32
@@ -330,10 +351,11 @@
             return false;
         }
 
-        std::optional<common::SettingsValue> value = InterpretValue(keyinfo, val ? &*val : nullptr, *flags, error);
-        if (!value) return false;
-
-        m_settings.command_line_options[keyinfo.name].push_back(*value);
+        if (auto value = InterpretValue(keyinfo, val, *flags, error)) {
+            m_settings.command_line_options[keyinfo.name].push_back(*value);
+        } else {
+            return false;
+        }
     }
 
     // we do not allow -includeconf from command line, only -noincludeconf
@@ -674,19 +696,19 @@
     }
 
     // Disallow flag combinations that would result in nonsensical behavior or a bad UX.
-    if ((flags & ALLOW_ANY) && (flags & (ALLOW_BOOL | ALLOW_INT | ALLOW_STRING))) {
+    if (IsAnyArg(flags) && IsTypedArg(flags)) {
         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)) {
+    if (IsBoolArg(flags) && DisallowElision(flags)) {
         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)) {
+    if (IsIntArg(flags) && IsStringArg(flags)) {
         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))) {
+    if (IsBoolArg(flags) && (IsIntArg(flags) || IsStringArg(flags))) {
         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));
     }
Index: src/common/config.cpp
<+>UTF-8
===================================================================
diff --git a/src/common/config.cpp b/src/common/config.cpp
--- a/src/common/config.cpp	(revision 5a945600451037693a032e6df94f99a666dd581f)
+++ b/src/common/config.cpp	(date 1727296525414)
@@ -105,11 +105,11 @@
                 error = strprintf("Multiple values specified for -%s in same section of config file.", key.name);
                 return false;
             }
-            std::optional<common::SettingsValue> value = InterpretValue(key, &option.second, *flags, error);
-            if (!value) {
-                return false;
-            }
-            m_settings.ro_config[key.section][key.name].push_back(*value);
+            if (auto value = InterpretValue(key, option.second, *flags, error)) {
+                m_settings.ro_config[key.section][key.name].push_back(*value);
+            } else {
+                return false;
+            }
         } else {
             if (ignore_invalid_keys) {
                 LogPrintf("Ignoring unknown configuration value %s\n", option.first);
Index: src/test/argsman_tests.cpp
<+>UTF-8
===================================================================
diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
--- a/src/test/argsman_tests.cpp	(revision 5a945600451037693a032e6df94f99a666dd581f)
+++ b/src/test/argsman_tests.cpp	(date 1727301503684)
@@ -232,7 +232,7 @@
     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);
+    BOOST_CHECK(!ParseOptions({}).enable_dns_seed);
     // Check passing -dnsseed makes it true.
     BOOST_CHECK_EQUAL(ParseOptions({"-dnsseed"}).enable_dns_seed, true);
     // Check passing -dnsseed=1 makes it true.
@@ -242,7 +242,7 @@
     // 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);
+    BOOST_CHECK(!ParseOptions({"-dnsseed=1", "-dnsseed="}).enable_dns_seed);
     // 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."});
 
@@ -276,7 +276,7 @@
     // Check default assumevalid value is unset.
     BOOST_CHECK(!ParseOptions({}).assumevalid);
     // Check passing -assumevalid=<hash> makes it set that hash.
-    BOOST_CHECK_EQUAL(ParseOptions({"-assumevalid=0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"}).assumevalid.value().ToString(), "0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff");
+    BOOST_CHECK_EQUAL(ParseOptions({"-assumevalid=0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"}).assumevalid, uint256{"0000111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"});
     // Check passing -noassumevalid makes it not assumevalid.
     BOOST_CHECK(!ParseOptions({"-noassumevalid"}).assumevalid);
     // Check adding -assumevalid= sets it back to default.
Index: src/common/args.h
<+>UTF-8
===================================================================
diff --git a/src/common/args.h b/src/common/args.h
--- a/src/common/args.h	(revision 5a945600451037693a032e6df94f99a666dd581f)
+++ b/src/common/args.h	(date 1727300249024)
@@ -77,8 +77,8 @@
 
 KeyInfo InterpretKey(std::string key);
 
-std::optional<common::SettingsValue> InterpretValue(const KeyInfo& key, const std::string* value,
-                                                         unsigned int flags, std::string& error);
+std::optional<common::SettingsValue> InterpretValue(const KeyInfo& key, std::optional<std::string_view> value,
+                                                    unsigned int flags, std::string& error);
 
 struct SectionInfo {
     std::string m_name;
@@ -561,12 +561,16 @@
         const std::map<std::string, std::vector<common::SettingsValue>>& args) const;
 };
 
-//! Whether the type of the argument has been specified and extra validation
-//! rules should apply.
-inline bool IsTypedArg(uint32_t flags)
-{
-    return flags & (ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT |  ArgsManager::ALLOW_STRING);
-}
+//! Whether the type of the argument has been specified and extra validation rules should apply.
+inline bool DisallowNegation(uint32_t flags) { return flags & ArgsManager::DISALLOW_NEGATION; }
+inline bool DisallowElision(uint32_t flags) { return flags & ArgsManager::DISALLOW_ELISION; }
+
+inline bool IsAnyArg(uint32_t flags) { return flags & ArgsManager::ALLOW_ANY; }
+inline bool IsBoolArg(uint32_t flags) { return flags & ArgsManager::ALLOW_BOOL; }
+inline bool IsIntArg(uint32_t flags) { return flags & ArgsManager::ALLOW_INT; }
+inline bool IsStringArg(uint32_t flags) { return flags & ArgsManager::ALLOW_STRING; }
+inline bool IsListArg(uint32_t flags) { return flags & ArgsManager::ALLOW_LIST; }
+inline bool IsTypedArg(uint32_t flags) { return IsBoolArg(flags) || IsIntArg(flags) || IsStringArg(flags); }
 
 extern ArgsManager gArgs;

@@ -68,6 +76,11 @@ FUZZ_TARGET(system, .init = initialize_system)
}
auto help = fuzzed_data_provider.ConsumeRandomLengthString(16);
auto flags = fuzzed_data_provider.ConsumeIntegral<unsigned int>() & ~ArgsManager::COMMAND;
// Avoid hitting "ALLOW_INT flag is incompatible with ALLOW_STRING", etc exceptions
if (flags & ArgsManager::ALLOW_ANY) flags &= ~(ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT | ArgsManager::ALLOW_STRING);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use the flag helper methods here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

let's use the flag helper methods here as well

Can see response to #16545 (comment), but logic in this fuzzing code is already obscure and I feel like hiding the flags in the if conditions while continuing to use the flags in the body of the if statements would only make it more obscure.

Copy link
Contributor

@l0rinc l0rinc Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, you've already extracted a IsTypedArg(flags) method for ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT | ArgsManager::ALLOW_STRING, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

I mean, you've already extracted a IsTypedArg(flags) method for ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT | ArgsManager::ALLOW_STRING, right?

IsTypedArg is used in higher level code to determine if type checking and coercion should be done. I think that code is easier to understand if the distinction between typed and untyped arguments is explicit and it is not dealing with individual flags.

By contrast, this code is low level fuzz code directly dealing with interactions between the flags, and I think it is clearer if it can reference them directly.

Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 5a94560 -> b5ef854 (pr/argcheck.42 -> pr/argcheck.43, compare) implementing review suggestions

return std::nullopt;
if (value) {
int64_t parsed_int;
if ((flags & ArgsManager::ALLOW_STRING) || !IsTypedArg(flags) || value->empty()) return *value;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

It's interesting to see what that looks like but I see few disadvantages to that approach:

  • Superficially, the resulting code is longer and the diff is bigger. Current PR is trying to minimally change this function.
  • It seems to be duplicating some code like "Cannot set -%s with no value" error, increasing risk that differences between the two copies of code will be introduced accidentally, and introducing unintended differences in behavior for typed and untyped arguments.
  • It seems to make it harder to get a high level sense of how argument parsing is intended to work, for example parsing of negated arguments is split up across three functions instead of being handled one place.
  • It seems to do more string manipulation, breaking up error messages into smaller strings that might be harder to search for and translate (if we want to support that later).
  • It might not work as well with my branch in RFC: ArgsManager type and range checking #22978. In this PR, distinction between typed and legacy arguments is all-or-nothing, while in the other branch I want allow supporting and disabling legacy behavior with granular options like SettingOptions::allow_double_negation to make code clearer and offer more flexibility for migrating away from legacy behaviors.

@@ -68,6 +76,11 @@ FUZZ_TARGET(system, .init = initialize_system)
}
auto help = fuzzed_data_provider.ConsumeRandomLengthString(16);
auto flags = fuzzed_data_provider.ConsumeIntegral<unsigned int>() & ~ArgsManager::COMMAND;
// Avoid hitting "ALLOW_INT flag is incompatible with ALLOW_STRING", etc exceptions
if (flags & ArgsManager::ALLOW_ANY) flags &= ~(ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT | ArgsManager::ALLOW_STRING);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

let's use the flag helper methods here as well

Can see response to #16545 (comment), but logic in this fuzzing code is already obscure and I feel like hiding the flags in the if conditions while continuing to use the flags in the body of the if statements would only make it more obscure.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Oct 4, 2024
achow101 added a commit that referenced this pull request Dec 4, 2024
95a0104 test: Add tests for directories in place of config files (Hodlinator)
e85abe9 args: Catch directories in place of config files (Hodlinator)
e4b6b18 test: Add tests for -noconf (Hodlinator)
483f0da args: Properly support -noconf (Hodlinator)
312ec64 test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning (Hodlinator)
7402658 test: -norpccookiefile (Hodlinator)
39cbd4f args: Support -norpccookiefile for bitcoind and bitcoin-cli (Hodlinator)
e82ad88 logs: Use correct path and more appropriate macros in cookie-related code (Hodlinator)
6e28c76 test: Harden testing of cookie file existence (Hodlinator)
75bacab test: combine_logs.py - Output debug.log paths on error (Hodlinator)
bffd92f args: Support -nopid (Hodlinator)
12f8d84 args: Disallow -nodatadir (Hodlinator)
6ff9662 scripted-diff: Avoid printing version information for -noversion (Hodlinator)
e8a2054 doc args: Document narrow scope of -color (Hodlinator)

Pull request description:

  - Document `-color` as only applying to `-getinfo`, to be less confusing for bitcoin-cli users.
  - No longer print version information when getting passed `-noversion`.
  - Disallow `-nodatadir` as we cannot run without one. It was previously interpreted as a mix of unset and as a relative path of "0".
  - Support `-norpccookiefile`
  - Support `-nopid`
  - Properly support `-noconf` (instead of working by accident). Also detect when directories are specified instead of files.

  Prompted by investigation in #16545 (review).

ACKs for top commit:
  l0rinc:
    utACK 95a0104
  achow101:
    ACK 95a0104
  ryanofsky:
    Code review ACK 95a0104. Looks good! Thanks for all your work on this breaking the changes down and making them simple.

Tree-SHA512: 5174251e6b9196a9c6d135eddcb94130295c551bcfccc78e633d9e118ff91523b1be0d72828fb49603ceae312e6e1f8ee2651c6a2b9e0f195603a73a9a622785
Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the reviews.

Updated b5ef854 -> d28a5c8 (pr/argcheck.43 -> pr/argcheck.44, compare) with some suggested changes.
Updated d28a5c8 -> 2057315 (pr/argcheck.44 -> pr/argcheck.45, compare) to fix typo in fuzz check
Updated 2057315 -> 4090dcf (pr/argcheck.45 -> pr/argcheck.46, compare) to fix typo in fuzz check

While I think this PR is in a good state, I'd encourage review of #31260 ahead of this, because #31260 allows setting types to be specified with C++ types instead of flags, which should semantics implemented here more obvious when this can be rebased on top.

return std::nullopt;
if (value) {
int64_t parsed_int;
if ((flags & ArgsManager::ALLOW_STRING) || !IsTypedArg(flags) || value->empty()) return *value;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

Currently I need to know too much about the possible states on a single level, would prefer excluding some states while I'm reviewing.

I think a good strategy is to review the function twice. Review it one time assuming the argument is typed, and another time assuming the argument is untyped, and make sure behavior makes sense in both cases. This approach should also make it easier to see that there aren't unnecessary inconsistencies between the two cases, since there aren't two functions that need to be compared.

Also, I expect this function to get simpler if #31260 is merged first and this is rebased on top. The int/string/bool parsing logic currently here should move into the SettingTraits classes introduced in that PR.

@@ -580,6 +672,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))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

I wouldn't really consider this encapsulating. It is adding a level of indirection and changing syntax used to access flags without reducing the need for callers to know everything about them. #31260 adds a better way of specifying types without flags so that could be a way forward here.

@@ -68,6 +76,11 @@ FUZZ_TARGET(system, .init = initialize_system)
}
auto help = fuzzed_data_provider.ConsumeRandomLengthString(16);
auto flags = fuzzed_data_provider.ConsumeIntegral<unsigned int>() & ~ArgsManager::COMMAND;
// Avoid hitting "ALLOW_INT flag is incompatible with ALLOW_STRING", etc exceptions
if (flags & ArgsManager::ALLOW_ANY) flags &= ~(ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT | ArgsManager::ALLOW_STRING);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #16545 (comment)

I mean, you've already extracted a IsTypedArg(flags) method for ArgsManager::ALLOW_BOOL | ArgsManager::ALLOW_INT | ArgsManager::ALLOW_STRING, right?

IsTypedArg is used in higher level code to determine if type checking and coercion should be done. I think that code is easier to understand if the distinction between typed and untyped arguments is explicit and it is not dealing with individual flags.

By contrast, this code is low level fuzz code directly dealing with interactions between the flags, and I think it is clearer if it can reference them directly.

@ryanofsky ryanofsky force-pushed the pr/argcheck branch 2 times, most recently from 2057315 to 4090dcf Compare December 9, 2024 21:15
achow101 added a commit that referenced this pull request Feb 14, 2025
…case behavior

a85e8c0 doc: Add some general documentation about negated options (Ryan Ofsky)
490c8fa doc: Add release notes summarizing negated option behavior changes. (Ryan Ofsky)
458ef0a refactor: Avoid using IsArgSet() on -connect list option (Ryan Ofsky)
752ab9c test: Add test to make sure -noconnect disables -dnsseed and -listen by default (Ryan Ofsky)
3c2920e refactor: Avoid using IsArgSet() on -signetseednode and -signetchallenge list options (Ryan Ofsky)
d056689 refactor: Avoid using IsArgSet() on -debug, -loglevel, and -vbparams list options (Ryan Ofsky)
3d1e8ca Normalize inconsistent -noexternalip behavior (Ryan Ofsky)
ecd590d Normalize inconsistent -noonlynet behavior (Ryan Ofsky)
5544a19 Fix nonsensical bitcoin-cli -norpcwallet behavior (Ryan Ofsky)
6e8e7f4 Fix nonsensical -noasmap behavior (Ryan Ofsky)
b6ab350 Fix nonsensical -notest behavior (Ryan Ofsky)
6768389 Fix nonsensical -norpcwhitelist behavior (Ryan Ofsky)
e03409c Fix nonsensical -norpcbind and -norpcallowip behavior (Ryan Ofsky)
40c4899 Fix nonsensical -nobind and -nowhitebind behavior (Ryan Ofsky)
5453e66 Fix nonsensical -noseednode behavior (Ryan Ofsky)

Pull request description:

  The PR changes behavior of negated `-noseednode`, `-nobind`, `-nowhitebind`, `-norpcbind`, `-norpcallowip`, `-norpcwhitelist`, `-notest`, `-noasmap`, `-norpcwallet`, `-noonlynet`, and `-noexternalip` options, so negating these options just clears previously specified values doesn't have other side effects.

  Negating options on the command line can be a useful way of resetting options that may have been set earlier in the command line or config file. But before this change, negating these options wouldn't fully reset them, and would have confusing and undocumented side effects (see commit descriptions for details). Now, negating these options just resets them and behaves the same as not specifying them.

  Motivation for this PR is to fix confusing behaviors and also to remove incorrect usages of the `IsArgSet()` function. Using `IsArgSet()` tends to lead to negated option bugs in general, but it especially causes bugs when used with list settings returned by `GetArgs()`, because when these settings are negated, `IsArgSet()` will return true but `GetArgs()` will return an empty list. This PR eliminates all uses of `IsArgSet()` and `GetArgs()` together, and followup PR #17783 makes it an error to use `IsArgSet()` on list settings, since calling `IsArgSet()` is never actually necessary. Most of the changes here were originally made in #17783 and then moved here to be easier to review and avoid a dependency on #16545.

ACKs for top commit:
  achow101:
    ACK a85e8c0
  danielabrozzoni:
    re-ACK a85e8c0
  hodlinator:
    re-ACK a85e8c0

Tree-SHA512: dd4b19faac923aeaa647b1c241d929609ce8242b43e3b7bc32523cc48ec92a83ac0dc5aee79f1eba8794535e0314b96cb151fd04ac973671a1ebb9b52dd16697
@hodlinator hodlinator mentioned this pull request Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.