Open JssDWt opened 2 months ago
[!IMPORTANT]
Auto Review Skipped
Auto reviews are disabled on this repository.
Please check the settings in the CodeRabbit UI or the
.coderabbit.yaml
file in this repository. To trigger a single review, invoke the@coderabbitai review
command.You can disable this status message by setting the
reviews.review_status
tofalse
in the CodeRabbit configuration file.
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
It's probably better to have an agreed value on dust_limit_satoshis
during the channel opening - the funder proposes dust_limit_satoshis
via open_channel
msg, and the fundee should set it to the same value in accept_channel
msg. Otherwise we'd always end up using max(local_dust_limit, remote_dust_limit)
since we'd also need to make sure ourBalance >= remoteDust
?
Otherwise we'd always end up using
max(local_dust_limit, remote_dust_limit)
since we'd also need to make sureourBalance >= remoteDust
?
That's a decision you can make. To either fail the closing procedure, because you have in invalid signature according to the remote party, or to omit your own output. Probably a good reasoning to decide whether you should omit your own output is:
For now, this PR only ensures you never create a transaction below your own dust limit in the first place. Deciding to omit your own output can be future improvements.
The reason we haven't done this is because iirc each implementation does something slightly different. I'd defer work on this PR until we can all agree on the logic here. Additionally, changing this now will result in incompatibility with older LND nodes.
Additionally, changing this now will result in incompatibility with older LND nodes.
I agree there will be an incompatibility with older LND nodes. In fact, there will be an incompatibility between upgraded LND nodes too. If one party omits the peers output, but the peer doesn't, there's an incompatibility. But it also doesn't ever make sense to construct a closing transaction with an output below your own dustlimit. So the incompatibility already exists. This change just makes it more obvious.
FWIW rbf-coop close will make this explicit, as the ender of a signature states which outputs is does/doesn't cover: #8453
@jssdwt, remember to re-request review from reviewers when ready
Change Description
When the remote dustlimit is lower than the local one, a cooperative closure could create an output that doesn't respect the local dustlimit. By comparing the remote balance to the local dustlimit we're protected against creating invalid cooperative close transactions according to the local standards.
This requirement is part of the BOLTs: https://github.com/lightning/bolts/blob/5f8fea8dc3c8c612167dd9645c4a21fe9de2f147/03-transactions.md?plain=1#L373
Unfortunately this now leads to failure to close the channel. But that's better than crafting invalid transactions in my opinion.
Next steps will be
Steps to Test
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.