From c48f32e4e1bb59acf9b41e4aad707312655f4e49 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 Changelog-Fixed: delpay be more pedantic about delete logic by allowing delete payments by status directly on the database. --- lightningd/pay.c | 14 +++++++------- wallet/wallet.c | 18 +++++++++++------- wallet/wallet.h | 5 ++--- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index b6e1d575ecff..159b869f9815 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -1654,13 +1654,13 @@ 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; struct sha256 *payment_hash; u64 *groupid, *partid; bool found; - const enum wallet_payment_status *found_status = NULL; if (!param(cmd, buffer, params, p_req("payment_hash", param_sha256, &payment_hash), @@ -1676,6 +1676,7 @@ static struct command_result *json_delpay(struct command *cmd, payments = wallet_payment_list(cmd, cmd->ld->wallet, payment_hash); + log_debug(cmd->ld->log, "Status decoded %s", payment_status_to_string(*status)); if (tal_count(payments) == 0) return command_fail(cmd, PAY_NO_SUCH_PAYMENT, "Unknown payment with payment_hash: %s", type_to_string(tmpctx, struct sha256, payment_hash)); @@ -1687,11 +1688,10 @@ static struct command_result *json_delpay(struct command *cmd, if (partid && payments[i]->partid != *partid) continue; - if (payments[i]->status != *status) { - found_status = &payments[i]->status; - continue; - } - found = true; + if (payments[i]->status == *status) + found = true; + + found_status = &payments[i]->status; } if (!found) { @@ -1707,7 +1707,7 @@ static struct command_result *json_delpay(struct command *cmd, "No payment for that payment_hash with that partid and groupid"); } - wallet_payment_delete(cmd->ld->wallet, payment_hash, *status, groupid, partid); + wallet_payment_delete(cmd->ld->wallet, payment_hash, groupid, partid, status); response = json_stream_success(cmd); json_array_start(response, "payments"); diff --git a/wallet/wallet.c b/wallet/wallet.c index 9321931dde25..598b94db06ec 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -3200,11 +3200,12 @@ u64 wallet_payment_get_groupid(struct wallet *wallet, void wallet_payment_delete(struct wallet *wallet, const struct sha256 *payment_hash, - enum wallet_payment_status status, - 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, @@ -3212,15 +3213,18 @@ void wallet_payment_delete(struct wallet *wallet, " WHERE payment_hash = ?" " AND status = ?" " AND groupid = ?" - " AND partid = ?")); - db_bind_u64(stmt, 2, *groupid); - db_bind_u64(stmt, 3, *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 = ?" - " AND status = ?")); + " AND status = ?")); + db_bind_u64(stmt, 1, *status); } db_bind_sha256(stmt, 0, payment_hash); db_bind_int(stmt, 1, wallet_payment_status_in_db(status)); diff --git a/wallet/wallet.h b/wallet/wallet.h index 8e168dabfe72..c78d45989ea7 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -1098,9 +1098,8 @@ void wallet_payment_store(struct wallet *wallet, */ void wallet_payment_delete(struct wallet *wallet, const struct sha256 *payment_hash, - enum wallet_payment_status status, - 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