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.

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

Use Long to back the UInt64 type #1109

Merged
merged 10 commits into from
Sep 4, 2019
2 changes: 0 additions & 2 deletions eclair-core/src/main/scala/fr/acinq/eclair/MilliSatoshi.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package fr.acinq.eclair

import com.google.common.primitives.UnsignedLongs
import fr.acinq.bitcoin.{Btc, BtcAmount, MilliBtc, Satoshi, btc2satoshi, millibtc2satoshi}

/**
Expand All @@ -41,7 +40,6 @@ 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
Expand Down
34 changes: 16 additions & 18 deletions eclair-core/src/main/scala/fr/acinq/eclair/UInt64.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,40 +16,38 @@

package fr.acinq.eclair

import java.math.BigInteger

import com.google.common.primitives.UnsignedLongs
import scodec.bits.ByteVector
import scodec.bits.HexStringSyntax

case class UInt64(private val underlying: BigInt) extends Ordered[UInt64] {

require(underlying >= 0, s"uint64 must be positive (actual=$underlying)")
require(underlying <= UInt64.MaxValueBigInt, s"uint64 must be < 2^64 -1 (actual=$underlying)")
case class UInt64(private val underlying: Long) extends Ordered[UInt64] {

override def compare(o: UInt64): Int = underlying.compare(o.underlying)
override def compare(o: UInt64): Int = UnsignedLongs.compare(underlying, o.underlying)
t-bast marked this conversation as resolved.
Show resolved Hide resolved
def compareUnsigned(other: MilliSatoshi) = other.toLong match {
t-bast marked this conversation as resolved.
Show resolved Hide resolved
case l if l < 0 => 1 // if @param 'other' is negative then is always smaller than 'this'
case _ => compare(UInt64(other.toLong))
}

def toByteVector: ByteVector = ByteVector.view(underlying.toByteArray.takeRight(8))
def toByteVector: ByteVector = underlying match {
case 0 => hex"00"
case _ => ByteVector.fromLong(underlying).dropWhile(_ == 0)
Copy link
Member

Choose a reason for hiding this comment

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

The dropWhile shouldn't be necessary anymore

Copy link
Member

Choose a reason for hiding this comment

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

I think we need it, otherwise we will always have something that's 8-bytes long?

Copy link
Member

Choose a reason for hiding this comment

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

Replacing the whole method by:

  def toByteVector: ByteVector = ByteVector.fromLong(underlying)

should work (except that it makes cosmetic tests fail). I think it is simpler that way, do you see a potential problem?

}

def toBigInt: BigInt = underlying
def toBigInt: BigInt = BigInt(signum = 1, toByteVector.toArray)
Copy link
Member

Choose a reason for hiding this comment

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

Feels a bit weird to go through toByteVector for converting to BigInt. Stackoverflow suggests:

Suggested change
def toBigInt: BigInt = BigInt(signum = 1, toByteVector.toArray)
def toBigInt: BigInt = (BigInt(underlying >>> 1) << 1) + (underlying & 1)

source: https://stackoverflow.com/questions/21212993/unsigned-variables-in-scala

Tests pass with this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change have been implemented in 8c60d6a, while a bit less readable i think this solution is elegant and more efficient than going through the byte vector conversion.


override def toString: String = underlying.toString
override def toString: String = UnsignedLongs.toString(underlying, 10)
t-bast marked this conversation as resolved.
Show resolved Hide resolved
}


object UInt64 {

private val MaxValueBigInt = BigInt(new BigInteger("ffffffffffffffff", 16))

val MaxValue = UInt64(MaxValueBigInt)
val MaxValue = UInt64(hex"0xffffffffffffffff")

def apply(bin: ByteVector) = new UInt64(new BigInteger(1, bin.toArray))

def apply(value: Long) = new UInt64(BigInt(value))
def apply(bin: ByteVector): UInt64 = UInt64(bin.toLong(signed = false))

object Conversions {
t-bast marked this conversation as resolved.
Show resolved Hide resolved

implicit def intToUint64(l: Int) = UInt64(l)

implicit def longToUint64(l: Long) = UInt64(l)
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ object Commitments {

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) {
if (commitments1.remoteParams.maxHtlcValueInFlightMsat.compareUnsigned(htlcValueInFlight) < 0) {
araspitzu marked this conversation as resolved.
Show resolved Hide resolved
// TODO: this should be a specific UPDATE error
return Left(HtlcValueTooHighInFlight(commitments.channelId, maximum = commitments1.remoteParams.maxHtlcValueInFlightMsat, actual = htlcValueInFlight))
}
Expand Down Expand Up @@ -184,7 +184,7 @@ object Commitments {

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) {
if (commitments1.localParams.maxHtlcValueInFlightMsat.compareUnsigned(htlcValueInFlight) < 0) {
throw HtlcValueTooHighInFlight(commitments.channelId, maximum = commitments1.localParams.maxHtlcValueInFlightMsat, actual = htlcValueInFlight)
}

Expand Down
12 changes: 0 additions & 12 deletions eclair-core/src/test/scala/fr/acinq/eclair/CoinUtilsSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,9 @@ 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))
Expand Down
35 changes: 32 additions & 3 deletions eclair-core/src/test/scala/fr/acinq/eclair/UInt64Spec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,52 @@ import scodec.bits._

class UInt64Spec extends FunSuite {

test("handle values from 0 to 2^63-1") {
test("handle values from 0 to 2^64-1") {
Copy link
Member

Choose a reason for hiding this comment

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

Nice find!

val a = UInt64(hex"0xffffffffffffffff")
val b = UInt64(hex"0xfffffffffffffffe")
val c = UInt64(42)
val z = UInt64(0)
val l = UInt64(Long.MaxValue)
val l1 = UInt64(hex"8000000000000000") // Long.MaxValue + 1

assert(a > b)
assert(a.toBigInt > b.toBigInt)
assert(b < a)
assert(z < a && z < b && z < c)
assert(b.toBigInt < a.toBigInt)
assert(l.toBigInt < l1.toBigInt)
assert(z < a && z < b && z < c && z < l && c < l && l < l1 && l < b && l < a)
assert(a == a)
assert(a == UInt64.MaxValue)
t-bast marked this conversation as resolved.
Show resolved Hide resolved
assert(l.toByteVector == hex"7fffffffffffffff")
assert(l.toString == Long.MaxValue.toString)
assert(l.toBigInt == BigInt(Long.MaxValue))
assert(l1.toByteVector == hex"8000000000000000")
assert(l1.toString == "9223372036854775808")
assert(l1.toBigInt == BigInt("9223372036854775808"))
assert(a.toByteVector === hex"0xffffffffffffffff")
assert(a.toString === "18446744073709551615")
assert(a.toString === "18446744073709551615") // 2^64 - 1
araspitzu marked this conversation as resolved.
Show resolved Hide resolved
assert(b.toByteVector === hex"0xfffffffffffffffe")
assert(b.toString === "18446744073709551614")
assert(c.toByteVector === hex"0x2a")
assert(c.toString === "42")
assert(z.toByteVector === hex"0x00")
assert(z.toString === "0")
assert(UInt64(hex"0xff").toByteVector == hex"0xff")
assert(UInt64(hex"0x800").toByteVector == hex"0x800")
}

test("use unsigned comparison when comparing millisatoshis to uint64") {
t-bast marked this conversation as resolved.
Show resolved Hide resolved
assert(UInt64(123).compareUnsigned(MilliSatoshi(123)) == 0)
assert(UInt64(123).compareUnsigned(MilliSatoshi(1234)) < 0)
assert(UInt64(1234).compareUnsigned(MilliSatoshi(123)) > 0)
assert(UInt64(hex"ffffffffffffffff").compareUnsigned(MilliSatoshi(123)) > 0)
assert(UInt64(hex"ffffffffffffffff").compareUnsigned(MilliSatoshi(-123)) > 0)
assert(UInt64(hex"7ffffffffffffffe").compareUnsigned(MilliSatoshi(Long.MaxValue)) < 0) // 7ffffffffffffffe == Long.MaxValue - 1
assert(UInt64(hex"7fffffffffffffff").compareUnsigned(MilliSatoshi(Long.MaxValue)) == 0) // 7fffffffffffffff == Long.MaxValue
assert(UInt64(hex"8000000000000000").compareUnsigned(MilliSatoshi(Long.MaxValue)) > 0) // 8000000000000000 == Long.MaxValue + 1
assert(UInt64(1).compareUnsigned(MilliSatoshi(-1)) > 0)
assert(UInt64(0).compareUnsigned(MilliSatoshi(Long.MinValue)) > 0)
}


}