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

brunoerg · 2023-01-16T16:16:15Z

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 · 2023-01-16T16:16:18Z

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.

willcl-ark · 2023-01-17T10:11:24Z

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 · 2023-01-17T11:54:55Z

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 · 2023-01-17T12:05:34Z

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

brunoerg · 2023-01-17T12:19:57Z

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

Done.

ekzyis · 2023-01-17T22:19:39Z

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 · 2023-01-18T20:53:17Z

Maybe it should be updated?

Sure, force-pushed doing it!

willcl-ark · 2023-01-23T09:41:36Z

re-ACK 58fab41

stickies-v

Concept ACK

src/init.cpp

brunoerg · 2023-01-24T12:54:56Z

Force-pushed changing -listen help (#26899 (comment))

stickies-v

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?

src/init.cpp

brunoerg · 2023-01-26T18:06:18Z

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 · 2023-03-09T03:40:24Z

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

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!

ghost

utACK 4e9d06b

vasild

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.

kristapsk · 2023-03-09T16:19:14Z

Concept ACK

…ue of -listen 5c938e7 Use string interpolation for default value of -listen (ekzyis) Pull request description: This is a refactoring change. So I have read the following and will try to answer why this change should be accepted > * Refactoring changes are only accepted if they are required for a feature or bug fix or **_otherwise improve developer experience significantly_**. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. I have noticed in bitcoin/bitcoin#26899 (comment) that the helper message for `-listen` does not use string interpolation. That confused me and I wasn't sure what the reasons for that are. So it could be argued this confusion (by possibly many people in the past and in the future) may already be enough to accept this change. However, not accepting this means that if `DEFAULT_LISTEN` is ever changed, this helper message will still use the old value (however unlikely that may be). Therefore, this PR makes the helper message consistent with how other helper messages are implemented (using string interpolation) which leads to less confusion and prevents possibly wrong documentation in the future. ACKs for top commit: stratospher: ACK 5c938e7. vasild: ACK 5c938e7 kristapsk: cr utACK 5c938e7 Tree-SHA512: 8905f548ff399967966e67b29962821c28fd2620ff788c2b2bdfd088bbca75457dc63e933ad1985f93580508a65b91930fe4b2d97a2e0a7d83a3748b9ea2c26f

…isten 5c938e7 Use string interpolation for default value of -listen (ekzyis) Pull request description: This is a refactoring change. So I have read the following and will try to answer why this change should be accepted > * Refactoring changes are only accepted if they are required for a feature or bug fix or **_otherwise improve developer experience significantly_**. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. I have noticed in bitcoin#26899 (comment) that the helper message for `-listen` does not use string interpolation. That confused me and I wasn't sure what the reasons for that are. So it could be argued this confusion (by possibly many people in the past and in the future) may already be enough to accept this change. However, not accepting this means that if `DEFAULT_LISTEN` is ever changed, this helper message will still use the old value (however unlikely that may be). Therefore, this PR makes the helper message consistent with how other helper messages are implemented (using string interpolation) which leads to less confusion and prevents possibly wrong documentation in the future. ACKs for top commit: stratospher: ACK 5c938e7. vasild: ACK 5c938e7 kristapsk: cr utACK 5c938e7 Tree-SHA512: 8905f548ff399967966e67b29962821c28fd2620ff788c2b2bdfd088bbca75457dc63e933ad1985f93580508a65b91930fe4b2d97a2e0a7d83a3748b9ea2c26f

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.

brunoerg · 2023-03-10T16:58:00Z

Rebased

ghost

reACK fabb95e

achow101 · 2023-03-15T16:33:22Z

ACK fabb95e

vasild · 2023-03-17T10:05:44Z

ACK fabb95e

…cti…

DrahtBot added the P2P label Jan 16, 2023

brunoerg force-pushed the 2023-01-maxconnections-0 branch from 3acbb5f to c85b634 Compare January 17, 2023 12:19

brunoerg force-pushed the 2023-01-maxconnections-0 branch from c85b634 to 58fab41 Compare January 18, 2023 20:53

stickies-v reviewed Jan 23, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

brunoerg force-pushed the 2023-01-maxconnections-0 branch from 58fab41 to 4e9d06b Compare January 24, 2023 12:53

stickies-v reviewed Jan 25, 2023

View reviewed changes

ekzyis reviewed Jan 25, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

stratospher reviewed Mar 9, 2023

View reviewed changes

ghost approved these changes Mar 9, 2023

View reviewed changes

ekzyis mentioned this pull request Mar 9, 2023

Use string interpolation for default value of -listen #27232

Merged

vasild approved these changes Mar 9, 2023

View reviewed changes

DrahtBot requested review from a user and willcl-ark March 9, 2023 12:52

DrahtBot removed the request for review from a user March 9, 2023 16:19

DrahtBot requested a review from a user March 9, 2023 16:19

DrahtBot added the Needs rebase label Mar 10, 2023

brunoerg added 2 commits March 10, 2023 13:42

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

c84c5f6

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.

doc: add release note for 26899

fabb95e

brunoerg force-pushed the 2023-01-maxconnections-0 branch from 928ff36 to fabb95e Compare March 10, 2023 16:54

DrahtBot removed the Needs rebase label Mar 10, 2023

ghost approved these changes Mar 10, 2023

View reviewed changes

DrahtBot requested a review from vasild March 10, 2023 21:59

DrahtBot removed the request for review from vasild March 17, 2023 10:05

achow101 merged commit b7edd55 into bitcoin:master Mar 20, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 20, 2023

Merge bitcoin#26899: p2p: set -dnsseed and -listen false if `maxconne…

4dfa49b

…cti…

bitcoin locked and limited conversation to collaborators Mar 19, 2024

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

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

Uh oh!

Conversation

brunoerg commented Jan 16, 2023

Uh oh!

DrahtBot commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

willcl-ark commented Jan 17, 2023

Uh oh!

brunoerg commented Jan 17, 2023

Uh oh!

maflcko commented Jan 17, 2023

Uh oh!

brunoerg commented Jan 17, 2023

Uh oh!

ekzyis commented Jan 17, 2023

Uh oh!

brunoerg commented Jan 18, 2023

Uh oh!

willcl-ark commented Jan 23, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brunoerg commented Jan 24, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brunoerg commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitiuttarwar commented Mar 9, 2023

Uh oh!

stratospher 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!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

kristapsk commented Mar 9, 2023

Uh oh!

brunoerg commented Mar 10, 2023

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Mar 15, 2023

Uh oh!

vasild commented Mar 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

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

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

DrahtBot commented Jan 16, 2023 •

edited

Loading

brunoerg commented Jan 26, 2023 •

edited

Loading