BOLT 12 offer parsing by jkczyz · Pull Request #1726 · lightningdevkit/rust-lightning

jkczyz · 2022-09-16T21:41:23Z

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts.

Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages.

The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Also, implement Writeable for Offer and have a corresponding TryFrom implementation for decoding the raw bytes.

codecov-commenter · 2022-09-16T21:54:27Z

Codecov Report

Base: 90.80% // Head: 91.95% // Increases project coverage by +1.14% 🎉

Coverage data is based on head (e916bf8) compared to base (f1428fd).
Patch coverage: 86.83% of modified lines in pull request are covered.

❗ Current head e916bf8 differs from pull request most recent head 1e26a2b. Consider uploading reports for the commit 1e26a2b to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1726 +/- ##
==========================================
+ Coverage 90.80% 91.95% +1.14% 
==========================================
Files 89 91 +2 Lines 47963 58814 +10851 Branches 47963 58814 +10851 ==========================================
+ Hits 43554 54081 +10527 - Misses 4409 4733 +324

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`91.70% <ø> (-0.10%)`	⬇️
lightning/src/util/ser_macros.rs	`89.15% <53.84%> (+0.74%)`	⬆️
lightning/src/offers/offer.rs	`91.77% <87.38%> (-2.79%)`	⬇️
lightning/src/offers/parse.rs	`93.47% <93.47%> (ø)`
lightning/src/util/events.rs	`34.91% <0.00%> (-2.72%)`	⬇️
lightning/src/chain/mod.rs	`66.66% <0.00%> (-1.52%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/reload_tests.rs	`95.24% <0.00%> (ø)`
... and 24 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.

valentinewallace · 2022-09-26T18:55:02Z

 	}
 }

+impl TryFrom<Vec<u8>> for Offer {


Readable seems more ergonomic as an API?

Readable only supports DecodeError. Could map ParseError to DecodeError::InvalidValue, but I don't think we'd want to swallow the more specific error. Plus, it would be inconsistent with parsing InvoiceRequest and Invoice, where it's helpful to know the error reason.

Forgot to mention but also note that Offer needs to take ownership of the bytes.

valentinewallace · 2022-09-27T15:07:31Z

+	MissingNodeId,
+	/// An empty set of blinded paths was provided.
+	MissingPaths,
+	/// A quantity representing an empty range or that was outside of a valid range was provided.


Bit confusing wording to parse

Rewrote the InvalidQuantity docs. PTAL.

jkczyz · 2022-10-07T21:16:45Z

Updated to parse bech32 strings directly into the higher-level structs instead of an intermediary TLV stream format by adding a TryFrom<Vec<u8>> trait bound to Bech32Encode and dropping the associated type. This is more straightforward and also makes it so that checking that all bytes are read in #1738 only needs to be defined in one place.

jkczyz · 2022-11-10T15:04:37Z

FYI, I refactored commit out of the next PR that limits TLV stream decoding to a type range and moved it here.

TheBlueMatt

Basically LGTM, really.

jkczyz

All good feedback

valentinewallace

Nothing major :)

valentinewallace · 2022-11-11T18:31:00Z

+ }
+
+ let mut builder = OfferBuilder::new("foo".into(), pubkey(42));
+ builder.offer.paths = Some(vec![]);


I don't quite get why this should cause us to fail to parse

I likely inferred it from this part of the spec:
- if it is connected only by private channels: - MUST include `offer_paths` containing one or more paths to the node from publicly reachable nodes. - otherwise: - MAY include `offer_paths`.

But doesn't that imply offer_paths is optional? Only for public nodes, but that's not something we know at decode-time.

True... I guess it is more that we fail if given a paths TLV record with length 0. We are fine parsing it if the paths TLV record doesn't exist. Could remove this check if you prefer.

Hmm, no strong opinion, I worry we'd spuriously fail there encountering those in the wild, though?

Removed the check.

TheBlueMatt · 2022-11-11T21:54:37Z

Feel free to squash IMO.

TheBlueMatt

A few nits, feel free to ignore and land, though.

TheBlueMatt · 2022-11-16T21:18:22Z

+
+	/// Parses a bech32-encoded message into a TLV stream.
+	fn from_bech32_str(s: &str) -> Result<Self, ParseError> {
+ // Offer encoding may be split by '+' followed by optional whitespace.


I'm confused - are offers allowed to have an arbitrary number of +( )*s? Is there some restriction as to where in the offer they can be that we can avoid copying the whole string next just to call bech32::decode? What is the reason they're allowed to have +s anyway?

According to BOLT 12 requirements:
Readers of a bolt12 string:
if it encounters a + followed by zero or more whitespace characters between two bech32 characters:
MUST remove the + and whitespace.
Rationale is for use in text fields with limited size, like Twitter.
BOLT 12 defines some test vectors currently exercised in fails_parsing_bech32_encoded_offers_with_invalid_continuations.
Could probably avoid the copy if bech32 crate worked on an iterator of characters. But currently bech32::decode also returns slices into the string for HRP and data.

Ugh....I wonder if it makes sense to at least have an optimization for cases with no +? Probably more work than it's worth but nice to avoid copying just because.

Done with an enum.

TheBlueMatt · 2022-11-16T21:33:30Z

+
+	/// Formats the message using bech32-encoding.
+	fn fmt_bech32_str(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+ bech32::encode_without_checksum_to_fmt(f, Self::BECH32_HRP, self.as_ref().to_base32())


Grr, its really kinda dumb we're forced to put the intermediate u5s in a vec in between here, but that's upstream's fault - rust-bitcoin/rust-bech32#81

TheBlueMatt · 2022-11-17T22:34:17Z

LGTM, feel free to squash IMO.

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts. Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages. The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Test semantic errors when parsing offer bytes.

BOLT 12 messages are limited to a range of TLV record types. Refactor decode_tlv_stream into a decode_tlv_stream_range macro for limiting which types are parsed. Requires a SeekReadable trait for rewinding when a type outside of the range is seen. This allows for composing TLV streams of different ranges. Updates offer parsing accordingly and adds a test demonstrating failure if a type outside of the range is included.

jkczyz · 2022-09-16T21:41:23Z

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts.

Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages.

The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Also, implement Writeable for Offer and have a corresponding TryFrom implementation for decoding the raw bytes.

codecov-commenter · 2022-09-16T21:54:27Z

Codecov Report

Base: 90.80% // Head: 91.95% // Increases project coverage by +1.14% 🎉

Coverage data is based on head (e916bf8) compared to base (f1428fd).
Patch coverage: 86.83% of modified lines in pull request are covered.

❗ Current head e916bf8 differs from pull request most recent head 1e26a2b. Consider uploading reports for the commit 1e26a2b to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1726 +/- ##
==========================================
+ Coverage 90.80% 91.95% +1.14% 
==========================================
Files 89 91 +2 Lines 47963 58814 +10851 Branches 47963 58814 +10851 ==========================================
+ Hits 43554 54081 +10527 - Misses 4409 4733 +324

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`91.70% <ø> (-0.10%)`	⬇️
lightning/src/util/ser_macros.rs	`89.15% <53.84%> (+0.74%)`	⬆️
lightning/src/offers/offer.rs	`91.77% <87.38%> (-2.79%)`	⬇️
lightning/src/offers/parse.rs	`93.47% <93.47%> (ø)`
lightning/src/util/events.rs	`34.91% <0.00%> (-2.72%)`	⬇️
lightning/src/chain/mod.rs	`66.66% <0.00%> (-1.52%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/reload_tests.rs	`95.24% <0.00%> (ø)`
... and 24 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.

valentinewallace · 2022-09-26T18:55:02Z

 	}
 }

+impl TryFrom<Vec<u8>> for Offer {


Readable seems more ergonomic as an API?

Readable only supports DecodeError. Could map ParseError to DecodeError::InvalidValue, but I don't think we'd want to swallow the more specific error. Plus, it would be inconsistent with parsing InvoiceRequest and Invoice, where it's helpful to know the error reason.

Forgot to mention but also note that Offer needs to take ownership of the bytes.

valentinewallace · 2022-09-27T15:07:31Z

+	MissingNodeId,
+	/// An empty set of blinded paths was provided.
+	MissingPaths,
+	/// A quantity representing an empty range or that was outside of a valid range was provided.


Bit confusing wording to parse

Rewrote the InvalidQuantity docs. PTAL.

jkczyz · 2022-10-07T21:16:45Z

Updated to parse bech32 strings directly into the higher-level structs instead of an intermediary TLV stream format by adding a TryFrom<Vec<u8>> trait bound to Bech32Encode and dropping the associated type. This is more straightforward and also makes it so that checking that all bytes are read in #1738 only needs to be defined in one place.

jkczyz · 2022-11-10T15:04:37Z

FYI, I refactored commit out of the next PR that limits TLV stream decoding to a type range and moved it here.

TheBlueMatt

Basically LGTM, really.

jkczyz

All good feedback

valentinewallace

Nothing major :)

valentinewallace · 2022-11-11T18:31:00Z

+ }
+
+ let mut builder = OfferBuilder::new("foo".into(), pubkey(42));
+ builder.offer.paths = Some(vec![]);


I don't quite get why this should cause us to fail to parse

I likely inferred it from this part of the spec:
- if it is connected only by private channels: - MUST include `offer_paths` containing one or more paths to the node from publicly reachable nodes. - otherwise: - MAY include `offer_paths`.

But doesn't that imply offer_paths is optional? Only for public nodes, but that's not something we know at decode-time.

True... I guess it is more that we fail if given a paths TLV record with length 0. We are fine parsing it if the paths TLV record doesn't exist. Could remove this check if you prefer.

Hmm, no strong opinion, I worry we'd spuriously fail there encountering those in the wild, though?

Removed the check.

TheBlueMatt · 2022-11-11T21:54:37Z

Feel free to squash IMO.

TheBlueMatt

A few nits, feel free to ignore and land, though.

TheBlueMatt · 2022-11-16T21:18:22Z

+
+	/// Parses a bech32-encoded message into a TLV stream.
+	fn from_bech32_str(s: &str) -> Result<Self, ParseError> {
+ // Offer encoding may be split by '+' followed by optional whitespace.


I'm confused - are offers allowed to have an arbitrary number of +( )*s? Is there some restriction as to where in the offer they can be that we can avoid copying the whole string next just to call bech32::decode? What is the reason they're allowed to have +s anyway?

According to BOLT 12 requirements:
Readers of a bolt12 string:
if it encounters a + followed by zero or more whitespace characters between two bech32 characters:
MUST remove the + and whitespace.
Rationale is for use in text fields with limited size, like Twitter.
BOLT 12 defines some test vectors currently exercised in fails_parsing_bech32_encoded_offers_with_invalid_continuations.
Could probably avoid the copy if bech32 crate worked on an iterator of characters. But currently bech32::decode also returns slices into the string for HRP and data.

Ugh....I wonder if it makes sense to at least have an optimization for cases with no +? Probably more work than it's worth but nice to avoid copying just because.

Done with an enum.

TheBlueMatt · 2022-11-16T21:33:30Z

+
+	/// Formats the message using bech32-encoding.
+	fn fmt_bech32_str(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+ bech32::encode_without_checksum_to_fmt(f, Self::BECH32_HRP, self.as_ref().to_base32())


Grr, its really kinda dumb we're forced to put the intermediate u5s in a vec in between here, but that's upstream's fault - rust-bitcoin/rust-bech32#81

TheBlueMatt · 2022-11-17T22:34:17Z

LGTM, feel free to squash IMO.

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts. Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages. The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Test semantic errors when parsing offer bytes.

BOLT 12 messages are limited to a range of TLV record types. Refactor decode_tlv_stream into a decode_tlv_stream_range macro for limiting which types are parsed. Requires a SeekReadable trait for rewinding when a type outside of the range is seen. This allows for composing TLV streams of different ranges. Updates offer parsing accordingly and adds a test demonstrating failure if a type outside of the range is included.

jkczyz · 2022-09-16T21:41:23Z

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts.

Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages.

The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Also, implement Writeable for Offer and have a corresponding TryFrom implementation for decoding the raw bytes.

codecov-commenter · 2022-09-16T21:54:27Z

Codecov Report

Base: 90.80% // Head: 91.95% // Increases project coverage by +1.14% 🎉

Coverage data is based on head (e916bf8) compared to base (f1428fd).
Patch coverage: 86.83% of modified lines in pull request are covered.

❗ Current head e916bf8 differs from pull request most recent head 1e26a2b. Consider uploading reports for the commit 1e26a2b to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1726 +/- ##
==========================================
+ Coverage 90.80% 91.95% +1.14% 
==========================================
Files 89 91 +2 Lines 47963 58814 +10851 Branches 47963 58814 +10851 ==========================================
+ Hits 43554 54081 +10527 - Misses 4409 4733 +324

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`91.70% <ø> (-0.10%)`	⬇️
lightning/src/util/ser_macros.rs	`89.15% <53.84%> (+0.74%)`	⬆️
lightning/src/offers/offer.rs	`91.77% <87.38%> (-2.79%)`	⬇️
lightning/src/offers/parse.rs	`93.47% <93.47%> (ø)`
lightning/src/util/events.rs	`34.91% <0.00%> (-2.72%)`	⬇️
lightning/src/chain/mod.rs	`66.66% <0.00%> (-1.52%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/reload_tests.rs	`95.24% <0.00%> (ø)`
... and 24 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.

valentinewallace · 2022-09-26T18:55:02Z

 	}
 }

+impl TryFrom<Vec<u8>> for Offer {


Readable seems more ergonomic as an API?

Readable only supports DecodeError. Could map ParseError to DecodeError::InvalidValue, but I don't think we'd want to swallow the more specific error. Plus, it would be inconsistent with parsing InvoiceRequest and Invoice, where it's helpful to know the error reason.

Forgot to mention but also note that Offer needs to take ownership of the bytes.

valentinewallace · 2022-09-27T15:07:31Z

+	MissingNodeId,
+	/// An empty set of blinded paths was provided.
+	MissingPaths,
+	/// A quantity representing an empty range or that was outside of a valid range was provided.


Bit confusing wording to parse

Rewrote the InvalidQuantity docs. PTAL.

jkczyz · 2022-10-07T21:16:45Z

Updated to parse bech32 strings directly into the higher-level structs instead of an intermediary TLV stream format by adding a TryFrom<Vec<u8>> trait bound to Bech32Encode and dropping the associated type. This is more straightforward and also makes it so that checking that all bytes are read in #1738 only needs to be defined in one place.

jkczyz · 2022-11-10T15:04:37Z

FYI, I refactored commit out of the next PR that limits TLV stream decoding to a type range and moved it here.

TheBlueMatt

Basically LGTM, really.

jkczyz

All good feedback

valentinewallace

Nothing major :)

valentinewallace · 2022-11-11T18:31:00Z

+ }
+
+ let mut builder = OfferBuilder::new("foo".into(), pubkey(42));
+ builder.offer.paths = Some(vec![]);


I don't quite get why this should cause us to fail to parse

I likely inferred it from this part of the spec:
- if it is connected only by private channels: - MUST include `offer_paths` containing one or more paths to the node from publicly reachable nodes. - otherwise: - MAY include `offer_paths`.

But doesn't that imply offer_paths is optional? Only for public nodes, but that's not something we know at decode-time.

True... I guess it is more that we fail if given a paths TLV record with length 0. We are fine parsing it if the paths TLV record doesn't exist. Could remove this check if you prefer.

Hmm, no strong opinion, I worry we'd spuriously fail there encountering those in the wild, though?

Removed the check.

TheBlueMatt · 2022-11-11T21:54:37Z

Feel free to squash IMO.

TheBlueMatt

A few nits, feel free to ignore and land, though.

TheBlueMatt · 2022-11-16T21:18:22Z

+
+	/// Parses a bech32-encoded message into a TLV stream.
+	fn from_bech32_str(s: &str) -> Result<Self, ParseError> {
+ // Offer encoding may be split by '+' followed by optional whitespace.


I'm confused - are offers allowed to have an arbitrary number of +( )*s? Is there some restriction as to where in the offer they can be that we can avoid copying the whole string next just to call bech32::decode? What is the reason they're allowed to have +s anyway?

According to BOLT 12 requirements:
Readers of a bolt12 string:
if it encounters a + followed by zero or more whitespace characters between two bech32 characters:
MUST remove the + and whitespace.
Rationale is for use in text fields with limited size, like Twitter.
BOLT 12 defines some test vectors currently exercised in fails_parsing_bech32_encoded_offers_with_invalid_continuations.
Could probably avoid the copy if bech32 crate worked on an iterator of characters. But currently bech32::decode also returns slices into the string for HRP and data.

Ugh....I wonder if it makes sense to at least have an optimization for cases with no +? Probably more work than it's worth but nice to avoid copying just because.

Done with an enum.

TheBlueMatt · 2022-11-16T21:33:30Z

+
+	/// Formats the message using bech32-encoding.
+	fn fmt_bech32_str(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+ bech32::encode_without_checksum_to_fmt(f, Self::BECH32_HRP, self.as_ref().to_base32())


Grr, its really kinda dumb we're forced to put the intermediate u5s in a vec in between here, but that's upstream's fault - rust-bitcoin/rust-bech32#81

TheBlueMatt · 2022-11-17T22:34:17Z

LGTM, feel free to squash IMO.

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts. Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages. The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Test semantic errors when parsing offer bytes.

BOLT 12 messages are limited to a range of TLV record types. Refactor decode_tlv_stream into a decode_tlv_stream_range macro for limiting which types are parsed. Requires a SeekReadable trait for rewinding when a type outside of the range is seen. This allows for composing TLV streams of different ranges. Updates offer parsing accordingly and adds a test demonstrating failure if a type outside of the range is included.

jkczyz · 2022-09-16T21:41:23Z

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts.

Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages.

The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Also, implement Writeable for Offer and have a corresponding TryFrom implementation for decoding the raw bytes.

codecov-commenter · 2022-09-16T21:54:27Z

Codecov Report

Base: 90.80% // Head: 91.95% // Increases project coverage by +1.14% 🎉

Coverage data is based on head (e916bf8) compared to base (f1428fd).
Patch coverage: 86.83% of modified lines in pull request are covered.

❗ Current head e916bf8 differs from pull request most recent head 1e26a2b. Consider uploading reports for the commit 1e26a2b to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1726 +/- ##
==========================================
+ Coverage 90.80% 91.95% +1.14% 
==========================================
Files 89 91 +2 Lines 47963 58814 +10851 Branches 47963 58814 +10851 ==========================================
+ Hits 43554 54081 +10527 - Misses 4409 4733 +324

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`91.70% <ø> (-0.10%)`	⬇️
lightning/src/util/ser_macros.rs	`89.15% <53.84%> (+0.74%)`	⬆️
lightning/src/offers/offer.rs	`91.77% <87.38%> (-2.79%)`	⬇️
lightning/src/offers/parse.rs	`93.47% <93.47%> (ø)`
lightning/src/util/events.rs	`34.91% <0.00%> (-2.72%)`	⬇️
lightning/src/chain/mod.rs	`66.66% <0.00%> (-1.52%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/reload_tests.rs	`95.24% <0.00%> (ø)`
... and 24 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.

valentinewallace · 2022-09-26T18:55:02Z

 	}
 }

+impl TryFrom<Vec<u8>> for Offer {


Readable seems more ergonomic as an API?

Readable only supports DecodeError. Could map ParseError to DecodeError::InvalidValue, but I don't think we'd want to swallow the more specific error. Plus, it would be inconsistent with parsing InvoiceRequest and Invoice, where it's helpful to know the error reason.

Forgot to mention but also note that Offer needs to take ownership of the bytes.

valentinewallace · 2022-09-27T15:07:31Z

+	MissingNodeId,
+	/// An empty set of blinded paths was provided.
+	MissingPaths,
+	/// A quantity representing an empty range or that was outside of a valid range was provided.


Bit confusing wording to parse

Rewrote the InvalidQuantity docs. PTAL.

jkczyz · 2022-10-07T21:16:45Z

Updated to parse bech32 strings directly into the higher-level structs instead of an intermediary TLV stream format by adding a TryFrom<Vec<u8>> trait bound to Bech32Encode and dropping the associated type. This is more straightforward and also makes it so that checking that all bytes are read in #1738 only needs to be defined in one place.

jkczyz · 2022-11-10T15:04:37Z

FYI, I refactored commit out of the next PR that limits TLV stream decoding to a type range and moved it here.

TheBlueMatt

Basically LGTM, really.

jkczyz

All good feedback

valentinewallace

Nothing major :)

valentinewallace · 2022-11-11T18:31:00Z

+ }
+
+ let mut builder = OfferBuilder::new("foo".into(), pubkey(42));
+ builder.offer.paths = Some(vec![]);


I don't quite get why this should cause us to fail to parse

I likely inferred it from this part of the spec:
- if it is connected only by private channels: - MUST include `offer_paths` containing one or more paths to the node from publicly reachable nodes. - otherwise: - MAY include `offer_paths`.

But doesn't that imply offer_paths is optional? Only for public nodes, but that's not something we know at decode-time.

True... I guess it is more that we fail if given a paths TLV record with length 0. We are fine parsing it if the paths TLV record doesn't exist. Could remove this check if you prefer.

Hmm, no strong opinion, I worry we'd spuriously fail there encountering those in the wild, though?

Removed the check.

TheBlueMatt · 2022-11-11T21:54:37Z

Feel free to squash IMO.

TheBlueMatt

A few nits, feel free to ignore and land, though.

TheBlueMatt · 2022-11-16T21:18:22Z

+
+	/// Parses a bech32-encoded message into a TLV stream.
+	fn from_bech32_str(s: &str) -> Result<Self, ParseError> {
+ // Offer encoding may be split by '+' followed by optional whitespace.


I'm confused - are offers allowed to have an arbitrary number of +( )*s? Is there some restriction as to where in the offer they can be that we can avoid copying the whole string next just to call bech32::decode? What is the reason they're allowed to have +s anyway?

According to BOLT 12 requirements:
Readers of a bolt12 string:
if it encounters a + followed by zero or more whitespace characters between two bech32 characters:
MUST remove the + and whitespace.
Rationale is for use in text fields with limited size, like Twitter.
BOLT 12 defines some test vectors currently exercised in fails_parsing_bech32_encoded_offers_with_invalid_continuations.
Could probably avoid the copy if bech32 crate worked on an iterator of characters. But currently bech32::decode also returns slices into the string for HRP and data.

Ugh....I wonder if it makes sense to at least have an optimization for cases with no +? Probably more work than it's worth but nice to avoid copying just because.

Done with an enum.

TheBlueMatt · 2022-11-16T21:33:30Z

+
+	/// Formats the message using bech32-encoding.
+	fn fmt_bech32_str(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+ bech32::encode_without_checksum_to_fmt(f, Self::BECH32_HRP, self.as_ref().to_base32())


Grr, its really kinda dumb we're forced to put the intermediate u5s in a vec in between here, but that's upstream's fault - rust-bitcoin/rust-bech32#81

TheBlueMatt · 2022-11-17T22:34:17Z

LGTM, feel free to squash IMO.

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts. Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages. The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Test semantic errors when parsing offer bytes.

BOLT 12 messages are limited to a range of TLV record types. Refactor decode_tlv_stream into a decode_tlv_stream_range macro for limiting which types are parsed. Requires a SeekReadable trait for rewinding when a type outside of the range is seen. This allows for composing TLV streams of different ranges. Updates offer parsing accordingly and adds a test demonstrating failure if a type outside of the range is included.

jkczyz · 2022-09-16T21:41:23Z

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts.

Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages.

The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Also, implement Writeable for Offer and have a corresponding TryFrom implementation for decoding the raw bytes.

codecov-commenter · 2022-09-16T21:54:27Z

Codecov Report

Base: 90.80% // Head: 91.95% // Increases project coverage by +1.14% 🎉

Coverage data is based on head (e916bf8) compared to base (f1428fd).
Patch coverage: 86.83% of modified lines in pull request are covered.

❗ Current head e916bf8 differs from pull request most recent head 1e26a2b. Consider uploading reports for the commit 1e26a2b to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1726 +/- ##
==========================================
+ Coverage 90.80% 91.95% +1.14% 
==========================================
Files 89 91 +2 Lines 47963 58814 +10851 Branches 47963 58814 +10851 ==========================================
+ Hits 43554 54081 +10527 - Misses 4409 4733 +324

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`91.70% <ø> (-0.10%)`	⬇️
lightning/src/util/ser_macros.rs	`89.15% <53.84%> (+0.74%)`	⬆️
lightning/src/offers/offer.rs	`91.77% <87.38%> (-2.79%)`	⬇️
lightning/src/offers/parse.rs	`93.47% <93.47%> (ø)`
lightning/src/util/events.rs	`34.91% <0.00%> (-2.72%)`	⬇️
lightning/src/chain/mod.rs	`66.66% <0.00%> (-1.52%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/reload_tests.rs	`95.24% <0.00%> (ø)`
... and 24 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.

valentinewallace · 2022-09-26T18:55:02Z

 	}
 }

+impl TryFrom<Vec<u8>> for Offer {


Readable seems more ergonomic as an API?

Readable only supports DecodeError. Could map ParseError to DecodeError::InvalidValue, but I don't think we'd want to swallow the more specific error. Plus, it would be inconsistent with parsing InvoiceRequest and Invoice, where it's helpful to know the error reason.

Forgot to mention but also note that Offer needs to take ownership of the bytes.

valentinewallace · 2022-09-27T15:07:31Z

+	MissingNodeId,
+	/// An empty set of blinded paths was provided.
+	MissingPaths,
+	/// A quantity representing an empty range or that was outside of a valid range was provided.


Bit confusing wording to parse

Rewrote the InvalidQuantity docs. PTAL.

jkczyz · 2022-10-07T21:16:45Z

Updated to parse bech32 strings directly into the higher-level structs instead of an intermediary TLV stream format by adding a TryFrom<Vec<u8>> trait bound to Bech32Encode and dropping the associated type. This is more straightforward and also makes it so that checking that all bytes are read in #1738 only needs to be defined in one place.

jkczyz · 2022-11-10T15:04:37Z

FYI, I refactored commit out of the next PR that limits TLV stream decoding to a type range and moved it here.

TheBlueMatt

Basically LGTM, really.

jkczyz

All good feedback

valentinewallace

Nothing major :)

valentinewallace · 2022-11-11T18:31:00Z

+ }
+
+ let mut builder = OfferBuilder::new("foo".into(), pubkey(42));
+ builder.offer.paths = Some(vec![]);


I don't quite get why this should cause us to fail to parse

I likely inferred it from this part of the spec:
- if it is connected only by private channels: - MUST include `offer_paths` containing one or more paths to the node from publicly reachable nodes. - otherwise: - MAY include `offer_paths`.

But doesn't that imply offer_paths is optional? Only for public nodes, but that's not something we know at decode-time.

True... I guess it is more that we fail if given a paths TLV record with length 0. We are fine parsing it if the paths TLV record doesn't exist. Could remove this check if you prefer.

Hmm, no strong opinion, I worry we'd spuriously fail there encountering those in the wild, though?

Removed the check.

TheBlueMatt · 2022-11-11T21:54:37Z

Feel free to squash IMO.

TheBlueMatt

A few nits, feel free to ignore and land, though.

TheBlueMatt · 2022-11-16T21:18:22Z

+
+	/// Parses a bech32-encoded message into a TLV stream.
+	fn from_bech32_str(s: &str) -> Result<Self, ParseError> {
+ // Offer encoding may be split by '+' followed by optional whitespace.


I'm confused - are offers allowed to have an arbitrary number of +( )*s? Is there some restriction as to where in the offer they can be that we can avoid copying the whole string next just to call bech32::decode? What is the reason they're allowed to have +s anyway?

According to BOLT 12 requirements:
Readers of a bolt12 string:
if it encounters a + followed by zero or more whitespace characters between two bech32 characters:
MUST remove the + and whitespace.
Rationale is for use in text fields with limited size, like Twitter.
BOLT 12 defines some test vectors currently exercised in fails_parsing_bech32_encoded_offers_with_invalid_continuations.
Could probably avoid the copy if bech32 crate worked on an iterator of characters. But currently bech32::decode also returns slices into the string for HRP and data.

Ugh....I wonder if it makes sense to at least have an optimization for cases with no +? Probably more work than it's worth but nice to avoid copying just because.

Done with an enum.

TheBlueMatt · 2022-11-16T21:33:30Z

+
+	/// Formats the message using bech32-encoding.
+	fn fmt_bech32_str(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+ bech32::encode_without_checksum_to_fmt(f, Self::BECH32_HRP, self.as_ref().to_base32())


Grr, its really kinda dumb we're forced to put the intermediate u5s in a vec in between here, but that's upstream's fault - rust-bitcoin/rust-bech32#81

TheBlueMatt · 2022-11-17T22:34:17Z

LGTM, feel free to squash IMO.

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts. Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages. The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Test semantic errors when parsing offer bytes.

BOLT 12 messages are limited to a range of TLV record types. Refactor decode_tlv_stream into a decode_tlv_stream_range macro for limiting which types are parsed. Requires a SeekReadable trait for rewinding when a type outside of the range is seen. This allows for composing TLV streams of different ranges. Updates offer parsing accordingly and adds a test demonstrating failure if a type outside of the range is included.

jkczyz · 2022-09-16T21:41:23Z

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts.

Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages.

The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Also, implement Writeable for Offer and have a corresponding TryFrom implementation for decoding the raw bytes.

codecov-commenter · 2022-09-16T21:54:27Z

Codecov Report

Base: 90.80% // Head: 91.95% // Increases project coverage by +1.14% 🎉

Coverage data is based on head (e916bf8) compared to base (f1428fd).
Patch coverage: 86.83% of modified lines in pull request are covered.

❗ Current head e916bf8 differs from pull request most recent head 1e26a2b. Consider uploading reports for the commit 1e26a2b to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1726 +/- ##
==========================================
+ Coverage 90.80% 91.95% +1.14% 
==========================================
Files 89 91 +2 Lines 47963 58814 +10851 Branches 47963 58814 +10851 ==========================================
+ Hits 43554 54081 +10527 - Misses 4409 4733 +324

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`91.70% <ø> (-0.10%)`	⬇️
lightning/src/util/ser_macros.rs	`89.15% <53.84%> (+0.74%)`	⬆️
lightning/src/offers/offer.rs	`91.77% <87.38%> (-2.79%)`	⬇️
lightning/src/offers/parse.rs	`93.47% <93.47%> (ø)`
lightning/src/util/events.rs	`34.91% <0.00%> (-2.72%)`	⬇️
lightning/src/chain/mod.rs	`66.66% <0.00%> (-1.52%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/reload_tests.rs	`95.24% <0.00%> (ø)`
... and 24 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.

valentinewallace · 2022-09-26T18:55:02Z

 	}
 }

+impl TryFrom<Vec<u8>> for Offer {


Readable seems more ergonomic as an API?

Readable only supports DecodeError. Could map ParseError to DecodeError::InvalidValue, but I don't think we'd want to swallow the more specific error. Plus, it would be inconsistent with parsing InvoiceRequest and Invoice, where it's helpful to know the error reason.

Forgot to mention but also note that Offer needs to take ownership of the bytes.

valentinewallace · 2022-09-27T15:07:31Z

+	MissingNodeId,
+	/// An empty set of blinded paths was provided.
+	MissingPaths,
+	/// A quantity representing an empty range or that was outside of a valid range was provided.


Bit confusing wording to parse

Rewrote the InvalidQuantity docs. PTAL.

jkczyz · 2022-10-07T21:16:45Z

Updated to parse bech32 strings directly into the higher-level structs instead of an intermediary TLV stream format by adding a TryFrom<Vec<u8>> trait bound to Bech32Encode and dropping the associated type. This is more straightforward and also makes it so that checking that all bytes are read in #1738 only needs to be defined in one place.

jkczyz · 2022-11-10T15:04:37Z

FYI, I refactored commit out of the next PR that limits TLV stream decoding to a type range and moved it here.

TheBlueMatt

Basically LGTM, really.

jkczyz

All good feedback

valentinewallace

Nothing major :)

valentinewallace · 2022-11-11T18:31:00Z

+ }
+
+ let mut builder = OfferBuilder::new("foo".into(), pubkey(42));
+ builder.offer.paths = Some(vec![]);


I don't quite get why this should cause us to fail to parse

I likely inferred it from this part of the spec:
- if it is connected only by private channels: - MUST include `offer_paths` containing one or more paths to the node from publicly reachable nodes. - otherwise: - MAY include `offer_paths`.

But doesn't that imply offer_paths is optional? Only for public nodes, but that's not something we know at decode-time.

True... I guess it is more that we fail if given a paths TLV record with length 0. We are fine parsing it if the paths TLV record doesn't exist. Could remove this check if you prefer.

Hmm, no strong opinion, I worry we'd spuriously fail there encountering those in the wild, though?

Removed the check.

TheBlueMatt · 2022-11-11T21:54:37Z

Feel free to squash IMO.

TheBlueMatt

A few nits, feel free to ignore and land, though.

TheBlueMatt · 2022-11-16T21:18:22Z

+
+	/// Parses a bech32-encoded message into a TLV stream.
+	fn from_bech32_str(s: &str) -> Result<Self, ParseError> {
+ // Offer encoding may be split by '+' followed by optional whitespace.


I'm confused - are offers allowed to have an arbitrary number of +( )*s? Is there some restriction as to where in the offer they can be that we can avoid copying the whole string next just to call bech32::decode? What is the reason they're allowed to have +s anyway?

According to BOLT 12 requirements:
Readers of a bolt12 string:
if it encounters a + followed by zero or more whitespace characters between two bech32 characters:
MUST remove the + and whitespace.
Rationale is for use in text fields with limited size, like Twitter.
BOLT 12 defines some test vectors currently exercised in fails_parsing_bech32_encoded_offers_with_invalid_continuations.
Could probably avoid the copy if bech32 crate worked on an iterator of characters. But currently bech32::decode also returns slices into the string for HRP and data.

Ugh....I wonder if it makes sense to at least have an optimization for cases with no +? Probably more work than it's worth but nice to avoid copying just because.

Done with an enum.

TheBlueMatt · 2022-11-16T21:33:30Z

+
+	/// Formats the message using bech32-encoding.
+	fn fmt_bech32_str(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+ bech32::encode_without_checksum_to_fmt(f, Self::BECH32_HRP, self.as_ref().to_base32())


Grr, its really kinda dumb we're forced to put the intermediate u5s in a vec in between here, but that's upstream's fault - rust-bitcoin/rust-bech32#81

TheBlueMatt · 2022-11-17T22:34:17Z

LGTM, feel free to squash IMO.

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts. Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages. The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Test semantic errors when parsing offer bytes.

BOLT 12 messages are limited to a range of TLV record types. Refactor decode_tlv_stream into a decode_tlv_stream_range macro for limiting which types are parsed. Requires a SeekReadable trait for rewinding when a type outside of the range is seen. This allows for composing TLV streams of different ranges. Updates offer parsing accordingly and adds a test demonstrating failure if a type outside of the range is included.

jkczyz · 2022-09-16T21:41:23Z

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts.

Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages.

The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Also, implement Writeable for Offer and have a corresponding TryFrom implementation for decoding the raw bytes.

codecov-commenter · 2022-09-16T21:54:27Z

Codecov Report

Base: 90.80% // Head: 91.95% // Increases project coverage by +1.14% 🎉

Coverage data is based on head (e916bf8) compared to base (f1428fd).
Patch coverage: 86.83% of modified lines in pull request are covered.

❗ Current head e916bf8 differs from pull request most recent head 1e26a2b. Consider uploading reports for the commit 1e26a2b to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1726 +/- ##
==========================================
+ Coverage 90.80% 91.95% +1.14% 
==========================================
Files 89 91 +2 Lines 47963 58814 +10851 Branches 47963 58814 +10851 ==========================================
+ Hits 43554 54081 +10527 - Misses 4409 4733 +324

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	`91.70% <ø> (-0.10%)`	⬇️
lightning/src/util/ser_macros.rs	`89.15% <53.84%> (+0.74%)`	⬆️
lightning/src/offers/offer.rs	`91.77% <87.38%> (-2.79%)`	⬇️
lightning/src/offers/parse.rs	`93.47% <93.47%> (ø)`
lightning/src/util/events.rs	`34.91% <0.00%> (-2.72%)`	⬇️
lightning/src/chain/mod.rs	`66.66% <0.00%> (-1.52%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/reload_tests.rs	`95.24% <0.00%> (ø)`
... and 24 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.

valentinewallace · 2022-09-26T18:55:02Z

 	}
 }

+impl TryFrom<Vec<u8>> for Offer {


Readable seems more ergonomic as an API?

Readable only supports DecodeError. Could map ParseError to DecodeError::InvalidValue, but I don't think we'd want to swallow the more specific error. Plus, it would be inconsistent with parsing InvoiceRequest and Invoice, where it's helpful to know the error reason.

Forgot to mention but also note that Offer needs to take ownership of the bytes.

valentinewallace · 2022-09-27T15:07:31Z

+	MissingNodeId,
+	/// An empty set of blinded paths was provided.
+	MissingPaths,
+	/// A quantity representing an empty range or that was outside of a valid range was provided.


Bit confusing wording to parse

Rewrote the InvalidQuantity docs. PTAL.

jkczyz · 2022-10-07T21:16:45Z

Updated to parse bech32 strings directly into the higher-level structs instead of an intermediary TLV stream format by adding a TryFrom<Vec<u8>> trait bound to Bech32Encode and dropping the associated type. This is more straightforward and also makes it so that checking that all bytes are read in #1738 only needs to be defined in one place.

jkczyz · 2022-11-10T15:04:37Z

FYI, I refactored commit out of the next PR that limits TLV stream decoding to a type range and moved it here.

TheBlueMatt

Basically LGTM, really.

jkczyz

All good feedback

valentinewallace

Nothing major :)

valentinewallace · 2022-11-11T18:31:00Z

+ }
+
+ let mut builder = OfferBuilder::new("foo".into(), pubkey(42));
+ builder.offer.paths = Some(vec![]);


I don't quite get why this should cause us to fail to parse

I likely inferred it from this part of the spec:
- if it is connected only by private channels: - MUST include `offer_paths` containing one or more paths to the node from publicly reachable nodes. - otherwise: - MAY include `offer_paths`.

But doesn't that imply offer_paths is optional? Only for public nodes, but that's not something we know at decode-time.

True... I guess it is more that we fail if given a paths TLV record with length 0. We are fine parsing it if the paths TLV record doesn't exist. Could remove this check if you prefer.

Hmm, no strong opinion, I worry we'd spuriously fail there encountering those in the wild, though?

Removed the check.

TheBlueMatt · 2022-11-11T21:54:37Z

Feel free to squash IMO.

TheBlueMatt

A few nits, feel free to ignore and land, though.

TheBlueMatt · 2022-11-16T21:18:22Z

+
+	/// Parses a bech32-encoded message into a TLV stream.
+	fn from_bech32_str(s: &str) -> Result<Self, ParseError> {
+ // Offer encoding may be split by '+' followed by optional whitespace.


I'm confused - are offers allowed to have an arbitrary number of +( )*s? Is there some restriction as to where in the offer they can be that we can avoid copying the whole string next just to call bech32::decode? What is the reason they're allowed to have +s anyway?

According to BOLT 12 requirements:
Readers of a bolt12 string:
if it encounters a + followed by zero or more whitespace characters between two bech32 characters:
MUST remove the + and whitespace.
Rationale is for use in text fields with limited size, like Twitter.
BOLT 12 defines some test vectors currently exercised in fails_parsing_bech32_encoded_offers_with_invalid_continuations.
Could probably avoid the copy if bech32 crate worked on an iterator of characters. But currently bech32::decode also returns slices into the string for HRP and data.

Ugh....I wonder if it makes sense to at least have an optimization for cases with no +? Probably more work than it's worth but nice to avoid copying just because.

Done with an enum.

TheBlueMatt · 2022-11-16T21:33:30Z

+
+	/// Formats the message using bech32-encoding.
+	fn fmt_bech32_str(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+ bech32::encode_without_checksum_to_fmt(f, Self::BECH32_HRP, self.as_ref().to_base32())


Grr, its really kinda dumb we're forced to put the intermediate u5s in a vec in between here, but that's upstream's fault - rust-bitcoin/rust-bech32#81

TheBlueMatt · 2022-11-17T22:34:17Z

LGTM, feel free to squash IMO.

Add common bech32 parsing for BOLT 12 messages. The encoding is similar to bech32 only without a checksum and with support for continuing messages across multiple parts. Messages implementing Bech32Encode are parsed into a TLV stream, which is converted to the desired message content while performing semantic checks. Checking after conversion allows for more elaborate checks of data composed of multiple TLV records and for more meaningful error messages. The parsed bytes are also saved to allow creating messages with mirrored data, even if TLV records are unknown.

Test semantic errors when parsing offer bytes.

BOLT 12 messages are limited to a range of TLV record types. Refactor decode_tlv_stream into a decode_tlv_stream_range macro for limiting which types are parsed. Requires a SeekReadable trait for rewinding when a type outside of the range is seen. This allows for composing TLV streams of different ranges. Updates offer parsing accordingly and adds a test demonstrating failure if a type outside of the range is included.

jkczyz force-pushed the 2022-09-offer-parsing branch 4 times, most recently from eb9a7ea to 77f0517Compare September 22, 2022 22:59

valentinewallace mentioned this pull request Sep 23, 2022
BOLT 12 offer encoding and building #1719
Merged

jkczyz force-pushed the 2022-09-offer-parsing branch from 77f0517 to 3253cb5Compare September 23, 2022 22:52

valentinewallace reviewed Sep 27, 2022
View reviewed changes

Comment threadlightning/src/offers/offer.rs Outdated

jkczyz force-pushed the 2022-09-offer-parsing branch 2 times, most recently from b35db47 to c9fc32fCompare September 29, 2022 01:44

jkczyz force-pushed the 2022-09-offer-parsing branch from c9fc32f to e584faeCompare October 7, 2022 21:08

jkczyz force-pushed the 2022-09-offer-parsing branch from e584fae to 4236762Compare October 20, 2022 14:50

jkczyz force-pushed the 2022-09-offer-parsing branch 2 times, most recently from 11b712e to 9eeccb2Compare November 1, 2022 19:10

jkczyz force-pushed the 2022-09-offer-parsing branch 3 times, most recently from 7fe129f to 0af1affCompare November 9, 2022 00:22

jkczyz marked this pull request as ready for review November 9, 2022 00:24

TheBlueMatt reviewed Nov 10, 2022
View reviewed changes

Comment threadlightning/src/offers/offer.rs Outdated
Comment threadlightning/src/util/ser_macros.rs

TheBlueMatt reviewed Nov 10, 2022
View reviewed changes

Comment threadlightning/src/offers/parse.rs Outdated

jkczyz force-pushed the 2022-09-offer-parsing branch from c15b93d to 0839966Compare November 10, 2022 23:53

jkczyz commented Nov 10, 2022
View reviewed changes

TheBlueMatt assigned TheBlueMatt and valentinewallace Nov 11, 2022

valentinewallace reviewed Nov 11, 2022
View reviewed changes

jkczyz force-pushed the 2022-09-offer-parsing branch 2 times, most recently from 391fa0b to 179e961Compare November 11, 2022 20:49

jkczyz force-pushed the 2022-09-offer-parsing branch from 179e961 to 0a56d98Compare November 15, 2022 21:21

TheBlueMatt previously approved these changes Nov 16, 2022
View reviewed changes

jkczyz dismissed TheBlueMatt’s stale review via 2a25525November 17, 2022 04:04

jkczyz force-pushed the 2022-09-offer-parsing branch from 0a56d98 to 2a25525Compare November 17, 2022 04:04

valentinewallace reviewed Nov 17, 2022
View reviewed changes

Comment threadlightning/src/util/ser_macros.rs
Comment threadlightning/src/offers/offer.rs Outdated

jkczyz force-pushed the 2022-09-offer-parsing branch from 2a25525 to e916bf8Compare November 17, 2022 22:32

jkczyz added 5 commits November 18, 2022 11:33

Use SemanticError in OfferBuilder::build
3a6d7b8

Offer parsing tests
03d0a4b
Test semantic errors when parsing offer bytes.

Expose the default Quantity::one as pub
1e26a2b

jkczyz force-pushed the 2022-09-offer-parsing branch from e916bf8 to 1e26a2bCompare November 18, 2022 17:34

valentinewallace approved these changes Nov 18, 2022
View reviewed changes

TheBlueMatt approved these changes Nov 18, 2022
View reviewed changes

TheBlueMatt merged commit 8d93dba into lightningdevkit:mainNov 18, 2022

valentinewallace mentioned this pull request Dec 6, 2022
BOLT 12 invoice_request encoding and building #1738
Merged

jkczyz mentioned this pull request May 10, 2023
BOLT12 Offers Tracking Issue #1970
Open
60 tasks