Break addnode out from the outbound connection limits.

gmaxwell commented at 5:01 am on December 11, 2016: contributor

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.

fanquake added the label P2P on Dec 11, 2016

in src/init.cpp: in 3708f01e62 outdated

873+    nMaxConnections = std::max(std::min(nMaxConnections, (int)(FD_SETSIZE - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS)), 0);
874+    nFD = RaiseFileDescriptorLimit(nMaxConnections + MIN_CORE_FILEDESCRIPTORS + MAX_ADDNODE_CONNECTIONS);
875     if (nFD < MIN_CORE_FILEDESCRIPTORS)
876         return InitError(_("Not enough file descriptors available."));
877-    nMaxConnections = std::min(nFD - MIN_CORE_FILEDESCRIPTORS, nMaxConnections);
878+    nMaxConnections = std::min(nFD - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS, nMaxConnections);

sipa commented at 5:06 am on December 11, 2016:

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

gmaxwell commented at 5:09 am on December 11, 2016:

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

fanquake added the label Refactoring on Dec 11, 2016

CodeShark commented at 8:32 am on December 11, 2016: contributor

concept ACK

in doc/release-notes.md: in 3708f01e62 outdated

67+
68+- Peers manually added through the addnode option or addnode RPC now have their own
69+  limit of eight connections which does not compete with other inbound or outbound
70+  connection usage and is not subject to the maxconnections limitation.
71+
72+- New connections to manually added peers are much faster and more aggressive.

paveljanik commented at 5:29 pm on December 11, 2016:

What do you mean by “aggressive"connection here?

in src/net_processing.cpp: in 3708f01e62 outdated

2574@@ -2575,6 +2575,8 @@ bool SendMessages(CNode* pto, CConnman& connman)
2575             state.fShouldBan = false;
2576             if (pto->fWhitelisted)
2577                 LogPrintf("Warning: not punishing whitelisted peer %s!\n", pto->addr.ToString());
2578+            else if (pto->fAddnode)

paveljanik commented at 5:32 pm on December 11, 2016:

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

gmaxwell commented at 8:07 pm on December 11, 2016:

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

in src/rpc/net.cpp: in 3708f01e62 outdated

151@@ -152,6 +152,7 @@ UniValue getpeerinfo(const JSONRPCRequest& request)
152         // their ver message.
153         obj.push_back(Pair("subver", stats.cleanSubVer));
154         obj.push_back(Pair("inbound", stats.fInbound));
155+        obj.push_back(Pair("addnode", stats.fAddnode));

paveljanik commented at 5:32 pm on December 11, 2016:

addnode missing in the helptext.

gmaxwell commented at 11:42 pm on December 11, 2016:

fixed.

laanwj commented at 4:53 pm on December 15, 2016: member

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 commented at 9:04 pm on December 24, 2016: member

ACK b8d170dab441896970d5ab0f15ca672173592065

Would very much like to see this for 0.14.

MarcoFalke added this to the milestone 0.14.0 on Dec 25, 2016

in src/net.cpp: in ad21bf5e31 outdated

1792                 MilliSleep(500);
1793             }
1794         }
1795-
1796-        MilliSleep(120000); // Retry every 2 minutes
1797+        MilliSleep(tried ? 60000 : 2000); // Retry every 60 seconds if a connection was attempted, otherwise two seconds

sipa commented at 4:29 pm on December 27, 2016:

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?

gmaxwell commented at 6:41 pm on December 29, 2016:

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.

theuni commented at 7:02 pm on January 2, 2017:

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.

in src/net.cpp: in ad21bf5e31 outdated

1625@@ -1625,7 +1626,7 @@ void CConnman::ThreadOpenConnections()
1626         {
1627             LOCK(cs_vNodes);
1628             BOOST_FOREACH(CNode* pnode, vNodes) {
1629-                if (!pnode->fInbound) {
1630+                if (!pnode->fInbound && !pnode->fAddnode) {

sipa commented at 4:36 pm on December 27, 2016:

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”?

gmaxwell commented at 6:33 pm on December 29, 2016:

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

sipa commented at 6:38 pm on December 29, 2016:

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

gmaxwell commented at 7:01 pm on December 29, 2016:

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

TheBlueMatt commented at 7:05 pm on December 29, 2016:

Sure, always add comments :).

gmaxwell commented at 2:56 am on December 30, 2016:

Done

sipa commented at 1:24 pm on December 30, 2016: member

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

sipa commented at 1:48 pm on December 30, 2016: member

utACK 1c7dcacabd7327273a138aec4fd74a404b32e596

kanoi commented at 10:58 pm on January 1, 2017: none

Missing command line and RPC options to set the limit.

MarcoFalke commented at 11:45 pm on January 1, 2017: member

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

kanoi commented at 0:35 am on January 2, 2017: none

That discussion is only about RPC.

TheBlueMatt commented at 1:09 am on January 2, 2017: member

@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 commented at 1:40 am on January 2, 2017: none

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 commented at 1:47 am on January 2, 2017: member

@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 commented at 1:55 am on January 2, 2017: none

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 commented at 1:59 am on January 2, 2017: member

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

kanoi commented at 2:14 am on January 2, 2017: none

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 commented at 2:16 am on January 2, 2017: member

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 approved

theuni commented at 7:31 pm on January 2, 2017: member

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

TheBlueMatt commented at 10:29 pm on January 4, 2017: member

Needs rebase, should be easy.

laanwj commented at 10:59 am on January 5, 2017: member

Conflicts:

 0<<<<<<< 406f35d99d0fcd2f50370280e605283d04da466a
 1                OpenNetworkConnection(CAddress(service, NODE_NONE), false, &grant, info.strAddedNode.c_str(), false);
 2                if (!interruptNet.sleep_for(std::chrono::milliseconds(500)))
 3                    return;
 4||||||| merged common ancestors
 5                OpenNetworkConnection(CAddress(service, NODE_NONE), false, &grant, info.strAddedNode.c_str(), false);
 6                MilliSleep(500);
 7=======
 8                OpenNetworkConnection(CAddress(service, NODE_NONE), false, &grant, info.strAddedNode.c_str(), false, false, true);
 9                MilliSleep(500);
10>>>>>>> Break addnode out from the outbound connection limits.
11            }
12        }
13<<<<<<< 406f35d99d0fcd2f50370280e605283d04da466a
14        if (!interruptNet.sleep_for(std::chrono::minutes(2)))
15            return;
16||||||| merged common ancestors
17
18        MilliSleep(120000); // Retry every 2 minutes
19=======
20        MilliSleep(tried ? 60000 : 2000); // Retry every 60 seconds if a connection was attempted, otherwise two seconds
21>>>>>>> Break addnode out from the outbound connection limits.

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.

50bd12ce0c

Add release notes for addnode changes. 90f13e1822

RPC help documentation for addnode peerinfo.

Also adds a comment about the netgroup exclusion behavior.

032ba3f066

gmaxwell commented at 7:03 pm on January 5, 2017: contributor

rebased.

sipa commented at 7:27 pm on January 5, 2017: member

utACK 032ba3f0665432bd15ff76ee01cde245ad29e3e6

TheBlueMatt commented at 10:27 pm on January 5, 2017: member

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

sipa merged this on Jan 6, 2017

sipa closed this on Jan 6, 2017

sipa referenced this in commit a55716abe5 on Jan 6, 2017

in src/net.cpp: in 032ba3f066

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

droark commented at 6:53 am on January 7, 2017:

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

0net.cpp:1807:75: warning: if statement has empty body [-Wempty-body]
1        if (!interruptNet.sleep_for(std::chrono::seconds(tried ? 60 : 2)));
2                                                                          ^
3net.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?

MarcoFalke commented at 2:04 pm on January 7, 2017:

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

droark commented at 5:51 pm on January 7, 2017:

Thanks. Just filed #9487.

droark referenced this in commit cc0589639c on Jan 7, 2017

btcdrak commented at 6:08 pm on January 7, 2017: contributor

Post humus ACK 032ba3f

JeremyRubin referenced this in commit e0455ba146 on Jan 12, 2017

codablock referenced this in commit de21f92613 on Jan 18, 2018

andvgal referenced this in commit 6fc85fe2c3 on Jan 6, 2019

lateminer referenced this in commit 376dc0fdf4 on Jan 10, 2019

CryptoCentric referenced this in commit 29dd0bf5a5 on Feb 26, 2019

laanwj referenced this in commit b8b6801412 on May 5, 2021

sidhujag referenced this in commit a48c44fabd on May 5, 2021

MarcoFalke locked this on Sep 8, 2021

Break addnode out from the outbound connection limits. #9319