From 50e9a09a8c572b45f68f7e3361d4a41aeb740761 Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Tue, 20 May 2014 15:11:17 +1000 Subject: [PATCH 01/11] Wallet: cleanup createTx control flow Unknowingly this also revealed a subtle bug in the previous implementation which allowed the creation of transactions even when no UTXOs existed. --- src/wallet.js | 49 +++++++++++++++++++++---------------------------- test/wallet.js | 18 ++++++++++++++---- 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/src/wallet.js b/src/wallet.js index cb5963e..e29409d 100644 --- a/src/wallet.js +++ b/src/wallet.js @@ -179,34 +179,40 @@ function Wallet(seed, options) { } this.createTx = function(to, value, fixedFee, changeAddress) { - checkDust(value) + if (isDust(value)) throw new Error("Value must be above dust threshold") + + var utxos = getCandidateOutputs(value) + var accum = 0 + var subTotal = value var tx = new Transaction() tx.addOutput(to, value) - var utxo = getCandidateOutputs(value) - var totalInValue = 0 - for(var i=0; i 0 && !isDust(change)) { - tx.addOutput(changeAddress || getChangeAddress(), change) + subTotal = value + fee + if (accum >= subTotal) { + var change = accum - subTotal + + if (!isDust(change)) { + tx.addOutput(changeAddress || getChangeAddress(), change) + } + + break } - break } - checkInsufficientFund(totalInValue, value, fee) + if (accum < subTotal) { + throw new Error('Not enough funds: ' + accum + ' < ' + subTotal) + } this.sign(tx) - return tx } @@ -232,12 +238,6 @@ function Wallet(seed, options) { return amount <= me.dustThreshold } - function checkDust(value){ - if (isNullOrUndefined(value) || isDust(value)) { - throw new Error("Value must be above dust threshold") - } - } - function getCandidateOutputs(value){ var unspent = [] for (var key in me.outputs){ @@ -263,13 +263,6 @@ function Wallet(seed, options) { return me.changeAddresses[me.changeAddresses.length - 1] } - function checkInsufficientFund(totalInValue, value, fee) { - if(totalInValue < value + fee) { - throw new Error('Not enough money to send funds including transaction fee. Have: ' + - totalInValue + ', needed: ' + (value + fee)) - } - } - this.sign = function(tx) { tx.ins.forEach(function(inp,i) { var output = me.outputs[inp.outpoint.hash + ':' + inp.outpoint.index] diff --git a/test/wallet.js b/test/wallet.js index 567bfd4..c91e422 100644 --- a/test/wallet.js +++ b/test/wallet.js @@ -492,13 +492,23 @@ describe('Wallet', function() { describe('testnet', function(){ it('should create transaction', function(){ - var to = 'mt7MyTVVEWnbwpF5hBn6fgnJcv95Syk2ue' var wallet = new Wallet(seed, {network: 'testnet'}) - var tx = wallet.createTx(to, value) + var address = wallet.generateAddress() + wallet.setUnspentOutputs([{ + hash: fakeTxHash(0), + outputIndex: 0, + address: address, + value: value + }]) + + var to = 'mt7MyTVVEWnbwpF5hBn6fgnJcv95Syk2ue' + var toValue = value - 20000 + + var tx = wallet.createTx(to, toValue) assert.equal(tx.outs.length, 1) assert.equal(tx.outs[0].address.toString(), to) - assert.equal(tx.outs[0].value, value) + assert.equal(tx.outs[0].value, toValue) }) }) @@ -605,7 +615,7 @@ describe('Wallet', function() { assert.throws(function() { wallet.createTx(to, value) - }, /Not enough money to send funds including transaction fee. Have: 1420000, needed: 1420001/) + }, /Not enough funds: 1420000 < 1420001/) }) }) }) From bd3690bdc032aa88dbc624de8ffa795760e90ce5 Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Tue, 20 May 2014 16:11:01 +1000 Subject: [PATCH 02/11] Wallet: remove async interface --- src/wallet.js | 27 ---------------- test/wallet.js | 83 -------------------------------------------------- 2 files changed, 110 deletions(-) diff --git a/src/wallet.js b/src/wallet.js index e29409d..22e3c73 100644 --- a/src/wallet.js +++ b/src/wallet.js @@ -84,17 +84,6 @@ function Wallet(seed, options) { this.outputs = outputs } - this.setUnspentOutputsAsync = function(utxo, callback) { - var error = null - try { - this.setUnspentOutputs(utxo) - } catch(err) { - error = err - } finally { - process.nextTick(function(){ callback(error) }) - } - } - function outputToUnspentOutput(output){ var hashAndIndex = output.receive.split(":") @@ -216,22 +205,6 @@ function Wallet(seed, options) { return tx } - this.createTxAsync = function(to, value, fixedFee, callback){ - if(fixedFee instanceof Function) { - callback = fixedFee - fixedFee = undefined - } - var tx = null - var error = null - - try { - tx = this.createTx(to, value, fixedFee) - } catch(err) { - error = err - } finally { - process.nextTick(function(){ callback(error, tx) }) - } - } this.dustThreshold = 5430 function isDust(amount) { diff --git a/test/wallet.js b/test/wallet.js index c91e422..64ddb8f 100644 --- a/test/wallet.js +++ b/test/wallet.js @@ -277,38 +277,6 @@ describe('Wallet', function() { assert.equal(output.address, utxo[0].address) } }) - - describe('setUnspentOutputsAsync', function(){ - var utxo - beforeEach(function(){ - utxo = cloneObject([expectedUtxo]) - }) - - afterEach(function(){ - wallet.setUnspentOutputs.restore() - }) - - it('calls setUnspentOutputs', function(done){ - sinon.stub(wallet, "setUnspentOutputs") - - var callback = function(){ - assert(wallet.setUnspentOutputs.calledWith(utxo)) - done() - } - - wallet.setUnspentOutputsAsync(utxo, callback) - }) - - it('when setUnspentOutputs throws an error, it invokes callback with error', function(done){ - sinon.stub(wallet, "setUnspentOutputs").throws() - - var callback = function(err){ - assert(err instanceof Error) - done() - } - wallet.setUnspentOutputsAsync(utxo, callback) - }) - }) }) describe('processTx', function(){ @@ -620,57 +588,6 @@ describe('Wallet', function() { }) }) - describe('createTxAsync', function(){ - var to, value, fee - - beforeEach(function(){ - to = '15mMHKL96tWAUtqF3tbVf99Z8arcmnJrr3' - value = 500000 - fee = 10000 - }) - - afterEach(function(){ - wallet.createTx.restore() - }) - - it('calls createTx', function(done){ - sinon.stub(wallet, "createTx").returns("fakeTx") - - var callback = function(err, tx){ - assert(wallet.createTx.calledWith(to, value)) - assert.equal(err, null) - assert.equal(tx, "fakeTx") - done() - } - - wallet.createTxAsync(to, value, callback) - }) - - it('calls createTx correctly when fee is specified', function(done){ - sinon.stub(wallet, "createTx").returns("fakeTx") - - var callback = function(err, tx){ - assert(wallet.createTx.calledWith(to, value, fee)) - assert.equal(err, null) - assert.equal(tx, "fakeTx") - done() - } - - wallet.createTxAsync(to, value, fee, callback) - }) - - it('when createTx throws an error, it invokes callback with error', function(done){ - sinon.stub(wallet, "createTx").throws() - - var callback = function(err, tx){ - assert(err instanceof Error) - done() - } - - wallet.createTxAsync(to, value, callback) - }) - }) - function assertEqual(obj1, obj2){ assert.equal(obj1.toString(), obj2.toString()) } From 4afdbc9f6820b24fc3a6c5dc58339682a8e5c19a Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Tue, 20 May 2014 16:17:44 +1000 Subject: [PATCH 03/11] Wallet: use dustThreshold directly The definition of a dust amount is pretty clear, and I feel it is less readable when represented as isDust(amount) or !isDust(amount), by comparison to amount <= dustThreshold or amount > dustThreshold. Also means I don't have to stray my eyes to understand the implemention by looking up isDust does. --- src/wallet.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/wallet.js b/src/wallet.js index 22e3c73..b7fe55e 100644 --- a/src/wallet.js +++ b/src/wallet.js @@ -22,6 +22,9 @@ function Wallet(seed, options) { this.addresses = [] this.changeAddresses = [] + // Dust value + this.dustThreshold = 5430 + // Transaction output data this.outputs = {} @@ -168,7 +171,7 @@ function Wallet(seed, options) { } this.createTx = function(to, value, fixedFee, changeAddress) { - if (isDust(value)) throw new Error("Value must be above dust threshold") + if (value <= this.dustThreshold) throw new Error("Value must be above dust threshold") var utxos = getCandidateOutputs(value) var accum = 0 @@ -189,7 +192,7 @@ function Wallet(seed, options) { if (accum >= subTotal) { var change = accum - subTotal - if (!isDust(change)) { + if (change > this.dustThreshold) { tx.addOutput(changeAddress || getChangeAddress(), change) } @@ -205,12 +208,6 @@ function Wallet(seed, options) { return tx } - - this.dustThreshold = 5430 - function isDust(amount) { - return amount <= me.dustThreshold - } - function getCandidateOutputs(value){ var unspent = [] for (var key in me.outputs){ From 2df790e2ab9a13a46a84c6c9701803d6c92375bb Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Wed, 21 May 2014 13:18:33 +1000 Subject: [PATCH 04/11] Wallet: remove use of hashLittleEndian --- src/wallet.js | 15 +++++++-------- test/wallet.js | 28 ++-------------------------- 2 files changed, 9 insertions(+), 34 deletions(-) diff --git a/src/wallet.js b/src/wallet.js index b7fe55e..206bf44 100644 --- a/src/wallet.js +++ b/src/wallet.js @@ -1,5 +1,4 @@ var Address = require('./address') -var convert = require('./convert') var HDNode = require('./hdwallet.js') var networks = require('./networks') var rng = require('secure-random') @@ -92,7 +91,6 @@ function Wallet(seed, options) { return { hash: hashAndIndex[0], - hashLittleEndian: convert.reverseEndian(hashAndIndex[0]), outputIndex: parseInt(hashAndIndex[1]), address: output.address, value: output.value @@ -100,7 +98,7 @@ function Wallet(seed, options) { } function unspentOutputToOutput(o) { - var hash = o.hash || convert.reverseEndian(o.hashLittleEndian) + var hash = o.hash var key = hash + ":" + o.outputIndex return { receive: key, @@ -112,8 +110,8 @@ function Wallet(seed, options) { function validateUnspentOutput(uo) { var missingField - if (isNullOrUndefined(uo.hash) && isNullOrUndefined(uo.hashLittleEndian)) { - missingField = "hash(or hashLittleEndian)" + if (isNullOrUndefined(uo.hash)) { + missingField = "hash" } var requiredKeys = ['outputIndex', 'address', 'value'] @@ -129,7 +127,7 @@ function Wallet(seed, options) { 'A valid unspent output must contain' ] message.push(requiredKeys.join(', ')) - message.push("and hash(or hashLittleEndian)") + message.push("and hash") throw new Error(message.join(' ')) } } @@ -163,9 +161,10 @@ function Wallet(seed, options) { tx.ins.forEach(function(txIn, i){ var op = txIn.outpoint - var o = me.outputs[op.hash+':'+op.index] + + var o = me.outputs[op.hash + ':' + op.index] if (o) { - o.spend = txhash + ':' +i + o.spend = txhash + ':' + i } }) } diff --git a/test/wallet.js b/test/wallet.js index 64ddb8f..b5187d1 100644 --- a/test/wallet.js +++ b/test/wallet.js @@ -168,7 +168,6 @@ describe('Wallet', function() { beforeEach(function(){ expectedUtxo = { "hash":"6a4062273ac4f9ea4ffca52d9fd102b08f6c32faa0a4d1318e3a7b2e437bb9c7", - "hashLittleEndian":"c7b97b432e7b3a8e31d1a4a0fa326c8fb002d19f2da5fc4feaf9c43a2762406a", "outputIndex": 0, "address" : "1AZpKpcfCzKDUeTFBQUL4MokQai3m3HMXv", "value": 20000 @@ -230,36 +229,13 @@ describe('Wallet', function() { utxo = cloneObject([expectedUtxo]) }) - it('uses hashLittleEndian when hash is not present', function(){ - delete utxo[0]['hash'] - - wallet.setUnspentOutputs(utxo) - verifyOutputs() - }) - - it('uses hash when hashLittleEndian is not present', function(){ - delete utxo[0]['hashLittleEndian'] - - wallet.setUnspentOutputs(utxo) - verifyOutputs() - }) - - it('uses hash when both hash and hashLittleEndian are present', function(){ + it('matches the expected behaviour', function(){ wallet.setUnspentOutputs(utxo) verifyOutputs() }) describe('required fields', function(){ - it("throws an error when hash and hashLittleEndian are both missing", function(){ - delete utxo[0]['hash'] - delete utxo[0]['hashLittleEndian'] - - assert.throws(function() { - wallet.setUnspentOutputs(utxo) - }, /Invalid unspent output: key hash\(or hashLittleEndian\) is missing/) - }); - - ['outputIndex', 'address', 'value'].forEach(function(field){ + ['outputIndex', 'address', 'hash', 'value'].forEach(function(field){ it("throws an error when " + field + " is missing", function(){ delete utxo[0][field] From 4952c5f4e7026661e8be3a6bb96759fc4aaf72fb Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Fri, 30 May 2014 18:41:03 +1000 Subject: [PATCH 05/11] HD/Wallet: use network objects, not strings --- src/hdwallet.js | 24 +++++++++++------------- src/wallet.js | 17 +++++++++-------- test/hdwallet.js | 44 +++++++++++++++++++++++++++++++++----------- test/wallet.js | 25 +++++++++++++------------ 4 files changed, 66 insertions(+), 44 deletions(-) diff --git a/src/hdwallet.js b/src/hdwallet.js index 20766ac..7df6207 100644 --- a/src/hdwallet.js +++ b/src/hdwallet.js @@ -12,14 +12,11 @@ var networks = require('./networks') var sec = require('./sec') var ecparams = sec("secp256k1") -function HDWallet(seed, networkString) { +function HDWallet(seed, network) { if (seed == undefined) return; // FIXME: Boo, should be stricter - this.network = networkString || 'bitcoin' - - if(!networks.hasOwnProperty(this.network)) { - throw new Error("Unknown network: " + this.network) - } + network = network || networks.bitcoin + assert(network.bip32, 'Unknown BIP32 constants for network') var I = crypto.HmacSHA512(seed, HDWallet.MASTER_SECRET) var IL = I.slice(0, 32) @@ -28,6 +25,7 @@ function HDWallet(seed, networkString) { // In case IL is 0 or >= n, the master key is invalid (handled by ECKey.fromBuffer) var pIL = BigInteger.fromBuffer(IL) + this.network = network this.priv = new ECKey(pIL, true) this.pub = this.priv.pub @@ -40,8 +38,8 @@ HDWallet.MASTER_SECRET = new Buffer('Bitcoin seed') HDWallet.HIGHEST_BIT = 0x80000000 HDWallet.LENGTH = 78 -HDWallet.fromSeedHex = function(hex, networkString) { - return new HDWallet(new Buffer(hex, 'hex'), networkString) +HDWallet.fromSeedHex = function(hex, network) { + return new HDWallet(new Buffer(hex, 'hex'), network) } HDWallet.fromBase58 = function(string) { @@ -66,14 +64,14 @@ HDWallet.fromBuffer = function(input) { var version = input.readUInt32BE(0) var type - for(var name in networks) { + for (var name in networks) { var network = networks[name] - for(var t in network.bip32) { + for (var t in network.bip32) { if (version != network.bip32[t]) continue type = t - hd.network = name + hd.network = network } } @@ -127,7 +125,7 @@ HDWallet.prototype.getAddress = function() { HDWallet.prototype.toBuffer = function(priv) { // Version - var version = networks[this.network].bip32[priv ? 'priv' : 'pub'] + var version = this.network.bip32[priv ? 'priv' : 'pub'] var buffer = new Buffer(HDWallet.LENGTH) // 4 bytes: version bytes @@ -254,7 +252,7 @@ HDWallet.prototype.derivePrivate = function(index) { } HDWallet.prototype.getKeyVersion = function() { - return networks[this.network].pubKeyHash + return this.network.pubKeyHash } HDWallet.prototype.toString = HDWallet.prototype.toBase58 diff --git a/src/wallet.js b/src/wallet.js index 206bf44..58b5748 100644 --- a/src/wallet.js +++ b/src/wallet.js @@ -1,14 +1,14 @@ -var Address = require('./address') -var HDNode = require('./hdwallet.js') var networks = require('./networks') var rng = require('secure-random') + +var Address = require('./address') +var HDNode = require('./hdwallet') var Transaction = require('./transaction').Transaction -function Wallet(seed, options) { +function Wallet(seed, network) { if (!(this instanceof Wallet)) { return new Wallet(seed, options); } - var options = options || {} - var network = options.network || 'bitcoin' + network = network || networks.bitcoin // Stored in a closure to make accidental serialization less likely var masterkey = null @@ -28,7 +28,7 @@ function Wallet(seed, options) { this.outputs = {} // Make a new master key - this.newMasterKey = function(seed, network) { + this.newMasterKey = function(seed) { seed = seed || new Buffer(rng(32)) masterkey = new HDNode(seed, network) @@ -43,7 +43,8 @@ function Wallet(seed, options) { me.outputs = {} } - this.newMasterKey(seed, network) + + this.newMasterKey(seed) this.generateAddress = function() { var key = externalAccount.derive(this.addresses.length) @@ -143,7 +144,7 @@ function Wallet(seed, options) { var address try { - address = Address.fromScriptPubKey(txOut.script, networks[network]).toString() + address = Address.fromScriptPubKey(txOut.script, network).toString() } catch(e) { if (!(e.message.match(/has no matching Address/))) throw e } diff --git a/test/hdwallet.js b/test/hdwallet.js index 0a826c7..03a7c85 100644 --- a/test/hdwallet.js +++ b/test/hdwallet.js @@ -1,4 +1,5 @@ var assert = require('assert') +var networks = require('../src/networks') var HDWallet = require('../src/hdwallet') var fixtures = require('./fixtures/hdwallet.json') @@ -68,6 +69,15 @@ describe('HDWallet', function() { buffer.writeUInt32BE(0xFFFFFFFF, 9) assert.throws(function() { HDWallet.fromBuffer(buffer) }, /Invalid index/) }) + + it('fails for an invalid network type', function() { + var buffer = new HDWallet(seed).toBuffer() + buffer.writeUInt32BE(0x00000000, 0) + + assert.throws(function() { + HDWallet.fromBuffer(buffer) + }, /Could not find version 0/) + }) }) }) @@ -119,24 +129,36 @@ describe('HDWallet', function() { }) describe('network types', function() { + var seed + + beforeEach(function() { + seed = new Buffer('foobar') + }) + + it('ensure that a bitcoin wallet is the default', function() { + var wallet = new HDWallet(seed) + + assert.equal(wallet.network, networks.bitcoin) + }) + it('ensures that a bitcoin Wallet generates bitcoin addresses', function() { - var wallet = new HDWallet(new Buffer('foobar'), 'bitcoin') - assert.equal(wallet.getAddress().toString(), '17SnB9hyGwJPoKpLb9eVPHjsujyEuBpMAA') + var wallet = new HDWallet(seed) + var address = wallet.getAddress().toString() + + assert.equal(address, '17SnB9hyGwJPoKpLb9eVPHjsujyEuBpMAA') }) it('ensures that a testnet Wallet generates testnet addresses', function() { - var wallet = new HDWallet(new Buffer('foobar'), 'testnet') - assert.equal(wallet.getAddress().toString(), 'mmxjUCnx5xjeaSHxJicsDCxCmjZwq8KTbv') - }) + var wallet = new HDWallet(seed, networks.testnet) + var address = wallet.getAddress().toString() - it('throws an exception when unknown network type is passed in', function() { - assert.throws(function() { new HDWallet(new Buffer('foobar'), 'doge') }, /Unknown network: doge/) + assert.equal(address, 'mmxjUCnx5xjeaSHxJicsDCxCmjZwq8KTbv') }) - it('throws an exception with bad network type using fromBuffer', function() { - var buffer = new HDWallet(new Buffer('foobar'), 'bitcoin').toBuffer() - buffer.writeUInt32BE(0x00000000, 0) - assert.throws(function() { HDWallet.fromBuffer(buffer) }, /Could not find version 0/) + it('throws an exception when unknown network type is passed in', function() { + assert.throws(function() { + new HDWallet(seed, {}) + }, /Unknown BIP32 constants for network/) }) }) }) diff --git a/test/wallet.js b/test/wallet.js index b5187d1..e54d603 100644 --- a/test/wallet.js +++ b/test/wallet.js @@ -1,5 +1,6 @@ var assert = require('assert') var crypto = require('../src/crypto') +var networks = require('../src/networks') var sinon = require('sinon') var Address = require('../src/address') @@ -29,7 +30,7 @@ describe('Wallet', function() { }) it('defaults to Bitcoin network', function() { - assert.equal(wallet.getMasterKey().network, 'bitcoin') + assert.equal(wallet.getMasterKey().network, networks.bitcoin) }) it("generates m/0' as the main account", function() { @@ -59,11 +60,11 @@ describe('Wallet', function() { describe('constructor options', function() { beforeEach(function() { - wallet = new Wallet(seed, {network: 'testnet'}) + wallet = new Wallet(seed, networks.testnet) }) it('uses the network if specified', function() { - assert.equal(wallet.getMasterKey().network, 'testnet') + assert.equal(wallet.getMasterKey().network, networks.testnet) }) }) }) @@ -98,7 +99,7 @@ describe('Wallet', function() { describe('generateAddress', function(){ it('generate receiving addresses', function(){ - var wallet = new Wallet(seed, {network: 'testnet'}) + var wallet = new Wallet(seed, networks.testnet) var expectedAddresses = [ "n1GyUANZand9Kw6hGSV9837cCC9FFUQzQa", "n2fiWrHqD6GM5GiEqkbWAc6aaZQp3ba93X" @@ -112,7 +113,7 @@ describe('Wallet', function() { describe('generateChangeAddress', function(){ it('generates change addresses', function(){ - var wallet = new Wallet(seed, {network: 'testnet'}) + var wallet = new Wallet(seed, networks.testnet) var expectedAddresses = ["mnXiDR4MKsFxcKJEZjx4353oXvo55iuptn"] assert.equal(wallet.generateChangeAddress(), expectedAddresses[0]) @@ -122,7 +123,7 @@ describe('Wallet', function() { describe('getPrivateKey', function(){ it('returns the private key at the given index of external account', function(){ - var wallet = new Wallet(seed, {network: 'testnet'}) + var wallet = new Wallet(seed, networks.testnet) assertEqual(wallet.getPrivateKey(0), wallet.getExternalAccount().derive(0).priv) assertEqual(wallet.getPrivateKey(1), wallet.getExternalAccount().derive(1).priv) @@ -131,7 +132,7 @@ describe('Wallet', function() { describe('getInternalPrivateKey', function(){ it('returns the private key at the given index of internal account', function(){ - var wallet = new Wallet(seed, {network: 'testnet'}) + var wallet = new Wallet(seed, networks.testnet) assertEqual(wallet.getInternalPrivateKey(0), wallet.getInternalAccount().derive(0).priv) assertEqual(wallet.getInternalPrivateKey(1), wallet.getInternalAccount().derive(1).priv) @@ -140,7 +141,7 @@ describe('Wallet', function() { describe('getPrivateKeyForAddress', function(){ it('returns the private key for the given address', function(){ - var wallet = new Wallet(seed, {network: 'testnet'}) + var wallet = new Wallet(seed, networks.testnet) wallet.generateChangeAddress() wallet.generateAddress() wallet.generateAddress() @@ -156,7 +157,7 @@ describe('Wallet', function() { }) it('raises an error when address is not found', function(){ - var wallet = new Wallet(seed, {network: 'testnet'}) + var wallet = new Wallet(seed, networks.testnet) assert.throws(function() { wallet.getPrivateKeyForAddress("n2fiWrHqD6GM5GiEqkbWAc6aaZQp3ba93X") }, /Unknown address. Make sure the address is from the keychain and has been generated./) @@ -434,9 +435,9 @@ describe('Wallet', function() { }) }) - describe('testnet', function(){ + describe(networks.testnet, function(){ it('should create transaction', function(){ - var wallet = new Wallet(seed, {network: 'testnet'}) + var wallet = new Wallet(seed, networks.testnet) var address = wallet.generateAddress() wallet.setUnspentOutputs([{ @@ -458,7 +459,7 @@ describe('Wallet', function() { describe('changeAddress', function(){ it('should allow custom changeAddress', function(){ - var wallet = new Wallet(seed, {network: 'testnet'}) + var wallet = new Wallet(seed, networks.testnet) var address = wallet.generateAddress() wallet.setUnspentOutputs([{ From cde285ccfcdd699cd99c10f92ea2006bfc04208d Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Fri, 30 May 2014 18:46:11 +1000 Subject: [PATCH 06/11] Wallet: enforce operator new --- src/wallet.js | 2 -- test/wallet.js | 4 ---- 2 files changed, 6 deletions(-) diff --git a/src/wallet.js b/src/wallet.js index 58b5748..0d41bb2 100644 --- a/src/wallet.js +++ b/src/wallet.js @@ -6,8 +6,6 @@ var HDNode = require('./hdwallet') var Transaction = require('./transaction').Transaction function Wallet(seed, network) { - if (!(this instanceof Wallet)) { return new Wallet(seed, options); } - network = network || networks.bitcoin // Stored in a closure to make accidental serialization less likely diff --git a/test/wallet.js b/test/wallet.js index e54d603..ef2d152 100644 --- a/test/wallet.js +++ b/test/wallet.js @@ -25,10 +25,6 @@ describe('Wallet', function() { }) describe('constructor', function() { - it('should be ok to call without new', function() { - assert.ok(Wallet(seed) instanceof Wallet) - }) - it('defaults to Bitcoin network', function() { assert.equal(wallet.getMasterKey().network, networks.bitcoin) }) From 80da2ed2d5e91864e7e9295b9e736a6f1588f761 Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Fri, 30 May 2014 19:00:49 +1000 Subject: [PATCH 07/11] HDWallet: add fromBase58 exception checks --- src/hdwallet.js | 4 ++-- test/fixtures/hdwallet.json | 17 ++++++++++++++++- test/hdwallet.js | 10 ++++++++++ 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/hdwallet.js b/src/hdwallet.js index 7df6207..b0ce031 100644 --- a/src/hdwallet.js +++ b/src/hdwallet.js @@ -49,8 +49,8 @@ HDWallet.fromBase58 = function(string) { var checksum = buffer.slice(-4) var newChecksum = crypto.hash256(payload).slice(0, 4) - assert.deepEqual(newChecksum, checksum) - assert.equal(payload.length, HDWallet.LENGTH) + assert.deepEqual(newChecksum, checksum, 'Invalid checksum') + assert.equal(payload.length, HDWallet.LENGTH, 'Invalid BIP32 string') return HDWallet.fromBuffer(payload) } diff --git a/test/fixtures/hdwallet.json b/test/fixtures/hdwallet.json index d3b2744..bcebf2c 100644 --- a/test/fixtures/hdwallet.json +++ b/test/fixtures/hdwallet.json @@ -176,5 +176,20 @@ ] } ], - "invalid": [] + "invalid": { + "fromBase58": [ + { + "exception": "Invalid checksum", + "string": "xprvQQQQQQQQQQQQQQQQCviVfJSKyQ1mDYahRjijr5idH2WwLsEd4Hsb2Tyh8RfQMuPh7f7RtyzTtdrbdqqsunu5Mm3wDvUAKRHSC34sJ7in334" + }, + { + "exception": "Invalid BIP32 string", + "string": "SQ8nQ2jWarXqLo9oHGKKP6iQDsQbPRftq7rjtYY3hqJRPQRgrmeunFnDKbH7B15yGPLZBrhhkKXx3pwD6LcBooJRGq6x7matAXpMsgn" + }, + { + "exception": "Invalid BIP32 string", + "string": "37hdAfw3aMiWcBGPP2ywmY5jizTeSSP5GXayKY3RxkEZ7f3SBnRE1pN6eY3VzGkgx6vbdNtuKfrHgEaYvW9KkFZCycaPvWiA9TtfmeVB592Sf9RfSzQzXo72" + } + ] + } } diff --git a/test/hdwallet.js b/test/hdwallet.js index 03a7c85..f4a5c99 100644 --- a/test/hdwallet.js +++ b/test/hdwallet.js @@ -33,6 +33,16 @@ describe('HDWallet', function() { }) }) + describe('fromBase58', function() { + fixtures.invalid.fromBase58.forEach(function(f) { + it('throws on ' + f.string, function() { + assert.throws(function() { + HDWallet.fromBase58(f.string) + }, new RegExp(f.exception)) + }) + }) + }) + describe('constructor & seed deserialization', function() { var expectedPrivateKey = '0fd71c652e847ba7ea7956e3cf3fc0a0985871846b1b2c23b9c6a29a38cee860' var seed = new Buffer([ From b9bdf21cbe67074c119b4cbdffd11b38532d665b Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Sat, 31 May 2014 13:18:57 +1000 Subject: [PATCH 08/11] bufferutils: use verifuint for 64 bit integers Taken from browserify-buffer. Also adds a few more tests to assert this is working correctly from both read and write perspectives. The assertion in for writePushDataInt in the 32 bit case was unnecessary as that is handled by buffer.writeUInt32LE anyway. --- src/bufferutils.js | 20 ++++++++++---------- test/bufferutils.js | 35 ++++++++++++++++++++++------------- test/fixtures/buffer.json | 10 ++++++++-- 3 files changed, 40 insertions(+), 25 deletions(-) diff --git a/src/bufferutils.js b/src/bufferutils.js index ac340e9..e24735f 100644 --- a/src/bufferutils.js +++ b/src/bufferutils.js @@ -1,6 +1,14 @@ var assert = require('assert') var opcodes = require('./opcodes') +// https://github.com/feross/buffer/blob/master/index.js#L1127 +function verifuint(value, max) { + assert(typeof value === 'number', 'cannot write a non-number as a number') + assert(value >= 0, 'specified a negative value for writing an unsigned value') + assert(value <= max, 'value is larger than maximum value for type') + assert(Math.floor(value) === value, 'value has a fractional component') +} + function pushDataSize(i) { return i < opcodes.OP_PUSHDATA1 ? 1 : i < 0xff ? 2 @@ -47,9 +55,7 @@ function readUInt64LE(buffer, offset) { var b = buffer.readUInt32LE(offset + 4) b *= 0x100000000 - // Javascript Safe Integer limitation - // assert(Number.isSafeInteger(value), 'value must be < 2^53') - assert(b + a < 0x0020000000000000, 'value must be < 2^53') + verifuint(b + a, 0x001fffffffffffff) return b + a } @@ -104,10 +110,6 @@ function writePushDataInt(buffer, number, offset) { // 32 bit } else { - // Javascript Safe Integer limitation - // assert(Number.isSafeInteger(value), 'value must be < 2^53') - assert(number < 0x0020000000000000, 'value must be < 2^53') - buffer.writeUInt8(opcodes.OP_PUSHDATA4, offset) buffer.writeUInt32LE(number, offset + 1) @@ -117,9 +119,7 @@ function writePushDataInt(buffer, number, offset) { } function writeUInt64LE(buffer, value, offset) { - // Javascript Safe Integer limitation - // assert(Number.isSafeInteger(value), 'value must be < 2^53') - assert(value < 0x0020000000000000, 'value must be < 2^53') + verifuint(value, 0x001fffffffffffff) buffer.writeInt32LE(value & -1, offset) buffer.writeUInt32LE(Math.floor(value / 0x100000000), offset + 4) diff --git a/test/bufferutils.js b/test/bufferutils.js index 72cec2d..d56dbcd 100644 --- a/test/bufferutils.js +++ b/test/bufferutils.js @@ -39,6 +39,16 @@ describe('bufferutils', function() { assert.equal(number, f.dec) }) }) + + fixtures.invalid.forEach(function(f) { + it('throws on ' + f.description, function() { + var buffer = new Buffer(f.hex64, 'hex') + + assert.throws(function() { + bufferutils.readUInt64LE(buffer, 0) + }, new RegExp(f.exception)) + }) + }) }) describe('readVarInt', function() { @@ -51,6 +61,16 @@ describe('bufferutils', function() { assert.equal(d.size, buffer.length) }) }) + + fixtures.invalid.forEach(function(f) { + it('throws on ' + f.description, function() { + var buffer = new Buffer(f.hexVI, 'hex') + + assert.throws(function() { + bufferutils.readVarInt(buffer, 0) + }, new RegExp(f.exception)) + }) + }) }) describe('varIntSize', function() { @@ -75,17 +95,6 @@ describe('bufferutils', function() { assert.equal(buffer.slice(0, n).toString('hex'), f.hexPD) }) }) - - fixtures.invalid.forEach(function(f) { - it('throws on ' + f.description, function() { - var buffer = new Buffer(5) - buffer.fill(0) - - assert.throws(function() { - bufferutils.writePushDataInt(buffer, f.dec, 0) - }, /value must be < 2\^53/) - }) - }) }) describe('writeUInt64LE', function() { @@ -106,7 +115,7 @@ describe('bufferutils', function() { assert.throws(function() { bufferutils.writeUInt64LE(buffer, f.dec, 0) - }, /value must be < 2\^53/) + }, new RegExp(f.exception)) }) }) }) @@ -129,7 +138,7 @@ describe('bufferutils', function() { assert.throws(function() { bufferutils.writeVarInt(buffer, f.dec, 0) - }, /value must be < 2\^53/) + }, new RegExp(f.exception)) }) }) }) diff --git a/test/fixtures/buffer.json b/test/fixtures/buffer.json index 116e70a..8c6881d 100644 --- a/test/fixtures/buffer.json +++ b/test/fixtures/buffer.json @@ -85,11 +85,17 @@ "invalid": [ { "description": "n === 2^53", - "value": 9007199254740992 + "exception": "value is larger than maximum value for type", + "hex64": "0000000000002000", + "hexVI": "ff0000000000000020", + "dec": 9007199254740992 }, { "description": "n > 2^53", - "value": 18374686479671624000 + "exception": "value is larger than maximum value for type", + "hex64": "0100000000002000", + "hexVI": "ff0100000000000020", + "dec": 9007199254740993 } ] } From 5b7873d05b8689ff64e03b923cdab1070afcbff8 Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Sat, 31 May 2014 13:19:46 +1000 Subject: [PATCH 09/11] bufferutils: test fixture filename copies parent --- test/bufferutils.js | 2 +- test/fixtures/{buffer.json => bufferutils.json} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename test/fixtures/{buffer.json => bufferutils.json} (100%) diff --git a/test/bufferutils.js b/test/bufferutils.js index d56dbcd..4882062 100644 --- a/test/bufferutils.js +++ b/test/bufferutils.js @@ -1,7 +1,7 @@ var assert = require('assert') var bufferutils = require('../src/bufferutils') -var fixtures = require('./fixtures/buffer.json') +var fixtures = require('./fixtures/bufferutils.json') describe('bufferutils', function() { describe('pushDataSize', function() { diff --git a/test/fixtures/buffer.json b/test/fixtures/bufferutils.json similarity index 100% rename from test/fixtures/buffer.json rename to test/fixtures/bufferutils.json From 3bce535e36d0537f4702ddc41efe17671a66c74b Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Sat, 31 May 2014 12:16:01 +1000 Subject: [PATCH 10/11] Wallet: use assert for consistency --- src/wallet.js | 7 +++---- test/wallet.js | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/wallet.js b/src/wallet.js index 0d41bb2..846a5ad 100644 --- a/src/wallet.js +++ b/src/wallet.js @@ -1,3 +1,4 @@ +var assert = require('assert') var networks = require('./networks') var rng = require('secure-random') @@ -169,7 +170,7 @@ function Wallet(seed, network) { } this.createTx = function(to, value, fixedFee, changeAddress) { - if (value <= this.dustThreshold) throw new Error("Value must be above dust threshold") + assert(value > this.dustThreshold, value + ' must be above dust threshold (' + this.dustThreshold + ' Satoshis)') var utxos = getCandidateOutputs(value) var accum = 0 @@ -198,9 +199,7 @@ function Wallet(seed, network) { } } - if (accum < subTotal) { - throw new Error('Not enough funds: ' + accum + ' < ' + subTotal) - } + assert(accum >= subTotal, 'Not enough funds (incl. fee): ' + accum + ' < ' + subTotal) this.sign(tx) return tx diff --git a/test/wallet.js b/test/wallet.js index ef2d152..c3b459b 100644 --- a/test/wallet.js +++ b/test/wallet.js @@ -546,7 +546,7 @@ describe('Wallet', function() { assert.throws(function() { wallet.createTx(to, value) - }, /Value must be above dust threshold/) + }, /5430 must be above dust threshold \(5430 Satoshis\)/) }) }) @@ -556,7 +556,7 @@ describe('Wallet', function() { assert.throws(function() { wallet.createTx(to, value) - }, /Not enough funds: 1420000 < 1420001/) + }, /Not enough funds \(incl. fee\): 1420000 < 1420001/) }) }) }) From 749943cb4e33e80873f12f1bbb6e173cd124ee8f Mon Sep 17 00:00:00 2001 From: Daniel Cousens Date: Sat, 31 May 2014 15:21:18 +1000 Subject: [PATCH 11/11] HDWallet: clarify test intention --- test/hdwallet.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/hdwallet.js b/test/hdwallet.js index f4a5c99..0fc7987 100644 --- a/test/hdwallet.js +++ b/test/hdwallet.js @@ -81,12 +81,12 @@ describe('HDWallet', function() { }) it('fails for an invalid network type', function() { - var buffer = new HDWallet(seed).toBuffer() - buffer.writeUInt32BE(0x00000000, 0) + var network = { bip32: { priv: 0x11111111, pub: 0x22222222 } } + var buffer = new HDWallet(seed, network).toBuffer() assert.throws(function() { HDWallet.fromBuffer(buffer) - }, /Could not find version 0/) + }, /Could not find version 22222222/) }) }) })