common: Disallow calling IsArgSet() on ALLOW_LIST options

Disallow calling IsArgSet() function on ALLOW_LIST options. Code that uses
IsArgSet() with list options is confusing and leads to mistakes due to the easy
to overlook case where an argument is negated and IsArgSet() returns true, but
GetArgs() returns an empty list.

src/common/args.cpp

@@ -489,6 +489,17 @@ std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
 
 bool ArgsManager::IsArgSet(const std::string& strArg) const
 {
+    // Don't allow IsArgSet() to be called on list arguments, because the odd
+    // case where an argument is negated and IsArgSet() returns true but
+    // GetArgs() returns an empty list has resulted in confusing code and bugs.
+    //
+    // In most cases it's best to treat empty lists and negated lists the same.
+    // In cases where it's useful treat them differently (cases where the
+    // default list value is effectively non-empty), code is less confusing if
+    // it explicitly calls IsArgNegated() to distinguish the negated and empty
+    // conditions instead of IsArgSet() which makes the distinction more
+    // indirectly.
+    CheckArgFlags(strArg, /* require= */ 0, /* forbid= */ ALLOW_LIST, __func__);
     return !GetSetting(strArg).isNull();
 }
 
@@ -624,7 +635,7 @@ bool SettingToBool(const common::SettingsValue& value, bool fDefault)
 bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strValue)
 {
     LOCK(cs_args);
-    if (IsArgSet(strArg)) return false;
+    if (!GetSetting(strArg).isNull()) return false;
     ForceSetArg(strArg, strValue);
     return true;
 }

src/test/argsman_tests.cpp

@@ -495,6 +495,14 @@ struct TestArgsManager : public ArgsManager
         if (test.arg) test.arg->m_flags &= ~ALLOW_LIST;
         return GetBoolArg(name, default_value);
     }
+    //! Call IsArgSet(), temporarily disabling ALLOW_LIST so call can succeed.
+    //! This is called by old tests written before ALLOW_LIST was enforced.
+    bool TestArgSet(const std::string& name)
+    {
+        TestFlags test(*this, name);
+        if (test.arg) test.arg->m_flags &= ~ALLOW_LIST;
+        return IsArgSet(name);
+    }
     using ArgsManager::GetSetting;
     using ArgsManager::GetSettingsList;
     using ArgsManager::ReadConfigStream;
@@ -811,7 +819,7 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters)
     // -a, -b and -ccc end up in map, -d ignored because it is after
     // a non-option argument (non-GNU option parsing)
     BOOST_CHECK(testArgs.m_settings.command_line_options.size() == 3 && testArgs.m_settings.ro_config.empty());
-    BOOST_CHECK(testArgs.IsArgSet("-a") && testArgs.IsArgSet("-b") && testArgs.IsArgSet("-ccc")
+    BOOST_CHECK(testArgs.IsArgSet("-a") && testArgs.IsArgSet("-b") && testArgs.TestArgSet("-ccc")
                 && !testArgs.IsArgSet("f") && !testArgs.IsArgSet("-d"));
     BOOST_CHECK(testArgs.m_settings.command_line_options.count("a") && testArgs.m_settings.command_line_options.count("b") && testArgs.m_settings.command_line_options.count("ccc")
                 && !testArgs.m_settings.command_line_options.count("f") && !testArgs.m_settings.command_line_options.count("d"));
@@ -1056,12 +1064,12 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
 
     BOOST_CHECK(test_args.IsArgSet("-a"));
     BOOST_CHECK(test_args.IsArgSet("-b"));
-    BOOST_CHECK(test_args.IsArgSet("-ccc"));
+    BOOST_CHECK(test_args.TestArgSet("-ccc"));
     BOOST_CHECK(test_args.IsArgSet("-d"));
     BOOST_CHECK(test_args.IsArgSet("-fff"));
     BOOST_CHECK(test_args.IsArgSet("-ggg"));
-    BOOST_CHECK(test_args.IsArgSet("-h"));
-    BOOST_CHECK(test_args.IsArgSet("-i"));
+    BOOST_CHECK(test_args.TestArgSet("-h"));
+    BOOST_CHECK(test_args.TestArgSet("-i"));
     BOOST_CHECK(!test_args.IsArgSet("-zzz"));
     BOOST_CHECK(!test_args.IsArgSet("-iii"));
 
@@ -1449,7 +1457,7 @@ BOOST_FIXTURE_TEST_CASE(util_ArgsMerge, ArgsMergeTestingSetup)
 
         desc += " || ";
 
-        if (!parser.IsArgSet(key)) {
+        if (!parser.TestArgSet(key)) {
             desc += "unset";
             BOOST_CHECK(!parser.IsArgNegated(key));
             BOOST_CHECK_EQUAL(parser.TestArgString(key, "default"), "default");

src/test/fuzz/system.cpp

@@ -71,6 +71,7 @@ FUZZ_TARGET(system, .init = initialize_system)
                 // Avoid hitting:
                 // common/args.cpp:563: void ArgsManager::AddArg(const std::string &, const std::string &, unsigned int, const OptionsCategory &): Assertion `ret.second' failed.
                 const std::string argument_name = GetArgumentName(fuzzed_data_provider.ConsumeRandomLengthString(16));
+                // Avoid assertion in AddArg if this would try to add an already registered flag.
                 if (args_manager.GetArgFlags(argument_name) != std::nullopt) {
                     return;
                 }
@@ -147,8 +148,18 @@ FUZZ_TARGET(system, .init = initialize_system)
     (void)args_manager.GetUnrecognizedSections();
     (void)args_manager.GetUnsuitableSectionOnlyArgs();
     (void)args_manager.IsArgNegated(s1);
-    (void)args_manager.IsArgSet(s1);
+    try {
+        (void)args_manager.IsArgSet(s1);
+    } catch (const std::logic_error&) {
+        // Will throw logic_error if called on an ALLOW_LIST arg.
+    }
 
-    (void)HelpRequested(args_manager);
+    try {
+        (void)HelpRequested(args_manager);
+    } catch (const std::logic_error&) {
+        // May throw logic_error in rare case where SetupHelpOptions randomly
+        // was not called above, but AddArg was called, with a valid arg name
+        // like "-?" combined with invalid flags like ALLOW_LIST.
+    }
 }
 } // namespace