options: flip listenonion to false if not listening #568

pull vasild wants to merge 1 commits into bitcoin-core:master from vasild:fListen changing 1 files +4 −1
  1. vasild commented at 10:36 am on March 23, 2022: contributor

    If the user has unchecked “Allow incoming connections” in Settings->Options...->Network then fListen=false is saved in ~/.config/Bitcoin/Bitcoin-Qt.conf. This flips -listen to false during startup, but leaves -listenonion to true.

    This flipping of -listen is done in OptionsModel::Init() after InitParameterInteraction() has been executed which would have flipped -listenonion, should it have seen -listen being false (this is a difference between bitcoind and bitcoin-qt).

    Fixes: https://github.com/bitcoin-core/gui/issues/567

  2. options: flip listenonion to false if not listening 
    If the user has unchecked "Allow incoming connections" in
`Settings->Options...->Network` then `fListen=false` is saved in
`~/.config/Bitcoin/Bitcoin-Qt.conf`. This flips `-listen` to `false`
during startup, but leaves `-listenonion` to `true`.

This flipping of `-listen` is done in `OptionsModel::Init()` after
`InitParameterInteraction()` has been executed which would have flipped
`-listenonion`, should it have seen `-listen` being `false`
(this is a difference between `bitcoind` and `bitcoin-qt`).

Fixes: https://github.com/bitcoin-core/gui/issues/567
    7f90dc26c8
  3. vasild cross-referenced this on Mar 23, 2022 from issue Revert "net: do not advertise address where nobody is listening" by hebasto
  4. vasild commented at 10:39 am on March 23, 2022: contributor
    Another approach would be to execute OptionsModel::Init() (which translates GUI options from ~/.config/Bitcoin/Bitcoin-Qt.conf to the “normal” config settings) before InitParameterInteraction(). That would be bigger and more intrusive change.
  5. hebasto commented at 10:50 am on March 23, 2022: member
    Concept ACK, considering (a) the short-term goal to backport this fix into 23.0rc3, and (b) non-supportive comments on bitcoin/bitcoin#24648.
  6. hebasto added this to the milestone 23.0 on Mar 23, 2022
  7. hebasto added the label UX on Mar 23, 2022
  8. hebasto added the label Needs backport (23.x) on Mar 23, 2022
  9. hebasto cross-referenced this on Mar 23, 2022 from issue Unable to open bitcoin-qt when incoming connections disabled in settings by ghost
  10. shaavan commented at 11:20 am on March 23, 2022: contributor

    Concept ACK

    • This fix addresses the issue and seems to correct it without removing a previously added feature.
    • As far as I understood the problem, the issue was because the setting of -listen to 0 depended on fListen, while the setting of -listenonion to 0 depending on -listen. And both of these variables were being set at different times during the startup, leading to incorrect values for them.
    • This PR fixes the problem by adding the code to set listenonion to 0 if fListen is false, consequently preventing the bug due to setting the value of listen and listenonion at different times in the startup.

    Let me test if this PR fixes the bug in question.

  11. hebasto commented at 11:31 am on March 23, 2022: member
    Tested 7f90dc26c8938f348938929b6d8bf1ea6f149209 on Ubuntu 22.04, the bitcoin-core/gui#567 has been fixed.
  12. hebasto commented at 11:57 am on March 23, 2022: member
    FWIW, bitcoin/bitcoin#15936 also fixes #567. And in long term I do prefer @ryanofsky’s PR.
  13. hebasto approved
  14. hebasto commented at 12:01 pm on March 23, 2022: member
    ACK 7f90dc26c8938f348938929b6d8bf1ea6f149209
  15. unknown commented at 12:11 pm on March 23, 2022: none

    It fixes the issue and bitcoin-qt starts without any error when incoming connections are disabled.

    image

    Not sure why second checkbox is disabled when running pull request branch: ‘Map port using NAT-PMP’

  16. jonatack commented at 12:18 pm on March 23, 2022: contributor
    utACK 7f90dc26c8938f348938929b6d8bf1ea6f149209
  17. ghost commented at 12:29 pm on March 23, 2022: none

    When I run RC2 binary with bitcoin-qt -listenonion=0, second checkbox for NAT-PMP is enabled:

    image

  18. hebasto commented at 1:03 pm on March 23, 2022: member

    @1440000bytes

    Not sure why second checkbox is disabled when running pull request branch: ‘Map port using NAT-PMP’

    When configuring the build of this PR, what is your output:

     0Options used to compile and link:
 1  external signer = yes
 2  multiprocess    = no
 3  with experimental syscall sandbox support = yes
 4  with libs       = yes
 5  with wallet     = yes
 6    with sqlite   = yes
 7    with bdb      = yes
 8  with gui / qt   = yes
 9    with qr       = yes
10  with zmq        = yes
11  with test       = yes
12  with fuzz binary = yes
13  with bench      = yes
14  with upnp       = yes
15  with natpmp     = yes
16...

    ?

  19. laanwj commented at 1:19 pm on March 23, 2022: member
    Concept ACK on disabling Tor listening when listening is disabled, this would be the least surprise. I don’t know enough about the forest of interacting options and Qt initialization to know if this is the clearest solution, but I think it will work.
  20. in src/qt/optionsmodel.cpp:157 in 7f90dc26c8 
    150@@ -151,8 +151,11 @@ void OptionsModel::Init(bool resetSettings)
151 
152     if (!settings.contains("fListen"))
153         settings.setValue("fListen", DEFAULT_LISTEN);
154-    if (!gArgs.SoftSetBoolArg("-listen", settings.value("fListen").toBool()))
155+    if (!gArgs.SoftSetBoolArg("-listen", settings.value("fListen").toBool())) {
156         addOverriddenOption("-listen");
157+    } else if (!settings.value("fListen").toBool()) {
158+        gArgs.SoftSetBoolArg("-listenonion", false);
    ryanofsky commented at 2:58 pm on March 23, 2022:

    In commit “options: flip listenonion to false if not listening” (7f90dc26c8938f348938929b6d8bf1ea6f149209)

    Would be good to add a comment here explaining this like “Handle interaction between -listen and -listenonion settings. This duplicates logic in InitParameterInteraction(), but is required because InitParameterInteraction() has already run before this point and will not be called again. Longer term fix will move these settings to settings.json, so normal parameter interaction will apply.”

    vasild commented at 4:24 pm on March 23, 2022:
    Right! I will look into adding the unit test and add a comment here as a followup.
    vasild commented at 5:45 pm on March 24, 2022:
    Added a test and comment in https://github.com/bitcoin-core/gui/pull/569
  21. ryanofsky approved
  22. ryanofsky commented at 3:16 pm on March 23, 2022: contributor

    Code review ACK 7f90dc26c8938f348938929b6d8bf1ea6f149209.

    Nice simple fix! Agree fixing this in the GUI is better than changing bitcoind behavior to enable listening.

    I think it might be possible to write a simple unit test for this bug in https://github.com/bitcoin/bitcoin/blob/master/src/qt/test/optiontests.cpp by setting the bad listen value, initializing the options model, and then calling AppInitParameterInteraction to trigger the “Cannot set -listen=0 together with -listenonion=1” error. I think it would be good to add the unit test because it seems to me that https://github.com/bitcoin/bitcoin/pull/15936 only partially fixes this bug and could cause a regression after this PR.

  23. mzumsande commented at 4:04 pm on March 23, 2022: contributor
    Tested ACK 7f90dc26c8938f348938929b6d8bf1ea6f149209 I verified locally that the problem occurs and is fixed by this patch. Also tried to find if any other parameter interactions might be affected by the same kind of problem, but couldn’t spot any.
  24. hebasto merged this on Mar 23, 2022
  25. hebasto closed this on Mar 23, 2022
  26. ghost commented at 4:23 pm on March 23, 2022: none

    @1440000bytes

    Not sure why second checkbox is disabled when running pull request branch: ‘Map port using NAT-PMP’

    When configuring the build of this PR, what is your output:

    @hebasto I went out after testing this pull request, don’t know how to check this and nothing can be confirmed with make or make install however I trust you and other reviewers in this pull request so I agree with the changes. Maybe @jonatack could add some information as I followed https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests to compile.

    Thanks @hebasto @vasild for fixing this issue :)

  27. hebasto referenced this in commit 70f2c579b9 on Mar 23, 2022
  28. hebasto cross-referenced this on Mar 23, 2022 from issue [23.x] GUI backports by hebasto
  29. hebasto commented at 4:29 pm on March 23, 2022: member
    Backported to 23.x in bitcoin/bitcoin#24596.
  30. hebasto removed the label Needs backport (23.x) on Mar 23, 2022
  31. vasild deleted the branch on Mar 23, 2022
  32. hebasto commented at 4:52 pm on March 23, 2022: member

    @1440000bytes

    Maybe @jonatack could add some information as I followed https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests to compile.

    Yeah, a package list in the guide is a bit outdated. Use https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md directly.

  33. ryanofsky cross-referenced this on Mar 23, 2022 from issue interfaces: Expose settings.json methods to GUI by ryanofsky
  34. sidhujag referenced this in commit aece612bba on Mar 24, 2022
  35. maflcko referenced this in commit 7d03cf632d on Mar 24, 2022
  36. jonatack commented at 10:51 am on March 24, 2022: contributor
    @1440000bytes The guide didn’t include some of the optional dependencies and encouraged readers to check the dependencies in doc/build-unix.md / doc/build-osx.md to match their needs. I’ve updated the guide now to include all optional dependencies.
  37. vasild referenced this in commit a39e6c173f on Mar 24, 2022
  38. vasild referenced this in commit 66fe59c414 on Mar 24, 2022
  39. vasild cross-referenced this on Mar 24, 2022 from issue test: add regression test for #567 by vasild
  40. fujicoin referenced this in commit 8699b26eb7 on Mar 25, 2022
  41. hebasto cross-referenced this on Mar 26, 2022 from issue v23.0 testing by laanwj
  42. vasild referenced this in commit 3b82608dd1 on Mar 31, 2022
  43. hebasto referenced this in commit 47bac475f0 on Apr 4, 2022
  44. janus referenced this in commit 08904e2903 on Aug 4, 2022
  45. backpacker69 referenced this in commit d65682a070 on Jan 18, 2023
  46. bitcoin-core locked this on Mar 24, 2023
  47. hebasto commented at 5:01 pm on February 24, 2024: member
    Unfortunately, the bug is not fixed on Windows. See #567 (comment).


vasild hebasto shaavan jonatack ghost laanwj ryanofsky mzumsande

Labels
UX

Milestone
23.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-24 11:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me