net: fixes #20657 - Advertised address where nobody is listening #20769

jadijadi · 2020-12-26T08:38:00Z

If the bitcoind starts when listen=0 but listenonion=1, the daemon will
advertise its onion address but nothing is listening for it.

This update will enforce listenonion=0 when the listen is 0.

src/init.cpp

amitiuttarwar · 2020-12-26T17:51:42Z

hi @jadijadi, welcome to the project!

if a PR gets merged, the commits get preserved in the history on master. here, it looks like the first commit introduces an extraneous change that gets removed in the second commit. please squash them so there are just relevant changes.

jadijadi · 2020-12-27T06:55:27Z

hi @jadijadi, welcome to the project!

if a PR gets merged, the commits get preserved in the history on master. here, it looks like the first commit introduces an extraneous change that gets removed in the second commit. please squash them so there are just relevant changes.

Thanks @amitiuttarwar for the welcome, help and being this patient with newcomers :) squashed them into one.

Rspigler · 2020-12-27T23:32:04Z

Concept ACK.

This would fix #20657

vasild · 2020-12-28T10:56:16Z

src/init.cpp

The rest of InitParameterInteraction() uses SoftSetBoolArg() which I guess is to avoid overriding a value that has been explicitly set by the user. InitParameterInteraction() only flips defaults. I think this behavior is sensible as it minimizes the user's surprise.

There is some other code which refuses to start the application if incompatible options are given - for example -prune=1 -txindex=1 or -bind=addr:port -listen=0.

Given the above, what about refusing to start if -listen=0 -listenonion=1 is explicitly given?

I think refusing to start (with an error message?) over changing what the user has set is better

Agree with you guys. Will update the PR to refuse the start instead of changing the explicitly set parameter.

I think refusing to start (with an error message?) over changing what the user has set is better

I've sent a new PR with this behavior. Refusing to start looks more UNIXish.

The rest of InitParameterInteraction() uses SoftSetBoolArg() which I guess is to avoid overriding a value that has been explicitly set by the user. InitParameterInteraction() only flips defaults. I think this behavior is sensible as it minimizes the user's surprise.

There is some other code which refuses to start the application if incompatible options are given - for example -prune=1 -txindex=1 or -bind=addr:port -listen=0.

Given the above, what about refusing to start if -listen=0 -listenonion=1 is explicitly given?

Thanks for the idea. I've updated the PR with this behavior.

src/init.cpp

jadijadi · 2021-01-02T16:04:29Z

Concept ACK.

This would fix #20657

Thanks for initial ACK. but based on the comment from @vasild I updated the PR for a more UNIXish behavior: refusing to start with incompatible inputs.

luke-jr

Concept ACK, but this needs changes to InitParameterInteraction to handle -connect and -proxy cleanly (perhaps if -listenonion is explicitly set true, soft-set -listen true also, as with -bind at the top?).

jadijadi · 2021-01-06T14:18:23Z

Concept ACK, but this needs changes to InitParameterInteraction to handle -connect and -proxy cleanly (perhaps if -listenonion is explicitly set true, soft-set -listen true also, as with -bind at the top?).

Thanks @luke-jr for the ACK and suggestion. In the initial PR, I was doing this in InitParameterInteraction as you said. But after above discussions, others suggested to do it here and preventing the system from start if the user explicitly set listenionion=1 and listen=0. In all other cases, the system handles the situation properly since the DEFAULT_LISTEN is 1 and DEFAULT_LISTEN_ONION is also 1.

felipsoarez

Concept ACK
I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged

vasild · 2021-01-14T10:25:19Z

b743feca5bcbdf95ac4993c7b371d05ef1324d99 looks good. The commit message and PR description need to be updated because they refer to the previous incarnation of this PR where it enforced listenonion=0.

It works as expected:

$ bitcoind -listen=0
InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0

$ bitcoind -listen=0 -listenonion=1
Error: Cannot set -listen=0 together with -listenonion=1

@luke-jr

this needs changes to InitParameterInteraction to handle -connect and -proxy cleanly (perhaps if -listenonion is explicitly set true, soft-set -listen true also, as with -bind at the top?).

I am not sure I understand. -listen is true by default, so if only bitcoind -listenonion=1 is given, then soft-set -listen to true would be a noop?

jarolrod

ACK b743feca5bcbdf95ac4993c7b371d05ef1324d99, Tested on macOS 11.2

Confirming the bug referenced here on master. This PR creates an InitParameterInteraction to set listenonion=0 when listen=0. This functionality works as intended in my testing.

Setting listen=0

2021-03-04T04:46:56Z InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0

Attempting to set both listen and listenonion

Error: Cannot set -listen=0 together with -listenonion=1

@jadijadi as others have stated you may want to fix the commit message, how about this as a suggestion for the title:

net: do not advertise address where nobody is listening

If the bitcoind starts when listen=0 but listenonion=1, the daemon will advertise its onion address but nothing is listening for it. This update will enforce listenonion=0 when the listen is 0. fixes bitcoin#20657

jadijadi · 2021-03-16T16:09:38Z

ACK b743fec, Tested on macOS 11.2

...
net: do not advertise address where nobody is listening

Thanks for the hints. I've updated the PR with the suggested commit message

vasild

ACK a381374

jarolrod

ACK a381374

ghost · 2021-03-17T00:26:28Z

Compiled successfully for Windows using the instructions mentioned here: https://github.com/bitcoin/bitcoin/blob/master/doc/build-windows.md

1 test failed and this is what I see in logs:

amitiuttarwar

ACK a381374

with several thoughts & questions for further improvements:

the init params are hard to keep track of & dependent on ordering, so it would be good to add a regression test. @jadijadi - I have a draft here (amitiuttarwar@ec622d1) if you'd like to include in this PR or add as a follow-up.
-listen has a lot of parameter interactions, I'm wondering if any other conflicts should similarly cause a failure-to-start instead of quietly failing, eg -discover, i2pacceptincoming... (cc @vasild if you have more insight into some of these params)
also would be cool to add test coverage to make explicit the other parameter interactions. for example, this change also disallows setting -connect=1.1.1.1 with -listenonion=1. that seems reasonable but would be nice to capture these sorts of complexities.

vasild · 2021-08-20T10:28:32Z

* I'm wondering if any other conflicts should similarly cause a failure-to-start instead of quietly failing, eg `-discover`, `i2pacceptincoming`

If -listen=0 is given and -i2pacceptincoming=... is not given, then the default of the latter is flipped from 1 to 0. If both are explicitly set: -listen=0 -i2pacceptincoming=1 then it works as instructed - no binding/listening on local address:port of the machine, but accept incoming I2P connections (they do not need binding/listening, so we don't have a bug like the one with Tor which the current PR aims to resolve).

Is -listen=0 -i2pacceptincoming=1 contradictory? Maybe, to some extent, it can be perceived as such. Should we abort startup if that is given? Maybe, I am not sure. (but if yes, then that is out of the scope of this PR).

this change also disallows setting -connect=1.1.1.1 with -listenonion=1

Right. That can be worked around with -connect=1.1.1.1 -listen=1 -listenonion=1.

…ere nobody is listening a381374 net: do not advertise address where nobody is listening (Jadi) Pull request description: If the bitcoind starts when listen=0 but listenonion=1, the daemon will advertise its onion address but nothing is listening for it. This update will enforce listenonion=0 when the listen is 0. ACKs for top commit: vasild: ACK a381374 jarolrod: ACK a381374 amitiuttarwar: ACK a381374 Tree-SHA512: e84a0a9a51f2217edf35d06c6cd9085d1e664452655ba92027195a1e88ba081d157310c84e9709a99ce5d46c94f231477ca2d36f010648b0c8b4f2a737d54e5d

hebasto · 2022-03-22T18:43:19Z

It seems this PR introduced a bug in the GUI: bitcoin-core/gui#567.

fanquake added the P2P label Dec 26, 2020

jadijadi mentioned this pull request Dec 26, 2020

Advertised address where nobody is listening #20657

Closed

prusnak reviewed Dec 26, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

jadijadi force-pushed the fixes-issue-20657 branch from 2c3c4d2 to beb7f1d Compare December 27, 2020 06:51

vasild reviewed Dec 28, 2020

View reviewed changes

jadijadi force-pushed the fixes-issue-20657 branch from beb7f1d to b743fec Compare January 2, 2021 15:59

luke-jr suggested changes Jan 3, 2021

View reviewed changes

felipsoarez reviewed Jan 6, 2021

View reviewed changes

jarolrod reviewed Mar 4, 2021

View reviewed changes

net: do not advertise address where nobody is listening

a381374

If the bitcoind starts when listen=0 but listenonion=1, the daemon will advertise its onion address but nothing is listening for it. This update will enforce listenonion=0 when the listen is 0. fixes bitcoin#20657

jadijadi force-pushed the fixes-issue-20657 branch from b743fec to a381374 Compare March 16, 2021 16:05

vasild approved these changes Mar 16, 2021

View reviewed changes

jarolrod reviewed Mar 16, 2021

View reviewed changes

Rspigler mentioned this pull request Jun 21, 2021

doc: Improve Tor docs #21157

Closed

fanquake requested a review from amitiuttarwar August 18, 2021 00:27

amitiuttarwar reviewed Aug 18, 2021

View reviewed changes

fanquake requested a review from ajtowns August 19, 2021 00:55

maflcko merged commit 58b559f into bitcoin:master Aug 23, 2021

jadijadi deleted the fixes-issue-20657 branch August 23, 2021 07:09

hebasto mentioned this pull request Mar 22, 2022

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

Open

hebasto mentioned this pull request Mar 23, 2022

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

Closed

bitcoin locked and limited conversation to collaborators Mar 22, 2023

net: fixes #20657 - Advertised address where nobody is listening #20769

net: fixes #20657 - Advertised address where nobody is listening #20769

Uh oh!

Conversation

jadijadi commented Dec 26, 2020

Uh oh!

Uh oh!

amitiuttarwar commented Dec 26, 2020

Uh oh!

jadijadi commented Dec 27, 2020

Uh oh!

Rspigler commented Dec 27, 2020

Uh oh!

vasild Dec 28, 2020

Choose a reason for hiding this comment

Uh oh!

Rspigler Dec 30, 2020

Choose a reason for hiding this comment

Uh oh!

jadijadi Dec 31, 2020

Choose a reason for hiding this comment

Uh oh!

jadijadi Jan 2, 2021

Choose a reason for hiding this comment

Uh oh!

jadijadi Jan 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jadijadi commented Jan 2, 2021

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

jadijadi commented Jan 6, 2021

Uh oh!

felipsoarez left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Jan 14, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

jadijadi commented Mar 16, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 17, 2021

Uh oh!

amitiuttarwar left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Aug 20, 2021

Uh oh!

hebasto commented Mar 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants