add: p2p suffixes to multiaddrs from identify

[koivunej]

not verified but indirectly used by the conformance tests when dialing a peer with an expected peerid.

Not 100% sure if this should be done, at least long as #105 is done at least to accept the multiaddrs with /p2p/.

[koivunej]

This still hasn't gone too stale but I suspect that this is the first step towards fixing #105 and after this the interface-ipfs-core tests should start hitting the #105 on every run.

[ljedrz]

Yoink! Assuming control of this one 😄.

Since we can now connect using multiaddr/p2p/peerid, this is a desirable change to /id and can be merged now.

[koivunej]

I don't think I can approve this, please do so @ljedrz.

The conformance test failure was:

  1) .swarm.disconnect
       should disconnect from a peer:

      AssertionError: expected [ Array(1) ] to have a length of 0 but got 1
      + expected - actual

      -1
      +0
      
      at Context.<anonymous> (node_modules/interface-ipfs-core/src/swarm/disconnect.js:44:29)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

  2) .swarm.disconnect
       "before each" hook for "should respect timeout option when disconnecting from a remote peer":
     HTTPError: Duplicate connection attempt
      at Object.errorHandler [as handleError] (node_modules/ipfs-http-client/src/lib/core.js:67:15)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)
      at async Client.fetch (node_modules/ipfs-utils/src/http.js:155:9)
      at async Object.connect (node_modules/ipfs-http-client/src/swarm/connect.js:10:17)
      at async Context.<anonymous> (node_modules/interface-ipfs-core/src/swarm/disconnect.js:30:7)

Not yet sure what this is.

Ok so the first one expects that after disconnect the peer disappears immediatedly. I wonder if I missed this change in #303, I did try to add warnings around it when I last wasted time on it.... Nope, the warning is still in place.

The second test should not be enabled at all... Though I do think there should not be that error though.

[ljedrz]

I'll check if I'm seeing those locally.

[ljedrz]

Ok, so this initially simple change has led to some more serious stuff after all. A bit of an explanation first: the conformance suite uses /id to obtain the address of a test node to connect to, and that Multiaddr now also additially contains /p2p/peer_id (AKA Protocol::P2p). However, since we are not dialing by multiaddr/p2p/peer_id (libp2p doesn't support it), until now our connections did not consider the PeerId as part of the Multiaddr, but handled it individually.

Since we have been planning to disallow connecting to a Multiaddr that doesn't contain a /p2p/ suffix (not unlike go-ipfs), the following was done in order to allow the identity change:

• ConnectionTarget is removed
• Ipfs::connect and the associated IpfsEvent::Connect use Multiaddr
• the Multiaddrs returned by Ipfs::{addrs_local, identity} (IpfsEvent::{GetAddresses, Listeners}) contain the Protocol::P2p
• a Ipfs::peer_id method is added
• SwarmApi::{inject_connection_established, inject_connection_closed} are adjusted so that the connection-handling objects register the Multiaddr containing Protocol::P2p
• tests::connect_two::{connect_two_nodes_by_addr_and_peer, connect_duplicate_peer_id} are removed as no longer applicable

Also, as a drive-by, SwarmApi::connect_registry::finish_subscription is now only called if we were the ones to perform the dial (otherwise there is no related subscription).

[ljedrz]

Comments addressed 👌.

[koivunej]

now that this is done here I wonder how should it be reflected in the return type (if at all) -- probably not, since there's no way to go from peerid -> publickey.

If this is moved here I think it should def be taken into account on this method doc, as it might be quite surprising for going in the other direction as rust-libp2p in general by appending the peer id. If we decide to keep this at this level (in ipfs::Ipfs::identity) we should get rid of the http append_p2p impl as well.

My reason for adding this initially only on http level was to keep the "rust stuff" aligned to rust-libp2p and only at http level "be like rest of ipfs."

[koivunej]

so I guess the decision is: do we keep aligning to rust-libp2p or not? I don't think there are many places where libp2p can leak but the decision around this seems unavoidable. ConnectionTarget may had been a path towards unifying and accepting both.

1. (PeerId, Multiaddr) with runtime check that the multiaddr doesnt end in p2p/peer_id
2. Multiaddr with runtime check to make sure it ends in p2p/peer_id

I am not actually sure if it's possible to filter out the end. relay addresses look at the moment like: (/ip4/7.7.7.7/tcp/55555/p2p/QmRelay)?/p2p-circuit/p2p/QmAlice (regexified by me to signal that it might be missing) which means that it is in fact possible to filter regarding the tail since there's the /p2p-circuit/ in between.

[koivunej]

Chatted about this: going ahead with Multiaddr with runtime checks here, upgrading those to MultiaddrWithPeerId and MultiaddrWithoutPeerId in a follow-up to centralize conversions and checks.

[koivunej]

As chatted lets go ahead with this one, transition towards MultiaddrWithPeerId and MultiaddrWithoutPeerId for the different kinds of Multiaddrs used.

[koivunej]

oops forgot:

bors r+

[bors]

Build succeeded:

• ci