Skip to content

NIP-57: drop description_hash validation for zaps#788

Open
callebtc wants to merge 1 commit intonostr-protocol:masterfrom
callebtc:nip57/drop_description_hash
Open

NIP-57: drop description_hash validation for zaps#788
callebtc wants to merge 1 commit intonostr-protocol:masterfrom
callebtc:nip57/drop_description_hash

Conversation

@callebtc
Copy link
Copy Markdown

This PR drops the description_hash requirement for Nostr zaps in accordance with a proposed update of LUD06 that has been well-received and already implemented by various Lightning wallets.

Background

Currently, the zap Server is required to respond with an invoice that has a description_hash set to the zap request sent in the LNURL-pay request by the Payer. Nostr Clients are supposed to parse zap responses and check that the SHA256(description) of the zap response matches the description_hash of the bolt11 field in the zap response.

Rationale

Similar to LNURL-pay, the purpose of this mechanism is to make Lightning invoices commit to the event being zapped. Unfortunately, this doesn't really improve anything since we still fully trust the Server to not inject a malicious invoice with the same description_hash as nothing prevents the server from doing so. This can also be done with invoices that are never actually paid. Since the description_hash requirement is redundant for bare LUD06, the same is true for zaps.

The construction of such a description_hash invoices with some Lightning backends such as CLN poses a major implementation hurdle (CLN requires you to pass and store the entire description, even if you don't use it). For some Lightning backends, it isn't possible to create description_hash invoices at all.

At the same time, the validation of zap receipts is one of the biggest challenges when implementing a zap parser which is why the majority of nostr clients completely ignore this rule to begin with. This PR harmonizes the spec with those implementations.

I propose to drop the description_hash requirement for zaps, starting with nostr clients that should not validate the description_hash in zap receipts. Zap Servers can update later by removing the construction of such invoices.

@fiatjaf fiatjaf requested review from jb55 and v0l September 20, 2023 11:39
@jb55
Copy link
Copy Markdown
Contributor

jb55 commented Sep 20, 2023 via email

@jb55
Copy link
Copy Markdown
Contributor

jb55 commented Sep 20, 2023 via email

@callebtc
Copy link
Copy Markdown
Author

callebtc commented Sep 20, 2023

Some more context: when I create deschash invoices via the invoice RPC in CLN, CLN will actually store the description in its database on invoice creation. Later when I am reacting to paid invoices I can pull this description even if its a deschash invoice.

This doesn't change how you create invoices, you can still do it that way. It shouldn't break your use case (at least the server) as this is a relaxation of requirements, not a tightening.

I don't see how my zapper could validate them before zapping, or even know if the paid invoice is a zap request invoice.

That's the same reason when it was implemented in LNURL-p but nobody ever makes use of that. As mentioned, everything is fakeable. If you assume a malicious server, it could just add the appropriate description_hash to a malicious invoice as there is nothing that prevents it from doing so.

@bitkarrot
Copy link
Copy Markdown
Contributor

This doesn't change how you create invoices, you can still do it that way. It shouldn't break your use case (at least the server) as this is a relaxation of requirements, not a tightening.

I say, Do It. Just drop the validation.

@jb55
Copy link
Copy Markdown
Contributor

jb55 commented Sep 21, 2023 via email

@callebtc
Copy link
Copy Markdown
Author

does this mean damus will have to stop validating description hashes on the client side or else some zaps will break for damus users?

No it won't break anything. Clients should simply not validate description hashes anymore. Servers should can drop description hashes at a later time.

@v0l
Copy link
Copy Markdown
Member

v0l commented Oct 15, 2023

@vitorpamplona
Copy link
Copy Markdown
Collaborator

Amethyst doesn't check as well.

@jb55
Copy link
Copy Markdown
Contributor

jb55 commented Oct 16, 2023 via email

@fiatjaf
Copy link
Copy Markdown
Member

fiatjaf commented Oct 16, 2023

I agree with @jb55.

For the LUD-06 case description_hash stopped having any meaning once the ecosystem in general decided that signed bolt11 invoices and descriptions and preimages had no value whatsoever, contrary to the original design of the invoice protocol.

I think the same reasoning does not apply for NIP-57, because the event that is hashed does have a meaning. Or maybe you can say it doesn't, but it wouldn't be for the same reasons that apply in the LNURL case.

@Egge21M
Copy link
Copy Markdown
Contributor

Egge21M commented Feb 1, 2024

I'd like to revisit this, as the requirement for description_hash validation inside NIP-57 is blocking people from using Lightning providers/backends that do not support description_hash invoices. My recent endeavor in building a Cashu based LNURL and Zap provider is one example.

This requirement only adds friction. If the idea is to stop Zap providers from forging Zap receipts than having the request as part of the tags would be enough, no? And as @jb55 already mentioned Zap providers could still replay their own and every other request. Thinking about how quick most implementers were to remove description_hash validation, it would surprise me if anyone right now was guarding against such replays...

@callebtc
Copy link
Copy Markdown
Author

callebtc commented Feb 1, 2024

I still think the check is useful for this purpose, otherwise what's the
point of description_hash invoices if we can't use them to check things
like this?

There is no point of description_hash invoices, they don't provide any security.

at least you could show that the nostr pubkey creating
the zap request was the one that initiated the request and that it was
associated with the invoice being paid.

Not quite accurate. You can show that a user intended to zap (by signing an event) and that's the end of the story of what you can prove. Everything else relies on trust in the zap server.

I think believing that these measures are for security or authenticity is playing a pretend game and everyone who would like to cheat can still do so, it's just marginally more annoying to cheat with the description_hash but definitely doesn't stop anyone.

If that's the case, then I think the approach is flawed and we should stop pretending that this offers any meaningful security or authenticity and go with the simpler solution that offers the same guarantees.

@jb55
Copy link
Copy Markdown
Contributor

jb55 commented Feb 1, 2024 via email

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

Successfully merging this pull request may close these issues.

7 participants