Args: -noconnect=0 is interpreted as -connect=0.0.0.1 #31426

1. 
   hodlinator commented at 8:49 am on December 5, 2024: contributor

   Motivation

   -noconnect=0 is interpreted as -connect=1 which is interpreted as -connect=0.0.0.1

   0₿ build/src/bitcoind -noconnect=0 -debug=net
   

   Produces the following output:

    02024-12-05T08:20:41Z Warning: parsed potentially confusing double-negative -connect=0
    12024-12-05T08:20:41Z Bitcoin Core version v28.99.0-95a0104f2e98-dirty (release build)
    22024-12-05T08:20:41Z parameter interaction: -connect or -maxconnections=0 set -> setting -dnsseed=0
    32024-12-05T08:20:41Z parameter interaction: -connect or -maxconnections=0 set -> setting -listen=0
    42024-12-05T08:20:41Z parameter interaction: -listen=0 -> setting -natpmp=0
    52024-12-05T08:20:41Z parameter interaction: -listen=0 -> setting -discover=0
    62024-12-05T08:20:41Z parameter interaction: -listen=0 -> setting -listenonion=0
    72024-12-05T08:20:41Z parameter interaction: -listen=0 -> setting -i2pacceptincoming=0
    8...
    92024-12-05T08:20:44Z net thread start
   102024-12-05T08:20:49Z [net] connection attempt to 0.0.0.1:8333 timed out
   112024-12-05T08:20:50Z [net] trying v2 connection 1 lastseen=0.0hrs
   

   Main issue

   bitcoind should not try to connect to the 0.0.0.1 IPv4 address.

   Bonus

   -noconnect=0 should not result in -dnsseed=0 and -listen=0 in the parameter interaction logic.

   Issue inspiration: #31212 (comment)

   Possible solution

   Probably best to fail the Init-stage for invalid -(no)connect(=value) permutations.

   Should include a functional test verifying that there is an Init-error for this case, possibly added to test/functional/feature_config_args.py.

2. hodlinator added the label good first issue on Dec 5, 2024
3. 
   vasild commented at 1:50 pm on December 5, 2024: contributor

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

   But:

   config	args.IsArgNegated("-connect")	args.GetArg("-connect").value()	args.GetArgs("-connect")[0]
   -connect=1.2.3.4:1234	false	“1.2.3.4:1234”	“1.2.3.4:1234”
   -connect=0	false	“0”	“0”
   -connect=1	false	“1”	“1”
   -connect=xyz	false	“xyz”	“xyz”
   -connect	false	""	""
   -noconnect=1.2.3.4:1234	true	“0”	(the array is empty)
   -noconnect=0	false	“1”	“1”
   -noconnect=1	true	“0”	(the array is empty)
   -noconnect=xyz	false	“1”	“1”
   -noconnect	true	“0”	(the array is empty)

   :sob:

4. 
   hodlinator commented at 3:02 pm on December 5, 2024: contributor

   Oh boy, maybe not a good first issue. :face_exhaling:
5. 
   hodlinator commented at 3:10 pm on December 5, 2024: contributor

   @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")?
6. 
   Julian128 commented at 3:17 pm on December 5, 2024: none

   Hi guys, did you just produce this table or is it from a documentation?
7. 
   hodlinator commented at 3:26 pm on December 5, 2024: contributor

   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.

8. fanquake removed the label good first issue on Dec 5, 2024
9. maflcko added the label Brainstorming on Dec 5, 2024
10. maflcko added the label Utils/log/libs on Dec 5, 2024
11. maflcko removed the label Brainstorming on Dec 5, 2024
12. 
    vasild commented at 4:46 pm on December 5, 2024: contributor

    @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 :face_with_diagonal_mouth: :grinning:

    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:

     0--- i/src/init.cpp
     1+++ w/src/init.cpp
     2@@ -1939,12 +1939,16 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     3             connOptions.vWhitelistedRangeOutgoing.push_back(subnet);
     4         }
     5     }
     6     
     7     connOptions.vSeedNodes = args.GetArgs("-seednode");
     8
     9+    return InitError(strprintf(_("is negated: %s, size: %u, value[0]: %s"),
    10+                               args.IsArgNegated("-connect") ? "true" : "false",
    11+                               args.GetArgs("-connect").size(),
    12+                               args.GetArgs("-connect").size() > 0 ? args.GetArgs("-connect")[0] : "empty"));
    13     // Initiate outbound connections unless connect=0
    14     connOptions.m_use_addrman_outgoing = !args.IsArgSet("-connect");
    15     if (!connOptions.m_use_addrman_outgoing) {
    16         const auto connect = args.GetArgs("-connect");
    17         if (connect.size() != 1 || connect[0] != "0") {
    18             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.

13. 
    ryanofsky commented at 5:59 pm on December 5, 2024: contributor

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

    0using ConnectSetting = common::Setting<
    1    "-connect=<ip>", std::variant<common::Disabled, std::vector<std::string>>, SettingOptions{.network_only = true},
    2    "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.">
    3    ::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:

    argument	behavior
    -connect=1.2.3.4:1234	Get returns vector {"1.2.3.4:1234"}
    -connect=0	Get returns vector {"0"}
    -connect=1	Get returns vector {"1"}
    -connect=xyz	Get returns vector {"xyz"}
    -connect	Startup error “Cannot set -connect with no value. Please specify value with -connect=value. It must be set to a string.”
    -noconnect=1.2.3.4:1234	Startup error “Cannot negate -connect at the same time as setting a value (‘‘1.2.3.4:1234”)."
    -noconnect=0	Startup error “Cannot negate -connect at the same time as setting a value (‘‘0”)."
    -noconnect=1	Get returns common::Disabled
    -noconnect=xyz	Startup error “Cannot negate -connect at the same time as setting a value (‘‘xyz”)."
    -noconnect	Get returns common::Disabled
    (none)	Get returns empty vector.`

    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.

14. 
    Julian128 commented at 6:39 pm on December 5, 2024: none

    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)

15. 
    l0rinc commented at 12:22 pm on December 6, 2024: contributor

    Agree, can we rather prohibit double negations?
16. 
    hodlinator commented at 12:55 pm on December 6, 2024: contributor

    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.

17. 
    ryanofsky commented at 2:00 pm on December 6, 2024: contributor

    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:

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

    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.

18. 
    Julian128 commented at 11:54 am on December 7, 2024: none

    So there is nothing left to do here, your existing PRs will eventually take care of it?
19. 
    pinheadmz commented at 3:13 pm on December 9, 2024: member

    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.
20. 
    hodlinator commented at 9:37 pm on December 11, 2024: contributor

    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).

Contributors
hodlinator vasild Julian128 ryanofsky l0rinc pinheadmz

Labels
Utils/log/libs