Exchange `splice_locked` messages #3741

jkczyz · 2025-04-16T17:11:41Z

After a splice has been negotiated, each party must send a splice_locked message to the other party once the splice transaction has had an acceptable number of confirmations. Update the logic for processing newly confirmed transactions and updated best block to send splice_locked when appropriate.

TODO: Likewise, handle splice_locked and promote the channel's FundingScope once both splice_locked messages have been exchanged.

ldk-reviews-bot · 2025-04-16T17:11:44Z

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

jkczyz · 2025-04-16T17:13:51Z

The logic around determining if the splice_locked messages have been exchanged is still a work in progress. It may need to be re-considered to work with chained 0-conf splices. See lightning/bolts#1160 (comment).

When processing confirmed transactions and updates to the best block, ChannelManager may be instructed to send a channel_ready message when a channel's funding transaction has confirmed and has met the required number of confirmations. A similar action is needed for sending splice_locked once splice transaction has confirmed with required number of confirmations. Generalize do_chain_event signature to allow for either scenario.

When processing confirmed transactions, if the funding transaction is found then information about it in the ChannelContext is updated. In preparation for splicing, move this data to FundingScope.

jkczyz · 2025-04-18T23:17:53Z

@wpaulino Ok, this is in better shape for a high-level look. I don't believe it correctly handles unconfirmed splice transactions yet. Also, doesn't yet re-send splice_locked on channel reestablishment, thought that may come in a follow-up.

codecov · 2025-04-18T23:24:11Z

Codecov Report

Attention: Patch coverage is 88.63636% with 40 lines in your changes missing coverage. Please review.

Project coverage is 90.15%. Comparing base (7b45811) to head (e4c0566).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	86.47%	30 Missing and 8 partials ⚠️
lightning/src/ln/channelmanager.rs	96.72%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3741      +/-   ##
==========================================
+ Coverage   89.10%   90.15%   +1.04%     
==========================================
  Files         156      156              
  Lines      123431   132183    +8752     
  Branches   123431   132183    +8752     
==========================================
+ Hits       109985   119164    +9179     
+ Misses      10760    10435     -325     
+ Partials     2686     2584     -102

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

ldk-reviews-bot · 2025-04-19T00:00:14Z

🔔 1st Reminder

Hey @wpaulino! 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.

optout21

LGTM. The preparational changes are very clear. The WIP part also makes sense so far.

optout21 · 2025-04-19T09:37:16Z

lightning/src/ln/channelmanager.rs

@@ -11725,6 +11727,13 @@ where
 											log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", funded_channel.context.channel_id());
 										}
 									},
+									#[cfg(splicing)]
+									Some(FundingConfirmedMessage::Splice(splice_locked)) => {
+										pending_msg_events.push(MessageSendEvent::SendSpliceLocked {


TODO note: An event will have to be sent here as well, similar to ChannelReady -- ChannelReady with a flag indicating it is for splicing, or a new, splicing-specific event.

I take it we want this only after both splice_locked messages have been exchanged? For naming, debating either SpliceLocked or SpliceReady. Something like SpliceFunded also a possibility though it seems odd if it is a splice-out.

Related: Do we want something similar to ChannelPending for when tx_signatures have been exchanged?

Do we want something similar to ChannelPending for when tx_signatures have been exchanged?

I think so, that way we can let the user know their splice was negotiated successfully and is pending confirmation.

optout21 · 2025-04-19T09:44:42Z

lightning/src/ln/channel.rs

@@ -1864,6 +1864,8 @@ impl FundingScope {
 #[cfg(splicing)]
 struct PendingSplice {
 	pub our_funding_contribution: i64,
+	sent_funding_txid: Option<Txid>,


Where is this field set? Is it when signatures have been exchanged for the the negotiated funding transaction / it has been broadcast?

I believe these are specific to splice_locked, so when that is sent/received.

lightning/src/ln/channel.rs

ldk-reviews-bot · 2025-04-21T00:00:36Z

🔔 2nd Reminder

Hey @wpaulino! 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.

wpaulino · 2025-04-21T18:45:04Z

lightning/src/ln/channel.rs

-							self.context.minimum_depth = Some(COINBASE_MATURITY);
+							self.funding.minimum_depth.unwrap_or(0) > 0 &&
+							self.funding.minimum_depth.unwrap_or(0) < COINBASE_MATURITY {
+							self.funding.minimum_depth = Some(COINBASE_MATURITY);


Can we somehow enforce the coinbase maturity without updating minimum_depth? We always keep the funding transaction around now so we should be able to check if it's a coinbase transaction or not. That would prevent us from tracking minimum_depth per scope.

Looks like for v1 channel establishment, the acceptor doesn't have the funding transaction until they see the funding txo on chain. So we need to set it then. We could only store it if it's the coinbase transaction, but I've set it unconditionally for now.

wpaulino · 2025-04-21T18:53:35Z

lightning/src/ln/channel.rs

@@ -1864,6 +1864,8 @@ impl FundingScope {
 #[cfg(splicing)]
 struct PendingSplice {
 	pub our_funding_contribution: i64,
+	sent_funding_txid: Option<Txid>,


I believe these are specific to splice_locked, so when that is sent/received.

wpaulino · 2025-04-21T18:58:29Z

lightning/src/ln/channel.rs

+
+		let funding_tx_confirmations = height as i64 - funding.funding_tx_confirmation_height as i64 + 1;
+		if funding_tx_confirmations <= 0 {
+			funding.funding_tx_confirmation_height = 0;


Feels weird to have this done here, as opposed to doing so in funding_transaction_unconfirmed or when we see the block at funding_tx_confirmation_height get disconnected.

Yeah, though note this code was moved from check_get_channel_ready which is used in other places as well.

wpaulino · 2025-04-21T19:03:21Z

lightning/src/ln/channel.rs

+				None
+			},
+			_ => {
+				Some(msgs::SpliceLocked {


Shouldn't sent_funding_txid get set here?

Yup, somehow missed this when moving some commits around.

wpaulino · 2025-04-21T19:04:07Z

lightning/src/ln/channel.rs

+
+		match pending_splice.sent_funding_txid {
+			Some(sent_funding_txid) if confirmed_funding_txid == sent_funding_txid => {
+				debug_assert!(false);


We may need to replay splice_locked if a disconnect happened before they were able to process it. Unclear if we'd want to use this same code path for it or not.

By this do you mean for channel_reestablish?

Yeah, I assume there is a similar retransmission case we need to handle there, like with channel_ready.

wpaulino · 2025-04-21T19:11:47Z

lightning/src/ln/channel.rs

+		};
+
+		let mut confirmed_funding_tx = None;
+		for &(index_in_block, tx) in txdata.iter() {


I think we may want to iterate over the funding scopes for each transaction instead, since we may be processing full block data with thousands of transactions and having to compute the txid of each one.

Added some fixups for this as discussed offline.

This reverts commit 4b4ce81.

When checking if channel_ready should be sent, the funding transaction must reach minimum_depth confirmations. The same logic is needed for splicing a channel, so refactor it into a helper method.

The minimum_depth of a channel is overridden to COINBASE_MATURITY if the funding transaction is the coinbase transaction. However, if this is to be reused for a splice's minimum_depth, it would be a problem since sending splice_locked would be unnecessarily delayed. Now that FundingScope contains the funding transaction, use this to check if it is a coinbase transaction instead of overriding minimum_depth.

jkczyz

Addressed most comments other than adding an event and for re-sending splice_locked. See comment replies for open questions.

Also, added code to insert the new scid in short_to_chan_info. Should we remove the old one?

jkczyz · 2025-04-22T21:36:52Z

lightning/src/ln/channel.rs

+
+		let funding_tx_confirmations = height as i64 - funding.funding_tx_confirmation_height as i64 + 1;
+		if funding_tx_confirmations <= 0 {
+			funding.funding_tx_confirmation_height = 0;


Yeah, though note this code was moved from check_get_channel_ready which is used in other places as well.

jkczyz · 2025-04-22T21:37:43Z

lightning/src/ln/channel.rs

+				None
+			},
+			_ => {
+				Some(msgs::SpliceLocked {


Yup, somehow missed this when moving some commits around.

jkczyz · 2025-04-24T18:06:29Z

lightning/src/ln/channel.rs

-							self.context.minimum_depth = Some(COINBASE_MATURITY);
+							self.funding.minimum_depth.unwrap_or(0) > 0 &&
+							self.funding.minimum_depth.unwrap_or(0) < COINBASE_MATURITY {
+							self.funding.minimum_depth = Some(COINBASE_MATURITY);


Looks like for v1 channel establishment, the acceptor doesn't have the funding transaction until they see the funding txo on chain. So we need to set it then. We could only store it if it's the coinbase transaction, but I've set it unconditionally for now.

jkczyz · 2025-04-29T19:12:05Z

lightning/src/ln/channel.rs

+		};
+
+		let mut confirmed_funding_tx = None;
+		for &(index_in_block, tx) in txdata.iter() {


Added some fixups for this as discussed offline.

jkczyz · 2025-04-29T19:13:35Z

lightning/src/ln/channel.rs

+
+		match pending_splice.sent_funding_txid {
+			Some(sent_funding_txid) if confirmed_funding_txid == sent_funding_txid => {
+				debug_assert!(false);


By this do you mean for channel_reestablish?

jkczyz · 2025-04-29T19:53:59Z

lightning/src/ln/channelmanager.rs

@@ -11725,6 +11727,13 @@ where
 											log_trace!(logger, "Sending channel_ready WITHOUT channel_update for {}", funded_channel.context.channel_id());
 										}
 									},
+									#[cfg(splicing)]
+									Some(FundingConfirmedMessage::Splice(splice_locked)) => {
+										pending_msg_events.push(MessageSendEvent::SendSpliceLocked {


I take it we want this only after both splice_locked messages have been exchanged? For naming, debating either SpliceLocked or SpliceReady. Something like SpliceFunded also a possibility though it seems odd if it is a splice-out.

Related: Do we want something similar to ChannelPending for when tx_signatures have been exchanged?

jkczyz requested a review from wpaulino April 16, 2025 17:11

jkczyz added 5 commits April 18, 2025 17:56

Move ChannelContext::funding_tx_confirmed_in to FundingScope

2e85dde

When processing confirmed transactions, if the funding transaction is found then information about it in the ChannelContext is updated. In preparation for splicing, move this data to FundingScope.

Move ChannelContext::funding_tx_confirmation_height to FundingScope

8d605b0

When processing confirmed transactions, if the funding transaction is found then information about it in the ChannelContext is updated. In preparation for splicing, move this data to FundingScope.

Move ChannelContext::short_channel_id to FundingScope

d830342

When processing confirmed transactions, if the funding transaction is found then information about it in the ChannelContext is updated. In preparation for splicing, move this data to FundingScope.

Move ChannelContext::minimum_depth to FundingScope

4b4ce81

When processing confirmed transactions, if the funding transaction is found then information about it in the ChannelContext is updated. In preparation for splicing, move this data to FundingScope.

jkczyz force-pushed the 2025-04-splice-locked branch from dfbc04e to e4c0566 Compare April 18, 2025 23:15

optout21 reviewed Apr 19, 2025

View reviewed changes

wpaulino reviewed Apr 21, 2025

View reviewed changes

jkczyz added 13 commits April 23, 2025 09:32

Revert "Move ChannelContext::minimum_depth to FundingScope"

801b58d

This reverts commit 4b4ce81.

Refactor funding tx confirmation check into helper

a64922b

When checking if channel_ready should be sent, the funding transaction must reach minimum_depth confirmations. The same logic is needed for splicing a channel, so refactor it into a helper method.

Add FundingScope::get_funding_txid helper

7ccb545

WIP: Check all funding transactions

2f0ca90

f - use FundingScope::get_funding_txid

93537d4

f - set sent_funding_txid

b2dc948

f - cache compute_txid

9477d43

f - fix test_funding_and_commitment_tx_confirm_same_block

1386bec

f - update short_to_chan_info

7e22541

Handle splice_locked message

531ade7

f - update short_to_chan_info

c19c2f5

f - use FundingScope::get_funding_txid

49f8ef6

jkczyz force-pushed the 2025-04-splice-locked branch from e4c0566 to 49f8ef6 Compare April 29, 2025 19:55

jkczyz commented Apr 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exchange `splice_locked` messages #3741

Exchange `splice_locked` messages #3741

jkczyz commented Apr 16, 2025

ldk-reviews-bot commented Apr 16, 2025 •

edited

Loading

jkczyz commented Apr 16, 2025

jkczyz commented Apr 18, 2025

codecov bot commented Apr 18, 2025 •

edited

Loading

ldk-reviews-bot commented Apr 19, 2025

optout21 left a comment

optout21 Apr 19, 2025

jkczyz Apr 29, 2025

wpaulino Apr 29, 2025

optout21 Apr 19, 2025

wpaulino Apr 21, 2025

ldk-reviews-bot commented Apr 21, 2025

wpaulino Apr 21, 2025

jkczyz Apr 24, 2025

wpaulino Apr 21, 2025

wpaulino Apr 21, 2025

jkczyz Apr 22, 2025

wpaulino Apr 21, 2025

jkczyz Apr 22, 2025

wpaulino Apr 21, 2025

jkczyz Apr 29, 2025

wpaulino Apr 29, 2025

wpaulino Apr 21, 2025

jkczyz Apr 29, 2025

jkczyz left a comment

jkczyz Apr 22, 2025

jkczyz Apr 22, 2025

jkczyz Apr 24, 2025

jkczyz Apr 29, 2025

jkczyz Apr 29, 2025

jkczyz Apr 29, 2025

Exchange splice_locked messages #3741

Are you sure you want to change the base?

Exchange splice_locked messages #3741

Conversation

jkczyz commented Apr 16, 2025

ldk-reviews-bot commented Apr 16, 2025 • edited Loading

jkczyz commented Apr 16, 2025

jkczyz commented Apr 18, 2025

codecov bot commented Apr 18, 2025 • edited Loading

Codecov Report

ldk-reviews-bot commented Apr 19, 2025

optout21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Apr 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Exchange `splice_locked` messages #3741

Exchange `splice_locked` messages #3741

ldk-reviews-bot commented Apr 16, 2025 •

edited

Loading

codecov bot commented Apr 18, 2025 •

edited

Loading