Probably best to just fail the Init-stage with an error if IsArgNegated() for -connect is true and the value is not 0.

But:

😭

Oh boy, maybe not a good first issue. 😮‍💨

@vasild how does the table look if you re-test using args.GetArgs("-connect") as is used in the code instead of args.GetArg("-connect")?

Hi guys, did you just produce this table or is it from a documentation?

Hi guys, did you just produce this table or is it from a documentation?

I believe vasild hand-rolled modifications to output the values.

On another note, it seems like I don't have permissions to remove the "good first issue" label. Anyone with access, please remove it.

@Julian128 feel free to take a stab at it anyway, but I'm no longer clear on what the solution would be. Will edit the description.

@vasild how does the table look if you re-test using args.GetArgs("-connect") as is used in the code instead of args.GetArg("-connect")?

I updated the table... but why did you had to ask! Now it looks even more convoluted 🫤 😀

Hi guys, did you just produce this table or is it from a documentation?

I created the table manually, and used something like this to get the values:

--- i/src/init.cpp
+++ w/src/init.cpp
@@ -1939,12 +1939,16 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             connOptions.vWhitelistedRangeOutgoing.push_back(subnet);
         }
     }
     
     connOptions.vSeedNodes = args.GetArgs("-seednode");

+    return InitError(strprintf(_("is negated: %s, size: %u, value[0]: %s"),
+                               args.IsArgNegated("-connect") ? "true" : "false",
+                               args.GetArgs("-connect").size(),
+                               args.GetArgs("-connect").size() > 0 ? args.GetArgs("-connect")[0] : "empty"));
     // Initiate outbound connections unless connect=0
     connOptions.m_use_addrman_outgoing = !args.IsArgSet("-connect");
     if (!connOptions.m_use_addrman_outgoing) {
         const auto connect = args.GetArgs("-connect");
         if (connect.size() != 1 || connect[0] != "0") {
             connOptions.m_specified_outgoing = connect;

What about, generally, forbidding =value if the config argument is prefixed with -no? That is, if we have -foo config option, then only -nofoo is allowed, but -nofoo=whatever gives an error.

I think a good way to handle this is to make the -connect setting typed using the syntax implemented in #31260:

using ConnectSetting = common::Setting<
    "-connect=<ip>", std::variant<common::Disabled, std::vector<std::string>>, SettingOptions{.network_only = true},
    "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.">
    ::Category<OptionsCategory::CONNECTION>;

Retrieving the setting with ConnectSetting::Get() would return the specified std::variant<common::Disabled, std::vector<std::string>> type which could be interpreted unambiguously. With the runtime type checking from #16545 also applied (the corresponding flags there are ALLOW_STRING | ALLOW_LIST), the results would be:

If using std::variant as suggested above seems cumbersome, #31260 also allows specifying the type as std::optional<std::vector<std::string>> which may be a little less obvious but is still unambiguous. Using that type, -noconnect=1 and -noconnect would cause Get() to return the empty vector, and specifying no -connect arguments would make Get() return std::nullopt

I do think, in general 0 and 1 addresses should be allowed since most network tools do allow them, 0 can be a convenient way to specify localhost, and in general network stacks probably have more context for interpreting addresses than we do.

These double negatives are inherently confusing.

I wonder if this is making things more convoluted than they have to be, what if we simply disallow double negatives?
So any negated arg with value 0 would return null.

Or is there any reason to use such a flag?

edit: I found this comment regarding this:
// Double negatives like -nofoo=0 are supported (but discouraged)

Agree, can we rather prohibit double negations?

Agree on the double negatives, I think @ryanofsky's change #16545 is pretty spot on in his table above for the -noconnect behavior. Just error when specifying any other value than -noconnect=1. Maybe that behavior could be broken out and submitted in its own PR? Shouldn't depend on the typing changes.

I do think, in general 0 and 1 addresses should be allowed

Oof, that's suprising to me, but never thought to try them in other networking contexts. -connect=10 is currently interpreted as 0.0.0.10, would almost have expected hex when there are no dots. -connect=256 becomes 0.0.1.0 which I guess makes sense.

These double negatives are inherently confusing.

Agree, can we rather prohibit double negations?

Agree on the double negatives

Not sure when double negatives (-noconnect=0) were introduced but they have been around a long time, and there has been a warning whenever double negatives are used since #12713. It seems ok if someone wants to make a PR changing double negatives from a warning to an error and including tests, release notes, etc. Though I think a more direct fix for this issue would probably be:

--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -363,7 +363,8 @@ std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const
 {
     std::vector<std::string> result;
     for (const common::SettingsValue& value : GetSettingsList(strArg)) {
-        result.push_back(value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str());
+        // Treat double-negative -noarg=0 the same as -arg and return ""
+        result.push_back(value.isFalse() ? "0" : value.isTrue() ? "" : value.get_str());
     }
     return result;
 }

IMO double negatives are just tip of the iceberg when it comes to unusual option parsing behavior, compared to more harmful behavior like interpreting -booloption=true as false and hard to predict merging behaviors described #17508. My PR #16545 and the followups listed there try to clean up all of these behaviors comprehensively by introducing typed settings. However the ALLOW_ flags introduced in #16545 have been shown to be pretty confusing and unappealing to reviewers, so I worked on #31260 to replace the ALLOW_ flags with c++ types and hopefully this can unblock #16545 and its followups.

#16545 does disallow double negatives for typed settings. But apart from this bug, which I think is caused more by bad API design than by the double negatives, that double negatives are pretty innocuous and just a throwback to a more casual computing culture where developers trusted users more and thought things like this were convenient and cute.

So there is nothing left to do here, your existing PRs will eventually take care of it?

Do we use type checking for any configuration argument? I just noticed i could start bitcoind with -prune=pancakes with no errors: also no idea what the prune limit would actually be set at.

So there is nothing left to do here, your existing PRs will eventually take care of it?

Given the complexity of existing behavior, it may be better to help review one of @ryanofsky's giga-PRs. The older one has had its 5th year anniversary already.

Do we use type checking for any configuration argument?

Approximately zero type checking on a generalized level (ArgsManager).