routerrpc: optionally return the new payment status #8177

yyforyongyu · 2023-11-14T12:39:16Z

Fixes #8143, this PR adds a new config value to signal whether to use the new payment status.

This commit is a pure refactoring which sorts the fields in `SendPaymentRequest` so it's easier to see what number to use when adding a new field.

carlaKC

Nice fix for a tricky upgrade situation 🌷

Wondering whether this could introduce a race in the trackpayment itest?

The standard Alice/Bob nodes are set up to send this new status, but we assert that we get IN_FLIGHT first.
Since SubscribePayment just grabs whatever state we currently have, the first status we get could be initiated or in flight?

Perhaps that's okay because it hasn't happened since the PR with the new state was added, but may come back to haunt us.

lnrpc/routerrpc/router.proto

carlaKC · 2023-11-14T15:25:35Z

lnrpc/routerrpc/config.go

+	//
+	// 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."`


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

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.

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 :(

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.

sample-lnd.conf

yyforyongyu · 2023-11-15T10:39:47Z

Wondering whether this could introduce a race in the trackpayment itest?

Nice catch @carlaKC ! I think this is indeed a case we need to fix. Turns out we need to notify the subscribers when a new payment is made, so I updated the InitPayment to always notify it. Pushed a new version and see if the tests pass.

This commit adds a new config value to signal that the user understands the new payment status `StatusInitiated`.

This commit fixes `InitPayment` method to make sure the subscribers get notified when new payments are created.

This commit adds an itest case to make sure when the flag `routerrpc.usestatusinitiated` is not set, the new status is not sent.

carlaKC

LGTM, only one comment on strategy for getting folks upgraded (opt in vs opt out), but it's a weak opinion weakly held.

lightninglabs-deploy · 2023-11-23T15:31:14Z

@bitromortac: review reminder

bitromortac

LGTM 🎉 Concerning the transition period for the removal of the flag, it would be good to add a permanent entry in the change log under breaking changes until v0.20 saying that developers should check any unexpected behavior of their software when enabling the flag (we could link to the issue of this PR as well).

bitromortac · 2023-11-28T10:55:11Z

lnrpc/routerrpc/config.go

+	//
+	// 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."`


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.

guggero · 2023-11-28T16:45:28Z

Sorry to chime in a bit late. But I'm not sure I understand how this flag actually helps us with the compatibility issue?
The issue as I understand it is that in a client application (e.g. litd's custodial accounts functionality) we need to know whether lnd will serve this status or not. Or to be even more precise, we need to make sure that if an version of the client code is used that does not yet understand the new initiated code that things fail gracefully instead of with unexpected behavior.
So what I think we need is some kind of RPC flag. For example a new field in the affected RPC methods, e.g. use_initiated_status. Then we can set that to true in the updated lndclient code. And we probably have to make that lndclient change breaking so every application that uses the new lndclient version is also forced to handle the new state.

Or am I completely misunderstanding something here?

yyforyongyu · 2023-11-28T19:32:05Z

@guggero so the idea here is if users don't explicit set this config then the new status won't be served so the clients should be able to understand the status field since nothing is changed.

The issue as I understand it is that in a client application (e.g. litd's custodial accounts functionality) we need to know whether lnd will serve this status or not.

yeah exactly. From client application's perspective, they set this config to know that lnd will serve it. And from lnd's view, it reads the config to be sure that the client understands it.

Or to be even more precise, we need to make sure that if an version of the client code is used that does not yet understand the new initiated code that things fail gracefully instead of with unexpected behavior.

If the client doesn't understand it, it won't be served. Things would break, however, if lnd is set to use --routerrpc.usestatusinitiated without the client being updated first, which is a misbehave at the client side.

So what I think we need is some kind of RPC flag.

There're offline discussions that rpc flag and config flag can both work(actually using RPC flag was the initial approach), while upgrading via config has several advantages:

it can easily be removed/deprecated(for instance we can use the hidden flag in config so we won't break users' scripts), while for RPC flags we need to first write compatible code then remove it in future versions.
it's a smaller change as opposed to RPC flags, as we need to visit SendPaymentRequest, TrackPaymentRequest, SendToRouteRequest and ListPaymentsRequest to add this new flag.

guggero · 2023-11-29T14:37:47Z

Okay, so I guess the goal of the flag then is to be able to test the new functionality (in manual or integration tests).
But any user unaware of the flag would keep their node at the old behavior, giving them time to upgrade. Then some point in the future we'd just remove the flag and turn on the behavior by default.

I guess in terms of implementation simplicity that is easier indeed. My main worry would be that we wouldn't discover most issues in the wild as 99.9% of the users wouldn't turn on the flag, even after upgrading their whole stack. But we can turn this on in litd when running in integrated mode, where we know the whole tech stack. So we still get to run this in production for at least a portion of our user base.

yyforyongyu · 2023-11-29T16:47:54Z

I guess in terms of implementation simplicity that is easier indeed. My main worry would be that we wouldn't discover most issues in the wild as 99.9% of the users wouldn't turn on the flag, even after upgrading their whole stack.

yeah i'm almost certain this would be the case😂, which is the concern mentioned here.

I think a future strategy of upgrading our RPCs could be,

use a config flag to let user specify they are ready for the new state
then after X releases, make this flag mandatory and lnd can give a detailed error message about how to proceed
then after Y releases, mention the flag will be removed and make this flag hidden so it won't break any shell scripts
then after Z releases, remove this flag

So in this case, this could be,

at version 0.2x.0, lnd won't start unless usestatusinitiated is set
at version 0.3x.0, usestatusinitiated will be hidden
at version 0.4x.0, usestatusinitiated will be removed

or something like that.

guggero · 2023-11-29T17:41:55Z

Sounds good to me 👍 Thanks for all the explanations!

yyforyongyu added 2 commits November 14, 2023 17:46

routerrpc: skip initiated payment in trackPaymentStream

922130c

routerrpc: modify field orders based on their assigned number

c58454e

This commit is a pure refactoring which sorts the fields in `SendPaymentRequest` so it's easier to see what number to use when adding a new field.

yyforyongyu self-assigned this Nov 14, 2023

yyforyongyu added payments Related to invoices/payments compatibility labels Nov 14, 2023

yyforyongyu added this to the v0.18.0 milestone Nov 14, 2023

yyforyongyu force-pushed the payment-status branch from a639b79 to 9de6608 Compare November 14, 2023 12:40

carlaKC reviewed Nov 14, 2023

View reviewed changes

yyforyongyu force-pushed the payment-status branch 2 times, most recently from 9b41f43 to cc7948a Compare November 15, 2023 10:38

yyforyongyu force-pushed the payment-status branch from cc7948a to 484bd45 Compare November 15, 2023 12:01

yyforyongyu added 4 commits November 15, 2023 20:18

multi: add new config usestatusinitiated for the new payment status

816a70e

This commit adds a new config value to signal that the user understands the new payment status `StatusInitiated`.

docs: update release notes

9e00799

routing: make sure StatusInitiated is notified when creating payments

9acbbf0

This commit fixes `InitPayment` method to make sure the subscribers get notified when new payments are created.

routerrpc+itest: make sure TrackPayments is compatible

9935431

This commit adds an itest case to make sure when the flag `routerrpc.usestatusinitiated` is not set, the new status is not sent.

yyforyongyu force-pushed the payment-status branch from 484bd45 to 9935431 Compare November 15, 2023 12:19

yyforyongyu requested a review from carlaKC November 15, 2023 16:41

carlaKC approved these changes Nov 15, 2023

View reviewed changes

saubyk requested a review from bitromortac November 16, 2023 15:27

bitromortac approved these changes Nov 28, 2023

View reviewed changes

yyforyongyu merged commit 9f42459 into lightningnetwork:master Nov 30, 2023
24 of 25 checks passed

yyforyongyu deleted the payment-status branch November 30, 2023 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

routerrpc: optionally return the new payment status #8177

routerrpc: optionally return the new payment status #8177

yyforyongyu commented Nov 14, 2023

carlaKC left a comment

carlaKC Nov 14, 2023

yyforyongyu Nov 15, 2023

carlaKC Nov 15, 2023

bitromortac Nov 28, 2023

yyforyongyu commented Nov 15, 2023

carlaKC left a comment

lightninglabs-deploy commented Nov 23, 2023

bitromortac left a comment

bitromortac Nov 28, 2023

guggero commented Nov 28, 2023

yyforyongyu commented Nov 28, 2023

guggero commented Nov 29, 2023

yyforyongyu commented Nov 29, 2023

guggero commented Nov 29, 2023

routerrpc: optionally return the new payment status #8177

routerrpc: optionally return the new payment status #8177

Conversation

yyforyongyu commented Nov 14, 2023

carlaKC left a comment

Choose a reason for hiding this comment

carlaKC Nov 14, 2023

Choose a reason for hiding this comment

yyforyongyu Nov 15, 2023

Choose a reason for hiding this comment

carlaKC Nov 15, 2023

Choose a reason for hiding this comment

bitromortac Nov 28, 2023

Choose a reason for hiding this comment

yyforyongyu commented Nov 15, 2023

carlaKC left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Nov 23, 2023

bitromortac left a comment

Choose a reason for hiding this comment

bitromortac Nov 28, 2023

Choose a reason for hiding this comment

guggero commented Nov 28, 2023

yyforyongyu commented Nov 28, 2023

guggero commented Nov 29, 2023

yyforyongyu commented Nov 29, 2023

guggero commented Nov 29, 2023