-
Notifications
You must be signed in to change notification settings - Fork 448
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
feat: auto relay network query #749
Conversation
baf0ff0
to
6bbd819
Compare
7073965
to
26648a5
Compare
return | ||
} | ||
|
||
remoteMultiaddr = remoteAddrs[0].multiaddr |
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.
While there is no support for signed peer records via delegate nodes (HTTP API), we need to fallback 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.
This address selection logic isn't great. If we don't have a certified record we might just be taking their private address if it's first (which I am concerned could be pretty common). The known public relays won't have this problem because they don't advertise private addresses, but we should be doing some level of sorting here to get the "best" address
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 added a sorter. In follow up PRs, I will add the support for custom sort function for dial and custom filter for announce. Then, it will be bubbled up to here. But I wanted to keep things simple in this PR
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 like the separation of the circuit transport file 👍
// Try to find relays to hop on the network | ||
try { | ||
const cid = await namespaceToCid(RELAY_RENDEZVOUS_NS) | ||
for await (const provider of this._libp2p.contentRouting.findProviders(cid)) { |
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.
We don't have support to do this right now, but it would be great to actually prioritize the results of this based on latency of the connection. Worth adding an issue for a future enhancement here I think
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, that would be good! I can create an issue for that. We should get that after the libp2p.discovery
API, also with the rendezvous
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.
26648a5
to
8060470
Compare
2f00f6c
to
3a119dc
Compare
8f40eb6
to
72fe150
Compare
3a119dc
to
6abea77
Compare
6abea77
to
57a07f1
Compare
return | ||
} | ||
|
||
remoteMultiaddr = remoteAddrs[0].multiaddr |
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.
This address selection logic isn't great. If we don't have a certified record we might just be taking their private address if it's first (which I am concerned could be pretty common). The known public relays won't have this problem because they don't advertise private addresses, but we should be doing some level of sorting here to get the "best" address
285bd0b
to
51e9e95
Compare
Co-authored-by: Jacob Heun <jacobheun@gmail.com>
51e9e95
to
8147383
Compare
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.
Last couple things otherwise this looks good
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.
LGTM, I'll leave the rebase/squash in your hands in case there are other branches you have based on this one.
This PR add to the auto-relay ambient discovery, the ability to query the network for providers of the hop relay service. This will be performed if there are not enough peers connected/known with that capability.
The concept of the libp2p discovery serviced is added here (still not formally, but will be once we integrate rendezvous).
The Circuit was decoupled into two parts, the Circuit Transport and the Relay Service. This decoupling is important, so that we have lifecycle methods (start and stop for the relay). These methods will be used for calling the
discovery.advertise
similarly to go-libp2p's relay implementation, as well as to future active relay service network query on start.The advertise options were added with the same defaults as go
Needs: