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

TLV testcases #631

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@rustyrussell
Copy link
Collaborator

commented Jul 9, 2019

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

@rustyrussell rustyrussell requested a review from t-bast Jul 9, 2019

@t-bast

t-bast approved these changes Jul 9, 2019

Copy link
Collaborator

left a comment

LGTM, thanks for this!

@cfromknecht
Copy link
Collaborator

left a comment

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.

1. Invalid stream: 0xfd01
2. Reason: type truncated

1. Invalid stream: 0xfd0001 01

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jul 10, 2019

Collaborator
Suggested change
1. Invalid stream: 0xfd0001 01
1. Invalid stream: 0xfd0100

the current value is minimally encoded, remove trailing 0x01

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 10, 2019

Author Collaborator

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?

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jul 10, 2019

Collaborator

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?

This comment has been minimized.

Copy link
@t-bast

t-bast Jul 10, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 11, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 11, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jul 11, 2019

Collaborator

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

1. Invalid stream: 0x0f fd0226
2. Reason: missing value

1. Invalid stream: 0x0f fd000101

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jul 10, 2019

Collaborator
Suggested change
1. Invalid stream: 0x0f fd000101
1. Invalid stream: 0x0f fd0100

is minimally encoded, remove trailing 0x01

1. Invalid stream: 0x10 00
2. Reason: Unknown even type.

1. Invalid stream: 0xfd0100 00

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jul 10, 2019

Collaborator
Suggested change
1. Invalid stream: 0xfd0100 00
1. Invalid stream: 0xfd0001 00
1. Invalid stream: 0xfd0100 00
2. Reason: Unknown even type.

1. Invalid stream: 0xfe01000000 00

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jul 10, 2019

Collaborator
Suggested change
1. Invalid stream: 0xfe01000000 00
1. Invalid stream: 0xfe00000001 00
1. Invalid stream: 0xfe01000000 00
2. Reason: Unknown even type.

1. Invalid stream: 0xff0100000000000000 00

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jul 10, 2019

Collaborator
Suggested change
1. Invalid stream: 0xff0100000000000000 00
1. Invalid stream: 0xff0000000000000001 00
1. Invalid stream: 0x1f 00 0f 01 2a
2. Reason: valid (ignored) tlv records but invalid ordering

1. Invalid stream: 0x02 08 0000000000000231 02 08 0000000000000451

This comment has been minimized.

Copy link
@t-bast

t-bast Jul 10, 2019

Collaborator

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?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 11, 2019

Author Collaborator

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

@t-bast t-bast self-requested a review Jul 10, 2019

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

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

Invalid stream: 0xffffffffffffffffff 00 00
Reason: type overflow
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

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

@rustyrussell rustyrussell requested a review from cfromknecht Jul 11, 2019

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

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

This comment has been minimized.

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

@t-bast sure not married to the name

@t-bast

t-bast approved these changes Jul 11, 2019

Copy link
Collaborator

left a comment

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

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2019

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

cfromknecht added some commits Jul 12, 2019

BOLT01: swap CompactSize for BigSize in TLV format
This commit modifies the varint encoding used for TLV types and lengths
to use a custom format called BigSize. The format is identical to
bitcoin's CompactSize, except it replaces the use of little-endian
encodings for multi-byte values with big-endian. This is done to prevent
mixing endianness within the protocol, since otherwise CompactSize would
be the first introduction of little-endian encodings.
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2019

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

rustyrussell added some commits Jul 9, 2019

tools/extract-formats.py: recognize numerics in field names.
For some reason (typo?) we only allowed "2", not other numbers!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
tools/extract-formats.py: handle continuous types in tlvs.
We were swallowing the unused line after `data`, but it's
normal to do:

```
1. tlvs: `n1`
2. types:
   1. type: 1 (`tlv1`)
   2. data:
     * [`tu64`:`amount_msat`]
   1. type: 2 (`tlv2`)
   2. data:
     * [`short_channel_id`:`scid`]
```

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
BOLT 1: explicitly disallow trailing data, require minimal values.
We didn't explicitly say that the TLV is bad if length exceeds
the message length!

We didn't specify whether to ignore extra bytes: we should.
Similarly, contents of values must be minimal (i.e. tu64 etc).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
BOLT 1: Add test vectors.
These are based on @t-bast's vectors from #607, with a few more
cases.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
BOLT 1: fix test vectors.
(Should be folded into prev commit before merge!)

1. As reported by @cfromknecht we didn't get the endian correct.

2. I updated the test vectors to definitively break in multiple places if
   that happens (for both length and type).

3. I also fixed the case where our "not minimally encoded type" was also invalid
   because it omitted the value (prefer single-cause failures per test).

4. I also test that we accept the encodings for 253, 254 and 255, which are
   unusual in that they take 3 bytes for a single byte value.

5. Eliminate duplicate redundant test reported by @t-bast.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2019

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 added some commits Jul 16, 2019

BOLT 1: Add wrap-around tlv type test vector.
Suggested-by: @cfromknecht
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
BOLT 1: Fix up endian... again!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
BOLT 1: fixup: use TLV instead of tlv
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
spellcheck: allow space-separated hex, and a few new terms.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:tlv-testcases branch from 99d3440 to d70fe60 Jul 16, 2019

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2019

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

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

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

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

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

rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Jul 16, 2019

run-tlvstream: update to use latest BOLT draft.
lightningnetwork/lightning-rfc#631

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

Tested-ACK d70fe60

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.