Use unsigned comparison for 'maxHtlcValueInFlightMsat' (#1105)

* Use unsigned comparison for HtlcValueTooHighInFlight

eclair-core/src/main/scala/fr/acinq/eclair/MilliSatoshi.scala

@@ -16,6 +16,7 @@
 
 package fr.acinq.eclair
 
+import com.google.common.primitives.UnsignedLongs
 import fr.acinq.bitcoin.{Btc, BtcAmount, MilliBtc, Satoshi, btc2satoshi, millibtc2satoshi}
 
 /**
@@ -40,6 +41,7 @@ case class MilliSatoshi(private val underlying: Long) extends Ordered[MilliSatos
   override def compare(other: MilliSatoshi): Int = underlying.compareTo(other.underlying)
   // Since BtcAmount is a sealed trait that MilliSatoshi cannot extend, we need to redefine comparison operators.
   def compare(other: BtcAmount): Int = compare(other.toMilliSatoshi)
+  def compareUnsigned(other: UInt64): Int = UnsignedLongs.compare(underlying, other.toBigInt.longValue())
   def <=(other: BtcAmount): Boolean = compare(other) <= 0
   def >=(other: BtcAmount): Boolean = compare(other) >= 0
   def <(other: BtcAmount): Boolean = compare(other) < 0

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: CltvExpiry, actual: CltvExpiry, blockCount: Long) extends ChannelException(channelId, s"expiry too small: minimum=$minimum actual=$actual blockCount=$blockCount")
 case class ExpiryTooBig                        (override val channelId: ByteVector32, maximum: CltvExpiry, actual: CltvExpiry, 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")

eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelTypes.scala

@@ -192,7 +192,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: CltvExpiryDelta,
@@ -204,7 +204,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: CltvExpiryDelta,

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}
 import fr.acinq.eclair.blockchain.fee.{FeeEstimator, FeeTargets}
@@ -145,8 +146,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))
     }
@@ -180,8 +182,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)
     }
 

eclair-core/src/main/scala/fr/acinq/eclair/package.scala

@@ -22,7 +22,6 @@ 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}
 

eclair-core/src/main/scala/fr/acinq/eclair/wire/LightningMessageTypes.scala

@@ -72,7 +72,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,
@@ -88,7 +88,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,

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))

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 = 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 = 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)

eclair-core/src/test/scala/fr/acinq/eclair/payment/RelayerSpec.scala

@@ -102,15 +102,15 @@ class RelayerSpec extends TestkitBaseClass {
 
     // channel returns an error
     val origin = Relayed(channelId_ab, originHtlcId = 42, amountIn = 1100000 msat, amountOut = 1000000 msat)
-    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), 1516977616L msat), origin, Some(channelUpdate_bc_1), originalCommand = Some(fwd1.message))))
 
     // second try
     val fwd2 = register.expectMsgType[Register.ForwardShortId[CMD_ADD_HTLC]]
     assert(fwd2.shortChannelId === channelUpdate_bc.shortChannelId)
     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), 1516977616L msat), origin, Some(channelUpdate_bc), originalCommand = Some(fwd2.message))))
 
     // the relayer should give up
     val fwdFail = register.expectMsgType[Register.Forward[CMD_FAIL_HTLC]]