From 8c5b2c851b79b197e06a018d22fb9f9caa76083b Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Mon, 18 May 2015 22:40:54 +0200 Subject: [PATCH 1/8] When checking for transaction serialization errors, check the output amount before checking fee errors. 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. --- lib/transaction/transaction.js | 4 +- test/transaction/transaction.js | 78 ++++++++++++++++----------------- 2 files changed, 40 insertions(+), 42 deletions(-) diff --git a/lib/transaction/transaction.js b/lib/transaction/transaction.js index 6d1feb6..e5e60a7 100644 --- a/lib/transaction/transaction.js +++ b/lib/transaction/transaction.js @@ -192,10 +192,10 @@ Transaction.prototype.getSerializationError = function(opts) { opts = opts || {}; return this._isInvalidSatoshis() || + this._hasMoreOutputThanInput(opts) || this._hasFeeError(opts) || this._hasDustOutputs(opts) || - this._isMissingSignatures(opts) || - this._hasMoreOutputThanInput(opts); + this._isMissingSignatures(opts); }; Transaction.prototype._isInvalidSatoshis = function() { diff --git a/test/transaction/transaction.js b/test/transaction/transaction.js index 66db3e5..cd0eefa 100644 --- a/test/transaction/transaction.js +++ b/test/transaction/transaction.js @@ -385,22 +385,36 @@ describe('Transaction', function() { return transaction.serialize(); }).to.throw(errors.Transaction.FeeError.Different); }); + it('checks output amount before fee errors', function() { + var transaction = new Transaction(); + transaction.from(simpleUtxoWith1BTC); + transaction + .to(toAddress, 10000000000000) + .change(changeAddress) + .fee(5); + + expect(function() { + return transaction.serialize(); + }).to.throw(errors.Transaction.InvalidOutputAmountSum); + }); describe('skipping checks', function() { - var buildSkipTest = function(builder, check) { + var buildSkipTest = function(builder, check, expectedError, opts) { return function() { var transaction = new Transaction(); transaction.from(simpleUtxoWith1BTC); builder(transaction); - var options = {}; + var options = opts || {}; options[check] = true; expect(function() { return transaction.serialize(options); }).not.to.throw(); + + options[check] = false; expect(function() { - return transaction.serialize(); - }).to.throw(); + return transaction.serialize(options); + }).to.throw(expectedError); }; }; it('can skip the check for too much fee', buildSkipTest( @@ -409,54 +423,38 @@ describe('Transaction', function() { .fee(50000000) .change(changeAddress) .sign(privateKey); - }, 'disableLargeFees')); + }, 'disableLargeFees', errors.Transaction.FeeError.TooLarge)); it('can skip the check for a fee that is too small', buildSkipTest( function(transaction) { return transaction .fee(1) .change(changeAddress) .sign(privateKey); - }, 'disableSmallFees')); + }, 'disableSmallFees', errors.Transaction.FeeError.TooSmall)); it('can skip the check that prevents dust outputs', buildSkipTest( function(transaction) { return transaction .to(toAddress, 100) .change(changeAddress) .sign(privateKey); - }, 'disableDustOutputs')); - it('can skip the check that prevents unsigned outputs', function() { - var transaction = new Transaction(); - transaction.from(simpleUtxoWith1BTC); - transaction.to(toAddress, 10000); - transaction.change(changeAddress); - var options = {}; - options.disableIsFullySigned = true; - expect(function() { - return transaction.serialize(options); - }).not.to.throw(errors.Transaction.MissingSignatures); - expect(function() { - return transaction.serialize(); - }).to.throw(errors.Transaction.MissingSignatures); - }); - it('can skip the check that avoids spending more bitcoins than the inputs for a transaction', function() { - var transaction = new Transaction(); - transaction.from(simpleUtxoWith1BTC); - transaction.to(toAddress, 10000000000000); - transaction.change(changeAddress); - expect(function() { - return transaction.serialize({ - disableSmallFees: true, - disableIsFullySigned: true, - disableMoreOutputThanInput: true - }); - }).not.to.throw(errors.Transaction.InvalidOutputAmountSum); - expect(function() { - return transaction.serialize({ - disableIsFullySigned: true, - disableSmallFees: true - }); - }).to.throw(errors.Transaction.InvalidOutputAmountSum); - }); + }, 'disableDustOutputs', errors.Transaction.DustOutputs)); + it('can skip the check that prevents unsigned outputs', buildSkipTest( + function(transaction) { + return transaction + .to(toAddress, 10000) + .change(changeAddress); + }, 'disableIsFullySigned', errors.Transaction.UnableToVerifySignature)); + it('can skip the check that avoids spending more bitcoins than the inputs for a transaction', buildSkipTest( + function(transaction) { + return transaction + .to(toAddress, 10000000000000) + .change(changeAddress); + }, 'disableMoreOutputThanInput', + errors.Transaction.InvalidOutputAmountSum, + { + 'disableSmallFees': true, + 'disableIsFullySigned': true + })); }); }); From 6729b389443066ec01d596a79cc745ce42a46d3c Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Tue, 19 May 2015 00:10:43 +0200 Subject: [PATCH 2/8] Expect the correct error to be thrown. --- test/transaction/transaction.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/transaction/transaction.js b/test/transaction/transaction.js index cd0eefa..052d7da 100644 --- a/test/transaction/transaction.js +++ b/test/transaction/transaction.js @@ -443,7 +443,7 @@ describe('Transaction', function() { return transaction .to(toAddress, 10000) .change(changeAddress); - }, 'disableIsFullySigned', errors.Transaction.UnableToVerifySignature)); + }, 'disableIsFullySigned', errors.Transaction.MissingSignatures)); it('can skip the check that avoids spending more bitcoins than the inputs for a transaction', buildSkipTest( function(transaction) { return transaction From dc07788e539215261b1a4a1a34cca068f9ad1a8d Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Tue, 19 May 2015 08:58:16 +0200 Subject: [PATCH 3/8] Put )); on a separate line. --- test/transaction/transaction.js | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/test/transaction/transaction.js b/test/transaction/transaction.js index 052d7da..aea6f7e 100644 --- a/test/transaction/transaction.js +++ b/test/transaction/transaction.js @@ -423,38 +423,43 @@ describe('Transaction', function() { .fee(50000000) .change(changeAddress) .sign(privateKey); - }, 'disableLargeFees', errors.Transaction.FeeError.TooLarge)); + }, 'disableLargeFees', errors.Transaction.FeeError.TooLarge + )); it('can skip the check for a fee that is too small', buildSkipTest( function(transaction) { return transaction .fee(1) .change(changeAddress) .sign(privateKey); - }, 'disableSmallFees', errors.Transaction.FeeError.TooSmall)); + }, 'disableSmallFees', errors.Transaction.FeeError.TooSmall + )); it('can skip the check that prevents dust outputs', buildSkipTest( function(transaction) { return transaction .to(toAddress, 100) .change(changeAddress) .sign(privateKey); - }, 'disableDustOutputs', errors.Transaction.DustOutputs)); + }, 'disableDustOutputs', errors.Transaction.DustOutputs + )); it('can skip the check that prevents unsigned outputs', buildSkipTest( function(transaction) { return transaction .to(toAddress, 10000) .change(changeAddress); - }, 'disableIsFullySigned', errors.Transaction.MissingSignatures)); + }, 'disableIsFullySigned', errors.Transaction.MissingSignatures + )); it('can skip the check that avoids spending more bitcoins than the inputs for a transaction', buildSkipTest( function(transaction) { return transaction .to(toAddress, 10000000000000) .change(changeAddress); - }, 'disableMoreOutputThanInput', - errors.Transaction.InvalidOutputAmountSum, - { - 'disableSmallFees': true, - 'disableIsFullySigned': true - })); + }, 'disableMoreOutputThanInput', + errors.Transaction.InvalidOutputAmountSum, + { + 'disableSmallFees': true, + 'disableIsFullySigned': true + } + )); }); }); From ac2fbe27771f467672484dfecd32cb5b36e5661d Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Wed, 20 May 2015 21:56:52 +0200 Subject: [PATCH 4/8] When disableMoreOutputThanInput is set for getSerializationError, also disable the fee checks as the concept of a fee is meaningless when unspent output value is negative. This also allows for removing the opts from buildSkipTest again and simplifying the skip test for disableMoreOutputThanInput. --- lib/transaction/transaction.js | 4 ++++ test/transaction/transaction.js | 18 ++++++------------ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/lib/transaction/transaction.js b/lib/transaction/transaction.js index e5e60a7..dede08a 100644 --- a/lib/transaction/transaction.js +++ b/lib/transaction/transaction.js @@ -205,6 +205,10 @@ Transaction.prototype._isInvalidSatoshis = function() { }; Transaction.prototype._hasFeeError = function(opts) { + if (opts.disableMoreOutputThanInput) { + // The concept of a fee is meaningless when the unspent output value is negative. + return; + } return this._isFeeDifferent() || this._isFeeTooLarge(opts) || this._isFeeTooSmall(opts); diff --git a/test/transaction/transaction.js b/test/transaction/transaction.js index aea6f7e..e445d41 100644 --- a/test/transaction/transaction.js +++ b/test/transaction/transaction.js @@ -398,22 +398,20 @@ describe('Transaction', function() { }).to.throw(errors.Transaction.InvalidOutputAmountSum); }); describe('skipping checks', function() { - var buildSkipTest = function(builder, check, expectedError, opts) { + var buildSkipTest = function(builder, check, expectedError) { return function() { var transaction = new Transaction(); transaction.from(simpleUtxoWith1BTC); builder(transaction); - var options = opts || {}; + var options = {}; options[check] = true; expect(function() { return transaction.serialize(options); }).not.to.throw(); - - options[check] = false; expect(function() { - return transaction.serialize(options); + return transaction.serialize(); }).to.throw(expectedError); }; }; @@ -452,13 +450,9 @@ describe('Transaction', function() { function(transaction) { return transaction .to(toAddress, 10000000000000) - .change(changeAddress); - }, 'disableMoreOutputThanInput', - errors.Transaction.InvalidOutputAmountSum, - { - 'disableSmallFees': true, - 'disableIsFullySigned': true - } + .change(changeAddress) + .sign(privateKey); + }, 'disableMoreOutputThanInput', errors.Transaction.InvalidOutputAmountSum )); }); }); From 3ace170ac5ef1cb91245a004cb05b1dd3c51f127 Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Thu, 21 May 2015 09:01:08 +0200 Subject: [PATCH 5/8] Ignore fee error when unspent output is actually negative, rather than already when the check for negative unspent output is disabled. --- lib/transaction/transaction.js | 2 +- test/transaction/transaction.js | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/transaction/transaction.js b/lib/transaction/transaction.js index dede08a..46983ad 100644 --- a/lib/transaction/transaction.js +++ b/lib/transaction/transaction.js @@ -205,7 +205,7 @@ Transaction.prototype._isInvalidSatoshis = function() { }; Transaction.prototype._hasFeeError = function(opts) { - if (opts.disableMoreOutputThanInput) { + if (this._getUnspentValue() < 0) { // The concept of a fee is meaningless when the unspent output value is negative. return; } diff --git a/test/transaction/transaction.js b/test/transaction/transaction.js index e445d41..673a52f 100644 --- a/test/transaction/transaction.js +++ b/test/transaction/transaction.js @@ -397,6 +397,18 @@ describe('Transaction', function() { return transaction.serialize(); }).to.throw(errors.Transaction.InvalidOutputAmountSum); }); + it('will throw fee error with disableMoreOutputThanInput enabled (but not triggered)', function() { + var transaction = new Transaction(); + transaction.from(simpleUtxoWith1BTC); + transaction + .to(toAddress, 90000000) + .change(changeAddress) + .fee(10000000); + + expect(function() { + return transaction.serialize({disableMoreOutputThanInput: true}); + }).to.throw(errors.Transaction.FeeError.TooLarge); + }); describe('skipping checks', function() { var buildSkipTest = function(builder, check, expectedError) { return function() { From 0b6eaf0f1edab160185110232db7210152df0f2c Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Thu, 21 May 2015 19:15:06 +0200 Subject: [PATCH 6/8] Call getUnspentValue() only once in getSerializationError(). --- lib/transaction/transaction.js | 37 ++++++++++++++++------------------ 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/lib/transaction/transaction.js b/lib/transaction/transaction.js index 46983ad..693e97f 100644 --- a/lib/transaction/transaction.js +++ b/lib/transaction/transaction.js @@ -191,9 +191,10 @@ Transaction.prototype.invalidSatoshis = function() { Transaction.prototype.getSerializationError = function(opts) { opts = opts || {}; + var unspent = this._getUnspentValue(); return this._isInvalidSatoshis() || - this._hasMoreOutputThanInput(opts) || - this._hasFeeError(opts) || + this._hasMoreOutputThanInput(opts, unspent) || + this._hasFeeError(opts, unspent) || this._hasDustOutputs(opts) || this._isMissingSignatures(opts); }; @@ -204,31 +205,28 @@ Transaction.prototype._isInvalidSatoshis = function() { } }; -Transaction.prototype._hasFeeError = function(opts) { - if (this._getUnspentValue() < 0) { +Transaction.prototype._hasFeeError = function(opts, unspent) { + if (unspent < 0) { // The concept of a fee is meaningless when the unspent output value is negative. return; } - return this._isFeeDifferent() || - this._isFeeTooLarge(opts) || - this._isFeeTooSmall(opts); + return this._isFeeDifferent(unspent) || + this._isFeeTooLarge(opts, unspent) || + this._isFeeTooSmall(opts, unspent); }; -Transaction.prototype._isFeeDifferent = function() { - if (!_.isUndefined(this._fee)) { - var fee = this._fee; - var unspent = this._getUnspentValue(); - if (fee !== unspent) { - return new errors.Transaction.FeeError.Different('Unspent value is ' + unspent + ' but specified fee is ' + fee); - } +Transaction.prototype._isFeeDifferent = function(unspent) { + if (!_.isUndefined(this._fee) && this._fee !== unspent) { + return new errors.Transaction.FeeError.Different( + 'Unspent value is ' + unspent + ' but specified fee is ' + this._fee + ); } }; -Transaction.prototype._isFeeTooLarge = function(opts) { +Transaction.prototype._isFeeTooLarge = function(opts, fee) { if (opts.disableLargeFees) { return; } - var fee = this._getUnspentValue(); var maximumFee = Math.floor(Transaction.FEE_SECURITY_MARGIN * this._estimateFee()); if (fee > maximumFee) { if (this._missingChange()) { @@ -238,11 +236,10 @@ Transaction.prototype._isFeeTooLarge = function(opts) { } }; -Transaction.prototype._isFeeTooSmall = function(opts) { +Transaction.prototype._isFeeTooSmall = function(opts, fee) { if (opts.disableSmallFees) { return; } - var fee = this._getUnspentValue(); var minimumFee = Math.ceil(this._estimateFee() / Transaction.FEE_SECURITY_MARGIN); if (fee < minimumFee) { return new errors.Transaction.FeeError.TooSmall('expected more than ' + minimumFee + ' but got ' + fee); @@ -275,11 +272,11 @@ Transaction.prototype._isMissingSignatures = function(opts) { } }; -Transaction.prototype._hasMoreOutputThanInput = function(opts) { +Transaction.prototype._hasMoreOutputThanInput = function(opts, unspent) { if (opts.disableMoreOutputThanInput) { return; } - if (this._getUnspentValue() < 0) { + if (unspent < 0) { return new errors.Transaction.InvalidOutputAmountSum(); } }; From 589d017a14e70b29e6f701a8d17c0205ae9a739e Mon Sep 17 00:00:00 2001 From: Braydon Fuller Date: Sat, 23 May 2015 19:58:22 -0400 Subject: [PATCH 7/8] Refactored transaction.getSerializationError to be more concise. - _hasMoreOutputThanInput() and _isInvalidSatoshis() merged with getSerializationError() - _isFeeDifferent(), _isFeeTooLarge() and _isFeeTooSmall merged with _hasFeeError() --- lib/transaction/transaction.js | 75 ++++++++++++++-------------------- 1 file changed, 31 insertions(+), 44 deletions(-) diff --git a/lib/transaction/transaction.js b/lib/transaction/transaction.js index 693e97f..232dafe 100644 --- a/lib/transaction/transaction.js +++ b/lib/transaction/transaction.js @@ -191,58 +191,54 @@ Transaction.prototype.invalidSatoshis = function() { Transaction.prototype.getSerializationError = function(opts) { opts = opts || {}; - var unspent = this._getUnspentValue(); - return this._isInvalidSatoshis() || - this._hasMoreOutputThanInput(opts, unspent) || - this._hasFeeError(opts, unspent) || - this._hasDustOutputs(opts) || - this._isMissingSignatures(opts); -}; - -Transaction.prototype._isInvalidSatoshis = function() { if (this.invalidSatoshis()) { return new errors.Transaction.InvalidSatoshis(); } -}; -Transaction.prototype._hasFeeError = function(opts, unspent) { + var unspent = this._getUnspentValue(); + var unspentError; if (unspent < 0) { - // The concept of a fee is meaningless when the unspent output value is negative. - return; + if (!opts.disableMoreOutputThanInput) { + unspentError = new errors.Transaction.InvalidOutputAmountSum(); + } + } else { + unspentError = this._hasFeeError(opts, unspent); } - return this._isFeeDifferent(unspent) || - this._isFeeTooLarge(opts, unspent) || - this._isFeeTooSmall(opts, unspent); + + return unspentError || + this._hasDustOutputs(opts) || + this._isMissingSignatures(opts); }; -Transaction.prototype._isFeeDifferent = function(unspent) { +Transaction.prototype._hasFeeError = function(opts, unspent) { + if (!_.isUndefined(this._fee) && this._fee !== unspent) { return new errors.Transaction.FeeError.Different( 'Unspent value is ' + unspent + ' but specified fee is ' + this._fee ); } -}; -Transaction.prototype._isFeeTooLarge = function(opts, fee) { - if (opts.disableLargeFees) { - return; - } - var maximumFee = Math.floor(Transaction.FEE_SECURITY_MARGIN * this._estimateFee()); - if (fee > maximumFee) { - if (this._missingChange()) { - return new errors.Transaction.ChangeAddressMissing('Fee is too large and no change address was provided'); + if (!opts.disableLargeFees) { + var maximumFee = Math.floor(Transaction.FEE_SECURITY_MARGIN * this._estimateFee()); + if (unspent > maximumFee) { + if (this._missingChange()) { + return new errors.Transaction.ChangeAddressMissing( + 'Fee is too large and no change address was provided' + ); + } + return new errors.Transaction.FeeError.TooLarge( + 'expected less than ' + maximumFee + ' but got ' + unspent + ); } - return new errors.Transaction.FeeError.TooLarge('expected less than ' + maximumFee + ' but got ' + fee); } -}; -Transaction.prototype._isFeeTooSmall = function(opts, fee) { - if (opts.disableSmallFees) { - return; - } - var minimumFee = Math.ceil(this._estimateFee() / Transaction.FEE_SECURITY_MARGIN); - if (fee < minimumFee) { - return new errors.Transaction.FeeError.TooSmall('expected more than ' + minimumFee + ' but got ' + fee); + if (!opts.disableSmallFees) { + var minimumFee = Math.ceil(this._estimateFee() / Transaction.FEE_SECURITY_MARGIN); + if (unspent < minimumFee) { + return new errors.Transaction.FeeError.TooSmall( + 'expected more than ' + minimumFee + ' but got ' + unspent + ); + } } }; @@ -272,15 +268,6 @@ Transaction.prototype._isMissingSignatures = function(opts) { } }; -Transaction.prototype._hasMoreOutputThanInput = function(opts, unspent) { - if (opts.disableMoreOutputThanInput) { - return; - } - if (unspent < 0) { - return new errors.Transaction.InvalidOutputAmountSum(); - } -}; - Transaction.prototype.inspect = function() { return ''; }; From ee651df635d4c340821b74e070415e0a521c729a Mon Sep 17 00:00:00 2001 From: Braydon Fuller Date: Sat, 23 May 2015 20:28:33 -0400 Subject: [PATCH 8/8] Added test for null input for transaction.verify() --- test/transaction/transaction.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/transaction/transaction.js b/test/transaction/transaction.js index 673a52f..77ae014 100644 --- a/test/transaction/transaction.js +++ b/test/transaction/transaction.js @@ -520,6 +520,23 @@ describe('Transaction', function() { }); + it('not if has null input (and not coinbase)', function() { + + var tx = new Transaction() + .from({ + 'txId': testPrevTx, + 'outputIndex': 0, + 'script': testScript, + 'satoshis': testAmount + }).to('mrU9pEmAx26HcbKVrABvgL7AwA5fjNFoDc', testAmount - 10000); + + tx.isCoinbase = sinon.stub().returns(false); + tx.inputs[0].isNull = sinon.stub().returns(true); + var verify = tx.verify(); + verify.should.equal('transaction input 0 has null input'); + + }); + }); describe('to and from JSON', function() {