Introduce splice-compatible commitment update monitor variants #3855

wpaulino · 2025-06-12T21:42:42Z

Depends on #3822.

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.

This new variant is a backwards-incompatible successor to `LatestHolderCommitmentTXInfo` that is capable of handling holder commitment updates while a splice is pending. Since all holder commitment candidates share the same set of non-dust and dust HTLCs (though each non-dust HTLC can have differing output indices), we can share the same set of HTLC source data across all candidates.

This new variant is a backwards-incompatible successor to `LatestCounterpartyCommitmentTXInfo` that is capable of handling counterparty commitment updates while a splice is pending. Since all counterparty commitment candidates share the same set of non-dust and dust HTLCs (though each non-dust HTLC can have differing output indices), we can share the same set of HTLC source data across all candidates. Unfortunately, each `FundingScope` tracks its own set of `counterparty_claimable_outpoints`, which includes the HTLC source data (though only for the current and previous counterparty commitments), so we end up duplicating it (both in memory and on disk) temporarily.

ldk-reviews-bot · 2025-06-12T21:42:44Z

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

wpaulino · 2025-06-12T21:48:23Z

lightning/src/chain/channelmonitor.rs

+		let htlcs_for_commitment = |commitment: &CommitmentTransaction| {
+			let mut nondust_htlcs = commitment.nondust_htlcs().clone().into_iter();
+			let mut sources = htlc_data.nondust_htlc_sources.clone().into_iter();


I'm not too happy with this, but trying to not duplicatively track the HTLC sources across FundingScopes is a good bit of non-trivial work. At least they'll only exist in the current and previous counterparty commitments, but those are still per FundingScope. The simplest thing we could do for now is Arc them to reduce memory bandwidth (unfortunately on disk we'll still have duplicates), but I chose not to pursue that unless we're sure of it.

codecov · 2025-06-13T01:02:13Z

Codecov Report

Attention: Patch coverage is 50.91650% with 241 lines in your changes missing coverage. Please review.

Project coverage is 89.71%. Comparing base (0848e7a) to head (7f942f4).
Report is 57 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	25.26%	209 Missing and 4 partials ⚠️
lightning/src/ln/channel.rs	86.81%	18 Missing and 6 partials ⚠️
lightning/src/chain/chainmonitor.rs	83.33%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3855      +/-   ##
==========================================
- Coverage   89.88%   89.71%   -0.18%     
==========================================
  Files         160      163       +3     
  Lines      129654   131592    +1938     
  Branches   129654   131592    +1938     
==========================================
+ Hits       116534   118052    +1518     
- Misses      10425    10842     +417     
- Partials     2695     2698       +3

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

tankyleo

Thanks dude, just went through the first commit, will continue tomorrow. I have a big-picture question in commitment_signed_batch take a look at that first.

tankyleo · 2025-06-13T03:54:59Z

lightning/src/chain/channelmonitor.rs

-	) {
-		let dust_htlcs: Vec<_> = if htlc_outputs.iter().any(|(_, s, _)| s.is_some()) {
+	) -> Result<(), &'static str> {
+		let htlc_data = if htlc_outputs.iter().any(|(_, s, _)| s.is_some()) {


nit: Can we keep this to dust_htlcs and build CommitmentHTLCData just once right after this statement ?

tankyleo · 2025-06-13T03:58:09Z

lightning/src/chain/channelmonitor.rs

+			return Err("Commitment transaction(s) mismatch");
+		}
+
+		let mut current_funding_commitment = commitment_txs.remove(0);


Tried a few quick things to remove this O(n) remove call here, didn't find anything. Are we cool with this ?

tankyleo · 2025-06-13T03:58:53Z

lightning/src/chain/channelmonitor.rs

+			return Err("Commitment transaction(s) mismatch");
+		}
+
+		let mut current_funding_commitment = commitment_txs.remove(0);


nit: Also seems to me the code would read smoother if we called this holder_commitment_tx

tankyleo · 2025-06-13T04:01:05Z

lightning/src/chain/channelmonitor.rs

-		self.prev_holder_htlc_data = Some(holder_htlc_data);
+	fn update_holder_commitment_data(
+		&mut self, mut commitment_txs: Vec<HolderCommitmentTransaction>,
+		mut htlc_data: CommitmentHTLCData, claimed_htlcs: Vec<(SentHTLCId, PaymentPreimage)>,


nit: similar style as holder_commitment_tx, use holder_htlc_data ?

tankyleo · 2025-06-13T04:02:20Z

lightning/src/chain/channelmonitor.rs

+				trusted_tx.built_transaction().transaction.tx_in(0).map(|input| input.previous_output).ok();
+			let expected_funding_outpoint_spent =
+				pending_funding.channel_parameters.funding_outpoint.map(|op| op.into_bitcoin_outpoint());
+			if funding_outpoint_spent != expected_funding_outpoint_spent {


Should we also check this for the current holder commitment tx ?

tankyleo · 2025-06-13T04:05:25Z

lightning/src/chain/channelmonitor.rs

+						commitment_tx.clone(), htlc_outputs.clone(), &claimed_htlcs,
+						nondust_htlc_sources.clone()
+					) {
+						log_error!(logger, "Failed updating latest holder commitment transaction info: {}", e);


Should we set ret = Err(()); here and below ?

tankyleo · 2025-06-13T04:10:09Z

lightning/src/ln/channel.rs

+			if htlc.transaction_output_index.is_some() {
+				if htlc.offered {
+					if let Some(source) = source_opt.take() {
+						nondust_htlc_sources.push(source.clone());
+					} else {
+						panic!("Missing outbound HTLC source");
+					}
+				}
+			} else {
+				dust_htlcs.push((htlc, source_opt.take().cloned()));
+			}


While we touch this, I think we can remove two indents here like this:

if htlc.transaction_output_index.is_some() && htlc.offered { nondust_htlc_sources.push(source_opt.take().expect("Missing outbound HTLC source").clone()); } else if htlc.transaction_output_index.is_none() { dust_htlcs.push((htlc, source_opt.take().cloned())); }

tankyleo · 2025-06-13T04:14:17Z

lightning/src/ln/channel.rs

+		let mut commitment_txs = Vec::with_capacity(self.pending_funding.len() + 1);
+		let mut htlc_data = None;
+		for funding in core::iter::once(&self.funding).chain(self.pending_funding.iter()) {
+			let funding_txid = funding.get_funding_txo().unwrap().txid;


nit: Let's add a txid function here like we have in monitor ? I know it was here before, just a thought I've been having here.

tankyleo · 2025-06-13T04:16:11Z

lightning/src/ln/channel.rs

 		}

 		self.context.latest_monitor_update_id += 1;
 		let mut monitor_update = ChannelMonitorUpdate {
 			update_id: self.context.latest_monitor_update_id,
-			updates,
+			updates: vec![update],


So far sounds like we don't need this to be a vector.

tankyleo · 2025-06-13T04:31:44Z

lightning/src/ln/channel.rs

+			let msg = messages
+				.get(&funding_txid)
+				.ok_or_else(|| ChannelError::close(format!("Peer did not send a commitment_signed for pending splice transaction: {}", funding_txid)))?;
+			let (commitment_tx, htlcs_included) = self.context.validate_commitment_signed(


It might not be worth the pain let me know:

Can we avoid doing the work of populating and sorting this table for all the pending scopes in build_commitment_transaction ? We only need it for the current scope here.

(In validate_commitment_signed, let's read the HTLCs from the commitment transaction instead of the HTLC-source table).

Also wondering if we can make the state and dust-vs-nondust HTLC sorting only once for all scopes.

tankyleo

Went through the second commit, some similar questions, some new ones.

tankyleo · 2025-06-13T23:52:17Z

lightning/src/chain/channelmonitor.rs

@@ -3175,11 +3171,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			self.counterparty_hash_commitment_number.insert(htlc.payment_hash, commitment_number);
 		}

-		log_trace!(logger, "Tracking new counterparty commitment transaction with txid {} at commitment number {} with {} HTLC outputs", txid, commitment_number, htlc_outputs.len());


You find this log not necessary ?

tankyleo · 2025-06-13T23:54:13Z

lightning/src/chain/channelmonitor.rs

+			let expected_funding_outpoint_spent =
+				pending_funding.channel_parameters.funding_outpoint.map(|op| op.into_bitcoin_outpoint());
+			if funding_outpoint_spent != expected_funding_outpoint_spent {
+				return Err("Funding outpoint mismatch");


Similar question here about also double checking the funding outpoint of the commitment transaction for confirmed / current the funding scope ?

tankyleo · 2025-06-13T23:55:13Z

lightning/src/chain/channelmonitor.rs

+				};
+
+				let mut source = None;
+				if htlc.offered {


Like you explained in channel, should we be checking for accepted HTLCs here ?

tankyleo · 2025-06-13T23:58:23Z

lightning/src/chain/channelmonitor.rs

+			let nondust_htlcs = core::iter::from_fn(move || {
+				let htlc = if let Some(htlc) = nondust_htlcs.next() {
+					htlc
+				} else {
+					debug_assert!(sources.next().is_none());
+					return None;
+				};
+
+				let mut source = None;
+				if htlc.offered {
+					source = Some(Box::new(sources.next().expect(
+						"Every offered non-dust HTLC should have a corresponding source"
+					)));
+				}
+
+				Some((htlc, source))
+			});


Some cleanups on this section:

debug_assert!(sources.len() <= nondust_htlcs.len()); let nondust_htlcs = core::iter::from_fn(move || { let htlc = nondust_htlcs.next()?; let source = (!htlc.offered).then(|| { Box::new(sources.next().expect( "Every offered non-dust HTLC should have a corresponding source" )) }); Some((htlc, source)) });

tankyleo · 2025-06-14T00:04:44Z

lightning/src/chain/channelmonitor.rs

+		};
+
+		let current_funding_commitment = commitment_txs.first().unwrap();
+		for (pending_funding, commitment_tx) in self.pending_funding.iter_mut().zip(commitment_txs.iter()) {


Are we off by one here? In the holder version we remove the first commitment transaction from the vector, then zip the vector together with the funding scopes.

tankyleo · 2025-06-14T00:08:42Z

lightning/src/chain/channelmonitor.rs

+				return Err("Funding outpoint mismatch");
+			}
+
+			pending_funding.prev_counterparty_commitment_txid =


Similar question here about finishing all validation before mutating state.

tankyleo · 2025-06-14T00:10:31Z

lightning/src/chain/channelmonitor.rs

@@ -3175,11 +3171,11 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			self.counterparty_hash_commitment_number.insert(htlc.payment_hash, commitment_number);
 		}

-		log_trace!(logger, "Tracking new counterparty commitment transaction with txid {} at commitment number {} with {} HTLC outputs", txid, commitment_number, htlc_outputs.len());
 		self.funding.prev_counterparty_commitment_txid = self.funding.current_counterparty_commitment_txid.take();
 		self.funding.current_counterparty_commitment_txid = Some(txid);
 		self.funding.counterparty_claimable_outpoints.insert(txid, htlc_outputs.clone());


Came across this while reading the code: does not seem like we need the clone here ?

tankyleo · 2025-06-14T00:16:32Z

lightning/src/chain/channelmonitor.rs

+				trusted_tx.built_transaction().transaction.tx_in(0).map(|input| input.previous_output).ok();
+			let expected_funding_outpoint_spent =
+				pending_funding.channel_parameters.funding_outpoint.map(|op| op.into_bitcoin_outpoint());
+			if funding_outpoint_spent != expected_funding_outpoint_spent {


This check here passes if both sides are None; should never happen I agree, my paranoia speaking.

tankyleo · 2025-06-14T00:21:58Z

lightning/src/ln/channel.rs

@@ -9379,32 +9379,54 @@ impl<SP: Deref> FundedChannel<SP> where
 		}
 		self.context.resend_order = RAACommitmentOrder::RevokeAndACKFirst;

-		let mut updates = Vec::with_capacity(self.pending_funding.len() + 1);
-		for funding in core::iter::once(&self.funding).chain(self.pending_funding.iter()) {
+		let update = if self.pending_funding.is_empty() {


In the future we'll still use the new variant even if we have no pending scopes right ?

tankyleo · 2025-06-14T00:25:14Z

lightning/src/ln/channel.rs

+			let commitment_txs = core::iter::once(&self.funding)
+				.chain(self.pending_funding.iter())
+				.map(|funding| {
+					let (htlcs_ref, counterparty_commitment_tx) =


Same question about only needing htlcs_ref a single time / some work getting duplicated across funding scopes.

wpaulino added 3 commits June 11, 2025 10:04

wpaulino requested a review from tankyleo June 12, 2025 21:43

wpaulino commented Jun 12, 2025

View reviewed changes

wpaulino requested a review from TheBlueMatt June 12, 2025 21:48

tankyleo reviewed Jun 13, 2025

View reviewed changes

tankyleo reviewed Jun 14, 2025

View reviewed changes

Introduce splice-compatible commitment update monitor variants #3855

Are you sure you want to change the base?

Introduce splice-compatible commitment update monitor variants #3855

Conversation

wpaulino commented Jun 12, 2025

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 13, 2025

Codecov Report

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

tankyleo Jun 13, 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!

tankyleo 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!

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!

ldk-reviews-bot commented Jun 12, 2025 •

edited

Loading

tankyleo Jun 13, 2025 •

edited

Loading