Introduce RenegotiatedFunding monitor update variant #3822

wpaulino · 2025-06-02T23:53:04Z

This is a new ChannelMonitorUpdateStep variant intended to be used whenever a new funding transaction for the channel has been negotiated via the InteractiveTxConstructor. This commit primarily focuses on its use for splices, but future work will expand where needed to support RBFs (both for the initial dual funding transaction, and splice transactions).

To draw a parallel to channel open, we generally want to have the commitment transactions negotiated for the funding transaction and committed to the respective ChannelMonitor before attempting to sign the funding transaction itself. This monitor update fulfills this need for a newly negotiated splice; it includes both the new holder and counterparty commitment transactions, and the new set of applicable ChannelTransactionParameters. Once the monitor update has been applied to the monitor and persisted, we allow the release of our tx_signatures for the splice transaction to wait for its confirmation.

Depends on #3774.

ldk-reviews-bot · 2025-06-02T23:53:07Z

👋 I see @TheBlueMatt was un-assigned.
If you'd like another reviewer assignemnt, please click here.

codecov · 2025-06-03T00:48:44Z

Codecov Report

Attention: Patch coverage is 23.61111% with 165 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (db165ad) to head (a94ac4c).

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	13.21%	151 Missing ⚠️
lightning/src/chain/chainmonitor.rs	6.66%	13 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3822      +/-   ##
==========================================
- Coverage   89.74%   89.62%   -0.13%     
==========================================
  Files         164      164              
  Lines      133079   133290     +211     
  Branches   133079   133290     +211     
==========================================
+ Hits       119437   119459      +22     
- Misses      10960    11141     +181     
- Partials     2682     2690       +8

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

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

ldk-reviews-bot · 2025-06-03T23:26:05Z

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

lightning/src/ln/channel.rs

tankyleo

First pass in channel monitor

lightning/src/chain/channelmonitor.rs

ldk-reviews-bot · 2025-06-11T00:35:24Z

🔔 1st Reminder

Hey @TheBlueMatt @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.

tankyleo

stared at the code again today, picked up nits on the way.

lightning/src/chain/channelmonitor.rs

lightning/src/chain/chainmonitor.rs

TheBlueMatt · 2025-06-11T14:58:58Z

lightning/src/ln/channel.rs

@@ -1677,7 +1677,33 @@ impl<SP: Deref> Channel<SP> where
 				res
 			},
 			ChannelPhase::Funded(mut funded_channel) => {
-				let res = funded_channel.commitment_signed(msg, logger).map(|monitor_update_opt| (None, monitor_update_opt));
+				#[cfg(splicing)]


Sheesh, is it too late to make this a new damn message? We now have three totally separate handling logic for commitment_signed, and this one doesn't even make any sense - we're receiving a single commitment_signed but we actually need two if we were in a normal state...

Sheesh, is it too late to make this a new damn message?

Probably not? Would be a trivial thing to change, but it would basically be a copy of the commitment_signed message.

this one doesn't even make any sense - we're receiving a single commitment_signed but we actually need two if we were in a normal state...

I'm not following, what do you mean by "normal state"? This is handling the case where we haven't finished negotiating a splice transaction. We expect a single commitment_signed to be exchanged for the alternative commitments at the current commitment number. After the negotiation finishes, if we're expecting a batch and they didn't send it, we'll fail the channel. If we're not expecting a batch and they did send it, we'll also fail the channel.

TheBlueMatt · 2025-06-11T15:00:28Z

lightning/src/ln/channel.rs

+			pending_splice_funding, &self.holder_commitment_point, msg, logger,
+		)?;
+
+		let counterparty_commitment_tx = self.context.build_commitment_transaction(


Bleh, we already build the commitment tx in validate_commitment_signed, lets not do it again.

We need to build both, validate_commitment_signed builds the holder one, this builds the counterparty one

lightning/src/ln/channel.rs

ldk-reviews-bot · 2025-06-14T00:00:05Z

🔔 2nd Reminder

Hey @TheBlueMatt @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-06-16T00:00:59Z

🔔 3rd Reminder

Hey @TheBlueMatt @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-06-16T00:01:18Z

🔔 1st Reminder

Hey @TheBlueMatt @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.

lightning/src/chain/chainmonitor.rs

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2025-06-17T14:32:57Z

lightning/src/chain/channelmonitor.rs

+		for (alternative_htlc, (htlc, _)) in
+			alternative_htlcs.iter().zip(htlcs_with_sources.iter_mut())
+		{
+			debug_assert!(htlc.transaction_output_index.is_some());


Same for these

The transaction_output_index.is_some() checks are more of an internal assertion, since there's a guarantee that CommitmentTransaction::nondust_htlcs returns HTLCs with their indices populated. The is_data_equal check should definitely return an error though.

TheBlueMatt · 2025-06-17T14:34:04Z

lightning/src/chain/channelmonitor.rs

+			debug_assert!(alternative_htlc.transaction_output_index.is_some());
+			debug_assert!(alternative_htlc.is_data_equal(htlc));
+			htlc.transaction_output_index = alternative_htlc.transaction_output_index;
+		}


Should we further check that all alternative htlcs that were not considered have no output index?

The alternative HTLCs (coming from the alternative counterparty commitment transaction) are non-dust only. The other HTLC data is coming from counterparty_claimable_outpoints which has already gone through validation.

TheBlueMatt · 2025-06-17T14:37:54Z

Needs rebase, it seems.

TheBlueMatt

Basically LGTM

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2025-06-17T23:18:22Z

lightning/src/ln/channel.rs

+			msg,
+			logger,
+		)?;
+		let counterparty_commitment_tx = self


We just built the commitment tx in validate_commitment_signed, let's not re-build it here.

They're not the same transaction, one is the holder and the other the counterparty

Hmm, okay this could use some comments, I think. So when we go to splice we (a) negotiate the new funding, then (b) exchange commitment_signed messages, then (c) exchange our new-funding-tx signatures.

AFAIU, the intent is that we'll update both the local and remote commitment txn after we receive the peer's commitment_signed at the end of (b), with a monitor update blocker preventing us from going to (c). This saves the extra monitor update when sending the commitment_signed at the start of (b). But ultimately we should have a comment, IMO, describing that the commitment_signed we sent at the top of (b) and the commitment we generate here are guaranteed to be identical (because we pause the state machine until we finish (c), I imagine?)

lightning/src/ln/channel.rs

This is a new `ChannelMonitorUpdateStep` variant intended to be used whenever a new funding transaction for the channel has been negotiated via the `InteractiveTxConstructor`. This commit primarily focuses on its use for splices, but future work will expand where needed to support RBFs (both for the initial dual funding transaction, and splice transactions). To draw a parallel to channel open, we generally want to have the commitment transactions negotiated for the funding transaction and committed to the respective `ChannelMonitor` before attempting to sign the funding transaction itself. This monitor update fulfills this need for a newly negotiated splice; it includes both the new holder and counterparty commitment transactions, and the new set of applicable `ChannelTransactionParameters`. Once the monitor update has been applied to the monitor and persisted, we allow the release of our `tx_signatures` for the splice transaction to wait for its confirmation.

TheBlueMatt · 2025-06-18T14:39:21Z

LGTM, I think, modulo the comment request above @jkczyz or @tankyleo can take a look now.

tankyleo

Code all LGTM pending Matt's last unresolved comment above.

@jkczyz would you be able to take a look at channel.rs on this PR ? Code makes sense to me, but not as confident with the p2p splicing dance :)

wpaulino requested a review from jkczyz June 2, 2025 23:53

jkczyz reviewed Jun 3, 2025

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

jkczyz reviewed Jun 5, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the renegotiated-funding-monitor-update branch from 6c78591 to ac8c8c5 Compare June 9, 2025 00:35

wpaulino requested a review from jkczyz June 9, 2025 00:35

tankyleo self-requested a review June 9, 2025 19:55

tankyleo reviewed Jun 9, 2025

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

wpaulino force-pushed the renegotiated-funding-monitor-update branch from ac8c8c5 to 5f0709f Compare June 10, 2025 18:58

wpaulino requested a review from TheBlueMatt June 10, 2025 18:58

tankyleo reviewed Jun 11, 2025

View reviewed changes

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Jun 11, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the renegotiated-funding-monitor-update branch from 5f0709f to 689d0c1 Compare June 11, 2025 17:04

wpaulino requested a review from TheBlueMatt June 12, 2025 17:23

wpaulino mentioned this pull request Jun 12, 2025

Introduce splice-compatible commitment update monitor variants #3855

Open

wpaulino force-pushed the renegotiated-funding-monitor-update branch from 689d0c1 to 4a38fd2 Compare June 13, 2025 17:20

wpaulino force-pushed the renegotiated-funding-monitor-update branch from 4a38fd2 to b274f78 Compare June 16, 2025 17:13

TheBlueMatt reviewed Jun 17, 2025

View reviewed changes

wpaulino force-pushed the renegotiated-funding-monitor-update branch from b274f78 to a201837 Compare June 17, 2025 17:38

wpaulino requested review from TheBlueMatt and tankyleo and removed request for jkczyz June 17, 2025 17:39

TheBlueMatt reviewed Jun 17, 2025

View reviewed changes

wpaulino force-pushed the renegotiated-funding-monitor-update branch from a201837 to a94ac4c Compare June 18, 2025 01:55

wpaulino requested a review from TheBlueMatt June 18, 2025 01:56

TheBlueMatt removed their request for review June 18, 2025 14:39

tankyleo reviewed Jun 18, 2025

View reviewed changes

Introduce RenegotiatedFunding monitor update variant #3822

Are you sure you want to change the base?

Introduce RenegotiatedFunding monitor update variant #3822

Uh oh!

Conversation

wpaulino commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 3, 2025

Uh oh!

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 11, 2025

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

ldk-reviews-bot commented Jun 14, 2025

Uh oh!

ldk-reviews-bot commented Jun 16, 2025

Uh oh!

ldk-reviews-bot commented Jun 16, 2025

Uh oh!

Uh oh!

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!

TheBlueMatt commented Jun 17, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

TheBlueMatt commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

wpaulino commented Jun 2, 2025 •

edited

Loading

ldk-reviews-bot commented Jun 2, 2025 •

edited

Loading

codecov bot commented Jun 3, 2025 •

edited

Loading

TheBlueMatt commented Jun 18, 2025 •

edited

Loading