Skip to content

Commit

Permalink
wallet: fix makeBatch to generate addresses early
Browse files Browse the repository at this point in the history
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
  • Loading branch information
rithvikvibhu committed Jul 13, 2023
1 parent 90cdf84 commit dee6d2e
Show file tree
Hide file tree
Showing 3 changed files with 171 additions and 20 deletions.
45 changes: 28 additions & 17 deletions lib/wallet/wallet.js
Expand Up @@ -1629,12 +1629,14 @@ class Wallet extends EventEmitter {
* @param {String} name
* @param {Number|String} acct
* @param {MTX?} mtx
* @param {Address?} addr
* @returns {Promise<MTX>}
*/

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}.`);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<MTX>}
*/

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}.`);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down
19 changes: 16 additions & 3 deletions test/wallet-auction-test.js
Expand Up @@ -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 () => {
Expand All @@ -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.'}
);
});

Expand Down
127 changes: 127 additions & 0 deletions 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);
});
});

0 comments on commit dee6d2e

Please sign in to comment.