Upgrade bech32 dependency (iterative) #3270

optout21 · 2024-08-23T21:54:18Z

Continuation of #3244 . Fixes #3176 .

Changes summary:

Upgraded bech32 dependency to 0.11.0 (from 0.9)
The previously used type bech32::u5 can be replaced by bech32::Fe32, mostly as a drop-in replacement.
u5::try_from_u8 is now Fe32::try_from
bech32::Error is mostly replaced by bech32::primitives::decode::CheckedHrpstringError
ToBase32 (and Base32Len) traits are discontinued in bech32, they have been reimplemented in iterative style as Base32Iterable. The implementations (Bolt11InvoiceFeatures/Payment*/...) have been moved to lightning-invoice crate (as the trait is defined there)
FromBase32 trait is discontinued in bech32, it has been taken over to lighning-invoice crate. The implementations have been moved to lightning-invoice crate (as the trait is defined there)

Here are some minor issues identified

Hrp/checksum parsing has changed (uses new iterative API)
Fe32 does not have Ord, so it had to be explicitly implemented for RawTaggedField
Fe32 does not have Default, for which some workaround was needed. This will be added in next bech32 0.12 release (see Add Ord trait to Fe32 (derived) rust-bitcoin/rust-bech32#186)
Conversions of bech32 error types are not always optimal
Parsing of tagged values in an invoice stays own implementation (not supported by bech32 crate)

TODO:

fix fuzz target
fix fmt
squash
Incremental-mutants

codecov · 2024-08-23T22:08:15Z

Codecov Report

Attention: Patch coverage is 93.90519% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.63%. Comparing base (4147de2) to head (aa2f6b4).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-invoice/src/de.rs	87.20%	10 Missing and 6 partials ⚠️
lightning/src/offers/parse.rs	63.63%	2 Missing and 2 partials ⚠️
lightning-invoice/src/test_ser_de.rs	97.00%	0 Missing and 3 partials ⚠️
lightning-invoice/src/lib.rs	96.55%	2 Missing ⚠️
lightning-invoice/src/ser.rs	98.65%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3270      +/-   ##
==========================================
- Coverage   89.68%   89.63%   -0.05%     
==========================================
  Files         126      127       +1     
  Lines      103306   106126    +2820     
  Branches   103306   106126    +2820     
==========================================
+ Hits        92651    95128    +2477     
- Misses       7936     8287     +351     
+ Partials     2719     2711       -8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

optout21 · 2024-08-29T01:50:31Z

CI: A few misses in iterative-mutants on the Bolt11InvoiceFeatures serialization method (unchanged). To see if checks can be added, or the method can be rewritten in a simpler form.

optout21 · 2024-08-31T05:02:32Z

CI: All builds OK. Iterative mutants is satisfied now, but the build was interrupted (?) after the tests (verified the logs + locally).

lightning-invoice/src/de.rs

lightning-invoice/src/lib.rs

lightning-invoice/src/ser.rs

lightning-invoice/src/de.rs

optout21 · 2024-09-08T15:06:00Z

Added a minor cleanup: hash_from_parts is called from a single place (from signable_hash(); in the other place in the SignedRawBolt11Invoice parser, it's called through signable_hash()), this way one version is sufficient.

optout21 · 2024-09-08T15:17:57Z

A note regarding bech32->bytes conversion with padding:
This is needed currently when computing hash of an invoice (hash_from_parts), and, if we want to switch for Features using bech32 library (as mentioned in a review comments above), also in the Features decoding.

I've raised an issue to rust-bech32, to possible offer an option for bech32->bytes conversion where last bits are not dropped but padded, that would be the nice solution.

I also realized padding can be done in an iterator adapter (counts the elements and and the end optionally outputs padding), so this padding can be applied nicely in the chained iterators approach (as discusses above).

optout21 · 2024-09-09T08:49:28Z

Awaiting (re)review, @TheBlueMatt . Thx.

TheBlueMatt

This is shaping up nicely. We took this opportunity to avoid allocations in the serialization pipeline (sadly with allocations to avoid GATs...) but there's also a ton of really useless allocations in the deserialization pipeline, some of which we can trivially remove. We certainly don't have to fix it all in one go, but it'd be nice to reduce them where its easy here.

In any case, please feel free to go ahead and rewrite the commit history to have a logical progression so its easy to review :)

lightning-invoice/src/test_ser_de.rs

lightning-invoice/src/de.rs

lightning-invoice/src/lib.rs

lightning-invoice/src/de.rs

optout21 · 2024-09-11T20:43:51Z

Commits squashed

lightning-invoice/src/de.rs

lightning-types/Cargo.toml

TheBlueMatt · 2024-09-16T20:13:16Z

We should get a second reviewer on this.

fuzz/Cargo.toml

arik-so

I cannot find any issues with the bech32 conversion in either direction.

Besides the nits I pointed out, I have two thoughts where other reviewers may disagree, bit I'll throw it out there: to_fes (as in bytes_to_fes looks like a misspelling of fees, so given that names are free, I wonder if to_finite_elements or to_fe_32s might slightly simplify legibility for future readers.

On a similar note, SIGNATURE_LEN5 also looks like "signature lens." Perhaps an underscore or a base_5 or something might help?

But these thoughts aside, looks good!

lightning-invoice/src/de.rs

arik-so · 2024-10-02T06:17:33Z

btw, if you rebase, the linting CI error should go away

optout21 · 2024-10-02T13:43:02Z

Addressed minor nit's, rebased

optout21 · 2024-10-02T19:26:16Z

Besides the nits I pointed out, I have two thoughts where other reviewers may disagree, bit I'll throw it out there: to_fes (as in bytes_to_fes looks like a misspelling of fees, so given that names are free, I wonder if to_finite_elements or to_fe_32s might slightly simplify legibility for future readers.

This naming is not the best... Firstly, I don't think the 'finite element' (of algebraic group) is a relevant property of this thing at all, for our purpose it's just a 5-bit value. I think simply u5 would be a more appropriate naming. But since we use an external dependency, we should use the same naming for consistency; Fe32 for the struct and fes in the iterator/function names.
For more specific naming (better readability?), maybe bytes_to_fe32s would be better.

To the ...LEN5 I've added an extra underscore.

TheBlueMatt

Thanks for all the work here! Gonna go ahead and land this and we can reduce allocations more later.

TheBlueMatt · 2024-10-03T15:02:43Z

lightning-invoice/src/de.rs

+	type Err = Bolt11ParseError;
+
+	fn from_base32(field_data: &[Fe32]) -> Result<Self, Self::Err> {
+		if field_data.len() != 52 {


This check is redundant with the one in [u8; 32]::from_base32, no?

TheBlueMatt · 2024-10-03T15:05:12Z

lightning-invoice/src/de.rs

+			let curr_in_as_u8 = curr_in.to_u8();
+			if carry_bits >= 3 {
+				// we have a new full byte -- 3, 4 or 5 carry bits, plus 5 new ones
+				// For combining with carry '|', '^', or '+' can be used (disjoint bit positions)


nit: then prefer the bitop versions - means the CPU doesn't have to do any carries (not that it matters much - an 8 bit add should be one cycle basically all the time).

TheBlueMatt · 2024-10-03T16:50:46Z

lightning/src/offers/parse.rs

-			if hrp != Self::BECH32_HRP {
+			let parsed = CheckedHrpstring::new::<NoChecksum>(encoded.as_ref())?;
+			let hrp = parsed.hrp();
+			if hrp.to_string() != Self::BECH32_HRP {


No need to allocate here.

Suggested change

if hrp.to_string() != Self::BECH32_HRP {

if hrp.as_str() != Self::BECH32_HRP {

optout21 force-pushed the bech32-iterser branch 2 times, most recently from 9372141 to b677ddb Compare August 23, 2024 22:00

optout21 force-pushed the bech32-iterser branch 5 times, most recently from 8eb0c4b to 1fa1e99 Compare August 26, 2024 14:59

optout21 changed the title ~~[Draft] Upgrade bech32 dependency (iterative)~~ Upgrade bech32 dependency (iterative) Aug 27, 2024

optout21 marked this pull request as ready for review August 27, 2024 19:17

optout21 mentioned this pull request Aug 29, 2024

Upgrade bech32 dependency #3244

Closed

4 tasks

optout21 force-pushed the bech32-iterser branch 7 times, most recently from 55e4259 to 5fa337a Compare August 31, 2024 04:50

TheBlueMatt reviewed Sep 5, 2024

View reviewed changes

optout21 force-pushed the bech32-iterser branch 2 times, most recently from 9abd92e to ca0e089 Compare September 8, 2024 15:01

optout21 mentioned this pull request Sep 8, 2024

Option for Fe32->bytes conversion (fes_to_bytes) to not drop last partial values rust-bitcoin/rust-bech32#198

Closed

TheBlueMatt mentioned this pull request Sep 9, 2024

rustfmt: Run on lightning-invoice #3221

Open

TheBlueMatt reviewed Sep 9, 2024

View reviewed changes

optout21 mentioned this pull request Sep 10, 2024

Base32 followups, Features serialization #3308

Open

optout21 force-pushed the bech32-iterser branch from 440fd35 to 1a69ccc Compare September 11, 2024 20:38

TheBlueMatt reviewed Sep 11, 2024

View reviewed changes

optout21 force-pushed the bech32-iterser branch 2 times, most recently from 1852781 to 89a6cec Compare September 13, 2024 12:44

TheBlueMatt added the Seeking Code Review label Sep 16, 2024

arik-so reviewed Sep 19, 2024

View reviewed changes

fuzz/Cargo.toml Outdated Show resolved Hide resolved

optout21 force-pushed the bech32-iterser branch from 89a6cec to 7da1b51 Compare September 20, 2024 08:37

TheBlueMatt assigned arik-so Oct 1, 2024

TheBlueMatt removed the Seeking Code Review label Oct 1, 2024

arik-so reviewed Oct 2, 2024

View reviewed changes

lightning-invoice/src/de.rs Outdated Show resolved Hide resolved

lightning-invoice/src/de.rs Outdated Show resolved Hide resolved

Add Bolt11InvoiceFeatures serialization tests

ad80e91

optout21 force-pushed the bech32-iterser branch from 7da1b51 to 3f4d5b7 Compare October 2, 2024 13:41

optout21 requested review from arik-so and TheBlueMatt October 2, 2024 15:14

Upgrade bech32 dependency, bech32 serialization improvements

aa2f6b4

optout21 force-pushed the bech32-iterser branch from 3f4d5b7 to aa2f6b4 Compare October 2, 2024 19:21

arik-so approved these changes Oct 3, 2024

View reviewed changes

TheBlueMatt approved these changes Oct 3, 2024

View reviewed changes

TheBlueMatt merged commit bc1931b into lightningdevkit:main Oct 3, 2024
20 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade bech32 dependency (iterative) #3270

Upgrade bech32 dependency (iterative) #3270

optout21 commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

optout21 commented Aug 29, 2024

optout21 commented Aug 31, 2024

optout21 commented Sep 8, 2024 •

edited

Loading

optout21 commented Sep 8, 2024

optout21 commented Sep 9, 2024

TheBlueMatt left a comment

optout21 commented Sep 11, 2024

TheBlueMatt commented Sep 16, 2024

arik-so left a comment

arik-so commented Oct 2, 2024

optout21 commented Oct 2, 2024

optout21 commented Oct 2, 2024

TheBlueMatt left a comment

TheBlueMatt Oct 3, 2024

TheBlueMatt Oct 3, 2024

TheBlueMatt Oct 3, 2024

	if hrp.to_string() != Self::BECH32_HRP {
	if hrp.as_str() != Self::BECH32_HRP {

Upgrade bech32 dependency (iterative) #3270

Upgrade bech32 dependency (iterative) #3270

Conversation

optout21 commented Aug 23, 2024 • edited Loading

codecov bot commented Aug 23, 2024 • edited Loading

Codecov Report

optout21 commented Aug 29, 2024

optout21 commented Aug 31, 2024

optout21 commented Sep 8, 2024 • edited Loading

optout21 commented Sep 8, 2024

optout21 commented Sep 9, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

optout21 commented Sep 11, 2024

TheBlueMatt commented Sep 16, 2024

arik-so left a comment

Choose a reason for hiding this comment

arik-so commented Oct 2, 2024

optout21 commented Oct 2, 2024

optout21 commented Oct 2, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Oct 3, 2024

Choose a reason for hiding this comment

TheBlueMatt Oct 3, 2024

Choose a reason for hiding this comment

TheBlueMatt Oct 3, 2024

Choose a reason for hiding this comment

optout21 commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

optout21 commented Sep 8, 2024 •

edited

Loading