Fix nonsensical -norpcwhitelist, -norpcallowip and related behavior

This change fixes some corner cases handling negated list options:
-norpcwhitelist, -norpcallowip, -norpcbind, -nobind, -nowhitebind, -noconnect,
-noexternalip, -noonlynet, and -nosignetchalleng. Negating these options is now
the same as not specifying them at all. This is useful for being able to
override config file options on the command line without seeing parameter
interaction side effects from the otherwise ignored config options.

The code change here is just avoid calling the IsArgSet() function on
ALLOW_LIST options, and to disallow such calls in the future. 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.

This change includes release notes, but the release notes don't go into details
about specific options. For reference this change:

- Treats specifying -norpcwhitelist exactly the same as not specifying any
  -rpcwhitelist, instead of behaving almost the same but flipping the default
  -rpcwhitelistdefault value.

- Treats specifying -norpcallowip and -norpcbind exactly the same as not
  specifying -rpcallowip or -rpcbind, instead of failing to bind to localhost
  and failing to show warnings when one value is set without the other.

- Treats specifying -nobind, -nowhitebind, and -noconnect exactly the same as
  not specifying -bind, -whitebind, or -connect values instead of treating them
  almost the same but causing parameter interactions with -dnsseed, -listen,
  and m_use_addrman_outgoing values.

- Treats specifying -noexternalip exactly the same as not specifying any
  -externalip, instead of treating it almost the same but interacting with the
  -discover value.

- Treats specifying -noonlynet exactly the same as not specifying -onlynet
  instead of marking all networks unreachable.

- Treats specifying -nosignetchallenge exactly the same as not specifying
  -signetchallenge instead of throwing strange error "-signetchallenge cannot
  be multiple values"

- Clarifies -vbparams and -debug handling code and fixes misleading comments
  without changing behavior.

doc/release-notes-17783.md

@@ -0,0 +1,4 @@
+Configuration
+-------------
+
+Some corner cases handling negated list options `-norpcallowip`, `-norpcbind`, `-nobind`, `-nowhitebind`, `-noconnect`, `-noexternalip`, `-noonlynet`, `-nosignetchallenge` have been fixed. Now negating these options is the same as not specifying them at all.

src/chainparams.cpp

@@ -274,7 +274,15 @@ class SigNetParams : public CChainParams {
         std::vector<uint8_t> bin;
         vSeeds.clear();
 
+<<<<<<< HEAD
         if (!args.IsArgSet("-signetchallenge")) {
+||||||| merged common ancestors
+        if (!args.IsArgSet("-signetchallenge")) {
+            LogPrintf("Using default signet network\n");
+=======
+        if (args.GetArgs("-signetchallenge").empty()) {
+            LogPrintf("Using default signet network\n");
+>>>>>>> Fix nonsensical -norpcwhitelist, -norpcallowip and related behavior
             bin = ParseHex("512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae");
             vSeeds.emplace_back("178.128.221.177");
             vSeeds.emplace_back("2a01:7c8:d005:390::5");
@@ -488,8 +496,6 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
         consensus.SegwitHeight = static_cast<int>(height);
     }
 
-    if (!args.IsArgSet("-vbparams")) return;
-
     for (const std::string& strDeployment : args.GetArgs("-vbparams")) {
         std::vector<std::string> vDeploymentParams;
         boost::split(vDeploymentParams, strDeployment, boost::is_any_of(":"));

src/httprpc.cpp

@@ -266,7 +266,7 @@ static bool InitRPCAuthentication()
         }
     }
 
-    g_rpc_whitelist_default = gArgs.GetBoolArg("-rpcwhitelistdefault", gArgs.IsArgSet("-rpcwhitelist"));
+    g_rpc_whitelist_default = gArgs.GetBoolArg("-rpcwhitelistdefault", gArgs.GetArgs("-rpcwhitelist").size() > 0);
     for (const std::string& strRPCWhitelist : gArgs.GetArgs("-rpcwhitelist")) {
         auto pos = strRPCWhitelist.find(':');
         std::string strUser = strRPCWhitelist.substr(0, pos);

src/httpserver.cpp

@@ -294,16 +294,20 @@ static bool HTTPBindAddresses(struct evhttp* http)
     std::vector<std::pair<std::string, uint16_t>> endpoints;
 
     // Determine what addresses to bind to
-    if (!(gArgs.IsArgSet("-rpcallowip") && gArgs.IsArgSet("-rpcbind"))) { // Default to loopback if not allowing external IPs
+    // To prevent misconfiguration and accidental exposure of the RPC
+    // interface, require -rpcallowip and -rpcbind to both be specified
+    // together. If either is missing, ignore both values, bind to localhost
+    // instead, and log warnings.
+    if (gArgs.GetArgs("-rpcallowip").size() == 0 || gArgs.GetArgs("-rpcbind").size() == 0) { // Default to loopback if not allowing external IPs
         endpoints.push_back(std::make_pair("::1", http_port));
         endpoints.push_back(std::make_pair("127.0.0.1", http_port));
-        if (gArgs.IsArgSet("-rpcallowip")) {
+        if (gArgs.GetArgs("-rpcallowip").size() > 0) {
             LogPrintf("WARNING: option -rpcallowip was specified without -rpcbind; this doesn't usually make sense\n");
         }
-        if (gArgs.IsArgSet("-rpcbind")) {
+        if (gArgs.GetArgs("-rpcbind").size() > 0) {
             LogPrintf("WARNING: option -rpcbind was ignored because -rpcallowip was not specified, refusing to allow everyone to connect\n");
         }
-    } else if (gArgs.IsArgSet("-rpcbind")) { // Specific bind address
+    } else { // Specific bind addresses
         for (const std::string& strRPCBind : gArgs.GetArgs("-rpcbind")) {
             uint16_t port{http_port};
             std::string host;

src/init.cpp

@@ -809,16 +809,16 @@ void InitParameterInteraction(ArgsManager& args)
 {
     // when specifying an explicit binding address, you want to listen on it
     // even when -connect or -proxy is specified
-    if (args.IsArgSet("-bind")) {
+    if (args.GetArgs("-bind").size() > 0) {
         if (args.SoftSetBoolArg("-listen", true))
             LogPrintf("%s: parameter interaction: -bind set -> setting -listen=1\n", __func__);
     }
-    if (args.IsArgSet("-whitebind")) {
+    if (args.GetArgs("-whitebind").size() > 0) {
         if (args.SoftSetBoolArg("-listen", true))
             LogPrintf("%s: parameter interaction: -whitebind set -> setting -listen=1\n", __func__);
     }
 
-    if (args.IsArgSet("-connect")) {
+    if (args.GetArgs("-connect").size() > 0) {
         // when only connecting to trusted nodes, do not seed via DNS, or listen by default
         if (args.SoftSetBoolArg("-dnsseed", false))
             LogPrintf("%s: parameter interaction: -connect set -> setting -dnsseed=0\n", __func__);
@@ -858,7 +858,7 @@ void InitParameterInteraction(ArgsManager& args)
         }
     }
 
-    if (args.IsArgSet("-externalip")) {
+    if (args.GetArgs("-externalip").size() > 0) {
         // if an explicit public IP is specified, do not try to find others
         if (args.SoftSetBoolArg("-discover", false))
             LogPrintf("%s: parameter interaction: -externalip set -> setting -discover=0\n", __func__);
@@ -1071,10 +1071,9 @@ bool AppInitParameterInteraction(const ArgsManager& args)
         InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections));
 
     // ********************************************************* Step 3: parameter-to-internal-flags
-    if (args.IsArgSet("-debug")) {
-        // Special-case: if -debug=0/-nodebug is set, turn off debugging messages
-        const std::vector<std::string> categories = args.GetArgs("-debug");
-
+    const std::vector<std::string> categories = args.GetArgs("-debug");
+    if (!categories.empty()) {
+        // Special-case: if -debug=0/-debug=none is set, turn off debugging messages
         if (std::none_of(categories.begin(), categories.end(),
             [](std::string cat){return cat == "0" || cat == "none";})) {
             for (const auto& cat : categories) {
@@ -1441,7 +1440,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             strSubVersion.size(), MAX_SUBVERSION_LENGTH));
     }
 
-    if (args.IsArgSet("-onlynet")) {
+    if (args.GetArgs("-onlynet").size() > 0) {
         std::set<enum Network> nets;
         for (const std::string& snet : args.GetArgs("-onlynet")) {
             enum Network net = ParseNetwork(snet);
@@ -1994,7 +1993,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     connOptions.vSeedNodes = args.GetArgs("-seednode");
 
     // Initiate outbound connections unless connect=0
-    connOptions.m_use_addrman_outgoing = !args.IsArgSet("-connect");
+    connOptions.m_use_addrman_outgoing = args.GetArgs("-connect").empty();
     if (!connOptions.m_use_addrman_outgoing) {
         const auto connect = args.GetArgs("-connect");
         if (connect.size() != 1 || connect[0] != "0") {

src/test/util_tests.cpp

@@ -256,6 +256,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;
@@ -538,7 +546,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"));
@@ -783,12 +791,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"));
 
@@ -1168,7 +1176,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/util/system.cpp

@@ -487,6 +487,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 explicity 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();
 }
 
@@ -602,7 +613,7 @@ bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const
 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;
 }