loopout: configurable payment timeout for off-chain payments #743

bhandras · 2024-05-10T15:10:32Z

With this client side RPC option the client will be able to set a payment timeout for individual off-chain payments. This applies both to the prepayment and the swap payment. With the swap payment, we may try to pay the swap invoice multiple times, but each attempt will use this configurable timeout.

Pull Request Checklist

Update release_notes.md if your PR contains major features, breaking changes or bugfixes

hieblmi

Very nice feature! I have a question regarding the cancellation of the payment context in addition to the paymentTimeout.
Since we are thinking about properly exiting lnd's payment loop in this PR lightningnetwork/lnd#8734 it might make sense to also make the context of awaitSendPayment bound to the timeout.

hieblmi · 2024-05-13T07:22:21Z

looprpc/client.proto

+    /*
+    The timeout in seconds to use for off-chain payments. Note that the swap
+    payment is attempted multiple times where each attempt will set this value
+    as the timeout for the payment.


In the high/low case, isn't the timeout value split across the attempts? If so, should we mention that here?

Oh, I see, we are adjusting the no of retry attempts in payInvoiceAsync if this setup exceeds the total config timeout.

Yeah it'll try to fill the total allocated time, but each attempt will timeout after the payment_timeout (if set).

hieblmi · 2024-05-13T10:05:39Z

loopdb/sql_store.go

@@ -443,6 +443,7 @@ func loopOutToInsertArgs(hash lntypes.Hash,
 		PrepayInvoice:       loopOut.PrepayInvoice,
 		MaxPrepayRoutingFee: int64(loopOut.MaxPrepayRoutingFee),
 		PublicationDeadline: loopOut.SwapPublicationDeadline.UTC(),
+		PaymentTimeout:      int32(loopOut.PaymentTimeout.Seconds()),


what happens for older entries? I guess the timeout is 0 then?

Old entries won't have this set, and for those we'll assume that the payment timeout will be equal to the total divided by the max configured retries (like before). Thanks for mentioning though as I left this specific case out for resumed swaps (now added back).

hieblmi · 2024-05-13T10:19:36Z

loopout.go

@@ -647,8 +650,8 @@ func (s *loopOutSwap) payInvoice(ctx context.Context, invoice string,
 		var result paymentResult

 		status, err := s.payInvoiceAsync(


If we follow s.payInvoiceAsync and the paymentTimeout it ends up in awaitSendPayment. The context here is passed to s.lnd.Router.SendPayment(ctx, *req).
Question: Does it make sense to cancel this context once the paymentTimeout is reached, given that we think about making this context cancelable to properly shutdown lnd's payment loop? The related PR is here: lightningnetwork/lnd#8734

Yeah I think this could be a good idea, but given that not all users switch to the latest LND release, we need to still rely on the request's timeout. For now I'll keep it as is and we can add it in a separate PR once we bump the minimum LND version.

lightninglabs-deploy · 2024-05-21T14:08:31Z

@starius: review reminder
@sputn1ck: review reminder
@hieblmi: review reminder

starius

LGTM 🎉

Left few small comments.

starius · 2024-05-23T02:00:49Z

loopd/swapclient_server.go

+		return nil, fmt.Errorf("payment timeout exceeds maximum "+
+			"allowed timeout of %v", s.config.TotalPaymentTimeout)


I propose to include paymentTimeout into the error message:

return nil, fmt.Errorf("payment timeout %v exceeds maximum " ...

starius · 2024-05-23T02:04:54Z

loopd/swapclient_server.go

@@ -101,6 +101,13 @@ func (s *swapClientServer) LoopOut(ctx context.Context,

 	log.Infof("Loop out request received")

+	paymentTimeout := time.Duration(in.PaymentTimeout) * time.Second
+	// Make sure we don't exceed the total allowed payment timeout.
+	if paymentTimeout > s.config.TotalPaymentTimeout {


I propose to add a comment that in.PaymentTimeout is uint32, so it is >= 0 (no need to check that separately).

starius · 2024-05-23T02:08:37Z

loopout.go

+		if paymentTimeout*time.Duration(maxRetries) >
+			totalPaymentTimeout {
+
+			maxRetries = int(totalPaymentTimeout / paymentTimeout)


I propose to add an info or warning here.

starius · 2024-05-23T02:10:32Z

loopout.go

+		if paymentTimeout*time.Duration(maxRetries) >
+			totalPaymentTimeout {
+
+			maxRetries = int(totalPaymentTimeout / paymentTimeout)


If totalPaymentTimeout is not divisible by paymentTimeout, the remainder of totalPaymentTimeout will be removed. Does it make sense for the last attempt to use a shorter timeout instead?

I don't think it's needed since both the individual timeout and the total timeout is configurable.

starius · 2024-05-23T02:13:03Z

cmd/loop/loopout.go

+	paymentTimeout := 0
+	if ctx.IsSet("payment_timeout") {
+		paymentTimeout = int(ctx.Duration("payment_timeout").Seconds())
+		if paymentTimeout < 0 {


If user passes --payment_timeout=0s, it should also fail, I think. So this check should be if paymentTimeout <= 0

starius · 2024-05-23T02:16:24Z

cmd/loop/loopout.go

@@ -235,6 +243,15 @@ func loopOut(ctx *cli.Context) error {
 		}
 	}

+	paymentTimeout := 0
+	if ctx.IsSet("payment_timeout") {
+		paymentTimeout = int(ctx.Duration("payment_timeout").Seconds())


I propose to add a check that the value is divisible by a second. The current implementation treats --payment_timeout=99.999s as --payment_timeout=99s silently. I think it should crash instead, complaining that sub-second accuracy is not supported.

starius · 2024-05-23T02:19:41Z

cmd/loop/loopout.go

@@ -235,6 +243,15 @@ func loopOut(ctx *cli.Context) error {
 		}
 	}

+	paymentTimeout := 0
+	if ctx.IsSet("payment_timeout") {
+		paymentTimeout = int(ctx.Duration("payment_timeout").Seconds())


I propose to check that paymentTimeout < math.MaxUint32 just in case, to make sure that it does not overflow in the conversion to uint32 below. It should be always satisfied, because time.Duration does not support such large values.

hieblmi

LGTM 💾

bhandras force-pushed the loop-out-timeout branch from 7428ef8 to 588145b Compare May 10, 2024 15:11

bhandras requested review from sputn1ck, hieblmi and starius May 10, 2024 15:16

bhandras force-pushed the loop-out-timeout branch from 588145b to 5af5697 Compare May 10, 2024 15:20

hieblmi reviewed May 13, 2024

View reviewed changes

bhandras force-pushed the loop-out-timeout branch from 5af5697 to b635394 Compare May 14, 2024 13:06

bhandras requested a review from hieblmi May 14, 2024 13:06

bhandras force-pushed the loop-out-timeout branch from b635394 to ff74ad2 Compare May 14, 2024 13:16

levmi assigned bhandras May 22, 2024

starius approved these changes May 23, 2024

View reviewed changes

hieblmi approved these changes May 23, 2024

View reviewed changes

bhandras added 4 commits May 23, 2024 17:17

looprpc: add payment_timeout to the LoopOutRequest

461ceee

loopdb: add migraton and DB code to handle payment timeout

4749c02

loopout: use the request defined payment timeout

0d0af5b

cli: add payment_timeout option to the CLI as well

01c017d

bhandras force-pushed the loop-out-timeout branch from ff74ad2 to 01c017d Compare May 23, 2024 15:17

bhandras merged commit e30afba into lightninglabs:master May 23, 2024
4 checks passed

bhandras deleted the loop-out-timeout branch May 23, 2024 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loopout: configurable payment timeout for off-chain payments #743

loopout: configurable payment timeout for off-chain payments #743

bhandras commented May 10, 2024

hieblmi left a comment

hieblmi May 13, 2024

hieblmi May 13, 2024

bhandras May 14, 2024

hieblmi May 13, 2024

bhandras May 14, 2024

hieblmi May 13, 2024

bhandras May 14, 2024

lightninglabs-deploy commented May 21, 2024

starius left a comment

starius May 23, 2024

bhandras May 23, 2024

starius May 23, 2024

bhandras May 23, 2024

starius May 23, 2024

bhandras May 23, 2024

starius May 23, 2024

bhandras May 23, 2024

starius May 23, 2024

bhandras May 23, 2024

starius May 23, 2024

bhandras May 23, 2024

starius May 23, 2024

bhandras May 23, 2024

hieblmi left a comment

		@@ -647,8 +650,8 @@ func (s *loopOutSwap) payInvoice(ctx context.Context, invoice string,
		var result paymentResult

		status, err := s.payInvoiceAsync(

		return nil, fmt.Errorf("payment timeout exceeds maximum "+
		"allowed timeout of %v", s.config.TotalPaymentTimeout)

loopout: configurable payment timeout for off-chain payments #743

loopout: configurable payment timeout for off-chain payments #743

Conversation

bhandras commented May 10, 2024

Pull Request Checklist

hieblmi left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented May 21, 2024

starius left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieblmi left a comment

Choose a reason for hiding this comment