Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade bech32 dependency #3244

Closed
wants to merge 1 commit into from
Closed

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Aug 15, 2024

Reopening of #3181 . Fixes #3176 .

Changes summary:

  • Upgraded bech32 dependency to 0.11.0 (from 0.9)
  • Crates affected: lightning/types, lightning-invoice and lightning
  • 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
  • To/FromBase32 traits are discontinued in bech32, they have been taken over to lighning-invoice crate. The implementations (for Bolt11InvoiceFeatures and Payment*) 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, add tests for ToBase32/FromBase32

Copy link

codecov bot commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 91.12150% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (c9a2c4a) to head (092445f).
Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
lightning-invoice/src/ser.rs 90.00% 0 Missing and 8 partials ⚠️
lightning-invoice/src/de.rs 94.44% 1 Missing and 3 partials ⚠️
lightning/src/offers/parse.rs 63.63% 2 Missing and 2 partials ⚠️
lightning-invoice/src/lib.rs 94.11% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3244      +/-   ##
==========================================
+ Coverage   89.80%   89.81%   +0.01%     
==========================================
  Files         126      126              
  Lines      102953   102981      +28     
  Branches   102953   102981      +28     
==========================================
+ Hits        92457    92497      +40     
+ Misses       7770     7767       -3     
+ Partials     2726     2717       -9     

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

@optout21
Copy link
Contributor Author

New round, ready for review @arik-so @TheBlueMatt ( @apoelstra )

@optout21 optout21 marked this pull request as ready for review August 15, 2024 12:18
@TheBlueMatt
Copy link
Collaborator

Hmm, this version mostly just converts what we were doing, which was pretty inefficient/allocation-heavy, and keeps doing the same. Can we explore @apoelstra's approach at https://github.com/apoelstra/rust-lightning/tree/2024-08--new-bech32 first? That avoids a ton of our allocations (or, it will once we have GATs and can drop the Box traits, but even with those its probably a bit better), which would be really nice.

@tnull
Copy link
Contributor

tnull commented Aug 18, 2024

This needs a rebase now that the rust-bitcoin 0.32 upgrade PR landed.

@optout21
Copy link
Contributor Author

Hmm, this version mostly just converts what we were doing, which was pretty inefficient/allocation-heavy, and keeps doing the same. Can we explore @apoelstra's approach at https://github.com/apoelstra/rust-lightning/tree/2024-08--new-bech32 first? That avoids a ton of our allocations (or, it will once we have GATs and can drop the Box traits, but even with those its probably a bit better), which would be really nice.

I propose to include the dependency update first, and switch to iterative conversion in a separate step.

The proposed changes break several tests, I fixed some, but not all yet.
Unintended change in serialization format may break serialized instances across version upgrade. More tests should be added before changing serialization code.

Iterative transformations are especially beneficial if they can be combined, but this is not the case (the iterative conversion trait is private). The main benefit of iterative transformation is less memory footprint, but with intermediary collect/reverse operations this is not always the case.

@apoelstra
Copy link

Iterative transformations are especially beneficial if they can be combined, but this is not the case (the iterative conversion trait is private).

It's private but it's used to serialize an entire lightning invoice. If you want to compose that further, without allocations, you can use the fmt::Display trait.

@arik-so
Copy link
Contributor

arik-so commented Aug 22, 2024

I propose to include the dependency update first, and switch to iterative conversion in a separate step.

That sounds reasonable to me, given that this PR, in isolation, wouldn't be penalizing performance compared to what we already have.

@optout21
Copy link
Contributor Author

I will continue to straighten out the iterative solution (WIP branch: https://github.com/optout21/rust-lightning/tree/bech32-iterser )
Depending on the results, I will see if it makes more sense to merge this PR as it current state, or we can go directly with the iterative approach.
Also, I've created a separate PR ( #3266 ) with tests only, to clearly see that behavior does not change here (but the separate step is optional).

@optout21 optout21 force-pushed the bech32-dep branch 2 times, most recently from 4b466a8 to 968a46c Compare August 23, 2024 13:50
@optout21
Copy link
Contributor Author

Replaced by #3270

@optout21 optout21 closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade bech32 dependency
5 participants