TLV parsing by niftynei · Pull Request #2501 · ElementsProject/lightning

niftynei · 2019-03-26T01:42:34Z

Have the wire generators be TLV savvy. Adds parsers for nested structs etc.

Addition of a new TLV type requires adding a new file gen_{tlv_name}_csv to the wires. The parsers will identify TLV fields from the _csv file, and then find the appropriate additional file to parse.

Also generates the structs necessary to figure out whether or not these fields have been included. In general, you can check for a TLV type's inclusion via whether or not the returned tlv struct's message field is null.

A few notes:

the size of any TLV individual message is ~~capped at u8~~ a varint.
we fail if an even numbered TLV type message is not understood by the parser

rustyrussell

Minor issues only. I'm happy if you want to keep the structs for now, since we can always change the API to open-code the fields if it proves a problem, and I don't want to create any roadblocks.

rustyrussell · 2019-04-01T22:19:13Z

 funding_locked,36
 funding_locked,0,channel_id,32
 funding_locked,32,next_per_commitment_point,33
+open_channel2,56


This is OK for now, but I think we're going to have to move to a system where we have experimental CSV files (which get compiled in if EXPERIMENTAL defined).

I went back and forth on whether or not to include these. Really they're only here because I needed something to test with 🌵

rustyrussell · 2019-04-01T22:20:22Z

+        self.is_tlv = False
+
+        # field name appended with '+' means this field contains a tlv
+        if name.endswith('+'):


This isn't going to work well in the spec; let's use name.endswith('tlv')?

rustyrussell · 2019-04-01T22:25:15Z

+        """ prints fromwire function definition for a TLV message.
+        these are significantly different in that they take in a struct
+        to populate, instead of fields, as well as a length to read in
+        """


This is an interesting choice, and I'm not sure it's the correct one (we changed from structs to individual parameters early on in our wire parsing).

We could parse TLVs just like we do a formatted field, except everything has to be a ptr-to-ptr so we can set it to NULL if it doesn't appear.

right, i think either of these is a valid way to do this. i had the impression that most TLV messages would be rather involved field wise, but it's also possible they're mostly one-off fields...

rustyrussell · 2019-04-01T22:28:48Z

 tlv__type_impl_towire_fields = """\tif ({tlv_name}->{name}) {{
 \t\ttlv_msg = tal_arr(ctx, u8, 0);
-\t\ttowire_u16(p, {enum});
+\t\ttowire_u8(p, {enum});


Except I was wrong and it (and type) should be varint :)

rustyrussell · 2019-04-01T22:31:40Z

 \t\t}}
 \t}}
-\tif (!*p) {{
+\tif (!*p || start_len - *plen != *len) {{


It's not entirely clear to me that this is correct. For onion, at least, we should on first type=0 and ignore any remaining. Seems consistent to do that elsewhere, too.

hmm you're right. there's also the 'ordering' aspect of the proposed TLV spec that this implementation is missing at the moment. will update.

Version 1.1 of the lightning-rfc spec introduces TLVs for optional data fields. This starts the process of updating our auto-gen'd wireformat parsers to be able to understand TLV fields. The general way to declare a new TLV field is to add a '+' to the end of the fieldname. All field type declarations for that TLV set should be added to a file in the same directory by the name `gen_<field_name>_csv`. Note that the FIXME included in this commit is difficult to fix, as we currently pass in the csv files via stdin (so there's no easy way to ascertain the originating directory of file)

'.c' wire format files include case statements to print the names of enums. Include such methods for the enums pertaining to tlv's as well.

Add tlv-messages to the general messages set so that their parsing messages get printed out. FIXME: figure out how to account for partial message length processing?

Since messages in TLV's are optional, the ideal way to deal with them is to have a 'master struct' object for every defined tlv, where the presence or lack of a field can be determined via the presence (or lack thereof) of a struct for each of the optional message types. In order to do this, appropriately, we need a struct for every TLV message. The next commit will make use of these. Note that right now TLV message structs aren't namespaced to the TLV they belong to, so there's the potential for collision. This should be fixed when/where it occurs (should fail to compile).

construct structs for the TLV's. these will be the 'return type' for tlv fields in parent messages (so to speak)

clean up basetype parsing code a bit

much better than statically calculating the sizeof

@rustyrussell

Suggested-By: @rustyrussell

@rustyrussell

Suggested-By: @rustyrussell

@rustyrussell

let's let the fromwire__tlv methods allocate the tlv-objects and return them. we also want to initialize all of their underlying messages to NULL, and fail if we discover a duplicate mesage type. if parsing fails, instead of returning a struct we return NULL. Suggested-By: @rustyrussell

fail if a message type is even and it's not included. otherwise, continue with the next message type.

passing back a null TLV was crashing here, because we tried to dereference a null pointer. instead, we put it into a temporary struct that we can check for NULL-ness, before assigning to the passed in pointer.

need to pass in a pointer to the array so that when we advance the array in the subcalls, it advances in the parent. oops

otherwise they'll get cleaned up when the message is free'd. it's nbd either way, but this seems tighter.

include includes for TLV _csv files

Updated to match what the CSV generator in the RFC repo actually outputs, see lightning/bolts#597

so we can put and pull bitcoin 'var_int' length types from the wire. for more info on variable integers, see http://learnmeabitcoin.com/glossary/varint

TLV's use var_int's for messages sizes, both internally and in the top level (you should really stack a var_int inside a var_int!!) this updates our automagick generator code to understand 'var_ints'

niftynei · 2019-04-03T02:37:31Z

@rustyrussell fixed up to exclude any and all references to 'dual funding' :)

if you're happy with this, i think we're good to merge for now. here's a short list of things that need to be improved in a follow up PR:

0x0 type stops parsing
ordering of messages as per spec
onion types need to be zero padded (?)
the print_towire/fromwire's for the devtool/decodemsg utility need to be amended

rustyrussell · 2019-04-03T03:10:09Z

@rustyrussell fixed up to exclude any and all references to 'dual funding' :)

if you're happy with this, i think we're good to merge for now. here's a short list of things that need to be improved in a follow up PR:
* 0x0 type stops parsing

I think at the moment it will break parsing, being considered even :)

* ordering of messages as per spec

I think this Just Works if the spec lists them in ascending order But probably good to check!

* onion types need to be zero padded (?)

Naah, I think what will happen is we'll memcpy it in, so remainder will be zero.

* the print_towire/fromwire's for the devtool/decodemsg utility need to be amended

Def. nice to have.

rustyrussell

Ack b58df84

rustyrussell · 2019-04-03T03:14:38Z

+	if (*max < 1) {
+		fromwire_fail(cursor, max);
+		return 0;
+	}


Note that this test is redundant!

niftynei requested review from cdecker and rustyrussell March 26, 2019 01:42

niftynei changed the title ~~TLV parsing~~ WIP: TLV parsing Mar 28, 2019

niftynei force-pushed the dual-funded-tlvs branch 5 times, most recently from c1a37c0 to 174e214 Compare March 29, 2019 00:20

niftynei changed the title ~~WIP: TLV parsing~~ TLV parsing Mar 29, 2019

niftynei added the v1.1 label Mar 29, 2019

rustyrussell approved these changes Apr 1, 2019

View reviewed changes

niftynei added 19 commits April 2, 2019 19:28

tlv: add tlv fields to enum declarations in implementation file

e3db317

'.c' wire format files include case statements to print the names of enums. Include such methods for the enums pertaining to tlv's as well.

tlv: parse fields for tlv messages

5b89270

tlv: add tlv messages to general message set

ac951a6

Add tlv-messages to the general messages set so that their parsing messages get printed out. FIXME: figure out how to account for partial message length processing?

tlv: build tlv top-level structs

ea5182b

construct structs for the TLV's. these will be the 'return type' for tlv fields in parent messages (so to speak)

tlv: add fromwire_ methods for TLV structs

63275ef

tlv: consolidate basetype parsing

0e9ad07

clean up basetype parsing code a bit

tlv: calculate sizeof by measuring message length

ea9c800

much better than statically calculating the sizeof

tlv: remove leading '_' from things

a9dee94

Suggested-By: @rustyrussell

tlv: make message sizes u8 not u16

2c698de

Suggested-By: @rustyrussell

tlv: it's ok to be odd

e161106

fail if a message type is even and it's not included. otherwise, continue with the next message type.

tlv: don't crash when you fail to parse the TLV

c06f2a7

passing back a null TLV was crashing here, because we tried to dereference a null pointer. instead, we put it into a temporary struct that we can check for NULL-ness, before assigning to the passed in pointer.

tlv: fail if parsed length doesn't match packet length

2302e43

tlv: fixup parsing for multi-message tlv's

3f711af

need to pass in a pointer to the array so that when we advance the array in the subcalls, it advances in the parent. oops

tlv: free intermediate messages when they're created

c8bc0a0

otherwise they'll get cleaned up when the message is free'd. it's nbd either way, but this seems tighter.

tlv: fixup FIXME -- remove comments + use includes

a889c6a

include includes for TLV _csv files

tlv: adapt to work with new output format

73e0518

Updated to match what the CSV generator in the RFC repo actually outputs, see lightning/bolts#597

niftynei added 2 commits April 2, 2019 19:30

wire: add var_int parsing functions

2cd37fa

so we can put and pull bitcoin 'var_int' length types from the wire. for more info on variable integers, see http://learnmeabitcoin.com/glossary/varint

tlv: use var_ints for size of messages

b58df84

TLV's use var_int's for messages sizes, both internally and in the top level (you should really stack a var_int inside a var_int!!) this updates our automagick generator code to understand 'var_ints'

niftynei force-pushed the dual-funded-tlvs branch from 8007a67 to b58df84 Compare April 3, 2019 02:32

rustyrussell approved these changes Apr 3, 2019

View reviewed changes

rustyrussell merged commit bad0ac6 into ElementsProject:master Apr 3, 2019

Conversation

niftynei commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niftynei Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niftynei commented Apr 3, 2019

Uh oh!

rustyrussell commented Apr 3, 2019

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

niftynei commented Mar 26, 2019 •

edited

Loading

niftynei Apr 3, 2019 •

edited

Loading