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

[WIP] feat: implement relay circuit #1

Merged
merged 16 commits into from Mar 27, 2017

Conversation

4 participants
@dryajov
Member

dryajov commented Feb 21, 2017

@RichardLitt

This comment has been minimized.

Show comment
Hide comment
@RichardLitt

RichardLitt Feb 22, 2017

Needs README?

RichardLitt commented Feb 22, 2017

Needs README?

@dryajov

This comment has been minimized.

Show comment
Hide comment
@dryajov

dryajov Feb 22, 2017

Member

@RichardLitt I think it does, and I have a stub in the PR, I belive?

I'm just getting acquainted with the dev cycle/practices, so please feel free to point out whenever something is off :).

As a suggestion, can we get a link to the contrib guidelines in every relevant README? That would make things a lot easier - ignore this, if they are already there, I might have simply missed them.

Also, if you have links/info of things that I should review regarding the above, please do link them here.

Member

dryajov commented Feb 22, 2017

@RichardLitt I think it does, and I have a stub in the PR, I belive?

I'm just getting acquainted with the dev cycle/practices, so please feel free to point out whenever something is off :).

As a suggestion, can we get a link to the contrib guidelines in every relevant README? That would make things a lot easier - ignore this, if they are already there, I might have simply missed them.

Also, if you have links/info of things that I should review regarding the above, please do link them here.

@RichardLitt

This comment has been minimized.

Show comment
Hide comment
@RichardLitt

RichardLitt Feb 27, 2017

@dryajov There is a stub, but I think a bit more would be great. Here is an example libp2p README template: ipfs/community#242.

RichardLitt commented Feb 27, 2017

@dryajov There is a stub, but I think a bit more would be great. Here is an example libp2p README template: ipfs/community#242.

@dryajov dryajov changed the title from Initial repo setup to [WIP] feat: implement relay circuit Mar 2, 2017

@dryajov

This comment has been minimized.

Show comment
Hide comment
@dryajov

dryajov Mar 6, 2017

Member

@diasdavid @dignifiedquire @lgierth

This should be implemented to the point were messages are being passed from the src to the dest peers over the relay. I think this is a good starting point and is ready for some review. I'd like to get as much feedback as possible before continuing, maybe some discussion around whats next is in order. I'll try to make it to the standup call tomorrow, but can't promise anything.

Member

dryajov commented Mar 6, 2017

@diasdavid @dignifiedquire @lgierth

This should be implemented to the point were messages are being passed from the src to the dest peers over the relay. I think this is a good starting point and is ready for some review. I'd like to get as much feedback as possible before continuing, maybe some discussion around whats next is in order. I'll try to make it to the standup call tomorrow, but can't promise anything.

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Mar 6, 2017

Member

@dryajov as we talked over IRC, I'm catching up with all the things, nevertheless, thank you so much for investing time into this.

I need to double check here a couple of things:

  • Has there been any decision on how to enable a Node to become a relay node? Perhaps by having a flag in the config?
  • Is there a test being added libp2p-ipfs-nodejs that shows how all of this got integrated?
Member

diasdavid commented Mar 6, 2017

@dryajov as we talked over IRC, I'm catching up with all the things, nevertheless, thank you so much for investing time into this.

I need to double check here a couple of things:

  • Has there been any decision on how to enable a Node to become a relay node? Perhaps by having a flag in the config?
  • Is there a test being added libp2p-ipfs-nodejs that shows how all of this got integrated?
@dryajov

This comment has been minimized.

Show comment
Hide comment
@dryajov

dryajov Mar 6, 2017

Member

@diasdavid

  • I remember seeing some mentions (tho would have to search for the issue) regarding this being controlled by a config value and was planing on adding it once I start integrating things.
  • Have not yet added any tests to libp2p-ipfs-node, but working on functional/unit tests in libp2p-circuit itself.
Member

dryajov commented Mar 6, 2017

@diasdavid

  • I remember seeing some mentions (tho would have to search for the issue) regarding this being controlled by a config value and was planing on adding it once I start integrating things.
  • Have not yet added any tests to libp2p-ipfs-node, but working on functional/unit tests in libp2p-circuit itself.
@lgierth

This comment has been minimized.

Show comment
Hide comment
@lgierth

lgierth Mar 6, 2017

Member

Yes the config option would control whether to relay connections for other nodes.

I imagine there could also be an option that enables constructing relay addrs for nodes which we don't have a usable address for. For example, in a browser environment, if we only know an /ip4/tcp address for a node we want to connect to, we'd take the /ipfs part form that (or soon /p2p) and make a /p2p-circuit addr from it. This part could also happen unconditionally though. The peerstore/peerbook would automatically add a /p2p-circuit addr for any peer it knows.

Member

lgierth commented Mar 6, 2017

Yes the config option would control whether to relay connections for other nodes.

I imagine there could also be an option that enables constructing relay addrs for nodes which we don't have a usable address for. For example, in a browser environment, if we only know an /ip4/tcp address for a node we want to connect to, we'd take the /ipfs part form that (or soon /p2p) and make a /p2p-circuit addr from it. This part could also happen unconditionally though. The peerstore/peerbook would automatically add a /p2p-circuit addr for any peer it knows.

@dryajov

This comment has been minimized.

Show comment
Hide comment
@dryajov

dryajov Mar 14, 2017

Member

@lgierth @diasdavid @dignifiedquire

Just a small update on where I'm at with this.

I've started reworking the dialer/listener parts as a transport. The dialer/listener take the swarm now and uses that to dial on the underlying low level transport, the listener is enabled in a similar way to identify with the relay method called from libp2p, which is what sets the relay flag in swarm and registers the listener.

This is a pretty rough WIP so far, and I understand that some of the integration points are probably wrong, partly because there doesn't seem to be a good mechanism for registering handlers that use the swarm such as identify or ping, partly because I'm still mulling over the architecture/design. That said, I think this is a good point to start a discussion/review of what I have, so that I don't get too far along on the wrong path.

Now, the fun part :)

It's possible that I'm doing something completely wrong, but I'm running into what seems to be a spdy/zlib issue. I'm running the simplest test, where I dial a peer B from peer A over the relay, but I'm running into an odd issue. Basically, I can see the relay being successfully negotiated up until the point where the swarm tries to use the relayed connection and sends a multistream header, after which Zlib throws:

Uncaught Error: incorrect header check
at Zlib._handle.onerror (zlib.js:370:17)

Here is a complete log of the run:

https://gist.github.com/dryajov/df444fe00a51298c8ca772adc13926f5#file-circuit-log

I've traced it down to spdy, which is where zlib is throwing, but I couldn't say for sure since that is a native module, and I haven't had time to debug node itself just yet. For pointers, it seems to happen somewhere around spdy-transport/protocol/spdy/zlib-pool.js.

I'm guessing there are too many layers of encryption/multiplexing happening on the same channel, maybe I just need to skip the second multiplexing attempt on the same channel if the channel is already being multiplexed?

In any case, if anyone is willing to dig through this with me, I'm available and can jump online for a debugging session. One thing to keep in mind, all of this is running locally only, if you want to run it yourself you'd have to pull in the PRs listed in the first comment, and then link things locally, so, it might be easier to do on my box at this point.

Member

dryajov commented Mar 14, 2017

@lgierth @diasdavid @dignifiedquire

Just a small update on where I'm at with this.

I've started reworking the dialer/listener parts as a transport. The dialer/listener take the swarm now and uses that to dial on the underlying low level transport, the listener is enabled in a similar way to identify with the relay method called from libp2p, which is what sets the relay flag in swarm and registers the listener.

This is a pretty rough WIP so far, and I understand that some of the integration points are probably wrong, partly because there doesn't seem to be a good mechanism for registering handlers that use the swarm such as identify or ping, partly because I'm still mulling over the architecture/design. That said, I think this is a good point to start a discussion/review of what I have, so that I don't get too far along on the wrong path.

Now, the fun part :)

It's possible that I'm doing something completely wrong, but I'm running into what seems to be a spdy/zlib issue. I'm running the simplest test, where I dial a peer B from peer A over the relay, but I'm running into an odd issue. Basically, I can see the relay being successfully negotiated up until the point where the swarm tries to use the relayed connection and sends a multistream header, after which Zlib throws:

Uncaught Error: incorrect header check
at Zlib._handle.onerror (zlib.js:370:17)

Here is a complete log of the run:

https://gist.github.com/dryajov/df444fe00a51298c8ca772adc13926f5#file-circuit-log

I've traced it down to spdy, which is where zlib is throwing, but I couldn't say for sure since that is a native module, and I haven't had time to debug node itself just yet. For pointers, it seems to happen somewhere around spdy-transport/protocol/spdy/zlib-pool.js.

I'm guessing there are too many layers of encryption/multiplexing happening on the same channel, maybe I just need to skip the second multiplexing attempt on the same channel if the channel is already being multiplexed?

In any case, if anyone is willing to dig through this with me, I'm available and can jump online for a debugging session. One thing to keep in mind, all of this is running locally only, if you want to run it yourself you'd have to pull in the PRs listed in the first comment, and then link things locally, so, it might be easier to do on my box at this point.

@dryajov

This comment has been minimized.

Show comment
Hide comment
@dryajov

dryajov Mar 16, 2017

Member

Small update on the issues I was running into above. There seem to be several possibly related issues going on at the same time, i'll try to list them bellow:

  1. If I have a setup where I would have a node with both TCP and WS enabled, and I try to dial into that node from two distinct nodes, each running on either TCP or WS, if the TCP node dials first the WS node would get stuck and the stream would end abruptly for the WS node, however, if I dial in with the WS node first and TCP second, it works as expected under SPDY but not under libp2p-multiplex. This is unrelated to relay, since for that test I would have it disabled.
    1. This would manifest in different ways depending on the muxer I was using, SPDY would give me the zlib error.
    2. libp2p-multiplex would just end the stream and return true as the error the in multistream-select in decodeFromReader, and hang up in different places.
      • It doesn't seem that the order of dialing (TCP/WS or WS/TCP) fixes this issue tho, so it could be a different issue altogether.

This could be just issues with my environment, since I've got so many modules linked. I'll upload what I have so far so that someone else can play with it, just be warned things are really messy right now. Good thing is that I can start cleaning them up now and write some decent tests.

It's not fun to be stuck ;)

Member

dryajov commented Mar 16, 2017

Small update on the issues I was running into above. There seem to be several possibly related issues going on at the same time, i'll try to list them bellow:

  1. If I have a setup where I would have a node with both TCP and WS enabled, and I try to dial into that node from two distinct nodes, each running on either TCP or WS, if the TCP node dials first the WS node would get stuck and the stream would end abruptly for the WS node, however, if I dial in with the WS node first and TCP second, it works as expected under SPDY but not under libp2p-multiplex. This is unrelated to relay, since for that test I would have it disabled.
    1. This would manifest in different ways depending on the muxer I was using, SPDY would give me the zlib error.
    2. libp2p-multiplex would just end the stream and return true as the error the in multistream-select in decodeFromReader, and hang up in different places.
      • It doesn't seem that the order of dialing (TCP/WS or WS/TCP) fixes this issue tho, so it could be a different issue altogether.

This could be just issues with my environment, since I've got so many modules linked. I'll upload what I have so far so that someone else can play with it, just be warned things are really messy right now. Good thing is that I can start cleaning them up now and write some decent tests.

It's not fun to be stuck ;)

@dryajov dryajov self-assigned this Mar 17, 2017

@diasdavid diasdavid referenced this pull request Mar 22, 2017

Closed

🚀 0.23 Release 🌟 #795

18 of 22 tasks complete

@diasdavid diasdavid changed the base branch from master to feat/v0.1.0 Mar 27, 2017

@diasdavid diasdavid merged commit 71ad80c into libp2p:feat/v0.1.0 Mar 27, 2017

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