fix(swaps): add resolve request validation slippage

[ghost]

In this commit we're relaxing raiden's resolve request validation by 3 blocks in order to prevent an issue where new blocks are mined during the payment.

This is a quick fix to the issue described in: #1184 (comment)

As a follow-up I'd like to add a simulation test scenario to make sure maker still has sufficient buffer.

[offerm]

consider to add the LOCK_EXPIRATION_SLIPPAGE to the maker's final_cltv_delta and here just check without the LOCK_EXPIRATION_SLIPPAGE.

[ghost]

I don't understand. In the case of raiden there's no concept of final_cltv_delta. The way I understand it is that we only want to add the slippage for raiden payments from taker to maker.

[offerm]

There can also be a slippage on the LN payment. Same reason. The maker, when calculating the lock_duration is using the route to the taker + ~24 hours. This is converted to the maker blocks. To this we should add 3 blocks before we give to the taker. When getting the payment we should make a normal check (as it is now, without the 3 blocks)

…

On Sun, Sep 8, 2019 at 10:38 AM Karl Ranna ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/swaps/Swaps.ts <#1207 (comment)>: > @@ -739,7 +739,8 @@ class Swaps extends EventEmitter { source = 'Taker'; destination = 'Maker'; const lockExpirationDelta = expiration - chain_height; - if (deal.makerCltvDelta! > lockExpirationDelta) { + const LOCK_EXPIRATION_SLIPPAGE = 3; + if (deal.makerCltvDelta! - LOCK_EXPIRATION_SLIPPAGE > lockExpirationDelta) { I don't understand. In the case of raiden there's no concept of final_cltv_delta. The way I understand it is that we only want to add the slippage for raiden payments from taker to maker. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1207?email_source=notifications&email_token=ACKDI4S7IKYNQXVWN4RQR7LQISTYZA5CNFSM4IUQ5E52YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCD75IEQ#discussion_r321995606>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACKDI4SZ3FC3EW6IJ5CEBGTQISTYZANCNFSM4IUQ5E5Q> .

[ghost]

There can also be a slippage on the LN payment. Same reason.

The cltv_expiry for LN payment is relative. How can there be slippage?

[sangaman]

Looks good to me, I'd only recommend a line or two of comments explaining what the slippage is and why we must account for it, so in the future it's not a mystery to someone reading the code that's unfamiliar with the concept.

As for applying this to the lnd invoices, I'm also under the impression that it's not necessary since the values are relative. Ultimately the check that we have sufficient cltv delay happens within lnd code as I understand it, not xud (we just pass lnd the relative value when adding an invoice).

[kilrau]

Looks good to me, I'd only recommend a line or two of comments explaining what the slippage is and why we must account for it, so in the future it's not a mystery to someone reading the code that's unfamiliar with the concept.

Agree, these comments would be very nice :)

As for applying this to the lnd invoices, I'm also under the impression that it's not necessary since the values are relative. Ultimately the check that we have sufficient cltv delay happens within lnd code as I understand it, not xud (we just pass lnd the relative value when adding an invoice).

That's also my understanding, could you confirm or explain what you mean so that this can be merged asap? @offerm

[kilrau]

I just tested this branch with a native simnet setup. 1. leg lightning, 2. leg raiden orders (xucli buy 1 btc/dai 10000) still not working. Error same: 09/09/2019 12:44:46.856 [SWAPS] debug: deal 92521de004cd68ff57f8854d895b25f62b6775bf101d70ba135de4d502bd57bc failed due to SwapTimedOut: undefined

Isn't this supposed to fix this or am I missing something?

[kilrau]

Oh, the xud1.simnet.exchangeunion.com would need to be updated too I guess. Here the log from xud1.simnet side: https://paste.ubuntu.com/p/vpv3MT7RYK/ . Shows, that the cltv delta is off by one block.

[kilrau]

There are some other weird entries with the node key being replaced by undefined. Do we know why is that?

[sangaman]

Those logs show 09/09/2019 10:44:18.393 [SWAPS] error: cltvDelta of 7359 does not meet 7360 minimum which indicates that it's failing due to the bug that's fixed here, so we're all good on that front.

I'll look into that node key being replaced by undefined issue, definitely separate from this though.