routing: add block padding to prevent inflight HTLC failures #3153

Crypt-iQ · 2019-06-03T20:43:21Z

Fixed some integration tests + unit tests. Pretty self-explanatory. Wasn't sure if I should have added an integration test?

joostjager · 2019-06-12T08:01:14Z

routing/router.go

@@ -1402,6 +1402,10 @@ func (r *ChannelRouter) FindRoute(source, target route.Vertex,
 		finalCLTVDelta = finalExpiry[0]
 	}

+	// Add BlockPadding to the finalCltvDelta so that the receiving node
+	// does not reject the HTLC if a block is mined while its in-flight.
+	finalCLTVDelta += BlockPadding


I am in doubt whether we should also pad when the user specified an explicit final cltv expiry for queryroutes. It is a more low level function and it may be confusing if it doesn't returns what was requested?

Maybe the same thing holds for sendpayment with a set expiry? Perhaps the padding should just happen when only an encoded payment request is supplied.

Will get back to you.

@Crypt-iQ it seems best to only apply the padding if no explicit final cltv delta is set in the rpc request.

looks like this still needs to be moved

Will do this week

Addressed the comments. This is related to #3150 since if there is no expiry set, the payment request will likely fail because of receiver-side checks (if the node is an lnd node and use a default expiry which is set to 40). The result of that discussion was to require the final cltv delta. Thoughts?

routing/payment_session.go

cfromknecht · 2019-07-09T22:24:15Z

routing/router.go

@@ -1402,6 +1402,10 @@ func (r *ChannelRouter) FindRoute(source, target route.Vertex,
 		finalCLTVDelta = finalExpiry[0]
 	}

+	// Add BlockPadding to the finalCltvDelta so that the receiving node
+	// does not reject the HTLC if a block is mined while its in-flight.
+	finalCLTVDelta += BlockPadding


looks like this still needs to be moved

lntest/itest/lnd_test.go

routing/payment_session_test.go

Roasbeef · 2019-07-13T00:33:13Z

routing/payment_session.go

+	// Add BlockPadding to the finalCltvDelta so that the receiving node
+	// does not reject the HTLC if a block is mined while its in-flight.
+	// This is only added if no finalCltvDelta was set.
+	if finalCltvDelta == 0 {


If we only add block padding in this case, then it doesn't address the common case where a users payment fails due to a block being mined right as they send. We always set our finalCltvDelta if the payment goes through SendPayment.

Ahhhh! I missed that. Joost had a comment above where he said the block padding should only be specified iff no finalCltvDelta was supplied. But it seems that has some problems as well.
see: #3153 (comment)

From my reading, his comment only applied to queryroutes. IMO, we should also apply the padding there, as users are known to execute commands that resemble: queryroutes | sendtoroute.

Yeah that's true, should it be set any time a route is requested then or perhaps this could be the default with an option to turn the padding off?

Another way to see this is, is from the perspective of keeping the api surface as small as possible. QueryRoutes (the rpc) is for advanced api users and you could also say that they should just supply just an absolute block height. This should include the final cltv expiry of the invoice and block padding. It would give more control over the time locks of the route returned, which may be required if there is chaining involved with time locks of other payments.

QueryRoutes (the rpc) is different from queryroutes (the cli command). We could choose to have lncli do some of the calculation to still be able to queryroutes | sendtoroute.

Roasbeef · 2019-07-17T00:09:14Z

SGTM

…

On Tue, Jul 16, 2019, 5:08 PM Eugene ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In routing/payment_session.go <#3153 (comment)>: > @@ -154,6 +158,13 @@ func (p *paymentSession) RequestRoute(payment *LightningPayment, return nil, fmt.Errorf("pre-built route already tried") } + // Add BlockPadding to the finalCltvDelta so that the receiving node + // does not reject the HTLC if a block is mined while its in-flight. + // This is only added if no finalCltvDelta was set. + if finalCltvDelta == 0 { Yeah that's true, should it be set any time a route is requested then or perhaps this could be the default with an option to turn the padding off? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3153?email_source=notifications&email_token=AAHTWLQGVGPESX4D6VKWYNDP7ZPGJA5CNFSM4HSR3NZ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB6UTSTI#discussion_r304174865>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHTWLV4STKS442375XZIFLP7ZPGJANCNFSM4HSR3NZQ> .

routing/pathfind_test.go

halseth

Needs a squash :)

halseth · 2019-07-22T09:24:07Z

routing/payment_session.go

@@ -8,6 +8,10 @@ import (
 	"github.com/lightningnetwork/lnd/routing/route"
 )

+// BlockPadding is used to increment the finalCltvDelta value for the last hop
+// to prevent an HTLC being failed if a block is mined while it's in-flight.


s/a block/some blocks

halseth · 2019-07-22T09:24:33Z

routing/payment_session.go

@@ -65,6 +69,10 @@ func (p *paymentSession) RequestRoute(payment *LightningPayment,
 		return nil, fmt.Errorf("pre-built route already tried")
 	}

+	// Add BlockPadding to the finalCltvDelta so that the receiving node
+	// does not reject the HTLC if a block is mined while its in-flight.


s/if a block/in case some blocks

This commit adds the BlockPadding value (currently 3) to newRoute calls so that if some blocks are mined while the htlc is in-flight, the exit hop won't reject it.

This commit adds a no_padding option to queryroutes lncli and rpc calls. This allows a caller to force lnd to omit any block padding that would otherwise be added as a safety measure.

cfromknecht

LGTM ✅

halseth · 2019-07-24T06:27:58Z

@Crypt-iQ Decided to make the changes only apply to SendPayment for now, and then we can move the queryroutes/sendToRoute discussion to a new PR.

Crypt-iQ · 2019-07-24T09:41:41Z

@Crypt-iQ Decided to make the changes only apply to SendPayment for now, and then we can move the queryroutes/sendToRoute discussion to a new PR.

See #3339

wpaulino · 2019-09-17T22:11:13Z

Needs a rebase!

joostjager · 2019-10-14T07:56:26Z

Closing until we have good reasons to also add implicit padding for the low-level calls SendToRoute and QueryRoutes.

halseth requested a review from joostjager June 11, 2019 13:45

joostjager reviewed Jun 12, 2019

View reviewed changes

Roasbeef added bug fix P2 should be fixed if one has time routing labels Jun 18, 2019

Roasbeef mentioned this pull request Jul 2, 2019

queryroutes + paytoroute should use delta expiry instead of absolute blockheights #1518 #1638

Closed

wpaulino added this to the 0.7.1 milestone Jul 2, 2019

wpaulino added the v0.7.1 label Jul 2, 2019

cfromknecht reviewed Jul 9, 2019

View reviewed changes

Roasbeef requested review from wpaulino and halseth and removed request for wpaulino July 9, 2019 23:26

Crypt-iQ force-pushed the inflight_block_fix_0531 branch from 061cd5d to 1522457 Compare July 11, 2019 18:26

Crypt-iQ requested a review from Roasbeef as a code owner July 11, 2019 18:26

Roasbeef requested changes Jul 13, 2019

View reviewed changes

Crypt-iQ force-pushed the inflight_block_fix_0531 branch 3 times, most recently from 5b5dd0c to 1cce9d9 Compare July 19, 2019 17:07

cfromknecht reviewed Jul 20, 2019

View reviewed changes

routing/pathfind_test.go Outdated Show resolved Hide resolved

routing/pathfind_test.go Outdated Show resolved Hide resolved

halseth reviewed Jul 22, 2019

View reviewed changes

Crypt-iQ added 3 commits July 22, 2019 11:42

routing: adding the BlockPadding value to newRoute calls

f5daade

This commit adds the BlockPadding value (currently 3) to newRoute calls so that if some blocks are mined while the htlc is in-flight, the exit hop won't reject it.

itest+routing: updating tests to account for BlockPadding

0769362

multi: adding no_padding flag to queryroutes lncli, rpc calls

76a2790

This commit adds a no_padding option to queryroutes lncli and rpc calls. This allows a caller to force lnd to omit any block padding that would otherwise be added as a safety measure.

Crypt-iQ force-pushed the inflight_block_fix_0531 branch from 1cce9d9 to 76a2790 Compare July 22, 2019 22:30

cfromknecht approved these changes Jul 23, 2019

View reviewed changes

cfromknecht removed the v0.7.1 label Jul 24, 2019

cfromknecht removed this from the 0.7.1 milestone Jul 24, 2019

cfromknecht added this to the 0.8.0 milestone Jul 24, 2019

wpaulino added the v0.8.0 label Jul 30, 2019

wpaulino modified the milestones: 0.8.0, 0.8.1 Sep 17, 2019

wpaulino added v0.8.1 and removed v0.8.0 labels Sep 17, 2019

wpaulino removed the v0.8.1 label Oct 8, 2019

joostjager added the v0.9.0 label Oct 8, 2019

joostjager modified the milestones: 0.8.1, 0.9 Oct 8, 2019

joostjager closed this Oct 14, 2019

joostjager removed this from the 0.9 milestone Oct 14, 2019

joostjager removed the v0.9.0 label Oct 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

routing: add block padding to prevent inflight HTLC failures #3153

routing: add block padding to prevent inflight HTLC failures #3153

Crypt-iQ commented Jun 3, 2019

joostjager Jun 12, 2019

joostjager Jun 17, 2019

cfromknecht Jul 9, 2019

Crypt-iQ Jul 9, 2019

Crypt-iQ Jul 11, 2019 •

edited

cfromknecht Jul 9, 2019

Roasbeef Jul 13, 2019

Crypt-iQ Jul 13, 2019

Roasbeef Jul 16, 2019

Crypt-iQ Jul 17, 2019

joostjager Jul 23, 2019

Roasbeef commented Jul 17, 2019 via email

halseth left a comment

halseth Jul 22, 2019

halseth Jul 22, 2019

cfromknecht left a comment

halseth commented Jul 24, 2019

Crypt-iQ commented Jul 24, 2019 •

edited

wpaulino commented Sep 17, 2019

joostjager commented Oct 14, 2019

routing: add block padding to prevent inflight HTLC failures #3153

routing: add block padding to prevent inflight HTLC failures #3153

Conversation

Crypt-iQ commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ Jul 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Jul 17, 2019 via email

halseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

halseth commented Jul 24, 2019

Crypt-iQ commented Jul 24, 2019 • edited

wpaulino commented Sep 17, 2019

joostjager commented Oct 14, 2019

Crypt-iQ Jul 11, 2019 •

edited

Crypt-iQ commented Jul 24, 2019 •

edited