Add HTLCsTimedOut closing reason #2887
Conversation
WalkthroughThe recent updates introduce a new closure reason, Changes
Assessment against linked issues
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- lightning/src/chain/channelmonitor.rs (3 hunks)
- lightning/src/events/mod.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (2 hunks)
- lightning/src/ln/functional_tests.rs (6 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/reorg_tests.rs (2 hunks)
Files not reviewed due to errors (1)
- lightning/src/ln/monitor_tests.rs (Error: unable to parse review)
Additional comments: 16
lightning/src/ln/reorg_tests.rs (2)
- 468-468: The closure reason has been updated from
HolderForceClosedtoHTLCsTimedOutin thetest_set_outpoints_partial_claimingfunction. This change aligns with the PR's objective to introduce a more accurate closure reason for channels force-closed due to HTLCs timing out.- 810-810: The closure reason has been updated from
HolderForceClosedtoHTLCsTimedOutin thedo_test_retries_own_commitment_broadcast_after_reorgfunction. This change is consistent with the PR's goal to provide a more precise reason for channel closures related to timed-out HTLCs.lightning/src/events/mod.rs (3)
- 235-236: The addition of the
HTLCsTimedOutvariant to theClosureReasonenum correctly implements the new closing reason for channels closed due to HTLCs timing out.- 260-260: Ensure the description for
HTLCsTimedOutin theDisplayimplementation forClosureReasonis clear and accurately reflects the scenario it represents.- 278-278: The serialization of the new
HTLCsTimedOutvariant in theimpl_writeable_tlv_based_enum_upgradable!macro forClosureReasonis correctly implemented.lightning/src/chain/channelmonitor.rs (3)
- 158-160: The addition of the
HTLCsTimedOutvariant to theMonitorEventenum is correctly implemented. Ensure that all instances whereHolderForceClosedwas used for HTLC timeouts are updated to useHTLCsTimedOut.- 193-193: The serialization mapping for
HTLCsTimedOutis correctly added with a unique identifier. Ensure that the removal ofHolderForceClosedin this context and the reassignment of identifiers do not conflict with backward compatibility or data migration needs.- 2719-2719: The push of
MonitorEvent::HTLCsTimedOuttopending_monitor_eventsis correctly implemented. Verify that this change aligns with the intended logic for handling timed-out HTLCs and that it triggers the appropriate downstream actions.lightning/src/ln/functional_tests.rs (6)
- 2420-2420: Ensure the updated closure reason
HTLCsTimedOutis consistently applied across all relevant test scenarios, reflecting the intended behavior accurately.- 2433-2433: Verify that the change to
HTLCsTimedOutaccurately represents the scenario being tested, particularly in the context of the channel's state and the events leading to closure.- 5685-5685: Confirm that the test scenario and the use of
HTLCsTimedOutas the closure reason are aligned with the expected outcomes when HTLCs time out.- 5716-5716: Check for consistency in the application of the
HTLCsTimedOutreason across similar test cases, ensuring it accurately reflects the test's intent.- 5762-5762: Assess whether the introduction of
HTLCsTimedOutin this scenario is appropriate, considering the specific conditions under which the channel is closed.- 8657-8657: Evaluate the correctness of using
HTLCsTimedOutin this context, especially given the specific block height and the state of the channel at the time of closure.lightning/src/ln/channelmanager.rs (2)
- 7318-7318: The pattern matching for
MonitorEvent::HolderForceClosedandMonitorEvent::HTLCsTimedOutis correctly implemented using a match arm with an or-pattern. This approach is concise and maintains readability.- 7336-7340: The logic to determine the
ClosureReasonbased on the type ofMonitorEventis correctly implemented using a match expression. This ensures that the closure reason accurately reflects the event that triggered the channel closure.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2887 +/- ##
==========================================
- Coverage 89.18% 89.16% -0.02%
==========================================
Files 117 117
Lines 95541 95574 +33
Branches 95541 95574 +33
==========================================
+ Hits 85205 85220 +15
- Misses 7840 7858 +18
Partials 2496 2496 ☔ View full report in Codecov by Sentry. |
| @@ -2712,7 +2716,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> { | |||
| self.best_block.height(), self.best_block.height() | |||
| ); | |||
| let mut claimable_outpoints = vec![commitment_package]; | |||
| self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0)); | |||
| self.pending_monitor_events.push(MonitorEvent::HTLCsTimedOut(self.funding_info.0)); | |||
There was a problem hiding this comment.
This method won't always be called when HTLCs time out, we need to provide the reason as an argument to it.
There was a problem hiding this comment.
Added, however, this didn't change any tests, a little suspect to me
benthecarman
force-pushed
the
htlc-timeout
branch
from
5f91790
to
cb51836
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- lightning/src/chain/channelmonitor.rs (10 hunks)
- lightning/src/events/mod.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (2 hunks)
- lightning/src/ln/functional_tests.rs (6 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/reorg_tests.rs (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- lightning/src/chain/channelmonitor.rs
- lightning/src/events/mod.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/reorg_tests.rs
benthecarman
force-pushed
the
htlc-timeout
branch
from
cb51836
to
80df48d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- lightning/src/chain/channelmonitor.rs (7 hunks)
- lightning/src/events/mod.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (2 hunks)
- lightning/src/ln/functional_tests.rs (6 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/reorg_tests.rs (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- lightning/src/chain/channelmonitor.rs
- lightning/src/events/mod.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/reorg_tests.rs
benthecarman
force-pushed
the
htlc-timeout
branch
from
80df48d
to
adab53c
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- lightning/src/chain/channelmonitor.rs (10 hunks)
- lightning/src/events/mod.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (3 hunks)
- lightning/src/ln/functional_tests.rs (6 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/reorg_tests.rs (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- lightning/src/events/mod.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/reorg_tests.rs
Additional comments: 9
lightning/src/chain/channelmonitor.rs (9)
- 53-53: The import statements and usage of various modules and traits appear to be correctly updated to support the new changes. However, ensure that all newly introduced dependencies are used efficiently and that there are no unnecessary imports after the changes.
- 158-168: The introduction of the
HolderForceClosedWithInfovariant in theMonitorEventenum is a significant change. This variant enhances the detail provided when a channel is closed, including the closure reason, outpoint, and channel ID. Ensure that all instances whereHolderForceClosedwas previously used are correctly updated to useHolderForceClosedWithInfowhere additional information about the closure is necessary or beneficial.Verification successful
The review comment is consistent with the findings in the codebase. The introduction of
HolderForceClosedWithInfoalongside the retention ofHolderForceClosedsuggests that the code has been updated to enhance detail where necessary while keeping the simpler variant for contexts where additional information is not required. This approach aligns with the recommendation to update instances ofHolderForceClosedtoHolderForceClosedWithInfowhere additional closure details are beneficial. Without further context indicating that the retained uses ofHolderForceClosedare inappropriate, the review comment is considered accurate.* 198-202: The serialization logic for `HolderForceClosedWithInfo` is correctly updated to include the new fields. However, ensure that the versioning and backward compatibility are maintained, especially when dealing with nodes that may not recognize the new variant.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for instances of `HolderForceClosed` to ensure they are correctly replaced or retained based on context. rg 'HolderForceClosed\('Length of output: 766
Verification successful
To verify the serialization logic for
HolderForceClosedWithInfoand its impact on backward compatibility, we need to inspect the specific serialization and deserialization code for this variant, including how versioning is handled. The providedrgoutput does not directly address these aspects. A more targeted search is required.
The serialization logic for
HolderForceClosedWithInfohas been correctly updated to include new fields (reason,outpoint,channel_id). Backward compatibility is maintained by writing bothHolderForceClosedandHolderForceClosedWithInfoevents, ensuring that older versions can still process theHolderForceClosedevents. This approach addresses the concerns raised in the review comment.* 1078-1078: The filtering logic in serialization to include `HolderForceClosedWithInfo` events is correctly updated. This ensures that the new event type is considered when counting and serializing pending monitor events. * 1088-1091: The comment about maintaining backward compatibility by writing a `HolderForceClosed` event along with the `HolderForceClosedWithInfo` event is crucial. This approach ensures that older nodes can still process events, albeit with less detail. It's important to verify that this deduplication does not introduce any unintended side effects in the event handling logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the serialization logic for older versions is not affected by the new changes. rg 'write_all' --context=5Length of output: 42357
Script:
#!/bin/bash # Locate the serialization and deserialization logic for HolderForceClosedWithInfo, focusing on versioning and backward compatibility. rg 'HolderForceClosedWithInfo' -A 30 -B 5Length of output: 19870
Verification successful
The review comment's concerns about maintaining backward compatibility by writing a
HolderForceClosedevent along with theHolderForceClosedWithInfoevent and ensuring this deduplication does not introduce unintended side effects in the event handling logic are addressed in the code. The implementation inchannelmonitor.rsand the handling inchannelmanager.rsdemonstrate a careful approach to backward compatibility and deduplication. The deduplication logic, which prefersHolderForceClosedWithInfooverHolderForceClosedwhen both are present, is designed to avoid side effects by retaining the most detailed event type. This approach ensures older nodes can process events with the available information without duplicating events, aligning with the comment's intent.* 2763-2773: The logic to generate a `HolderForceClosedWithInfo` event based on the `is_htlc_timeout` flag is a key part of the changes. This logic correctly captures the intent of providing more detailed closure reasons. Ensure that the `is_htlc_timeout` flag is accurately set in all relevant contexts to reflect the actual reason for channel closure. * 2810-2810: The call to `generate_claimable_outpoints_and_watch_outputs` with a hardcoded `false` value for the `is_htlc_timeout` parameter in this context seems appropriate, assuming this is a scenario where HTLC timeout is not the cause of channel closure. However, ensure that the use of hardcoded boolean values is consistent with the intended logic across all calls to this function. * 3813-3813: The conditional call to `generate_claimable_outpoints_and_watch_outputs` with `true` indicates a scenario where HTLCs timing out is the reason for generating new outpoints and outputs. This is a direct application of the new logic introduced by the PR. Ensure that the condition `should_broadcast` accurately reflects scenarios where HTLCs have timed out.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the deduplication logic in the reader does not introduce any side effects. rg 'HolderForceClosedWithInfo' --context=10Length of output: 12934
Verification successful
To accurately verify the original comment, we need to examine the implementation of
generate_claimable_outpoints_and_watch_outputsand understand howshould_broadcastis determined within that context, especially in relation to HTLC timeouts. The provided script output does not directly linkshould_broadcastto HTLC timeout scenarios in the context of this method. Further investigation into the method's implementation and the conditions under whichshould_broadcastis set totrueis necessary.
The examination of the
generate_claimable_outpoints_and_watch_outputsmethod and its usage of theis_htlc_timeoutparameter suggests that the method is designed to handle different scenarios, including those where HTLCs are timing out. The decision to generate new outpoints and outputs based on HTLC timeouts is influenced by theshould_broadcastcondition, which is determined by theself.should_broadcast_holder_commitment_txn(logger)method. This setup aligns with the original comment's concern about accurately capturing scenarios where HTLCs have timed out.However, without explicit details on the implementation of
should_broadcast_holder_commitment_txnand its direct relation to HTLC timeouts, a full verification of the original comment cannot be conclusively made. The evidence suggests a logical connection between HTLC timeouts and the generation of new outpoints and outputs, but the specific mechanics of this relationship are not fully detailed in the provided output.* 4640-4649: The deduplication logic to remove `HolderForceClosed` events if `HolderForceClosedWithInfo` events are also present is a thoughtful addition for maintaining backward compatibility while transitioning to the new event type. Ensure that this logic does not inadvertently remove relevant events in scenarios where both event types are legitimately present for different reasons.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `should_broadcast` condition accurately captures scenarios of HTLC timeouts. rg 'should_broadcast' --context=10Length of output: 27548
Script:
#!/bin/bash # Examine the implementation of generate_claimable_outpoints_and_watch_outputs and its usage of should_broadcast rg "generate_claimable_outpoints_and_watch_outputs" --context=10Length of output: 5334
Verification successful
The deduplication logic in question is specifically designed for handling backward compatibility during the deserialization process of
ChannelMonitorinstances. It ensures that redundantHolderForceClosedevents are removed when bothHolderForceClosedandHolderForceClosedWithInfoevents are present due to historical data. This logic does not operate during runtime event processing but rather during the loading of data, addressing concerns about inadvertently removing relevant events in complex scenarios. Based on the provided context and the analysis of howpending_monitor_eventsis used across the codebase, the implementation appears to correctly handle the deduplication of events for backward compatibility purposes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the deduplication logic does not remove relevant events in complex scenarios. rg 'pending_monitor_events' --context=10Length of output: 65314
benthecarman
force-pushed
the
htlc-timeout
branch
from
adab53c
to
df6ed25
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- lightning/src/chain/channelmonitor.rs (10 hunks)
- lightning/src/events/mod.rs (4 hunks)
- lightning/src/ln/channelmanager.rs (3 hunks)
- lightning/src/ln/functional_tests.rs (6 hunks)
- lightning/src/ln/monitor_tests.rs (1 hunks)
- lightning/src/ln/reorg_tests.rs (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- lightning/src/chain/channelmonitor.rs
- lightning/src/events/mod.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/ln/functional_tests.rs
- lightning/src/ln/monitor_tests.rs
- lightning/src/ln/reorg_tests.rs
|
Any update on this @benthecarman? Would be happy to have someone working ft on LDK take this over if you don't have the time. |
|
Sorry have been traveling, back in action this week though, will get to it |
|
Thanks! |
benthecarman
force-pushed
the
htlc-timeout
branch
from
df6ed25
to
d8c63a5
Compare
| @@ -155,6 +155,17 @@ pub enum MonitorEvent { | |||
| /// A monitor event containing an HTLCUpdate. | |||
| HTLCEvent(HTLCUpdate), | |||
|
|
|||
| /// Indicates we broadcasted the channel's latest commitment transaction and thus closed the | |||
| /// channel. Holds information about the channel and why it was closed. | |||
| HolderForceClosedWithInfo { | |||
There was a problem hiding this comment.
Should we name this HolderForceClosed and rename the existing variant to HolderForceClosedWithoutInfo instead? I'm somewhat indifferent on doing this now but seems like the one used going forward shouldn't have a suffix.
There was a problem hiding this comment.
imo it makes more sense like this and it can be renamed when HolderForceClosed is removed
| let reason = if let MonitorEvent::HolderForceClosedWithInfo { reason, .. } = monitor_event { | ||
| reason | ||
| } else { | ||
| ClosureReason::HolderForceClosed | ||
| }; |
There was a problem hiding this comment.
Could you expand upon this change in the commit message? Seems the previous reason used was incorrect?
benthecarman
force-pushed
the
htlc-timeout
branch
from
d8c63a5
to
1e8637b
Compare
Before a force closure from timed out HTLCs was treated the same as when the user manually force closed the channel. This leads to various UX issues. This adds a new `ClosureReason` called `HTLCsTimedOut` that signifies that the closure was caused because the HTLCs timed out. To go along with this, previously we'd always send "Channel force-closed" when force closing the channel in the error message which was ambigous, now we send the force closure reason so the peer can know why the channel was closed.
benthecarman
force-pushed
the
htlc-timeout
branch
from
1e8637b
to
9b5ebc4
Compare
Closes #2283
Before a force closure from timed out HTLCs was treated the same as when
the user manually force closed the channel. This leads to various UX
issues. This adds a new
ClosureReasoncalledHTLCsTimedOutthatsignifies that the closure was caused because the HTLCs timed out. To go
along with this, previously we'd always send "Channel force-closed" when
force closing the channel in the error message which was ambigous, now
we send the force closure reason so the peer can know why the channel
was closed.