Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(dcutr): always check for relayed connection first #3982

Merged
merged 11 commits into from
May 31, 2023
Merged

fix(dcutr): always check for relayed connection first #3982

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4ed1427
Fix DCUtR NetworkBehavior's established connection handler
arpankapoor May 24, 2023
bf1dfce
Use early return
arpankapoor May 25, 2023
770e9d0
Assert that outgoing_direct_connection_attempts entry was removed
arpankapoor May 25, 2023
527371e
Fix failing test
arpankapoor May 25, 2023
3b444eb
Add back removal of outgoing_direct_connection_attempts entry
arpankapoor May 25, 2023
5e9e0a3
Add assert on Listener for outgoing_direct_connection_attempts entry …
arpankapoor May 26, 2023
d1aba5f
Return dummy handler directly in inbound handler
arpankapoor May 29, 2023
b3c0996
Fix clippy warning
arpankapoor May 29, 2023
5948aee
Add comment
arpankapoor May 30, 2023
981735b
Merge branch 'master' into dcutr-fix
thomaseizinger May 30, 2023
bf8d1b6
Merge branch 'master' into dcutr-fix
mergify[bot] May 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 39 additions & 49 deletions protocols/dcutr/src/behaviour_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,37 +242,27 @@ 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<THandler<Self>, ConnectionDenied> {
match self
.outgoing_direct_connection_attempts
.remove(&(connection_id, peer))
{
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(),
)))
}
if is_relayed(local_addr) {
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.
}

assert!(
self.direct_to_relayed_connections
.get(&connection_id)
.is_none(),
"state mismatch"
);

Ok(Either::Right(Either::Right(dummy::ConnectionHandler)))
}

fn handle_established_outbound_connection(
Expand All @@ -282,33 +272,33 @@ impl NetworkBehaviour for Behaviour {
addr: &Multiaddr,
role_override: Endpoint,
) -> Result<THandler<Self>, ConnectionDenied> {
match self
.outgoing_direct_connection_attempts
.remove(&(connection_id, peer))
if is_relayed(addr) {
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.
}

// Whether this is a connection requested by this behaviour.
if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id)
arpankapoor marked this conversation as resolved.
Show resolved Hide resolved
{
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(_) => {
if role_override == Endpoint::Listener {
assert!(
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
!is_relayed(addr),
"`Prototype::DirectConnection` is never created for relayed connection."
self.outgoing_direct_connection_attempts
.remove(&(relayed_connection_id, peer))
.is_some(),
"state mismatch"
);

Ok(Either::Right(Either::Left(
handler::direct::Handler::default(),
)))
}

return Ok(Either::Right(Either::Left(
handler::direct::Handler::default(),
)));
Comment on lines +296 to +298
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With recent refactorings in libp2p-swarm, there is actually no more need for handler::direct::Handler at all. The only thing that it does is report an event to the behaviour that is then bubbled to the user. The behaviour could as well just bubble the event to the user here already and return just a dummy handler.

The above improvement is only slightly related to this pull request. In case you want to tackle it @arpankapoor in this pull request, that would be much appreciated. Though I am happy to merge as is and tackle it some other time.

Idea raised by @thomaseizinger in a synchronous discussion just now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favor of that refactoring but no need to include in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it could be tackled in another PR perhaps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll merge this as is then :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked here #4013. Also unresolving here for easier discovery in the future.

}

Ok(Either::Right(Either::Right(dummy::ConnectionHandler)))
}

fn on_connection_handler_event(
Expand Down