Be more agressive in getting connections to peers with relevant services. #8949

gmaxwell commented at 11:19 PM on October 17, 2016: contributor

Currently, we make 40 requests to addrman for peers with our relevant services then give up and take a node without them. When relevant services are rare in our addrman data this means we can get all 8 peers up without them. If they happen to be reliable peers they may stay up filling our connection slots for months, even though our addrman has since been populated with plenty of relevant peers.

To avoid this this PR changes the behavior to be only willing to ignore relevant services until half the outbound connections are up.

Also, dnsseed does not fire if we have at least two connections (in or out) 11 seconds after startup. It currently doesn't perform any sanity checking of the connections-- they could be two lite-client inbound peers that will never send us addr message and our addrman might be filled with stupidity. To improve that somewhat, we check that they at least send the relevant service flags before deciding to not request dnsseeding.

Be more aggressive in connecting to peers with relevant services.

Only allow skipping relevant services until there are four outbound
 connections up.

This avoids quickly filling up with peers lacking the relevant
 services when addrman has few or none of them.

9583477288

sipa commented at 12:04 AM on October 18, 2016: member

Concept ACK.

Things that come to mind:

What happens when no relevant-flags peers are available at all?
Should CConnman::ForEachNode be used instead of an explicit loop (may complicate porting to 0.13)?

gmaxwell commented at 12:17 AM on October 18, 2016: contributor

What happens when no relevant-flags peers are available at all?

My intention is that it would get four connections up, and it would continue polling addrman until it finally got something relevant. Even absent seeding, eventually it should learn something via the four that are up or via feelers.

The alternative I considered to the connect-to-relevant commit was adding something to periodically disconnect one of the peers without relevant services if all outbound slots were full. I decided this would be a more invasive change. A third alternative would be to allow a feeler with relevant services to replace a peer without relevant services; this would still probably be a more invasive change (but maybe only 3 LOC, e.g. if this is a feeler, and relevant, scan outbound peers, and if you find a non-relevant one, set disconnect on it instead).

I did not consider any alternatives to the DNSseed commit.

I'd rather not do anything here which would complicate a 0.13 backport.

fanquake added the label P2P on Oct 18, 2016

in src/net.cpp:None in 443f8116d1 outdated

1466 | @@ -1467,7 +1467,11 @@ void CConnman::ThreadDNSAddressSeed()
1467 |          MilliSleep(11 * 1000);
1468 |  
1469 |          LOCK(cs_vNodes);
1470 | -        if (vNodes.size() >= 2) {
1471 | +        int nRelevant = 0;
1472 | +        BOOST_FOREACH(CNode* pnode, vNodes) {

suncho commented at 6:42 PM on October 18, 2016:

Why not C++11 range-based for?

MarcoFalke commented at 7:01 PM on October 18, 2016:

This is meant for backport, so no cpp11

sipa commented at 7:02 PM on October 18, 2016:

Only backport to 0.13 though, which does have c++11?

rebroad commented at 1:10 AM on October 19, 2016: contributor

@gmaxwell What are the disadvantages of forcing dns seeding on every startup? (Or even running a re-seed every 24 hours)?

laanwj added the label Needs backport on Oct 19, 2016

laanwj added this to the milestone 0.13.1 on Oct 19, 2016

laanwj commented at 7:39 AM on October 19, 2016: member

@gmaxwell What are the disadvantages of forcing dns seeding on every startup? (Or even running a re-seed every 24 hours)?

Increasing load on the DNS seeds, as well as privacy - when the DNS seed gets a request every time it still knows you're running a node. For these reasons the number of requests to DNS seeds is minimized (see also #4559). There is an option to force querying them though.

btcdrak commented at 7:42 AM on October 19, 2016: contributor

Tested both with and without the patch. I was unable to replicate the poor behaviour without the patch, but observed no ill effects with the patch, which seems logical in any case.

tACK 443f8116d1c4e2fd28c4730f085db41d9088681b @rebroad DNS seeds are more of a way for new nodes to get started to join the network which once joined, they can learn about other nodes directly from the network. We shouldn't be putting too much reliance on the seeds.

Make dnsseed's definition of acute need include relevant services.

We normally prefer to connect to peers offering the relevant services.

If we're not connected to enough peers with relevant services, we
 probably don't know about them and could use dnsseed's help.

4630479135

gmaxwell commented at 8:09 AM on October 19, 2016: contributor

I added a comment that explains why we avoid the dnsseed and switched to the range based for (for some reason I thought that was too C++11 for 0.13). Thanks all.

laanwj merged this on Oct 19, 2016

laanwj closed this on Oct 19, 2016

laanwj referenced this in commit e44753c067 on Oct 19, 2016

btcdrak commented at 8:54 AM on October 19, 2016: contributor

re utACK 46304791353d2bb61004a035869612620c30b4eb

laanwj referenced this in commit 33cd5539b2 on Oct 19, 2016

laanwj referenced this in commit 91ae0b06b9 on Oct 19, 2016

MarcoFalke removed the label Needs backport on Oct 19, 2016

laanwj referenced this in commit 0dbc48a5bd on Oct 19, 2016

jtimon referenced this in commit b69d8b2b27 on Feb 2, 2017

jtimon referenced this in commit 59f1dd5403 on Feb 2, 2017

jtimon referenced this in commit 777da7d69c on Feb 2, 2017

codablock referenced this in commit 785d4d0ee5 on Jan 26, 2018

lateminer referenced this in commit 784ceae891 on Oct 18, 2018

lateminer referenced this in commit 7b0cd213fc on Oct 18, 2018

lateminer referenced this in commit a14ed45674 on Oct 18, 2018

andvgal referenced this in commit 4a8c2a5630 on Jan 6, 2019

andvgal referenced this in commit 540d58d641 on Jan 6, 2019

CryptoCentric referenced this in commit cd619ac244 on Feb 27, 2019

MarcoFalke locked this on Sep 8, 2021