Skip to content
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

Remove panics for sign_holder_commitment_and_htlcs when a signature i… #2608

Closed
wants to merge 3 commits into from

Conversation

rmalonson
Copy link
Contributor

…s not immediately available.

@wpaulino wpaulino self-requested a review September 27, 2023 17:32
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include the why/what/how in your commit descriptions, not just a simple title.

/// provided later when the signer is online.
NotAvailable,
/// The signer failed permanently and we should attempt to close the
/// channel.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure I understand the value of this. If we can't sign, we also can't close the channel, and in fact the current handling just panics. We could certainly have this here (like we do for I/O) just as a shortcut for the user to panic, but we should think hard about the value of that first.

If we do keep this error, it should move out of the first commit into a new first commit separately.

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved
@wpaulino
Copy link
Contributor

@TheBlueMatt FYI I'm taking over this PR.

@moneyball moneyball added this to the 0.0.119 milestone Oct 26, 2023
@wpaulino
Copy link
Contributor

Closing this for #2703 instead.

@wpaulino wpaulino closed this Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants