-
Notifications
You must be signed in to change notification settings - Fork 470
Release held LSPS2 HTLCs when abandoning JIT opens #4703
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1316,6 +1316,8 @@ where | |
| /// This removes the intercept SCID, any outbound channel state, and associated | ||
| /// channel‐ID mappings for the specified `user_channel_id`, but only while no payment | ||
| /// has been forwarded yet and no channel has been opened on-chain. | ||
| /// Any held HTLCs for the pending flow are failed backwards before the local state | ||
| /// is removed. | ||
| /// | ||
| /// Returns an error if: | ||
| /// - there is no channel matching `user_channel_id`, or | ||
|
|
@@ -1351,25 +1353,27 @@ where | |
|
|
||
| let jit_channel = peer_state | ||
| .outbound_channels_by_intercept_scid | ||
| .get(&intercept_scid) | ||
| .get_mut(&intercept_scid) | ||
| .ok_or_else(|| APIError::APIMisuseError { | ||
| err: format!( | ||
| "Failed to map intercept_scid {} for user_channel_id {} to a channel.", | ||
| intercept_scid, user_channel_id, | ||
| ), | ||
| })?; | ||
| err: format!( | ||
| "Failed to map intercept_scid {} for user_channel_id {} to a channel.", | ||
| intercept_scid, user_channel_id, | ||
| ), | ||
| })?; | ||
|
|
||
| let is_pending = matches!( | ||
| jit_channel.state, | ||
| OutboundJITChannelState::PendingInitialPayment { .. } | ||
| | OutboundJITChannelState::PendingChannelOpen { .. } | ||
| ); | ||
| let intercepted_htlcs = match &mut jit_channel.state { | ||
| OutboundJITChannelState::PendingInitialPayment { payment_queue } | ||
| | OutboundJITChannelState::PendingChannelOpen { payment_queue, .. } => payment_queue.clear(), | ||
| _ => { | ||
| return Err(APIError::APIMisuseError { | ||
| err: "Cannot abandon channel open after channel creation or payment forwarding" | ||
| .to_string(), | ||
| }); | ||
| }, | ||
| }; | ||
|
|
||
| if !is_pending { | ||
| return Err(APIError::APIMisuseError { | ||
| err: "Cannot abandon channel open after channel creation or payment forwarding" | ||
| .to_string(), | ||
| }); | ||
| for htlc in intercepted_htlcs { | ||
| let _ = self.channel_manager.get_cm().fail_intercepted_htlc(htlc.intercept_id); | ||
| } | ||
|
|
||
| peer_state.intercept_scid_by_user_channel_id.remove(&user_channel_id); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This cleanup removes the three per-peer maps ( Consequence: after abandon,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We decided to only do the minimal fix
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See below, maybe we should still leave 1-2 TODO comments?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Codex:
Given we decided to not fully fix all cases, should we at least leave 1-2 TODOs in the respective places so we don't forget to revisit them? |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need all this seemingly unrelated refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one do you mean exactly?
We need to get and drain the payment queue, and by putting the return in the _ arm, not another bool is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, originally this code had
matches andifs all over the place, so we spent some time before making it more readable. Moving things to dedicated bools was part of that effort of making the code more readable when read top to bottom. Any case, it's not too terrible, but IMO a step back.