From bb90e49bebb1bae7ff5ce0dfa9342031dbd9ef8d Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Thu, 6 Aug 2020 12:49:25 +0800 Subject: [PATCH 01/14] tests/test_pay.py: Add test for conflict between presplit and routehints paymods. --- tests/test_pay.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/test_pay.py b/tests/test_pay.py index 8d22f86589b4..3dca2ac3d7a8 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3221,3 +3221,32 @@ def test_bolt11_null_after_pay(node_factory, bitcoind): pays = l2.rpc.listpays()["pays"] assert(pays[0]["bolt11"] == invl1) assert('amount_msat' in pays[0] and pays[0]['amount_msat'] == amt) + + +@pytest.mark.xfail(strict=True) +def test_mpp_presplit_routehint_conflict(node_factory, bitcoind): + ''' + We have a bug where pre-splitting the payment prevents *any* + routehints from being taken. + We tickle that bug here by building l1->l2->l3, but with + l2->l3 as an unpublished channel. + If the payment is large enough to trigger pre-splitting, the + routehints are not applied in any of the splits. + ''' + l1, l2, l3 = node_factory.get_nodes(3) + + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l1l2 = l1.fund_channel(l2, 10**7, announce_channel=True) + l2.rpc.connect(l3.info['id'], 'localhost', l3.port) + l2.fund_channel(l3, 10**7, announce_channel=False) + + bitcoind.generate_block(6) + sync_blockheight(bitcoind, [l1, l2, l3]) + + # Wait for l3 to learn about l1->l2, otherwise it will think + # l2 is a deadend and not add it to the routehint. + wait_for(lambda: len(l3.rpc.listchannels(l1l2)['channels']) >= 2) + + inv = l3.rpc.invoice(Millisatoshi(2 * 10000 * 1000), 'i', 'i', exposeprivatechannels=True)['bolt11'] + + l1.rpc.pay(inv) From d1a5be758c728f8a1cc7f520a59cc6f3db2dabe0 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Thu, 6 Aug 2020 12:56:25 +0800 Subject: [PATCH 02/14] plugins/pay.c: Fix the routehints/presplit conflict. Changelog-Fixed: pay: Fixed a bug where routehints would be ignored if the payment exceeded 10,000 satoshi. This is particularly bad if the payee is only reachable via routehints in an invoice. --- plugins/pay.c | 18 +++++++++++++++++- tests/test_pay.py | 3 +-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/plugins/pay.c b/plugins/pay.c index f7c9595a47a2..3acafcf219d6 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -1881,9 +1881,25 @@ struct payment_modifier *paymod_mods[] = { &local_channel_hints_pay_mod, &exemptfee_pay_mod, &directpay_pay_mod, - &presplit_pay_mod, &shadowroute_pay_mod, + /* NOTE: The order in which these two paymods are executed is + * significant! + * routehints *must* execute first before presplit. + * + * FIXME: Giving an ordered list of paymods to the paymod + * system is the wrong interface, given that the order in + * which paymods execute is significant. + * (This is typical of Entity-Component-System pattern.) + * What should be done is that libplugin-pay should have a + * canonical list of paymods in the order they execute + * correctly, and whether they are default-enabled/default-disabled, + * and then clients like `pay` and `keysend` will disable/enable + * paymods that do not help them, instead of the current interface + * where clients provide an *ordered* list of paymods they want to + * use. + */ &routehints_pay_mod, + &presplit_pay_mod, &waitblockheight_pay_mod, &retry_pay_mod, &adaptive_splitter_pay_mod, diff --git a/tests/test_pay.py b/tests/test_pay.py index 3dca2ac3d7a8..5baeeb1eb234 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3223,10 +3223,9 @@ def test_bolt11_null_after_pay(node_factory, bitcoind): assert('amount_msat' in pays[0] and pays[0]['amount_msat'] == amt) -@pytest.mark.xfail(strict=True) def test_mpp_presplit_routehint_conflict(node_factory, bitcoind): ''' - We have a bug where pre-splitting the payment prevents *any* + We had a bug where pre-splitting the payment prevents *any* routehints from being taken. We tickle that bug here by building l1->l2->l3, but with l2->l3 as an unpublished channel. From 4955266d1dad443c88580554a3de653e5e515c39 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Thu, 6 Aug 2020 16:52:33 +0200 Subject: [PATCH 03/14] pay: Add timestamp of first part to `listpays` Changelog-Added: JSON-RPC: The result returned by `listpays` now includes the timestamp of the first part of the payment --- plugins/pay.c | 12 +++++++++++- tests/test_pay.py | 1 + 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/plugins/pay.c b/plugins/pay.c index 3acafcf219d6..883ddd693777 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -1666,6 +1666,9 @@ struct pay_mpp { * only). Null if we have any part for which we didn't know the * amount. */ struct amount_msat *amount; + + /* Timestamp of the first part */ + u32 timestamp; }; static const struct sha256 *pay_mpp_key(const struct pay_mpp *pm) @@ -1735,6 +1738,8 @@ static void add_new_entry(struct json_stream *ret, json_object_start(ret, NULL); json_add_string(ret, "bolt11", pm->b11); json_add_string(ret, "status", pm->status); + json_add_u32(ret, "created_at", pm->timestamp); + if (pm->label) json_add_tok(ret, "label", pm->label, buf); if (pm->preimage) @@ -1777,15 +1782,19 @@ static struct command_result *listsendpays_done(struct command *cmd, ret = jsonrpc_stream_success(cmd); json_array_start(ret, "pays"); json_for_each_arr(i, t, arr) { - const jsmntok_t *status, *b11tok, *hashtok; + const jsmntok_t *status, *b11tok, *hashtok, *createdtok; const char *b11 = b11str; struct sha256 payment_hash; + u32 created_at; b11tok = json_get_member(buf, t, "bolt11"); hashtok = json_get_member(buf, t, "payment_hash"); + createdtok = json_get_member(buf, t, "created_at"); assert(hashtok != NULL); + assert(createdtok != NULL); json_to_sha256(buf, hashtok, &payment_hash); + json_to_u32(buf, createdtok, &created_at); if (b11tok) b11 = json_strdup(cmd, buf, b11tok); @@ -1800,6 +1809,7 @@ static struct command_result *listsendpays_done(struct command *cmd, pm->amount = talz(pm, struct amount_msat); pm->num_nonfailed_parts = 0; pm->status = NULL; + pm->timestamp = created_at; pay_map_add(&pay_map, pm); } diff --git a/tests/test_pay.py b/tests/test_pay.py index 5baeeb1eb234..7c59dd345a52 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3221,6 +3221,7 @@ def test_bolt11_null_after_pay(node_factory, bitcoind): pays = l2.rpc.listpays()["pays"] assert(pays[0]["bolt11"] == invl1) assert('amount_msat' in pays[0] and pays[0]['amount_msat'] == amt) + assert('created_at' in pays[0]) def test_mpp_presplit_routehint_conflict(node_factory, bitcoind): From 26b5baf40b13ea48561530f683b3608c312e8dad Mon Sep 17 00:00:00 2001 From: Vincent Date: Wed, 29 Jul 2020 10:48:47 +0200 Subject: [PATCH 04/14] listpays: fixed bolt11 null with keysend and update doc command listpays: make doc-all missed Changelog-Added: JSON-RPC: `listpays` can be used to query payments using the `payment_hash` Changelog-Added: JSON-RPC: `listpays` now includes the `payment_hash` --- doc/lightning-listpays.7 | 14 +++++++++----- doc/lightning-listpays.7.md | 13 ++++++++----- plugins/pay.c | 12 ++++++++++-- tests/test_pay.py | 24 ++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 12 deletions(-) diff --git a/doc/lightning-listpays.7 b/doc/lightning-listpays.7 index adff03ec3521..30573f34ec51 100644 --- a/doc/lightning-listpays.7 +++ b/doc/lightning-listpays.7 @@ -3,12 +3,12 @@ lightning-listpays - Command for querying payment status .SH SYNOPSIS -\fBlistpays\fR [bolt11] +\fBlistpays\fR [bolt11] [payment_hash] .SH DESCRIPTION The \fBlistpay\fR RPC command gets the status of all \fIpay\fR commands, or a -single one if \fIbolt11\fR is specified\. +single one if either \fIbolt11\fR or \fIpayment_hash\fR was specified\. .SH RETURN VALUE @@ -16,7 +16,11 @@ On success, an array of objects is returned\. Each object contains: \fIbolt11\fR -the \fIbolt11\fR argument given to \fIpay\fR (see below for exceptions)\. +the \fIbolt11\fR invoice if provided to \fBpay\fR\. + + + \fIpayment_hash\fR +the \fIpayment_hash\fR of the payment\. \fIstatus\fR @@ -24,11 +28,11 @@ one of \fIcomplete\fR, \fIfailed\fR or \fIpending\fR\. \fIpayment_preimage\fR -(if \fIstatus\fR is \fIcomplete\fR) proves payment was received\. +if \fIstatus\fR is \fIcomplete\fR\. \fIlabel\fR -optional \fIlabel\fR, if provided to \fIpay\fR\. +optional \fIlabel\fR, if provided to \fIpay\fR or \fIsendonion\fR\. \fIamount_sent_msat\fR diff --git a/doc/lightning-listpays.7.md b/doc/lightning-listpays.7.md index 1cb8d0e19b14..4620dfabfdc8 100644 --- a/doc/lightning-listpays.7.md +++ b/doc/lightning-listpays.7.md @@ -4,13 +4,13 @@ lightning-listpays -- Command for querying payment status SYNOPSIS -------- -**listpays** \[bolt11\] +**listpays** \[bolt11\] \[payment_hash\] DESCRIPTION ----------- The **listpay** RPC command gets the status of all *pay* commands, or a -single one if *bolt11* is specified. +single one if either *bolt11* or *payment_hash* was specified. RETURN VALUE ------------ @@ -18,16 +18,19 @@ RETURN VALUE On success, an array of objects is returned. Each object contains: *bolt11* -the *bolt11* argument given to *pay* (see below for exceptions). +the *bolt11* invoice if provided to `pay`. + + *payment_hash* +the *payment_hash* of the payment. *status* one of *complete*, *failed* or *pending*. *payment\_preimage* -(if *status* is *complete*) proves payment was received. +if *status* is *complete*. *label* -optional *label*, if provided to *pay*. +optional *label*, if provided to *pay* or *sendonion*. *amount\_sent\_msat* total amount sent, in "NNNmsat" format. diff --git a/plugins/pay.c b/plugins/pay.c index 883ddd693777..1d63d99a3214 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -1736,7 +1736,10 @@ static void add_new_entry(struct json_stream *ret, const struct pay_mpp *pm) { json_object_start(ret, NULL); - json_add_string(ret, "bolt11", pm->b11); + if (pm->b11) + json_add_string(ret, "bolt11", pm->b11); + + json_add_sha256(ret, "payment_hash", pm->payment_hash); json_add_string(ret, "status", pm->status); json_add_u32(ret, "created_at", pm->timestamp); @@ -1854,11 +1857,13 @@ static struct command_result *json_listpays(struct command *cmd, const jsmntok_t *params) { const char *b11str; + struct sha256 *payment_hash; struct out_req *req; /* FIXME: would be nice to parse as a bolt11 so check worked in future */ if (!param(cmd, buf, params, p_opt("bolt11", param_string, &b11str), + p_opt("payment_hash", param_sha256, &payment_hash), NULL)) return command_param_failed(); @@ -1867,6 +1872,9 @@ static struct command_result *json_listpays(struct command *cmd, cast_const(char *, b11str)); if (b11str) json_add_string(req->js, "bolt11", b11str); + + if (payment_hash) + json_add_sha256(req->js, "payment_hash", payment_hash); return send_outreq(cmd->plugin, req); } @@ -2054,7 +2062,7 @@ static const struct plugin_command commands[] = { }, { "listpays", "payment", - "List result of payment {bolt11}, or all", + "List result of payment {bolt11} or {payment_hash}, or all", "Covers old payments (failed and succeeded) and current ones.", json_listpays }, diff --git a/tests/test_pay.py b/tests/test_pay.py index 7c59dd345a52..5872f2fe4bbf 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3250,3 +3250,27 @@ def test_mpp_presplit_routehint_conflict(node_factory, bitcoind): inv = l3.rpc.invoice(Millisatoshi(2 * 10000 * 1000), 'i', 'i', exposeprivatechannels=True)['bolt11'] l1.rpc.pay(inv) + + +def test_listpay_result_with_paymod(node_factory, bitcoind): + """ + The object of this test is to verify the correct behavior + of the RPC command listpay e with two different type of + payment, such as: keysend (without invoice) and pay (with invoice). + l1 -> keysend -> l2 + l2 -> pay invoice -> l3 + """ + + amount_sat = 10 ** 6 + + l1, l2, l3 = node_factory.line_graph(3) + + invl2 = l2.rpc.invoice(amount_sat * 2, "inv_l2", "inv_l2") + l1.rpc.pay(invl2['bolt11']) + + l2.rpc.keysend(l3.info['id'], amount_sat * 2, "keysend_l3") + + assert 'bolt11' in l1.rpc.listpays()['pays'][0] + assert 'payment_hash' in l2.rpc.listpays()['pays'][0] + assert 'payment_hash' in l1.rpc.listpays()['pays'][0] + assert 'bolt11' not in l2.rpc.listpays()['pays'][0] From 9a3073a53886d54c7ddffc6a70353e7d0053a0fe Mon Sep 17 00:00:00 2001 From: Vincenzo Palazzo Date: Fri, 31 Jul 2020 17:19:22 +0200 Subject: [PATCH 05/14] listpays mod 1: add destination inside the response when bolt11 is null Changelog-Added: JSON-RPC: `listpays` now lists the `destination` if it was provided (e.g., via the `pay` plugin or `keysend` plugin) --- doc/lightning-sendonion.7 | 6 +++++- doc/lightning-sendonion.7.md | 5 ++++- lightningd/pay.c | 4 +++- plugins/libplugin-pay.c | 3 +++ plugins/pay.c | 15 ++++++++++++++- tests/test_pay.py | 4 +++- 6 files changed, 32 insertions(+), 5 deletions(-) diff --git a/doc/lightning-sendonion.7 b/doc/lightning-sendonion.7 index 260921fee390..0206ea52f84c 100644 --- a/doc/lightning-sendonion.7 +++ b/doc/lightning-sendonion.7 @@ -3,7 +3,8 @@ lightning-sendonion - Send a payment with a custom onion packet .SH SYNOPSIS -\fBsendonion\fR \fIonion\fR \fIfirst_hop\fR \fIpayment_hash\fR [\fIlabel\fR] [\fIshared_secrets\fR] [\fIpartid\fR] [\fIbolt11\fR] [\fImsatoshi\fR] +\fBsendonion\fR \fIonion\fR \fIfirst_hop\fR \fIpayment_hash\fR [\fIlabel\fR] [\fIshared_secrets\fR] [\fIpartid\fR] [\fIbolt11\fR] +[\fImsatoshi\fR] [\fIdestination\fR] .SH DESCRIPTION @@ -90,6 +91,9 @@ The \fIbolt11\fR parameter, if provided, will be returned in \fIwaitsendpay\fR and \fIlistsendpays\fR results\. +The \fIdestination\fR parameter, if provided, will be returned in \fBlistpays\fR result\. + + The \fImsatoshi\fR parameter is used to annotate the payment, and is returned by \fIwaitsendpay\fR and \fIlistsendpays\fR\. diff --git a/doc/lightning-sendonion.7.md b/doc/lightning-sendonion.7.md index cde69a1dbe19..0b4e18f423b2 100644 --- a/doc/lightning-sendonion.7.md +++ b/doc/lightning-sendonion.7.md @@ -4,7 +4,8 @@ lightning-sendonion -- Send a payment with a custom onion packet SYNOPSIS -------- -**sendonion** *onion* *first_hop* *payment_hash* \[*label*\] \[*shared_secrets*\] \[*partid*\] \[*bolt11*\] \[*msatoshi*\] +**sendonion** *onion* *first_hop* *payment_hash* \[*label*\] \[*shared_secrets*\] \[*partid*\] \[*bolt11*\] +\[*msatoshi*\] \[*destination*\] DESCRIPTION ----------- @@ -78,6 +79,8 @@ partial payments with the same *payment_hash*. The *bolt11* parameter, if provided, will be returned in *waitsendpay* and *listsendpays* results. +The *destination* parameter, if provided, will be returned in **listpays** result. + The *msatoshi* parameter is used to annotate the payment, and is returned by *waitsendpay* and *listsendpays*. diff --git a/lightningd/pay.c b/lightningd/pay.c index f9dc40f5715e..d6a013d51766 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -1178,6 +1178,7 @@ static struct command_result *json_sendonion(struct command *cmd, struct sha256 *payment_hash; struct lightningd *ld = cmd->ld; const char *label, *b11str; + struct node_id *destination; struct secret *path_secrets; struct amount_msat *msat; u64 *partid; @@ -1191,6 +1192,7 @@ static struct command_result *json_sendonion(struct command *cmd, p_opt_def("partid", param_u64, &partid, 0), p_opt("bolt11", param_string, &b11str), p_opt_def("msatoshi", param_msat, &msat, AMOUNT_MSAT(0)), + p_opt("destination", param_node_id, &destination), NULL)) return command_param_failed(); @@ -1204,7 +1206,7 @@ static struct command_result *json_sendonion(struct command *cmd, return send_payment_core(ld, cmd, payment_hash, *partid, first_hop, *msat, AMOUNT_MSAT(0), - label, b11str, &packet, NULL, NULL, NULL, + label, b11str, &packet, destination, NULL, NULL, path_secrets); } diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 965fff5cf4e3..d279afea5461 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1045,6 +1045,9 @@ static struct command_result *payment_createonion_success(struct command *cmd, if (p->bolt11) json_add_string(req->js, "bolt11", p->bolt11); + if (p->destination) + json_add_node_id(req->js, "destination", p->destination); + send_outreq(p->plugin, req); return command_still_pending(cmd); } diff --git a/plugins/pay.c b/plugins/pay.c index 1d63d99a3214..3832e8900aa1 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -1669,6 +1669,9 @@ struct pay_mpp { /* Timestamp of the first part */ u32 timestamp; + + /* The destination of the payment, if specified. */ + struct node_id *destination; }; static const struct sha256 *pay_mpp_key(const struct pay_mpp *pm) @@ -1739,6 +1742,9 @@ static void add_new_entry(struct json_stream *ret, if (pm->b11) json_add_string(ret, "bolt11", pm->b11); + if (pm->destination) + json_add_node_id(ret, "destination", pm->destination); + json_add_sha256(ret, "payment_hash", pm->payment_hash); json_add_string(ret, "status", pm->status); json_add_u32(ret, "created_at", pm->timestamp); @@ -1785,13 +1791,15 @@ static struct command_result *listsendpays_done(struct command *cmd, ret = jsonrpc_stream_success(cmd); json_array_start(ret, "pays"); json_for_each_arr(i, t, arr) { - const jsmntok_t *status, *b11tok, *hashtok, *createdtok; + const jsmntok_t *status, *b11tok, *hashtok, *destinationtok, *createdtok; const char *b11 = b11str; struct sha256 payment_hash; + struct node_id destination; u32 created_at; b11tok = json_get_member(buf, t, "bolt11"); hashtok = json_get_member(buf, t, "payment_hash"); + destinationtok = json_get_member(buf, t, "destination"); createdtok = json_get_member(buf, t, "created_at"); assert(hashtok != NULL); assert(createdtok != NULL); @@ -1801,11 +1809,15 @@ static struct command_result *listsendpays_done(struct command *cmd, if (b11tok) b11 = json_strdup(cmd, buf, b11tok); + if (destinationtok) + json_to_node_id(buf, destinationtok, &destination); + pm = pay_map_get(&pay_map, &payment_hash); if (!pm) { pm = tal(cmd, struct pay_mpp); pm->payment_hash = tal_dup(pm, struct sha256, &payment_hash); pm->b11 = tal_steal(pm, b11); + pm->destination = tal_dup(pm,struct node_id, &destination); pm->label = json_get_member(buf, t, "label"); pm->preimage = NULL; pm->amount_sent = AMOUNT_MSAT(0); @@ -1875,6 +1887,7 @@ static struct command_result *json_listpays(struct command *cmd, if (payment_hash) json_add_sha256(req->js, "payment_hash", payment_hash); + return send_outreq(cmd->plugin, req); } diff --git a/tests/test_pay.py b/tests/test_pay.py index 5872f2fe4bbf..2740eb13e08d 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3271,6 +3271,8 @@ def test_listpay_result_with_paymod(node_factory, bitcoind): l2.rpc.keysend(l3.info['id'], amount_sat * 2, "keysend_l3") assert 'bolt11' in l1.rpc.listpays()['pays'][0] + assert 'bolt11' not in l2.rpc.listpays()['pays'][0] assert 'payment_hash' in l2.rpc.listpays()['pays'][0] assert 'payment_hash' in l1.rpc.listpays()['pays'][0] - assert 'bolt11' not in l2.rpc.listpays()['pays'][0] + assert 'destination' in l1.rpc.listpays()['pays'][0] + assert 'destination' in l2.rpc.listpays()['pays'][0] From edf7174aa04a7e3ca18a28961f7b651d719463c7 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 11 Aug 2020 15:23:33 +0200 Subject: [PATCH 06/14] pytest: Reproduce #3915 --- plugins/libplugin-pay.c | 5 +++++ tests/test_pay.py | 42 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index d279afea5461..3add240382ca 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -2624,6 +2624,11 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) struct payment *c = payment_new(p, NULL, p, p->modifiers); + /* Annotate the subpayments with the bolt11 string, + * they'll be used when aggregating the payments + * again. */ + c->bolt11 = tal_strdup(c, p->bolt11); + /* Get ~ target, but don't exceed amt */ c->amount = fuzzed_near(target, amt); diff --git a/tests/test_pay.py b/tests/test_pay.py index 2740eb13e08d..8bfa9d97e802 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -6,7 +6,7 @@ from pyln.proto.onion import TlvPayload from utils import ( DEVELOPER, wait_for, only_one, sync_blockheight, SLOW_MACHINE, TIMEOUT, - VALGRIND, EXPERIMENTAL_FEATURES + VALGRIND, EXPERIMENTAL_FEATURES, env ) import copy import os @@ -3276,3 +3276,43 @@ def test_listpay_result_with_paymod(node_factory, bitcoind): assert 'payment_hash' in l1.rpc.listpays()['pays'][0] assert 'destination' in l1.rpc.listpays()['pays'][0] assert 'destination' in l2.rpc.listpays()['pays'][0] + + +@unittest.skipIf(env('COMPAT') != 1, "legacypay requires COMPAT=1") +def test_listpays_ongoing_attempt(node_factory, bitcoind, executor): + """Test to reproduce issue #3915. + + The issue is that the bolt11 string is not initialized if the root payment + was split (no attempt with the bolt11 annotation ever hit `lightningd`, + hence we cannot filter by that. In addition keysends never have a bolt11 + string, so we need to switch to payment_hash comparisons anyway. + """ + plugin = os.path.join(os.path.dirname(__file__), 'plugins', 'hold_htlcs.py') + l1, l2, l3 = node_factory.line_graph(3, opts=[{}, {}, {'plugin': plugin}], + wait_for_announce=True) + + f = executor.submit(l1.rpc.keysend, l3.info['id'], 100) + l3.daemon.wait_for_log(r'Holding onto an incoming htlc') + l1.rpc.listpays() + f.result() + + inv = l2.rpc.invoice(10**6, 'legacy', 'desc')['bolt11'] + l1.rpc.legacypay(inv) + l1.rpc.listpays() + + # Produce loads of parts to increase probability of hitting the issue, + # should result in 100 splits at least + inv = l3.rpc.invoice(10**9, 'mpp invoice', 'desc')['bolt11'] + + # Start the payment, it'll get stuck for 10 seconds at l3 + executor.submit(l1.rpc.pay, inv) + l1.daemon.wait_for_log(r'Split into [0-9]+ sub-payments due to initial size') + l3.daemon.wait_for_log(r'Holding onto an incoming htlc') + + # While that is going on, check in with `listpays` to see if aggregation + # is working. + l1.rpc.listpays() + + # Now restart and see if we still can aggregate things correctly. + l1.restart() + l1.rpc.listpays() From 0b2e996c2999d1e5e175edc8a1b142931bc3addc Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 11 Aug 2020 16:41:50 +0200 Subject: [PATCH 07/14] pay: Inherit payment label to all children --- plugins/libplugin-pay.c | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 3add240382ca..d303a890e2b5 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -36,6 +36,7 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd, tal_arr_expand(&parent->children, p); p->destination = parent->destination; p->amount = parent->amount; + p->label = parent->label; p->payment_hash = parent->payment_hash; p->partid = payment_root(p->parent)->next_partid++; p->plugin = parent->plugin; From 19bcb4b8c1973e8d2f0d85b1c12e9d139273304e Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 11 Aug 2020 16:42:20 +0200 Subject: [PATCH 08/14] pay: Group payments by their payment_hash, not by bolt11 string This is quicker and guaranteed to work even for payments that were not initiated via an invoice. --- plugins/pay.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/plugins/pay.c b/plugins/pay.c index 3832e8900aa1..37c6f42d8b8e 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -80,6 +81,8 @@ struct pay_status { /* Array of payment attempts. */ struct pay_attempt *attempts; + + struct sha256 payment_hash; }; struct pay_command { @@ -1266,6 +1269,8 @@ static struct pay_status *add_pay_status(struct pay_command *pc, ps->shadow = NULL; ps->exclusions = NULL; ps->attempts = tal_arr(ps, struct pay_attempt, 0); + hex_decode(pc->payment_hash, strlen(pc->payment_hash), + &ps->payment_hash, sizeof(ps->payment_hash)); list_add_tail(&pay_status, &ps->list); return ps; @@ -1618,7 +1623,7 @@ static struct command_result *json_paystatus(struct command *cmd, return command_finished(cmd, ret); } -static bool attempt_ongoing(const char *b11) +static bool attempt_ongoing(const struct sha256 *payment_hash) { struct pay_status *ps; struct payment *root; @@ -1628,14 +1633,14 @@ static bool attempt_ongoing(const char *b11) final_states = PAYMENT_STEP_FAILED | PAYMENT_STEP_SUCCESS; list_for_each(&pay_status, ps, list) { - if (!streq(b11, ps->bolt11)) + if (!sha256_eq(payment_hash, &ps->payment_hash)) continue; attempt = &ps->attempts[tal_count(ps->attempts)-1]; return attempt->result == NULL && attempt->failure == NULL; } list_for_each(&payments, root, list) { - if (root->bolt11 == NULL || !streq(b11, root->bolt11)) + if (!sha256_eq(payment_hash, root->payment_hash)) continue; res = payment_collect_result(root); diff = res.leafstates & ~final_states; @@ -1842,7 +1847,7 @@ static struct command_result *listsendpays_done(struct command *cmd, if (!pm->status || !streq(pm->status, "complete")) pm->status = "pending"; } else { - if (attempt_ongoing(pm->b11)) { + if (attempt_ongoing(pm->payment_hash)) { /* Failed -> pending; don't downgrade success. */ if (!pm->status || !streq(pm->status, "complete")) From 9b5977f8e8539ea6fb0a70dcb42c1eaf96da69b8 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Sun, 9 Aug 2020 07:56:27 +0800 Subject: [PATCH 09/14] tests/test_pay.py: Provide test showing that blockheight disagreement causes us to advance routehints too aggressively. The worst effect is that unpublished nodes are harder to pay, but even published ones make us do unnecessary work, since we are losing routehints from the published ones that could help us actually route better to them. --- tests/test_pay.py | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/test_pay.py b/tests/test_pay.py index 8bfa9d97e802..7eb24dc0b001 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3316,3 +3316,52 @@ def test_listpays_ongoing_attempt(node_factory, bitcoind, executor): # Now restart and see if we still can aggregate things correctly. l1.restart() l1.rpc.listpays() + + +@pytest.mark.xfail(strict=True) +@unittest.skipIf(not DEVELOPER, "needs use_shadow") +def test_mpp_waitblockheight_routehint_conflict(node_factory, bitcoind, executor): + ''' + We have a bug where a blockheight disagreement between us and + the receiver causes us to advance through the routehints a bit + too aggressively. + ''' + l1, l2, l3 = node_factory.get_nodes(3) + + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l1l2 = l1.fund_channel(l2, 10**7, announce_channel=True) + l2.rpc.connect(l3.info['id'], 'localhost', l3.port) + l2.fund_channel(l3, 10**7, announce_channel=False) + + bitcoind.generate_block(6) + sync_blockheight(bitcoind, [l1, l2, l3]) + + # Wait for l3 to learn about l1->l2, otherwise it will think + # l2 is a deadend and not add it to the routehint. + wait_for(lambda: len(l3.rpc.listchannels(l1l2)['channels']) >= 2) + + # Now make the l1 payer stop receiving blocks. + def no_more_blocks(req): + return {"result": None, + "error": {"code": -8, "message": "Block height out of range"}, "id": req['id']} + l1.daemon.rpcproxy.mock_rpc('getblockhash', no_more_blocks) + + # Increase blockheight by 2, like in test_blockheight_disagreement. + bitcoind.generate_block(2) + sync_blockheight(bitcoind, [l3]) + + inv = l3.rpc.invoice(Millisatoshi(2 * 10000 * 1000), 'i', 'i', exposeprivatechannels=True)['bolt11'] + + # Have l1 pay l3 + def pay(l1, inv): + l1.rpc.dev_pay(inv, use_shadow=False) + fut = executor.submit(pay, l1, inv) + + # Make sure l1 sends out the HTLC. + l1.daemon.wait_for_logs([r'NEW:: HTLC LOCAL']) + + # Unblock l1 from new blocks. + l1.daemon.rpcproxy.mock_rpc('getblockhash', None) + + # pay command should complete without error + fut.result(TIMEOUT) From fa64750b400b2207b7508d2bcc31c8d429253ea5 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Sun, 9 Aug 2020 15:19:21 +0800 Subject: [PATCH 10/14] plugins/libplugin-pay.c: Be less aggressive with advancing through routehints. Only advance through routehints if no route was found at all, or if the estimated capacity at the routehint is lower than the amount that we have to send through the routehint. Changelog-Fixed: pay: Be less aggressive with forgetting routehints. --- plugins/libplugin-pay.c | 56 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index d303a890e2b5..9d5431c5f9a7 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -384,6 +384,7 @@ static struct command_result *payment_getroute_result(struct command *cmd, /* Ensure that our fee and CLTV budgets are respected. */ if (amount_msat_greater(fee, p->constraints.fee_budget)) { payment_exclude_most_expensive(p); + p->route = tal_free(p->route); payment_fail( p, "Fee exceeds our fee budget: %s > %s, discarding route", type_to_string(tmpctx, struct amount_msat, &fee), @@ -393,9 +394,11 @@ static struct command_result *payment_getroute_result(struct command *cmd, } if (p->route[0].delay > p->constraints.cltv_budget) { + u32 delay = p->route[0].delay; payment_exclude_longest_delay(p); + p->route = tal_free(p->route); payment_fail(p, "CLTV delay exceeds our CLTV budget: %d > %d", - p->route[0].delay, p->constraints.cltv_budget); + delay, p->constraints.cltv_budget); return command_still_pending(cmd); } @@ -1764,12 +1767,17 @@ static struct route_info **filter_routehints(struct routehints_data *d, return tal_steal(d, hints); } +static bool route_msatoshi(struct amount_msat *total, + const struct amount_msat msat, + const struct route_info *route, size_t num_route); + static bool routehint_excluded(struct payment *p, const struct route_info *routehint) { const struct node_id *nodes = payment_get_excluded_nodes(tmpctx, p); const struct short_channel_id_dir *chans = payment_get_excluded_channels(tmpctx, p); + const struct channel_hint *hints = payment_root(p)->channel_hints; /* Note that we ignore direction here: in theory, we could have * found that one direction of a channel is unavailable, but they @@ -1783,6 +1791,41 @@ static bool routehint_excluded(struct payment *p, for (size_t j = 0; j < tal_count(chans); j++) if (short_channel_id_eq(&chans[j].scid, &r->short_channel_id)) return true; + + /* Skip the capacity check if this is the last hop + * in the routehint. + * The last hop in the routehint delivers the exact + * final amount to the destination, which + * payment_get_excluded_channels uses for excluding + * already. + * Thus, the capacity check below only really matters + * for multi-hop routehints. + */ + if (i == tal_count(routehint) - 1) + continue; + + /* Check our capacity fits. */ + struct amount_msat needed_capacity; + if (!route_msatoshi(&needed_capacity, p->amount, + r + 1, tal_count(routehint) - i - 1)) + return true; + /* Why do we scan the hints again if + * payment_get_excluded_channels already does? + * Because payment_get_excluded_channels checks the + * amount at destination, but we know that we are + * a specific distance from the destination and we + * know the exact capacity we need to send via this + * channel, which is greater than the destination. + */ + for (size_t j = 0; j < tal_count(hints); j++) { + if (!short_channel_id_eq(&hints[j].scid.scid, &r->short_channel_id)) + continue; + /* We exclude on equality because we set the estimate + * to the smallest failed attempt. */ + if (amount_msat_greater_eq(needed_capacity, + hints[j].estimated_capacity)) + return true; + } } return false; } @@ -2007,9 +2050,14 @@ static struct routehints_data *routehint_data_init(struct payment *p) pd = payment_mod_routehints_get_data(payment_root(p)); d->destination_reachable = pd->destination_reachable; d->routehints = pd->routehints; - if (p->parent->step == PAYMENT_STEP_RETRY) - d->offset = pd->offset + 1; - else + pd = payment_mod_routehints_get_data(p->parent); + if (p->parent->step == PAYMENT_STEP_RETRY) { + d->offset = pd->offset; + /* If the previous try failed to route, advance + * to the next routehint. */ + if (!p->parent->route) + ++d->offset; + } else d->offset = 0; return d; } else { From 6ae412ee6c884cd881f40c012dfce80c9345a1f4 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sun, 9 Aug 2020 15:26:46 +0200 Subject: [PATCH 11/14] pay: Fix final TLV payload if not going through MPP modifiers Reported-by: ZmnSCPxj Signed-off-by: Christian Decker <@cdecker> Changelog-Fixed: pay: Correct a case where we put the sub-payment value instead of the *total* value in the `total_msat` field of a multi-part payment. --- plugins/libplugin-pay.c | 6 ++++-- tests/test_pay.py | 1 - 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 9d5431c5f9a7..7a5dfa4aea1b 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1080,6 +1080,7 @@ static void payment_add_hop_onion_payload(struct payment *p, u32 cltv = p->start_block + next->delay + 1; u64 msat = next->amount.millisatoshis; /* Raw: TLV payload generation*/ struct tlv_field **fields; + struct payment *root = payment_root(p); static struct short_channel_id all_zero_scid = {.u64 = 0}; /* This is the information of the node processing this payload, while @@ -1115,8 +1116,9 @@ static void payment_add_hop_onion_payload(struct payment *p, if (payment_secret != NULL) { assert(final); - tlvstream_set_tlv_payload_data(fields, payment_secret, - msat); + tlvstream_set_tlv_payload_data( + fields, payment_secret, + root->amount.millisatoshis); /* Raw: TLV payload generation*/ } break; } diff --git a/tests/test_pay.py b/tests/test_pay.py index 7eb24dc0b001..9d9c2ca9030f 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3318,7 +3318,6 @@ def test_listpays_ongoing_attempt(node_factory, bitcoind, executor): l1.rpc.listpays() -@pytest.mark.xfail(strict=True) @unittest.skipIf(not DEVELOPER, "needs use_shadow") def test_mpp_waitblockheight_routehint_conflict(node_factory, bitcoind, executor): ''' From c93a0a5eed62adbfd49dbb852fcd3635ca53eeaf Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Mon, 10 Aug 2020 15:25:41 +0800 Subject: [PATCH 12/14] plugins/libplugin-pay.c: Make sure blockheight disagreement does not prevent all future progress. Blockheight disagreement is signalled with a permanent failure at the end node, but is actually a transient failure. --- plugins/libplugin-pay.c | 65 +++++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 7a5dfa4aea1b..817d5ea80fc8 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -723,12 +723,50 @@ static void report_tampering(struct payment *p, } } +static bool +failure_is_blockheight_disagreement(const struct payment *p, + u32 *blockheight) +{ + struct amount_msat unused; + + assert(p && p->result); + + if (p->result->failcode == 17 /* Former final_expiry_too_soon */) + *blockheight = p->start_block + 1; + else if (!fromwire_incorrect_or_unknown_payment_details( + p->result->raw_message, + &unused, blockheight)) + /* If it's incorrect_or_unknown_payment_details, that tells us + * what height they're at */ + return false; + + /* If we are already at the desired blockheight there is no point in + * waiting, and it is likely just some other error. Notice that + * start_block gets set by the initial getinfo call for each + * attempt.*/ + if (*blockheight <= p->start_block) + return false; + + return true; +} + static struct command_result * handle_final_failure(struct command *cmd, struct payment *p, const struct node_id *final_id, enum onion_type failcode) { + u32 unused; + + /* Need to check for blockheight disagreement case here, + * otherwise we would set the abort flag too eagerly. + */ + if (failure_is_blockheight_disagreement(p, &unused)) { + plugin_log(p->plugin, LOG_DBG, + "Blockheight disagreement, not aborting."); + goto nonerror; + } + /* We use an exhaustive switch statement here so you get a compile * warning when new ones are added, and can think about where they go */ switch (failcode) { @@ -807,8 +845,10 @@ handle_final_failure(struct command *cmd, p->result->code = PAY_DESTINATION_PERM_FAIL; payment_root(p)->abort = true; +nonerror: payment_fail(p, "%s", p->result->message); return command_still_pending(cmd); + } @@ -2471,9 +2511,7 @@ static void waitblockheight_cb(void *d, struct payment *p) struct out_req *req; struct timeabs now = time_now(); struct timerel remaining; - u32 blockheight = p->start_block; - int failcode; - const u8 *raw_message; + u32 blockheight; if (p->step != PAYMENT_STEP_FAILED) return payment_continue(p); @@ -2484,27 +2522,10 @@ static void waitblockheight_cb(void *d, struct payment *p) if (time_after(now, p->deadline)) return payment_continue(p); - failcode = p->result->failcode; - raw_message = p->result->raw_message; remaining = time_between(p->deadline, now); - if (failcode == 17 /* Former final_expiry_too_soon */) { - blockheight = p->start_block + 1; - } else { - /* If it's incorrect_or_unknown_payment_details, that tells us - * what height they're at */ - struct amount_msat unused; - const void *ptr = raw_message; - if (!fromwire_incorrect_or_unknown_payment_details( - ptr, &unused, &blockheight)) - return payment_continue(p); - } - - /* If we are already at the desired blockheight there is no point in - * waiting, and it is likely just some other error. Notice that - * start_block gets set by the initial getinfo call for each - * attempt.*/ - if (blockheight <= p->start_block) + /* *Was* it a blockheight disagreement that caused the failure? */ + if (!failure_is_blockheight_disagreement(p, &blockheight)) return payment_continue(p); plugin_log(p->plugin, LOG_INFORM, From 9d4ba426b362627492947289c9de3f0752fbab28 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 12 Aug 2020 17:19:43 +0200 Subject: [PATCH 13/14] repro: Allow dashes in the version number --- Makefile | 2 +- tools/repro-build.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index fed668a80cf2..ce2008d6187b 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ #! /usr/bin/make # Extract version from git, or if we're from a zipfile, use dirname -VERSION=$(shell git describe --always --dirty=-modded --abbrev=7 2>/dev/null || pwd | sed -n 's|.*/c\{0,1\}lightning-v\{0,1\}\([0-9a-f.rc]*\)$$|\1|gp') +VERSION=$(shell git describe --always --dirty=-modded --abbrev=7 2>/dev/null || pwd | sed -n 's|.*/c\{0,1\}lightning-v\{0,1\}\([0-9a-f.rc\-]*\)$$|\1|gp') ifeq ($(VERSION),) $(error "ERROR: git is required for generating version information") diff --git a/tools/repro-build.sh b/tools/repro-build.sh index f7fadf7a4fc2..6e6fd98e115f 100755 --- a/tools/repro-build.sh +++ b/tools/repro-build.sh @@ -51,7 +51,7 @@ else fi PLATFORM="$OS"-"$VER" -VERSION=$(git describe --always --dirty=-modded --abbrev=7 2>/dev/null || pwd | sed -n 's,.*/clightning-\(v[0-9.rc]*\)$,\1,p') +VERSION=$(git describe --always --dirty=-modded --abbrev=7 2>/dev/null || pwd | sed -n 's,.*/clightning-\(v[0-9.rc\-]*\)$,\1,p') # eg. ## [0.6.3] - 2019-01-09: "The Smallblock Conspiracy" # Skip 'v' here in $VERSION From c42ddcd70683f91814d4fe25545fe5da44171bc8 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 10 Aug 2020 22:17:48 +0200 Subject: [PATCH 14/14] release: Add changelog for hotfix release v0.9.0.1 --- CHANGELOG.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d2b3cb18ecf..97570a54378c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,38 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.9.0-1] - 2020-08-12: "(Still) Rat Poison Squared on Steroids" + +This is a point release, mainly including some omissions in the `listpays` result, and a fix for routehints from invoices being skipped for payments larger than 10,000 satoshis. + +### Added + + - JSON-RPC: `listpays` now lists the `destination` if it was provided (e.g., via the `pay` plugin or `keysend` plugin) ([None](https://github.com/ElementsProject/lightning/pull/None)) + - JSON-RPC: `listpays` can be used to query payments using the `payment_hash` ([3888](https://github.com/ElementsProject/lightning/pull/3888)) + - JSON-RPC: `listpays` now includes the `payment_hash` ([3888](https://github.com/ElementsProject/lightning/pull/3888)) + - JSON-RPC: The result returned by `listpays` now includes the timestamp of the first part of the payment ([3909](https://github.com/ElementsProject/lightning/pull/3909)) + +### Changed + + +### Deprecated + +Note: You should always set `allow-deprecated-apis=false` to test for changes. + + +### Removed + + +### Fixed + + - pay: Fixed a bug where routehints would be ignored if the payment exceeded 10,000 satoshi. This is particularly bad if the payee is only reachable via routehints in an invoice. ([3908](https://github.com/ElementsProject/lightning/pull/3908)) + - pay: Fixed a crash of the `pay` plugin if the presplit modifier skipped the root payment. ([3927](https://github.com/ElementsProject/lightning/pull/3927)) + - pay: Correct a case where we put the sub-payment value instead of the *total* value in the `total_msat` field of a multi-part payment. + - pay: Be less aggressive with forgetting routehints. + + +### Security + ## [0.9.0] - 2020-07-31: "Rat Poison Squared on Steroids" @@ -785,6 +817,7 @@ There predate the BOLT specifications, and are only of vague historic interest: 6. [0.5.1] - 2016-10-21 7. [0.5.2] - 2016-11-21: "Bitcoin Savings & Trust Daily Interest II" +[0.9.0-1]: https://github.com/ElementsProject/lightning/releases/tag/v0.9.0-1 [0.9.0]: https://github.com/ElementsProject/lightning/releases/tag/v0.9.0 [0.8.2]: https://github.com/ElementsProject/lightning/releases/tag/v0.8.2 [0.8.1]: https://github.com/ElementsProject/lightning/releases/tag/v0.8.1