Option for Fe32->bytes conversion (fes_to_bytes) to not drop last partial values

[optout21]

This is a minor feature request:
The Fe32->bytes conversion -- fes_to_bytes in primitives/iter.rs -- drops the last incomplete byte:

If the total number of bits is not a multiple of 8, any trailing bits are simply dropped

In some use cases padding with 0 bits is needed (rather than dropping bits). Currently this can be achieved by appending the input with 0, 1, or 2 0 values (depending the input length), before doing the conversion.
It would be nice if this is provided as a possibility in conversion (e.g. by a different method fes_to_bytes_pad or an parameter flag).

An example with pre-padding is here: https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-invoice/src/lib.rs#L963

If the idea is accepted/clarified, I can prepare a PR.

[apoelstra]

The code in question is pretty bizarre. It takes byte-encoded data, encodes it into bech32 without adding any padding (because it is eventually part of some larger bech32 string) and then does this insane transformation to convert back to base 256.

Since lightningdevkit/rust-lightning#3234 (a PR that Matt created specifically to deal with this case) you can change your signing method to just take a raw HRP invoice instead of taking a partially-serialized shadow of one, and you can throw away all these useless conversions (along with the useless allocations and error paths).

So concept NACK from me. Needing to do this is indicative of confused logic, and this particular example is super confused.

[optout21]

Unfortunately it's not the case, it's not so simple.
The reason is that structuring the parts of the invoice elements is defined on the bech32 level: it is not possible to concatenate/parse parts on the byte level. When constructing an invoice, the parts are converted to Bech32, padded and then concatenated.
However, for computing the hash the bytes are needed, the bytes of the concatenated parts, which is not the concatenation of the byte parts!

I show this through a simple (made-up) example.
Let's say I want to create a lightning invoice with:

• prefix lnbc
• timestamp: value 100_000 encoded qqqrp4q
• description: tag d length 13 qd value ascii "AAAAAABCD" encoded g9q5zs2pg9pyx3q
• payment hash: tag p length 52 p5 value [2; 32] encoded qgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpq

(So far it looks lnbc 1 qqqrp4q d q0 g9q5zs2pg9pyx3q p p5 qgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpq.)

For the signature the hash is needed, and the input for the hash is the data part (qqqrp4qdq0g9q5zs2pg9pyx3qpp5qgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpq) as bytes (prepended with prefix bytes).
That is:

[108, 110, 98, 99, 0, 0, 48, 212, 13, 3, 208, 80, 80, 80, 80, 80, 80, 144, 209, 0, 67, 64, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32]

These bytes can't be derived from the bytes of the original data parts! If you look for the repeated values 65 (for A from the description) or 2 (from the 'p' hash), they are not present, they are bit-shifted.

I can see no practical way to derive it apart from decoding from the encoded Bech32 value.

Is it sub-optimal? Arguably, but this is how lightning invoices are defined and used.

Note that the same operation is needed in the usecase of verifying the signature of an existing invoice.

Moreover, there is another, low-prio use case where Base32->Base256 conversion with padding is needed, is the internal encoding of Feature bits. This is a whole different story, it's LDK specific, and the encoding is all reversed there -- both the byte order and the bit order. Currently we have custom conversion there, but if we want to reuse rust-bech32 -- which we want, to maximize reuse -- we need to pre-pad. More details on this here: lightningdevkit/rust-lightning#3270 (comment)

I raised this issue as I thought there is no way to optimize out this problem, and the only solution is the pre-pad before invoking rust-bech32. I still think so.

If you prefer not to have this option in rust-bech32, I accept, and we will keep the workaround in LDK. It's a bit sub-optimal, but not a big deal.

(In the meantime I've realized that the padding can also be done in an iterator adapter (counts the elements, pads at the end if needed), and in that way it it's still a small workaround but it fits nicer into iterative processing).

[apoelstra]

Thanks for writing up this detailed example. I think you're right that, given the bizarre definition of the LN invoice format, it makes sense to support this decoding mode. I'm sure there are hoops that ldk can jump through to avoid this, but should everybody who wants to parse/sign LN invoices need to do this?

It seems to me that if a major part of the ecosystem is specified in a way that conceptually requires this... we ought to add it here.

(In the meantime I've realized that the padding can also be done in an iterator adapter (counts the elements, pads at the end if needed), and in that way it it's still a small workaround but it fits nicer into iterative processing).

I believe this is what I did when I worked through this. But maybe I didn't get far enough along in my rewrite to get here. It's possible I was just confused -- I woud up being blocked on 3234 before I was able to get my code to even compile, let alone the unit tests passing.

[apoelstra]

So concept NACK from me. Needing to do this is indicative of confused logic, and this particular example is super confused.

I rescind this. The confused logic is embedded in a specification and we need to live with it, just like the dozens of similar weird things in Bitcoin itself.

concept ACK.

[optout21]

This is not an urgent, nor a major issue in LDK, and implementation in rust-bech32 is probably small -- an additional condition in the conversion to output the last incomplete byte.
I'm happy to contribute to create a PR if needed, but the API for it has to be specified. My suggestion would be a new version of fes_to_bytes (e.g. fes_to_bytes_notrim), but I can imagine other solutions (input parameter flag, member variable flag, etc.).

[apoelstra]

My suggestion would be a new version of fes_to_bytes (e.g. fes_to_bytes_notrim), but I can imagine other solutions (input parameter flag, member variable flag, etc.)

This is my preferred approach. Though you can reuse the FesToBytes structure of course if that simplifies things.

We should maybe call it _zeropad rather than _notrim to emphasize that we are adding zeroes to get to a multiple of 8. But I'm not too concerned with the name, as long as it's somewhat obvious to a LN developer that this is the function they want.

[clarkmoody]

I second the new function approach. No need to break the API by adding a parameter to an existing function.