[BUG] Lightning Address Description Hash about lightning-browser-extension HOT 11 CLOSED

Works perfectly @bumi thanks for updating that! 🎉

from lightning-browser-extension.

yay!! thanks again for reporting and the test.

from lightning-browser-extension.

Thanks for debugging this!!
So CLN actually is aware of the JSON and does the serialization? We need to get CLN to implement that a description_hash can be passed. IMO CLN should not do the hashing.

I review that we don't break anything and then change how we serialize that JSON to remove the space.

we should add this note to the LUD 06 description.

from lightning-browser-extension.

I am not exactly sure of the internal CLN process, I am somewhat assuming what what is happening under the hood based on what I am observing. I think it is fair that they validate the description_hash by hashing the passed in description parameter to ensure the payer is paying what they think they are paying.

I review that we don't break anything and then change how we serialize that JSON to remove the space.

Yeah I was trying to think through if this could break other implementations or not be backwards compatible and I think it should be fine as this is the way that all other implementations work, so wallets must be handling the case of no spaces in the serialised JSON string anyways.

we should add this note to the LUD 06 description.

Yeah I agree, this should definitely be in the spec.

from lightning-browser-extension.

@aaronbarnardsound
you try to pay to a LN address? Why do you even need to do something with the description _hash on the node? don't you get an bolt11 invoice via HTTP, then you can validate the description_hash (invoice description_hash == hash(metadata) ) and pay the bolt11 through CLN.

what am I missing?

from lightning-browser-extension.

@bumi yep we get a bolt11 to pay via http and then instruct CoreLN to pay it using the pay RPC method. From the docs:

description is only required for bolt11 invoices which do not contain a description themselves, but contain a description hash: in this case description is required. description is then checked against the hash inside the invoice before it will be paid.

We do perform an internal check that checks the hashed metadata string against the description_hash in the returned invoice and it all checks out client side. The problem is that we need to pass the metadata to CLN via the description parameter as a raw object (not a string, so in the case of metadata an array) and then CLN will then stringify and hash and check against the description_hash which fails due to the spaces being removed before hashing. This makes the invoice unpayable with CLN.

Let me know if that clarifies!

from lightning-browser-extension.

sorry I still don't fully understand (it might be too late here :D)
why do you need to pass the metadata to CLN? which CLN call do you make?

from lightning-browser-extension.

No worries! We are using the pay RPC method to pay the payment request. That method requires that a description parameter is also passed in if the payment request has a description_hash.

From the docs for the pay method:

description is only required for bolt11 invoices which do not contain a description themselves, but contain a description hash: in this case description is required. description is then checked against the hash inside the invoice before it will be paid.

This is required so that CoreLN can validate that the hash of the description matches the description_hash.

from lightning-browser-extension.

oohh, ok...sorry did not think of that CLN requirement.
The issue is that when I make this change we break things for some users who have a static copy of our lnurl-pay response on their own domain. (like: https://nvk.org/alby-lnurlp )
I will try to give a bit of notice before we deploy the change.

from lightning-browser-extension.

Ah damn! Yeah I was hoping it would not be a breaking change. Is there a way that you could make it backwards compatible for the existing static lnurl pay links but make the change for all new ones?

from lightning-browser-extension.

@aaronbarnardsound deployed the update. can you check if this is now correct? thanks

from lightning-browser-extension.