fix: handle pending payments when failed payments with same hash exist #2151

vindard · 2023-01-10T12:13:53Z

Description

This PR fixes an issue where our updatePendingPayments isn't able to progress past the isLnTxRecorded step because it wasn't able to register if pending ledger transactions also exist alongside settled failed ones.

The first step was to adjust the isLnTxRecorded check
The next step was to then fail a pending payment if its amount did not match the amount we got back from lnd (in the case of different amounts used for no-amount invoices)

Note

This PR assumes that there can't be 2 simultaneous pending payments (see a4f9e06) for the same hash with the same correct amount. It assumes there can only be 1 pending payment for the correct amount and multiple other failed payments for the same hash.

If this assumption does not hold then there is the risk that multiple payments out get settled for the same single lnd payment which would mean that users would notice an extra payment out. If we think this assumption isn't fair, we can consider adding some provision to account for potentially multiple pending payments (or we can wait until a user observes this in actual usage).

gitguardian · 2023-01-20T20:51:58Z

⚠️ GitGuardian has uncovered 2 secrets 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 secrets in your pull request

GitGuardian id	Secret	Commit	Filename
5547718	Elliptic Curve Private Key	`42ab2a4`	dev/lnd/tls.key	View secret
5547719	Elliptic Curve Private Key	`42ab2a4`	dev/lnd/tls.key.base64	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
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.

Our GitHub checks need improvements? Share your feedbacks!}

dolcalmi · 2023-02-01T17:28:14Z

src/app/payments/update-pending-payments.ts

+ if (
+ status === PaymentStatus.Failed ||
+ // pendingPayment is a different version to latest payment from lnd
+ satsAmount !== toSats(paymentFlow.btcPaymentAmount.amount)


this comparison should be made with invoice amount because fees can change depending on the route

I think both sides of this is the invoice amount without fees (left from lnd, right from PaymentFlow)... is it?

src/app/payments/update-pending-payments.ts

dolcalmi · 2023-02-01T17:32:29Z

src/app/payments/update-pending-payments.ts

+ if (!(filteredPayments && filteredPayments.length))
+ return new CouldNotFindTransactionError()


not sure about this.... so I cant re-try a failed invoice? an example can be done with OBW, create an invoice, close the app, try a payment (it will fail because is not connected) then open OBW wait to connect and try payment again (should succeed)

I don't understand how this affects retry. As-is now we can always retry as tested here? https:/GaloyMoney/galoy/pull/2151/files#diff-15e9812156b4ff1c33faaaf99ffcc368a8af2756ccb7a5e81acdcfc46782db9fR2363

This should be the same logic as what was here before at Ln259, except done over an array instead of a single payment instead

but you are including amount validation... lets validate it in staging.

vindard · 2023-02-07T13:43:51Z

src/services/ledger/index.ts

+ hash: paymentHash,
+ })
+
+ return totalNotPending > 0 && totalPending === 0


This is the material change that allows us to retry if there are both pending and settled txns (from multiple attempts)

dolcalmi

Approved with small comment, also, please let me know to test this in staging.

This is to accomodate the case where there may be multiple pending txns for the same no-amount invoice with different amounts sent.

This partially reverts commit 429613e.

The 2nd success payment can't be made for some reason even with liquidity available on the path. This could probably use some more troubleshooting to determine if it's a: - path liquidity problem - lnd mission control issue

Needed to: 1. Make sure payment is above channel reserve 2. Probe 1st to avoid duplicate fee_reimburse when undoing txns 3. Add a manual 1-sat reimburse to accommodate sub-sat accounting inconsistency

vindard requested a review from dolcalmi January 10, 2023 12:13

vindard changed the title ~~fix: handle pending payments when failed payments with same has exist~~ fix: handle pending payments when failed payments with same hash exist Jan 10, 2023

vindard force-pushed the add-mult-payment-handling branch from 507fdbe to 9285a96 Compare January 10, 2023 19:41

vindard force-pushed the add-mult-payment-handling branch 6 times, most recently from a845fc4 to 45bad5c Compare January 20, 2023 16:56

vindard marked this pull request as ready for review January 20, 2023 17:36

vindard force-pushed the add-mult-payment-handling branch 4 times, most recently from c1408ea to 17dfd87 Compare January 20, 2023 20:51

vindard force-pushed the add-mult-payment-handling branch 3 times, most recently from ff66d6d to baf7aa7 Compare February 1, 2023 15:08

dolcalmi requested changes Feb 1, 2023

View reviewed changes

vindard commented Feb 7, 2023

View reviewed changes

dolcalmi previously approved these changes Feb 13, 2023

View reviewed changes

vindard added 9 commits February 13, 2023 13:00

test: add in-transit payment attempt to expect error

eb62000

test: add post-failed payment attempt to expect error

46e9ed0

test: add lnd-outside-3 node to integration test setup

4adbc32

test: renew lnd1 tls cert to apply new tlsextradomain

42ab2a4

test: add lndOutside3 to integration test setup

b78beb5

test: add helpers to reverse a settled ledger transaction

4fda845

test: add updatePendingPayment from multiple attempts tests

0ee47d4

refactor: change 'isLnTxRecorded' to return 3 result states

84ff551

refactor: include 'inputAmount' when reconstructing PaymentFlow

d678fbd

This is to accomodate the case where there may be multiple pending txns for the same no-amount invoice with different amounts sent.

vindard added 8 commits February 13, 2023 13:10

refactor: fail pending payments that don't match payment from lnd

07ffe0b

refactor: change 'isLnTxRecorded' back to 2 bool result states

a008cd6

This partially reverts commit 429613e.

test: skip flaky multiple payment test

71c228c

The 2nd success payment can't be made for some reason even with liquidity available on the path. This could probably use some more troubleshooting to determine if it's a: - path liquidity problem - lnd mission control issue

test: add lnd-outside-3 to e2e docker-compose deps

49c41d3

test: swap pending-payments tests order

2f9e735

test: tune 'success' pending pay test to accommodate inconsistencies

2bf9065

Needed to: 1. Make sure payment is above channel reserve 2. Probe 1st to avoid duplicate fee_reimburse when undoing txns 3. Add a manual 1-sat reimburse to accommodate sub-sat accounting inconsistency

refactor: remove some else branches in update-pending-payments

25e7e87

refactor: add early return for pending payments in update

352ee2a

vindard dismissed dolcalmi’s stale review via 352ee2a February 13, 2023 17:18

vindard force-pushed the add-mult-payment-handling branch from 78af2d2 to 352ee2a Compare February 13, 2023 17:18

dolcalmi approved these changes Feb 13, 2023

View reviewed changes

vindard merged commit 30781a3 into main Feb 13, 2023

vindard mentioned this pull request Feb 13, 2023

chore: add error catch for multiple pending payments for single hash #2320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle pending payments when failed payments with same hash exist #2151

fix: handle pending payments when failed payments with same hash exist #2151

vindard commented Jan 10, 2023 •

edited

Loading

gitguardian bot commented Jan 20, 2023 •

edited

Loading

dolcalmi Feb 1, 2023

vindard Feb 2, 2023 •

edited

Loading

dolcalmi Feb 1, 2023

vindard Feb 2, 2023 •

edited

Loading

vindard Feb 7, 2023

dolcalmi Feb 13, 2023

vindard Feb 7, 2023

dolcalmi left a comment

		if (!(filteredPayments && filteredPayments.length))
		return new CouldNotFindTransactionError()

fix: handle pending payments when failed payments with same hash exist #2151

fix: handle pending payments when failed payments with same hash exist #2151

Conversation

vindard commented Jan 10, 2023 • edited Loading

Description

Note

gitguardian bot commented Jan 20, 2023 • edited Loading

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

dolcalmi Feb 1, 2023

Choose a reason for hiding this comment

vindard Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

dolcalmi Feb 1, 2023

Choose a reason for hiding this comment

vindard Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

vindard Feb 7, 2023

Choose a reason for hiding this comment

dolcalmi Feb 13, 2023

Choose a reason for hiding this comment

vindard Feb 7, 2023

Choose a reason for hiding this comment

dolcalmi left a comment

Choose a reason for hiding this comment

vindard commented Jan 10, 2023 •

edited

Loading

gitguardian bot commented Jan 20, 2023 •

edited

Loading

vindard Feb 2, 2023 •

edited

Loading

vindard Feb 2, 2023 •

edited

Loading