Wallet.fund() messes up existing inputs #639

Anunayj · 2021-10-02T05:45:21Z

wallet.fund() used to mess up existing inputs for reasons mentioned below, this made it incredibly awkward to create transactions with custom inputs, this should fix those issues.

As I understand this, the issue was

The CoinSelector works by making a clone of the MTX and trying to add inputs to it till the output's value is met, and then returning those coins.

hsd/lib/primitives/mtx.js

Lines 1420 to 1427 in 484ede0

    
              * Create a coin selector. 
        
              * @constructor 
        
              * @param {TX} tx 
        
              * @param {Object?} options 
        
              */ 
        
             constructor(tx, options) { 
        
               this.tx = tx.clone();

Because of how MTX cloning is done, the CoinView is not cloned, CoinView contained the input values, without which determining if a inputAmount is adequate is not possible.

hsd/lib/primitives/mtx.js

Lines 103 to 109 in 484ede0

    
              * Clone the transaction. Note that 
        
              * this will not carry over the view. 
        
              * @returns {MTX} 
        
              */ 
        
             inject(mtx) { 
        
               assert(mtx instanceof this.constructor);

So there is this "hack" in place which basically works by checking the prevout in the utxo set, and then "adding them back" and returning their list, but this would not always work cause the prevout could possibly be not in the utxo.

hsd/lib/primitives/mtx.js

Lines 1692 to 1717 in 484ede0

    
           if (this.inputs.size > 0) { 
        
             const coins = []; 
        
             for (let i = 0; i < this.inputs.size; i++) 
        
               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); 
        
             } 
        
           }

After which all the inputs of the MTX are wiped and the inputs from above are added "back"

hsd/lib/primitives/mtx.js

Lines 1126 to 1133 in 484ede0

    
           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);

~~This required changes to the current implementation of interactive-name-swap test since it used a "hack" which is no longer needed.~~
Backward compatibility with the previous hack is maintained for now.
Compatibility with Bob and Shakedex needs to be checked before merging, but is likely gonna be a non-issue.

…#639

refer to handshake-org/hsd#639

coveralls · 2021-10-02T14:41:01Z

Pull Request Test Coverage Report for Build 1527358618

24 of 27 (88.89%) changed or added relevant lines in 2 files are covered.
1148 unchanged lines in 8 files lost coverage.
Overall coverage increased (+0.3%) to 62.505%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/primitives/mtx.js	12	15	80.0%

Files with Coverage Reduction	New Missed Lines	%
lib/protocol/consensus.js	1	82.79%
lib/mining/template.js	7	87.2%
lib/net/packets.js	13	93.16%
lib/node/node.js	30	65.98%
lib/wallet/walletdb.js	34	78.96%
lib/primitives/tx.js	153	69.41%
lib/blockchain/chain.js	313	66.56%
lib/node/rpc.js	597	30.59%

Totals
Change from base Build 1287810189:	0.3%
Covered Lines:	21237
Relevant Lines:	31784

💛 - Coveralls

MTX clone() would not carry over coinview, in order to get around this the CoinSelector would remove all inputs and add them back which made working with partially signed transactions a pain, this should fix that issue.

Previous version of interactive-name-swap used a ugly hack to get around limitations on wallet funding, since wallet.fund() no longer wipes previous inputs, the previous implementation is not longer the recommended way.

Anunayj · 2021-10-02T15:55:10Z

lib/wallet/wallet.js

@@ -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);


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.

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

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

pinheadmz · 2021-11-30T17:07:38Z

Have you looked in to this?

I wonder why we don't clone the view here, and how much simpler that will make your PR?

hsd/lib/primitives/mtx.js

Lines 102 to 113 in eea15c2

    
             /** 
        
              * Clone the transaction. Note that 
        
              * this will not carry over the view. 
        
              * @returns {MTX} 
        
              */ 
        
             inject(mtx) { 
        
               assert(mtx instanceof this.constructor); 
        
               super.inject(mtx); 
        
               this.changeIndex = mtx.changeIndex; 
        
               return this; 
        
             }

For example you probbaly won't need to add this.parent = tx to CoinSelector consturctor

test/interactive-swap-test.js

pinheadmz · 2021-11-30T19:13:58Z

I'm going to continue to review this, but I think it would be helpful to have an explicit test case to show what the change is. i.e. a straightforward test that fails without the patch and succeeds with the patch. I know we have the atomic swap example working already but I think something simple in mtx-test.js or wallet-test.js would be helpful too.

nodech · 2021-12-02T10:09:41Z

lib/primitives/mtx.js

+    // 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())


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.

nodech · 2021-12-02T10:15:20Z

lib/primitives/mtx.js

@@ -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


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.

nodech · 2021-12-02T10:18:00Z

lib/primitives/mtx.js

@@ -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
+    for (const input of this.inputs) {


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)

nodech · 2021-12-02T10:22:53Z

lib/primitives/mtx.js

@@ -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:


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.

nodech · 2021-12-02T10:27:58Z

lib/primitives/mtx.js

+    // (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) {


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 )

nodech · 2021-12-02T10:29:43Z

lib/primitives/mtx.js

-    if (this.inputs.size > 0) {
-      const coins = [];
-
-      for (let i = 0; i < this.inputs.size; i++)


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.

nodech · 2021-12-02T10:31:05Z

lib/wallet/wallet.js

@@ -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);


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

Anunayj · 2022-03-06T16:16:20Z

Closing in favour of #666

Anunayj force-pushed the coinview-fix branch from 1f601d2 to 86de795 Compare October 2, 2021 11:12

Anunayj added a commit to Anunayj/bob-wallet that referenced this pull request Oct 2, 2021

replaced addOutpoint with addCoin with reference to handshake-org/hsd…

30d9a78

…#639

Anunayj added a commit to Anunayj/bob-wallet that referenced this pull request Oct 2, 2021

replaced addOutpoint with addCoin

ded3d5b

refer to handshake-org/hsd#639

Anunayj force-pushed the coinview-fix branch from 86de795 to 485b17c Compare October 2, 2021 15:33

Anunayj added 2 commits October 2, 2021 21:08

Fixed MTX cloning not carrying over CoinView

961645a

MTX clone() would not carry over coinview, in order to get around this the CoinSelector would remove all inputs and add them back which made working with partially signed transactions a pain, this should fix that issue.

Improved interactive-name-swap test

c9d0d8d

Previous version of interactive-name-swap used a ugly hack to get around limitations on wallet funding, since wallet.fund() no longer wipes previous inputs, the previous implementation is not longer the recommended way.

Anunayj force-pushed the coinview-fix branch from 485b17c to c9d0d8d Compare October 2, 2021 15:39

Anunayj commented Oct 2, 2021

View reviewed changes

Anunayj marked this pull request as ready for review October 3, 2021 15:25

pinheadmz mentioned this pull request Oct 21, 2021

shakedex: use derive path for private key kyokan/bob-wallet#416

Open

5 tasks

Anunayj changed the title ~~Wallet.find() messes up existing inputs~~ Wallet.fund() messes up existing inputs Oct 25, 2021

Anunayj mentioned this pull request Oct 26, 2021

Split domain management #651

Closed

pinheadmz reviewed Nov 30, 2021

View reviewed changes

test/interactive-swap-test.js Outdated Show resolved Hide resolved

pinheadmz reviewed Nov 30, 2021

View reviewed changes

test/interactive-swap-test.js Outdated Show resolved Hide resolved

pinheadmz added 4 commits December 1, 2021 12:47

mtx: nits and comments about preferred inputs and coins

3506db5

mtx: do not duplicate inputs if adding duplicate coin

76b06e2

mtx: inject spent coins from view when cloning

43bf26c

mtx: do not search for preferred coins we already have in view

edc1f25

nodech requested changes Dec 2, 2021

View reviewed changes

nodech added bug general - Something isn't working primitives part of the codebase wallet part of the codebase coins part of the codebase labels Dec 2, 2021

nodech mentioned this pull request Dec 3, 2021

MTX.fund - Fixes #666

Closed

nodech added the patch Release version label Dec 9, 2021

Anunayj closed this Mar 6, 2022

Anunayj mentioned this pull request Aug 20, 2023

Lockup Loans: Staking on Handshake opensystm/handshake-micro-grants#2

Closed

nodech mentioned this pull request Aug 26, 2024

MTX - Resolve coins from coinview as well during coinselection. #901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wallet.fund() messes up existing inputs #639

Wallet.fund() messes up existing inputs #639

Anunayj commented Oct 2, 2021 •

edited

Loading

coveralls commented Oct 2, 2021 •

edited

Loading

Anunayj Oct 2, 2021

pinheadmz Dec 1, 2021

nodech Dec 2, 2021

pinheadmz commented Nov 30, 2021

pinheadmz commented Nov 30, 2021

nodech Dec 2, 2021

nodech Dec 2, 2021

nodech Dec 2, 2021

nodech Dec 2, 2021

nodech Dec 2, 2021

nodech Dec 2, 2021

nodech Dec 2, 2021

Anunayj commented Mar 6, 2022

	* Create a coin selector.
	* @constructor
	* @param {TX} tx
	* @param {Object?} options
	*/

	constructor(tx, options) {
	this.tx = tx.clone();

	* Clone the transaction. Note that
	* this will not carry over the view.
	* @returns {MTX}
	*/

	inject(mtx) {
	assert(mtx instanceof this.constructor);

	if (this.inputs.size > 0) {
	const coins = [];

	for (let i = 0; i < this.inputs.size; i++)
	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);
	}
	}

	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);

Wallet.fund() messes up existing inputs #639

Wallet.fund() messes up existing inputs #639

Conversation

Anunayj commented Oct 2, 2021 • edited Loading

coveralls commented Oct 2, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1527358618

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinheadmz commented Nov 30, 2021

pinheadmz commented Nov 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anunayj commented Mar 6, 2022

Anunayj commented Oct 2, 2021 •

edited

Loading

coveralls commented Oct 2, 2021 •

edited

Loading