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 6 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
90 changes: 43 additions & 47 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,33 +246,30 @@ impl NetworkBehaviour for Behaviour {
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."
);
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.
}

Ok(Either::Right(Either::Left(
handler::direct::Handler::default(),
)))
}
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?

{
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(),
)));
}

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

fn handle_established_outbound_connection(
Expand All @@ -282,33 +279,32 @@ 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(_) => {
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(),
"DCUtR state tracking is buggy!"
arpankapoor marked this conversation as resolved.
Show resolved Hide resolved
);

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