receiver and sender fallback #1523

riccardobl · 2024-10-27T13:49:12Z

The PR implements fallbacks on receiver, lnurlp for attached wallets and does some refactoring to the wallet interface.

Closes #1496
Closes #1494
partially solves #1558 by cancelling and creating a new invoice before showing the qr code

Changelog:

made pessimistic actions retryable
implemented receiver fallback
implemented sender fallback

Overview

This PR enhances payment reliability between sender and receiver wallets by adding fallback mechanisms.

The first part ensures that if a receiver’s wallet fails to generate a valid and wrappable invoice, the system will automatically attempt to create the invoice with the next available wallet, repeating this process until a valid invoice is obtained.

The second part addresses situations where a sender might have difficulty paying the invoice. Instead of simply trying a single payment path, the PR introduces a retry system that cycles through different sender-receiver wallet pairs. This way, if the first pair can’t connect, the system will increment an attempt counter and try another receiver wallet to find a working path. This approach helps avoid payment failures caused by both wallets being online but unable to connect directly.

If these attempts still don’t succeed, the system will prioritize fee credits or reward sats. ie. if a p2p payment was prioritized initially by the action but fails, the system will reprioritize internal payment options. This prioritization allows the payment to go through automatically if sufficient funds are available. If these methods do not work, the system will fallback to the next paymentMethod in the action, and generate a new invoice, which the client displays as a QR code for the user to complete the payment manually.

P2P preference flow

When a paid action is tried, the serverside code will go through the payment methods supported by the action, following the priority order. This behavior can be mutated by passing:

forceInternal : to use only the internal payment methods that are supported by the action (currently only FEE_CREDIT)
prioritizeInternal : to move the internal payment methods at the beginning in the priority list, making them the preferable methods while keeping all the others as fallbacks.

The paid action mutation returns a retriable flag that signals the sender it can retry the action using retryPaidMutation without changing anything, but just by increasing the attempt counter. This means the server side code has other ways to make the the transaction happen that were not tried yet. In the scope of this PR it means it has more p2p attachments to try.

Whenever a payment fails, the clientside code queries the paidAction to check if there is an invoiceForward's withdrawl status

if this is present the clientside code can determine that the payment went through on his side but failed to be forwarded on the other side, this will make the clientside code increase the attempt counter and retry if retriable is true, to let the serverside code use the next p2p attachment on the receiving side
if this is not present the clientside code will mark the sender as failed and retry with the next sender without increasing the attempt counter. This in turn will make the serverside code reuse the same receiving attachment.

If all the senders have failed or the retriable flag is false (all the receiver have failed), the clientside code will fallback to retry the payment using prioritizeInternal to hopefully complete it with FEE_CREDITS, if this fails too an invoice will be returned and passed to the qr code payment.

Note: this is not explicit in the code, but it is an effect of the logic: if we previously tried all the receivers and the forward failed for all of them, we will have an attempt counter high enough to exceed the number of receiving wallets, this will skip the p2p method, as it should, since there is no way to pay, even by qr code.

Non P2P flow (not internal priority)

The paid actions is tried, the response is returned with a retriable flag always set to false and the invoice to pay.
The clientside code tries to pay with every sender, once all the senders have failed it fallbacks to use prioritizeInternal, if this fails, an invoice is returned for the qr code

Non P2P flow (internal priority)

The paid actions is tried, the response is returned with a retriable flag always set to false and the invoice to pay.
The clientside code tries to pay with every sender, once all the senders have failed it fallbacks to use prioritizeInternal(FIXME: this is useless, it attempts the same thing twice, it should just skip instead)

if this fails, an invoice is returned for the qr code.

Note on invoice reuse

Every time a new attempt is made, the old invoice is cancelled and a new invoice is regenerated

Tests

setup

stacker1:
- fee credits: 1000
- sending attachments [good, bad]
- receiving attachments []
stacker2:
- fee credits: 0
- sending attachments: [bad, good]
- receiving attachments: []
stacker3:
- fee credits: 1000
- sending attachments: [bad,bad]
- receiving attachments []
stacker4:
- fee credits: 0
- sending attachments: []
- receiving attachments: [good, bad]
stacker5:
- fee credits: 0
- sending attachments: []
- receiving attachments: [bad, good]
stacker6:
- fee credits: 0
- sending attachments: [bad]
- receiving attachments: [bad, bad]
stacker7:
- fee credits: 0
- sending attachments: []
- receiving attachments: []
stacker8:
- fee credits: 1 000 000
- sending attachments: []
- receiving attachments: []

Zap

send receive	stacker1	stacker2	stacker3	stacker4
stacker4	[x] p2p	[x] p2p	[x] cc	-
sacker5	[x] p2p	[x] p2p	[x] cc	[x] qr
stacker6	[x] cc	[x ] cc	[x] cc	[x] qr
stacker7	[x] cc	[x] cc	[x] cc	[x] qr

post result

stacker1 posts spending fee credits
stacker2 posts spending from attached wallet
stacker3 posts spending fee credits
stacker4 shows qr
stacker5 shows qr
stacker6 fails, shows qr on manual retry

LNURLP

stacker3: cc
stacker4: p2p
stacker5: p2p
stacker6: cc

Territory

stacker1 pays from attached wallet
stacker3 pays from qr code (bug: if you cancel the qr code it goes to error 404)
stacker8 pays from fee credits

riccardobl · 2024-11-02T16:46:54Z

on hold until #1507 is merged, i will rebase any change on top of the new code

riccardobl · 2024-11-06T19:54:36Z

components/use-paid-mutation.js

+      }
+      await waitForQrPayment(invoice, null, { persistOnNavigate, waitFor })
+      return { invoice, response }
+    }


anons gets the qr code right away

api/paidAction/boost.js

riccardobl · 2024-11-09T19:38:06Z

api/paidAction/index.js

-    }
+    context.me = context.me ? await context.models.user.findUnique({ where: { id: context.me.id } }) : undefined
+    context.actionAttempt = context.actionAttempt ?? 0
+    context.forceInternal = context.forceInternal ?? false


This replaces forceFeeCredits and it is currently used by things like auto territory renewal to force the code to use only internal payments (fee credits and reward sats).
The difference between forceInternal and the old forceFeeCredits is that forceInternal allows the code to use reward sats too, not only fee credits.

riccardobl · 2024-11-09T19:40:36Z

api/paidAction/index.js

+    context.me = context.me ? await context.models.user.findUnique({ where: { id: context.me.id } }) : undefined
+    context.actionAttempt = context.actionAttempt ?? 0
+    context.forceInternal = context.forceInternal ?? false
+    context.prioritizeInternal = context.prioritizeInternal ?? false


Contrary to forceInternal, this will just prioritize reward sats and fee credits.
This is used by the client side code after every sending wallet failed to pay every receiving wallet, so it will try to do an internal transaction, if this fails too they payment will fallback to the other methods in order, this gives another invoice that is then presented to the user with a qr code.

riccardobl · 2024-11-09T19:58:18Z

api/resolvers/notifications.js

+          SELECT 1
+          FROM "ItemAct"
+          WHERE "ItemAct"."invoiceId" = "Invoice".id
+        )


when we have a failed invoiced action that it is then retried successfully with a fee credit or reward sats payment, we have this situation where there is a failed invoice that used to be bound to this action but it is not anymore.
This check is to exclude it from notifications. Maybe there are other ways, but i am unsure.

api/resolvers/paidAction.js

riccardobl · 2024-11-09T20:01:21Z

components/use-paid-mutation.js

+        setInnerResult(r => addPayError(e, r))
+      })
+
+      if (wait) await p


i've de-duplicated the code, but the logic should be the same

ekzyis

I didn't run the code yet, but this is my review so far from looking at most of the code

wallets/server.js

api/paidAction/index.js

ekzyis · 2024-11-17T04:45:58Z

api/paidAction/index.js

+    if (forceInternal) {
+      //  we keep only the payment methods that qualify as internal payments
+      if (!me) {
+        throw new Error('user must be logged in to use internal payments')
+      }
+      const forcedPaymentMethods = []
+      // reset the supported payment methods to only include internal methods
+      // that are supported by the action
+      if (context.supportedPaymentMethods.includes(PAID_ACTION_PAYMENT_METHODS.FEE_CREDIT)) {
+        forcedPaymentMethods.push(PAID_ACTION_PAYMENT_METHODS.FEE_CREDIT)
+      }
+      // TODO: add reward sats
+      // ...
+      if (forcedPaymentMethods.length === 0) {
+        throw new Error('action does not support internal payments')
+      }
+      context.supportedPaymentMethods = forcedPaymentMethods
+    } else if (prioritizeInternal) {
+      // prefer internal payment methods over the others (if they are supported)
+      const priority = {
+        [PAID_ACTION_PAYMENT_METHODS.FEE_CREDIT]: -2
+        // add other internal methods here
+      }
+      context.supportedPaymentMethods = context.supportedPaymentMethods.sort((a, b) => {
+        return priority[a] - priority[b]
+      })


Reward sats don't need to be a separate payment method since they can only be used if you pay with fee credits (they get converted into ccs just-in-time).

I think if you add reward sats as a separate payment method, that would mean that they can also be used for p2p zaps which is not what we want.

This means that forcedPaymentMethods doesn't have to be an array, it will always only contain FEE_CREDIT.

This means I also think the distinction between "internal payment method" and "fee credits" isn't needed. Internal always means fee credits.

it seems like they will be implemented as a separated payment method in ex-nov-5 pr, that's why i made the distinction

https://github.com/stackernews/stacker.news/blob/nov-5/api/paidAction/itemUpdate.js#L9-L13

Mhh, I see. @huumn, does it make sense to add REWARD_SATS as a payment method? I understood it as an "implementation detail" of the FEE_CREDIT payment method where they are converted into CCs just-in-time. In that case, isn't it unnecessary as a separate payment method?

api/paidAction/index.js

components/payment.js

api/paidAction/index.js

wallets/server.js

ekzyis

I looked through the sending code in detail now and tested the fallback using LNbits that fails and NWC that succeeds. I noticed that the client cancels the invoice. Is there a specific reason for that?

I still need to test more, especially the receiving part.

components/qr.js

components/use-paid-mutation.js

components/poll.js

ekzyis · 2024-11-18T06:45:01Z

api/paidAction/index.js

-    actionId
+  if (failedInvoice.actionState !== 'FAILED') {
+    // you should cancel the invoice before retrying the action!
+    throw new Error(`actions is not in a retriable state: ${failedInvoice.actionState}`)


Why does the client have to cancel the invoice? Can't we do it here?

Canceling on the client is an additional round-trip and the code might be simpler if retrying implicitly cancels the existing invoice.

What do you think?

Hmm, I did consider implementing it like you've suggested but i've kept it this way because it is what the original code does.

But thinking about this in more depth, i think you are right, and that probably the transaction isolation should be serializable, because there is a race condition here (and in the original code), where two transactions could start with a committed 'failed' invoice state, but then end up with two different results.

What do you think?

ekzyis

Did you test this with multiple zaps in-flight? I can consistently reproduce a transaction timeout if I zap more than three times in quick succession:

2024-11-19.05-41-10.mp4

I am not sure what is causing this, but my first guess would be that there is a race condition that results in a deadlock.

This shouldn't touch any of the new receiving code since I am zapping someone who has no wallets attached.

Maybe it helps if you focus on getting the sending part right before caring about the receiving part.

Also noticed an issue with QR payments since there, the invoice is not refreshed.

I didn’t look further into the receiving part because I am already lacking confidence in the sending part.

ekzyis · 2024-11-19T02:22:04Z

components/qr.js

+            break
+          } catch (e) {
+            console.log(e?.message)
+          }


This will attempt to pay the same invoice again. So afaict, this will require the same retry logic as in usePaidMutation.

Since the logic in there isn't easily extractable to apply it here (as it is written, it requires to be in the scope of usePaidMutation), I think we need to rewrite the retry logic so we can reuse it here or QR codes can't use sender fallbacks.

ekzyis · 2024-11-19T02:27:55Z

components/use-paid-mutation.js

+          attempt++
+        } else {
+          // SN didn't receive the payment, so the sender must have failed
+          senderWallet.failed = true


Tbh, I am surprised this works to mark the wallet as failed in the array above. Apparently array.find() returns a reference if it's not a primitive value.

yeah, well technically speaking Javascript is natively oop and objects are always passed as reference, like in most oop languages that don't have pointers, copying the memory every time would be pretty bad.

ekzyis · 2024-11-19T02:33:10Z

components/use-paid-mutation.js

+        await invoiceHelper.cancel(invoice)
+        console.log('old invoice canceled')
+      } catch (err) {
+        console.error('could not cancel old invoice', err)


You're awaiting cancelInvoice but you're catching all errors here so you're actually never sure if the invoice was canceled.

You do check in the backend if the invoice was canceled so refreshInvoice will throw but it looks weird to me to let the backend tell you that the cancel failed on your next request when you can already "handle" that case here. Handle in quotation marks since I think we cannot recover from a failed cancel.

i am not sure, won't this throw if the invoice was cancelled twice or is expired too?
IMO it is better to not lock out the user from retrying an action just because we can't cancel an invoice, since the action could still be in the failing state for other reasons.

Canceling the previous invoice is pretty important since we don't want them to accidentally pay us twice or lose money. This code doesn't run exclusively for p2p zaps where we wouldn't be able to settle incoming invoices ourselves without the receiver settling first; this runs for every paid action. Our node would absolutely settle an old invoice even though they already paid for the action if the payment wasn't stolen on route already if it's the same invoice.

so the problem is exactly as you noticed:

Ok, i understand now this is reintroducing #1558 in the qr.js code.

So, this is an issue for optimistic actions, that can fail after the invoice is settled, but not for pessimistic actions that use hodl invoice and set the FAILED state on settling, is this correct?

riccardobl · 2024-11-19T08:57:26Z

I am not sure what is causing this, but my first guess would be that there is a race condition that results in a deadlock.

This shouldn't touch any of the new receiving code since I am zapping someone who has no wallets attached.

Maybe it helps if you focus on getting the sending part right before caring about the receiving part.

Also noticed an issue with QR payments since there, the invoice is not refreshed.

I didn’t look further into the receiving part because I am already lacking confidence in the sending part.

I don't think it is a deadlock, but the action (the lnd node?) being slow.
In your test, the first wallet fails, so you are getting into the retryPaidAction logic, that runs everything into a transaction, if the action is slow this causes the transaction to timeout.
I will think to a fix.

EDIT: At a second glance, only the actionState change needs the rollback, so i've moved this code inside the smaller transactions in performPaidAction, this should fix the issue

riccardobl · 2024-11-19T09:24:07Z

This will attempt to pay the same invoice again. So afaict, this will require the same retry logic as in usePaidMutation.

Since the logic in there isn't easily extractable to apply it here (as it is written, it requires to be in the scope of usePaidMutation), I think we need to rewrite the retry logic so we can reuse it here or QR codes can't use sender fallbacks.

The logic on use-paid-mutation is there because we are cancelling and recreating the invoices.
In this case we don't do anything like that, so i think we won't need the complex retry logic here

Ok, i understand now this is reintroducing #1558 in the qr.js code.

We don't know where this invoice is coming from in the qr logic, so how do we ask for another one?
To keep this code flexible, it is probably better to just disable the fallback in this case. (Done in the last commit)

ekzyis · 2024-11-19T14:35:27Z

We don't know where this invoice is coming from in the qr logic, so how do we ask for another one?

Why do we not know where the invoice is coming from? Yes, the client can't tell but the backend can, that's why you need the same refresh logic.

To keep this code flexible, it is probably better to just disable the fallback in this case. (Done in the last commit)

Okay for now but I don't understand the argument about "to keep this code flexible". What is flexible about this code? The problem is that your retry code is not flexible enough to easily apply it here.

riccardobl · 2024-11-19T14:46:04Z

Why do we not know where the invoice is coming from? Yes, the client can't tell but the backend can, that's why you need the same refresh logic.

I am very confused. If this QR code is used in use-paid-mutation, what is the point of trying to pay with attached wallets if this is shown only after all the attached wallets fail?
If it is used elsewhere, we can't use the retry logic from the paidActions. Am i missing something?

argument about "to keep this code flexible"

My argument is that i was under the impression that this code was to use attached wallets to pay non paidActions invoices (eg. a withdraw or something), and we would need a retry mechanism that is specific for what we want to pay.

ekzyis · 2024-11-19T14:59:32Z

I am saying that qr.js contains code to pay with an attached wallet and thus it probably makes sense to also have fallbacks here for a consistent experience. I am not saying that we should use the code in qr.js in use-paid-mutation but the other way around.

Maybe we don't need paying from attached wallets for QR codes anymore if a QR code is indeed only shown if all wallets already failed in all cases, but I would need to check myself first. All I saw is that there was a loop that would pay the same invoice again.

riccardobl · 2024-11-19T15:12:22Z

Maybe we don't need paying from attached wallets for QR codes anymore if a QR code is indeed only shown if all wallets already failed in all cases, but I would need to check myself first. All I saw is that there was a loop that would pay the same invoice again.

From what i can tell, the QR code wallet payment is used only in pages/invoices/[id].js.
To me, this seems like a generic interface to pay invoices.

riccardobl marked this pull request as draft October 27, 2024 22:54

riccardobl changed the title ~~minor wallet refactoring (server only)~~ fallback + lnurlp and minor refactoring with types (server only) Oct 28, 2024

riccardobl mentioned this pull request Oct 28, 2024

Improve wallet fallbacks #1489

Closed

riccardobl force-pushed the walletref branch 2 times, most recently from 8b22e03 to e647d2e Compare October 30, 2024 00:19

riccardobl added the blocked label Nov 2, 2024

riccardobl force-pushed the walletref branch from 49e4fb9 to 36c8cfb Compare November 4, 2024 13:39

riccardobl changed the title ~~fallback + lnurlp and minor refactoring with types (server only)~~ receiver fallback and p2p lnurlp Nov 4, 2024

riccardobl removed the blocked label Nov 4, 2024

riccardobl marked this pull request as ready for review November 4, 2024 14:05

riccardobl marked this pull request as draft November 4, 2024 21:36

riccardobl changed the title ~~receiver fallback and p2p lnurlp~~ receiver and sender fallback and p2p lnurlp Nov 4, 2024

riccardobl force-pushed the walletref branch 2 times, most recently from c36b04e to 860fab0 Compare November 6, 2024 13:08

riccardobl commented Nov 6, 2024

View reviewed changes

riccardobl mentioned this pull request Nov 8, 2024

Timeout wallet payments #1558

Open

riccardobl changed the base branch from master to nov-5 November 8, 2024 15:58

riccardobl force-pushed the walletref branch from 6bffd20 to 78988a7 Compare November 8, 2024 15:59

riccardobl force-pushed the nov-5 branch from f762165 to 9f9804a Compare November 8, 2024 16:42

riccardobl force-pushed the walletref branch 4 times, most recently from 707826c to 994b69d Compare November 9, 2024 19:30

riccardobl commented Nov 9, 2024

View reviewed changes

api/paidAction/boost.js Show resolved Hide resolved

riccardobl commented Nov 9, 2024

View reviewed changes

api/resolvers/paidAction.js Show resolved Hide resolved

riccardobl commented Nov 9, 2024

View reviewed changes

riccardobl force-pushed the walletref branch from 952d705 to a016427 Compare November 14, 2024 14:26

riccardobl added 8 commits November 14, 2024 15:37

comments

bf04342

expose only the withdrawl status

22c0e80

bail early if the invoice was canceled or expired

0fab13d

login check for retryPaidAction

468df05

comments

0e8b8ce

cleanup

a6ffcda

Merge remote-tracking branch 'upstream/master' into walletref

0df1b28

lint fix

8567815

ekzyis reviewed Nov 17, 2024

View reviewed changes

riccardobl added 3 commits November 17, 2024 10:43

fix merging issue and apply review suggestions

4e96d5d

remove unrelated change

f94c92b

remove unused import

03f9a92

ekzyis reviewed Nov 18, 2024

View reviewed changes

apply review suggestions

811aa6c

riccardobl requested a review from ekzyis November 18, 2024 09:49

riccardobl added 3 commits November 18, 2024 18:26

fix bigint conversion lost in merge

f3e80b1

move wallet map to failing states in use-paid-mutation

6a1d184

Merge branch 'master' into walletref

67a17cc

ekzyis requested changes Nov 19, 2024

View reviewed changes

fix tx timeout

69b59b8

riccardobl requested a review from ekzyis November 19, 2024 09:24

no fallback in qr payment

1dc983f

throw if invoice fails to cancel

ecb908a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

receiver and sender fallback #1523

receiver and sender fallback #1523

riccardobl commented Oct 27, 2024 •

edited

Loading

riccardobl commented Nov 2, 2024

riccardobl Nov 6, 2024

riccardobl Nov 9, 2024 •

edited

Loading

riccardobl Nov 9, 2024 •

edited

Loading

riccardobl Nov 9, 2024

riccardobl Nov 9, 2024

ekzyis left a comment

ekzyis Nov 17, 2024

riccardobl Nov 17, 2024

ekzyis Nov 18, 2024

ekzyis left a comment

ekzyis Nov 18, 2024

riccardobl Nov 18, 2024 •

edited

Loading

ekzyis left a comment

ekzyis Nov 19, 2024

ekzyis Nov 19, 2024

riccardobl Nov 19, 2024 •

edited

Loading

ekzyis Nov 19, 2024

riccardobl Nov 19, 2024 •

edited

Loading

ekzyis Nov 19, 2024 •

edited

Loading

riccardobl Nov 19, 2024

riccardobl commented Nov 19, 2024 •

edited

Loading

riccardobl commented Nov 19, 2024 •

edited

Loading

ekzyis commented Nov 19, 2024 •

edited

Loading

riccardobl commented Nov 19, 2024 •

edited

Loading

ekzyis commented Nov 19, 2024 •

edited

Loading

riccardobl commented Nov 19, 2024

receiver and sender fallback #1523

Are you sure you want to change the base?

receiver and sender fallback #1523

Conversation

riccardobl commented Oct 27, 2024 • edited Loading

Overview

P2P preference flow

Non P2P flow (not internal priority)

Non P2P flow (internal priority)

Note on invoice reuse

Tests

setup

Zap

post result

LNURLP

Territory

riccardobl commented Nov 2, 2024

Choose a reason for hiding this comment

riccardobl Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

riccardobl Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekzyis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekzyis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardobl Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

ekzyis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardobl Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardobl Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

ekzyis Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riccardobl commented Nov 19, 2024 • edited Loading

riccardobl commented Nov 19, 2024 • edited Loading

ekzyis commented Nov 19, 2024 • edited Loading

riccardobl commented Nov 19, 2024 • edited Loading

ekzyis commented Nov 19, 2024 • edited Loading

riccardobl commented Nov 19, 2024

riccardobl commented Oct 27, 2024 •

edited

Loading

riccardobl Nov 9, 2024 •

edited

Loading

riccardobl Nov 9, 2024 •

edited

Loading

riccardobl Nov 18, 2024 •

edited

Loading

riccardobl Nov 19, 2024 •

edited

Loading

riccardobl Nov 19, 2024 •

edited

Loading

ekzyis Nov 19, 2024 •

edited

Loading

riccardobl commented Nov 19, 2024 •

edited

Loading

riccardobl commented Nov 19, 2024 •

edited

Loading

ekzyis commented Nov 19, 2024 •

edited

Loading

riccardobl commented Nov 19, 2024 •

edited

Loading

ekzyis commented Nov 19, 2024 •

edited

Loading