From 8f2b8f739c7056ef2d028e632c7f945e3c40ec72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Wed, 5 Aug 2026 20:59:28 -0300 Subject: [PATCH 1/9] xpay: quote BOLT #12 requirements in the offer payment path. We check the offer and the fetched invoice in several places without saying which requirement each check comes from, so it's hard to tell what we implement and what we merely assume. Add the quotes and let check-source-bolt keep them current. Changelog-None: comments only. (cherry picked from commit 07734f782f72c2ad9c68a30b32e5c2d0c8525ca7) --- plugins/xpay/xpay.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index 5b37463edbb4..20af0dd16848 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -2219,6 +2219,17 @@ static struct command_result *check_offer_payable(struct command *cmd, if (!b12offer) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Invalid bolt12 offer: %s", err); + /* BOLT #12: + * - if `offer_amount` is not present: + * - MUST specify `invreq_amount`. + * - otherwise: + * - MAY omit `invreq_amount`. + * - if it sets `invreq_amount`: + * - MUST specify `invreq_amount`.`msat` as greater or equal to amount expected by `offer_amount` (and, if present, `offer_currency` and `invreq_quantity`). + */ + /* We can't work out the expected amount without a conversion rate, so we + * refuse currency offers here. We also require the exact offer amount, + * which is stricter than the "greater or equal" the spec allows. */ /* We will only one-shot if we know amount! (FIXME: Convert!) */ if (b12offer->offer_currency) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, @@ -2645,6 +2656,13 @@ static struct command_result *xpay_core(struct command *cmd, if (amount_msat_is_zero(amount_msat(*b12inv->invoice_amount))) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Invalid bolt12 invoice with zero amount"); + /* BOLT #12: + * - if `invoice_relative_expiry` is present: + * - MUST reject the invoice if the current time since 1970-01-01 UTC is greater than `invoice_created_at` plus `seconds_from_creation`. + * - otherwise: + * - MUST reject the invoice if the current time since 1970-01-01 UTC is greater than `invoice_created_at` plus 7200. + */ + /* invoice_expiry() applies the 7200 second default for us. */ invexpiry = invoice_expiry(b12inv); invoice_msat = amount_msat(*b12inv->invoice_amount); From 3f00cf8f4177bef779fab27974ffaf0c9f2b6e82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Wed, 5 Aug 2026 21:00:33 -0300 Subject: [PATCH 2/9] xpay: test that a fetched invoice's amount matches what we asked for. invoice_amount sits outside the TLV ranges an invoice must copy from our invoice_request, so the payee sets it independently of the amount in our request. (cherry picked from commit 808ad86b24d06f43a9225cb3926e0c16e1f2ee4b) --- .../plugins/xpay_mismatched_invoice_amount.py | 30 ++++++++++++++ tests/test_xpay.py | 40 +++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100755 tests/plugins/xpay_mismatched_invoice_amount.py diff --git a/tests/plugins/xpay_mismatched_invoice_amount.py b/tests/plugins/xpay_mismatched_invoice_amount.py new file mode 100755 index 000000000000..f38c0e45277a --- /dev/null +++ b/tests/plugins/xpay_mismatched_invoice_amount.py @@ -0,0 +1,30 @@ +#!/usr/bin/env python3 +"""Plugin which makes fetchinvoice hand back a mismatched invoice. + +Stands in for a payee which sets invoice_amount to something other than the +amount our invoice_request asked for. The invoice is validly signed; +fetchinvoice reports the difference in `changes.amount_msat`. + +Call `setinvoiceamount` to arm it; until then fetchinvoice behaves normally. +""" +from pyln.client import Plugin + +plugin = Plugin() + + +@plugin.method("setinvoiceamount") +def setinvoiceamount(plugin, invoice, amount_msat): + plugin.mismatched = {"invoice": invoice, + "changes": {"amount_msat": amount_msat}} + return {} + + +@plugin.hook("rpc_command") +def on_rpc_command(plugin, rpc_command, **kwargs): + if rpc_command["method"] != "fetchinvoice" or plugin.mismatched is None: + return {"result": "continue"} + return {"return": {"result": plugin.mismatched}} + + +plugin.mismatched = None +plugin.run() diff --git a/tests/test_xpay.py b/tests/test_xpay.py index 1568438f6d15..20df2d4103e4 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -1031,6 +1031,46 @@ def test_xpay_offer(node_factory): l1.rpc.xpay(offer2, 5000) +@pytest.mark.xfail(strict=True) +def test_xpay_offer_invoice_amount_mismatch(node_factory): + """xpay must not pay an invoice whose amount isn't the one we asked for. + + invoice_amount is not one of the fields the invoice has to echo from our + invoice_request, so the payee sets it independently. BOLT #12 requires us + to reject the invoice if it doesn't equal the invreq_amount we sent, in + either direction. + """ + plugin = Path(__file__).parent / "plugins" / "xpay_mismatched_invoice_amount.py" + l1, l2 = node_factory.line_graph(2, wait_for_announce=True, + opts=[{'plugin': str(plugin)}, {}]) + + offer = l2.rpc.offer('any')['bolt12'] + + # Genuine, validly-signed invoices from l2, for amounts we won't ask for. + # Fetch both before arming the plugin, since it intercepts fetchinvoice. + larger = l1.rpc.fetchinvoice(offer, 10000000)['invoice'] + smaller = l1.rpc.fetchinvoice(offer, 50000)['invoice'] + + before = only_one(l1.rpc.listpeerchannels()['channels'])['to_us_msat'] + for inv, amount_msat in ((larger, 10000000), (smaller, 50000)): + l1.rpc.call('setinvoiceamount', {'invoice': inv, + 'amount_msat': amount_msat}) + with pytest.raises(RpcError, match=r"Invoice amount"): + l1.rpc.xpay(offer, 100000) + after = only_one(l1.rpc.listpeerchannels()['channels'])['to_us_msat'] + assert before == after + + # We send no invreq_amount when the offer has its own amount, so here the + # offer amount is what we authorized. + fixed = l2.rpc.offer('100000msat', 'fixed amount offer')['bolt12'] + l1.rpc.call('setinvoiceamount', {'invoice': larger, + 'amount_msat': 10000000}) + with pytest.raises(RpcError, match=r"Invoice amount"): + l1.rpc.xpay(fixed) + after = only_one(l1.rpc.listpeerchannels()['channels'])['to_us_msat'] + assert before == after + + def test_xpay_circular_routehint(node_factory): """Test that xpay gracefully skips a circular bolt11 routehint (src == dst).""" l1, l2 = node_factory.line_graph(2) From 605a7540cd3b1506c750c21927872e023baa8d07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Wed, 5 Aug 2026 21:04:03 -0300 Subject: [PATCH 3/9] xpay: honour the BOLT #12 invoice_amount check. We sent invreq_amount, so the invoice must have an equal invoice_amount; we passed msat=NULL into xpay_core() and used the invoice's own figure instead. The default fee cap scales off that same figure. Carry the amount we asked for through to invoice_fetched() and check it. Changelog-Fixed: xpay: check a fetched bolt12 invoice's amount is the one we asked for. (cherry picked from commit 51c9a1fc72cad8f83f2ed08f5120e5a41a8372ba) --- plugins/xpay/xpay.c | 81 +++++++++++++++++++++++++++++++++++++++++---- tests/test_xpay.py | 1 - 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/plugins/xpay/xpay.c b/plugins/xpay/xpay.c index 20af0dd16848..b24e0cc5cd19 100644 --- a/plugins/xpay/xpay.c +++ b/plugins/xpay/xpay.c @@ -2206,9 +2206,11 @@ preapprove_succeed(struct command *cmd, const char *method, const char *buf, return age_layer(cmd, payment); } +/* If it returns NULL, *authorized is the most we agreed to pay. */ static struct command_result *check_offer_payable(struct command *cmd, const char *offerstr, - const struct amount_msat *msat) + const struct amount_msat *msat, + struct amount_msat *authorized) { const char *err; struct tlv_offer *b12offer = offer_decode(tmpctx, @@ -2252,6 +2254,11 @@ static struct command_result *check_offer_payable(struct command *cmd, return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Cannot xpay recurring offers"); + if (msat) + *authorized = *msat; + else + *authorized = amount_msat(*b12offer->offer_amount); + return NULL; } @@ -2286,6 +2293,10 @@ check_offer_sendamount_payable(struct command *cmd, const char *offerstr) struct xpay_params { struct amount_msat *msat, *maxfee, *partial, *includefees_msat; + /* What we agreed to pay, if we're paying an offer: the amount we sent + * as invreq_amount, or the offer amount if we sent none. NULL for + * sendamount, where xpay_core demands the invoice match *msat. */ + struct amount_msat *authorized_msat; const char **layers; unsigned int retryfor; u32 maxdelay; @@ -2301,11 +2312,51 @@ invoice_fetched(struct command *cmd, const jsmntok_t *result, struct xpay_params *params) { - const char *inv; + const char *inv, *err; inv = json_strdup(tmpctx, buf, json_get_member(buf, result, "invoice")); - inv = to_canonical_invstr(NULL, inv); - return xpay_core(cmd, take(inv), + inv = to_canonical_invstr(tmpctx, inv); + + /* BOLT #12: + * - if `invreq_amount` is present: + * - MUST reject the invoice if `invoice_amount` is not equal to `invreq_amount` + * - otherwise: + * - SHOULD confirm authorization if `invoice_amount`.`msat` is not within + * the amount range authorized. + */ + /* invoice_amount is not one of the fields the invoice must copy from our + * invoice_request, so it is set independently of what we asked for and + * has to be checked here. We set invreq_amount iff we were given an + * amount, which selects which of the two rules above applies. */ + if (params->authorized_msat) { + struct amount_msat invoice_msat; + struct tlv_invoice *b12inv + = invoice_decode(tmpctx, inv, strlen(inv), + plugin_feature_set(cmd->plugin), + chainparams, &err); + if (!b12inv) + return command_fail(cmd, OFFER_BAD_INVREQ_REPLY, + "Invalid bolt12 invoice: %s", err); + /* invoice_decode() has already insisted on invoice_amount. */ + invoice_msat = amount_msat(*b12inv->invoice_amount); + if (params->msat) { + if (!amount_msat_eq(invoice_msat, *params->authorized_msat)) + return command_fail(cmd, OFFER_BAD_INVREQ_REPLY, + "Invoice amount is %s, but we asked for %s", + fmt_amount_msat(tmpctx, invoice_msat), + fmt_amount_msat(tmpctx, + *params->authorized_msat)); + } else if (amount_msat_greater(invoice_msat, + *params->authorized_msat)) { + return command_fail(cmd, OFFER_BAD_INVREQ_REPLY, + "Invoice amount is %s, more than the %s we authorized", + fmt_amount_msat(tmpctx, invoice_msat), + fmt_amount_msat(tmpctx, + *params->authorized_msat)); + } + } + + return xpay_core(cmd, inv, NULL, params->maxfee, params->layers, params->retryfor, params->partial, params->maxdelay, params->label, NULL, false, false, @@ -2360,8 +2411,15 @@ bip353_fetched(struct command *cmd, if (xparams->includefees_msat) ret = check_offer_sendamount_payable(cmd, offerstr); - else - ret = check_offer_payable(cmd, offerstr, xparams->msat); + else { + struct amount_msat authorized; + ret = check_offer_payable(cmd, offerstr, xparams->msat, + &authorized); + if (!ret) + xparams->authorized_msat + = tal_dup(xparams, struct amount_msat, + &authorized); + } if (ret) return ret; @@ -2404,8 +2462,9 @@ static struct command_result *json_xpay_params(struct command *cmd, /* Is this a one-shot vibe payment? Kids these days! */ if (!as_pay && bolt12_has_offer_prefix(invstring)) { struct command_result *ret; + struct amount_msat authorized; - ret = check_offer_payable(cmd, invstring, msat); + ret = check_offer_payable(cmd, invstring, msat, &authorized); if (ret) return ret; @@ -2431,6 +2490,8 @@ static struct command_result *json_xpay_params(struct command *cmd, xparams->payer_note = payer_note; xparams->label = label; xparams->includefees_msat = NULL; + xparams->authorized_msat = tal_dup(xparams, struct amount_msat, + &authorized); return do_fetchinvoice(cmd, invstring, xparams); } @@ -2459,6 +2520,8 @@ static struct command_result *json_xpay_params(struct command *cmd, xparams->payer_note = payer_note; xparams->label = label; xparams->includefees_msat = NULL; + /* Set once bip353_fetched() knows the offer. */ + xparams->authorized_msat = NULL; req = jsonrpc_request_start(cmd, "fetchbip353", bip353_fetched, @@ -2955,6 +3018,8 @@ static struct command_result *json_sendamount(struct command *cmd, xparams->bip353 = NULL; xparams->payer_note = payer_note; xparams->label = label; + /* xpay_core() insists the invoice match *msat exactly here. */ + xparams->authorized_msat = NULL; return do_fetchinvoice(cmd, invstring, xparams); } @@ -2973,6 +3038,8 @@ static struct command_result *json_sendamount(struct command *cmd, xparams->bip353 = invstring; xparams->payer_note = payer_note; xparams->label = label; + /* xpay_core() insists the invoice match *msat exactly here. */ + xparams->authorized_msat = NULL; req = jsonrpc_request_start(cmd, "fetchbip353", bip353_fetched, forward_error, xparams); diff --git a/tests/test_xpay.py b/tests/test_xpay.py index 20df2d4103e4..3dba55eab128 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -1031,7 +1031,6 @@ def test_xpay_offer(node_factory): l1.rpc.xpay(offer2, 5000) -@pytest.mark.xfail(strict=True) def test_xpay_offer_invoice_amount_mismatch(node_factory): """xpay must not pay an invoice whose amount isn't the one we asked for. From a5f2a4d0fc3030ae6e2d9ee39b4a6e432100a20f Mon Sep 17 00:00:00 2001 From: Sangbida Chaudhuri <101164840+sangbida@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:21:48 +0930 Subject: [PATCH 4/9] hsmd: return io_close when the handler reports an error When libhsmd rejects a request it returns NULL; close the client then, instead of from hsmd_status_bad_request. Share the report path in report_bad_req so it cannot drift from bad_req_fmt. Changelog-Fixed: print the commitment index in "bad commit secret" instead of a stack address (cherry picked from commit 869bd4da44df842d5f54e0370e95bffa7cf5a8b4) --- hsmd/hsmd.c | 74 ++++++++++++++++++++++++++++++-------------------- hsmd/libhsmd.c | 2 +- 2 files changed, 45 insertions(+), 31 deletions(-) diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 739a148bda66..e240e64715e8 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -102,31 +102,15 @@ static bool is_lightningd(const struct client *client) /* Pre-declare this, due to mutual recursion */ static struct io_plan *handle_client(struct io_conn *conn, struct client *c); -/*~ ccan/compiler.h defines PRINTF_FMT as the gcc compiler hint so it will - * check that fmt and other trailing arguments really are the correct type. +/*~ Tell lightningd a client sent a bad request. This should never + * happen, of course, but we definitely want to log if it does. * - * This is a convenient helper to tell lightningd we've received a bad request - * and closes the client connection. This should never happen, of course, but - * we definitely want to log if it does. - */ -static struct io_plan *bad_req_fmt(struct io_conn *conn, - struct client *c, - const u8 *msg_in, - const char *fmt, ...) - PRINTF_FMT(4,5); - -static struct io_plan *bad_req_fmt(struct io_conn *conn, - struct client *c, - const u8 *msg_in, - const char *fmt, ...) + * Does not close the connection: the caller decides. bad_req_fmt + * closes immediately; hsmd_status_bad_request returns NULL and + * handle_client closes, so we don't io_close/free conn before + * handle_client can return. */ +static void report_bad_req(struct client *c, const u8 *msg_in, const char *str) { - va_list ap; - char *str; - - va_start(ap, fmt); - str = tal_vfmt(tmpctx, fmt, ap); - va_end(ap); - /*~ If the client was actually lightningd, it's Game Over; we actually * fail in this case, and it will too. */ if (is_lightningd(c)) { @@ -148,6 +132,33 @@ static struct io_plan *bad_req_fmt(struct io_conn *conn, &c->id, str, msg_in))); +} + +/*~ ccan/compiler.h defines PRINTF_FMT as the gcc compiler hint so it will + * check that fmt and other trailing arguments really are the correct type. + * + * This is a convenient helper to tell lightningd we've received a bad request + * and closes the client connection. + */ +static struct io_plan *bad_req_fmt(struct io_conn *conn, + struct client *c, + const u8 *msg_in, + const char *fmt, ...) + PRINTF_FMT(4,5); + +static struct io_plan *bad_req_fmt(struct io_conn *conn, + struct client *c, + const u8 *msg_in, + const char *fmt, ...) +{ + va_list ap; + char *str; + + va_start(ap, fmt); + str = tal_vfmt(tmpctx, fmt, ap); + va_end(ap); + + report_bad_req(c, msg_in, str); /*~ The way ccan/io works is that you return the "plan" for what to do * next (eg. io_read). io_close() is special: it means to close the @@ -658,10 +669,11 @@ u8 *hsmd_status_bad_request(struct hsmd_client *client, const u8 *msg, const cha /* Extract the pointer to the hsmd representation of the * client which has access to the underlying connection. */ struct client *c = (struct client*)client->extra; - bad_req_fmt(c->conn, c, msg, "%s", error); + + report_bad_req(c, msg, error); /* We often use `return hsmd_status_bad_request` to drop out, and NULL - * means we encountered an error. */ + * means we encountered an error. handle_client then io_close's. */ return NULL; } @@ -802,11 +814,13 @@ static struct io_plan *handle_client(struct io_conn *conn, struct client *c) case WIRE_HSMD_SIGN_ANY_REMOTE_HTLC_TO_US: case WIRE_HSMD_SIGN_ANY_LOCAL_HTLC_TX: case WIRE_HSMD_SIGN_ANCHORSPEND: - case WIRE_HSMD_SIGN_HTLC_TX_MINGLE: - /* Hand off to libhsmd for processing */ - return req_reply(conn, c, - take(hsmd_handle_client_message( - tmpctx, c->hsmd_client, c->msg_in))); + case WIRE_HSMD_SIGN_HTLC_TX_MINGLE: { + u8 *reply = hsmd_handle_client_message(tmpctx, c->hsmd_client, + c->msg_in); + if (!reply) + return io_close(conn); + return req_reply(conn, c, take(reply)); + } case WIRE_HSMD_ECDH_RESP: case WIRE_HSMD_CANNOUNCEMENT_SIG_REPLY: diff --git a/hsmd/libhsmd.c b/hsmd/libhsmd.c index c605198a056f..80fd029dc7f2 100644 --- a/hsmd/libhsmd.c +++ b/hsmd/libhsmd.c @@ -232,7 +232,7 @@ static u8 *hsmd_status_bad_request_fmt(struct hsmd_client *client, char *str; va_start(ap, fmt); - str = tal_fmt(tmpctx, fmt, ap); + str = tal_vfmt(tmpctx, fmt, ap); va_end(ap); return hsmd_status_bad_request(client, msg, str); } From 01f1cee7f4cfbd405f1de0da905dd323473477fd Mon Sep 17 00:00:00 2001 From: Sangbida Chaudhuri <101164840+sangbida@users.noreply.github.com> Date: Tue, 18 Aug 2026 20:09:14 +0930 Subject: [PATCH 5/9] hsmd: test that a NULL reply closes the client exactly once libhsmd returning NULL has no black-box behaviour change, so drive hsmd.c's io loop directly: io_set_finish counts the close, and a write after close aborts. Changelog-None (cherry picked from commit 523be992eb1b16df30c3e366129854539cce36ab) --- hsmd/libhsmd.c | 8 ++ hsmd/libhsmd.h | 7 ++ hsmd/test/Makefile | 19 ++++ hsmd/test/run-bad-request-close.c | 159 ++++++++++++++++++++++++++++++ 4 files changed, 193 insertions(+) create mode 100644 hsmd/test/Makefile create mode 100644 hsmd/test/run-bad-request-close.c diff --git a/hsmd/libhsmd.c b/hsmd/libhsmd.c index 80fd029dc7f2..2dfe79f0d297 100644 --- a/hsmd/libhsmd.c +++ b/hsmd/libhsmd.c @@ -2593,3 +2593,11 @@ u8 *hsmd_init(const u8 *secret_data, size_t secret_len, const u64 hsmd_version, &node_id, &secretstuff.bip32, &bolt12, tlvs)); } + +void hsmd_deinit(void) +{ + /* Frees off NULL, so it also fires the mlock_tal_memory destructor + * which wipes and munlocks it. */ + secretstuff.bip32_seed = tal_free(secretstuff.bip32_seed); + initialized = false; +} diff --git a/hsmd/libhsmd.h b/hsmd/libhsmd.h index 484089d2c009..f69e9f0f59b1 100644 --- a/hsmd/libhsmd.h +++ b/hsmd/libhsmd.h @@ -49,6 +49,13 @@ struct hsmd_client { u8 *hsmd_init(const u8 *secret_data, size_t secret_len, const u64 hsmd_version, struct bip32_key_version bip32_key_version, u8 hsm_secret_type); +/* Release the secrets hsmd_init() cached. + * + * hsmd itself never needs this: it holds the seed for the life of the process + * and exits with it. Unit tests do, since they return from main() and a + * process-lifetime allocation still shows up as a leak under valgrind. */ +void hsmd_deinit(void); + struct hsmd_client *hsmd_client_new_main(const tal_t *ctx, u64 capabilities, void *extra); diff --git a/hsmd/test/Makefile b/hsmd/test/Makefile new file mode 100644 index 000000000000..36605a4e65ef --- /dev/null +++ b/hsmd/test/Makefile @@ -0,0 +1,19 @@ +check-units: hsmd-tests + +# Note that these actually #include everything they need, except ccan/ and bitcoin/. +# That allows for unit testing of statics, and special effects. +HSMD_TEST_SRC := $(wildcard hsmd/test/run-*.c) +HSMD_TEST_OBJS := $(HSMD_TEST_SRC:.c=.o) +HSMD_TEST_PROGRAMS := $(HSMD_TEST_OBJS:.o=) + +ALL_C_SOURCES += $(HSMD_TEST_SRC) +ALL_TEST_PROGRAMS += $(HSMD_TEST_PROGRAMS) + +# hsmd.c is #included by the tests (so we can see statics). Do not also +# link hsmd.o, or we get duplicate symbols. libhsmd.o still supplies the +# request handlers. +$(HSMD_TEST_PROGRAMS): libcommon.a hsmd/libhsmd.o hsmd/hsm_utxo.o hsmd/hsmd_wiregen.o + +$(HSMD_TEST_OBJS): $(HSMD_HEADERS) $(HSMD_SRC) hsmd/test/Makefile + +hsmd-tests: $(HSMD_TEST_PROGRAMS:%=unittest/%) diff --git a/hsmd/test/run-bad-request-close.c b/hsmd/test/run-bad-request-close.c new file mode 100644 index 000000000000..78c96effc229 --- /dev/null +++ b/hsmd/test/run-bad-request-close.c @@ -0,0 +1,159 @@ +/* Test the client-connection lifecycle when a request is rejected: + * when libhsmd returns NULL from hsmd_handle_client_message, hsmd must + * close the client connection exactly once, and must not touch it + * again afterwards. + * + * That contract isn't observable from outside the process (the client + * just sees its fd close), so a black-box test can't verify it. + * Instead we compile hsmd.c into this test and watch the connection + * itself: + * + * 1. io_set_finish() on the client conn: ccan/io runs the finish + * callback exactly when the conn is closed, so counting calls tells + * us the conn was closed once (not zero times, not twice). + * + * 2. Every io_write_wire() from hsmd.c is intercepted (see the macro + * below), and a write to the client conn *after* its finish + * callback has run aborts the test. This makes any ordering + * violation fail deterministically, without needing ASan or + * valgrind to notice. + */ +#include "config.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* The client conn created in main(), watched by the checks below. */ +static struct io_conn *client_conn; + +/* How many times the client conn's finish callback has run. */ +static int finish_count; + +/* Distinct pointer for io_break, so main() knows why io_loop returned. */ +static char closed_token[] = "closed"; + +/* Wrapper around the real io_write_wire_: a closed conn (finish + * callback already ran) must never be written to again. */ +static struct io_plan *check_client_write(struct io_conn *conn, + const u8 *data, + struct io_plan *(*next)(struct io_conn *, void *), + void *next_arg) +{ + if (conn == client_conn && finish_count != 0) { + fprintf(stderr, + "write to client conn after close (finish_count=%d)\n", + finish_count); + abort(); + } + return io_write_wire_(conn, data, next, next_arg); +} + +/* Redefine io_write_wire before pulling in hsmd.c, so every write the + * daemon makes (req_reply in particular) goes through the check above. */ +#undef io_write_wire +#define io_write_wire(conn, data, next, arg) \ + check_client_write((conn), (data), \ + typesafe_cb_preargs(struct io_plan *, void *, \ + (next), (arg), \ + struct io_conn *), \ + (arg)) + +/* Include the daemon itself so we can drive its statics (new_client, + * handle_client via the io loop). Rename its main() out of the way. */ +int unused_main(int argc, char *argv[]); +#define main unused_main +#include "../hsmd.c" +#undef main +#undef io_write_wire + +/* Finish callback for the client conn: ccan/io calls this exactly when + * the conn is closed. Closing is what ends the test, so break out. */ +static void client_finished(struct io_conn *conn UNUSED, int *count) +{ + (*count)++; + io_break(closed_token); +} + +/* Any valid pubkey will do as the client's node id. */ +static void make_peer_id(struct node_id *id) +{ + struct privkey priv; + struct pubkey pub; + + memset(&priv, 1, sizeof(priv)); + assert(pubkey_from_privkey(&priv, &pub)); + node_id_from_pubkey(id, &pub); +} + +int main(int argc, char *argv[]) +{ + int status_fds[2], client_fds[2]; + struct node_id peer_id; + struct secret secret; + struct client *c; + const struct chainparams *params; + u8 hsmseed[32]; + u8 *msg; + void *ret; + + common_setup(argv[0]); + uintmap_init(&clients); + + /* hsmd reports bad requests to lightningd over status_conn; give + * it a socketpair whose other end we simply never read. */ + assert(socketpair(AF_LOCAL, SOCK_STREAM, 0, status_fds) == 0); + status_conn = daemon_conn_new(NULL, status_fds[0], NULL, NULL, NULL); + status_setup_async(status_conn); + + /* Initialize libhsmd's secrets from a dummy seed (normally done + * via the WIRE_HSMD_INIT message from lightningd). */ + params = chainparams_for_network("regtest"); + memset(hsmseed, 1, sizeof(hsmseed)); + msg = hsmd_init(hsmseed, sizeof(hsmseed), 6, + params->bip32_key_version, HSM_SECRET_PLAIN); + assert(msg); + /* hsmd_init returns take(): consume the marker, then free. */ + taken(msg); + tal_free(msg); + + /* Connect a fake per-channel client (as if lightningd had passed + * an fd to a channeld). dbid 1 = an ordinary channel client; + * HSM_PERM_COMMITMENT_POINT allows check_future_secret. */ + assert(socketpair(AF_LOCAL, SOCK_STREAM, 0, client_fds) == 0); + make_peer_id(&peer_id); + c = new_client(NULL, params, &peer_id, 1, + HSM_PERM_COMMITMENT_POINT, client_fds[0]); + client_conn = c->conn; + io_set_finish(c->conn, client_finished, &finish_count); + + /* Send a request libhsmd is guaranteed to reject: commitment + * index 2^48 is beyond the shachain, so per_commit_secret fails + * and the handler returns via hsmd_status_bad_request -> NULL. */ + memset(&secret, 0, sizeof(secret)); + msg = towire_hsmd_check_future_secret(NULL, 1ULL << SHACHAIN_BITS, + &secret); + assert(wire_sync_write(client_fds[1], take(msg))); + + /* Run the daemon's io loop. It reads the request, rejects it, + * and must close the client conn, which fires client_finished. */ + ret = io_loop(NULL, NULL); + assert(ret == closed_token); + + /* The heart of the test: closed exactly once. */ + assert(finish_count == 1); + + close(client_fds[1]); + close(status_fds[1]); + tal_free(status_conn); + hsmd_deinit(); + common_shutdown(); + return 0; +} From cd4f9a04b5ed343aaac433917c7dc3a71714eb4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Mon, 24 Aug 2026 17:30:30 -0300 Subject: [PATCH 6/9] offers: treat an unparseable reply path as absent, not fatal A remote peer supplies the reply path of an onion message, and its blinded hop count is a single unbounded byte. A zero-hop path parses to an empty hop array, which json_to_blinded_path rejects with NULL; offers then called plugin_err, and as an important builtin its death takes lightningd down with it. Drop a hopless reply path in lightningd when the message is decoded, and in offers log and ignore any reply path that fails to parse rather than aborting: a value that arrived from a remote peer must never terminate the plugin. Changelog-Fixed: plugins: `offers` no longer stops the node when an onion message carries a reply path with no hops. Co-Authored-By: Claude (cherry picked from commit 5fe925d2996bdcda04e3eebfb1fbabc9296de498) --- lightningd/onion_message.c | 6 ++++++ plugins/offers.c | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/lightningd/onion_message.c b/lightningd/onion_message.c index 70efb66e53d1..84d2f6a9b95e 100644 --- a/lightningd/onion_message.c +++ b/lightningd/onion_message.c @@ -133,6 +133,12 @@ void handle_onionmsg_to_us(struct lightningd *ld, const u8 *msg) } tal_free(submsg); + /* A reply path with no hops is unusable: treat it as absent. */ + if (payload->reply_path && tal_count(payload->reply_path->path) == 0) { + log_debug(ld->log, "Ignoring reply path with no hops"); + payload->reply_path = tal_free(payload->reply_path); + } + /* Make sure connectd gets this right. */ log_debug(ld->log, "Got onionmsg%s%s", payload->pathsecret ? " with pathsecret": "", diff --git a/plugins/offers.c b/plugins/offers.c index 2cd0c97f1a87..a214e5790f4d 100644 --- a/plugins/offers.c +++ b/plugins/offers.c @@ -306,8 +306,10 @@ static struct command_result *onion_message_recv(struct command *cmd, replytok = json_get_member(buf, om, "reply_blindedpath"); if (replytok) { reply_path = json_to_blinded_path(cmd, buf, replytok); + /* Remote-supplied: a bad reply path must not kill us. */ if (!reply_path) - plugin_err(cmd->plugin, "Invalid reply path %.*s?", + plugin_log(cmd->plugin, LOG_UNUSUAL, + "Ignoring invalid reply path %.*s", json_tok_full_len(replytok), json_tok_full(buf, replytok)); } From fd3e5d8cb430b43f8b718f76863784f710196ab5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?N=C3=ADckolas=20Goline?= Date: Mon, 24 Aug 2026 17:30:37 -0300 Subject: [PATCH 7/9] pytest: onion message reply path with no hops must not stop the node Regression test: inject an onion message whose blinded reply path declares num_hops=0 and check the node processes it and stays up. Co-Authored-By: Claude (cherry picked from commit 5580b1b9d8595e947504a1600181f9df1ef90615) --- tests/test_connection.py | 47 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/test_connection.py b/tests/test_connection.py index 0e672477c94d..baa3f1345464 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -3,7 +3,9 @@ from decimal import Decimal from pathlib import Path from pyln.client import RpcError, Millisatoshi +from pyln.proto.onion import TlvPayload import pyln.proto.wire as wire +from hashlib import sha256 from utils import ( only_one, wait_for, sync_blockheight, TIMEOUT, expected_peer_features, expected_node_features, @@ -15,6 +17,8 @@ ) from pyln.testing.utils import VALGRIND, EXPERIMENTAL_DUAL_FUND, FUNDAMOUNT, RUST, SLOW_MACHINE +import coincurve +import hmac import os import pytest import random @@ -4579,6 +4583,49 @@ def test_injectonionmessage(node_factory): l1.daemon.wait_for_log('lightningd: Got onionmsg with pathsecret') +def test_onionmessage_reply_path_no_hops(node_factory): + """A reply_path with num_hops=0 must not take the node down. + + The blinded reply_path's num_hops is a single attacker-supplied byte + with no lower bound; a zero-hop path serialises an empty "hops" array + to the offers plugin, which used to reject it via plugin_err and (as + an important builtin) stop lightningd. + """ + l1, l2 = node_factory.line_graph(2) + + def ecdh(privkey_bytes, pubkey_bytes): + priv = coincurve.PrivateKey(privkey_bytes) + return priv.ecdh(coincurve.PublicKey(pubkey_bytes).public_key) + + # Build an onion message to l2 with a reply_path carrying no hops. + l2_pub = bytes.fromhex(l2.info['id']) + blinding = coincurve.PrivateKey() + path_key = blinding.public_key.format(True) + + # Route-blinding tweak so the onion decrypts as l2's real key. + ss = ecdh(blinding.secret, l2_pub) + tweak = hmac.new(b'blinded_node_id', ss, sha256).digest() + blinded_pub = coincurve.PublicKey(l2_pub).multiply(tweak).format(True) + + # blinded_path: first_node_id (point), first_path_key (point), num_hops=0 + reply_path = l2_pub + path_key + b'\x00' + tlv = TlvPayload() + tlv.add_field(2, reply_path) + + onion = l1.rpc.createonion(hops=[{'pubkey': blinded_pub.hex(), + 'payload': tlv.to_bytes().hex()}], + assocdata="") + + l2.rpc.injectonionmessage(message=onion['onion'], path_key=path_key.hex()) + + # With the fix, lightningd drops the hopless reply path when it decodes + # the message (logged synchronously, before the offers hook runs), so the + # node stays up. Without the fix this log never appears: the offers plugin + # calls plugin_err and lightningd_exit takes the node down instead. + l2.daemon.wait_for_log('Ignoring reply path with no hops', timeout=30) + assert l2.rpc.getinfo()['id'] == l2.info['id'] + + def test_connect_ratelimit(node_factory, bitcoind): """l1 has 5 peers, restarts, make sure we limit""" # Sending nodes SIGSTOP at the wrong time makes connectd complain about From fde06cd1773c2db8d880fc53b93357c8fc8993ad Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 17 Aug 2026 11:38:53 +0930 Subject: [PATCH 8/9] gossipd: don't bother looking up ancient blocks. Signed-off-by: Rusty Russell (cherry picked from commit 511106d1cb0102e437c53650ce01823950c81a49) --- gossipd/gossmap_manage.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gossipd/gossmap_manage.c b/gossipd/gossmap_manage.c index 82a20a8bd617..3676fa9caaa6 100644 --- a/gossipd/gossmap_manage.c +++ b/gossipd/gossmap_manage.c @@ -669,6 +669,10 @@ const char *gossmap_manage_channel_announcement(const tal_t *ctx, if (!bitcoin_blkid_eq(&chain_hash, &chainparams->genesis_blockhash)) return NULL; + /* Immediately discard claims of ancient channels */ + if (short_channel_id_blocknum(scid) < chainparams->when_lightning_became_cool) + return tal_fmt(ctx, "Unknown UTXO %s", fmt_short_channel_id(tmpctx, scid)); + /* If a prior txout lookup failed there is little point it trying * again. Just drop the announcement and walk away whistling. * From 1e3a12d4ad070d8a6ff113c67bae5105521cb938 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 17 Aug 2026 11:39:03 +0930 Subject: [PATCH 9/9] lightningd: prefer latest blocks when querying historic blocks. Worst case, we can get flooded with 10,000 scids, and we work through them one at a time. In practice, there are more channels in more recent blocks, so we should bias towards those. The iteration is not free, but compared to all those getutxo calls with each block, it's invisible. ``` $ ./devtools/dump-gossipstore gossip_store-2026-08-17 | grep -v ' t=0 ' | sed -n 's/.*channel_announcement(\([0-9]*\)x.*).*/\1/p' | stats --histogram 962804************************************************************************ |******************************************************** |********************************** |*************** |********** |********** |******** |******* |******** |*** |* |* |*** 515419 ``` Signed-off-by: Rusty Russell (cherry picked from commit cf06f3d3a206df0d36ae240535af9c4582e73fc8) --- lightningd/bitcoind.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/lightningd/bitcoind.c b/lightningd/bitcoind.c index bc336c700aa2..317180e85ee5 100644 --- a/lightningd/bitcoind.c +++ b/lightningd/bitcoind.c @@ -778,8 +778,24 @@ static void process_getfilteredblock_step1(struct bitcoind *bitcoind, } } +/* Find the pending call for the highest block height: we prefer to + * satisfy the most recent request first, since it's usually the most + * urgent (e.g. catching up to the chain tip). */ +static struct filteredblock_call * +most_recent_filteredblock_call(struct bitcoind *bitcoind) +{ + struct filteredblock_call *c, *best = NULL; + + list_for_each(&bitcoind->pending_getfilteredblock, c, list) { + if (!best || c->height > best->height) + best = c; + } + return best; +} + /* Takes a call, dispatches it to all queued requests that match the same - * height, and then kicks off the next call. */ + * height, and then kicks off the call for the highest height still + * pending. */ static void process_getfiltered_block_final(struct bitcoind *bitcoind, const struct filteredblock_call *call) @@ -804,8 +820,8 @@ process_getfiltered_block_final(struct bitcoind *bitcoind, /* Nothing to free here, since `*call` was already deleted during the * iteration above. It was also removed from the list, so no need to * pop here. */ - if (!list_empty(&bitcoind->pending_getfilteredblock)) { - c = list_top(&bitcoind->pending_getfilteredblock, struct filteredblock_call, list); + c = most_recent_filteredblock_call(bitcoind); + if (c) { bitcoind_getrawblockbyheight(bitcoind, bitcoind, c->height, process_getfilteredblock_step1, c); }