Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove requirements to disconnect on warnings #1075

Merged
merged 1 commit into from
May 9, 2023

Conversation

t-bast
Copy link
Collaborator

@t-bast t-bast commented May 5, 2023

We generally shouldn't disconnect when sending or receiving warning messages. Whenever disconnecting after a warning makes sense, it should be specified in the requirements linked to that specific scenario.

Fixes #1072

We generally shouldn't disconnect when sending or receiving warning
messages. Whenever disconnecting after a warning makes sense, it should
be specified in the requirements linked to that specific scenario.

Fixes lightning#1072
@t-bast t-bast requested a review from TheBlueMatt May 5, 2023 07:39
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 3747ba8

I super agree, thanks to do this!

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.

LGTM, yea, we already added a ton of "and close the connection" comments when we moved to warnings, so these are just weird.

Copy link
Collaborator

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack 3747ba8

@t-bast
Copy link
Collaborator Author

t-bast commented May 8, 2023

@Roasbeef can someone from LL ACK this and check lnd's behavior, which seems to disconnect when receiving a warning message?

@ellemouton
Copy link
Contributor

ellemouton commented May 9, 2023

As far as I can tell, since 0.16.0-beta, LND no longer disconnects when a warning is received. This issue was tracked here.

Let me just double confirm this & will get back with an ACK. But defs a concept ACK from me.

Copy link
Contributor

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

ACK :)

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🌟

Yep, we'll no longer disconnect (fixed a bug in this area in 0.16.0). We only disconnect on Error.

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🌟

Yep, we'll no longer disconnect (fixed a bug in this area in 0.16.0). We only disconnect on Error.

@TheBlueMatt TheBlueMatt merged commit 2bf041f into lightning:master May 9, 2023
@t-bast t-bast deleted the remove-disconnect-warning branch May 10, 2023 07:22
SomberNight added a commit to SomberNight/electrum that referenced this pull request May 10, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing the disconnect-on-warning
6 participants