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

Feature: use bech32-encoding everywhere instead of hex-encoding some places. #190

Open
AndySchroder opened this issue Oct 7, 2024 · 11 comments

Comments

@AndySchroder
Copy link

decode-invoice Decodes a bech32-encoded invoice string into a BOLT 12 invoice

get-invoice fetches a BOLT 12 invoice and returns as a hex-encoded string.

pay-invoice pays a hex-encoded BOLT12 invoice

Would be nice if all commands used a bech32-encoded invoice string instead of hex-encoded in some places. This is the behavior of CLN (https://docs.corelightning.org/reference/lightning-fetchinvoice , https://docs.corelightning.org/reference/lightning-pay).

@supertestnet
Copy link

I too want get-invoice to return a bech32-encoded bolt12 invoice

In the meantime, does anyone know a workaround to convert a hex-encoded invoice to a bech32-encoded invoice? I tried to just modify the encoding and prefix "LNI" to it, but then I passed that through a third-party bolt12 parser to see if I did it right and I got an "invalid padding" error.

@orbitalturtle
Copy link
Collaborator

Sure thing, I think we can change this to bech32 to make the API similar to CLN etc. @mrfelton I can't remember the reason for using hex, would this change break anything on Strike's end or would it be an easy swap?

In the meantime, does anyone know a workaround to convert a hex-encoded invoice to a bech32-encoded invoice? I tried to just modify the encoding and prefix "LNI" to it, but then I passed that through a third-party bolt12 parser to see if I did it right and I got an "invalid padding" error.

This is a common misconception I think - there is actually no lni prefix for invoices. In bolt 12 only lnr for the invoice requests and lno are defined, IIRC directly this is because the invoice is supposed to be hidden from the end user? Because of that, I'd be curious if the parser you're using parses bolt 12 invoices at all - but if it does, does it work with the lni removed?

Sounds like this isn't for your particular use case, but in a lot of cases just using pay-offer (which bundles get-invoice and pay-invoice into one) and not touching the invoice at all is the way to go.

@orbitalturtle
Copy link
Collaborator

Oh though I just remembered something @supertestnet, if your goal is to inspect the invoice, the decode-invoice cli command does that. I think if you convert the hex to bech32 w/o the lni prefix it should work.

@AndySchroder
Copy link
Author

AndySchroder commented Oct 13, 2024

This is a common misconception I think - there is actually no lni prefix for invoices. In bolt 12 only lnr for the invoice requests and lno are defined, IIRC directly this is because the invoice is supposed to be hidden from the end user? Because of that, I'd be curious if the parser you're using parses bolt 12 invoices at all - but if it does, does it work with the lni removed?

Yes, the prefix lni is not defined in the spec, but I think it should be. I'd like to use the lni prefix for embedding a serialized refund invoice inside an invoice_request (see discussion here: https://delvingbitcoin.org/t/expanding-on-bolt12/1167#p-3280-automatic-refunds-3 ).

@mrfelton
Copy link
Contributor

@mrfelton I can't remember the reason for using hex,

Primarily because it's not defined in the spec, and the recommendation at the time was encode as hex.

I've seen quite a few discussions about this, however. The spec authors argue that there is no need for the bech32 encoding for Invoices and no need for a human readable prefix because Invoices are not intended to be passed around between people.

In our own use case, invoices are passed around, although not between humans. They are stored in databases and passed between systems. hex encoding does work fine for that.

would this change break anything on Strike's end or would it be an easy swap?

Switching to a different encoding format would be a breaking change. I'd suggest adding a switch to allow the encoding scheme to be specified if you want to go in that direction. But I'd be weary of introducing a new human readable prefix that is not part of the spec. If there is broad consensus however that bech32 encoded invoices with a human readable prefix was a missing part of the spec then maybe work towards getting it added to the spec, or better understanding the reasons why it was not included.

@AndySchroder
Copy link
Author

Why does decode-invoice use bech32-encoded invoice string then? Seems inconsistent. Are you arguing that should actually accept hex instead?

@AndySchroder
Copy link
Author

Here is some more discussion of exposing the BOLT12 invoice: https://delvingbitcoin.org/t/blip-bolt-11-invoice-blinded-path-tagged-field/991/2 . We have some benefits that the BOLT12 invoice supports blinded paths and can be expanded to include more generic fields than a BOLT11 invoice. Seems like there are a lot of scenarios where skipping the offer/invoice_request steps could improve efficiency/speed and standardizing on BOLT11 instead of BOLT12 could be good.

@AndySchroder
Copy link
Author

Also, in #170 (comment) and #181 I requested to return the invoice back over the RPC as an option instead of onion messaging. That's another use case for passing invoices around and I think it would make sense to use bech32 encoding and not hex.

@mrfelton
Copy link
Contributor

Why does decode-invoice use bech32-encoded invoice string then?

It doesn't. It accepts a hex encoded invoice.

lndk/src/cli.rs

Lines 153 to 169 in 925ce05

Commands::DecodeInvoice { invoice_string } => {
println!("Decoding invoice: {invoice_string}.");
let invoice_string: Bolt12InvoiceString = invoice_string.clone().into();
match Bolt12Invoice::try_from(invoice_string) {
Ok(invoice) => {
println!("Decoded invoice: {:?}.", invoice);
}
Err(e) => {
println!(
"ERROR please provide hex-encoded invoice string. Provided invoice is \
invalid, failed to decode with error: {:?}.",
e
);
exit(1);
}
}

There may be a mistake in the docs.

@AndySchroder
Copy link
Author

There may be a mistake in the docs.

Okay, it seems that lndK is a bit more consistent then. My original issue attempted (but probably didn't say it clearly enough) to address (1) the inconsistency and (2) that I think people would prefer a bech32-encoded invoice prefixed with lni. It seems like (1) may not be up for much debate if the only inconsistency is a documentation error, but (2) may be still worth debating.

@a-mpch
Copy link
Contributor

a-mpch commented Dec 13, 2024

@AndySchroder so, when I started writing #129 the original decision was that bech32 is only used when presented to humans and spec uses TLV stream encoding for machines.

Here we are in a middle ground(?) as it is mostly expected that invoices are used by machines, but you could still use it as human using the CLI, even if is not the use case.

What plans or use cases do you have in mind?

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

No branches or pull requests

5 participants