From b92b9f074dc2fa26df3bde4a8f9f5896dd6f059a Mon Sep 17 00:00:00 2001 From: Vincenzo Palazzo Date: Wed, 22 Mar 2023 20:22:25 +0100 Subject: [PATCH] delpay: delete the payment by status from the db MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are cases (difficult to reproduce with a test) where a payment will fail one time and succeed later. As far I understand in this case the groupid field of the payment is the same, and the only thing that change is the status, so our logic inside the delpay is ambiguous where it is not possible to delete a payment as described in https://github.com/ElementsProject/lightning/issues/6114 A sequence of commands that explain the problem is ``` $ lc -k listpays payment_hash=H { "pays": [ { "bolt11": "I", "destination": "redacted", "payment_hash": "H", "status": "complete", "created_at": redacted, "completed_at": redacted, "preimage": "P", "amount_msat": "redacted", "amount_sent_msat": "redacted" } ] } $ lc delpay H complete { "code": 211, "message": "Payment with hash H has failed status but it should be complete" } ``` In this case, the delpay is not able to delete a payment because the listpays is returning only the succeeded one, so by running the listsendpays we may see the following result where our delpay logic will be stuck because it works to ensure that all the payments stored in the database has the status specified by the user ``` ➜ VincentSSD clightning --testnet listsendpays -k payment_hash=7fc74bedbb78f2f3330155d919a54e730cf19c11bc73e96c027f5cd4a34e53f4 { "payments": [ { "id": 322, "payment_hash": "7fc74bedbb78f2f3330155d919a54e730cf19c11bc73e96c027f5cd4a34e53f4", "groupid": 1, "partid": 1, "destination": "030b686a163aa2bba03cebb8bab7778fac251536498141df0a436d688352d426f6", "amount_msat": 300, "amount_sent_msat": 1664, "created_at": 1679510203, "completed_at": 1679510205, "status": "failed", "bolt11": "lntb1pjpkj4xsp52trda39rfpe7qtqahx8jjplhnj3tatxy8rh6sc6afgvmdz7n0llspp50lr5hmdm0re0xvcp2hv3nf2wwvx0r8q3h3e7jmqz0awdfg6w206qdp0w3jhxarfdenjqargv5sxgetvwpshjgrzw4njqun9wphhyaqxqyjw5qcqp2rzjqtp28uqy77te96ylt7ek703h4ayldljsf8rnlztgf3p8mg7pd0qzwf8a3yqqpdqqqyqqqqt2qqqqqqgqqc9qxpqysgqgeya2lguaj6sflc4hx2d89jvah8mw9uax4j77d8rzkut3rkm0554x37fc7gy92ws9l76yprdva2lalrs7fqjp9lcx40zuty8gca0g5spme3dup" }, { "id": 323, "payment_hash": "7fc74bedbb78f2f3330155d919a54e730cf19c11bc73e96c027f5cd4a34e53f4", "groupid": 1, "partid": 2, "destination": "030b686a163aa2bba03cebb8bab7778fac251536498141df0a436d688352d426f6", "amount_msat": 300, "amount_sent_msat": 3663, "created_at": 1679510205, "completed_at": 1679510207, "status": "failed" }, { "id": 324, "payment_hash": "7fc74bedbb78f2f3330155d919a54e730cf19c11bc73e96c027f5cd4a34e53f4", "groupid": 1, "partid": 3, "destination": "030b686a163aa2bba03cebb8bab7778fac251536498141df0a436d688352d426f6", "amount_msat": 300, "amount_sent_msat": 3663, "created_at": 1679510207, "completed_at": 1679510209, "status": "failed" }, { "id": 325, "payment_hash": "7fc74bedbb78f2f3330155d919a54e730cf19c11bc73e96c027f5cd4a34e53f4", "groupid": 1, "partid": 4, "destination": "030b686a163aa2bba03cebb8bab7778fac251536498141df0a436d688352d426f6", "amount_msat": 300, "amount_sent_msat": 4663, "created_at": 1679510209, "completed_at": 1679510221, "status": "complete", "payment_preimage": "43f746f2d28d4902489cbde9b3b8f3d04db5db7e973f8a55b7229ce774bf33a7" } ] } ``` This commit solves the problem by forcing the delete query in the database to specify status too, and work around this kind of ambiguous case. Fixes: f52ff07558709bd1f7ed0cdca65c891d80b1a785 (lightningd: allow delpay to delete a specific payment.) Reported-by: Antoine Poinsot Link: https://github.com/ElementsProject/lightning/issues/6114 Signed-off-by: Vincenzo Palazzo Co-Developed-by: Rusty Russell Changelog-Fixed: delpay be more pedantic about delete logic by allowing delete payments by status directly on the database. --- doc/lightning-delpay.7.md | 2 +- lightningd/pay.c | 22 +++++++++++++++------- tests/test_pay.py | 23 +++++++++++++++++++++++ wallet/wallet.c | 14 ++++++++++---- wallet/wallet.h | 4 ++-- 5 files changed, 51 insertions(+), 14 deletions(-) diff --git a/doc/lightning-delpay.7.md b/doc/lightning-delpay.7.md index 021787d7b8af..cc57d4d7d85d 100644 --- a/doc/lightning-delpay.7.md +++ b/doc/lightning-delpay.7.md @@ -9,7 +9,7 @@ SYNOPSIS DESCRIPTION ----------- -The **delpay** RPC command deletes a payment with the given `payment_hash` if its status is either `complete` or `failed`. Deleting a `pending` payment is an error. If *partid* and *groupid* are not specified, all payment parts are deleted. +The **delpay** RPC command deletes a payment with the given `payment_hash` if its status is either `complete` or `failed`. Deleting a `pending` payment is an error. If *partid* and *groupid* are not specified, all payment parts with matchin status are deleted. - *payment\_hash*: The unique identifier of a payment. - *status*: Expected status of the payment. diff --git a/lightningd/pay.c b/lightningd/pay.c index 921f46ed581d..0f09532462a7 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -1654,6 +1654,7 @@ static struct command_result *json_delpay(struct command *cmd, const jsmntok_t *obj UNNEEDED, const jsmntok_t *params) { + const enum wallet_payment_status *found_status = NULL; struct json_stream *response; const struct wallet_payment **payments; enum wallet_payment_status *status; @@ -1686,21 +1687,26 @@ static struct command_result *json_delpay(struct command *cmd, if (partid && payments[i]->partid != *partid) continue; - found = true; - if (payments[i]->status != *status) { - return command_fail(cmd, PAY_STATUS_UNEXPECTED, "Payment with hash %s has %s status but it should be %s", - type_to_string(tmpctx, struct sha256, payment_hash), - payment_status_to_string(payments[i]->status), - payment_status_to_string(*status)); + if (payments[i]->status == *status) { + found = true; + break; } + + found_status = &payments[i]->status; } if (!found) { + if (found_status) + return command_fail(cmd, PAY_NO_SUCH_PAYMENT, "Payment with hash %s has %s status but it different from the one provided %s", + type_to_string(tmpctx, struct sha256, payment_hash), + payment_status_to_string(*found_status), + payment_status_to_string(*status)); + return command_fail(cmd, PAY_NO_SUCH_PAYMENT, "No payment for that payment_hash with that partid and groupid"); } - wallet_payment_delete(cmd->ld->wallet, payment_hash, groupid, partid); + wallet_payment_delete(cmd->ld->wallet, payment_hash, groupid, partid, status); response = json_stream_success(cmd); json_array_start(response, "payments"); @@ -1709,6 +1715,8 @@ static struct command_result *json_delpay(struct command *cmd, continue; if (partid && payments[i]->partid != *partid) continue; + if (payments[i]->status != *status) + continue; json_object_start(response, NULL); json_add_payment_fields(response, payments[i]); json_object_end(response); diff --git a/tests/test_pay.py b/tests/test_pay.py index 6cca937b8f78..4cab55bb1234 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -4096,6 +4096,29 @@ def test_delpay_payment_split(node_factory, bitcoind): assert len(l1.rpc.listpays()['pays']) == 0 +@pytest.mark.developer("needs dev-no-reconnect, dev-routes to force failover") +def test_delpay_mixed_status(node_factory, bitcoind): + """ + One failure, one success; we only want to delete the failed one! + """ + l1, l2, l3 = node_factory.line_graph(3, fundamount=10**5, + wait_for_announce=True) + # Expensive route! + l4 = node_factory.get_node(options={'fee-per-satoshi': 1000, + 'fee-base': 2000}) + node_factory.join_nodes([l1, l4, l3], wait_for_announce=True) + + # Don't give a hint, so l1 chooses cheapest. + inv = l3.dev_invoice(10**5, 'lbl', 'desc', dev_routes=[]) + l3.rpc.disconnect(l2.info['id'], force=True) + l1.rpc.pay(inv['bolt11']) + + assert len(l1.rpc.listsendpays()['payments']) == 2 + delpay_result = l1.rpc.delpay(inv['payment_hash'], 'failed')['payments'] + assert len(delpay_result) == 1 + assert len(l1.rpc.listsendpays()['payments']) == 1 + + def test_listpay_result_with_paymod(node_factory, bitcoind): """ The object of this test is to verify the correct behavior diff --git a/wallet/wallet.c b/wallet/wallet.c index 05bf340b0725..65b02ad581cc 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -3294,24 +3294,30 @@ u64 wallet_payment_get_groupid(struct wallet *wallet, void wallet_payment_delete(struct wallet *wallet, const struct sha256 *payment_hash, - const u64 *groupid, - const u64 *partid) + const u64 *groupid, const u64 *partid, + const enum wallet_payment_status *status) { struct db_stmt *stmt; + + assert(status); if (groupid) { assert(partid); stmt = db_prepare_v2(wallet->db, SQL("DELETE FROM payments" " WHERE payment_hash = ?" " AND groupid = ?" - " AND partid = ?")); + " AND partid = ?" + " AND status = ?")); db_bind_u64(stmt, 1, *groupid); db_bind_u64(stmt, 2, *partid); + db_bind_u64(stmt, 3, *status); } else { assert(!partid); stmt = db_prepare_v2(wallet->db, SQL("DELETE FROM payments" - " WHERE payment_hash = ?")); + " WHERE payment_hash = ?" + " AND status = ?")); + db_bind_u64(stmt, 1, *status); } db_bind_sha256(stmt, 0, payment_hash); db_exec_prepared_v2(take(stmt)); diff --git a/wallet/wallet.h b/wallet/wallet.h index ec7bfc6a26fa..b6c9bb4081ec 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -1106,8 +1106,8 @@ void wallet_payment_store(struct wallet *wallet, */ void wallet_payment_delete(struct wallet *wallet, const struct sha256 *payment_hash, - const u64 *groupid, - const u64 *partid); + const u64 *groupid, const u64 *partid, + const enum wallet_payment_status *status); /** * wallet_local_htlc_out_delete - Remove a local outgoing failed HTLC