Make payment secret compulsory on new invoices, and assume unknown nodes support TLV onions

common/route.c

@@ -107,7 +107,9 @@ static bool dijkstra_to_hops(struct route_hop **hops,
 	/* Find other end of channel. */
 	next = gossmap_nth_node(gossmap, c, !(*hops)[num_hops].direction);
 	gossmap_node_get_id(gossmap, next, &(*hops)[num_hops].node_id);
-	if (gossmap_node_get_feature(gossmap, next, OPT_VAR_ONION) != -1)
+	/* If we don't have a node_announcement, we *assume* modern */
+	if (next->nann_off == 0
+	    || gossmap_node_get_feature(gossmap, next, OPT_VAR_ONION) != -1)
 		(*hops)[num_hops].style = ROUTE_HOP_TLV;
 	else
 		(*hops)[num_hops].style = ROUTE_HOP_LEGACY;

connectd/connectd.c

@@ -479,13 +479,16 @@ struct io_plan *peer_connected(struct io_conn *conn,
 	unsup = features_unsupported(daemon->our_features, their_features,
 				     INIT_FEATURE);
 	if (unsup != -1) {
+		status_peer_unusual(id, "Unsupported feature %u", unsup);
 		msg = towire_warningfmt(NULL, NULL, "Unsupported feature %u",
 					unsup);
 		msg = cryptomsg_encrypt_msg(tmpctx, cs, take(msg));
 		return io_write(conn, msg, tal_count(msg), io_close_cb, NULL);
 	}
 
 	if (!feature_check_depends(their_features, &depender, &missing)) {
+		status_peer_unusual(id, "Feature %zu requires feature %zu",
+				    depender, missing);
 		msg = towire_warningfmt(NULL, NULL,
 				      "Feature %zu requires feature %zu",
 				      depender, missing);

contrib/pyln-testing/pyln/testing/utils.py

@@ -996,7 +996,10 @@ def pay(self, dst, amt, label=None):
         assert len(self.rpc.listpeers(dst_id).get('peers')) == 1
 
         # make an invoice
-        rhash = dst.rpc.invoice(amt, label, label)['payment_hash']
+        inv = dst.rpc.invoice(amt, label, label)
+        # FIXME: pre 0.10.1 invoice calls didn't have payment_secret field
+        psecret = dst.rpc.decodepay(inv['bolt11'])['payment_secret']
+        rhash = inv['payment_hash']
         invoices = dst.rpc.listinvoices(label)['invoices']
         assert len(invoices) == 1 and invoices[0]['status'] == 'unpaid'
 
@@ -1008,7 +1011,7 @@ def pay(self, dst, amt, label=None):
         }
 
         # sendpay is async now
-        self.rpc.sendpay([routestep], rhash)
+        self.rpc.sendpay([routestep], rhash, payment_secret=psecret)
         # wait for sendpay to comply
         result = self.rpc.waitsendpay(rhash)
         assert(result.get('status') == 'complete')

doc/lightning-invoice.7.md

@@ -75,6 +75,7 @@ RETURN VALUE
 On success, an object is returned, containing:
 - **bolt11** (string): the bolt11 string
 - **payment_hash** (hex): the hash of the *payment_preimage* which will prove payment (always 64 characters)
+- **payment_secret** (hex): the *payment_secret* to place in the onion (always 64 characters)
 - **expires_at** (u64): UNIX timestamp of when invoice expires
 
 The following warnings may also be returned:
@@ -111,4 +112,4 @@ RESOURCES
 
 Main web site: <https://github.com/ElementsProject/lightning>
 
-[comment]: # ( SHA256STAMP:e63e87b91a14b8ae823ae67fc25315bc31b15a14378c05f4c972830bf3515af1)
+[comment]: # ( SHA256STAMP:cba838098a7f95ab74906f387b2d67777b33ccc40349cb9a8ef1f7c9cc28c6ef)

doc/schemas/invoice.schema.json

@@ -2,7 +2,7 @@
     "$schema": "http://json-schema.org/draft-07/schema#",
     "type": "object",
     "additionalProperties": false,
-    "required": [ "payment_hash", "expires_at", "bolt11" ],
+    "required": [ "payment_hash", "expires_at", "bolt11", "payment_secret" ],
     "properties": {
 	"bolt11": {
 	    "type": "string",
@@ -14,6 +14,12 @@
 	    "maxLength": 64,
 	    "minLength": 64
 	},
+	"payment_secret": {
+	    "type": "hex",
+	    "description": "the *payment_secret* to place in the onion",
+	    "maxLength": 64,
+	    "minLength": 64
+	},
 	"expires_at": {
 	    "type": "u64",
 	    "description": "UNIX timestamp of when invoice expires"

lightningd/htlc_set.c

@@ -1,3 +1,4 @@
+#include <common/features.h>
 #include <common/timeout.h>
 #include <lightningd/htlc_end.h>
 #include <lightningd/htlc_set.h>
@@ -114,6 +115,17 @@ void htlc_set_add(struct lightningd *ld,
 		return;
 	}
 
+	/* If we insist on a payment secret, it must always have it */
+	if (feature_is_set(details->features, COMPULSORY_FEATURE(OPT_PAYMENT_SECRET))
+	    && !payment_secret) {
+		log_debug(ld->log, "Missing payment_secret, but required for %s",
+			  type_to_string(tmpctx, struct sha256,
+					 &hin->payment_hash));
+		local_fail_in_htlc(hin,
+				   take(failmsg_incorrect_or_unknown(NULL, ld, hin)));
+		return;
+	}
+
 	/* BOLT #4:
 	 *  - otherwise, if it supports `basic_mpp`:
 	 *    - MUST add it to the HTLC set corresponding to that `payment_hash`.

lightningd/invoice.c

@@ -855,6 +855,7 @@ invoice_complete(struct invoice_info *info,
 	struct invoice invoice;
 	char *b11enc;
 	const struct invoice_details *details;
+	struct secret payment_secret;
 	struct wallet *wallet = info->cmd->ld->wallet;
 
 	b11enc = bolt11_encode(info, info->b11, false,
@@ -904,6 +905,8 @@ invoice_complete(struct invoice_info *info,
 	json_add_sha256(response, "payment_hash", &details->rhash);
 	json_add_u64(response, "expires_at", details->expiry_time);
 	json_add_string(response, "bolt11", details->invstring);
+	invoice_secret(&details->r, &payment_secret);
+	json_add_secret(response, "payment_secret", &payment_secret);
 
 	notify_invoice_creation(info->cmd->ld, info->b11->msat,
 				info->payment_preimage, info->label);

lightningd/lightningd.c

@@ -811,7 +811,7 @@ static struct feature_set *default_features(const tal_t *ctx)
 		OPTIONAL_FEATURE(OPT_UPFRONT_SHUTDOWN_SCRIPT),
 		OPTIONAL_FEATURE(OPT_GOSSIP_QUERIES),
 		OPTIONAL_FEATURE(OPT_VAR_ONION),
-		OPTIONAL_FEATURE(OPT_PAYMENT_SECRET),
+		COMPULSORY_FEATURE(OPT_PAYMENT_SECRET),
 		OPTIONAL_FEATURE(OPT_BASIC_MPP),
 		OPTIONAL_FEATURE(OPT_GOSSIP_QUERIES_EX),
 		OPTIONAL_FEATURE(OPT_STATIC_REMOTEKEY),

lightningd/test/run-invoice-select-inchan.c

@@ -334,6 +334,11 @@ void json_add_node_id(struct json_stream *response UNNEEDED,
 void json_add_preimage(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED,
 		     const struct preimage *preimage UNNEEDED)
 { fprintf(stderr, "json_add_preimage called!\n"); abort(); }
+/* Generated stub for json_add_secret */
+void json_add_secret(struct json_stream *response UNNEEDED,
+		     const char *fieldname UNNEEDED,
+		     const struct secret *secret UNNEEDED)
+{ fprintf(stderr, "json_add_secret called!\n"); abort(); }
 /* Generated stub for json_add_sha256 */
 void json_add_sha256(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED,
 		     const struct sha256 *hash UNNEEDED)

plugins/keysend.c

@@ -1,5 +1,7 @@
 #include <bitcoin/preimage.h>
 #include <ccan/array_size/array_size.h>
+#include <ccan/asort/asort.h>
+#include <ccan/cast/cast.h>
 #include <ccan/tal/str/str.h>
 #include <common/gossmap.h>
 #include <common/type_to_string.h>
@@ -259,17 +261,55 @@ struct keysend_in {
 	struct tlv_field *preimage_field;
 };
 
+static int tlvfield_cmp(const struct tlv_field *a,
+			const struct tlv_field *b, void *unused)
+{
+	if (a->numtype > b->numtype)
+		return 1;
+	else if (a->numtype < b->numtype)
+		return -1;
+	return 0;
+}
+
 static struct command_result *
-htlc_accepted_invoice_created(struct command *cmd, const char *buf UNUSED,
-			      const jsmntok_t *result UNUSED,
+htlc_accepted_invoice_created(struct command *cmd, const char *buf,
+			      const jsmntok_t *result,
 			      struct keysend_in *ki)
 {
+	struct tlv_field field;
 	int preimage_field_idx = ki->preimage_field - ki->payload->fields;
 
 	/* Remove the preimage field so `lightningd` knows how to handle
 	 * this. */
 	tal_arr_remove(&ki->payload->fields, preimage_field_idx);
 
+	/* Now we can fill in the payment secret, from invoice. */
+	ki->payload->payment_data = tal(ki->payload,
+					struct tlv_tlv_payload_payment_data);
+	json_to_secret(buf, json_get_member(buf, result, "payment_secret"),
+		       &ki->payload->payment_data->payment_secret);
+
+	/* We checked that amt_to_forward was non-NULL before */
+	ki->payload->payment_data->total_msat = *ki->payload->amt_to_forward;
+
+	/* In order to put payment_data into ->fields, I'd normally re-serialize,
+	 * but we can have completely unknown fields.  So insert manually. */
+	/* BOLT #4:
+	 *     1. type: 8 (`payment_data`)
+	 *     2. data:
+	 *         * [`32*byte`:`payment_secret`]
+	 *         * [`tu64`:`total_msat`]
+	 */
+	field.numtype = 8;
+	field.value = tal_arr(ki->payload, u8, 0);
+	towire_secret(&field.value, &ki->payload->payment_data->payment_secret);
+	towire_tu64(&field.value, ki->payload->payment_data->total_msat);
+	field.length = tal_bytelen(field.value);
+	tal_arr_expand(&ki->payload->fields, field);
+
+	asort(ki->payload->fields, tal_count(ki->payload->fields),
+	      tlvfield_cmp, NULL);
+
 	/* Finally we can resolve the payment with the preimage. */
 	plugin_log(cmd->plugin, LOG_INFORM,
 		   "Resolving incoming HTLC with preimage for payment_hash %s "
@@ -279,6 +319,20 @@ htlc_accepted_invoice_created(struct command *cmd, const char *buf UNUSED,
 
 }
 
+static struct command_result *
+htlc_accepted_invoice_failed(struct command *cmd, const char *buf,
+			     const jsmntok_t *error,
+			     struct keysend_in *ki)
+{
+	plugin_log(cmd->plugin, LOG_BROKEN,
+		   "Could not create invoice for keysend: %.*s",
+		   json_tok_full_len(error),
+		   json_tok_full(buf, error));
+	/* Continue, but don't change it: it will fail. */
+	return htlc_accepted_continue(cmd, NULL);
+
+}
+
 static struct command_result *htlc_accepted_call(struct command *cmd,
 						 const char *buf,
 						 const jsmntok_t *params)
@@ -349,6 +403,12 @@ static struct command_result *htlc_accepted_call(struct command *cmd,
 #endif
 	}
 
+	/* If malformed (amt is compulsory), let lightningd handle it. */
+	if (!payload->amt_to_forward) {
+		plugin_log(cmd->plugin, LOG_UNUSUAL,
+			   "Sender omitted amount.  Ignoring this HTLC.");
+		return htlc_accepted_continue(cmd, NULL);
+	}
 
 	/* If the preimage is not 32 bytes long then we can't accept the
 	 * payment. */
@@ -391,7 +451,7 @@ static struct command_result *htlc_accepted_call(struct command *cmd,
 	 * will be nice and reject the payment. */
 	req = jsonrpc_request_start(cmd->plugin, cmd, "invoice",
 				    &htlc_accepted_invoice_created,
-				    &htlc_accepted_invoice_created,
+				    &htlc_accepted_invoice_failed,
 				    ki);
 
 	plugin_log(cmd->plugin, LOG_INFORM, "Inserting a new invoice for keysend with payment_hash %s", type_to_string(tmpctx, struct sha256, &payment_hash));

tests/benchmark.py

@@ -30,19 +30,20 @@ def test_single_hop(node_factory, executor):
     fs = []
     invoices = []
     for i in tqdm(range(num_payments)):
-        invoices.append(l2.rpc.invoice(1000, 'invoice-%d' % (i), 'desc')['payment_hash'])
+        inv = l2.rpc.invoice(1000, 'invoice-%d' % (i), 'desc')
+        invoices.append((inv['payment_hash'], inv['payment_secret']))
 
     route = l1.rpc.getroute(l2.rpc.getinfo()['id'], 1000, 1)['route']
     print("Sending payments")
     start_time = time()
 
-    def do_pay(i):
-        p = l1.rpc.sendpay(route, i)
+    def do_pay(i, s):
+        p = l1.rpc.sendpay(route, i, payment_secret=s)
         r = l1.rpc.waitsendpay(p['payment_hash'])
         return r
 
-    for i in invoices:
-        fs.append(executor.submit(do_pay, i))
+    for i, s in invoices:
+        fs.append(executor.submit(do_pay, i, s))
 
     for f in tqdm(futures.as_completed(fs), total=len(fs)):
         f.result()