search

libp2p / js-peer-info

libp2p Peer abstraction Node.js implementation
https://libp2p.io
MIT License
37 stars 28 forks source link

Added distinctMultiaddr method #29

Closed SidHarder closed 7 years ago

SidHarder commented 7 years ago

This method can be called from libp2p-swarm/lib/transport.js just before the createListeners method is called and may be a solution to issue https://github.com/ipfs/js-ipfs/issues/228. This method returns an array of multiaddr which are distinct by port. Transport is not considered, if there are two transports with the same port the first multiaddr will be returned.

Question:

  1. Does this seem like a resonable solution?
  2. Should both port and transport be used to determine what is distinct?
coveralls commented 7 years ago

Coverage Status

Coverage decreased (-4.5%) to 93.846% when pulling 0c40408aed91eb41f282e8d6eab8634128136b32 on SidHarder:master into 465ffab78ed2f6731bdb7b62faa94600a3a201af on libp2p:master.

daviddias commented 7 years ago

Almost! :) Unfortunately (or fortunately, I still don't know if there is a valid reason), TCP and UDP ports are different and some kernels also offer SCTP, which I'm not 100% sure if they share the same port space as UDP, that being said, making it unique by port would filter out valid addresses.

You also might want to use multiple network interfaces with the same port

SidHarder commented 7 years ago

I added to the transport object in js-multiaddr so that I could filter on it here.

If you have this: "Addresses": { "Swarm": [ "/ip4/0.0.0.0/tcp/4001", "/ip6/::/tcp/4001", "/ip4/0.0.0.0/udp/4001", "/ip6/::/udp/4001" ], "API": "/ip4/127.0.0.1/tcp/5001", "Gateway": "/ip4/127.0.0.1/tcp/8080" },

The filter will return this: "/ip4/0.0.0.0/tcp/4001" "/ip4/0.0.0.0/udp/4001"

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-4.5%) to 93.846% when pulling e1bc4a45e5acb2fbe20c851f405a8e0122f3f26e on SidHarder:master into 465ffab78ed2f6731bdb7b62faa94600a3a201af on libp2p:master.

daviddias commented 7 years ago

Looking good! Can you also add tests?

SidHarder commented 7 years ago

Yes, I will work on adding some tests.

SidHarder commented 7 years ago

diasdavid: There are two test that are failing for the repo now which I don't believe has anything to do with what I have been working on. After looking at the 2 tests I don't see how the test would ever pass. The 2 test are:

1) add multiaddr that are buffers 2) peer-info add multiaddr that are buffers:

Do you have any insight on this?

daviddias commented 7 years ago

CI looks fine to me, with the exception that SauceLabs is being wonky. where do you see those tests failing specifically @SidHarder ?

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 98.462% when pulling c6121ce0ec92ff023278dfbcdad6fe9f4707df10 on SidHarder:master into 64c1dc0e4a337eb7a34e9f8b949e5e550e917eee on libp2p:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 98.462% when pulling c6121ce0ec92ff023278dfbcdad6fe9f4707df10 on SidHarder:master into 64c1dc0e4a337eb7a34e9f8b949e5e550e917eee on libp2p:master.