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

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

Make negating options the same as not setting them for -norpcwhitelist, -norpcallowip, -norpcbind, -nobind, -nowhitebind, -noexternalip, -noonlynet, -noseednode, -nosignetchallenge, -nosignetseednode, -notest, and -norpcwallet options, instead of triggering confusing side effects and errors, like -norpcwallet trying to make RPC calls to the “0” wallet.

The first 3 commits just clean up code without changing behavior: adding tests and documentation and clarifying code.

The last 2 commits change handling of negated options to avoid nonsensical behavior. The idea is to treat most negated options the same as unspecified options by avoiding or working around IsArgSet() calls which return false for unspecified options but true for negated ones. The specific changes are described in the commit messages.

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

For detailed information about the code coverage, see the test coverage report.

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:

#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

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.

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.

0e98e5b908

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.

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.

668417c38e

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.

9797b3fe0c

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 at all.

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.

40ecf91068

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.

a1c9d23ff5

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:740 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 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:342 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:1936 in a1c9d23ff5

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.

in src/init.cpp:1942 in a1c9d23ff5

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

in src/torcontrol.cpp:406 in a1c9d23ff5

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

in src/init.cpp:1931 in a1c9d23ff5

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        }

in test/functional/feature_config_args.py:333 in a1c9d23ff5

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=")])

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.

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

Code Coverage

Reviews

Conflicts