parallelize dialbacks #20
Conversation
ghost
assigned 
|
Looking 👀 |
| private := 0 | ||
| public := 0 | ||
|
|
||
| probe := 3 - as.confidence |
There was a problem hiding this comment.
Could do:
var probe int
if probe = 3 - as.confidence; probe > len(peers) {
probe = len(peers)
} else if probe == 0 {
probe = 1
}There was a problem hiding this comment.
sure, can do if you think it's cleaner this way.
There was a problem hiding this comment.
there is a problem with doing it this way: if peers = 0 and probe is also 0 (confidence = 3), then we'll have an out of bounds slice.
then again we can't possibly have no peers and confidence = 3, so it should be ok.
There was a problem hiding this comment.
but I'd rather not have to think about this when reading the code (apparent bug that is not)
There was a problem hiding this comment.
I think I'll keep the original code here, as it needs no comments.
| for _, pi := range peers[:probe] { | ||
| wg.Add(1) | ||
| go func(pi pstore.PeerInfo) { | ||
| as.host.Peerstore().AddAddrs(pi.ID, pi.Addrs, pstore.TempAddrTTL) |
There was a problem hiding this comment.
Note to self: using AddAddrs is ok here because if the peerstore has a longer TTL, it won't shorten in.
autonat.go
Outdated
|
|
||
| var mx sync.Mutex | ||
| var pubaddr ma.Multiaddr | ||
| private := 0 |
There was a problem hiding this comment.
Can we enclose these vars in an anonymous struct? It'll be easier to track the scope of these things, and the zero values are our friend here:
var result struct {
sync.Mutex
private int
public int
pubaddr ma.Multiaddr
}Then below:
result.Lock()
result.private++
result.Unlock()There was a problem hiding this comment.
sure, that's nicer indeed.
autonat.go
Outdated
| log.Debugf("Dialback through %s successful; public address is %s", pi.ID.Pretty(), a.String()) | ||
| mx.Lock() | ||
| public++ | ||
| pubaddr = a |
There was a problem hiding this comment.
Is it possible that different peers will report different public endpoints, and we'll overwrite them here? This could happen if we're using different network interfaces for each peer, or if we're behind a corporate NAT that uses different exit points?
There was a problem hiding this comment.
we only report one address, so the override is inevitable. I don't think it matters much though, as we don't currently use the reported address for anything.
| as.confidence++ | ||
| if public > 0 { | ||
| log.Debugf("NAT status is public") | ||
| if as.status == NATStatusPrivate { |
There was a problem hiding this comment.
A comment here saying that we're flipping our asserted NAT status?
There was a problem hiding this comment.
A log comment I take it. Sure, will do.
| log.Debugf("NAT status is private") | ||
| if as.status == NATStatusPublic { |
| } else { | ||
| log.Debugf("NAT status is unknown") |
There was a problem hiding this comment.
Is it ok for a single round with unresponsive peers to reset our NAT status entirely? I think we need some error tolerance here.
There was a problem hiding this comment.
I'd decrement the confidence and retain our previous status, until confidence drops to zero and then we can switch to Unknown?
There was a problem hiding this comment.
Done - won't flip to unknown if we have any confidence, but rather reduce the confidence and wait for the next round.
It will only become unknown again if the confidence drops to 0.
autonat.go
Outdated
| default: | ||
| log.Debugf("Error dialing through %s: %s", pi.ID.Pretty(), err.Error()) | ||
| } | ||
| wg.Done() |
| } | ||
|
|
||
| wg.Wait() |
There was a problem hiding this comment.
Do we need to wait for all of them? That is, could we like, try N and wait for M?
There was a problem hiding this comment.
Ask N nodes for dial backs but walk away after M responses. It may not be worth it but it would allow us to tolerate a few bad nodes.
There was a problem hiding this comment.
Note: I'm find merging this as-is.
There was a problem hiding this comment.
I don't think it adds much to wait for fewer nodes, other than adding complexity. At worst a bad node will timeout, which is 30s.
| log.Debugf("NAT status is public") | ||
| if as.status == NATStatusPrivate { | ||
| // we are flipping our NATStatus, so confidence drops to 0 | ||
| as.confidence = 0 |
There was a problem hiding this comment.
Nit: Shouldn't this be a confidence of 1?
There was a problem hiding this comment.
I prefer this to have a confidence of 0 if there is a flip, so that we get more observations in the next round.
full close the autonat stream
Dialing multiple autonat peers sequentially unnecessarily slows down NAT autodetection.
This changes the detection logic to perform dialbacks in parallel.
cc @ZenGround0