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

Specify tlv format #603

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@rustyrussell
Copy link
Collaborator

commented May 6, 2019

This first commit is the same as the previous proposal, but the second makes type a varint and explicitly handles integer fields.

rustyrussell added some commits Feb 12, 2019

BOLT 1: Specify General Rules for TLV fields.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
fixup! BOLT 1: Specify General Rules for TLV fields.
Allow var_int for type, just in cast.

Make handling of integer fields explicit.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@@ -82,6 +83,50 @@ however, adding a 6-byte padding after the type field was considered
wasteful: alignment may be achieved by decrypting the message into
a buffer with 6-bytes of pre-padding.

## Type-Length-Value Format

In various places in the protocol we use a `tlv` format. This is

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht May 9, 2019

Contributor

Atm the proposal uses tlv to refer to a single record and the stream interchangeably, defining them distinctly would reduce the ambiguity

The sending node:
- MUST NOT set `type` to 0.
- MUST use the shortest possible representation for `type` and `length`
- MUST order `tlv` in ascending type, length and value.

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht May 9, 2019

Contributor

Should only be sorted by type?

This comment has been minimized.

Copy link
@t-bast

t-bast May 15, 2019

If we only sort on type we don't have a canonical encoding of a tlv stream if we have multiple occurrences of the same type. I think that's why there is this requirement to then sort on length and value (and I think it makes sense).

- MUST ignore the `tlv`
- otherwise `type` is even:
- MUST fail to parse the `tlv`
- otherwise, if `type` indicates that `value` is an integer:

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht May 9, 2019

Contributor

This is a good optimization, however I think it violates some abstraction levels by defining this in the parsing requirements. The requirements shouldn’t be concerned with the underlying encoding of value. Instead, I think it’d be better to define this in “the known encoding” for type within its namespace.

while still allowing non-backwards compatible changes in future.

The requirement on writers for ordering ensures a canonical ordering
for any `tlv` stream; readers do not check this for simplicity and

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht May 9, 2019

Contributor

What do we gain by not enforcing canonical ordering from the start? Apart from saving a few lines of code, to me this just invites more bugs (through side effects) in parsing duplicate records, or forcing the receiver to do more work in parsing the same field multiple times. It also means we can’t make other optimizations in decoding because we must support parsing the fields in any order. Simplicity and flexibility alone aren’t very compelling IMO, are there other reasons?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.