Skip to content

Minor bugfixes and test improvements to #4109 - #4182

Merged
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom
TheBlueMatt:2025-10-4109-followups
Oct 29, 2025
Merged

Minor bugfixes and test improvements to #4109#4182
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom
TheBlueMatt:2025-10-4109-followups

Conversation

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

No description provided.

In 6c5ef04 we prevented broadcast
of the commitment transactions if the funding transaction has not
yet appeared on-chain for manual-broadcast channels to avoid
spurious bumps or unbroadcastable transactions. It also updated
the documentation on
`ChanelMonitor::broadcast_latest_holder_commitment_txn` to
explicitly state that it will override the manual-broadcast state
and broadcast the latest commitment anyway.
However, 4131680 accidentally
reverted this behavior by updating
`generate_claimable_outpoints_and_watch_outputs`, which is caled by
`broadcast_latest_holder_commitment_txn` to also refuse to
broadcast if funding has not been seen on chain.
Here we fix this, passing through the `require_funding_seen` bool
to allow `broadcast_latest_holder_commitment_txn` to broadcast
immediately.
@TheBlueMattTheBlueMatt added this to the 0.2 milestone Oct 28, 2025
@ldk-reviews-bot

ldk-reviews-bot commented Oct 28, 2025

Copy link
Copy Markdown

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

codecovBot commented Oct 28, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.74436% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.83%. Comparing base (17f7858) to head (e1a4259).
⚠️ Report is 7 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/ln/functional_test_utils.rs96.42%2 Missing ⚠️
lightning/src/ln/functional_tests.rs98.30%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #4182 +/- ##
==========================================
- Coverage 88.83% 88.83% -0.01% 
==========================================
Files 180 180 Lines 137504 137503 -1 Branches 137504 137503 -1 ==========================================
- Hits 122155 122150 -5 - Misses 12538 12542 +4 
Partials 2811 2811 
FlagCoverage Δ
fuzzing20.89% <10.81%> (-0.60%)⬇️
tests88.67% <97.74%> (-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.

Comment threadlightning/src/chain/channelmonitor.rs
Comment threadlightning/src/ln/functional_test_utils.rs Outdated
@ldk-reviews-bot

Copy link
Copy Markdown

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

@TheBlueMatt
TheBlueMattforce-pushed the 2025-10-4109-followups branch 2 times, most recently from 91b71c1 to 59f28e1CompareOctober 28, 2025 23:10
@ldk-reviews-bot

Copy link
Copy Markdown

✅ Added second reviewer: @jkczyz

@wpaulino

Copy link
Copy Markdown
Contributor

Feel free to squash

In 6c5ef04 we prevented broadcast
of the commitment transactions if the funding transaction has not
yet appeared on-chain for manual-broadcast channels to avoid
spurious bumps or unbroadcastable transactions. However, we missed
the case where a channel is closed due to HTLCs timing out.
Here we fix that by doing the same broadcast-gating when
automatically force-closing a channel due to HTLC timeouts.
This rewrites
`test_manual_broadcast_skips_commitment_until_funding` to test
manual-broadcast channel closing for both forced-broadcast and
close-due-to-HTLC-expiry closes as well as 0-conf and non-0-conf
channels.
The remaining two manual-broadcast tests were made redundant by the
previous commit which expanded the coverage of the first test. Thus
we remove the two other tests.
In `generate_claimable_outpoints_and_watch_outputs` if we're going
to decide to return nothing and defer broadcasting the commitment
transaction, there's no need to prepare the broadcast tracking
objects, so skip it.
@TheBlueMatt
TheBlueMattforce-pushed the 2025-10-4109-followups branch from 59f28e1 to e1a4259CompareOctober 29, 2025 01:15
@TheBlueMatt
TheBlueMatt merged commit 9a845bb into lightningdevkit:mainOct 29, 2025
22 of 25 checks passed
@TheBlueMattTheBlueMatt mentioned this pull request Oct 29, 2025
@TheBlueMatt

Copy link
Copy Markdown
CollaboratorAuthor

Backported to 0.2 in #4185.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@ldk-reviews-bot@wpaulino@jkczyz