From 4ed142744caa3bc20a0e00f0d2ecb4ddf834ce6c Mon Sep 17 00:00:00 2001 From: Arpan Kapoor Date: Wed, 24 May 2023 22:10:48 +0530 Subject: [PATCH 1/9] Fix DCUtR NetworkBehavior's established connection handler --- protocols/dcutr/src/behaviour_impl.rs | 82 +++++++++++---------------- 1 file changed, 32 insertions(+), 50 deletions(-) diff --git a/protocols/dcutr/src/behaviour_impl.rs b/protocols/dcutr/src/behaviour_impl.rs index 57692d38898..c3a4c6df347 100644 --- a/protocols/dcutr/src/behaviour_impl.rs +++ b/protocols/dcutr/src/behaviour_impl.rs @@ -246,32 +246,23 @@ impl NetworkBehaviour for Behaviour { local_addr: &Multiaddr, remote_addr: &Multiaddr, ) -> Result, ConnectionDenied> { - match self - .outgoing_direct_connection_attempts - .remove(&(connection_id, peer)) + if is_relayed(local_addr) { + Ok(Either::Left(handler::relayed::Handler::new( + ConnectedPoint::Listener { + local_addr: local_addr.clone(), + send_back_addr: remote_addr.clone(), + }, + ))) // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. + } else if let Some(&relayed_connection_id) = + self.direct_to_relayed_connections.get(&connection_id) { - None => { - let handler = if is_relayed(local_addr) { - Either::Left(handler::relayed::Handler::new(ConnectedPoint::Listener { - local_addr: local_addr.clone(), - send_back_addr: remote_addr.clone(), - })) // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. - } else { - Either::Right(Either::Right(dummy::ConnectionHandler)) - }; - - Ok(handler) - } - Some(_) => { - assert!( - !is_relayed(local_addr), - "`Prototype::DirectConnection` is never created for relayed connection." - ); - - Ok(Either::Right(Either::Left( - handler::direct::Handler::default(), - ))) - } + self.outgoing_direct_connection_attempts + .remove(&(relayed_connection_id, peer)); + Ok(Either::Right(Either::Left( + handler::direct::Handler::default(), + ))) + } else { + Ok(Either::Right(Either::Right(dummy::ConnectionHandler))) } } @@ -282,32 +273,23 @@ impl NetworkBehaviour for Behaviour { addr: &Multiaddr, role_override: Endpoint, ) -> Result, ConnectionDenied> { - match self - .outgoing_direct_connection_attempts - .remove(&(connection_id, peer)) + if is_relayed(addr) { + Ok(Either::Left(handler::relayed::Handler::new( + ConnectedPoint::Dialer { + address: addr.clone(), + role_override, + }, + ))) // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. + } else if let Some(&relayed_connection_id) = + self.direct_to_relayed_connections.get(&connection_id) { - None => { - let handler = if is_relayed(addr) { - Either::Left(handler::relayed::Handler::new(ConnectedPoint::Dialer { - address: addr.clone(), - role_override, - })) // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. - } else { - Either::Right(Either::Right(dummy::ConnectionHandler)) - }; - - Ok(handler) - } - Some(_) => { - assert!( - !is_relayed(addr), - "`Prototype::DirectConnection` is never created for relayed connection." - ); - - Ok(Either::Right(Either::Left( - handler::direct::Handler::default(), - ))) - } + self.outgoing_direct_connection_attempts + .remove(&(relayed_connection_id, peer)); + Ok(Either::Right(Either::Left( + handler::direct::Handler::default(), + ))) + } else { + Ok(Either::Right(Either::Right(dummy::ConnectionHandler))) } } From bf1dfce9d9cd571b696fe826393fa472f913a7cf Mon Sep 17 00:00:00 2001 From: Arpan Kapoor Date: Thu, 25 May 2023 13:26:58 +0530 Subject: [PATCH 2/9] Use early return --- protocols/dcutr/src/behaviour_impl.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/protocols/dcutr/src/behaviour_impl.rs b/protocols/dcutr/src/behaviour_impl.rs index c3a4c6df347..a5de26e4af0 100644 --- a/protocols/dcutr/src/behaviour_impl.rs +++ b/protocols/dcutr/src/behaviour_impl.rs @@ -247,14 +247,15 @@ impl NetworkBehaviour for Behaviour { remote_addr: &Multiaddr, ) -> Result, ConnectionDenied> { if is_relayed(local_addr) { - Ok(Either::Left(handler::relayed::Handler::new( + return Ok(Either::Left(handler::relayed::Handler::new( ConnectedPoint::Listener { local_addr: local_addr.clone(), send_back_addr: remote_addr.clone(), }, - ))) // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. - } else if let Some(&relayed_connection_id) = - self.direct_to_relayed_connections.get(&connection_id) + ))); // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. + } + + if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id) { self.outgoing_direct_connection_attempts .remove(&(relayed_connection_id, peer)); @@ -274,14 +275,15 @@ impl NetworkBehaviour for Behaviour { role_override: Endpoint, ) -> Result, ConnectionDenied> { if is_relayed(addr) { - Ok(Either::Left(handler::relayed::Handler::new( + return Ok(Either::Left(handler::relayed::Handler::new( ConnectedPoint::Dialer { address: addr.clone(), role_override, }, - ))) // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. - } else if let Some(&relayed_connection_id) = - self.direct_to_relayed_connections.get(&connection_id) + ))); // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. + } + + if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id) { self.outgoing_direct_connection_attempts .remove(&(relayed_connection_id, peer)); From 770e9d07b381b2b9c0df4a272bd79893137822c0 Mon Sep 17 00:00:00 2001 From: Arpan Kapoor Date: Thu, 25 May 2023 17:59:01 +0530 Subject: [PATCH 3/9] Assert that outgoing_direct_connection_attempts entry was removed --- protocols/dcutr/src/behaviour_impl.rs | 34 +++++++++++++++++---------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/protocols/dcutr/src/behaviour_impl.rs b/protocols/dcutr/src/behaviour_impl.rs index a5de26e4af0..c223926ec27 100644 --- a/protocols/dcutr/src/behaviour_impl.rs +++ b/protocols/dcutr/src/behaviour_impl.rs @@ -257,14 +257,19 @@ impl NetworkBehaviour for Behaviour { if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id) { - self.outgoing_direct_connection_attempts - .remove(&(relayed_connection_id, peer)); - Ok(Either::Right(Either::Left( + assert!( + self.outgoing_direct_connection_attempts + .remove(&(relayed_connection_id, peer)) + .is_some(), + "DCUtR state tracking is buggy!" + ); + + return Ok(Either::Right(Either::Left( handler::direct::Handler::default(), - ))) - } else { - Ok(Either::Right(Either::Right(dummy::ConnectionHandler))) + ))); } + + Ok(Either::Right(Either::Right(dummy::ConnectionHandler))) } fn handle_established_outbound_connection( @@ -285,14 +290,19 @@ impl NetworkBehaviour for Behaviour { if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id) { - self.outgoing_direct_connection_attempts - .remove(&(relayed_connection_id, peer)); - Ok(Either::Right(Either::Left( + assert!( + self.outgoing_direct_connection_attempts + .remove(&(relayed_connection_id, peer)) + .is_some(), + "DCUtR state tracking is buggy!" + ); + + return Ok(Either::Right(Either::Left( handler::direct::Handler::default(), - ))) - } else { - Ok(Either::Right(Either::Right(dummy::ConnectionHandler))) + ))); } + + Ok(Either::Right(Either::Right(dummy::ConnectionHandler))) } fn on_connection_handler_event( From 527371e85747e298e0f0330cbae0216223ea76d1 Mon Sep 17 00:00:00 2001 From: Arpan Kapoor Date: Thu, 25 May 2023 21:50:01 +0530 Subject: [PATCH 4/9] Fix failing test --- protocols/dcutr/src/behaviour_impl.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/protocols/dcutr/src/behaviour_impl.rs b/protocols/dcutr/src/behaviour_impl.rs index c223926ec27..1aa1c6221ac 100644 --- a/protocols/dcutr/src/behaviour_impl.rs +++ b/protocols/dcutr/src/behaviour_impl.rs @@ -275,7 +275,7 @@ impl NetworkBehaviour for Behaviour { fn handle_established_outbound_connection( &mut self, connection_id: ConnectionId, - peer: PeerId, + _peer: PeerId, addr: &Multiaddr, role_override: Endpoint, ) -> Result, ConnectionDenied> { @@ -288,14 +288,13 @@ impl NetworkBehaviour for Behaviour { ))); // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. } - if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id) + if self + .direct_to_relayed_connections + .get(&connection_id) + .is_some() { - assert!( - self.outgoing_direct_connection_attempts - .remove(&(relayed_connection_id, peer)) - .is_some(), - "DCUtR state tracking is buggy!" - ); + // outgoing_direct_connection_attempts is populated only by the listener + // so we don't need to delete any entries here. return Ok(Either::Right(Either::Left( handler::direct::Handler::default(), From 3b444ebc86897db2179a07d887050643efb973c2 Mon Sep 17 00:00:00 2001 From: Arpan Kapoor Date: Thu, 25 May 2023 22:32:53 +0530 Subject: [PATCH 5/9] Add back removal of outgoing_direct_connection_attempts entry --- protocols/dcutr/src/behaviour_impl.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/protocols/dcutr/src/behaviour_impl.rs b/protocols/dcutr/src/behaviour_impl.rs index 1aa1c6221ac..88879ede110 100644 --- a/protocols/dcutr/src/behaviour_impl.rs +++ b/protocols/dcutr/src/behaviour_impl.rs @@ -275,7 +275,7 @@ impl NetworkBehaviour for Behaviour { fn handle_established_outbound_connection( &mut self, connection_id: ConnectionId, - _peer: PeerId, + peer: PeerId, addr: &Multiaddr, role_override: Endpoint, ) -> Result, ConnectionDenied> { @@ -288,13 +288,12 @@ impl NetworkBehaviour for Behaviour { ))); // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. } - if self - .direct_to_relayed_connections - .get(&connection_id) - .is_some() + if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id) { - // outgoing_direct_connection_attempts is populated only by the listener - // so we don't need to delete any entries here. + // outgoing_direct_connection_attempts is populated only by the listener. + // so this may/may not return an entry + self.outgoing_direct_connection_attempts + .remove(&(relayed_connection_id, peer)); return Ok(Either::Right(Either::Left( handler::direct::Handler::default(), From 5e9e0a3d70112fe79a5b0661df51638df730c2f9 Mon Sep 17 00:00:00 2001 From: Arpan Kapoor Date: Fri, 26 May 2023 16:23:36 +0530 Subject: [PATCH 6/9] Add assert on Listener for outgoing_direct_connection_attempts entry removal --- protocols/dcutr/src/behaviour_impl.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/protocols/dcutr/src/behaviour_impl.rs b/protocols/dcutr/src/behaviour_impl.rs index 88879ede110..82c206ca4c6 100644 --- a/protocols/dcutr/src/behaviour_impl.rs +++ b/protocols/dcutr/src/behaviour_impl.rs @@ -290,10 +290,14 @@ impl NetworkBehaviour for Behaviour { if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id) { - // outgoing_direct_connection_attempts is populated only by the listener. - // so this may/may not return an entry - self.outgoing_direct_connection_attempts - .remove(&(relayed_connection_id, peer)); + if role_override == Endpoint::Listener { + assert!( + self.outgoing_direct_connection_attempts + .remove(&(relayed_connection_id, peer)) + .is_some(), + "DCUtR state tracking is buggy!" + ); + } return Ok(Either::Right(Either::Left( handler::direct::Handler::default(), From d1aba5fbcf48adc4a69dbffacaa521ab0d445b81 Mon Sep 17 00:00:00 2001 From: Arpan Kapoor Date: Mon, 29 May 2023 12:46:49 +0530 Subject: [PATCH 7/9] Return dummy handler directly in inbound handler --- protocols/dcutr/src/behaviour_impl.rs | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/protocols/dcutr/src/behaviour_impl.rs b/protocols/dcutr/src/behaviour_impl.rs index 82c206ca4c6..8b555d8a7e3 100644 --- a/protocols/dcutr/src/behaviour_impl.rs +++ b/protocols/dcutr/src/behaviour_impl.rs @@ -255,19 +255,12 @@ impl NetworkBehaviour for Behaviour { ))); // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. } - if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id) - { - assert!( - self.outgoing_direct_connection_attempts - .remove(&(relayed_connection_id, peer)) - .is_some(), - "DCUtR state tracking is buggy!" - ); - - return Ok(Either::Right(Either::Left( - handler::direct::Handler::default(), - ))); - } + assert!( + self.direct_to_relayed_connections + .get(&connection_id) + .is_none(), + "state mismatch" + ); Ok(Either::Right(Either::Right(dummy::ConnectionHandler))) } @@ -295,7 +288,7 @@ impl NetworkBehaviour for Behaviour { self.outgoing_direct_connection_attempts .remove(&(relayed_connection_id, peer)) .is_some(), - "DCUtR state tracking is buggy!" + "state mismatch" ); } From b3c0996776be42f1162cab9ba70fa3b91e220e9c Mon Sep 17 00:00:00 2001 From: Arpan Kapoor Date: Mon, 29 May 2023 12:55:35 +0530 Subject: [PATCH 8/9] Fix clippy warning --- protocols/dcutr/src/behaviour_impl.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/dcutr/src/behaviour_impl.rs b/protocols/dcutr/src/behaviour_impl.rs index 8b555d8a7e3..a6799f343c0 100644 --- a/protocols/dcutr/src/behaviour_impl.rs +++ b/protocols/dcutr/src/behaviour_impl.rs @@ -242,7 +242,7 @@ impl NetworkBehaviour for Behaviour { fn handle_established_inbound_connection( &mut self, connection_id: ConnectionId, - peer: PeerId, + _peer: PeerId, local_addr: &Multiaddr, remote_addr: &Multiaddr, ) -> Result, ConnectionDenied> { From 5948aee86765678a4246daf18cc3773445afe15d Mon Sep 17 00:00:00 2001 From: Arpan Kapoor Date: Tue, 30 May 2023 12:38:27 +0530 Subject: [PATCH 9/9] Add comment --- protocols/dcutr/src/behaviour_impl.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/protocols/dcutr/src/behaviour_impl.rs b/protocols/dcutr/src/behaviour_impl.rs index a6799f343c0..5ef422c079c 100644 --- a/protocols/dcutr/src/behaviour_impl.rs +++ b/protocols/dcutr/src/behaviour_impl.rs @@ -281,6 +281,7 @@ impl NetworkBehaviour for Behaviour { ))); // TODO: We could make two `handler::relayed::Handler` here, one inbound one outbound. } + // Whether this is a connection requested by this behaviour. if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id) { if role_override == Endpoint::Listener {