Specify tlv format #603
Specify tlv format #603
Conversation
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 | |||
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Should only be sorted by type?
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
|
Closed in favor of #607 |
This first commit is the same as the previous proposal, but the second makes type a varint and explicitly handles integer fields.