Set custom tlvs to `update_add_htlc` via the `htlc_accepted_hook` by nepet · Pull Request #8433 · ElementsProject/lightning

nepet · 2025-07-30T14:53:54Z

Important

25.09 FREEZE July 28TH: Non-bugfix PRs not ready by this date will wait for 25.12.

RC1 is scheduled on August 11th

The final release is scheduled for September 1st.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

The changelog has been updated in the relevant commit(s) according to the guidelines.
Tests have been added or modified to reflect the changes.
Documentation has been reviewed and updated as needed.
Related issues have been listed and linked, including any that this PR closes.

This PR allows plugins that registered to th htlc_accepted_hook to get and replace the TLV-stream update_add_htlc_tlvs attached to incomming update_add_htlc messages, adding a new HTLC.

If a plugin want's to replace the TLV-stream with custom TLVS, it needs to return

{
    "result": "continue",
    "extra_tlvs": [hex-encoded string]
}

Specifying extra_tlvs will replace the TLV-stream attached to the HTLC (also the blinding path-key) and will be - in case of a forward - forwarded as the update_add_htlc_tlvs to the peer.

This PR is a necessary precondition to implement the LSPS2 protocol and Resolves #6663

madelinevibes · 2025-08-07T09:07:15Z

this PR is necessary for LSPS2 - doesn't conclude LSPS2.

cdecker · 2025-08-12T13:10:30Z

Excellent change @nepet 🚀
I just left a couple of comments and clarifications. The one that we should likely address directly is the TAKES annotation which can lead to a memory leak unless I'm mistaken, everything else is small enough to end up in a followup PR.

cdecker · 2025-08-12T12:55:19Z

common/htlc_wire.c

 	else
 		existing->failed = NULL;
+	if (extra_tlvs) {
+		existing->extra_tlvs = tal_dup_talarr(existing, struct tlv_field, extra_tlvs);


With the function called with take() on the extra_htlcs this ends up reparenting the array, but then creates copies in the for loop below. Leaving the old .value stranded. Please don't make the type TAKES, it might result in slightly more copies, but it means we can tal_free() it in the caller and clean up the child allocations along with it, which would otherwise be left dangling here.

And yes, tal_dup_talarr being magic in the sense it sometimes just reparents is weird :-)

Yes, but a flat array with pointers is already playing dangerously. I've fixed this though (existing callers don't care, but it's still good).

cdecker · 2025-08-12T12:57:11Z

common/htlc_wire.c

+		u8 *tmp_pptr = tal_arr(tmpctx, u8, 0);
+		towire_tlvstream_raw(&tmp_pptr, added->extra_tlvs);
+
+		towire_bool(pptr, true);
+		towire_u16(pptr, tal_bytelen(tmp_pptr));
+		towire_u8_array(pptr, tmp_pptr,
+				tal_bytelen(tmp_pptr));


Since we're doing this at least twice (manual serialization), maybe we want to encapsulate this in a towire_tlvstream_prefixed? Probably for a followup PR, not necessary here.

lightningd/htlc_end.h

cdecker · 2025-08-12T13:06:07Z

wallet/wallet.c

+	/* FIXME: save extra_tlvs in db! But: check the implications that a
+	 * spammy peer - giving us big extra tlvs - would have on our database.
+	 * Right now, not saving the extra tlvs in the db seems OK as it is
+	 * only relevant in the case that I forward but restart in the middle
+	 * of a payment.
+	 */


Notice that messages in the LN protocol are limited to 65KiB, and update_add_htlc contains at least a 1365B onion, so 64KiB is the natural limit for the extra_tlvs (and there are other fields that constrain further), as such a limit on pending HTLCs is likely sufficient to also limit the DB space used for extra TLVs.

cdecker · 2025-08-12T13:07:05Z

tests/plugins/htlc_accepted-customtlv.py

+@plugin.init()
+def on_init(**kwargs):
+    global custom_tlvs
+    custom_tlvs = None


This seems to have no effect, with the above declaration of custom_tlvs.

rustyrussell

I added the changed Christian suggested (at the end).

rustyrussell · 2025-08-14T01:49:38Z

common/htlc_wire.c

 	else
 		existing->failed = NULL;
+	if (extra_tlvs) {
+		existing->extra_tlvs = tal_dup_talarr(existing, struct tlv_field, extra_tlvs);


Yes, but a flat array with pointers is already playing dangerously. I've fixed this though (existing callers don't care, but it's still good).

lightningd/htlc_end.h

We currently only consider known tlv types in the internal representation of a htlc. This commit adds the remaining unknown tlv fields to the htlc as well. This is in prepareation to forward these to the htlc_accepted_hook. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

This appends the extra_tlvs to the internal wire htlcs "added" and "existing" for the extra tlvs to be handed to lightningd. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

This appends the extra_tlvs to the internal channeld_offer_htlc wire msg. We also recombine the extra_tlvs with the blinded path key for forwarding htlcs. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

Add serializing and deserializing of the extra tlvs to to the htlc_accepted_hook to allow plugin users to replace the tlv stream that is attached to the update_add_htlc message on forwards. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

Adds some testcases for custom tlvs, set by a htlc_accepted_hook. We check that the custom tlvs replace the update_add_htlc_tlvs and get forwarded to the peer. We also check that a malformed tlv will result in a **BROKEN** behaviour. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

Changelog-Added: The `htlc_accepted_hook` now gets the TLV-stream attached to the HTLC passed through as `extra_tlvs` and can replace it. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

There was a problem with a ‘highlight’ that was misunderstood as a spelling mistake in lib-wally. Since ‘hightlight’ is already filtered out, we simply instruct grep to ignore upper/lower case when filtering. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

The rare case happened where a lockfile sha-sum contained a "Ctlv" which spell-check complained about. Stupid lockfiles that don't know it is actually "cltv"! Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

Reported-by: Christian Decker Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

And make sure we check the length properly in fromwire! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2025-08-14T05:21:24Z

New flake8 (thanks uv) got stricter, so fixed test plugin.

nepet force-pushed the 2519-extra-tlv-on-htlc_accepted-hook branch 7 times, most recently from a9bd415 to b0c1c55 Compare July 31, 2025 19:07

madelinevibes added this to the v25.09 milestone Aug 7, 2025

madelinevibes requested review from cdecker and rustyrussell and removed request for cdecker August 7, 2025 09:12

madelinevibes added the Status::Ready for Review The work has been completed and is now awaiting evaluation or approval. label Aug 11, 2025

cdecker reviewed Aug 12, 2025

View reviewed changes

cdecker approved these changes Aug 12, 2025

View reviewed changes

rustyrussell approved these changes Aug 14, 2025

View reviewed changes

nepet and others added 10 commits August 14, 2025 14:46

channeld: Add extra_tlvs to wire htlcs

ab8b83f

This appends the extra_tlvs to the internal wire htlcs "added" and "existing" for the extra tlvs to be handed to lightningd. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

channeld: Add extra_tlvs to incomming wire msg

604af9c

This appends the extra_tlvs to the internal channeld_offer_htlc wire msg. We also recombine the extra_tlvs with the blinded path key for forwarding htlcs. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

docs: Add extra_tlvs to the htlc_accepted_hook doc

2f9904d

Changelog-Added: The `htlc_accepted_hook` now gets the TLV-stream attached to the HTLC passed through as `extra_tlvs` and can replace it. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

tools: Remove lockfiles from spell-checking

3a3ece0

The rare case happened where a lockfile sha-sum contained a "Ctlv" which spell-check complained about. Stupid lockfiles that don't know it is actually "cltv"! Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>

common: handle taken() extra_tlvs in new_existing_htlc properly.

1a076ce

Reported-by: Christian Decker Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common/htlc_wire: add towire/fromwire helpers for wrapped tlv streams.

7c929d7

And make sure we check the length properly in fromwire! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the 2519-extra-tlv-on-htlc_accepted-hook branch from a0045af to 7c929d7 Compare August 14, 2025 05:21

rustyrussell enabled auto-merge (rebase) August 14, 2025 05:21

rustyrussell merged commit 6fbc5d0 into ElementsProject:master Aug 14, 2025
146 of 154 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set custom tlvs to `update_add_htlc` via the `htlc_accepted_hook`#8433

Set custom tlvs to `update_add_htlc` via the `htlc_accepted_hook`#8433
rustyrussell merged 10 commits intoElementsProject:masterfrom
nepet:2519-extra-tlv-on-htlc_accepted-hook

nepet commented Jul 30, 2025

Uh oh!

madelinevibes commented Aug 7, 2025

Uh oh!

cdecker commented Aug 12, 2025

Uh oh!

cdecker Aug 12, 2025

Uh oh!

cdecker Aug 12, 2025

Uh oh!

rustyrussell Aug 14, 2025

Uh oh!

cdecker Aug 12, 2025

Uh oh!

Uh oh!

cdecker Aug 12, 2025

Uh oh!

cdecker Aug 12, 2025

Uh oh!

rustyrussell left a comment

Uh oh!

rustyrussell Aug 14, 2025

Uh oh!

Uh oh!

rustyrussell commented Aug 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

nepet commented Jul 30, 2025

Checklist

Uh oh!

madelinevibes commented Aug 7, 2025

Uh oh!

cdecker commented Aug 12, 2025

Uh oh!

cdecker Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

cdecker Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

rustyrussell Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

cdecker Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cdecker Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

cdecker Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

rustyrussell Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rustyrussell commented Aug 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants