-
Notifications
You must be signed in to change notification settings - Fork 157
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(p2p): implement hole punching #126
Conversation
0ea074a
to
5c5c1b0
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.
Did a quick skim, pretty small surface and LGTM, let's get this merged and gather some data
🚀 I will take a closer look. |
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.
Let me know how well this is working. Also let me know in case you need any additional infrastructure to test this against.
(Hole punching client side is slowly rolling out on IPFS, thus you might as well already be able to punch with a Golang client.)
Again, happy to see this happening!
relay_client: Toggle<relay::v2::client::Client>, | ||
dcutr: Toggle<dcutr::behaviour::Behaviour>, |
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.
Ideally we would have an AndBehaviour
, that way you could do:
hole_punching_client: Toggle<AndBehaviour<relay::v2::client::Client, dcutr::behaviour::Behaviour>>`
That way it is enforced at compile time that you always either enable both or disable both.
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.
yep, that would would be nice
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.
👍 Tracked here: libp2p/rust-libp2p#2719
// TODO: should we remove them at some point? | ||
if protocols | ||
.iter() | ||
.any(|p| p.as_bytes() == b"/libp2p/autonat/1.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.
How about exposing the protocol ID as a constant from libp2p-autonat
and using that here instead?
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.
yep, that's the todo above, I can make a 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.
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.
yep, that's the todo above
Uuups 🤦
Thanks!
.as_mut() | ||
.map(|k| k.add_address(&peer_id, addr)); | ||
if let Some(autonat) = self.swarm.behaviour_mut().autonat.as_mut() { | ||
autonat.add_server(peer_id, Some(addr)); |
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.
As mentioned elsewhere, using autonat::Config::use_connected
might be good enough. Though adding them explicitly and thus knowing for sure that they support the autonat protocol is likely the better bet.
For the record, this would be simplified with libp2p/rust-libp2p#2680.
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 wanted to be sure 😅 so did both for now
let transport = transport | ||
.upgrade(core::upgrade::Version::V1Lazy) | ||
.authenticate(auth_config) | ||
.multiplex(mplex_config) |
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.
Not related to this pull request, though very relevant in general, why use mplex
and not yamux
or both with a preference on yamux
?
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.
if you look a little above it is actually both mplex and yamux just the name is wrong 😅
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.
👍 Please keep prioritizing Yamux over Mplex.
.boxed() | ||
if config.relay_client { | ||
let (relay_transport, relay_client) = | ||
libp2p::relay::v2::client::Client::new_transport_and_behaviour( |
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.
Hope the creation of the NetworkBehaviour
and the Transport
was OK to follow. Tried to make it impossible to missuse at compile time, likely at the expense of being easy to use.
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.
yep, a little unfortunate to have to pass these around, but was quite clear from the types and the example, what needed to be done 👍
First indicators are good, suggesting an increase in addrs we are able to dial succesfully |
@mxinden currently there is no metrics implementation for |
refactor: make on_collection take a reference
libp2p::autonat
libp2p::dcutr
andlibp2p::relay::v2
Closes n0-computer/beetle#240