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

Use warning message in quick close #904

Merged
merged 1 commit into from
Feb 15, 2022
Merged

Use warning message in quick close #904

merged 1 commit into from
Feb 15, 2022

Conversation

t-bast
Copy link
Collaborator

@t-bast t-bast commented Aug 31, 2021

Once #834 have been accepted to the spec, we should use a warning in quick close instead of failing the connection.

@Roasbeef
Copy link
Collaborator

Roasbeef commented Sep 1, 2021

If a warning message is added (again without a feature bit??), and you send this to a peer that maybe understands the new co-op close TLV field (since there's actually no feature bit, so you just need to guess), then how do you know they actually even support this feature?

This is the whole reason we added dependencies to feature bits: we can make this relationship explicit, namely that the new co-op close TLV depends on understanding of the warning message (as it's used to signal an error case).

@t-bast
Copy link
Collaborator Author

t-bast commented Sep 1, 2021

you send this to a peer that maybe understands the new co-op close TLV field

That condition is nested below - if the message contains a fee_range: so your peer does understand this TLV field.

Again, your argument is true in general but in this specific case it's completely unnecessary.

@TheBlueMatt
Copy link
Collaborator

Additional context here may be useful: the warning message is only used to indicate "hey, we couldn't come to agreement!", but that's also implied by the fact that, well, you didn't see a useful response. No matter if the peer responds or not, you have to eventually time-out the negotiation and force-close (or let the user do that, which they'll presumably do if we take forever to negotiate fees), which is the same as today. The warning message is just a helpful bit of context for users who read their debug logs, but the response to the condition that is warned on should really be the same either way.

@t-bast t-bast marked this pull request as ready for review January 4, 2022 10:22
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 db636f0

@rustyrussell
Copy link
Collaborator

Ack, easy!

@t-bast t-bast merged commit 5fa6ff3 into master Feb 15, 2022
@t-bast t-bast deleted the closing-fee-warning branch February 15, 2022 07:47
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.

5 participants