Mismatch with the usage of fully and non-fully qualified multiaddr (/p2p suffix)

[stormshield-frb]

Summary

There seems to be no uniformization with the use of fully-qualified multiaddr (ie. with p2p suffix). This leads to unfortunate side-effects.

For example, like it was mentionned in #2280, multiaddr are stored fully-qualified in kad behaviour, but they are stored not fully-qualified in identify behaviour.

The identify case

In identify, the cache is constructed with addresses received from the listen_addrs fields which do not contains the /p2p/ part (due to back and forth conversions between MultiAddr and SocketAddr).

However, removal of such addresses occurrs on DialError

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

Lines 389 to 397 in ecdd0ff

	FromSwarm::DialFailure(DialFailure { peer_id, error, .. }) => {
	if let Some(entry) = peer_id.and_then(|id| self.discovered_peers.get_mut(&id)) {
	if let DialError::Transport(errors) = error {
	for (addr, _error) in errors {
	entry.remove(addr);
	}
	}
	}
	}

and on ConnectionEstablished::failed_addresses

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

Lines 218 to 222 in ecdd0ff

	if let Some(entry) = self.discovered_peers.get_mut(&peer_id) {
	for addr in failed_addresses {
	entry.remove(addr);
	}
	}

with fully-qualified multiaddr. This result in addresses never being removed since /ip4/127.0.0.1/tcp/4001/p2p/Qm... does not match /ip4/127.0.0.1/tcp/4001.

The kad case

With the current implementation of the kademlia behaviour, events like ConnectionEstablished and DialError always contain fully-qualified multiaddr.

But the application can also manually add multiaddresses into the k-bucket (with the add_address method) : if we call add_address with a fully-qualified multiaddr it will successfully be removed when a DialError occurrs, but if we call add_address with a non-fully-qualified multiaddr it will never be removed (it might be an expected behaviour for a bootnode for example, we are not sure about it).

Also, non-fully-qualified addresses already present will be duplicated (with /p2p suffix) on a ConnectionEstablished event as a dialer.

Expected behaviour

We would expect to have a coherent behaviour when using multiaddr with or without /p2p/ suffix.

It would be nice if the documentation specify whether we should provide fully or non-fully qualified addresses (and the possible side effect if there is any).

Actual behaviour

Mixed of full and non-full qualified leads to addresses never being removed from the identify cache and the DHT.

Possible Solution

The identify case

We have two possible solutions to this problem :

1. ensure that only fully-qualified multiaddr are inserted into the cache
2. keep non-fully-qualified multiaddr in the cache but remove them accordingly

The kad case

Ensure that we always insert fully-qualified multiaddr when calling add_address.

Version

• libp2p version : 0.52.3

Would you like to work on fixing this bug?

Yes. We already started to work to address this issue. You can find a proof of concept in the following commit : d63cd80

This was made as a starting point for a discussion.

[thomaseizinger]

Thank you for reporting this! I do agree that we should do better on this front. Using a non-qualified address is totally legitimate so I am afraid we cannot always enforce them being fully-qualified. I think we should be appending /p2p where possible.

Something we could do is change the PartialEq function of Multiaddr to always parse both addresses and check whether they would be equal except for one of them missing a `/p2p? Code is here: https://github.com/multiformats/rust-multiaddr/blob/d9108c9cfce89cfe936944e499faa5ba2c9eb213/src/lib.rs#L40

Other than that, I think we'll have to change all caches / maps of addresses to always check for both.

A long-shot could be to refactor Multiaddr and create a type-safe way of expressing both variants, i.e. encode in the type-system, whether a Multiaddr has a /p2p suffix or not.

[stormshield-frb]

Thanks for your reply. We totally agree that using non-qualified address is totally legitimate and that we should be able to handle both cases. In that regard, we think that the pragmatic approach is to always make sure that the libp2p API appends /p2p/ when receiving a non-fully-qualified address (our POC goes in that direction: d63cd80).

We are not particularly convinced with the change to PartialEq because we think it might bring a lot of confusion for end users (and maybe some side effects too). Moreover, if we did such a change, we would also have to change the Hash implementation (cf. the identify behaviour stores its addresses in a hashset). The only way we see this change appropriate is if the lip2p specs specifies that fully-qualified and non-fully-qualified addresses are equivalent (which is probably not the case).

Finally, a refactor of Multiaddr with a type-safe system would probably be the safest way to enforce the correct usage of fully-qualified or non-fully-qualified multiaddr (depending on the context) but we probably cannot take in charge a such vast implementation which would require an advanced knowledge of the libp2p specifications (when apply or not apply a fully-qualified or a non-fully-qualified address).

We think that the first solution is the simplest approach and the least likely to have any side effects so we are going to keep working on our POC (always appending /p2p) because this issue really impacts us. Would you be interested to integrate a solution like this for now ?

[thomaseizinger]

We think that the first solution is the simplest approach and the least likely to have any side effects so we are going to keep working on our POC (always appending /p2p) because this issue really impacts us. Would you be interested to integrate a solution like this for now ?

Yes! Feel free to open a draft PR at any time and we can collaborate from there! :)