Broadcast holder commitment for currently confirmed funding by wpaulino · Pull Request #3939 · lightningdevkit/rust-lightning

wpaulino · 2025-07-17T17:12:16Z

A splice's FundingScope can only be promoted once a ChannelMonitorUpdateStep::RenegotiatedFundingLocked is applied, or if the monitor is no longer accepting updates, once the splice transaction is no longer under reorg risk. Because of this, our current FundingScope may not reflect the latest confirmed state in the chain. Before making a holder commitment broadcast, we must check which FundingScope is currently confirmed to ensure that it can propagate throughout the network.

ldk-reviews-bot · 2025-07-17T17:12:19Z

👋 Thanks for assigning @jkczyz 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-07-18T22:42:46Z

Codecov Report

❌ Patch coverage is 47.00000% with 106 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.76%. Comparing base (ecce268) to head (89ce01d).
⚠️ Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	43.24%	92 Missing and 13 partials ⚠️
lightning/src/chain/onchaintx.rs	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3939 +/- ##
==========================================
- Coverage 88.93% 88.76% -0.18% 
==========================================
Files 174 173 -1 Lines 123842 124326 +484 Branches 123842 124326 +484 ==========================================
+ Hits 110142 110357 +215 - Misses 11254 11553 +299 + Partials 2446 2416 -30

Flag	Coverage Δ
fuzzing	`?`
tests	`88.76% <47.00%> (-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.

ldk-reviews-bot · 2025-07-21T00:01:01Z

🔔 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-07-22T00:15:42Z

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

ldk-reviews-bot · 2025-07-26T00:00:27Z

🔔 1st Reminder

Hey @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-07-28T00:00:59Z

🔔 2nd Reminder

Hey @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-07-30T00:01:40Z

🔔 3rd Reminder

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

TheBlueMatt · 2025-07-30T14:45:20Z

+ debug_assert!(self.alternative_funding_confirmed.is_none());
+ self
+ .pending_funding
+ .drain(..)


This looks like it needs to handle multiple incomplete 0conf fundings too, though - we can accept one 0conf splice then another before either confirm, or accept another after the first only has 1 conf. We'd need to handle that here by only dropping some fundings...
I kinda feel like we should just drop the last commit and do it later, rather than trying to support it right now.

Ah right, the key aspect here is that the zero conf splices chain upon one another, unlike RBFs.

ldk-reviews-bot · 2025-08-02T00:00:22Z

🔔 4th 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-08-02T00:00:27Z

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

ldk-reviews-bot · 2025-08-04T00:01:00Z

🔔 5th 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-08-04T00:01:06Z

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

jkczyz · 2025-08-05T22:20:36Z

+ debug_assert!(self.no_further_updates_allowed());
+ debug_assert_ne!(self.funding.funding_txid(), entry.txid);
+ if let Err(_) = self.promote_funding(entry.txid) {
+ log_error!(logger, "Missing scope for alternative funding confirmation with txid {}", entry.txid);


When would this happen in practice? I assume it's some corner case, but is there any danger in this happening? What would a user do if it happened?

It shouldn't happen at all. It would mean that the scope was somehow removed, and the only way to do that currently is via promote_funding. The monitor is already not accepting updates so that's good, but it could lead to not being able to claim funds from a closed channel.

Whether it's a splice, or a dual-funded RBF, we need to know which funding transaction out of all of the negotiated ones is currently confirmed in case we need to broadcast the holder commitment.

A `FundingScope` can only be promoted once a `ChannelMonitorUpdateStep::RenegotiatedFundingLocked` is applied, or if the monitor is no longer accepting updates, once the renegotiated funding transaction is no longer under reorg risk. Because of this, our current `FundingScope` may not reflect the latest confirmed state in the chain. Before making a holder commitment broadcast, we must check which `FundingScope` is currently confirmed to ensure that it can propogate throughout the network.

jkczyz

LGTM, but should let @TheBlueMatt take a final pass as I'm not intimately familiar with this code.

ldk-reviews-bot · 2025-08-06T00:01:44Z

🔔 3rd 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

A few assertion nits and one real question that IMO should be addressed, but doesn't have to be in this PR. Gonna land to unblock progress.

wpaulino added this to Weekly Goals Jul 17, 2025

ldk-reviews-bot requested a review from joostjager July 17, 2025 17:23

jkczyz mentioned this pull request Jul 17, 2025
Dual-funded channels and Splicing Project Tracking #1621
Closed
7 tasks

wpaulino removed the request for review from joostjager July 17, 2025 21:00

wpaulino force-pushed the splice-funding-commitment-broadcast branch from a783444 to a8ae4b7Compare July 18, 2025 22:23

wpaulino requested a review from TheBlueMatt July 18, 2025 22:23

wpaulino mentioned this pull request Jul 21, 2025
Introduce RenegotiatedFundingLocked monitor update variant #3894
Merged

TheBlueMatt reviewed Jul 22, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated

TheBlueMatt reviewed Jul 22, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino force-pushed the splice-funding-commitment-broadcast branch from a8ae4b7 to 86b53faCompare July 22, 2025 16:39

wpaulino requested a review from TheBlueMatt July 22, 2025 16:40

TheBlueMatt reviewed Jul 22, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino force-pushed the splice-funding-commitment-broadcast branch from 86b53fa to 48629c7Compare July 22, 2025 23:15

jkczyz self-requested a review July 23, 2025 18:18

wpaulino force-pushed the splice-funding-commitment-broadcast branch from 48629c7 to 60d97d0Compare July 24, 2025 00:10

wpaulino requested a review from TheBlueMatt July 24, 2025 00:11

graphite-appBot reviewed Jul 24, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated

TheBlueMatt reviewed Jul 25, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs

wpaulino requested a review from TheBlueMatt July 29, 2025 01:09

TheBlueMatt reviewed Jul 29, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino force-pushed the splice-funding-commitment-broadcast branch 3 times, most recently from 582ad71 to 511fdc8Compare July 29, 2025 19:39

TheBlueMatt reviewed Jul 30, 2025
View reviewed changes

wpaulino force-pushed the splice-funding-commitment-broadcast branch from 511fdc8 to 9051703Compare July 30, 2025 18:27

wpaulino requested a review from TheBlueMatt July 30, 2025 18:30

TheBlueMatt assigned wpaulino Jul 31, 2025

wpaulino force-pushed the splice-funding-commitment-broadcast branch from 9051703 to 919b1d8Compare July 31, 2025 18:18

wpaulino mentioned this pull request Jul 31, 2025
Consider currently confirmed FundingScope when claiming commitments #3980
Merged

jkczyz reviewed Aug 4, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
Comment threadlightning/src/chain/channelmonitor.rs

wpaulino requested a review from jkczyz August 4, 2025 21:40

wpaulino force-pushed the splice-funding-commitment-broadcast branch from 919b1d8 to 88b2c22Compare August 4, 2025 21:40

jkczyz reviewed Aug 5, 2025
View reviewed changes

wpaulino added 2 commits August 5, 2025 16:10

Detect channel alternative funding transaction confirmation
ad99d9a
Whether it's a splice, or a dual-funded RBF, we need to know which funding transaction out of all of the negotiated ones is currently confirmed in case we need to broadcast the holder commitment.

wpaulino force-pushed the splice-funding-commitment-broadcast branch from 88b2c22 to 89ce01dCompare August 5, 2025 23:10

wpaulino requested a review from jkczyz August 5, 2025 23:11

jkczyz approved these changes Aug 5, 2025
View reviewed changes

TheBlueMatt approved these changes Aug 6, 2025
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs
Comment threadlightning/src/chain/channelmonitor.rs
Comment threadlightning/src/chain/channelmonitor.rs

TheBlueMatt merged commit de2005a into lightningdevkit:mainAug 6, 2025
23 of 25 checks passed

github-project-automationBot moved this to Done in Weekly Goals Aug 6, 2025

wpaulino deleted the splice-funding-commitment-broadcast branch August 6, 2025 20:03

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented Jul 21, 2025

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 26, 2025

Uh oh!

ldk-reviews-bot commented Jul 28, 2025

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMattJul 30, 2025

Choose a reason for hiding this comment

Uh oh!

wpaulinoJul 30, 2025

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Aug 2, 2025

Uh oh!

ldk-reviews-bot commented Aug 2, 2025

Uh oh!

ldk-reviews-bot commented Aug 4, 2025

Uh oh!

ldk-reviews-bot commented Aug 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyzAug 5, 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!

wpaulinoAug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Aug 6, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

wpaulino commented Jul 17, 2025•
edited
Loading

ldk-reviews-bot commented Jul 17, 2025•
edited
Loading

codecovBot commented Jul 18, 2025•
edited
Loading

jkczyzAug 5, 2025•
edited
Loading