review Wire protocol section from the relay spec #18

daviddias · 2017-05-09T18:40:27Z

Reviewed the wire protocol section. There were some incoherences in the high-level order of events and some question marks everywhere, I fixed them based on commons and our conversations.

ghost · 2017-05-11T00:33:42Z

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/relay/hop` 


It's /libp2p/relay/circuit/1.0.0/hop

ghost · 2017-05-11T00:34:06Z

relay/README.md

+  - A writes Bs multiaddr `/p2p/QmB` on `sAR`
+  - R receives stream `sAR` and reads `/p2p/QmB` from it.
+- phase II: Open a stream to be relayed (R to B)
+  - R opens a new stream `sRB` to B using protocol `/libp2p/relay/stop`


Same here /libp2p/relay/circuit/1.0.0/stop

And a couple more of the same throughout the rest of the doc

ghost · 2017-05-11T00:35:28Z

relay/README.md

-#### Function definitions
-##### Process for reading a multiaddr
-We define `readLpMaddr` to be the following:
+Read a Uvarint `V` from the stream. If the value is higher than `maxAddrLen`, writes an error code back, closes the stream and halts the relay attempt. If not, then reads `V` bytes from the stream and checks if its a valid multiaddr. Check for multiaddr validity and error back if not a valid formated multiaddr.


This line should be indented as a sub-item of the previous list item

This is the definition of that function. I can change how that is communicated.

Just trying to save space :)

ghost

Thanks! Just a few

ghost · 2017-05-11T00:47:51Z

relay/README.md

-Peer R checks to make sure that `<src>` matches the remote peer of the stream its reading
-from. If it does not match, it (writes an error back?) closes the stream and halts the relay attempt.
+
+Peer R checks to make sure that `<src>` matches the remote peer of the stream its reading from. If it does not match, it writes an Error message, closes the stream and halts the relay attempt.


typo its/it's

ghost · 2017-05-11T00:48:56Z

relay/README.md

 ```
 <uvarint len(maddrA)><madddrA><uvarint len(maddrB)><madddrB>
 ```

-After this, R simply pipes the stream from A and the stream it opened to B together. R's job is complete.
+After this, R pipes the stream from A and the stream it opened to B together. R also writes OK back to A. R's job is complete.


Maybe worth noting that R's job actually continues: it now needs to keep the connections alive

dryajov · 2017-05-11T04:04:52Z

relay/README.md


-### 'stop' protocol handler
+Peer B receives a new stream on the '/libp2p/relay/stop' protocol. It then calls `readLpMaddr` twice on this stream.


Writing both the <src> and <dst> seems a bit redundant here. What is the use case for sending the <dst> as well?

Other than that LGTM! 👍

daviddias · 2017-05-11T14:54:30Z

Thanks for the review @lgierth @dryajov <3

There is just one thing that is itching me, the fact that we are mixing a protocol codec with an RPC method name on having /libp2p/relay/circuit/1.0.0/hop and /libp2p/relay/circuit/1.0.0/stop. I'm afraid that this will encourage people to start designing RPCs using multistream-select as their mechanism to hit the right function and as far libp2p should be a concern, it should just do the Protocol Muxing, not RPC select.

If we want to say that we have the hop protocol and the stop protocol within relay/circuit scope, that is '''fine''', but that would mean that semver would appear at the end.

I also don't understand the resistance to use a serializable data format like protobuf or cbor this time, we use it everywhere already (see Identify, Bitswap, DHT, and others) and it gives a nice feature that we can add more information later without having to do complicated retro compatibility tests to make sure all the bytes are read in the same order and what not. Using a protobuf this time would give us the ability to just specify the type of message (hop, stop) on the message itself.

dryajov · 2017-05-15T22:33:55Z

There is just one thing that is itching me, the fact that we are mixing a protocol codec with an RPC method name on having /libp2p/relay/circuit/1.0.0/hop and /libp2p/relay/circuit/1.0.0/stop. I'm afraid that this will encourage people to start designing RPCs using multistream-select as their mechanism to hit the right function and as far libp2p should be a concern, it should just do the Protocol Muxing, not RPC select.

For the sake of playing the devils advocate, what is the danger of using multistream select as an RPC mechanism (note, I'm neither proposing nor condemning that use case). Also, how placing the semver at the end would prevent that - the current syntax maps nicely to a sub protocol in my mind, which is accurate in this case.

I also don't understand the resistant to use a serializable data format like protobuf or cbor this time, we use it everywhere already (see Identify, Bitswap, DHT, and others) and it gives a nice feature that we can add more information later without having to do complicated retro compatibility tests to make sure all the bytes are read in the same order and what not. Using a protobuf this time would give us the ability to just specify the type of message (hop, stop) on the message itself.

I would love to hear the reasoning behind this also, the only thing that comes to mind is that the amount of structured data exchanged between peers is minimal, and going with a text based protocol saves the additional dependencies, complexity and overhead that cbor/protobuf introduce?

daviddias · 2017-05-15T22:56:59Z

what is the danger of using multistream select as an RPC mechanism

multistream-select has a cost:

it adds to the list of protocols supported, which means that there is one more element for the linear search that happens every time a protocol mux happens.
it makes it weird with semver, because as you said 'subprotocol', what is the criteria that the subprotocol doesn't have a version as well? When is a thing a protocol and when is a thing a subprotocol? It just adds complexity to the parsing
if we decide to argue about bytes (which has been a discussion in the past even when the argument is just to save 1 byte), hop and stop will be more bytes than just a number on a protobuf message, since multistream messages are not binary packed
it encourages to others do the same and you should not have to inspect multistream-select logs to check behaviors of your own protocol that is mounted on top

the only thing that comes to mind is that the amount of structured data exchanged between peers is minimal, and going with a text based protocol saves the additional dependencies, complexity and overhead that cbor/protobuf introduce?

protobuf RPC exist to reduce complexity and we already know that things change over time really well, that is why we have a whole project just for self-describable data formats :)

ghost · 2017-05-15T23:07:56Z

/hop and /stop were introduced to differentiate semantics (opening vs. receiving a relayed connection). The second multiaddr in the header was removed because it wasn't needed anymore once this differentiation was there. I agree that byte-savings are a weak argument here :)

I'd be down for us reworking this part -- I kinda agree that the changes to multistream-select would be non-trivial and likely risky, while the neccessity for them isn't very strong.

I'd also be down to at the very least put a multicodec-packed header in front of the header. (Good call.)

// edit: multicodec-packed header

dryajov · 2017-05-15T23:15:19Z

@lgierth

What do you mean by this:

I'd also be down to at the very least put a multicodec header in front of the header. (Good call.)

ghost · 2017-05-15T23:17:51Z

What you mean by this:

Similar to how there's a multicodec-packed header in a CID to describe the following hash, we'd put one in front of the relay header to describe what follows. For now this would just be the multicodec code for multiaddr (0x32), but being able to switch this over to e.g. the cbor code, would make the header more future-proof.

See https://github.com/multiformats/multicodec

dryajov · 2017-05-15T23:31:22Z

@lgierth ah, makes sense. Thanks!

dryajov · 2017-06-16T19:17:24Z

Re: RPC

I think we need to make a decision here to move circuit forward. Lets time box this, how about we decide by next Friday, Jun. 23rd the latest?

@lgierth what are your thoughts on this, I gather that you tend to agree with what @diasdavid is proposing.

I'm on board with reworking the RPC calls with a proper, binary packed format instead.

Next question is, what format should we use - cbor or protobuf (or something else)? I see that libp2p uses protobuf heavily, is that the preferred route? I don't have strong opinion on neither one of them as I haven't used either.

Lets put this to a vote.

Vote 👍 or 👎 on this issue to reimplement or leave it as is.

Use the links bellow to 👍 or 👎 CBOR vs Protobufs.

dryajov · 2017-06-16T19:23:05Z

Vote:

CBOR 👍 or 👎

dryajov · 2017-06-16T19:23:16Z

Vote:

protobufs 👍 or 👎

whyrusleeping · 2017-06-20T23:15:54Z

i prefer protobufs here, pretty much just because they will be much smaller

daviddias · 2017-06-20T23:29:23Z

I'm also in favor to use protobufs. It has suited well our needs and as @whyrusleeping points out, they are smaller.

dryajov · 2017-06-26T08:40:00Z

Looks like we're rewriting the protocol with protobufs - If there are no objections I'll start working on this ASAP.

@whyrusleeping @diasdavid @lgierth @dignifiedquire

dryajov · 2017-07-03T16:14:41Z

@lgierth @diasdavid @whyrusleeping

I though I put something together for the actual protobuf message to get the ball rolling. This is merely to start the discussion, i haven't implemented anything around this yet.

message Circuit {
  optional string version = 1;
  optional RelayMessage message = 2;
  
  enum MessageType {
    HOP = 1; 
    STOP = 2; 
  }
  
  message Peer {
    optional string id = 1; // peer id
    repeated string address = 2; // peer's dialable addresses      
  }
  
  message RelayMessage {
    optional MessageType type = 1;
    optional Peer source = 2;
    optional Peer dest = 3;
  }
}

review Wire protocol section from the relay spec

1759516

daviddias added the in progress label May 9, 2017

daviddias requested review from a user, dryajov and whyrusleeping May 9, 2017 18:40

daviddias mentioned this pull request May 9, 2017

The Circuit Relay Specification: The first iteration #15

Closed

ghost reviewed May 11, 2017

View reviewed changes

ghost suggested changes May 11, 2017

View reviewed changes

dryajov suggested changes May 11, 2017

View reviewed changes

dryajov mentioned this pull request Jun 19, 2017

feat/v0.1.0 libp2p/js-libp2p-circuit#9

Closed

53 tasks

daviddias merged commit 76dfb72 into feat/update-relay-spec Jul 8, 2017

daviddias deleted the review/relay/wire-protocol branch July 8, 2017 10:17

daviddias mentioned this pull request Jul 8, 2017

The Circuit Relay Specification #22

Merged

dryajov mentioned this pull request Aug 17, 2017

P2P Circuit Advertisements #33

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

review Wire protocol section from the relay spec #18

review Wire protocol section from the relay spec #18

daviddias commented May 9, 2017

ghost May 11, 2017

ghost May 11, 2017

ghost May 11, 2017

ghost May 11, 2017

daviddias May 11, 2017

ghost May 15, 2017

ghost left a comment

ghost May 11, 2017

ghost May 11, 2017

dryajov May 11, 2017 •

edited

Loading

dryajov May 11, 2017

daviddias commented May 11, 2017 •

edited

Loading

dryajov commented May 15, 2017

daviddias commented May 15, 2017

ghost commented May 15, 2017 •

edited by ghost

Loading

dryajov commented May 15, 2017 •

edited

Loading

ghost commented May 15, 2017

dryajov commented May 15, 2017

dryajov commented Jun 16, 2017 •

edited

Loading

dryajov commented Jun 16, 2017 •

edited

Loading

dryajov commented Jun 16, 2017 •

edited

Loading

whyrusleeping commented Jun 20, 2017

daviddias commented Jun 20, 2017 •

edited

Loading

dryajov commented Jun 26, 2017

dryajov commented Jul 3, 2017 •

edited

Loading


		### 'stop' protocol handler
		Peer B receives a new stream on the '/libp2p/relay/stop' protocol. It then calls `readLpMaddr` twice on this stream.

review Wire protocol section from the relay spec #18

review Wire protocol section from the relay spec #18

Conversation

daviddias commented May 9, 2017

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

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented May 11, 2017 • edited Loading

dryajov commented May 15, 2017

daviddias commented May 15, 2017

ghost commented May 15, 2017 • edited by ghost Loading

dryajov commented May 15, 2017 • edited Loading

ghost commented May 15, 2017

dryajov commented May 15, 2017

dryajov commented Jun 16, 2017 • edited Loading

dryajov commented Jun 16, 2017 • edited Loading

dryajov commented Jun 16, 2017 • edited Loading

whyrusleeping commented Jun 20, 2017

daviddias commented Jun 20, 2017 • edited Loading

dryajov commented Jun 26, 2017

dryajov commented Jul 3, 2017 • edited Loading

dryajov May 11, 2017 •

edited

Loading

daviddias commented May 11, 2017 •

edited

Loading

ghost commented May 15, 2017 •

edited by ghost

Loading

dryajov commented May 15, 2017 •

edited

Loading

dryajov commented Jun 16, 2017 •

edited

Loading

dryajov commented Jun 16, 2017 •

edited

Loading

dryajov commented Jun 16, 2017 •

edited

Loading

daviddias commented Jun 20, 2017 •

edited

Loading

dryajov commented Jul 3, 2017 •

edited

Loading