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

Add support for Nostr Zaps #9

Merged
merged 2 commits into from
Mar 8, 2023
Merged

Add support for Nostr Zaps #9

merged 2 commits into from
Mar 8, 2023

Conversation

mutatrum
Copy link
Contributor

No description provided.

@mutatrum
Copy link
Contributor Author

Pending blc-org/una#37.

@mutatrum
Copy link
Contributor Author

Also pending jb55/nostr-js#11, currently the zap request signature is not verified.

@mutatrum
Copy link
Contributor Author

This is fine to run on LND unless you have gc-canceled-invoices-on-the-fly=true configured.

@sommerfelddev
Copy link

Does this work if I run the branch or do I need to wait for the issues you mentioned to get fixed as well?

@mutatrum
Copy link
Contributor Author

mutatrum commented Feb 22, 2023

I ran into a problem because of gc-canceled-invoices-on-the-fly=true. If you don't have that set in your LND config, I think it defaults to false and it should be ok to run then, but I have not tested this.

The other issue is that it currently doesn't verify the signature on the zap request.

@ok300
Copy link

ok300 commented Feb 22, 2023

Each new zap will create a zap request

ligess/nostr.js

Lines 76 to 78 in 216d0e2

const storePendingZapRequest = (paymentHash, zapRequest, comment) => {
pendingZapRequests[paymentHash] = {zapRequest: zapRequest, comment: comment}
}

which the server will remember (i.e. store in RAM) until it either expires or is paid.

What is preventing someone from DDoS-ing via an endless stream of zap requests?

@mutatrum
Copy link
Contributor Author

Good point, although this is already a DDoS vector for any lightning address handler. I don't see a rate limit option, as one can generate a new pubkey for every request and make valid zap requests.

A simple way of at least preventing OOM would be to define a max number of entries and remove them FIFO.

@seak1234
Copy link

Hey, thanks for your work, this is truly amazing! Got it the zaps to work.

To the other rookies out there, these are some steps I had to do additionally to the README.md instructions (other than checking out the specific pull request (ID=9) from github):

  1. before running "yarn install" I actually had to install yarn first:
    sudo echo "deb https://dl.yarnpkg.com/debian/ stable main" | sudo tee /etc/apt/sources.list.d/yarn.list
    sudo apt-get update
    sudo apt-get install yarn

  2. I also had to install the nodemon module for npm:
    sudo npm install -g nodemon

  3. I personally then also had to change the port of this app. If you have issues you can change it in this file: router.js

Remember to use HEX (!) values for the macaroons and nostr private key.

Again, thank you so much, this is fun!

@ok300
Copy link

ok300 commented Feb 23, 2023

@mutatrum yes, configuring a "max number of pending zaps" limit can defend against OOM.

But ironically that enables another kind of DoS attack, whereby the attacker floods with enough requests to reach that limit. At that point, nobody else can zap to this receiver.

Is there a way to defend against that?

There's a similar thread in the BTCPay repo: btcpayserver/btcpayserver#4642 (reply in thread)

@mutatrum
Copy link
Contributor Author

mutatrum commented Feb 23, 2023

I used npm for the install, and that was a straightforward npm install. Can't remember doing anything else to be honest. I haven't used the yarn installation method.

Adding support for npriv and custom ports can be done, but probably outside the scope on the current PR. Both are trivial to add but I don't want to make this changeset much larger.

@mutatrum
Copy link
Contributor Author

Is there a way to defend against that?

Not that I can think of, other than rate-limiting the HTTP requests, the rest is trivial to generate. Either way at some point the service will be unavailable during the attack, but that's the nature of open internet I guess.

Thanks for the thread link, added a comment there.

@sommerfelddev
Copy link

3. I personally then also had to change the port of this app. If you have issues you can change it in this file: router.js

Looking at the relevant code:

process.env.PORT || 3000,

You should be able to change the binding port by passing the environment variable PORT in the .env file
E.g.:

PORT=<some other port other than the default(3000)>

@Sebastix
Copy link

Can confirm the zaps are working here, it's running now for two days. Some people had some errors but I think it's due their used clients. Still debugging with them but nothing has come out relevant for this integration of zaps.

@mutatrum
Copy link
Contributor Author

Some clients are buggy. I just had a zap request without any relays, so it didn't send the zap note.

@sommerfelddev
Copy link

It's working for me: note17pfxx953dym0lzlf5flxu6ztdnjgdfqsuk83ghu7f9fmxv0vkmvq0dglaf
Thanks @mutatrum !

package.json Outdated Show resolved Hide resolved
@mutatrum
Copy link
Contributor Author

mutatrum commented Mar 1, 2023

Should it return an error if the zap request fails any of the verifications? Currently it logs the issue, but still creates the invoice. Problem with this is that it leads to silent failures of zaps not working. Upside is that even with buggy implementations, payment will work, just not the zap.

@mutatrum
Copy link
Contributor Author

mutatrum commented Mar 6, 2023

All all pending PRs have been resolved and dependencies have been bumped. I also changed the error handling to return an error on any invalid zap requests. Running this version now, to see if anything weird pops up.

@Dolu89 Dolu89 merged commit 9237d82 into Dolu89:master Mar 8, 2023
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.

6 participants