Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix sendpay aggregation and ordering in listpays #4567

Merged
merged 25 commits into from
Oct 13, 2021
Merged

Fix sendpay aggregation and ordering in listpays #4567

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
db5181f
pytest: Reproduce #4482
cdecker May 28, 2021
9dc4726
db: Add groupid to the payments table
cdecker Sep 28, 2021
af371b6
jsonrpc: Add groupid to `sendpay` and `sendonion`
cdecker Sep 28, 2021
9f887f3
pyln: Add groupid to `sendpay` and `sendonion`
cdecker Sep 28, 2021
8163ae3
pay: Add `groupid` to the payment struct
cdecker Sep 29, 2021
e73564d
wallet: Add function to retrieve the latest groupid for a payment
cdecker Sep 30, 2021
779826a
db: Add `groupid` to HTLCs
cdecker Sep 29, 2021
2529daa
jsonrpc: Add missing `partid` in `listsendpays` schema
cdecker Sep 29, 2021
b83fd20
pay: Call `listsendpays` to find prior attempts and abort if needed
cdecker Sep 29, 2021
fc62b7f
doc: Add missing `amount_sent_msat` to `listpays`
cdecker Sep 29, 2021
38ec80d
jsonrpc: Add `groupid` to `waitsendpay`
cdecker Sep 29, 2021
95641e3
pay: Make `pay` idempotent
cdecker Sep 30, 2021
dd78d61
pay: Do not delete old sendpay attempts if we retry
cdecker May 31, 2021
6e1341e
paycore: Prevent multiple concurrent payment groups
cdecker Oct 4, 2021
99d5808
paycore: Default `groupid` to increment from last one
cdecker Oct 4, 2021
d555a99
pytest: Fix up `test_partial_payment` to use a single `groupid`
cdecker Oct 4, 2021
75a4f5b
pay: Fail a `sendpay` or `sendonion` that'd produce a DB collision
cdecker Oct 4, 2021
fc94852
pytest: Add `groupid` to `test_partial_payment_{timeout,restart}`
cdecker Oct 4, 2021
fea3d08
pytest: Fix `test_onchain_timeout` to use `groupid`
cdecker Oct 4, 2021
f726fc6
libplugin: Add callbacks for successful and failed payments
cdecker Oct 4, 2021
11d18c1
pytest: Adjust `test_sendpay` to the new semantics
cdecker Oct 5, 2021
b8280be
pay: Mark completed payments as such by nullifying `cmd`
cdecker Oct 5, 2021
0da9623
pay: Stash and forward results for duplicate `pay` calls
cdecker Oct 5, 2021
03847bd
pay: listpays groups by payment_hash and groupid
cdecker May 28, 2021
71752df
db: Fix a syntax error with the optional parameters
cdecker Oct 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions lightningd/pay.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ struct sendpay_command {

struct sha256 payment_hash;
u64 partid;
u64 groupid;
rustyrussell marked this conversation as resolved.
Show resolved Hide resolved
struct command *cmd;
};

Expand Down Expand Up @@ -77,12 +78,13 @@ static void
add_sendpay_waiter(struct lightningd *ld,
struct command *cmd,
const struct sha256 *payment_hash,
u64 partid)
u64 partid, u64 groupid)
{
struct sendpay_command *pc = tal(cmd, struct sendpay_command);

pc->payment_hash = *payment_hash;
pc->partid = partid;
pc->groupid = groupid;
pc->cmd = cmd;
list_add(&ld->sendpay_commands, &pc->list);
tal_add_destructor(pc, destroy_sendpay_command);
Expand All @@ -94,12 +96,13 @@ static void
add_waitsendpay_waiter(struct lightningd *ld,
struct command *cmd,
const struct sha256 *payment_hash,
u64 partid)
u64 partid, u64 groupid)
{
struct sendpay_command *pc = tal(cmd, struct sendpay_command);

pc->payment_hash = *payment_hash;
pc->partid = partid;
pc->groupid = groupid;
pc->cmd = cmd;
list_add(&ld->waitsendpay_commands, &pc->list);
tal_add_destructor(pc, destroy_sendpay_command);
Expand Down Expand Up @@ -285,6 +288,8 @@ static void tell_waiters_failed(struct lightningd *ld,
continue;
if (payment->partid != pc->partid)
continue;
if (payment->groupid != pc->groupid)
continue;

sendpay_fail(pc->cmd, payment, pay_errcode, onionreply, fail,
errmsg);
Expand All @@ -311,6 +316,8 @@ static void tell_waiters_success(struct lightningd *ld,
continue;
if (payment->partid != pc->partid)
continue;
if (payment->groupid != pc->groupid)
continue;

sendpay_success(pc->cmd, payment);
}
Expand All @@ -327,7 +334,7 @@ void payment_succeeded(struct lightningd *ld, struct htlc_out *hout,
PAYMENT_COMPLETE, rval);
payment = wallet_payment_by_hash(tmpctx, ld->wallet,
&hout->payment_hash,
hout->partid);
hout->partid, hout->groupid);
assert(payment);

if (payment->local_offer_id)
Expand Down Expand Up @@ -539,7 +546,7 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout,

payment = wallet_payment_by_hash(tmpctx, ld->wallet,
&hout->payment_hash,
hout->partid);
hout->partid, hout->groupid);

#ifdef COMPAT_V052
/* Prior to "pay: delete HTLC when we delete payment." we would
Expand Down Expand Up @@ -652,7 +659,7 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout,
static struct command_result *wait_payment(struct lightningd *ld,
struct command *cmd,
const struct sha256 *payment_hash,
u64 partid)
u64 partid, u64 groupid)
{
struct wallet_payment *payment;
struct onionreply *failonionreply;
Expand All @@ -668,7 +675,7 @@ static struct command_result *wait_payment(struct lightningd *ld,
errcode_t rpcerrorcode;

payment = wallet_payment_by_hash(tmpctx, ld->wallet,
payment_hash, partid);
payment_hash, partid, groupid);
if (!payment) {
return command_fail(cmd, PAY_NO_SUCH_PAYMENT,
"Never attempted payment part %"PRIu64
Expand All @@ -678,12 +685,12 @@ static struct command_result *wait_payment(struct lightningd *ld,
payment_hash));
}

log_debug(cmd->ld->log, "Payment part %"PRIu64"/%"PRIu64" status %u",
partid, payment->partid, payment->status);
log_debug(cmd->ld->log, "Payment part %"PRIu64"/%"PRIu64"/%"PRIu64" status %u",
partid, payment->partid, payment->groupid, payment->status);

switch (payment->status) {
case PAYMENT_PENDING:
add_waitsendpay_waiter(ld, cmd, payment_hash, partid);
add_waitsendpay_waiter(ld, cmd, payment_hash, partid, groupid);
return NULL;

case PAYMENT_COMPLETE:
Expand All @@ -694,6 +701,7 @@ static struct command_result *wait_payment(struct lightningd *ld,
wallet_payment_get_failinfo(tmpctx, ld->wallet,
payment_hash,
partid,
groupid,
&failonionreply,
&faildestperm,
&failindex,
Expand Down Expand Up @@ -1078,7 +1086,7 @@ send_payment_core(struct lightningd *ld,
/* We write this into db when HTLC is actually sent. */
wallet_payment_setup(ld->wallet, payment);

add_sendpay_waiter(ld, cmd, rhash, partid);
add_sendpay_waiter(ld, cmd, rhash, partid, group);
return command_still_pending(cmd);
}

Expand Down Expand Up @@ -1487,16 +1495,21 @@ static struct command_result *json_waitsendpay(struct command *cmd,
struct sha256 *rhash;
unsigned int *timeout;
struct command_result *res;
u64 *partid;
u64 *partid, *groupid;

if (!param(cmd, buffer, params,
p_req("payment_hash", param_sha256, &rhash),
p_opt("timeout", param_number, &timeout),
p_opt_def("partid", param_u64, &partid, 0),
p_opt("groupid", param_u64, &groupid),
NULL))
return command_param_failed();

res = wait_payment(cmd->ld, cmd, rhash, *partid);
if (groupid == NULL) {
groupid = tal(cmd, u64);
*groupid = wallet_payment_get_groupid(cmd->ld->wallet, rhash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer this function to be called "wallet_payment_get_last_groupid" to be clearer? (or max_groupid)?

}
res = wait_payment(cmd->ld, cmd, rhash, *partid, *groupid);
if (res)
return res;

Expand Down
3 changes: 2 additions & 1 deletion lightningd/peer_htlcs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1622,7 +1622,8 @@ static bool update_out_htlc(struct channel *channel,
if (hout->am_origin) {
payment = wallet_payment_by_hash(tmpctx, ld->wallet,
&hout->payment_hash,
hout->partid);
hout->partid,
hout->groupid);
assert(payment);
payment_store(ld, take(payment));
}
Expand Down
1 change: 1 addition & 0 deletions plugins/libplugin-pay.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1490,6 +1490,7 @@ static struct command_result *payment_sendonion_success(struct command *cmd,
payment_waitsendpay_finished, p);
json_add_sha256(req->js, "payment_hash", p->payment_hash);
json_add_num(req->js, "partid", p->partid);
json_add_u64(req->js, "groupid", p->groupid);
send_outreq(p->plugin, req);

return command_still_pending(cmd);
Expand Down
6 changes: 3 additions & 3 deletions wallet/test/run-wallet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1820,13 +1820,13 @@ static bool test_payment_crud(struct lightningd *ld, const tal_t *ctx)
t->payment_preimage = NULL;
memset(&t->payment_hash, 1, sizeof(t->payment_hash));
t->partid = 0;
t->groupid = 0;
t->groupid = 12345;

db_begin_transaction(w->db);
t2 = tal_dup(NULL, struct wallet_payment, t);
wallet_payment_setup(w, t2);
wallet_payment_store(w, take(t2));
t2 = wallet_payment_by_hash(ctx, w, &t->payment_hash, 0);
t2 = wallet_payment_by_hash(ctx, w, &t->payment_hash, 0, t->groupid);
CHECK(t2 != NULL);
CHECK(t2->status == t->status);
CHECK(sha256_eq(&t2->payment_hash, &t->payment_hash));
Expand All @@ -1842,7 +1842,7 @@ static bool test_payment_crud(struct lightningd *ld, const tal_t *ctx)
memset(t->payment_preimage, 2, sizeof(*t->payment_preimage));
wallet_payment_set_status(w, &t->payment_hash, t->partid, t->groupid,
t->status, t->payment_preimage);
t2 = wallet_payment_by_hash(ctx, w, &t->payment_hash, t->partid);
t2 = wallet_payment_by_hash(ctx, w, &t->payment_hash, t->partid, t->groupid);
CHECK(t2 != NULL);
CHECK(t2->status == t->status);
CHECK(sha256_eq(&t2->payment_hash, &t->payment_hash));
Expand Down
17 changes: 9 additions & 8 deletions wallet/wallet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2910,9 +2910,10 @@ void wallet_payment_store(struct wallet *wallet,
stmt =
db_prepare_v2(wallet->db, SQL("SELECT status FROM payments"
" WHERE payment_hash=?"
" AND partid = ?;"));
" AND partid = ? AND groupid = ?;"));
db_bind_sha256(stmt, 0, &payment->payment_hash);
db_bind_u64(stmt, 1, payment->partid);
db_bind_u64(stmt, 2, payment->groupid);
db_query_prepared(stmt);
res = db_step(stmt);
assert(res);
Expand Down Expand Up @@ -3134,18 +3135,15 @@ static struct wallet_payment *wallet_stmt2payment(const tal_t *ctx,
} else
payment->local_offer_id = NULL;

if (!db_column_is_null(stmt, 17))
payment->groupid = db_column_u64(stmt, 17);
else
payment->groupid = 0;
payment->groupid = db_column_u64(stmt, 17);

return payment;
}

struct wallet_payment *
wallet_payment_by_hash(const tal_t *ctx, struct wallet *wallet,
const struct sha256 *payment_hash,
u64 partid)
u64 partid, u64 groupid)
{
struct db_stmt *stmt;
struct wallet_payment *payment;
Expand Down Expand Up @@ -3176,10 +3174,11 @@ wallet_payment_by_hash(const tal_t *ctx, struct wallet *wallet,
", groupid"
" FROM payments"
" WHERE payment_hash = ?"
" AND partid = ?"));
" AND partid = ? AND groupid=?"));

db_bind_sha256(stmt, 0, payment_hash);
db_bind_u64(stmt, 1, partid);
db_bind_u64(stmt, 2, groupid);
db_query_prepared(stmt);
if (db_step(stmt)) {
payment = wallet_stmt2payment(ctx, stmt);
Expand Down Expand Up @@ -3245,6 +3244,7 @@ void wallet_payment_get_failinfo(const tal_t *ctx,
struct wallet *wallet,
const struct sha256 *payment_hash,
u64 partid,
u64 groupid,
/* outputs */
struct onionreply **failonionreply,
bool *faildestperm,
Expand All @@ -3266,9 +3266,10 @@ void wallet_payment_get_failinfo(const tal_t *ctx,
", failnode, failchannel"
", failupdate, faildetail, faildirection"
" FROM payments"
" WHERE payment_hash=? AND partid=?;"));
" WHERE payment_hash=? AND partid=? AND groupid=?;"));
db_bind_sha256(stmt, 0, payment_hash);
db_bind_u64(stmt, 1, partid);
db_bind_u64(stmt, 2, groupid);
db_query_prepared(stmt);
resb = db_step(stmt);
assert(resb);
Expand Down
3 changes: 2 additions & 1 deletion wallet/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1048,7 +1048,7 @@ void wallet_local_htlc_out_delete(struct wallet *wallet,
struct wallet_payment *
wallet_payment_by_hash(const tal_t *ctx, struct wallet *wallet,
const struct sha256 *payment_hash,
u64 partid);
u64 partid, u64 groupid);

/**
* Retrieve maximum groupid for a given payment_hash.
Expand Down Expand Up @@ -1082,6 +1082,7 @@ void wallet_payment_get_failinfo(const tal_t *ctx,
struct wallet *wallet,
const struct sha256 *payment_hash,
u64 partid,
u64 groupid,
/* outputs */
struct onionreply **failonionreply,
bool *faildestperm,
Expand Down