Skip to content
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.

Sign up for GitHub

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

Fixup funder fee buffer #1364

Merged
merged 1 commit into from
Sep 29, 2020
Merged

Fixup funder fee buffer #1364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 53 additions & 19 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,32 @@ case class Commitments(channelVersion: ChannelVersion,

val announceChannel: Boolean = (channelFlags & 0x01) != 0

// NB: when computing availableBalanceForSend and availableBalanceForReceive, the funder keeps an extra buffer on top
// of its usual channel reserve to avoid getting channels stuck in case the on-chain feerate increases (see
// https://github.com/lightningnetwork/lightning-rfc/issues/728 for details).
//
// This extra buffer (which we call "funder fee buffer") is calculated as follows:
// 1) Simulate a x2 feerate increase and compute the corresponding commit tx fee (note that it may trim some HTLCs)
// 2) Add the cost of adding a new untrimmed HTLC at that increased feerate. This ensures that we'll be able to
// actually use the channel to add new HTLCs if the feerate doubles.
//
// If for example the current feerate is 1000 sat/kw, the dust limit 546 sat, and we have 3 pending outgoing HTLCs for
// respectively 1250 sat, 2000 sat and 2500 sat.
// commit tx fee = commitWeight * feerate + 3 * htlcOutputWeight * feerate = 724 * 1000 + 3 * 172 * 1000 = 1240 sat
// To calculate the funder fee buffer, we first double the feerate and calculate the corresponding commit tx fee.
// By doubling the feerate, the first HTLC becomes trimmed so the result is: 724 * 2000 + 2 * 172 * 2000 = 2136 sat
// We then add the additional fee for a potential new untrimmed HTLC: 172 * 2000 = 344 sat
// The funder fee buffer is 2136 + 344 = 2480 sat
//
// If there are many pending HTLCs that are only slightly above the trim threshold, the funder fee buffer may be
// smaller than the current commit tx fee because those HTLCs will be trimmed and the commit tx weight will decrease.
// For example if we have 10 outgoing HTLCs of 1250 sat:
// - commit tx fee = 724 * 1000 + 10 * 172 * 1000 = 2444 sat
// - commit tx fee at twice the feerate = 724 * 2000 = 1448 sat (all HTLCs have been trimmed)
// - cost of an additional untrimmed HTLC = 172 * 2000 = 344 sat
// - funder fee buffer = 1448 + 344 = 1792 sat
// In that case the current commit tx fee is higher than the funder fee buffer and will dominate the balance restrictions.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a fee-buffer can act in two different directions, meaning that in case of a down-spike former dust htlcs can use this buffer as well. So I would argue to not really dynamically simulate a 2x increase in the feerate and based of this calculate the feebuffer but rather just take the worst case scenario and account for the double fee at the current feerate (thats basically a 2x simulation without taking any trimmed htlcs into account) ? Why not consider tripped htlcs? Because trimmed htlcs are accounted for fees anyways. So in your above example when all 10 htlcs are trimmed, we have a final fee of
0.724 * 2000 + 10 * 1250 = 13950 sasts, so we could just keep the double fee as a buffer because this would have the advantage, if there are already trimmed away htlcs on the commitment, the feerate could also be decreased in an event of a fee-dump ? wdyt ?


lazy val availableBalanceForSend: MilliSatoshi = {
// we need to base the next current commitment on the last sig we sent, even if we didn't yet receive their revocation
val remoteCommit1 = remoteNextCommitInfo.left.toOption.map(_.nextRemoteCommit).getOrElse(remoteCommit)
Expand All @@ -109,16 +135,19 @@ case class Commitments(channelVersion: ChannelVersion,
if (localParams.isFunder) {
// The funder always pays the on-chain fees, so we must subtract that from the amount we can send.
val commitFees = commitTxFeeMsat(remoteParams.dustLimit, reduced, commitmentFormat)
// the funder needs to keep an extra reserve to be able to handle fee increase without getting the channel stuck
// (see https://github.com/lightningnetwork/lightning-rfc/issues/728)
val funderFeeReserve = htlcOutputFee(reduced.feeratePerKw * 2, commitmentFormat)
val htlcFees = htlcOutputFee(reduced.feeratePerKw, commitmentFormat)
if (balanceNoFees - commitFees < offeredHtlcTrimThreshold(remoteParams.dustLimit, reduced, commitmentFormat)) {
// the funder needs to keep a "funder fee buffer" (see explanation above)
val funderFeeBuffer = commitTxFeeMsat(remoteParams.dustLimit, reduced.copy(feeratePerKw = reduced.feeratePerKw * 2), commitmentFormat) + htlcOutputFee(reduced.feeratePerKw * 2, commitmentFormat)
val amountToReserve = commitFees.max(funderFeeBuffer)
pm47 marked this conversation as resolved.
Show resolved Hide resolved
if (balanceNoFees - amountToReserve < offeredHtlcTrimThreshold(remoteParams.dustLimit, reduced, commitmentFormat)) {
// htlc will be trimmed
(balanceNoFees - commitFees - funderFeeReserve).max(0 msat)
(balanceNoFees - amountToReserve).max(0 msat)
} else {
// htlc will have an output in the commitment tx, so there will be additional fees.
(balanceNoFees - commitFees - funderFeeReserve - htlcFees).max(0 msat)
val commitFees1 = commitFees + htlcOutputFee(reduced.feeratePerKw, commitmentFormat)
// we take the additional fees for that htlc output into account in the fee buffer at a x2 feerate increase
val funderFeeBuffer1 = funderFeeBuffer + htlcOutputFee(reduced.feeratePerKw * 2, commitmentFormat)
val amountToReserve1 = commitFees1.max(funderFeeBuffer1)
pm47 marked this conversation as resolved.
Show resolved Hide resolved
(balanceNoFees - amountToReserve1).max(0 msat)
}
} else {
// The fundee doesn't pay on-chain fees.
Expand All @@ -135,16 +164,19 @@ case class Commitments(channelVersion: ChannelVersion,
} else {
// The funder always pays the on-chain fees, so we must subtract that from the amount we can receive.
val commitFees = commitTxFeeMsat(localParams.dustLimit, reduced, commitmentFormat)
// we expect the funder to keep an extra reserve to be able to handle fee increase without getting the channel stuck
// (see https://github.com/lightningnetwork/lightning-rfc/issues/728)
val funderFeeReserve = htlcOutputFee(reduced.feeratePerKw * 2, commitmentFormat)
val htlcFees = htlcOutputFee(reduced.feeratePerKw, commitmentFormat)
if (balanceNoFees - commitFees < receivedHtlcTrimThreshold(localParams.dustLimit, reduced, commitmentFormat)) {
// we expected the funder to keep a "funder fee buffer" (see explanation above)
val funderFeeBuffer = commitTxFeeMsat(localParams.dustLimit, reduced.copy(feeratePerKw = reduced.feeratePerKw * 2), commitmentFormat) + htlcOutputFee(reduced.feeratePerKw * 2, commitmentFormat)
val amountToReserve = commitFees.max(funderFeeBuffer)
if (balanceNoFees - amountToReserve < receivedHtlcTrimThreshold(localParams.dustLimit, reduced, commitmentFormat)) {
// htlc will be trimmed
(balanceNoFees - commitFees - funderFeeReserve).max(0 msat)
(balanceNoFees - amountToReserve).max(0 msat)
} else {
// htlc will have an output in the commitment tx, so there will be additional fees.
(balanceNoFees - commitFees - funderFeeReserve - htlcFees).max(0 msat)
val commitFees1 = commitFees + htlcOutputFee(reduced.feeratePerKw, commitmentFormat)
// we take the additional fees for that htlc output into account in the fee buffer at a x2 feerate increase
val funderFeeBuffer1 = funderFeeBuffer + htlcOutputFee(reduced.feeratePerKw * 2, commitmentFormat)
val amountToReserve1 = commitFees1.max(funderFeeBuffer1)
(balanceNoFees - amountToReserve1).max(0 msat)
}
}
}
Expand Down Expand Up @@ -176,7 +208,7 @@ object Commitments {
*
* @param commitments current commitments
* @param cmd add HTLC command
* @return either Left(failure, error message) where failure is a failure message (see BOLT #4 and the Failure Message class) or Right((new commitments, updateAddHtlc)
* @return either Left(failure, error message) where failure is a failure message (see BOLT #4 and the Failure Message class) or Right(new commitments, updateAddHtlc)
*/
def sendAdd(commitments: Commitments, cmd: CMD_ADD_HTLC, blockHeight: Long, feeConf: OnChainFeeConf): Either[ChannelException, (Commitments, UpdateAddHtlc)] = {
// we don't want to use too high a refund timeout, because our funds will be locked during that time if the payment is never fulfilled
Expand Down Expand Up @@ -210,10 +242,12 @@ object Commitments {

// note that the funder pays the fee, so if sender != funder, both sides will have to afford this payment
val fees = commitTxFee(commitments1.remoteParams.dustLimit, reduced, commitments.commitmentFormat)
// the funder needs to keep an extra reserve to be able to handle fee increase without getting the channel stuck
// (see https://github.com/lightningnetwork/lightning-rfc/issues/728)
val funderFeeReserve = htlcOutputFee(reduced.feeratePerKw * 2, commitments.commitmentFormat)
val missingForSender = reduced.toRemote - commitments1.remoteParams.channelReserve - (if (commitments1.localParams.isFunder) fees + funderFeeReserve else 0.msat)
// the funder needs to keep an extra buffer to be able to handle a x2 feerate increase and an additional htlc to avoid
// getting the channel stuck (see https://github.com/lightningnetwork/lightning-rfc/issues/728).
val funderFeeBuffer = commitTxFeeMsat(commitments1.remoteParams.dustLimit, reduced.copy(feeratePerKw = reduced.feeratePerKw * 2), commitments.commitmentFormat) + htlcOutputFee(reduced.feeratePerKw * 2, commitments.commitmentFormat)
// NB: increasing the feerate can actually remove htlcs from the commit tx (if they fall below the trim threshold)
// which may result in a lower commit tx fee; this is why we take the max of the two.
val missingForSender = reduced.toRemote - commitments1.remoteParams.channelReserve - (if (commitments1.localParams.isFunder) fees.max(funderFeeBuffer.truncateToSatoshi) else 0.sat)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that this is not what c-lightning has implemented: https://github.com/ElementsProject/lightning/blob/af7e87930878117a0633bb7a8cd4488b8e9c5b6d/channeld/full_channel.c#L430-L440

they trim htlcs at the current feerate, and then double the feerate and not trim again

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what do you think of it? I think their choice is a bit weird and the additional reserve it ends up adding may be too big, but that's just my opinion.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were to plot the commitment tx fees as a function of feerate, because of trimming, I expect it would be a spiky curve. This PR atm (sort of) takes the max of two samplings, while c-lightning takes a conservative upper bound.
As feerates change, the max of two samplings might be lower than the actual commitment tx fees, and potentially the issue the additional reserve is trying to fix could resurface (i.e. the fee spike buffer that is supposed to handle fee spikes up to 2x of current feerate might not be sufficient to handle all feerates in between). Though admittedly I did not look at specific numbers so perhaps this is highly unlikely in practice.

My personal opinion is that it might make more sense to take an upper bound of the curve, as c-lightning does, as then that should cover all possible feerates up to the chosen parameter (of "2x").

Copy link
Member Author

@t-bast t-bast Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were to plot the commitment tx fees as a function of feerate, because of trimming, I expect it would be a spiky curve.

Yes exactly, and that can be surprising at first!

the issue the additional reserve is trying to fix could resurface (i.e. the fee spike buffer that is supposed to handle fee spikes up to 2x of current feerate might not be sufficient to handle all feerates in between).

Yes but only temporarily, because the HTLCs you have in the commit tx will eventually settle.
Once they settle that gives you money back and unblocks the channel. And I think that's an important insight.

Taking the conservative upper bound also works, but it means there's more of your channel balance that you just can't use, which I want to avoid unless absolutely necessary. It's already hard to tell users "you put X mBTC in this channel but in fact what you'll be able to use is less than that, but trust us, this is necessary", so let's not add more to that if we can avoid it. Does that make sense?

val missingForReceiver = reduced.toLocal - commitments1.localParams.channelReserve - (if (commitments1.localParams.isFunder) 0.sat else fees)
if (missingForSender < 0.msat) {
return Left(InsufficientFunds(commitments.channelId, amount = cmd.amount, missing = -missingForSender.truncateToSatoshi, reserve = commitments1.remoteParams.channelReserve, fees = if (commitments1.localParams.isFunder) fees else 0.sat))
Expand Down
Loading