From 16dc489b082680842b74621277fa243c3e9806df Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Sat, 9 May 2015 18:44:26 +0200 Subject: [PATCH 1/5] Make sure a specified transaction fee and outputs add up to the sum of the inputs. Don't ignore the fee when it's explicitly specified. --- lib/transaction/transaction.js | 28 +++++++++++++++++++++++----- test/transaction/transaction.js | 10 ++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/lib/transaction/transaction.js b/lib/transaction/transaction.js index 8749355..145f8b0 100644 --- a/lib/transaction/transaction.js +++ b/lib/transaction/transaction.js @@ -196,11 +196,15 @@ Transaction.prototype.getSerializationError = function(opts) { return new errors.Transaction.InvalidSatoshis(); } + var feeIsDifferent = this._isFeeDifferent(); var missingChange = this._missingChange(); var feeIsTooLarge = this._isFeeTooLarge(); var feeIsTooSmall = this._isFeeTooSmall(); var isFullySigned = this.isFullySigned(); + if (!opts.disableDifferentFees && feeIsDifferent) { + return new errors.Transaction.FeeError(feeIsDifferent); + } if (!opts.disableLargeFees && feeIsTooLarge) { if (missingChange) { return new errors.Transaction.ChangeAddressMissing('Fee is too large and no change address was provided'); @@ -221,8 +225,17 @@ Transaction.prototype.getSerializationError = function(opts) { } }; +Transaction.prototype._isFeeDifferent = function() { + var fee = this.getFee(); + var unspent = this._getUnspentValue(); + if (fee !== unspent) { + return 'Unspent value ' + unspent + ' is different from specified fee ' + + fee + ' and no change address is specified'; + } +}; + Transaction.prototype._isFeeTooLarge = function() { - var fee = this._getUnspentValue(); + var fee = this.getFee(); var maximumFee = Math.floor(Transaction.FEE_SECURITY_MARGIN * this._estimateFee()); if (fee > maximumFee) { return 'Fee is too large: expected less than ' + maximumFee + ' but got ' + fee; @@ -230,7 +243,7 @@ Transaction.prototype._isFeeTooLarge = function() { }; Transaction.prototype._isFeeTooSmall = function() { - var fee = this._getUnspentValue(); + var fee = this.getFee(); var minimumFee = Math.ceil(this._estimateFee() / Transaction.FEE_SECURITY_MARGIN); if (fee < minimumFee) { return 'Fee is too small: expected more than ' + minimumFee + ' but got ' + fee; @@ -766,6 +779,8 @@ Transaction.prototype._updateChangeOutput = function() { /** * Calculates the fee of the transaction. * + * If there's a fixed fee set, return that. + * * If there is no change output set, the fee is the * total value of the outputs minus inputs. Note that * a serialized transaction only specifies the value @@ -774,17 +789,20 @@ Transaction.prototype._updateChangeOutput = function() { * This method therefore raises a "MissingPreviousOutput" * error when called on a serialized transaction. * - * If there's a fixed fee set, return that. - * If there's no fee set, estimate it based on size. + * If there's no fee set and no change address, + * estimate the fee based on size. * * @return {Number} fee of this transaction in satoshis */ Transaction.prototype.getFee = function() { + if (!_.isUndefined(this._fee)) { + return this._fee; + } // if no change output is set, fees should equal all the unspent amount if (!this._changeScript) { return this._getUnspentValue(); } - return _.isUndefined(this._fee) ? this._estimateFee() : this._fee; + return this._estimateFee(); }; /** diff --git a/test/transaction/transaction.js b/test/transaction/transaction.js index 7841f8a..f3ffbb3 100644 --- a/test/transaction/transaction.js +++ b/test/transaction/transaction.js @@ -364,6 +364,16 @@ describe('Transaction', function() { return transaction.serialize(); }).to.not.throw(errors.Transaction.DustOutputs); }); + it('fails when outputs and fee don\'t add to total input', function() { + var transaction = new Transaction() + .from(simpleUtxoWith1BTC) + .to(toAddress, 99900000) + .fee(99999) + .sign(privateKey); + expect(function() { + return transaction.serialize(); + }).to.throw(errors.Transaction.FeeError); + }); describe('skipping checks', function() { var buildSkipTest = function(builder, check) { return function() { From d1eb190626a756bd206e3aa150aae3fd25440dbe Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Sat, 9 May 2015 21:06:40 +0200 Subject: [PATCH 2/5] Introduce different kinds of FeeError to distinguish the different cases. Fix the issue uncovered by this, which is that getFee might not be the actual fee, but only an estimate, if a change address is specified but there isn't enough to pay a fee and have change. --- lib/errors/spec.js | 10 ++++++++-- lib/transaction/transaction.js | 25 +++++++++++++------------ test/transaction/transaction.js | 6 +++--- 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/lib/errors/spec.js b/lib/errors/spec.js index 81a3e2f..c7adece 100644 --- a/lib/errors/spec.js +++ b/lib/errors/spec.js @@ -85,8 +85,14 @@ module.exports = [{ name: 'InvalidSatoshis', message: 'Output satoshis are invalid', }, { - name: 'FeeError', - message: 'Fees are not correctly set {0}', + name: 'SmallFeeError', + message: 'Fee is too small: {0}', + }, { + name: 'LargeFeeError', + message: 'Fee is too large: {0}', + }, { + name: 'DifferentFeeError', + message: 'Unspent value is different from specified fee: {0}', }, { name: 'ChangeAddressMissing', message: 'Change address is missing' diff --git a/lib/transaction/transaction.js b/lib/transaction/transaction.js index 145f8b0..8729557 100644 --- a/lib/transaction/transaction.js +++ b/lib/transaction/transaction.js @@ -203,16 +203,16 @@ Transaction.prototype.getSerializationError = function(opts) { var isFullySigned = this.isFullySigned(); if (!opts.disableDifferentFees && feeIsDifferent) { - return new errors.Transaction.FeeError(feeIsDifferent); + return new errors.Transaction.DifferentFeeError(feeIsDifferent); } if (!opts.disableLargeFees && feeIsTooLarge) { if (missingChange) { return new errors.Transaction.ChangeAddressMissing('Fee is too large and no change address was provided'); } - return new errors.Transaction.FeeError(feeIsTooLarge); + return new errors.Transaction.LargeFeeError(feeIsTooLarge); } if (!opts.disableSmallFees && feeIsTooSmall) { - return new errors.Transaction.FeeError(feeIsTooSmall); + return new errors.Transaction.SmallFeeError(feeIsTooSmall); } if (!opts.disableDustOutputs && this._hasDustOutputs()) { return new errors.Transaction.DustOutputs(); @@ -226,27 +226,28 @@ Transaction.prototype.getSerializationError = function(opts) { }; Transaction.prototype._isFeeDifferent = function() { - var fee = this.getFee(); - var unspent = this._getUnspentValue(); - if (fee !== unspent) { - return 'Unspent value ' + unspent + ' is different from specified fee ' + - fee + ' and no change address is specified'; + if (!_.isUndefined(this._fee)) { + var fee = this._fee; + var unspent = this._getUnspentValue(); + if (fee !== unspent) { + return 'Unspent value is ' + unspent + ' but specified fee is ' + fee; + } } }; Transaction.prototype._isFeeTooLarge = function() { - var fee = this.getFee(); + var fee = this._getUnspentValue(); var maximumFee = Math.floor(Transaction.FEE_SECURITY_MARGIN * this._estimateFee()); if (fee > maximumFee) { - return 'Fee is too large: expected less than ' + maximumFee + ' but got ' + fee; + return 'expected less than ' + maximumFee + ' but got ' + fee; } }; Transaction.prototype._isFeeTooSmall = function() { - var fee = this.getFee(); + var fee = this._getUnspentValue(); var minimumFee = Math.ceil(this._estimateFee() / Transaction.FEE_SECURITY_MARGIN); if (fee < minimumFee) { - return 'Fee is too small: expected more than ' + minimumFee + ' but got ' + fee; + return 'expected more than ' + minimumFee + ' but got ' + fee; } }; diff --git a/test/transaction/transaction.js b/test/transaction/transaction.js index f3ffbb3..cf8ff21 100644 --- a/test/transaction/transaction.js +++ b/test/transaction/transaction.js @@ -266,7 +266,7 @@ describe('Transaction', function() { .sign(privateKey); expect(function() { return transaction.serialize(); - }).to.throw(errors.Transaction.FeeError); + }).to.throw(errors.Transaction.SmallFeeError); }); it('on second call to sign, change is not recalculated', function() { var transaction = new Transaction() @@ -332,7 +332,7 @@ describe('Transaction', function() { .to(toAddress, 40000000); expect(function() { return transaction.serialize(); - }).to.throw(errors.Transaction.FeeError); + }).to.throw(errors.Transaction.LargeFeeError); }); it('fails if a dust output is created', function() { var transaction = new Transaction() @@ -372,7 +372,7 @@ describe('Transaction', function() { .sign(privateKey); expect(function() { return transaction.serialize(); - }).to.throw(errors.Transaction.FeeError); + }).to.throw(errors.Transaction.DifferentFeeError); }); describe('skipping checks', function() { var buildSkipTest = function(builder, check) { From 8da9c4a44a98e409a1e6362919c6d104e8a21f75 Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Sun, 10 May 2015 00:32:27 +0200 Subject: [PATCH 3/5] Give the 3 fee errors a common parent error. --- lib/errors/spec.js | 20 ++++++++++++-------- lib/transaction/transaction.js | 6 +++--- test/transaction/transaction.js | 6 +++--- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/lib/errors/spec.js b/lib/errors/spec.js index c7adece..03eabc4 100644 --- a/lib/errors/spec.js +++ b/lib/errors/spec.js @@ -85,14 +85,18 @@ module.exports = [{ name: 'InvalidSatoshis', message: 'Output satoshis are invalid', }, { - name: 'SmallFeeError', - message: 'Fee is too small: {0}', - }, { - name: 'LargeFeeError', - message: 'Fee is too large: {0}', - }, { - name: 'DifferentFeeError', - message: 'Unspent value is different from specified fee: {0}', + name: 'Fee', + message: 'Internal Error on Fee {0}', + errors: [{ + name: 'TooSmallError', + message: 'Fee is too small: {0}', + }, { + name: 'TooLargeError', + message: 'Fee is too large: {0}', + }, { + name: 'DifferentError', + message: 'Unspent value is different from specified fee: {0}', + }] }, { name: 'ChangeAddressMissing', message: 'Change address is missing' diff --git a/lib/transaction/transaction.js b/lib/transaction/transaction.js index 8729557..9890cf9 100644 --- a/lib/transaction/transaction.js +++ b/lib/transaction/transaction.js @@ -203,16 +203,16 @@ Transaction.prototype.getSerializationError = function(opts) { var isFullySigned = this.isFullySigned(); if (!opts.disableDifferentFees && feeIsDifferent) { - return new errors.Transaction.DifferentFeeError(feeIsDifferent); + return new errors.Transaction.Fee.DifferentError(feeIsDifferent); } if (!opts.disableLargeFees && feeIsTooLarge) { if (missingChange) { return new errors.Transaction.ChangeAddressMissing('Fee is too large and no change address was provided'); } - return new errors.Transaction.LargeFeeError(feeIsTooLarge); + return new errors.Transaction.Fee.TooLargeError(feeIsTooLarge); } if (!opts.disableSmallFees && feeIsTooSmall) { - return new errors.Transaction.SmallFeeError(feeIsTooSmall); + return new errors.Transaction.Fee.TooSmallError(feeIsTooSmall); } if (!opts.disableDustOutputs && this._hasDustOutputs()) { return new errors.Transaction.DustOutputs(); diff --git a/test/transaction/transaction.js b/test/transaction/transaction.js index cf8ff21..18c8e71 100644 --- a/test/transaction/transaction.js +++ b/test/transaction/transaction.js @@ -266,7 +266,7 @@ describe('Transaction', function() { .sign(privateKey); expect(function() { return transaction.serialize(); - }).to.throw(errors.Transaction.SmallFeeError); + }).to.throw(errors.Transaction.Fee.TooSmallError); }); it('on second call to sign, change is not recalculated', function() { var transaction = new Transaction() @@ -332,7 +332,7 @@ describe('Transaction', function() { .to(toAddress, 40000000); expect(function() { return transaction.serialize(); - }).to.throw(errors.Transaction.LargeFeeError); + }).to.throw(errors.Transaction.Fee.TooLargeError); }); it('fails if a dust output is created', function() { var transaction = new Transaction() @@ -372,7 +372,7 @@ describe('Transaction', function() { .sign(privateKey); expect(function() { return transaction.serialize(); - }).to.throw(errors.Transaction.DifferentFeeError); + }).to.throw(errors.Transaction.Fee.DifferentError); }); describe('skipping checks', function() { var buildSkipTest = function(builder, check) { From b1e54101d35b0d4faba38582c8f590c2f1974bb2 Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Sun, 10 May 2015 00:38:56 +0200 Subject: [PATCH 4/5] Call the parent error of the fee errors FeeError for backwards compatibility. --- lib/errors/spec.js | 8 ++++---- lib/transaction/transaction.js | 6 +++--- test/transaction/transaction.js | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/errors/spec.js b/lib/errors/spec.js index 03eabc4..dd46d6f 100644 --- a/lib/errors/spec.js +++ b/lib/errors/spec.js @@ -85,16 +85,16 @@ module.exports = [{ name: 'InvalidSatoshis', message: 'Output satoshis are invalid', }, { - name: 'Fee', + name: 'FeeError', message: 'Internal Error on Fee {0}', errors: [{ - name: 'TooSmallError', + name: 'TooSmall', message: 'Fee is too small: {0}', }, { - name: 'TooLargeError', + name: 'TooLarge', message: 'Fee is too large: {0}', }, { - name: 'DifferentError', + name: 'Different', message: 'Unspent value is different from specified fee: {0}', }] }, { diff --git a/lib/transaction/transaction.js b/lib/transaction/transaction.js index 9890cf9..47c8ff5 100644 --- a/lib/transaction/transaction.js +++ b/lib/transaction/transaction.js @@ -203,16 +203,16 @@ Transaction.prototype.getSerializationError = function(opts) { var isFullySigned = this.isFullySigned(); if (!opts.disableDifferentFees && feeIsDifferent) { - return new errors.Transaction.Fee.DifferentError(feeIsDifferent); + return new errors.Transaction.FeeError.Different(feeIsDifferent); } if (!opts.disableLargeFees && feeIsTooLarge) { if (missingChange) { return new errors.Transaction.ChangeAddressMissing('Fee is too large and no change address was provided'); } - return new errors.Transaction.Fee.TooLargeError(feeIsTooLarge); + return new errors.Transaction.FeeError.TooLarge(feeIsTooLarge); } if (!opts.disableSmallFees && feeIsTooSmall) { - return new errors.Transaction.Fee.TooSmallError(feeIsTooSmall); + return new errors.Transaction.FeeError.TooSmall(feeIsTooSmall); } if (!opts.disableDustOutputs && this._hasDustOutputs()) { return new errors.Transaction.DustOutputs(); diff --git a/test/transaction/transaction.js b/test/transaction/transaction.js index 18c8e71..dd38f8f 100644 --- a/test/transaction/transaction.js +++ b/test/transaction/transaction.js @@ -266,7 +266,7 @@ describe('Transaction', function() { .sign(privateKey); expect(function() { return transaction.serialize(); - }).to.throw(errors.Transaction.Fee.TooSmallError); + }).to.throw(errors.Transaction.FeeError.TooSmall); }); it('on second call to sign, change is not recalculated', function() { var transaction = new Transaction() @@ -332,7 +332,7 @@ describe('Transaction', function() { .to(toAddress, 40000000); expect(function() { return transaction.serialize(); - }).to.throw(errors.Transaction.Fee.TooLargeError); + }).to.throw(errors.Transaction.FeeError.TooLarge); }); it('fails if a dust output is created', function() { var transaction = new Transaction() @@ -372,7 +372,7 @@ describe('Transaction', function() { .sign(privateKey); expect(function() { return transaction.serialize(); - }).to.throw(errors.Transaction.Fee.DifferentError); + }).to.throw(errors.Transaction.FeeError.Different); }); describe('skipping checks', function() { var buildSkipTest = function(builder, check) { From 056f171e22eed972792e506df95ae80196106564 Mon Sep 17 00:00:00 2001 From: David de Kloet Date: Sun, 10 May 2015 01:32:28 +0200 Subject: [PATCH 5/5] Remove the ability to disable the check that a specified fee is equal to the unspent value. --- lib/transaction/transaction.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/transaction/transaction.js b/lib/transaction/transaction.js index 47c8ff5..4d56e67 100644 --- a/lib/transaction/transaction.js +++ b/lib/transaction/transaction.js @@ -197,14 +197,15 @@ Transaction.prototype.getSerializationError = function(opts) { } var feeIsDifferent = this._isFeeDifferent(); + if (feeIsDifferent) { + return new errors.Transaction.FeeError.Different(feeIsDifferent); + } + var missingChange = this._missingChange(); var feeIsTooLarge = this._isFeeTooLarge(); var feeIsTooSmall = this._isFeeTooSmall(); var isFullySigned = this.isFullySigned(); - if (!opts.disableDifferentFees && feeIsDifferent) { - return new errors.Transaction.FeeError.Different(feeIsDifferent); - } if (!opts.disableLargeFees && feeIsTooLarge) { if (missingChange) { return new errors.Transaction.ChangeAddressMissing('Fee is too large and no change address was provided');