feat(swarm): allow NetworkBehaviour to report remote address

[mxinden]

Description

Allow a NetworkBehaviour to report the external address of a remote peer to other NetworkBehaviours.

Possible approach (though up for discussion):

• Extend ToSwarm with ToSwarm::NewExternalAddrOfPeer.
• Have libp2p-identify emit the new event.
• Extend FromSwarm with FromSwarm::NewExternalAddrOfPeer.
• Have libp2p-kad consume the new event.

Motivation

• Allows libp2p-identify to e.g. tell libp2p-kad of an external address of a remote peer and thus allows libp2p-kad to track that address in its Kademlia routing table.

Are you planning to do it yourself in a pull request?

No

[dariusc93]

I assume this will not just be limited to just libp2p-identify emitting the event but any behaviour that has obtain a peer remote address?

[mxinden]

Yes, any NetworkBehaviour, not constrained to libp2p-identify.

[thomaseizinger]

I assume this will not just be limited to just libp2p-identify emitting the event but any behaviour that has obtain a peer remote address?

Yes although there aren't that many which can discover "new" addresses. I'd say it is unnecessary to emit an event for an address we just connected to for example.

Behaviours I can think of that can discover new addresses are:

• identify
• kademlia
• mDNS
• rendezvous

[StemCll]

Taking a look into this

[mxinden]

(1) libp2p-request-response consuming FromSwarm::NewExternalAddrOfPeer

Another consumer of the new FromSwarm::NewExternalAddrOfPeer can be libp2p-request-response.

This could feed into its addresses cache:

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

Lines 336 to 337 in c6984dd

	/// Externally managed addresses via `add_address` and `remove_address`.
	addresses: HashMap<PeerId, SmallVec<[Multiaddr; 6]>>,

Though we would need some garbage collection mechanism along the lines of libp2p-identify:

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

Lines 53 to 54 in c6984dd

	/// The addresses of all peers that we have discovered.
	discovered_peers: PeerCache,

(2) Generic Swarm::add_external_addr_of_peer triggering FromSwarm::NewExternalAddrOfPeer

If we go one step further and add Swarm::add_external_addr_of_peer we could deprecate the protocol specific methods in libp2p-request-response and libp2p-kad:

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

Lines 443 to 450 in c6984dd

	/// Adds a known address for a peer that can be used for
	/// dialing attempts by the `Swarm`, i.e. is returned
	/// by [`NetworkBehaviour::handle_pending_outbound_connection`].
	///
	/// Addresses added in this way are only removed by `remove_address`.
	pub fn add_address(&mut self, peer: &PeerId, address: Multiaddr) {
	self.addresses.entry(*peer).or_default().push(address);
	}

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

Lines 510 to 527 in c6984dd

	/// Adds a known listen address of a peer participating in the DHT to the
	/// routing table.
	///
	/// Explicitly adding addresses of peers serves two purposes:
	///
	/// 1. In order for a node to join the DHT, it must know about at least
	/// one other node of the DHT.
	///
	/// 2. When a remote peer initiates a connection and that peer is not
	/// yet in the routing table, the `Kademlia` behaviour must be
	/// informed of an address on which that peer is listening for
	/// connections before it can be added to the routing table
	/// from where it can subsequently be discovered by all peers
	/// in the DHT.
	///
	/// If the routing table has been updated as a result of this operation,
	/// a [`KademliaEvent::RoutingUpdated`] event is emitted.
	pub fn add_address(&mut self, peer: &PeerId, address: Multiaddr) -> RoutingUpdate {

---

@thomaseizinger @StemCll thoughts on (1) and (2)?

@StemCll I suggest not doing (1) or (2) as part of #4371 to keep the pull request's scope small.

[thomaseizinger]

Instead of (1), I thought we wanted to do #4103?

[mxinden]

In my eyes no protocol should depend on #4103. In other words, all protocols should function without #4103. I don't think we should move away from the current convention where every protocol keeps its own cache of peer information.

In my eyes #4103 is mostly for users to interact with. E.g. retrieving the currently connected nodes, their addresses, ...

[thomaseizinger]

In my eyes no protocol should depend on #4103. In other words, all protocols should function without #4103. I don't think we should move away from the current convention where every protocol keeps its own cache of peer information.

In my eyes #4103 is mostly for users to interact with. E.g. retrieving the currently connected nodes, their addresses, ...

Agreed in principle.

At the moment, libp2p-request-response has APIs that IMO belong onto #4103, like add_address.

I am not against adding a cache of discovered addresses although that might be better implemented on a Swarm level as an implementation detail without an external API so that not every protocol has to re-implement it.

Once we have #4103, I'd propose to remove the add_address API on libp2p-request-response.

Protocols should be able to perform their primary function in isolation but for me, that excludes keeping an address book. I can use libp2p-request-response just fine if I've made a dial with addresses to the peer already. Thus, I don't think that is conflicting and from that PoV, I don't see why libp2p-request-response should be a consumer of this new event.

[thomaseizinger]

I've commented with a different idea on #4371: Instead of removing the add_address API on libp2p-request-response as a result of #4103, we probably should instead add a function to Swarm that allows users to trigger this event. That is how we've previously solved this. Behaviours like kademlia & request-response can then have an internal state where they respond to the event and (temporarily) store the address.

[thomaseizinger]

I've commented with a different idea on #4371: Instead of removing the add_address API on libp2p-request-response as a result of #4103, we probably should instead add a function to Swarm that allows users to trigger this event. That is how we've previously solved this. Behaviours like kademlia & request-response can then have an internal state where they respond to the event and (temporarily) store the address.

I just realised that this is exactly (2) from above. Sorry @mxinden, I must have not read the above comment properly 😅

Or as they say: Great minds think alike 😇

[thomaseizinger]

Instead of sharing addresses, should we be thinking about sharing peer records?

[mxinden]

Given that we don't support it in libp2p-identity yet, would you construct the peer record locally? If so, what would you set on PeerRecord::envelope?

rust-libp2p/core/src/peer_record.rs

Lines 18 to 27 in 7d1d67c

	pub struct PeerRecord {
	peer_id: PeerId,
	seq: u64,
	addresses: Vec<Multiaddr>,
	/// A signed envelope representing this [`PeerRecord`].
	///
	/// If this [`PeerRecord`] was constructed from a [`SignedEnvelope`], this is the original instance.
	envelope: SignedEnvelope,
	}

Long term, yes, I think this mechanism should use peer records. Short term, to not block it on the role out of peer records, I suggest using addresses only.

[thomaseizinger]

You can't construct it locally because it needs to be signed so yeah it would have to come from libp2p-identify but that is a trivial addition compared to this feature.

I thought of it because we will need a mechanism for sharing peer records for anything that is like libp2p/specs#587 and it is kind of unfortunate to design a new API that we already know will be obsoleted.

[thomaseizinger]

For reference, here is the ticket: #4017

[mxinden]

You can't construct it locally because it needs to be signed so yeah it would have to come from libp2p-identify but that is a trivial addition compared to this feature.

libp2p-identify is not the only protocol that can discover addresses of remote peers. E.g. libp2p-kad can do so as well, e.g. on Kademlia::find_closest. Unfortunately the libp2p Kademlia specification only supports plain addresses today. See https://github.com/libp2p/specs/blob/master/kad-dht/README.md.

and it is kind of unfortunate to design a new API that we already know will be obsoleted.

With the above in mind, I don't think the new API will be obsolete any time soon. I don't see the large libp2p DHT deployments (e.g. IPFS) move to signed peer records any time soon.

[thomaseizinger]

Yeah that is fair! I guess once we have non-exhaustive, we can easily add more variants :)