BOLT 4: link to BOLT 1 for tlv_payload format

[bitonic-cjp]

It might seem a bit redundant, but for me it would have helped to have this link.

I assumed that the payload has the format of tlv_stream. Technically, this leads to a small inconsistency though: BOLT 4 says somewhere that "no TLV value can ever be shorter than 2 bytes", but tlv_stream can technically consist of zero tlv_record elements, resulting in zero bytes. I don't know if it is ever possible for a node to know what to do when it receives a zero-byte payload. It'd have to be some kind of (supposedly useful and well-defined) "default behavior" of the node.

Closes #800

[t-bast]

Small comment about existing tooling to check, but otherwise I think it's a good idea to add the link to bolt 1.

[t-bast]

I'm not sure @rustyrussell's tooling to automatically generate the C definitions from the spec will like that change

[cdecker]

BOLT 4 says somewhere that "no TLV value can ever be shorter than 2 bytes", but tlv_stream can technically consist of zero tlv_record elements, resulting in zero bytes.

It is consistent: a stream may have length 0 if it contains no records. A record cannot be shorter than 2 bytes (1+ byte type, 1+ bytes length, 0+ bytes value).

Though I have to admit that an empty stream is not really useful, it is valid. The interpretation and default behavior depends on the context. Allowing a zero-length stream allowed us for example to pretend messages always had a tlv stream attached to its end, and be backward compatible without any changes.

[bitonic-cjp]

BOLT 4 says somewhere that "no TLV value can ever be shorter than 2 bytes", but tlv_stream can technically consist of zero tlv_record elements, resulting in zero bytes.

It is consistent: a stream may have length 0 if it contains no records. A record cannot be shorter than 2 bytes (1+ byte type, 1+ bytes length, 0+ bytes value).

Where the BOLT says "This is safe since no TLV value can ever be shorter than 2 bytes", I guess it should technically say "This is safe since tlv_payload cannot have a length of one byte". It can technically have a length of zero bytes (if it contains zero tlv_record entries) - in that case you'd have a problem, since length would have to be zero, and the receiving node would incorrectly interpret that as signaling a legacy hop_data payload.

Please correct me if I'm wrong.

[cdecker]

I see, I must've missed that you were referring to the onion (rereading I should have seen that, since you mention bolt4). Indeed a zero-length tlv stream in an onion doesn't make sense, and the comment refers to the former realm bytes being 0x00 (legacy payload) and 0x01 (escape hatch for future payload changes) are not going to be used with tlv streams.

[rustyrussell]

Well, the 'tlv_stream' vs 'tlv_record' nomenclature came a bit later (the tlvs here stands for tlv_stream`).

We could use tlv_stream: instead of tlvs: everywhere, but then tools/extract-formats.py would need updating.

[t-bast]

We could use tlv_stream: instead of tlvs: everywhere, but then tools/extract-formats.py would need updating.

Right, so either we update this tool in the same PR, or simply revert the naming change @bitonic-cjp

[bitonic-cjp]

I'm willing to try and see if I can consistently replace tlvs by tlv_stream everywhere, including in tools/extract-formats.py.

Going through the BOLTs, I found cases where I'm not sure how to read them and how to modify them. For instance, in BOLT 02 I find this usage:

   * [`accept_channel_tlvs`:`tlvs`]

1. tlvs: `accept_channel_tlvs`

Based on the previously listed items, [accept_channel_tlvs:tlvs] should be in [type, name] order, but it seems to be the other way around. Or should I actually read accept_channel_tlvs as the type (a subtype of tlv_stream, I suppose), and tlvs as the name of the field? It might be more clear if it is written as [tlv_stream: accept_channel_tlvs], so that tlv_stream is the type (like all fundamental types, defined in BOLT 01). The modified text would then become:

   * [`tlv_stream`: `accept_channel_tlvs`]

1. `tlv_stream`: `accept_channel_tlvs`

Is this OK? Would this require more changes to existing tooling?

[Roasbeef]

LGTM 🏆

[rustyrussell]

The type names are global. So, accept_channel ends in an accept_channel_tlvs, which is defined to be a tlvs. We also have subtype.

The name of the field in this case (tlvs) is a bit redundant, sure, since we can only have one...

[bitonic-cjp]

Based on @rustyrussell 's description above, I propose a change along the lines of

   * [`accept_channel_tlvs`:`tlvs`]

1. `tlv_stream`: `accept_channel_tlvs`

...which would mean the following:

• the accept_channel structure has a final element of type accept_channel_tlvs, which is named tlvs.
• The accept_channel_tlvs type is a tlv_stream type, with possible elements listed below.

I'll make these changes, together with the tooling changes, and add them to this pull request.

[bitonic-cjp]

Should be good now. extract-formats.py seems to work for me.

[rustyrussell]

Ack d7e463d