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

Timeout wallet payments #1558

Open
huumn opened this issue Nov 8, 2024 · 7 comments
Open

Timeout wallet payments #1558

huumn opened this issue Nov 8, 2024 · 7 comments
Labels
enhancement improvements to existing features wallets
Milestone

Comments

@huumn
Copy link
Member

huumn commented Nov 8, 2024

Wallet payments can fail silently, e.g. the NWC consumer doesn't respond to a payment request, in which case we will wait until the invoice expires (10 minutes in most cases) - which is way too long. Stackers will assume we're unresponsive.

It's not as simple as timing out and displaying a QR code for the same invoice however. It's possible the QR code is paid and then the NWC consumer comes online and attempts to pay the same invoice. This is dangerous because once the QR code is paid, the preimage for the payment has been revealed, and the sats in the second payment can be stolen by nodes along the route that know the preimage.

To timeout safely, we'll want to cancel the original invoice, then generate a new invoice to display in the QR.

@huumn huumn added enhancement improvements to existing features wallets labels Nov 8, 2024
@riccardobl
Copy link
Member

this is solved by #1523

@ekzyis
Copy link
Member

ekzyis commented Nov 9, 2024

Can you extract the commit from #1523 for this into a separate PR if it doesn't mess/isn't related too much with the other code in it? #1523 already has a lot of changes and we can merge and review a smaller PR faster.

@riccardobl
Copy link
Member

Can you extract the commit from #1523 for this into a separate PR if it doesn't mess/isn't related too much with the other code in it? #1523 already has a lot of changes and we can merge and review a smaller PR faster.

I'll see what i can do, but the fix is a side effect of the different retrying mechanism. Anyway i am cleaning up the pr, so maybe we'll end up with fewer changes

@riccardobl
Copy link
Member

correction: my PR fixes only this part

This is dangerous because once the QR code is paid, the preimage for the payment has been revealed, and the sats in the second payment can be stolen by nodes along the route that know the preimage.

the use-paid-mutation calls retryPaidAction that, with the changes in the PR, cancels the previous invoice and returns a new one for the QR code.

However it doesn't change the long timeout

@ekzyis
Copy link
Member

ekzyis commented Nov 11, 2024

Wallet payments can fail silently, e.g. the NWC consumer doesn't respond to a payment request, in which case we will wait until the invoice expires (10 minutes in most cases) - which is way too long. Stackers will assume we're unresponsive.

Since this ticket was created because of an unresponsive territory payment, can't we control the timeout simply via low invoice expiration for invoices that pay SN from an attached wallet?

We can't do this for p2p payments because we don't control the outgoing invoice and expiry of incoming invoice must be higher and have a buffer, see here:

// validate the expiration
if (new Date(inv.expires_at) < new Date(Date.now() + INCOMING_EXPIRATION_BUFFER_MSECS)) {
throw new Error('Invoice expiration is too soon')
} else if (new Date(inv.expires_at) > new Date(Date.now() + MAX_EXPIRATION_INCOMING_MSECS)) {
// trim the expiration to the maximum allowed with a buffer
wrapped.expires_at = new Date(Date.now() + MAX_EXPIRATION_INCOMING_MSECS - INCOMING_EXPIRATION_BUFFER_MSECS)
} else {
// give the existing expiration a buffer
wrapped.expires_at = new Date(new Date(inv.expires_at).getTime() - INCOMING_EXPIRATION_BUFFER_MSECS)
}

But stackers don't wait for these payments since they are handled in the background so I think failing fast is less important.

Maybe this ticket was only about SN invoices anyway.

@huumn
Copy link
Member Author

huumn commented Nov 11, 2024

We can. This issue is mostly about falling back to QR after a reasonable amount of time, so I'm not sure how that fits into what you're saying.

For this case, we probably want to throw a TimeoutError, cancel the original invoice, and popup a QR with a fresh invoice.

@ekzyis
Copy link
Member

ekzyis commented Nov 11, 2024

This issue is mostly about falling back to QR after a reasonable amount of time, so I'm not sure how that fits into what you're saying.

waitForWalletPayment should fail automatically if the invoice expires because we're polling for the payment next to it (and if not, we can make it fail). So if the invoice expires in 15 seconds, we will show them a QR in 15 seconds. That should solve the issue using invoice expiry instead of timeout on payments + cancel (maybe we should still cancel the invoice to make sure even after it expired).

Mentioning this because it seems unnecessary to me to use timeouts in our application code when we can "timeout the invoice" itself.

@ekzyis ekzyis added this to the November 5 milestone Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvements to existing features wallets
Projects
None yet
Development

No branches or pull requests

3 participants