Annotate payments initiated using sendonion with the amount at the destination #3881

cdecker · 2020-07-27T15:37:08Z

Since we use sendonion in the new payment flow we end up not knowing the amount received by the destination, only the amount we sent (which includes fees).

This PR adds an optional annotation that tells us what the intended amount to be sent is.

Notice that it is still possible to call sendonion without the annotation and readers calling listpays or listsendpays should fall back to the amount being sent if the amount at the destination is unknown. The amount_sent is always guaranteed to be known and set.

Closes shesek/spark-wallet#146
Changelog-None

vincenzopalazzo · 2020-07-27T15:44:34Z

I agree with changs inside this PR, the normal user (like me) can feel a little confused when see the amount propriety null.

ZmnSCPxj

Some style nits.

wallet/wallet.c

lightningd/pay.c

darosior

Looks good to me, though i did not follow the latest change to our pay logic closely enough to ack :-)

cdecker · 2020-07-27T19:57:05Z

Looks good to me, though i did not follow the latest change to our pay logic closely enough to ack :-)

I should probably add a bit more context: since we switched from the pay plugin using sendpay to using sendonion the main daemon no longer has a way to infer the amount received by the recipient purely from the route (which in sendpay is passed in as cleartext). This fills that gap, allowing us to return the amount received by the recipient from this (partial-)payment, if the payment was successful.

One of the things we broke downstream was spark-wallet (shesek/spark-wallet#146) which uses listsendpay to list recent payment (it should switch to listpays as well see shesek/spark-wallet#147), and it was using that field to display the amount in the details and list for the payment.

vincenzopalazzo · 2020-07-27T20:19:39Z

I can share a my generic think, I'm working to test it but I think is a good idea share it in this PR.

@cdecker mentioned the command listpays, I noted that the command return a bolt11 null when is used the keysend command. Do you think is a problem? or is it only a fault in my mind?

My complete idea shared here

rustyrussell · 2020-07-27T21:17:13Z

plugins/pay.c

+
+	if (pm->amount != NULL)
+		json_add_string(ret, "amount_msat",
+				fmt_amount_msat(tmpctx, &pm->amount_sent));


Hmm, this will print 0 for failed payments (since it's allocated and never updated).

Probably need to gate this on "status" != "failed"?

doc/lightning-sendonion.7.md

rustyrussell · 2020-07-27T21:20:24Z

We also need to stage the removal of the null field, since it's an API break... I can do this too.

rustyrussell · 2020-07-27T21:21:21Z

lightningd/pay.c

@@ -97,9 +97,6 @@ void json_add_payment_fields(struct json_stream *response,
 	if (amount_msat_greater(t->msatoshi, AMOUNT_MSAT(0)))
 		json_add_amount_msat_compat(response, t->msatoshi, "msatoshi",
 					    "amount_msat");
-	else
-		json_add_null(response, "amount_msat");
-


Needs a compat gate, and 6 month deprecation cycle.

vincenzopalazzo · 2020-07-27T21:25:03Z

@rustyrussell if you mean the bolt11 null, I wrote a little code and I'm waiting to make a test with my testnet network. vincenzopalazzo@b59f72b

rustyrussell · 2020-07-27T21:27:01Z

@rustyrussell if you mean the bolt11 null, I wrote a little code and I'm waiting to make a test with my testnet network. vincenzopalazzo@b59f72b

That's different (though a good idea for next release): I was referring to the old "amount_msat": null which this series removed. That's an API break, and we require a 6 month notification for those by policy. I am worried it will (further!) break Spark...

Thanks!

rustyrussell · 2020-07-28T00:26:03Z

OK, I dropped the commit which removed the ": null" ( json-rpc: Do not return a null amount_msat to listpays ) because it really should be done via a deprecation cycle, which means we should really pass "allow-deprecated-apis" to getmanifest, and that's too big a change for an -rc. I'll queue as a separate PR...

rustyrussell · 2020-07-28T00:26:56Z

Also, I prefer not to push to branches on this repo, because Travis tests them twice...

@rustyrussell

Since we started using `sendonion` in the `pay` plugin we no longer automatically have the `amount` annotation on (partial) payments. This replicates the issue so we can fix it. Reported-by: Rusty Russell <@rustyrussell>

These were spamming my logs and could result in misleading results being returned on `listpays` and `listsendpays`.

@rustyrussell

While not directly necessary, it still feeds the `listpays` result, and so we should pass it along if we can, so we don't have to rely solely on the `amount_sent` field, which includes the fees. Reported-by: Rusty Russell <@rustyrussell>

@rustyrussell

We sum up the amounts just like we do with `amount_sent`, however we may have parts that don't have that annotation, so make it optional. Suggested-by: Rusty Russell <@rustyrussell>

ZmnSCPxj

ACK 315f1d3

d4amenace · 2020-07-28T23:26:26Z

Hello all...

So to my understanding, sendpay is being used instead of pay call for the forseeable future? or temporarily? (This has also broken my front end for my app in development)

ZmnSCPxj · 2020-07-29T01:54:48Z

No, pay should still exist. If it does not exist, it probably means the pay plugin crashed, Try checking for plugin-pay in your lightningd logs. It should mention that the plugin-pay started up, and if it did not, something is incomplete in your install. Alternately, it might mention plugin-pay started up, but for some reason crashed, and we would want to know the reason for the crash.

d4amenace · 2020-07-29T06:31:08Z

No, pay should still exist. If it does not exist, it probably means the pay plugin crashed, Try checking for plugin-pay in your lightningd logs. It should mention that the plugin-pay started up, and if it did not, something is incomplete in your install. Alternately, it might mention plugin-pay started up, but for some reason crashed, and we would want to know the reason for the crash.

resolved issue with pay plugin but......

d4amenace · 2020-07-29T06:32:36Z

Ok, so new node. Neither listsendpays nor listpays give complete payment info...now missing bolt11 invoice on listpays....and on listsendpays also no bolt 11 invoice??

minipc@minipc-desktop:$ lightning-cli listsendpays
{
"payments": [
{
"id": 1,
"payment_hash": "3c43486e64284cd5d17e8a38bee76b42310bf195adf93d56cf88658929516a0e",
"amount_msat": null,
"msatoshi_sent": 10000000,
"amount_sent_msat": "10000000msat",
"created_at": 1596002463,
"status": "complete",
"payment_preimage": "7fe29f11eff1960ddfcc58268ad5dc6dedc057d909796f875563c1e6cc3619a1"
}
]
}
minipc@minipc-desktop:$ lightning-cli listpays
{
"pays": [
{
"bolt11": "(null)",
"status": "complete",
"preimage": "7fe29f11eff1960ddfcc58268ad5dc6dedc057d909796f875563c1e6cc3619a1",
"amount_sent_msat": "10000000msat"
}
]
}

Either listsendpays or listpays MUST include bolt11,destination and payment hash. App is using listsendpays to list all payments sent...cannot identify without any identifiers.

Need to call listsendpays or listpays with bolt 11 parameter....neither tracks so how can lookup particular invoice? (used by app to confirm payments after executing pay call)

ZmnSCPxj · 2020-07-29T07:14:21Z

This should have been fixed by #3878 which was merged into master two days ago. What version are you using?

d4amenace · 2020-07-29T07:16:54Z

This should have been fixed by #3878 which was merged into master two days ago. What version are you using?

0.9.0 rc3

ZmnSCPxj · 2020-07-29T07:26:46Z

rc3 is known to be buggy. It seems the bug you are reporting is already fixed in master. Can you try uninstalling and reinstalling again with master?

cdecker requested review from rustyrussell and darosior July 27, 2020 15:37

cdecker mentioned this pull request Jul 27, 2020

Amount KeySend received correct, amount KeySend sent isn't shesek/spark-wallet#146

Closed

cdecker added this to the v0.9.0 milestone Jul 27, 2020

ZmnSCPxj reviewed Jul 27, 2020

View reviewed changes

wallet/wallet.c Outdated Show resolved Hide resolved

lightningd/pay.c Show resolved Hide resolved

darosior reviewed Jul 27, 2020

View reviewed changes

cdecker force-pushed the sendonion-msatoshi branch from 159d4bd to e36840e Compare July 27, 2020 19:53

rustyrussell requested changes Jul 27, 2020

View reviewed changes

rustyrussell reviewed Jul 27, 2020

View reviewed changes

rustyrussell force-pushed the sendonion-msatoshi branch from e36840e to 3a846c1 Compare July 28, 2020 00:24

rustyrussell approved these changes Jul 28, 2020

View reviewed changes

cdecker added 5 commits July 28, 2020 10:36

pytest: Replicate the missing amount_msat in listpays

afc3a7c

Since we started using `sendonion` in the `pay` plugin we no longer automatically have the `amount` annotation on (partial) payments. This replicates the issue so we can fix it. Reported-by: Rusty Russell <@rustyrussell>

db: Guard against accessing NULL partid and total_msat

2ea5cbe

These were spamming my logs and could result in misleading results being returned on `listpays` and `listsendpays`.

jsonrpc: Add amount received by recipient to listpays result

e007feb

We sum up the amounts just like we do with `amount_sent`, however we may have parts that don't have that annotation, so make it optional. Suggested-by: Rusty Russell <@rustyrussell>

doc: Document the msatoshi parameter for sendonion

315f1d3

rustyrussell force-pushed the sendonion-msatoshi branch from 3a846c1 to 315f1d3 Compare July 28, 2020 01:06

ZmnSCPxj approved these changes Jul 28, 2020

View reviewed changes

rustyrussell merged commit 14d08b7 into master Jul 28, 2020

vincenzopalazzo mentioned this pull request Jul 28, 2020

listsendpays sending back POST which is missing info and some null response #3885

Closed

rustyrussell deleted the sendonion-msatoshi branch August 15, 2022 00:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotate payments initiated using sendonion with the amount at the destination #3881

Annotate payments initiated using sendonion with the amount at the destination #3881

cdecker commented Jul 27, 2020 •

edited by rustyrussell

Loading

vincenzopalazzo commented Jul 27, 2020

ZmnSCPxj left a comment

darosior left a comment

cdecker commented Jul 27, 2020

vincenzopalazzo commented Jul 27, 2020 •

edited

Loading

rustyrussell Jul 27, 2020

rustyrussell commented Jul 27, 2020

rustyrussell Jul 27, 2020

vincenzopalazzo commented Jul 27, 2020

rustyrussell commented Jul 27, 2020

rustyrussell commented Jul 28, 2020

rustyrussell commented Jul 28, 2020

ZmnSCPxj left a comment

d4amenace commented Jul 28, 2020

ZmnSCPxj commented Jul 29, 2020

d4amenace commented Jul 29, 2020

d4amenace commented Jul 29, 2020 •

edited

Loading

ZmnSCPxj commented Jul 29, 2020

d4amenace commented Jul 29, 2020

ZmnSCPxj commented Jul 29, 2020

Annotate payments initiated using sendonion with the amount at the destination #3881

Annotate payments initiated using sendonion with the amount at the destination #3881

Conversation

cdecker commented Jul 27, 2020 • edited by rustyrussell Loading

vincenzopalazzo commented Jul 27, 2020

ZmnSCPxj left a comment

Choose a reason for hiding this comment

darosior left a comment

Choose a reason for hiding this comment

cdecker commented Jul 27, 2020

vincenzopalazzo commented Jul 27, 2020 • edited Loading

rustyrussell Jul 27, 2020

Choose a reason for hiding this comment

rustyrussell commented Jul 27, 2020

rustyrussell Jul 27, 2020

Choose a reason for hiding this comment

vincenzopalazzo commented Jul 27, 2020

rustyrussell commented Jul 27, 2020

rustyrussell commented Jul 28, 2020

rustyrussell commented Jul 28, 2020

ZmnSCPxj left a comment

Choose a reason for hiding this comment

d4amenace commented Jul 28, 2020

ZmnSCPxj commented Jul 29, 2020

d4amenace commented Jul 29, 2020

d4amenace commented Jul 29, 2020 • edited Loading

ZmnSCPxj commented Jul 29, 2020

d4amenace commented Jul 29, 2020

ZmnSCPxj commented Jul 29, 2020

cdecker commented Jul 27, 2020 •

edited by rustyrussell

Loading

vincenzopalazzo commented Jul 27, 2020 •

edited

Loading

d4amenace commented Jul 29, 2020 •

edited

Loading