Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: Circuit Relay #1063

Merged
merged 27 commits into from
Mar 16, 2018
Merged

feat: Circuit Relay #1063

merged 27 commits into from
Mar 16, 2018

Conversation

dryajov
Copy link
Member

@dryajov 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 Figure out why tests are failing #1072 (comment)
  • Complete interop tests in Feat/circuit relay interop#6
  • Add documentation on how to enable Relay

@ghost ghost assigned dryajov Nov 8, 2017
@ghost ghost added the status/in-progress In progress label Nov 8, 2017
@daviddias daviddias changed the title fix: failing circuit interop tests (wip) Complete the Circuit Relay Endeavour Nov 8, 2017
@dryajov
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

dryajov commented Nov 9, 2017

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

@richardschneider
Copy link
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.

@dryajov
Copy link
Member Author

dryajov commented Nov 10, 2017

@richardschneider that would be great!

@richardschneider
Copy link
Contributor

Raised windows interop issue.

@daviddias
Copy link
Member

@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
Copy link
Contributor

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

@daviddias
Copy link
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
Copy link
Member

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

@dryajov
Copy link
Member Author

dryajov commented Nov 11, 2017

@diasdavid thanks!

@daviddias
Copy link
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
Copy link
Member Author

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
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

dryajov commented Nov 18, 2017

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

@daviddias
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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) => {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why skip?

@dryajov
Copy link
Member Author

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
Copy link
Member

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

@daviddias
Copy link
Member

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
@ghost ghost removed the status/in-progress In progress label Mar 16, 2018
@daviddias daviddias deleted the feat/circuit-interop branch March 16, 2018 22:05
@dryajov
Copy link
Member Author

dryajov commented Mar 16, 2018

Weeeeee!!!!! ❤️

@nezzard
Copy link

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 } } }

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/wizard Extensive knowledge (implications, ramifications) required P0 Critical: Tackled by core team ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants