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

Don't disconnect on manual force closes #3088

Merged

Conversation

alecchendev
Copy link
Contributor

@alecchendev alecchendev commented Jun 3, 2024

Previously we'd always disconnect on force close because the peer had done something bad, but on manual force closes that isn't always the case, e.g. when rejecting a new channel request. This just sends an error message by default instead.

///
/// By default, this will send a generic force close error message to the peer if `err_msg`
/// is not specified. If `disconnect_peer` is set to true, the peer will also be disconnected.
pub fn force_close_broadcasting_latest_txn(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh, oops, so #2889 already adds the user-provided error message and should be close to merge, you might want to just rebase on that.

Also, I'm not entirely convinced we want to add the disconnection bool - its not clear to me in what case(s) a user would prefer to disconnect a peer vs just sending an error message and calling it a day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, oops, so #2889 already adds the user-provided error message and should be close to merge, you might want to just rebase on that.

Oh nice ok

Also, I'm not entirely convinced we want to add the disconnection bool - its not clear to me in what case(s) a user would prefer to disconnect a peer vs just sending an error message and calling it a day.

SGTM, i can just make it so we don't disconnect by default then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking we'd just make it so we never disconnect :)

@alecchendev alecchendev force-pushed the 2024-06-fc-no-disconnect branch from 97c13b0 to eb84e4c Compare June 3, 2024 22:04
@alecchendev alecchendev changed the title Parameterize disconnection and error message on manual force close Don't disconnect on manual force closes Jun 3, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.88%. Comparing base (993cd1e) to head (eb84e4c).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3088      +/-   ##
==========================================
+ Coverage   89.86%   89.88%   +0.01%     
==========================================
  Files         117      117              
  Lines       97458    97458              
  Branches    97458    97458              
==========================================
+ Hits        87581    87599      +18     
+ Misses       7303     7288      -15     
+ Partials     2574     2571       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Thanks!

Copy link
Contributor

@arik-so arik-so left a comment

Choose a reason for hiding this comment

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

Would it be useful to pass a force-close boolean to force_close_sending_error, or do we figure the user can just manually disconnect the peer in the next method call?

@TheBlueMatt
Copy link
Collaborator

I'm not clear on any cases where the user would actually care about disconnecting the peer, and they can certainly do so manually if they want.

@TheBlueMatt TheBlueMatt merged commit 96fe185 into lightningdevkit:main Jun 4, 2024
15 of 16 checks passed
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.

None yet

4 participants