Review of NetworkBehaviour

[dvc94ch]

I actually kind of like the NetworkBehaviour in general. Some things that may help would be:

inject_new_external_addr needs a inject_expired_external_addr when Swarm::remove_external_address is called.

inject_new_listen_addr and inject_expired_listen_addr should pass a ListenerId.

SwarmEvent and NetworkBehaviour inject methods seem to be two ways of doing the same thing (in many cases). Maybe deprecate SwarmEvent and adding some missing methods to NetworkBehaviour would be sensible?

For writing tests (where I think SwarmEvent is probably most useful), a NetworkBehaviour can be added that allows registering a channel to get something like a SwarmEvent.

Another idea would be remove swarm methods completely and make NetworkBehaviourAction more powerful instead.

[romanb]

SwarmEvent and NetworkBehaviour inject methods seem to be two ways of doing the same thing (in many cases). Maybe deprecate SwarmEvent and adding some missing methods to NetworkBehaviour would be sensible?

The SwarmEvent is actually the "newer" of these, at least in its current form, see e.g. #1515. I agree that the current situation, where there are NetworkEvents, SwarmEvents (an almost 1-1 mirror of NetworkEvent by now) as well as all the callbacks on a NetworkBehaviour, makes for quite redundant APIs. Personally, I would actually prefer going in the direction of removing libp2p-swarm (and libp2p-swarm-derive) and providing a much more minimal interface and API for composing protocols by means of composing event handlers that directly consume NetworkEvents as part of libp2p-core. A kind of more radical variant of #1522. The main problems I see with the granular callback-based APIs of the NetworkBehaviour are:

1. It is often unclear, especially when getting started, in which order the callbacks are invoked and what guarantees are given in terms of when / if they are invoked and what the pre- and post-conditions of each callback method are. But precisely understanding the semantics of each callback is paramount to implementing a NetworkBehaviour that works correctly and e.g. does not leak memory in local data structures.
2. Adding new callback methods can be rather painful and even subject to pitfalls if they are given default implementations that are not properly overridden in types that serve as combinators. This is especially true for the ProtocolsHandler API. Composition of NetworkBehaviours and ProtocolsHandlers in general gets more cumbersome the larger the API is (and the less clear all the details are w.r.t. the contract of each method, as mentioned in 1.). See also Rework the way one combines NetworkBehaviours together #1021. swarm-derive adds to this burden, as every NetworkBehaviour API change requires adapting that complicated derive macro.

Assuming the current design of libp2p-core remains event-based and with background tasks for connections, I see the challenge as essentially defining an interface for composable NetworkEvent listeners that is as minimal as possible, letting individual application protocols directly consume these NetworkEvents together with the ability to have access to a &mut Network. The substream protocol upgrade logic currently in the NodeHandlerWrapper of libp2p-swarms ProtocolsHandler certainly would need to be turned into reusable code in libp2p-core. The use of connection background tasks and the event-based communication with ConnectionHandlers/ProtocolsHandlers may certainly also be reviewed for improvement in that context, see #685. A main challenge would of course also be to provide a smooth upgrade path for the existing protocols in src/protocols, not to mention external implementations of NetworkBehaviour.

This is just a rough outline of the direction that I would personally pursue. In fairness I need to mention though that as I'm switching employers, I'm wrapping up my work on rust-libp2p and will no longer be actively working on it by the end of this week already, apart from assisting in PR reviews where requested. So I'm just sharing my thoughts here in case they may be useful and won't actually do any of this. I'm confident though that @mxinden along with all the other old and new contributors, yourself included, will steer it in a good direction.

[dvc94ch]

really sad to hear you're leaving, you and mxinden are currently the main maintainers/contributors. will there be someone from parity taking over your role in libp2p or does parity consider libp2p "finished"? also curious who your new employer is, although maybe slightly off topic.

the approach you suggest sounds good, but also fairly complicated and involved. Maybe we can merge #1995 anyway as a plaster until someone tackles it like you suggest.

[romanb]

will there be someone from parity taking over your role in libp2p or does parity consider libp2p "finished"?

I honestly don't know what the plans are w.r.t. Parity and rust-libp2p.

Maybe we can merge #1995 anyway as a plaster until someone tackles it like you suggest.

I assume you meant #2007. I've given it a review.

[mxinden]

First off, I appreciate the detailed comments above, especially the many links to past discussions. Below are my thoughts on the matter.

Status quo

I agree that the status quo, and more particular the design around NetworkBehaviour NetworkEvent and SwarmEvent is problematic due to:

1. It's sheer complexity, especially for newcomers, (see @romanb description above).
2. The problems arising from it combining a synchronous method approach (e.g. swarm.behaviour_mut().send_request(..)) and an event driven approach (e.g. via SwarmEvent). One such problem is, that it is hard to enforce back-pressure as an implementor of NetworkBehaviour, as one can not block the callee on methods calls like RequestResponse::send_request. This results in the many unbounded queues that we have today, e.g. RequestResponse::pending_outbound_requests, though I guess one could require the callee to pass a Context, allowing similar mechanisms like Sink::poll_ready.

Minimal interface suggestion

Personally, I would actually prefer going in the direction of removing libp2p-swarm (and libp2p-swarm-derive) and providing a much more minimal interface and API for composing protocols by means of composing event handlers that directly consume NetworkEvents as part of libp2p-core.

I am not sure I fully understand the suggestion above, thus please correct me if I am wrong with the small note below.

I am currently conducting user interviews with various users of rust-libp2p. (As an aside, in case users are reading this and would like to share thoughts, suggestions ... on rust-libp2p as a whole in a call, send me a mail). One thing that came up multiple times is the lack of guidance rust-libp2p gives to users / implementors. Lack of guidance in the sense that while rust-libp2p is very versatile, at the same time it is hard to use given there being various possible solutions to a given problem. While not perfect, the design around NetworkBehaviour does give users / implementors a predefined structure which is in some sense guiding them. I feel like that (among other things) the somewhat static structure of NetworkBehaviour is one reason why many implementations in rust-libp2p/protocols/ share the same architecture, which I find to be a great benefit. I think this is something worth keeping in mind when removing libp2p-swarm.

[tomaka]

will there be someone from parity taking over your role in libp2p or does parity consider libp2p "finished"?

We have a job offer up: https://www.parity.io/apply/?gh_jid=4347843003

[romanb]

One thing that came up multiple times is the lack of guidance [..] While not perfect, the design around NetworkBehaviour does give users / implementors a predefined structure which is in some sense guiding them. [..]

Note that, to oversimplify a bit, libp2p-swarm just turns NetworkEvents into calls to NetworkBehaviour callbacks, plus some extra logic. I don't think in that sense that an interface with callbacks provides more or less guidance than the NetworkEvent enum variants (or SwarmEvent for that matter). As mentioned before, to me these coexisting different APIs just seem too overlapping and I don't think anything important is lost by providing a simpler interface whereby one just directly gets to process events. In my mind a simpler interface (i.e. less methods and their potential interactions to understand) is pretty much always better. I may be mistaken, of course, but I just don't think the libp2p-swarm indirection / abstraction is that beneficial over an API that gives more direct access to the Network and NetworkEvents. The substream upgrade logic seems to me to be the only really important part in libp2p-swarm that one certainly wants to have in one form or another. There is certainly still a need for an interface that application protocols can implement, like the NetworkBehaviour, my suggestion is simply to take #1522 one step further by dropping the SwarmEvent / Swarm abstraction.

Is anyone opposed if we move this to a github discussion, rather than keeping it as an issue?

[dvc94ch]

If I understand correctly the changes would be:

1. The new NetworkBehaviour would have the new_handler, addresses_of_peer, inject_event, poll, where inject_event gets passed an additional NetworkEvent variant.
2. Some of the stuff that the swarm does would be moved into core, including the substream upgrade logic.
3. The NetworkBehaviour gets a new method that takes a Network? (still don't fully understand how the Network and the NetworkBehaviour would interact)

[romanb]

The new NetworkBehaviour would have the new_handler, addresses_of_peer, inject_event, poll, where inject_event gets passed an additional NetworkEvent variant.

Perhaps, yes. I was thinking about something even more minimal though. These are completely unfinished thoughts but what I at one point drafted in pseudo-code was something like

trait Protocol {
    type Command;
    fn notify(&mut self, e: &mut NetworkEvent<'_>) -> Result<Self::Command>;
    fn execute(&mut self, n: &mut Network<...>, c: Self::Command) -> Result<???>;
    fn poll(&mut self, ...) -> Poll<???>;
}

where the methods would be called for each "registered" protocol in the order listed by some NetworkProtocolStack that owns a Network. The motivation for the notify() + execute() sequence being that it should allow the NetworkEvent, as it currently does, to borrow stuff from the Network, meaning we cannot pass both a NetworkEvent and a &mut Network to the same callback. At the same time, Self::Command allows a protocol to pass commands from notify() to execute() without necessarily keeping local state between these calls. If keeping the connection background tasks and ConnectionHandlers, poll() would probably allow sending events to these very similarly to the current NetworkBehaviourActions.

Some of the stuff that the swarm does would be moved into core, including the substream upgrade logic.

Yes, certainly.

The NetworkBehaviour gets a new method that takes a Network? (still don't fully understand how the Network and the NetworkBehaviour would interact)

Like I said, I really didn't think this through and I have no doubt there are many obstacles ahead. Experimenting and thinking through all the details would be what it entails to explore this direction.