multi: include commitment fees in dust calculation #8824
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Crypt-iQ
force-pushed
the
precise_dust_limit
branch
4 times, most recently
from
0364eae to
8043864
Compare
Crypt-iQ
force-pushed
the
precise_dust_limit
branch
from
8043864 to
6266d9d
Compare
There was a problem hiding this comment.
My main feedback here is around terminology. I think we should avoid stretching the definition of "dust" since this conveys a certain understanding that spans more than just LND/Lightning and is an industry wide term.
I would recommend using terminology like "fee exposure" since that is both more accurate in what it is measuring and also accurate with respect to how we are letting the value influence our decisionmaking.
The implementation is fine but I'd like to see the optional parameter made required. I anticipate there may be hiccups here as it would require you to be able to supply the current state at the callsite. I don't think that computing that will be too onerous, but let me know if you think otherwise.
htlcswitch/interfaces.go
Outdated
| getDustSum(remote bool, | ||
| fee fn.Option[chainfee.SatPerKWeight]) lnwire.MilliSatoshi | ||
|
|
||
| // getFeeRate returns the current channel feerate. | ||
| getFeeRate() chainfee.SatPerKWeight | ||
|
|
||
| // getDustClosure returns a closure that can evaluate whether a passed | ||
| // HTLC is dust. | ||
| getDustClosure() dustClosure | ||
|
|
||
| // getCommitFee returns the commitment fee in satoshis from either the | ||
| // local or remote commitment. | ||
| getCommitFee(remote bool) btcutil.Amount |
There was a problem hiding this comment.
I think the main question I have at this point is what the formal distinction is between the commit fees and the dust. It seems like starting with this commit, you want to redefine dust to be the commitment fees themselves.
There was a problem hiding this comment.
the commit fees here is the CommitFee field on the commitment struct which doesn't include dust. can update the comment to explicitly state that
Crypt-iQ
force-pushed
the
precise_dust_limit
branch
from
6266d9d to
720f46f
Compare
htlcswitch/link.go
Outdated
| localDustSum += lnwire.NewMSatFromSatoshis(commitFee) | ||
| remoteDustSum += lnwire.NewMSatFromSatoshis(commitFee) | ||
|
|
||
| // Calculate the additional fee increase if this is a non-dust HTLC. | ||
| weight := lntypes.WeightUnit(input.HTLCWeight) | ||
| additional := lnwire.NewMSatFromSatoshis( | ||
| feeRate.FeeForWeight(weight), | ||
| ) | ||
|
|
||
| if isLocalDust { | ||
| // If this is dust, it doesn't contribute to weight but does | ||
| // contribute to the overall dust sum. | ||
| localDustSum += lnwire.NewMSatFromSatoshis(amount) | ||
| } else { | ||
| // Account for the fee increase that comes with an increase in | ||
| // weight. | ||
| localDustSum += additional | ||
| } | ||
|
|
||
| if localDustSum > l.cfg.MaxFeeExposure { | ||
| // The max fee exposure was exceeded. | ||
| return true | ||
| } | ||
|
|
||
| if isRemoteDust { | ||
| // If this is dust, it doesn't contribute to weight but does | ||
| // contribute to the overall dust sum. | ||
| remoteDustSum += lnwire.NewMSatFromSatoshis(amount) | ||
| } else { | ||
| // Account for the fee increase that comes with an increase in | ||
| // weight. | ||
| remoteDustSum += additional | ||
| } | ||
|
|
||
| if remoteDustSum > l.cfg.MaxFeeExposure { | ||
| // The max fee exposure was exceeded. | ||
| return true | ||
| } | ||
|
|
||
| return false |
There was a problem hiding this comment.
I'd recommend consolidating some of the branch execution logic in favor of something more expression oriented like this. Assigning variables once makes it easier for people to read the definition and not have to reason about operational semantics in order to understand what the code is doing.
// localDustSum += lnwire.NewMSatFromSatoshis(commitFee) --> Delete
// remoteDustSum += lnwire.NewMSatFromSatoshis(commitFee) --> Delete
// Calculate the additional fee increase if this is a non-dust HTLC.
weight := lntypes.WeightUnit(input.HTLCWeight)
additional := lnwire.NewMSatFromSatoshis(
feeRate.FeeForWeight(weight),
)
var addedLocalFees lnwire.MilliSatoshi
if isLocalDust {
// If this is dust, it doesn't contribute to weight but does
// contribute to the overall dust sum.
addedLocalFees = lnwire.NewMSatFromSatoshis(amount)
} else {
// Account for the fee increase that comes with an increase in
// weight.
addedLocalFees = additional
}
var addedRemoteFees lnwire.MilliSatoshi
if isRemoteDust {
// If this is dust, it doesn't contribute to weight but does
// contribute to the overall dust sum.
addedRemoteFees = lnwire.NewMSatFromSatoshis(amount)
} else {
// Account for the fee increase that comes with an increase in
// weight.
addedRemoteFees = additional
}
localFeeExposure := localDustSum + addedLocalFees + commitFee
remoteFeeExposure := remoteDustSum + addedRemoteFees + commitFee
return localFeeExposure > l.cfg.MaxFeeExposure ||
remoteFeeExposure > l.cfg.MaxFeeExposureThere was a problem hiding this comment.
don't see a problem with code as-is, it's pretty easy to understand what's going on imo
There was a problem hiding this comment.
There are two main things I'm trying to discourage:
+=and other forms of variable reassignment make it harder to reason locally about code. It also means that whatever the variable is named is wrong for some portion of the lifetime of that variable. Either it starts correct and is wrong later when reassigned, or it starts wrong and ends correct later when it is reassigned. When reassignment is present it is also not obvious which of these two scenarios we are in.- Early returns make it harder to grok the overall picture of what the expression is. In my offered solution we can see quite easily that if either the local or remote fee exposure blows the limit, then we are overexposed, and we can see the constituent components of the overall exposure.
This isn't a blocking comment, but I'm generally trying to reduce complexity and increase the comprehension ability of the codebase, and so I offered an alternative that solves both of these things.
|
@Crypt-iQ, remember to re-request review from reviewers when ready |
Crypt-iQ
force-pushed
the
precise_dust_limit
branch
from
720f46f to
50b9b1d
Compare
Crypt-iQ
force-pushed
the
precise_dust_limit
branch
from
50b9b1d to
cc4a703
Compare
Crypt-iQ
force-pushed
the
precise_dust_limit
branch
2 times, most recently
from
c9fd46d to
6279406
Compare
This commit expands the definition of the dust limit to take into account commitment fees as well as dust HTLCs. The dust limit is now known as a fee exposure threshold. Dust HTLCs are fees anyways so it makes sense to account for commitment fees as well. The link has been modified slightly to calculate dust. In the future, the switch dust calculations can be removed.
Crypt-iQ
force-pushed
the
precise_dust_limit
branch
from
6279406 to
74636e9
Compare
| // Here we override the view's fee-rate if a dry-run fee-rate was | ||
| // passed in. | ||
| if !updateState { | ||
| feePerKw = dryRunFee.UnwrapOr(feePerKw) | ||
| } |
Small-ish change to the link to include commitment fees in dust calculation.