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 <rusty@rustcorp.com.au>

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\.

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*.

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);
 }
 

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)