refactor(swarm)!: don't share event buffer for established connections

[thomaseizinger]

Description

Currently, we only have a single channel for all established connections. This requires us to construct the channel ahead of time, before we even have a connection. As it turns out, sharing this buffer across all connections actually has downsides. In particular, this means a single, very busy connection can starve others by filling up this buffer, forcing other connections to wait until they can emit an event.

Notes

Depends-On: #3187

Links to any relevant issues

Open Questions

• Does this need a changelog entry?

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]

In general, having a channel per connection sounds reasonable to me.

I am not yet sure I agree with the underlying motivation, namely whether we should get rid of SwarmBuilder. Happy to proceed here though.

[thomaseizinger]

I am not yet sure I agree with the underlying motivation, namely whether we should get rid of SwarmBuilder.

Do you mind posting your concerns/arguments to #3186?

[elenaf9]

I am not sure whether we should simply release this as a patch.
The default for this value was (and remains) 7.
Users that are dealing with a large number of connections per node probably changed this value in their implementation to a much higher number (e.g. iroh to 256).
Buffering 256 events in total vs 256 events per connection is imo a significant difference.

I would actually prefer to do an explicit breaking change here by removing/ renaming connection_event_buffer_size and adding per_connection_event_buffer_size to ensure that users take note of this change.

[thomaseizinger]

Explicit breaking changes can be a useful tool but I think it might be a bit exaggerated here. What problems other than more memory usage do you see that might arise from this?

Also, breaking changes in libp2p-swarm and libp2p-core are such a pain to pull through the workspace that I am honestly not willing to do it for such a (IMO) minor thing. Happy to do it if we are already in the middle of a release cycle with breaking changes in it.

[dignifiedquire]

From a user perspective I think, as long as this is clearly communicated in the release notes/changelog, it is fine to not make this a breaking change.

[dignifiedquire]

Generally I am glad to see this change, as I suspect we actually ran into this issue, where a protocol emitting a lot of events per connection would overflow the rest of the system.

[mergify]

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

[elenaf9]

Explicit breaking changes can be a useful tool but I think it might be a bit exaggerated here. What problems other than more memory usage do you see that might arise from this?

Also, breaking changes in libp2p-swarm and libp2p-core are such a pain to pull through the workspace that I am honestly not willing to do it for such a (IMO) minor thing. Happy to do it if we are already in the middle of a release cycle with breaking changes in it.

From a user perspective I think, as long as this is clearly communicated in the release notes/changelog, it is fine to not make this a breaking change.

I don't feel strongly about it; maybe I am being overly careful. I am currently on vacation and don't have time to re-review.
Feel free to move forward here without the explicit breaking change.

[mergify]

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

[mxinden]

Generally I am glad to see this change, as I suspect we actually ran into this issue, where a protocol emitting a lot of events per connection would overflow the rest of the system.

@dignifiedquire note that this does not introduce a limit per protocol, but per connection. Thus, in case your protocol was previously reaching the limit and thus harming other protocols, it might still be doing so with this change, just a little delayed and per connection.

[mxinden]

I am not sure whether we should simply release this as a patch. The default for this value was (and remains) 7. Users that are dealing with a large number of connections per node probably changed this value in their implementation to a much higher number (e.g. iroh to 256). Buffering 256 events in total vs 256 events per connection is imo a significant difference.

I would actually prefer to do an explicit breaking change here by removing/ renaming connection_event_buffer_size and adding per_connection_event_buffer_size to ensure that users take note of this change.

I agree with @elenaf9. I do think this is a significant change for folks in low-resource or high-connection environments and thus I think this change in functionality warrants a breaking change.

What problems other than more memory usage do you see that might arise from this?

Higher memory usage can be used as an attack, e.g. forcing the process to run out of memory. This is especially relevant as this is the size of the channel from the ConnectionHandler to the NetworkBehaviour and not the other way around. The former is potentially controlled by an attacker, in the worst case with large-sized messages.

[thomaseizinger]

I am not sure whether we should simply release this as a patch. The default for this value was (and remains) 7. Users that are dealing with a large number of connections per node probably changed this value in their implementation to a much higher number (e.g. iroh to 256). Buffering 256 events in total vs 256 events per connection is imo a significant difference.
I would actually prefer to do an explicit breaking change here by removing/ renaming connection_event_buffer_size and adding per_connection_event_buffer_size to ensure that users take note of this change.

I agree with @elenaf9. I do think this is a significant change for folks in low-resource or high-connection environments and thus I think this change in functionality warrants a breaking change.

Should it come with a deprecation warning first then? i.e. should we maintain the old functionality in parallel so users can gradually migrate?

[thomaseizinger]

Also, are we fine with users who are sticking to the default value potentially not noticing this?

[mxinden]

Should it come with a deprecation warning first then? i.e. should we maintain the old functionality in parallel so users can gradually migrate?

I am assuming (intuition) that maintaining both introduces unreasonable complexity. Would you agree?

I am fine with a hard breaking change without deprecation. It does not require a large change on the user side. I consider this an expert-option, only set by people deeply familiar with libp2p.

Also, are we fine with users who are sticking to the default value potentially not noticing this?

I consider both the former and the new default value sane. Thus I don't think further notifications beyond the changelog are needed.

[thomaseizinger]

Ready for re-review.

Code has changed.

[mxinden]

Thanks for the follow-ups!