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

jadijadi commented at 8:38 AM on December 26, 2020: contributor

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.

fanquake added the label P2P on Dec 26, 2020

in src/init.cpp:511 in 86e5167014 outdated

 507 | @@ -508,6 +508,7 @@ void SetupServerArgs(NodeContext& node)
 508 |      argsman.AddArg("-checkmempool=<n>", strprintf("Run checks every <n> transactions (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 509 |      argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 510 |      argsman.AddArg("-deprecatedrpc=<method>", "Allows deprecated RPC method(s) to be used", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 511 | +    argsman.AddArg("-dropmessagestest=<n>", "Randomly drop 1 of every <n> network messages", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

prusnak commented at 11:39 AM on December 26, 2020:

This line does not look like it belongs to this PR

jadijadi commented at 11:50 AM on December 26, 2020:

no. what is this? my mistake on copy paste from another check. sorry.

jadijadi commented at 12:05 PM on December 26, 2020:

removed this line

amitiuttarwar commented at 5:51 PM on December 26, 2020: contributor

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 force-pushed on Dec 27, 2020

jadijadi commented at 6:55 AM on December 27, 2020: contributor

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 commented at 11:32 PM on December 27, 2020: contributor

Concept ACK.

This would fix https://github.com/bitcoin/bitcoin/issues/20657

in src/init.cpp:831 in beb7f1dde0 outdated

 826 | @@ -827,8 +827,10 @@ void InitParameterInteraction(ArgsManager& args)
 827 |              LogPrintf("%s: parameter interaction: -listen=0 -> setting -upnp=0\n", __func__);
 828 |          if (args.SoftSetBoolArg("-discover", false))
 829 |              LogPrintf("%s: parameter interaction: -listen=0 -> setting -discover=0\n", __func__);
 830 | -        if (args.SoftSetBoolArg("-listenonion", false))
 831 | +        if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
 832 | +            args.ForceSetArg("-listenonion", std::string("0"));

vasild commented at 10:56 AM on December 28, 2020:

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?

Rspigler commented at 11:07 PM on December 30, 2020:

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

jadijadi commented at 7:38 AM on December 31, 2020:

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

jadijadi commented at 4:02 PM on January 2, 2021:

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.

jadijadi commented at 4:03 PM on January 2, 2021:

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.

in src/init.cpp:859 in beb7f1dde0 outdated

 831 | +        if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
 832 | +            args.ForceSetArg("-listenonion", std::string("0"));
 833 |              LogPrintf("%s: parameter interaction: -listen=0 -> setting -listenonion=0\n", __func__);
 834 | +        }
 835 |      }
 836 |

vasild commented at 11:01 AM on December 28, 2020:

nit on the commit message: I think it is more common to put "fixes #20657" at the bottom of the commit message on a line of its own, rather than inside the first line. But feel free to ignore this.

jadijadi commented at 4:04 PM on March 16, 2021:

moved it to the bottom. thanks for the hint

jadijadi force-pushed on Jan 2, 2021

jadijadi commented at 4:04 PM on January 2, 2021: contributor

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 changes_requested

luke-jr commented at 11:26 PM on January 3, 2021: member

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 commented at 2:18 PM on January 6, 2021: contributor

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 commented at 8:18 PM on January 6, 2021: none

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

vasild commented at 10:25 AM on January 14, 2021: member

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 commented at 4:59 AM on March 4, 2021: member

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

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 #20657

a38137479b

jadijadi force-pushed on Mar 16, 2021

jadijadi commented at 4:09 PM on March 16, 2021: contributor

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 approved

vasild commented at 4:25 PM on March 16, 2021: member

ACK a38137479bf5e25bf65a62e46b81fc43fb3df75c

jarolrod commented at 4:29 PM on March 16, 2021: member

ACK a38137479bf5e25bf65a62e46b81fc43fb3df75c

ghost commented at 12:26 AM on March 17, 2021: none

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:

fanquake requested review from amitiuttarwar on Aug 18, 2021

amitiuttarwar commented at 7:53 PM on August 18, 2021: contributor

ACK a38137479bf5e25bf65a62e46b81fc43fb3df75c

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 (https://github.com/amitiuttarwar/bitcoin/commit/ec622d153d777bf556ac8f2d74a353bd8f324294) 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.

fanquake requested review from ajtowns on Aug 19, 2021

vasild commented at 10:28 AM on August 20, 2021: member

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

MarcoFalke merged this on Aug 23, 2021

MarcoFalke closed this on Aug 23, 2021

jadijadi deleted the branch on Aug 23, 2021

sidhujag referenced this in commit c40c657ac9 on Aug 23, 2021

hebasto commented at 6:43 PM on March 22, 2022: member

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

DrahtBot locked this on Mar 22, 2023