Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update closing_signed fee requirement #847
Update closing_signed fee requirement #847
Changes from 2 commits
69a11c2
f029164
8683525
c990020
034486c
aaae6bc
92d2af0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be clearer if the non-funder was explicitly mentioned here instead of "it" (ambiguous context imo).
Also it isn't guaranteed that things complete in three messages if the funder chooses to pick a different value, right (responder wants a fee too low, maybe below the current mempool high water mark)? Since the non-funder may not like that advertised fee.
Instead, seems this is just best effort, and if the responder doesn't immediately reply with the same value, then things fall back to the "old" method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things never fall back to the "old" method? The funder can start over, so, yes, the text should be changed, but there's no fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Roasbeef the case you describe is a spec violation. The non-funder either chooses a fee inside the advertised
fee_range
, or doesn't answer (sending a warning once that has been added to the spec) potentially leading to a force-close instead if no progress is made.We only fall back to the old negotiation when the response doesn't contain a
fee_range
(indicating that the receiver didn't support this new negotiation scheme).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flow from the node operator's point of view in that case is:
fee_range
that the node operator thinks makes sensefee_range
, do nothing or force-closeIn the future we may want to automate that retry, but based on our users' feedback, they would really prefer having a human in the loop to decide whether they should adapt their
fee_range
or not. Many have been disgruntled by the feerates chosen by estimators, so they're not interested in an automated retry here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our commitment transaction, or our counterparty's commitment transaction (I know this isn't new on this PR, I just realized it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, it's more subtle than it seems. We use our commitment transaction in eclair, and I think it's what makes most sense from the sender's point of view: we must create a transaction that's cheaper than the alternative we can unilaterally broadcast (even though you'd also need to factor in csv delays into the choice of which one to broadcast).
However, the receiver would probably want to receive a proposal that also pays less fees than his commit tx, so would it make more sense to use the min of both commit tx fees here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the recipient MUST fail/force-close if we're too high, min seems like the obvious choice. Are there any edge-cases where one party has a high dust limit, causing one side of the commitment transaction to be missing an output (skewing the fee pretty low) whereas both outputs would appear in the cooperative shutdown case? In that world you risk always having to force-close (but maybe that's what you want - at least if the side that gets to skip the counterparty's output is the initiator who pays fees).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this edge case can meaningfully happen thanks to the channel reserve (which must be greater than the
dust_limit
), unless the channel has just been opened and the reserve requirement was never met.I'm happy to change this to specify that it should be the min of both commitments (even though this requirement will disappear anyway since it doesn't apply to anchor outputs channels).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, should we drop this requirement for the channel funder? I don't see a reason why the node not paying the fee should care one bit if the channel opener wants to close with a higher feerate than the existing channel one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I did in the first version of the PR, I had dropped this requirement completely, but in order to keep existing channel behavior I was asked to drop it only for anchor outputs. I'm happy to remove it for all channel types honestly, probably worth discussing again during the next spec meeting to see if we can convince @rustyrussell ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, its a compatibility issue with existing nodes, so not ideal, but its not a super critical one imo - a few extra force-closes instead of cooperative closes while nodes upgrade isn't the end of the world. If it resulted in force-closes of otherwise-functional channels it'd be much worse, but we're talking about closing the channel anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to gravedig, but what is meant by overlap here? Is it the union of the two ranges or the intersection? If it's the intersection of the two ranges, then the statement a bit above
if fee_satoshis matches its previously sent fee_range:
could make this "overlap" statement redundant by changing the SHOULD to a MUST?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, overlap means intersection. We can make the
SHOULD
aMUST
, that would make sense.