From 191355e0e7e88fa9d67de1dd904920688738b9c7 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Sep 2020 15:09:04 +0930 Subject: [PATCH] pay: fix handling of legacy vs tlv encoding. As revealed by the failure of tests in #3936, where we ended up trying to send a partial payment using legacy style, we are not handling style properly. 1. BOLT9 has features, so we can *know* that the destination supports MPP. We may not have seen a node_announcement. 2. We can't assume that nodes inside routehints support TLV. 3. We can't assume direct peers support TLV. The keysend code tried to fix this up, so I'm not sure that this caused the issue in #3968, though. Signed-off-by: Rusty Russell Changelog-Fixed: `pay` will now make reliable multi-part payments to nodes it doesn't have a node_announcement for. --- plugins/keysend.c | 10 +--------- plugins/libplugin-pay.c | 16 +++++++++++----- plugins/libplugin-pay.h | 2 ++ plugins/pay.c | 1 + 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/plugins/keysend.c b/plugins/keysend.c index 40510ecc2..617788271 100644 --- a/plugins/keysend.c +++ b/plugins/keysend.c @@ -52,18 +52,9 @@ static struct keysend_data *keysend_init(struct payment *p) } static void keysend_cb(struct keysend_data *d, struct payment *p) { - struct route_hop *last_hop; struct createonion_hop *last_payload; size_t hopcount; - if (p->step == PAYMENT_STEP_GOT_ROUTE) { - /* Force the last step to be a TLV, we might not have an - * announcement and it still supports it. Required later when - * we adjust the payload. */ - last_hop = &p->route[tal_count(p->route) - 1]; - last_hop->style = ROUTE_HOP_TLV; - } - if (p->step != PAYMENT_STEP_ONION_PAYLOAD) return payment_continue(p); @@ -143,6 +134,7 @@ static struct command_result *json_keysend(struct command *cmd, const char *buf, p->json_buffer = tal_steal(p, buf); p->json_toks = params; p->destination = tal_steal(p, destination); + p->destination_has_tlv = true; p->payment_secret = NULL; p->amount = *msat; p->invoice = NULL; diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 5fee19ec9..f7fb6f35c 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -45,6 +45,7 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd, assert(cmd == NULL); tal_arr_expand(&parent->children, p); p->destination = parent->destination; + p->destination_has_tlv = parent->destination_has_tlv; p->amount = parent->amount; p->label = parent->label; p->payment_hash = parent->payment_hash; @@ -1266,6 +1267,7 @@ static void payment_add_hop_onion_payload(struct payment *p, struct route_hop *node, struct route_hop *next, bool final, + bool force_tlv, struct secret *payment_secret) { struct createonion_request *cr = p->createonion_request; @@ -1279,9 +1281,11 @@ static void payment_add_hop_onion_payload(struct payment *p, * `next` are the instructions to include in the payload, which is * basically the channel going to the next node. */ dst->style = node->style; + if (force_tlv) + dst->style = ROUTE_HOP_TLV; dst->pubkey = node->nodeid; - switch (node->style) { + switch (dst->style) { case ROUTE_HOP_LEGACY: dst->legacy_payload = tal(cr->hops, struct legacy_payload); dst->legacy_payload->forward_amt = next->amount; @@ -1338,7 +1342,8 @@ static void payment_compute_onion_payloads(struct payment *p) /* The message is destined for hop i, but contains fields for * i+1 */ payment_add_hop_onion_payload(p, &cr->hops[i], &p->route[i], - &p->route[i + 1], false, NULL); + &p->route[i + 1], false, false, + NULL); tal_append_fmt(&routetxt, "%s -> ", type_to_string(tmpctx, struct short_channel_id, &p->route[i].channel_id)); @@ -1347,7 +1352,8 @@ static void payment_compute_onion_payloads(struct payment *p) /* Final hop */ payment_add_hop_onion_payload( p, &cr->hops[hopcount - 1], &p->route[hopcount - 1], - &p->route[hopcount - 1], true, root->payment_secret); + &p->route[hopcount - 1], true, root->destination_has_tlv, + root->payment_secret); tal_append_fmt(&routetxt, "%s", type_to_string(tmpctx, struct short_channel_id, &p->route[hopcount - 1].channel_id)); @@ -2281,7 +2287,7 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p) } hop.nodeid = *route_pubkey(p, routehint, i + 1); - hop.style = ROUTE_HOP_TLV; + hop.style = ROUTE_HOP_LEGACY; hop.channel_id = routehint[i].short_channel_id; hop.amount = dest_amount; hop.delay = route_cltv(d->final_cltv, routehint + i + 1, @@ -2665,7 +2671,7 @@ static void direct_pay_override(struct payment *p) { p->route[0].channel_id = hint->scid.scid; p->route[0].direction = hint->scid.dir; p->route[0].nodeid = *p->destination; - p->route[0].style = ROUTE_HOP_TLV; + p->route[0].style = p->destination_has_tlv ? ROUTE_HOP_TLV : ROUTE_HOP_LEGACY; paymod_log(p, LOG_DBG, "Found a direct channel (%s) with sufficient " "capacity, skipping route computation.", diff --git a/plugins/libplugin-pay.h b/plugins/libplugin-pay.h index 4fc270dd9..534b18808 100644 --- a/plugins/libplugin-pay.h +++ b/plugins/libplugin-pay.h @@ -169,6 +169,8 @@ struct payment { /* Real destination we want to route to */ struct node_id *destination; + /* Do we know for sure that this supports OPT_VAR_ONION? */ + bool destination_has_tlv; /* Payment hash extracted from the invoice if any. */ struct sha256 *payment_hash; diff --git a/plugins/pay.c b/plugins/pay.c index 6f5d7c249..bfda620d6 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -2021,6 +2021,7 @@ static struct command_result *json_paymod(struct command *cmd, p->json_buffer = tal_steal(p, buf); p->json_toks = params; p->destination = &b11->receiver_id; + p->destination_has_tlv = feature_offered(b11->features, OPT_VAR_ONION); p->payment_hash = tal_dup(p, struct sha256, &b11->payment_hash); p->payment_secret = b11->payment_secret ? tal_dup(p, struct secret, b11->payment_secret)