Transaction serialization errors: check the output amount before fee errors

[dskloet]

With transaction serialization errors check the output amount before fee errors. Also added a test for it and also improved buildSkipTest by specifying which error to expect and using it for some tests where it wasn't used yet.

[coveralls]

Coverage remained the same at 97.95% when pulling 98aba35 on dskloet:fix/transaction-error-order into b53f249 on bitpay:master.

[braydonf]

Rather than check being an argument, couldn't we pass it more explicitly as part of the opts argument? Nevermind, the option is checked with values true and false.

[braydonf]

When disableMoreOutputThanInput is enabled, I'm getting an error that says: "Fee is too small: expected more than 667 but got -9999900000000 Use Transaction#uncheckedSerialize if you want to skip security checks. See http://bitcore.io/guide/transaction.html#Serialization for more info."

A few reasons why I don't think we need to have this option:

• The transaction will never get into a block, it's not merely non-standard.
• Other checks depend on there being a precondition that "outputs total <= inputs total"

Edit: The only case I can see is if there are more inputs that will be expected to be added later. However in that case, perhaps toObject() can be used instead that will preserve previous output information.

[braydonf]

)); can go on the same column as it( similar to more common });

[dskloet]

Wouldn't there be more output than input with an anyone-can-pay transaction such as Lighthouse uses for assurance contracts? If not, I'm happy to remove the option to disable the check.

[coveralls]

Coverage remained the same at 97.95% when pulling b33f008 on dskloet:fix/transaction-error-order into b53f249 on bitpay:master.

[dskloet]

If we do want to allow disabling the output amount check, it should automatically also disable all the fee errors as the concept of fee is meaningless when the unspent output is negative.
That would also simplify the skip test as we would no longer need to disable the fee errors there.

[dskloet]

Shall I remove the ability to disable the outputs check or shall I disable the fee errors also when the output check is disabled?

[maraoz]

@dskloet I wouldn't remove the ability to disable outputs>=intputs check... in some cases (CoinJoin or assurance contracts, for example), you'll send a totally invalid transaction (with regards to outputs>=inputs). I think your idea of disabling fee checks when that check is disabled is the right way to go

[coveralls]

Coverage increased (+0.0%) to 97.95% when pulling 2ebe521 on dskloet:fix/transaction-error-order into b53f249 on bitpay:master.

[braydonf]

What about the case that disableMoreOutputThanInput is enabled however the outputs are not greater than the inputs and the fees are too large?

[dskloet]

I personally don't worry about it. As soon as output is allowed to be more than input the concept of fee is meaningless. But if you prefer I don't mind adding it.

[braydonf]

I think a better way to handle this is to either check the fees or that the output amount is greater than input: dskloet#1

[coveralls]

Coverage increased (+0.0%) to 97.95% when pulling 35426ee on dskloet:fix/transaction-error-order into b53f249 on bitpay:master.

[braydonf]

We can simplify this check to only run once, as this._getUnspentValue() < 0 is the same as the _hasMoreOutputThanInput() check: https://github.com/dskloet/bitcore/blob/fix/transaction-error-order/lib/transaction/transaction.js#L278

[dskloet]

I'm not sure what you are suggesting. This check here looks very simple to me.
Is _getUnspentValue() expensive that we shouldn't call it more than once?

[braydonf]

We could probably use benchmarking to help in that regard.

[braydonf]

As it stands now _getUnspentValue() is called around five times for the serialization check, would be interesting to see if this problematic with a transaction with many inputs/outputs.

[dskloet]

We could, but that doesn't answer my question. Is the cost of _getUnspentValue() what you were worried about or was it something else? Were you suggesting to merge _hasFeeError() and _hasMoreOutputThanInput() into one method?
I think that would make things more complicated rather than simpler. I just did the previous pull request in order to split all of that to reduce the cyclomatic complexity that lint was complaining about. I'd rather not undo that work.

If you are worried about calling _getUnspentValue() multiple times, I'd rather cache the result than merging several small methods into one big method. The fee checks also call _getUnspentValue() separately so it's not just here and in _hasMoreOutputThanInput().

[braydonf]

What I'm suggesting is something more along the lines of: braydonf@9c55b02

[dskloet]

Undoing my work from 26bd5a8 ?

[dskloet]

When you say "simplify" do you mean "optimize"?

[coveralls]

Coverage decreased (-0.0%) to 97.94% when pulling fc974f5 on dskloet:fix/transaction-error-order into b53f249 on bitpay:master.

[dskloet]

I have no idea why it says the coverage is decreased. Only one lines is not covered and it wasn't covered before either.

[dskloet]

Can we move forward with this pull request?
Is there something I need to do before it is ready?
Is there any other problem?
Braydon, I'm starting to wonder if you have a problem with my coding style since your suggestions basically undo the simplifications I made in my previous pull request.

It would also help if I had a better idea about how all of this works. Are you guys BitPay employees? Is Bitcore your full time job or a side project? Do you require extra time to discuss some pull requests on another channel that I can't see to make sure you agree about it? The whole process seems pretty opaque and contributing would be more fun if I could understand it better. Or maybe there isn't more too it than I can see but that would be good to know as well.

[braydonf]

Yes, we can move forward. Functionally it LGTM.

Only a few thoughts on organization: Reducing the complexity and number of statements for individual functions can then make it tedious to read the operation as a whole, making it more complex. So I think there is a balance.

I think these functions can be simplified together and still be under 15 statements:

• _hasMoreOutputThanInput() and _isInvalidSatoshis() can be a part of getSerializationError()
• _isFeeDifferent(), _isFeeTooLarge(), _isFeeTooSmall() can all be a part of _hasFeeError()

These were separate methods from the beginning, however it could be good to take care of now while we're improving these methods. I think the addition of _hasFeeError was a good one because we can now combine the fee methods together. Thanks for your contributions.

[dskloet]

Can you maybe explain your criteria for simplicity? What is it you find tedious to read specifically? I honestly don't understand how the merging of several small methods into one big method could be considered a simplification.

[maraoz]

LGTM

[braydonf]

Example:

Transaction.prototype.getSerializationError = function(opts) {
   ...
   return this._isInvalidSatoshis() ||
   ...
}

Transaction.prototype._isInvalidSatoshis = function() {
  if (this.invalidSatoshis()) {
    return new errors.Transaction.InvalidSatoshis();
  }
};

Transaction.prototype.invalidSatoshis = function() {
  var invalid = false;
  for (var i = 0; i < this.outputs.length; i++) {
    if (this.outputs[i].invalidSatoshis()) {
      invalid = true;
    }
  }
  return invalid;
};

Would be more concise and with less function calls as:

Transaction.prototype.getSerializationError = function(opts) {
  ...
  if (this.invalidSatoshis()) {
    return new errors.Transaction.InvalidSatoshis();
  }
  ...
}

Transaction.prototype.invalidSatoshis = function() {
  var invalid = false;
  for (var i = 0; i < this.outputs.length; i++) {
    if (this.outputs[i].invalidSatoshis()) {
      invalid = true;
    }
  }
  return invalid;
};

There are similar examples with the other methods mentioned, and related to handling of the method _hasMoreOutputThanInput and the unspent/fee value.

[dskloet]

So you are really asking me to undo my work on #1226. If you didn't like one simple function per error, why didn't you say so on that pull request?

I don't understand what this even has to do with this pull request but I'm not going to undo my own work. If that's what it takes to contribute my spare time to this project then it's not as much fun as I thought.