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

pull ryanofsky wants to merge 5 commits into bitcoin:master from ryanofsky:pr/listset changing 11 files +73 −38
  1. 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.

  2. 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
    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)
    • #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.

  3. 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?
  4. 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, …

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

  6. 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
  7. 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
  8. 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
  9. 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
  10. 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
  11. ryanofsky referenced this in commit ea7dbfba8a on Jul 29, 2024
  12. ryanofsky force-pushed on Jul 29, 2024
  13. 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
  14. ryanofsky force-pushed on Jul 29, 2024
  15. ryanofsky referenced this in commit 77ecca94a0 on Aug 2, 2024
  16. 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

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

  18. ryanofsky referenced this in commit dd0ad55a4e on Aug 23, 2024
  19. 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.

  20. 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.
  21. ryanofsky force-pushed on Aug 26, 2024
  22. ryanofsky commented at 7:05 pm on August 26, 2024: contributor
    Updated 10770c164485324af8acdd3b932b51d32862c41f -> a1c9d23ff587b84d37175b2993ea6760f9762177 (pr/listset.3 -> pr/listset.4, compare) with suggested changes
  23. 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.
  24. 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.

  25. DrahtBot added the label CI failed on Oct 13, 2024
  26. achow101 commented at 3:49 pm on October 15, 2024: member
  27. achow101 requested review from mzumsande on Oct 15, 2024
  28. maflcko commented at 3:26 pm on October 16, 2024: member
    Could rebase for fresh CI?

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-18 04:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me