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

BOLT#02: change SHOULD to MUST for fee-range mismatch #1039

Closed
wants to merge 1 commit into from

Conversation

Crypt-iQ
Copy link
Contributor

@Crypt-iQ Crypt-iQ commented Nov 7, 2022

Addresses most of #1013, but without some warning codes the funder won't know what to change. This patch requires that the sender of fee_range doesn't send another until receiving a response via closing_signed or warning.

@t-bast
Copy link
Collaborator

t-bast commented Nov 7, 2022

This looks like a hacky way of making the closing protocol compatible with Musig2 funding outputs (and failing to mention that this is the motivation for the change). I think we should instead work on creating a cleaner protocol for closing taproot channels. My personal preference is to make it an explicit turn-based protocol (instead of hijacking the unrelated warning message).

The new requirement doesn't even seem doable since there's no reestablish logic to know that when you sent a fee_range it was received by your peer, so you may end up with deadlocks?

@Crypt-iQ
Copy link
Contributor Author

Crypt-iQ commented Nov 7, 2022

This looks like a hacky way of making the closing protocol compatible with Musig2 funding outputs

this isn't the motivation for the change though - the current protocol as written fails in an edge case

The new requirement doesn't even seem doable since there's no reestablish logic

since things start over if the conn restarts, this doesn't seem necessary since you just start over

@t-bast
Copy link
Collaborator

t-bast commented Nov 7, 2022

since things start over if the conn restarts, this doesn't seem necessary since you just start over

Gotcha, that should be clarified, I read the requirement as something that must be applied throughout reconnects.

the current protocol as written fails in an edge case

That's because it was designed that way, not being a turn-based protocol and requiring node operator intervention to resolve feerate conflicts / lack of progress. I now think that this was a mistake and I should have made it a strict turn-based protocol...

I don't think it makes any sense to require a node to send a warning. Warnings are optional hints that nodes send to each other when they detect weird behavior from their peer, and I believe they should stay that way. Using them to transform this protocol into a strict turn-based protocol is a red herring to me. If we want to make it a turn-based protocol, we should be more explicit about it (and as I previously stated in the related discussions, add an explicit reject message). I'm curious to get feedback from the other implementations though?

@Crypt-iQ
Copy link
Contributor Author

Crypt-iQ commented Nov 7, 2022

an explicit reject message is better imo, this patch was motivated by this comment #1013 (comment)

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.

I'm also in favor of another message here but I'm not sure what kind of message in order to avoid making the protocol less verbose.

Also, encoding some data in the warning message could work, but I need to think more about this use case

@TheBlueMatt
Copy link
Collaborator

FWIW, I think a MUST makes sense, I don't think its worth bothering with a new message unless we need it for something else - it was mentioned on the spec call that for taproot we'll need to send new nonces when rejecting, which definitely implies we want a new message there. Given we need more fields in a rejection, I'm okay with a new message.

@t-bast
Copy link
Collaborator

t-bast commented Nov 17, 2022

Shall we change this PR to become the spec for a new reject_closing_signed message or should we close it and open a separate PR?

@Crypt-iQ
Copy link
Contributor Author

Can close this, the reject version of coop closing is in this PR #1016 -- I think that addresses what this PR is trying to fix

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