Skip to content

Commit 45fe549

Browse files
rustyrussellrustyrussell
committed
pay: change amount-in-flight check to be more nuanced.
We currently refuse a payment if one is already in flight. For parallel payments, it's a bit more subtle: we want to refuse if it we already have the total-amount-of-invoice in flight. So we get all the current payments, and sum the pending ones. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
1 parent 43ba99e commit 45fe549

2 files changed

Lines changed: 92 additions & 26 deletions

File tree

‎lightningd/pay.c‎

Lines changed: 90 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -733,42 +733,108 @@ send_payment_core(struct lightningd *ld,
733733
struct short_channel_id *route_channels TAKES,
734734
struct secret *path_secrets)
735735
{
736-
struct wallet_payment *payment;
736+
const struct wallet_payment **payments, *old_payment = NULL;
737737
struct channel *channel;
738738
enum onion_type failcode;
739739
struct htlc_out *hout;
740740
struct routing_failure *fail;
741+
struct amount_msat msat_already_pending = AMOUNT_MSAT(0);
742+
743+
/* Now, do we already have one or more payments? */
744+
payments = wallet_payment_list(tmpctx, ld->wallet, rhash);
745+
for (size_t i = 0; i < tal_count(payments); i++) {
746+
log_debug(ld->log, "Payment %zu/%zu: %s %s",
747+
i, tal_count(payments),
748+
type_to_string(tmpctx, struct amount_msat,
749+
&payments[i]->msatoshi),
750+
payments[i]->status == PAYMENT_COMPLETE ? "COMPLETE"
751+
: payments[i]->status == PAYMENT_PENDING ? "PENDING"
752+
: "FAILED");
753+
754+
switch (payments[i]->status) {
755+
case PAYMENT_COMPLETE:
756+
if (payments[i]->parallel_id != parallel_id)
757+
continue;
741758

742-
/* Now, do we already have a payment? */
743-
payment = wallet_payment_by_hash(tmpctx, ld->wallet, rhash, parallel_id);
744-
if (payment) {
745-
/* FIXME: We should really do something smarter here! */
746-
if (payment->status == PAYMENT_PENDING) {
747-
log_debug(ld->log, "send_payment: previous still in progress");
748-
return json_sendpay_in_progress(cmd, payment);
749-
}
750-
if (payment->status == PAYMENT_COMPLETE) {
751-
log_debug(ld->log, "send_payment: previous succeeded");
752759
/* Must match successful payment parameters. */
753-
if (!amount_msat_eq(payment->msatoshi, msat)) {
760+
if (!amount_msat_eq(payments[i]->msatoshi, msat)) {
754761
return command_fail(cmd, PAY_RHASH_ALREADY_USED,
755762
"Already succeeded "
756763
"with amount %s",
757764
type_to_string(tmpctx,
758765
struct amount_msat,
759-
&payment->msatoshi));
766+
&payments[i]->msatoshi));
760767
}
761-
if (payment->destination && destination
762-
&& !node_id_eq(payment->destination, destination)) {
768+
if (payments[i]->destination && destination
769+
&& !node_id_eq(payments[i]->destination,
770+
destination)) {
763771
return command_fail(cmd, PAY_RHASH_ALREADY_USED,
764772
"Already succeeded to %s",
765773
type_to_string(tmpctx,
766774
struct node_id,
767-
payment->destination));
775+
payments[i]->destination));
768776
}
769-
return sendpay_success(cmd, payment);
770-
}
771-
log_debug(ld->log, "send_payment: found previous, retrying");
777+
return sendpay_success(cmd, payments[i]);
778+
779+
case PAYMENT_PENDING:
780+
/* Can't mix non-parallel and parallel payments! */
781+
if (!payments[i]->parallel_id != !parallel_id) {
782+
return command_fail(cmd, PAY_IN_PROGRESS,
783+
"Already have %s payment in progress",
784+
payments[i]->parallel_id ? "parallel" : "non-parallel");
785+
}
786+
if (payments[i]->parallel_id == parallel_id)
787+
return json_sendpay_in_progress(cmd, payments[i]);
788+
/* You shouldn't change your mind about amount being
789+
* sent, since we'll use it in onion! */
790+
else if (!amount_msat_eq(payments[i]->msatoshi_total,
791+
msat_total))
792+
return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
793+
"msatoshi was previously %s, now %s",
794+
type_to_string(tmpctx,
795+
struct amount_msat,
796+
&payments[i]->msatoshi_total),
797+
type_to_string(tmpctx,
798+
struct amount_msat,
799+
&msat_total));
800+
801+
802+
if (!amount_msat_add(&msat_already_pending,
803+
msat_already_pending,
804+
payments[i]->msatoshi)) {
805+
return command_fail(cmd, LIGHTNINGD,
806+
"Internal amount overflow!"
807+
" %s + %s in %zu/%zu",
808+
type_to_string(tmpctx,
809+
struct amount_msat,
810+
&msat_already_pending),
811+
type_to_string(tmpctx,
812+
struct amount_msat,
813+
&payments[i]->msatoshi),
814+
i, tal_count(payments));
815+
}
816+
break;
817+
818+
case PAYMENT_FAILED:
819+
if (payments[i]->parallel_id == parallel_id)
820+
old_payment = payments[i];
821+
}
822+
}
823+
824+
/* BOLT-3a09bc54f8443c4757b47541a5310aff6377ee21 #4:
825+
*
826+
* - MUST NOT send another HTLC if the total `amount_msat` of the HTLC
827+
* set is already greater or equal to `total_msat`.
828+
*/
829+
/* We don't do this for single 0-value payments (sendonion does this) */
830+
if (!amount_msat_eq(msat_total, AMOUNT_MSAT(0))
831+
&& amount_msat_greater_eq(msat_already_pending, msat_total)) {
832+
return command_fail(cmd, PAY_IN_PROGRESS,
833+
"Already have %s of %s payments in progress",
834+
type_to_string(tmpctx, struct amount_msat,
835+
&msat_already_pending),
836+
type_to_string(tmpctx, struct amount_msat,
837+
&msat_total));
772838
}
773839

774840
channel = active_channel_by_id(ld, &first_hop->nodeid, NULL);
@@ -795,7 +861,7 @@ send_payment_core(struct lightningd *ld,
795861
&first_hop->channel_id,
796862
&channel->peer->id);
797863

798-
return sendpay_fail(cmd, payment, PAY_TRY_OTHER_ROUTE,
864+
return sendpay_fail(cmd, old_payment, PAY_TRY_OTHER_ROUTE,
799865
NULL, fail, "First peer not ready");
800866
}
801867

@@ -804,14 +870,14 @@ send_payment_core(struct lightningd *ld,
804870
* a possibility, and we end up in handle_missing_htlc_output->
805871
* onchain_failed_our_htlc->payment_failed with no payment.
806872
*/
807-
if (payment) {
808-
wallet_payment_delete(ld->wallet, rhash, payment->parallel_id);
873+
if (old_payment) {
874+
wallet_payment_delete(ld->wallet, rhash, parallel_id);
809875
wallet_local_htlc_out_delete(ld->wallet, channel, rhash,
810-
payment->parallel_id);
876+
parallel_id);
811877
}
812878

813879
/* If hout fails, payment should be freed too. */
814-
payment = tal(hout, struct wallet_payment);
880+
struct wallet_payment *payment = tal(hout, struct wallet_payment);
815881
payment->id = 0;
816882
payment->payment_hash = *rhash;
817883
payment->parallel_id = parallel_id;

‎tests/test_pay.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -531,12 +531,12 @@ def check_balances():
531531
wait_for(check_balances)
532532

533533
# Repeat will "succeed", but won't actually send anything (duplicate)
534-
assert not l1.daemon.is_in_log('... succeeded')
534+
assert not l1.daemon.is_in_log('Payment 0/1: .* COMPLETE')
535535
details = l1.rpc.sendpay([routestep], rhash)
536536
assert details['status'] == "complete"
537537
preimage2 = details['payment_preimage']
538538
assert preimage == preimage2
539-
l1.daemon.wait_for_log('... succeeded')
539+
l1.daemon.wait_for_log('Payment 0/1: .* COMPLETE')
540540
assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['status'] == 'paid'
541541
assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['msatoshi_received'] == rs['msatoshi']
542542

0 commit comments

Comments
 (0)