Test payments with HODL invoices #1341
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ekzyis
force-pushed
the
hodl-test-payments
branch
5 times, most recently
from
6d487fa
to
957b520
Compare
|
Mhh, this PR would be ready for review if it wouldn't be for this mysterious bug: For some reason, NWC test payments sometimes fail with This looks like a race condition since it only happens sometimes but I made sure that Additionally, this only happens for test payments and only for NWC. I never had this error with LNbits. NWC payments with actions attached always succeed. The difference is that payments for action settle immediately. When I print the bolt11 for a test payment in the console and pay it manually with I am super confused by this and need to investigate this more. video2024-09-04.06-14-10.mp4Sample invoice flow logs in worker where this bug happenedLogs generated running this patch with console.log and cancel by us excludeddiff --git a/api/resolvers/wallet.js b/api/resolvers/wallet.js
index 03965f6a..9d3c2c81 100644
--- a/api/resolvers/wallet.js
+++ b/api/resolvers/wallet.js
@@ -415,6 +415,7 @@ const resolvers = {
if (!timingSafeEqual(Buffer.from(hmac), Buffer.from(hmac2))) {
throw new GraphQLError('bad hmac', { extensions: { code: 'FORBIDDEN' } })
}
+ console.log('cancelInvoice::', hash)
await finalizeHodlInvoice({ data: { hash }, lnd, models, boss })
return await models.invoice.findFirst({ where: { hash } })
},
diff --git a/wallets/index.js b/wallets/index.js
index eaca1f7c..179d63a1 100644
--- a/wallets/index.js
+++ b/wallets/index.js
@@ -182,11 +182,12 @@ function useConfig (wallet) {
logger.info('sending test payment:', `payment_hash=${inv.hash}`)
+ let invHeldPromise, paymentPromise
try {
- const paymentPromise = wallet.sendPayment(inv.bolt11, config, context)
+ paymentPromise = wallet.sendPayment(inv.bolt11, config, context)
// test is successful if invoice is held
- const invHeldPromise = invoice.waitUntilPaid({
+ invHeldPromise = invoice.waitUntilPaid({
id: inv.id,
waitFor: inv => inv.isHeld
})
@@ -207,11 +208,12 @@ function useConfig (wallet) {
// which we will never do for test payments
logger.ok('test payment received')
} catch (err) {
+ console.log(invHeldPromise, paymentPromise)
// test payment failures are not blocking attachment,
// we only want to let the user know that there _might_ be an issue
logger.warn('test payment failed:', err.message)
} finally {
- await invoice.cancel(inv)
+ // await invoice.cancel(inv)
}
}, [invoice, wallet])
diff --git a/wallets/nwc/index.js b/wallets/nwc/index.js
index 254ac428..e980e936 100644
--- a/wallets/nwc/index.js
+++ b/wallets/nwc/index.js
@@ -72,6 +72,8 @@ export async function nwcCall ({ nwcUrl, method, params }, { logger, timeout } =
const decrypted = await nip04.decrypt(secret, walletPubkey, response.content)
const content = JSON.parse(decrypted)
+ console.log(content)
+
if (content.error) throw new Error(content.error.message)
if (content.result) return content.result
diff --git a/worker/index.js b/worker/index.js
index 759d6ad7..3387d09d 100644
--- a/worker/index.js
+++ b/worker/index.js
@@ -88,7 +88,7 @@ async function work () {
if (isServiceEnabled('payments')) {
await subscribeToWallet(args)
- await boss.work('finalizeHodlInvoice', jobWrapper(finalizeHodlInvoice))
+ // await boss.work('finalizeHodlInvoice', jobWrapper(finalizeHodlInvoice))
await boss.work('checkPendingDeposits', jobWrapper(checkPendingDeposits))
await boss.work('checkPendingWithdrawals', jobWrapper(checkPendingWithdrawals))
await boss.work('autoDropBolt11s', jobWrapper(autoDropBolt11s))
diff --git a/worker/wallet.js b/worker/wallet.js
index 4ffdaf88..f5845eb1 100644
--- a/worker/wallet.js
+++ b/worker/wallet.js
@@ -113,6 +113,7 @@ function subscribeToHodlInvoice (args) {
}
export async function checkInvoice ({ data: { hash }, boss, models, lnd }) {
+ console.trace('checkInvoice::', hash)
const inv = await getInvoice({ id: hash, lnd })
// invoice could be created by LND but wasn't inserted into the database yet
@@ -371,6 +372,7 @@ export async function autoDropBolt11s ({ models, lnd }) {
// The callback subscriptions above will NOT get called for JIT invoices that are already paid.
// So we manually cancel the HODL invoice here if it wasn't settled by user action
export async function finalizeHodlInvoice ({ data: { hash }, models, lnd, boss, ...args }) {
+ console.log('finalizeHodlInvoice::', hash)
const inv = await getInvoice({ id: hash, lnd })
if (inv.is_confirmed) {
return
@@ -398,6 +400,8 @@ export async function finalizeHodlInvoice ({ data: { hash }, models, lnd, boss,
return
}
+ console.log('cancelHodlInvoice::', hash)
+
await cancelHodlInvoice({ id: hash, lnd })
// sync LND invoice status with invoice status in database
await checkInvoice({ data: { hash }, models, lnd, ...args })
@@ -410,8 +414,8 @@ export async function checkPendingDeposits (args) {
try {
await checkInvoice({ data: { hash: d.hash }, ...args })
await sleep(10)
- } catch {
- console.error('error checking invoice', d.hash)
+ } catch (err) {
+ console.error('error checking invoice', d.hash, err)
}
}
}
|
This works for LNbits consistently but for some reason, the invoice is _sometimes_ canceled on the backend when a NWC payment is held for NWC. This means that Promise.race rejects because paymentPromise rejects before invHeldPromise.
ekzyis
force-pushed
the
hodl-test-payments
branch
from
957b520
to
107e26e
Compare
|
Is it expiring? |
No, it has the normal expiry of 3 minutes and when it is held, we run |
|
I'll try to pull this today and look at it |
|
I this is caused because Edit: Actually, that might be unrelated. |
Yes, unrelated, it should then fail every time Does this mean you were able to reproduce the bug? Not necessarily consistently, but it at least also happened on your machine at least once? |
I was getting that invalid payment details message because I was doing p2p noncustodial zap to the same node. It was exactly as you described - an unexpected cancelation of the invoice. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR replaces the
testSendPaymentfunction by wrappingsendPaymentwith code that actually attempts to pay an invoice.That invoice is a HODL invoice and when it is held, we cancel it and return success. If the payment is not received within a timeout, or if the wallet throws an error first, the test fails and the wallet is not attached.
However, I realized that payments might also fail because of insufficient balance. The wallet should still get attached in that case, see TODO.
TODO:
how to attach wallet if payment fails due to insufficient balance? can we assume and parse the error message?update: we will log errors as warnings and not block attachment
loggerinsendPaymentis optional in every walletsendPayment(or find an alternative solution for LNC)I added optional
transformConfigto the wallet interface but didn't test yet with LNCBased on #1340
Additional Context
Wallets that can't pay regtest invoices like phoenixd or Blink will not be attachable during development.
Update: test payments will only be used to indicate to the user that something might be wrong but it will not block attachments. We should show errors during test payments as warnings.
Checklist
Are your changes backwards compatible? Please answer below:
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:
For frontend changes: Tested on mobile? Please answer below:
Did you introduce any new environment variables? If so, call them out explicitly here: