Rounding error and incorrect fee

[dskloet]

I tried to create a transaction with an output of 81.99 mBTC and a fee of 10 bits. But because 81.99 * 100000 becomes 8198999.999999999 I actually created a transaction with a non-integer amount of satoshis. This should simply have failed (or at most rounded to 81.99000 satoshis) but instead bitcore seems to have truncated it down to 8198999 satoshis because the resulting transaction was:

https://test-insight.bitpay.com/tx/8ed23b2513a8960e78602d2fb5b9e12eb7ba922f63ada6ddc4a75faf44f50e31

Since I didn't specify a change address, this also affected the fee, which became 1001 even though I explicitly specified it to be 1000.

So I see 2 bugs here:

1. A non-integer amount of satoshis was truncated without error.
2. A transaction was serialized with a fee different from the explicitly specified fee.

[maraoz]

Can you show the part of the code where you created the transaction please? We'll investigate

[dskloet]

I did something like

var amount = 81.99 * 100000;
var fee = 1000;
var tx = new bitcore.Transaction().
    from(cachedOutputs).
    to(toAddress, amount).
    fee(fee);

Do you need a full working example?

[maraoz]

Please note that JavaScript has some problems when dealing with different bitcoin units. That's why we built the Unit class (http://bitcore.io/guide/unit.html)
Look:

$ node
> 81.99 * 100000
8198999.999999999
> var bitcore = require('bitcore');
undefined
> var Unit = bitcore.Unit;
undefined
> Unit.fromMilis(81.99).toSatoshis()
8199000

could this be the problem?

[dskloet]

Thanks, good to know about Unit. But yes, I already knew the floating point imprecision was the problem. Did you see my points 1 and 2 at the bottom?

1. Do you agree that it should be an error to specify a non-integer number of satoshis and that it shouldn't just quietly truncate the amount?
2. And do you agree that if a fee is specified, it should never create a transaction with a different fee without error?

[maraoz]

aaah, I missunderstood your two points, sorry. Yes, I agree with your two ideas. I'll move this issue to "ready". Feel free to implement this fixes in a PR, or we'll look at it in the future

[dskloet]

I may try next weekend but it's a bit of a hassle since my employer needs to approve before I can contribute code to an open source project. So don't hold your breath.

[dskloet]

gulp test runs all the tests. Is there a way to run only an individual test?

[maraoz]

In development, you can append .only to an it or describe, like so:

it.only('something', function() {
  // test
});

you can also run just one file with:

sudo gulp install -g mocha
mocha test/somefile.js 

[dskloet]

Where would I write this it.only? And what would I then do to run that test? What exactly do you mean by "in development"?

Is there maybe some guide I should read that explains all of this? I found https://github.com/bitpay/bitcore/blob/master/CONTRIBUTING.md but it only talks about coding style and briefly about pull requests, unless I missed a section.

By the way, I tried Unit but Transaction.to() doesn't actually accept a Unit amount. It passes the Unit to a BN which crashes because it isn't a number.

[dskloet]

What's the best way to get some help contributing? Maybe not on this issue thread?

[maraoz]

Let's chat on IRC, you can find me on #bitcoin-dev, #bitcoin, #bitcoinjs-dev, etc on freenode. Ping me

[dskloet]

IRC doesn't seem to work very well. I seem to have been disconnected so I don't know if you answered my last question.

Can you tell me what your development cycle is? What do you do after making a small change to see if it fixed a particular test? Do you let gulp fire up a bunch of browsers or is there a way where you just reload a single page in an existing browser and run only a few tests?

[maraoz]

you can run a single test with .only, as I told you above:

it.only('something', function() {
  // test
});

If you don't want to run tests using karma, you can try opening the https://github.com/bitpay/bitcore/blob/master/test/index.html in your browser (npm install mocha first) and refresh it each time you want to test. I find it more comfortable to use a cli, though.

[braydonf]

Part 1 should be fixed in: 3005e19

[braydonf]

Part 2 fixed in #1214

[dskloet]

Thanks dskloet, for contributing this fix!

You're welcome!

[braydonf]

Thanks for the contribution :)

[dskloet]

I enjoyed it :). Hopefully I can do more in the future.

[maraoz]

Great work @dskloet !