Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior

ryanofsky commented at 1:42 pm on July 26, 2024: contributor

The PR changes behavior of negated -noseednode, -nobind, -nowhitebind, -norpcbind, -norpcallowip, -norpcwhitelist, -notest, -noasmap, -norpcwallet, -noonlynet, and -noexternalip options, so negating these options just clears previously specified values doesn’t have other side effects.

Negating options on the command line can be a useful way of resetting options that may have been set earlier in the command line or config file. But before this change, negating these options wouldn’t fully reset them, and would have confusing and undocumented side effects (see commit descriptions for details). Now, negating these options just resets them and behaves the same as not specifying them.

Motivation for this PR is to fix confusing behaviors and also to remove incorrect usages of the IsArgSet() function. Using IsArgSet() tends to lead to negated option bugs in general, but it especially causes bugs when used with list settings returned by GetArgs(), because when these settings are negated, IsArgSet() will return true but GetArgs() will return an empty list. This PR eliminates all uses of IsArgSet() and GetArgs() together, and followup PR #17783 makes it an error to use IsArgSet() on list settings, since calling IsArgSet() is never actually necessary. Most of the changes here were originally made in #17783 and then moved here to be easier to review and avoid a dependency on #16545.

DrahtBot commented at 1:42 pm on July 26, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30529.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator, danielabrozzoni
Concept NACK	maflcko
Concept ACK	stratospher, mzumsande
Stale ACK	naiyoma

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28792 (Embed default ASMap as binary dump header file by fjahr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

mzumsande commented at 2:06 pm on July 26, 2024: contributor

My understanding of the args parsing code is superficial, but why is changing IsArgSet() behavior not an option, instead of telling people not to use it anymore? Are there many spots that rely on it returning true in connection with a negated arg?

maflcko commented at 2:07 pm on July 26, 2024: member

Make negating options the same as not setting them … -noconnect

Maybe I am misunderstanding, but if you are trying to say that this treats -noconnect as if it never was set at all, then I tend to NACK, because that’d break pretty much every setup, including the tests, see #10085 (comment), https://github.com/bitcoin/bitcoin/blob/30cef53707fb62ab1c9e73c2b5bc8b006e0613d3/src/init.cpp#L520, https://github.com/bitcoin/bitcoin/blob/30cef53707fb62ab1c9e73c2b5bc8b006e0613d3/doc/bitcoin-conf.md?plain=1#L17, …

ryanofsky commented at 3:04 pm on July 26, 2024: contributor

Make negating options the same as not setting them … -noconnect

Maybe I am misunderstanding, but if you are trying to say that this treats -noconnect as if it never was set at all, then I tend to NACK

Nice catch! Definitely agree it would be a mistake to change -noconnect behavior, so I’ll revert those changes and add comments to code to clarify the intent. Will also look into why feature_config_args.py test didn’t catch this since it is testing -noconnect explicitly.

re: #30529#pullrequestreview-2201953036

but why is changing IsArgSet() behavior not an option, instead of telling people not to use it anymore? Are there many spots that rely on it returning true in connection with a negated arg?

I don’t think there is something wrong with IsArgSet() behavior. The function is doing what you’d expect it to do, but the it’s just not a very useful function to call in most cases compared to alternatives. I think we could actually eliminate it pretty easily but did not want to expand the scope of this PR.

ryanofsky referenced this in commit ea7dbfba8a on Jul 29, 2024

ryanofsky force-pushed on Jul 29, 2024

ryanofsky commented at 3:56 am on July 29, 2024: contributor

Updated 3c17b800ca3273dcb5bd9b94c400396fdc80cd23 -> 02dde1ca402cc495949ac20f2b6d3c3626bd0144 (pr/listset.1 -> pr/listset.2, compare) reverting -noconnect change and adding a test for the regression, also splitting up the main commit a bit and dropping the documentation change to make this PR more focused Updated 02dde1ca402cc495949ac20f2b6d3c3626bd0144 -> 10770c164485324af8acdd3b932b51d32862c41f (pr/listset.2 -> pr/listset.3, compare) tweaking comments and commit message

ryanofsky force-pushed on Jul 29, 2024

ryanofsky referenced this in commit 77ecca94a0 on Aug 2, 2024

in src/init.cpp:1924 in d07937070f outdated

1923-    connOptions.m_use_addrman_outgoing = !args.IsArgSet("-connect");
1924-    if (!connOptions.m_use_addrman_outgoing) {
1925-        const auto connect = args.GetArgs("-connect");
1926+    const auto connect = args.GetArgs("-connect");
1927+    if (!connect.empty() || args.IsArgNegated("-connect")) {
1928+        // Do not initiate addrman connections when only connecting to trusted

stratospher commented at 6:33 pm on August 8, 2024:

d079370: nit: maybe use the term outbound connections instead of addrman connections?

ryanofsky commented at 6:44 pm on August 26, 2024:

re: #30529 (review)

maybe use the term outbound connections instead of addrman connections?

Thanks, used and updated this comment

in test/functional/feature_config_args.py:330 in ea7dbfba8a outdated

325+            # Need to temporarily remove these settings from the config file in
326+            # order for the two log messages to appear
327+            self.nodes[0].replace_in_config([("bind=", "#bind"), ("dnsseed=", "#dnsseed=")])
328+            with self.nodes[0].assert_debug_log(expected_msgs=addcon_thread_started) as info:
329+                self.restart_node(0, extra_args=[connect_arg])
330+            self.nodes[0].replace_in_config([("#bind=", "bind"), ("#dnsseed=", "dnsseed=")])

stratospher commented at 12:39 pm on August 16, 2024:

ea7dbfb:

0            self.nodes[0].replace_in_config([("#bind", "bind="), ("#dnsseed=", "dnsseed=")])

ryanofsky commented at 7:02 pm on August 26, 2024:

re: #30529 (review)

ea7dbfb:

Thanks! That might have caused problems if more tests were added after this

ryanofsky referenced this in commit dd0ad55a4e on Aug 23, 2024

in src/init.cpp:726 in d07937070f outdated

736@@ -737,8 +737,9 @@ void InitParameterInteraction(ArgsManager& args)
737             LogInfo("parameter interaction: -whitebind set -> setting -listen=1\n");
738     }
739 
740-    if (args.IsArgSet("-connect") || args.GetIntArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS) <= 0) {
741+    if (!args.GetArgs("-connect").empty() || args.IsArgNegated("-connect") || args.GetIntArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS) <= 0) {

stratospher commented at 6:56 am on August 26, 2024:

d079370: (feel free to ignore/not a blocker since noconnect=0 isn’t recommended)

would IsArgSet be simpler compared to GetArgs()/IsArgNegated() here? or is something else meant by preferring GetArgs()/IsArgNegated() in most cases (but not all) over IsArgSet.

i just found this behaviour of noconnect confusing - even though both do the same thing, GetArgs() returns empty in one case and not empty in the other case. (and similarly for IsArgNegated)

when noconnect=1
- !args.GetArgs("-connect").empty() = 0 and args.IsArgNegated("-connect") = 1
when noconnect=0
- !args.GetArgs("-connect").empty() = 1 and args.IsArgNegated("-connect") = 0

ryanofsky commented at 6:44 pm on August 26, 2024:

re: #30529 (review)

would IsArgSet be simpler compared to GetArgs()/IsArgNegated() here?

It’s true double negation behavior, treating -noconnect=0 like -connect=1, is confusing and not obvious. But ArgsManager code goes out of its way to treat double negated settings the same as setting 1, so you generally do not need to think about this case when you are looking at code calling ArgsManager methods. If the code is doing the right thing for single negated and non-negated cases, it will probably be doing something sensible for the double negated case, too.

The more common case that does currently require thought to get right is the single negation case -noconnect=1 -nobind=1 -norpcwhitelist=1 etc, where the IsArgSet method is a real source of confusion and bugs. Of the 15 IsArgSet calls removed in this PR, 12 of the calls were buggy, 2 of the calls were not buggy but unnecessary, and one call (this one) was correct but inexplicit and unclear.

The code !args.GetArgs("-connect").empty() || args.IsArgNegated("-connect") is supposed to disable DNS seeds and listening when explicit connections are specified or when connections are disabled. I think it’s clearer to list those two cases separately instead of relying on knowledge that IsArgSet returns true in both cases, mostly because of all the other evidence that the IsArgSet method is misunderstood and not used correctly.

That said, I would not have a problem reverting this line and line 1923 below if reviewers think calling IsArgSet these two places is actually clearer.

or is something else meant by preferring GetArgs()/IsArgNegated() in most cases (but not all) over IsArgSet.

This PR should be consistently removing IsArgSet calls for all list settings. It is not removing other IsArgSet calls because the scope of the PR is limited, but I don’t think there are any cases where IsArgSet should be preferred at this point. I think it is basically a footgun.

stratospher commented at 8:13 am on August 26, 2024: contributor

Concept ACK. nice to have more consistent behaviour for what to expect with the negated version of the config flags compared to on master.

ryanofsky referenced this in commit 0e98e5b908 on Aug 26, 2024

ryanofsky force-pushed on Aug 26, 2024

ryanofsky commented at 7:05 pm on August 26, 2024: contributor

Updated 10770c164485324af8acdd3b932b51d32862c41f -> a1c9d23ff587b84d37175b2993ea6760f9762177 (pr/listset.3 -> pr/listset.4, compare) with suggested changes

naiyoma commented at 11:53 am on September 5, 2024: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/30529/commits/0e98e5b908dfc7cc8f6b4d2dca4d67b7fd8b5f7c Ran the functional test locally and test passes successfully, checks -noconnect and -connect=0 disables -listen and -dnsseed.

in test/functional/feature_config_args.py:410 in 0e98e5b908 outdated

337+            with self.nodes[0].assert_debug_log(expected_msgs=addcon_thread_started) as info:
338+                self.restart_node(0, extra_args=[connect_arg, '-dnsseed', '-proxy=localhost:1080'])
339+            for msg in dnsseed_ignored:
340+                if msg not in info.log:
341+                    raise AssertionError(f"Expected {msg!r} in -noconnect log message")
342+

naiyoma commented at 12:05 pm on September 5, 2024:

nit : not sure about this but I think it would be useful to check that when -noconnect/connect=0 is set no outbound connections are made.

ryanofsky commented at 2:41 pm on September 5, 2024:

That would be nice and if I can figure out a way to check for that more directly I could drop the second half of this new test. It looks like there is a log message “%i block-relay-only anchors will be tried for connections” that gets printed when m_use_addrman_outgoing is false so maybe I could check for that when -noconnect is specified, and have better confirmation that noconnect is working correctly than this dnsseed_ignored check provides.

I think the first half of the test checking for dnsseed_disabled and listen_disabled is useful in either case since it is providing test coverage for InitParameterInteraction code handling noconnect instead of AppInitMain code handling it.

DrahtBot added the label CI failed on Oct 13, 2024

achow101 commented at 3:49 pm on October 15, 2024: member

cc @l0rinc @hodlinator

achow101 requested review from mzumsande on Oct 15, 2024

maflcko commented at 3:26 pm on October 16, 2024: member

Could rebase for fresh CI?

DrahtBot removed the label CI failed on Oct 23, 2024

in src/init.cpp:1957 in a1c9d23ff5 outdated

1935         }
1936+
1937+        // Warn that -seednode setting will be ignored if trusted nodes were
1938+        // specified.
1939         if (!connOptions.m_specified_outgoing.empty() && !connOptions.vSeedNodes.empty()) {
1940             LogPrintf("-seednode is ignored when -connect is used\n");

hodlinator commented at 8:42 pm on October 24, 2024:

nit: LogPrintf-string provides enough info that I find this added comment redundant.

ryanofsky commented at 9:39 pm on December 6, 2024:

re: #30529 (review)

nit: LogPrintf-string provides enough info that I find this added comment redundant.

That makes sense, will drop.

in src/init.cpp:1961 in a1c9d23ff5 outdated

1941         }
1942 
1943+        // Warn that -dnsseed will be ignored if automatic connections are
1944+        // disabled.
1945         if (args.IsArgSet("-dnsseed") && args.GetBoolArg("-dnsseed", DEFAULT_DNSSEED) && args.IsArgSet("-proxy")) {
1946             LogPrintf("-dnsseed is ignored when -connect is used and -proxy is specified\n");

hodlinator commented at 9:00 pm on October 24, 2024:

Better to explain why than what (log message already explains what). To me it’s not immediately obvious that we should avoid using DNS seeds when having a Tor SOCKS5 proxy?

Furthermore, as I understand it we don’t really ignore -dnsseed when having a proxy; we use them differently, making the log message not correct to begin with?: https://github.com/bitcoin/bitcoin/blob/a1c9d23ff587b84d37175b2993ea6760f9762177/src/net.cpp#L2284-L2288

ryanofsky commented at 10:03 pm on December 6, 2024:

re: #30529 (review)

Better to explain why than what (log message already explains what). To me it’s not immediately obvious that we should avoid using DNS seeds when having a Tor SOCKS5 proxy?

Just dropped the new comments for now. There should be more chances to make improvements here later.

The comment was saying why the dnsseed is ignored: because automatic connections are disabled. The log message says the seed is ignored because -connect is specified, but it is not obvious (IMO) that specifying -connect disables other connections.

The -proxy part of this check seems buggy to me and I mentioned that in the commit message, but didn’t look into the history, and in any case don’t want to change the behavior in this PR.

hodlinator commented at 1:23 pm on December 16, 2024:

Unintended negation in commit message of 0a954ce80cf9d9744eaf213e3b81d4016b19bf88? “Not sure if it really makes sense >not< to warn about seednode being ignored if -noconnect is specified, and only to warn about -dnsseed being ignored when -proxy is specified, but these behaviors are not changed from before.”

ryanofsky commented at 7:49 pm on December 19, 2024:

re: #30529 (review)

I think the comment was accurate but I deleted it now since I already deleted the other comments about this code, too. (The comment was saying that if -noconnect was specified the seednode would be ignored but no “-seednode is ignored when -connect is used” warning would be shown, because m_specified_outgoing would be empty. I don’t think having the m_specified_outgoing condition for that warning makes sense, just like I don’t think having the args.IsArgSet("-proxy") condition for the “-dnsseed is ignored when -connect is used” warning makes sense.)

in src/torcontrol.cpp:406 in a1c9d23ff5 outdated

402@@ -403,7 +403,7 @@ void TorController::get_socks_cb(TorControlConnection& _conn, const TorControlRe
403     const auto onlynets = gArgs.GetArgs("-onlynet");
404 
405     const bool onion_allowed_by_onlynet{
406-        !gArgs.IsArgSet("-onlynet") ||
407+        gArgs.GetArgs("-onlynet").empty() ||

hodlinator commented at 9:14 pm on October 24, 2024:

Use local variable.

0        onlynets.empty() ||

ryanofsky commented at 10:06 pm on December 6, 2024:

re: #30529 (review)

Use local variable.

Good catch, updated

in src/init.cpp:1955 in a1c9d23ff5 outdated

1930+        connOptions.m_use_addrman_outgoing = false;
1931+
1932+        // Assign addresses of trusted nodes, if any.
1933         if (connect.size() != 1 || connect[0] != "0") {
1934             connOptions.m_specified_outgoing = connect;
1935         }

hodlinator commented at 9:38 pm on October 24, 2024:

More useful comment IMO:

0        // Populate outgoing connections unless negated or disabled.
1        if (connect.size() != 1 || connect[0] != "0") {
2            connOptions.m_specified_outgoing = connect;
3        }

ryanofsky commented at 9:31 pm on December 6, 2024:

re #30529 (review)

More useful comment IMO:

Just dropped this comment for now too.

I don’t think the “negated or disabled” part of that suggestion is accurate. I’m also not sure “Populate outgoing connections” more helpfully describes the assignment than “Assign addresses of trusted nodes”, but happy to add this if you can explain a little why that’s better the other comment.

In this code m_specified_outgoing will usually be assigned here if the argument is negated, depending on what follows the negated argument. The if statement here is just checking for a different case where -connect=0 is specified and it is the last and only argument.

I think the behavior here is slightly broken because if -connect=0 is followed by more -connect options the 0 address will be added. Also, the 0 address can be a valid address (synonym for localhost in linux), so maybe it should always be added. In any case it seems not great to add 0 address in some cases but not others. Probably better to drop it consistently.

But this PR is not trying to change networking behaior so I’m just adding a few comments to clarify intent of the code and say what it is doing at a high level.

hodlinator commented at 1:33 pm on December 16, 2024:

Yeah, I think my understanding of how list-args are treated is less solid than yours. Calling connect-values “trusted” added some context, but is kind of implied. Removing the comment was a better choice than taking my suggestion. :+1:

in test/functional/feature_config_args.py:405 in a1c9d23ff5 outdated

328+            with self.nodes[0].assert_debug_log(expected_msgs=addcon_thread_started) as info:
329+                self.restart_node(0, extra_args=[connect_arg])
330+            self.nodes[0].replace_in_config([("#bind=", "bind="), ("#dnsseed=", "dnsseed=")])
331+            for msg in [dnsseed_disabled, listen_disabled]:
332+                if msg not in info.log:
333+                    raise AssertionError(f"Expected {msg!r} in -noconnect log message")

hodlinator commented at 9:58 pm on October 24, 2024:

nit: Would move this line resetting the config after the for-loop to clarify that it does not affect it (though I understand the urge to keep it close to the original replace).

(Verified locally that test still succeeds with the move just to be sure).

0            for msg in [dnsseed_disabled, listen_disabled]:
1                if msg not in info.log:
2                    raise AssertionError(f"Expected {msg!r} in -noconnect log message")
3            self.nodes[0].replace_in_config([("#bind=", "bind="), ("#dnsseed=", "dnsseed=")])

ryanofsky commented at 9:10 pm on December 6, 2024:

re: #30529 (review)

nit: Would move this line resetting the config after the for-loop to clarify that it does not affect it (though I understand the urge to keep it close to the original replace).

(Verified locally that test still succeeds with the move just to be sure).

Makes sense, moved this line

hodlinator commented at 10:00 pm on October 24, 2024: contributor

Concept ACK a1c9d23ff587b84d37175b2993ea6760f9762177

Good to fix weird negation corner cases.

(Very tempting to slap DISALLOW_NEGATION on non-list args for some reason, but I guess someone could be using norpcwallet to reset earlier values instead of rpcwallet=).

mzumsande commented at 8:26 pm on October 28, 2024: contributor

Concept ACK

Does this PR have the aspiration to fix all instances where it makes sense?

If so, I think that the pattern from 40ecf91068545ba55a6af5e2536a61c459334925 could also be applied to -port and -rpcport in CheckHostPortOptions() or -asmap, which all fail init in master when negated and which could be used to negate earlier options if changed.

DrahtBot added the label Needs rebase on Dec 5, 2024

Fix nonsensical -noseednode behavior

Treat specifying -noseednode the same as not specifying any -seednode value,
instead of enabling the seed node timeout and log messages, and waiting longer
to add other seeds.

b6ff1aa8b8

Fix nonsensical -nobind and -nowhitebind behavior

Treat specifying -nobind and -nowhitebind the same as not specifying -bind and
-whitebind values instead of causing them to soft-set -listen=1.

c1d1ff6350

Fix nonsensical -norpcbind and -norpcallowip behavior

Treat specifying -norpcbind and -norpcallowip the same as not specifying
-rpcbind or -rpcallowip, instead of failing to bind to localhost and failing to
show warnings.

Also add code comment to clarify what intent of existing code is.

80d2668768

Fix nonsensical -norpcwhitelist behavior

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

This is confusing because before this change if -norpcwhitelist was specified
it would block users from calling any RPC methods.

91b2523df4

Fix nonsensical -notest behavior

Treat specifying -notest exactly the same as not specifying any
-test value, instead of complaining that it must be used with -regtest.

d3e9daf675

ryanofsky referenced this in commit f6d638331f on Dec 6, 2024

ryanofsky referenced this in commit bd524060c3 on Dec 6, 2024

ryanofsky force-pushed on Dec 6, 2024

ryanofsky commented at 10:39 pm on December 6, 2024: contributor

Thanks for the reviews!

Rebased a1c9d23ff587b84d37175b2993ea6760f9762177 -> 54ad580775b8379a62b05b1f2cdf8b7fde993306 (pr/listset.4 -> pr/listset.5, compare) due to conflict with #31212 and to make suggested changes.

re: #30529#pullrequestreview-2400135259

Does this PR have the aspiration to fix all instances where it makes sense?

If so, I think that the pattern from 40ecf91 could also be applied to -port and -rpcport in CheckHostPortOptions() or -asmap, which all fail init in master when negated and which could be used to negate earlier options if changed.

Need to look into these. There are definitely more places where behavior could be improved. This PR was specifically trying to cover all the cases where IsArgSet and GetArgs were being used together with goal of simplifying #17783 and doesn’t look at most cases beyond that.

DrahtBot removed the label Needs rebase on Dec 6, 2024

in test/functional/feature_config_args.py:405 in 54ad580775 outdated

400+        self.nodes[0].replace_in_config([("bind=", "#bind="), ("dnsseed=", "#dnsseed=")])
401+        with self.nodes[0].assert_debug_log(expected_msgs=addcon_thread_started) as info:
402+            self.restart_node(0, extra_args=[connect_arg])
403+        for msg in [dnsseed_disabled, listen_disabled]:
404+            if msg not in info.log:
405+                raise AssertionError(f"Expected {msg!r} in -noconnect log message")

hodlinator commented at 9:48 am on December 16, 2024:

prefer raising AssertionError in less roundabout way:

0            assert msg in info.log, f"Expected {msg!r} in -noconnect log message"

+same pattern below.

(Or change assert_debug_log-inputs and remove these parts entirely, as described in other comment).

hodlinator commented at 10:11 am on December 16, 2024:

Instead of waiting for addcon_thread_started, one could wait directly on the asserted content, removing the need to add functionality for yeilding the log content:

0        with self.nodes[0].assert_debug_log(expected_msgs=[dnsseed_disabled, listen_disabled]) as info:
1            self.restart_node(0, extra_args=[connect_arg])

+similar below

ryanofsky commented at 7:26 pm on December 19, 2024:

re: #30529 (review)

prefer raising AssertionError in less roundabout way:

Reason it’s written this way is so -O or PYTHONOPTIMIZE doesn’t skip checks this test is trying to perform. (IMO it only makes sense to use assert to check for internal assumptions made by test code, which could be skipped without changing coverage provided by the test.)

ryanofsky commented at 7:26 pm on December 19, 2024:

re: #30529 (review)

Instead of waiting for addcon_thread_started, one could wait directly on the asserted content, removing the need to add functionality for yeilding the log content

Reason for doing this is to fit in with existing tests in this file, and to fail right away instead of timing out when the expected log prints are missing. Tests that time out instead of failing are unpleasant to work with.

hodlinator commented at 9:59 am on December 20, 2024:

Should people really be running functional tests with those flags?

0~/bitcoin/test/functional
1₿ git grep -E "\bassert\b" |wc -l
21499
3
4~/bitcoin/test/functional
5₿ git grep assert |wc -l
67228

My bet is that more than a handful of those 20.7% cases are not for internal assumptions.

If we really want to only use assert for internal assumptions maybe we shouldn’t be using AssertionError for other cases and should not be calling our custom functions assert_*?

Tried running functional tests (without BDB etc) with PYTHONOPTIMIZE=1 and it made the following tests fail:

p2p_ping.py
rpc_preciousblock.py
rpc_psbt.py –descriptors
tool_signet_miner.py –descriptors
wallet_miniscript.py –descriptors

Maybe best to disallow running with those flags for now?

hodlinator commented at 10:08 am on December 20, 2024:

Reason for doing this is to fit in with existing tests in this file, and to fail right away instead of timing out when the expected log prints are missing. Tests that time out instead of failing are unpleasant to work with.

The rest of the tests are not using the new info functionality, so my suggestion is arguably a better fit, and your version would still time out if addcon_thread_started was not found (although that timeout would happen in earlier checks). Not sure we should be optimizing for the failure case. I guess either is acceptable though.

ryanofsky commented at 5:20 pm on January 10, 2025:

re: #30529 (review)

Thanks, I dropped the AssertionError now thanks to your other suggested simplification, and your observations above about assert statement usage and PYTHONOPTIMIZE are also interesting. IMO, it would be good to clean up the failing tests and maybe add a warning that some failures might not be detected if optimizations are turned on.

More broadly, I think there are common conventions for using assert statements that make code easier to read and intent easier to see, namely that:

assert statements can be removed without changing the meaning of the code
if an assert statement fails, it means an internal assumption is wrong, not that an external failure has occurred

These are conventions I like to follow and benefit from when they are followed other places. I wouldn’t insist that anybody else needs to follow these conventions, but I wouldn’t write code breaking them myself, just like I wouldn’t write code calling string variables i and int variables s, because there’s a common and useful convention that does the opposite of this and I don’t want to break.

ryanofsky commented at 5:20 pm on January 10, 2025:

re: #30529 (review)

Thanks, I adopted your suggestion now. I assumed the other tests were also checking for addcon_thread_started to fail quickly instead of timing out if expected messages did not show up. But actually the other tests are using addcon_thread_started, because they are checking for absence not presence of other log messages, so they don’t have any other alternative except checking for the later log message.

I still think waiting for a message that you know will show up and then checking for other conditions can be a good approach to avoid test timeouts and make tests easier to run and modify, but agree now the simpler approach works better here.

in test/functional/test_framework/test_node.py:506 in 54ad580775 outdated

498@@ -498,7 +499,8 @@ def assert_debug_log(self, expected_msgs, unexpected_msgs=None, timeout=2):
499         time_end = time.time() + timeout * self.timeout_factor
500         prev_size = self.debug_log_size(encoding="utf-8")  # Must use same encoding that is used to read() below
501 
502-        yield
503+        info = types.SimpleNamespace()
504+        yield info

hodlinator commented at 10:01 am on December 16, 2024:

Could add comment about why this is useful, either in the code or commit (f6d638331fff8a9b1bd670abe65ee70d1441ad9a)?

Something like:

0# Can be used to grep for log content earlier in the
1# log once all expected messages are found.

hodlinator commented at 1:54 pm on December 16, 2024:

(Not applicable if #30529 (review) is followed).

ryanofsky commented at 7:27 pm on December 19, 2024:

re: #30529 (review)

Could add comment about why this is useful, either in the code or commit (f6d6383)?

Thanks, added comment to explain this.

in src/init/common.cpp:83 in 54ad580775 outdated

81-    if (args.IsArgSet("-debug")) {
82-        // Special-case: if -debug=0/-nodebug is set, turn off debugging messages
83+        // Special-case: if -debug=0/-debug=none is set, turn off debugging messages
84         const std::vector<std::string> categories = args.GetArgs("-debug");
85 
86         if (std::none_of(categories.begin(), categories.end(),

hodlinator commented at 10:32 am on December 16, 2024:

nit: Would be nice with a corresponding else + check that at least warned if other categories were being mixed together with 0/none.

ryanofsky commented at 7:26 pm on December 19, 2024:

re: #30529 (review)

nit: Would be nice with a corresponding else + check that at least warned if other categories were being mixed together with 0/none.

Definitely agree. I don’t think it relates to this PR, but the error checking implemented in this part of code is not great.

in src/chainparams.cpp:34 in 54ad580775 outdated

31     }
32-    if (args.IsArgSet("-signetchallenge")) {
33+    if (!args.GetArgs("-signetchallenge").empty()) {
34         const auto signet_challenge = args.GetArgs("-signetchallenge");
35         if (signet_challenge.size() != 1) {
36             throw std::runtime_error("-signetchallenge cannot be multiple values.");

hodlinator commented at 10:44 am on December 16, 2024:

nit: Just seems plain wrong to use GetArgs for this parameter if we only allow one value. The only benefit of doing it this way is if one doesn’t want to silently overwrite a previous setting value with a following one I guess?

ryanofsky commented at 7:26 pm on December 19, 2024:

re: #30529 (review)

nit: Just seems plain wrong to use GetArgs for this parameter if we only allow one value. The only benefit of doing it this way is if one doesn’t want to silently overwrite a previous setting value with a following one I guess?

Yes that goes back to introduction of this code in e8990f121405af8cd539b904ef082439261e6c93 from #18267. I don’t think this is ideal, but I could also understand allow not wanting to allow certain options to be specified multiple times, and ArgsManager doesn’t provide another way of doing that.

hodlinator commented at 1:52 pm on December 16, 2024: contributor

Code reviewed 54ad580775b8379a62b05b1f2cdf8b7fde993306

Commit message 594acdb8e23509350a5d3a9db18b76823ca5bce3

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

It strikes me that maybe someone found the old behavior of disabling all networks though negating this arg useful. One cannot disable all networks by just passing -onlynet without a value since it results in:

0Error: Unknown network specified in -onlynet: ''

But maybe -noonlynet is counter-intuitive enough for disabling all networks that your change is justified all the same.

Fix nonsensical -noasmap behavior

Instead of failing with "fread failed: iostream error" error when -noasmap is
specified, just don't load an asmap file.

43b73e3ed4

Fix nonsensical bitcoin-cli -norpcwallet behavior

Treat specifying -norpcwallet the same as not specifying any -rpcwallet option,
instead of treating it like -rpcwallet=0 with 0 as the wallet name.

This restores previous behavior before 743077544b5420246ef29e0b708c90e3a8dfeeb6
from https://github.com/bitcoin/bitcoin/pull/18594, which inadvertently changed
it.

2e993f33fe

Normalize inconsistent -noonlynet behavior

Treat specifying -noonlynet the same as not specifying -onlynet, instead of
marking all networks unreachable.

Before this change, specifying -noonlynet cleared list of reachable networks
and did not allow connecting to any network. It was basically an undocumented
synonym for -noconnect.

After this change, specifying -nononlynet just clears previously specifed
-onlynet options and allows connecting to all networks, restoring default
behavior as if no -onlynet options were specified.

Before this change, there was no way to restore default behavior once an
-onlynet option was specified. So for example, if a config file specifed
onlynet settings, they couldn't be reset on the command line without disabling
the entire config file.

The previous -noonlynet behavior wasn't neccessarily bad, but it was
undocumented, redundant with the -noconnect option, inconsistent with behavior
of other list options, and inconsistent with being able to use the command line
to selectively override config options. It was also probably unintended,
arising from use of the IsArgSet() method and its interaction with negated
options.

61342af6a2

Normalize inconsistent -noexternalip behavior

Treat specifying -noexternalip the same as not specifying -externalip, instead
of causing it to soft-set the -discover default to false.

Before this change, was -noexternalip basically an undocumented synonym for
-nodiscover.

After this change, specifying -noexternalip just clears previously specifed
-externalip options, restoring default behavior as if they were not were
specified.

The previous -noexternalip behavior wasn't neccessarily bad, but it was
undocumented, redundant with the -nodiscover option, and inconsistent with
behavior of other list options.

b445bd1059

refactor: Avoid using IsArgSet() on -debug, -loglevel, and -vbparams list options

This commit does not change behavior, it just drops unnecessary IsArgSet()
calls for -debug, -loglevel, and -vbparams options. The calls are unnecessary
because GetArgs() already returns empty arrays if these arguments are not
specified.

This change also fixes a comment about -debug=none handling that mistakenly
references -nodebug.

76fc51774e

refactor: Avoid using IsArgSet() on -signetseednode and -signetchallenge list options

This commit does not change behavior because negation of -signetseednode and
-signetchallenge parameters has been disallowed since these were introduced in
#18267, so calling IsArgSet() is equivalent to checking if GetArgs() returns a
non-empty list.

d392f493b7

test: Add test to make sure -noconnect disables -dnsseed and -listen by default

Make sure -noconnect has same effect as -connect for disabling DNS seeding and
listening by default, and warning about -dnsseed being ignored with the -proxy
setting.

Initial implementation of https://github.com/bitcoin/bitcoin/pull/30529
accidentally broke this behavior, so having coverage may be useful to make sure
it does not break again.

Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

114d92a8d9

refactor: Avoid using IsArgSet() on -connect list option

This commit does not change behavior, it just changes code to handle -noconnect
values explicitly with IsArgNegated() instead of implicitly with IsArgSet(),
and adds comments to make it clear what behavior is intended when -noconnect is
specified.

283e5c3c79

doc: Add release notes summarizing negated option behavior changes. 5c4e6b15c9

ryanofsky referenced this in commit 7a6fc7728b on Dec 19, 2024

ryanofsky force-pushed on Dec 19, 2024

ryanofsky commented at 8:05 pm on December 19, 2024: contributor

Thanks for the new review!

Updated 54ad580775b8379a62b05b1f2cdf8b7fde993306 -> 80608ba5d282ae8713ea0136ea9a0208b254c082 (pr/listset.5 -> pr/listset.6, compare) improving various comments and adding a new commit to handle -noasmap based on Martin’s suggestion #30529#pullrequestreview-2400135259.

re: #30529#pullrequestreview-2400135259

Does this PR have the aspiration to fix all instances where it makes sense?

If so, I think that the pattern from 40ecf91 could also be applied to -port and -rpcport in CheckHostPortOptions() or -asmap, which all fail init in master when negated and which could be used to negate earlier options if changed.

Looked into each of these (-noport -norpcport and -asmap) and I think all could be improved but the port options would be difficult to fix in a clean way without #31260 because of their interactions with bind options. I added a new commit to this PR to fix -noasmap behavior, and listed other ideas for improving these options below.

Currently -noasmap fails with error “fread failed: iostream error” because it tries to open an empty file path. I added a new commit to fix this and make -noasmap just not load a file. (Separately I think there are other problems with the -asmap option implemention. The -asmap option is documented have a default value of ip_asn.map but this is not accurate, because no asmap file is actually loaded by default, and if an ip_asn.map file happens to exist it is just ignored. In order to cause an ASN map to be loaded you have to explicitly pass -asmap. I don’t think it makes sense to have a default asmap path if it will not actually be loaded by default. Better behavior would be to treat ip_asn.map like bitcoin.conf and just load it by default as long as it exists and another path is not specified.)
Currently -norpcport is treated like -rpcport=0 and always results in “Invalid port specified in -rpcport: ‘0’” errors (whether or not it is specified in combination with -rpcbind). I think more ideally, -norpcport would not be an error, it would just be accept and stop the RPC server binding to a port. Additionally, I don’t think current behavior of -norpcbind causing the RPC server to bind to localhost makes sense. Based on behavior of -noconnect and the meaning of the word “no” I would expect -norpcbind to cause RPC server not to bind to an address. Better behavior would be easier to implement in a clean way with #31260 so settings could be given explicit types. It would make sense for the -rpcport setting to have type std::optional<uint16_t> and for the -rpcbind setting to have type std::optional<std::vector<std::string>>, and then the following code could be implemented
```
 0// -rpcport is a more concise but less flexible alternative to -rpcbind, and
 1// -rpcbind takes precedence if the options conflict. Negation is allowed, with
 2// -norpcbind or -norpcport causing the RPC server not to bind to an address.
 3const auto rpc_port = settings::RpcPort::Get(args);
 4const auto rpc_bind = settings::RpcBind::Get(args);
 5const uint16_t default_rpc_port = rpc_port && *rpc_port ? *rpc_port : base_params.RPCPort();
 6std::vector<std::pair<std::string, uint16_t>> endpoints;
 7if (rpc_bind) {
 8   for(const auto std::string& addr : *rpc_bind) {
 9      endpoints.emplace_back(SplitHostPort(addr, default_rpc_port));
10   }
11} else if (rpc_port != 0) {
12    endpoints.emplace_back("::1", default_rpc_port);
13    endpoints.emplace_back("127.0.0.1", default_rpc_port);
14}
```
Currently -noport is just treated as -port=0 and always results in “Invalid port specified in -port: ‘0’”. Also currently -nobind binds to “0.0.0.0” instead of not binding to an address. IMO it would make more sense for -noport or -nobind to disable binding, in the same way suggested for -rpcport and `-rpcbind above.

re: #30529#pullrequestreview-2505673099

It strikes me that maybe someone found the old behavior of disabling all networks though negating this arg useful.

I think if someone wanted to not connect to any network they would use -noconnect instead of -noonlynet. I’m not sure how someone would actually discover that -noonlynet disables network connections, since I’d think a more logical expectation would be that -onlynet disables networks so -noonlynet re-enables them.

in doc/release-notes-30529.md:4 in 80608ba5d2 outdated

0@@ -0,0 +1,4 @@
1+Configuration
2+-------------
3+
4+Some corner cases handling negated list options `-norpcwhitelist`, `-norpcallowip`, `-norpcbind`, `-nobind`, `-nowhitebind`, `-noexternalip`, `-noonlynet`, `-noseednode`, `-nosignetchallenge`, `-nosignetseednode`, `-notest`, `-norpcwallet`, and `-no have been fixed. Now negating these options is the same as not specifying them.

hodlinator commented at 12:40 pm on December 20, 2024:

Typo:

0`-norpcwallet`, and `-no have

Also not sure this should be included in this sentence assuming it is -asmap since it was not used as a list arg?

ryanofsky commented at 5:19 pm on January 10, 2025:

re: #30529 (review)

Thanks, good catches. All of this should be fixed now.

hodlinator commented at 12:52 pm on December 20, 2024: contributor

Code reviewed 80608ba5d282ae8713ea0136ea9a0208b254c082

git range-diff master 54ad580 80608ba

in doc/release-notes-30529.md:4 in b8c83c3a90 outdated

0@@ -0,0 +1,4 @@
1+Configuration
2+-------------
3+
4+Some corner cases handling negated list options `-norpcwhitelist`, `-norpcallowip`, `-norpcbind`, `-nobind`, `-nowhitebind`, `-noexternalip`, `-noonlynet`, `-noseednode`, `-nosignetchallenge`, `-nosignetseednode`, and `-notest` have been fixed. Now negating these options is the same as not specifying them.

danielabrozzoni commented at 5:20 pm on January 2, 2025:

In b8c83c3a90452a35f63daac70d7cdf3ad0f2e280:

The commit message and release notes say that there are corner cases being fixed in -nosignetseednode and -nosignetchallenge, but I can’t seem to reproduce them, as the code disallows the two options:

0❯ ./build/src/bitcoind -nosignetchallenge
1Error: Error parsing command line arguments: Negating of -signetchallenge is meaningless and therefore forbidden
2                                                                                                                                            
3❯ ./build/src/bitcoind -nosignetseednode 
4Error: Error parsing command line arguments: Negating of -signetseednode is meaningless and therefore forbidden

Am I doing something wrong? If that’s not the case, can you specify in the commit message that for those two calls the behavior is not impacted, but the code is refactored for clarity?

ryanofsky commented at 5:19 pm on January 10, 2025:

re: #30529 (review)

Am I doing something wrong?

No, thanks for testing and this is a good catch. The commit message was describing what the change in behavior would have been if negating these options was allowed. But since these options aren’t allowed to be negated their behavior isn’t changing here. I updated the commit message and release notes and PR description to reflect this now. Thanks for catching it.

danielabrozzoni commented at 5:44 pm on January 2, 2025: contributor

Concept ACK - I still haven’t reviewed every commit, but this seems a nice cleanup overall.

doc: Add some general documentation about negated options eb325bc0ef

ryanofsky referenced this in commit b4e6f76739 on Jan 10, 2025

ryanofsky force-pushed on Jan 10, 2025

ryanofsky referenced this in commit dcdbfe7afc on Jan 10, 2025

ryanofsky force-pushed on Jan 10, 2025

ryanofsky commented at 6:28 pm on January 10, 2025: contributor

Note to previous reviewers: I broke up and rearranged all the commits here and rewrote most of the descriptions to make the purpose of the PR clearer, but there are no significant code changes since the last push (just a small refactoring in the -onlynet code and a suggested test simplification).

Updated 80608ba5d282ae8713ea0136ea9a0208b254c082 -> d4b02f04c8960f9d1fa9ff69977b21eccb104d41 (pr/listset.6 -> pr/listset.7, compare) rearranging commits and improving documentation Updated d4b02f04c8960f9d1fa9ff69977b21eccb104d41 -> a0941aaa0336440458ee4343118977d3989943eb (pr/listset.7 -> pr/listset.8, compare) fixing typos and making a suggested test simplification. Updated a0941aaa0336440458ee4343118977d3989943eb -> eb325bc0efd3f999189e155ba836be92e6cc9af7 (pr/listset.8 -> pr/listset.9, compare) simplifying bitcoin-cli code and adding a test and making other small cleanups

in doc/bitcoin-conf.md:68 in a0941aaa03 outdated

63+
64+Almost all options can be negated by being specified with a `no` prefix. For example an option `-foo` could be negated by writing `nofoo=1` or `nofoo=` in the configuration file or `-nofoo=1` or `-nofoo` on the command line.
65+
66+In general, negating an option is like setting it to `0` if it is a boolean or integer option, and setting it to an empty string or path or list if it is a string or path or list option.
67+
68+However, there are exceptions to this general rule. For example, it is an error to negate some options (e.g. `-nodatadir` is disallowed), and some negated strings are treated like `"0"` instead of `""` (e.g. `-noproxy` is treated like `-proxy=0`), and some negating some lists can have side effects in addition to clearing the lists (e.g. `-noconnect` disables automatic connections in addition to dropping any manual connections specified previously with `-connect=<host>`). When there are exceptions to the rule, they should either be obvious from context, or should be mentioned in usage documentation. Nonobvious, undocumented exceptions should be reported as bugs.

hodlinator commented at 10:37 am on January 13, 2025:

Typo

0However, there are exceptions to this general rule. For example, it is an error to negate some options (e.g. `-nodatadir` is disallowed), and some negated strings are treated like `"0"` instead of `""` (e.g. `-noproxy` is treated like `-proxy=0`), and negating some lists can have side effects in addition to clearing the lists (e.g. `-noconnect` disables automatic connections in addition to dropping any manual connections specified previously with `-connect=<host>`). When there are exceptions to the rule, they should either be obvious from context, or should be mentioned in usage documentation. Nonobvious, undocumented exceptions should be reported as bugs.

ryanofsky force-pushed on Jan 13, 2025

DrahtBot added the label CI failed on Jan 13, 2025

DrahtBot removed the label CI failed on Jan 13, 2025

hodlinator approved

hodlinator commented at 9:55 pm on January 13, 2025: contributor

ACK eb325bc0efd3f999189e155ba836be92e6cc9af7

Nice break up of commits for correcting each -arg.

In my case I still reviewed mostly through:

0₿ git diff 2eccb8bc5e2245e42e05b8df3d9aa264b5f21a9a 80608ba5d282ae8713ea0136ea9a0208b254c082 > old
1₿ git diff 2eccb8bc5e2245e42e05b8df3d9aa264b5f21a9a eb325bc0efd3f999189e155ba836be92e6cc9af7 > new
2₿ meld old new &

feature_config_args.py - Nice that we are no longer relying on “leaked” connect_arg from prior for-loop, instead modifying the loop body now. Thanks for taking my feedback into account.

Good additions to bitcoin-conf.md.

Good to see added -norpcwallet test and RpcWalletName()-function.

Passed unit & functional tests locally.

DrahtBot requested review from danielabrozzoni on Jan 13, 2025

DrahtBot requested review from mzumsande on Jan 13, 2025

DrahtBot requested review from naiyoma on Jan 13, 2025

DrahtBot requested review from stratospher on Jan 13, 2025

danielabrozzoni approved

danielabrozzoni commented at 9:21 pm on January 16, 2025: contributor

tACK eb325bc0efd3f999189e155ba836be92e6cc9af7 - I reviewed the code and manually verified the behavior of the affected RPC arguments, both old and new.

Thank you for splitting the changes into several separate commits; it made the review process much easier! :)

Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior #30529

Code Coverage & Benchmarks

Reviews

Conflicts

Commit message 594acdb8e23509350a5d3a9db18b76823ca5bce3