New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added -whiteconnections=<n> option #5288

Merged
merged 1 commit into from Jul 10, 2015

Conversation

Projects
None yet
5 participants
@Krellan
Contributor

Krellan commented Nov 16, 2014

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

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Nov 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!

Contributor

Krellan commented Nov 18, 2014

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

View changes

Show outdated Hide outdated src/net.h
@@ -122,6 +122,7 @@ extern uint64_t nLocalServices;
extern uint64_t nLocalHostNonce;
extern CAddrMan addrman;
extern int nMaxConnections;
extern int nWhiteConnections;

This comment has been minimized.

@sipa

sipa Nov 18, 2014

Member

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

@sipa

sipa Nov 18, 2014

Member

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 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.

Member

sipa commented Nov 18, 2014

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

@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Nov 19, 2014

Contributor

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

Contributor

Krellan commented Nov 19, 2014

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

@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Dec 28, 2014

Contributor

Rebased against the latest, no changes were necessary :)

Contributor

Krellan commented Dec 28, 2014

Rebased against the latest, no changes were necessary :)

@laanwj laanwj added Feature P2P labels Jan 8, 2015

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

This comment has been minimized.

@fanquake

fanquake Feb 2, 2015

Member

Could you update this to use our doxygen comment format.

@fanquake

fanquake Feb 2, 2015

Member

Could you update this to use our doxygen comment format.

This comment has been minimized.

@laanwj

laanwj Jun 12, 2015

Member

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.

@laanwj

laanwj Jun 12, 2015

Member

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

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Feb 9, 2015

Contributor

Thanks, will update it.

Contributor

Krellan commented Feb 9, 2015

Thanks, will update it.

@21E14

This comment has been minimized.

Show comment
Hide comment
@21E14

21E14 Mar 11, 2015

Contributor

utAck. Is the pending update keeping the merge up?

Contributor

21E14 commented Mar 11, 2015

utAck. Is the pending update keeping the merge up?

@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Mar 11, 2015

Contributor

Yikes! Forgot about this. Will update ASAP.

Contributor

Krellan commented Mar 11, 2015

Yikes! Forgot about this. Will update ASAP.

@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Mar 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....

Contributor

Krellan commented Mar 12, 2015

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

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Mar 12, 2015

Contributor

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

Contributor

Krellan commented Mar 12, 2015

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

@21E14

This comment has been minimized.

Show comment
Hide comment
@21E14

21E14 Mar 12, 2015

Contributor

Thanks!

Contributor

21E14 commented Mar 12, 2015

Thanks!

// User not using whitelist feature.
// Silently disable connection reservation,
// for the same reason as above.
nWhiteConnections = 0;

This comment has been minimized.

@laanwj

laanwj Mar 18, 2015

Member

What if the user is using whitebind instead?

@laanwj

laanwj Mar 18, 2015

Member

What if the user is using whitebind instead?

@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Mar 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.

Contributor

Krellan commented Mar 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Mar 18, 2015

Contributor

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

Contributor

Krellan commented Mar 18, 2015

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

Show outdated Hide outdated src/init.cpp
nWhiteConnections = std::max(nUserWhiteConnections, 0);
if ((mapArgs.count("-whitelist")) || (mapArgs.count("-whitebind"))) {
if (!(mapArgs.count("-maxconnections"))) {

This comment has been minimized.

@laanwj

laanwj Mar 19, 2015

Member

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

@laanwj

laanwj Mar 19, 2015

Member

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

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Mar 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.

Contributor

Krellan commented Mar 20, 2015

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.

@21E14

This comment has been minimized.

Show comment
Hide comment
@21E14

21E14 Mar 24, 2015

Contributor

Changing from utAck to tAck.

Contributor

21E14 commented Mar 24, 2015

Changing from utAck to tAck.

@21E14

This comment has been minimized.

Show comment
Hide comment
@21E14

21E14 Apr 21, 2015

Contributor

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

Contributor

21E14 commented Apr 21, 2015

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

@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Apr 22, 2015

Contributor

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

Contributor

Krellan commented Apr 22, 2015

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

@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Apr 25, 2015

Contributor

Trivial change to keep up with latest upstream.

Contributor

Krellan commented Apr 25, 2015

Trivial change to keep up with latest upstream.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 26, 2015

Member

@Krellan There are several unaddressed comments?

Member

sipa commented Apr 26, 2015

@Krellan There are several unaddressed comments?

@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Apr 26, 2015

Contributor

@sipa Which comments? Thought I addressed all concerns.

Contributor

Krellan commented Apr 26, 2015

@sipa Which comments? Thought I addressed all concerns.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 26, 2015

Member

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

Member

sipa commented Apr 26, 2015

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 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?

Member

sipa commented Apr 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@Krellan

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

Contributor

Krellan commented May 8, 2015

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

@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

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

Contributor

Krellan commented May 26, 2015

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.

Show outdated Hide outdated src/init.cpp
if (nFD < MIN_CORE_FILEDESCRIPTORS)
return InitError(_("Not enough file descriptors available."));
if (nFD - MIN_CORE_FILEDESCRIPTORS < nMaxConnections)
nMaxConnections = nFD - MIN_CORE_FILEDESCRIPTORS;

This comment has been minimized.

@laanwj

laanwj Jun 12, 2015

Member

Slightly preferred for readability:

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

laanwj Jun 12, 2015

Member

Slightly preferred for readability:

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 12, 2015

Member

utACK apart from nit (but needs rebase)

Member

laanwj commented Jun 12, 2015

utACK apart from nit (but needs rebase)

@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Jun 14, 2015

Contributor

Rebased.

Contributor

Krellan commented Jun 14, 2015

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.
@Krellan

This comment has been minimized.

Show comment
Hide comment
@Krellan

Krellan Jun 14, 2015

Contributor

Addressed the nit, good catch.

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

Contributor

Krellan commented Jun 14, 2015

Addressed the nit, good catch.

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 14, 2015

Member

Untested ACK

Member

sipa commented Jun 14, 2015

Untested ACK

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Merge pull request #5288
e3cae52 Added -whiteconnections=<n> option (Josh Lehan)

str4d added a commit to str4d/zcash that referenced this pull request Jul 13, 2017

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

str4d added a commit to str4d/zcash that referenced this pull request Nov 9, 2017

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

str4d added a commit to str4d/zcash that referenced this pull request Apr 5, 2018

Re-organize -maxconnections option handling
Zcash: Was "Added -whiteconnections=<n> option" from bitcoin/bitcoin#5288. The
option was later removed in bitcoin/bitcoin#6374 which we merged in #1258. This
commit contains the difference between the two.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment