Pay enhancements: remove legacy, add maxfee and description params #5122

rustyrussell · 2022-03-24T02:14:35Z

~~This builds on #5121 (first three commits).~~ merged, rebased

That PR made me realize we violate bolt11 when paying a description_hash invoice, so add a description parameter and deprecate payment of such an invoice without it, and I want to do that this release if possible.

However, there was some more work I wanted to do on the way...

Trips on our RPC checking introduced at the same time: msat should be an integer or an "xxxmsat"/sat/btc string. Signed-off-by: Rusty Russell <[email protected]>

I think the new pay command has proven itself in the last 18 months! Also various pay tests took "compat" then didn't use it, so clean them up. Signed-off-by: Rusty Russell <[email protected]> Changelog-Removed: JSON-RPC: `legacypay` (`pay` replaced it in 0.9.0).

@shesek

Fixes: ElementsProject#4665 Reported-by: @shesek Signed-off-by: Rusty Russell <[email protected]>

This is what LND does, and it's better for upper layers than trying to twist our maxfeepercent / exemptfee heuristics to suit. (I don't remember who complained about this, sorry!) I'm doing this now because I want to add YA parameter next! Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: JSON-RPC: `pay` has new parameter `maxfee` for setting absolute fee (instead of using `maxfeepercent` and/or `exemptfee`)

Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: JSON-RPC: `pay` has `description` parameter, will be required if bolt11 only has a hash. Changelog-Deprecated: JSON-RPC: `pay` for a bolt11 which uses a `description_hash`, without setting `description`.

…ash. Signed-off-by: Rusty Russell <[email protected]>

cdecker

The requirement for description to be provided as a raw JSON value is too much of a restriction, and copying a potentially large description around to every single sendpay is a horrible waste of bytes (as well as copying the invstring, but that somehow sneaked in while I wasn't looking).

cdecker · 2022-04-02T12:23:20Z

plugins/libplugin-pay.c

+			if (p->description)
+				c->description = tal_strdup(c, p->description);


This is really bad, as it replicates the description in all child-payments, bloating the database with duplicate values. I thought we did the sane thing, and didn't copy the invstring either, as that is constant in the entire tree, but apparently it got changed somewhere along the line.

There is no point in duplicating these values, and we can use the convention of attaching them only to the root payment.

Hmm, fixing this is a significant change. Let me try... (and add a test!). I copied the invstring, assuming that was doing The Right Thing.

Actually, this was all a mess! This duplicate is only for the pre-splitter, but it turns out we were often returning listpays() without any bolt11.

I've fixed that.

cdecker · 2022-04-02T12:25:56Z

doc/lightning-pay.7.md

+*description* is only required for bolt11 invoices which do not
+contain a description themselves, but contain a description hash.
+*description* is then checked against the hash inside the invoice
+before it will be paid.


This requirement is incredibly restrictive: it limits us to invoices with description_hash where the description itself is representable as a raw JSON value. Anything like a PDF, which is something that people have been asking for, is essentially impossible to do with this, unless we allows a hex-encoded value to be provided instead, which makes this cumbersome to use. I know I voted in favor of requiring the description but this version is too limiting imho

Yes, the limits of json!

But we can add descriptionhex later for this case?

I looked at this, we need a non-trivial amount of re-engineering to add this. We cannot handle bolt11 descriptions which are not basically valid strings right now, and changing that would be an API change everywhere we print description.

So I'm leaving that for next release.

We were setting it on the root, but that doesn't get handed to sendpay. Our schema doesn't *require* bolt11, either, so this was missed (there could be a *bolt12* instead). Signed-off-by: Rusty Russell <[email protected]> Changelog-Fixed: JSON-RPC: `listpays` always includes `bolt11` or `bolt12` field.

Signed-off-by: Rusty Russell <[email protected]>

rustyrussell · 2022-04-04T03:03:40Z

(Fixed keysend with the new invstring deduplication: it can still be NULL in this case!

vincenzopalazzo

LGTM, just some minor doc format, where I'm not sure that the suggested syntax is required.

ACK 9d71db2

vincenzopalazzo · 2022-04-04T08:46:10Z

doc/lightning-pay.7.md

@@ -5,8 +5,8 @@ SYNOPSIS
 --------

 **pay** *bolt11* [*msatoshi*] [*label*] [*riskfactor*]
-[*maxfeepercent*] [*retry\_for*] [*maxdelay*] [*exemptfee*]
-[*exclude*]
+[*maxfeepercent*] [*retry_for*] [*maxdelay*] [*exemptfee*]


Suggested change

[*maxfeepercent*] [*retry_for*] [*maxdelay*] [*exemptfee*]

[*maxfeepercent*] [*retry\_for*] [*maxdelay*] [*exemptfee*]

No, we've been eliminating those _

oh ok is the opposite!

vincenzopalazzo · 2022-04-04T08:48:03Z

doc/lightning-sendpay.7.md

+[*bolt11*] [*payment_secret*] [*partid*] [*localofferid*] [*groupid*]
+[*payment_metadata*] [*description*]


Suggested change

[*bolt11*] [*payment_secret*] [*partid*] [*localofferid*] [*groupid*]

[*payment_metadata*] [*description*]

[*bolt11*] [*payment\_secret*] [*partid*] [*localofferid*] [*groupid*]

[*payment\_metadata*] [*description*]

this is not a new change, at this point, I'm not really sure that \ is required!

rustyrussell added this to the v0.11 milestone Mar 24, 2022

rustyrussell requested review from ZmnSCPxj and cdecker as code owners March 24, 2022 02:14

rustyrussell force-pushed the pay-enhancements branch 2 times, most recently from b0cf6b0 to a4c1d38 Compare March 30, 2022 03:59

rustyrussell added 6 commits April 2, 2022 12:47

pytest: fix invalid invoice() call.

9fd6f9e

Trips on our RPC checking introduced at the same time: msat should be an integer or an "xxxmsat"/sat/btc string. Signed-off-by: Rusty Russell <[email protected]>

doc: document pay localofferid field.

335b007

Fixes: ElementsProject#4665 Reported-by: @shesek Signed-off-by: Rusty Russell <[email protected]>

pay/sendpay: also store description in case bolt11 uses description_h…

2a4a412

…ash. Signed-off-by: Rusty Russell <[email protected]>

rustyrussell force-pushed the pay-enhancements branch from a4c1d38 to 2a4a412 Compare April 2, 2022 02:35

cdecker requested changes Apr 2, 2022

View reviewed changes

rustyrussell requested a review from cdecker April 3, 2022 06:22

rustyrussell force-pushed the pay-enhancements branch from a2d89a4 to 586407b Compare April 3, 2022 06:25

rustyrussell added 2 commits April 4, 2022 12:26

pytest: test that deduplication for bolt11 works as expected.

9d71db2

Signed-off-by: Rusty Russell <[email protected]>

rustyrussell force-pushed the pay-enhancements branch from 586407b to 9d71db2 Compare April 4, 2022 03:02

vincenzopalazzo reviewed Apr 4, 2022

View reviewed changes

rustyrussell merged commit c1e7c14 into ElementsProject:master Apr 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pay enhancements: remove legacy, add maxfee and description params #5122

Pay enhancements: remove legacy, add maxfee and description params #5122

rustyrussell commented Mar 24, 2022 •

edited

Loading

cdecker left a comment

cdecker Apr 2, 2022

rustyrussell Apr 2, 2022

rustyrussell Apr 3, 2022

cdecker Apr 2, 2022

rustyrussell Apr 2, 2022

rustyrussell Apr 3, 2022 •

edited

Loading

rustyrussell commented Apr 4, 2022

vincenzopalazzo left a comment

vincenzopalazzo Apr 4, 2022

rustyrussell Apr 4, 2022

vincenzopalazzo Apr 4, 2022

vincenzopalazzo Apr 4, 2022

		if (p->description)
		c->description = tal_strdup(c, p->description);

	[maxfeepercent] [retry_for] [maxdelay] [exemptfee]
	[maxfeepercent] [retry\_for] [maxdelay] [exemptfee]

		[bolt11] [payment_secret] [partid] [localofferid] [groupid]
		[payment_metadata] [description]

Pay enhancements: remove legacy, add maxfee and description params #5122

Pay enhancements: remove legacy, add maxfee and description params #5122

Conversation

rustyrussell commented Mar 24, 2022 • edited Loading

cdecker 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

rustyrussell Apr 3, 2022 • edited Loading

Choose a reason for hiding this comment

rustyrussell commented Apr 4, 2022

vincenzopalazzo 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

rustyrussell commented Mar 24, 2022 •

edited

Loading

rustyrussell Apr 3, 2022 •

edited

Loading