protocols/: Implement Direct Connection Upgrade through Relay (DCUtR) by mxinden · Pull Request #2438 · libp2p/rust-libp2p

mxinden · 2022-01-15T20:33:36Z

Enables two peers to coordinate a hole punch (direct connection upgrade)
via a relayed connection.

See https://github.com/libp2p/specs/blob/master/relay/DCUtR.md for
specification.

Depends on either:

Replaces #2076.

Enables two peers to coordinate a hole punch (direct connection upgrade) via a relayed connection. See https://github.com/libp2p/specs/blob/master/relay/DCUtR.md for specification.

mxinden · 2022-01-19T14:55:05Z

This is the last step needed for basic hole punching in rust-libp2p (see also #2052).

Anyone interested in reviewing? @thomaseizinger maybe? 😇

MarcoPolo

First pass. Looking good! I can't believe this is so simple.

Overall I guess I'm wondering how much of the failure cases have to be handled by the end user vs the library. Things such as:

The direct connection fails but the relay connection still works (can happen if the nat state gets reset for example).
timeouts

and some questions in general from me.

Thanks!

MarcoPolo · 2022-01-29T04:53:10Z

+        }
+    }
+
+    fn inject_listen_upgrade_error(


I know this isn't related at all, but shouldn't these be renamed to inject_error_inbound to match the above inject_fully_negotiated_inbound, or is there some subtlety that I'm missing?

thomaseizinger · 2022-01-31T00:55:02Z

This is the last step needed for basic hole punching in rust-libp2p (see also #2052).

Anyone interested in reviewing? @thomaseizinger maybe? innocent

Sorry, GitHub for some reason filtered this out of my "review requested" list :(

thomaseizinger

Some comments, nothing blocking :)

thomaseizinger · 2022-01-31T00:58:56Z

+            futures::select! {
+                            event = swarm.next() => {
+                                match event.unwrap() {
+                                    SwarmEvent::NewListenAddr { address, .. } => {


Why not return from this if that is what we are waiting for (based on the comment?).

Yes, this is misleading. We are giving Swarm some time to set up listeners. Though it might be more than one, depending on the number of network interfaces, thus returning after a single NewListenAddr might wait for one network interface, but not all.

Waiting for a second instead is a hack, but a hack that works. Happy for alternative suggestions.

At least better documented with 21ae03b.

thomaseizinger · 2022-01-31T00:59:43Z

+        }
+    });
+
+    if matches!(opts.mode, Mode::Dial) {


Nit: Could be a == if you would derive PartialEq.

👍 fixed with 10046f2. I think I previously carried something within the Dial variant, though no longer.

thomaseizinger · 2022-01-31T01:00:11Z

+            .unwrap();
+    }
+
+    block_on(futures::future::poll_fn(move |cx: &mut Context<'_>| {


Why poll_fn here and and async loop above?

Good point. Replaced with async in 128d801.

thomaseizinger · 2022-01-31T01:05:48Z

+/// The events produced by the [`Behaviour`].
+#[derive(Debug)]
+pub enum Event {
+    InitiateDirectConnectionUpgrade {


The naming of this variant doesn't indicate an event but a command. Is that on purpose?

Unless I misunderstand the logic, this could be named InitiatedDirectConnectionUpgrade?

DirectConnectedUpgradeInitiated might be even better but then it is not in line with RemoteInitiatedDirectConnectionUpgrade.

Good idea. Renamed to InitiatedDirectConnectionUpgrade for now with 8384b4e48b27e807a2b066cf21f9aea26b8ac1f7.

I am fine with DirectConnectedUpgradeInitiated as well.

thomaseizinger · 2022-01-31T01:11:04Z

+            let msg = HolePunch {
+                r#type: hole_punch::Type::Sync.into(),
+                obs_addrs: vec![],
+            };
+
+            let mut encoded_msg = BytesMut::new();
+            msg.encode(&mut encoded_msg)
+                .expect("BytesMut to have sufficient capacity.");
+
+            substream.send(encoded_msg.freeze()).await?;


Might be worthwhile to extract some helpers to hide the details of constructing one of these messages from the overall flow.

Good idea. We can even go one step further and implement asynchronous_codec::Encoder and asynchronous_codec::Decoder for a custom Codec that takes care of all the prost::Message encoding, wrapping a UviBytes.

What do you think of 354900f @thomaseizinger?

We could take this one step further, making Codec abstract over prost::Message and thus use the same Codec in all protocols/XXX. What do you think?

thomaseizinger · 2022-01-31T01:18:28Z

+/// A [`NetworkBehaviourAction`], either complete, or still requiring data from [`PollParameters`]
+/// before being returned in [`Behaviour::poll`].
+#[allow(clippy::large_enum_variant)]
+enum Action {


Suggested change

enum Action {

enum ActionBuilder {

👍 Done with f18a863.

thomaseizinger · 2022-01-31T01:21:00Z

+    fn addresses_of_peer(&mut self, _peer_id: &PeerId) -> Vec<Multiaddr> {
+        vec![]
+    }
+
+    fn inject_connected(&mut self, _peer_id: &PeerId) {}


Nit: I think these have a default now.

You are right. Removed with 831deb5.

thomaseizinger · 2022-01-31T01:22:06Z

+                self.queued_actions.push_back(Action::Connect {
+                    peer_id: *peer_id,
+                    attempt: 1,
+                    handler: NotifyHandler::One(*connection_id),
+                });
+                let local_addr = match connected_point {
+                    ConnectedPoint::Listener { local_addr, .. } => local_addr,
+                    ConnectedPoint::Dialer { .. } => unreachable!("Due to outer if."),
+                };
+                self.queued_actions.push_back(
+                    NetworkBehaviourAction::GenerateEvent(Event::InitiateDirectConnectionUpgrade {
+                        remote_peer_id: *peer_id,
+                        local_relayed_addr: local_addr.clone(),
+                    })
+                    .into(),
+                );


This could be written as a single .extend of an array to make it clear that the actions belong together.

Sounds good to me. Done across the whole file with b50708b.

mxinden

Sorry for the delay here, still not yet done going through all the comments. Very much appreciate that you two took the time for a review @MarcoPolo and @thomaseizinger.

mxinden · 2022-02-01T19:34:50Z

+    fn addresses_of_peer(&mut self, _peer_id: &PeerId) -> Vec<Multiaddr> {
+        vec![]
+    }
+
+    fn inject_connected(&mut self, _peer_id: &PeerId) {}


You are right. Removed with 831deb5.

mxinden · 2022-02-01T19:37:49Z

+/// The events produced by the [`Behaviour`].
+#[derive(Debug)]
+pub enum Event {
+    InitiateDirectConnectionUpgrade {


Good idea. Renamed to InitiatedDirectConnectionUpgrade for now with 8384b4e48b27e807a2b066cf21f9aea26b8ac1f7.

I am fine with DirectConnectedUpgradeInitiated as well.

mxinden · 2022-02-01T19:45:10Z

+            .unwrap();
+    }
+
+    block_on(futures::future::poll_fn(move |cx: &mut Context<'_>| {


Good point. Replaced with async in 128d801.

mxinden · 2022-02-01T19:47:11Z

+        }
+    });
+
+    if matches!(opts.mode, Mode::Dial) {


👍 fixed with 10046f2. I think I previously carried something within the Dial variant, though no longer.

mxinden · 2022-02-01T19:52:18Z

+            futures::select! {
+                            event = swarm.next() => {
+                                match event.unwrap() {
+                                    SwarmEvent::NewListenAddr { address, .. } => {


Yes, this is misleading. We are giving Swarm some time to set up listeners. Though it might be more than one, depending on the number of network interfaces, thus returning after a single NewListenAddr might wait for one network interface, but not all.

Waiting for a second instead is a hack, but a hack that works. Happy for alternative suggestions.

At least better documented with 21ae03b.

mxinden · 2022-02-03T17:00:57Z

I addressed all the comments. Thanks for the reviews!

@thomaseizinger and @MarcoPolo would you mind taking another look?

MarcoPolo

TIL about the custom codec. Neat.

mxinden · 2022-02-08T14:56:54Z

Thanks @MarcoPolo and @thomaseizinger for the help here!

protocols/: Implement Direct Connection Upgrade through Relay (DCUtR)

7ea9809

Enables two peers to coordinate a hole punch (direct connection upgrade) via a relayed connection. See https://github.com/libp2p/specs/blob/master/relay/DCUtR.md for specification.

mxinden mentioned this pull request Jan 15, 2022

protocols: Implement Direct Connection Upgrade through Relay (dcutr) #2076

Closed

mxinden added 2 commits January 15, 2022 21:35

misc/metrics: Run rust fmt

e6551aa

protocols/dcutr: Reference merged DCUtR specification

b1cb588

mxinden commented Jan 15, 2022

View reviewed changes

Comment thread protocols/dcutr/src/handler.rs

mxinden added 5 commits January 17, 2022 16:44

Merge branch 'master' of into dcutr-2

c3ac0de

protocols/dcutr: Dial as_listener when DCUtR server

436d565

protocols/dcutr: Filter relayed addresses on protocol level

7bc75a5

protocols/dcutr: Fix intra-doc link

bafef69

Merge branch 'libp2p/master' into dcutr-2

cab9e38

mxinden requested a review from thomaseizinger January 19, 2022 14:53

MarcoPolo reviewed Jan 29, 2022

View reviewed changes

mxinden mentioned this pull request Jan 30, 2022

src/tutorials: Add hole punching tutorial #2460

Merged

thomaseizinger approved these changes Jan 31, 2022

View reviewed changes

mxinden added 8 commits February 1, 2022 20:02

protocols/dcutr/tests: Remove unused libp2p-ping

4a5461c

protocols/dcutr: Fix expect message typo

e7fa5e9

protocols/dcutr: Remove empty Behaviour::inject_connected

831deb5

protocols/dcutr: Rename to InitiatedDirectConnectionUpgrade

c1deddb

protocols/dcutr/examples: Use async instead of poll_fn

128d801

protocols/dcutr/examples: Replace matches! with ==

10046f2

protocols/dcutr/examples: Document delay hack

21ae03b

protocols/dcutr/examples: Remove outdated comment on relay mode

da5e48f

mxinden commented Feb 1, 2022

View reviewed changes

mxinden added 5 commits February 2, 2022 11:38

protocols/dcutr/src/behaviour: Consolidate obs_addrs collection

3b75add

protocols/dcutr: Document reversed connection and substream roles

9d99850

protocols/dcutr/src/handler: Don't KeepAlive::No on all errors

0194655

protocols/dcutr: Extract MAX_NUMBER_OF_UPGRADE_ATTEMPTS constant

ce93ec2

protocols/dcutr: Implement Encoder and Decoder for DCUtR prost messages

354900f

mxinden added 3 commits February 2, 2022 20:02

protocols/dcutr: Use extend instead of double push_back

b50708b

protocols/dcutr: Rename Action to ActionBuilder

f18a863

Merge branch 'libp2p/master' into dcutr-2

e66ad8a

mxinden mentioned this pull request Feb 2, 2022

src/_blog: Add libp2p hole punching post ipfs/ipfs-blog#375

Merged

4 tasks

Merge branch 'libp2p/master' into dcutr-2

d1bced7

mxinden requested a review from thomaseizinger February 3, 2022 17:00

MarcoPolo approved these changes Feb 3, 2022

View reviewed changes

iduartgomez mentioned this pull request Feb 3, 2022

NAT traversal freenet/freenet-core#2

Closed

10 tasks

mxinden added 2 commits February 8, 2022 15:16

Merge branch 'libp2p/master' into dcutr-2

bca9c90

protocols/dcutr/tests: Fix flaky test

7934db5

mxinden merged commit 0bb8ee9 into libp2p:master Feb 8, 2022

This was referenced Feb 9, 2022

NAT traversal #2052

Closed

NAT traversal tracking issue libp2p/specs#312

Open

Conversation

mxinden commented Jan 15, 2022

Uh oh!

Uh oh!

mxinden commented Jan 19, 2022

Uh oh!

MarcoPolo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thomaseizinger commented Jan 31, 2022

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mxinden commented Feb 3, 2022