Skip to content

Account for dust in closing channel fee #2344

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

Closed

Conversation

benthecarman
Copy link
Contributor

Before we were ignoring dust when creating the closing channel fee. Now if an output would be dust, we will recalculate the fee with the missing output. This should fix some force closes that happen with lnd nodes.

Draft for now because no tests, couldn't find if this function was tested already

Before we were ignoring dust when creating the closing channel fee. Now
if an output would be dust, we will recalculate the fee with the missing
output. This should fix some force closes that happen with lnd nodes.
@benthecarman benthecarman force-pushed the calc-fees-correctly branch from 877eca8 to a2741a0 Compare June 7, 2023 21:12
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (f068df0) 90.37% compared to head (a2741a0) 90.38%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2344      +/-   ##
==========================================
+ Coverage   90.37%   90.38%   +0.01%     
==========================================
  Files         104      104              
  Lines       53779    53787       +8     
  Branches    53779    53787       +8     
==========================================
+ Hits        48601    48618      +17     
+ Misses       5178     5169       -9     
Impacted Files Coverage Δ
lightning/src/ln/channel.rs 89.71% <100.00%> (+0.02%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TheBlueMatt
Copy link
Collaborator

While its probably true that we need to update the code here as well, I don't think we handle the "remove my output cause its small" case in closing_signed either - we build a closing_tx based on the removal of the counterparty's output, but we don't adjust the fee range accordingly - we should be willing to pay a much higher fee if they removed their output, cause its not our money.

@TheBlueMatt
Copy link
Collaborator

Let's tackle this as a part of #2433 (which removes all shutdown FCs). I'm kinda hoping we can do that, get LND to do it too, and then never have to think about the old fee negotiation code again (and hopefully rip it out entirely in a year or two, just force-close always if the peer doesn't support the sensible approach). If you want to pick this back up just ping and I'm happy to reopen it, but (a) I'm not super convinced this is a common force-closure reason - more commonly both sides want their balance but one side just sends a nonsense fee, usually because of a Bitcoin Core crash or restart and (b) I hate this code and all the close negotiation code and am excited to never look at it again so I'm gonna tentatively close this.

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.

3 participants