Insert channel funding outputs into Wallet #726

benthecarman · 2025-12-03T19:53:56Z

When doing a splice, to properly calculate fees we need the channel's funding utxo in our wallet, otherwise our wallet won't know the channel's original size. This adds the channel funding txo on ChannelReady events and modifies the splicing test to make sure we can calculate fees on splice-ins.

ldk-reviews-bot · 2025-12-03T19:53:59Z

👋 Thanks for assigning @jkczyz 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.

tnull

Makes sense to me. Do we think this is worth a backport for 0.7 or are we fine waiting for 0.8 with shipping this?

The different cases in the event handler could use a comment or two to explain when we'd land on which side of the if/else clauses there. Also, we really need to be careful with just replaying events in cases where we don't expect immediate resolution of the error on retry, as we'd otherwise just spin and block any other event processing. This could also use better test coverage to show why we need this change in the first place.

Besides that, also pinging @jkczyz as a second reviewer on this.

tnull · 2025-12-04T07:58:20Z

src/event.rs

+								self.logger,
+								"Failed to find channel info for pending channel {channel_id} with counterparty {counterparty_node_id}"
+							);
+							return Err(ReplayEvent());


This would induce an infinite loop in event handling, no? Rather than doing this, we probably need to just log the error for now, and abort the flow when we can with 0.3? Also, would it make sense to add a debug_assert here, given that we expect this always to be avaialble? (same below)

The error below can only happen on a persistence failure, elsewhere in the event handling we replay on those errors so I think it makes sense to keep as is.

For this one, it should always be available so I will just change to a debug_assert

tnull · 2025-12-04T07:59:12Z

src/event.rs

+						},
+					}
 				} else {
 					log_info!(


In what case would we end up in this else branch, and would we need to do something related to the redeemscript, too?

According to the docs, the funding_txo "Will be None if the channel's funding transaction reached an acceptable depth prior to version 0.2". Since this was also added in 0.2, the redeem script would also be None so there shouldn't be anything to add. Either way, we need the funding txo to be able to insert so we can't add it.

tnull · 2025-12-04T08:03:58Z

tests/integration_tests_rust.rs

 	node_b.splice_in(&user_channel_id_b, node_a.node_id(), 4_000_000).unwrap();

-	expect_splice_pending_event!(node_a, node_b.node_id());
+	let txo = expect_splice_pending_event!(node_a, node_b.node_id());


These test changes pass on main. Can you add coverage for the cases where the previous approach would lead to inaccurate fee estimations?

The bug/issue only exists with bitcoind syncing so to do so, we'd have to change the test to sync with bitcoind rpc. Is that wanted?

The bug/issue only exists with bitcoind syncing so to do so, we'd have to change the test to sync with bitcoind rpc. Is that wanted?

Sure, feel free to do so. Alternatively, you could also add a _bitcoind variant test case (ofc reusing the logic) so we have two test cases for bitcoind and esplora - that's if we think there would be more chain source specific edge cases.

tnull · 2025-12-04T08:08:56Z

src/ffi/types.rs

 	}
 }

+impl UniffiCustomTypeConverter for ScriptBuf {


nit: While this should be fine for now, we generally try to get away from the UniffiCustomTypeConverter approach, and instead convert more and more objects to proper interfaces.

ldk-reviews-bot · 2025-12-06T08:12:07Z

🔔 1st Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-12-08T08:12:56Z

🔔 2nd Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2025-12-08T23:43:51Z

src/event.rs

+							);
+						},
+						Some(output) => {
+							if let Err(e) = self.wallet.insert_txo(funding_txo, output) {


Hmmm... I guess we didn't really need the redeem script in ChannelPending and SplicePending since we only need the previous funding output in the wallet for the next splice. Might have been better in ChannelReady. We'd be able to avoid the look-up, although it wouldn't remove the Option check.

Yeah sadly I messed up here, i think we needed to add the redeem script along with the new channel size so we had the full utxo information

Yeah, though if we insert the previous funding txo when initiating the splice as mentioned in the other comment, then I think we'll have all the necessary information in ChannelDetails? Essentially, we'd be lazily adding the previous funding output to the wallet when initiating the splice.

Made so we add it before a splice-in. I still think this one is good to keep so if you're doing any other wallet related action, it will have a more complete set of information and you don't need to initiate a splice for it to have the channel information

I still think this one is good to keep so if you're doing any other wallet related action, it will have a more complete set of information and you don't need to initiate a splice for it to have the channel information

Could you provide an example for what else we would use this information? If we don't need it I'd prefer to drop this change to declutter the logic here and avoid the extra persistence roundtrip.

jkczyz · 2025-12-08T23:54:00Z

src/event.rs

+					let chans =
+						self.channel_manager.list_channels_with_counterparty(&counterparty_node_id);
+					let chan_output = chans
+						.iter()
+						.find(|c| c.user_channel_id == user_channel_id)
+						.and_then(|c| c.get_funding_output());


Doing this in ChannelReady wouldn't work for splicing preexisting channels. Wonder if we should instead do this when initiating a splice? There we already look up the channel. It would also let us use the real shared_input when selecting UTXOs.

Doing this in ChannelReady wouldn't work for splicing preexisting channels.

I dont think this is right, in the docs it says we get a ChannelReady for splices, also my test seems to confirm

Correct, we do get it for spliced channels, but we need the initial funding output because that is the one being spent. And for channels existing before this change, we would have already processed the ChannelReady event for the initial funding.

ah i see for backward compat sake, added!

jkczyz · 2025-12-09T17:26:59Z

src/lib.rs

 			let shared_output = bitcoin::TxOut {
 				value: shared_input.previous_utxo.value + Amount::from_sat(splice_amount_sats),
-				script_pubkey: make_funding_redeemscript(&dummy_pubkey, &dummy_pubkey).to_p2wsh(),
+				script_pubkey: funding_output.script_pubkey,


This should still be the dummy one or note that it is not really the shared output's script_pubkey. I guess the amount is already fake since we don't know what the counterparty will contribute.

added a comment

jkczyz · 2025-12-09T17:32:08Z

src/lib.rs

+			self.wallet.insert_txo(funding_txo.into_bitcoin_outpoint(), funding_output).map_err(
+				|e| {
+					log_error!(self.logger, "Failed to splice channel: {:?}", e);
+					Error::ChannelSplicingFailed
+				},
+			)?;


@tnull Don't know what the convention is here. Should we replace the Error::PersistenceFailed here with Error::ChannelSplicingFailed?

Yeah, I think it's preferable to bubble up the PersistenceFailed error.

…lices LDK gives us the actual funding output so we no longer need to create a dummy one with fake pubkeys

We insert a channel's funding utxo into our wallet so we can later calculate the fees for the transaction, otherwise our wallet would have incomplete information. We do it before the splice-in and not just on the ChannelReady event to ensure better backwards compatibility.

When doing a splice, to properly calculate fees we need the channel's funding utxo in our wallet, otherwise our wallet won't know the channel's original size. This adds the channel funding txo on ChannelReady events and modifies the splicing test to make sure we can calculate fees on splice-ins.

Exposes the funding_redeem_script that LDK already exposes

tnull

Excuse the delay here

tnull · 2025-12-12T13:19:59Z

src/lib.rs

+			self.wallet.insert_txo(funding_txo.into_bitcoin_outpoint(), funding_output).map_err(
+				|e| {
+					log_error!(self.logger, "Failed to splice channel: {:?}", e);
+					Error::ChannelSplicingFailed
+				},
+			)?;


Yeah, I think it's preferable to bubble up the PersistenceFailed error.

tnull · 2025-12-12T13:24:12Z

src/event.rs

+							);
+						},
+						Some(output) => {
+							if let Err(e) = self.wallet.insert_txo(funding_txo, output) {


I still think this one is good to keep so if you're doing any other wallet related action, it will have a more complete set of information and you don't need to initiate a splice for it to have the channel information

Could you provide an example for what else we would use this information? If we don't need it I'd prefer to drop this change to declutter the logic here and avoid the extra persistence roundtrip.

tnull · 2025-12-12T13:25:25Z

tests/integration_tests_rust.rs

 	node_b.splice_in(&user_channel_id_b, node_a.node_id(), 4_000_000).unwrap();

-	expect_splice_pending_event!(node_a, node_b.node_id());
+	let txo = expect_splice_pending_event!(node_a, node_b.node_id());


The bug/issue only exists with bitcoind syncing so to do so, we'd have to change the test to sync with bitcoind rpc. Is that wanted?

Sure, feel free to do so. Alternatively, you could also add a _bitcoind variant test case (ofc reusing the logic) so we have two test cases for bitcoind and esplora - that's if we think there would be more chain source specific edge cases.

tnull · 2025-12-12T13:27:43Z

src/types.rs

+	/// When a channel is spliced, this continues to refer to the original pre-splice channel
+	/// state until the splice transaction reaches sufficient confirmations to be locked (and we
+	/// exchange `splice_locked` messages with our peer).
+	pub funding_redeem_script: Option<bitcoin::ScriptBuf>,


nit: Please import ScriptBuf instead of using the bitcoin:: prefix.

benthecarman requested a review from tnull December 3, 2025 19:53

benthecarman force-pushed the add-chan-to-wallet branch 2 times, most recently from 091e391 to d7f8bc9 Compare December 3, 2025 20:14

tnull reviewed Dec 4, 2025

View reviewed changes

tnull requested a review from jkczyz December 4, 2025 08:11

benthecarman force-pushed the add-chan-to-wallet branch from d7f8bc9 to b2c3793 Compare December 4, 2025 19:29

jkczyz reviewed Dec 8, 2025

View reviewed changes

benthecarman force-pushed the add-chan-to-wallet branch from b2c3793 to 8518f3d Compare December 9, 2025 07:41

jkczyz reviewed Dec 9, 2025

View reviewed changes

benthecarman added 4 commits December 9, 2025 13:06

Use actual funding output when constructing shared input/output in sp…

31c12f9

…lices LDK gives us the actual funding output so we no longer need to create a dummy one with fake pubkeys

Add funding_redeem_script to ChannelDetails

4c3450e

Exposes the funding_redeem_script that LDK already exposes

benthecarman force-pushed the add-chan-to-wallet branch from 8518f3d to 4c3450e Compare December 9, 2025 20:21

tnull reviewed Dec 12, 2025

View reviewed changes

Insert channel funding outputs into Wallet #726

Are you sure you want to change the base?

Insert channel funding outputs into Wallet #726

Uh oh!

Conversation

benthecarman commented Dec 3, 2025

Uh oh!

ldk-reviews-bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

benthecarman Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benthecarman Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ldk-reviews-bot commented Dec 6, 2025

Uh oh!

ldk-reviews-bot commented Dec 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benthecarman Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Reviewers

Assignees

Labels

ldk-reviews-bot commented Dec 3, 2025 •

edited

Loading

tnull left a comment •

edited

Loading

benthecarman Dec 4, 2025 •

edited

Loading

benthecarman Dec 4, 2025 •

edited

Loading

benthecarman Dec 9, 2025 •

edited

Loading