Conversation
54116bf to
c1500a4
Compare
dryajov
left a comment
There was a problem hiding this comment.
I've added a few comments that I think we should address before merging, otherwise looks good!
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
| | +------- multiaddr of the listening node | ||
| | | ||
| +------------ multiaddr of the dialing node | ||
| message CircuitRelay { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- 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.
There was a problem hiding this comment.
@diasdavid you're correct, reading a bit more into protobufs clarified this.
| enum Type { // RPC identifier, either HOP, STOP or STATUS | ||
| HOP = 1; | ||
| STOP = 2; | ||
| STATUS = 3; |
There was a problem hiding this comment.
If we're returning the status as part of the message, we should probably make the return codes protobuf enums as well.
JustinDrake
left a comment
There was a problem hiding this comment.
Very nice to have a spec for this critical piece of infrastructure 👍
| **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`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
relay/README.md
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
Typo "mean" should be "means". (The singular of "means" is "means".)
relay/README.md
Outdated
| | 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 | |
| - 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
If A writes on sAR and sAR hasn't been piped to sRB then the message is buffered (backpressure).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@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.
relay/README.md
Outdated
| | 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 | |
There was a problem hiding this comment.
What is a BAD ERROR? The name "BAD" isn't very descriptive.
relay/README.md
Outdated
| @@ -1,69 +1,48 @@ | |||
| # Circuit Relay | |||
There was a problem hiding this comment.
Maybe add the version number (0.1.0)
relay/README.md
Outdated
| Examples: | ||
|
|
||
| - `/p2p-circuit/p2p/QmVT6GYwjeeAF5TR485Yc58S3xRF5EFsZ5YAF4VcP3URHt` - Arbitrary relay node | ||
| - `/ip4/127.0.0.1/tcp/5002/ipfs/QmdPU7PfRyKehdrP5A3WqmjyD6bhVpU1mLGKppa2FjGDjZ/p2p-circuit/p2p/QmVT6GYwjeeAF5TR485Yc58S3xRF5EFsZ5YAF4VcP3URHt` - Specific relay node |
There was a problem hiding this comment.
/ipfs should be /p2p. Same with /ipfs/QmTwo => /p2p/QmTwo.
|
|
||
| | Code | Message | Meaning | | ||
| | ----- |:--------------------------------------------------|:----------:| | ||
| | 100 | OK | Relay was setup correctly | |
There was a problem hiding this comment.
Should this be "One leg of the relay was setup correctly", instead of the full relay?
There was a problem hiding this comment.
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)
| - Awkward silence. | ||
|
|
||
| Scene 2: | ||
| - All three nodes have learned to speak the `/ipfs/relay/circuit` protocol. |
There was a problem hiding this comment.
/ipfs/relay/circuit should be /libp2p/relay/circuit.
relay/README.md
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
"(e.g IPFS)" => "(e.g. IPFS nodes)"
(add missing . and clarify)
relay/README.md
Outdated
| (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. |
There was a problem hiding this comment.
"(instead of e.g. TCP.)" => remove extraneous dot
| `/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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The wording might not be perfect. Could you illustrate the case that doesn't make sense to you?
There was a problem hiding this comment.
The case that is not allowed is <relay peer multiaddr>/p2p-circuit/ (destination omitted). What about the following wording?
A
/p2p-circuitmultiaddress needs to encapsulate a/p2pdestination address, and optionally be encapsulated by a/p2prelay address.
There was a problem hiding this comment.
I think this can be added as additional bullet points to the main sentense:
/p2p-circuitmultiaddrs don't carry any meaning of their own. They need to encapsulate a/p2paddress, or be encapsulated in a/p2paddress, 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
|
|
||
| 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. |
There was a problem hiding this comment.
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 was a problem hiding this comment.
There is nothing wrong with experimentation. It was part of Future Work spec wise from the beginning.
Adding Status enum
|
From IRC: 📣 If no blockers appear, I shall merge the spec today 📯 |
|
@diasdavid Are you going to address #22 (comment) before merging? |
|
@JustinDrake addressed last Wednesday |
add laurentsenta to test-plans collaborators
chore: sync libp2p (libp2p#22)

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 :)