Break addnode out from the outbound connection limits. #9319

gmaxwell · 2016-12-11T05:01:59Z

Previously addnodes were in competition with outbound connections
for access to the eight outbound slots.

One result of this is that frequently a node with several addnode
configured peers would end up connected to none of them, because
while the addnode loop was in its two minute sleep the automatic
connection logic would fill any free slots with random peers.
This is particularly unwelcome to users trying to maintain links
to specific nodes for fast block relay or monitoring purposes.

Another result is that a group of nine or more nodes which are
have addnode configured towards each other can become partitioned
from the public network.

This commit introduces a new limit of eight connections just for
addnode peers which is not subject to any of the other connection
limitations (including maxconnections).

The choice of eight is sufficient so that under no condition would
a user find themselves connected to fewer addnoded peers than
previously. It is also low enough that users who are confused
about the significance of more connections and have gotten too
copy-and-paste happy will not consume more than twice the slot
usage of a typical user.

Any additional load on the network resulting from this will likely
be offset by a reduction in users applying even more wasteful
workaround for the prior behavior.

The retry delays are reduced to avoid nodes sitting around without
their added peers up, but are still sufficient to prevent overly
aggressive repeated connections. The reduced delays also make
the system much more responsive to the addnode RPC.

Ban-disconnects are also exempted for peers added via addnode since
the outbound addnode logic ignores bans. Previously it would ban
an addnode then immediately reconnect to it.

A minor change was also made to CSemaphoreGrant so that it is
possible to re-acquire via an object whos grant was moved.

sipa · 2016-12-11T05:06:32Z

src/init.cpp

Are addnode connections no longer counted under the normal max connection limit?

They are not. See also commit message, pr text, and release note. :)

CodeShark · 2016-12-11T08:32:34Z

concept ACK

paveljanik · 2016-12-11T17:29:14Z

doc/release-notes.md

What do you mean by "aggressive"connection here?

paveljanik · 2016-12-11T17:32:05Z

src/net_processing.cpp

This is new behaviour, addnoded nodes are not punished/banned. Right?

"Ban-disconnects are also exempted for peers added via addnode since
the outbound addnode logic ignores bans. Previously it would ban
an addnode then immediately reconnect to it."

paveljanik · 2016-12-11T17:32:38Z

src/rpc/net.cpp

addnode missing in the helptext.

laanwj · 2016-12-15T16:53:13Z

Adding a specific limit for user-added nodes makes it easier to reason about, and avoids some annoying edge cases. Good idea. Concept ACK.

TheBlueMatt · 2016-12-24T21:04:59Z

ACK b8d170dab441896970d5ab0f15ca672173592065

Would very much like to see this for 0.14.

sipa · 2016-12-27T16:29:37Z

src/net.cpp

If no connection was established, this sleep runs while holding the semaphore. Is that a problem? I think this is the only thread that accesses the semaphore, but if that's the case... why are we using a semaphore?

It's okay to hold extras here because this is the only place they're taken. What they're used for is to sleep until a release, which is also the case for outbound peers use of a semaphore. The result is so that when this thread is inactive because we're at connection maximum and a connection goes down the code to make a new connection begins instantly.

Perhaps it would be better to use some other notification mechanism; e.g. a sleep that can wake early on a condition variable, triggered whenever an addnoded peer is disconnected... but this is what the outbound connections currently do, and I didn't see a reason to make them any different. If you want I could add a change to rework both to whatever mechanism you prefer. The goal should be that if we're sleeping because there is nothing else to do (because we're at our limit, or because we have no down addnodes to try) we ought to wake instantly when a disconnection (or rpc reconfiguration, I guess) potentially changes that situation.

We'll soon have an OnDisconnected() event that will allow us to change this (and possibly the outbound behavior) more easily. Using the same mechanism here makes sense for now, imo.

sipa · 2016-12-27T16:36:59Z

src/net.cpp

This makes addnoded connections no longer prevent random connections to the same netgroups. Is that change implied by "which is not subject to any of the other connection limitations"?

That was the intent. At least the idea was to make the automatic and addnode connections operate largely independent of each other, not auto-connecting to amazon because you've got an addnode to amazon seems odd.

At least my perspective on the netgroup limitation is that with your limited 8 outbound slots you don't want to harm your partition resistance by spending multiple of them on a single netgroup. That no longer applies when addnodes are not consuming your outbound slots, just like it doesn't apply to inbound peers. (though inbound has a stronger additional reason: they're attacker controlled).

That makes sense. I just wanted to make sure it was intentional.

Do you think I should add a comment like, "Inbound and addnode connections don't use our limited pool of automatic outbound connections, so we don't consider them for netgroup exclusion."?

Sure, always add comments :).

sipa · 2016-12-30T13:24:55Z

One nit: your last commit has 'documnetation' in the title.

sipa · 2016-12-30T13:48:07Z

utACK 1c7dcacabd7327273a138aec4fd74a404b32e596

kanoi · 2017-01-01T22:58:22Z

Missing command line and RPC options to set the limit.

maflcko · 2017-01-01T23:45:30Z

@kanoi You can find the reasons why this is not supported in #9217 and similar discussions.

kanoi · 2017-01-02T00:35:42Z

That discussion is only about RPC.

TheBlueMatt · 2017-01-02T01:09:19Z

@kanoi that discussion is about outbound connection P2P slots, not RPC. The issue here is that lots of folks configure Bitcoin Core by adding a long list of known IP addresses from arbitrary sources to addnode. This used to be especially common, but still exists today. In those cases, we absolutely cannot default to simply make 100 outbound connections (for the same reasons as discussed in #9217).

kanoi · 2017-01-02T01:40:35Z

Nothing to do with defaults - I'm referring to having an arbitrary constant number that can't be changed without modifying the code - there's no logical reason to set it at 8 and enforce that number for eternity.
Default of 8 is not the issue, forcing it to always be 8 is the issue.

TheBlueMatt · 2017-01-02T01:47:29Z

@kanoi changing the maximum outbound connections is far out of scope for this PR. If you wish to do so, please comment on #9217, #6533, #6014, etc.

kanoi · 2017-01-02T01:55:01Z

I don't see the reason for adding a new setting and ignoring adding an option to configure it.
If the setting isn't necessary then why add it.
If the setting is necessary, why not allow it to be configured.

TheBlueMatt · 2017-01-02T01:59:57Z

There is no new setting here? It uses the same connection slot count as previously (8).

kanoi · 2017-01-02T02:14:08Z

Really?

This commit introduces a new limit of eight connections just for
addnode peers which is not subject to any of the other connection
limitations (including maxconnections).

sipa · 2017-01-02T02:16:31Z

If the setting is necessary, why not allow it to be configured.

Configurable settings are useful when there are distinctly different environments that require different behaviour. Reasons for a higher number of outgoing connections: * Better partition resistance (but more than a few has hardly any effect). Reasons for a lower number of outgoing connections: * More load on the network (not something we want to encourage, having seen what the effects of no open connectable slots on the network are). * Making your own node's performance and responsiveness worse, and increasing your bandwidth (which can adequately be controlled by using less -addnode's and/or lower -maxconnection setting). As a result, I do not see a reason to make it configurable. Doing so would indicate that there is a usefulness to changing it, which likely leads to misunderstandings ("I want the most connections because I want the most verified blockchain!" or "I want to help the network with lots of connections!" or "My validation is too slow, I want to download faster!"), without any advantages for anyone. If you want a configurable setting here, there would need to be a condition under which a higher or lower value would be recommendable.

theuni

utACK. I like having added connections independent of the automatic ones. As-is, it's been unclear what should take precedence in some cases.

theuni · 2017-01-02T19:02:32Z

src/net.cpp

We'll soon have an OnDisconnected() event that will allow us to change this (and possibly the outbound behavior) more easily. Using the same mechanism here makes sense for now, imo.

TheBlueMatt · 2017-01-04T22:29:14Z

Needs rebase, should be easy.

laanwj · 2017-01-05T10:59:11Z

Conflicts:

<<<<<<< 406f35d99d0fcd2f50370280e605283d04da466a
                OpenNetworkConnection(CAddress(service, NODE_NONE), false, &grant, info.strAddedNode.c_str(), false);
                if (!interruptNet.sleep_for(std::chrono::milliseconds(500)))
                    return;
||||||| merged common ancestors
                OpenNetworkConnection(CAddress(service, NODE_NONE), false, &grant, info.strAddedNode.c_str(), false);
                MilliSleep(500);
=======
                OpenNetworkConnection(CAddress(service, NODE_NONE), false, &grant, info.strAddedNode.c_str(), false, false, true);
                MilliSleep(500);
>>>>>>> Break addnode out from the outbound connection limits.
            }
        }
<<<<<<< 406f35d99d0fcd2f50370280e605283d04da466a
        if (!interruptNet.sleep_for(std::chrono::minutes(2)))
            return;
||||||| merged common ancestors

        MilliSleep(120000); // Retry every 2 minutes
=======
        MilliSleep(tried ? 60000 : 2000); // Retry every 60 seconds if a connection was attempted, otherwise two seconds
>>>>>>> Break addnode out from the outbound connection limits.

Previously addnodes were in competition with outbound connections for access to the eight outbound slots. One result of this is that frequently a node with several addnode configured peers would end up connected to none of them, because while the addnode loop was in its two minute sleep the automatic connection logic would fill any free slots with random peers. This is particularly unwelcome to users trying to maintain links to specific nodes for fast block relay or purposes. Another result is that a group of nine or more nodes which are have addnode configured towards each other can become partitioned from the public network. This commit introduces a new limit of eight connections just for addnode peers which is not subject to any of the other connection limitations (including maxconnections). The choice of eight is sufficient so that under no condition would a user find themselves connected to fewer addnoded peers than previously. It is also low enough that users who are confused about the significance of more connections and have gotten too copy-and-paste happy will not consume more than twice the slot usage of a typical user. Any additional load on the network resulting from this will likely be offset by a reduction in users applying even more wasteful workaround for the prior behavior. The retry delays are reduced to avoid nodes sitting around without their added peers up, but are still sufficient to prevent overly aggressive repeated connections. The reduced delays also make the system much more responsive to the addnode RPC. Ban-disconnects are also exempted for peers added via addnode since the outbound addnode logic ignores bans. Previously it would ban an addnode then immediately reconnect to it. A minor change was also made to CSemaphoreGrant so that it is possible to re-acquire via an object whos grant was moved.

Also adds a comment about the netgroup exclusion behavior.

gmaxwell · 2017-01-05T19:03:31Z

rebased.

sipa · 2017-01-05T19:27:47Z

utACK 032ba3f

TheBlueMatt · 2017-01-05T22:27:25Z

Diff looks equivalent to me from my previous ACK at b8d170dab441896970d5ab0f15ca672173592065 to 032ba3f

032ba3f RPC help documentation for addnode peerinfo. (Gregory Maxwell) 90f13e1 Add release notes for addnode changes. (Gregory Maxwell) 50bd12c Break addnode out from the outbound connection limits. (Gregory Maxwell)

droark · 2017-01-07T06:53:17Z

src/net.cpp

        }
-        if (!interruptNet.sleep_for(std::chrono::minutes(2)))
+        // Retry every 60 seconds if a connection was attempted, otherwise two seconds
+        if (!interruptNet.sleep_for(std::chrono::seconds(tried ? 60 : 2)));


FYI, I'm getting a warning for this on macOS 10.12.

net.cpp:1807:75: warning: if statement has empty body [-Wempty-body] if (!interruptNet.sleep_for(std::chrono::seconds(tried ? 60 : 2))); ^ net.cpp:1807:75: note: put the semicolon on a separate line to silence this warning

Looks like a stray semicolon to me. For reference in the future, if I come across something like this again, should I bring this to the attention of the engineer, or should I just file a PR if the commit has already occurred?

@droark If it is not merged, provide feedback. After it is merged, pull requests are very welcomed.

Thanks. Just filed #9487.

Empty body introduced by commit bitcoin#9319 should not be empty.

btcdrak · 2017-01-07T18:08:18Z

Post humus ACK 032ba3f

…mits. 032ba3f RPC help documentation for addnode peerinfo. (Gregory Maxwell) 90f13e1 Add release notes for addnode changes. (Gregory Maxwell) 50bd12c Break addnode out from the outbound connection limits. (Gregory Maxwell)

Empty body introduced by commit bitcoin#9319 should not be empty.

…mits. 032ba3f RPC help documentation for addnode peerinfo. (Gregory Maxwell) 90f13e1 Add release notes for addnode changes. (Gregory Maxwell) 50bd12c Break addnode out from the outbound connection limits. (Gregory Maxwell)

…ections config options b4fcbcf doc: update -maxconnections config option help (Jon Atack) 79685a8 doc: update -addnode config option help (Jon Atack) 2896c6c doc: update addnode rpc help (Jon Atack) Pull request description: Since #9319 proposed by Gregory Maxwell and released in v0.14, peers manually added through the `-addnode` config option or using the `addnode` RPC have their own separate limit of 8 connections that does not compete with other inbound or outbound connection usage and is not subject to the limitation imposed by the `-maxconnections` option. This PR updates the `-addnode` and `-maxconnections` config options and the `addnode` RPC help docs with this information. `-addnode` config option help ``` $ bitcoind -h | grep -A5 addnode= -addnode=<ip> Add a node to connect to and attempt to keep the connection open (see the addnode RPC help for more info). This option can be specified multiple times to add multiple nodes; connections are limited to 8 at a time and are counted separately from the -maxconnections limit. $ bitcoind -h | grep -A3 maxconnections= -maxconnections=<n> Maintain at most <n> connections to peers (default: 125). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of 8. ``` `addnode` rpc help ``` $ bitcoin-cli help addnode addnode "node" "command" Attempts to add or remove a node from the addnode list. Or try a connection to a node once. Nodes added using addnode (or -connect) are protected from DoS disconnection and are not required to be full nodes/support SegWit as other outbound peers are (though such peers will not be synced from). Addnode connections are limited to 8 at a time and are counted separately from the -maxconnections limit. ``` ACKs for top commit: prayank23: ACK b4fcbcf jarolrod: ACK b4fcbcf Tree-SHA512: b6d69baa6cbf6d53f91bac5b39b549d49db6c95f92ea1bdd3588a6432794a25ac2c8b3c89e2c72bb9097e61f2717c8b5ecc404745d5992b88e523db03200898f

…maxconnections config options b4fcbcf doc: update -maxconnections config option help (Jon Atack) 79685a8 doc: update -addnode config option help (Jon Atack) 2896c6c doc: update addnode rpc help (Jon Atack) Pull request description: Since bitcoin#9319 proposed by Gregory Maxwell and released in v0.14, peers manually added through the `-addnode` config option or using the `addnode` RPC have their own separate limit of 8 connections that does not compete with other inbound or outbound connection usage and is not subject to the limitation imposed by the `-maxconnections` option. This PR updates the `-addnode` and `-maxconnections` config options and the `addnode` RPC help docs with this information. `-addnode` config option help ``` $ bitcoind -h | grep -A5 addnode= -addnode=<ip> Add a node to connect to and attempt to keep the connection open (see the addnode RPC help for more info). This option can be specified multiple times to add multiple nodes; connections are limited to 8 at a time and are counted separately from the -maxconnections limit. $ bitcoind -h | grep -A3 maxconnections= -maxconnections=<n> Maintain at most <n> connections to peers (default: 125). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of 8. ``` `addnode` rpc help ``` $ bitcoin-cli help addnode addnode "node" "command" Attempts to add or remove a node from the addnode list. Or try a connection to a node once. Nodes added using addnode (or -connect) are protected from DoS disconnection and are not required to be full nodes/support SegWit as other outbound peers are (though such peers will not be synced from). Addnode connections are limited to 8 at a time and are counted separately from the -maxconnections limit. ``` ACKs for top commit: prayank23: ACK bitcoin@b4fcbcf jarolrod: ACK b4fcbcf Tree-SHA512: b6d69baa6cbf6d53f91bac5b39b549d49db6c95f92ea1bdd3588a6432794a25ac2c8b3c89e2c72bb9097e61f2717c8b5ecc404745d5992b88e523db03200898f

fanquake added the P2P label Dec 11, 2016

sipa reviewed Dec 11, 2016

View reviewed changes

fanquake added the Refactoring label Dec 11, 2016

paveljanik reviewed Dec 11, 2016

View reviewed changes

doc/release-notes.md Outdated

Copy link

Contributor

paveljanik Dec 11, 2016

Choose a reason for hiding this comment

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

What do you mean by "aggressive"connection here?

paveljanik reviewed Dec 11, 2016

View reviewed changes

maflcko added this to the 0.14.0 milestone Dec 25, 2016

sipa reviewed Dec 27, 2016

View reviewed changes

theuni approved these changes Jan 2, 2017

View reviewed changes

gmaxwell added 3 commits January 5, 2017 19:02

Add release notes for addnode changes.

90f13e1

RPC help documentation for addnode peerinfo.

032ba3f

Also adds a comment about the netgroup exclusion behavior.

sipa merged commit 032ba3f into bitcoin:master Jan 6, 2017

droark reviewed Jan 7, 2017

View reviewed changes

maflcko mentioned this pull request Jan 7, 2017

Use TravisCI to enforce copyright header rules for source files #9452

Closed

droark pushed a commit to droark/bitcoin that referenced this pull request Jan 7, 2017

Remove stray semicolon (Fix empty body warning)

cc05896

Empty body introduced by commit bitcoin#9319 should not be empty.

droark mentioned this pull request Jan 7, 2017

Remove stray semicolon (Fix empty body warning) #9487

Merged

sipa mentioned this pull request Jan 23, 2017

Headers-first is disconnecting nodes added via config. #5097

Open

SwissKnife1000 mentioned this pull request Apr 30, 2018

PORT::::::::Break addnode out from the outbound connection limits TO GAPCOIN MASTER gapcoin/gapcoin#7

Closed

lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jan 10, 2019

Remove stray semicolon (Fix empty body warning)

376dc0f

Empty body introduced by commit bitcoin#9319 should not be empty.

wkarshat mentioned this pull request Jun 18, 2019

Inbound Outbound Connections ZcashFoundation/parity-zcash#26

Open

jonatack mentioned this pull request Apr 18, 2021

doc: update helps for addnode rpc and -addnode/-maxconnections config options #21710

Merged

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

Break addnode out from the outbound connection limits. #9319

Break addnode out from the outbound connection limits. #9319

Uh oh!

Conversation

gmaxwell commented Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodeShark commented Dec 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 15, 2016

Uh oh!

TheBlueMatt commented Dec 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell Dec 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Dec 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Dec 30, 2016

Uh oh!

kanoi commented Jan 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 1, 2017

Uh oh!

kanoi commented Jan 2, 2017

Uh oh!

TheBlueMatt commented Jan 2, 2017

Uh oh!

kanoi commented Jan 2, 2017

Uh oh!

TheBlueMatt commented Jan 2, 2017

Uh oh!

kanoi commented Jan 2, 2017

Uh oh!

TheBlueMatt commented Jan 2, 2017

Uh oh!

kanoi commented Jan 2, 2017

Uh oh!

sipa commented Jan 2, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jan 4, 2017

gmaxwell commented Dec 11, 2016 •

edited

Loading

gmaxwell Dec 29, 2016 •

edited

Loading

sipa commented Dec 30, 2016 •

edited

Loading

kanoi commented Jan 1, 2017 •

edited

Loading

sipa commented Jan 2, 2017 via email •

edited

Loading