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 to the wallet before a splice as we only really need this information for splices and not for all channels.

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.

Okay yeah on further reflection, no real use case, especially with the api that ldk-node exposes to the wallet.

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

src/lib.rs

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

tnull

Excuse the delay here

src/lib.rs

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.

src/types.rs

benthecarman · 2025-12-12T19:27:52Z

Addressed review, ended up removing it on events and just doing it before a splice. Made sure test coverage hits on both backends and for splice in and out

src/lib.rs

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 as we only really need this information for splices and not for all channels.

Exposes the funding_redeem_script that LDK already exposes

tnull

LGTM, one nit. Otherwise should be ready pending @jkczyz's review.

tnull · 2025-12-15T08:37:52Z

tests/integration_tests_rust.rs


+#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+async fn splice_channel() {
+	run_splice_channel_test(false).await;


nit: Not that it matters much, but elsewhere in LDK Node / LDK tests the usual pattern would be to name the 'inner' test methods do_splice_channel. Feel free to make that rename if you touch the code again.

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

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

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

benthecarman force-pushed the add-chan-to-wallet branch from 4c3450e to 95c0819 Compare December 12, 2025 19:27

jkczyz reviewed Dec 12, 2025

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

benthecarman added 2 commits December 12, 2025 23:02

Add funding_redeem_script to ChannelDetails

9b325ff

Exposes the funding_redeem_script that LDK already exposes

benthecarman force-pushed the add-chan-to-wallet branch from 95c0819 to 9b325ff Compare December 13, 2025 05:02

tnull approved these changes Dec 15, 2025

View reviewed changes

jkczyz approved these changes Dec 15, 2025

View reviewed changes

tnull merged commit bbefa73 into lightningdevkit:main Dec 16, 2025
18 checks passed

benthecarman deleted the add-chan-to-wallet branch December 16, 2025 09:02

Insert channel funding outputs into Wallet #726

Insert channel funding outputs into Wallet #726

Uh oh!

Conversation

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

Uh oh!

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!

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!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benthecarman commented Dec 12, 2025

Uh oh!

Uh oh!

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

benthecarman commented Dec 3, 2025 •

edited

Loading

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

tnull left a comment •

edited

Loading