Somewhat clean up closure pipelines #3881
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
👋 Thanks for assigning @wpaulino as a reviewer! |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
tnull
reviewed
Jun 23, 2025
There was a problem hiding this comment.
I think it would be good to document all these subtle API changes in a pending_changelog, as users might be leaning on a certain error variant.
There was a problem hiding this comment.
This seems unaddressed so far?
| @@ -4350,85 +4360,66 @@ where | |||
| /// `peer_msg` should be set when we receive a message from a peer, but not set when the | |||
| /// user closes, which will be re-exposed as the `ChannelClosed` reason. | |||
| #[rustfmt::skip] | |||
Mind putting that PR up as draft already? Would help me gauge whether I should hold off on simple close related refactors until these two PRs are in. |
TheBlueMatt
force-pushed
the
2025-06-fc-always-broadcast
branch
from
12c0a08 to
e73434f
Compare
`ChannelManager::force_close_channel_with_peer` takes the peer's node_id as a parameter and then returns it, presumably leftover from before we stored channels indexed by peers. Here we simply drop the return value.
When a channel is "coop" closed prior to it being funded, it is closed immediately. Instead of reporting `ClosureReason::*InitiatedCooperativeClosure` (which would be somewhat misleading), we report `ClosureReason::CounterpartyCoopClosedUnfundedCHannel` when our counterparty does it. However, when we do it, we report `ClosureReason::HolderForceClosed`, which is highly confusing given the user did *not* call a force-closure method. Instead, here, we add a `ClosureReason::LocallyCoopClosedUnfundedChannel` to match the `CounterpartyCoopClosedUnfundedCHannel` variant and use it.
`ClosureReason::HolderForceClosed` says explicitly in its docs "Closure generated from `ChannelManager...`, called by the user." However, we were using it extensively when we fail a channel due to errors handling messages or generating our own responses. Here, we convert these cases to the catch-all `ClosureReason::ProcessingError` which exists to communicate errors including a string that describes what happened. This should substantially improve debug-ability in closure cases by avoiding having to pull up logs.
Here we fix a few cases of swallowing appropriate `ClosureReason`s and expand `ClosureReason::FundingTimedOut` to include when we didn't get a channel funded in time, rather than using `HolderForceClosed`.
In d17e759 we added the ability for users to decide which message to send to peers when we force-close a channel. Here we pipe that message back out through the channel closure event via a new `message` field in `ClosureReason::HolderForceClosed`. This is nice to have but more importantly will be used in a coming commit to simplify the internal interface to the force-closure logic.
Removes totally unnecessary `#[inline]`s on `MsgHandleErrInternal` methods (LLVM will decide for us if it makes sense to inline something, explicit `#[inline(always)]` belongs only on incredibly performance-sensitive code and `#[inline]` belongs only on short public methods that should be inlined into downstream crates) and `rustfmt` `MsgHandleErrInternal::from_chan_no_close`.
`ChannelManager` has two public methods to force-close a (single) channel - `force_close_broadcasting_latest_txn` and `force_close_without_broadcasting_txn`. The second exists to allow a user who has restored a stale backup to (sort of) recover their funds by starting, force-closing channels without broadcasting their latest state, and only then reconnecting to peers (which would otherwise cause a panic due to the stale state). To my knowledge, no one has ever implemented this (fairly half-assed) recovery flow, and more importantly its rather substantially broken. `ChannelMonitor` has never tracked any concept of whether a channel is stale, and if we connect blocks towards an HTLC time-out it will immediately broadcast the stale state anyway. Finally, we really shouldn't encourage users to try to "recover" from a stale state by running an otherwise-operational node on the other end. Rather, we should formalize such a recovery flow by having a `ChainMonitor` variant that takes a list of `ChannelMonitor`s and claims available funds, dropping the `ChannelManager` entirely. While work continues towards that goal, having long-broken functionality in `ChannelManager` is also not acceptable, so here we simply remove the "without broadcasting" force-closure version. Fixes lightningdevkit#2875
While most of our closure logic mostly lives in `locked_close_chan`, some important steps happen in `convert_channel_err` and `handle_error` (mostly broadcasting a channel closure `ChannelUpdate` and sending a relevant error message to our peer). While its fine to use `locked_close_chan` directly when we manually write out the relevant extra steps, its nice to avoid it to DRY up some of our closure logic, which we do here.
`Channel`'s new `FundingScope` exists to store both the channel's live funding information as well as any in-flight splices. In order to keep the patches which introduced and began using it simple, it was exposed outside of `channel.rs` to `channelmanager.rs`, breaking the `Channel` abstraction somewhat. Here we remove one case of `FundingScope` being passed around `channelmanager.rs` (in the hopes of eventually keeping it entirely contained within `channel.rs`). Specifically, we remove the `FundingScope` parameter from `locked_close_channel`.
In a previous commit, we removed the ability for users to pick whether we will broadcast a commitment transaction on channel closure. However, that doesn't mean that there is no value in never broadcasting commitment transactions on channel closure. Rather, we use it to avoid broadcasting transactions which we know cannot confirm if the channel's funding transaction was not broadcasted. Here we make this relationship more formal by splitting the force-closure handling logic in `Channel` into the existing `ChannelContext::force_shutdown` as well as a new `ChannelContext::abandon_unfunded_chan`. `ChannelContext::force_shutdown` is the only public method, but it delegates to `abandon_unfunded_chan` based on the channel's state. This has the nice side effect of avoiding commitment transaction broadcasting when a batch open fails to get past the funding stage.
TheBlueMatt
force-pushed
the
2025-06-fc-always-broadcast
branch
from
e73434f to
544175b
Compare
TheBlueMatt
commented
Jun 28, 2025
There was a problem hiding this comment.
Mind putting that PR up as draft already? Would help me gauge whether I should hold off on simple close related refactors until these two PRs are in.
Yea, I wanted to do a few further things, but I just don't think I have time. I put up what I have at #3899.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3881 +/- ##
==========================================
+ Coverage 88.86% 88.89% +0.03%
==========================================
Files 165 165
Lines 118886 118921 +35
Branches 118886 118921 +35
==========================================
+ Hits 105650 105718 +68
+ Misses 10911 10878 -33
Partials 2325 2325 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tnull
reviewed
Jul 1, 2025
There was a problem hiding this comment.
This seems unaddressed so far?
| @@ -3160,6 +3162,11 @@ macro_rules! handle_error { | |||
| /// | |||
| /// Note that this step can be skipped if the channel was never opened (through the creation of a | |||
| /// [`ChannelMonitor`]/channel funding transaction) to begin with. | |||
| /// | |||
| /// For non-coop-close cases, you should generally prefer to call `convert_channel_err` and | |||
There was a problem hiding this comment.
Should we maybe also update the docs on finish_close_channel to mention the convert alternative?
| } | ||
| } | ||
|
|
||
| /// Shuts down this channel (no more calls into this Channel may be made afterwards except |
| } | ||
|
|
||
| /// Shuts down this channel (no more calls into this Channel may be made afterwards except | ||
| /// those explicitly stated to be alowed after shutdown, eg some simple getters). |
There was a problem hiding this comment.
nit: Decide on 'e.g.'/'i.e.' vs. 'eg.'/'ie.'.
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Still have a few commits on top here but this ended up getting quite big so I'll do them in a followup. There's a pile of things to DRY things up and make small wins here or there, but the main two commits are:
and