Track funding tx channelmonitor #4109

martinsaposnic · 2025-09-23T13:29:54Z

As part of the client_trusts_lsp LSPS2 work, we decided to move the channelmonitor logic into a new PR so the other part could get merged

~~This comment is not yet done #3838 (comment), hence this was created as draft~~

ldk-reviews-bot · 2025-09-23T13:29:57Z

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

codecov · 2025-09-23T13:43:51Z

Codecov Report

❌ Patch coverage is 92.88538% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.80%. Comparing base (05f2848) to head (5b85deb).

Files with missing lines	Patch %	Lines
lightning/src/ln/functional_tests.rs	90.66%	11 Missing and 3 partials ⚠️
lightning/src/chain/channelmonitor.rs	94.64%	2 Missing and 1 partial ⚠️
lightning/src/ln/functional_test_utils.rs	97.82%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4109      +/-   ##
==========================================
+ Coverage   88.78%   88.80%   +0.01%     
==========================================
  Files         180      180              
  Lines      137004   137237     +233     
  Branches   137004   137237     +233     
==========================================
+ Hits       121642   121873     +231     
- Misses      12538    12547       +9     
+ Partials     2824     2817       -7

Flag	Coverage Δ
fuzzing	`21.55% <19.80%> (+0.59%)`	⬆️
tests	`88.64% <92.88%> (+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.

martinsaposnic · 2025-09-23T17:27:21Z

lightning/src/chain/channelmonitor.rs

+	/// `true` when absent during upgrade so holder broadcasts aren't gated unexpectedly.
+	funding_seen_onchain: bool,
+	/// Tracks whether manual-broadcasting was requested before the funding transaction appeared on-chain.
+	manual_broadcast_pending: bool,


original comment from @TheBlueMatt #3838 (comment)

Hmm, I feel like we can just use holder_tx_signed rather than adding a new bool.

@TheBlueMatt how do I prevent queue_latest_holder_commitment_txn_for_broadcast from being called multiple times? holder_tx_signed is never set to false, so not sure how could I prevent it

ISTM we could call queue_latest_holder_commitment_txn_for_broadcast when we set funding_seen_onchain if holder_tx_signed is true? That should make it called only once cause we can use funding_seen_onchain indirectly to control it.

would this be ok? 6d4f901

ldk-reviews-bot · 2025-09-25T14:38:17Z

🔔 1st Reminder

Hey @tnull! 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

tnull

Did a first pass, changes look good so far I think.

Mind cleaning up the commit history to a) include a proper title/description for each feature commit b) maybe move the 'track funding transaction' and 'account for manual broadcast' parts into two different feature commits.

tnull · 2025-09-26T08:37:49Z

lightning/src/chain/channelmonitor.rs

 	pending_funding: Vec<FundingScope>,

+	/// True if this channel was configured for manual funding broadcasts. Monitors written by
+	/// versions prior to introducing the flag will load with `false` until a new update persists it.


Please be more specific on the version here and below, as it also gives us a hint when we can assume the new behavior and delete the comment/potentially old code.

Suggested change

/// versions prior to introducing the flag will load with `false` until a new update persists it.

/// versions prior to LDK 0.2 will load with `false` until a new update persists it.

tnull · 2025-09-26T08:43:49Z

lightning/src/chain/channelmonitor.rs

 			(32, pending_funding, optional_vec),
 			(33, htlcs_resolved_to_user, option),
 			(34, alternative_funding_confirmed, option),
+			(35, is_manual_broadcast, option),


Those could be simplified by reading with (default_value, X).

tnull · 2025-09-26T08:45:30Z

lightning/src/chain/channelmonitor.rs

+	/// transactions that cannot be confirmed until the funding transaction is visible.
+	///
+	/// [`Event::BumpTransaction`]: crate::events::Event::BumpTransaction
 	#[rustfmt::skip]


Mind dropping this rustfmt::skip in a separate (prefactor) commit while we're here?

tnull · 2025-09-26T09:59:14Z

lightning/src/chain/channelmonitor.rs

+
+		if self.is_manual_broadcast && !funding_seen_before && self.funding_seen_onchain && self.holder_tx_signed
+		{
+			self.queue_latest_holder_commitment_txn_for_broadcast(


I'm a bit confused - why do we queue the broadcast here immediately? Should we only do this if should_broadcast_commitment?

good catch. this was a silly mistake. I'm working on a fix but can't find the right answer. if I do

if self.is_manual_broadcast && !funding_seen_before && self.funding_seen_onchain && self.holder_tx_signed { should_broadcast_commitment = true; }

and not return immediately then it enqueues txs twice so tests fail. I'm working on it

TheBlueMatt · 2025-09-26T14:18:30Z

lightning/src/chain/channelmonitor.rs

+		// the funding transaction on-chain, do not queue any transactions.
+		if require_funding_seen && self.is_manual_broadcast && !self.funding_seen_onchain {
+			log_info!(logger, "Not broadcasting holder commitment for manual-broadcast channel before funding appears on-chain");
+			return;


I think we want to set holder_tx_signed and push the monitor event here. Basically, we should always call generate_claimable_outpoints_and_watch_outputs but we can skip actually passing them to the onchain_tx_handler, similar to what you did elsewhere.

TheBlueMatt · 2025-09-26T17:25:58Z

Feel free to squash, I think this is mostly there. Looks like CI is failing and can you write a commit message for the commit?

martinsaposnic · 2025-09-26T17:32:11Z

can you write a commit message for the commit?

sorry, what do you mean by this? @TheBlueMatt

TheBlueMatt · 2025-09-26T17:50:04Z

Err, sorry, write commit messages for all the commits. Your commits currently only have titles (and many of them are too long). The subject line should be no longer than ~70 chars, followed by description of why and what was done, as well as anything that might be surprising to someone reading the commit in 5 years. See https://cbea.ms/git-commit/ for more info.

martinsaposnic · 2025-09-30T11:18:31Z

@TheBlueMatt @tnull what do you think about the last fixup? I'm really not sure about the solution here, and I want your ack before I squash&rebase&rewrite the commit message

TheBlueMatt · 2025-10-01T09:37:26Z

lightning/src/chain/channelmonitor.rs

-				watch_outputs.append(&mut outputs);
+			// Only generate claims immediately if block_confirmed 
+			// won't also generate them to avoid duplicate registrations.
+			let should_broadcast = self.should_broadcast_holder_commitment_txn(logger);


Hmmm, yea, it does seem a bit strange to check if block_confirmed will do something and disable an important step if it will. But then if block_confirmed changes this code will be automatically broken without touching this code. Rather, ISTM block_confirmed needs to check if we already broadcasted before broadcasting.

TheBlueMatt

The new fixup looks good. Please feel free to squash fixups and clean up the git commit messages/history.

TheBlueMatt · 2025-10-05T09:38:05Z

lightning/src/chain/channelmonitor.rs

-			claimable_outpoints.append(&mut new_outpoints);
-			watch_outputs.append(&mut new_outputs);
+		// Only generate claims if we haven't already done so (e.g., in transactions_confirmed).
+		if claimable_outpoints.is_empty() && watch_outputs.is_empty() {


nit maybe just the first.

Suggested change

if claimable_outpoints.is_empty() && watch_outputs.is_empty() {

if claimable_outpoints.is_empty() {

martinsaposnic · 2025-10-06T12:18:59Z

The new fixup looks good. Please feel free to squash fixups and clean up the git commit messages/history.

ok, all squashed and history cleaned up with improved messages

one last small fixup to review @TheBlueMatt

TheBlueMatt

Feel free to squash, the changes themselves LGTM.

TheBlueMatt · 2025-10-07T14:02:31Z

lightning/src/chain/channelmonitor.rs

 	}

+	#[test]
+	fn test_manual_broadcast_skips_commitment_until_funding_seen() {


FWIW it might have been easier to write these as functional tests, which I generally personally prefer as they demonstrate the overall behavior rather than specific unit tests. It doesn't matter too much tho.

+1, doing so would avoid a lot of the boilerplate here

martinsaposnic · 2025-10-07T14:19:37Z

ok, fixups squashed https://github.com/lightningdevkit/rust-lightning/compare/9890bf9f63aabeb19dca56ef8e0f89fa9a732c2b..400730f25974061fc2c3b01d491a89261bad0706

tnull

Looks good from my side, mod the pending comments from other reviewers.

tnull · 2025-10-08T10:50:03Z

lightning/src/chain/channelmonitor.rs

+	/// observed on-chain, unless `require_funding_seen` is `false`. This prevents attempting to
+	/// broadcast unconfirmable holder commitment transactions before the funding is visible.
+	/// See also
+	/// [`crate::chain::channelmonitor::ChannelMonitor::broadcast_latest_holder_commitment_txn`].


nit: Instead of having this huge doc-link, we'd usually just link to ChannelMonitor::broadcast_latest_holder_commitment_txn and create a corresponding footnote below.

tnull · 2025-10-08T11:33:45Z

lightning/src/chain/channelmonitor.rs

+			if !self.funding_seen_onchain && (txid == self.funding.funding_txid() ||
+				self.pending_funding.iter().any(|f| f.funding_txid() == txid))
+			{
+				self.funding_seen_onchain = true;


Do we need to reset this in case of a reorg, i.e., in transactions_unconfirmed?

see #3838 (comment)

see #3838 (comment)

Ah, right, I knew I saw a related comment somewhere. Mind adding a comment (or two) in the code explaining that and why we're fine with it never being unset? FWIW, the current behavior might be okay for the current use case, but if someone would ever reuse the flag for some other application they might be surprised to find it's not unset if there's a reorg.

ldk-reviews-bot · 2025-10-09T19:31:31Z

🔔 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

Still lgtm, at least once the pending comments are addressed. Feel free to immediately squash when you do so, I think.

tnull · 2025-10-16T08:18:10Z

@martinsaposnic Any update here? Would be good to still land this for 0.2-final.

TheBlueMatt · 2025-10-16T12:35:38Z

He's at tab, but indicated he'd try to find some time this week.

martinsaposnic · 2025-10-19T21:56:44Z

just pushed 4 short fixups addressing comments,

the only pending is writing functional tests instead of unit tests, which is WIP and will push soon

…t_txn

Adds `is_manual_broadcast` and `funding_seen_onchain` flags to track whether the channel uses manual funding broadcasts and whether we've seen the funding tx confirm. This enables deferring holder commitment broadcasts until after the funding tx is actually broadcast. For example, in LSPS2 with client_trusts_lsp=true, the LSP may defer broadcasting the funding tx until the client claims an HTLC, so we need to avoid broadcasting commitments that reference outputs that don't exist yet.

Marks funding_seen_onchain when we see the funding tx confirm.

Don't queue holder commitment broadcasts until funding is confirmed, unless explicitly overridden via broadcast_latest_holder_commitment_txn. Attempting to broadcast commitments before funding confirms would fail mempool validation since the funding output doesn't exist yet.

For manually-broadcast funding, we can't track claimable outputs until the funding tx is actually onchain. Otherwise we'd try to claim outputs that don't exist yet.

Sets should_broadcast_commitment=true when funding confirms. Since we skip the initial broadcast when funding_seen_onchain is false, we need to queue it once funding actually hits the chain.

Tests that holder commitment broadcasts are properly deferred until funding confirms, and that the full manual-funding flow works correctly.

…and_watch_outputs

ldk-reviews-bot · 2025-10-23T13:05:45Z

🔔 1st Reminder

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

ldk-reviews-bot · 2025-10-25T00:00:18Z

🔔 1st Reminder

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

ldk-reviews-bot · 2025-10-25T00:00:20Z

🔔 1st Reminder

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

ldk-reviews-bot · 2025-10-25T13:06:30Z

🔔 2nd Reminder

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

TheBlueMatt added this to the 0.2 milestone Sep 23, 2025

martinsaposnic mentioned this pull request Sep 23, 2025

Support client_trusts_lsp on LSPS2 #3838

Merged

tnull self-requested a review September 23, 2025 14:37

martinsaposnic commented Sep 23, 2025

View reviewed changes

martinsaposnic force-pushed the track-funding-tx-channelmonitor branch from 236e01f to 6d4f901 Compare September 25, 2025 16:13

martinsaposnic marked this pull request as ready for review September 25, 2025 16:14

graphite-app bot reviewed Sep 25, 2025

View reviewed changes

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

martinsaposnic requested a review from TheBlueMatt September 25, 2025 17:33

tnull reviewed Sep 26, 2025

View reviewed changes

TheBlueMatt reviewed Sep 26, 2025

View reviewed changes

martinsaposnic force-pushed the track-funding-tx-channelmonitor branch from a257c8e to 61d9c28 Compare September 26, 2025 16:18

TheBlueMatt reviewed Oct 1, 2025

View reviewed changes

martinsaposnic force-pushed the track-funding-tx-channelmonitor branch from 3db3230 to 9bd2ccc Compare October 2, 2025 08:59

TheBlueMatt reviewed Oct 5, 2025

View reviewed changes

martinsaposnic force-pushed the track-funding-tx-channelmonitor branch 2 times, most recently from 860ba2d to 9890bf9 Compare October 6, 2025 12:16

martinsaposnic requested a review from TheBlueMatt October 6, 2025 12:19

TheBlueMatt requested a review from tnull October 7, 2025 14:03

TheBlueMatt reviewed Oct 7, 2025

View reviewed changes

martinsaposnic force-pushed the track-funding-tx-channelmonitor branch from 9890bf9 to 400730f Compare October 7, 2025 14:18

tnull reviewed Oct 8, 2025

View reviewed changes

TheBlueMatt added the backport 0.2 label Oct 9, 2025

TheBlueMatt reviewed Oct 9, 2025

View reviewed changes

martinsaposnic added 8 commits October 19, 2025 22:57

Prefactor: drop #[rustfmt::skip] on broadcast_latest_holder_commitmen…

f2a1a73

…t_txn

Set funding_seen_onchain=true in filter_block

787e0a9

Marks funding_seen_onchain when we see the funding tx confirm.

Defer claimable tracking until funding tx confirms

f98ffba

For manually-broadcast funding, we can't track claimable outputs until the funding tx is actually onchain. Otherwise we'd try to claim outputs that don't exist yet.

Queue holder commit once funding tx confirms

943f176

Sets should_broadcast_commitment=true when funding confirms. Since we skip the initial broadcast when funding_seen_onchain is false, we need to queue it once funding actually hits the chain.

Test manual broadcast tracking and holder commit flow

c7642e6

Tests that holder commitment broadcasts are properly deferred until funding confirms, and that the full manual-funding flow works correctly.

fixup: move funding_seen_onchain tracking to transactions_confirmed

2646567

martinsaposnic force-pushed the track-funding-tx-channelmonitor branch from ea53d35 to 4a6e4e3 Compare October 20, 2025 01:58

martinsaposnic added 4 commits October 20, 2025 12:39

fixup: use footnote for broadcast_latest_holder_commitment_txn doc link

5f9d67c

fixup: doc that funding_seen_onchain is never reset

e9403f4

fixup: make tests functional instead of unit tests

8963823

fixup: move manual broadcast check into generate_claimable_outpoints_…

d88d594

…and_watch_outputs

martinsaposnic force-pushed the track-funding-tx-channelmonitor branch from 94ccd18 to d88d594 Compare October 20, 2025 16:00

fixup: making ci happy

5b85deb

martinsaposnic requested review from TheBlueMatt, tnull and wpaulino October 21, 2025 13:04

	/// versions prior to introducing the flag will load with `false` until a new update persists it.
	/// versions prior to LDK 0.2 will load with `false` until a new update persists it.

	if claimable_outpoints.is_empty() && watch_outputs.is_empty() {
	if claimable_outpoints.is_empty() {

Track funding tx channelmonitor #4109

Are you sure you want to change the base?

Track funding tx channelmonitor #4109

Conversation

martinsaposnic commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

ldk-reviews-bot commented Sep 25, 2025

Uh oh!

Uh oh!

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

TheBlueMatt commented Sep 26, 2025

Uh oh!

martinsaposnic commented Sep 26, 2025

Uh oh!

TheBlueMatt commented Sep 26, 2025

Uh oh!

martinsaposnic commented Sep 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinsaposnic commented Oct 6, 2025

Uh oh!

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

martinsaposnic commented Oct 7, 2025

Uh oh!

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

ldk-reviews-bot commented Oct 9, 2025

Uh oh!

TheBlueMatt left a comment

martinsaposnic commented Sep 23, 2025 •

edited

Loading

ldk-reviews-bot commented Sep 23, 2025 •

edited

Loading

codecov bot commented Sep 23, 2025 •

edited

Loading