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

lightningd: disallow msatoshi arg to sendpay unless exact when non-… #3470

Merged
merged 1 commit into from
Jan 31, 2020
Merged

lightningd: disallow msatoshi arg to sendpay unless exact when non-… #3470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions doc/lightning-sendpay.7
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions doc/lightning-sendpay.7.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

This parameter used to be used for value randomization, where we overpay the payee, but record the exact amount indicated in the invoice in our database rather than the overpaid amount (i.e. treating the overpayment as a fee). We have since replaced value randomization with shadow routing, also overpaying the payee, but it seems this was no longer used to override the database record. I think it is better if our permanent records did not reveal the use of overpayment to obscure the location of the payee, but well. In any case changing msatoshi to make it signal multipath is a breaking change in the previous interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the original intent with mpp was to only override the semantic for msatoshi for this case, but since it's broken in 0.8.0 anyway :(

The original use has been largely superceded by the fact that we now save the bolt11 string with the payment.

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*.
Expand Down
38 changes: 20 additions & 18 deletions lightningd/pay.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1297,31 +1297,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));
Expand All @@ -1333,8 +1335,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);
}

Expand Down
34 changes: 33 additions & 1 deletion tests/test_pay.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down Expand Up @@ -2800,3 +2800,35 @@ def pay(l1, inv):

# 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.
"""
Comment on lines +2806 to +2809
Copy link
Member

Choose a reason for hiding this comment

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

Nit: comments should be a short single line description (<80 chars), followed by a blank line, and then a multiline description (if any) that is indented to match the """, finally a """ on it's own on a last line. You comment would become this:

    """Verify that the msatoshi arg is checked for `sendpay`.

    The `sendpay` `msatoshi` argument 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)