Added -whiteconnections=<n> option

Krellan commented at 11:50 am on November 16, 2014: contributor

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 force-pushed on Nov 16, 2014

Krellan force-pushed on Nov 18, 2014

Krellan commented at 7:33 am on November 18, 2014: contributor

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!

in src/net.h: in 46a7467896 outdated

121@@ -122,6 +122,7 @@ extern uint64_t nLocalServices;
122 extern uint64_t nLocalHostNonce;
123 extern CAddrMan addrman;
124 extern int nMaxConnections;
125+extern int nWhiteConnections;

sipa commented at 1:34 pm on November 18, 2014:

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

sipa commented at 1:43 pm on November 18, 2014: member

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

Krellan force-pushed on Nov 19, 2014

Krellan commented at 7:43 am on November 19, 2014: contributor

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

Krellan force-pushed on Nov 24, 2014

Krellan commented at 0:09 am on December 28, 2014: contributor

Rebased against the latest, no changes were necessary :)

Krellan force-pushed on Dec 28, 2014

laanwj added the label Feature on Jan 8, 2015

laanwj added the label P2P on Jan 8, 2015

in src/net.h: in 910d9a8407 outdated

122@@ -123,7 +123,17 @@ extern bool fListen;
123 extern uint64_t nLocalServices;
124 extern uint64_t nLocalHostNonce;
125 extern CAddrMan addrman;
126+

fanquake commented at 12:57 pm on February 2, 2015:

Could you update this to use our doxygen comment format.

laanwj commented at 3:10 pm on June 12, 2015:

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 commented at 8:52 pm on February 9, 2015: contributor

Thanks, will update it.

ghost commented at 7:04 am on March 11, 2015: none

utAck. Is the pending update keeping the merge up?

Krellan commented at 9:10 am on March 11, 2015: contributor

Yikes! Forgot about this. Will update ASAP.

Krellan force-pushed on Mar 12, 2015

Krellan commented at 9:47 am on March 12, 2015: contributor

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 force-pushed on Mar 12, 2015

Krellan commented at 6:09 pm on March 12, 2015: contributor

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

ghost commented at 10:28 pm on March 12, 2015: none

Thanks!

in src/init.cpp: in 55b94ad23f outdated

686+        }
687+    } else {
688+        // User not using whitelist feature.
689+        // Silently disable connection reservation,
690+        // for the same reason as above.
691+        nWhiteConnections = 0;

laanwj commented at 1:49 pm on March 18, 2015:

What if the user is using whitebind instead?

Krellan commented at 10:32 pm on March 18, 2015: contributor

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 force-pushed on Mar 18, 2015

Krellan commented at 11:30 pm on March 18, 2015: contributor

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

in src/init.cpp: in d7fcdd03d8 outdated

673+    nMaxConnections = std::max(nUserMaxConnections, 0);
674+    int nUserWhiteConnections = GetArg("-whiteconnections", 4);
675+    nWhiteConnections = std::max(nUserWhiteConnections, 0);
676+
677+    if ((mapArgs.count("-whitelist")) || (mapArgs.count("-whitebind"))) {
678+        if (!(mapArgs.count("-maxconnections"))) {

laanwj commented at 12:37 pm on March 19, 2015:

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 commented at 6:33 am on March 20, 2015: contributor

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.

ghost commented at 9:59 pm on March 24, 2015: none

Changing from utAck to tAck.

ghost commented at 11:11 pm on April 21, 2015: none

I am not sure what’s been keeping the PR open?

Krellan commented at 1:30 am on April 22, 2015: contributor

I’m not sure either. I’m hoping there is no issue preventing this from being merged.

Krellan force-pushed on Apr 25, 2015

Krellan commented at 7:33 am on April 25, 2015: contributor

Trivial change to keep up with latest upstream.

sipa commented at 9:25 pm on April 26, 2015: member

@Krellan There are several unaddressed comments?

Krellan commented at 9:38 pm on April 26, 2015: contributor

@sipa Which comments? Thought I addressed all concerns.

sipa commented at 9:41 pm on April 26, 2015: member

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

sipa commented at 12:27 pm on April 27, 2015: member

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 commented at 6:35 am on May 8, 2015: contributor

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

Krellan force-pushed on May 26, 2015

Krellan commented at 9:03 am on May 26, 2015: contributor

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.

in src/init.cpp: in 40dc4a7ea8 outdated

739+    nMaxConnections = std::max(std::min(nMaxConnections, (int)(FD_SETSIZE - nBind - MIN_CORE_FILEDESCRIPTORS)), 0);
740+    int nFD = RaiseFileDescriptorLimit(nMaxConnections + MIN_CORE_FILEDESCRIPTORS);
741+    if (nFD < MIN_CORE_FILEDESCRIPTORS)
742+        return InitError(_("Not enough file descriptors available."));
743+    if (nFD - MIN_CORE_FILEDESCRIPTORS < nMaxConnections)
744+        nMaxConnections = nFD - MIN_CORE_FILEDESCRIPTORS;

laanwj commented at 3:08 pm on June 12, 2015:

Slightly preferred for readability:

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

laanwj commented at 3:10 pm on June 12, 2015: member

utACK apart from nit (but needs rebase)

Krellan force-pushed on Jun 14, 2015

Krellan commented at 8:57 am on June 14, 2015: contributor

Rebased.

Added -whiteconnections=<n> option

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.

e3cae52538

Krellan force-pushed on Jun 14, 2015

Krellan commented at 9:20 am on June 14, 2015: contributor

Addressed the nit, good catch.

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

sipa commented at 1:06 pm on June 14, 2015: member

Untested ACK

laanwj merged this on Jul 10, 2015

laanwj closed this on Jul 10, 2015

laanwj referenced this in commit 445220544e on Jul 10, 2015

str4d referenced this in commit 0a1ac9e3ce on Jul 13, 2017

str4d referenced this in commit e3564bae93 on Nov 9, 2017

str4d referenced this in commit d7f224b2be on Apr 5, 2018

str4d referenced this in commit 39dddced8f on Feb 18, 2021

str4d referenced this in commit d3a2f120f5 on Feb 18, 2021

zkbot referenced this in commit e10008da66 on Feb 18, 2021

zkbot referenced this in commit 777deea264 on Feb 19, 2021

zkbot referenced this in commit b62e35dee8 on Feb 19, 2021

MarcoFalke locked this on Sep 8, 2021

Added -whiteconnections= option #5288