From f3600d22a08e65f917186c295bb5bd993d8f95ec Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 30 Jan 2020 12:32:37 +1030 Subject: [PATCH] lightningd: disallow `msatoshi` arg to sendpay unless exact when non-MPP. Using it with a different value to the amount sent causes a crash in 0.8.0, which is effectively deprecating it, so let's disallow it now. Changelog-Changed: If the optional `msatoshi` param to sendpay for non-MPP is set, it must be the exact amount sent to the final recipient. Signed-off-by: Rusty Russell --- doc/lightning-sendpay.7 | 4 ++-- doc/lightning-sendpay.7.md | 4 ++-- lightningd/pay.c | 38 ++++++++++++++++++++------------------ tests/test_pay.py | 34 +++++++++++++++++++++++++++++++++- 4 files changed, 57 insertions(+), 23 deletions(-) diff --git a/doc/lightning-sendpay.7 b/doc/lightning-sendpay.7 index 1ffd7f391..c8fd3c608 100644 --- a/doc/lightning-sendpay.7 +++ b/doc/lightning-sendpay.7 @@ -29,8 +29,8 @@ The \fIlabel\fR and \fIbolt11\fR parameters, if provided, will be returned in \fIwaitsendpay\fR and \fIlistsendpays\fR results\. -The \fImsatoshi\fR amount, if provided, is the amount that will be recorded -as the target payment value\. If not specified, it will be the final +The \fImsatoshi\fR amount must be provided if \fIpartid\fR is non-zero, otherwise +it must be equal to the final amount to the destination\. By default it is in millisatoshi precision; it can be a whole number, or a whole number 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\. diff --git a/doc/lightning-sendpay.7.md b/doc/lightning-sendpay.7.md index 21ef96cf0..774daa166 100644 --- a/doc/lightning-sendpay.7.md +++ b/doc/lightning-sendpay.7.md @@ -27,8 +27,8 @@ definite failure. The *label* and *bolt11* parameters, if provided, will be returned in *waitsendpay* and *listsendpays* results. -The *msatoshi* amount, if provided, is the amount that will be recorded -as the target payment value. If not specified, it will be the final +The *msatoshi* amount must be provided if *partid* is non-zero, otherwise +it must be equal to the final amount to the destination. By default it is in millisatoshi precision; it can be a whole number, or a whole number 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*. diff --git a/lightningd/pay.c b/lightningd/pay.c index 12aa704eb..b97268294 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -1328,31 +1328,33 @@ static struct command_result *json_sendpay(struct command *cmd, route[i].direction = direction ? *direction : 0; } - /* The given msatoshi is the actual payment that the payee is - * requesting. The final hop amount is what we actually give, which can - * be from the msatoshi to twice msatoshi. */ - if (*partid && !msat) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Must specify msatoshi with partid"); - /* finalhop.amount > 2 * msatoshi, fail. */ - if (msat) { - struct amount_msat limit; + const struct amount_msat final_amount = route[routetok->size-1].amount; - if (!amount_msat_add(&limit, *msat, *msat)) - return command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "Unbelievable msatoshi %s", - type_to_string(tmpctx, - struct amount_msat, - msat)); + if (msat && !*partid && !amount_msat_eq(*msat, final_amount)) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Do not specify msatoshi (%s) without" + " partid: if you do, it must be exactly" + " the final amount (%s)", + type_to_string(tmpctx, struct amount_msat, + msat), + type_to_string(tmpctx, struct amount_msat, + &final_amount)); - if (amount_msat_greater(route[routetok->size-1].amount, limit)) + /* For MPP, the total we send must *exactly* equal the amount + * we promise to send (msatoshi). So no single payment can be + * > than that. */ + if (*partid) { + if (amount_msat_greater(final_amount, *msat)) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "final %s more than twice msatoshi %s", + "Final amount %s is greater than" + " %s, despite MPP", type_to_string(tmpctx, struct amount_msat, - &route[routetok->size-1].amount), + &final_amount), type_to_string(tmpctx, struct amount_msat, msat)); @@ -1364,8 +1366,8 @@ static struct command_result *json_sendpay(struct command *cmd, return send_payment(cmd->ld, cmd, rhash, *partid, route, - route[routetok->size-1].amount, - msat ? *msat : route[routetok->size-1].amount, + final_amount, + msat ? *msat : final_amount, label, b11str, payment_secret); } diff --git a/tests/test_pay.py b/tests/test_pay.py index 420e66560..05b8ec850 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2601,7 +2601,7 @@ def test_partial_payment(node_factory, bitcoind, executor): with pytest.raises(RpcError, match=r'Already have parallel payment in progress'): l1.rpc.sendpay(route=r124, payment_hash=inv['payment_hash'], - msatoshi=1000, + msatoshi=499, payment_secret=paysecret) # It will not allow a parallel with different msatoshi! @@ -2800,3 +2800,35 @@ def test_blockheight_disagreement(node_factory, bitcoind, executor): # pay command should complete without error fut.result() + + +def test_sendpay_msatoshi_arg(node_factory): + """sendpay msatoshi arg was used for non-MPP to indicate the amount +they asked for. But using it with anything other than the final amount +caused a crash in 0.8.0, so we then disallowed it. + """ + l1, l2 = node_factory.line_graph(2) + + inv = l2.rpc.invoice(1000, 'inv', 'inv') + + # Can't send non-MPP payment which specifies msatoshi != final. + with pytest.raises(RpcError, match=r'Do not specify msatoshi \(1001msat\) without' + ' partid: if you do, it must be exactly' + r' the final amount \(1000msat\)'): + l1.rpc.sendpay(route=l1.rpc.getroute(l2.info['id'], 1000, 1)['route'], payment_hash=inv['payment_hash'], msatoshi=1001, bolt11=inv['bolt11']) + with pytest.raises(RpcError, match=r'Do not specify msatoshi \(999msat\) without' + ' partid: if you do, it must be exactly' + r' the final amount \(1000msat\)'): + l1.rpc.sendpay(route=l1.rpc.getroute(l2.info['id'], 1000, 1)['route'], payment_hash=inv['payment_hash'], msatoshi=999, bolt11=inv['bolt11']) + + # Can't send MPP payment which pays any more than amount. + with pytest.raises(RpcError, match=r'Final amount 1001msat is greater than 1000msat, despite MPP'): + l1.rpc.sendpay(route=l1.rpc.getroute(l2.info['id'], 1001, 1)['route'], payment_hash=inv['payment_hash'], msatoshi=1000, bolt11=inv['bolt11'], partid=1) + + # But this works + l1.rpc.sendpay(route=l1.rpc.getroute(l2.info['id'], 1001, 1)['route'], payment_hash=inv['payment_hash'], msatoshi=1001, bolt11=inv['bolt11']) + l1.rpc.waitsendpay(inv['payment_hash']) + + inv = only_one(l2.rpc.listinvoices('inv')['invoices']) + assert inv['status'] == 'paid' + assert inv['amount_received_msat'] == Millisatoshi(1001)