make it possible to listen to some events in a derived NetworkBehaviour

[rkuhn]

Yesterday I had a case where deriving the behaviour was perfect apart from one line — I needed to stay informed of peers disconnecting. With the ability to expand the proc macro in VS Code this was a matter of seconds, but it would be nicer for readers of the code and for maintainability if this patch could be formulated more elegantly, for example with syntax like

#derive(NetworkBehaviour)
#behaviour(event_process = true, inject_connection_closed = "inject_closed")
struct MyBehaviour {
    // sub-behaviours
}

[mxinden]

I am not sure I follow. You want to be able to override the generated implementation of NetworkBehaviour::inject_connection_closed?

[rkuhn]

Given the current structure, it would be more consistent if the generated implementation would be extended to call the provided associated function. This way sub-behaviours keep working as they were but the higher-level behaviour can still see the events it needs to see. If we allow the implementation to override the behaviour hook, then the higher-level behaviour could filter the events for its sub-behaviours — for better or worse.

To my mind it would be a worthy goal that a NetworkBehaviour works as an encapsulated black box, to be reused in larger behaviour hierarchies. This requires a perfectly balanced API surface so that code on the exterior can do many useful things without being able to break the code on the interior.

[mxinden]

If I understand your suggestion correctly, this could e.g. be useful in libp2p-autonat where many NetworkBehaviour methods simply delegate to the underlying libp2p-request-response NetworkBehaviour, while for other methods one wants to override the delegation:

rust-libp2p/protocols/autonat/src/behaviour.rs

Lines 462 to 499 in 861e15d

	fn new_handler(&mut self) -> Self::ProtocolsHandler {
	self.inner.new_handler()
	}
	fn addresses_of_peer(&mut self, peer: &PeerId) -> Vec<Multiaddr> {
	self.inner.addresses_of_peer(peer)
	}
	fn inject_event(
	&mut self,
	peer_id: PeerId,
	conn: ConnectionId,
	event: RequestResponseHandlerEvent<AutoNatCodec>,
	) {
	self.inner.inject_event(peer_id, conn, event)
	}
	fn inject_listen_failure(
	&mut self,
	local_addr: &Multiaddr,
	send_back_addr: &Multiaddr,
	handler: Self::ProtocolsHandler,
	) {
	self.inner
	.inject_listen_failure(local_addr, send_back_addr, handler)
	}
	fn inject_new_listener(&mut self, id: ListenerId) {
	self.inner.inject_new_listener(id)
	}
	fn inject_listener_error(&mut self, id: ListenerId, err: &(dyn std::error::Error + 'static)) {
	self.inner.inject_listener_error(id, err)
	}
	fn inject_listener_closed(&mut self, id: ListenerId, reason: Result<(), &std::io::Error>) {
	self.inner.inject_listener_closed(id, reason)
	}

[elenaf9]

I agree that this would be useful. But in AutoNAT there are quite a lot of methods with custom implementation, so doing this via the suggested #[behaviour(inject_connection_closed = "inject_closed", ..)] for all custom methods could end up a bit messy.
I wonder if there is a cleaner way to do this via a trait with auto implementations; something along the lines of:

trait WrapperBehaviour {

    type Inner: NetworkBehaviour;

    fn inner(&mut self) -> &mut Self::Inner;
    
    // Mirror all NetworkBehaviour methods with empty body.
    // Implementors can overwrite them if they want a custom implementation.
    fn inject_new_listener(&mut self, id: ListenerId) {}
    ..
}

impl<T: WrapperBehaviour> NetworkBehaviour for T {
    fn inject_new_listener(&mut self, id: ListenerId) { 
        self.inject_new_listener(id);
        self.inner().inject_new_listener(id);
    } 
    ..
}

Behaviours like AutoNAT could then implement WrapperBehaviour and overwrite all methods where they need custom implementations.

I did not think this through yet though.
Also probably a bit more complicated when you wrap multiple Behaviours, but maybe WrapperBehaviour::inner could then return a derived NetworkBehaviour that just wraps these inner behaviors without any additional logic. Or an iterator over mut references to the inner behaviours (probably difficult though because of the types)?

What do you think?

[rkuhn]

Yes, @elenaf9 that is going in the direction I’ve been pondering since opening this issue. What I’d like to have is the ability to compose behaviours as mostly black boxes — and what I couldn’t yet figure out is how large the deviation from “black” will need to be. In the metaphor, we’d need at least some non-black lettering to state the purpose and abilities of the box :-)

Here’s my current train of thought, which has not yet reached its destination. A network behaviour represents the ability to trigger something to happen inside a swarm (whereby I mean multiple nodes). This could be moving data around, keeping tabs on who’s who, gossiping some (future) information, etc. The current NetworkBehaviour macro solves one part of this problem, namely the case where the local node confidently presumes the capabilities of its environment. I seek to go one step further: there are multiple ways of actually performing that “something” in the swarm, since protocols evolve over time, and each node probably knows several of these ways. The strength I see in libp2p is that it can serve as a matchmaker, selecting the preferred option from the intersection of capabilities of peers.

In this sense I want to compose behaviours. It is clear that different purposes require different forms of composition. For example gossiping may well opt to try all known mechanisms to ensure that the message gets spread while moving data around may try multiple ways of finding the data but only one to move it (to not waste bandwidth). And making a request to a single peer should make it only once, choosing the preferred mechanism.

My preliminary conclusion from this is that we’d need something like the generic derived composition of sub-behaviours like @elenaf9 describes, to keep the black boxes functioning normally while they are in fact composed. The second part we’d need is a controller that can influence the composition. For this purpose, it will need to have access to the inputs and outputs of the sub-behaviours. The degree to which this controller will need to understand the inner workings of the composed behaviours will depend on the form of composition. It could be necessary to filter the messages sent from behaviour to protocol handler, for example. As far as I can see, the infrastructure for this is already in place, it is just inconvenient to use (as in: needs a lot of boilerplate code).

---

Sorry for not directly answering your question, Elena, but before I can comment on API I need to better understand the nature of the problem myself. Your proposal feels like it is going in the right direction, though.

[rkuhn]

Sunday morning, cold and bright weather, out in the countryside, taking a walk … and I finally understood:

• NetworkBehaviour operates on the level of a peer, it defines what shall happen
• ProtocolsHandler operates on the level of a connection, i.e. a pair of peers, it defines how things are communicated

This has implications on my desire of protocol reuse sketched above. I now think that my current use-case — namely old and new variants of a streaming response protocol — is not a behaviour concern, it is a protocol concern and should be encapsulated in the ProtocolsHandler: the unit of reuse needs to be the sub-protocol, not the sub-behaviour.

---

With this in mind, I see the following behaviour classes:

• always on: do a certain thing on all established connections, like Ping and Identify
• on-demand: do a certain thing with one select peer at a given time, like OneShot, RequestResponse, StreamingResponse
• autonomous: a behaviour that takes local inputs, but also performs communication on its own accord, like GossipSub and Kademlia — I’d also put bitswap in this category

While the last one is by design bespoke, the first two could be implemented generically on the behaviour level, parameterised over ProtocolsHandlers and some configuration. Then, on the protocol level, it should be possible to compose protocols offering essentially the same shape of communication in a generic fashion, with automatic negotiation for each pair of peers. This approach would make the original point of this issue moot.

What do you think @mxinden @thomaseizinger @elenaf9? How would AutoNAT fit into this?

[thomaseizinger]

What do you think @mxinden @thomaseizinger @elenaf9? How would AutoNAT fit into this?

From the list above, I'd say it falls into autonomous although the line with always-on is a bit blurry. Most of the standard libp2p protocols fall into one or the other.

I have thought a lot about this so I would like to share a few more thoughts. The main driving forces of the current design in my observation are:

• Performance: Separation of a protocol into peer-local state (ProtocolsHandler run in their own task in the runtime) and orchestration logic (NetworkBehaviour)
• Encapsulation: Protocols like AutoNAT or Identify can operate completely independently and with a very small, public API surface, thus allowing a "plug-and-play" like design where you can drop in libp2p-mdns and discovery "just works TM".
• Composability: Allow for a Swarm to have an arbitrary number of NetworkBehaviours that combined provide the Swarms functionality.

The performance aspect drives the separation into two interfaces which introduces the event-driven communication and separated state.
The encapsulation aspect together with composability drives the callback-based design of the NetworkBehaviour trait so a protocol can learn about everything that is happening and act accordingly.

What we can currently not support particularly well with these abstractions are: Protocols that don't have any cross-peer state/logic (Which is not surprising, given that the design is optimized FOR these).

In absence of cross-peer state/logic, the NetworkBehaviour part is usually a complete pass-through to a connection managed by a ProtocolsHandler and the triggers for communication are usually located outside of the NetworkBehaviour. request-response is one of these candidates but also almost every protocol we have built at work falls into the category (we have a lot usecases that require an N-message exchange with a particular peer but nothing else).

---

Circling back to this original issue: I think the deeper issue at hand here is that not every protocol needs all of the 3 above mentioned characteristics.

Some protocols (like request-response) would be much better served if they wouldn't need to go though the ProtocolsHandler abstraction but would like to operate on a substream directly. Same for one-shot and I think also streaming-response? IMO, the fact these (have to) exist is a sign that it is not simple enough to get direct access to a created substream, hence we need them to reduce the boilerplate for users. This is where it gets interesting.

If getting access to the substreams would be easier, then the need for composing a request-response protocol in your own NetworkBehaviour AND listening for certain callbacks would go away: You would just implement NetworkBehaviour manually and listen for the desired callbacks whilst managing the substreams in whichever way you need.

Shameless-plug: I've experimented with an idea in regards to this here:

rust-libp2p/protocols/rendezvous/src/substream_handler.rs

Lines 175 to 185 in 861e15d

	/// An implementation of [`ProtocolsHandler`] that delegates to individual [`SubstreamHandler`]s.
	pub struct SubstreamProtocolsHandler<TInboundSubstream, TOutboundSubstream, TOutboundOpenInfo> {
	inbound_substreams: HashMap<InboundSubstreamId, TInboundSubstream>,
	outbound_substreams: HashMap<OutboundSubstreamId, TOutboundSubstream>,
	next_inbound_substream_id: InboundSubstreamId,
	next_outbound_substream_id: OutboundSubstreamId,
	new_substreams: VecDeque<TOutboundOpenInfo>,
	initial_keep_alive_deadline: Instant,
	}

What this essentially does is create a reusable ProtocolsHandler that allows you to "ignore" connections and instead operate on the basis of substreams directly from the NetworkBehaviour:

rust-libp2p/protocols/rendezvous/src/client.rs

Lines 238 to 248 in 861e15d

	Ok(peer_record) => NetworkBehaviourAction::NotifyHandler {
	peer_id: rendezvous_node,
	event: handler::OutboundInEvent::NewSubstream {
	open_info: OpenInfo::RegisterRequest(NewRegistration {
	namespace,
	record: peer_record,
	ttl,
	}),
	},
	handler: NotifyHandler::Any,
	},

There are obviously multiple ways of how we can tackle this but it is my overall opinion that the current design of NetworkBehaviour and ProtocolsHandler are too geared towards the autonomous and always-on protocols that are defined within the libp2p standards. For use-cases where people just want to use libp2p as a transport layer between two peers, these interfaces are not particularly useful. All of this being said, with #2492, we are making it more clear that these design choices very much belong into libp2p-swarm and thus, the usecases for "libp2p as a transport layer" should likely just use libp2p-core directly.

[rkuhn]

continued at #2938