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

routerrpc: optionally return the new payment status #8177

Merged
merged 6 commits into from
Nov 30, 2023
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
5 changes: 5 additions & 0 deletions docs/release-notes/release-notes-0.18.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@
* A new config value,
[http-header-timeout](https://github.com/lightningnetwork/lnd/pull/7715), is added so users can specify the amount of time the http server will wait for a request to complete before closing the connection. The default value is 5 seconds.

* [`routerrpc.usestatusinitiated` is
introduced](https://github.com/lightningnetwork/lnd/pull/8177) to signal that
the new payment status `Payment_INITIATED` should be used for payment-related
RPCs. It's recommended to use it to provide granular controls over payments.

## RPC Additions

* [Deprecated](https://github.com/lightningnetwork/lnd/pull/7175)
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 @@ -490,6 +490,10 @@ var allTestCases = []*lntest.TestCase{
Name: "trackpayments",
TestFunc: testTrackPayments,
},
{
Name: "trackpayments compatible",
TestFunc: testTrackPaymentsCompatible,
},
{
Name: "open channel fee policy",
TestFunc: testOpenChannelUpdateFeePolicy,
Expand Down
11 changes: 11 additions & 0 deletions itest/lnd_max_htlcs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@ func testMaxHtlcPathfind(ht *lntest.HarnessTest) {
maxHtlcs := 5

alice, bob := ht.Alice, ht.Bob

// Restart nodes with the new flag so they understand the new payment
// status.
ht.RestartNodeWithExtraArgs(alice, []string{
"--routerrpc.usestatusinitiated",
})
ht.RestartNodeWithExtraArgs(bob, []string{
"--routerrpc.usestatusinitiated",
})

ht.EnsureConnected(alice, bob)
chanPoint := ht.OpenChannel(
alice, bob, lntest.OpenChannelParams{
Amt: 1000000,
Expand Down
125 changes: 106 additions & 19 deletions itest/lnd_trackpayments_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,16 @@ import (
// testTrackPayments tests whether a client that calls the TrackPayments api
// receives payment updates.
func testTrackPayments(ht *lntest.HarnessTest) {
// Open a channel between alice and bob.
alice, bob := ht.Alice, ht.Bob

// Restart Alice with the new flag so she understands the new payment
// status.
ht.RestartNodeWithExtraArgs(alice, []string{
"--routerrpc.usestatusinitiated",
})

// Open a channel between alice and bob.
ht.EnsureConnected(alice, bob)
channel := ht.OpenChannel(
alice, bob, lntest.OpenChannelParams{
Amt: btcutil.Amount(300000),
Expand Down Expand Up @@ -47,36 +55,115 @@ func testTrackPayments(ht *lntest.HarnessTest) {

// Make sure the payment doesn't error due to invalid parameters or so.
_, err := paymentClient.Recv()
require.NoError(ht, err, "unable to get payment update.")
require.NoError(ht, err, "unable to get payment update")

// Assert the first payment update is an inflight update.
// Assert the first payment update is an initiated update.
update1, err := tracker.Recv()
require.NoError(ht, err, "unable to receive payment update 1.")
require.NoError(ht, err, "unable to receive payment update 1")

require.Equal(
ht, lnrpc.PaymentFailureReason_FAILURE_REASON_NONE,
update1.FailureReason,
)
require.Equal(ht, lnrpc.Payment_IN_FLIGHT, update1.Status)
require.Equal(ht, lnrpc.Payment_INITIATED, update1.Status)
require.Equal(ht, lnrpc.PaymentFailureReason_FAILURE_REASON_NONE,
update1.FailureReason)
require.Equal(ht, invoice.PaymentRequest, update1.PaymentRequest)
require.Equal(ht, amountMsat, update1.ValueMsat)

// Assert the second payment update is a payment success update.
// Assert the first payment update is an inflight update.
update2, err := tracker.Recv()
require.NoError(ht, err, "unable to receive payment update 2.")
require.NoError(ht, err, "unable to receive payment update 2")

require.Equal(
ht, lnrpc.PaymentFailureReason_FAILURE_REASON_NONE,
update2.FailureReason,
)
require.Equal(ht, lnrpc.Payment_SUCCEEDED, update2.Status)
require.Equal(ht, lnrpc.Payment_IN_FLIGHT, update2.Status)
require.Equal(ht, lnrpc.PaymentFailureReason_FAILURE_REASON_NONE,
update2.FailureReason)
require.Equal(ht, invoice.PaymentRequest, update2.PaymentRequest)
require.Equal(ht, amountMsat, update2.ValueMsat)
require.Equal(
ht, hex.EncodeToString(invoice.RPreimage),
update2.PaymentPreimage,

// Assert the third payment update is a payment success update.
update3, err := tracker.Recv()
require.NoError(ht, err, "unable to receive payment update 3")

require.Equal(ht, lnrpc.Payment_SUCCEEDED, update3.Status)
require.Equal(ht, lnrpc.PaymentFailureReason_FAILURE_REASON_NONE,
update3.FailureReason)
require.Equal(ht, invoice.PaymentRequest, update3.PaymentRequest)
require.Equal(ht, amountMsat, update3.ValueMsat)
require.Equal(ht, hex.EncodeToString(invoice.RPreimage),
update3.PaymentPreimage)

// TODO(yy): remove the sleep once the following bug is fixed.
// When the invoice is reported settled, the commitment dance is not
// yet finished, which can cause an error when closing the channel,
// saying there's active HTLCs. We need to investigate this issue and
// reverse the order to, first finish the commitment dance, then report
// the invoice as settled.
time.Sleep(2 * time.Second)

ht.CloseChannel(alice, channel)
}

// testTrackPaymentsCompatible checks that when `routerrpc.usestatusinitiated`
// is not set, the new Payment_INITIATED is replaced with Payment_IN_FLIGHT.
func testTrackPaymentsCompatible(ht *lntest.HarnessTest) {
// Open a channel between alice and bob.
alice, bob := ht.Alice, ht.Bob
channel := ht.OpenChannel(
alice, bob, lntest.OpenChannelParams{
Amt: btcutil.Amount(300000),
},
)

// Call the TrackPayments api to listen for payment updates.
req := &routerrpc.TrackPaymentsRequest{
NoInflightUpdates: false,
}
tracker := alice.RPC.TrackPayments(req)

// Create an invoice from bob.
var amountMsat int64 = 1000
invoiceResp := bob.RPC.AddInvoice(
&lnrpc.Invoice{
ValueMsat: amountMsat,
},
)
invoice := bob.RPC.LookupInvoice(invoiceResp.RHash)

// Send payment from alice to bob.
paymentClient := alice.RPC.SendPayment(
&routerrpc.SendPaymentRequest{
PaymentRequest: invoice.PaymentRequest,
TimeoutSeconds: 60,
},
)

// Assert the first track payment update is an inflight update.
update1, err := tracker.Recv()
require.NoError(ht, err, "unable to receive payment update 1")
require.Equal(ht, lnrpc.Payment_IN_FLIGHT, update1.Status)

// Assert the first track payment update is an inflight update.
update2, err := tracker.Recv()
require.NoError(ht, err, "unable to receive payment update 2")
require.Equal(ht, lnrpc.Payment_IN_FLIGHT, update2.Status)

// Assert the third track payment update is a payment success update.
update3, err := tracker.Recv()
require.NoError(ht, err, "unable to receive payment update 3")
require.Equal(ht, lnrpc.Payment_SUCCEEDED, update3.Status)

// Assert the first payment client update is an inflight update.
payment1, err := paymentClient.Recv()
require.NoError(ht, err, "unable to get payment update")
require.Equal(ht, lnrpc.Payment_IN_FLIGHT, payment1.Status)

// Assert the second payment client update is an inflight update.
payment2, err := paymentClient.Recv()
require.NoError(ht, err, "unable to get payment update")
require.Equal(ht, lnrpc.Payment_IN_FLIGHT, payment2.Status)

// Assert the third payment client update is a success update.
payment3, err := paymentClient.Recv()
require.NoError(ht, err, "unable to get payment update")
require.Equal(ht, lnrpc.Payment_SUCCEEDED, payment3.Status)

// TODO(yy): remove the sleep once the following bug is fixed.
// When the invoice is reported settled, the commitment dance is not
// yet finished, which can cause an error when closing the channel,
Expand Down
9 changes: 9 additions & 0 deletions lnrpc/routerrpc/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,18 @@ import (
// The fields with struct tags are meant to be parsed as normal configuration
// options, while if able to be populated, the latter fields MUST also be
// specified.
//
//nolint:lll
type Config struct {
RoutingConfig

// UseStatusInitiated is a boolean that indicates whether the router
// should use the new status code `Payment_INITIATED`.
//
// TODO(yy): remove this config after the new status code is fully
// deployed to the network(v0.20.0).
UseStatusInitiated bool `long:"usestatusinitiated" description:"If true, the router will send Payment_INITIATED for new payments, otherwise Payment_In_FLIGHT will be sent for compatibility concerns."`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the plan here to default this to true in 19 and then remove in 20?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah think it'd give people enough time to upgrade? The other approach is to let users specify an RPC version in SendPayment request, but that would end up being messy. My concern is, if people do not use this new flag in the coming versions, we'd still have this compatibility issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah think it'd give people enough time to upgrade? The other approach is to let users specify an RPC version in SendPayment request, but that would end up being messy.

Makes sense! Agree that putting it on the RPC is going to be ugly, we use the payment state in too many places (eg, would also need it in ListPayments, for example).

My concern is, if people do not use this new flag in the coming versions, we'd still have this compatibility issue.

Yah this is tough. Could force it more by making this flag DisableStatusInitiated and then people have to set it to prevent the breaking change, but there really is no easy path :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be part of every release notes (under breaking changes) until v0.20 in order for devs to check for any unwanted behavior, otherwise there won't be any real deployment process. DisableStatusInitiated would be too dangerous, I think.


// RouterMacPath is the path for the router macaroon. If unspecified
// then we assume that the macaroon will be found under the network
// directory, named DefaultRouterMacFilename.
Expand Down
Loading
Loading