[Net] Retry feeler connection if OpenNetworkConnection fails in under 1ms. #8695

pstratem commented at 12:47 AM on September 10, 2016: contributor

Avoids issues with feelers to ip space we don't know how to route.

pstratem force-pushed on Sep 10, 2016

MarcoFalke added the label P2P on Sep 11, 2016

laanwj commented at 3:26 PM on September 13, 2016: member

Doesn't this need a protection to only do this once? (we wouldn't want to repeat failing commands every 1ms)

pstratem commented at 4:40 PM on September 13, 2016: contributor

@laanwj The address being connected it selected new each time the loop runs, because of how the logic works it won't try the same thing in a loop.

laanwj commented at 4:35 PM on September 14, 2016: member

Sure, so it won't 'DoS' a single host, but what if there is a connection problem with a whole IP range? E.g. from the local network to the internet. Then you don't want to keep retrying every ms either.

pstratem commented at 12:36 PM on September 15, 2016: contributor

@laanwj It re-tries every 500ms

gmaxwell commented at 9:53 PM on September 18, 2016: contributor

ACK. Maybe it could use a comment to point out that it uses and depends on the outer loop sleep.

pstratem commented at 10:25 PM on November 14, 2016: contributor

rebased and comment added

pstratem renamed this:
~~Retry feeler connection if OpenNetworkConnection fails in under 1ms.~~
[Net] Retry feeler connection if OpenNetworkConnection fails in under 1ms.
on Nov 14, 2016

in src/net.cpp:None in ce1e69ab93 outdated

1740 | +            bool fRet = OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, NULL, false, fFeeler);
1741 | +
1742 | +            // Retry feeler connection if OpenNetworkConnection fails in under 1ms.
1743 | +            // Avoids issues with feelers to IP space we don't know how to route.
1744 | +            if (!fRet && GetTimeMicros() - nStartTime < 1000) {
1745 | +                nNextFeeler = GetTime();

sipa commented at 6:37 PM on November 15, 2016:

nNextFeeler is in microseconds, so you should reuse the value of the call to GetTimeMicros instead (or just assign 0, which will have the same effect).

pstratem commented at 7:59 PM on December 20, 2016:

Fixed.

sipa commented at 4:44 PM on December 27, 2016:

Not fixed.

pstratem commented at 6:44 PM on February 3, 2017:

I changed it to be 0 but i've now changed it to GetTimeMicros()

TheBlueMatt commented at 7:03 PM on February 3, 2017:

Did you forget to push?

gmaxwell commented at 2:16 AM on December 2, 2016: contributor

ping @pstratem

in src/net.cpp:None in ce1e69ab93 outdated

1733 | @@ -1734,7 +1734,15 @@ void ThreadOpenConnections()
1734 |                  LogPrint("net", "Making feeler connection to %s\n", addrConnect.ToString());
1735 |              }
1736 |  
1737 | -            OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, NULL, false, fFeeler);
1738 | +            int64_t nStartTime = GetTimeMicros();
1739 | +
1740 | +            bool fRet = OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, NULL, false, fFeeler);

rebroad commented at 4:48 AM on December 11, 2016:

nit: "ret" seems to be the standard so far, with 69 instances, compared to only 10 instances of "fRet" in this project.

pstratem commented at 7:59 PM on December 20, 2016:

I'm matching the style of code around this, all variable names start with their type.

rebroad commented at 5:28 AM on December 27, 2016: contributor

tACK

Retry feeler connection if OpenNetworkConnection fails in under 1ms.

Avoids issues with feelers to IP space we don't know how to route.

7d18e69e51

pstratem force-pushed on Feb 4, 2017

in src/net.cpp:None in 7d18e69e51

1707 | +            bool fRet = OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, NULL, false, fFeeler);
1708 | +
1709 | +            // Retry feeler connection if OpenNetworkConnection fails in under 1ms.
1710 | +            // Avoids issues with feelers to IP space we don't know how to route.
1711 | +            // The outer loop has a 500ms delay, we rely on that delay here
1712 | +            if (!fRet && GetTimeMicros() - nStartTime < 1000) {

TheBlueMatt commented at 3:48 PM on February 7, 2017:

Does this need an && fFeeler in ti?

TheBlueMatt commented at 3:48 PM on February 7, 2017: member

Can you provide more justification for this change? Why do only feelers matter (or is it all connections?)? What are the "issues" you are referring to?

pstratem commented at 12:38 AM on February 8, 2017: contributor

@TheBlueMatt Currently if the address selected for the feeler logic is in ipv6 space and you can only route ipv4 you will wait about 120 seconds to retry, this is simply to avoid that delay.

sipa commented at 3:27 PM on April 9, 2017: member

@pstratem Can you address https://github.com/bitcoin/bitcoin/pull/8695/files#r99852785 ?

sipa commented at 5:06 PM on March 6, 2018: member

@pstratem Is this going anywhere?

MarcoFalke added the label Up for grabs on Mar 18, 2018

jnewbery commented at 5:19 PM on April 3, 2018: member

No activity. Closing with 'up for grabs' for now. @pstratem - feel free to re-open if you pick this up again.

jnewbery closed this on Apr 3, 2018

MarcoFalke locked this on Sep 8, 2021