Added -whiteconnections=<n> option #5288

Krellan · 2014-11-16T11:50:01Z

This allows whitelisted peers a higher connection limit,
so they can exceed the normal -maxconnections limit,
useful for ensuring your local users and miners can always get in.

Krellan · 2014-11-18T07:33:56Z

Refactored, after conversation with gmaxwell on #bitcoin-dev.

Default of whiteconnections is now 4 (if user isn't using whitelist feature at all, default is 0).

Now acts to essentially reserve slots for exclusive use by whitelisted peers, against the given maxconnections limit, instead of increasing the maxconnections limit.

Warning message given if whiteconnections value is so high that it will reserve all inbound slots, preventing any non-whitelisted peer from ever being able to connect.

If user has not given a maxconnections value, then as a special case it adds the value of whiteconnections to the default for maxconnections. This is to avoid establishing a new default which would inadvertently reduce the number of incoming slots available for public (non-whitelisted) use. With the popularity of SPV wallets these days, incoming slots on full nodes are a rather precious resource.

This whiteconnections feature is rather nice, especially for long-lived full nodes which reach their capacity of inbound connections. Now, you don't have to restart your node whenever you restart your miner, just to free up a slot so that your miner can get in!

sipa · 2014-11-18T13:34:56Z

src/net.h

Can you add a comment here saying that this number is the reserved number of connections for white connections?

sipa · 2014-11-18T13:43:18Z

Tested that whitelisted connections are restricted to the whitelisted reserved count, and that the implicit + 4 without specified -maxconnections works as intended.

Krellan · 2014-11-19T07:43:12Z

Thanks, sipa, I added a comment showing some of the theory of operation behind nMaxConnections and nWhiteConnections.

Krellan · 2014-12-28T00:09:15Z

Rebased against the latest, no changes were necessary :)

fanquake · 2015-02-02T12:57:44Z

src/net.h

Could you update this to use our doxygen comment format.

It's not easy to integrate a 'floating' comment like this into doxygen.
One way would be to group nMaxConnections and nWhiteConnections using @{ @} and add it as doxygen comment of the group.

Krellan · 2015-02-09T20:52:10Z

Thanks, will update it.

ghost · 2015-03-11T07:04:09Z

utAck. Is the pending update keeping the merge up?

Krellan · 2015-03-11T09:10:19Z

Yikes! Forgot about this. Will update ASAP.

Krellan · 2015-03-12T09:47:17Z

There, got it merged with latest master, and made the requested documentation cleanup changes!

The Travis build still fails for the "no wallet" case, but the same thing happens right now for the latest upstream master....

Krellan · 2015-03-12T18:09:13Z

No code change, just rebased with latest master to pick up the Travis build fix.

ghost · 2015-03-12T22:28:28Z

Thanks!

laanwj · 2015-03-18T13:49:08Z

src/init.cpp

What if the user is using whitebind instead?

Krellan · 2015-03-18T22:32:32Z

Good to point out! I will change it so that the "User is using whitelist feature" test matches on either -whitelist or -whitebind options.

Krellan · 2015-03-18T23:30:02Z

There, updated it as described (both whitelist and whitebind will work to trigger the nMaxConnections fixup). Also rebased against latest master.

laanwj · 2015-03-19T12:37:02Z

src/init.cpp

This inner if() seems a bit overcomplicated. I think this addition should either be always done (when whitelisting is used), or never at all. To me it doesn't hold up to the principle of least surprise to do this only to the default -maxconnections but not when the user provides one. Too much parameter interaction...

Krellan · 2015-03-20T06:33:20Z

I thought about that as well, during initial implementation thoughts over IRC. Talked about it with a few others. Tried to avoid surprise and unintended behavior as best as possible, and this was a compromise.

If the increase was done always, then it would be surprising to the user, because it would look like we were blatantly disobeying their given maxconnections limit.

If user doesn't provide maxconnections at all, then the thinking was that they wouldn't care as much, therefore we could get away with doing the increase. We definitely do want to do the increase, as mentioned in the comments, to avoid reducing connection capacity of the Bitcoin network.

So, there's thinking that went into both sides of this decision.

Krellan · 2015-04-25T07:33:45Z

Trivial change to keep up with latest upstream.

sipa · 2015-04-26T21:25:24Z

@Krellan There are several unaddressed comments?

Krellan · 2015-04-26T21:38:45Z

@sipa Which comments? Thought I addressed all concerns.

sipa · 2015-04-26T21:41:36Z

Sorry, I missed that you answered in the PR rather than on the commits.

sipa · 2015-04-27T12:27:03Z

The "increase the maxconnections if not specified, don't if it is" is fine, I think, but I wonder if the -whiteconnections default = 4 is needed. An existing user of one of the white* features won't expect a change in behaviour.

I wonder if this isn't a concise way to specify it:

-whiteconnections defaults to 0, no matter what
-maxconnections defaults to 125 + [-whiteconnections](or whatever the FD limit permits)

Seems easier to explain, and wouldn't violate the least surprise principle either?

Krellan · 2015-05-08T06:35:15Z

Good point. I will make it default to 0 instead of 4, so that the default behavior is to change nothing at all.

Krellan · 2015-05-26T09:03:35Z

There, as requested, changed the default from 4 to 0. Made no other code changes, just these constants. Also rebased to latest upstream.

So, the behavior is now unchanged, by default. If the user wants the benefit of reserved slots for their whitelisted incoming connections, they will have to explicitly specify how many slots they want to reserve.

laanwj · 2015-06-12T15:08:10Z

src/init.cpp

Slightly preferred for readability:

nMaxConnections = std::min(nFD - MIN_CORE_FILEDESCRIPTORS, nMaxConnections);

laanwj · 2015-06-12T15:10:42Z

utACK apart from nit (but needs rebase)

Krellan · 2015-06-14T08:57:31Z

Rebased.

This sets aside a number of connection slots for whitelisted peers, useful for ensuring your local users and miners can always get in, even if your limit on inbound connections has already been reached.

Krellan · 2015-06-14T09:20:49Z

Addressed the nit, good catch.

Also found another place where the default was still 4, changed to 0.

sipa · 2015-06-14T13:06:49Z

Untested ACK

e3cae52 Added -whiteconnections=<n> option (Josh Lehan)

Zcash: Was "Added -whiteconnections=<n> option" from bitcoin/bitcoin#5288. The option was later removed in bitcoin/bitcoin#6374 which we merged in zcash#1258. This commit contains the difference between the two.

Bitcoin 0.12 misc P2P/Net PRs Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#5288 - Only the reorg, option was removed in bitcoin/bitcoin#6374 which we merged in #1258 - bitcoin/bitcoin#6561 - bitcoin/bitcoin#6728 - bitcoin/bitcoin#6829 - bitcoin/bitcoin#6974 - bitcoin/bitcoin#7075 - bitcoin/bitcoin#7166 Part of #2074.

Krellan force-pushed the whiteconnections branch 3 times, most recently from 5b7faeb to 46a7467 Compare November 18, 2014 07:13

sipa reviewed Nov 18, 2014
View reviewed changes

src/net.h Outdated

Copy link

Member

sipa Nov 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here saying that this number is the reserved number of connections for white connections?

Krellan force-pushed the whiteconnections branch from 46a7467 to cae9ef9 Compare November 19, 2014 07:40

Krellan force-pushed the whiteconnections branch from cae9ef9 to 48610c2 Compare November 24, 2014 06:36

Krellan force-pushed the whiteconnections branch from 48610c2 to 910d9a8 Compare December 28, 2014 00:11

laanwj added Feature P2P labels Jan 8, 2015

fanquake reviewed Feb 2, 2015
View reviewed changes

Krellan force-pushed the whiteconnections branch from 910d9a8 to 334299f Compare March 12, 2015 09:09

Krellan force-pushed the whiteconnections branch from 334299f to 55b94ad Compare March 12, 2015 18:07

Krellan mentioned this pull request Mar 15, 2015

Add new command line option '-maxoutbound' #4687

Closed

laanwj reviewed Mar 18, 2015
View reviewed changes

src/init.cpp

Copy link

Member

laanwj Mar 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the user is using whitebind instead?

Krellan force-pushed the whiteconnections branch 2 times, most recently from e317c43 to d7fcdd0 Compare March 18, 2015 23:25

laanwj reviewed Mar 19, 2015
View reviewed changes

Krellan force-pushed the whiteconnections branch from 316ceed to 40dc4a7 Compare May 26, 2015 08:59

laanwj reviewed Jun 12, 2015
View reviewed changes

src/init.cpp Outdated

Copy link

Member

laanwj Jun 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly preferred for readability:

nMaxConnections = std::min(nFD - MIN_CORE_FILEDESCRIPTORS, nMaxConnections);

Krellan force-pushed the whiteconnections branch from 40dc4a7 to 2edda2b Compare June 14, 2015 08:56

Added -whiteconnections=<n> option

e3cae52

This sets aside a number of connection slots for whitelisted peers, useful for ensuring your local users and miners can always get in, even if your limit on inbound connections has already been reached.

Krellan force-pushed the whiteconnections branch from 2edda2b to e3cae52 Compare June 14, 2015 09:14

laanwj merged commit e3cae52 into bitcoin:master Jul 10, 2015

laanwj added a commit that referenced this pull request Jul 10, 2015

Merge pull request #5288

4452205

e3cae52 Added -whiteconnections=<n> option (Josh Lehan)

laanwj mentioned this pull request Jul 28, 2015

Connection slot exhaustion DoS mitigation #6374

Merged

str4d mentioned this pull request Jul 14, 2017

Bitcoin 0.12 P2P/Net PRs 1 zcash/zcash#2534

Draft

str4d mentioned this pull request Feb 18, 2021

Bitcoin 0.12 misc P2P/Net PRs zcash/zcash#4994

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Added -whiteconnections=<n> option #5288

Added -whiteconnections=<n> option #5288

Uh oh!

Conversation

Krellan commented Nov 16, 2014

Uh oh!

Krellan commented Nov 18, 2014

Uh oh!

sipa Nov 18, 2014

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 18, 2014

Uh oh!

Krellan commented Nov 19, 2014

Uh oh!

Krellan commented Dec 28, 2014

Uh oh!

fanquake Feb 2, 2015

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 12, 2015

Choose a reason for hiding this comment

Uh oh!

Krellan commented Feb 9, 2015

Uh oh!

ghost commented Mar 11, 2015

Uh oh!

Krellan commented Mar 11, 2015

Uh oh!

Krellan commented Mar 12, 2015

Uh oh!

Krellan commented Mar 12, 2015

Uh oh!

ghost commented Mar 12, 2015

Uh oh!

laanwj Mar 18, 2015

Choose a reason for hiding this comment

Uh oh!

Krellan commented Mar 18, 2015

Uh oh!

Krellan commented Mar 18, 2015

Uh oh!

laanwj Mar 19, 2015

Choose a reason for hiding this comment

Uh oh!

Krellan commented Mar 20, 2015

Uh oh!

Krellan commented Apr 25, 2015

Uh oh!

sipa commented Apr 26, 2015

Uh oh!

Krellan commented Apr 26, 2015

Uh oh!

sipa commented Apr 26, 2015

Uh oh!

sipa commented Apr 27, 2015

Uh oh!

Krellan commented May 8, 2015

Uh oh!

Krellan commented May 26, 2015

Uh oh!

laanwj Jun 12, 2015

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 12, 2015

Uh oh!

Krellan commented Jun 14, 2015

Uh oh!

Krellan commented Jun 14, 2015

Uh oh!

sipa commented Jun 14, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants