On connection management

[thomaseizinger]

At the moment, implementations of NetworkBehaviour always need to implement their own connection management.

I'd like discuss how we can improve this situation.

1. NetworkBehaviourAction::NotifyHandler silently drops an event if no connection exists. As we can see from libp2p-request-response, there is clearly a demand for functionality a la "send this message, if no connection exists, make a new one".

2. There is no concept of an "Address Book" on the Swarm level. Often times, it would be nice if we could feed the Swarm with a mapping of PeerIds and Multiaddr that would be taken into account when dialing peers. We might even want to go as far as using a similar mechanism as was taken for external addresses and entirely remove addresses_of_peer from NetworkBehaviour in favor of a NetworkBehaviourAction where NetworkBehaviours can inform the Swarm about new addresses they discovered. Internally, the Swarm would keep all these addresses and score them. I think it would also be useful if there was an easy way of dumping and restoring that information so it can be persisted between restarts.

[mxinden]

Agreed that there is room for improvement on address management in rust-libp2p. A couple of follow-ups to the suggestions above:

• While I do think NetworkBehaviour::addresses_of_peer in some sense serves as a decentralized address book, there are likely use-cases that are better served with a single central address book. To explore which interfaces work the best, I would suggest starting with an AddressBook that implements NetworkBehaviour. Paired with the ReportRemoteAddr below, this should enable a proof-of-concept.

• I think adding something along the lines of ReportRemoteAddr to NetworkBehaviourAction is beneficial, even beyond the idea of a central address book. E.g. libp2p-kad would not depend on add_address to be called by the user any longer after discovering addresses e.g. through libp2p-identify.

• Signed Peer Records (Signed Peer Records #2081) should be a core building block of the new address book component.

I think it would also be useful if there was an easy way of dumping and restoring that information so it can be persisted between restarts.

That would be a cool feature for the address book, indeed.

NetworkBehaviourAction::NotifyHandler silently drops an event if no connection exists. As we can see from libp2p-request-response, there is clearly a demand for functionality a la "send this message, if no connection exists, make a new one".

Agreed, that this is less than ideal for libp2p-request-response, though I am not sure how often this happens in practice, as one is on the order of ms (connections) and the other on the order of μs (state changes). Has anyone by any chance collected data on this?

[thomaseizinger]

To explore which interfaces work the best, I would suggest starting with an AddressBook that implements NetworkBehaviour.

I understand the reasoning but I would want to discuss an alternative idea as well before we straight go for that. In particular, I think we can provide a better user experience if the AddressBook is a first-class citizen within the Swarm, contrary to the user having to compose an AddressBook into their behaviour.

If we implement a ReportRemoteAddr feature anyway, I think we can get away with not touching the public API of Swarm at all while at the same time adding an AddressBook within it that gets taken into account as part of looking up addresses for a peer.

At a later stage, once we collected some feedback on how well this works, we can add public APIs on Swarm for manually adding / removing / dumping these addresses. Less (in this case no) changes on the public API should allow for easier iteration on the feature.

Signed Peer Records should be a core building block of the new address book component.

I agree that they should definitely play a role, can you expand on what "core" means for you? I think there are usecases where we'd want to add an entry to the address book without having a signed peer record. Those should still be possible so whatever API we come up with shouldn't enforce the use of a signed peer record IMO.

[mxinden]

I haven't yet formed a strong opinion whether the AddressBook should be within libp2p-swarm or implemented as a NetworkBehaviour. As far as I can tell NetworkBehaviour with the addition of ReportRemoteAddr has all the hooks it needs to implement an AddressBook, thus my suggestion to start as a NetworkBehaviour with the option to be included into libp2p-swarm directly in the future. Either way, whoever wants to explore this, should make the choice how they want to explore it. While I am happy to help, I currently want to focus on #2052.

Signed Peer Records should be a core building block of the new address book component.

I agree that they should definitely play a role, can you expand on what "core" means for you? I think there are usecases where we'd want to add an entry to the address book without having a signed peer record. Those should still be possible so whatever API we come up with shouldn't enforce the use of a signed peer record IMO.

I think we are on the same page here.

[thomaseizinger]

I haven't yet formed a strong opinion whether the AddressBook should be within libp2p-swarm or implemented as a NetworkBehaviour. As far as I can tell NetworkBehaviour with the addition of ReportRemoteAddr has all the hooks it needs to implement an AddressBook, thus my suggestion to start as a NetworkBehaviour with the option to be included into libp2p-swarm directly in the future.

We'd also need to extend NetworkBehaviour with a callback along the lines of remote_address_reported, otherwise an AddressBook wouldn't be able to learn about addresses reported by other behaviours via ReportRemoteAddr.

I don't think that such an API change is a good idea while we are in an exploration phase of what an address book should look like, esp. given the vision that at some point AddressBook will be a first-class citizen within a Swarm. Once that is the case, such a callback would have to be deprecated / removed again.

I'll give it a crack this weekend and see what I can come up with!

[thomaseizinger]

Submitted a PR here: #2133

[thomaseizinger]

Regarding this:

NetworkBehaviourAction::NotifyHandler silently drops an event if no connection exists. As we can see from libp2p-request-response, there is clearly a demand for functionality a la "send this message, if no connection exists, make a new one".

What are your thoughts on introducing something like NotifyHandler::New?

Alternatively, we could do the same thing we do for substreams (OpenInfo). With substreams, we can pass the an event together with OutboundSubstreamRequest. We could extend DialPeer with an (optional?) field event: TInEvent that gets injected into the handler right after the connection has been established.

[mxinden]

Meta: Thank you for all the work you put into improving the libp2p core and swarm interfaces!

---

What are your thoughts on introducing something like NotifyHandler::New?

I am not opposed to the idea, though I wonder whether there is an even better abstraction. One thing that I can think of would be to add an inject_notify_handler_failure callback, returning the message destined for a handler to the NetworkBehaviour in case it could not be delivered.

Alternatively, we could do the same thing we do for substreams (OpenInfo). With substreams, we can pass the an event together with OutboundSubstreamRequest. We could extend DialPeer with an (optional?) field event: TInEvent that gets injected into the handler right after the connection has been established.

I had a similar thought while working on #2076, though in order to be consistent with the behavior of ProtocolHandlerEvent::OutboundSubstreamRequest, the OpenInfo would need to be injected into NetworkBehaviour::inject_connection_established and not the ProtocolHandler, right?

Given that you are writing "alternatively", I am assuming that you have an idea to solve the above problem with this, namely "send this message, if no connection exists, make a new one". Can you expand how an OpenInfo for connections would solve this?

[thomaseizinger]

I am not opposed to the idea, though I wonder whether there is an even better abstraction. One thing that I can think of would be to add an inject_notify_handler_failure callback, returning the message destined for a handler to the NetworkBehaviour in case it could not be delivered.

Yeah that might be useful on its own. Certainly better than failing silently :)

I had a similar thought while working on #2076, though in order to be consistent with the behavior of ProtocolHandlerEvent::OutboundSubstreamRequest, the OpenInfo would need to be injected into NetworkBehaviour::inject_connection_established and not the ProtocolHandler, right?

Given that you are writing "alternatively", I am assuming that you have an idea to solve the above problem with this, namely "send this message, if no connection exists, make a new one". Can you expand how an OpenInfo for connections would solve this?

My vision is to reduce the amount of state that NetworkBehaviours need to keep in order to function properly. Ideally, the only state necessary is a VecDeque of NetworkBehaviourActions and based on callbacks being invoked, we push a different event.

The more state we can push into libp2p-core/libp2p-swarm, the easier it is to write correct NetworkBehaviours.

If we have an OpenInfo that we can attach to DialPeer, the workflow in a NetworkBehaviour could look like this (assuming we have inject_notify_handler_failure):

1. User wants to send a message to a peer: We emit NetworkBehaviourAction::NotifyHandler.
2. If that succeeds, happy days.
3. If it doesn't succeed, we need to make a new connection: Upon inject_notify_handler_failure, we can push a NetworkBehaviourAction::DialPeer together with the message we want to send in the OpenInfo.
4. If that works, we get called with inject_connected and can push another event NetworkBehaviourAction::NotifyHandler for the newly established connection.
5. If the dialing fails inject_dial_failure gets called (with the OpenInfo as well) and we can push a NetworkBehaviourAction::Custom where we tell the user that whatever they tried to do failed because we couldn't dial.

To speed this up a bit, we could also extend PollParameters with a function to check if we are currently connected to a peer, then we could immediately make the decision on whether to use NotifyHandler or DialPeer.

though in order to be consistent with the behavior of ProtocolHandlerEvent::OutboundSubstreamRequest, the OpenInfo would need to be injected into NetworkBehaviour::inject_connection_established and not the ProtocolHandler, right?

For consistency reasons, yeah that might good. But it requires a lot of API changes. On the other hand, injecting it directly into the handler is what you always might want to do?

[mxinden]

My vision is to reduce the amount of state that NetworkBehaviours need to keep in order to function properly.

The more state we can push into libp2p-core/libp2p-swarm, the easier it is to write correct NetworkBehaviours.

👍 definitely a goal worth striving for.

If we have an OpenInfo that we can attach to DialPeer, the workflow in a NetworkBehaviour could look like this (assuming we have inject_notify_handler_failure):

1. User wants to send a message to a peer: We emit `NetworkBehaviourAction::NotifyHandler`.

2. If that succeeds, happy days.

3. If it doesn't succeed, we need to make a new connection: Upon `inject_notify_handler_failure`, we can push a `NetworkBehaviourAction::DialPeer` together with the message we want to send in the `OpenInfo`.

4. If that works, we get called with `inject_connected` and can push another event `NetworkBehaviourAction::NotifyHandler` for the newly established connection.

5. If the dialing fails `inject_dial_failure` gets called (with the `OpenInfo` as well) and we can push a `NetworkBehaviourAction::Custom` where we tell the user that whatever they tried to do failed because we couldn't dial.

This sounds great.

To speed this up a bit, we could also extend PollParameters with a function to check if we are currently connected to a peer, then we could immediately make the decision on whether to use NotifyHandler or DialPeer.

PollParameters were always a thorn in my side, since they mix the otherwise event driven architecture with a synchronous call interface. I haven't formed a strong opinion on those yet, though off the top of my head, I would prefer not extending them.

For consistency reasons, yeah that might good. But it requires a lot of API changes. On the other hand, injecting it directly into the handler is what you always might want to do?

One example would be the Direct Connection Upgrade through Relay protocol (see #2076). Once a direct (hole punched) connection is established with the remote, one does not want to notify the protocols handler of the direct connection, but much rather the protocols handler of the relayed connection, in order to close the relayed connection. Thus receiving the OpenInfo at the NetworkBehaviour instead of the ProtocolsHandler directly would simplify the implementation. Not sure though, whether this use-case should influence the design.

[thomaseizinger]

PollParameters were always a thorn in my side, since they mix the otherwise event driven architecture with a synchronous call interface. I haven't formed a strong opinion on those yet, though off the top of my head, I would prefer not extending them.

I see your concern. It might be worth exploring ways of getting rid of them then. Will keep it in mind.

One example would be the Direct Connection Upgrade through Relay protocol (see #2076). Once a direct (hole punched) connection is established with the remote, one does not want to notify the protocols handler of the direct connection, but much rather the protocols handler of the relayed connection, in order to close the relayed connection. Thus receiving the OpenInfo at the NetworkBehaviour instead of the ProtocolsHandler directly would simplify the implementation. Not sure though, whether this use-case should influence the design.

That is a good enough usecase to show that we should not be opinionated and here and just inject it into the NetworkBehaviour.

[mxinden]

Just for your information @thomaseizinger: I am trialing the additional OpenInfo in NetworkBehaviourAction::{DialAddress, DialPeer} for #2076 locally.

[mxinden]

Following up on this discussion, though with a slight change of theme.

Today a user can:

1. Set a limit for incoming/outgoing/pending connections, global and per peer.
2. Close a connection via NetworkBehaviour.
3. Keep a connection alive via ConnectionHandler::keep_alive.

A user is restricted in (1) as they can not do anything beyond setting upper bound limits. E.g. they can not decide whether to accept or deny an incoming connection based on the IP, the PeerId or on their current CPU or memory utilization.

Say we would introduce a mechanism for a user to make these decisions (while still offering a default), the only open question for me is whether the user can decide in a synchronous fashion, or whether they would need an asynchronous communication style. To be more concrete, when provided with the ask to accept or deny an incoming connection, can the user respond directly, or would the user need an asynchronous mechanism?

@AgeManning @divagant-martian What would you need to make connection management in lighthouse simpler? Would the connection management logic be able to make a synchronous decision, or does it need to communicate with another component in an asynchronous fashion (e.g. through a channel) to make the decision?

@dignifiedquire would you be able to make such decision in iroh in a synchronous fashion?

[mxinden]

Friendly ping @dignifiedquire.

[dignifiedquire]

there is a high likelihood that I would need to trigger a database access later down the line, which will entail a network call in somw cases, so having that forced to be sync would be workable, but not ideal

[mxinden]

For the record, 3 proposals for generic connection management are documented below.

#2824

[AgeManning]

For us (which is likely quite specific to our design) our connection management is handled via mutexes, so we can obtain a result synchronously.

This feature would be a nice-to-have for us because, as you've mentioned we currently lack the ability to accept/deny incoming connections on other heuristics. We still currently do this, but we do this at a higher level, once the connection is established (or via the swarm banning mechanism) if we don't want to accept connections from specific peer-ids.

So for us, synchronous is fine.