feat(lnd): mpp #1978
feat(lnd): mpp #1978
Conversation
sangaman
force-pushed
the
lnd/mpp
branch
3 times, most recently
from
c2264be to
acfe6f5
Compare
|
As just discussed, please give this some extensive real-world testing @raladev . If this passes your code-level review we can comment out the simulation tests for now and merge as is @erkarl |
2_fails_through_thrid_node_maker.log Does it contain breaking change for xud? |
Did you run a similar test for the SendPaymentV2 PR? I'm just wondering to try to narrow down what the cause might be. There's actually a simulation test that tests multi-hop payments, so it's odd that it would work in the simulation but not in your manual test. I'm going to try a simulation test that's an mpp using 2 channels between 2 nodes to see if it works. |
|
utACK from code side |
Yeap, im sure that multi-hop payments worked fine in v2payments PR |
|
Want to prep an intro of how you intend to move forward here and we do a sync call with e.g. @erkarl @michael1011 to go through it and make sure we are not missing anything? @sangaman |
|
Relevant part of of the latest LND email of Alex Bosworth: |
sangaman
force-pushed
the
lnd/mpp
branch
4 times, most recently
from
7895523 to
1ab79d2
Compare
|
An update here on my trials & findings: Currently, when taker pays maker without a direct channel, even without using MPP or specifying max parts, the payment fails with On the maker (receiver) side I also found some odd output where the invoice was seemingly canceled right after being added, I have no explanation for this but you can see it's for the same hash as above. It may be unrelated but it was too odd to ignore. If I change xud code to go back to specifying the hash, destination, amount, and final cltv delta manually rather than the payment request (aka invoice), multi hop payments work again. That won't work with multi path payments since those require the payment address secret which cannot be specified on its own currently, but at least that isolates the problem. I've decoded the payment request to make sure everything looks correct and in fact it matches up with the values we specify independently. Here's an example decoded invoice: Setting that invoice on the Next things to try are:
If those don't work I think it needs to be looked into on the lnd side of things because I'm out of ideas from what to change or try in the xud code. |
|
Did you try https://github.com/lightningnetwork/lnd/releases/tag/v0.12.0-beta.rc2 ? |
|
I am further mystified now because intercepting a pay req generated by xud and paying it via lncli worked. You can see the output below with the extended cltv expiry for the payment. You can also see from the payment history that it took two hops Lncli is using SendPaymentV2, as far as I can tell I'm doing the exact same thing. I've been trying again to comb through every line of code to see what the difference/problem may be. |
sangaman
force-pushed
the
lnd/mpp
branch
2 times, most recently
from
97343bc to
c98ef76
Compare
|
I finally figured out the culprit. Here's the old code for setting the fee limit. const fee = Math.floor(MAX_FEE_RATIO * request.getAmt());
request.setFeeLimitSat(fee);When we stop setting the I cleaned up the commit history - all tests are passing now and this is ready for real review and testing. |
ghost
changed the title
| @@ -917,6 +937,9 @@ class Swaps extends EventEmitter { | |||
| return; | |||
| } | |||
|
|
|||
| // TODO: re-enable add invoice *after* swap accepted once we are able to specify the | |||
There was a problem hiding this comment.
Do we want to clean up this TODO in this PR?
Steps:
Actual result: Notes: my trading limits and channels after second step: maker: taker: |
|
How about regular swaps (fitting into one channel)? @raladev |
no problem with one-channel swaps |
| @@ -575,6 +591,7 @@ class LndClient extends SwapClient { | |||
| }; | |||
|
|
|||
| public sendSmallestAmount = async (rHash: string, destination: string): Promise<string> => { | |||
| // TODO: as of lnd 0.12.0, this won't work as PaymentAddr will be required and the only way to specify it will be with the pay req | |||
There was a problem hiding this comment.
We should update this one too asap. Maybe using keysend could be an alternative? Will LND 0.12 also break keysends?
There was a problem hiding this comment.
I'm not sure about keysends, but since this code isn't being used right now it's not urgent. This whole method could be a todo stub for the time being since we're not using sanity swaps which is what this is for.
| request.setPaymentRequest(payReq); | ||
| request.setMaxParts(LndClient.MAX_PARTS); | ||
| } else { | ||
| // TODO: as of lnd 0.12.0, this won't work as PaymentAddr will be required and the only way to specify it will be with the pay req |
There was a problem hiding this comment.
I don't know how I feel about keeping this code. Maybe in the first release after LND v0.12.0 we should remove it?
There was a problem hiding this comment.
It's basically there for backwards compatibility with older lnd and older xud versions, it's ignored by other peers using any version of xud that includes the commits from this PR. I figure removing it after we've confirmed that xud works with lnd 0.12+ would be fine, in my one quick test using 0.12 for the simtests caused them to break so there's some work/investigation to be done there.
Do you possibly have the lnd logs from this swap? If not I'll try to recreate myself and see what's happening. The swap very nearly exceeds the maker's outbound ltc capacity, but not quite, so I don't see why it wouldn't work. Let me know if there's a docker image from this branch that you've already built & pushed. |
Could you kindly provide this to speed things up? @raladev |
If u want more fresh xud: |
Those are only the xud logs right? It's the lnd logs I'm interestd in.
Thanks I'll try this soon |
|
Also needs a rebase |
This enables multi path payments with lnd. This involves several significant changes to the swap flow, including a new
payReqfield added to theSwapReqeuetandSwapAcceptedpackets. A new test case is added to simulate multipath payments by executing an order between Alice and Bob for an amount that exceeds their direct channel between each other but not the combined balance including their channels with Carol.Xud as of this PR supports making lnd payments with or without using the invoice payment request string, so payments with older nodes should remain backwards compatible. However, as of lnd 0.12.0 we expect that only payments using the payment request string will work, due to not being able to assign a value to the secret payment address field otherwise.
EDIT by @kilrau : Closes #1975
EDIT by @kilrau : Closes #1934