Bolt 1: Clarify the definition of tu fields

[nepet]

The sentence ..., leading zeros in integers can be omitted in the Bolt 1 fundamental type section: https://github.com/lightning/bolts/blob/50b2df24a27879e8329712c275db78876fd022fe/01-messaging.md#fundamental-types imposes that it is not necessary, but optional to truncate leading zeros in tuXX fields.

Having a look at the tuXX parsers across the ln implementations, it seems that it is expected to omit leading zeros.
This PR tries to find a better explanation/definition to avoid uncertainties.

[morehouse]

Which implementations require leading zeros to be truncated? From a quick look at CLN and LND, they appear to work fine for untruncated integers.

[t-bast]

Eclair requires those values to be minimally-encoded, but I never realized the spec didn't make it a requirement...

I don't see any good reason to keep the redundant leading zeros, if we're using a truncated integer that's specifically because we want to remove them, right? So I'd be inclined to accept this PR to make eclair's behavior official 😄

[TheBlueMatt]

SGTM

[t-bast]

ACK 60b77f6

[nepet]

Which implementations require leading zeros to be truncated? From a quick look at CLN and LND, they appear to work fine for untruncated integers.

Actually afaict, cln does check on leading zero bytes and fails if there are any. The following lines from fromwire_tlv_uint(const u8 **cursor, size_t *max, size_t maxlen) check that the first value read from stream that was stored in the bytes buffer in big-endian order is not zero.

	if (length > 0 && bytes[sizeof(bytes) - length] == 0) {
		SUPERVERBOSE("not minimal");
		fromwire_fail(cursor, max);
		return 0;
	}

Also, sorry for the (maybe) wrong terminology, I was in believe that the t in tuXX stands for truncated instead of the obvious tlv.

Let me know if you would like me to change the wording.

[morehouse]

Actually afaict, cln does check on leading zero bytes and fails if there are any. The following lines from fromwire_tlv_uint(const u8 **cursor, size_t *max, size_t maxlen) check that the first value read from stream that was stored in the bytes buffer in big-endian order is not zero.

	if (length > 0 && bytes[sizeof(bytes) - length] == 0) {
		SUPERVERBOSE("not minimal");
		fromwire_fail(cursor, max);
		return 0;
	}

Ah, you're right. I completely skipped over that check.

Also, sorry for the (maybe) wrong terminology, I was in believe that the t in tuXX stands for truncated instead of the obvious tlv.

Let me know if you would like me to change the wording.

I also assumed it meant "truncated unsigned integer".

[Roasbeef]

ACK on the changes, could also be an opportunity to add some basic test vectors here to tighten things up.

[nepet]

If I understand the following lines from the ## Appendix B: Type-Length-Value Test Vectors section correctly, the minimum encoding is already addressed.

bolts/01-messaging.md

Lines 709 to 715 in 50b2df2

	The n1 namespace supports the following TLV types:
	1. `tlv_stream`: `n1`
	2. types:
	1. type: 1 (`tlv1`)
	2. data:
	* [`tu64`:`amount_msat`]

bolts/01-messaging.md

Lines 791 to 799 in 50b2df2

	1. Invalid stream: 0x01 01 00
	2. Reason: encoding for `n1`s `tlv1`s `amount_msat` is not minimal
	1. Invalid stream: 0x01 02 0001
	2. Reason: encoding for `n1`s `tlv1`s `amount_msat` is not minimal
	1. Invalid stream: 0x01 03 000100
	2. Reason: encoding for `n1`s `tlv1`s `amount_msat` is not minimal

Maybe we can further clarify it by providing separate test vectors for the fundamental tuXX type. But if people already are using the given set of test vectors the cases should be covered and the rewording of the definition might be sufficient to grasp the requirements of those fields.

[t-bast]

ACK bf97a79

[morehouse]

ACK bf97a79

[vincenzopalazzo]

ACK bf97a79

[rustyrussell]

Ack bf97a79