Produce FundingInfo::Contribution variants in ChannelMonitor#4498
Produce FundingInfo::Contribution variants in ChannelMonitor#4498wpaulino wants to merge 2 commits intolightningdevkit:mainfrom
Conversation
wpaulino
commented
Mar 19, 2026
|
👋 Thanks for assigning @jkczyz as a reviewer! |
|
I've thoroughly reviewed the entire PR diff, reading every file and hunk. I also reviewed my prior comments to avoid duplication. No new issues found. Review SummaryAfter examining all changed files (
Prior review items (still applicable)The issues flagged in my previous review pass remain relevant:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4498 +/- ##
==========================================
- Coverage 86.11% 86.11% -0.01%
==========================================
Files 157 157
Lines 108851 108845 -6
Branches 108851 108845 -6
==========================================
- Hits 93735 93727 -8
+ Misses 12503 12497 -6
- Partials 2613 2621 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 13th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 14th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 15th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 16th Reminder Hey @jkczyz! This PR has been waiting for your review. |
ca819d9 to
e07c154
Compare
| let signing_session = self | ||
| .context | ||
| .interactive_tx_signing_session | ||
| .as_ref() | ||
| .map(|signing_session| !signing_session.has_received_tx_signatures()) | ||
| .unwrap_or(false)); | ||
| .expect("Signing session must exist for negotiated pending splice"); | ||
| debug_assert!(!signing_session.has_received_tx_signatures()); |
There was a problem hiding this comment.
Nit: This changes from a debug_assert (no-op in release builds) to expect (panics in release builds). The old code combined the existence check and state check into a single debug assertion; now existence is enforced unconditionally while the state check remains debug-only.
This is arguably safer (fail-fast instead of silent invariant violation), but since signing_session isn't used beyond the debug_assert on the next line, consider whether returning a ChannelError would be more robust than panicking — consistent with how this function handles other missing state (e.g., the commitment_point error path at line 8340).
Exposing the amounts for each output isn't very helpful because it's possible that they vary across over multiple splice candidates due to RBF. This commit changes `FundingInfo::Contribution` and several of the helpers used to derive it to be based on output scripts instead.
Similar to the `ChannelManager`, we expose the contributed inputs and outputs of a splice via `FundingInfo::Contribution` at the `ChannelMonitor` level such that we don't lose the context when the channel closes while a splice is still pending. This relies on tracking the `FundingContribution` that was provided to the `ChannelManager` prior to negotiating the new funding transaction. If no `FundingContribution` exists, then we continue to emit the `FundingInfo::OutPoint` variant.
e07c154 to
7e806f9
Compare