mod+itest: reproduce and fix reverse direction invoice payment, bump to rc4 #790

guggero · 2024-07-11T10:05:05Z

Reported offline by @jamaljsr.

Depends on lightningnetwork/lnd#8908.
Depends on lightninglabs/taproot-assets#1011.

Fixes a bug that when an invoice is paid, the full invoice amount is checked when validating the local channel balance and reserve. But for custom channels the actual HTLC amount might be just above dust and shouldn't validate the channel reserve.
We update our itest to reproduce the issue and then update to the lnd commit that fixes it.

GeorgeTsagk

Diff LGTM
itest seems unhappy, doesn't look related to these changes
https://github.com/lightninglabs/lightning-terminal/actions/runs/9890241755/job/27318070143?pr=790#step:6:1335

GeorgeTsagk · 2024-07-11T11:19:03Z

itest/litd_custom_channels_test.go

 	)
-	logBalance(t.t, nodes, assetID, "after keysend back")
+	payInvoiceWithAssets(t.t, dave, charlie, invoiceResp, assetID)


so prior to fix the sender would crash here saying there's no available path?

Yes, a NO_ROUTE error would be thrown here.

guggero · 2024-07-11T11:26:22Z

itest seems unhappy, doesn't look related to these changes

Oops, I had a change in the flag name in there that isn't referenced yet. Will need to update tapd once this is merged: lightninglabs/taproot-assets#1011 (which will then require the rename of the mock price flag).

Liongrass · 2024-07-11T16:23:14Z

I assume I should have expected this version of litd to not recognize the config option unknown option: taproot-assets.experimental.rfq.mockoraclecentpersat?

I tried to run this patch, but ran into the following panic:

2024-07-11 16:22:35.716 [INF] LITD: LiT version: 0.13.99-alpharc3 commit=v0.13.99-alpha.rc3-3-g787c9136fc04f6cc7117bcec132eefb85eb8b3f7
2024-07-11 16:22:35.717 [DBG] STAT: Setting the accounts sub-server as enabled
2024-07-11 16:22:35.717 [DBG] STAT: Setting the faraday sub-server as enabled
2024-07-11 16:22:35.717 [DBG] STAT: Setting the loop sub-server as enabled
2024-07-11 16:22:35.718 [DBG] STAT: Setting the pool sub-server as enabled
panic: unknown chain: testnet

goroutine 1 [running]:
github.com/lightninglabs/taproot-assets/address.ParamsForChain({0xc0006a2738, 0x7})
	github.com/lightninglabs/[email protected]/address/params.go:174 +0x1fa
github.com/lightninglabs/lightning-terminal/subservers.NewTaprootAssetsSubServer({0xc0006a2738, 0x7}, 0xc0001fb808, 0xc0003e38f0, 0x0, 0x0)
	github.com/lightninglabs/lightning-terminal/subservers/taproot-assets.go:50 +0x65
github.com/lightninglabs/lightning-terminal.(*LightningTerminal).initSubServers(0xc000129e60)
	github.com/lightninglabs/lightning-terminal/terminal.go:1743 +0x493
github.com/lightninglabs/lightning-terminal.(*LightningTerminal).Run(0xc000129e60)
	github.com/lightninglabs/lightning-terminal/terminal.go:283 +0x627
main.main()
	github.com/lightninglabs/lightning-terminal/cmd/litd/main.go:14 +0x7f

guggero · 2024-07-11T16:34:27Z

I assume I should have expected this version of litd to not recognize the config option unknown option: taproot-assets.experimental.rfq.mockoraclecentpersat?

Sorry, I forgot to mention that. Yes, we renamed the config flag because its name was actually wrong.
Can you update your guides/documents accordingly please?
There's now also an alternative option: taproot-assets.experimental.rfq.mockoraclesatsperasset which allows you to specify the mock exchange rate in a different way.

jamaljsr · 2024-07-11T16:46:15Z

A direct peer payment in the opposite direction now works, but when I perform a multi-hop payment in the opposite direction, it results in a FAILURE_REASON_NO_ROUTE error.

I have setup a regtest network with the following setup.

alice --[asset]-> bob --[sats]-> carol --[asset]-> dave

I performed the following steps:

Mint 1000 PBUX on alice
Send 500 PBUX from alice to dave
Open 400 PBUX channel from alice to bob
Open 400 PBUX channel from carol to dave
Open 500K sats channel from bob to carol
Pay a 200K sat invoice from bob to carol
Pay a 200 PBUX invoice from alice to dave (success)
Pay a 100PBUX invoice from dave to alice (FAILURE_REASON_NO_ROUTE)

Here are the logs for all of the nodes.
docker-logs.zip

jamaljsr · 2024-07-11T16:59:00Z

When I try to make a large direct peer invoice payment, it results in the litd process hanging and needing to be killed. It won't shutdown gracefully.

This regtest network just has 2 litd nodes. I perform these steps to reproduce the error.

Mint 1,000,000 PBUX on alice
Open 500,000 PBUX channel from alice to bob
Create a 10,000 PBUX invoice on bob
Paying the invoice on alice hangs the litd node

Here are the logs for both nodes.
alice.log
bob.log

guggero · 2024-07-11T17:36:48Z

The first issue above (#790 (comment)) is fixed with the latest push.

The second issue (#790 (comment)) is likely a bit more involved, so I spun it out into a separate issue report: lightninglabs/taproot-assets#1013

Liongrass · 2024-07-11T17:54:50Z

I am running this now, litd started up promptly and seems to be running smoothly!

jamaljsr

The first issue above (#790 (comment)) is fixed with the latest push.

tACK 🔥 Confirmed the first issue is resolved now. Thank you for the quick fix 💪

Fixes a bug that when an invoice is paid, the full invoice amount is checked when validating the local channel balance and reserve. But for custom channels the actual HTLC amount might be just above dust and shouldn't validate the channel reserve. We update our itest to reproduce the issue and then update to the lnd commit that fixes it.

GeorgeTsagk

re-ACK

We don't need to set the shard size in every test, as the max_htlc value in the actual routing network will force splits if needed. When we go to send a large payment, with this value, and the default max parts value of 16, we'll never be able to send enough shards to finalize a payment amount.

We add a 1 second sleep as at times the balances haven't fully propagated, so we'll tend to use a max amount value of 0.

guggero requested a review from jamaljsr July 11, 2024 10:05

GeorgeTsagk approved these changes Jul 11, 2024

View reviewed changes

guggero force-pushed the reverse-invoice-fix branch from 9d14d7c to 4c72c55 Compare July 11, 2024 11:25

guggero force-pushed the reverse-invoice-fix branch 3 times, most recently from 783e679 to 787c913 Compare July 11, 2024 12:45

dstadulis assigned guggero Jul 11, 2024

guggero force-pushed the reverse-invoice-fix branch from 787c913 to 87fca7d Compare July 11, 2024 16:30

jamaljsr approved these changes Jul 11, 2024

View reviewed changes

guggero force-pushed the reverse-invoice-fix branch 2 times, most recently from 78fa03c to 29be963 Compare July 11, 2024 19:49

guggero added 2 commits July 12, 2024 09:38

multi: fix broken links to renamed doc folder

ddf711b

guggero force-pushed the reverse-invoice-fix branch from 9e15e61 to 997854b Compare July 12, 2024 07:56

guggero mentioned this pull request Jul 12, 2024

itest: add asset invoice normal payment case #794

Closed

GeorgeTsagk approved these changes Jul 12, 2024

View reviewed changes

guggero and others added 7 commits July 12, 2024 18:12

mod+itest: bump tapd version to fix multiple small issues

083650a

itest: add large payment integration test

9aef462

cmd/litcli: use correct expiry for invoice

96da2e9

itest: wait to propagate balances

227c4f9

We add a 1 second sleep as at times the balances haven't fully propagated, so we'll tend to use a max amount value of 0.

itest: add asset invoice normal payment case

3a043ce

build: bump version to v0.13.99-alpha.rc4

b9e6bb5

guggero force-pushed the reverse-invoice-fix branch from e3f94e4 to b9e6bb5 Compare July 12, 2024 16:12

guggero changed the title ~~mod+itest: reproduce and fix reverse direction invoice payment~~ mod+itest: reproduce and fix reverse direction invoice payment, bump to rc4 Jul 12, 2024

guggero merged commit d3bdd03 into 0-19-staging Jul 12, 2024
12 of 13 checks passed

guggero deleted the reverse-invoice-fix branch July 12, 2024 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mod+itest: reproduce and fix reverse direction invoice payment, bump to rc4 #790

mod+itest: reproduce and fix reverse direction invoice payment, bump to rc4 #790

guggero commented Jul 11, 2024 •

edited

Loading

GeorgeTsagk left a comment

GeorgeTsagk Jul 11, 2024

guggero Jul 11, 2024

guggero commented Jul 11, 2024

Liongrass commented Jul 11, 2024

guggero commented Jul 11, 2024

jamaljsr commented Jul 11, 2024

jamaljsr commented Jul 11, 2024

guggero commented Jul 11, 2024

Liongrass commented Jul 11, 2024

jamaljsr left a comment

GeorgeTsagk left a comment

mod+itest: reproduce and fix reverse direction invoice payment, bump to rc4 #790

mod+itest: reproduce and fix reverse direction invoice payment, bump to rc4 #790

Conversation

guggero commented Jul 11, 2024 • edited Loading

GeorgeTsagk left a comment

Choose a reason for hiding this comment

GeorgeTsagk Jul 11, 2024

Choose a reason for hiding this comment

guggero Jul 11, 2024

Choose a reason for hiding this comment

guggero commented Jul 11, 2024

Liongrass commented Jul 11, 2024

guggero commented Jul 11, 2024

jamaljsr commented Jul 11, 2024

jamaljsr commented Jul 11, 2024

guggero commented Jul 11, 2024

Liongrass commented Jul 11, 2024

jamaljsr left a comment

Choose a reason for hiding this comment

GeorgeTsagk left a comment

Choose a reason for hiding this comment

guggero commented Jul 11, 2024 •

edited

Loading