Skip to content

Commit

Permalink
delpay: delete the payment by status from the db
Browse files Browse the repository at this point in the history 
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 #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: f52ff07 (lightningd: allow delpay to delete a specific payment.)
Reported-by: Antoine Poinsot <darosior@protonmail.com>
Link: #6114
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Changelog-Fixed: delpay be more pedantic about delete logic by allowing
delete payments by status directly on the database.
  • Loading branch information
@vincenzopalazzo
vincenzopalazzo committed Mar 23, 2023
1 parent 88a4789 commit c48f32e
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 17 deletions.
14 changes: 7 additions & 7 deletions lightningd/pay.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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));
Expand All @@ -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) {
Expand All @@ -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");
Expand Down
18 changes: 11 additions & 7 deletions wallet/wallet.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3200,27 +3200,31 @@ 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,
SQL("DELETE FROM payments"
" 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));
Expand Down
5 changes: 2 additions & 3 deletions wallet/wallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit c48f32e

Please sign in to comment.