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

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 2 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
84 changes: 34 additions & 50 deletions protocols/dcutr/src/behaviour_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,32 +246,24 @@ impl NetworkBehaviour for Behaviour {
local_addr: &Multiaddr,
remote_addr: &Multiaddr,
) -> Result<THandler<Self>, ConnectionDenied> {
match self
.outgoing_direct_connection_attempts
.remove(&(connection_id, peer))
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.
}

if let Some(&relayed_connection_id) = self.direct_to_relayed_connections.get(&connection_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this can ever be hit actually. In hole-punching, we cannot for sure tell which inbound connection was the one that the remote achieved through hole punching.

Plus, all this state tracking here uses ConnectionIds that are created as part of dialing, i.e. outbound connections.

So unless I am missing something, this function can be reduced to always return a dummy::ConnectionHandler for non-relayed connections.

@mxinden Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I agree with your assessment. I'll make this change after @mxinden's comments.

Copy link
Member

Choose a reason for hiding this comment

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

You are right @thomaseizinger.

For what it is worth, likely my train of thought that ended up with the code above:

In NetworkBehaviour::on_connection_handler_event on OutboundConnectNegotiated we trigger a Dial with role_override() (i.e. as_listener) and then insert the maybe_direct_connection_id from the DialOpts into self.direct_to_relayed_connections.

rust-libp2p/protocols/dcutr/src/behaviour_impl.rs

Lines 379 to 395 in d4c4078

Either::Left(handler::relayed::Event::OutboundConnectNegotiated { remote_addrs }) => {
let opts = DialOpts::peer_id(event_source)
.condition(dial_opts::PeerCondition::Always)
.addresses(remote_addrs)
.override_role()
.build();
let maybe_direct_connection_id = opts.connection_id();
self.direct_to_relayed_connections
.insert(maybe_direct_connection_id, relayed_connection_id);
*self
.outgoing_direct_connection_attempts
.entry((relayed_connection_id, event_source))
.or_default() += 1;
self.queued_events.push_back(ToSwarm::Dial { opts });
}

On success this would be reported to the Behaviour via NetworkBehaviour::handle_established_outbound_connection and not as assumed here in NetworkBehaviour::handle_established_inbound_connection.

Copy link
Member

Choose a reason for hiding this comment

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

How about adding an assert here, that connection_id is not in self.direct_to_relayed_connections?

{
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)))
}
}

Expand All @@ -282,32 +274,24 @@ 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.
}

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(_) => {
assert!(
!is_relayed(addr),
"`Prototype::DirectConnection` is never created for relayed connection."
);

Ok(Either::Right(Either::Left(
handler::direct::Handler::default(),
)))
}
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
self.outgoing_direct_connection_attempts
.remove(&(relayed_connection_id, peer));
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
Ok(Either::Right(Either::Left(
handler::direct::Handler::default(),
)))
} else {
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
Ok(Either::Right(Either::Right(dummy::ConnectionHandler)))
}
}

Expand Down