autonat: fix interaction with autorelay #2967
Conversation
sukunrt
force-pushed
the
sukun/addr-factory
branch
5 times, most recently
from
3e54651
to
f79b989
Compare
sukunrt
force-pushed
the
sukun/addr-factory
branch
from
f79b989
to
bc8f18f
Compare
p2p/host/autonat/autonat.go
Outdated
| // We want an update when our public non-relay listen addresses have changed. | ||
| // EvtLocalAddressesUpdated is a poor proxy for that. It works when the host is Public, | ||
| // but fails when the host is private and used with AutoRelay. | ||
| addrChangeTicker := time.NewTicker(1 * time.Minute) |
There was a problem hiding this comment.
Why? Nothing about the doc comment in EvtLocalAddressesUpdated mentions this limitation.
There was a problem hiding this comment.
I've changed the comment. But we should figure out what this bug is.
There was a problem hiding this comment.
There's no bug. The public address is not reachable so we don't advertise it.
Once we obtain relay reservations with AutoRelay, we will only advertise those public circuit v2 addresses and not our unreachable public addresses.
For AutoNAT we need our public, reachable or unreachable, non circuitv2, addresses.
p2p/host/autonat/autonat.go
Outdated
|
|
||
| for _, p := range peers { | ||
| n := len(peers) | ||
| for i, j := rand.Intn(n), 0; j < n; i, j = (i+1)%n, j+1 { |
There was a problem hiding this comment.
I don't understand the point of starting at a random offset. First, the code is slightly confusing. But, more importantly, you probably want to actually shuffle the list, rather than starting at a random offset. Shuffling correctly is surprisingly subtle.
There was a problem hiding this comment.
We are only interested in the first element that satisfies our condition, so I think it was fine to not shuffle the whole thing.
Separately, theres rand.Shuffle if you do want to shuffle.
There was a problem hiding this comment.
We start at a random offset to not rely on the peerstore's ordering of peers.
There was a problem hiding this comment.
TIL about rand.Shuffle. We should make a PR that updates our usages of shuffling throughout the codebase.
I think we do want to shuffle here, although I don't think I could prove why.
| @@ -399,33 +399,30 @@ func (as *AmbientAutoNAT) getPeerToProbe() peer.ID { | |||
| return "" | |||
| } | |||
|
|
|||
| candidates := make([]peer.ID, 0, len(peers)) | |||
| // clean old probes | |||
There was a problem hiding this comment.
Do we need to decrement pending probes?
Nevermind, this is about our throttling, not about outstanding requests. May help to wordsmith the comment.
MarcoPolo
force-pushed
the
sukun/addr-factory
branch
from
ada26c4
to
71c66ee
Compare
* holepunch: pass address function in constructor * nit * Remove getPublicAddrs --------- Co-authored-by: Marco Munizaga <[email protected]>
p2p/host/autonat/autonat.go
Outdated
|
|
||
| for _, p := range peers { | ||
| n := len(peers) | ||
| for i, j := rand.Intn(n), 0; j < n; i, j = (i+1)%n, j+1 { |
p2p/host/autonat/autonat.go
Outdated
| // We want an update when our public non-relay listen addresses have changed. | ||
| // EvtLocalAddressesUpdated is a poor proxy for that. It works when the host is Public, | ||
| // but fails when the host is private and used with AutoRelay. | ||
| addrChangeTicker := time.NewTicker(1 * time.Minute) |
There was a problem hiding this comment.
I've changed the comment. But we should figure out what this bug is.
I agree. |
Fixes: #2964