Only log addresses that are actually being listened on

[dignifiedquire]

In https://github.com/ipfs/js-ipfs/blob/master/src/core/ipfs/libp2p.js#L16-L18 it prints all addresses that are passed in but sometimes those listens fail or do not happen at all (e.g. in the browser).

[daviddias]

👍 , note however, that adding websockets, tcp or any multiaddrs to the browser PeerInfo is not recommended (at all), because those multiaddrs will be propagate and no other peer will be able to dial to the browser on those multiaddrs.

[daviddias]

@dignifiedquire wanna handle this one?

[dignifiedquire]

@diasdavid sure

[dignifiedquire]

So this is more tricky than I thought, I would suggest changing libp2p-swarm in a way that it drops all multiaddrs in swarm.peerInfo.multiaddrs that are not listened on and as such the log statement in js-ipfs doesn't need to be changed. Any better ideas @diasdavid ?

[daviddias]

We can't drop the multiaddrs that we weren't able to listen into, since these multiaddrs might be valid only under certain networks, so the fact that I'm not listening in an addr now, might just mean that on the wifi network that I'm right now, that multiaddr doesn't work, but I might change networks (without closing my daemon) and then I want to listen on that multiaddr.

We need to upgrade peer-info to enumerate the active addresses. This will also be useful for knowing which multiaddr we used to connect to a certain peer.

[victorb]

would it make sense for the daemon to listen for network changes and try to
start listen to interfaces after start and print if it manages to connect
to a new interface?

On Mon, Sep 26, 2016 at 12:33 PM, David Dias notifications@github.com
wrote:

We can't drop the multiaddrs that we weren't able to listen into, since
these multiaddrs might be valid only under certain networks, so the fact
that I'm not listening in an addr now, might just mean that on the wifi
network that I'm right now, that multiaddr doesn't work, but I might change
networks (without closing my daemon) and then I want to listen on that
multiaddr.

We need to upgrade peer-info to enumerate the active addresses. This will
also be useful for knowing which multiaddr we used to connect to a certain
peer.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#213 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAcD9Ficn7Bpl8KXVk9qrG8RtoGZzE9Hks5qt5-MgaJpZM4IcUp8
.

[daviddias]

Hey @dignifiedquire mind adding a test case for this showing the failure?

[dignifiedquire]

@diasdavid not sure how to exactly, and I don't think that I will have time for this soon.

[daviddias]

@dryajov does this sound like something you could handle?

[dryajov]

@diasdavid sure thing, I'll take a look!

[dryajov]

I did some initial digging and this is what I've found.

There is currently no way of telling if an address succeed listening or not outside of swarm. Tho addresses do get updated if listening succeed with whatever the getAddrs method from the transport returns - https://github.com/libp2p/js-libp2p-swarm/blob/f2df5c1e394052dd31ce91b0be215a3895991646/src/transport.js#L94-L96, they get added along side those that didn't, so there is no way of telling if a particular address in the peerinfo succeeded listening or not. I like the approach of modifying PeerInfo and I think we can add another field (activeMultiaddrs) that would store the active addresses. If the purpose is display only, that should work pretty well and would require minimal changes.

To @victorbjelkholm point, do we perform any sort of active reconnect on network changes? @diasdavid
I digged around a bit but haven't seen anything yet - I'll keep looking. However, if we're currently not handling reconnect, I would like to address that as well.

One more thing I found is that 0.0.0.0 addresses are not being correctly reported by any other transport but TCP, I think this needs fixing since its confusing to the user (I know, it has bit me a couple of times 😛 )

If there are no objections, I'll start making the changes to PeerInfo.

I'll create issues for the other two things I identified.

@diasdavid @victorbjelkholm @dignifiedquire

[daviddias]

One more thing I found is that 0.0.0.0 addresses are not being correctly reported by any other transport but TCP, I think this needs fixing since its confusing to the user (I know, it has bit me a couple of times 😛 )

The correct statement is that 0.0.0.0 should be also supported by Websockets transport. Here is an issue to track that: libp2p/js-libp2p-websockets#12

webrtc-star and websocket-star won't use 0.0.0.0 at all. 0.0.0.0 means listen in all interfaces

I like the approach of modifying PeerInfo and I think we can add another field (activeMultiaddrs) that would store the active addresses. If the purpose is display only, that should work pretty well and would require minimal changes.

The current "activeMultiaddrs" is, in reality, the "peer.multiaddrs" as these get updated when on every listener that is created (i.e 0.0.0.0 gets expanded to the interfaces). They just do not get removed when a listen call fails and I believe that is where the bug needs to be fixed.

[dryajov]

The reason I suggested having a separate field for active connections, is because of this statement:

We can't drop the multiaddrs that we weren't able to listen into, since these multiaddrs might be valid only under certain networks, so the fact that I'm not listening in an addr now, might just mean that on the wifi network that I'm right now, that multiaddr doesn't work, but I might change networks (without closing my daemon) and then I want to listen on that multiaddr.
#213 (comment)

Would this still be an issue if we remove addresses from peer.multiaddrs?

How about the point on reconnecting on network changes, what do you think about that - is it something we already do? If not, do you see any value on handling those cases?