fix: improve connectivity #1128
Conversation
|
/netsim |
|
question tho, what are these ubuntu cli failures? |
|
|
|
FWIW this still fails in netsim for the NAT tests. |
|
You can get tests on your PR running if you modify It should error out on those tests until they pass. |
|
@dignifiedquire you did the wrong one, |
|
It's fine to keep it though if we're going to fix nat stuff in this PR |
That's the plan, it is fixed in my home test setup, when both parties are behind different nats. |
|
When running manually on my end, seems to do more now, but still fails connecting to the provider. |
dignifiedquire
force-pushed
the
fix-odd-issues
branch
from
03c8d58
to
06ab62f
Compare
| self.trust_best_addr_until | ||
| .replace(*now + Duration::from_secs(60 * 60)); |
There was a problem hiding this comment.
maybe trust from last_ping time, rather than now? & if that time exceeds an hour it is no longer a best_addr candidate?
|
/netsim |
|
|
- send full pings on best_addr ping if needed - only store endpoints with known keys
|
/netsim |
|
|
|
Seems like the failures are real connectivity issues. (again with a NAT on the provide side) |
| if let Some(addr) = udp_addr { | ||
| self.best_addr = Some(AddrLatency { | ||
| addr, | ||
| latency: None, | ||
| }); | ||
|
|
||
| self.trust_best_addr_until = Some(*now + Duration::from_secs(15)); |
There was a problem hiding this comment.
Okay, so from looking at logs in the failing cli test, there are two main issues i'm seeing.
This issue is apparent on the get side, since as soon as we connect we try to send something.
Issue One, that needs some more thinking:
The first time we attempt to do a full ping with a callmemaybe over derp is the first time we ever connect to the derp server. This requires a full TLS handshake and takes some time to negotiate. But callmemaybe is sensitive to time. This is highly likely to never hole punch.
Issue Two:
This is related to the actual code I'm commenting on 🤣 . Once we set the best_addr and trust_best_addr, we are potentially condemned to using a bad address for 15 seconds. Plus, since it's considered valid, we won't back it up with a derp address even though we have had no proof (yet) that it is a valid address.
One possible solution would be to keep self.trust_best_addr_until None, until a full_ping comes back successfully. But, this means that if another pong has come back on a different address during this time, we won't ever attempt to use it, because we still have a bad address as the best_addr.
I'm proposing, instead, that we never set an address as a best_addr unless it has a latency.
So that would mean just removing these lines. (this fixes the bugs, locally for me, at least).
if we are worried about candidate address flip/flopping, we can keep track of a candidate_addr, that is the address we have randomly chosen as the default to use until we get any pongs back. We can loop through the addresses, and if none have any latency, we default to using the previous candiate_addr.
There was a problem hiding this comment.
are potentially condemned to using a bad address for 15 seconds.
Well only for 5s, as the pings get sent out and if we time out, it gets removed.
Plus, since it's considered valid,
this was the big problem, we drop derp because we think it is valid, forgot my own logic last night..
Should improve #1098
Fixes #1084