Uh oh!
There was an error while loading. Please reload this page.
Additional closing cleanups post-#3881 - #3899
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
Uh oh!
There was an error while loading. Please reload this page.
0dac107 to
77fabefCompareCodecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #3899 +/- ##
==========================================
- Coverage 89.01% 89.00% -0.02%
==========================================
Files 168 168 Lines 121784 121799 +15 Branches 121784 121799 +15 ==========================================
- Hits 108411 108407 -4 - Misses 10966 10983 +17 - Partials 2407 2409 +2
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:
|
tnull
left a comment
There was a problem hiding this comment.
Will take a closer look soon, but, FYI, CI is unhappy.
TheBlueMatt
commented
Jun 30, 2025
Yea, realized that but will probably fix after #3881 |
853bdfc to
a099c02CompareTheBlueMatt
commented
Jul 14, 2025
Squashed and fixed rustfmt. |
a099c02 to
afc42abComparetnull
left a comment
There was a problem hiding this comment.
Changes by themselves LGTM.
Once this is land I'll need to see how to proceed with the simple-close flow.
ldk-reviews-bot
commented
Jul 17, 2025
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
afc42ab to
59bf265CompareTheBlueMatt
commented
Jul 17, 2025
Rebased. |
Uh oh!
There was an error while loading. Please reload this page.
| // watch for old state broadcasts)! | ||
| locked_close_channel!(self, peer_state, chan, close_res, FUNDED); | ||
| (Some(tx), Some(chan_entry.remove()), Some(close_res)) | ||
| let (_, err) = convert_channel_err!(self, peer_state, close_res, chan, &msg.channel_id, COOP_CLOSED); |
There was a problem hiding this comment.
I find it odd that coop close gets classified as an error when it's something both nodes agreed upon, but I guess it's too late to really do anything about it
There was a problem hiding this comment.
I mean I can add a new, wrapping, macro to give it a new name, or rename convert_channel_err?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
2d5a81d to
731657bComparetnull
left a comment
There was a problem hiding this comment.
Fixups LGTM, feel free to squash from my side.
ldk-reviews-bot
commented
Jul 22, 2025
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
731657b to
fc69f66CompareTheBlueMatt
commented
Jul 22, 2025
Squashed without further changes. |
wpaulino
left a comment
There was a problem hiding this comment.
Would still like to see the closing logs cleaned up a bit more, but it doesn't have to block this
Our coop close methods `Channel::maybe_propose_closing_signed` and `Channel::closing_signed` may return a `Transaction` to broadcast as well as a `ShutdownResult` to provide the post-shutdown handling fields. However, it only does either both of them or neither - we only and always broadcast when we're done closing. Here we tweak the return values to match the possible states, combining the two fields in the return value into a single `Option`.
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). In a previous commit we removed most instances of `locked_close_channel` in our closure pipeline, however it was still called in cases where we did a cooperative close. Here we remove the remaining direct references to it, always using `convert_channel_err` instead (now with a new `COOP_CLOSED` macro variant).
fc69f66 to
f11e798CompareTheBlueMatt
commented
Jul 22, 2025
Rebased without any further changes. |
The `channel_id` argument to `convert_channel_err` allows us to set a different channel ID in the error message around the funding process. However, its not exactly critical to make sure we get it right, and yet another macro argument is annoying to deal with, so here we simply drop it and use the `Channel` value. This means that when force-closing we sometimes send an `error` with the `temporary_channel_id` rather than the funded `channel_id`, but its not that critical to get right (and we don't in all cases anyway), the peer will eventually figure it out when we reconnect or they try to send more messages about the channel.
Now that we never call `locked_close_channel` directly, the documentation on it should be updated to note that it shouldn't be used, rather relying on `convert_channel_err`, which now gets much of the substantive documentation on closing logic. Similarly, as `finish_close_channel` is now only called from `handle_error` we update the docs on both to note the new semantics.
f11e798 to
0b3ba85CompareTheBlueMatt
commented
Jul 22, 2025
Grrrr, damn cfg flags $ git diff-tree -U2 f11e79809 0b3ba8519
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 590f57e108..acc549fcb5 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -13004,5 +13004,5 @@ where
let mut pending_events = self.pending_events.lock().unwrap();
pending_events.push_back((events::Event::ChannelReady {
- channel_id: *channel_id,+ channel_id,
user_channel_id: funded_channel.context.get_user_id(),
counterparty_node_id, |
Uh oh!
There was an error while loading. Please reload this page.
I had really hoped to spend some time moving some of the monitor completion and closure logic out of macros (as I fear they are a nontrivial portion of our compile time), but sadly I just really don't have the time. So instead here's the few additional small things I wanted to get out of the way.