routing: add payment failure reason `FailureReasonCancel` #8836

hieblmi · 2024-06-13T07:48:24Z

This PR depends on #8734 which introduced a user-cancelable payment loop.

It adds a new failure reason code channeldb.FailureReasonCanceled to distinguish payment-timeout cancels in the db from manual user cancellations.

coderabbitai · 2024-06-13T07:48:31Z

Important

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

yyforyongyu · 2024-06-18T05:57:30Z

channeldb/payments.go

-	// TODO(halseth): cancel state.
+	// FailureReasonCanceled indicates that the payment was canceled by the
+	// user.
+	FailureReasonCanceled FailureReason = 5


need to update the String method below, also wondering if it's easy to modify one of the itest cases to check this behavior (probably not as we don't have an easy way to cancel context there). Mentioning the test because I think there are other places that also need to be updated, such as lnrpc.PaymentFailureReason.

Thank you for noticing this, I've adjusted the String method and complemented the lnrpc part.

For an itest we'd probably have to create harness rpc method like SendPaymentWithContext or so, then pre-cancel it and try sending the payment, which should result in lnrpc.FAILURE_REASON_CANCELED.

Should we do that?

yeah sounds feasible to me! So sth like,

stream := alice.RPC.SendPaymentWithContext(ctx, req) ht.AssertPaymentStatusFromStream(stream, lnrpc.Payment_IN_FLIGHT) ctx.Cancel() ht.AssertPaymentStatusFromStream(stream, lnrpc.Payment_CANCELED)

I am unsure if we should introduce lnrpc.Payment_CANCELED because adding this state would be a deviation from how we handle other payment FailureReasons.

In the reference below we define state lnrpc.Payment_FAILED as an umbrella state for any FailureReason. So adding a further distinction here would complicate the flow.

lnd/channeldb/payment_status.go

Line 180 in 8c0d786

paymentFailed = true

I think the proper way to check for a cancel is first to check the payment for lnrpc.Payment_FAILED, then check the payment failure reason for ~~PaymentFailureReason_FAILURE_REASON_INCORRECT_PAYMENT_DETAILS~~lnrpc. PaymentFailureReason_FAILURE_REASON_CANCELED.

yea my bad that code ref is incorrect as we def don't want to add a new status, what I meant is,

stream := alice.RPC.SendPaymentWithContext(ctx, req) ht.AssertPaymentStatusFromStream(stream, lnrpc.Payment_IN_FLIGHT) ctx.Cancel() payment := ht.AssertPaymentStatusFromStream(stream, lnrpc.Payment_FAILED) require.Equal(ht, lnrpc.PaymentFailureReason_FAILURE_REASON_CANCELED, payment.FailureReason)

If there is nothing preventing the payment from settling then the payment status after the ctx.Cancel() should be lnrpc.Payment_SUCCEEDED since the cancellation can't abort the current payment attempt. But the failure reason should be canceled.

If attached an itest.

Edit: I've extended the test to resemble a real world example more closely.
In the itest A tries to pay C's invoice in A - B - C. B intercepts and holds the htlc until A cancels the payment context, then B fails the htlc. The expected result is that no further payment attempts are taken, that the payment is in payment state FAILED and the failure reason is FAILURE_REASON_CANCELED.

yyforyongyu

Pending an itest, otherwise LGTM!

yyforyongyu · 2024-07-10T15:16:32Z

channeldb/payments.go

-	// TODO(halseth): cancel state.
+	// FailureReasonCanceled indicates that the payment was canceled by the
+	// user.
+	FailureReasonCanceled FailureReason = 5


yeah sounds feasible to me! So sth like,

stream := alice.RPC.SendPaymentWithContext(ctx, req) ht.AssertPaymentStatusFromStream(stream, lnrpc.Payment_IN_FLIGHT) ctx.Cancel() ht.AssertPaymentStatusFromStream(stream, lnrpc.Payment_CANCELED)

lightninglabs-deploy · 2024-07-24T15:46:12Z

@bitromortac: review reminder
@hieblmi, remember to re-request review from reviewers when ready

yyforyongyu

Looking good, only a few comments in the itest.

yyforyongyu · 2024-07-29T07:57:55Z

itest/lnd_payment_test.go

+// FAILURE_REASON_CANCELED. This failure reason indicates that the context was
+// cancelled manually by the user. It does not interrupt the current payment
+// attempt, but will prevent any further payment attempts. The test steps are:
+// 1.) Alice pays Carol's invoice through Bob.


itest/lnd_payment_test.go

yyforyongyu · 2024-07-29T08:05:15Z

itest/lnd_payment_test.go

+	cancelInterceptor()
+
+	// Make sure all goroutines are finished.
+	select {


this is unnecessary as we know paymentStream was returned in the above goroutine.

yyforyongyu · 2024-07-29T08:05:55Z

lntest/rpc/router.go

+func (h *HarnessRPC) SendPaymentWithContext(context context.Context,
+	req *routerrpc.SendPaymentRequest) PaymentClient {
+
+	require.NotNil(h.T, context, "context must not be nil")


yyforyongyu · 2024-07-29T08:08:20Z

itest/lnd_payment_test.go

+
+	var preimage lntypes.Preimage
+	copy(preimage[:], invoice.RPreimage)
+	ht.AssertPaymentStatus(alice, preimage, lnrpc.Payment_IN_FLIGHT)


I think this method already returns a payment and we can assert the failure reason directly via payment.FailureReason.

This works here, but below we need to call AssertPaymentFailureReason since the failure reason update on the payment doesn't seem to happen immediately.

yyforyongyu · 2024-07-29T08:09:22Z

lntest/harness_assertion.go

@@ -1639,6 +1639,19 @@ func (h *HarnessTest) AssertPaymentStatus(hn *node.HarnessNode,
 	return target
 }

+// AssertPaymentFailureReason asserts that the given node lists a payment with
+// the given preimage which has the expected failure reason.
+func (h *HarnessTest) AssertPaymentFailureReason(hn *node.HarnessNode,


think this assertion is unnecessary since we always have the payment and can assert the fields on it directly.

not addressed

I've removed the Predicate but for mentioned reason I couldn't remove the assertion.

yyforyongyu · 2024-07-29T08:10:24Z

lntest/harness_assertion.go

+	preimage lntypes.Preimage, reason lnrpc.PaymentFailureReason) {
+
+	payHash := preimage.Hash()
+	err := wait.Predicate(func() bool {


future tips: we almost never use Predicate and favor NoError as it gives us more descriptive error - maybe we should remove it someday.

bitromortac

Nice, almost ready 👍. Have a suggestion concerning the itest

bitromortac · 2024-07-29T07:57:58Z

routing/payment_lifecycle_test.go

@@ -772,17 +772,17 @@ func TestResumePaymentFailContextCancel(t *testing.T) {
 	cancel()

 	m.control.On(
-		"FailPayment", p.identifier, channeldb.FailureReasonTimeout,
+		"FailPayment", p.identifier, channeldb.FailureReasonCanceled,


nit: could you squash this with the previous commit, to make the unit test pass there? I think it's important to let every unit test pass in each commit, to make bisecting of commits easier

itest/lnd_payment_test.go

yyforyongyu

One final comment, otherwise good to go!

yyforyongyu · 2024-07-31T20:06:29Z

itest/lnd_payment_test.go

+	require.NoError(ht, err, "failed to send request")
+
+	// Assert that the payment status is as expected.
+	ht.AssertPaymentStatus(alice, preimage, expectedPaymentStatus)


think we can grab the payment here and check its FailureReason below.

The status on the payment here is not set yet, hence waiting for it in the extra assertion, see comment #8836 (comment)

yyforyongyu · 2024-07-31T20:06:42Z

lntest/harness_assertion.go

@@ -1639,6 +1639,19 @@ func (h *HarnessTest) AssertPaymentStatus(hn *node.HarnessNode,
 	return target
 }

+// AssertPaymentFailureReason asserts that the given node lists a payment with
+// the given preimage which has the expected failure reason.
+func (h *HarnessTest) AssertPaymentFailureReason(hn *node.HarnessNode,


not addressed

yyforyongyu

Thanks for the fix! LGTM🛳️

bitromortac

The PR looks good 🎉, but technically this is a breaking change, should somebody rely on the timeout failure code which now becomes canceled (in some cases). So therefore v0.18.3 may not be the best release for that.

hieblmi · 2024-08-03T11:41:14Z

The PR looks good 🎉, but technically this is a breaking change, should somebody rely on the timeout failure code which now becomes canceled (in some cases). So therefore v0.18.3 may not be the best release for that.

canceled only appears if the new cancelable flag on the payment request is set, so that it doesn't break old clients. Is your concern still valid then?

hieblmi requested review from yyforyongyu and bitromortac June 13, 2024 07:48

hieblmi force-pushed the payment-failure-reason-cancel branch from ffe9cf8 to a8a49f2 Compare June 13, 2024 07:53

hieblmi self-assigned this Jun 13, 2024

hieblmi marked this pull request as draft June 13, 2024 07:53

hieblmi force-pushed the payment-failure-reason-cancel branch from a8a49f2 to 1b2ec34 Compare June 13, 2024 08:21

hieblmi mentioned this pull request Jun 13, 2024

routing: cancelable payment loop #8734

Merged

hieblmi force-pushed the payment-failure-reason-cancel branch 2 times, most recently from d67ffc1 to 3cb50a8 Compare June 17, 2024 11:03

hieblmi marked this pull request as ready for review June 17, 2024 11:04

hieblmi force-pushed the payment-failure-reason-cancel branch from 3cb50a8 to bd6c7f4 Compare June 17, 2024 11:29

yyforyongyu requested changes Jun 18, 2024

View reviewed changes

hieblmi force-pushed the payment-failure-reason-cancel branch 2 times, most recently from 3c1d246 to 4ac08ce Compare July 8, 2024 10:26

hieblmi requested a review from yyforyongyu July 9, 2024 12:55

yyforyongyu reviewed Jul 10, 2024

View reviewed changes

hieblmi force-pushed the payment-failure-reason-cancel branch from 4ac08ce to e95fe0d Compare July 26, 2024 15:19

hieblmi requested a review from yyforyongyu July 26, 2024 15:32

hieblmi force-pushed the payment-failure-reason-cancel branch 6 times, most recently from aa68ded to 0dc2414 Compare July 28, 2024 13:11

hieblmi added this to the v0.18.3 milestone Jul 28, 2024

yyforyongyu requested changes Jul 29, 2024

View reviewed changes

bitromortac reviewed Jul 29, 2024

View reviewed changes

hieblmi force-pushed the payment-failure-reason-cancel branch from 0dc2414 to 9c97a61 Compare July 30, 2024 18:49

hieblmi force-pushed the payment-failure-reason-cancel branch 2 times, most recently from 81589c4 to b55ff63 Compare July 30, 2024 20:21

hieblmi requested review from bitromortac and yyforyongyu July 31, 2024 05:35

yyforyongyu reviewed Jul 31, 2024

View reviewed changes

hieblmi added 5 commits August 1, 2024 10:24

lnrpc: payment failure reason canceled

d18c4d6

routing: new failure reason for cancelled payments

2e3c96f

itest: format and typo fixes

653226d

itest: payment failure reason canceled

0273eac

docs: update release notes

808d958

hieblmi force-pushed the payment-failure-reason-cancel branch from b55ff63 to 808d958 Compare August 1, 2024 08:27

hieblmi requested a review from yyforyongyu August 1, 2024 09:16

yyforyongyu approved these changes Aug 1, 2024

View reviewed changes

bitromortac approved these changes Aug 1, 2024

View reviewed changes

Roasbeef merged commit c46b1a4 into lightningnetwork:master Aug 1, 2024
32 of 34 checks passed

hieblmi deleted the payment-failure-reason-cancel branch August 3, 2024 06:21

routing: add payment failure reason FailureReasonCancel #8836

routing: add payment failure reason FailureReasonCancel #8836

Conversation

hieblmi commented Jun 13, 2024 • edited Loading

coderabbitai bot commented Jun 13, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieblmi Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieblmi Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Jul 24, 2024

yyforyongyu 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

bitromortac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu 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

yyforyongyu left a comment

Choose a reason for hiding this comment

bitromortac left a comment • edited Loading

Choose a reason for hiding this comment

hieblmi commented Aug 3, 2024

routing: add payment failure reason `FailureReasonCancel` #8836

routing: add payment failure reason `FailureReasonCancel` #8836

hieblmi commented Jun 13, 2024 •

edited

Loading

coderabbitai bot commented Jun 13, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

hieblmi Jul 26, 2024 •

edited

Loading

hieblmi Jul 26, 2024 •

edited

Loading

bitromortac left a comment •

edited

Loading