Expose `impl_writeable_tlv_based` macro #1823

mariocynicys · 2022-11-02T13:03:28Z

Every exported macro needed to have all the macros used inside it:
1- be exported as well.
2- be called from the $crate namespace so it works in other crates.

Some structs in lightning::util::ser needed to be made public as they were used inside the exported macros.

Usage:

lightning::impl_writeable_tlv_based!(...)

Couldn't cleanly rebase the old PR so here is this one instead.
cc/ @tnull
Closes #1539

codecov-commenter · 2022-11-02T16:52:16Z

Codecov Report

Base: 90.71% // Head: 90.72% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (266a95d) compared to base (b79ff71).
Patch coverage: 96.72% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1823   +/-   ##
=======================================
  Coverage   90.71%   90.72%           
=======================================
  Files          96       96           
  Lines       50113    50134   +21     
  Branches    50113    50134   +21     
=======================================
+ Hits        45459    45482   +23     
+ Misses       4654     4652    -2

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`86.66% <ø> (ø)`
lightning/src/util/chacha20poly1305rfc.rs	`96.34% <ø> (ø)`
lightning/src/util/ser.rs	`91.84% <ø> (ø)`
lightning/src/util/ser_macros.rs	`87.03% <91.66%> (+0.04%)`	⬆️
lightning/src/ln/msgs.rs	`86.02% <100.00%> (ø)`
lightning/src/onion_message/packet.rs	`76.03% <100.00%> (ø)`
lightning/src/routing/gossip.rs	`92.13% <100.00%> (ø)`
lightning/src/util/events.rs	`29.35% <0.00%> (-0.23%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.88% <0.00%> (-0.10%)`	⬇️
lightning/src/offers/parse.rs	`94.00% <0.00%> (ø)`
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Basically fine, needs way more details in the docs, though. If you prefer to not write those just let me know and I can suggest concrete changes.

TheBlueMatt · 2022-11-02T23:05:23Z

lightning/src/util/ser_macros.rs

-// If it is provided, it will be called with the custom type and the `FixedLengthReader` containing
-// the message contents. It should return `Ok(true)` if the custom message is successfully parsed,
-// `Ok(false)` if the message type is unknown, and `Err(DecodeError)` if parsing fails.
+/// Implements the TLVs deserialization part in a Readable implementation of a struct.


Hmm, this is confusing, it can be used anywhere, it doesn't have to be in a Readable implementation - it just happens to return Err(DecodeError) so it has to be in a function which does so.

Right, I wrote this doc based on how the macro was used. But yeah this should be more generic.

TheBlueMatt · 2022-11-02T23:06:08Z

lightning/src/util/ser_macros.rs

+/// `$decode_custom_tlv` is a closure that may be optionally provided to handle custom message types.
+/// If it is provided, it will be called with the custom type and the `FixedLengthReader` containing
+/// the message contents. It should return `Ok(true)` if the custom message is successfully parsed,
+/// `Ok(false)` if the message type is unknown, and `Err(DecodeError)` if parsing fails.


This should have an example of how to call it if we're gonna make it for public consumption, including what the option, required, etc types mean and how to use them (we can leave off some of the more complicated ones, preferring to just document ones we want to support).

Also needs to note that it will always fully read the stream provided to the end.

TheBlueMatt · 2022-11-02T23:07:03Z

lightning/src/util/ser_macros.rs

@@ -351,17 +379,21 @@ macro_rules! read_ver_prefix {
 	} }
 }

-/// Reads a suffix added by write_tlv_fields.
+/// Reads a suffix added by write_tlv_fields!().


I dunno if we need to expose the lenght-prefixed versions of the macros.

Aren't {write,read}_tlv_fields length-prefixed already?

TheBlueMatt · 2022-11-02T23:08:52Z

lightning/src/util/ser_macros.rs

@@ -28,6 +34,8 @@ macro_rules! encode_tlv {
 	};
 }

+/// Implements the TLVs serialization part in a Writeable implementation of a struct.


Same comments, basically, apply here - we need to describe what this does, when users should want to use it, and what they need to do to use it, including examples of how to call it with the behavior of the various types spelled out.

TheBlueMatt · 2022-11-02T23:09:58Z

lightning/src/util/ser_macros.rs

@@ -396,11 +431,12 @@ macro_rules! init_tlv_field_var {
 /// If $fieldty is `option`, then $field is optional field.
 /// if $fieldty is `vec_type`, then $field is a Vec, which needs to have its individual elements
 /// serialized.
+#[macro_export]


Hmm, do you have an immediate need for this? If not we can skip it, but if so the same comments above apply, basically, needs way more docs.

Fair enough. It'd be nice to add an example for the docs here.

tnull · 2022-11-03T12:53:16Z

lightning/src/util/ser_macros.rs

@@ -28,6 +34,8 @@ macro_rules! encode_tlv {
 	};
 }

+/// Implements the TLVs serialization part in a Writeable implementation of a struct.


Suggested change

/// Implements the TLVs serialization part in a Writeable implementation of a struct.

/// Implements the TLVs serialization part in a [`Writeable`] implementation of a struct.

tnull · 2022-11-03T12:53:33Z

lightning/src/util/ser_macros.rs

 				}
 				last_seen = Some($type);
 			)*
 		}
 	} }
 }

-macro_rules! get_varint_length_prefixed_tlv_length {
+/// Adds the length of the serialized field to a LengthCalculatingWriter.


Suggested change

/// Adds the length of the serialized field to a LengthCalculatingWriter.

/// Adds the length of the serialized field to a [`LengthCalculatingWriter`].

tnull · 2022-11-03T12:53:57Z

lightning/src/util/ser_macros.rs

@@ -78,32 +90,38 @@ macro_rules! get_varint_length_prefixed_tlv_length {
 	};
 }

-macro_rules! encode_varint_length_prefixed_tlv {
+/// See the documentation of write_tlv_fields!().


Suggested change

/// See the documentation of write_tlv_fields!().

/// See the documentation of [`write_tlv_fields`].

tnull · 2022-11-03T12:54:45Z

lightning/src/util/ser_macros.rs

+/// Implements the TLVs deserialization part in a Readable implementation of a struct.
+///
+/// `$decode_custom_tlv` is a closure that may be optionally provided to handle custom message types.
+/// If it is provided, it will be called with the custom type and the `FixedLengthReader` containing


Suggested change

/// If it is provided, it will be called with the custom type and the `FixedLengthReader` containing

/// If it is provided, it will be called with the custom type and the [`FixedLengthReader`] containing

mariocynicys · 2022-11-03T17:42:53Z

@TheBlueMatt

If you prefer to not write those just let me know and I can suggest concrete changes.

Yes please, I spend hours overthinking what would be the best way to write a comment 😞

TheBlueMatt · 2022-11-11T02:02:48Z

For the _* macros, I think we can #[doc(hidden)] them so that they dont show up in auto-generated docs at all. For decode_tlv_stream let's split the $deode_custom_tlv argument-version out into a new macro and only export one without that (for now). Draft docs are

/// Implements the TLVs deserialization part in a Readable implementation of a struct.
///
/// This should be called inside a method which returns `Result<_, `[`DecodeError`]`>`, such as
/// [`Readable::read`]. It will either return an `Err` or ensure all `required` fields have been
/// read and optionally read `optional` fields.
///
/// `$stream` must be a [`Read`] and will be fully consumed, reading until no more bytes remain
/// (i.e. it returns [`DecodeError::ShortRead`]).
///
/// Fields MUST be sorted in `$type`-order.
///
/// Note that the lightning TLV requirements require that a single type not appear more than once,
/// that TLVs are sorted in type-ascending order, and that any even types be understood by the
/// decoder.
///
/// For example,
/// ```
/// # use lightning::decode_tlv_stream;
/// # fn read<R: lightning::io::Read> (stream: R) -> Result<(), lightning::ln::msgs::DecodeError> {
/// let mut required_value = 0u64;
/// let mut optional_value: Option<u64> = None;
/// decode_tlv_stream!(stream, {
///     (0, required_value, required),
///     (2, optional_value, option),
/// });
/// // At this point, `required_value` has been overwritten with the TLV with type 0.
/// // `optional_value` may have been overwritten, setting it to `Some` if a TLV with type 2 was
/// // present.
/// # Ok(())
/// # }
/// ```
///
/// [`DecodeError`]: crate::ln::msgs::DecodeError
/// [`Readable::read`]: crate::util::ser::Readable::read
/// [`Read`]: crate::io::Read
/// [`DecodeError::ShortRead`]: crate::ln::msgs::DecodeError::ShortRead

For encode_tlv_stream, draft docs maybe are:

/// Implements the TLVs serialization part in a Writeable implementation of a struct.
///
/// This should be called inside a method which returns `Result<_, `[`io::Error`]`>`, such as
/// [`Writeable::write`]. It will only return an `Err` if the stream `Err`s or [`Writeable::write`]
/// on one of the fields `Err`s.
///
/// `$stream` must be a `&mut `[`Writer`] which will receive the bytes for each TLV in the stream.
///
/// Fields MUST be sorted in `$type`-order.
///
/// Note that the lightning TLV requirements require that a single type not appear more than once,
/// that TLVs are sorted in type-ascending order, and that any even types be understood by the
/// decoder.
///
/// Any `option` fields which have a value of `None` will not be serialized at all.
///
/// For example,
/// ```
/// # use lightning::encode_tlv_stream;
/// # fn write<W: lightning::util::ser::Writer> (stream: &mut W) -> Result<(), lightning::io::Error> {
/// let mut required_value = 0u64;
/// let mut optional_value: Option<u64> = None;
/// encode_tlv_stream!(stream, {
///     (0, required_value, required),
///     (1, Some(42u64), option),
///     (2, optional_value, option),
/// });
/// // At this point `required_value` has been written as a TLV of type 0, `42u64` has been written
/// // as a TLV of type 1 (indicating the reader may ignore it if it is not understood), and *no*
/// // TLV is written with type 2.
/// # Ok(())
/// # }
/// ```
///
/// [`io::Error`]: crate::io::Error
/// [`Writeable::write`]: crate::util::ser::Writeable::write
/// [`Writer`]: crate::util::ser::Writer

TheBlueMatt · 2022-11-11T02:03:04Z

Sadly we've had a number of PRs that touched serialization macros so this needs rebase now :(

tnull

Generally looks good, just a number of doc nits.

tnull · 2022-12-05T10:03:56Z

lightning/src/ln/msgs.rs

@@ -822,7 +822,7 @@ pub struct CommitmentUpdate {

 /// Messages could have optional fields to use with extended features
 /// As we wish to serialize these differently from Option<T>s (Options get a tag byte, but


Suggested change

/// As we wish to serialize these differently from Option<T>s (Options get a tag byte, but

/// As we wish to serialize these differently from `Option<T>`s (`Option`s get a tag byte, but

tnull · 2022-12-05T10:04:21Z

lightning/src/ln/msgs.rs

@@ -822,7 +822,7 @@ pub struct CommitmentUpdate {

 /// Messages could have optional fields to use with extended features
 /// As we wish to serialize these differently from Option<T>s (Options get a tag byte, but
-/// OptionalFeild simply gets Present if there are enough bytes to read into it), we have a
+/// OptionalField simply gets Present if there are enough bytes to read into it), we have a


Suggested change

/// OptionalField simply gets Present if there are enough bytes to read into it), we have a

/// [`OptionalField`] simply gets `Present` if there are enough bytes to read into it), we have a

tnull · 2022-12-05T10:05:05Z

lightning/src/util/ser.rs

 	read: R,
 	bytes_read: u64,
 	total_bytes: u64,
 }
 impl<R: Read> FixedLengthReader<R> {
+    /// Returns a new FixedLengthReader.


Suggested change

/// Returns a new FixedLengthReader.

/// Returns a new [`FixedLengthReader`].

tnull · 2022-12-05T10:05:18Z

lightning/src/util/ser.rs

@@ -94,21 +94,24 @@ impl Writer for LengthCalculatingWriter {

 /// Essentially std::io::Take but a bit simpler and with a method to walk the underlying stream


Suggested change

/// Essentially std::io::Take but a bit simpler and with a method to walk the underlying stream

/// Essentially [`std::io::Take`] but a bit simpler and with a method to walk the underlying stream

tnull · 2022-12-05T10:05:59Z

lightning/src/util/ser.rs

@@ -146,11 +149,13 @@ impl<R: Read> LengthRead for FixedLengthReader<R> {

 /// A Read which tracks whether any bytes have been read at all. This allows us to distinguish


Suggested change

/// A Read which tracks whether any bytes have been read at all. This allows us to distinguish

/// A [`Read`] implementation which tracks whether any bytes have been read at all. This allows us to distinguish

tnull · 2022-12-05T10:06:28Z

lightning/src/util/ser.rs

 	pub have_read: bool,
 }
 impl<R: Read> ReadTrackingReader<R> {
+    /// Returns a new ReadTrackingReader.


Suggested change

/// Returns a new ReadTrackingReader.

/// Returns a new [`ReadTrackingReader`].

tnull · 2022-12-05T10:06:58Z

lightning/src/util/ser_macros.rs

@@ -7,17 +7,24 @@
 // You may not use this file except in accordance with one or both of these
 // licenses.

-macro_rules! encode_tlv {
+//! Some macros that implement Readable/Writeable traits for lightning messages.


Suggested change

//! Some macros that implement Readable/Writeable traits for lightning messages.

//! Some macros that implement [`Readable`]/[`Writeable`] traits for lightning messages.

tnull · 2022-12-05T10:14:53Z

lightning/src/util/ser_macros.rs

@@ -254,9 +376,9 @@ macro_rules! decode_tlv_stream_range {
 				},
 				_ => {},
 			}
-			// As we read types, make sure we hit every required type:
+			// As we read types, make sure we hit every required type between last_seen_type and typ:


Suggested change

// As we read types, make sure we hit every required type between last_seen_type and typ:

// As we read types, make sure we hit every required type `between last_seen_type` and `typ`:

tnull · 2022-12-05T10:15:22Z

lightning/src/util/ser_macros.rs

@@ -350,12 +472,14 @@ macro_rules! impl_writeable {
 ///               object.
 /// $min_version_that_can_read_this is the minimum reader version which can understand this
 ///                                 serialized object. Previous versions will simply err with a
-///                                 DecodeError::UnknownVersion.
+///                                 [`DecodeError::UnknownVersion`].
 ///
 /// Updates to either $this_version or $min_version_that_can_read_this should be included in


Suggested change

/// Updates to either $this_version or $min_version_that_can_read_this should be included in

/// Updates to either `$this_version` or `$min_version_that_can_read_this` should be included in

tnull · 2022-12-05T10:15:47Z

lightning/src/util/ser_macros.rs

 /// serialization logic for this object. This is compared against the
-/// $min_version_that_can_read_this added by write_ver_prefix!().
+/// $min_version_that_can_read_this added by [`write_ver_prefix`].


Suggested change

/// $min_version_that_can_read_this added by [`write_ver_prefix`].

/// `$min_version_that_can_read_this` added by [`write_ver_prefix`].

TheBlueMatt · 2023-01-05T23:31:34Z

Looks like this needs a trivial rebase, any desire to pick it back up this year?

TheBlueMatt

This looks basically good to go from my side, can you squash the fixup commits so that the PR is only a few freestanding commits where later commits don't fix things done in previous commits. https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits is a good guide for it and we generally follow Bitcoin Core contributing guidelines on git.

TheBlueMatt · 2023-01-07T18:14:02Z

lightning/src/util/ser.rs

 	read: R,
 	bytes_read: u64,
 	total_bytes: u64,
 }
 impl<R: Read> FixedLengthReader<R> {
+    /// Returns a new [`FixedLengthReader`].


nit: here and the two changes below - tabs not spaces :)

TheBlueMatt · 2023-01-07T19:09:36Z

lightning/src/util/ser_macros.rs

-macro_rules! check_encoded_tlv_order {
+/// Panics if the last seen TLV type is not numerically less than the TLV type currently being checked.
+/// This is exported for use by other exported macros, do not use directly.
+#[macro_export]


There's a number of the dont-use macros that are missing #[doc(hidden)].

TheBlueMatt · 2023-01-07T19:15:32Z

lightning/src/util/ser_macros.rs

 ///
 /// This is the preferred method of adding new fields that old nodes can ignore and still function
 /// correctly.
+///
+/// [`DecodeError::UnknownRequiredFeature`]: crate::ln::msgs::DecodeError::UnknownRequiredFeature
+#[macro_export]
 macro_rules! write_tlv_fields {


We should mention that this "writes out a length-prefixed TLV stream"

TheBlueMatt · 2023-01-07T19:39:02Z

lightning/src/util/ser_macros.rs

@@ -396,11 +431,12 @@ macro_rules! init_tlv_field_var {
 /// If $fieldty is `option`, then $field is optional field.
 /// if $fieldty is `vec_type`, then $field is a Vec, which needs to have its individual elements
 /// serialized.
+#[macro_export]


Fair enough. It'd be nice to add an example for the docs here.

mariocynicys · 2023-01-08T17:18:57Z

I don't really know why GitHub auto removes one review request when I add another :(.
Would like both your feedback @tnull @TheBlueMatt .

tnull

LGTM, feel free to squash fix-up commits.

Every exported macro needed to have all the macros used inside it: 1- to be exported as well. 2- be called from the `$crate` namespace so it works in other crates. Some structs in `lightning::util::ser` needed to be made public as they were used inside the exported macros. Use the macros like this: ```Rust lightning::impl_writeable_tlv_based!(...) ```

Types must be unique and monotonically increasing (using < instead of <=)

TheBlueMatt reviewed Nov 2, 2022

View reviewed changes

tnull reviewed Nov 3, 2022

View reviewed changes

tnull mentioned this pull request Nov 3, 2022

Expose some useful tlv macros #1551

Closed

mariocynicys force-pushed the expose-tlv-macros2 branch 2 times, most recently from b5bdfa9 to 32b419c Compare December 4, 2022 20:12

mariocynicys requested review from tnull and TheBlueMatt and removed request for tnull and TheBlueMatt December 4, 2022 20:19

tnull reviewed Dec 5, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Jan 5, 2023

Persist payment info to disk lightningdevkit/ldk-sample#90

Closed

mariocynicys force-pushed the expose-tlv-macros2 branch 2 times, most recently from 1f1e432 to 6e3ddac Compare January 6, 2023 09:10

TheBlueMatt reviewed Jan 7, 2023

View reviewed changes

mariocynicys requested review from tnull and TheBlueMatt and removed request for tnull January 8, 2023 17:15

tnull reviewed Jan 9, 2023

View reviewed changes

mariocynicys added 2 commits January 9, 2023 21:16

Fix an incorrect assertion in tlv stream encoding

0acd5d3

Types must be unique and monotonically increasing (using < instead of <=)

mariocynicys force-pushed the expose-tlv-macros2 branch from 266a95d to 0acd5d3 Compare January 9, 2023 19:24

tnull approved these changes Jan 11, 2023

View reviewed changes

TheBlueMatt approved these changes Jan 11, 2023

View reviewed changes

TheBlueMatt merged commit e8b91a4 into lightningdevkit:main Jan 11, 2023

jkczyz mentioned this pull request Jan 18, 2023

BOLT 12 invoice encoding and building #1926

Merged

	/// Implements the TLVs serialization part in a Writeable implementation of a struct.
	/// Implements the TLVs serialization part in a [`Writeable`] implementation of a struct.

	/// Adds the length of the serialized field to a LengthCalculatingWriter.
	/// Adds the length of the serialized field to a [`LengthCalculatingWriter`].

	/// See the documentation of write_tlv_fields!().
	/// See the documentation of [`write_tlv_fields`].

	/// If it is provided, it will be called with the custom type and the `FixedLengthReader` containing
	/// If it is provided, it will be called with the custom type and the [`FixedLengthReader`] containing

		@@ -822,7 +822,7 @@ pub struct CommitmentUpdate {

		/// Messages could have optional fields to use with extended features
		/// As we wish to serialize these differently from Option<T>s (Options get a tag byte, but

	/// OptionalField simply gets Present if there are enough bytes to read into it), we have a
	/// [`OptionalField`] simply gets `Present` if there are enough bytes to read into it), we have a

	/// Returns a new FixedLengthReader.
	/// Returns a new [`FixedLengthReader`].

		@@ -94,21 +94,24 @@ impl Writer for LengthCalculatingWriter {

		/// Essentially std::io::Take but a bit simpler and with a method to walk the underlying stream

	/// Essentially std::io::Take but a bit simpler and with a method to walk the underlying stream
	/// Essentially [`std::io::Take`] but a bit simpler and with a method to walk the underlying stream

		@@ -146,11 +149,13 @@ impl<R: Read> LengthRead for FixedLengthReader<R> {

		/// A Read which tracks whether any bytes have been read at all. This allows us to distinguish

	/// A Read which tracks whether any bytes have been read at all. This allows us to distinguish
	/// A [`Read`] implementation which tracks whether any bytes have been read at all. This allows us to distinguish

	/// Returns a new ReadTrackingReader.
	/// Returns a new [`ReadTrackingReader`].

	//! Some macros that implement Readable/Writeable traits for lightning messages.
	//! Some macros that implement [`Readable`]/[`Writeable`] traits for lightning messages.

	// As we read types, make sure we hit every required type between last_seen_type and typ:
	// As we read types, make sure we hit every required type `between last_seen_type` and `typ`:

	/// Updates to either $this_version or $min_version_that_can_read_this should be included in
	/// Updates to either `$this_version` or `$min_version_that_can_read_this` should be included in

	/// $min_version_that_can_read_this added by [`write_ver_prefix`].
	/// `$min_version_that_can_read_this` added by [`write_ver_prefix`].

Expose impl_writeable_tlv_based macro #1823

Expose impl_writeable_tlv_based macro #1823

Conversation

mariocynicys commented Nov 2, 2022 • edited by tnull

codecov-commenter commented Nov 2, 2022 • edited

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

Choose a reason for hiding this comment

mariocynicys commented Nov 3, 2022

TheBlueMatt commented Nov 11, 2022

TheBlueMatt commented Nov 11, 2022

tnull 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

TheBlueMatt commented Jan 5, 2023

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

mariocynicys commented Jan 8, 2023

tnull left a comment

Choose a reason for hiding this comment

Expose `impl_writeable_tlv_based` macro #1823

Expose `impl_writeable_tlv_based` macro #1823

mariocynicys commented Nov 2, 2022 •

edited by tnull

codecov-commenter commented Nov 2, 2022 •

edited