Receiver fallbacks

[ekzyis]

Description

Receiver fallbacks are implemented by making getInvoiceableWallets aware of previously failed wrapped invoices. If the id of such a failed invoice is passed to it (predecessorId), it will skip any wallets for which a forward was already attempted.

For this, a column predecessorId was added to the Invoice table which points to the previously failed invoice.

With this link back to the "predecessor invoice", we can check which wallet was already tried and skip that wallet in this recursive query.

Since we want to retry with the same sender wallet on receiver errors, the client is made aware of receiver errors via the existing invoice polling. If status is FAILED_FORWARD or the invoice is held + cancelled, the invoice polling will throw WalletReceiverError now. We need to check both since it's dependent on timing what the client will see first (FAILED_FORWARD transitions almost immediately to FAILED).

If there is no more receiver wallet to try, we fallback to an invoice that pays SN and credits the receiver with CCs.

close #1494 based on #1679 based on #1700

TODOs

• don't log failed forwards as sender errors
• make sure we avoid infinite retry loops if there ever is a bug?

Tests

• sender with 1 good wallet and no CCs pays receiver with 2 bad wallets: falls back to CCs
• sender with 1 bad and 1 good wallet and no CCs pays receiver with 2 bad wallets: falls back to CCs
• paying with CCs (even though both have wallets attached)
• sender with 1 bad and 1 good wallet and no CCs pays receiver with 2 good wallets: p2p zap success to first good receiver wallet

Additional Context

• I am testing failed forward using this patch:

diff --git a/worker/paidAction.js b/worker/paidAction.js
index 70396ddb..9d711b0f 100644
--- a/worker/paidAction.js
+++ b/worker/paidAction.js
@@ -268,7 +268,7 @@ export async function paidActionForwarding ({ data: { invoiceId, ...args }, mode
     payViaPaymentRequest({
       lnd,
       request: bolt11,
-      max_fee_mtokens: String(maxFeeMsats),
+      max_fee_mtokens: String(0),
       pathfinding_timeout: LND_PATHFINDING_TIMEOUT_MS,
       max_timeout_height: maxTimeoutHeight
     }).catch(console.error)

• The properties wallet, message, reason and invoice of the custom errors can probably be moved into the parent class but that should be refactored in a separate PR imo.

• Receiver fallbacks #1688 (comment)

Checklist

Are your changes backwards compatible? Please answer below:

yes, this only adds a nullable database column

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

7. See tests above and I looked very closely at the code for extended periods of time

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

n/a

Did you introduce any new environment variables? If so, call them out explicitly here:

no

[riccardobl]

Won't this cause unwanted CC payments?
I think it should prioritize p2p if the receiver has attached wallets.
If the receiver doesnt' have an attached wallet of if they all failed, it should try to pay with CC -> CC.
And only if CC->CC is not possible it should use LN->CC.
So it doesn't end up locking more and more sats into CCs or prioritizing them against p2p, otherwise i think the system will end up becoming almost CC exclusive.

That's what i did in my original PR.

[ekzyis]

I think it should prioritize p2p if the receiver has attached wallets.

I think zaps should be paid in the interest of the sender by default. Therefore, without a setting where the sender can express their preference that depends on if the receiver can receive real sats, I think we should try CCs first.

I think it would also be confusing if we try CCs first for posts and replies but not for zaps.¹ Preferring sats over CCs should be opt-in imo so this would need a setting which I think we can add later if there's enough demand from users. We shouldn't assume too much about user preferences and I think receiver fallbacks without this setting are valuable enough by themselves already.

So currently, I think it's not worth it to add the complexity of two different payment flows (one for SN fees, one for zaps²) where one even now also includes a sending dependency on receiving next to the already existing dependency of receiving on sending (a receiver cannot avoid to get paid in CCs if the sender only has CCs). The dependency of the receiver on the sender is already tricky UX-wise (need to explain to them that their wallet isn't broken but what they receive depends on the sender etc) so I'd rather not make sending also depend on receiving unless there is enough user demand for such a thing.

Thanks for flagging though, this wasn't on my mind! I'll look into which changes would be required in theory to accommodate this if/when we need it while I review my implementation.

I am also sorry if I missed some existing discussion between @huumn and you about this. I am pretty sure I did. Could be that I am also disagreeing with @huumn regarding this here 👀

otherwise i think the system will end up becoming almost CC exclusive.

30% of each zap goes into rewards (which become sats) so the half life of a CC is almost exactly two zaps: 1 * 0.7 * 0.7 = 0.49 < 0.50.

Therefore, I think how users zap each other doesn't matter much for the amount of CCs in the system, it matters more how many use attached wallets. If everybody has an attached wallet (send+recv), CCs will cease to exist quickly enough.

Footnotes

1. I assume for posting fees it's clear that everybody will want to try CCs first and we won't need a setting? ↩

2. was this handled in your PR? ↩

[huumn]

#1678 tries p2p first if the sender and receiver has an attached wallet. If the receiver wallet fails, then I think the sender should just do CCs (either those they already have, or jit purchasing them).

Either way, it shouldn't require a massive overhaul to change the priority of payment methods. I'd just get it working however you see fit. Sender and receiver fallbacks should be pretty well encapsulated from payment method fallback, and we can massage it however we need.

[ekzyis]

Converted to draft because this requires a rebase on #1700

[gitguardian]

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
14821997	Triggered	Elliptic Curve Private Key	5910f6b	docker/lnd/router/tls.key	View secret

🛠 Guidelines to remediate hardcoded secrets

1. Understand the implications of revoking this secret by investigating where it is used in your code.
2. Replace and store your secret safely. Learn here the best practices.
3. Revoke and rotate this secret.
4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

• following these best practices for managing and storing secrets including API keys and other credentials
• install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

---

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

[ekzyis]

Okay, superficially tested again based on #1700 and worked as expected, this is ready for review again.

[ekzyis]

Mhhh, I think WHERE "Retries"."isHeld" = 't' instead of "InvoiceForward"."withdrawlId" IS NOT NULL would also work and would not require a join 🤔

But checking for a withdrawal is safer since isHeld set only means the sender wallet didn't fail but not necessarily that the withdrawal failed. Could be that something in between failed.

However, the client currently considers failed + isHeld as a receiver error.

Basically, the client considers every failure after it was able to pay as a receiver error even though maybe SN failed to do something and here we only consider something as a receiver failure if a withdrawal exists ...

This could be a bug in theory 👀

[huumn]

we can check actionError to be sure

[ekzyis]

It is currently not set for forward failures but I can change that if you meant that

[huumn]

I have a fix I'm working on locally. I just need to make sure I didn't regress it.

[huumn]

I don't know if this changed or was always there, but fees are being deducted from p2p zap receives and shouldn't be

[huumn]

Then my notification adds the fee too

[ekzyis]

but fees are being deducted from p2p zap receives and shouldn't be

Mhh, I thought I fixed that at some point. If the wallet created a 70 sat invoice, it should obviously receive 70 sats since senders pay network fees.

Then my notification adds the fee too

This is also wrong, right? Imo, the notification should show how much you received and if you click on it, the fees are shown separately.

So for a 100 sat zap, it should show "70 sats were zapped directly to your attached wallet" and if you click into it, it will show "70 sats with 1 sat routing fee".

Does this make sense / do you agree?

[huumn]

SN pays the fee from SN to the receiver. So it shouldn't show any fee.

[huumn]

I fixed the sender error checking by returning fowardStatus which exposes the status of the withdrawal to the sender.

Other than the logging issues (which I suspect are unrelated) this works well. I'm very happy with predecessor recursive query - it's reliable and simple.

[ekzyis]

I fixed the sender error checking by returning fowardStatus which exposes the status of the withdrawal to the sender.

Should we also use that in the recursive query so client and server definitely have the same view about receiver errors?

[huumn]

Should we also use that in the recursive query so client and server definitely have the same view about receiver errors?

Up to you