Merge #16097: Refactor: Add Flags enum to ArgsManager class

e6f649cb2c07bf55d9214c2876619c56f1d6fe30 test: Make tests arg type specific (Hennadii Stepanov)
b70cc5d73357ea11296f3b6bb81193ba1101e73b Revamp option negating policy (Hennadii Stepanov)
db08edb3038a085d3dbce7bb4ec3c1d9b9a5b281 Replace IsArgKnown() with FlagsOfKnownArg() (Hennadii Stepanov)
dde80c272ae584410532f48d23866d7d8581a1cc Use ArgsManager::NETWORK_ONLY flag (Hennadii Stepanov)
9a12733508e47f558959f1b0ed9937bc3eff8962 Remove unused m_debug_only member from Arg struct (Hennadii Stepanov)
fb4b9f9e3b433d8848832e2c2686cf7b1f212a5e scripted-diff: Use ArgsManager::DEBUG_ONLY flag (Hennadii Stepanov)
1b4b9422cad28d1bead24ff5fd472536954cfaf9 scripted-diff: Use Flags enum in AddArg() (Hennadii Stepanov)
265c1b58d89b7b6fb30468ba402d7f75cc59a510 Add Flags enum to ArgsManager (Hennadii Stepanov)
e0d187dfeb18b026de22bd7960b2a50c2b958e1a Refactor InterpretNegatedOption() function (Hennadii Stepanov)
e0e18a1017fa3dc5d6ebeda6ec35c4263327d17c refactoring: Check IsArgKnown() early (Hennadii Stepanov)

Pull request description:

  This PR adds the `Flags` enum to the `ArgsManager` class. Also the `m_flags` member is added to the `Arg` struct. Flags denote an allowed type of an arg value and special hints.

  This PR is only a refactoring and does not change behavior.

ACKs for top commit:
  jamesob:
    ACK https://github.com/bitcoin/bitcoin/pull/16097/commits/e6f649cb2c07bf55d9214c2876619c56f1d6fe30
  MarcoFalke:
    ACK e6f649cb2c07bf55d9214c2876619c56f1d6fe30 thanks for adding types to the command line options

Tree-SHA512: b867f8a9cbce2d2473c293d534af662d8cd5be15060ff0682e97af678974bdaac35e8bc6328ccba32f105034bcd38f169b92a6fb67798667891ce14d5d2a2dea

2 files changed, 83 insertions, 64 deletions

diff --git a/src/util/system.cpp b/src/util/system.cpp
index c27b0cc105..f8fcbc1206 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -268,22 +268,24 @@ public:
  * This method also tracks when the -no form was supplied, and if so,
  * checks whether there was a double-negative (-nofoo=0 -> -foo=1).
  *
- * If there was not a double negative, it removes the "no" from the key,
- * and returns true, indicating the caller should clear the args vector
- * to indicate a negated option.
+ * If there was not a double negative, it removes the "no" from the key
+ * and clears the args vector to indicate a negated option.
  *
  * If there was a double negative, it removes "no" from the key, sets the
- * value to "1" and returns false.
+ * value to "1" and pushes the key and the updated value to the args vector.
  *
- * If there was no "no", it leaves key and value untouched and returns
- * false.
+ * If there was no "no", it leaves key and value untouched and pushes them
+ * to the args vector.
  *
  * Where an option was negated can be later checked using the
  * IsArgNegated() method. One use case for this is to have a way to disable
  * options that are not normally boolean (e.g. using -nodebuglogfile to request
  * that debug log output is not sent to any file at all).
  */
-static bool InterpretNegatedOption(std::string& key, std::string& val)
+
+NODISCARD static bool InterpretOption(std::string key, std::string val, unsigned int flags,
+                                      std::map<std::string, std::vector<std::string>>& args,
+                                      std::string& error)
 {
     assert(key[0] == '-');
 
@@ -294,31 +296,25 @@ static bool InterpretNegatedOption(std::string& key, std::string& val)
         ++option_index;
     }
     if (key.substr(option_index, 2) == "no") {
-        bool bool_val = InterpretBool(val);
         key.erase(option_index, 2);
-        if (!bool_val ) {
+        if (flags & ArgsManager::ALLOW_BOOL) {
+            if (InterpretBool(val)) {
+                args[key].clear();
+                return true;
+            }
             // Double negatives like -nofoo=0 are supported (but discouraged)
             LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);
             val = "1";
         } else {
-            return true;
+            error = strprintf("Negating of %s is meaningless and therefore forbidden", key.c_str());
+            return false;
         }
     }
-    return false;
+    args[key].push_back(val);
+    return true;
 }
 
-ArgsManager::ArgsManager() :
-    /* These options would cause cross-contamination if values for
-     * mainnet were used while running on regtest/testnet (or vice-versa).
-     * Setting them as section_only_args ensures that sharing a config file
-     * between mainnet and regtest/testnet won't cause problems due to these
-     * parameters by accident. */
-    m_network_only_args{
-      "-addnode", "-connect",
-      "-port", "-bind",
-      "-rpcport", "-rpcbind",
-      "-wallet",
-    }
+ArgsManager::ArgsManager()
 {
     // nothing to do
 }
@@ -384,6 +380,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
 
     for (int i = 1; i < argc; i++) {
         std::string key(argv[i]);
+        if (key == "-") break; //bitcoin-tx using stdin
         std::string val;
         size_t is_index = key.find('=');
         if (is_index != std::string::npos) {
@@ -403,19 +400,14 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
         if (key.length() > 1 && key[1] == '-')
             key.erase(0, 1);
 
-        // Check for -nofoo
-        if (InterpretNegatedOption(key, val)) {
-            m_override_args[key].clear();
-        } else {
-            m_override_args[key].push_back(val);
-        }
-
-        // Check that the arg is known
-        if (!(IsSwitchChar(key[0]) && key.size() == 1)) {
-            if (!IsArgKnown(key)) {
-                error = strprintf("Invalid parameter %s", key.c_str());
+        const unsigned int flags = FlagsOfKnownArg(key);
+        if (flags) {
+            if (!InterpretOption(key, val, flags, m_override_args, error)) {
                 return false;
             }
+        } else {
+            error = strprintf("Invalid parameter %s", key.c_str());
+            return false;
         }
     }
 
@@ -432,21 +424,30 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
     return true;
 }
 
-bool ArgsManager::IsArgKnown(const std::string& key) const
+unsigned int ArgsManager::FlagsOfKnownArg(const std::string& key) const
 {
+    assert(key[0] == '-');
+
     size_t option_index = key.find('.');
-    std::string arg_no_net;
     if (option_index == std::string::npos) {
-        arg_no_net = key;
+        option_index = 1;
     } else {
-        arg_no_net = std::string("-") + key.substr(option_index + 1, std::string::npos);
+        ++option_index;
     }
+    if (key.substr(option_index, 2) == "no") {
+        option_index += 2;
+    }
+
+    const std::string base_arg_name = '-' + key.substr(option_index);
 
     LOCK(cs_args);
     for (const auto& arg_map : m_available_args) {
-        if (arg_map.second.count(arg_no_net)) return true;
+        const auto search = arg_map.second.find(base_arg_name);
+        if (search != arg_map.second.end()) {
+            return search->second.m_flags;
+        }
     }
-    return false;
+    return ArgsManager::NONE;
 }
 
 std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
@@ -538,24 +539,29 @@ void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strV
     m_override_args[strArg] = {strValue};
 }
 
-void ArgsManager::AddArg(const std::string& name, const std::string& help, const bool debug_only, const OptionsCategory& cat)
+void ArgsManager::AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat)
 {
     // Split arg name from its help param
     size_t eq_index = name.find('=');
     if (eq_index == std::string::npos) {
         eq_index = name.size();
     }
+    std::string arg_name = name.substr(0, eq_index);
 
     LOCK(cs_args);
     std::map<std::string, Arg>& arg_map = m_available_args[cat];
-    auto ret = arg_map.emplace(name.substr(0, eq_index), Arg(name.substr(eq_index, name.size() - eq_index), help, debug_only));
+    auto ret = arg_map.emplace(arg_name, Arg{name.substr(eq_index, name.size() - eq_index), help, flags});
     assert(ret.second); // Make sure an insertion actually happened
+
+    if (flags & ArgsManager::NETWORK_ONLY) {
+        m_network_only_args.emplace(arg_name);
+    }
 }
 
 void ArgsManager::AddHiddenArgs(const std::vector<std::string>& names)
 {
     for (const std::string& name : names) {
-        AddArg(name, "", false, OptionsCategory::HIDDEN);
+        AddArg(name, "", ArgsManager::ALLOW_ANY, OptionsCategory::HIDDEN);
     }
 }
 
@@ -614,7 +620,7 @@ std::string ArgsManager::GetHelpMessage() const
         if (arg_map.first == OptionsCategory::HIDDEN) break;
 
         for (const auto& arg : arg_map.second) {
-            if (show_debug || !arg.second.m_debug_only) {
+            if (show_debug || !(arg.second.m_flags & ArgsManager::DEBUG_ONLY)) {
                 std::string name;
                 if (arg.second.m_help_param.empty()) {
                     name = arg.first;
@@ -635,7 +641,7 @@ bool HelpRequested(const ArgsManager& args)
 
 void SetupHelpOptions(ArgsManager& args)
 {
-    args.AddArg("-?", "Print this help message and exit", false, OptionsCategory::OPTIONS);
+    args.AddArg("-?", "Print this help message and exit", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     args.AddHiddenArgs({"-h", "-help"});
 }
 
@@ -839,22 +845,18 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file
         return false;
     }
     for (const std::pair<std::string, std::string>& option : options) {
-        std::string strKey = std::string("-") + option.first;
-        std::string strValue = option.second;
-
-        if (InterpretNegatedOption(strKey, strValue)) {
-            m_config_args[strKey].clear();
+        const std::string strKey = std::string("-") + option.first;
+        const unsigned int flags = FlagsOfKnownArg(strKey);
+        if (flags) {
+            if (!InterpretOption(strKey, option.second, flags, m_config_args, error)) {
+                return false;
+            }
         } else {
-            m_config_args[strKey].push_back(strValue);
-        }
-
-        // Check that the arg is known
-        if (!IsArgKnown(strKey)) {
-            if (!ignore_invalid_keys) {
+            if (ignore_invalid_keys) {
+                LogPrintf("Ignoring unknown configuration value %s\n", option.first);
+            } else {
                 error = strprintf("Invalid configuration value %s", option.first.c_str());
                 return false;
-            } else {
-                LogPrintf("Ignoring unknown configuration value %s\n", option.first);
             }
         }
     }
diff --git a/src/util/system.h b/src/util/system.h
index 66a9eb4612..75e8096826 100644
--- a/src/util/system.h
+++ b/src/util/system.h
@@ -127,6 +127,23 @@ struct SectionInfo
 
 class ArgsManager
 {
+public:
+    enum Flags {
+        NONE = 0x00,
+        // Boolean options can accept negation syntax -noOPTION or -noOPTION=1
+        ALLOW_BOOL = 0x01,
+        ALLOW_INT = 0x02,
+        ALLOW_STRING = 0x04,
+        ALLOW_ANY = ALLOW_BOOL | ALLOW_INT | ALLOW_STRING,
+        DEBUG_ONLY = 0x100,
+        /* Some options would cause cross-contamination if values for
+         * mainnet were used while running on regtest/testnet (or vice-versa).
+         * Setting them as NETWORK_ONLY ensures that sharing a config file
+         * between mainnet and regtest/testnet won't cause problems due to these
+         * parameters by accident. */
+        NETWORK_ONLY = 0x200,
+    };
+
 protected:
     friend class ArgsManagerHelper;
 
@@ -134,9 +151,7 @@ protected:
     {
         std::string m_help_param;
         std::string m_help_text;
-        bool m_debug_only;
-
-        Arg(const std::string& help_param, const std::string& help_text, bool debug_only) : m_help_param(help_param), m_help_text(help_text), m_debug_only(debug_only) {};
+        unsigned int m_flags;
     };
 
     mutable CCriticalSection cs_args;
@@ -256,7 +271,7 @@ public:
     /**
      * Add argument
      */
-    void AddArg(const std::string& name, const std::string& help, const bool debug_only, const OptionsCategory& cat);
+    void AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat);
 
     /**
      * Add many hidden arguments
@@ -269,6 +284,7 @@ public:
     void ClearArgs() {
         LOCK(cs_args);
         m_available_args.clear();
+        m_network_only_args.clear();
     }
 
     /**
@@ -277,9 +293,10 @@ public:
     std::string GetHelpMessage() const;
 
     /**
-     * Check whether we know of this arg
+     * Return Flags for known arg.
+     * Return ArgsManager::NONE for unknown arg.
      */
-    bool IsArgKnown(const std::string& key) const;
+    unsigned int FlagsOfKnownArg(const std::string& key) const;
 };
 
 extern ArgsManager gArgs;