Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring libp2p-websocket-star to the Transports family! 🌟 #122

Merged
merged 8 commits into from
Nov 27, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions .aegir.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ const Node = require('./test/nodejs-bundle/nodejs-bundle.js')
const PeerInfo = require('peer-info')
const PeerId = require('peer-id')
const pull = require('pull-stream')
const {parallel} = require('async')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use standard require('async/parallel')


const sigServer = require('libp2p-webrtc-star/src/sig-server')
const wsRendezvous = require('libp2p-websocket-star-rendezvous')
let server
let server2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we now have two "servers", let's use qualifying names:

  • wrtcRendezvous
  • wsRendezvous


let node
const rawPeer = require('./test/browser-bundle/peer.json')
Expand All @@ -15,14 +18,28 @@ const before = (done) => {
let count = 0
const ready = () => ++count === 2 ? done() : null

sigServer.start({ port: 15555 }, (err, _server) => {
sigServer.start({
port: 15555
}, (err, _server) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary style change.

if (err) {
throw err
}
server = _server
ready()
})

wsRendezvous.start({
port: 14444,
refreshPeerListIntervalMS: 1000,
strictMultiaddr: false
}, (err, _server) => {
if (err) {
throw err
}
server2 = _server
ready()
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkg20001 mind putting the two tasks, sigServer.start and wsRendezvous.start in a async.parallel flow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are already parallel...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, to make it more structured with Parallel. I also see Ready being called 3 times and only checked for 2


PeerId.createFromJSON(rawPeer, (err, peerId) => {
if (err) {
return done(err)
Expand All @@ -38,12 +55,7 @@ const before = (done) => {
}

const after = (done) => {
setTimeout(() => node.stop((err) => {
if (err) {
return done(err)
}
server.stop(done)
}), 2000)
setTimeout(() => parallel([node, server, server2].map(s => cb => s.stop(cb)), done), 2000)
}

module.exports = {
Expand All @@ -52,4 +64,3 @@ module.exports = {
post: after
}
}

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@
"libp2p-tcp": "~0.11.1",
"libp2p-webrtc-star": "~0.13.2",
"libp2p-websockets": "~0.10.4",
"libp2p-websocket-star": "~0.5.0",
"libp2p-websocket-star-rendezvous": "~0.2.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for keeping the ~ for <1.0.0 :)

"lodash.times": "^4.3.2",
"pre-commit": "^1.2.2",
"pull-goodbye": "0.0.2",
Expand Down
9 changes: 8 additions & 1 deletion test/browser-bundle/browser-bundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const WS = require('libp2p-websockets')
const WebRTCStar = require('libp2p-webrtc-star')
const WebSocketStar = require('libp2p-websocket-star')
const spdy = require('libp2p-spdy')
const multiplex = require('libp2p-multiplex')
const secio = require('libp2p-secio')
Expand Down Expand Up @@ -36,11 +37,13 @@ class Node extends libp2p {
constructor (peerInfo, peerBook, options) {
options = options || {}
const webRTCStar = new WebRTCStar()
const wsStar = new WebSocketStar({ id: peerInfo.id })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it still require that the PeerInfo gets passed on the constructor? Why?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the README of ws-star it says: const ws = new WSStar({ id: id }) // the id is required for the crypto challenge

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this requierment to have the { id: peerInfo.id } makes it impossible to configure the WebSocketStar through the IPFS node constructor like:

libp2p: { // add custom modules to the libp2p stack of your node
    modules: {}
  }

because the peer id is not available at that point

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How else can the transport get the id?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first question should be "why does this Id exist in the first place", if:

  • a) is to auth with the rendezvous point, the reality is that the rendezvous point doesn't have any list of pre-shared ids, so using the PeerId or something else at random is the same
  • b) is to auth with the other peers, then that is a double crypto challenge because secio already does that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two things here:

  • a) Authentication between peers (end to end) - assured by SECIO ✔️
  • b) Authentication with the rendezvous point - what you are trying to achieve with the PeerId

b) is a bit more complicated because you want to peers to prove who they are with the rendezvous point.

I'm torn that one the best solution is through PeerId because that really breaks the transport interface expectation. We also want to move away from centralized rendezvous points and just make any node be able to do that, which kind of means that a transport will need access to libp2p itself. Once we end up doing this, it might lead to changing how modules are loaded to a DI scheme.

Copy link
Member Author

@mkg20001 mkg20001 Nov 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are now 5 possible solutions:

  • Use wsstar.lazySetId() somewhere later in the code
  • Don't add the peerId and run the server with --disableCryptoChallenge (insecure!)
  • libp2p-websocket-star #117 (comment) (or similar)
  • DI scheme as mentioned above (isn't implemented so this isn't a good short-term solution)
  • Something else that works short-term at least

Which one would be the best? (I prefer the first one)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkg20001 what about deploying a first version without the cryptoChallenge so that js-ipfs users can try it out today and we work on the DI scheme (which @pgte and me will start using more of his time towards that) as a separate endeavor so that then we can add the crypto challenge?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diasdavid The peerId IS accesible at the time of transport creation. No need to use the insecure version. https://github.com/ipfs/js-ipfs/compare/master...mkg20001:master.diff

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diasdavid Insecure server running at /dns/ws-star-signal-4.servep2p.com/wss/p2p-websocket-star/


const modules = {
transport: [
new WS(),
webRTCStar
webRTCStar,
wsStar
],
connection: {
muxer: getMuxers(options.muxer),
Expand All @@ -55,6 +58,10 @@ class Node extends libp2p {
modules.discovery.push(webRTCStar.discovery)
}

if (options.wsStar) {
modules.discovery.push(wsStar.discovery)
}

if (options.bootstrap) {
const r = new Railing(options.bootstrap)
modules.discovery.push(r)
Expand Down
121 changes: 121 additions & 0 deletions test/browser-bundle/websocket-star-only.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
/* eslint-env mocha */
'use strict'

const chai = require('chai')
chai.use(require('dirty-chai'))
const expect = chai.expect
const PeerInfo = require('peer-info')
const PeerId = require('peer-id')
const parallel = require('async/parallel')
const pull = require('pull-stream')

const Node = require('./browser-bundle')

describe('libp2p-ipfs-browser (websocket-star only)', () => {
let peer1
let peer2
let node1
let node2

it('create two peerInfo with websocket-star addrs', (done) => {
parallel([
(cb) => PeerId.create({ bits: 1024 }, cb),
(cb) => PeerId.create({ bits: 1024 }, cb)
], (err, ids) => {
expect(err).to.not.exist()

peer1 = new PeerInfo(ids[0])
const ma1 = '/ip4/127.0.0.1/tcp/14444/ws/p2p-websocket-star/'
peer1.multiaddrs.add(ma1)

peer2 = new PeerInfo(ids[1])
const ma2 = '/ip4/127.0.0.1/tcp/14444/ws/p2p-websocket-star/'
peer2.multiaddrs.add(ma2)

done()
})
})

it('create two libp2p nodes with those peers', (done) => {
node1 = new Node(peer1, null, { wsStar: true })
node2 = new Node(peer2, null, { wsStar: true })
done()
})

it('listen on the two libp2p nodes', (done) => {
parallel([
(cb) => node1.start(cb),
(cb) => node2.start(cb)
], done)
})

it('handle a protocol on the first node', () => {
node2.handle('/echo/1.0.0', (protocol, conn) => pull(conn, conn))
})

it('dial from the second node to the first node', (done) => {
node1.dial(peer2, '/echo/1.0.0', (err, conn) => {
expect(err).to.not.exist()
setTimeout(check, 500)

function check () {
const text = 'hello'
const peers1 = node1.peerBook.getAll()
expect(Object.keys(peers1)).to.have.length(1)

const peers2 = node2.peerBook.getAll()
expect(Object.keys(peers2)).to.have.length(1)

pull(
pull.values([Buffer.from(text)]),
conn,
pull.collect((err, data) => {
expect(err).to.not.exist()
expect(data[0].toString()).to.equal(text)
done()
})
)
}
})
})

it('node1 hangUp node2', (done) => {
node1.hangUp(peer2, (err) => {
expect(err).to.not.exist()
setTimeout(check, 500)

function check () {
const peers = node1.peerBook.getAll()
expect(Object.keys(peers)).to.have.length(1)
expect(Object.keys(node1.swarm.muxedConns)).to.have.length(0)
done()
}
})
})

it('create a third node and check that discovery works', (done) => {
let counter = 0

function check () {
if (++counter === 3) {
expect(Object.keys(node1.swarm.muxedConns).length).to.equal(1)
expect(Object.keys(node2.swarm.muxedConns).length).to.equal(1)
done()
}
}

PeerId.create((err, id3) => {
expect(err).to.not.exist()

const peer3 = new PeerInfo(id3)
const ma3 = '/ip4/127.0.0.1/tcp/14444/ws/p2p-websocket-star/ipfs/' + id3.toB58String()
peer3.multiaddrs.add(ma3)

node1.on('peer:discovery', (peerInfo) => node1.dial(peerInfo, check))
node2.on('peer:discovery', (peerInfo) => node2.dial(peerInfo, check))

const node3 = new Node(peer3, null, { wsStar: true })
node3.start(check)
})
})
})
1 change: 1 addition & 0 deletions test/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ const w = require('webrtcsupport')
require('./base')
require('./browser-bundle/websockets-only')
if (w.support) { require('./browser-bundle/webrtc-star-only') }
require('./browser-bundle/websocket-star-only')
1 change: 1 addition & 0 deletions test/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ require('./base')
require('./nodejs-bundle/tcp')
require('./nodejs-bundle/tcp+websockets')
require('./nodejs-bundle/tcp+websockets+webrtc-star')
require('./nodejs-bundle/tcp+websockets+websocket-star')
require('./nodejs-bundle/stream-muxing')
require('./nodejs-bundle/discovery')
require('./nodejs-bundle/peer-routing')
Expand Down
Loading