p2p: set `-dnsseed` and `-listen` false if `maxconnections=0` #26899

brunoerg commented at 4:16 PM on January 16, 2023: contributor

If maxconnections=0, it means our possible connections are going to be manual (e.g via addnode). For this reason, we can skip DNS seeds and set listen false.

DrahtBot commented at 4:16 PM on January 16, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	1440000bytes, achow101, vasild
Concept ACK	ekzyis, stickies-v, stratospher, kristapsk
Stale ACK	willcl-ark

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:

#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

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.

DrahtBot added the label P2P on Jan 16, 2023

willcl-ark commented at 10:11 AM on January 17, 2023: contributor

ACK c5c6116

Current behaviour is that a node run with -maxconnections=0 will query the DNS seeds and fetch addresses, but not make any connections out to them. Testing with a mainnet node saw it fetch ~300 addresses but not make any connections after that.

With this patch the node no longer makes connections to the DNS seeds to fetch addresses. (You may want to amend the wording for the -dnsseed option to explain that this will not apply in this new situation also?)

I think that the new behaviour makes sense as a user specifying an option -maxconnections=0 would not expect their software to begin by, making connections... especially a privacy-focussed software like Bitcoin Core. The only use-case lost would be users who wanted to fetch addresses from DNS seeds using Bitcoin Core, but they can continue to do this (more) easily with dig or nslookup.

brunoerg commented at 11:54 AM on January 17, 2023: contributor

You may want to amend the wording for the -dnsseed option to explain that this will not apply in this new situation also?

Perfect, force-pushed addressing it!

maflcko commented at 12:05 PM on January 17, 2023: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

brunoerg force-pushed on Jan 17, 2023

brunoerg commented at 12:19 PM on January 17, 2023: contributor

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Done.

ekzyis commented at 10:19 PM on January 17, 2023: none

Concept ACK c85b634340727b54e4dc8c6fd02fdc2e06531e31

However, the log messages at line 678 and 680 now may be confusing. It only mentions -connect.

Maybe it should be updated?

brunoerg force-pushed on Jan 18, 2023

brunoerg commented at 8:53 PM on January 18, 2023: contributor

Maybe it should be updated?

Sure, force-pushed doing it!

willcl-ark commented at 9:41 AM on January 23, 2023: contributor

re-ACK 58fab41

in src/init.cpp:476 in 58fab41ab7 outdated

 460 | @@ -461,7 +461,7 @@ void SetupServerArgs(ArgsManager& argsman)
 461 |      argsman.AddArg("-connect=<ip>", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
 462 |      argsman.AddArg("-discover", "Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 463 |      argsman.AddArg("-dns", strprintf("Allow DNS lookups for -addnode, -seednode and -connect (default: %u)", DEFAULT_NAME_LOOKUP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 464 | -    argsman.AddArg("-dnsseed", strprintf("Query for peer addresses via DNS lookup, if low on addresses (default: %u unless -connect used)", DEFAULT_DNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 465 | +    argsman.AddArg("-dnsseed", strprintf("Query for peer addresses via DNS lookup, if low on addresses (default: %u unless -connect used or -maxconnections=0)", DEFAULT_DNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

stickies-v commented at 4:30 PM on January 23, 2023:

I think the same applies to the -listen help?

brunoerg commented at 12:53 PM on January 24, 2023:

Yes, just changed it to "default: 1 if no -proxy, -connect or -maxconnections=0"

stickies-v commented at 4:40 PM on January 23, 2023: contributor

Concept ACK

brunoerg force-pushed on Jan 24, 2023

brunoerg commented at 12:54 PM on January 24, 2023: contributor

Force-pushed changing -listen help (https://github.com/bitcoin/bitcoin/pull/26899#discussion_r1084278913)

stickies-v commented at 2:24 PM on January 25, 2023: contributor

I think this PR warrants a (very short) release note to describe the behaviour change, and that -dnsseed=1 and -listen=1 can be specified to preserve existing behaviour?

The only use-case lost would be users who wanted to fetch addresses from DNS seeds using Bitcoin Core,

Users can still manually set -dnsseed=1 to preserve existing behaviour I think?

in src/init.cpp:485 in 4e9d06bf22 outdated

 482 | +    argsman.AddArg("-dnsseed", strprintf("Query for peer addresses via DNS lookup, if low on addresses (default: %u unless -connect used or -maxconnections=0)", DEFAULT_DNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 483 |      argsman.AddArg("-externalip=<ip>", "Specify your own public address", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 484 |      argsman.AddArg("-fixedseeds", strprintf("Allow fixed seeds if DNS seeds don't provide peers (default: %u)", DEFAULT_FIXEDSEEDS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 485 |      argsman.AddArg("-forcednsseed", strprintf("Always query for peer addresses via DNS lookup (default: %u)", DEFAULT_FORCEDNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 486 | -    argsman.AddArg("-listen", "Accept connections from outside (default: 1 if no -proxy or -connect)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 487 | +    argsman.AddArg("-listen", "Accept connections from outside (default: 1 if no -proxy, -connect or -maxconnections=0)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

ekzyis commented at 2:45 PM on January 25, 2023:

Just asking out of interest and totally unrelated to this MR: Why does this not use string interpolation using a variable DEFAULT_LISTEN like the other arguments?

Historical reasons and no one bothered yet to make this consistent with the other arguments?

ekzyis commented at 2:46 PM on January 25, 2023:

At line 717, I can even see that there exists a variable DEFAULT_LISTEN

stratospher commented at 8:12 AM on March 9, 2023:

Historical reasons and no one bothered yet to make this consistent with the other arguments?

most possible reason :)

ekzyis commented at 9:56 AM on March 9, 2023:

Added a PR if you want to take a look :)

brunoerg commented at 6:06 PM on January 26, 2023: contributor

Users can still manually set -dnsseed=1 to preserve existing behaviour I think?

Yes, they can. I just force-pushed adding a release note to specify this behavior when using -maxconnections=0.

amitiuttarwar commented at 3:40 AM on March 9, 2023: contributor

are there situations where you'd recommend a node operator to use -maxconnections=0 over -connect=0 or -noconnect? I find the current behavior with -maxconnections=0 to be odd how the node just stalls with very little guidance for the user.

I can't think of cases where I'd recommend an operator to set -maxconnections to be less than 8 or 10 for security reasons. If someone was seeking a lower bandwidth environment, I'd recommend disabling automatic connections & having manual trusted connections.

an alternative to consider would be recommend to users trying to set -maxconnections=0 to use -noconnect instead. this means that the code could utilize the existing -connect interactions instead of reproducing them and increasing the complexity of the init.cpp code we have to maintain over time. for example, -connect also has an interaction with -seednode and disables p2p functionality using m_use_addrman_outgoing, should those also be duplicated here?

another option would be to enforce a minimum number of connections to be 8 or 10. this is a stronger stance so I'm not sure how it'd be received, but it would make sense to me to prevent users from configuring insecure settings.

just some thoughts to consider, I would probably opt for a user facing recommendation to use -noconnect instead. this offers a balance between minimizing codebase complexity and taking a conservative approach of what functionality we disable.

stratospher commented at 8:09 AM on March 9, 2023: contributor

Concept ACK. DNS seeding not happening when maxconnections=0 makes sense.

Tested maxconnections=0 meant dns seeding didn't happen. Overriding it with dnsseed=1 preserved the existing behaviour and people might possibly want it for the lost usecase mentioned in #26899(comment)?
Overriding with listen=1 doesn't really make sense though when maxconnections=0 and i did find it confusing to see both options together in the help text.

are there situations where you'd recommend a node operator to use -maxconnections=0 over -connect=0 or -noconnect? I find the current behavior with -maxconnections=0 to be odd how the node just stalls with very little guidance for the user.

maxconnections=0 must have been widely used in earlier clients since -connect=0 didn't disable automatic outbound connections. so those node operators would still use maxconnections=0. also possible we'd indirectly hit the variable storing maxconnections=0 if there's system space limitation too (not something a user would input though).

i assume most people use -noconnect nowadays to achieve the same functionality. (don't really know though)

I would probably opt for a user facing recommendation to use -noconnect instead. this offers a balance between minimizing codebase complexity and taking a conservative approach of what functionality we disable.

I like this!

unknown approved

unknown commented at 8:33 AM on March 9, 2023: none

utACK https://github.com/bitcoin/bitcoin/pull/26899/commits/4e9d06bf22da5e972c541b61036ffda73a51a114

vasild approved

vasild commented at 12:51 PM on March 9, 2023: contributor

ACK 928ff360ebc4b76db65b8ad5bc688f4404b32dec

This change, by itself is an improvement and I am ok to get it merged and move on.

However, can we do better? This PR does not change the behavior of -maxconnections=0 -listen=1, but lets change that too! :) It does not make sense and probably means that the user does not know what they are doing.

What about either giving an error on -maxconnections=0 -listen=1 or disallowing low values for maxconnections?

My request is kind of feature creep or scope creep for this PR. Again, I am ok to merge it as it is.

DrahtBot requested review from ghost on Mar 9, 2023

DrahtBot requested review from willcl-ark on Mar 9, 2023

kristapsk commented at 4:19 PM on March 9, 2023: contributor

Concept ACK

DrahtBot removed review request from ghost on Mar 9, 2023

DrahtBot requested review from ghost on Mar 9, 2023

fanquake referenced this in commit 99b64eec1b on Mar 10, 2023

sidhujag referenced this in commit 9ac4af26d7 on Mar 10, 2023

DrahtBot added the label Needs rebase on Mar 10, 2023

p2p: set `-dnsseed` and `-listen` false if `maxconnections=0`

If `maxconnections=0`, it means our possible connections are
going to be manual (e.g via `addnode`). For this reason, we
can skip DNS seeds and set `listen` false.

c84c5f6e89

doc: add release note for 26899 fabb95e7bf

brunoerg force-pushed on Mar 10, 2023

brunoerg commented at 4:58 PM on March 10, 2023: contributor

Rebased

DrahtBot removed the label Needs rebase on Mar 10, 2023

unknown approved

unknown commented at 9:59 PM on March 10, 2023: none

reACK https://github.com/bitcoin/bitcoin/pull/26899/commits/fabb95e7bf02f3d8e663a02dd845d42e09d330ec

DrahtBot requested review from vasild on Mar 10, 2023

achow101 commented at 4:33 PM on March 15, 2023: member

ACK fabb95e7bf02f3d8e663a02dd845d42e09d330ec

vasild commented at 10:05 AM on March 17, 2023: contributor

ACK fabb95e7bf02f3d8e663a02dd845d42e09d330ec

DrahtBot removed review request from vasild on Mar 17, 2023

achow101 merged this on Mar 20, 2023

achow101 closed this on Mar 20, 2023

sidhujag referenced this in commit 4dfa49be86 on Mar 20, 2023

bitcoin locked this on Mar 19, 2024