TLV testcases

[rustyrussell]

Came across some new requirements and a few tool fixes along the way...

[t-bast]

LGTM, thanks for this!

[cfromknecht]

Thanks for getting the ball rolling on the test vectors @rustyrussell! I'm about half way through, but the main thing i'm noticing so far is a discrepancy in the way that the varints are being encoded. CompactSize encodes any multi-byte values using little endian, while the tests are using big-endian.

[cfromknecht]

Suggested change

	1. Invalid stream: 0xfd0001 01
	1. Invalid stream: 0xfd0100

the current value is minimally encoded, remove trailing 0x01

[rustyrussell]

Thanks for getting the ball rolling on the test vectors @rustyrussell! I'm about half way through, but the main thing i'm noticing so far is a discrepancy in the way that the varints are being encoded. CompactSize encodes any multi-byte values using little endian, while the tests are using big-endian.

You're right :( Bitcoin strikes again...

I assume we should follow (and document!) Bitcoin CompactSize endian here, since that was the rationale for not inventing our own?

[cfromknecht]

It should be implicit from the test vectors, but if we want to be more specific fine by me :)

fwiw I have a series of varint test vectors, should I make a separate pr with those?

[t-bast]

Good catch @cfromknecht !
That shows I went too quickly when comparing my test suite and this PR, I'll spend more time on it today.

[rustyrussell]

Unfortunately, my tests passed after I fixed the endian without fixing the data. I'll add some test vectors which only work in the correct endian.

[rustyrussell]

And we need the trailing 01, otherwise it is invalid for a different reason: no length field. Though that should be 00 not 01.

[cfromknecht]

shouldn't the check for whether the type is minimally encoded be applied before attempting to parse the length?

[t-bast]

I think this is a duplicate of line 613.
Maybe replace by 0x0f 01 2a 0f 01 2b to disallow duplicate of unknown odd types?

[rustyrussell]

Next line tests duplicate ignored. I'll just remove this one, good spotting!

[cfromknecht]

would also be useful to add this to the decoding failures:

Invalid stream: 0xffffffffffffffffff 00 00
Reason: type overflow

[rustyrussell]

would also be useful to add this to the decoding failures:

Invalid stream: 0xffffffffffffffffff 00 00
Reason: type overflow

I'm confused, why is that an overflow? That's just the max possible type, no? Which is odd, so that is valid (assuming unknown)?

[rustyrussell]

Please test the New Hotness, which contains all the fixes and some new vectors.

[cfromknecht]

I'm confused, why is that an overflow? That's just the max possible type, no? Which is odd, so that is valid (assuming unknown)?

The type is max uint64 and has length 0, which is optional and so we ignore it. The following type is 0, which wraps around and breaks monotonicity, and so is not canonical

[cfromknecht]

@rustyrussell here's how we detect this case https://github.com/lightningnetwork/lnd/pull/3061/files#diff-b776b065cfabb730273c018554f81204R218

[t-bast]

The type is max uint64 and has length 0, which is optional and so we ignore it. The following type is 0, which wraps around and breaks monotonicity, and so is not canonical

I agree, but the error message shouldn't be type overflow, it should be tlv records must be ordered by monotonically-increasing types or something like that.

[cfromknecht]

@t-bast sure not married to the name

[t-bast]

I integrated all of those in my test suite and everything is green!
ACK 99d3440 (and good luck with the annoying spell-checking failures)

[Roasbeef]

Please, let's not mix endianness in the protocol. There's no reason to inherit this wart of Bitcoin which makes it that much harder to understand.

[cfromknecht]

I made a PR that adds test vectors for the varint scheme. Given the recent discussion i made them using a big-endian encoding, but either way it's probably good to have something like this: #640

[rustyrussell]

OK, I'm about to add @cfromknecht 's max-then-min type test, and then rebase on top of #640...

[rustyrussell]

OK, so @cfromknecht 's ffff to 0 test fails for other reasons: 0 is even and unknown, and there's no length field. I prefer tests which test one thing at a time; I'll change n2s tlv1 to be 0 type, then this can be an invalid under n2 test: 0xffffffffffffffffff 00 00 00

[rustyrussell]

"This time for sure!" Please re-retest!

[cfromknecht]

OK, so @cfromknecht 's ffff to 0 test fails for other reasons: 0 is even and unknown, and there's no length field. I prefer tests which test one thing at a time; I'll change n2s tlv1 to be 0 type, then this can be an invalid under n2 test: 0xffffffffffffffffff 00 00 00

The way i have it implemented, we check that the types are canonically ordered before parsing the length (or checking if the type is known).

I think this is also relevant to my other comment where the canonical varint check for the type is being applied after parsing the length. In our implementation we check that each varint is canonical at the time it is parsed

(I think this last bit might be resolved if the varint also passed the BigSize test vectors?)

[cfromknecht]

I'll proceed in updating to the latest test vectors and report back :)

[rustyrussell]

OK, so @cfromknecht 's ffff to 0 test fails for other reasons: 0 is even and unknown, and there's no length field. I prefer tests which test one thing at a time; I'll change n2s tlv1 to be 0 type, then this can be an invalid under n2 test: 0xffffffffffffffffff 00 00 00

The way i have it implemented, we check that the types are canonically ordered before parsing the length (or checking if the type is known).

I think this is also relevant to my other comment where the canonical varint check for the type is being applied after parsing the length. In our implementation we check that the varint is canonical at the time it is parsed

Ah, OK. We run our checks in the following order instead (kinda arbitrary, really):

1. check type decodes.
2. check length decodes.
3. check there's enough data for length.
4. check type ordering.
5. lookup type.
6. if found:
   1. Decode value. If that fails, fail.
   2. Check no bytes remain.
7. Otherwise:
   1. If type is even, fail.
   2. if type is odd, skip.

It's nice to have implementations do it in different orders, though...

[cfromknecht]

That makes sense then :)

We do

1. check type decodes and canonical
2. check type ordering
3. check length decodes and canonical
4. lookup type
5. if found:
   1. Decode value. If that fails, fail.
6. Otherwise:
   1. If type is even, fail.
   2. If type is odd, skip.

The check that there's enough data for length is applied while decoding or discarding the value

[t-bast]

Tested-ACK d70fe60

[cfromknecht]

@rustyrussell @t-bast updated our implementation with the latest test vectors, I think we're about ready! Noticed a few small things, but as is our implementation is all green 🚀

[cfromknecht]

i'm wondering, should this produce an error in either namespace? sending a non-canonical stream is more severe than sending an unknown required type, so failing with unknown required type on n1 would effectively downgrade the error. if we were to standardize something like:

- if stream is not canonical:
   - close channel
- if stream has unknown required type:
   - disconnect

our implementations will behave differently

[cfromknecht]

The purpose of the toy example was to just to highlight that we might need take different actions when encountering a parsing failure vs a negotiation failure.

With the current proposal feature negotiation is bundled alongside message deserialization, so we are forced to differentiate the errors at the tlv encoding level and be consistent across implementations.

[t-bast]

It's probably a nit, but I'm not a big fan of the name BigSize. It's not obvious at all that the Big is for big-endian especially since we're doing big-endian everywhere (so big-endian should be implicit). It sounds a lot like a BigInt type which is really something else.
What about CompactSizeBE instead? Or we don't name it at all and just mention that varint is a variable-length, unsigned integer encoding using Bitcoin's CompactSize format with big-endian encoding instead of little-endian?
WDYT?

[cfromknecht]

It's not obvious at all that the Big is for big-endian especially since we're doing big-endian everywhere (so big-endian should be implicit). It sounds a lot like a BigInt type which is really something else.

It's supposed to be a somewhat of a pun :P

What about CompactSizeBE instead?

Was hoping for something a little less bland tbh

Or we don't name it at all and just mention that varint is a variable-length, unsigned integer encoding using Bitcoin's CompactSize format with big-endian encoding instead of little-endian?
WDYT?

I'd prefer not to have this verbose description everywhere we want to reference it, I think we need some name just to make it easy to refer to in the spec. It was already painful enough to write it twice.

Didn't want to waste too much time on a name when drafting the proposal, and so just went with BigSize. Definitely open to more suggestions if you have any! :)

[t-bast]

I don't feel very strongly about the name and don't have a better choice than either BigSize or CompactSizeBE, I just want it to be simple to understand for spec readers without too much context.
If you and @rustyrussell like BigSize, let's go for it ;)

[rustyrussell]

Rebased and squashed commits ready for merge.