From 5f35aafc29ccee41e1f293aa3f453e3b7c9ce4fe Mon Sep 17 00:00:00 2001 From: Ivan Socolsky Date: Thu, 28 Jan 2016 11:01:03 -0300 Subject: [PATCH 1/6] fix proposal signature validation in publishTx --- lib/server.js | 22 +++++++++------------- test/integration/server.js | 36 +++++++++++++++++------------------- 2 files changed, 26 insertions(+), 32 deletions(-) diff --git a/lib/server.js b/lib/server.js index 9b731c1..dc7f857 100644 --- a/lib/server.js +++ b/lib/server.js @@ -650,8 +650,8 @@ WalletService.prototype.joinWallet = function(opts, cb) { } if (_.find(wallet.copayers, { - xPubKey: opts.xPubKey - })) return cb(Errors.COPAYER_IN_WALLET); + xPubKey: opts.xPubKey + })) return cb(Errors.COPAYER_IN_WALLET); if (wallet.copayers.length == wallet.n) return cb(Errors.WALLET_FULL); @@ -744,8 +744,8 @@ WalletService.prototype._canCreateAddress = function(ignoreMaxGap, cb) { isChange: true }), Defaults.MAX_MAIN_ADDRESS_GAP); if (latestAddresses.length < Defaults.MAX_MAIN_ADDRESS_GAP || _.any(latestAddresses, { - hasActivity: true - })) return cb(null, true); + hasActivity: true + })) return cb(null, true); var bc = self._getBlockchainExplorer(latestAddresses[0].network); var activityFound = false; @@ -1618,8 +1618,6 @@ WalletService.prototype._verifyRequestPubKey = function(requestPubKey, signature * @param {Object} opts * @param {string} opts.txProposalId - The tx id. * @param {string} opts.proposalSignature - S(raw tx). Used by other copayers to verify the proposal. - * @param {string} opts.proposalSignaturePubKey - (Optional) An alternative public key used to verify the proposal signature. - * @param {string} opts.proposalSignaturePubKeySig - (Optional) A signature used to validate the opts.proposalSignaturePubKey. */ WalletService.prototype.publishTx = function(opts, cb) { var self = this; @@ -1648,13 +1646,11 @@ WalletService.prototype.publishTx = function(opts, cb) { return cb(new ClientError('Invalid proposal signature')); } - // Verify signingKey - if (opts.proposalSignaturePubKey) { - if (opts.proposalSignaturePubKey != signingKey || - !self._verifyRequestPubKey(opts.proposalSignaturePubKey, opts.proposalSignaturePubKeySig, copayer.xPubKey) - ) { - return cb(new ClientError('Invalid proposal signing key')); - } + // Save signature info for other copayers to check + txp.proposalSignature = opts.proposalSignature; + if (signingKey.selfSigned) { + txp.proposalSignaturePubKey = signingKey.key; + txp.proposalSignaturePubKeySig = signingKey.signature; } // Verify UTXOs are still available diff --git a/test/integration/server.js b/test/integration/server.js index b245558..c2bbd67 100644 --- a/test/integration/server.js +++ b/test/integration/server.js @@ -2739,6 +2739,7 @@ describe('Wallet service', function() { server.getPendingTxs({}, function(err, txs) { should.not.exist(err); txs.length.should.equal(1); + should.exist(txs[0].proposalSignature); done(); }); }); @@ -2795,21 +2796,14 @@ describe('Wallet service', function() { should.not.exist(err); should.exist(txp); - var raw = txp.getRawTx(); - var proposalSignature = helpers.signMessage(raw, TestData.copayers[0].privKey_1H_0); - var pubKey = new Bitcore.PrivateKey(TestData.copayers[0].privKey_1H_0).toPublicKey().toString(); - var pubKeySig = helpers.signMessage(pubKey, TestData.copayers[1].privKey_1H_0); - var publishOpts = { txProposalId: txp.id, - proposalSignature: proposalSignature, - proposalSignaturePubKey: pubKey, - proposalSignaturePubKeySig: pubKeySig, + proposalSignature: helpers.signMessage(txp.getRawTx(), TestData.copayers[1].privKey_1H_0), } server.publishTx(publishOpts, function(err) { should.exist(err); - err.message.should.contain('Invalid proposal signing key'); + err.message.should.contain('Invalid proposal signature'); done(); }); }); @@ -2818,18 +2812,17 @@ describe('Wallet service', function() { it('should accept a tx proposal signed with a custom key', function(done) { var reqPrivKey = new Bitcore.PrivateKey(); - var reqPubKey = reqPrivKey.toPublicKey(); + var reqPubKey = reqPrivKey.toPublicKey().toString(); var xPrivKey = TestData.copayers[0].xPrivKey_44H_0H_0H; - var sig = helpers.signRequestPubKey(reqPubKey, xPrivKey); - var opts = { + var accessOpts = { copayerId: TestData.copayers[0].id44, requestPubKey: reqPubKey, - signature: sig, + signature: helpers.signRequestPubKey(reqPubKey, xPrivKey), }; - server.addAccess(opts, function(err) { + server.addAccess(accessOpts, function(err) { should.not.exist(err); helpers.stubUtxos(server, wallet, [1, 2], function() { @@ -2846,14 +2839,19 @@ describe('Wallet service', function() { var publishOpts = { txProposalId: txp.id, proposalSignature: helpers.signMessage(txp.getRawTx(), reqPrivKey), - proposalSignaturePubKey: reqPubKey, - proposalSignaturePubKeySig: sig, } server.publishTx(publishOpts, function(err) { - should.exist(err); - err.message.should.contain('Invalid proposal signing key'); - done(); + should.not.exist(err); + server.getTx({ + txProposalId: txp.id + }, function(err, x) { + should.not.exist(err); + x.proposalSignature.should.equal(publishOpts.proposalSignature); + x.proposalSignaturePubKey.should.equal(accessOpts.requestPubKey); + x.proposalSignaturePubKeySig.should.equal(accessOpts.signature); + done(); + }); }); }); }); From 6f196aa1b998cd907ce5ef00dc5ad0d010e81ef1 Mon Sep 17 00:00:00 2001 From: Ivan Socolsky Date: Thu, 28 Jan 2016 17:53:22 -0300 Subject: [PATCH 2/6] allow fee to be specified on proposal creation --- lib/common/defaults.js | 1 + lib/model/txproposal.js | 5 +++-- lib/server.js | 23 +++++++++++++++++------ test/integration/helpers.js | 2 +- test/integration/server.js | 19 +++++++++++++++++++ 5 files changed, 41 insertions(+), 9 deletions(-) diff --git a/lib/common/defaults.js b/lib/common/defaults.js index b5b366a..2b87252 100644 --- a/lib/common/defaults.js +++ b/lib/common/defaults.js @@ -5,6 +5,7 @@ var Defaults = {}; Defaults.DEFAULT_FEE_PER_KB = 10000; Defaults.MIN_FEE_PER_KB = 0; Defaults.MAX_FEE_PER_KB = 1000000; +Defaults.MIN_TX_FEE = 0; Defaults.MAX_TX_FEE = 1 * 1e8; Defaults.MAX_KEYS = 100; diff --git a/lib/model/txproposal.js b/lib/model/txproposal.js index f3747cc..2a3d115 100644 --- a/lib/model/txproposal.js +++ b/lib/model/txproposal.js @@ -44,7 +44,7 @@ TxProposal.create = function(opts) { x.requiredRejections = Math.min(x.walletM, x.walletN - x.walletM + 1), x.status = 'temporary'; x.actions = []; - x.fee = null; + x.fee = opts.fee; x.feePerKb = opts.feePerKb; x.excludeUnconfirmedUtxos = opts.excludeUnconfirmedUtxos; @@ -74,6 +74,7 @@ TxProposal.fromObj = function(obj) { x.id = obj.id; x.walletId = obj.walletId; x.creatorId = obj.creatorId; + x.network = obj.network; x.outputs = obj.outputs; x.amount = obj.amount; x.message = obj.message; @@ -93,7 +94,6 @@ TxProposal.fromObj = function(obj) { }); x.outputOrder = obj.outputOrder; x.fee = obj.fee; - x.network = obj.network; x.feePerKb = obj.feePerKb; x.excludeUnconfirmedUtxos = obj.excludeUnconfirmedUtxos; x.addressType = obj.addressType; @@ -232,6 +232,7 @@ TxProposal.prototype.getEstimatedSize = function() { }; TxProposal.prototype.estimateFee = function() { + $.checkState(_.isNumber(this.feePerKb)); var fee = this.feePerKb * this.getEstimatedSize() / 1000; this.fee = parseInt(fee.toFixed(0)); }; diff --git a/lib/server.js b/lib/server.js index dc7f857..e1ed48e 100644 --- a/lib/server.js +++ b/lib/server.js @@ -1219,8 +1219,11 @@ WalletService.prototype._checkTxAndEstimateFee = function(txp) { serializationOpts.disableLargeFees = true; } - try { + if (_.isNumber(txp.feePerKb)) { txp.estimateFee(); + } + + try { var bitcoreTx = txp.getBitcoreTx(); bitcoreError = bitcoreTx.getSerializationError(serializationOpts); if (!bitcoreError) { @@ -1535,7 +1538,8 @@ WalletService.prototype.createTxLegacy = function(opts, cb) { * @param {string} opts.outputs[].message - A message to attach to this output. * @param {string} opts.message - A message to attach to this transaction. * @param {Array} opts.inputs - Optional. Inputs for this TX - * @param {string} opts.feePerKb - Optional. Use an alternative fee per KB for this TX + * @param {string} opts.fee - Optional. Use an alternative fee for this TX (mutually exclusive with feePerKb) + * @param {string} opts.feePerKb - Optional. Use an alternative fee per KB for this TX (mutually exclusive with fee) * @param {string} opts.payProUrl - Optional. Paypro URL for peers to verify TX * @param {string} opts.excludeUnconfirmedUtxos[=false] - Optional. Do not use UTXOs of unconfirmed transactions as inputs * @param {string} opts.validateOutputs[=true] - Optional. Perform validation on outputs. @@ -1547,9 +1551,15 @@ WalletService.prototype.createTx = function(opts, cb) { if (!Utils.checkRequired(opts, ['outputs'])) return cb(new ClientError('Required argument missing')); - var feePerKb = opts.feePerKb || Defaults.DEFAULT_FEE_PER_KB; - if (feePerKb < Defaults.MIN_FEE_PER_KB || feePerKb > Defaults.MAX_FEE_PER_KB) - return cb(new ClientError('Invalid fee per KB value')); + if (_.isNumber(opts.fee)) { + if (opts.fee < Defaults.MIN_FEE || opts.fee > Defaults.MAX_FEE) + return cb(new ClientError('Invalid fee')); + } else { + opts.fee = null; + opts.feePerKb = opts.feePerKb || Defaults.DEFAULT_FEE_PER_KB; + if (opts.feePerKb < Defaults.MIN_FEE_PER_KB || opts.feePerKb > Defaults.MAX_FEE_PER_KB) + return cb(new ClientError('Invalid fee per KB')); + } self._runLocked(cb, function(cb) { self.getWallet({}, function(err, wallet) { @@ -1574,7 +1584,8 @@ WalletService.prototype.createTx = function(opts, cb) { outputs: opts.outputs, message: opts.message, changeAddress: wallet.createAddress(true), - feePerKb: feePerKb, + fee: opts.fee, + feePerKb: opts.feePerKb, payProUrl: opts.payProUrl, walletM: wallet.m, walletN: wallet.n, diff --git a/test/integration/helpers.js b/test/integration/helpers.js index 99b43b9..957efd7 100644 --- a/test/integration/helpers.js +++ b/test/integration/helpers.js @@ -411,7 +411,7 @@ helpers.createProposalOpts2 = function(outputs, moreOpts, inputs) { }; if (moreOpts) { - moreOpts = _.pick(moreOpts, ['feePerKb', 'customData', 'message']); + moreOpts = _.pick(moreOpts, ['fee', 'feePerKb', 'customData', 'message']); opts = _.assign(opts, moreOpts); } diff --git a/test/integration/server.js b/test/integration/server.js index c2bbd67..94433c5 100644 --- a/test/integration/server.js +++ b/test/integration/server.js @@ -2721,6 +2721,25 @@ describe('Wallet service', function() { }); }); + it('should be able to specify the final fee', function(done) { + helpers.stubUtxos(server, wallet, [1, 2], function() { + var txOpts = helpers.createProposalOpts2([{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8, + }], { + fee: 123400, + }); + server.createTx(txOpts, function(err, tx) { + should.not.exist(err); + should.exist(tx); + tx.fee.should.equal(123400); + var t = tx.getBitcoreTx(); + t.getFee().should.equal(123400); + done(); + }); + }); + }); + it('should be able to send a temporary tx proposal', function(done) { helpers.stubUtxos(server, wallet, [1, 2], function() { var txOpts = helpers.createProposalOpts2([{ From 77dc536a157b243caf4f000d2598edbab8faf578 Mon Sep 17 00:00:00 2001 From: Ivan Socolsky Date: Thu, 28 Jan 2016 18:11:53 -0300 Subject: [PATCH 3/6] check fee limits --- lib/common/defaults.js | 2 +- lib/server.js | 4 +- test/integration/server.js | 105 +++++++++++++++++++++++-------------- 3 files changed, 69 insertions(+), 42 deletions(-) diff --git a/lib/common/defaults.js b/lib/common/defaults.js index 2b87252..897ddbd 100644 --- a/lib/common/defaults.js +++ b/lib/common/defaults.js @@ -6,7 +6,7 @@ Defaults.DEFAULT_FEE_PER_KB = 10000; Defaults.MIN_FEE_PER_KB = 0; Defaults.MAX_FEE_PER_KB = 1000000; Defaults.MIN_TX_FEE = 0; -Defaults.MAX_TX_FEE = 1 * 1e8; +Defaults.MAX_TX_FEE = 0.1 * 1e8; Defaults.MAX_KEYS = 100; diff --git a/lib/server.js b/lib/server.js index e1ed48e..337843a 100644 --- a/lib/server.js +++ b/lib/server.js @@ -1,4 +1,5 @@ 'use strict'; + var _ = require('lodash'); var $ = require('preconditions').singleton(); var async = require('async'); @@ -1552,7 +1553,8 @@ WalletService.prototype.createTx = function(opts, cb) { return cb(new ClientError('Required argument missing')); if (_.isNumber(opts.fee)) { - if (opts.fee < Defaults.MIN_FEE || opts.fee > Defaults.MAX_FEE) + opts.feePerKb = null; + if (opts.fee < Defaults.MIN_TX_FEE || opts.fee > Defaults.MAX_TX_FEE) return cb(new ClientError('Invalid fee')); } else { opts.fee = null; diff --git a/test/integration/server.js b/test/integration/server.js index 94433c5..43267d8 100644 --- a/test/integration/server.js +++ b/test/integration/server.js @@ -2693,13 +2693,14 @@ describe('Wallet service', function() { it('should create a tx', function(done) { helpers.stubUtxos(server, wallet, [1, 2], function() { - var txOpts = helpers.createProposalOpts2([{ - toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', - amount: 0.8 - }], { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8 * 1e8, + }], message: 'some message', customData: 'some custom data', - }); + }; server.createTx(txOpts, function(err, tx) { should.not.exist(err); should.exist(tx); @@ -2723,12 +2724,13 @@ describe('Wallet service', function() { it('should be able to specify the final fee', function(done) { helpers.stubUtxos(server, wallet, [1, 2], function() { - var txOpts = helpers.createProposalOpts2([{ - toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', - amount: 0.8, - }], { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8 * 1e8, + }], fee: 123400, - }); + }; server.createTx(txOpts, function(err, tx) { should.not.exist(err); should.exist(tx); @@ -2740,15 +2742,33 @@ describe('Wallet service', function() { }); }); + it('should check explicit fee to be below max', function(done) { + helpers.stubUtxos(server, wallet, [1, 2], function() { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8 * 1e8, + }], + fee: 1e8, + }; + server.createTx(txOpts, function(err, tx) { + should.exist(err); + err.message.should.contain('fee'); + done(); + }); + }); + }); + it('should be able to send a temporary tx proposal', function(done) { helpers.stubUtxos(server, wallet, [1, 2], function() { - var txOpts = helpers.createProposalOpts2([{ - toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', - amount: 0.8 - }], { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8 * 1e8, + }], message: 'some message', customData: 'some custom data', - }); + }; server.createTx(txOpts, function(err, txp) { should.not.exist(err); should.exist(txp); @@ -2782,12 +2802,13 @@ describe('Wallet service', function() { it('should fail to send tx proposal with wrong signature', function(done) { helpers.stubUtxos(server, wallet, [1, 2], function() { - var txOpts = helpers.createProposalOpts2([{ - toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', - amount: 0.8 - }], { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8 * 1e8, + }], message: 'some message', - }); + }; server.createTx(txOpts, function(err, txp) { should.not.exist(err); should.exist(txp); @@ -2805,12 +2826,13 @@ describe('Wallet service', function() { it('should fail to send tx proposal not signed by the creator', function(done) { helpers.stubUtxos(server, wallet, [1, 2], function() { - var txOpts = helpers.createProposalOpts2([{ - toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', - amount: 0.8 - }], { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8 * 1e8, + }], message: 'some message', - }); + }; server.createTx(txOpts, function(err, txp) { should.not.exist(err); should.exist(txp); @@ -2845,12 +2867,13 @@ describe('Wallet service', function() { should.not.exist(err); helpers.stubUtxos(server, wallet, [1, 2], function() { - var txOpts = helpers.createProposalOpts2([{ - toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', - amount: 0.8 - }], { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8 * 1e8, + }], message: 'some message', - }); + }; server.createTx(txOpts, function(err, txp) { should.not.exist(err); should.exist(txp); @@ -2879,12 +2902,13 @@ describe('Wallet service', function() { it('should fail to send a temporary tx proposal if utxos are unavailable', function(done) { var txp1, txp2; - var txOpts = helpers.createProposalOpts2([{ - toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', - amount: 0.8 - }], { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8 * 1e8, + }], message: 'some message', - }); + }; async.waterfall([ @@ -2946,13 +2970,14 @@ describe('Wallet service', function() { it('should fail to list pending proposals from legacy client', function(done) { helpers.stubUtxos(server, wallet, [1, 2], function() { - var txOpts = helpers.createProposalOpts2([{ - toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', - amount: 0.8 - }], { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8 * 1e8, + }], message: 'some message', customData: 'some custom data', - }); + }; server.createTx(txOpts, function(err, txp) { should.not.exist(err); should.exist(txp); From 8cf56c2a3d7d64f0ed6b5456a6602adb39f8be10 Mon Sep 17 00:00:00 2001 From: Ivan Socolsky Date: Thu, 28 Jan 2016 18:40:10 -0300 Subject: [PATCH 4/6] remove unused code --- test/integration/helpers.js | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/test/integration/helpers.js b/test/integration/helpers.js index 957efd7..0f2e9d3 100644 --- a/test/integration/helpers.js +++ b/test/integration/helpers.js @@ -400,28 +400,6 @@ helpers.createExternalProposalOpts = function(toAddress, amount, signingKey, mor }; -helpers.createProposalOpts2 = function(outputs, moreOpts, inputs) { - _.each(outputs, function(output) { - output.amount = helpers.toSatoshi(output.amount); - }); - - var opts = { - outputs: outputs, - inputs: inputs || [], - }; - - if (moreOpts) { - moreOpts = _.pick(moreOpts, ['fee', 'feePerKb', 'customData', 'message']); - opts = _.assign(opts, moreOpts); - } - - opts = _.defaults(opts, { - message: null - }); - - return opts; -}; - helpers.getProposalSignatureOpts = function(txp, signingKey) { var raw = txp.getRawTx(); var proposalSignature = helpers.signMessage(raw, signingKey); From 23cddbe47f4a13c3a78fcc2d68c7f079f66e79c3 Mon Sep 17 00:00:00 2001 From: Ivan Socolsky Date: Fri, 29 Jan 2016 10:51:06 -0300 Subject: [PATCH 5/6] fix broadcasting new proposals --- lib/model/txproposal.js | 4 + test/integration/helpers.js | 11 ++ test/integration/server.js | 381 +++++++++++++++++++++--------------- 3 files changed, 233 insertions(+), 163 deletions(-) diff --git a/lib/model/txproposal.js b/lib/model/txproposal.js index 2a3d115..af4f764 100644 --- a/lib/model/txproposal.js +++ b/lib/model/txproposal.js @@ -209,6 +209,10 @@ TxProposal.prototype.getBitcoreTx = function() { return t; }; +TxProposal.prototype.getNetworkName = function() { + return this.network; +}; + TxProposal.prototype.getRawTx = function() { var t = this.getBitcoreTx(); diff --git a/test/integration/helpers.js b/test/integration/helpers.js index 0f2e9d3..e816790 100644 --- a/test/integration/helpers.js +++ b/test/integration/helpers.js @@ -468,4 +468,15 @@ helpers.createAddresses = function(server, wallet, main, change, cb) { }); }; +helpers.createAndPublishTx = function(server, txOpts, signingKey, cb) { + server.createTx(txOpts, function(err, txp) { + should.not.exist(err); + var publishOpts = helpers.getProposalSignatureOpts(txp, signingKey); + server.publishTx(publishOpts, function(err) { + should.not.exist(err); + return cb(txp); + }); + }); +}; + module.exports = helpers; diff --git a/test/integration/server.js b/test/integration/server.js index 43267d8..16cbce6 100644 --- a/test/integration/server.js +++ b/test/integration/server.js @@ -3429,207 +3429,262 @@ describe('Wallet service', function() { describe('#broadcastTx & #broadcastRawTx', function() { var server, wallet, txpid, txid; - beforeEach(function(done) { - helpers.createAndJoinWallet(1, 1, function(s, w) { - server = s; - wallet = w; - helpers.stubUtxos(server, wallet, [10, 10], function() { - var txOpts = helpers.createSimpleProposalOpts('18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', 9, TestData.copayers[0].privKey_1H_0, { - message: 'some message' - }); - server.createTxLegacy(txOpts, function(err, txp) { - should.not.exist(err); - should.exist(txp); - var signatures = helpers.clientSign(txp, TestData.copayers[0].xPrivKey_44H_0H_0H); - server.signTx({ - txProposalId: txp.id, - signatures: signatures, - }, function(err, txp) { + describe('Legacy', function() { + + beforeEach(function(done) { + helpers.createAndJoinWallet(1, 1, function(s, w) { + server = s; + wallet = w; + helpers.stubUtxos(server, wallet, [10, 10], function() { + var txOpts = helpers.createSimpleProposalOpts('18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', 9, TestData.copayers[0].privKey_1H_0, { + message: 'some message' + }); + server.createTxLegacy(txOpts, function(err, txp) { should.not.exist(err); should.exist(txp); - txp.isAccepted().should.be.true; - txp.isBroadcasted().should.be.false; - txid = txp.txid; - txpid = txp.id; - done(); + var signatures = helpers.clientSign(txp, TestData.copayers[0].xPrivKey_44H_0H_0H); + server.signTx({ + txProposalId: txp.id, + signatures: signatures, + }, function(err, txp) { + should.not.exist(err); + should.exist(txp); + txp.isAccepted().should.be.true; + txp.isBroadcasted().should.be.false; + txid = txp.txid; + txpid = txp.id; + done(); + }); }); }); }); }); - }); - it('should broadcast a tx', function(done) { - var clock = sinon.useFakeTimers(1234000, 'Date'); - helpers.stubBroadcast(); - server.broadcastTx({ - txProposalId: txpid - }, function(err) { - should.not.exist(err); - server.getTx({ - txProposalId: txpid - }, function(err, txp) { - should.not.exist(err); - should.not.exist(txp.raw); - txp.txid.should.equal(txid); - txp.isBroadcasted().should.be.true; - txp.broadcastedOn.should.equal(1234); - clock.restore(); - done(); - }); - }); - }); - - it('should broadcast a raw tx', function(done) { - helpers.stubBroadcast(); - server.broadcastRawTx({ - network: 'testnet', - rawTx: 'raw tx', - }, function(err, txid) { - should.not.exist(err); - should.exist(txid); - done(); - }); - }); - - it('should fail to brodcast a tx already marked as broadcasted', function(done) { - helpers.stubBroadcast(); - server.broadcastTx({ - txProposalId: txpid - }, function(err) { - should.not.exist(err); + it('should broadcast a tx', function(done) { + var clock = sinon.useFakeTimers(1234000, 'Date'); + helpers.stubBroadcast(); server.broadcastTx({ txProposalId: txpid }, function(err) { - should.exist(err); - err.code.should.equal('TX_ALREADY_BROADCASTED'); + should.not.exist(err); + server.getTx({ + txProposalId: txpid + }, function(err, txp) { + should.not.exist(err); + should.not.exist(txp.raw); + txp.txid.should.equal(txid); + txp.isBroadcasted().should.be.true; + txp.broadcastedOn.should.equal(1234); + clock.restore(); + done(); + }); + }); + }); + + it('should broadcast a raw tx', function(done) { + helpers.stubBroadcast(); + server.broadcastRawTx({ + network: 'testnet', + rawTx: 'raw tx', + }, function(err, txid) { + should.not.exist(err); + should.exist(txid); done(); }); }); - }); - it('should auto process already broadcasted txs', function(done) { - helpers.stubBroadcast(); - server.getPendingTxs({}, function(err, txs) { - should.not.exist(err); - txs.length.should.equal(1); - blockchainExplorer.getTransaction = sinon.stub().callsArgWith(1, null, { - txid: 999 - }); - server.getPendingTxs({}, function(err, txs) { + it('should fail to brodcast a tx already marked as broadcasted', function(done) { + helpers.stubBroadcast(); + server.broadcastTx({ + txProposalId: txpid + }, function(err) { should.not.exist(err); - txs.length.should.equal(0); - done(); + server.broadcastTx({ + txProposalId: txpid + }, function(err) { + should.exist(err); + err.code.should.equal('TX_ALREADY_BROADCASTED'); + done(); + }); }); }); - }); - it('should process only broadcasted txs', function(done) { - helpers.stubBroadcast(); - var txOpts = helpers.createSimpleProposalOpts('18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', 9, TestData.copayers[0].privKey_1H_0, { - message: 'some message 2' - }); - server.createTxLegacy(txOpts, function(err, txp) { - should.not.exist(err); + it('should auto process already broadcasted txs', function(done) { + helpers.stubBroadcast(); server.getPendingTxs({}, function(err, txs) { should.not.exist(err); - txs.length.should.equal(2); + txs.length.should.equal(1); blockchainExplorer.getTransaction = sinon.stub().callsArgWith(1, null, { txid: 999 }); server.getPendingTxs({}, function(err, txs) { should.not.exist(err); - txs.length.should.equal(1); - txs[0].status.should.equal('pending'); - should.not.exist(txs[0].txid); + txs.length.should.equal(0); + done(); + }); + }); + }); + + it('should process only broadcasted txs', function(done) { + helpers.stubBroadcast(); + var txOpts = helpers.createSimpleProposalOpts('18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', 9, TestData.copayers[0].privKey_1H_0, { + message: 'some message 2' + }); + server.createTxLegacy(txOpts, function(err, txp) { + should.not.exist(err); + server.getPendingTxs({}, function(err, txs) { + should.not.exist(err); + txs.length.should.equal(2); + blockchainExplorer.getTransaction = sinon.stub().callsArgWith(1, null, { + txid: 999 + }); + server.getPendingTxs({}, function(err, txs) { + should.not.exist(err); + txs.length.should.equal(1); + txs[0].status.should.equal('pending'); + should.not.exist(txs[0].txid); + done(); + }); + }); + }); + }); + + it('should fail to brodcast a not yet accepted tx', function(done) { + helpers.stubBroadcast(); + var txOpts = helpers.createSimpleProposalOpts('18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', 9, TestData.copayers[0].privKey_1H_0, { + message: 'some message' + }); + server.createTxLegacy(txOpts, function(err, txp) { + should.not.exist(err); + should.exist(txp); + server.broadcastTx({ + txProposalId: txp.id + }, function(err) { + should.exist(err); + err.code.should.equal('TX_NOT_ACCEPTED'); + done(); + }); + }); + }); + + it('should keep tx as accepted if unable to broadcast it', function(done) { + blockchainExplorer.broadcast = sinon.stub().callsArgWith(1, 'broadcast error'); + blockchainExplorer.getTransaction = sinon.stub().callsArgWith(1, null, null); + server.broadcastTx({ + txProposalId: txpid + }, function(err) { + should.exist(err); + err.toString().should.equal('broadcast error'); + server.getTx({ + txProposalId: txpid + }, function(err, txp) { + should.not.exist(err); + should.exist(txp.txid); + txp.isBroadcasted().should.be.false; + should.not.exist(txp.broadcastedOn); + txp.isAccepted().should.be.true; + done(); + }); + }); + }); + + it('should mark tx as broadcasted if accepted but already in blockchain', function(done) { + blockchainExplorer.broadcast = sinon.stub().callsArgWith(1, 'broadcast error'); + blockchainExplorer.getTransaction = sinon.stub().callsArgWith(1, null, { + txid: '999' + }); + server.broadcastTx({ + txProposalId: txpid + }, function(err) { + should.not.exist(err); + server.getTx({ + txProposalId: txpid + }, function(err, txp) { + should.not.exist(err); + should.exist(txp.txid); + txp.isBroadcasted().should.be.true; + should.exist(txp.broadcastedOn); + done(); + }); + }); + }); + + it('should keep tx as accepted if broadcast fails and cannot check tx in blockchain', function(done) { + blockchainExplorer.broadcast = sinon.stub().callsArgWith(1, 'broadcast error'); + blockchainExplorer.getTransaction = sinon.stub().callsArgWith(1, 'bc check error'); + server.broadcastTx({ + txProposalId: txpid + }, function(err) { + should.exist(err); + err.toString().should.equal('bc check error'); + server.getTx({ + txProposalId: txpid + }, function(err, txp) { + should.not.exist(err); + should.exist(txp.txid); + txp.isBroadcasted().should.be.false; + should.not.exist(txp.broadcastedOn); + txp.isAccepted().should.be.true; done(); }); }); }); }); - - - - - it('should fail to brodcast a not yet accepted tx', function(done) { - helpers.stubBroadcast(); - var txOpts = helpers.createSimpleProposalOpts('18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', 9, TestData.copayers[0].privKey_1H_0, { - message: 'some message' + describe('New', function() { + beforeEach(function(done) { + helpers.createAndJoinWallet(1, 1, function(s, w) { + server = s; + wallet = w; + helpers.stubUtxos(server, wallet, [10, 10], function() { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 9e8, + }], + message: 'some message', + }; + helpers.createAndPublishTx(server, txOpts, TestData.copayers[0].privKey_1H_0, function(txp) { + should.exist(txp); + var signatures = helpers.clientSign(txp, TestData.copayers[0].xPrivKey_44H_0H_0H); + server.signTx({ + txProposalId: txp.id, + signatures: signatures, + }, function(err, txp) { + should.not.exist(err); + should.exist(txp); + txp.isAccepted().should.be.true; + txp.isBroadcasted().should.be.false; + txid = txp.txid; + txpid = txp.id; + done(); + }); + }); + }); + }); }); - server.createTxLegacy(txOpts, function(err, txp) { - should.not.exist(err); - should.exist(txp); + + it('should broadcast a tx', function(done) { + var clock = sinon.useFakeTimers(1234000, 'Date'); + helpers.stubBroadcast(); server.broadcastTx({ - txProposalId: txp.id + txProposalId: txpid }, function(err) { - should.exist(err); - err.code.should.equal('TX_NOT_ACCEPTED'); - done(); - }); - }); - }); - - it('should keep tx as accepted if unable to broadcast it', function(done) { - blockchainExplorer.broadcast = sinon.stub().callsArgWith(1, 'broadcast error'); - blockchainExplorer.getTransaction = sinon.stub().callsArgWith(1, null, null); - server.broadcastTx({ - txProposalId: txpid - }, function(err) { - should.exist(err); - err.toString().should.equal('broadcast error'); - server.getTx({ - txProposalId: txpid - }, function(err, txp) { should.not.exist(err); - should.exist(txp.txid); - txp.isBroadcasted().should.be.false; - should.not.exist(txp.broadcastedOn); - txp.isAccepted().should.be.true; - done(); + server.getTx({ + txProposalId: txpid + }, function(err, txp) { + should.not.exist(err); + should.not.exist(txp.raw); + txp.txid.should.equal(txid); + txp.isBroadcasted().should.be.true; + txp.broadcastedOn.should.equal(1234); + clock.restore(); + done(); + }); }); }); - }); - it('should mark tx as broadcasted if accepted but already in blockchain', function(done) { - blockchainExplorer.broadcast = sinon.stub().callsArgWith(1, 'broadcast error'); - blockchainExplorer.getTransaction = sinon.stub().callsArgWith(1, null, { - txid: '999' - }); - server.broadcastTx({ - txProposalId: txpid - }, function(err) { - should.not.exist(err); - server.getTx({ - txProposalId: txpid - }, function(err, txp) { - should.not.exist(err); - should.exist(txp.txid); - txp.isBroadcasted().should.be.true; - should.exist(txp.broadcastedOn); - done(); - }); - }); - }); - - it('should keep tx as accepted if broadcast fails and cannot check tx in blockchain', function(done) { - blockchainExplorer.broadcast = sinon.stub().callsArgWith(1, 'broadcast error'); - blockchainExplorer.getTransaction = sinon.stub().callsArgWith(1, 'bc check error'); - server.broadcastTx({ - txProposalId: txpid - }, function(err) { - should.exist(err); - err.toString().should.equal('bc check error'); - server.getTx({ - txProposalId: txpid - }, function(err, txp) { - should.not.exist(err); - should.exist(txp.txid); - txp.isBroadcasted().should.be.false; - should.not.exist(txp.broadcastedOn); - txp.isAccepted().should.be.true; - done(); - }); - }); }); }); From 4bad2819667a7bc01a6424f6f1c32ec1f67d8f69 Mon Sep 17 00:00:00 2001 From: Ivan Socolsky Date: Fri, 29 Jan 2016 11:22:20 -0300 Subject: [PATCH 6/6] delay NewTxProposal notification until published --- lib/server.js | 14 ++++++------- test/integration/server.js | 43 +++++++++++++++++++++++++++++++------- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/lib/server.js b/lib/server.js index 337843a..06a270c 100644 --- a/lib/server.js +++ b/lib/server.js @@ -1607,12 +1607,7 @@ WalletService.prototype.createTx = function(opts, cb) { self.storage.storeTx(wallet.id, txp, function(err) { if (err) return cb(err); - - self._notify('NewTxProposal', { - amount: txp.getTotalAmount() - }, function() { - return cb(null, txp); - }); + return cb(null, txp); }); }); }); @@ -1681,7 +1676,12 @@ WalletService.prototype.publishTx = function(opts, cb) { txp.status = 'pending'; self.storage.storeTx(self.walletId, txp, function(err) { if (err) return cb(err); - return cb(); + + self._notify('NewTxProposal', { + amount: txp.getTotalAmount() + }, function() { + return cb(null, txp); + }); }); }); }); diff --git a/test/integration/server.js b/test/integration/server.js index 16cbce6..c6d801b 100644 --- a/test/integration/server.js +++ b/test/integration/server.js @@ -2759,7 +2759,7 @@ describe('Wallet service', function() { }); }); - it('should be able to send a temporary tx proposal', function(done) { + it('should be able to publish a temporary tx proposal', function(done) { helpers.stubUtxos(server, wallet, [1, 2], function() { var txOpts = { outputs: [{ @@ -2786,7 +2786,36 @@ describe('Wallet service', function() { }); }); - it('should fail to send non-existent tx proposal', function(done) { + it('should delay NewTxProposal notification until published', function(done) { + helpers.stubUtxos(server, wallet, [1, 2], function() { + var txOpts = { + outputs: [{ + toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', + amount: 0.8 * 1e8, + }], + message: 'some message', + }; + server.createTx(txOpts, function(err, txp) { + should.not.exist(err); + should.exist(txp); + server.getNotifications({}, function(err, notifications) { + should.not.exist(err); + _.pluck(notifications, 'type').should.not.contain('NewTxProposal'); + var publishOpts = helpers.getProposalSignatureOpts(txp, TestData.copayers[0].privKey_1H_0); + server.publishTx(publishOpts, function(err) { + should.not.exist(err); + server.getNotifications({}, function(err, notifications) { + should.not.exist(err); + _.pluck(notifications, 'type').should.contain('NewTxProposal'); + done(); + }); + }); + }); + }); + }); + }); + + it('should fail to publish non-existent tx proposal', function(done) { server.publishTx({ txProposalId: 'wrong-id', proposalSignature: 'dummy', @@ -2800,7 +2829,7 @@ describe('Wallet service', function() { }); }); - it('should fail to send tx proposal with wrong signature', function(done) { + it('should fail to publish tx proposal with wrong signature', function(done) { helpers.stubUtxos(server, wallet, [1, 2], function() { var txOpts = { outputs: [{ @@ -2824,7 +2853,7 @@ describe('Wallet service', function() { }); }); - it('should fail to send tx proposal not signed by the creator', function(done) { + it('should fail to publish tx proposal not signed by the creator', function(done) { helpers.stubUtxos(server, wallet, [1, 2], function() { var txOpts = { outputs: [{ @@ -2900,7 +2929,7 @@ describe('Wallet service', function() { }); }); - it('should fail to send a temporary tx proposal if utxos are unavailable', function(done) { + it('should fail to publish a temporary tx proposal if utxos are unavailable', function(done) { var txp1, txp2; var txOpts = { outputs: [{ @@ -2931,7 +2960,7 @@ describe('Wallet service', function() { var publishOpts = helpers.getProposalSignatureOpts(txp1, TestData.copayers[0].privKey_1H_0); server.publishTx(publishOpts, next); }, - function(next) { + function(txp, next) { var publishOpts = helpers.getProposalSignatureOpts(txp2, TestData.copayers[0].privKey_1H_0); server.publishTx(publishOpts, function(err) { should.exist(err); @@ -2955,7 +2984,7 @@ describe('Wallet service', function() { var publishOpts = helpers.getProposalSignatureOpts(txp3, TestData.copayers[0].privKey_1H_0); server.publishTx(publishOpts, next); }, - function(next) { + function(txp, next) { server.getPendingTxs({}, function(err, txs) { should.not.exist(err); txs.length.should.equal(2);