search

btcsuite / btcutil

Provides bitcoin-specific convenience functions and types
477 stars 410 forks source link

btcutil.NewAmount can overflow #71

Closed jrick closed 8 years ago

jrick commented 8 years ago

Multiplying the float64 input by 1e8 can result in the value overflowing the maximum value representable by an int64, causing truncation during the type conversion. This needs to be detected and an error should be returned.

jrick commented 8 years ago

Considering closing this, and instead changing the API so that it validates that the floating point value does not exceed [-21e14, 21e14].

While the overflow checks could be done (I've already determined the min/max constants), it is still dangerous to use floating point values this large or small because the next largest/smallest floating point values might result in a 2 satoshi difference. Inherit to floating points are losses of precision, and it would be improper to convert values such as these to an atomic Amount, even if it doesn't over/underflow the int64 data type.

If this change is made, I think it would be beneficial to introduce a new API which can convert string representations of floating point values to Amount, instead of lossy floats.

cjepson commented 8 years ago

Totally in agreement with the new API to handle floats. I think the string representation should be 0-padded to ensure the mantissa is 8-decimals, then then pop off the decimal as cast with Atoi.