chore(libp2p): expose Behaviour::connected and Behaviour::addresses

[onur-ozkan]

Description

This pull request aims to meet the need of addresses_of_peer function which was removed from the codebase. By exposing these fields, developers will be able to create their own abstracted layers without the need for direct implementation in the libp2p tree.

Notes

see #3254 (comment)

[thomaseizinger]

I am okay with exposing this information but I'd like to do it via a function please and not by making fields public.

[onur-ozkan]

This had to be pub for:

rust-libp2p/protocols/request-response/src/lib.rs

Lines 355 to 359 in 9c75886

	/// The currently connected peers, their pending outbound and inbound responses and their known,
	/// reachable addresses, if any.
	pub fn connected(&self) -> HashMap<PeerId, SmallVec<[Connection; 2]>> {
	self.connected.clone()
	}

[onur-ozkan]

Because of the existing similar implementations, I decided to clone(or copy if possible) them on return. Let me know if you want to make these functions return references and leave the cloning decision to developers(not every case requires cloning, so cloning them for accessing these values might impact the performance slightly).

[thomaseizinger]

Thinking about it, I'd say it would be better to create a new NetworkBehaviour that just tracks this information and exposes it.

That keeps the API of others small. Other implementations have a PeerStore, perhaps it is time we add that to rust-libp2p?

Design-sketch:

• In-memory storage only
• Exposes connected peers
• Allows adding and removing of addresses for a peer
• Name: libp2p-memory-peerstore

Alternatively, we could add a primitive such as ConnectedPeers, similarly as we have it for ListenAddresses, ExternalAddresses etc.

What exactly is your usecase?

[thomaseizinger]

which was removed from the codebase

Note that the API of NetworkBehaviour is only meant to be used by implementations. We will obv. correctly version changes on it but it will change with the needs we want to serve and what Swarm needs.

As such, I wouldn't rely on it in your code.

[onur-ozkan]

What exactly is your usecase?

We need to find addresses associated with a given peer(e.g. https://github.com/KomodoPlatform/atomicDEX-API/blob/1d8bebd150785123168e770b33e87fac236f0949/mm2src/mm2_libp2p/src/peers_exchange.rs#L110-L126) in our p2p implementation. The layer was built on top of our fork, now I am migrating/refactoring the p2p layer of our project and this PR is kind a blocker for us(just like #3973).

Alternatively, we could add a primitive such as ConnectedPeers, similarly as we have it for ListenAddresses, ExternalAddresses etc.

Not all implementations of Behaviour have the same structure(e.g. identify::Behaviour doesn't have address field), which makes me think that the cost of implementing/maintaining this feature may outweigh its benefits. I believe That keeps the API of others small. Other implementations have a PeerStore, perhaps it is time we add that to rust-libp2p? could be a long-term solution for this issue. Should we do that in this PR? It would be really great if we can land this PR(it's dead simple and doesn't require any testing) and implement your idea bit later.

PS: It seems that we are not the only ones facing this problem. The reason I proposed this PR was due to a discussion I came across at #3254 (comment).

[thomaseizinger]

It would be really great if we can land this PR(it's dead simple and doesn't require any testing) and implement your idea bit later.

The issue is that we are adding a public API here and removing that is a breaking change later. It might be simple in implementation but it is still a commitment to our users. Once it is released, people will start using it.

The quickest solution is for you to write yourself a PeerStore behaviour which only tracks connected peers based on the ConnectionEstablished and ConnectionClosed events. Knowing that the API is battle-tested in your application would make it a lot easier to later upstream it to the repository :)

Similarly to https://github.com/libp2p/rust-libp2p/blob/master/misc/allow-block-list/src/lib.rs, you can use dummy::ConnectionHandler. All you need is one of those HashMaps as we have them in any of the behaviours touched in this PR and you are good to go!

Does that work for you?

[thomaseizinger]

I opened an issue here: #4103

[onur-ozkan]

The issue is that we are adding a public API here and removing that is a breaking change later. It might be simple in implementation but it is still a commitment to our users. Once it is released, people will start using it.

Right. I see the point, thanks for clarification.

The quickest solution is for you to write yourself a PeerStore behaviour which only tracks connected peers based on the ConnectionEstablished and ConnectionClosed events. Knowing that the API is battle-tested in your application would make it a lot easier to later upstream it to the repository :)

Similarly to https://github.com/libp2p/rust-libp2p/blob/master/misc/allow-block-list/src/lib.rs, you can use dummy::ConnectionHandler. All you need is one of those HashMaps as we have them in any of the behaviours touched in this PR and you are good to go!

Does that work for you?

Yeah, we can do it that way too. I keep this PR as a draft until then.

[onur-ozkan]

It appears that Connection and it's fields need to be accessible in order for us to implement things like PeerStore.

For instance, have a look at

rust-libp2p/protocols/request-response/src/lib.rs

Lines 611 to 638 in 85a846a

	fn on_connection_established(
	&mut self,
	ConnectionEstablished {
	peer_id,
	connection_id,
	endpoint,
	other_established,
	..
	}: ConnectionEstablished,
	) {
	let address = match endpoint {
	ConnectedPoint::Dialer { address, .. } => Some(address.clone()),
	ConnectedPoint::Listener { .. } => None,
	};
	self.connected
	.entry(peer_id)
	.or_default()
	.push(Connection::new(connection_id, address));
	if other_established == 0 {
	if let Some(pending) = self.pending_outbound_requests.remove(&peer_id) {
	for request in pending {
	let request = self.try_send_request(&peer_id, request);
	assert!(request.is_none());
	}
	}
	}
	}

[onur-ozkan]

Even if we create libp2p-memory-peerstore now in libp2p tree, because it's another crate, Connection implementation still needs to be accessible.

[thomaseizinger]

It appears that Connection and it's fields need to be accessible in order for us to implement things like PeerStore.

For instance, have a look at

rust-libp2p/protocols/request-response/src/lib.rs

Lines 611 to 638 in 85a846a

	fn on_connection_established(
	&mut self,
	ConnectionEstablished {
	peer_id,
	connection_id,
	endpoint,
	other_established,
	..
	}: ConnectionEstablished,
	) {
	let address = match endpoint {
	ConnectedPoint::Dialer { address, .. } => Some(address.clone()),
	ConnectedPoint::Listener { .. } => None,
	};
	self.connected
	.entry(peer_id)
	.or_default()
	.push(Connection::new(connection_id, address));
	if other_established == 0 {
	if let Some(pending) = self.pending_outbound_requests.remove(&peer_id) {
	for request in pending {
	let request = self.try_send_request(&peer_id, request);
	assert!(request.is_none());
	}
	}
	}
	}

I don't really understand why you need the libp2p-request-response crate?

You can just implement NetworkBehaviour yourself and track the state based on events fed to you in on_swarm_event:

rust-libp2p/swarm/src/behaviour.rs

Line 194 in 85a846a

fn on_swarm_event(&mut self, event: FromSwarm<Self::ConnectionHandler>);

See https://github.com/libp2p/rust-libp2p/blob/master/misc/allow-block-list/src/lib.rs for a really simple implementation (but with a different goal).

[onur-ozkan]

I don't really understand why you need the libp2p-request-response crate?

To include Connection too in PeerStore structure, I guess I will create it's own types then.

You can just implement NetworkBehaviour yourself and track the state based on events fed to you in on_swarm_event

That's what I am already doing

[thomaseizinger]

I don't really understand why you need the libp2p-request-response crate?

To include Connection too in PeerStore structure, I guess I will create it's own types then.

Yeah, just duplicate what is necessary :)
You can't properly reuse that type anyway because it has RequestId in it.

We try to avoid dependencies between top-level crates like this. It is "bad enough" that every crate depends on libp2p-core but we are in the (long) process of fixing this: #3271

[folex]

Because of the existing similar implementations, I decided to clone(or copy if possible) them on return. Let me know if you want to make these functions return references and leave the cloning decision to developers(not every case requires cloning, so cloning them for accessing these values might impact the performance slightly).

I'd prefer it to return a reference, so I can decide whether to clone or not, on the call site.

[onur-ozkan]

I'd prefer it to return a reference, so I can decide whether to clone or not, on the call site.

This PR will be refactored almost completely(to implement PeerStore behaviour) which makes this topic outdated

[dariusc93]

Could you explain the motive behind exposing NetworkBehaviour::connected?

[thomaseizinger]

Closing this in favor of #4103.