refactor(dcutr): simplify public API

We refactor the `libp2p-dcutr` API to only emit a single event: whether the hole-punch was successful or not. All other intermediate events are removed. Hole-punching is something that we try to do automatically as soon as we are connected to a peer over a relayed connection. The lack of explicit user intent means any event we emit is at best informational and not a "response" that the user would wait for.

Thus, I chose to not expose the details of why the hole-punch failed but return an opaque error.

Lastly, this PR also removes the usage of `ConnectionHandlerEvent::Close`. Just because something went wrong during the DCUtR handshake, doesn't mean we should close the relayed connection.

Related: #3591.

Pull-Request: #4749.

hole-punching-tests/src/main.rs

@@ -111,12 +111,10 @@ async fn main() -> Result<()> {
                     .await?;
             }
             (
-                SwarmEvent::Behaviour(BehaviourEvent::Dcutr(
-                    dcutr::Event::DirectConnectionUpgradeSucceeded {
-                        remote_peer_id,
-                        connection_id,
-                    },
-                )),
+                SwarmEvent::Behaviour(BehaviourEvent::Dcutr(dcutr::Event {
+                    remote_peer_id,
+                    result: Ok(connection_id),
+                })),
                 _,
                 _,
             ) => {
@@ -138,13 +136,11 @@ async fn main() -> Result<()> {
                 return Ok(());
             }
             (
-                SwarmEvent::Behaviour(BehaviourEvent::Dcutr(
-                    dcutr::Event::DirectConnectionUpgradeFailed {
-                        remote_peer_id,
-                        error,
-                        ..
-                    },
-                )),
+                SwarmEvent::Behaviour(BehaviourEvent::Dcutr(dcutr::Event {
+                    remote_peer_id,
+                    result: Err(error),
+                    ..
+                })),
                 _,
                 _,
             ) => {

libp2p/src/tutorials/hole_punching.rs

@@ -166,18 +166,9 @@
 //!    [2022-01-30T12:54:10Z INFO  client] Established connection to PeerId("12D3KooWPjceQrSwdWXPyLLeABRXmuqt69Rg3sBYbU1Nft9HyQ6X") via Dialer { address: "/ip4/$RELAY_PEER_ID/tcp/4001/p2p/12D3KooWDpJ7As7BWAwRMfu1VU2WCqNjvq387JEYKDBj4kx6nXTN/p2p-circuit/p2p/12D3KooWPjceQrSwdWXPyLLeABRXmuqt69Rg3sBYbU1Nft9HyQ6X", role_override: Dialer }
 //!    ```
 //!
-//! 2. The listening client initiating a direct connection upgrade for the new relayed connection.
-//!    Reported by [`dcutr`](crate::dcutr) through
-//!    [`Event::RemoteInitiatedDirectConnectionUpgrade`](crate::dcutr::Event::RemoteInitiatedDirectConnectionUpgrade).
+//! 2. The direct connection upgrade, also known as hole punch, succeeding.
+//!    Reported by [`dcutr`](crate::dcutr) through [`Event`](crate::dcutr::Event) containing [`Result::Ok`] with the [`ConnectionId`](libp2p_swarm::ConnectionId) of the new direct connection.
 //!
 //!    ``` ignore
-//!    [2022-01-30T12:54:11Z INFO  client] RemoteInitiatedDirectConnectionUpgrade { remote_peer_id: PeerId("12D3KooWPjceQrSwdWXPyLLeABRXmuqt69Rg3sBYbU1Nft9HyQ6X"), remote_relayed_addr: "/ip4/$RELAY_PEER_ID/tcp/4001/p2p/12D3KooWDpJ7As7BWAwRMfu1VU2WCqNjvq387JEYKDBj4kx6nXTN/p2p-circuit/p2p/12D3KooWPjceQrSwdWXPyLLeABRXmuqt69Rg3sBYbU1Nft9HyQ6X" }
-//!    ```
-//!
-//! 3. The direct connection upgrade, also known as hole punch, succeeding. Reported by
-//!    [`dcutr`](crate::dcutr) through
-//!    [`Event::RemoteInitiatedDirectConnectionUpgrade`](crate::dcutr::Event::DirectConnectionUpgradeSucceeded).
-//!
-//!    ``` ignore
-//!    [2022-01-30T12:54:11Z INFO  client] DirectConnectionUpgradeSucceeded { remote_peer_id: PeerId("12D3KooWPjceQrSwdWXPyLLeABRXmuqt69Rg3sBYbU1Nft9HyQ6X") }
+//!    [2022-01-30T12:54:11Z INFO  client] Event { remote_peer_id: PeerId("12D3KooWPjceQrSwdWXPyLLeABRXmuqt69Rg3sBYbU1Nft9HyQ6X"), result: Ok(2) }
 //!    ```

misc/metrics/src/dcutr.rs

@@ -49,31 +49,20 @@ struct EventLabels {
 
 #[derive(Debug, Clone, Hash, PartialEq, Eq, EncodeLabelValue)]
 enum EventType {
-    InitiateDirectConnectionUpgrade,
-    RemoteInitiatedDirectConnectionUpgrade,
     DirectConnectionUpgradeSucceeded,
     DirectConnectionUpgradeFailed,
 }
 
 impl From<&libp2p_dcutr::Event> for EventType {
     fn from(event: &libp2p_dcutr::Event) -> Self {
         match event {
-            libp2p_dcutr::Event::InitiatedDirectConnectionUpgrade {
+            libp2p_dcutr::Event {
                 remote_peer_id: _,
-                local_relayed_addr: _,
-            } => EventType::InitiateDirectConnectionUpgrade,
-            libp2p_dcutr::Event::RemoteInitiatedDirectConnectionUpgrade {
-                remote_peer_id: _,
-                remote_relayed_addr: _,
-            } => EventType::RemoteInitiatedDirectConnectionUpgrade,
-            libp2p_dcutr::Event::DirectConnectionUpgradeSucceeded {
-                remote_peer_id: _,
-                connection_id: _,
+                result: Ok(_),
             } => EventType::DirectConnectionUpgradeSucceeded,
-            libp2p_dcutr::Event::DirectConnectionUpgradeFailed {
+            libp2p_dcutr::Event {
                 remote_peer_id: _,
-                connection_id: _,
-                error: _,
+                result: Err(_),
             } => EventType::DirectConnectionUpgradeFailed,
         }
     }

protocols/dcutr/CHANGELOG.md

@@ -1,13 +1,13 @@
 ## 0.11.0 - unreleased
 
 - Add `ConnectionId` to `Event::DirectConnectionUpgradeSucceeded` and `Event::DirectConnectionUpgradeFailed`.
-  See [PR 4558].
-
-[PR 4558]: https://github.com/libp2p/rust-libp2p/pull/4558
-
+  See [PR 4558](https://github.com/libp2p/rust-libp2p/pull/4558).
 - Exchange address _candidates_ instead of external addresses in `CONNECT`.
   If hole-punching wasn't working properly for you until now, this might be the reason why.
   See [PR 4624](https://github.com/libp2p/rust-libp2p/pull/4624).
+- Simplify public API.
+  We now only emit a single event: whether the hole-punch was successful or not.
+  See [PR XXXX](https://github.com/libp2p/rust-libp2p/pull/XXXX).
 
 ## 0.10.0 
 

protocols/dcutr/Cargo.toml

@@ -25,6 +25,7 @@ quick-protobuf-codec = { workspace = true }
 thiserror = "1.0"
 void = "1"
 lru = "0.11.1"
+futures-bounded = { workspace = true }
 
 [dev-dependencies]
 async-std = { version = "1.12.0", features = ["attributes"] }

protocols/dcutr/src/behaviour.rs

@@ -20,7 +20,7 @@
 
 //! [`NetworkBehaviour`] to act as a direct connection upgrade through relay node.
 
-use crate::handler;
+use crate::{handler, protocol};
 use either::Either;
 use libp2p_core::connection::ConnectedPoint;
 use libp2p_core::multiaddr::Protocol;
@@ -32,7 +32,7 @@ use libp2p_swarm::{
     dummy, ConnectionDenied, ConnectionHandler, ConnectionId, NewExternalAddrCandidate, THandler,
     THandlerOutEvent,
 };
-use libp2p_swarm::{NetworkBehaviour, NotifyHandler, StreamUpgradeError, THandlerInEvent, ToSwarm};
+use libp2p_swarm::{NetworkBehaviour, NotifyHandler, THandlerInEvent, ToSwarm};
 use lru::LruCache;
 use std::collections::{HashMap, HashSet, VecDeque};
 use std::num::NonZeroUsize;
@@ -44,32 +44,25 @@ pub(crate) const MAX_NUMBER_OF_UPGRADE_ATTEMPTS: u8 = 3;
 
 /// The events produced by the [`Behaviour`].
 #[derive(Debug)]
-pub enum Event {
-    InitiatedDirectConnectionUpgrade {
-        remote_peer_id: PeerId,
-        local_relayed_addr: Multiaddr,
-    },
-    RemoteInitiatedDirectConnectionUpgrade {
-        remote_peer_id: PeerId,
-        remote_relayed_addr: Multiaddr,
-    },
-    DirectConnectionUpgradeSucceeded {
-        remote_peer_id: PeerId,
-        connection_id: ConnectionId,
-    },
-    DirectConnectionUpgradeFailed {
-        remote_peer_id: PeerId,
-        connection_id: ConnectionId,
-        error: Error,
-    },
+pub struct Event {
+    pub remote_peer_id: PeerId,
+    pub result: Result<ConnectionId, Error>,
+}
+
+#[derive(Debug, Error)]
+#[error("Failed to hole-punch connection: {inner}")]
+pub struct Error {
+    inner: InnerError,
 }
 
 #[derive(Debug, Error)]
-pub enum Error {
-    #[error("Failed to dial peer.")]
-    Dial,
-    #[error("Failed to establish substream: {0}.")]
-    Handler(StreamUpgradeError<Void>),
+enum InnerError {
+    #[error("Giving up after {0} dial attempts")]
+    AttemptsExceeded(u8),
+    #[error("Inbound stream error: {0}")]
+    InboundError(protocol::inbound::Error),
+    #[error("Outbound stream error: {0}")]
+    OutboundError(protocol::outbound::Error),
 }
 
 pub struct Behaviour {
@@ -142,13 +135,12 @@ impl Behaviour {
                 event: Either::Left(handler::relayed::Command::Connect),
             })
         } else {
-            self.queued_events.extend([ToSwarm::GenerateEvent(
-                Event::DirectConnectionUpgradeFailed {
-                    remote_peer_id: peer_id,
-                    connection_id: failed_direct_connection,
-                    error: Error::Dial,
-                },
-            )]);
+            self.queued_events.extend([ToSwarm::GenerateEvent(Event {
+                remote_peer_id: peer_id,
+                result: Err(Error {
+                    inner: InnerError::AttemptsExceeded(MAX_NUMBER_OF_UPGRADE_ATTEMPTS),
+                }),
+            })]);
         }
     }
 
@@ -197,13 +189,6 @@ impl NetworkBehaviour for Behaviour {
                 handler::relayed::Handler::new(connected_point, self.observed_addresses());
             handler.on_behaviour_event(handler::relayed::Command::Connect);
 
-            self.queued_events.extend([ToSwarm::GenerateEvent(
-                Event::InitiatedDirectConnectionUpgrade {
-                    remote_peer_id: peer,
-                    local_relayed_addr: local_addr.clone(),
-                },
-            )]);
-
             return Ok(Either::Left(handler)); // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound.
         }
         self.direct_connections
@@ -255,12 +240,10 @@ impl NetworkBehaviour for Behaviour {
                 );
             }
 
-            self.queued_events.extend([ToSwarm::GenerateEvent(
-                Event::DirectConnectionUpgradeSucceeded {
-                    remote_peer_id: peer,
-                    connection_id,
-                },
-            )]);
+            self.queued_events.extend([ToSwarm::GenerateEvent(Event {
+                remote_peer_id: peer,
+                result: Ok(connection_id),
+            })]);
         }
 
         Ok(Either::Right(dummy::ConnectionHandler))
@@ -284,15 +267,7 @@ impl NetworkBehaviour for Behaviour {
         };
 
         match handler_event {
-            Either::Left(handler::relayed::Event::InboundConnectRequest { remote_addr }) => {
-                self.queued_events.extend([ToSwarm::GenerateEvent(
-                    Event::RemoteInitiatedDirectConnectionUpgrade {
-                        remote_peer_id: event_source,
-                        remote_relayed_addr: remote_addr,
-                    },
-                )]);
-            }
-            Either::Left(handler::relayed::Event::InboundConnectNegotiated(remote_addrs)) => {
+            Either::Left(handler::relayed::Event::InboundConnectNegotiated { remote_addrs }) => {
                 log::debug!(
                     "Attempting to hole-punch as dialer to {event_source} using {remote_addrs:?}"
                 );
@@ -308,14 +283,23 @@ impl NetworkBehaviour for Behaviour {
                     .insert(maybe_direct_connection_id, relayed_connection_id);
                 self.queued_events.push_back(ToSwarm::Dial { opts });
             }
-            Either::Left(handler::relayed::Event::OutboundNegotiationFailed { error }) => {
-                self.queued_events.push_back(ToSwarm::GenerateEvent(
-                    Event::DirectConnectionUpgradeFailed {
-                        remote_peer_id: event_source,
-                        connection_id: relayed_connection_id,
-                        error: Error::Handler(error),
-                    },
-                ));
+            Either::Left(handler::relayed::Event::InboundConnectFailed { error }) => {
+                self.queued_events.push_back(ToSwarm::GenerateEvent(Event {
+                    remote_peer_id: event_source,
+                    result: Err(Error {
+                        inner: InnerError::InboundError(error),
+                    }),
+                }));
+            }
+            Either::Left(handler::relayed::Event::OutboundConnectFailed { error }) => {
+                self.queued_events.push_back(ToSwarm::GenerateEvent(Event {
+                    remote_peer_id: event_source,
+                    result: Err(Error {
+                        inner: InnerError::OutboundError(error),
+                    }),
+                }));
+
+                // Maybe treat these as transient and retry?
             }
             Either::Left(handler::relayed::Event::OutboundConnectNegotiated { remote_addrs }) => {
                 log::debug!(