deps(yamux): update yamux to v0.12

[thomaseizinger]

Description

Notes

As discussed in libp2p/rust-yamux#142 (comment), I would suggest to release this as an alpha and then ask our users to depend on that alpha directly instead of using libp2p::yamux so we can get some feedback on how this implementation performs in the wild.

I think we should probably hold off on merging this but keep this PR to discuss any feedback that we might get. Perhaps the changelog entry should be merged to ensure that one is correct.

Thoughts?

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]

The inter-op tests fail because we are patching yamux in the entire workspace and testground doesn't seem to like that as it is now failing to compile the previous rust-libp2p version.

All local tests are green though which is exciting 😊

[mxinden]

Cross referencing libp2p/rust-yamux#142 (comment) here for visibility.

[thomaseizinger]

@mxinden: In case you are happy with the changes, can you release this on crates.io please? I'll then write a "call for testing" comment on #3202.

[thomaseizinger]

The last commit is required so that our test suite actually runs.

[rkuhn]

Since I won’t be able to do a full review of the underlying changes: is my assumption correct that the new version shall be 100% network compatible with the current version?

[thomaseizinger]

Since I won’t be able to do a full review of the underlying changes: is my assumption correct that the new version shall be 100% network compatible with the current version?

Yes, should be 100% network compatible.

[thomaseizinger]

@mxinden: In case you are happy with the changes, can you release this on crates.io please? I'll then write a "call for testing" comment on #3202.

@mxinden: Friendly ping.

[mxinden]

Tagged and published.

I think we should probably hold off on merging

👍

[mxinden]

Changes (excluding dependency override) look good to me.

Thank you for debugging this across implementations!

[thomaseizinger]

@mxinden can you give libp2p/rust-yamux#153 another review?

[mxinden]

Blocked on libp2p/rust-yamux#166.

[mxinden]

@mxinden can you give libp2p/rust-yamux#153 another review?

Done. Approved.

[thomaseizinger]

Something is not quite right, still. For some reason, the kademlia test two_servers_add_each_other_to_routing_table now always fails.

It seems like some of the events now happen in a different order. Interestingly, we already seem to be receiving events for kademlia before event emitting the ConnectionEstablished event. I am guessing this happens because the connection tasks are actually running parallel in the background. For Swarm::new_ephemeral though, we don't use an executor so there is no actual background work happening.

[mxinden]

@thomaseizinger do you have time this or next week to look into the above libp2p-kad failures? I think realizing the many improvements in yamux v0.12 in libp2p is worth the additional effort.

[thomaseizinger]

@thomaseizinger do you have time this or next week to look into the above libp2p-kad failures? I think realizing the many improvements in yamux v0.12 in libp2p is worth the additional effort.

I'll have a look.

[thomaseizinger]

@mxinden I adapted the tests so they are passing now. I ended up being as simple as removing two events from the match that are now (consistently) emitted earlier and thus "swallowed" by the swarm.listen call whilst waiting for the connection to be fully setup.

This is ready for review / merge now!

[mxinden]

Changes look good to me. Unless the performance benchmarks (see link above) show any regressions, this is good to merge from my end.

[thomaseizinger]

Can't see a regression but also unfortunately no improvement in performance: https://observablehq.com/@libp2p-workspace/performance-dashboard?branch=perf-rust-libp2p-yamux-v0.12#branch

I am guessing we are just severely limited by the initial receive window.

[thomaseizinger]

With the above results, I am merging here!

[mxinden]

I am guessing we are just severely limited by the initial receive window.

I am operating under the same assumption.