From 2c79fe6bed5c459caac1a56b542e62de42b5bc13 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 21 Jul 2020 13:50:39 +0930 Subject: [PATCH 1/3] sendpay: don't allow a new part payment if any part has succeeded. This wasn't important before, but now we have MPP it's good to enforce. Reported-by: Christian Decker Signed-off-by: Rusty Russell --- lightningd/pay.c | 14 ++++++++++++-- tests/test_pay.py | 8 ++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index e100d3b6d308..716a54842066 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -789,6 +789,7 @@ send_payment_core(struct lightningd *ld, struct htlc_out *hout; struct routing_failure *fail; struct amount_msat msat_already_pending = AMOUNT_MSAT(0); + bool have_complete = false; /* Now, do we already have one or more payments? */ payments = wallet_payment_list(tmpctx, ld->wallet, rhash); @@ -803,6 +804,7 @@ send_payment_core(struct lightningd *ld, switch (payments[i]->status) { case PAYMENT_COMPLETE: + have_complete = true; if (payments[i]->partid != partid) continue; @@ -810,10 +812,12 @@ send_payment_core(struct lightningd *ld, if (!amount_msat_eq(payments[i]->msatoshi, msat)) { return command_fail(cmd, PAY_RHASH_ALREADY_USED, "Already succeeded " - "with amount %s", + "with amount %s (not %s)", type_to_string(tmpctx, struct amount_msat, - &payments[i]->msatoshi)); + &payments[i]->msatoshi), + type_to_string(tmpctx, + struct amount_msat, &msat)); } if (payments[i]->destination && destination && !node_id_eq(payments[i]->destination, @@ -871,6 +875,12 @@ send_payment_core(struct lightningd *ld, } } + /* If any part has succeeded, you can't start a new one! */ + if (have_complete) { + return command_fail(cmd, PAY_RHASH_ALREADY_USED, + "Already succeeded other parts"); + } + /* BOLT #4: * * - MUST NOT send another HTLC if the total `amount_msat` of the HTLC diff --git a/tests/test_pay.py b/tests/test_pay.py index c3d870dab203..782c84361fa7 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2639,6 +2639,14 @@ def test_partial_payment(node_factory, bitcoind, executor): assert pay['number_of_parts'] == 2 assert pay['amount_sent_msat'] == Millisatoshi(1002) + # It will immediately succeed if we pay again. + pay = l1.rpc.sendpay(route=r124, payment_hash=inv['payment_hash'], msatoshi=1000, bolt11=inv['bolt11'], payment_secret=paysecret, partid=2) + assert pay['status'] == 'complete' + + # If we try with an unknown partid, it will refuse. + with pytest.raises(RpcError, match=r'Already succeeded'): + l1.rpc.sendpay(route=r124, payment_hash=inv['payment_hash'], msatoshi=1000, bolt11=inv['bolt11'], payment_secret=paysecret, partid=3) + def test_partial_payment_timeout(node_factory, bitcoind): l1, l2 = node_factory.line_graph(2) From 63007ba524bf7d3db6231773fe109104c4a0d11d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 21 Jul 2020 13:52:31 +0930 Subject: [PATCH 2/3] sendpay: insist that partid be an exact duplicate if in progress. The test had part 1 and 2 backward, but still worked. When I copied that to *after* the test had succeeded, it complained. It should always complain, to catch bugs. Signed-off-by: Rusty Russell --- lightningd/pay.c | 23 ++++++++++++++++++++++- tests/test_pay.py | 9 ++++++--- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index 716a54842066..178d1b187b86 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -837,8 +837,29 @@ send_payment_core(struct lightningd *ld, "Already have %s payment in progress", payments[i]->partid ? "parallel" : "non-parallel"); } - if (payments[i]->partid == partid) + if (payments[i]->partid == partid) { + /* You can't change details while it's pending */ + if (!amount_msat_eq(payments[i]->msatoshi, msat)) { + return command_fail(cmd, PAY_RHASH_ALREADY_USED, + "Already pending " + "with amount %s (not %s)", + type_to_string(tmpctx, + struct amount_msat, + &payments[i]->msatoshi), + type_to_string(tmpctx, + struct amount_msat, &msat)); + } + if (payments[i]->destination && destination + && !node_id_eq(payments[i]->destination, + destination)) { + return command_fail(cmd, PAY_RHASH_ALREADY_USED, + "Already pending to %s", + type_to_string(tmpctx, + struct node_id, + payments[i]->destination)); + } return json_sendpay_in_progress(cmd, payments[i]); + } /* You shouldn't change your mind about amount being * sent, since we'll use it in onion! */ else if (!amount_msat_eq(payments[i]->total_msat, diff --git a/tests/test_pay.py b/tests/test_pay.py index 782c84361fa7..48c38dab6c64 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2609,9 +2609,12 @@ def test_partial_payment(node_factory, bitcoind, executor): l1.rpc.sendpay(route=r124, payment_hash=inv['payment_hash'], msatoshi=1000, bolt11=inv['bolt11'], payment_secret=paysecret, partid=3) - # But repeat is a NOOP. - l1.rpc.sendpay(route=r124, payment_hash=inv['payment_hash'], msatoshi=1000, bolt11=inv['bolt11'], payment_secret=paysecret, partid=1) - l1.rpc.sendpay(route=r134, payment_hash=inv['payment_hash'], msatoshi=1000, bolt11=inv['bolt11'], payment_secret=paysecret, partid=2) + # But repeat is a NOOP, as long as they're exactly the same! + with pytest.raises(RpcError, match=r'Already pending with amount 501msat \(not 499msat\)'): + l1.rpc.sendpay(route=r124, payment_hash=inv['payment_hash'], msatoshi=1000, bolt11=inv['bolt11'], payment_secret=paysecret, partid=1) + + l1.rpc.sendpay(route=r134, payment_hash=inv['payment_hash'], msatoshi=1000, bolt11=inv['bolt11'], payment_secret=paysecret, partid=1) + l1.rpc.sendpay(route=r124, payment_hash=inv['payment_hash'], msatoshi=1000, bolt11=inv['bolt11'], payment_secret=paysecret, partid=2) # Make sure they've done the suppress-commitment thing before we unsuppress l2.daemon.wait_for_log(r'dev_disconnect') From 72e735ec19190605b38ee060dd7c010342235509 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 21 Jul 2020 13:52:56 +0930 Subject: [PATCH 3/3] doc: document the payment_secret argument to sendpay. Signed-off-by: Rusty Russell --- doc/lightning-sendpay.7 | 8 +++++++- doc/lightning-sendpay.7.md | 7 ++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/doc/lightning-sendpay.7 b/doc/lightning-sendpay.7 index c8fd3c60863d..2454f0fa675a 100644 --- a/doc/lightning-sendpay.7 +++ b/doc/lightning-sendpay.7 @@ -4,7 +4,7 @@ lightning-sendpay - Low-level command for sending a payment via a route .SH SYNOPSIS \fBsendpay\fR \fIroute\fR \fIpayment_hash\fR [\fIlabel\fR] [\fImsatoshi\fR] -[\fIbolt11\fR] [\fIpartid\fR] +[\fIbolt11\fR] [\fIpayment_secret\fR] [\fIpartid\fR] .SH DESCRIPTION @@ -36,6 +36,12 @@ ending in \fImsat\fR or \fIsat\fR, or a number with three decimal places ending in \fIsat\fR, or a number with 1 to 11 decimal places ending in \fIbtc\fR\. +The \fIpayment_secret\fR is the value that the final recipient requires to +accept the payment, as defined by the \fBpayment_data\fR field in BOLT 4 +and the \fBs\fR field in the BOLT 11 invoice format\. It is required if +\fIpartid\fR is non-zero\. + + The \fIpartid\fR value, if provided and non-zero, allows for multiple parallel partial payments with the same \fIpayment_hash\fR\. The \fImsatoshi\fR amount (which must be provided) for each \fBsendpay\fR with matching diff --git a/doc/lightning-sendpay.7.md b/doc/lightning-sendpay.7.md index 774daa166306..819e0d55d13c 100644 --- a/doc/lightning-sendpay.7.md +++ b/doc/lightning-sendpay.7.md @@ -5,7 +5,7 @@ SYNOPSIS -------- **sendpay** *route* *payment\_hash* \[*label*\] \[*msatoshi*\] -\[*bolt11*\] \[*partid*\] +\[*bolt11*\] \[*payment_secret*\] \[*partid*\] DESCRIPTION ----------- @@ -33,6 +33,11 @@ amount to the destination. By default it is in millisatoshi precision; it can be ending in *msat* or *sat*, or a number with three decimal places ending in *sat*, or a number with 1 to 11 decimal places ending in *btc*. +The *payment_secret* is the value that the final recipient requires to +accept the payment, as defined by the `payment_data` field in BOLT 4 +and the `s` field in the BOLT 11 invoice format. It is required if +*partid* is non-zero. + The *partid* value, if provided and non-zero, allows for multiple parallel partial payments with the same *payment_hash*. The *msatoshi* amount (which must be provided) for each **sendpay** with matching