Skip to content

Core sendpay fixes #3860

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

Merged
cdecker merged 3 commits into from
Jul 21, 2020
Merged

Core sendpay fixes #3860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c79fe6
sendpay: don't allow a new part payment if any part has succeeded.
rustyrussell Jul 21, 2020
63007ba
sendpay: insist that partid be an exact duplicate if in progress.
rustyrussell Jul 21, 2020
72e735e
doc: document the payment_secret argument to sendpay.
rustyrussell Jul 21, 2020
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
8 changes: 7 additions & 1 deletion 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.

7 changes: 6 additions & 1 deletion doc/lightning-sendpay.7.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ SYNOPSIS
--------

**sendpay** *route* *payment\_hash* \[*label*\] \[*msatoshi*\]
\[*bolt11*\] \[*partid*\]
\[*bolt11*\] \[*payment_secret*\] \[*partid*\]

DESCRIPTION
-----------
Expand Down Expand Up @@ -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
Expand Down
37 changes: 34 additions & 3 deletions lightningd/pay.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -803,17 +804,20 @@ send_payment_core(struct lightningd *ld,

switch (payments[i]->status) {
case PAYMENT_COMPLETE:
have_complete = true;
if (payments[i]->partid != partid)
continue;

/* Must match successful payment parameters. */
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,
Expand All @@ -833,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,
Expand Down Expand Up @@ -871,6 +896,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
Expand Down
17 changes: 14 additions & 3 deletions tests/test_pay.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -2639,6 +2642,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)
Expand Down