Detect commitment transaction confirmation in ChannelMonitor instead#4013
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
This was already the case, at least if you connected the block to the monitor first, probably worth updating the commit message. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
CI is unhappy but otherwise happy to land this.
|
👋 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. |
Previously, the `ChannelManager` would assume a `Channel` was closed the moment it saw a spend for its funding input. With splicing, this will no longer be the case. Since the `ChannelMonitor` is already responsible for reliably tracking each onchain transaction relevant to a channel, we now produce a `MonitorEvent::CommitmentTxConfirmed` event to inform the `ChannelManager` the channel can be considered closed and removed. As a result of this change, many tests failed now that we rely on handling the `MonitorEvent::CommitmentTxConfirmed` first before seeing the `ChannelMonitorUpdateStep::ChannelForceClosed` go out.
wpaulino
force-pushed
the
commitment-tx-confirmed-from-monitor
branch
from
da958b0 to
68cd71c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4013 +/- ##
==========================================
- Coverage 88.85% 88.84% -0.02%
==========================================
Files 175 175
Lines 127710 127723 +13
Branches 127710 127723 +13
==========================================
- Hits 113478 113476 -2
- Misses 11675 11686 +11
- Partials 2557 2561 +4
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:
|
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Gonna go ahead and land this. Its really trivial code-change, and while its a bit of a change logic-wise, we already have to support basically this - someone can connect a block to the ChannelMonitor and then sit around for a minute before connecting the block to the ChannelManager, so any behavior that relied on the Channel being closed directly would have been buggy anyway.
It does mean we will sometimes send a channel_ready and then close later if a channel is closed in the same block it was opened, but who cares.
| let err = ChannelError::Close((reason.to_string(), reason)); | ||
| let mut chan = chan_entry.remove(); | ||
| let (_, e) = convert_channel_err!(self, peer_state, err, &mut chan); | ||
| failed_channels.push((Err(e), counterparty_node_id)); |
There was a problem hiding this comment.
We always generate a ChannelMonitorUpdate to tell the ChannelMonitor that this channel is closed...which the monitor just told us about. The only thing it really does is set lockdown_from_offchain in the ChannelMonitor which adds panics if we later provide a ChannelMonitorUpdate for the channel, but those are just sanity checks and definitely aren't something to protect for an extra monitor update.
Isn't required here cause its not new, but we should drop the ChannelMonitorUpdate in a followup.
3 participants
Previously, the
ChannelManagerwould assume aChannelwas closed the moment it saw a spend for its funding input. With splicing, this will no longer be the case. Since theChannelMonitoris already responsible for reliably tracking each onchain transaction relevant to a channel, we now produce aMonitorEvent::CommitmentTxConfirmedevent to inform theChannelManagerthe channel can be considered closed and removed.As a result of this change, many tests failed now that we rely on handling the
MonitorEvent::CommitmentTxConfirmedfirst before seeing theChannelMonitorUpdateStep::ChannelForceClosedgo out.