Skip to content

Conversation

@benthecarman
Copy link
Contributor

@benthecarman benthecarman commented Nov 19, 2025

Original context and motivation comes from here: lightningdevkit/ldk-node#677 (comment)

When splicing-in, the default case is our channel utxo + our wallet utxos being combined. This works great however, it can give our wallet issues calculating fees after the fact because our wallet needs to know about our channel's utxo. We currently have it's outpoint and satoshi value available, but not its output script so we are unable to construct the TxOut for the channel. This adds the redeem script to the ChannelDetails and ChannelPending event which gives us enough information to be able to construct it.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Nov 19, 2025

👋 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.

@benthecarman benthecarman requested a review from jkczyz November 19, 2025 15:44
@benthecarman benthecarman force-pushed the redeem-script-channel-pending-detials branch 4 times, most recently from b2f8584 to 1c29113 Compare November 19, 2025 16:24
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@benthecarman benthecarman force-pushed the redeem-script-channel-pending-detials branch from 1c29113 to 436dc4b Compare November 19, 2025 16:42
@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

❌ Patch coverage is 69.04762% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.32%. Comparing base (6d9c676) to head (e3ebe7a).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channel_state.rs 46.66% 8 Missing ⚠️
lightning/src/routing/router.rs 50.00% 4 Missing ⚠️
lightning/src/ln/channelmanager.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4234      +/-   ##
==========================================
- Coverage   89.32%   89.32%   -0.01%     
==========================================
  Files         180      180              
  Lines      138641   138680      +39     
  Branches   138641   138680      +39     
==========================================
+ Hits       123844   123877      +33     
- Misses      12174    12179       +5     
- Partials     2623     2624       +1     
Flag Coverage Δ
fuzzing 35.97% <46.66%> (+<0.01%) ⬆️
tests 88.69% <69.04%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz
jkczyz previously approved these changes Nov 19, 2025
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in lightningdevkit/ldk-node#677 (comment), we should include this in SplicePending, too, since subsequent splices will spend the new funding output. Feel free to do it in a follow-up if you prefer.

@benthecarman
Copy link
Contributor Author

Added to the SplicePending event

Comment on lines 6492 to 6494
new_funding_redeem_script: chan
.funding
.get_funding_redeemscript(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will still be the old funding until the splice is promoted. You'll need to communicate it through SpliceFundingNegotiated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@benthecarman benthecarman force-pushed the redeem-script-channel-pending-detials branch 2 times, most recently from e0c0b62 to 5b5c8d6 Compare November 19, 2025 20:06
jkczyz
jkczyz previously approved these changes Nov 19, 2025
channel_type: Option<ChannelTypeFeatures>,
/// The witness script that can be used to spend the funding output for this channel.
///
/// This field will be `None` for objects serialized with LDK versions prior to 0.2.0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like this won't make 0.2 but feel free to petition @TheBlueMatt :)

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be honest I'm pretty annoyed we're talking about changing the 0.2 API almost two months after we intended to ship it, but if we land this today maybe we still include it in 0.2.

Comment on lines 453 to 482
/// The witness script that can be used to spend the funding output for this channel.
///
/// This field will be `None` for objects serialized with LDK versions prior to 0.2.0.
pub funding_redeem_script: Option<bitcoin::ScriptBuf>,
}

impl ChannelDetails {
/// Gets the current SCID which should be used to identify this channel for inbound payments.
/// This should be used for providing invoice hints or in any other context where our
/// counterparty will forward a payment to us.
///
/// This is either the [`ChannelDetails::inbound_scid_alias`], if set, or the
/// [`ChannelDetails::short_channel_id`]. See those for more information.
pub fn get_inbound_payment_scid(&self) -> Option<u64> {
self.inbound_scid_alias.or(self.short_channel_id)
}

/// Gets the current SCID which should be used to identify this channel for outbound payments.
/// This should be used in [`Route`]s to describe the first hop or in other contexts where
/// we're sending or forwarding a payment outbound over this channel.
///
/// This is either the [`ChannelDetails::short_channel_id`], if set, or the
/// [`ChannelDetails::outbound_scid_alias`]. See those for more information.
///
/// [`Route`]: crate::routing::router::Route
pub fn get_outbound_payment_scid(&self) -> Option<u64> {
self.short_channel_id.or(self.outbound_scid_alias)
}

/// Gets the funding output for this channel, if available.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need docs on when it changes wrt a splice. When a splice happens at what point in the splice do these switch over to the new values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, looked through the code and seems it happens after the commitment signed messages were exchanged.

Comment on lines 484 to 485
/// During a splice, the funding output will change and this value will be updated
/// after the `commitment_signed` messages have been exchanged.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... this should only change when the splice has been promoted (i.e., when the FundingScope from the pending splice is swapped in for the current FundingScope). That happens after splice_locked has been exchanged.

core::mem::swap(&mut self.funding, funding);

Maybe you were looking at channel establishment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, i think what I was looking at when that FundedChannel was being created/updated here:

self.phase = ChannelPhase::Funded(funded_channel);

Original context and motivation comes from here: lightningdevkit/ldk-node#677 (comment)

When splicing-in, the default case is our channel utxo + our wallet utxos
being combined. This works great however, it can give our wallet issues
calculating fees after the fact because our wallet needs to know about
our channel's utxo. We currently have it's outpoint and satoshi value
available, but not its output script so we are unable to construct the
TxOut for the channel. This adds the redeem script to the
`ChannelDetails` and `ChannelPending` event which gives us enough
information to be able to construct it.
@benthecarman benthecarman force-pushed the redeem-script-channel-pending-detials branch from c5428d2 to e3ebe7a Compare November 20, 2025 16:35
@jkczyz jkczyz merged commit f325458 into lightningdevkit:main Nov 21, 2025
26 checks passed
@benthecarman benthecarman deleted the redeem-script-channel-pending-detials branch November 21, 2025 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants