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

WIP: TLV parsing #2802

Merged
merged 5 commits into from Jul 18, 2019
Merged

Conversation

@rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Jul 9, 2019

This is a TLV parsing implementation which meets the latest BOLT 1 spec. It does not include the autogeneration stuff, but I think it would be a decent basis for that work since @niftynei promised to rewrite it :)

@rustyrussell rustyrussell requested a review from niftynei Jul 9, 2019
Copy link
Collaborator

@niftynei niftynei left a comment

lgtm (other than the valgrind errors)

==12046== Conditional jump or move depends on uninitialised value(s)
==12046==    at 0x4272B6: tlv_n1_eq (run-tlvstream.c:504)
==12046==    by 0x427A3B: main (run-tlvstream.c:619)
==12046==  Uninitialised value was created by a heap allocation
==12046==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12046==    by 0x4216E8: allocate (tal.c:245)
==12046==    by 0x421C75: tal_alloc_ (tal.c:423)
==12046==    by 0x42792E: main (run-tlvstream.c:608)
==12046== 
==12046== Conditional jump or move depends on uninitialised value(s)
==12046==    at 0x427312: tlv_n1_eq (run-tlvstream.c:512)
==12046==    by 0x427A3B: main (run-tlvstream.c:619)
==12046==  Uninitialised value was created by a heap allocation
==12046==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12046==    by 0x4216E8: allocate (tal.c:245)
==12046==    by 0x421C75: tal_alloc_ (tal.c:423)
==12046==    by 0x42792E: main (run-tlvstream.c:608)
==12046== 
==12046== Conditional jump or move depends on uninitialised value(s)
==12046==    at 0x42737C: tlv_n1_eq (run-tlvstream.c:520)
==12046==    by 0x427A3B: main (run-tlvstream.c:619)
==12046==  Uninitialised value was created by a heap allocation
==12046==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12046==    by 0x4216E8: allocate (tal.c:245)
==12046==    by 0x421C75: tal_alloc_ (tal.c:423)
==12046==    by 0x42792E: main (run-tlvstream.c:608)
==12046== 
==12046== Conditional jump or move depends on uninitialised value(s)
==12046==    at 0x427432: tlv_n1_eq (run-tlvstream.c:532)
==12046==    by 0x427A3B: main (run-tlvstream.c:619)
==12046==  Uninitialised value was created by a heap allocation
==12046==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==12046==    by 0x4216E8: allocate (tal.c:245)
==12046==    by 0x421C75: tal_alloc_ (tal.c:423)
==12046==    by 0x42792E: main (run-tlvstream.c:608)
==12046== 

Loading

void *record)
{
u64 prev_type;
bool first = true;
Copy link
Collaborator

@niftynei niftynei Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may be a bit of a dumb question, but is there not a 'null' value of prev_type that can serve the same function as this first flag?

Loading

Copy link
Contributor Author

@rustyrussell rustyrussell Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered the same thing, but the safe answer is "no". There's no ban on a zero TLV type, nor on 0xfffffff....

Loading

@rustyrussell
Copy link
Contributor Author

@rustyrussell rustyrussell commented Jul 10, 2019

lgtm zap (other than the valgrind errors)

==12046== Conditional jump or move depends on uninitialised value(s)
==12046==    at 0x4272B6: tlv_n1_eq (run-tlvstream.c:504)
==12046==    by 0x427A3B: main (run-tlvstream.c:619)
==12046==  Uninitialised value was created by a heap allocation

Actual defect: we need to initialize all the pointers in the tlv structures. There are fancier ways to do this (have fromwire_tlvs know the offset inside the struct of which ptr corresponds to which TLV record, or even go full typesafe and instead of the struct make it a varargs function which takes tuples...).

But creating a constructor which uses talz is simpler :)

Loading

@rustyrussell
Copy link
Contributor Author

@rustyrussell rustyrussell commented Jul 11, 2019

Ok, I have an updated version which meets the latest spec PR (in particular, endianness of CompactInt). Will push when I get home...

Loading

@rustyrussell rustyrussell requested a review from cdecker as a code owner Jul 11, 2019
The new TLV spec uses BigSize, like Bitcoin's CompactInt but
*little-endian*.  So change our name for clarity, and insist that
decoding be minimal as the spec requires.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
lightning/bolts#631

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Based on:
	lightning/bolts#640

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

@rustyrussell rustyrussell commented Jul 18, 2019

Since this code isn't currently used, I'm committing even though spec isn't final. Nice to have this infra in-tree since everything else will depend on it. Rebased and fixed check-source warnings (BOLT quotes are not in master yet!)

Ack 2a0ad5a

Loading

@rustyrussell rustyrussell merged commit e737fe7 into ElementsProject:master Jul 18, 2019
2 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants