Introduce splice-compatible commitment update monitor variants by wpaulino · Pull Request #3855 · lightningdevkit/rust-lightning

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

No description provided.

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

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

Given its just for the latest and prev commitment txn I feel like its okay. It also gives us room to relax the restriction on multiple splices having the same dust/non-dust HTLC sets in the future.

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

Codecov Report

Attention: Patch coverage is 50.93750% with 157 lines in your changes missing coverage. Please review.

Project coverage is 88.80%. Comparing base (c48e0a8) to head (4dc47f9).
Report is 43 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	50.71%	96 Missing and 8 partials ⚠️
lightning/src/ln/channel.rs	50.00%	53 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3855      +/-   ##
==========================================
- Coverage   89.65%   88.80%   -0.86%     
==========================================
  Files         164      165       +1     
  Lines      134658   119288   -15370     
  Branches   134658   119288   -15370     
==========================================
- Hits       120734   105937   -14797     
+ Misses      11246    11020     -226     
+ Partials     2678     2331     -347

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

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

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.

Hm yeah this would be nice, this code was mostly written before we made the change to populate the source table separately. I wouldn't consider it a blocker to this PR, unless @TheBlueMatt feels strongly.

tankyleo

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

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

lightning/src/chain/channelmonitor.rs

 			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 ?

@wpaulino just want to confirm here - i think the worry is hey these could be used to cheat so make sure we are tracking them ?

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

ldk-reviews-bot · 2025-06-16T00:01:21Z

🔔 1st Reminder

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

🔔 2nd Reminder

Hey @TheBlueMatt! 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/channelmonitor.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2025-06-18T15:44:58Z

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();


Given its just for the latest and prev commitment txn I feel like its okay. It also gives us room to relax the restriction on multiple splices having the same dust/non-dust HTLC sets in the future.

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

tankyleo

Another pass on the first commit, will continue first thing tomorrow...

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

tankyleo

Done with the two other commits

lightning/src/chain/channelmonitor.rs

ldk-reviews-bot · 2025-06-25T18:33:15Z

🔔 1st Reminder

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

TheBlueMatt · 2025-06-27T19:15:49Z

Mind addressing @tankyleo's feedback?

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.

They don't need to be tracked as we can recompute them easily, though we still write them in the `ChannelMonitor` for the current `FundingScope` for backwards compatibility. The serialization implementation for `FundingScope` can now use the `impl_writeable_tlv_based` macro as a result.

graphite-app · 2025-06-27T21:56:31Z

lightning/src/chain/channelmonitor.rs

+	fn update_counterparty_commitment_data(
+		&mut self, commitment_txs: &[CommitmentTransaction], htlc_data: &CommitmentHTLCData,
+	) -> Result<(), &'static str> {
+		self.verify_matching_commitment_transactions(commitment_txs.iter())?;


The function verify_matching_commitment_transactions returns a Result, but the error case isn't being handled here. Consider propagating the error back to the caller by changing this to:

self.verify_matching_commitment_transactions(commitment_txs.iter())?;

This matches the pattern used in update_holder_commitment_data and ensures errors aren't silently ignored.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

ldk-reviews-bot · 2025-06-30T00:00:48Z

🔔 1st Reminder

Hey @TheBlueMatt @tankyleo! 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-30T00:00:49Z

🔔 1st Reminder

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

TheBlueMatt · 2025-06-29T23:47:17Z

lightning/src/chain/channelmonitor.rs

+
+		let current_funding_commitment_tx = commitment_txs.first().unwrap();
+		self.current_holder_commitment_number = current_funding_commitment_tx.commitment_number();
+		self.onchain_tx_handler.provide_latest_holder_tx(current_funding_commitment_tx.clone());


Mind leaving a comment here on why OnchainTxHandler doesn't need to know about other commitments?

TheBlueMatt · 2025-06-30T00:17:54Z

lightning/src/chain/channelmonitor.rs

+				let source = (!htlc.offered).then(|| {
+					let source = sources
+						.next()
+						.expect("Every inbound non-dust HTLC should have a corresponding source")


We generally call these "outbound" HTLCs, since they're outbound, just !offered on the commitment tx.

Ah yes my bad should be every outbound :)

lightning/src/ln/channel.rs

tankyleo

Nothing blocking, just have a pending question further above about why we delete

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

TheBlueMatt · 2025-07-01T23:41:52Z

Landing this, we can figure out the logging and comments in a followup.

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

TheBlueMatt reviewed Jun 18, 2025

View reviewed changes

tankyleo mentioned this pull request Jun 20, 2025

[Custom Transactions] Add TxBuilder trait, support fixed additional outputs #3775

Merged

jkczyz mentioned this pull request Jun 21, 2025

Dual-funded channels and Splicing Project Tracking #1621

Closed

7 tasks

wpaulino force-pushed the splice-commitment-monitor-update branch from 7f942f4 to fedd0ff Compare June 23, 2025 18:31

wpaulino requested review from TheBlueMatt and tankyleo June 23, 2025 18:32

wpaulino force-pushed the splice-commitment-monitor-update branch from fedd0ff to 0e88cdf Compare June 24, 2025 21:50

wpaulino mentioned this pull request Jun 24, 2025

Introduce RenegotiatedFunding monitor update variant #3822

Merged

graphite-app bot reviewed Jun 24, 2025

View reviewed changes

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

tankyleo reviewed Jun 25, 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

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

jkczyz added this to Weekly Goals Jun 26, 2025

TheBlueMatt removed their request for review June 27, 2025 19:16

wpaulino added 3 commits June 27, 2025 14:52

wpaulino force-pushed the splice-commitment-monitor-update branch from 0e88cdf to 4dc47f9 Compare June 27, 2025 21:54

wpaulino requested review from TheBlueMatt and tankyleo June 27, 2025 21:55

graphite-app bot reviewed Jun 27, 2025

View reviewed changes

TheBlueMatt approved these changes Jun 30, 2025

View reviewed changes

tankyleo approved these changes Jul 1, 2025

View reviewed changes

TheBlueMatt merged commit 34664bc into lightningdevkit:main Jul 1, 2025
26 of 28 checks passed

github-project-automation bot moved this to Done in Weekly Goals Jul 1, 2025

wpaulino deleted the splice-commitment-monitor-update branch July 7, 2025 16:54

wpaulino mentioned this pull request Jul 8, 2025

Produce a single monitor update per commitment number #3738

Closed

TheBlueMatt assigned wpaulino Jul 31, 2025

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 16, 2025

Uh oh!

ldk-reviews-bot commented Jun 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

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 25, 2025

Uh oh!

TheBlueMatt commented Jun 27, 2025

Uh oh!

wpaulino commented Jun 12, 2025 •

edited

Loading

ldk-reviews-bot commented Jun 12, 2025 •

edited

Loading

codecov bot commented Jun 13, 2025 •

edited

Loading