-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ObsAddrManager: Infer external addresses for transports that share the same listening address. #2892
Conversation
Not quite :( |
6ec6085
to
2ad6c5c
Compare
p2p/protocol/identify/obsaddr.go
Outdated
twToObserverSets[localTWStr] = append(twToObserverSets[localTWStr], o.getTopExternalAddrs(localTWStr)...) | ||
} | ||
} | ||
for _, a := range o.listenAddrs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll need to infer for interfaceListenAddrs
also.
It's okay for the time being since that's only relevant for tcp and we don't share tcp addresses with any other transport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not completely sure about this but I think this is what happens.
When listening on 0.0.0.0
on tcp, the listen address is 0.0.0.0
but conn.LocalMultiAddr()
on the connection is something like 192.168.1.34
. which is the specific network interface that was used for the connection.
On UDP, conn.LocalMultiAddr()
is always 0.0.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I'm now doing it on both listenAddrs and interfaceListenAddrs. I think in the ideal world we would just use interfaceListenAddrs
because those are what map to the 4-tuple for connection flows. For UDP right now, this doesn't work yet as you say. So we have to do it for both because the 0.0.0.0 is what shows up in the observed address mapping due to it being the conn.LocalMultiAddr result.
addrs = append(addrs, s.cacheMultiaddr(t.Rest)) | ||
} | ||
} | ||
return addrs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do a ma.Unique
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where we have multiple identical normalized listen addresses?
2ad6c5c
to
ad98c91
Compare
ad98c91
to
2666c46
Compare
The Observed Address manager can now infer external addresses for transports that share the same listening address if another transport has an observation.
For example, say I'm listening on UDP 0.0.0.0:4001 for QUIC, WebTransport, and WebRTC. And I learn that my external IP address for QUIC is 5.4.3.2:4001 (as reported by my peers). This means I can infer that my external WebTransport and WebRTC addresses are also 5.4.3.2:4001 since these other transports are using the same underlying socket.
This PR also removes the old hack that inferred WebTransport address. It was hacky because we didn't have access to the connection of external <-> local address like the observed address manager does.