The Circuit Relay Specification #22

daviddias · 2017-07-08T16:05:30Z

As promised on Friday, July 8, I've cleaned up, coalesced and added what was missing to the Relay spec, taking the bits from PRs #18 and #15 and all the comments.

Hope you enjoy :)

dryajov

I've added a few comments that I think we should address before merging, otherwise looks good!

dryajov · 2017-07-08T19:24:21Z

relay/README.md

-source and the circuit followed to establish communication between the two.
-This provides the destination side with full knowledge of the circuit which,
-if needed, could be rebuilt in the opposite direction.
+Unlike a transparent **tunnel**, where a libp2p peer would just proxy a communication stream to a destination (the destination being unaware of the original source), a circuit relay makes the destination aware of the original source and the circuit followed to establish communication between the two. This provides the destination side with full knowledge of the circuit which, if needed, could be rebuilt in the opposite direction.


Do we really need this? It seems like there could be a lot of complexity involved in making this work properly, as opposed to leaving the job of re-establishing a dropped connection to the source. So far I have left this out in the js implementation.

if needed, could be rebuilt

It is not handled by the module. It is "if needed, the dst has the information to rebuild the conn back"

dryajov · 2017-07-08T19:27:01Z

relay/README.md

- |    +------- multiaddr of the listening node
- |
- +------------ multiaddr of the dialing node
+message CircuitRelay {


Looks good, but should we have a version as part of the message as well? I know its communicated as part of the multistream-select rpc end point, but having a version in the message might help preventing stupid mistakes like we made changes to the protobuff msg, but forgot to bump the multistream version.

protobufs have a very clean way to be updated already

"we made changes to the protobuf msg, but forgot to bump the multistream version" - this is not a valid technical reason, the truth is that adding a version won't stop you from making any of the mistakes you suggest, in fact, it opens the door to even more.

@diasdavid you're correct, reading a bit more into protobufs clarified this.

dryajov · 2017-07-08T19:28:29Z

relay/README.md

+  enum Type { // RPC identifier, either HOP, STOP or STATUS
+    HOP = 1; 
+    STOP = 2; 
+    STATUS = 3;


If we're returning the status as part of the message, we should probably make the return codes protobuf enums as well.

I like that 👍

@dryajov wanna PR that addition?

@diasdavid sure thing.

@diasdavid - here it is #23

JustinDrake

Very nice to have a spec for this critical piece of infrastructure 👍

JustinDrake · 2017-07-09T12:44:20Z

relay/README.md

+**Events:**
+- phase I: Open a request for a relayed stream (A to R).
+  - A dials a new stream `sAR` to R using protocol `/libp2p/circuit/relay/0.1.0`.
+  - A sends a CircuitRelay message with `{ type: 'HOP', srcPeer: '/p2p/QmA', dstPeer: '/p2p/QmB' }` to R through `sAR`.


Should this HOP message be immediately acknowledged by R with a message? Opening a new stream sRB can take some time, and it would be good for A to be able to distinguish timeouts where R is slow, or where B is slow.

It's acknowledged in the sense that:

a connection is open

a multistream + secio + spdy + multistream again to /libp2p/circuit/relay is done

The next ack is when R finishes sRB

JustinDrake · 2017-07-09T12:45:51Z

relay/README.md

 ## Overview

-The circuit relay is a means of establishing connectivity between
-libp2p nodes (such as IPFS) that wouldn't otherwise be able to connect to each other.
+The circuit relay is a mean to establish connectivity between libp2p nodes (e.g IPFS) that wouldn't otherwise be able to establish a direct connection to each other.


Typo "mean" should be "means". (The singular of "means" is "means".)

JustinDrake · 2017-07-09T12:46:16Z

relay/README.md

+| 251   | "failed to parse dst addr: no such protocol ipfs" | The `<dst>` multiaddr in the header was invalid |
+| 260   | "passive relay has no connection to dst"          | |
+| 261   | "active relay could'nt dial to dst: conn refused" | relay could not form new connection to target peer |
+| 262   | "could'nd dial to dst: BAD ERROR"                 | relay has conn to dst, but failed to open a stream |


Typo "could'nd" -> "couldn't"

JustinDrake · 2017-07-09T12:52:53Z

relay/README.md

+  - R opens a new stream `sRB` to B using protocol `/libp2p/circuit/relay/0.1.0`.
+  - R sends a CircuitRelay message with `{ type: 'STOP', srcPeer: '/p2p/QmA', dstPeer: '/p2p/QmB' }` on `sRB`.
+  - R sends a CircuitRelay message with `{ type: 'STATUS', code: 'OK' }` on `sAR`.
+- phase III: Streams are piped together, establishing a circuit


Where exactly does the piping happen? Does it happen after R sends { type: 'STATUS', code: 'OK' } on sAR? If so, is there a race condition here? Should it happen before R sends that message?

It's after R sends that { type: 'STATUS', code: 'OK' }. The order in which the items are listed is relevant.

Where is the race condition here?

The race condition is as follows. A may try to send a message for B on sAR after receiving { type: 'STATUS', code: 'OK' } from R, but before piping has happened, i.e. before a circuit to B is properly formed. In practice, network latency will likely be high enough that piping will happen in time. Still, piping should maybe happen before sending { type: 'STATUS', code: 'OK' } to avoid the (theoretical) race condition.

@diasdavid agree with @JustinDrake in the current js implementation OK is sent after the pipe has been established otherwise there is no way to tell if the pipe is ready without an additional message that would indicate that.

If A writes on sAR and sAR hasn't been piped to sRB then the message is buffered (backpressure).

If A writes on sAR and sAR hasn't been piped to sRB then the message is buffered (backpressure).

Backpressure may not completely address the race condition. The reason is that the recipient of messages sent to the sAR stream could be either R or B. As it stands the protocol is unambiguous because after A sends the initial HOP message to R it is not expected to send further messages to R. But in theory the protocol could be extended to allow for A to send further messages to R before the circuit is established. Doing things the other way round may be more futureproof.

Looking at this closer, I see another issue that might come up with multihop dialing. In shallow/matreshka dialing mode, there is no way for the R to tell weather the circuit has been completed or not until the last portion of the multihop address is dialed, for that reason I have the STOP handler signaling the OK rather than HOP.

@lgierth @whyrusleeping @diasdavid wanna chime in on this one? Seems like the only contention point right now? I agree with the points that @JustinDrake is bringing up.

i'm not sure i see an issue here, If R sends the message to A, then pipes the connection, any message sent from A to R will just be buffered (or A will not be able to finish the write) until things are piped. It is assumed we are operating over a reliable ordered stream (not something like UDP where packets could get dropped if we don't read them)

@whyrusleeping I guess you're right. I remember dealing with some issues initially, which made me experiment a bit with where the ok is sent, but most likely it was unrelated to the ordering of the messages. I'll give it another go sinse I'll have to rewrite message handling anyways.

JustinDrake · 2017-07-09T12:55:17Z

relay/README.md

+| 251   | "failed to parse dst addr: no such protocol ipfs" | The `<dst>` multiaddr in the header was invalid |
+| 260   | "passive relay has no connection to dst"          | |
+| 261   | "active relay could'nt dial to dst: conn refused" | relay could not form new connection to target peer |
+| 262   | "could'nd dial to dst: BAD ERROR"                 | relay has conn to dst, but failed to open a stream |


What is a BAD ERROR? The name "BAD" isn't very descriptive.

JustinDrake · 2017-07-09T12:58:03Z

relay/README.md

@@ -1,69 +1,48 @@
 # Circuit Relay


Maybe add the version number (0.1.0)

JustinDrake · 2017-07-09T12:58:34Z

relay/README.md

+Examples:
+
+- `/p2p-circuit/p2p/QmVT6GYwjeeAF5TR485Yc58S3xRF5EFsZ5YAF4VcP3URHt` - Arbitrary relay node
+- `/ip4/127.0.0.1/tcp/5002/ipfs/QmdPU7PfRyKehdrP5A3WqmjyD6bhVpU1mLGKppa2FjGDjZ/p2p-circuit/p2p/QmVT6GYwjeeAF5TR485Yc58S3xRF5EFsZ5YAF4VcP3URHt` - Specific relay node


/ipfs should be /p2p. Same with /ipfs/QmTwo => /p2p/QmTwo.

JustinDrake · 2017-07-09T13:02:03Z

relay/README.md

+
+| Code  | Message                                           | Meaning    |
+| ----- |:--------------------------------------------------|:----------:|
+| 100   | OK                                                | Relay was setup correctly |


Should this be "One leg of the relay was setup correctly", instead of the full relay?

This is a very good question, I need it needs to be clarified.

Dialing from A to R self-confirms because of all the libp2p dance (multistream, secio, protocol muxing), we really just need to signal that we managed to reach the other end (B).

This brings be back to => #22 (comment)

JustinDrake · 2017-07-09T18:55:23Z

relay/README.md

@@ -73,106 +52,156 @@ Cast:
 - QmRelay, a node which speaks the circuit relay protocol (go-ipfs or js-ipfs).

 Scene 1:
- QmOne wants to connect to QmTwo,
-  and through peer routing has acquired a set of addresses of QmTwo.
+- QmOne wants to connect to QmTwo, and through peer routing has acquired a set of addresses of QmTwo.
 - QmTwo doesn't support any of the transports used by QmOne.
 - Awkward silence.

 Scene 2:
 - All three nodes have learned to speak the `/ipfs/relay/circuit` protocol.


/ipfs/relay/circuit should be /libp2p/relay/circuit.

JustinDrake · 2017-07-09T18:56:29Z

relay/README.md

 ## Overview

-The circuit relay is a means of establishing connectivity between
-libp2p nodes (such as IPFS) that wouldn't otherwise be able to connect to each other.
+The circuit relay is a means to establish connectivity between libp2p nodes (e.g IPFS) that wouldn't otherwise be able to establish a direct connection to each other.


"(e.g IPFS)" => "(e.g. IPFS nodes)"

(add missing . and clarify)

JustinDrake · 2017-07-09T18:56:54Z

relay/README.md

-(instead of e.g. TCP.): One node asks a relay node to connect to another node
-on its behalf. The relay node shortcircuits its streams to the two nodes,
-and they are then connected through the relay.
+Apart from that, this relayed connection behaves just like a regular connection would, but over an existing swarm stream with another peer (instead of e.g. TCP.). A node asks a relay node to connect to another node on its behalf. The relay node short-circuits streams between the two nodes, enabling them to reach each other.


"(instead of e.g. TCP.)" => remove extraneous dot

JustinDrake · 2017-07-09T19:02:18Z

relay/README.md

-`/p2p-circuit` multiaddrs don't carry any meaning of their own.
-They need to encapsulate a `/p2p` address, or
-be encapsulated in a `/p2p` address, or both.
+`/p2p-circuit` multiaddrs don't carry any meaning of their own. They need to encapsulate a `/p2p` address, or be encapsulated in a `/p2p` address, or both.


How can a /p2p-circuit multiaddr be encapsulated in a /p2p address without encapsulating a /p2p address? In other words, shouldn't the destination /p2p address be mandatory?

The wording might not be perfect. Could you illustrate the case that doesn't make sense to you?

The case that is not allowed is <relay peer multiaddr>/p2p-circuit/ (destination omitted). What about the following wording?

A /p2p-circuit multiaddress needs to encapsulate a /p2p destination address, and optionally be encapsulated by a /p2p relay address.

I think this can be added as additional bullet points to the main sentense:

/p2p-circuit multiaddrs don't carry any meaning of their own. They need to encapsulate a /p2p address, or be encapsulated in a /p2p address, or both.

/p2p-circuit multiaddress encapsulates a /p2p destination address - e.g. /p2p-circuit/p2p/QmHash

can optionally be encapsulated by a /p2p relay address - e.g. /p2p/ipfs/QmRelay/p2p-circuit/p2p/QmDest

dryajov · 2017-07-09T20:21:01Z

relay/README.md

+
+We have considered more features but won't be adding them on the first iteration of Circuit Relay, the features are:
+
+- Multihop relay - With this specification, we are only enabling single hop relays to exist. Multihop relay will come at a later stage as Packet Switching.


The js does implement multihop dialing in the form of shallot/matreshka dialing. This was based on the conversations we had at the time of me starting the implementation, what has changed that we now have scrapped this part?

There is nothing wrong with experimentation. It was part of Future Work spec wise from the beginning.

Makes sense. :)

Adding Status enum

daviddias · 2017-07-12T05:14:38Z

From IRC:

06:06 <@daviddias> Seems that other than some miner typos, there are no outstanding issues in the relay spec
06:07 <@daviddias> Shall we merge it today?
06:07 <@whyrusleeping> sounds good to me

📣 If no blockers appear, I shall merge the spec today 📯

JustinDrake · 2017-07-12T16:25:09Z

@diasdavid Are you going to address #22 (comment) before merging?

daviddias · 2017-07-14T11:24:13Z

@JustinDrake addressed last Wednesday

daviddias · 2017-07-14T11:28:35Z

All right, this is it, no one has raised any other blockers, we can say that this spec is COMPLETE

spec: The circuit relay spec

c1500a4

daviddias force-pushed the spec/circuit-relay branch from 54116bf to c1500a4 Compare July 8, 2017 16:06

daviddias mentioned this pull request Jul 8, 2017

The Circuit Relay Specification: The first iteration #15

Closed

daviddias requested review from a user, dryajov, whyrusleeping and Kubuxu July 8, 2017 16:08

daviddias mentioned this pull request Jul 8, 2017

Clarify/confirm <src><dst><data> multistream flow in relay #12

Closed

dryajov suggested changes Jul 8, 2017

View reviewed changes

JustinDrake reviewed Jul 9, 2017

View reviewed changes

apply review

8967cc1

JustinDrake reviewed Jul 9, 2017

View reviewed changes

dryajov reviewed Jul 9, 2017

View reviewed changes

dryajov and others added 2 commits July 9, 2017 13:33

Adding Status enum

77d8bdf

Merge pull request #23 from libp2p/spec/circuit-relay-status-enum

b1ea4ca

Adding Status enum

This was referenced Jul 9, 2017

⛵️ Captain.log - libp2p JavaScript Implementation 🌟 libp2p/js-libp2p#6

Closed

⚡️ v0.25.0 RELEASE 🚀 ipfs/js-ipfs#910

Closed

fix typos

f8254b5

dryajov approved these changes Jul 12, 2017

View reviewed changes

daviddias merged commit 596c4f8 into master Jul 14, 2017

daviddias deleted the spec/circuit-relay branch July 14, 2017 11:28

dryajov mentioned this pull request Aug 17, 2017

P2P Circuit Advertisements #33

Open


		We have considered more features but won't be adding them on the first iteration of Circuit Relay, the features are:

		- Multihop relay - With this specification, we are only enabling single hop relays to exist. Multihop relay will come at a later stage as Packet Switching.

The Circuit Relay Specification #22

The Circuit Relay Specification #22

Conversation

daviddias commented Jul 8, 2017 • edited Loading

dryajov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov Jul 9, 2017 • edited Loading

Choose a reason for hiding this comment

JustinDrake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinDrake Jul 9, 2017 • edited by daviddias Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinDrake Jul 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov Jul 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Jul 12, 2017 • edited Loading

📣 If no blockers appear, I shall merge the spec today 📯

JustinDrake commented Jul 12, 2017

daviddias commented Jul 14, 2017

daviddias commented Jul 14, 2017

daviddias commented Jul 8, 2017 •

edited

Loading

dryajov Jul 9, 2017 •

edited

Loading

JustinDrake Jul 9, 2017 •

edited by daviddias

Loading

JustinDrake Jul 9, 2017 •

edited

Loading

dryajov Jul 12, 2017 •

edited

Loading

dryajov Jul 9, 2017 •

edited

Loading

daviddias commented Jul 12, 2017 •

edited

Loading