swarm/: Support generic connection management through NetworkBehaviour #2828

mxinden · 2022-08-19T04:30:20Z

Description

This pull request enables generic connection management by implementing option (2) of #2824:

Extend NetworkBehaviour with methods called to review a pending or established connection.

Very much work in progress at this point.

Links to any relevant issues

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

When generating the `OutEvent` for a `NetworkBehaviour`, add the `where` clause of the `NetworkBehaviour` `struct` to the generated `enum`.

When generating an `OutEvent` `enum` definition for a user, derive `Debug` for that `enum`. Why not derive `Clone`, `PartialEq` and `Eq` for the generated `enum` definition? While I think it is fine to require all sub-`OutEvent`s to implement `Debug`, I don't think the same applies to traits like `Clone`. I suggest users that need `Clone` to define their own `OutEvent`.

…eneric-connection-management

thomaseizinger

Hope you don't mind the early, unsolicited review :)
I've added a few thoughts but overall, this looks really interesting. I like how we move more and more to a plugin system and libp2p-swarm is becoming a kind of "executor" for these plugins.

thomaseizinger · 2022-08-19T08:26:39Z

Cargo.toml

+    # TODO: Should this really be a default?
+    "connection-limit",


Long term, I would say no because of #2173. For consistency with the current way of doing things, probably yes.

thomaseizinger · 2022-08-19T08:29:06Z

protocols/connection-limit/Cargo.toml

@@ -0,0 +1,24 @@
+[package]
+name = "libp2p-connection-limit"


Does this have to be its own crate? I think this could also live in libp2p-swarm.

It can still be feature-flagged there if we want but looking at the dependencies of this crate, we don't really gain much at the moment from separating this out.

Separate crate forces us to deliver on the promise of being able to implement ones own.

That said, agreed that we don't gain much more and instead add complexity.

thomaseizinger · 2022-08-19T08:29:39Z

protocols/connection-limit/src/lib.rs

+
+impl NetworkBehaviour for Behaviour {
+    type ConnectionHandler = DummyConnectionHandler;
+    type OutEvent = ();


Perhaps use Void instead?

thomaseizinger · 2022-08-19T08:33:13Z

swarm/src/behaviour.rs

+// TODO: Needed in the first place? Are there some common errors that we want?
+#[derive(Debug)]
+pub enum ReviewDenied {
+    Error(Box<dyn Error>),


This should definitely have a 'static bound.

thomaseizinger · 2022-08-19T08:33:47Z

swarm/src/behaviour.rs

@@ -778,3 +808,9 @@ impl Default for CloseConnection {
        CloseConnection::All
    }
 }
+
+// TODO: Needed in the first place? Are there some common errors that we want?


Perhaps just having these function return Box<dyn Error + 'static> is enough?

Something like LimitReached seems to very common to me, so would be nice to have that as a specifc one, maybe with an optional message for customizability

thomaseizinger · 2022-08-19T08:37:52Z

protocols/connection-limit/tests/lib.rs

+        for _ in 0..outbound {
+            swarm
+                .dial(
+                    DialOpts::peer_id(PeerId::random())


Does PeerId::random only work here because we never poll the swarm?

I think this is misleading, why not dial without PeerId instead?

thomaseizinger · 2022-08-19T08:40:23Z

protocols/connection-limit/tests/lib.rs

+
+#[test]
+fn enforces_pending_outbound_connection_limit() {
+    fn prop(outbound: u8) {


Sorry for OT but we have so many different ways of testing swarms in our repo, it makes me uneasy every time 😅

I'll put this on my list to work on from September forward :D

thomaseizinger · 2022-08-19T08:44:50Z

swarm/src/lib.rs

@@ -282,6 +285,7 @@ where
    /// similar mechanisms.
    external_addrs: Addresses,

+    // TODO: I think this should move into the connection manager behaviour as well.


Into the behaviour or into a behaviour. I was thinking the same thing when I thought about this topic. Keeping a record of banned peers can likely be a NetworkBehaviour itself IMO.

thomaseizinger · 2022-08-19T08:45:45Z

swarm/src/behaviour.rs

+        _peer_id: Option<PeerId>,
+        // TODO: Maybe an iterator is better?
+        _addresses: &[Multiaddr],
+        _endpoint: Endpoint,


Instead of passing the endpoint, would it make sense to have two different callbacks, one for incoming and one for outgoing?

thomaseizinger · 2022-08-19T08:49:49Z

swarm/src/behaviour.rs

@@ -191,6 +192,35 @@ pub trait NetworkBehaviour: 'static {
        vec![]
    }

+    // TODO: Make sure this and all the methods below are really implemented across all behaviours.
+    fn review_pending_connection(


I am not fully sold on the "review" terminology but I also can't think of something better off the top of my head.

Not a fan of "review" either. Grateful for naming suggestions!

thomaseizinger · 2022-12-16T01:12:50Z

I am closing this in favor of #3099.

mxinden added 12 commits August 14, 2022 13:37

*: Generic connection management

3927ca2

protocols/connection-limit: Introduce

7bfdba6

protocols/connection-limit: Test pending inbound connection limit

835394c

*: Test outbound established limit

553f280

swarm-derive/: Add where clause of behaviour to generated out event

cb42dd3

When generating the `OutEvent` for a `NetworkBehaviour`, add the `where` clause of the `NetworkBehaviour` `struct` to the generated `enum`.

swarm-derive/: Do the changes

3f58ccd

*: Review established connections

0e682ec

*: Enforce outgoing established connection limit

ebe1131

*: Enforce outgoing established connection limit

7335a94

Merge branch 'master' of https://github.com/libp2p/rust-libp2p into g…

486daba

…eneric-connection-management

protocols/connection-limit: Decrease count

a7eb4cb

This was referenced Aug 19, 2022

swarm/: Support generic connection management #2824

Closed

Use Multiaddr instead of ConnectedPoint in DialError::WrongPeerId #2793

Draft

thomaseizinger reviewed Aug 19, 2022

View reviewed changes

mxinden mentioned this pull request Aug 20, 2022

swarm/behaviour: Inject events via single method #2832

Closed

mxinden mentioned this pull request Sep 26, 2022

core/muxing: Use total number of alive inbound streams for backpressure #2878

Closed

8 tasks

mxinden mentioned this pull request Oct 5, 2022

Allow to read current connection limits #2809

Closed

ramfox mentioned this pull request Feb 17, 2023

iroh p2p connect improvements n0-computer/beetle#150

Open

thomaseizinger closed this Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm/: Support generic connection management through NetworkBehaviour #2828

swarm/: Support generic connection management through NetworkBehaviour #2828

mxinden commented Aug 19, 2022

thomaseizinger left a comment

thomaseizinger Aug 19, 2022

thomaseizinger Aug 19, 2022

mxinden Aug 23, 2022

thomaseizinger Aug 19, 2022

thomaseizinger Aug 19, 2022

thomaseizinger Aug 19, 2022

dignifiedquire Aug 23, 2022

thomaseizinger Aug 19, 2022

thomaseizinger Aug 19, 2022

thomaseizinger Aug 19, 2022

thomaseizinger Aug 19, 2022

thomaseizinger Aug 19, 2022

thomaseizinger Aug 19, 2022

mxinden Aug 23, 2022

thomaseizinger commented Dec 16, 2022

swarm/: Support generic connection management through NetworkBehaviour #2828

swarm/: Support generic connection management through NetworkBehaviour #2828

Conversation

mxinden commented Aug 19, 2022

Description

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Dec 16, 2022