Skip to content

Conversation

wpaulino
Copy link
Contributor

This also caught a bug where we weren't including signatures for pending HTLCs in the initial commitment_signed for the splice.

Depends on #4054.

@wpaulino wpaulino added this to the 0.2 milestone Sep 11, 2025
@wpaulino wpaulino self-assigned this Sep 11, 2025
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Sep 11, 2025

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

@ldk-reviews-bot
Copy link

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

@wpaulino wpaulino force-pushed the test-splice-commitment-broadcast branch from 527c78a to 7456483 Compare September 18, 2025 00:15
Copy link

codecov bot commented Sep 18, 2025

Codecov Report

❌ Patch coverage is 96.98795% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.70%. Comparing base (5a84daf) to head (aa912ce).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/splicing_tests.rs 97.16% 2 Missing and 2 partials ⚠️
lightning/src/ln/channel.rs 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4068      +/-   ##
==========================================
- Coverage   88.71%   88.70%   -0.01%     
==========================================
  Files         177      177              
  Lines      133245   133404     +159     
  Branches   133245   133404     +159     
==========================================
+ Hits       118202   118331     +129     
- Misses      12331    12361      +30     
  Partials     2712     2712              
Flag Coverage Δ
fuzzing 21.73% <4.00%> (-0.01%) ⬇️
tests 88.54% <96.98%> (-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
Copy link

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

1 similar comment
@ldk-reviews-bot
Copy link

🔔 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
Copy link

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

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM, I'll leave @jkczyz to review the tests in more detail.

OnchainEvent::FundingSpendConfirmation { commitment_tx_to_counterparty_output, .. } => {
self.funding_spend_confirmed = Some(entry.txid);
self.confirmed_commitment_tx_counterparty_output = commitment_tx_to_counterparty_output;
if self.alternative_funding_confirmed.is_none() && !self.pending_funding.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would we get this event when self.alternative_funding_confirmed is Some? Is there some re-org case that could cause this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always get a FundingSpendConfirmation when we see a commitment transaction confirm, alternative_funding_confirmed just tells us if a funding transaction other than the one tracked by self.funding is the one that confirmed instead.

@wpaulino wpaulino force-pushed the test-splice-commitment-broadcast branch from 7456483 to 35c37cb Compare September 23, 2025 21:46
@wpaulino wpaulino requested a review from jkczyz September 23, 2025 21:46
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_splice_commitment_broadcast failing in CI

While we did consider the pending HTLCs when generating the signatures,
we did not include them in the resulting `commitment_signed` message
sent because we assumed it was only used within a dual-funding context
where there are no pending HTLCs.
This ensures a valid commitment transaction is broadcast according to
the different stages of a splice:

1. Negotiated but unconfirmed
2. Confirmed but not locked
3. Locked
When adding support for emitting these events in the channel monitor, we
only covered the case where one of the splice transaction candidates
confirmed. We also need to emit an event when none of them can confirm
due to a commitment transaction confirming (and no longer under reorg
risk) for the pre-splice funding.
@wpaulino wpaulino force-pushed the test-splice-commitment-broadcast branch from 35c37cb to aa912ce Compare September 24, 2025 02:56
@wpaulino
Copy link
Contributor Author

CI was failing due to not being rebased on #3984

@wpaulino wpaulino requested a review from jkczyz September 24, 2025 02:58
@jkczyz jkczyz merged commit 3e21ba3 into lightningdevkit:main Sep 24, 2025
25 checks passed
@wpaulino wpaulino deleted the test-splice-commitment-broadcast branch September 24, 2025 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants