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

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

This PR drops all calls to ArgsManager::IsArgSet() on list arguments that are retrieved with ArgsManager::GetArgs().

Most of these calls are buggy because they cause negated lists to be treated differently than empty lists, leading to a variety of nonsensical behaviors for -norpcwhitelist, -norpcallowip, -norpcbind, -nobind, -nowhitebind, -noexternalip, -noonlynet, -noseednode, -nosignetchallenge, -nosignetseednode, -notest, and -norpcwallet options which are described in detail in commit messages. The remaining calls which are not buggy are redundant, and are removed here with no changes in behavior.

The first 3 commits just clean up code without changing behavior: adding tests and documentation and making checks for negated options explicit.

The last 2 commits change handling of negated options to avoid nonsensical behavior.

These changes were originally part of #17783, which adds enforcement that IsArgSet() cannot be called on list options, so similar problems do not happen in the future. But progress on #17783 was slower than expected, so it seems good to improve current behavior and documentation while stricter checking is being worked on in #17783.

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
Concept NACK	maflcko
Concept ACK	stratospher, hodlinator, 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:

#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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:416 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:1956 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:1960 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:1954 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:333 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

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.

7a6fc7728b

refactor: Clarify handling of -noconnect option

Handle -noconnect setting explicity with IsArgNegated() function instead of
implicitly with IsArgSet() and add comments so it is clearer what the code is
trying to do when -noconnect is specified.

This commit is a refactoring does not change behavior. Test coverage for this
behavior was added in the previous commit.

82af7e092a

refactor: Clarify handling of -nodebug option

Drop unnecessary IsArgSet calls for -debug, -loglevel, and -vbparams options
and fix inaccurate comment about -nodebug.

This commit is a refactoring and does not change behavior. It is not necessary
to check IsArgSet before calling GetArgs, because if IsArgSet returns false
GetArgs just returns an empty vector.

55c361657a

Fix nonsensical -norpcwhitelist, -norpcallowip and related behavior

This change fixes some corner cases handling negated list options:
-norpcwhitelist, -norpcallowip, -norpcbind, -nobind, -nowhitebind,
-noexternalip, -noonlynet, -noseednode, -nosignetchallenge, -nosignetseednode,
and -notest.

Negating list options on the command line is 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 cause
side effects interacting with other parameters (listed below). Now, negating
these options behaves the same as not setting them, and negated lists are
treated the same as empty lists.

The code change in this commit is just to avoid using IsArgSet() and GetArgs()
together on the same options. Using IsArgSet() and GetArgs() together
frequently leads to bugs because it overlooks the case where an argument is
negated and IsArgSet() returns true while GetArgs() returns an empty list. It
almost always makes sense to call GetArgs("-option").empty() instead
IsArgSet("-option") for list options that are allowed to be called multiple
times.

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, and -nowhitebind exactly the same as not
  specifying -bind and -whitebind values instead of causing them to soft-set
  -listen=1.

- 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 -noseednode exactly the same as not specifying any
   -seednode value, instead of enabling seed node timeout and log messages

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

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

b8c83c3a90

Fix nonsensical bitcoin-cli -norpcwallet behavior

Treat specifying -norpcwallet exactly 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.

8709dc0879

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.

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.

80608ba5d2

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

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?

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

Code reviewed 80608ba5d282ae8713ea0136ea9a0208b254c082

git range-diff master 54ad580 80608ba

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

Code Coverage & Benchmarks

Reviews

Conflicts

Commit message 594acdb8e23509350a5d3a9db18b76823ca5bce3