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

feat: Circuit Relay #1063

Merged
merged 27 commits into from Mar 16, 2018

Conversation

Projects
None yet
7 participants
@dryajov
Member

dryajov commented Nov 8, 2017

Remaining tasks:

  • Fix Node.js tests
  • update ipfsd-ctl to support spawning js and go nodes
  • add ability to ipfsd-ctl to spawn remote js and go nodes to enable browser testing
  • Add a test for Relay On without having any transport attached. Caught a big hairy bug here #1072 (comment)
  • Complete interop tests in ipfs/interop#6
  • Add documentation on how to enable Relay

@wafflebot wafflebot bot added the in progress label Nov 8, 2017

@daviddias daviddias changed the title from fix: failing circuit interop tests (wip) to Complete the Circuit Relay Endeavour Nov 8, 2017

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 9, 2017

  • Consolidate the 3 ways in which a daemon can be spawned

I did a review of all the different ways we have to spawn go and js daemons as well as past conversations on IRC

[2017-09-04 08:38:04] <daviddias> dryajov: the issue on ipfsd-ctl suggests that we were already unhappy with the fragmentation. Why not evolve ipfsd-ctl to support spawning js-ipfs nodes as well?

and it seems like, what we're looking for is actually extending the ipfsd-ctl daemon to spawn js nodes as well?

  • Create browser interop tests

It seems like the best course of action to get tests in browsers across the board is to port the code from http-api/test/ipfs-factory which allows spawning nodes remotely (using a hapi backend).

Given the above notes, I believe the tasks should be expanded to:

  • Do the required modifications to ipfsd-ctl to support spawning both go and js nodes
  • Extract and generalize the code under http-api/test/ipfs-factory. This will allow us to run our tests seamlessly in several environments - node, browser (electron?), etc, as well as reusing them across other projects.

@diasdavid @VictorBjelkholm

@daviddias

This comment has been minimized.

Member

daviddias commented Nov 9, 2017

upgrading ipfsd-ctl

Yes! Let's do that

port the code from http-api/test/ipfs-factory

Learn how https://github.com/ipfs/js-ipfs-api/tree/master/test/ipfs-factory is doing it. Ideally this should be all ipfd-ctl

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 9, 2017

@diasdavid woot! Awesome, I'll do that.

@richardschneider

This comment has been minimized.

Contributor

richardschneider commented Nov 9, 2017

@dryajov I was just looking at ipfs-ctl. As usual, it does not run on windows. I'll be happy to get it running on windows, once you do your consolidation.

@richardschneider richardschneider referenced this pull request Nov 10, 2017

Closed

Windows Support #1017

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 10, 2017

@richardschneider that would be great!

@richardschneider

This comment has been minimized.

Contributor

richardschneider commented Nov 10, 2017

Raised windows interop issue.

@daviddias

This comment has been minimized.

Member

daviddias commented Nov 10, 2017

@richardschneider ipfsd-ctl does run on Windows, in fact it was one of the first modules to get support :) -- https://ci.appveyor.com/project/diasdavid/js-ipfsd-ctl-a9ywu -- thanks to @thisconnect :)

@richardschneider

This comment has been minimized.

Contributor

richardschneider commented Nov 10, 2017

@diasdavid as usually you are correct. I'll close the issue.

@daviddias

This comment has been minimized.

Member

daviddias commented Nov 11, 2017

Compatability table

test goD relay jsD relay jsB relay
goD⚡️goD
goD⚡️jsD
goD⚡️jsB
jsD⚡️goD
jsD⚡️jsD
jsD⚡️jsB
jsB⚡️goD
jsB⚡️jsD
jsB⚡️jsB
@daviddias

This comment has been minimized.

Member

daviddias commented Nov 11, 2017

@dryajov brought back the compatibility table and added a column for when the browser acts as a Relay #1063 (comment)

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 11, 2017

@diasdavid thanks!

@daviddias

This comment has been minimized.

Member

daviddias commented Nov 12, 2017

EDIT: moved this tasks to the first comment (dryajov)

- [ ] Add a test for Relay On without having any transport attached. Caught a big hairy bug here #1072 (comment)
- [ ] Add documentation on how to enable Relay

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 13, 2017

Add a test for Relay On without having any transport attached. Caught a big hairy bug here #1072 (comment)

@diasdavid, I believe all we vant to do is to set the dafault flag for EXPERIMENTAL.Swarm.DisableRelay to true, I see that you changed the options and now they don't match Go's -

relay: {
enabled: !get(config, 'EXPERIMENTAL.Swarm.DisableRelay', false),
hop: {
enabled: get(config, 'EXPERIMENTAL.Swarm.EnableRelayHop', false),
active: get(config, 'EXPERIMENTAL.Swarm.RelayHopActive', false)
}
}

I also think that in addition to this, there should be a check to skip circuit if swarm doesn't have any transports registered (that is skip it in swarm proper).

@daviddias

This comment has been minimized.

Member

daviddias commented Nov 13, 2017

@dryajov EXPERIMENTAL flags are not the same as things in the CONFIG file. If you want to have that information in both places that is fine but for now, js-ipfs won't have Relay enabled by default until it is properly tested (i.e this PR finished)

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 13, 2017

@diasdavid I agree, we should disable circuit for now (i.e. enabled: false). But I don't think we should change the config flags, since they stop matching Go's at that point.

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 13, 2017

I think I know what you're saying now - yes, we should keep the config flags and the EXPERIMENTAL flags the same and also disable relay by default with enabled: !get(config, 'EXPERIMENTAL.Swarm.DisableRelay', true),.

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 18, 2017

@diasdavid this should address dialing when no transports are registered - libp2p/js-libp2p-switch#236

@daviddias

This comment has been minimized.

Member

daviddias commented Nov 24, 2017

@dryajov I know you are working on upgrading ipfsd-ctl, just wanted to suggest that it would be good to fix the issues to make the Circuit Relay tests in test/core/circuit-relay.spec.js pass first and the same goes for the Interop tests that are already there. The ipfsd-ctl upgrade is just needed for some of the tests.

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 24, 2017

@diasdavid I agree, ipfsd-ctl will mostly help covering the browser based cases. I'll fix the tests we currently have.

@daviddias

This comment has been minimized.

Member

daviddias commented Nov 29, 2017

Hi @dryajov, mind rebasing master onto this branch. CI should be green (at least Travis and Circle)

.aegir.js Outdated
@@ -18,7 +18,8 @@ function start (done) {
(cb) => js([`${base}/10012`, `${base}/20012/ws`], true, 31012, 32012, cb),
(cb) => js([`${base}/10013`, `${base}/20013/ws`], true, 31013, 32013, cb),
(cb) => js([`${base}/10014`, `${base}/20014/ws`], true, 31014, 32014, cb),
(cb) => js([`${base}/10015`, `${base}/20015/ws`], true, 31015, 32015, cb)
(cb) => js([`${base}/10015`, `${base}/20015/ws`], true, 31015, 32015, cb),
(cb) => go([`${base}/10027`, `${base}/20027/ws`], true, 33027, 44027, cb), // we need this for circuit for now

This comment has been minimized.

@daviddias

daviddias Nov 29, 2017

Member

Not true, you should move the interop test to the interop suite

This comment has been minimized.

@dryajov

dryajov Nov 29, 2017

Member

Interop tests don't run in browsers right now, I thought we wanted some browser coverage?

// TODO: 1) figure out why this test hangs randomly
// TODO: 2) move this test to the interop batch
it.skip('node1 <-> goRelay <-> node2', (done) => {
it('node1 <-> goRelay <-> node2', (done) => {

This comment has been minimized.

@daviddias

daviddias Nov 29, 2017

Member

Please move this test to the interop suite. That was what was in the TODO

This comment has been minimized.

@dryajov

dryajov Nov 29, 2017

Member

I can move those to the interop tests, but we'll lose any sort of browser coverage until ipfsd-ctl is ready.

], done)
})
})
it('node1 <-> jsRelay <-> node2', (done) => {
it.skip('node1 <-> jsRelay <-> node2', (done) => {

This comment has been minimized.

@daviddias

daviddias Nov 29, 2017

Member

Why skip?

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 29, 2017

@diasdavid the tests in test/core/circuit-relay.spec.js are already part of the interop suit, the only reason those were added is to get some sort of browser coverage, since interop tests don't run in the browser.

@daviddias

This comment has been minimized.

Member

daviddias commented Nov 30, 2017

since interop tests don't run in the browser.

You can run interop tests in the browser too, it uses aegir today, it follows the same rules

@dryajov

This comment has been minimized.

Member

dryajov commented Nov 30, 2017

@diasdavid that's true, but we would have to rely on prestarted nodes as we do in core, which wasn't the pattern that the rest of the interop tests were following, when initially developed it didn't seem like the right way to go, because of above reason. Now with ipfsd-ctl being close to working in node and browsers we should be able to run this tests everywhere without relying on ad-hoc nodes.

@daviddias

This comment has been minimized.

Member

daviddias commented Mar 16, 2018

I'm going ahead and merge this one. Let's save the fireworks for when ipfs/interop#6 is merged though :) Anyways, great work! :D It took a while but it got done! :)

@daviddias daviddias merged commit f7eaa43 into master Mar 16, 2018

2 of 7 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
Node Security No known vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Mar 16, 2018

@daviddias daviddias deleted the feat/circuit-interop branch Mar 16, 2018

@dryajov

This comment has been minimized.

Member

dryajov commented Mar 16, 2018

Weeeeee!!!!! ❤️

@ya7ya ya7ya referenced this pull request Apr 4, 2018

Merged

activating ipfs relay #182

@nezzard

This comment has been minimized.

nezzard commented Apr 14, 2018

Why i get "Circuit not enabled" after ipfs.swarm.connect ?
repo: '.repo' + Math.random(), start: true, EXPERIMENTAL: { pubsub: true, relay: { enabled: true, hop: { enabled: true } } }

@norzak norzak referenced this pull request Apr 15, 2018

Closed

WebTorrent DHT integration #1152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment