lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.15k stars 363 forks source link

Upgrade bech32 dependency (iterative) #3270

Open optout21 opened 1 month ago

optout21 commented 1 month ago

Continuation of #3244 . Fixes #3176 .

Changes summary:

Here are some minor issues identified

TODO:

codecov[bot] commented 1 month ago

Codecov Report

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

Project coverage is 89.64%. Comparing base (66fb520) to head (7da1b51).

Files with missing lines Patch % Lines
lightning-invoice/src/de.rs 86.99% 10 Missing and 6 partials :warning:
lightning/src/offers/parse.rs 63.63% 2 Missing and 2 partials :warning:
lightning-invoice/src/test_ser_de.rs 97.00% 0 Missing and 3 partials :warning:
lightning-invoice/src/lib.rs 96.55% 2 Missing :warning:
lightning-invoice/src/ser.rs 98.65% 0 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3270 +/- ## ========================================== - Coverage 89.64% 89.64% -0.01% ========================================== Files 126 126 Lines 102676 102753 +77 Branches 102676 102753 +77 ========================================== + Hits 92045 92109 +64 - Misses 7912 7932 +20 + Partials 2719 2712 -7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

optout21 commented 1 month ago

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 commented 1 month ago

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

optout21 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

Awaiting (re)review, @TheBlueMatt . Thx.

optout21 commented 2 weeks ago

Commits squashed

TheBlueMatt commented 2 weeks ago

We should get a second reviewer on this.