-
Notifications
You must be signed in to change notification settings - Fork 371
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
Cleanup Bech32 logic, isolate or make away with bech32 dependency #3195
Comments
I had a bit of a play with both the PRs aimed at closing this issue, unless I'm missing something I don't see any reason to keep your custom Admittedly the iterator api code is a bit gnarly to read, I'm happy to help if you get stuck. |
We can add these if they make derives more convenient (I think For that matter I'm curious where you're directly using field elements at all. |
Reading through the codebase it looks like you have a ton of custom checksummed-data-parsing code that should be able to be removed entirely and replaced with calls into the new API, and you shouldn't ever need to deal with FEs. |
Ah, ok, because LN invoices have fields which are 5-bit aligned you probably do want to work with |
@optout21 so, after having worked on this for a few days I've nearly converted the serialization logic to the new bech32 library. I think this issue is really nontrivial. Essentially, the LN invoice format is a collection of fields which are ad-hoc aligned on 5-bit and 8-bit boundaries, and in the old bech32 API (which provided no assistance with conversions) you were expected to just "produce a slice of This results in the following issues:
So if you naively try to translate the old logic to the new one by just replacing types, you'll basically take an existing huge mess and kinda smear it around. What you need to do is (a) experiment a bit with the new iterator-based API (which probably needs much more extensive doccomments; I hadn't considered something as elaborate as the LN invoice format when I wrote that), and (b) identify all the existing places where bech32 logic is re-implemented inline in rust-lighting, and (c) rewrite all the serialization logic. As I said, I'm nearly finish this on the serialization side. I haven't looked at deserialization yet. Will try to open a PR today or tomorrow, though I'm a bit behind on other stuff since the recent rust-bitcoin summit. |
Ordering would have been needed for the tags in a bech32-encoded lightning invoice, each tag has a 32-bit tag value (usually denoted by their Bech32 char representation), and they are kept in a map, they are ordered, etc. BTW, here the 32-bit elements are just that: 32-bit numbers, the field properties are not used. |
Thanks for looking into this, @apoelstra , you're right. |
lightning-invoices have a bech32 checksum on them. This requires field properties and is exactly what |
Update: |
FYI I have a branch where I've been working on this https://github.com/apoelstra/rust-lightning/tree/2024-08--new-bech32 Though it is a bit of a mess; it needs #3234 |
Bech32 logic is used for Lightning invoice decoding & encoding, and the
bech32
crate is used for that.This analysis is triggered by the upgrade from
bech32 v0.9.1
tobech32 v0.11.0
, during which the library API has changed significantly.The upgrade is being handled by PR #3181 ( #3176 ) .
Here is an analysis of needed/used functionalities, and recommendations:
Type for 5-bit numbers
u5
type, a wrapped byte, but that's replaced byFe32
, which is roughly similar, but has different focus (field element arithmetic)Fe32
has some minor shortcomings (noOrd
ordering, no default)u8
wrapper with a few conversion methods.Bech32 character encoding.
Fe32
Encoding/decoding packed 5 bit values to bytes
bech32
crate (Fe32IterExt, ByteIterExt).Checksum verification and creation
bech32
createParsing of HRP, data part and Checksum
Parsing of tagged values
bech32
has no lightning-specific logic (and probably will not have later, not in scope).Conclusion:
u5
type, packed decoding, etc.), while reusing some logic from the external crate,but in a reduced and isolated way (checksum handling). Getting rid of the external dependency entirely is also a possibility.
Options for location of bech32 logic:
lightning
crate (e.g.bech32.rs
)bech32-lightning
)lightning-invoice
crate is NOT a good option, aslightning
crate cannot use it, and currently there is some need there (invoice encoding for signing).The text was updated successfully, but these errors were encountered: