Skip to content

Commit

Permalink
tests: testing multiaddress trim leaves relevant addresses untouched
Browse files Browse the repository at this point in the history
  • Loading branch information
pgte committed Dec 1, 2017
1 parent b7698d7 commit 5936229
Showing 1 changed file with 14 additions and 3 deletions.
17 changes: 14 additions & 3 deletions test/multiaddr-trim.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,25 @@ const createNode = require('./utils/node').createNode

describe('multiaddr trim', () => {
let node

after((done) => {
if (node) {
node.stop(done)
} else {
done()
}
})

it('can create a test node with an irrelevant multiaddr', (done) => {
createNode(
[
'/ip4/0.0.0.0/tcp/0/p2p-webrtc-direct'
'/ip4/0.0.0.0/tcp/0/wss/p2p-webrtc-direct',
'/ip4/0.0.0.0/tcp/0'
],
(err, _node) => {
expect(err).to.not.exist()
node = _node
expect(node.peerInfo.multiaddrs.toArray()).to.have.length(1)
expect(node.peerInfo.multiaddrs.toArray()).to.have.length(2)
done()
})
})
Expand All @@ -26,7 +36,8 @@ describe('multiaddr trim', () => {
})

it('irrelevant multiaddr got trimmed', (done) => {
expect(node.peerInfo.multiaddrs.toArray()).to.have.length(0)
expect(node.peerInfo.multiaddrs.toArray()).to.have.length(1)
expect(node.peerInfo.multiaddrs.toArray()[0].toString()).to.match(/^\/ip4\/0.0.0.0\/tcp\/0\/ipfs\/\w+/)

This comment has been minimized.

Copy link
@daviddias

daviddias Dec 1, 2017

Member

(my regex foo might be rusty) isn't this checking that 0.0.0.0 still continues there?

This comment has been minimized.

Copy link
@pgte

pgte Dec 1, 2017

Author Contributor

Forgot to escape. Fixed by ee65d2f

Yes, the TCP one remaining is something like
/ip4/0.0.0.0/tcp/0/ipfs/QmSNLvtwzoEbKhwAfXgeNYX9CPBZyKCZq9GauNVsgoHjyp.

This comment has been minimized.

Copy link
@pgte

pgte Dec 1, 2017

Author Contributor

Is this wrong? (the test is simply making sure the webrtc thing was taken out of the final set).

This comment has been minimized.

Copy link
@daviddias

daviddias Dec 1, 2017

Member

It is indeed. Because we what 0.0.0.0 means is for the transport to listen in all interfaces (which happens) and 0 means listen in a random port (which also happens). Once the Listen is finished, the multiaddrs array will have the new multiaddrs. So it should check that those 0.0.0.0 are not there anymore.

This comment has been minimized.

Copy link
@pgte

pgte Dec 1, 2017

Author Contributor

OK, but AFAIK, that replacement logic hasn't been changed by this PR, so something may have already been wrong here.. Will dig further..

This comment has been minimized.

Copy link
@pgte

pgte Dec 1, 2017

Author Contributor

@diasdavid I may have spotted the problem, and it's related to the MultiaddrSet#distinct() method in peer-id.

In our case, both multi addresses /ip4/0.0.0.0/tcp/0/wss/p2p-webrtc-direct and /ip4/0.0.0.0/tcp/0 are mistankenly conflated because they share the same port and transport.

Fix:
I think that, in this case, 0.0.0.0 and the port 0 should be considered wildcards in peer-info, but these values are too transport-specific to be hard-coded into peer-info, wouldn't you agree?

This comment has been minimized.

Copy link
@daviddias

daviddias Dec 1, 2017

Member

Interesting. I didn't know we were using /ip4/0.0.0.0/tcp/0/wss/p2p-webrtc-direct anywhere though. That said. After a .listen, 0.0.0.0 should not exist anymore on the peerInfo.multiaddrs

This comment has been minimized.

Copy link
@pgte

pgte Dec 1, 2017

Author Contributor

It's just a case where these are different MAs that are mistaken for being the same because of sharing the transport and port..

The problem derived from the previous one is that, in libp2p-swarm, on this line, the first address (for which there is no transport) is selected over the TCP one, and then, it's not diable on the TCP transport, rendering an empty set as a result on that line.

This leads to that TCP address not being listened to.

@diasdavid what do you think should be the fix?

This comment has been minimized.

Copy link
@pgte

pgte Dec 1, 2017

Author Contributor

Summarized issue here: libp2p/js-peer-info#60

done()
})
})

0 comments on commit 5936229

Please sign in to comment.