From fd9a8d1349030eb46a05c7151ae4de83284c7793 Mon Sep 17 00:00:00 2001 From: Andrea Date: Thu, 22 Aug 2019 14:29:13 +0200 Subject: [PATCH 1/2] Use unsigned comparison for HtlcValueTooHighInFlight --- .../fr/acinq/eclair/channel/ChannelExceptions.scala | 2 +- .../scala/fr/acinq/eclair/channel/ChannelTypes.scala | 4 ++-- .../scala/fr/acinq/eclair/channel/Commitments.scala | 11 +++++++---- .../src/main/scala/fr/acinq/eclair/package.scala | 4 ++-- .../fr/acinq/eclair/wire/LightningMessageTypes.scala | 4 ++-- .../test/scala/fr/acinq/eclair/CoinUtilsSpec.scala | 11 +++++++++++ .../eclair/channel/states/e/NormalStateSpec.scala | 4 ++-- .../scala/fr/acinq/eclair/payment/RelayerSpec.scala | 4 ++-- 8 files changed, 29 insertions(+), 15 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelExceptions.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelExceptions.scala index 56fc890eb5..9a724a6658 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelExceptions.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelExceptions.scala @@ -64,7 +64,7 @@ case class UnexpectedHtlcId (override val channelId: ByteVect case class ExpiryTooSmall (override val channelId: ByteVector32, minimum: Long, actual: Long, blockCount: Long) extends ChannelException(channelId, s"expiry too small: minimum=$minimum actual=$actual blockCount=$blockCount") case class ExpiryTooBig (override val channelId: ByteVector32, maximum: Long, actual: Long, blockCount: Long) extends ChannelException(channelId, s"expiry too big: maximum=$maximum actual=$actual blockCount=$blockCount") case class HtlcValueTooSmall (override val channelId: ByteVector32, minimum: MilliSatoshi, actual: MilliSatoshi) extends ChannelException(channelId, s"htlc value too small: minimum=$minimum actual=$actual") -case class HtlcValueTooHighInFlight (override val channelId: ByteVector32, maximum: UInt64, actual: UInt64) extends ChannelException(channelId, s"in-flight htlcs hold too much value: maximum=$maximum actual=$actual") +case class HtlcValueTooHighInFlight (override val channelId: ByteVector32, maximum: UInt64, actual: MilliSatoshi) extends ChannelException(channelId, s"in-flight htlcs hold too much value: maximum=$maximum actual=$actual") case class TooManyAcceptedHtlcs (override val channelId: ByteVector32, maximum: Long) extends ChannelException(channelId, s"too many accepted htlcs: maximum=$maximum") case class InsufficientFunds (override val channelId: ByteVector32, amount: MilliSatoshi, missing: Satoshi, reserve: Satoshi, fees: Satoshi) extends ChannelException(channelId, s"insufficient funds: missing=$missing reserve=$reserve fees=$fees") case class InvalidHtlcPreimage (override val channelId: ByteVector32, id: Long) extends ChannelException(channelId, s"invalid htlc preimage for htlc id=$id") diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelTypes.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelTypes.scala index 151cf582d5..a42d5f586a 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelTypes.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelTypes.scala @@ -193,7 +193,7 @@ final case class DATA_WAIT_FOR_REMOTE_PUBLISH_FUTURE_COMMITMENT(commitments: Com final case class LocalParams(nodeId: PublicKey, channelKeyPath: DeterministicWallet.KeyPath, dustLimit: Satoshi, - maxHtlcValueInFlightMsat: UInt64, + maxHtlcValueInFlightMsat: UInt64, // this is not MilliSatoshi because it can exceed the total amount of MilliSatoshi channelReserve: Satoshi, htlcMinimum: MilliSatoshi, toSelfDelay: Int, @@ -205,7 +205,7 @@ final case class LocalParams(nodeId: PublicKey, final case class RemoteParams(nodeId: PublicKey, dustLimit: Satoshi, - maxHtlcValueInFlightMsat: UInt64, + maxHtlcValueInFlightMsat: UInt64, // this is not MilliSatoshi because it can exceed the total amount of MilliSatoshi channelReserve: Satoshi, htlcMinimum: MilliSatoshi, toSelfDelay: Int, 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 21533f0dba..04842b5f24 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 @@ -17,6 +17,7 @@ package fr.acinq.eclair.channel import akka.event.LoggingAdapter +import com.google.common.primitives.UnsignedLongs import fr.acinq.bitcoin.Crypto.{PrivateKey, PublicKey, sha256} import fr.acinq.bitcoin.{ByteVector32, ByteVector64, Crypto, Satoshi} import fr.acinq.eclair @@ -148,8 +149,9 @@ object Commitments { // the HTLC we are about to create is outgoing, but from their point of view it is incoming val outgoingHtlcs = reduced.htlcs.filter(_.direction == IN) - val htlcValueInFlight = UInt64(outgoingHtlcs.map(_.add.amountMsat).sum.toLong) - if (htlcValueInFlight > commitments1.remoteParams.maxHtlcValueInFlightMsat) { + val htlcValueInFlight = outgoingHtlcs.map(_.add.amountMsat).sum + // we must use unsigned comparison here because 'maxHtlcValueInFlightMsat' is effectively an uint64 and can exceed the capacity of MilliSatoshi class + if (htlcValueInFlight.compareUnsigned(commitments1.remoteParams.maxHtlcValueInFlightMsat) > 0) { // TODO: this should be a specific UPDATE error return Left(HtlcValueTooHighInFlight(commitments.channelId, maximum = commitments1.remoteParams.maxHtlcValueInFlightMsat, actual = htlcValueInFlight)) } @@ -183,8 +185,9 @@ object Commitments { val reduced = CommitmentSpec.reduce(commitments1.localCommit.spec, commitments1.localChanges.acked, commitments1.remoteChanges.proposed) val incomingHtlcs = reduced.htlcs.filter(_.direction == IN) - val htlcValueInFlight = UInt64(incomingHtlcs.map(_.add.amountMsat).sum.toLong) - if (htlcValueInFlight > commitments1.localParams.maxHtlcValueInFlightMsat) { + val htlcValueInFlight = incomingHtlcs.map(_.add.amountMsat).sum + // we must use unsigned comparison here because 'maxHtlcValueInFlightMsat' is effectively an uint64 and can exceed the capacity of MilliSatoshi class + if (htlcValueInFlight.compareUnsigned(commitments1.localParams.maxHtlcValueInFlightMsat) > 0) { throw HtlcValueTooHighInFlight(commitments.channelId, maximum = commitments1.localParams.maxHtlcValueInFlightMsat, actual = htlcValueInFlight) } diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/package.scala b/eclair-core/src/main/scala/fr/acinq/eclair/package.scala index 2d7709c974..f3a7519e8f 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/package.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/package.scala @@ -17,12 +17,11 @@ package fr.acinq import java.security.SecureRandom - +import com.google.common.primitives.UnsignedLongs import fr.acinq.bitcoin.Crypto.PrivateKey import fr.acinq.bitcoin._ import scodec.Attempt import scodec.bits.{BitVector, ByteVector} - import scala.concurrent.duration.Duration import scala.util.{Failure, Success, Try} @@ -173,6 +172,7 @@ package object eclair { def *(m: Double) = MilliSatoshi((amount * m).toLong) def /(d: Long) = MilliSatoshi(amount / d) def compare(other: MilliSatoshi): Int = if (amount == other.amount) 0 else if (amount < other.amount) -1 else 1 + def compareUnsigned(other: UInt64): Int = UnsignedLongs.compare(amount, other.toBigInt.longValue()) def <= (that: MilliSatoshi): Boolean = compare(that) <= 0 def >= (that: MilliSatoshi): Boolean = compare(that) >= 0 def < (that: MilliSatoshi): Boolean = compare(that) < 0 diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/wire/LightningMessageTypes.scala b/eclair-core/src/main/scala/fr/acinq/eclair/wire/LightningMessageTypes.scala index 57d3738918..f93c2d25f9 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/wire/LightningMessageTypes.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/wire/LightningMessageTypes.scala @@ -71,7 +71,7 @@ case class OpenChannel(chainHash: ByteVector32, fundingSatoshis: Satoshi, pushMsat: MilliSatoshi, dustLimitSatoshis: Satoshi, - maxHtlcValueInFlightMsat: UInt64, + maxHtlcValueInFlightMsat: UInt64, // this is not MilliSatoshi because it can exceed the total amount of MilliSatoshi channelReserveSatoshis: Satoshi, htlcMinimumMsat: MilliSatoshi, feeratePerKw: Long, @@ -87,7 +87,7 @@ case class OpenChannel(chainHash: ByteVector32, case class AcceptChannel(temporaryChannelId: ByteVector32, dustLimitSatoshis: Satoshi, - maxHtlcValueInFlightMsat: UInt64, + maxHtlcValueInFlightMsat: UInt64, // this is not MilliSatoshi because it can exceed the total amount of MilliSatoshi channelReserveSatoshis: Satoshi, htlcMinimumMsat: MilliSatoshi, minimumDepth: Long, diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/CoinUtilsSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/CoinUtilsSpec.scala index 8e051c2c93..809210524f 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/CoinUtilsSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/CoinUtilsSpec.scala @@ -18,10 +18,21 @@ package fr.acinq.eclair import fr.acinq.bitcoin.{Btc, MilliBtc, Satoshi} import org.scalatest.FunSuite +import scodec.bits._ class CoinUtilsSpec extends FunSuite { + test("use unsigned comparison when comparing millisatoshis to uint64") { + assert(MilliSatoshi(123).compareUnsigned(UInt64(123)) == 0) + assert(MilliSatoshi(1234).compareUnsigned(UInt64(123)) > 0) + assert(MilliSatoshi(123).compareUnsigned(UInt64(1234)) < 0) + assert(MilliSatoshi(123).compareUnsigned(UInt64(hex"ffffffffffffffff")) < 0) + assert(MilliSatoshi(-123).compareUnsigned(UInt64(hex"ffffffffffffffff")) < 0) + assert(MilliSatoshi(Long.MaxValue).compareUnsigned(UInt64(hex"7fffffffffffffff")) == 0) // 7fffffffffffffff == Long.MaxValue + assert(MilliSatoshi(Long.MaxValue).compareUnsigned(UInt64(hex"8000000000000000")) < 0) // 8000000000000000 == Long.MaxValue + 1 + } + test("Convert string amount to the correct BtcAmount") { val am_btc: MilliSatoshi = CoinUtils.convertStringAmountToMsat("1", BtcUnit.code) assert(am_btc == MilliSatoshi(100000000000L)) 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 2300d25e05..1414556559 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 @@ -205,7 +205,7 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods { val initialState = bob.stateData.asInstanceOf[DATA_NORMAL] val add = CMD_ADD_HTLC(MilliSatoshi(151000000), randomBytes32, 400144, TestConstants.emptyOnionPacket, upstream = Left(UUID.randomUUID())) sender.send(bob, add) - val error = HtlcValueTooHighInFlight(channelId(bob), maximum = 150000000, actual = 151000000) + val error = HtlcValueTooHighInFlight(channelId(bob), maximum = 150000000, actual = MilliSatoshi(151000000)) sender.expectMsg(Failure(AddHtlcFailed(channelId(bob), add.paymentHash, error, Local(add.upstream.left.get, Some(sender.ref)), Some(initialState.channelUpdate), Some(add)))) bob2alice.expectNoMsg(200 millis) } @@ -381,7 +381,7 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods { val tx = alice.stateData.asInstanceOf[DATA_NORMAL].commitments.localCommit.publishableTxs.commitTx.tx alice2bob.forward(alice, UpdateAddHtlc(ByteVector32.Zeroes, 0, MilliSatoshi(151000000), randomBytes32, 400144, TestConstants.emptyOnionPacket)) val error = alice2bob.expectMsgType[Error] - assert(new String(error.data.toArray) === HtlcValueTooHighInFlight(channelId(alice), maximum = 150000000, actual = 151000000).getMessage) + assert(new String(error.data.toArray) === HtlcValueTooHighInFlight(channelId(alice), maximum = 150000000, actual = MilliSatoshi(151000000)).getMessage) awaitCond(alice.stateName == CLOSING) // channel should be advertised as down assert(channelUpdateListener.expectMsgType[LocalChannelDown].channelId === alice.stateData.asInstanceOf[DATA_CLOSING].channelId) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/RelayerSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/RelayerSpec.scala index 97db737ee4..ef5a92b363 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/RelayerSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/payment/RelayerSpec.scala @@ -101,7 +101,7 @@ class RelayerSpec extends TestkitBaseClass { // channel returns an error val origin = Relayed(channelId_ab, originHtlcId = 42, amountIn = MilliSatoshi(1100000), amountOut = MilliSatoshi(1000000)) - sender.send(relayer, Status.Failure(AddHtlcFailed(channelId_bc_1, paymentHash, HtlcValueTooHighInFlight(channelId_bc_1, UInt64(1000000000L), UInt64(1516977616L)), origin, Some(channelUpdate_bc_1), originalCommand = Some(fwd1.message)))) + sender.send(relayer, Status.Failure(AddHtlcFailed(channelId_bc_1, paymentHash, HtlcValueTooHighInFlight(channelId_bc_1, UInt64(1000000000L), MilliSatoshi(1516977616L)), origin, Some(channelUpdate_bc_1), originalCommand = Some(fwd1.message)))) // second try val fwd2 = register.expectMsgType[Register.ForwardShortId[CMD_ADD_HTLC]] @@ -109,7 +109,7 @@ class RelayerSpec extends TestkitBaseClass { assert(fwd2.message.upstream === Right(add_ab)) // failure again - sender.send(relayer, Status.Failure(AddHtlcFailed(channelId_bc, paymentHash, HtlcValueTooHighInFlight(channelId_bc, UInt64(1000000000L), UInt64(1516977616L)), origin, Some(channelUpdate_bc), originalCommand = Some(fwd2.message)))) + sender.send(relayer, Status.Failure(AddHtlcFailed(channelId_bc, paymentHash, HtlcValueTooHighInFlight(channelId_bc, UInt64(1000000000L), MilliSatoshi(1516977616L)), origin, Some(channelUpdate_bc), originalCommand = Some(fwd2.message)))) // the relayer should give up val fwdFail = register.expectMsgType[Register.Forward[CMD_FAIL_HTLC]] From 4bff7abe24d3dcaa6d38c6a11f4c9a1e65f3273f Mon Sep 17 00:00:00 2001 From: Andrea Date: Thu, 29 Aug 2019 11:10:50 +0200 Subject: [PATCH 2/2] Use msat postfix notation --- .../fr/acinq/eclair/channel/states/e/NormalStateSpec.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 ed9a996e4e..c66daeedb7 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 @@ -204,7 +204,7 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods { val initialState = bob.stateData.asInstanceOf[DATA_NORMAL] val add = CMD_ADD_HTLC(151000000 msat, randomBytes32, CltvExpiryDelta(144).toCltvExpiry, TestConstants.emptyOnionPacket, upstream = Left(UUID.randomUUID())) sender.send(bob, add) - val error = HtlcValueTooHighInFlight(channelId(bob), maximum = 150000000, actual = MilliSatoshi(151000000)) + val error = HtlcValueTooHighInFlight(channelId(bob), maximum = 150000000, actual = 151000000 msat) sender.expectMsg(Failure(AddHtlcFailed(channelId(bob), add.paymentHash, error, Local(add.upstream.left.get, Some(sender.ref)), Some(initialState.channelUpdate), Some(add)))) bob2alice.expectNoMsg(200 millis) } @@ -380,7 +380,7 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods { val tx = alice.stateData.asInstanceOf[DATA_NORMAL].commitments.localCommit.publishableTxs.commitTx.tx alice2bob.forward(alice, UpdateAddHtlc(ByteVector32.Zeroes, 0, 151000000 msat, randomBytes32, CltvExpiryDelta(144).toCltvExpiry, TestConstants.emptyOnionPacket)) val error = alice2bob.expectMsgType[Error] - assert(new String(error.data.toArray) === HtlcValueTooHighInFlight(channelId(alice), maximum = 150000000, actual = MilliSatoshi(151000000)).getMessage) + assert(new String(error.data.toArray) === HtlcValueTooHighInFlight(channelId(alice), maximum = 150000000, actual = 151000000 msat).getMessage) awaitCond(alice.stateName == CLOSING) // channel should be advertised as down assert(channelUpdateListener.expectMsgType[LocalChannelDown].channelId === alice.stateData.asInstanceOf[DATA_CLOSING].channelId)