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

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

Depends on #824.

Also needs the following patch on top of 2026-02-ldk-node-base in bitcoin-payment-instructions.

diff --git a/Cargo.toml b/Cargo.toml
index de128db..811cd36 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -21,8 +21,8 @@ default = ["std"]
 [dependencies]
 #lightning-invoice = { version = "0.34", default-features = false }
 #lightning = { version = "0.2.0", default-features = false, features = ["dnssec"] }
-lightning-invoice = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "98393b3de3d8aec897e9ab783cb2418da504e204", default-features = false }
-lightning = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "98393b3de3d8aec897e9ab783cb2418da504e204", default-features = false, features = ["dnssec"] }
+lightning-invoice = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "128ead25e8d62b0262b71711631220983c48756a", default-features = false }
+lightning = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "128ead25e8d62b0262b71711631220983c48756a", default-features = false, features = ["dnssec"] }
 bitcoin = { version = "0.32", default-features = false }
 getrandom = { version = "0.3", default-features = false, optional = true }
 dnssec-prover = { version = "0.6", default-features = false, optional = true, features = ["validation", "std", "tokio"] }
@@ -34,5 +34,5 @@ serde_json = { version = "1.0", default-features = false, optional = true, featu
 tokio = { version = "1.0", default-features = false, features = ["rt", "macros"] }
 #lightning = { version = "0.2.0", features = ["std"] }
 #lightning-net-tokio = { version = "0.2.0", default-features = false }
-lightning = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "98393b3de3d8aec897e9ab783cb2418da504e204", features = ["std"] }
-lightning-net-tokio = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "98393b3de3d8aec897e9ab783cb2418da504e204", default-features = false }
+lightning = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "128ead25e8d62b0262b71711631220983c48756a", features = ["std"] }
+lightning-net-tokio = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "128ead25e8d62b0262b71711631220983c48756a", default-features = false }```

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?

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

src/event.rs

 					if claim_from_onchain_tx {
 						log_info!(
 						self.logger,
 						"Forwarded payment{}{} of {}msat, earning {}msat in fees from claiming onchain.",


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?

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

Update rust lightning version to include trampoline changes

e60ce2e

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

		// 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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Mar 13, 2026 •

edited

Loading