delpay: introduced a new rpc method to delete a payment by hash #3899

vincenzopalazzo · 2020-08-03T14:51:51Z

Hi all.

This Draft implement a new RPC command to delete the payment with the status complete failed marked like good first issue #1995

However, this PR depends from the PR where I proposed to update the result of listpays command with payment_hash inside the result #3888. In addition, with the @cdecker suggestion to add the payment_hash in all cases of types of payments, the command delpay can use only the payment_hash to identify the payment to remove and not bolt11.

This PR is a draft yet because I have some point to clarify with the team, for instance, the type of error that the command should be returned in cases of error. In other words, I see that the LIGHTNINGD error is marked with a FIXME because each command should use the correct error code. I'm not able to find the payment error codes inside the code. maybe we could define the payment error code?

Docs update with the change proposed in this Draft
Fix error code inside the command implementation

Changelog-Added: JSON-RPC: delpay a new method to delete the payment completed or failed.

ZmnSCPxj

I see that the LIGHTNINGD error is marked with a FIXME because each command should use the correct error code. I'm not able to find the payment error codes inside the code. maybe we could define the payment error code?

PAY_NO_SUCH_PAYMENT seems appropriate if the specified hash does not match any payments. Define a new PAY_STATUS_UNEXPECTED for the case where it does not match the expected status.

doc/lightning-delpay.7.md

ZmnSCPxj · 2020-08-04T02:09:40Z

In addition, it seems to me better to just delete all partids, not delete them one-by-one. Loss of one partid of a multi-part payment is already a loss of information anyway, so just delete them all in one database transaction.

vincenzopalazzo · 2020-08-04T10:16:20Z

Hi @ZmnSCPxj, thanks to review this PR.

I will make the changes inside the docs and inside the JSON RPC errors, sorry about that.

In addition, about your suggestion.

In addition, it seems to me better to just delete all partid, not delete them one-by-one. Loss of one partid of a multi-part payment is already a loss of information anyway, so just delete them all in one database transaction.

For my code comprehension, to make this work, so, to delete the complete payment and not each payment by partid, I need a new function inside the wallet.c to run the query "DELETE FROM payments WHERE payment_hash=?"

I'm losing something?

Maybe I can use the function wallet_payment_list and run for each element inside the list wallet_payment_delete but I think that the first solution is cleaner but I don't know if it is safe to run on the lightning node the delete query by hash?

ZmnSCPxj · 2020-08-04T14:36:14Z

DELETE FROM TABLE payments WHERE payment_hash=? should be fine. You might want to do some check to check if this results in any changes (I think we have a function to check the number of changes a query makes) and if it is 0 that is probably a "payment not found" error.

But you want to check the state of the entire payment first. The logic for a purported "payment state":

If any part has succeeded, then the entire payment succeeded.
If any part is pending, then the entire payment is pending.
Otherwise, all parts are failed, so the entire payment is failed.

It is safe to delete a payment if the entire payment is failed or succeeded, and we should probably disallow deleting pending payments (which might still be operated by pay over in the pay plugin).

Note that if you do not pass control to a function with a callback, then the entire processing is done inside a database transaction, so you are assured that between the check and the deletion, the database state is consistent. But if you pass control to a function with a callback, the database transaction will end at that point, and by the time the callback is called, you are in a new transaction. So do not call into any callback-using functions between checking the state of the payment and deleting the whole payment.

vincenzopalazzo · 2020-08-04T20:51:38Z

Thanks for the suggestion @ZmnSCPxj.

You might want to do some check to check if this results in any changes (I think we have a function to check the number of changes a query makes) and if it is 0 that is probably a "payment not found" error.

I make the sanity test of input before to call the database function, but I can improve the check maybe I can make the following test:

payment not found: I used the wallet_payment_by_hash to find the payment, I think that is possible use this function to get the payment with hash and partid=0, if exist this part, the payment exists, right?
payment status not valid: from your previous message I understand the payment status is decided from the status of all part, so, I should check the status of all payment find with wallet_payment_list and check the status for each payment and make the correct decision.

of course, there are all checks on the input but, they are not relevant into this description.

It is safe to delete a payment if the entire payment is failed or succeeded, and we should probably disallow deleting pending payments (which might still be operated by pay over in the pay plugin).

Yep, I agree, with my actual logic inside the delpay command you can remove only the payment with the status complete or failed, but of course, with your suggestion the logic change a little.

ZmnSCPxj · 2020-08-05T03:38:00Z

partid 0 is for non-split payments. If we split payments at all, there might not be a partid 0.

vincenzopalazzo · 2020-08-07T17:09:34Z

Rebased 01a82d3 and updated vincenzopalazzo@8fef912

ZmnSCPxj

Mostly style nits.

doc/lightning-delpay.7.md

lightningd/pay.c

wallet/wallet.c

lightningd/pay.c

vincenzopalazzo

@ZmnSCPxj In addition, I need a feedback on this change 😄 sorry about that

lightningd/pay.c

tests/test_pay.py

doc/lightning-delpay.7.md

niftynei · 2020-08-18T20:21:53Z

Mostly grammatical/spelling fixes. In general, for documentation, less words is better :)

vincenzopalazzo · 2020-08-19T10:42:07Z

Rebase and update with suggestions.

Thanks @niftynei about the fixes and sorry for the spelling errors. I'm still searching somethings where I'm good to do but I'm sure that I'm very bad to write sentences 😄

doc/lightning-delpay.7.md

cdecker · 2020-08-24T14:02:07Z

doc/lightning-delpay.7.md

+RETURN VALUE
+------------
+
+On success, the command will return a payment object, such as the **listsendpays**. If the payment is aa MPP (multi part payment) the command return a list of 


Suggested change

On success, the command will return a payment object, such as the **listsendpays**. If the payment is aa MPP (multi part payment) the command return a list of

If successful the command returns a payment object, in the same format as **listsendpays**.

If the payment is a multi-part payment (MPP) the command return a list of

cdecker · 2020-08-24T14:04:37Z

lightningd/pay.c

+	if (status == PAYMENT_PENDING)
+		return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Invalid status: %s",
+							payment_status_to_string(status));


Probably it's better to check if we have one of the valid statuses instead of not having the third. If we ever add a new status this is would not cover that fourth state. If you use a switch statement then the compiler will make sure we cover all cases exhaustively.

Thanks for this suggestion, I made the change from if to switch!

cdecker · 2020-08-24T14:08:11Z

tests/test_pay.py

+    # payment unpaid with wrong status (pending status is a illegal input)
+    with pytest.raises(RpcError):
+        l2.rpc.delpay(payment_hash, 'pending')


Payment was no attempted at all, so it'd fail for that reason already.

Basically this is testing our sanity check order, and not the underlying pending vs. non-pending cases.

This comment brings me to reflect on the sens of this test!

I moved the pay command to the top of this sanity check, so, with this new situation the unit test has more sense to me because it does the sanity check and the command behavior checks

tests/test_pay.py

cdecker · 2020-08-24T14:12:51Z

tests/test_pay.py

+
+    l1, l2, l3 = node_factory.line_graph(
+        3, fundamount=10**8, wait_for_announce=True,
+        opts={'wumbo': None}


Wumbo is not needed here: MPP_TARGET_SIZE is denominated in msats, whereas the fundamount is denominated in sats. So this creates a channel that is 4 orders of magnitude larger than the amount being transferred:

Channel capacity:10**8 * 1000 = 10**11

Payment amount: 5 * 10**7

Doesn't hurt though 😉

Doesn't hurt though

Yes, it is, but it is also my logic error, I discovered only now that fundamount is denominated in sats. I think that 10**5 is enough

vincenzopalazzo · 2020-08-25T09:57:19Z

Rebase and update with review 3314a05

~~Waiting to Travis test result~~

cdecker · 2020-09-09T12:48:20Z

Rebased on top of master

cdecker

ACK 56a5dc9

Addressed

Changelog-Added: JSON-RPC: delpay a new method to delete the payment completed or failed. Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

rustyrussell · 2020-09-10T06:04:35Z

Ack db53235

ZmnSCPxj requested changes Aug 4, 2020

View reviewed changes

doc/lightning-delpay.7.md Outdated Show resolved Hide resolved

doc/lightning-delpay.7.md Show resolved Hide resolved

vincenzopalazzo force-pushed the delpayment branch from bfb4282 to 3a5f0be Compare August 7, 2020 17:05

vincenzopalazzo marked this pull request as ready for review August 7, 2020 17:05

vincenzopalazzo requested a review from cdecker as a code owner August 7, 2020 17:05

vincenzopalazzo requested a review from ZmnSCPxj August 7, 2020 17:06

vincenzopalazzo force-pushed the delpayment branch from 3a5f0be to 8fef912 Compare August 7, 2020 17:21

ZmnSCPxj requested changes Aug 7, 2020

View reviewed changes

ZmnSCPxj previously requested changes Aug 7, 2020

View reviewed changes

lightningd/pay.c Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the delpayment branch from 8fef912 to 627a374 Compare August 8, 2020 15:48

vincenzopalazzo commented Aug 8, 2020

View reviewed changes

lightningd/pay.c Show resolved Hide resolved

vincenzopalazzo force-pushed the delpayment branch from 627a374 to 86a0fa5 Compare August 13, 2020 18:44

niftynei reviewed Aug 18, 2020

View reviewed changes

vincenzopalazzo force-pushed the delpayment branch from 159a349 to 8d3cf55 Compare August 19, 2020 10:39

vincenzopalazzo force-pushed the delpayment branch from 8d3cf55 to 68a2b06 Compare August 23, 2020 16:32

cdecker reviewed Aug 24, 2020

View reviewed changes

vincenzopalazzo force-pushed the delpayment branch from 68a2b06 to 3314a05 Compare August 25, 2020 09:52

vincenzopalazzo force-pushed the delpayment branch 5 times, most recently from 4274fe4 to 8879acc Compare August 25, 2020 12:52

vincenzopalazzo force-pushed the delpayment branch 2 times, most recently from 5ad8db7 to ef0bd3a Compare August 31, 2020 12:44

rustyrussell added this to the v0.9.1 milestone Sep 7, 2020

vincenzopalazzo force-pushed the delpayment branch 2 times, most recently from b4fe30f to da4ff41 Compare September 8, 2020 09:49

rustyrussell requested a review from cdecker September 9, 2020 10:55

cdecker force-pushed the delpayment branch from da4ff41 to 75501d9 Compare September 9, 2020 12:48

cdecker force-pushed the delpayment branch 3 times, most recently from d792432 to a184f3b Compare September 9, 2020 13:35

cdecker requested a review from ZmnSCPxj September 9, 2020 13:49

cdecker force-pushed the delpayment branch from a184f3b to 56a5dc9 Compare September 9, 2020 14:24

cdecker approved these changes Sep 9, 2020

View reviewed changes

vincenzopalazzo added 2 commits September 10, 2020 15:30

delpay: code style changes and fixed docs

4831ff3

Changelog-Added: JSON-RPC: delpay a new method to delete the payment completed or failed. Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

Added additional fix to code formatting and English spelling.

db53235

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

rustyrussell force-pushed the delpayment branch from 56a5dc9 to db53235 Compare September 10, 2020 06:02

rustyrussell mentioned this pull request Sep 10, 2020

Release 0.9.1 rc1 #4036

Merged

rustyrussell merged commit f62d7bb into ElementsProject:master Sep 10, 2020

vincenzopalazzo mentioned this pull request Sep 27, 2020

Create the equivalent of "delinvoice" for payments #1995

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delpay: introduced a new rpc method to delete a payment by hash #3899

delpay: introduced a new rpc method to delete a payment by hash #3899

vincenzopalazzo commented Aug 3, 2020 •

edited

Loading

ZmnSCPxj left a comment

ZmnSCPxj commented Aug 4, 2020

vincenzopalazzo commented Aug 4, 2020

ZmnSCPxj commented Aug 4, 2020 •

edited

Loading

vincenzopalazzo commented Aug 4, 2020

ZmnSCPxj commented Aug 5, 2020

vincenzopalazzo commented Aug 7, 2020 •

edited

Loading

ZmnSCPxj left a comment

vincenzopalazzo left a comment •

edited

Loading

niftynei commented Aug 18, 2020 •

edited

Loading

vincenzopalazzo commented Aug 19, 2020

cdecker Aug 24, 2020

cdecker Aug 24, 2020

vincenzopalazzo Aug 25, 2020

cdecker Aug 24, 2020

cdecker Aug 24, 2020

vincenzopalazzo Aug 25, 2020

cdecker Aug 24, 2020 •

edited

Loading

vincenzopalazzo Aug 25, 2020

vincenzopalazzo commented Aug 25, 2020 •

edited

Loading

cdecker commented Sep 9, 2020

cdecker left a comment

rustyrussell commented Sep 10, 2020

	On success, the command will return a payment object, such as the listsendpays. If the payment is aa MPP (multi part payment) the command return a list of
	If successful the command returns a payment object, in the same format as listsendpays.
	If the payment is a multi-part payment (MPP) the command return a list of

delpay: introduced a new rpc method to delete a payment by hash #3899

delpay: introduced a new rpc method to delete a payment by hash #3899

Conversation

vincenzopalazzo commented Aug 3, 2020 • edited Loading

ZmnSCPxj left a comment

Choose a reason for hiding this comment

ZmnSCPxj commented Aug 4, 2020

vincenzopalazzo commented Aug 4, 2020

ZmnSCPxj commented Aug 4, 2020 • edited Loading

vincenzopalazzo commented Aug 4, 2020

ZmnSCPxj commented Aug 5, 2020

vincenzopalazzo commented Aug 7, 2020 • edited Loading

ZmnSCPxj left a comment

Choose a reason for hiding this comment

vincenzopalazzo left a comment • edited Loading

Choose a reason for hiding this comment

niftynei commented Aug 18, 2020 • edited Loading

vincenzopalazzo commented Aug 19, 2020

cdecker Aug 24, 2020

Choose a reason for hiding this comment

cdecker Aug 24, 2020

Choose a reason for hiding this comment

vincenzopalazzo Aug 25, 2020

Choose a reason for hiding this comment

cdecker Aug 24, 2020

Choose a reason for hiding this comment

cdecker Aug 24, 2020

Choose a reason for hiding this comment

vincenzopalazzo Aug 25, 2020

Choose a reason for hiding this comment

cdecker Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

vincenzopalazzo Aug 25, 2020

Choose a reason for hiding this comment

vincenzopalazzo commented Aug 25, 2020 • edited Loading

cdecker commented Sep 9, 2020

cdecker left a comment

Choose a reason for hiding this comment

rustyrussell commented Sep 10, 2020

vincenzopalazzo commented Aug 3, 2020 •

edited

Loading

ZmnSCPxj commented Aug 4, 2020 •

edited

Loading

vincenzopalazzo commented Aug 7, 2020 •

edited

Loading

vincenzopalazzo left a comment •

edited

Loading

niftynei commented Aug 18, 2020 •

edited

Loading

cdecker Aug 24, 2020 •

edited

Loading

vincenzopalazzo commented Aug 25, 2020 •

edited

Loading