Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: Tangle Message #17
RFC: Tangle Message #17
Changes from 5 commits
3357004
805758f
fa24ebb
c778814
915cd2b
b973ccf
5803db6
4ce1327
04b35a7
67c8004
9d1f832
8ee6316
2caf731
a426446
5eabf5e
7b003e2
385abff
35b8653
c795980
54b0a0a
1150c72
2a43097
cc15a51
3d92d83
793378d
dcbf444
8a69a4c
0e5d060
98e7437
e323b18
dd826e0
5843c38
d3a46d9
d650bb8
ecb8b66
92a2f73
3b330c1
f4f94cd
4e9ecdd
f1b5775
466d328
d4eea48
2ef7705
e5d02e2
1ba4de5
d479e9e
c3ace55
5168461
f641685
74a46d1
b687e55
2e2ef6f
92cc7ef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we gain by using a varint here? Are you planning to have megabyte-sized messages? Make it a uint32 and reduce complexity instead of making even the header of the message variable in size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wanted to not have too many types in the message.
The idea is that less types means more simple descriptions and code.
Also, for lots of cases where we describe quantities
uint8
oruint16
could have sufficed.So the idea is that choosing varints will result mostly in shorter messages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced it will be simpler code. It's not native primitive in many languages.
So we either implement one ourselves or use the existing one. Both would end up with more validation rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
uint8_t
oruint16_t
is sufficed, why not just use one of them? they are primitive types in most languages. Thevarint
is good for data serialization but it is not easy to operate and memory management in a data structure, especially for microcontrollers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
due to recent conversations outside of github I am also leaning towards dropping varints
this decision will be finalized on the next protocol team meeting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: I would rather set fixed reasonable-sized types with some slack (you never know) replacing most varints.
varints
are somewhat complex types to parse, but I would like to draw your attention to the creation of such message rather than the parsing of it.varints
type force the entire message structure to "shift" instead of having fixed offsets. This is quite cumbersome when forming a message where all the parts are not necessarily known before-hand, and therefore the byte-requirements of lengthvarint
types is also unknown: when you fill in the data section of the message, you may see yourself push it forward in order to accommodate the correspondingvarint
size without overflowing into the data section itself. Sure, modern programming languages and libs provide abstract structures for all these, but at a cost of memory reallocation, memory concatenation and other forms of memory juggling. Leveling the participation field ultimately accounts for the flexibility of a solution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually put the
t
before theb
, sot6b1
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, trits, not trytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fundamentally different:
t5b1
encodes ternary data into binary, by converting each group of 5 trits into one byte; it cannot decode arbitrary binary datab1t6
encodes binary data into ternary, by converting each byte into 6 trits; it cannot decode arbitrary ternary dataIn RFC-15 it is also called
b1t6
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe I can delete this entire section. Now we have an RFC dedicated for computing this PoW hash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the entire section should be deleted. See #17 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
due to me looking at the RFC while you are commenting I repeated your remarks :-P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you register a custom (community) payload type?
Is 16383 sufficient for all applications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, in our meetings we threw ideas on how it might be.
I think one idea was that community member will also create RFCs but not sure this is the best way to approach this
I think that together with community volunteers we should define at least one community payload. Then we should properly document and implement it, so other community members can easily follow.
We should probably talk about it more in a meeting.
Regarding the 16,383 (2^14 - 1) limit, I just thought it was a high enough number...
Given that we support colored coins it is hard for me to imagine that the payload feature will become so popular that this limit won't suffice...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should really explain how we envision this community-driven payload draft process.
I guess we want to have a table somewhere where people can create a PR with a new ID and link to the document / RFC describing the payload.
Does it maybe make more sense to have a separate RFC with this table (similar to SLIP-44 or SLIP-173)? That could make the process considerably easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a varint if you have a max bound of 2 bytes? It is way easier to define a uint16 here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other answer
Also Wolfgang seems to not like this bound, so maybe this bound will be removed (still need to discuss this with him)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the bound (for the non-core payloads) seems arbitrary and should be removed. We should allow for the full uint32 maybe even uint64 space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the message length protection suffices.
Actually, while thinking about the case for the bound, I started to question whether the message length is really needed...