refactor(core): blanket implementation of connection upgrade

[PanGan21]

Description

Introduces blanket implementation of {In,Out}boundConnectionUpgrade and uses it in transport upgrade infrastructure.

Resolves: #4307.

Notes & open questions

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]

Just approved CI to run.

[mxinden]

Good work. Thank you.

Next steps for this pull request would be:

• Deprecate the existing {Out,In}boundUpgrade trait definitions.
• Bump libp2p-core patch version.
• Opposite to what @thomaseizinger suggests in core: introduce {In,Out}boundConnectionUpgrade traits #4307 I think we can already update the transports/ and muxers/ protocols to the new {Out,In}boundConnectionUpgrades. Given the blanket implementation, this is a backwards compatible change. Am I missing something @thomaseizinger?

[thomaseizinger]

• Given the blanket implementation, this is a backwards compatible change. Am I missing something @thomaseizinger?

It is not. Removing a trait implementation for a public struct is a breaking change. Imagine someone having a function:

fn bar(foo: impl InboundUpgrade);

Currently, calling this function with noise::Config will compile. If we remove the implementation for InboundUprade, it will stop compiling, hence it is a breaking change.

[thomaseizinger]

Deprecate the existing {Out,In}boundUpgrade trait definitions.

Should we already do that in here? I will require us spread a lot of allow(deprecated) over the code. I'd like to first have things like futures-bounded in place to actually allow users to migrate their protocol implementations. We will also have to write a detailed migration guide for this I think.

In order to not block this PR on it, I'd suggest we don't deprecate this in here.

[PanGan21]

Deprecate the existing {Out,In}boundUpgrade trait definitions.

Should we already do that in here? I will require us spread a lot of allow(deprecated) over the code. I'd like to first have things like futures-bounded in place to actually allow users to migrate their protocol implementations. We will also have to write a detailed migration guide for this I think.

In order to not block this PR on it, I'd suggest we don't deprecate this in here.

Let me know what is required. I would be open to continue this work in a separate pr (with some guidance ofc)

[mxinden]

• Given the blanket implementation, this is a backwards compatible change. Am I missing something @thomaseizinger?

It is not. Removing a trait implementation for a public struct is a breaking change. Imagine someone having a function:

fn bar(foo: impl InboundUpgrade);

Currently, calling this function with noise::Config will compile. If we remove the implementation for InboundUprade, it will stop compiling, hence it is a breaking change.

Oh, yes, quite obviously. You are right. Thank you.

[mxinden]

Deprecate the existing {Out,In}boundUpgrade trait definitions.

Should we already do that in here? I will require us spread a lot of allow(deprecated) over the code. I'd like to first have things like futures-bounded in place to actually allow users to migrate their protocol implementations. We will also have to write a detailed migration guide for this I think.

In order to not block this PR on it, I'd suggest we don't deprecate this in here.

👍 makes sense. Thank you for expanding @thomaseizinger.

[mxinden]

Let me know what is required. I would be open to continue this work in a separate pr (with some guidance ofc)

Thank you @PanGan21. Let's get this pull request merged first. There are still multiple {In,Out}boundUpgrade usages in core/, e.g. core/src/upgrade/select. The only usage of {In,Out}boundUpgrade in core/ should be in the blanket implementation for {In,Out}boundConnectionUpgrade.

[PanGan21]

Let me know what is required. I would be open to continue this work in a separate pr (with some guidance ofc)

Thank you @PanGan21. Let's get this pull request merged first. There are still multiple {In,Out}boundUpgrade usages in core/, e.g. core/src/upgrade/select. The only usage of {In,Out}boundUpgrade in core/ should be in the blanket implementation for {In,Out}boundConnectionUpgrade.

By doing this and replacing the usage of the previous traits with the new ones I am getting error for conflicting implementations. Am I missing something important?
E.g.:

error[E0119]: conflicting implementations of trait `OutboundConnectionUpgrade<_>` for type `DeniedUpgrade`
   --> core/src/upgrade.rs:176:1
    |
176 | impl<U, T> OutboundConnectionUpgrade<T> for U
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `DeniedUpgrade`
    |
   ::: core/src/upgrade/denied.rs:50:1
    |
50  | impl<C> OutboundConnectionUpgrade<C> for DeniedUpgrade {
    | ------------------------------------------------------ first implementation here
    |
    = note: downstream crates may implement trait `upgrade::OutboundUpgrade<_>` for type `upgrade::denied::DeniedUpgrade`

[thomaseizinger]

Let me know what is required. I would be open to continue this work in a separate pr (with some guidance ofc)

Thank you @PanGan21. Let's get this pull request merged first. There are still multiple {In,Out}boundUpgrade usages in core/, e.g. core/src/upgrade/select. The only usage of {In,Out}boundUpgrade in core/ should be in the blanket implementation for {In,Out}boundConnectionUpgrade.

I am not sure that is correct. Most implementations of {In,Out}boundUpgrade in libp2p-core are not actually for connection upgrades but for stream upgrades, e.g. DeniedUpgrade etc. I think the only one that is useful for connections is SelectUpgrade (to compose muxers or encryption protocols).

I think the way we have to handle this for now is to tag all those with #[allow(deprecated)]. Once we remove {In,Out}boundUpgrade, most of those upgrades will simply be deleted because they no longer serve a purpose. I envision that once we land #4120, users should no longer need to compose any connection upgrades themselves anyway, meaning we can probably delete most of them or make them implementation details of the new SwarmBuilder.

[mxinden]

I think the way we have to handle this for now is to tag all those with #[allow(deprecated)].

Sounds good to me. @PanGan21 sorry for the confusion.

I envision that once we land #4120, users should no longer need to compose any connection upgrades themselves anyway, meaning we can probably delete most of them or make them implementation details of the new SwarmBuilder.

I don't think the first iteration of #4120 can cover all cases and thus I don't think we can remove the libp2p_core::upgrade infrastructure any time soon. Though I don't think that decision is relevant for this pull request.

[thomaseizinger]

I envision that once we land #4120, users should no longer need to compose any connection upgrades themselves anyway, meaning we can probably delete most of them or make them implementation details of the new SwarmBuilder.

I don't think the first iteration of #4120 can cover all cases and thus I don't think we can remove the libp2p_core::upgrade infrastructure any time soon. Though I don't think that decision is relevant for this pull request.

Maybe not in the first iteration but hopefully at some point :)

If we keep it, we could at least move it to some kind of util package that isn't part of the public API of libp2p such that we can more easily make changes to it without affecting the majority of users.

[PanGan21]

Hi guys. I am just wondering, is there something I can do more to have this pr done? Or it is targeting a later release so I have to wait?

[thomaseizinger]

Sorry that this has been dragging on for so long! Normally we don't have such a long turn-around time for PRs but this one somehow slipped through!

It looks good to me! I'll write up a follow-up issue on what needs to be done next! :)