Multiaddr duplicated p2p protocol in relay reservation

[stormshield-pj50]

Summary

P2p protocol is duplicated in Multiaddr when a relay, using TCP transport with port reuse enabled, accepts a reservation request. This leads to an invalid Multiaddr.

In order to reproduce the bug, just turn on port reuse when creating TcpConfig in relay_v2 example:

rust-libp2p/protocols/relay/examples/relay_v2.rs

Line 49 in fcc987e

let tcp_transport = TcpConfig::new();

Please find some of the logs dumped on the listening client showing the duplicated P2p protocol:

[2022-06-07T08:38:16Z INFO  client] Relay accepted our reservation request.
[2022-06-07T08:38:16Z INFO  client] Listening on "/ip4/$RELAY_SERVER_IP/tcp/4001/p2p/12D3KooWDpJ7As7BWAwRMfu1VU2WCqNjvq387JEYKDBj4kx6nXTN/p2p/12D3KooWDpJ7As7BWAwRMfu1VU2WCqNjvq387JEYKDBj4kx6nXTN/p2p-circuit/p2p/12D3KooWPjceQrSwdWXPyLLeABRXmuqt69Rg3sBYbU1Nft9HyQ6X"

Expected behavior

The relayed Multiaddr should not contain a duplicated P2p protocol.

Possible Solution

Thought about 3 possibilities:

1. In Identify:

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

   Line 327 in fcc987e

   let observed = self

   when building the observed response in Identify, do not include P2p protocol in the Multiaddr.
2. In transports/tcp:

   rust-libp2p/transports/tcp/src/lib.rs

   Line 438 in fcc987e

   fn address_translation(&self, listen: &Multiaddr, observed: &Multiaddr) -> Option<Multiaddr> {

   the address_translation function should return the same kind of Multiaddr whether port reuse is enabled or not. So maybe patch the function in the match case when port reuse is on, and pop the P2p protocol if present.
3. In relay_v2:

   rust-libp2p/protocols/relay/src/v2/relay.rs

   Line 758 in fcc987e

   .external_addresses()

   don't append P2p protocol if already present.

Also wondering if external and observed addresses need to contain the P2p protocol ?

Version

libp2p version 0.45

Would you like to work on fixing this bug?

Yes

[mxinden]

3. In relay_v2:

rust-libp2p/protocols/relay/src/v2/relay.rs

Line 758 in fcc987e

.external_addresses()

I think this is the best way to tackle the issue. Thanks for debugging. Would you mind proposing a patch?

Also wondering if external and observed addresses need to contain the P2p protocol ?

I haven't put enough thoughts into this, though I think you are right.

[mxinden]

Thanks for preparing #2701.

I have been thinking about this some more. Sorry for not doing so earlier.

The process of a client requesting a reservation with a relay:

1. When accepting the reservation the relay tells the client its external addresses.
2. The client needs to tell everyone else which relays it is listening via, i.e. needs to tell everyone else the addresses of the relay.

We need the /p2p/xxx component with the relay PeerId in the Multiaddr in step (2) in order to secure consecutive connections. Conceptually it does not matter whether the relay adds it to the Multiaddr before sending it in step (1) or the client adds it before step (2).

I wonder why I added the /p2p/xxx protocol before step (1) and not step (2). Off the top of my head the client should do it before (2), among other things to support relays that don't do so before (1).

I still need to research what the Go implementation does. As far as I can tell the Golang client implementation does not require the /p2p/xxx component.

https://github.com/libp2p/go-libp2p/blob/master/p2p/protocol/circuitv2/client/reservation.go#L94-L103

@marten-seemann in the Go implementation, does the relay add its PeerId to its Multiaddr when accepting a reservation, or does the client add the relay's PeerId to the relay's Multiaddr before advertising the final /p2p-circuit address to other peers?

Let me know if the above makes sense @Pj50. Again, sorry for not raising this earlier.

[stormshield-pj50]

@mxinden Don't worry for your post being late, you summed up well the mechanism that adds the P2p protocol during relay reservation. Honestly I don't know which one between adding it before step(1) or before step (2) is the best.

[mxinden]

Turns out, we actually settled on (1) in the past:

the addrs field contains all the public relay addrs, including the peer ID of the relay node but not the trailing p2p-circuit part;

https://github.com/libp2p/specs/blob/master/relay/circuit-v2.md

Sorry for the noise here. Will review #2701 next.

@marten-seemann in the Go implementation, does the relay add its PeerId to its Multiaddr when accepting a reservation,

For the record, yes it does.

[mxinden]

Closing here with #2701 merged.