From 0ac66cf1c5a9ab9fed8733ca8769b390090f156d Mon Sep 17 00:00:00 2001 From: Bastien Teinturier Date: Mon, 30 Sep 2019 16:00:46 +0200 Subject: [PATCH 1/2] Commitments: take HTLC fee into account --- .../fr/acinq/eclair/channel/Commitments.scala | 18 +++++++++-- .../eclair/transactions/Transactions.scala | 20 +++++++++--- .../eclair/channel/CommitmentsSpec.scala | 32 +++++++++++++++---- .../channel/states/e/NormalStateSpec.scala | 6 ++-- 4 files changed, 60 insertions(+), 16 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala index f6cf5ca873..44cb6c0913 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala @@ -88,14 +88,28 @@ case class Commitments(channelVersion: ChannelVersion, // 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) val reduced = CommitmentSpec.reduce(remoteCommit1.spec, remoteChanges.acked, localChanges.proposed) + // The funder always pays the on-chain fees, so we must subtract that from the amount we can send. val feesMsat = if (localParams.isFunder) commitTxFee(remoteParams.dustLimit, reduced).toMilliSatoshi else 0.msat - (reduced.toRemote - remoteParams.channelReserve.toMilliSatoshi - feesMsat).max(0 msat) + val balance = (reduced.toRemote - remoteParams.channelReserve.toMilliSatoshi - feesMsat).max(0 msat) + // We also need to take into account the fact that adding an htlc increases the commit tx fee if it's not trimmed. + if (localParams.isFunder && balance >= offeredHtlcTrimThreshold(remoteParams.dustLimit, reduced)) { + (balance - htlcOutputFee(reduced.feeratePerKw)).max(0 msat) + } else { + balance + } } lazy val availableBalanceForReceive: MilliSatoshi = { val reduced = CommitmentSpec.reduce(localCommit.spec, localChanges.acked, remoteChanges.proposed) + // The funder always pays the on-chain fees, so we must subtract that from the amount we can send. val feesMsat = if (localParams.isFunder) 0.msat else commitTxFee(localParams.dustLimit, reduced).toMilliSatoshi - (reduced.toRemote - localParams.channelReserve.toMilliSatoshi - feesMsat).max(0 msat) + val balance = (reduced.toRemote - localParams.channelReserve.toMilliSatoshi - feesMsat).max(0 msat) + // We also need to take into account the fact that adding an htlc increases the commit tx fee if it's not trimmed. + if (!localParams.isFunder && balance >= receivedHtlcTrimThreshold(localParams.dustLimit, reduced)) { + (balance - htlcOutputFee(reduced.feeratePerKw)).max(0 msat) + } else { + balance + } } } diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala b/eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala index fd89a2b9b3..1ba742a336 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala @@ -95,6 +95,7 @@ object Transactions { * these values are defined in the RFC */ val commitWeight = 724 + val htlcOutputWeight = 172 val htlcTimeoutWeight = 663 val htlcSuccessWeight = 703 @@ -118,26 +119,35 @@ object Transactions { */ def fee2rate(fee: Satoshi, weight: Int) = (fee.toLong * 1000L) / weight + /** Offered HTLCs below this amount will be trimmed. */ + def offeredHtlcTrimThreshold(dustLimit: Satoshi, spec: CommitmentSpec): Satoshi = dustLimit + weight2fee(spec.feeratePerKw, htlcTimeoutWeight) + def trimOfferedHtlcs(dustLimit: Satoshi, spec: CommitmentSpec): Seq[DirectedHtlc] = { - val htlcTimeoutFee = weight2fee(spec.feeratePerKw, htlcTimeoutWeight) + val threshold = offeredHtlcTrimThreshold(dustLimit, spec) spec.htlcs .filter(_.direction == OUT) - .filter(htlc => htlc.add.amountMsat >= (dustLimit + htlcTimeoutFee)) + .filter(htlc => htlc.add.amountMsat >= threshold) .toSeq } + /** Received HTLCs below this amount will be trimmed. */ + def receivedHtlcTrimThreshold(dustLimit: Satoshi, spec: CommitmentSpec): Satoshi = dustLimit + weight2fee(spec.feeratePerKw, htlcSuccessWeight) + def trimReceivedHtlcs(dustLimit: Satoshi, spec: CommitmentSpec): Seq[DirectedHtlc] = { - val htlcSuccessFee = weight2fee(spec.feeratePerKw, htlcSuccessWeight) + val threshold = receivedHtlcTrimThreshold(dustLimit, spec) spec.htlcs .filter(_.direction == IN) - .filter(htlc => htlc.add.amountMsat >= (dustLimit + htlcSuccessFee)) + .filter(htlc => htlc.add.amountMsat >= threshold) .toSeq } + /** Fee for an un-trimmed HTLC. */ + def htlcOutputFee(feeratePerKw: Long): Satoshi = weight2fee(feeratePerKw, htlcOutputWeight) + def commitTxFee(dustLimit: Satoshi, spec: CommitmentSpec): Satoshi = { val trimmedOfferedHtlcs = trimOfferedHtlcs(dustLimit, spec) val trimmedReceivedHtlcs = trimReceivedHtlcs(dustLimit, spec) - val weight = commitWeight + 172 * (trimmedOfferedHtlcs.size + trimmedReceivedHtlcs.size) + val weight = commitWeight + htlcOutputWeight * (trimmedOfferedHtlcs.size + trimmedReceivedHtlcs.size) weight2fee(spec.feeratePerKw, weight) } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/CommitmentsSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/CommitmentsSpec.scala index eb23739a34..9b8be1b0d5 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/CommitmentsSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/CommitmentsSpec.scala @@ -44,12 +44,32 @@ class CommitmentsSpec extends TestkitBaseClass with StateTestsHelperMethods { } } + test("take additional HTLC fee into account") { f => + import f._ + val htlcOutputFee = 1720000 msat + val a = 772760000 msat // initial balance alice + val ac0 = alice.stateData.asInstanceOf[DATA_NORMAL].commitments + val bc0 = bob.stateData.asInstanceOf[DATA_NORMAL].commitments + // we need to take the additional HTLC fee into account because balances are above the trim threshold. + assert(ac0.availableBalanceForSend == a - htlcOutputFee) + assert(bc0.availableBalanceForReceive == a - htlcOutputFee) + + val (_, cmdAdd) = makeCmdAdd(a - htlcOutputFee - 1000.msat, bob.underlyingActor.nodeParams.nodeId, currentBlockHeight) + val Right((ac1, add)) = sendAdd(ac0, cmdAdd, Local(UUID.randomUUID, None), currentBlockHeight) + val bc1 = receiveAdd(bc0, add) + val (_, commit1) = sendCommit(ac1, alice.underlyingActor.nodeParams.keyManager) + val (bc2, _) = receiveCommit(bc1, commit1, bob.underlyingActor.nodeParams.keyManager) + // we don't take into account the additional HTLC fee since Alice's balance is below the trim threshold. + assert(ac1.availableBalanceForSend == 1000.msat) + assert(bc2.availableBalanceForReceive == 1000.msat) + } + test("correct values for availableForSend/availableForReceive (success case)") { f => import f._ - val a = 772760000 msat // initial balance alice - val b = 190000000 msat // initial balance bob val fee = 1720000 msat // fee due to the additional htlc output + val a = (772760000 msat) - fee // initial balance alice + val b = 190000000 msat // initial balance bob val p = 42000000 msat // a->b payment val ac0 = alice.stateData.asInstanceOf[DATA_NORMAL].commitments @@ -131,9 +151,9 @@ class CommitmentsSpec extends TestkitBaseClass with StateTestsHelperMethods { test("correct values for availableForSend/availableForReceive (failure case)") { f => import f._ - val a = 772760000 msat // initial balance alice - val b = 190000000 msat // initial balance bob val fee = 1720000 msat // fee due to the additional htlc output + val a = (772760000 msat) - fee // initial balance alice + val b = 190000000 msat // initial balance bob val p = 42000000 msat // a->b payment val ac0 = alice.stateData.asInstanceOf[DATA_NORMAL].commitments @@ -215,9 +235,9 @@ class CommitmentsSpec extends TestkitBaseClass with StateTestsHelperMethods { test("correct values for availableForSend/availableForReceive (multiple htlcs)") { f => import f._ - val a = 772760000 msat // initial balance alice - val b = 190000000 msat // initial balance bob val fee = 1720000 msat // fee due to the additional htlc output + val a = (772760000 msat) - fee // initial balance alice + val b = 190000000 msat // initial balance bob val p1 = 10000000 msat // a->b payment val p2 = 20000000 msat // a->b payment val p3 = 40000000 msat // b->a payment diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala index f665dbc66b..00d239048c 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala @@ -184,15 +184,15 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods { test("recv CMD_ADD_HTLC (HTLC dips remote funder below reserve)") { f => import f._ val sender = TestProbe() - addHtlc(MilliSatoshi(771000000), alice, bob, alice2bob, bob2alice) + addHtlc(771000000 msat, alice, bob, alice2bob, bob2alice) crossSign(alice, bob, alice2bob, bob2alice) assert(alice.stateData.asInstanceOf[DATA_NORMAL].commitments.availableBalanceForSend === 40000.msat) // actual test begins // at this point alice has the minimal amount to sustain a channel (29000 sat ~= alice reserve + commit fee) - val add = CMD_ADD_HTLC(MilliSatoshi(120000000), randomBytes32, CltvExpiry(400144), TestConstants.emptyOnionPacket, upstream = Left(UUID.randomUUID())) + val add = CMD_ADD_HTLC(120000000 msat, randomBytes32, CltvExpiry(400144), TestConstants.emptyOnionPacket, upstream = Left(UUID.randomUUID())) sender.send(bob, add) - val error = RemoteCannotAffordFeesForNewHtlc(channelId(bob), add.amount, missing = 1680.sat, 10000.sat, 10680.sat) + val error = RemoteCannotAffordFeesForNewHtlc(channelId(bob), add.amount, missing = 1680 sat, 10000 sat, 10680 sat) sender.expectMsg(Failure(AddHtlcFailed(channelId(bob), add.paymentHash, error, Local(add.upstream.left.get, Some(sender.ref)), Some(bob.stateData.asInstanceOf[DATA_NORMAL].channelUpdate), Some(add)))) } From 4abe042f48efe88ba3e9894a372026cd5742ef88 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier Date: Mon, 30 Sep 2019 16:25:46 +0200 Subject: [PATCH 2/2] Refactor: make calculation steps clearer. --- .../fr/acinq/eclair/channel/Commitments.scala | 42 ++++++++++++------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala index 44cb6c0913..27db4747c3 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala @@ -88,27 +88,41 @@ case class Commitments(channelVersion: ChannelVersion, // 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) val reduced = CommitmentSpec.reduce(remoteCommit1.spec, remoteChanges.acked, localChanges.proposed) - // The funder always pays the on-chain fees, so we must subtract that from the amount we can send. - val feesMsat = if (localParams.isFunder) commitTxFee(remoteParams.dustLimit, reduced).toMilliSatoshi else 0.msat - val balance = (reduced.toRemote - remoteParams.channelReserve.toMilliSatoshi - feesMsat).max(0 msat) - // We also need to take into account the fact that adding an htlc increases the commit tx fee if it's not trimmed. - if (localParams.isFunder && balance >= offeredHtlcTrimThreshold(remoteParams.dustLimit, reduced)) { - (balance - htlcOutputFee(reduced.feeratePerKw)).max(0 msat) + val balanceNoFees = (reduced.toRemote - remoteParams.channelReserve).max(0 msat) + if (localParams.isFunder) { + // The funder always pays the on-chain fees, so we must subtract that from the amount we can send. + val commitFees = commitTxFee(remoteParams.dustLimit, reduced).toMilliSatoshi + val htlcFees = htlcOutputFee(reduced.feeratePerKw) + if (balanceNoFees - commitFees < offeredHtlcTrimThreshold(remoteParams.dustLimit, reduced)) { + // htlc will be trimmed + (balanceNoFees - commitFees).max(0 msat) + } else { + // htlc will have an output in the commitment tx, so there will be additional fees. + (balanceNoFees - commitFees - htlcFees).max(0 msat) + } } else { - balance + // The fundee doesn't pay on-chain fees. + balanceNoFees } } lazy val availableBalanceForReceive: MilliSatoshi = { val reduced = CommitmentSpec.reduce(localCommit.spec, localChanges.acked, remoteChanges.proposed) - // The funder always pays the on-chain fees, so we must subtract that from the amount we can send. - val feesMsat = if (localParams.isFunder) 0.msat else commitTxFee(localParams.dustLimit, reduced).toMilliSatoshi - val balance = (reduced.toRemote - localParams.channelReserve.toMilliSatoshi - feesMsat).max(0 msat) - // We also need to take into account the fact that adding an htlc increases the commit tx fee if it's not trimmed. - if (!localParams.isFunder && balance >= receivedHtlcTrimThreshold(localParams.dustLimit, reduced)) { - (balance - htlcOutputFee(reduced.feeratePerKw)).max(0 msat) + val balanceNoFees = (reduced.toRemote - localParams.channelReserve).max(0 msat) + if (localParams.isFunder) { + // The fundee doesn't pay on-chain fees so we don't take those into account when receiving. + balanceNoFees } else { - balance + // The funder always pays the on-chain fees, so we must subtract that from the amount we can receive. + val commitFees = commitTxFee(localParams.dustLimit, reduced).toMilliSatoshi + val htlcFees = htlcOutputFee(reduced.feeratePerKw) + if (balanceNoFees - commitFees < receivedHtlcTrimThreshold(localParams.dustLimit, reduced)) { + // htlc will be trimmed + (balanceNoFees - commitFees).max(0 msat) + } else { + // htlc will have an output in the commitment tx, so there will be additional fees. + (balanceNoFees - commitFees - htlcFees).max(0 msat) + } } } }