net: only enforce expected services for half of outgoing connections #10441

theuni commented at 5:59 PM on May 22, 2017: member

This ensures that future flags can be turned off after being enabled without causing all outbound nodes to refuse to connect to it in the future for lack of expected services.

Should probably be backported to 0.14 as well.

fanquake added the label Needs backport on May 23, 2017

fanquake added the label P2P on May 23, 2017

laanwj commented at 4:44 PM on May 23, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/10441/commits/0f1e7d3623f37d8ba12ea63543794c90682567bd

jonasschnelli approved

jonasschnelli commented at 6:33 AM on May 24, 2017: contributor

utACK 0f1e7d3623f37d8ba12ea63543794c90682567bd

theuni force-pushed on May 25, 2017

theuni commented at 3:03 PM on May 25, 2017: member

Updated to fix an issue pointed out by @gmaxwell

sdaftuar commented at 3:43 PM on May 25, 2017: member

utACK 7e1718d

morcos commented at 3:44 PM on May 25, 2017: member

utACK 7e1718d

jonasschnelli commented at 6:51 PM on May 25, 2017: contributor

(re)utACK 7e1718d389d6cb1223124cefa594f136f56ec4d1

gmaxwell commented at 7:31 PM on May 25, 2017: contributor

Perhaps consider bracing the conditionals? (at least the new ones?)

theuni force-pushed on May 26, 2017

theuni commented at 5:06 AM on May 26, 2017: member

@gmaxwell done. diff from before is cosmetic only:

diff --git a/src/net.cpp b/src/net.cpp
index 1e50cfb..22d26a9 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1796,11 +1796,13 @@ void CConnman::ThreadOpenConnections()
 
             // only consider nodes missing relevant services after 40 failed attempts and only if less than half the outbound are up.
             ServiceFlags nRequiredServices = nRelevantServices;
-            if (nTries >= 40 && nOutbound < (nMaxOutbound >> 1))
+            if (nTries >= 40 && nOutbound < (nMaxOutbound >> 1)) {
                 nRequiredServices = REQUIRED_SERVICES;
+            }
 
-            if ((addr.nServices & nRequiredServices) != nRequiredServices)
+            if ((addr.nServices & nRequiredServices) != nRequiredServices) {
                 continue;
+            }
 
             // do not allow non-default ports, unless after 50 invalid addresses selected already
             if (addr.GetPort() != Params().GetDefaultPort() && nTries < 50)

in src/net.cpp:1816 in 4cc2e4d0c0 outdated

1812 |              addrConnect = addr;
1813 | +
1814 | +            // regardless of the services assumed to be available, only require the minimum if half or more outbound have relevant services
1815 | +            if (nOutboundRelevant >= (nMaxOutbound >> 1)) {
1816 | +                addrConnect.nServices = REQUIRED_SERVICES;
1817 | +            } else {

sipa commented at 6:54 PM on May 26, 2017:

I think this else branch is unnecessary?

theuni commented at 8:49 PM on May 26, 2017:

It possibly downgrades the requirement from line 1799.

sipa commented at 7:07 AM on May 28, 2017:

Indeed!

sipa commented at 7:10 AM on May 28, 2017: member

net.cpp: In member function ‘void CConnman::ThreadOpenConnections()’:
net.cpp:1732:115: warning: self-comparison always evaluates to true [-Wtautological-compare]
                     if (pnode->fSuccessfullyConnected && !pnode->fFeeler && (pnode->nServices & nRelevantServices == nRelevantServices)) {
                                                                                                 ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
net.cpp:1732:115: warning: suggest parentheses around comparison in operand of ‘&’ [-Wparentheses]

theuni force-pushed on May 28, 2017

theuni commented at 2:18 PM on May 28, 2017: member

Yikes, thanks. Pushed the fixup.

net: only enforce the services required to connect

also once half of all outgoing nodes have our preferred flags, require only
minimal flags from the rest.

b6fbfc2282

laanwj merged this on Jun 1, 2017

laanwj closed this on Jun 1, 2017

laanwj referenced this in commit cb1716acc7 on Jun 1, 2017

laanwj referenced this in commit 9e3ad50078 on Jun 1, 2017

TheBlueMatt commented at 2:36 PM on June 1, 2017: member

utACK b6fbfc228236947eaea5c14dda299f5a01810e92

nomnombtc referenced this in commit d3f477498e on Jul 17, 2017

laanwj removed the label Needs backport on Sep 5, 2017

codablock referenced this in commit c716ee03f1 on Jan 26, 2018

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

andvgal referenced this in commit 7766b15914 on Jan 6, 2019

CryptoCentric referenced this in commit bf4ac85fc6 on Feb 27, 2019

DrahtBot locked this on Sep 8, 2021