fix(SwarmBuilder): prioritize relay, then websocket, then any other

[thomaseizinger]

Description

There is a niche case where dialing a relay address does not work if the other transport is a DNS transport. By composing the relay transport first, we avoid this issue.

Notes & open questions

I am not sure if this is the fully correct fix.

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[mxinden]

@thomaseizinger can you expand on your use-case? Why do you pass a DNS transport via with_other_transport and not via with_dns? DNS needs special handling, given that it never returns Err(MultiaddrNotSupported), but instead, errors later on, asynchronously. That is why it always has to come last.

rust-libp2p/transports/dns/src/lib.rs

Line 273 in c442df8

Ok(async move {

Yes, the proposed patch here is a solution. Though I wonder whether we can do better.

[thomaseizinger]

Try https://github.com/libp2p/rust-libp2p/blob/master/examples/dcutr/src/main.rs. It doesn't work at the moment :)

[mxinden]

Ah, yes, that makes a lot of sense. Let's go with your patch. Also, let's move with_relay after with_websocket just in case someone wants to establish a relayed connection over a websocket connection.

I will push another commit shortly.

[mxinden]

Thank you Thomas for catching this

[thomaseizinger]

@mxinden Meta-question: I thought we use the "scope" of conventional commits for the crate that we are changing, which is why I originally named it as fix(libp2p). Curious to hear what you think.

[mxinden]

No strong opinion. To me SwarmBuilder was a more descriptive scope. I am assuming that most folks working on rust-libp2p know where the new SwarmBuilder is located, namely in libp2p. Happy to restrict the scope to crates only. Thoughts?

[thomaseizinger]

No strong opinion. To me SwarmBuilder was a more descriptive scope. I am assuming that most folks working on rust-libp2p know where the new SwarmBuilder is located, namely in libp2p. Happy to restrict the scope to crates only. Thoughts?

It is the better trade-off I think, despite the ambiguity with the old SwarmBuilder.

[mxinden]

SwarmBuilder is not yet released, thus adding the internal-changes label.

[thomaseizinger]

SwarmBuilder is not yet released, thus adding the internal-changes label.

Should we name it allow-no-changelog perhaps?

[mxinden]

SwarmBuilder is not yet released, thus adding the internal-changes label.

Should we name it allow-no-changelog perhaps?

I am fine either way.