From dee6d2efc6982eee4102e0af81c63b792da162d3 Mon Sep 17 00:00:00 2001 From: Rithvik Vibhu Date: Thu, 13 Jul 2023 14:25:20 +0530 Subject: [PATCH] wallet: fix makeBatch to generate addresses early Bid actions in makeBatch had their addresses overridden after nonce generation, making it impossible to recover the wallet with importnonce. With this change, the address is derived and passed to make* so the correct address is used to generate nonces. changes --- lib/wallet/wallet.js | 45 ++++++----- test/wallet-auction-test.js | 19 ++++- test/wallet-importnonce-test.js | 127 ++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 20 deletions(-) create mode 100644 test/wallet-importnonce-test.js diff --git a/lib/wallet/wallet.js b/lib/wallet/wallet.js index f17cbccc5..cae9d4393 100644 --- a/lib/wallet/wallet.js +++ b/lib/wallet/wallet.js @@ -1629,12 +1629,14 @@ class Wallet extends EventEmitter { * @param {String} name * @param {Number|String} acct * @param {MTX?} mtx + * @param {Address?} addr * @returns {Promise} */ - async makeOpen(name, acct, mtx) { + async makeOpen(name, acct, mtx, addr) { assert(typeof name === 'string'); assert((acct >>> 0) === acct || typeof acct === 'string'); + assert(addr == null || addr instanceof Address); if (!rules.verifyName(name)) throw new Error(`Invalid name: ${name}.`); @@ -1670,7 +1672,8 @@ class Wallet extends EventEmitter { if (start !== 0 && start !== height) throw new Error(`Name is already opening: ${name}.`); - const addr = await this.receiveAddress(acct); + if (!addr) + addr = await this.receiveAddress(acct); const output = new Output(); output.address = addr; @@ -1760,14 +1763,16 @@ class Wallet extends EventEmitter { * @param {Number} lockup * @param {Number|String} acct * @param {MTX?} mtx - * @returns {MTX} + * @param {Address?} addr + * @returns {Promise} */ - async makeBid(name, value, lockup, acct, mtx) { + async makeBid(name, value, lockup, acct, mtx, addr) { assert(typeof name === 'string'); assert(Number.isSafeInteger(value) && value >= 0); assert(Number.isSafeInteger(lockup) && lockup >= 0); assert((acct >>> 0) === acct || typeof acct === 'string'); + assert(addr == null || addr instanceof Address); if (!rules.verifyName(name)) throw new Error(`Invalid name: ${name}.`); @@ -1797,7 +1802,9 @@ class Wallet extends EventEmitter { `Bid (${value}) exceeds lockup value (${lockup}): ${name}.` ); - const addr = await this.receiveAddress(acct); + if (!addr) + addr = await this.receiveAddress(acct); + const blind = await this.generateBlind(nameHash, addr, value); const output = new Output(); @@ -3760,6 +3767,11 @@ class Wallet extends EventEmitter { } }); + // Some actions accept output addresses to avoid address reuse. + // We track that by bumping receiveIndex. + const account = await this.getAccount(acct); + let receiveIndex = account.receiveDepth - 1; + // "actions" are arrays that start with a covenant type (or meta-type) // followed by the arguments expected by the corresponding "make" function. for (const action of actions) { @@ -3776,11 +3788,17 @@ class Wallet extends EventEmitter { break; case 'OPEN': assert(action.length === 1, 'Bad arguments for OPEN.'); - await this.makeOpen(...action, acct, mtx); + { + const address = account.deriveReceive(receiveIndex++).getAddress(); + await this.makeOpen(...action, acct, mtx, address); + } break; case 'BID': assert(action.length === 3, 'Bad arguments for BID.'); - await this.makeBid(...action, acct, mtx); + { + const address = account.deriveReceive(receiveIndex++).getAddress(); + await this.makeBid(...action, acct, mtx, address); + } break; case 'REVEAL': if (action.length === 1) { @@ -3842,6 +3860,9 @@ class Wallet extends EventEmitter { if (rules.countRenewals(mtx) > consensus.MAX_BLOCK_RENEWALS) throw new Error('Too many RENEWs.'); + + if (receiveIndex > account.receiveDepth - 1 + account.lookahead) + throw new Error('Batch output addresses would exceed lookahead.'); } if (!mtx.outputs.length) @@ -3850,10 +3871,6 @@ class Wallet extends EventEmitter { // Clean up. // 1. Some actions MUST be the ONLY action for a name. // i.e. no duplicate OPENs or REVOKE/FINALIZE for same name in one tx. - // 2. Some outputs may reuse same address from this.receieveAddress(acct) - // We can bump those to the next receive address, - const account = await this.getAccount(acct); - let receiveIndex = account.receiveDepth - 1; const set = new BufferSet(); for (const output of mtx.outputs) { const {covenant} = output; @@ -3865,13 +3882,10 @@ class Wallet extends EventEmitter { switch (covenant.type) { case types.CLAIM: case types.OPEN: - output.address = account.deriveReceive(receiveIndex++).getAddress(); - assert(!set.has(nameHash), 'Duplicate name with exclusive action.'); set.add(nameHash); break; case types.BID: - output.address = account.deriveReceive(receiveIndex++).getAddress(); case types.REVEAL: case types.REDEEM: break; @@ -3885,9 +3899,6 @@ class Wallet extends EventEmitter { set.add(nameHash); break; } - - if (receiveIndex > account.receiveDepth - 1 + account.lookahead) - throw new Error('Batch output addresses would exceed lookahead.'); } return mtx; diff --git a/test/wallet-auction-test.js b/test/wallet-auction-test.js index a4883a3e5..a9abf7d8f 100644 --- a/test/wallet-auction-test.js +++ b/test/wallet-auction-test.js @@ -828,15 +828,28 @@ describe('Wallet Auction', function() { let startHeight; const oldRenewalWindow = network.names.renewalWindow; - before(() => { + let oldLookahead; + + before(async () => { network.names.renewalWindow = 160; for (let i = 0; i < 800; i++) names.push(`name_${i}`); + + // Increase lookahead + oldLookahead = (await wallet.getAccount('default')).lookahead; + await wallet.modifyAccount('default', { + lookahead: consensus.MAX_BLOCK_OPENS + 1 + }); }); - after(() => { + after(async () => { network.names.renewalWindow = oldRenewalWindow; + + // Reset lookahead + await wallet.modifyAccount('default', { + lookahead: oldLookahead + }); }); it('should not batch too many OPENs', async () => { @@ -846,7 +859,7 @@ describe('Wallet Auction', function() { await assert.rejects( wallet.createBatch(batch), - {message: 'Too many OPENs.'} // Might exceed wallet lookahead also + {message: 'Too many OPENs.'} ); }); diff --git a/test/wallet-importnonce-test.js b/test/wallet-importnonce-test.js new file mode 100644 index 000000000..2e4befbfd --- /dev/null +++ b/test/wallet-importnonce-test.js @@ -0,0 +1,127 @@ +'use strict'; + +const assert = require('bsert'); +const FullNode = require('../lib/node/fullnode'); +const Network = require('../lib/protocol/network'); +const Address = require('../lib/primitives/address'); +const rules = require('../lib/covenants/rules'); + +const network = Network.get('regtest'); + +const node = new FullNode({ + memory: true, + network: network.type, + plugins: [require('../lib/wallet/plugin')] +}); + +const { wdb } = node.require('walletdb'); + +async function mineBlocks(n, addr) { + addr = addr ? addr : new Address().toString(network); + for (let i = 0; i < n; i++) { + const block = await node.miner.mineBlock(null, addr); + await node.chain.add(block); + } +} + +describe('Wallet Import Nonce', function () { + let + /** @type {import('../lib/wallet/wallet')} */ walletA, + /** @type {import('../lib/wallet/wallet')} */ walletB; + + const NAME = rules.grindName(10, 1, network); + const NAMEHASH = rules.hashName(NAME); + const BIDS = [ + { value: 1e6, lockup: 2e6, addr: undefined }, // sendbid + { value: 2e6, lockup: 4e6, addr: undefined }, // -|sendbatch + { value: 4e6, lockup: 8e6, addr: undefined } // -|sendbatch + ]; + + before(async () => { + await node.ensure(); + await node.open(); + + // Both wallets have the same seed + walletA = await wdb.create(); + walletB = await wdb.create({ mnemonic: walletA.master.mnemonic }); + assert.bufferEqual(walletA.master.writeKey(), walletB.master.writeKey()); + }); + + after(async () => { + await node.close(); + }); + + it('should fund wallet', async () => { + await mineBlocks(2, await walletA.receiveAddress()); + }); + + it('should open an auction and advance to bidding period', async () => { + await walletA.sendOpen(NAME); + await mineBlocks(network.names.treeInterval + 1); + }); + + it('should bid with sendbid', async () => { + const bid = BIDS[0]; + + const bidTx = await walletA.sendBid(NAME, bid.value, bid.lockup); + + // Save address for importnonce later + bid.addr = bidTx.outputs[0].address; + }); + + it('should bid with sendbatch', async () => { + const batch = [ + ['BID', NAME, BIDS[1].value, BIDS[1].lockup], + ['BID', NAME, BIDS[2].value, BIDS[2].lockup] + ]; + + const bidTx = await walletA.sendBatch(batch); + + // Save address for importnonce later + for (const output of bidTx.outputs) { + if (!output.covenant.isBid()) + continue; + + const index = BIDS.findIndex(bid => bid.lockup === output.value); + BIDS[index].addr = output.address; + } + }); + + it('should verify bids were placed', async () => { + await mineBlocks(1); + const bidsA = await walletA.getBidsByName(NAME); + assert.strictEqual(bidsA.length, BIDS.length); + }); + + it('should not be known by other wallet', async () => { + const bidsB = await walletB.getBidsByName(NAME); + assert.strictEqual(bidsB.length, BIDS.length); + + for (const bid of bidsB) { + assert.strictEqual(bid.value, -1); + } + }); + + it('should be imported by other wallet', async () => { + for (const bid of BIDS) { + await walletB.generateBlinds(NAMEHASH, bid.addr, bid.value); + } + + const bidsB = await walletB.getBidsByName(NAME); + assert.strictEqual(bidsB.length, BIDS.length); + + // Ensure bids have correct true bid values + for (const bid of bidsB) { + const index = BIDS.findIndex(x => x.lockup === bid.lockup); + assert.strictEqual(BIDS[index].value, bid.value); + } + }); + + it('should reaveal all bids from other wallet', async () => { + await mineBlocks(network.names.biddingPeriod); + + const revealTx = await walletB.sendRevealAll(); + const revealOutputs = revealTx.outputs.filter(out => out.covenant.isReveal()); + assert.strictEqual(revealOutputs.length, BIDS.length); + }); +});