Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

routing: add payment failure reason FailureReasonCancel #8836

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions channeldb/payments.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ var (
)

var (
// ErrNoSequenceNumber is returned if we lookup a payment which does
// ErrNoSequenceNumber is returned if we look up a payment which does
// not have a sequence number.
ErrNoSequenceNumber = errors.New("sequence number not found")

Expand Down Expand Up @@ -147,18 +147,20 @@ const (
// balance to complete the payment.
FailureReasonInsufficientBalance FailureReason = 4

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

@hieblmi hieblmi Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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_DETAILSlnrpc. PaymentFailureReason_FAILURE_REASON_CANCELED.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

@hieblmi hieblmi Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// TODO(joostjager): Add failure reasons for:
// LocalLiquidityInsufficient, RemoteCapacityInsufficient.
)

// Error returns a human readable error string for the FailureReason.
// Error returns a human-readable error string for the FailureReason.
func (r FailureReason) Error() string {
return r.String()
}

// String returns a human readable FailureReason.
// String returns a human-readable FailureReason.
func (r FailureReason) String() string {
switch r {
case FailureReasonTimeout:
Expand All @@ -171,6 +173,8 @@ func (r FailureReason) String() string {
return "incorrect_payment_details"
case FailureReasonInsufficientBalance:
return "insufficient_balance"
case FailureReasonCanceled:
return "canceled"
}

return "unknown"
Expand Down
5 changes: 5 additions & 0 deletions docs/release-notes/release-notes-0.18.3.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ commitment when the channel was force closed.
`--amp` flag when sending a payment specifying the payment request.

## Code Health

* [Added](https://github.com/lightningnetwork/lnd/pull/8836) a new failure
reason `FailureReasonCanceled` to the list of payment failure reasons. It
indicates that a payment was manually cancelled by the user.

## Breaking Changes
## Performance Improvements

Expand Down
4 changes: 4 additions & 0 deletions itest/list_on_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ var allTestCases = []*lntest.TestCase{
Name: "immediate payment after channel opened",
TestFunc: testPaymentFollowingChannelOpen,
},
{
Name: "payment failure reason canceled",
TestFunc: testPaymentFailureReasonCanceled,
},
{
Name: "invoice update subscription",
TestFunc: testInvoiceSubscriptions,
Expand Down
162 changes: 148 additions & 14 deletions itest/lnd_payment_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package itest

import (
"context"
"crypto/sha256"
"encoding/hex"
"fmt"
Expand All @@ -13,19 +14,21 @@ import (
"github.com/lightningnetwork/lnd/lnrpc/routerrpc"
"github.com/lightningnetwork/lnd/lntest"
"github.com/lightningnetwork/lnd/lntest/node"
"github.com/lightningnetwork/lnd/lntest/rpc"
"github.com/lightningnetwork/lnd/lntest/wait"
"github.com/lightningnetwork/lnd/lntypes"
"github.com/stretchr/testify/require"
)

// testSendDirectPayment creates a topology Alice->Bob and then tests that
// Alice can send a direct payment to Bob. This test modifies the fee estimator
// to return floor fee rate(1 sat/vb).
// testSendDirectPayment creates a topology Alice->Bob and then tests that Alice
// can send a direct payment to Bob. This test modifies the fee estimator to
// return floor fee rate(1 sat/vb).
func testSendDirectPayment(ht *lntest.HarnessTest) {
// Grab Alice and Bob's nodes for convenience.
alice, bob := ht.Alice, ht.Bob

// Create a list of commitment types we want to test.
commitTyes := []lnrpc.CommitmentType{
commitmentTypes := []lnrpc.CommitmentType{
lnrpc.CommitmentType_ANCHORS,
lnrpc.CommitmentType_SIMPLE_TAPROOT,
}
Expand Down Expand Up @@ -109,7 +112,7 @@ func testSendDirectPayment(ht *lntest.HarnessTest) {
}

// Run the test cases.
for _, ct := range commitTyes {
for _, ct := range commitmentTypes {
ht.Run(ct.String(), func(t *testing.T) {
st := ht.Subtest(t)

Expand All @@ -132,8 +135,9 @@ func testSendDirectPayment(ht *lntest.HarnessTest) {
}

// Open private channel for taproot channels.
params.Private = ct ==
lnrpc.CommitmentType_SIMPLE_TAPROOT
if ct == lnrpc.CommitmentType_SIMPLE_TAPROOT {
params.Private = true
}

testSendPayment(st, params)
})
Expand Down Expand Up @@ -429,7 +433,7 @@ func runAsyncPayments(ht *lntest.HarnessTest, alice, bob *node.HarnessNode,
// likely be lower, but we can't guarantee that any more HTLCs will
// succeed due to the limited path diversity and inability of the router
// to retry via another path.
numInvoices := int(input.MaxHTLCNumber / 2)
numInvoices := input.MaxHTLCNumber / 2

bobAmt := int64(numInvoices * paymentAmt)
aliceAmt := info.LocalBalance - bobAmt
Expand Down Expand Up @@ -534,10 +538,10 @@ func testBidirectionalAsyncPayments(ht *lntest.HarnessTest) {

// We'll create a number of invoices equal the max number of HTLCs that
// can be carried in one direction. The number on the commitment will
// likely be lower, but we can't guarantee that any more HTLCs will
// succeed due to the limited path diversity and inability of the router
// to retry via another path.
numInvoices := int(input.MaxHTLCNumber / 2)
// likely be lower, but we can't guarantee that more HTLCs will succeed
// due to the limited path diversity and inability of the router to
// retry via another path.
numInvoices := input.MaxHTLCNumber / 2

// Nodes should exchange the same amount of money and because of this
// at the end balances should remain the same.
Expand Down Expand Up @@ -597,7 +601,7 @@ func testBidirectionalAsyncPayments(ht *lntest.HarnessTest) {
assertChannelState(ht, alice, chanPoint, aliceAmt, bobAmt)

// Next query for Bob's and Alice's channel states, in order to confirm
// that all payment have been successful transmitted.
// that all payment have been successfully transmitted.
assertChannelState(ht, bob, chanPoint, bobAmt, aliceAmt)

// Finally, immediately close the channel. This function will also
Expand Down Expand Up @@ -662,7 +666,7 @@ func testInvoiceSubscriptions(ht *lntest.HarnessTest) {

// Now that the set of invoices has been added, we'll re-register for
// streaming invoice notifications for Bob, this time specifying the
// add invoice of the last prior invoice.
// add index of the last prior invoice.
req = &lnrpc.InvoiceSubscription{AddIndex: lastAddIndex}
bobInvoiceSubscription = bob.RPC.SubscribeInvoices(req)

Expand Down Expand Up @@ -766,3 +770,133 @@ func assertChannelState(ht *lntest.HarnessTest, hn *node.HarnessNode,
}, lntest.DefaultTimeout)
require.NoError(ht, err, "timeout while chekcing for balance")
}

// testPaymentFailureReasonCanceled ensures that the cancellation of a
// SendPayment request results in the payment failure reason
// 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice👍

// 2.) Bob intercepts the htlc, keeping the payment pending.
// 3.) Alice cancels the payment context, the payment is still pending.
// 4.) Bob fails OR resumes the intercepted HTLC.
// 5.) Alice observes a failed OR succeeded payment with failure reason
// FAILURE_REASON_CANCELED which suppresses further payment attempts.
func testPaymentFailureReasonCanceled(ht *lntest.HarnessTest) {
Roasbeef marked this conversation as resolved.
Show resolved Hide resolved
// Initialize the test context with 3 connected nodes.
ts := newInterceptorTestScenario(ht)

alice, bob, carol := ts.alice, ts.bob, ts.carol

// Open and wait for channels.
const chanAmt = btcutil.Amount(300000)
p := lntest.OpenChannelParams{Amt: chanAmt}
reqs := []*lntest.OpenChannelRequest{
{Local: alice, Remote: bob, Param: p},
{Local: bob, Remote: carol, Param: p},
}
resp := ht.OpenMultiChannelsAsync(reqs)
cpAB, cpBC := resp[0], resp[1]

// Make sure Alice is aware of channel Bob=>Carol.
ht.AssertTopologyChannelOpen(alice, cpBC)

// First we check that the payment is successful when bob resumes the
// htlc even though the payment context was canceled before invoice
// settlement.
sendPaymentInterceptAndCancel(
ht, ts, cpAB, routerrpc.ResolveHoldForwardAction_RESUME,
lnrpc.Payment_SUCCEEDED,
)

// Next we check that the context cancellation results in the expected
// failure reason while the htlc is being held and failed after
// cancellation.
// Note that we'd have to reset Alice's mission control if we tested the
// htlc fail case before the htlc resume case.
sendPaymentInterceptAndCancel(
ht, ts, cpAB, routerrpc.ResolveHoldForwardAction_FAIL,
lnrpc.Payment_FAILED,
)

// Finally, close channels.
ht.CloseChannel(alice, cpAB)
ht.CloseChannel(bob, cpBC)
}

func sendPaymentInterceptAndCancel(ht *lntest.HarnessTest,
ts *interceptorTestScenario, cpAB *lnrpc.ChannelPoint,
interceptorAction routerrpc.ResolveHoldForwardAction,
expectedPaymentStatus lnrpc.Payment_PaymentStatus) {

// Prepare the test cases.
alice, bob, carol := ts.alice, ts.bob, ts.carol

// Connect the interceptor.
interceptor, cancelInterceptor := bob.RPC.HtlcInterceptor()

// Prepare the test cases.
addResponse := carol.RPC.AddInvoice(&lnrpc.Invoice{
ValueMsat: 1000,
})
invoice := carol.RPC.LookupInvoice(addResponse.RHash)

// We initiate a payment from Alice and define the payment context
// cancellable.
ctx, cancelPaymentContext := context.WithCancel(context.Background())
var paymentStream rpc.PaymentClient
go func() {
req := &routerrpc.SendPaymentRequest{
PaymentRequest: invoice.PaymentRequest,
TimeoutSeconds: 60,
FeeLimitSat: 100000,
Cancelable: true,
}

paymentStream = alice.RPC.SendPaymentWithContext(ctx, req)
}()

// We start the htlc interceptor with a simple implementation that
// saves all intercepted packets. These packets are held to simulate a
// pending payment.
packet := ht.ReceiveHtlcInterceptor(interceptor)

// Here we should wait for the channel to contain a pending htlc, and
// also be shown as being active.
yyforyongyu marked this conversation as resolved.
Show resolved Hide resolved
ht.AssertIncomingHTLCActive(bob, cpAB, invoice.RHash)

// Ensure that Alice's payment is in-flight because Bob is holding the
// htlc.
ht.AssertPaymentStatusFromStream(paymentStream, lnrpc.Payment_IN_FLIGHT)

// Cancel the payment context. This should end the payment stream
// context, but the payment should still be in state in-flight without a
// failure reason.
cancelPaymentContext()

var preimage lntypes.Preimage
copy(preimage[:], invoice.RPreimage)
payment := ht.AssertPaymentStatus(
alice, preimage, lnrpc.Payment_IN_FLIGHT,
)
reasonNone := lnrpc.PaymentFailureReason_FAILURE_REASON_NONE
require.Equal(ht, reasonNone, payment.FailureReason)

// Bob sends the interceptor action to the intercepted htlc.
err := interceptor.Send(&routerrpc.ForwardHtlcInterceptResponse{
IncomingCircuitKey: packet.IncomingCircuitKey,
Action: interceptorAction,
})
require.NoError(ht, err, "failed to send request")

// Assert that the payment status is as expected.
ht.AssertPaymentStatus(alice, preimage, expectedPaymentStatus)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Since the payment context was cancelled, no further payment attempts
// should've been made, and we observe FAILURE_REASON_CANCELED.
expectedReason := lnrpc.PaymentFailureReason_FAILURE_REASON_CANCELED
ht.AssertPaymentFailureReason(alice, preimage, expectedReason)

// Cancel the context, which will disconnect the above interceptor.
cancelInterceptor()
}
Loading
Loading