Expose some useful tlv macros #1551

mariocynicys · 2022-06-19T12:06:19Z

This exposes the 2 macros impl_writeable_msg! & impl_writeable! and any macros/structs called/used inside them.
Also all the imports (uses) in the macros are prefixed with $crate (essentially, making them absolute paths) so the macro works fine with any rust edition (2015 and others).

I have added some docs for the exported macros & pubed structs based on my understanding of them.

Let me know if I should expose more/less macros or edit docs.

Addressing #1539

Some structs had to become public so the macros can be used when exposed This change doesn't add docs and works around the missing docs by adding `msgs_macros` feature

…th any rust edition Previously, imports or usage of structs from different modules/submodules were using the `::module` syntax, which wouldn't work as expected if the macros are used in a rust edition > 2015 Mainly, the two macros `impl_writeable` and `impl_writeable_msg` are now exported and any other macro that was used inside of them. This also exposes some structs that the macros *might* construct for some macro arguments. This also adds some docs to the exposed structs which are missing docs + remove the `msgs_macros` feature which was used to workaround missing docs, though docs for the macros have not been added yet.

See: https://github.com/lightning/bolts/blob/master/01-messaging.md#requirements

I wrote these comments before while misunderstanding what these macros do. Updated them.

codecov-commenter · 2022-06-19T12:29:02Z

Codecov Report

Merging #1551 (3cab634) into main (c180ddd) will decrease coverage by 0.01%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##             main    #1551      +/-   ##
==========================================
- Coverage   90.96%   90.94%   -0.02%     
==========================================
  Files          80       80              
  Lines       43533    43610      +77     
  Branches    43533    43610      +77     
==========================================
+ Hits        39599    39661      +62     
- Misses       3934     3949      +15

Impacted Files	Coverage Δ
lightning/src/ln/msgs.rs	`86.81% <ø> (ø)`
lightning/src/util/ser.rs	`91.28% <ø> (ø)`
lightning/src/util/ser_macros.rs	`89.13% <95.65%> (ø)`
lightning-net-tokio/src/lib.rs	`76.85% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.94% <0.00%> (-0.22%)`	⬇️
lightning/src/ln/payment_tests.rs	`99.06% <0.00%> (-0.19%)`	⬇️
lightning/src/routing/scoring.rs	`97.08% <0.00%> (ø)`
lightning/src/ln/channelmanager.rs	`84.38% <0.00%> (+0.04%)`	⬆️
lightning/src/chain/package.rs	`93.04% <0.00%> (+0.17%)`	⬆️
lightning/src/routing/gossip.rs	`91.94% <0.00%> (+0.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c180ddd...3cab634. Read the comment docs.

…`use lightning::*;` when using them

0 > u64 is redundant because a u64 can't be less than zero

TheBlueMatt

In general we should be as conservative as possible in what we export and what we tell users to use, as exporting things is a commitment to users to support it, and I don't think we want to support all of these macros individually. In general, I'm not sure if we want to expose impl_writeable_tlv_based either, vs just exposing the tlv-stream write/read macros and letting users figure it out? Do you have a specific usecase in mind?

TheBlueMatt · 2022-06-21T20:59:56Z

lightning/src/util/ser_macros.rs

@@ -353,12 +395,14 @@ macro_rules! init_tlv_based_struct_field {
 	};
 }

+/// Initializes the variables we are going to read the TLVs into.
+#[macro_export]


We should prefix this with a _ and make the docs just say something about not using this directly.

TheBlueMatt · 2022-06-21T21:00:33Z

lightning/src/util/ser_macros.rs

@@ -254,10 +293,13 @@ macro_rules! impl_writeable_msg {
 	}
 }

+/// Implements Readable/Writeable for a struct. Note that this macro doesn't handle messages
+/// containing TLV records. If your message contains TLVs, use `impl_writeable_msg!` instead.
+#[macro_export]


I'd rather not and just expose the TLV stuff? New structs that want to implement writeable should do so as a tlv.

hmmm, why should they do it as tlv??
A custom message (types 32768-65535) may have a non-tlv fields.

I mean if users want to implement their own specific serialization they can do that, these macros don't really change much in terms of how much work that is. TLV streams, on the other hand, are very lightning specific and specific in requirements, so exposing that for users is more important IMO. In general, though, we are moving away from the "just write the fields" serialization and towards "TLV stream everything", so I'd prefer to just expose the future, not the past :)

TheBlueMatt · 2022-06-21T21:01:11Z

lightning/src/util/ser_macros.rs

+/// 	(3, street_number, option),
+/// });
+/// ```
+#[macro_export]


Same here, lets just expose impl_writeable_tlv_based rather than this.

I don't have a specific use case for impl_writeable_tlv_based in mind. I didn't think someone would generally try to craft a custom message that is tlv only. I don't agree with exposing it though.

But for impl_writeable_msg, I think this is more of a universal macro (encodes both tlv & non-tlv message fields).
Most probably, this is the one a custom message maker would try to use.

See a use case for impl_writeable_msg here.

I think we should also make it so impl_writeable_msg can handle required, default, and vec_type tlv types.

I really don't want to get into the business of providing downstream users a general-purpose serialization framework. There are probably better options if that's what users want. I'm okay with exposing lightning-specific stuff like TLV streams, but I'm not super happy with downstream users using our serialization stuff for their own objects.

Fine.
So what macros should be exposed??
I'm thinking {decode,encoder}_tlv_stream (for non varint prefixed uses) and {read,write}_tlv_fields (for varint prefixed uses).

Actually maybe lets skip write_tlv_fields? Its really just writing a tlv stream with a length prefix, which isn't a "lightning thing" so much as a "LDK thing". In general, I'd really like to see a user who wants to use this stuff before we start exposing willy-nilly, but writing a TLV stream is understandable enough that I think we should just do it.

TheBlueMatt · 2022-06-21T21:01:30Z

lightning/src/util/ser_macros.rs

@@ -146,15 +158,17 @@ macro_rules! check_missing_tlv {
 	}};
 }

+/// Implements deserialization for a single TLV record.
+#[macro_export]


Lets prefix this with a _ and have the docs just say that you shouldnt use this.

TheBlueMatt · 2022-06-21T21:01:40Z

lightning/src/util/ser_macros.rs

@@ -120,16 +130,18 @@ macro_rules! check_tlv_order {
 	}};
 }

+/// Errors if there are missing required TLV types after the last seen type.
+#[macro_export]


Same, _ and dont use.

TheBlueMatt · 2022-06-21T21:01:47Z

lightning/src/util/ser_macros.rs

@@ -94,16 +102,18 @@ macro_rules! encode_varint_length_prefixed_tlv {
 	} }
 }

+/// Errors if there are missing required TLV types between the last seen type and the type currently being processed.
+#[macro_export]


Same, _ and dont use.

tnull · 2022-09-29T09:07:52Z

@meryacine Are you still interested in continuing this PR? Would be great to have the serialization macros exported to be used downstream. No worries if not, in this case I'd consider opening a new PR to the same effect.

mariocynicys · 2022-10-04T06:40:01Z

@tnull Yeah sure. I ended up copying what I needed from these macros and modified them a bit that's why the PR got staled.

Would be great to have the serialization macros exported to be used downstream.

Are we talking about {decode,encoder}_tlv_stream here?

tnull · 2022-10-04T08:26:04Z

Are we talking about {decode,encoder}_tlv_stream here?

Well, I'm currently implementing serialization manually, so the macros would mostly improve brevity and readability. It would therefore be great to have impl_writeable_tlv_based. As it is however dependent on quite a few other macros, we'd probably need to expose them too. We should however as @TheBlueMatt mentioned take some precautions, i.e., mark them as internal with an _ prefix and document their usage.

tnull · 2022-10-26T14:12:30Z

This unfortunately needs a rebase now.

tnull · 2022-11-03T12:59:10Z

Closing as superseded by #1823.

mariocynicys added 6 commits June 16, 2022 00:41

Exposing minimal macro set

a3cec7b

Some structs had to become public so the macros can be used when exposed This change doesn't add docs and works around the missing docs by adding `msgs_macros` feature

TLV types MUST be strictly increasing (< and not <=)

5c8807d

See: https://github.com/lightning/bolts/blob/master/01-messaging.md#requirements

Fix a typo

319de94

Add docs for exposed (de)serialization macros

402aeb4

Fix some macro docs

4bab16e

I wrote these comments before while misunderstanding what these macros do. Updated them.

mariocynicys added 2 commits June 20, 2022 09:39

Prefix usage of exported macros with $crate so users don't have to …

594c101

…`use lightning::*;` when using them

These comparisons are always false

3cab634

0 > u64 is redundant because a u64 can't be less than zero

TheBlueMatt reviewed Jun 21, 2022

View reviewed changes

tnull mentioned this pull request Sep 27, 2022

Initial event handling implementation lightningdevkit/ldk-node#10

Merged

tnull mentioned this pull request Sep 29, 2022

Re-use rust-lightning's serialization macros lightningdevkit/ldk-node#23

Closed

mariocynicys mentioned this pull request Nov 2, 2022

Expose impl_writeable_tlv_based macro #1823

Merged

tnull closed this Nov 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose some useful tlv macros #1551

Expose some useful tlv macros #1551

mariocynicys commented Jun 19, 2022 •

edited

Loading

codecov-commenter commented Jun 19, 2022 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt Jun 21, 2022

TheBlueMatt Jun 21, 2022

mariocynicys Jun 22, 2022

TheBlueMatt Jun 22, 2022

TheBlueMatt Jun 21, 2022

mariocynicys Jun 22, 2022

TheBlueMatt Jun 22, 2022

mariocynicys Jun 23, 2022

TheBlueMatt Jun 24, 2022

TheBlueMatt Jun 21, 2022

TheBlueMatt Jun 21, 2022

TheBlueMatt Jun 21, 2022

tnull commented Sep 29, 2022 •

edited

Loading

mariocynicys commented Oct 4, 2022

tnull commented Oct 4, 2022 •

edited

Loading

tnull commented Oct 26, 2022 •

edited

Loading

tnull commented Nov 3, 2022

Expose some useful tlv macros #1551

Expose some useful tlv macros #1551

Conversation

mariocynicys commented Jun 19, 2022 • edited Loading

codecov-commenter commented Jun 19, 2022 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Sep 29, 2022 • edited Loading

mariocynicys commented Oct 4, 2022

tnull commented Oct 4, 2022 • edited Loading

tnull commented Oct 26, 2022 • edited Loading

tnull commented Nov 3, 2022

mariocynicys commented Jun 19, 2022 •

edited

Loading

codecov-commenter commented Jun 19, 2022 •

edited

Loading

tnull commented Sep 29, 2022 •

edited

Loading

tnull commented Oct 4, 2022 •

edited

Loading

tnull commented Oct 26, 2022 •

edited

Loading