Skip to content

Commit

Permalink
Fix availableForSend/Receive (#1293)
Browse files Browse the repository at this point in the history 
There was a rounding issue with the availableForSend/Receive calculation.
Because CommitTx fee and Htlc fee were computed separately,
but each was individually rounded down to Satoshis, we could
end up with an off-by-one error.

This resulted in an incapacity to send/receive the maximum amount available.
We now allow computing fees in msat, which removes rounding issues.
  • Loading branch information
@t-bast
t-bast committed Jan 28, 2020
1 parent d5cdd6a commit 0a66d3f
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 40 deletions.
5 changes: 2 additions & 3 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import fr.acinq.bitcoin.Crypto.{PrivateKey, PublicKey, sha256}
import fr.acinq.bitcoin.{ByteVector32, ByteVector64, Crypto}
import fr.acinq.eclair.blockchain.fee.{FeeEstimator, FeeTargets}
import fr.acinq.eclair.crypto.{Generators, KeyManager, ShaChain, Sphinx}
import fr.acinq.eclair.payment._
import fr.acinq.eclair.payment.relay.{Origin, Relayer}
import fr.acinq.eclair.transactions.Transactions._
import fr.acinq.eclair.transactions._
Expand Down Expand Up @@ -92,7 +91,7 @@ case class Commitments(channelVersion: ChannelVersion,
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 commitFees = commitTxFeeMsat(remoteParams.dustLimit, reduced)
val htlcFees = htlcOutputFee(reduced.feeratePerKw)
if (balanceNoFees - commitFees < offeredHtlcTrimThreshold(remoteParams.dustLimit, reduced)) {
// htlc will be trimmed
Expand All @@ -115,7 +114,7 @@ case class Commitments(channelVersion: ChannelVersion,
balanceNoFees
} else {
// 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 commitFees = commitTxFeeMsat(localParams.dustLimit, reduced)
val htlcFees = htlcOutputFee(reduced.feeratePerKw)
if (balanceNoFees - commitFees < receivedHtlcTrimThreshold(localParams.dustLimit, reduced)) {
// htlc will be trimmed
Expand Down
21 changes: 15 additions & 6 deletions eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,15 +109,16 @@ object Transactions {
val mainPenaltyWeight = 484
val htlcPenaltyWeight = 578 // based on spending an HTLC-Success output (would be 571 with HTLC-Timeout)

def weight2fee(feeratePerKw: Long, weight: Int) = Satoshi((feeratePerKw * weight) / 1000)
def weight2feeMsat(feeratePerKw: Long, weight: Int) = MilliSatoshi(feeratePerKw * weight)

def weight2fee(feeratePerKw: Long, weight: Int): Satoshi = weight2feeMsat(feeratePerKw, weight).truncateToSatoshi

/**
*
* @param fee tx fee
* @param weight tx weight
* @return the fee rate (in Satoshi/Kw) for this tx
*/
def fee2rate(fee: Satoshi, weight: Int) = (fee.toLong * 1000L) / weight
def fee2rate(fee: Satoshi, weight: Int): Long = (fee.toLong * 1000L) / weight

/** Offered HTLCs below this amount will be trimmed. */
def offeredHtlcTrimThreshold(dustLimit: Satoshi, spec: CommitmentSpec): Satoshi = dustLimit + weight2fee(spec.feeratePerKw, htlcTimeoutWeight)
Expand All @@ -142,15 +143,23 @@ object Transactions {
}

/** Fee for an un-trimmed HTLC. */
def htlcOutputFee(feeratePerKw: Long): Satoshi = weight2fee(feeratePerKw, htlcOutputWeight)
def htlcOutputFee(feeratePerKw: Long): MilliSatoshi = weight2feeMsat(feeratePerKw, htlcOutputWeight)

def commitTxFee(dustLimit: Satoshi, spec: CommitmentSpec): Satoshi = {
/**
* While fees are generally computed in Satoshis (since this is the smallest on-chain unit), it may be useful in some
* cases to calculate it in MilliSatoshi to avoid rounding issues.
* If you are adding multiple fees together for example, you should always add them in MilliSatoshi and then round
* down to Satoshi.
*/
def commitTxFeeMsat(dustLimit: Satoshi, spec: CommitmentSpec): MilliSatoshi = {
val trimmedOfferedHtlcs = trimOfferedHtlcs(dustLimit, spec)
val trimmedReceivedHtlcs = trimReceivedHtlcs(dustLimit, spec)
val weight = commitWeight + htlcOutputWeight * (trimmedOfferedHtlcs.size + trimmedReceivedHtlcs.size)
weight2fee(spec.feeratePerKw, weight)
weight2feeMsat(spec.feeratePerKw, weight)
}

def commitTxFee(dustLimit: Satoshi, spec: CommitmentSpec): Satoshi = commitTxFeeMsat(dustLimit, spec).truncateToSatoshi

/**
*
* @param commitTxNumber commit tx number
Expand Down
112 changes: 110 additions & 2 deletions eclair-core/src/test/scala/fr/acinq/eclair/channel/CommitmentsSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,21 @@ package fr.acinq.eclair.channel

import java.util.UUID

import fr.acinq.bitcoin.{DeterministicWallet, Satoshi, Transaction}
import fr.acinq.eclair.channel.Commitments._
import fr.acinq.eclair.channel.Helpers.Funding
import fr.acinq.eclair.channel.states.StateTestsHelperMethods
import fr.acinq.eclair.crypto.ShaChain
import fr.acinq.eclair.payment.relay.Origin.Local
import fr.acinq.eclair.wire.IncorrectOrUnknownPaymentDetails
import fr.acinq.eclair.transactions.CommitmentSpec
import fr.acinq.eclair.transactions.Transactions.CommitTx
import fr.acinq.eclair.wire.{IncorrectOrUnknownPaymentDetails, UpdateAddHtlc}
import fr.acinq.eclair.{TestkitBaseClass, _}
import org.scalatest.Outcome
import org.scalatest.{Outcome, Tag}
import scodec.bits.ByteVector

import scala.concurrent.duration._
import scala.util.{Failure, Random, Success, Try}

class CommitmentsSpec extends TestkitBaseClass with StateTestsHelperMethods {

Expand Down Expand Up @@ -379,4 +386,105 @@ class CommitmentsSpec extends TestkitBaseClass with StateTestsHelperMethods {
assert(ac16.availableBalanceForReceive == b + p1 - p3)
}

test("can send availableForSend") { f =>
for (isFunder <- Seq(true, false)) {
val c = CommitmentsSpec.makeCommitments(702000000 msat, 52000000 msat, 2679, 546 sat, isFunder)
val (_, cmdAdd) = makeCmdAdd(c.availableBalanceForSend, randomKey.publicKey, f.currentBlockHeight)
val result = sendAdd(c, cmdAdd, Local(UUID.randomUUID, None), f.currentBlockHeight)
assert(result.isRight, result)
}
}

test("can receive availableForReceive") { f =>
for (isFunder <- Seq(true, false)) {
val c = CommitmentsSpec.makeCommitments(31000000 msat, 702000000 msat, 2679, 546 sat, isFunder)
val add = UpdateAddHtlc(randomBytes32, c.remoteNextHtlcId, c.availableBalanceForReceive, randomBytes32, CltvExpiry(f.currentBlockHeight), TestConstants.emptyOnionPacket)
receiveAdd(c, add)
}
}

test("should always be able to send availableForSend", Tag("fuzzy")) { f =>
val maxPendingHtlcAmount = 1000000.msat
case class FuzzTest(isFunder: Boolean, pendingHtlcs: Int, feeRatePerKw: Long, dustLimit: Satoshi, toLocal: MilliSatoshi, toRemote: MilliSatoshi)
for (_ <- 1 to 100) {
val t = FuzzTest(
isFunder = Random.nextInt(2) == 0,
pendingHtlcs = Random.nextInt(10),
feeRatePerKw = Random.nextInt(10000),
dustLimit = Random.nextInt(1000).sat,
// We make sure both sides have enough to send/receive at least the initial pending HTLCs.
toLocal = maxPendingHtlcAmount * 2 * 10 + Random.nextInt(1000000000).msat,
toRemote = maxPendingHtlcAmount * 2 * 10 + Random.nextInt(1000000000).msat)
var c = CommitmentsSpec.makeCommitments(t.toLocal, t.toRemote, t.feeRatePerKw, t.dustLimit, t.isFunder)
// Add some initial HTLCs to the pending list (bigger commit tx).
for (_ <- 0 to t.pendingHtlcs) {
val amount = Random.nextInt(maxPendingHtlcAmount.toLong.toInt).msat
val (_, cmdAdd) = makeCmdAdd(amount, randomKey.publicKey, f.currentBlockHeight)
sendAdd(c, cmdAdd, Local(UUID.randomUUID, None), f.currentBlockHeight) match {
case Right((cc, _)) => c = cc
case Left(e) => fail(s"$t -> could not setup initial htlcs: $e")
}
}
val (_, cmdAdd) = makeCmdAdd(c.availableBalanceForSend, randomKey.publicKey, f.currentBlockHeight)
val result = sendAdd(c, cmdAdd, Local(UUID.randomUUID, None), f.currentBlockHeight)
assert(result.isRight, s"$t -> $result")
}
}

test("should always be able to receive availableForReceive", Tag("fuzzy")) { f =>
val maxPendingHtlcAmount = 1000000.msat
case class FuzzTest(isFunder: Boolean, pendingHtlcs: Int, feeRatePerKw: Long, dustLimit: Satoshi, toLocal: MilliSatoshi, toRemote: MilliSatoshi)
for (_ <- 1 to 100) {
val t = FuzzTest(
isFunder = Random.nextInt(2) == 0,
pendingHtlcs = Random.nextInt(10),
feeRatePerKw = Random.nextInt(10000),
dustLimit = Random.nextInt(1000).sat,
// We make sure both sides have enough to send/receive at least the initial pending HTLCs.
toLocal = maxPendingHtlcAmount * 2 * 10 + Random.nextInt(1000000000).msat,
toRemote = maxPendingHtlcAmount * 2 * 10 + Random.nextInt(1000000000).msat)
var c = CommitmentsSpec.makeCommitments(t.toLocal, t.toRemote, t.feeRatePerKw, t.dustLimit, t.isFunder)
// Add some initial HTLCs to the pending list (bigger commit tx).
for (_ <- 0 to t.pendingHtlcs) {
val amount = Random.nextInt(maxPendingHtlcAmount.toLong.toInt).msat
val add = UpdateAddHtlc(randomBytes32, c.remoteNextHtlcId, amount, randomBytes32, CltvExpiry(f.currentBlockHeight), TestConstants.emptyOnionPacket)
Try(receiveAdd(c, add)) match {
case Success(cc) => c = cc
case Failure(e) => fail(s"$t -> could not setup initial htlcs: $e")
}
}
val add = UpdateAddHtlc(randomBytes32, c.remoteNextHtlcId, c.availableBalanceForReceive, randomBytes32, CltvExpiry(f.currentBlockHeight), TestConstants.emptyOnionPacket)
Try(receiveAdd(c, add)) match {
case Success(_) => ()
case Failure(e) => fail(s"$t -> $e")
}
}
}

}

object CommitmentsSpec {

def makeCommitments(toLocal: MilliSatoshi, toRemote: MilliSatoshi, feeRatePerKw: Long = 0, dustLimit: Satoshi = 0 sat, isFunder: Boolean = true, announceChannel: Boolean = true): Commitments = {
val localParams = LocalParams(randomKey.publicKey, DeterministicWallet.KeyPath(Seq(42L)), dustLimit, UInt64.MaxValue, 0 sat, 1 msat, CltvExpiryDelta(144), 50, isFunder, ByteVector.empty, ByteVector.empty)
val remoteParams = RemoteParams(randomKey.publicKey, dustLimit, UInt64.MaxValue, 0 sat, 1 msat, CltvExpiryDelta(144), 50, randomKey.publicKey, randomKey.publicKey, randomKey.publicKey, randomKey.publicKey, randomKey.publicKey, ByteVector.empty)
val commitmentInput = Funding.makeFundingInputInfo(randomBytes32, 0, (toLocal + toRemote).truncateToSatoshi, randomKey.publicKey, remoteParams.fundingPubKey)
Commitments(
ChannelVersion.STANDARD,
localParams,
remoteParams,
channelFlags = if (announceChannel) ChannelFlags.AnnounceChannel else ChannelFlags.Empty,
LocalCommit(0, CommitmentSpec(Set.empty, feeRatePerKw, toLocal, toRemote), PublishableTxs(CommitTx(commitmentInput, Transaction(2, Nil, Nil, 0)), Nil)),
RemoteCommit(0, CommitmentSpec(Set.empty, feeRatePerKw, toRemote, toLocal), randomBytes32, randomKey.publicKey),
LocalChanges(Nil, Nil, Nil),
RemoteChanges(Nil, Nil, Nil),
localNextHtlcId = 1,
remoteNextHtlcId = 1,
originChannels = Map.empty,
remoteNextCommitInfo = Right(randomKey.publicKey),
commitInput = commitmentInput,
remotePerCommitmentSecrets = ShaChain.init,
channelId = randomBytes32)
}

}
34 changes: 5 additions & 29 deletions eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartPaymentLifecycleSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,11 @@ import java.util.UUID
import akka.actor.{ActorRef, ActorSystem}
import akka.testkit.{TestFSMRef, TestKit, TestProbe}
import fr.acinq.bitcoin.Crypto.PrivateKey
import fr.acinq.bitcoin.{Block, Crypto, DeterministicWallet, Satoshi, Transaction}
import fr.acinq.bitcoin.{Block, Crypto, Satoshi}
import fr.acinq.eclair.TestConstants.TestFeeEstimator
import fr.acinq.eclair._
import fr.acinq.eclair.blockchain.fee.FeeratesPerKw
import fr.acinq.eclair.channel.Helpers.Funding
import fr.acinq.eclair.channel.{ChannelFlags, Commitments, Upstream}
import fr.acinq.eclair.channel.{ChannelFlags, Commitments, CommitmentsSpec, Upstream}
import fr.acinq.eclair.crypto.Sphinx
import fr.acinq.eclair.payment.PaymentSent.PartialPayment
import fr.acinq.eclair.payment.relay.Relayer.{GetOutgoingChannels, OutgoingChannel, OutgoingChannels}
Expand All @@ -35,8 +34,6 @@ import fr.acinq.eclair.payment.send.MultiPartPaymentLifecycle._
import fr.acinq.eclair.payment.send.PaymentInitiator.SendPaymentConfig
import fr.acinq.eclair.payment.send.PaymentLifecycle.SendPayment
import fr.acinq.eclair.router._
import fr.acinq.eclair.transactions.CommitmentSpec
import fr.acinq.eclair.transactions.Transactions.CommitTx
import fr.acinq.eclair.wire._
import org.scalatest.{Outcome, Tag, fixture}
import scodec.bits.ByteVector
Expand Down Expand Up @@ -546,29 +543,8 @@ object MultiPartPaymentLifecycleSpec {

val emptyStats = NetworkStats(0, 0, Stats(Seq(0), d => Satoshi(d.toLong)), Stats(Seq(0), d => CltvExpiryDelta(d.toInt)), Stats(Seq(0), d => MilliSatoshi(d.toLong)), Stats(Seq(0), d => d.toLong))

def makeCommitments(canSend: MilliSatoshi, feeRatePerKw: Long, announceChannel: Boolean = true): Commitments = {
import fr.acinq.eclair.channel._
import fr.acinq.eclair.crypto.ShaChain
// We are only interested in availableBalanceForSend so we can put dummy values in most places.
val localParams = LocalParams(randomKey.publicKey, DeterministicWallet.KeyPath(Seq(42L)), 0 sat, UInt64(50000000), 0 sat, 1 msat, CltvExpiryDelta(144), 50, isFunder = true, ByteVector.empty, ByteVector.empty)
val remoteParams = RemoteParams(randomKey.publicKey, 0 sat, UInt64(5000000), 0 sat, 1 msat, CltvExpiryDelta(144), 50, randomKey.publicKey, randomKey.publicKey, randomKey.publicKey, randomKey.publicKey, randomKey.publicKey, ByteVector.empty)
val commitmentInput = Funding.makeFundingInputInfo(randomBytes32, 0, canSend.truncateToSatoshi, randomKey.publicKey, remoteParams.fundingPubKey)
Commitments(
ChannelVersion.STANDARD,
localParams,
remoteParams,
channelFlags = if (announceChannel) ChannelFlags.AnnounceChannel else ChannelFlags.Empty,
LocalCommit(0, CommitmentSpec(Set.empty, feeRatePerKw, canSend, 0 msat), PublishableTxs(CommitTx(commitmentInput, Transaction(2, Nil, Nil, 0)), Nil)),
RemoteCommit(0, CommitmentSpec(Set.empty, feeRatePerKw, 0 msat, canSend), randomBytes32, randomKey.publicKey),
LocalChanges(Nil, Nil, Nil),
RemoteChanges(Nil, Nil, Nil),
localNextHtlcId = 1,
remoteNextHtlcId = 1,
originChannels = Map.empty,
remoteNextCommitInfo = Right(randomKey.publicKey),
commitInput = commitmentInput,
remotePerCommitmentSecrets = ShaChain.init,
channelId = randomBytes32)
}
// We are only interested in availableBalanceForSend so we can put dummy values for the rest.
def makeCommitments(canSend: MilliSatoshi, feeRatePerKw: Long, announceChannel: Boolean = true): Commitments =
CommitmentsSpec.makeCommitments(canSend, 0 msat, feeRatePerKw, 0 sat, announceChannel = announceChannel)

}

0 comments on commit 0a66d3f

Please sign in to comment.