lnwallet: apply dust amounts to fees #214
Conversation
|
@bryanvu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Roasbeef, @AndrewSamokhvalov and @cjamthagen to be potential reviewers. |
|
Coverage increased (+0.006%) to 72.982% when pulling db7e585657aa78deb286622edbb77d845e522333 on bryanvu:dustfees into 74897dc on lightningnetwork:master. |
|
Suggestion regarding how to deal with cases where HTLC’s value is below a node’s dust limit: rather than the entire amount for that HTLC being applied to the fee used for the channel’s commitment Further concepts relating to this suggestion are here and also here. Edit: As typical safety circumstance, if for some reason the channel is force closed during payment, and dust htlc/payment aren't settled after aggregation, then it is not included in the commitment transaction (thus, it would be given as a fee to miners). |
There was a problem hiding this comment.
Suggested changes at #214 (comment) (Fee - transaction - donation)
|
@ABISprotocol that can't be done as is. Doing so would require a new transaction to be create when closing a transaction forcibly which would be a protocol modification. While your idea may be interesting when applied to a user facing wallet with a GUI, it isn't very applicable to this situation. |
There was a problem hiding this comment.
Nice work breaking up the dust-limit related unit tests to focus on a particular scenario at a time! As we discussed the original test sprawled a bit and was rather fragile.
My comments are pretty minor so, if after this round of review and a bit of local testing I don't find any issues, then we should be able to get this merged pretty quickly.
lnwallet/channel.go
Outdated
| @@ -1309,12 +1310,14 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool, | |||
| numHTLCs := 0 | |||
| for _, htlc := range filteredHTLCView.ourUpdates { | |||
| if htlc.Amount < dustLimit { | |||
| dustFees = dustFees + htlc.Amount | |||
There was a problem hiding this comment.
Can be simplified to dustFees += htlc.Amount.
lnwallet/channel.go
Outdated
| continue | ||
| } | ||
| numHTLCs++ | ||
| } | ||
| for _, htlc := range filteredHTLCView.theirUpdates { | ||
| if htlc.Amount < dustLimit { | ||
| dustFees = dustFees + htlc.Amount |
There was a problem hiding this comment.
Can be simplified to dustFees += htlc.Amount.
lnwallet/channel.go
Outdated
| @@ -1301,6 +1301,7 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool, | |||
| // * when dumping fees back into initiator output, only dumb explicit | |||
| // fee | |||
| var dustLimit btcutil.Amount | |||
| var dustFees btcutil.Amount | |||
There was a problem hiding this comment.
Minor style nit: within the codebase whenever we have two or more variable declarations following each other we collapse them into a single var declaration like so:
var (
dustLimit btcutil.Amount
dustFees
)Alternatively, since they're of the same type, we can do something like this:
var dustLimit, dustFees btcutil.Amount
lnwallet/channel.go
Outdated
| @@ -1324,8 +1327,8 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool, | |||
| // on its total weight. Once we have the total weight, we'll multiply | |||
| // by the current fee-per-kw, then divide by 1000 to get the proper | |||
| // fee. | |||
| totalCommitWeight := commitWeight + btcutil.Amount(htlcWeight*numHTLCs) | |||
| commitFee := (lc.channelState.FeePerKw * totalCommitWeight) / 1000 | |||
| weight := commitWeight + btcutil.Amount(htlcWeight*numHTLCs) | |||
There was a problem hiding this comment.
Hmm, downgraded to a less descriptive variable name? Was this only to ensure that the line below (with the added operation) wouldn't overflow 80 characters? If so, then either the line should be wrapped, or the - dustFees can mutate the commitFee variable on a new line. In either case, we end up with an additional line.
If an HTLC’s value is below a node’s dust limit, the amount for that HTLC should be applied to to the fee used for the channel’s commitment transaction.
Split up and simplified dust limit tests.
|
Just pushed with the requested changes. |
If an HTLC’s value is below a node’s dust limit, the amount for that
HTLC should be applied to to the fee used for the channel’s commitment
transaction. This PR also adds minor refactoring of the dust limit unit tests.