common: fix BOLT 12 payer_proof marker-jump via next_marker helper

common/bolt12_proof.c

@@ -10,6 +10,52 @@
 #include <common/utils.h>
 #include <inttypes.h>
 
+/* BOLT-payer_proof #12:
+ * A *marker number* is a `bigsize` in the ranges `1..239` or
+ * `1000000000..3999999999`.
+ *
+ * The *successor marker* of `prev`, written `next_marker(prev)`, is the
+ * smallest marker number strictly greater than `prev`.  So:
+ *
+ *   next_marker(0)          = 1
+ *   next_marker(1..238)     = prev + 1
+ *   next_marker(239)        = 1000000000
+ *   next_marker(>=240, <1000000000) = 1000000000 (jumps the reserved gap)
+ *   next_marker(1000000000..3999999998) = prev + 1
+ *
+ * Both the writer-side marker emission and the reader-side validation use
+ * this single helper, which resolves the writer/reader rule asymmetry
+ * raised on https://github.com/lightning/bolts/pull/1295#discussion_r3286972971
+ * by giving both sides one definition of "what comes after prev".
+ */
+static bigsize_t next_marker(bigsize_t prev)
+{
+	if (prev < 1)
+		return 1;
+	if (prev < 239)
+		return prev + 1;
+	if (prev < 1000000000)
+		return 1000000000;
+	/* prev is already in the high marker range. */
+	return prev + 1;
+}
+
+/* Is there an included non-signature TLV in `pptlv` whose successor marker
+ * is exactly `omitted`?  Used by the reader to accept a marker that follows
+ * an included TLV rather than the previous marker. */
+static bool find_included_marker_predecessor(const struct tlv_payer_proof *pptlv,
+					     bigsize_t omitted)
+{
+	for (size_t i = 0; i < tal_count(pptlv->fields); i++) {
+		const struct tlv_field *f = &pptlv->fields[i];
+		if (is_tlv_signature_field(f))
+			continue;
+		if (next_marker(f->numtype) == omitted)
+			return true;
+	}
+	return false;
+}
+
 struct creator {
 	size_t n_inv, n_included;
 	const struct tlv_invoice *inv;
@@ -102,17 +148,20 @@ struct tlv_payer_proof *make_unsigned_proof_(const tal_t *ctx,
 		/* BOLT-payer_proof #12:
 		 *   - otherwise, if the TLV type is not zero:
 		 *     - MUST append a *marker number* to `proof_omitted_tlvs`
-		 *       - If the previous TLV type was included:
-		 *         - The *marker number* is that previous tlv type,
-		 *           plus one.
-		 *       - Otherwise, if `proof_omitted_tlvs` is empty:
-		 *         - The *marker number* is 1.
-		 *       - Otherwise:
-		 *         - The *marker number* is one greater than the last
-		 *           `proof_omitted_tlvs` entry.
+		 *       - The *marker number* is `next_marker(prev)`, where
+		 *         `prev` is the previous included TLV type (if the
+		 *         previous non-signature TLV was included), the last
+		 *         `proof_omitted_tlvs` entry (otherwise), or 0 (if
+		 *         `proof_omitted_tlvs` is still empty).
+		 *
+		 * Using `next_marker` here keeps the writer and the reader in
+		 * lockstep across the `239 -> 1000000000` jump.  See
+		 *   https://github.com/lightning/bolts/pull/1295#discussion_r3286972971
 		 */
-		if (f->numtype != 0)
-			tal_arr_expand(&omitted, ++last_type);
+		if (f->numtype != 0) {
+			last_type = next_marker(last_type);
+			tal_arr_expand(&omitted, last_type);
+		}
 	}
 
 	/* Arg for next_field_create and add_merkle */
@@ -366,23 +415,27 @@ const char *check_payer_proof(const tal_t *ctx,
 		}
 		/* BOLT-payer_proof #12:
 		 *...
-		 *   - `proof_omitted_tlvs` is not one greater than:
-		 *      - an included TLV number, or
-		 *      - the previous `proof_omitted_tlvs` or 0 if it is the first
-		 *        number.
+		 *   - `proof_omitted_tlvs[i]` is not `next_marker(prev)`, where
+		 *     `prev` is either the previous `proof_omitted_tlvs` entry
+		 *     (or 0 if `i` is 0), or the TLV number of any included
+		 *     non-signature field.
+		 *
+		 * Mirroring the writer-side `next_marker` use removes the
+		 * 239 -> 1000000000 carve-out that previously lived only on the
+		 * writer.  See
+		 *   https://github.com/lightning/bolts/pull/1295#discussion_r3286972971
 		 */
 		if (i > 0)
 			prev_omitted = pptlv->proof_omitted_tlvs[i-1];
 		else
 			prev_omitted = 0;
 
-		if (omitted != prev_omitted + 1) {
-			/* O(n^2) but doesn't matter */
-			if (!find_tlv_num(pptlv, omitted - 1)) {
-				return tal_fmt(ctx, "proof_omitted_tlvs[%zi] is"
-					       " not one greater than the previous %"PRIu64" nor an included tlv entry",
-					       i, prev_omitted);
-			}
+		if (omitted != next_marker(prev_omitted)
+		    && !find_included_marker_predecessor(pptlv, omitted)) {
+			return tal_fmt(ctx, "proof_omitted_tlvs[%zi] = %"PRIu64
+				       " is not next_marker(previous=%"PRIu64
+				       ") nor next_marker of any included TLV",
+				       i, omitted, prev_omitted);
 		}
 	}
 

common/test/Makefile

@@ -119,7 +119,7 @@ common/test/run-bolt12_merkle-json:			\
 	common/base32.o					\
 	common/wireaddr.o
 
-common/test/run-bolt12_proof common/test/run-bolt12_proof_vectors: \
+common/test/run-bolt12_proof common/test/run-bolt12_proof_vectors common/test/run-bolt12_proof_marker_jump: \
 	common/bolt12.o					\
 	common/bigsize.o				\
 	common/amount.o					\

common/test/run-bolt12_proof_marker_jump.c

@@ -0,0 +1,251 @@
+/* End-to-end test of the 239 -> 1000000000 marker-jump scenario from
+ * BOLT 12 PR #1295 discussion r3286972971.
+ *
+ * Spec discussion:
+ *   https://github.com/lightning/bolts/pull/1295#discussion_r3286972971
+ *
+ * The writer rule (lines 1041-1049, head db3eff3a54) and the reader rule
+ * (lines 1065-1067) disagree on what happens after a `proof_omitted_tlvs`
+ * entry of 239: the writer skips to 1000000000, the reader as originally
+ * written rejected anything that was not `prev + 1`.
+ *
+ * This test exercises the fix on the CLN side, where both the writer-side
+ * marker emission and the reader-side validation share a single
+ * `next_marker(prev)` helper (see common/bolt12_proof.c).  The test:
+ *
+ *   1. Builds an invoice with the usual required fields PLUS 64 unknown
+ *      TLVs at types 1000000000..1000000063 (legal payload range).
+ *   2. Builds a `payer_proof` that includes only the required fields,
+ *      omitting offer_issuer_id, the other optional invoice fields, and
+ *      all 64 dummies.  The marker sequence ends with [..., 239, 1000000000]
+ *      because next_marker(239) == 1000000000.
+ *   3. Calls `check_payer_proof` and asserts it returns NULL, then sanity-
+ *      checks that the produced marker sequence really did contain the
+ *      239 -> 1000000000 jump.
+ *
+ * Parallel LDK reproduction: lightningdevkit/rust-lightning#4297, test
+ * `spec_writer_reader_rules_contradict_on_gap_jump` in
+ * lightning/src/offers/merkle.rs (commit 0034c79ae).
+ */
+#include "config.h"
+#include <stdio.h>
+#include "../bolt12_proof.c"
+#include "../bolt12_merkle.c"
+#include "../bech32_util.c"
+#include "../bech32.c"
+#include "../json_parse.c"
+#include "../json_parse_simple.c"
+#include <ccan/ptrint/ptrint.h>
+#include <common/features.h>
+#include <common/setup.h>
+#include <secp256k1_schnorrsig.h>
+
+/* AUTOGENERATED MOCKS START */
+/* Generated stub for features_unsupported */
+int features_unsupported(const struct feature_set *our_features UNNEEDED,
+			 const u8 *their_features UNNEEDED,
+			 enum feature_place p UNNEEDED)
+{ fprintf(stderr, "features_unsupported called!\n"); abort(); }
+/* Generated stub for mvt_tag_parse */
+bool mvt_tag_parse(const char *buf UNNEEDED, size_t len UNNEEDED, enum mvt_tag *tag UNNEEDED)
+{ fprintf(stderr, "mvt_tag_parse called!\n"); abort(); }
+/* Generated stub for node_id_from_hexstr */
+bool node_id_from_hexstr(const char *str UNNEEDED, size_t slen UNNEEDED, struct node_id *id UNNEEDED)
+{ fprintf(stderr, "node_id_from_hexstr called!\n"); abort(); }
+/* Generated stub for pubkey_from_node_id */
+bool pubkey_from_node_id(struct pubkey *key UNNEEDED, const struct node_id *id UNNEEDED)
+{ fprintf(stderr, "pubkey_from_node_id called!\n"); abort(); }
+/* Generated stub for siphash_seed */
+const struct siphash_seed *siphash_seed(void)
+{ fprintf(stderr, "siphash_seed called!\n"); abort(); }
+/* AUTOGENERATED MOCKS END */
+
+#define N_DUMMIES 64
+
+static struct pubkey *pubkey_for_letter(const tal_t *ctx, char letter)
+{
+	struct secret secret;
+	struct pubkey *pk;
+
+	pk = tal(ctx, struct pubkey);
+	memset(&secret, letter, sizeof(secret));
+	assert(pubkey_from_secret(&secret, pk));
+	return pk;
+}
+
+static secp256k1_keypair keypair_for_letter(char letter)
+{
+	struct secret secret;
+	secp256k1_keypair kp;
+
+	memset(&secret, letter, sizeof(secret));
+
+	if (secp256k1_keypair_create(secp256k1_ctx, &kp,
+				     secret.data) != 1)
+		abort();
+	return kp;
+}
+
+static struct bip340sig *invoice_signature(const tal_t *ctx, struct tlv_invoice *inv, char letter)
+{
+	struct sha256 merkle, sha;
+	struct bip340sig *sig;
+	secp256k1_keypair kp = keypair_for_letter(letter);
+
+	tlv_update_fields(inv, tlv_invoice, &inv->fields);
+	merkle_tlv(inv->fields, &merkle);
+	inv->signature = tal(inv, struct bip340sig);
+	sighash_from_merkle("invoice", "signature", &merkle, &sha);
+
+	sig = tal(ctx, struct bip340sig);
+	assert(secp256k1_schnorrsig_sign32(secp256k1_ctx, sig->u8,
+					   sha.u.u8,
+					   &kp,
+					   NULL) == 1);
+	return sig;
+}
+
+/* Include only the three required fields; omit everything else.  Combined
+ * with the 64 dummy TLVs we inject in main(), the resulting marker sequence
+ * has the 239 -> 1000000000 jump near the end. */
+static bool include_required_only(const struct tlv_field *f, void *arg UNNEEDED)
+{
+	if (f->numtype == 0)
+		return false;
+	return f->numtype == TLV_INVOICE_INVREQ_PAYER_ID
+		|| f->numtype == TLV_INVOICE_INVOICE_PAYMENT_HASH
+		|| f->numtype == TLV_INVOICE_INVOICE_NODE_ID;
+}
+
+static bool sign(const char *messagename,
+		 const char *fieldname,
+		 const struct sha256 *msg,
+		 struct bip340sig *sig,
+		 secp256k1_keypair *kp)
+{
+	struct sha256 shash;
+
+	sighash_from_merkle(messagename, fieldname, msg, &shash);
+	return secp256k1_schnorrsig_sign32(secp256k1_ctx, sig->u8,
+					   shash.u.u8,
+					   kp,
+					   NULL) == 1;
+}
+
+int main(int argc, char *argv[])
+{
+	struct tlv_invoice *inv;
+	struct preimage preimage;
+	struct tlv_payer_proof *proof;
+	const char *err;
+	secp256k1_keypair kp;
+	bool saw_239 = false, saw_1e9_after_239 = false;
+	size_t n_omitted;
+
+	common_setup(argv[0]);
+
+	memset(&preimage, 0x1, sizeof(preimage));
+
+	/* Standard minimal invoice (same skeleton as run-bolt12_proof.c). */
+	inv = tlv_invoice_new(tmpctx);
+	inv->invreq_metadata = tal_arrz(inv, u8, 16);
+	inv->offer_issuer_id = pubkey_for_letter(inv, 'A');
+
+	inv->invreq_amount = tal(inv, u64);
+	*inv->invreq_amount = 1;
+	inv->invreq_payer_id = pubkey_for_letter(inv, 'B');
+
+	inv->invoice_paths = tal_arr(inv, struct blinded_path *, 1);
+	inv->invoice_paths[0] = tal(inv->invoice_paths, struct blinded_path);
+	sciddir_or_pubkey_from_pubkey(&inv->invoice_paths[0]->first_node_id,
+				      pubkey_for_letter(tmpctx, 'C'));
+	inv->invoice_paths[0]->first_path_key = *pubkey_for_letter(tmpctx, 'D');
+	inv->invoice_paths[0]->path = tal_arr(inv->invoice_paths[0], struct blinded_path_hop *, 1);
+	inv->invoice_paths[0]->path[0] = tal(inv->invoice_paths[0]->path,
+					     struct blinded_path_hop);
+	inv->invoice_paths[0]->path[0]->blinded_node_id = *pubkey_for_letter(tmpctx, 'E');
+	inv->invoice_paths[0]->path[0]->encrypted_recipient_data = tal_arrz(inv->invoice_paths[0]->path[0], u8, 16);
+	inv->invoice_blindedpay = tal_arr(inv, struct blinded_payinfo *, 1);
+	inv->invoice_blindedpay[0] = tal(inv->invoice_blindedpay, struct blinded_payinfo);
+	inv->invoice_blindedpay[0]->fee_base_msat = 1;
+	inv->invoice_blindedpay[0]->fee_proportional_millionths = 2;
+	inv->invoice_blindedpay[0]->cltv_expiry_delta = 3;
+	inv->invoice_blindedpay[0]->htlc_minimum_msat = AMOUNT_MSAT(4);
+	inv->invoice_blindedpay[0]->htlc_maximum_msat = AMOUNT_MSAT(5);
+	inv->invoice_blindedpay[0]->features = NULL;
+
+	inv->invoice_created_at = tal(inv, u64);
+	*inv->invoice_created_at = 1733458312;
+	inv->invoice_payment_hash = tal(inv, struct sha256);
+	sha256(inv->invoice_payment_hash, &preimage, sizeof(preimage));
+	inv->invoice_amount = tal(inv, u64);
+	*inv->invoice_amount = 1;
+	inv->invoice_node_id = pubkey_for_letter(inv, 'F');
+
+	/* Inject 64 dummy unknown TLVs at *odd* types in the extension range
+	 * (1000000001, 1000000003, ...).  Unknown even TLVs are rejected by
+	 * BOLT-12's general TLV rule, so we keep them odd to be ignored on
+	 * decode while still appearing in inv->fields and counting toward the
+	 * marker emission.  These get omitted in the proof; combined with the
+	 * included required fields, the writer-side marker emission walks
+	 * through the 1..239 range and then jumps to 1000000000 on the 64th
+	 * dummy. */
+	for (size_t i = 0; i < N_DUMMIES; i++) {
+		struct tlv_field f;
+		f.meta = NULL;
+		f.numtype = 1000000001 + 2 * i;
+		f.length = 1;
+		f.value = tal_arrz(inv, u8, 1);
+		f.value[0] = (u8)(i + 1);
+		tal_arr_expand(&inv->fields, f);
+	}
+
+	inv->signature = invoice_signature(inv, inv, 'F');
+
+	/* We skip the usual encode/decode round-trip because invoice_decode
+	 * enforces invoice-level invariants (signature check, required-field
+	 * check) that are not what this test is exercising.  inv->fields is
+	 * already canonicalised by invoice_signature's call to
+	 * tlv_update_fields. */
+
+	/* Build proof: include the three required fields, omit everything
+	 * else.  The 64 trailing omitted dummies push the marker counter past
+	 * 239, triggering the gap-jump emission. */
+	proof = make_unsigned_proof(tmpctx, inv, &preimage, "marker-jump",
+				    include_required_only, NULL);
+	kp = keypair_for_letter('B');
+	proof->proof_signature = payer_proof_signature(proof, proof, sign, &kp);
+
+	/* The fix under test: with the next_marker helper used on both writer
+	 * and reader sides, the produced sequence ends with [..., 239,
+	 * 1000000000] and check_payer_proof accepts it. */
+	err = check_payer_proof(tmpctx, proof);
+	if (err)
+		fprintf(stderr, "check_payer_proof returned: %s\n", err);
+	assert(err == NULL);
+
+	/* Sanity-check that the marker sequence actually did contain the
+	 * 239 -> 1000000000 jump we were trying to exercise.  Without this
+	 * check the test could go green for the wrong reason. */
+	n_omitted = tal_count(proof->proof_omitted_tlvs);
+	for (size_t i = 0; i < n_omitted; i++) {
+		bigsize_t m = proof->proof_omitted_tlvs[i];
+		if (m == 239)
+			saw_239 = true;
+		if (m == 1000000000 && i > 0
+		    && proof->proof_omitted_tlvs[i-1] == 239)
+			saw_1e9_after_239 = true;
+	}
+	fprintf(stderr, "marker sequence (%zu entries):", n_omitted);
+	for (size_t i = 0; i < n_omitted; i++)
+		fprintf(stderr, " %"PRIu64, proof->proof_omitted_tlvs[i]);
+	fprintf(stderr, "\n");
+	fprintf(stderr, "saw 239: %s, saw 1000000000 immediately after: %s\n",
+		saw_239 ? "yes" : "no",
+		saw_1e9_after_239 ? "yes" : "no");
+	assert(saw_239);
+	assert(saw_1e9_after_239);
+
+	common_shutdown();
+	return 0;
+}