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?

[thomaseizinger]

Thank you for pointing out this overlap! It allowed me to look at the source code from a new perspective and I agree with you that it should be possible to provide a much simpler abstraction layer on top of libp2p-core that fulfills the same role as libp2p-swarm but with less indirection.

I did some thinking on what such a simplified abstraction could look like. The thing I am primarily interested in is a design that conceptually merges NetworkBehaviour and ProtocolsHandler (these are both concepts of libp2p-swarm so they would not actually be present). In our usage of libp2p, these abstractions are most of the time just boilerplate because we have a lot of single peer but multi-message protocols. Hence an abstraction that gives me access to a substream right away would be a lot easier to use.

For simple protocols with a single peer like request-response, it should be possible to only provide:

• The protocol identifier
• A future that is given the substream so that I can use write_one and read_one in the appropriate order to exchange the messages I want

Protocols with more than two messages could easily be built on top of that.

Following down the code paths, I came across the ConnectionHandler abstraction which seems to be component at the lowest level that follows the callback API style. Did you put any thoughts into whether that abstraction would have to change as well or would you build anything new on top of that?

[thomaseizinger]

Looking again, the ConnectionHandler abstraction seems to be fine. The only thing that will certainly get in the way is that the Network is parameterized over the In/Out-Events of the ConnectionHandler. Hence, it is not quite as simply has having the user provide implementations of ConnectionHandler for all their protocols because the types would not fit.

Maybe a custom derive to compose ConnectionHandlers into a tree could do the trick, the same way the NetworkBehaviour macro works?

[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<Option<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.