keysend: just strip even unknown fields.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Plugins: `keysend` now removes unknown even (technically illegal!) fields, to try to accept more payments.

plugins/keysend.c

@@ -248,7 +248,7 @@ struct keysend_in {
 	struct preimage payment_preimage;
 	char *label;
 	struct tlv_tlv_payload *payload;
-	struct tlv_field *preimage_field;
+	struct tlv_field *preimage_field, *desc_field;
 };
 
 static int tlvfield_cmp(const struct tlv_field *a,
@@ -267,11 +267,27 @@ htlc_accepted_invoice_created(struct command *cmd, const char *buf,
 			      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);
+	/* Remove all unknown fields: complain about unused even ones!
+	 * (Keysend is not a design, it's a kludge by people who
+	 * didn't read the spec, and Just Made Something Work). */
+	for (size_t i = 0; i < tal_count(ki->payload->fields); i++) {
+		/* Odd fields are fine, leave them. */
+		if (ki->payload->fields[i].numtype % 2)
+			continue;
+		/* Same with known fields */
+		if (ki->payload->fields[i].meta)
+			continue;
+		/* Complain about it, at least. */
+		if (ki->preimage_field != &ki->payload->fields[i]) {
+			plugin_log(cmd->plugin, LOG_INFORM,
+				   "Keysend payment uses illegal even field %"
+				   PRIu64 ": stripping",
+				   ki->payload->fields[i].numtype);
+		}
+		tal_arr_remove(&ki->payload->fields, i);
+		i--;
+	}
 
 	/* Now we can fill in the payment secret, from invoice. */
 	ki->payload->payment_data = tal(ki->payload,
@@ -331,7 +347,7 @@ static struct command_result *htlc_accepted_call(struct command *cmd,
 	struct sha256 payment_hash;
 	size_t max;
 	struct tlv_tlv_payload *payload;
-	struct tlv_field *preimage_field = NULL, *unknown_field = NULL, *desc_field = NULL;
+	struct tlv_field *preimage_field = NULL, *desc_field = NULL;
 	bigsize_t s;
 	struct keysend_in *ki;
 	struct out_req *req;
@@ -375,9 +391,6 @@ static struct command_result *htlc_accepted_call(struct command *cmd,
 		if (field->numtype == PREIMAGE_TLV_TYPE) {
 			preimage_field = field;
 			continue;
-		} else if (field->numtype % 2 == 0 && field->meta == NULL) {
-			/* This can only happen with FROMWIRE_TLV_ANY_TYPE! */
-			unknown_field = field;
 		}
 
 		/* Longest (unknown) text field wins. */
@@ -392,13 +405,6 @@ static struct command_result *htlc_accepted_call(struct command *cmd,
 	if (preimage_field == NULL)
 		return htlc_accepted_continue(cmd, NULL);
 
-	if (unknown_field != NULL) {
-		plugin_log(cmd->plugin, LOG_INFORM,
-			   "Experimental: Accepting the keysend payment "
-			   "despite having unknown even TLV type %" PRIu64 ".",
-			   unknown_field->numtype);
-	}
-
 	/* If malformed (amt is compulsory), let lightningd handle it. */
 	if (!payload->amt_to_forward) {
 		plugin_log(cmd->plugin, LOG_UNUSUAL,
@@ -421,6 +427,7 @@ static struct command_result *htlc_accepted_call(struct command *cmd,
 	ki->label = tal_fmt(ki, "keysend-%lu.%09lu", (unsigned long)now.ts.tv_sec, now.ts.tv_nsec);
 	ki->payload = tal_steal(ki, payload);
 	ki->preimage_field = preimage_field;
+	ki->desc_field = desc_field;
 
 	/* If the preimage doesn't hash to the payment_hash we must continue,
 	 * maybe someone else knows how to handle these. */

tests/test_pay.py

@@ -3580,8 +3580,8 @@ def test_keysend(node_factory):
         l3.rpc.keysend(l4.info['id'], amt)
 
 
-def test_keysend_extra_tlvs(node_factory):
-    """Use the extratlvs option to deliver a message with sphinx' TLV type.
+def test_keysend_strip_tlvs(node_factory):
+    """Use the extratlvs option to deliver a message with sphinx' TLV type, which keysend strips.
     """
     amt = 10000
     l1, l2 = node_factory.line_graph(
@@ -3593,7 +3593,6 @@ def test_keysend_extra_tlvs(node_factory):
                 'accept-htlc-tlv-types': '133773310,99990',
             },
             {
-                'accept-htlc-tlv-types': '133773310',
                 "plugin": os.path.join(os.path.dirname(__file__), "plugins/sphinx-receiver.py"),
             },
         ]
@@ -3601,11 +3600,10 @@ def test_keysend_extra_tlvs(node_factory):
 
     # Make sure listconfigs works here
     assert l1.rpc.listconfigs()['accept-htlc-tlv-types'] == '133773310,99990'
-    assert l2.rpc.listconfigs()['accept-htlc-tlv-types'] == '133773310'
 
     l1.rpc.keysend(l2.info['id'], amt, extratlvs={133773310: 'FEEDC0DE'})
     inv = only_one(l2.rpc.listinvoices()['invoices'])
-    assert(l2.daemon.is_in_log(r'plugin-sphinx-receiver.py.*extratlvs.*133773310.*feedc0de'))
+    assert not l2.daemon.is_in_log(r'plugin-sphinx-receiver.py.*extratlvs.*133773310.*feedc0de')
 
     assert(inv['amount_received_msat'] >= Millisatoshi(amt))
     assert inv['description'] == 'keysend'
@@ -3615,6 +3613,7 @@ def test_keysend_extra_tlvs(node_factory):
     l1.rpc.keysend(l2.info['id'], amt, extratlvs={133773310: b'hello there'.hex()})
     inv = only_one(l2.rpc.listinvoices()['invoices'])
     assert inv['description'] == 'keysend: hello there'
+    l2.daemon.wait_for_log('Keysend payment uses illegal even field 133773310: stripping')
     l2.rpc.delinvoice(inv['label'], 'paid')
 
     # We can (just!) fit a giant description in.
@@ -3622,6 +3621,7 @@ def test_keysend_extra_tlvs(node_factory):
     inv = only_one(l2.rpc.listinvoices()['invoices'])
     assert inv['description'] == 'keysend: ' + 'a' * 1100
     l2.rpc.delinvoice(inv['label'], 'paid')
+    l2.daemon.wait_for_log('Keysend payment uses illegal even field 133773310: stripping')
 
     # Now try with some special characters
     ksinfo = """💕 ₿"'
@@ -3630,6 +3630,7 @@ More info
     l1.rpc.keysend(l2.info['id'], amt, extratlvs={133773310: bytes(ksinfo, encoding='utf8').hex()})
     inv = only_one(l2.rpc.listinvoices()['invoices'])
     assert inv['description'] == 'keysend: ' + ksinfo
+    l2.daemon.wait_for_log('Keysend payment uses illegal even field 133773310: stripping')
 
 
 def test_keysend_routehint(node_factory):