[WIP] refactor(lnd): use SendPayment instead of SendToRoute

[ghost]

In this PR we refactor the LndClient.sendPayment logic to use lndrpc.SendPaymentSync instead of lndrpc.SendToRoute. As a result we can now use the same buildSendRequest and executeSendRequest logic for sanity swaps and "regular swaps".

In addition we:

• as a safeguard increase the default LndClient.cltvDelta to 576 if it's not specified via the config file
• wait for hold invoice state to become open in the context of LndClient.addInvoice

I also added TODOs for cltvDelta values (to be discussed and addressed in later PR) because I believe the logic as is does not work for Raiden.

Fixes the 2nd part of #984 (comment)

[kilrau]

Discussion with @offerm if this is really not possible, if not unfortunately close this and @offerm will digg into the sendtoroute problem: #1028

Might be the solution for still using SendPayment:
cltv_limit uint32 An optional maximum total time lock for the route. If zero, there is no maximum enforced.

[ghost]

@kilrau @offerm @sangaman

So, the problem is that:

1. Taker sends X BTC with a timelock of 30 minutes
2. Maker sends Y LTC with a timelock of 60 minutes
3. Taker waits 30 min
4. Taker claims LTC and "releases" the preimage
5. ERROR! -> Maker can no longer claim with the released preimage because 30 minutes has passed

Looking at the SendPayment API we can specify the following:

• final_cltv_delta - The CLTV delta from the current height that should be used to set the timelock for the final hop.
• cltv_limit - An optional maximum total time lock for the route. If zero, there is no maximum enforced.

As a maker we're currently using SendToRoute because that way we can ensure that the cltv value won't be longer than predetermined Z. I fail to see why we couldn't specify cltv_limit to ensure that the maker's timelock is shorter than the taker's, preventing the above mentioned scenario.

[ghost]

When maker is creating an invoice for the taker we don't know the total timelock of the route, but we are setting the CltvExpiry for the last hop of the route. So, we know the minimum.

Based on that we can calculate a shorter timelock for maker's payment and force it with cltvLimit (or even final_cltv_delta in the case of direct channels).

[offerm]

Sorry for the length of the below.

consider a channel between A and B, each side has 25 BTC. Each side holds a signed commitment transactions that it can use to close the channel by sending to the chain.
If A is sending 1 BTC to B, they just need to exchange a new commitment transaction saying that A has 24 and B has 26 (I simplify things here, I know).

If we consider A<>B<>C and a payment is done from A to C, so A sends 1 BTC to B and B sends 1 BTC to C, the above will not work any more. If A and B agrees on a new commitment transaction (24-26), B may decide not to send the 1 BTC to C. For this case we have HTLC payment.

HTLC is another output in the new commitment transaction between A and B allowing B to claim the amount (1 BTC) if B has the preimage and allowing A to claim the amount after an agreed time (blocks) in case B does not have the preimage or decided not to use it.
So now, A sends 1 BTC to B with HTLC and B can send 1 BTC to C knowing that if this works he can get his 1 BTC from A (off-chain or on-chain).
Once B has the preimage he can share it with A via LN protocol and A and B agree on a new commitment transaction (24,26) without the HTLC and the payment is done and forgotten.
B already sent 1 BTC to C and must get his 1 BTC from A - but what if B can do that off-chain? maybe A is offline or A is not cooperative ? What B should do now is:

• wait some time to give A a change to recover
• close the channel by sending the last commitment transaction to the chain
• use the preimage to move the 1 BTC from the HTLC output to his wallet.

The above steps may take some time and B should also consider that he may have connectivity issues himself. B must allow himself time to be sure that he can grab is 1 BTC before A comes and get the 1 BTC back to his wallet (after the agreed time). To protect himself B (the payee) sets the timeout and the payer must respect the value set by the payee.
If the payee set a 144 blocks timeout and the payer issues a payment with a shorter timeout, the payee will reject the payment and will not route it. The payer can use a larget timeout if she likes but not a smaller one.

When having a route A<>B<>C<>D, there are actually 3 payments involved - 3 payees - 3 lock time value. Each payee decides his lock time value and each payer must respect it. Since the route and all payments are decided by A, A must know these values for all nodes along the route and create a valid HTLCs.
Lets consider the following lock time value: B - 20, C -30, D- 40. D should get 40 block to claim the payment on-chain. In the worst case D will do it in the last block. Now, C has the hash and should claim the payment from B - worst case is that this should be done on-chain and C does it with 30 blocks - so we need 40 blocks for D and another 30 blocks for C (total for 70). Similarly, we would need to add another 20 blocks for B. So when A makes the payment via that route it MUST give the each node his expected time lock value. A will be able to claim the payment to B on-chain only after 90 blocks.

lock time value is expressed in absolute block numbers. So when A makes the payment he should take the current block height and add the timelocks value. When each nodes receive the HTLC, each node subtract the current block height from the HTLC value and check that it was given enough time.

Now we can look at the output of queryroutes:

{
    "routes": [
        {
            "total_time_lock": 2008,
            "total_fees": "2",
            "total_amt": "12",
            "hops": [
                {
                    "chan_id": "1632774767312896",
                    "chan_capacity": "2500000000",
                    "amt_to_forward": "11",
                    "fee": "1",
                    "expiry": 1968,
                    "amt_to_forward_msat": "11000",
                    "fee_msat": "1000",
                    "pub_key": "02b11c79b28c3507366fd5263657ae12ccf0103bf4b6776eb52de48d840c672d8c"
                },
                {
                    "chan_id": "1119302837141505",
                    "chan_capacity": "2500000000",
                    "amt_to_forward": "10",
                    "fee": "1",
                    "expiry": 1928,
                    "amt_to_forward_msat": "10000",
                    "fee_msat": "1000",
                    "pub_key": "03a9dd7dbc1d5a7e5489368aa4865cefd1ab91165b2c44bfcd168817a72fb99625"
                },
                {
                    "chan_id": "1117103813885953",
                    "chan_capacity": "2500000000",
                    "amt_to_forward": "10",
                    "fee": "0",
                    "expiry": 1928,
                    "amt_to_forward_msat": "10000",
                    "fee_msat": "0",
                    "pub_key": "02e97fab85e75ede7e30e196ecb553b4e91b26374765c1a5e121c696a73cf6f03c"
                }
            ],
            "total_fees_msat": "2000",
            "total_amt_msat": "12000"
        }
    ]
}

We can see route with 4 nodes. with the following lock time values: 2008, 1968,1928.
So the value each node is using is 40.

From where LND gets these numbers? these are shared in the LN network by all nodes. You can see them with lncli describegraph

When doing SWAP we have 2 payments - BTC and LTC. Say BTC route is A<>B<>C<>D and LTC route is D<>B<>E<>A.
Logically, this is a single payment with the a route - A<>B<>C<>D<>B<>E<>A
As we have seen before the timelock for each node should include the timelock for the nodes after him. So the timelock of the first leg must include the time lock of the second leg.

Now, if we understand all the above, lets see how this is done in XUD today (before this PR) with the invoice and pay to route:

• How do we calculate the LTV for each node in the route (first and send leg)
• How do we check that we got the right value when being the payee (maker and taker)

The log record related to a swap does not show enough information to be sure that we are doing it right.

IMHO, if we are doing something wrong, it can explain the unknownexthop error.
I can explain how this was done with a resolver, if needed.

[ghost]

Let's take a look at how the current implementation is doing it:

1. Maker (outbound LTC, inbound BTC) receives a SwapRequestPacket from the taker (outbound BTC, inbound LTC) - acceptDeal
2. Maker queries LTC routes to the taker. Route CLTV delta 2008 blocks.
3. Maker calculates cltvDeltaFactor based on the makerCurrency and takerCurrency clients' cltvDeltas. 144 / 576 = 0.25
4. Maker calculates the total makerCltvDelta 144 + 2008 * 0.25 = 646
5. Maker creates an invoice to receive BTC. CltvExpiry = 144
6. Maker sends SwapAcceptedPacket to taker.
7. Taker creates an invoice to receive LTC. CltvExpiry = 576
8. Taker pays BTC invoice, setting FinalCltvDelta to 646 - taker.sendPayment
9. Maker pays LTC invoice using SendToRoute with a total time lock of 2008 blocks - maker.sendPayment

Judging by the TODOs in the code and the above mentioned steps I feel like the current implementation is broken because of step 5. Maker sets the minimum cltv (final hop) to 144, but does not control how long the total route will be. Loss of funds can occur if the taker pays the invoice with total cltv smaller than 2008 * 0,25 = 502.

The correct step would be:
5. Maker creates an invoice to receive BTC with CltvExpiry=646. This will force the first leg to include the time lock of the second leg.

When using SendPayment instead of SendToRoute we can achieve the same result, but gain the added benefit of not being restricted to 1 route. Step 9 would be:
9. Maker pays LTC invoice using SendPayment and sets the cltvLimit to 2008 blocks.

[ghost]

Putting this one on hold/WIP until #1037 is done.

[kilrau]

Do you see any problem using https://api.lightning.community/#sendpaymentsync with cltv_limit = 2008 for Step 9? As far as I understand the second LTC route has to be 2008 LTC blocks or shorter (just not longer) to respect the first leg's cltv time plus makerCltvDelta. That's exactly what cltv_limit specifies. Let us know if you see a problem with that @offerm

Question: Does cltv_expiry of the LTC invoice allow for a shorter-than cltv 2008 route? @erkarl

[sangaman]

When using SendPayment instead of SendToRoute we can achieve the same result, but gain the added benefit of not being restricted to 1 route.

The general approach of setting cltv_limit seems to me like it should work. But we would be limited to the cltv limit of the first route we get back from QueryRoutes - which anyhow is only going to return a single route in the future as returning multiple routes is deprecated. This means that if we have potential routes with greater total cltv delta, they won't work with SendPayment.

I spent a lot of time reading the thoughtful comments here and thinking about this. I can think of 2 more considerations here:

1. If our first payment fails due to insufficient balance along one edge of the route, we could retry the payment by calling QueryRoutes setting the failed edge in ignored_edges. Then we'd have to cancel and recreate new invoices as well as inform our peer of the new cltv expiry to use. This might be pretty complicated to implement but it's something to keep in mind down the road.

2. I'm wondering if we should consider a more conservative formula for the cltvDeltaFactor. In the long run it's 0.25, but in practice the BTC chain could mine quickly while LTC goes slowly. In the examples above it's possible that the BTC HTLCs expire before the LTC ones do. I haven't done the math and I figure that 144 blocks is probably large enough to make this very unlikely, but there are probabilistic formulas we could here to ensure us specific chance of at least X blocks delay on the BTC chain based on the total delay for the LTC chain.

This is pretty complex so let me know if this makes sense.

[kilrau]

Note: CltvExpiry = 576 in step 7 seems not really looked at or used

[ghost]

@sangaman

If our first payment fails due to insufficient balance along one edge of the route, we could retry the payment by calling QueryRoutes setting the failed edge in ignored_edges...

Definitely an optimization that we could look into post 1.0.

@kilrau

Note: CltvExpiry = 576 in step 7 seems not really looked at or used

Nice observation. This is corrected in https://github.com/ExchangeUnion/xud/pull/1037/files#diff-207d104426c9b9c110cdd1c78f1fc6ecR487 where we use takerCltvDelta where we query makerToTakerRoutes.

[ghost]

Closing in favor of #1121