feat(swarm): allow both Disconnected and NotDialing condition combined

[b-zee]

Description

Dialing by default uses the condition Disconnected, but it seems appropriate to additionally prevent a dial if already dialing. This means a combination of Disconnected and NotDialing.

The default is changed to a new variant DisconnectedAndNotDialing. This is a breaking change.

Related: #4189.

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

[thomaseizinger]

I moved this to draft to prevent it being merged accidentally. I am surprised cargo semver-checks doesn't pick up on this?

[b-zee]

semver-checks does pick it up for me:

$ cargo semver-checks --baseline-rev origin/master --package=libp2p-swarm
     Cloning origin/master
     Parsing libp2p-swarm v0.43.1 (current)
     Parsing libp2p-swarm v0.43.1 (baseline)
    Checking libp2p-swarm v0.43.1 -> v0.43.1 (no change)
   Completed [   0.057s] 45 checks; 44 passed, 1 failed, 0 unnecessary

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.22.1/src/lints/enum_variant_added.ron

Failed in:
  variant PeerCondition:DisconnectedAndNotDialing in /home/b-zee/src/rust-libp2p/swarm/src/dial_opts.rs:325
       Final [   0.058s] semver requires new major version: 1 major and 0 minor checks failed

[thomaseizinger]

I noticed that we are two versions behind so maybe that is why :)

[mergify]

This pull request has merge conflicts. Could you please resolve them @b-zee? 🙏

[mxinden]

Thank you. To me this is the most concise solution to the problem.

Would you mind adding a changelog entry to swarm/CHANGELOG.md and bump the crate version in swarm/Cargo.toml? Don't worry about other version bumps. For now it is fine for CI to be red. I just want to make sure we don't forget the changelog entry and the version bump once we cut v0.53.0.

Thank you for the continued work.

[b-zee]

Done and rebased to fix merge conflicts.

[mergify]

This pull request has merge conflicts. Could you please resolve them @b-zee? 🙏

[mxinden]

This pull request has merge conflicts. Could you please resolve them @b-zee? pray

No need to resolve. Conflicts are simple. Let's resolve them close to v0.53 release unless difficult conflicts come up.

[thomaseizinger]

This pull request has merge conflicts. Could you please resolve them @b-zee? pray

No need to resolve. Conflicts are simple. Let's resolve them close to v0.53 release unless difficult conflicts come up.

We are thinking of cutting a new release soon, mind resolving the conflicts @b-zee?

[b-zee]

Rebased. The changes in master that were supposed to be a minor swarm release of 0.43.4, I have changed to 0.44.0, as this PR requires that if I'm not mistaken.

[thomaseizinger]

Thanks for the update! There seems to be a failing test still :)

[b-zee]

There seems to be a failing test still :)

I'm not sure how to attack that one. I don't think I changed anything dependency wise.

Anyhow, just rebased again and that might even fix the cargo-deny action. 🤞

[thomaseizinger]

There seems to be a failing test still :)

I'm not sure how to attack that one. I don't think I changed anything dependency wise.

It seems the only failing one is in libp2p-memory-connection-limits which I am guessing is just a missing and now explicitly required DialCondition to not use the new default?

Anyhow, just rebased again

Merging would be preferred! 🙏

See https://github.com/libp2p/rust-libp2p/blob/master/CONTRIBUTING.md#we-squash-merge-pull-requests.

[b-zee]

Pushed the fix for the test. I've taken the liberty to use the Always condition. As that test should dial always, but only fail by the connection limit itself. In theory, a connection could have been established by one of the dials, making it fail for that reason and not for what is described in the test.

[thomaseizinger]

Pushed the fix for the test. I've taken the liberty to use the Always condition. As that test should dial always, but only fail by the connection limit itself. In theory, a connection could have been established by one of the dials, making it fail for that reason and not for what is described in the test.

Sounds good! Can you apply the same fix to the other tests?

[b-zee]

Now that I've come back from vacation I have looked at it proper. Tests are fixed now, sorry for the back and forth.

The mdns test fails. I've looked at a bit, but I do not see the relation between that test and my changes. I'm quite sure the dialing changes some behavior here, but I can't pick up on it. Do you have any thoughts, @thomaseizinger?

[thomaseizinger]

The mdns test fails. I've looked at a bit, but I do not see the relation between that test and my changes. I'm quite sure the dialing changes some behavior here, but I can't pick up on it. Do you have any thoughts, @thomaseizinger?

I've restarted that one. It is probably just a rare flake :/

[thomaseizinger]

Setting this to draft because it is part of the next milestone.

[mergify]

This pull request has merge conflicts. Could you please resolve them @b-zee? 🙏

[mergify]

This pull request has merge conflicts. Could you please resolve them @b-zee? 🙏

[b-zee]

Thanks @thomaseizinger, wasn't sure when this would be merged so didn't want to keep the mergify bot happy 😁