Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only generate a post-close lock ChannelMonitorUpdate if we need one #3619

Merged
merged 3 commits into from
Mar 3, 2025
Merged

Only generate a post-close lock ChannelMonitorUpdate if we need one #3619

merged 3 commits into from
Mar 3, 2025

Conversation

TheBlueMatt
Copy link
Collaborator 

If a channel is closed on startup, but we find that the
`ChannelMonitor` isn't aware of this, we generate a
`ChannelMonitorUpdate` containing a
`ChannelMonitorUpdateStep::ChannelForceClosed`. This ensures that
the `ChannelMonitor` will not accept any future updates in case we
somehow load up a previous `ChannelManager` (though that really
shouldn't happen).

Previously, we'd apply this update only if we detected that the
`ChannelManager` had not yet informed the `ChannelMonitor` about
the channel's closure, even if the `ChannelMonitor` would already
refuse any other updates because it detected a channel closure
on chain.

This doesn't accomplish anything but an extra I/O write, so we
remove it here.

Further, a user reported that, in regtest, they could:
 (a) coop close a channel (not generating a `ChannelMonitorUpdate`)
 (b) wait just under 4032 blocks (on regtest, taking only a day)
 (c) restart the `ChannelManager`, generating the above update
 (d) connect a block or two (during the startup sequence), making
     the `ChannelMonitor` eligible for archival,
 (d) restart the `ChannelManager` again (without applying the
     update from (c), but after having archived the
     `ChannelMonitor`, leading to a failure to deserialize as we
     have a pending `ChannelMonitorUpdate` for a `ChannelMonitor`
     that has been archived.

Though it seems very unlikely this would happen on mainnet, it is
theoretically possible.

@TheBlueMatt
Copy link
Collaborator Author

I didn't bother writing a test here because (a) we have decent coverage of monitors on shutdown already, (b) this is kinda more of an optimization around I/O than a bugfix and its quite straightforward, and (c) it would break when we fix #2238. I could write a temporary test knowing that we're gonna remove it (hopefully) soon, if folks prefer, though.

@wpaulino
Copy link
Contributor

LGTM. The title of the first commit makes it sound like monitors were blocked from signing when cancelling claims, but claims were only being cancelled based on whether signing already happened or not. I'm fine with not needing test coverage here though.

@TheBlueMatt
Cancel claims signed by a remote ChannelMonitor when reorging
1f0e69d 
In `ChannelMonitorImpl::cancel_prev_commitment_claims` we need to
cancel any claims against a removed commitment transaction. We
were checking if `holder_tx_signed` before checking if either the
current or previous holder commitment transaction had pending
claims against it, but (a) there's no need to do this, there's not
a big performance cost to just always trying to remove claims and
(b) we can't actually rely on `holder_tx_signed`.

`holder_tx_signed` being set doesn't necessarily imply that the
`ChannelMonitor` was persisted (i.e. it may simply be lost in a
poorly-timed restart) but we also (somewhat theoretically) allow
for multiple copies of a `ChannelMonitor` to exist, and a different
one could have signed the commitment transaction which was
confirmed (and then unconfirmed).

Thus, we simply remove the additional check here.
@TheBlueMatt
Drop return value from provide_latest_holder_commitment_tx
489d70a 
`provide_latest_holder_commitment_tx` is used to handle
`ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo` updates
and returns an `Err` if we've set `holder_tx_signed`.

However, later in `ChannelMonitorImpl::update_monitor` (the only
non-test place that `provide_latest_holder_commitment_tx` is
called), we will fail the entire update if `holder_tx_signed` is
(or a few other flags are) are set if the update contained a
`LatestHolderCommitmentTXInfo` (or a few other update types).

Thus, the check in `provide_latest_holder_commitment_tx` is
entirely redundant and can be removed.
@TheBlueMatt
Only generate a post-close lock ChannelMonitorUpdate if we need one
cd2e169 
If a channel is closed on startup, but we find that the
`ChannelMonitor` isn't aware of this, we generate a
`ChannelMonitorUpdate` containing a
`ChannelMonitorUpdateStep::ChannelForceClosed`. This ensures that
the `ChannelMonitor` will not accept any future updates in case we
somehow load up a previous `ChannelManager` (though that really
shouldn't happen).

Previously, we'd apply this update only if we detected that the
`ChannelManager` had not yet informed the `ChannelMonitor` about
the channel's closure, even if the `ChannelMonitor` would already
refuse any other updates because it detected a channel closure
on chain.

This doesn't accomplish anything but an extra I/O write, so we
remove it here.

Further, a user reported that, in regtest, they could:
 (a) coop close a channel (not generating a `ChannelMonitorUpdate`)
 (b) wait just under 4032 blocks (on regtest, taking only a day)
 (c) restart the `ChannelManager`, generating the above update
 (d) connect a block or two (during the startup sequence), making
     the `ChannelMonitor` eligible for archival,
 (d) restart the `ChannelManager` again (without applying the
     update from (c), but after having archived the
     `ChannelMonitor`, leading to a failure to deserialize as we
     have a pending `ChannelMonitorUpdate` for a `ChannelMonitor`
     that has been archived.

Though it seems very unlikely this would happen on mainnet, it is
theoretically possible.
@TheBlueMatt TheBlueMatt force-pushed the 2025-02-merge-chanmon-lockdown branch from cc543b5 to cd2e169 Compare February 26, 2025 21:11
@TheBlueMatt
Copy link
Collaborator Author

Fixed the commit message

@TheBlueMatt TheBlueMatt added the weekly goal Someone wants to land this this week label Feb 27, 2025
@valentinewallace
Copy link
Contributor

Landing since CI failures look unrelated

@valentinewallace valentinewallace merged commit 5786674 into lightningdevkit:main Mar 3, 2025
22 of 26 checks passed
@wpaulino wpaulino mentioned this pull request Mar 5, 2025
Open
@TheBlueMatt
Copy link
Collaborator Author

Backported in #3613.

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

@wpaulino wpaulino wpaulino approved these changes

@valentinewallace valentinewallace valentinewallace approved these changes

Assignees
No one assigned
Labels
weekly goal Someone wants to land this this week
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TheBlueMatt @wpaulino @valentinewallace