Skip to content
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

bubble the spec up #7

Merged
merged 4 commits into from
May 11, 2016
Merged

bubble the spec up #7

merged 4 commits into from
May 11, 2016

Conversation

daviddias
Copy link
Member

as suggested on #3 (comment)

ls\n

# response
# TODO: maybe include a varint number of protocols here ?
Copy link
Member Author

Choose a reason for hiding this comment

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

@whyrusleeping @jbenet this is something we never set in stone.

Should we include a varint at the beginning of a ls response with the number of protocols (length prefixed messages) that will come next, or a varint with the size of all of those protocols summed together?

Copy link
Member

Choose a reason for hiding this comment

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

i think both: a varint of protocols and a varint of total size in bytes (the second is hard on dynamic systems, but may be fine and prevent attack on unsuspecting clients)

Copy link
Member

Choose a reason for hiding this comment

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

GREAT catch

Copy link
Member

Choose a reason for hiding this comment

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

i think the way I do it is a varint saying the entire size of the response

Copy link
Member Author

Choose a reason for hiding this comment

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

Having only a varint with the number of protocols might make things simpler to parse, since we know the number of rounds to apply.

I'm not seeing any benefit from having another varint for the global size added to the first one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you :)

Just to confirm, the will still be <> per protocol, correct?

(note: We've being including \n on the end of the protos https://github.com/diasdavid/js-multistream/blob/master/src/lib/interactive.js#L45)

Copy link
Member

Choose a reason for hiding this comment

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

The current way I do the ls response is to send:

<varint length of entire response>
<varint protocol name length>/protocol/name<newline>
...
...
...
...
<newline>

Copy link
Member Author

@daviddias daviddias Apr 26, 2016

Choose a reason for hiding this comment

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

I am expanding the example to remove any kind of ambiguity. This is what Juan proposes:

<varint-total-response-size-in-bytes><varint-number-of-protocols>\n
<varint of (protocol length + newline)><protocol><newline>
<varint of (protocol length + newline)><protocol><newline>
...
<varint of (protocol length + newline)><protocol><newline>

@whyrusleeping sounds good?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we have a verdict here? :)

Copy link
Member

@jbenet jbenet May 11, 2016

Choose a reason for hiding this comment

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

@diasdavid we settled this before -- #7 (comment) -- go-multistream needs to change

< <varint-protocol-length-plus-new-line><protocol><newline>
# ...
< <varint-protocol-length-plus-new-line><protocol><newline>
```
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've added this example in detail to solve the ambiguity of the spec. @jbenet @whyrusleeping please confirm

Copy link
Member

Choose a reason for hiding this comment

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

👍

@jbenet
Copy link
Member

jbenet commented Apr 30, 2016

hm..... not enough varints.

@daviddias
Copy link
Member Author

Going to merge, woooo \o/ :D

@daviddias daviddias merged commit ec19765 into master May 11, 2016
@daviddias daviddias deleted the bubble-spec-up branch May 11, 2016 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants