Bump rust-lightning to include trampoline changes by carlaKC · Pull Request #825 · lightningdevkit/ldk-node

carlaKC · 2026-03-13T17:38:33Z

~~Also needs the following patch on top of 2026-02-ldk-node-base in bitcoin-payment-instructions.~~
Pointing to my own bitcoin-payment-instructions based on previous branch as discussed offline.

ldk-reviews-bot · 2026-03-13T17:38:38Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

carlaKC · 2026-03-13T17:46:09Z

src/event.rs

+/// A set of multiple htlcs all associated with same forward.
+#[derive(Debug, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
+pub struct HTLCSet(Vec<HTLCLocator>);


Interested in some feedback here 🙏

To use default_value to fill in legacy fields when the new field isn't present, we need to implement Readable/Writeable for Vec<HTLCLocator>. We can't do this because we don't own the trait or the type (orphan rule).

The solution I've gone with here is to use a wrapper struct and pull in the code we have in impl_for_vec in LDK.

Alternatives would be:

Don't use default_value, and write manual Readable/Writeable impls for Event so that we can manually fill the legacy values (I can't find a way to do it within macros).

Just use required_vec and don't fill legacy values (seems bad).

carlaKC · 2026-03-13T17:46:52Z

src/event.rs

+		// For backwards compatibility, write the first prev/next_htlc to our legacy fields. This
+		// allows use to downgrade with some information loss about the remaining htlcs.


Not sure what the project's philosophy is for backwards compatibility, so just gone with what I would have done in LDK - happy to do something else if it's preferred!

Yeah, while we def. want to get to support downgrades at some point, we currently don't. So we can just drop this extra logic as long as we're positive backwards compatibility during upgrades is ensured.

jkczyz · 2026-03-16T17:11:52Z

Cargo.toml

+# TODO: need to push branch to Jeff's fork?
+bitcoin-payment-instructions = { git = "https://github.com/carlaKC/bitcoin-payment-instructions", rev = "c22c9b836b70d4c915dd28701e11083a8b558d56" }


@tnull had been doing most of the updates until recently when my PRs (and others) started breaking LDK Node. I ended up setting up a remote to his branch and based mine off of it. Personally, I'd be fine if you want to do something similar, as that at least gives us a common history for bitcoin-payment-instructions bumps.

tnull

Thanks for looking into this! Needs a rebase by now as #824 was merged.

tnull · 2026-03-19T08:30:50Z

Cargo.toml

 #lightning-transaction-sync = { path = "../rust-lightning/lightning-transaction-sync" }
 #lightning-liquidity = { path = "../rust-lightning/lightning-liquidity" }
-#lightning-macros = { path = "../rust-lightning/lightning-macros" }
+lightning-macros = { path = "../rust-lightning/lightning-macros" }


nit: This seems unrelated.

tnull · 2026-03-19T08:42:43Z

src/event.rs

+	pub node_id: Option<PublicKey>,
+}
+
+impl_writeable_tlv_based!(HTLCLocator, {


Hmm, I'd really like to avoid duplicating upstream's serialization logic, which always risks to get out-of-sync. Can we rather also impl From<Vec<HTLCLocator>> for Vec<LdkHTLCLocator> and use upstream's write? I admit that is also not great, but maybe preferable to duplicating even more logic?

I admit that is also not great, but maybe preferable to duplicating even more logic?

Given this a shot, it's a little unwieldy but I agree probably preferable to duplication.

Indeed unwieldy and not loving the reallocation either, but oh well...

But we can now remove the impl_writeable_tlv_based here, right?

tnull · 2026-03-19T08:44:19Z

src/event.rs


+/// A set of multiple htlcs all associated with same forward.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct HTLCSet(pub Vec<HTLCLocator>);


Why do we need this additional newtype? I think even for bindings we should be able to leave it as Vec<HTLCLocator>?

This was to get around the orphan rule for serialization (we can't implement a trait we don't own - Readable/Writeable - for a type we don't own - Vec<T>).

If we don't need backwards compat, then we don't need default_value and can clean up a whole bunch of things including this 👌

tnull · 2026-03-19T08:44:51Z

src/event.rs

+	(5, node_id, option),
+});
+
+impl From<lightning::events::HTLCLocator> for HTLCLocator {


nit: Please import the upstream type as LdkHTLCLocator, which is the usual pattern.

tnull · 2026-03-19T08:46:13Z

src/event.rs

+		// For backwards compatibility, write the first prev/next_htlc to our legacy fields. This
+		// allows use to downgrade with some information loss about the remaining htlcs.


Yeah, while we def. want to get to support downgrades at some point, we currently don't. So we can just drop this extra logic as long as we're positive backwards compatibility during upgrades is ensured.

tnull

Hmm, I do wonder if we could improve things and reuse the upstream struct entirely if we introduced UserChannelId there.

tnull · 2026-03-20T15:48:52Z

src/event.rs

+	pub node_id: Option<PublicKey>,
+}
+
+impl_writeable_tlv_based!(HTLCLocator, {


Indeed unwieldy and not loving the reallocation either, but oh well...

But we can now remove the impl_writeable_tlv_based here, right?

tnull · 2026-03-20T16:05:31Z

src/event.rs

-						.await;
+					for next_htlc in next_htlcs.iter() {
+						liquidity_source
+							.handle_payment_forwarded(Some(next_htlc.channel_id), skimmed_fee_msat)


Hmm, wouldn't this have us / the accounting logic believe that we withheld the skimmed amount multiple times?

Hm yes, but I don't think we can hit it unless lsps2 supports opening multiple JIT channels and also being a trampoline at the same time.

Would you be okay with passing multiple next_htlc.channel_id to handle_payment_forwarded and then just logging an error if there's more than one?

tnull · 2026-03-20T16:06:08Z

src/event.rs

Does this also need adjustments?

tnull · 2026-03-20T16:07:18Z

src/event.rs

+		// We cannot implement Readable/Writeable for Vec<HTLCLocator> because we do not own the
+		// trait or the type (Vec). To work around this, and prevent duplicating serialization code,
+		// we map to the underlying LdkHTLCLocator type for serialization.
+		(15, prev_htlcs, (custom, Vec<LdkHTLCLocator>,


Can we try to streamline this a bit? Maybe introduce a helper method to dedup the code? At the very least it seems we can drop a few lines:

diff --git a/src/event.rs b/src/event.rs index 417f5941..0505a870 100644 --- a/src/event.rs +++ b/src/event.rs @@ -369,16 +369,14 @@ impl_writeable_tlv_based_enum!(Event, // we map to the underlying LdkHTLCLocator type for serialization. (15, prev_htlcs, (custom, Vec<LdkHTLCLocator>, |v: Option<Vec<LdkHTLCLocator>>| { - let res: Result<Vec<HTLCLocator>, lightning::ln::msgs::DecodeError> = - Ok(v.map(|ldk_vec| ldk_vec.into_iter().map(HTLCLocator::from).collect()) + Ok(v.map(|ldk_vec| ldk_vec.into_iter().map(HTLCLocator::from).collect()) .unwrap_or_else(|| { legacy_prev_channel_id.map(|ch| vec![HTLCLocator { channel_id: ch, user_channel_id: legacy_prev_user_channel_id.map(UserChannelId), node_id: legacy_prev_node_id, }]).unwrap_or_default() - })); - res + })) }, |us: &Event| { if let Event::PaymentForwarded { ref prev_htlcs, .. } = us { @@ -390,7 +388,6 @@ impl_writeable_tlv_based_enum!(Event, )), (17, next_htlcs, (custom, Vec<LdkHTLCLocator>, |v: Option<Vec<LdkHTLCLocator>>| { - let res: Result<Vec<HTLCLocator>, lightning::ln::msgs::DecodeError> = Ok(v.map(|ldk_vec| ldk_vec.into_iter().map(HTLCLocator::from).collect()) .unwrap_or_else(|| { legacy_next_channel_id.map(|ch| vec![HTLCLocator { @@ -398,8 +395,7 @@ impl_writeable_tlv_based_enum!(Event, user_channel_id: legacy_next_user_channel_id.map(UserChannelId), node_id: legacy_next_node_id, }]).unwrap_or_default() - })); - res + })) }, |us: &Event| { if let Event::PaymentForwarded { ref next_htlcs, .. } = us {

tnull · 2026-03-20T16:08:07Z

src/event.rs

+}
+
+impl_writeable_tlv_based!(HTLCLocator, {
+	(1, channel_id, required),


Should we make these even, given its a new struct?

In LDK I'd only go with an even TLV if not understanding the field means that we'll lose money, because odd gives us much more flexibility for making changes in the future if needed. Happy to change to even if that's not the philosophy here!

because odd gives us much more flexibility for making changes in the future if needed

Hmm, you probably can't ever really repurpose an odd field in LDK either so, it's mostly about upgrades/backwards compat of pre-existing objects, no?

Yea the reason we've been doing it in the lightning crate is because it means were could remove them in a future version, making it a good default.

In preparation for trampoline, rust-lightning has been updated to report multiple HTLCs incoming and outgoing in PaymentForwarded. Support is not yet complete, so we can still expect that we only get a single HTLC in/out for now. This change writes the first entry of the new fields to our PaymentForwarded event, because we don't yet have the serialization macros we need to persist a vec with a default value (as the trait and the type Vec<T> are not owned by this crate). We're not losing any information here, as we only expect regular forwards with this version of LDK. We'll follow with a change that writes the full vec once upstream changes are made to LDK's serialization macros.

carlaKC · 2026-03-24T13:29:38Z

Updated to assume that we always have one HTLC in/out (which is true for the version of LDK, because we haven't turned trampoline on yet). Will follow with changes to properly write the full vec.

tnull

LGTM, thanks!

carlaKC commented Mar 13, 2026

View reviewed changes

carlaKC force-pushed the trampoline-deps branch 2 times, most recently from 76e3d44 to dd42f5f Compare March 13, 2026 17:56

jkczyz reviewed Mar 16, 2026

View reviewed changes

tnull self-requested a review March 17, 2026 09:23

tnull mentioned this pull request Mar 19, 2026

Bump rust-lightning to 49912057895ddfbd69d503de67c80d5576c09953 #833

Closed

tnull reviewed Mar 19, 2026

View reviewed changes

carlaKC force-pushed the trampoline-deps branch from dd42f5f to e60ce2e Compare March 19, 2026 14:26

carlaKC marked this pull request as ready for review March 19, 2026 19:21

carlaKC requested a review from tnull March 19, 2026 19:21

tnull reviewed Mar 20, 2026

View reviewed changes

carlaKC force-pushed the trampoline-deps branch from e60ce2e to 8e716d7 Compare March 23, 2026 21:02

This was referenced Mar 23, 2026

util: add default_value_vec for defaults without LengthReadable lightningdevkit/rust-lightning#4507

Draft

Trampoline prev/next_htlcs serialization #838

Open

carlaKC force-pushed the trampoline-deps branch from 8e716d7 to 130a27b Compare March 24, 2026 13:26

tnull approved these changes Mar 24, 2026

View reviewed changes

tnull merged commit 63c7f9b into lightningdevkit:main Mar 24, 2026
19 checks passed

		// For backwards compatibility, write the first prev/next_htlc to our legacy fields. This
		// allows use to downgrade with some information loss about the remaining htlcs.

		# TODO: need to push branch to Jeff's fork?
		bitcoin-payment-instructions = { git = "https://github.com/carlaKC/bitcoin-payment-instructions", rev = "c22c9b836b70d4c915dd28701e11083a8b558d56" }

Conversation

carlaKC commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlaKC commented Mar 24, 2026

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

carlaKC commented Mar 13, 2026 •

edited

Loading

ldk-reviews-bot commented Mar 13, 2026 •

edited

Loading