What should the sign_invoice API look like?

[apoelstra]

The NodeSigner::sign_invoice method currently has a pretty crappy API which pretty-much guarantees needless allocations and extra conversions.

The data to be signed consists of an ASCII HRP followed by the non-HRP data of a Lightning invoice encoded as a bytestring. BTW, where is this specified? It it supposed to be the HRP or is it just a "coincidence" that both the bech32 strings and the signed data have the HRP data like this). Currently the API takes this data directly, by taking the HRP as a &[u8], and taking the non-HRP data as a &[u5], which it then internally converts to a bytestring.

This has the following issues:

• It requires the user to get a &[u5] from somewhere that specifically represents the non-HRP non-checksum data of an invoice. This object must be specifically constructed and serves no other purpose. Currently it is constructed by the allocation-heavy RawDataPart::to_base32 function but in a later version of rust-bech32 we will lose that method.
• It requires the user to have all the data available in a slice, which basically means that the caller needs to construct Vecs. But the API just takes this raw data and feeds it into a hash engine. If it were able to take a streaming source of bytes (an io::Read or a core::iter::Iterator) we could avoid ever allocating a vector.

Ideally this function would accept a "fully parsed bolt11 invoice", but the type for this is lightning-invoice::RawBolt11Invoice, which is not available in rust-lightning. Alternately it could accept a bech32 string and attempt to parse it itself, but then it would need to deal with error paths related to string parsing. Still alternately it could take a sha256::Hash directly, expecting the caller to do the hashing, but this is a potentially dangerous API (though not that the existing one is any safer..).

For now, a simple and obvious improvement would be for the method to take a &[u8] rather than a &[u5]. Then it could avoid doing an expensive conversion from base 32 to 256 and simply feed its input into a hash engine. But it still forces the caller to produce a Vec<u8> that would just be thrown away.

Where is this API used and what are its requirements?

Related to #3195

[TheBlueMatt]

Right, sadly this is terrible because lightning-invoice depends on lightning rather than the other way around (which it should be). We need to refactor a few types out into some kind of lightning-types crate (PaymentHash, PaymentSecret and maybe one or two others), we just never got around to it. So, as a result, we can't use the native BOLT11Invoice type which we should, and rather end up with a terrible API. Frankly, I don't really care how that API gets changed, its terrible and the end state is not any intermediate thing.

The only thing we can't do is pass a SHA256 hash blind, we need the signer to be able to actually see what its signing, even if parsing it back into a proper invoice to check it is hard. Maybe for now we just pass a String of the serialized invoice lol.

[apoelstra]

Ok, great, glad I'm not misunderstanding things.

I will sleep on this and likely just change the &[u5] to a &[u8] (and the HRP to a bech32::Hrp once we update that dep). But maybe I will try to create a new dummy type which breaks things down a little bit more without replicating all the logic of lightning-invoice.

It's tempting to take a &str, from which I believe I can get a hash to sign without doing any "real" parsing beyond the bech32 stuff, but as mentioned above, it would pollute the signing API with parsing/checksum errors.

[tnull]

Right, sadly this is terrible because lightning-invoice depends on lightning rather than the other way around (which it should be). We need to refactor a few types out into some kind of lightning-types crate (PaymentHash, PaymentSecret and maybe one or two others), we just never got around to it. So, as a result, we can't use the native BOLT11Invoice type which we should, and rather end up with a terrible API. Frankly, I don't really care how that API gets changed, its terrible and the end state is not any intermediate thing.

Seems this might be another contender we should get done for the 0.1 release?

[TheBlueMatt]

Seems this might be another contender we should get done for the 0.1 release?

Hmm, we haven't previously considered "API we're happy with" to be a major blocker for 0.1, just "no known bugs in the features we have".