options: flip listenonion to false if not listening #568

vasild · 2022-03-23T10:36:41Z

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: #567

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: bitcoin-core#567

vasild · 2022-03-23T10:39:32Z

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.

hebasto · 2022-03-23T10:50:48Z

Concept ACK, considering (a) the short-term goal to backport this fix into 23.0rc3, and (b) non-supportive comments on bitcoin/bitcoin#24648.

shaavan

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.

hebasto

Tested 7f90dc2 on Ubuntu 22.04, the #567 has been fixed.

hebasto · 2022-03-23T11:57:00Z

FWIW, bitcoin/bitcoin#15936 also fixes #567. And in long term I do prefer @ryanofsky's PR.

hebasto

ACK 7f90dc2

ghost

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

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

jonatack

utACK 7f90dc2

ghost · 2022-03-23T12:29:40Z

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

hebasto · 2022-03-23T13:03:07Z

@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:

Options used to compile and link:
  external signer = yes
  multiprocess    = no
  with experimental syscall sandbox support = yes
  with libs       = yes
  with wallet     = yes
    with sqlite   = yes
    with bdb      = yes
  with gui / qt   = yes
    with qr       = yes
  with zmq        = yes
  with test       = yes
  with fuzz binary = yes
  with bench      = yes
  with upnp       = yes
  with natpmp     = yes
...

?

laanwj · 2022-03-23T13:19:11Z

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.

ryanofsky

Code review ACK 7f90dc2.

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 bitcoin/bitcoin#15936 only partially fixes this bug and could cause a regression after this PR.

src/qt/optionsmodel.cpp

mzumsande · 2022-03-23T16:04:13Z

Tested ACK 7f90dc2
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.

ghost · 2022-03-23T16:23:10Z

@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 :)

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: bitcoin-core/gui#567 Github-Pull: bitcoin-core/gui#568 Rebased-From: 7f90dc2

hebasto · 2022-03-23T16:29:35Z

Backported to 23.x in bitcoin/bitcoin#24596.

hebasto · 2022-03-23T16:52:15Z

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

… listening 7f90dc2 options: flip listenonion to false if not listening (Vasil Dimov) Pull request description: 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: bitcoin-core/gui#567 ACKs for top commit: mzumsande: Tested ACK 7f90dc2 hebasto: ACK 7f90dc2 jonatack: utACK 7f90dc2 ryanofsky: Code review ACK 7f90dc2. Tree-SHA512: ff5095096858eae696293dc58d1cd5bd1bb60ef7c5d07d87308a0cf71c67da88cc00b301b550704625f136c4ba3a29905a934a766535a6422fe85d9662299d32

70f2c57 options: flip listenonion to false if not listening (Vasil Dimov) 642f272 gui: restore Send for external signer (Sjors Provoost) 9406946 refactor: helper function signWithExternalSigner() (Sjors Provoost) fc421d4 move-only: helper function to present PSBT (Sjors Provoost) Pull request description: Backports from the GUI repo: - bitcoin-core/gui#555 - bitcoin-core/gui#568 ACKs for top commit: Sjors: utACK 70f2c57 gruve-p: ACK 70f2c57 Tree-SHA512: 883c442f8b789a9d11c949179e4382843cbb979a89a625bef3f481c7070421681d9db2af0e5b2449abca362c8ba05cf61db5893aeb6a9237b02088c2fb71e93e

jonatack · 2022-03-24T10:51:03Z

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

A followup to bitcoin-core/gui#568 Co-authored-by: Jon Atack <[email protected]>

A followup to bitcoin-core#568 Co-authored-by: Jon Atack <[email protected]>

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: bitcoin-core/gui#567 Github-Pull: bitcoin-core/gui#568 Rebased-From: 7f90dc26c8938f348938929b6d8bf1ea6f149209

A followup to bitcoin-core/gui#568 Co-authored-by: Jon Atack <[email protected]>

4d4dca4 test: add regression test for /issues/567 (Vasil Dimov) 3b82608 options: add a comment for -listenonion and dedup a long expression (Vasil Dimov) Pull request description: Add a test that would fail, should #567 resurface. Also, add a comment and dedup a long expression. ACKs for top commit: jarolrod: reACK 4d4dca4 jonatack: ACK 4d4dca4 hebasto: ACK 4d4dca4, tested with reverting changes from #568, and getting an expected test failure. shaavan: ACK 4d4dca4 Tree-SHA512: 59f069bdaa84586bb599e9372f89e4e66a3cafcbf58677fdf913d685c17dfa9c3d5b118829d81021a9a33b4fd8e46d4c7eb68c1dd902cf1c44a41b8e66e2967b

A followup to bitcoin-core/gui#568 Co-authored-by: Jon Atack <[email protected]>

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: bitcoin-core/gui#567 Github-Pull: bitcoin-core/gui#568 Rebased-From: 7f90dc26c8938f348938929b6d8bf1ea6f149209

hebasto · 2024-02-24T17:01:58Z

Unfortunately, the bug is not fixed on Windows. See #567 (comment).

vasild mentioned this pull request Mar 23, 2022

Revert "net: do not advertise address where nobody is listening" bitcoin/bitcoin#24648

Closed

hebasto added this to the 23.0 milestone Mar 23, 2022

hebasto added UX All about "how to get things done" Needs backport (23.x) labels Mar 23, 2022

hebasto mentioned this pull request Mar 23, 2022

Unable to open bitcoin-qt when incoming connections disabled in settings #567

Open

shaavan reviewed Mar 23, 2022

View reviewed changes

hebasto reviewed Mar 23, 2022

View reviewed changes

hebasto approved these changes Mar 23, 2022

View reviewed changes

ghost reviewed Mar 23, 2022

View reviewed changes

jonatack reviewed Mar 23, 2022

View reviewed changes

ryanofsky approved these changes Mar 23, 2022

View reviewed changes

src/qt/optionsmodel.cpp Show resolved Hide resolved

hebasto merged commit f9ed0ae into bitcoin-core:master Mar 23, 2022

hebasto mentioned this pull request Mar 23, 2022

[23.x] GUI backports bitcoin/bitcoin#24596

Merged

hebasto removed the Needs backport (23.x) label Mar 23, 2022

vasild deleted the fListen branch March 23, 2022 16:38

ryanofsky mentioned this pull request Mar 23, 2022

interfaces: Expose settings.json methods to GUI bitcoin/bitcoin#15936

Merged

vasild added a commit to vasild/bitcoin that referenced this pull request Mar 24, 2022

gui: add a comment for -listenonion and dedup a long expression

a39e6c1

A followup to bitcoin-core/gui#568 Co-authored-by: Jon Atack <[email protected]>

vasild added a commit to vasild/bitcoin-core-gui that referenced this pull request Mar 24, 2022

options: add a comment for -listenonion and dedup a long expression

66fe59c

A followup to bitcoin-core#568 Co-authored-by: Jon Atack <[email protected]>

vasild mentioned this pull request Mar 24, 2022

test: add regression test for #567 #569

Merged

hebasto mentioned this pull request Mar 26, 2022

v23.0 testing bitcoin/bitcoin#24501

Closed

5 tasks

vasild added a commit to vasild/bitcoin that referenced this pull request Mar 31, 2022

options: add a comment for -listenonion and dedup a long expression

3b82608

A followup to bitcoin-core/gui#568 Co-authored-by: Jon Atack <[email protected]>

janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Aug 4, 2022

options: add a comment for -listenonion and dedup a long expression

08904e2

A followup to bitcoin-core/gui#568 Co-authored-by: Jon Atack <[email protected]>

bitcoin-core locked and limited conversation to collaborators Mar 24, 2023

options: flip listenonion to false if not listening #568

options: flip listenonion to false if not listening #568

Uh oh!

Conversation

vasild commented Mar 23, 2022

Uh oh!

vasild commented Mar 23, 2022

Uh oh!

hebasto commented Mar 23, 2022

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 23, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 23, 2022

Uh oh!

hebasto commented Mar 23, 2022

Uh oh!

laanwj commented Mar 23, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande commented Mar 23, 2022

Uh oh!

ghost commented Mar 23, 2022

Uh oh!

hebasto commented Mar 23, 2022

Uh oh!

hebasto commented Mar 23, 2022

Uh oh!

jonatack commented Mar 24, 2022

Uh oh!

hebasto commented Feb 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants