Identify protocol is not working?

[echupriyanov]

I was trying to build working kademlia example by splitting it into separate dialer/listener parts, but it still doesn't work.

As it turned out, the issue is lying in the following code:

    let transport = libp2p::identify::PeerIdTransport::new(transport, addr_resolver)
        .and_then({
            let peer_store = peer_store.clone();
            move |id_out, _, remote_addr| {
                let socket = id_out.socket;
                let original_addr = id_out.original_addr;
                id_out.info.map(move |info| {
                    let peer_id = info.info.public_key.into_peer_id();
                    peer_store.peer_or_create(&peer_id).add_addr(original_addr, Duration::from_secs(3600));
                    (socket, remote_addr)
                })
            }
        });

The Future id_out.info never resolves, so no connection is passed upward.

To illustrate this issue I've created the simplified example by modifying echo-dialer/echo-server to include Identify transport. Complete example is located here - https://github.com/emotiq/rust-libp2p-identify-issue

Will try to dig deeper into the library to find out what is going on inside the protocol..

[echupriyanov]

Actually, as it turned out, the issue is not with the Identify protocol, but with Identify transport.
If I don't use identify transport on Listener side and implement Identify protocol responding with Node info, dialer successfully identifies dialed node and is able to proceed with higher level protocol.

But I still can't figure out how to use Identify transport and protocol on both sides.

[echupriyanov]

What is actually happening here, lets name nodes as D (dialer) and L (listener)

1. D dialing L for echo

   1. D opens stream 0 to L for echo and returns future, which will open separate stream to D for identify protocol and lock cache entry for D until reply received.
   2. This code in D:

             id_out.info.map(move |info| {
                 let peer_id = info.info.public_key.into_peer_id();
                 peer_store.peer_or_create(&peer_id).add_addr(original_addr, Duration::from_secs(3600));
                 (socket, remote_addr)
             })

   effectively tries to resolve future opening stream and placing the lock on cache entry.

2. L listening for incoming connections.

   1. L accepts stream 0.
   2. L creates lazy future which lock on cache entry for D and open stream 1 to D for id protocol.
   3. The same code tries to resolve the future placing the lock and opening stream to D
   4. L accepts stream 2, creates another future, tries resolve it once more, blocks on the lock.
   5. D accepts stream, again tries to resolve peer information, but cache is already locked.

This way we end up in mutually locked nodes.

[tomaka]

I've realized this issue before. It can be bypassed by not resolving the info future if you receive a stream for identify.

In polkadot we've moved to making secio mandatory so that we are guaranteed to have a peer ID without any trouble. If you really don't want secio, I think that an alternative would be to write a transport that handles identify on the side, using a different code path than the other protocols.

[echupriyanov]

While, in general, its a good thing to make secio mandatory, I'd like to have an option to use PeerIdTransport over the unencrypted channels.

Looking at #443, maybe it makes sense to implement protocol 3 (public-keys only) directly in IdentifyTransport? And make Node's public key part of IdentifyTransport and PeerIdTransport structs?

[tomaka]

Looking at #443, maybe it makes sense to implement protocol 3 (public-keys only) directly in IdentifyTransport? And make Node's public key part of IdentifyTransport and PeerIdTransport structs?

Yes! That's indeed what I'd be going for.