Clean up and split up is_pre_funded_state#4021
Clean up and split up is_pre_funded_state#4021TheBlueMatt merged 3 commits intolightningdevkit:mainfrom
is_pre_funded_state#4021Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4021 +/- ##
==========================================
- Coverage 88.78% 88.75% -0.03%
==========================================
Files 176 176
Lines 128139 128833 +694
Branches 128139 128833 +694
==========================================
+ Hits 113768 114348 +580
- Misses 11803 11894 +91
- Partials 2568 2591 +23
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:
|
wpaulino
left a comment
wpaulino
left a comment
There was a problem hiding this comment.
I think we could make UnfundedV2 channels be resumable as well, currently we don't keep them around after a disconnect
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
Discussed this offline yesterday and I believe we concluded this is probably the right way to go (keeping the |
Do you plan to address this here as well @TheBlueMatt? Feel free to squash |
|
Oops, sorry, I missed that comment. I don't really want to change the set of resumable channels in this PR, its already complicated enough as it is :/ |
TheBlueMatt
force-pushed
the
2025-08-pre-funded-state-checks
branch
from
9e89ac9 to
9a24ecb
Compare
|
Went ahead and squashed. |
TheBlueMatt
force-pushed
the
2025-08-pre-funded-state-checks
branch
from
9a24ecb to
ad3ff19
Compare
|
Grr, also had to rebase. |
`Channel::is_pre_funded_state` is used to mean several different things. In this case, its used to skip all the `shutdown` logic as the funding transaction can't possibly have been broadcasted so there's really no ned to try to sign a transaction spending it. Here, we really want to capture any channel in `NegotiatingFunding` or any V1 channel in `FundingNegotiated` or, finally, any V2 channel in `FundingNegotiated` where we haven't yet sent our signatures (which is not captured in `is_pre_funded_state`). Instead of a new helper, we just check the states directly in `shutdown` handling.
`Channel::is_pre_funded_state` is used to mean several different things. In this case its used to decide if we should provide a `ChannelMonitorUpdate` marking a channel as closed when we go to force-close it. Here, we want to capture exactly when the original `ChannelMonitor` is first created, but were doing so indirectly by looking at the channel's state. Worse, `is_pre_funded_state` got updated to be false whenever there is an interctive signing session, which isn't correct for this use - we may have an interactive signing session but have already persisted the original `ChannelMonitor` when we received the first `commitment_signed`. Instead, we just move to examining `counterparty_next_commitment_transaction_number` which is decrementing for the first time at exactly the time we create the original `ChannelMonitor`, so it provides a much simpler test. Fixes lightningdevkit#3880
`Channel::is_pre_funded_state` is used to mean several different things. In the past few commits we stopped using it for a few conflicting uses, but here we break out the remaining uses and rename the remnants for clarity. `is_funding_broadcast` was using `is_pre_funded_state` and was then later used to decide if the `Channel` could be written to disk (because it can be resumed on restart), if we should broadcast a force-close transaction, and when to emit a `ChannelPending` event. These were also somewhat divergent - we shouldn't generate a `ChannelReady` event or broadcast a force-closing transaction until we've actually broadcasted but want to write the `Channel` to disk once we enter funding signature exchange for dual-funded open. Thus, the ability to write a `Channel` to disk is provided by a new `can_resume_on_restart` method. Then, `is_funding_broadcast` is updated to only consider funding broadcasted after we provide our funding signatures (i.e. the funding *could* have been broadcasted). This is still a bit early to generate a `ChannelPending` event (as the funding may not actually have been broadcasted yet), but its better than it was. Finally, the remaining `is_pre_funded_state` is renamed `can_resume_on_reconnect`, which has slightly different semantics than on-restart channels in batch opens.
TheBlueMatt
force-pushed
the
2025-08-pre-funded-state-checks
branch
from
ad3ff19 to
4cd52cb
Compare
|
Fixed commit messages and squash-pushed a method rename (which is only inaccurate for the manual-broadcast case, but it is in that case): $ git diff-tree -U1 ad3ff19d1 4cd52cb9c
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index d97c5a346f..db8a36b4ad 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -4003,3 +4003,3 @@ where
pub(crate) fn should_emit_channel_pending_event(&mut self) -> bool {
- self.is_funding_broadcast() && !self.channel_pending_event_emitted
+ self.is_funding_broadcastable() && !self.channel_pending_event_emitted
}
@@ -4099,3 +4099,3 @@ where
/// funding transaction has been broadcast if necessary.
- fn is_funding_broadcast(&self) -> bool {
+ fn is_funding_broadcastable(&self) -> bool {
match self.channel_state {
@@ -5451,3 +5451,3 @@ where
- let broadcast = self.is_funding_broadcast();
+ let broadcast = self.is_funding_broadcastable(); |
4 participants
This turns out to have been kinda buggy, and impossible to read the code, so clean it up a bunch.