Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wallet.fund() messes up existing inputs #639

Closed
wants to merge 6 commits into from
Closed

Wallet.fund() messes up existing inputs #639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
961645a
Fixed MTX cloning not carrying over CoinView
Anunayj Oct 2, 2021
c9d0d8d
Improved interactive-name-swap test
Anunayj Oct 2, 2021
3506db5
mtx: nits and comments about preferred inputs and coins
pinheadmz Dec 1, 2021
76b06e2
mtx: do not duplicate inputs if adding duplicate coin
pinheadmz Dec 1, 2021
43bf26c
mtx: inject spent coins from view when cloning
pinheadmz Dec 1, 2021
edc1f25
mtx: do not search for preferred coins we already have in view
pinheadmz Dec 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 40 additions & 35 deletions lib/primitives/mtx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,21 @@ class MTX extends TX {
}

/**
* Clone the transaction. Note that
* this will not carry over the view.
* Clone the transaction and it's input coins.
* @returns {MTX}
*/

inject(mtx) {
assert(mtx instanceof this.constructor);
super.inject(mtx);
this.changeIndex = mtx.changeIndex;

// CoinView has other properties like UndoCoins and BitView
// that an MTX doesn't care about. We just want a copy of the
// coins spent by the MTX.
for (const [key, value] of mtx.view.map.entries())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason behind not cloning the view is that in most cases it's not necessary and it's complex.
Also we have couple of variations on the view. Notably, the new WalletCoinView needs different approach to be properly cloned.

So in most cases mtx clone is done like:

const nmtx = mtx.clone();
nmtx.view = mtx.view;

Referencing the old view. It needs double checking if there's a case where old way messes things up, but I doubt. Most views in the wallet side are short lived.

this.view.map.set(key, value);

return this;
}

Expand Down Expand Up @@ -158,11 +164,19 @@ class MTX extends TX {
addCoin(coin) {
assert(coin instanceof Coin, 'Cannot add non-coin.');

const input = Input.fromCoin(coin);
// Avoid duplicate inputs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addCoin, addInput, addOutpoint and addTX are utilities that are meant to be as simple as possible.
They don't do anything extra, pretty dumb methods. So complicating addCoin here I don't think is necessary, as you can always use pretty cheap check: isSane or checkSanity after you are done with everything.

for (const input of this.inputs) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another note is this is inefficient way of checking this O(n^2). If we really wanted to do this(and as I mentioned above, we should not), we have to create new Map for the hash/index pair of each input similar to CoinView (Not coinview is not good case for this, as it may not have some coins)

const {hash, index} = input.prevout;
if (hash.equals(coin.hash) && index === coin.index) {
this.view.addCoin(coin);
return input;
}
}

// Coin represents a new input.
const input = Input.fromCoin(coin);
this.inputs.push(input);
this.view.addCoin(coin);

return input;
}

Expand Down Expand Up @@ -1122,12 +1136,31 @@ class MTX extends TX {
assert(options, 'Options are required.');
assert(options.changeAddress, 'Change address is required.');

// Ensure backward compatibility with the old API:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to remove old API? Other than incorrectly managing preferred inputs, was there another thing wrong with it?

As far as I can see, what we want to fix is - when tx.clone ing in the coinSelector, CoinSelector should use existing mtx.view as reference lookup table for these coins and don't expect them to be supplied with coins array that is coming from the database. That way "preferred" inputs that have coins wont get removed, but be checked against supplied coinview.

// Inputs to MTX were added with mtx.addOutpoint() and then later
// CoinSelector.fund() was expected to match those outpoints with coins
// from the wallet. The problem with this is that when CoinSelector
// initialized, it wiped out all the inputs, including some that may have
// already been signed outside the wallet for custom scripts or CoinJoins.
// With the new API, mtx.addCoin() can be used to either fund existing
// (pre-signed) inputs OR add new "preferred" inputs for covenants.
// The logic below was moved here from CoinSelector.fund() in case
// the old method was used and "preferred" coins are not yet in the view.
for (const input of this.inputs) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will become totally unnecessary, if we do the thing mentioned above.

Also Complexity of this will be off charts. We already have issues with the coins being too big for the big wallets, and iterating it for every input will just explode )

const {prevout} = input;
if (this.view.hasEntry(prevout))
continue;

for (const coin of coins) {
if (prevout.hash.equals(coin.hash) && prevout.index === coin.index) {
this.view.addCoin(coin);
}
}
}

// Select necessary coins.
const select = await this.selectCoins(coins, options);

// Make sure we empty the input array.
this.inputs.length = 0;

// Add coins to transaction.
for (const coin of select.chosen)
this.addCoin(coin);
Expand Down Expand Up @@ -1585,7 +1618,6 @@ class CoinSelector {
this.chosen = [];
this.change = 0;
this.fee = CoinSelector.MIN_FEE;
this.tx.inputs.length = 0;

switch (this.selection) {
case 'all':
Expand Down Expand Up @@ -1688,33 +1720,6 @@ class CoinSelector {
*/

fund() {
// Ensure all preferred inputs first.
if (this.inputs.size > 0) {
const coins = [];

for (let i = 0; i < this.inputs.size; i++)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here instead of pushing blindly, we check if view has it, if it already has it we don't push this. Everything else can remain same.

coins.push(null);

for (const coin of this.coins) {
const {hash, index} = coin;
const key = Outpoint.toKey(hash, index);
const i = this.inputs.get(key);

if (i != null) {
coins[i] = coin;
this.inputs.delete(key);
}
}

if (this.inputs.size > 0)
throw new Error('Could not resolve preferred inputs.');

for (const coin of coins) {
this.tx.addCoin(coin);
this.chosen.push(coin);
}
}

if (this.isFull())
return;

Expand Down
25 changes: 12 additions & 13 deletions lib/wallet/wallet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ const {types} = rules;
const {Mnemonic} = HD;
const {BufferSet} = require('buffer-map');
const Coin = require('../primitives/coin');
const Outpoint = require('../primitives/outpoint');

/*
* Constants
Expand Down Expand Up @@ -1839,7 +1838,7 @@ class Wallet extends EventEmitter {
output.covenant.pushHash(nameHash);
output.covenant.pushU32(height);
output.covenant.pushHash(nonce);
reveal.addOutpoint(Outpoint.fromTX(bid, bidOuputIndex));
reveal.addCoin(bidCoin);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are technically not necessary because the Coin would attempted to be added to CoinView if it's already not there. But this is cleaner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think now with this logic: edc1f25 using addCoin() is actually an optimization!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addCoin will still stay an optimization if we use my suggestion.

reveal.outputs.push(output);

await this.fill(reveal, { ...options, coins: coins });
Expand Down Expand Up @@ -1926,7 +1925,7 @@ class Wallet extends EventEmitter {
output.covenant.pushU32(ns.height);
output.covenant.pushHash(nonce);

mtx.addOutpoint(prevout);
mtx.addCoin(coin);
mtx.outputs.push(output);
}

Expand Down Expand Up @@ -2051,7 +2050,7 @@ class Wallet extends EventEmitter {
output.covenant.pushU32(ns.height);
output.covenant.pushHash(nonce);

mtx.addOutpoint(prevout);
mtx.addCoin(coin);
mtx.outputs.push(output);
}

Expand Down Expand Up @@ -2176,7 +2175,7 @@ class Wallet extends EventEmitter {
if (coin.height < ns.height)
continue;

mtx.addOutpoint(prevout);
mtx.addCoin(coin);

const output = new Output();
output.address = coin.address;
Expand Down Expand Up @@ -2298,7 +2297,7 @@ class Wallet extends EventEmitter {
if (coin.height < ns.height)
continue;

mtx.addOutpoint(prevout);
mtx.addCoin(coin);

const output = new Output();
output.address = coin.address;
Expand Down Expand Up @@ -2444,7 +2443,7 @@ class Wallet extends EventEmitter {
output.covenant.pushHash(await this.wdb.getRenewalBlock());

const mtx = new MTX();
mtx.addOutpoint(ns.owner);
mtx.addCoin(coin);
mtx.outputs.push(output);

return mtx;
Expand Down Expand Up @@ -2524,7 +2523,7 @@ class Wallet extends EventEmitter {
output.covenant.push(raw);

const mtx = new MTX();
mtx.addOutpoint(ns.owner);
mtx.addCoin(coin);
mtx.outputs.push(output);

return mtx;
Expand Down Expand Up @@ -2663,7 +2662,7 @@ class Wallet extends EventEmitter {
output.covenant.pushHash(await this.wdb.getRenewalBlock());

const mtx = new MTX();
mtx.addOutpoint(ns.owner);
mtx.addCoin(coin);
mtx.outputs.push(output);

return mtx;
Expand Down Expand Up @@ -2797,7 +2796,7 @@ class Wallet extends EventEmitter {
output.covenant.push(address.hash);

const mtx = new MTX();
mtx.addOutpoint(ns.owner);
mtx.addCoin(coin);
mtx.outputs.push(output);

return mtx;
Expand Down Expand Up @@ -2933,7 +2932,7 @@ class Wallet extends EventEmitter {
output.covenant.push(EMPTY);

const mtx = new MTX();
mtx.addOutpoint(ns.owner);
mtx.addCoin(coin);
mtx.outputs.push(output);

return mtx;
Expand Down Expand Up @@ -3077,7 +3076,7 @@ class Wallet extends EventEmitter {
output.covenant.pushHash(await this.wdb.getRenewalBlock());

const mtx = new MTX();
mtx.addOutpoint(ns.owner);
mtx.addCoin(coin);
mtx.outputs.push(output);

return mtx;
Expand Down Expand Up @@ -3208,7 +3207,7 @@ class Wallet extends EventEmitter {
output.covenant.pushU32(ns.height);

const mtx = new MTX();
mtx.addOutpoint(ns.owner);
mtx.addCoin(coin);
mtx.outputs.push(output);

return mtx;
Expand Down
19 changes: 5 additions & 14 deletions test/interactive-swap-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,11 +208,12 @@ describe('Interactive name swap', function() {

// Bob should verify all the data in the MTX to ensure everything is valid,
// but this is the minimum.
const input0 = mtx.input(0).clone(); // copy input with Alice's signature
const coinEntry = await node.chain.db.readCoin(input0.prevout);
const coinEntry = await node.chain.db.readCoin(mtx.input(0).prevout);
assert(coinEntry); // ensures that coin exists and is still unspent

const coin = coinEntry.toCoin(input0.prevout);
const coin = coinEntry.toCoin(mtx.input(0).prevout);
mtx.addCoin(coin);

assert(coin.covenant.type === types.TRANSFER);
const addr = new Address({
version: coin.covenant.items[2].readInt8(),
Expand All @@ -221,17 +222,7 @@ describe('Interactive name swap', function() {
assert.deepStrictEqual(addr, bobReceive); // transfer is to Bob's address

// Fund the TX.
// The hsd wallet is not designed to handle partially-signed TXs
// or coins from outside the wallet, so a little hacking is needed.
const changeAddress = await bob.changeAddress();
const rate = await wdb.estimateFee();
const coins = await bob.getSmartCoins();
// Add the external coin to the coin selector so we don't fail assertions
coins.push(coin);
await mtx.fund(coins, {changeAddress, rate});
// The funding mechanism starts by wiping out existing inputs
// which for us includes Alice's signature. Replace it from our backup.
mtx.inputs[0].inject(input0);
await bob.fund(mtx);

// Rearrange outputs.
// Since we added a change output, the SINGELREVERSE is now broken:
Expand Down
57 changes: 57 additions & 0 deletions test/mtx-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ const bio = require('bufio');
const CoinView = require('../lib/coins/coinview');
const WalletCoinView = require('../lib/wallet/walletcoinview');
const MTX = require('../lib/primitives/mtx');
const Address = require('../lib/primitives/address');
const Coin = require('../lib/primitives/coin');
const Path = require('../lib/wallet/path');
const common = require('./util/common');

Expand Down Expand Up @@ -62,4 +64,59 @@ describe('MTX', function() {
assert.ok(view instanceof CoinView);
assert.deepStrictEqual(got, want);
});

it('should clone MTX including view', () => {
const coin1 = new Coin({
version: 1,
value: 1000001,
hash: Buffer.alloc(32, 0x01),
index: 1
});

const coin1alt = new Coin({
version: 1,
value: 9999999,
hash: Buffer.alloc(32, 0x01),
index: 1
});

const coin2 = new Coin({
version: 1,
value: 2000002,
hash: Buffer.alloc(32, 0x02),
index: 2
});

const addr = new Address({
version: 0,
hash: Buffer.alloc(20, 0xdb)
});

const value = coin1.value + coin2.value;

const mtx1 = new MTX();
mtx1.addCoin(coin1);
mtx1.addCoin(coin2);
mtx1.addOutput(addr, value);

// Verify clone including view
const mtx2 = mtx1.clone();
assert.deepStrictEqual(mtx1.toJSON(), mtx2.toJSON());
assert.strictEqual(mtx1.getInputValue(), mtx2.getInputValue());
assert.strictEqual(mtx1.view.map.size, 2);
assert.strictEqual(mtx2.view.map.size, 2);

// Sanity check: verify deep clone by modifying original data
mtx1.view.remove(coin1.hash);
assert.notDeepStrictEqual(mtx1.toJSON(), mtx2.toJSON());
assert.notDeepStrictEqual(mtx1.getInputValue(), mtx2.getInputValue());
assert.strictEqual(mtx1.view.map.size, 1);
assert.strictEqual(mtx2.view.map.size, 2);

mtx1.view.addCoin(coin1alt);
assert.notDeepStrictEqual(mtx1.toJSON(), mtx2.toJSON());
assert.notStrictEqual(mtx1.getInputValue(), mtx2.getInputValue());
assert.strictEqual(mtx1.view.map.size, 2);
assert.strictEqual(mtx2.view.map.size, 2);
});
});
Loading