pay: fix handling of legacy vs tlv encoding.

As revealed by the failure of tests in ElementsProject#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 ElementsProject#3968, though.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: `pay` will now make reliable multi-part payments to nodes it doesn't have a node_announcement for.

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;

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.",

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;

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)