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
wpaulino
commented
|
👋 Thanks for assigning @jkczyz as a reviewer! |
Review SummaryAfter a thorough review of all files and hunks in this PR, I found the code to be generally well-structured and correct. The prior review already identified the key issues (HashSet ordering nondeterminism,
Cross-cutting observations (not tied to specific lines):
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4498 +/- ##
==========================================
- Coverage 87.11% 87.09% -0.03%
==========================================
Files 161 161
Lines 109246 109322 +76
Branches 109246 109322 +76
==========================================
+ Hits 95173 95209 +36
- Misses 11592 11627 +35
- Partials 2481 2486 +5
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. |
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.
wpaulino
force-pushed
the
channel-monitor-discard-funding-contribution
branch
from
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).
| self.pending_events.push(Event::DiscardFunding { | ||
| channel_id: self.channel_id, | ||
| funding_info: FundingInfo::Contribution { | ||
| inputs: discarded_inputs.into_iter().collect(), | ||
| outputs: discarded_outputs.into_iter().collect(), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Hmm... in channelmanager.rs, we make a separate DiscardFunding event for each attempt. Should we do the same here? It would remove the non-determinism introduced by the hash set.
4 participants
