Request-Response: Add method connected_peers

[elenaf9]

The request-response protocol manages a list of connected peers. This list is currently not exposed to the user, only the method RequestResponse::is_connected allows to the check the connection for a specific peers.
However, in case that the user needs to know the whole list of connected peers, it forces them to also track connected peers themself, which is redundant as the information is already there, e.g. as it currently has to be done in the AutoNAT draft #2262.
56f1ede adds a method RequestResponse::connected_peers to share the list of connected peers.

Apart from that, I noticed that at the moment inject_address_change is not handled, which means that the associated Connection still has the address of the old ConnectedPoint. bd6622a changes this, unless there was a specific reason to do so?
Note that with this change, if the ConnectedPoint changed from Dialer to Listener, the address will change from Some(old_addr) to None. Are there cases in which we still want to keep the old address? If I am not mistaken, the address is only used to return it in addresses_of_peer, therefore I would say it's valid to remove an outdated address?

[mxinden]

The request-response protocol manages a list of connected peers. This list is currently not exposed to the user, only the method RequestResponse::is_connected allows to the check the connection for a specific peers. However, in case that the user needs to know the whole list of connected peers, it forces them to also track connected peers themself, which is redundant as the information is already there, e.g. as it currently has to be done in the AutoNAT draft #2262. 56f1ede adds a method RequestResponse::connected_peers to share the list of connected peers.

I am undecided on this one. On the one hand, yes, it is just a small getter method. On the other hand:

• It includes an internal concept in the public API. Say e.g. that at some point we don't want to track all, but only a subset of connected peers. Even though nothing within libp2p-request-response would break, it would still require a breaking version release.
• I would argue that tracking the connected peers in each behaviour is easy (via inject_connected and inject_disconnected) and I doubt it is performance relevant.
• I oftentimes find myself having to track additional data with each peer, specific to the NetworkBehaviour implementation. E.g. for libp2p-autonat we might want to track in the future, whether the connected peer is within our subnetwork, or connected via the public internet. If I am not mistaken libp2p-autonat would then have to do its own tracking anyways.

Apart from that, I noticed that at the moment inject_address_change is not handled, which means that the associated Connection still has the address of the old ConnectedPoint. bd6622a changes this, unless there was a specific reason to do so? Note that with this change, if the ConnectedPoint changed from Dialer to Listener, the address will change from Some(old_addr) to None. Are there cases in which we still want to keep the old address? If I am not mistaken, the address is only used to return it in addresses_of_peer, therefore I would say it's valid to remove an outdated address?

Good catch. I am not aware of any cases, where this would not be desired behaviour. Thanks for adding it.

[elenaf9]

It includes an internal concept in the public API. Say e.g. that at some point we don't want to track all, but only a subset of connected peers. Even though nothing within libp2p-request-response would break, it would still require a breaking version release.

Isn't this already the case right now because we have is_connected? I can not think of a change that would break the implementation of connected_peers, but not of is_connected, since they both depend on the same property.

I would argue that tracking the connected peers in each behaviour is easy (via inject_connected and inject_disconnected) and I doubt it is performance relevant.

I oftentimes find myself having to track additional data with each peer, specific to the NetworkBehaviour implementation. E.g. for libp2p-autonat we might want to track in the future, whether the connected peer is within our subnetwork, or connected via the public internet. If I am not mistaken libp2p-autonat would then have to do its own tracking anyways.

In regard to AutoNAT you are probably right, yes. But in general this implies that the user implements the behaviour manually. In case of #[derive(NetworkBehaviour)] this would not be possible, so if then a user needs the list of connected peers they would have to implement the whole logic on a higher level when polling the Swarm.

I don't really have a strong opinion on this. For AutoNAT I agree with you that eventually we have to track the connected peers separately anyway. And e.g. Swarm also only provides a is_connected method, but not a connected_peers.
So by explicitly not providing these methods it enforces the usage of NetworkBehaviour trait implementation and / or SwarmEvents to track the networks state, which is also a valid design.
I am fine with either, so if you'd like to close this PR I could just open a new one that only cherry picks the inject_address_change patch bd6622a.

[mxinden]

It includes an internal concept in the public API. Say e.g. that at some point we don't want to track all, but only a subset of connected peers. Even though nothing within libp2p-request-response would break, it would still require a breaking version release.

Isn't this already the case right now because we have is_connected? I can not think of a change that would break the implementation of connected_peers, but not of is_connected, since they both depend on the same property.

Fair point!

In regard to AutoNAT you are probably right, yes. But in general this implies that the user implements the behaviour manually. In case of #[derive(NetworkBehaviour)] this would not be possible, so if then a user needs the list of connected peers they would have to implement the whole logic on a higher level when polling the Swarm.

Do I understand correctly that this mechanism would not only be useful for a NetworkBehaviour wrapping RequestResponse, but also for a user of Swarm who could then do swarm.behaviour().connected_peers? For the latter I would much rather prefer exposing Network::connected_peers via Swarm::connected_peers, thus returning the data right from the source (Network), always available no matter if the user uses RequestResponse or not.

I am fine with either, so if you'd like to close this PR I could just open a new one that only cherry picks the inject_address_change patch bd6622a.

My preference is still to include bd6622a only. Others might have an opinion as well though.

[elenaf9]

Do I understand correctly that this mechanism would not only be useful for a NetworkBehaviour wrapping RequestResponse, but also for a user of Swarm who could then do swarm.behaviour().connected_peers? For the latter I would much rather prefer exposing Network::connected_peers via Swarm::connected_peers, thus returning the data right from the source (Network), always available no matter if the user uses RequestResponse or not.

Good point, yes I agree.
Since nobody else voiced any opinion on this (and it's only a minor thing anyway), I am closing this PR now. Anybody who needs Swarm::connected_peers should feel free to add a PR for it themself, or cherry-pick 56f1ede if they need it in the Request-Response protocol after all.
PR #2362 will add the address-change handling.

[mxinden]

For the record connected_peers is added via #2378.