MTX.fund - Fixes #666

nodech · 2021-12-03T12:54:30Z

This is alternative implementation of #639 that solves issue in the CoinSelector.

Separate existing and preferred inputs:

existing are the inputs that are present in the mtx.
preferred now refers to inputs passed by options.inputs.
Use CoinView to resolve coins.

Closes #639
Closes #901

nodech · 2021-12-03T17:46:05Z

Test is just repeat of the matrix, it can be rewritten and shortened as a function..

coveralls · 2021-12-03T17:55:51Z

coverage: 69.875% (+0.07%) from 69.802%
when pulling c9cc309 on nodech:mtx-fund
into 9ddb69e on handshake-org:master.

pinheadmz

Questions and comments below. One other thing I think we should test is some signatures / witness stacks. There's no mtx.check() in these tests and one of the important reasons for this patch is to enable the wallet to fund a MTX that already has a signed input. Use cases are coinjoins, atomic swaps, wallet plugins that handle fancy non standard scripts, etc. We already kinda have that in the atomic-swap test but I think it'd be prudent to include at least one external signature check in mtx-test.js as well.

pinheadmz · 2022-01-02T18:00:19Z

lib/primitives/mtx.js

@@ -1413,6 +1413,27 @@ class MTX extends TX {
 /**
 * Coin Selector
 * @alias module:primitives.CoinSelector


Nice job on the jsdocs properties -- do we actually need this alias?

http://hsd-dev.org/hsd/module-primitives.CoinSelector.html

I'm not sure why it needs to be listed as a primitive since the code is not in the primitves/ directory...

http://hsd-dev.org/hsd/module-primitives.html

Also we're not describing the options here but I see that is missing from many classes in hsd, so that should be a future project...

hm.. CoinSelector is part of the primitives/mtx - so I guess that's the reason?

Will also add options. It's almost exact as properties but still.

lib/primitives/mtx.js

pinheadmz · 2022-01-02T18:25:08Z

test/mtx-test.js

+      const coin = dummyCoin(wallet1.getAddress(), 1000000);
+
+      mtx.addOutput(wallet2.getAddress(), 1500000);
+      mtx.view.addCoin(coin);


Could also be mtx.addCoin(coin) right? I tried this and test passed -- is there any significant difference?

Same as above. We want to make sure view is processed properly.

pinheadmz · 2022-01-02T18:41:39Z

lib/primitives/mtx.js


+    // Ensure we have coins for the existing inputs.
+    const view = this.tx.view;


This is confusing me a little bit because the constructor already assigns this.tx = tx.clone(); and this.view = tx.view; Except for the last for loop (line 1782) you use this.view which is what I'd expect ...?

Because mtx.clone does not clone coinview, this.tx has fresh view.

So this method ensures we have all necessary coins in the original CoinView and copies to the new one.

lib/primitives/mtx.js

pinheadmz · 2022-01-02T19:08:36Z

Another thing I noticed is that in the worst case scenario, we will loop through this.coins twice and then iterate through a third time to fill the remaining inputs -- do you think it makes any sense to merge the existing & preferred loops? This will also get a lot better when we refactor txdb and not pull ALL COINS out for every tx...

pinheadmz · 2022-01-02T19:11:06Z

lib/primitives/mtx.js

+    for (const coin of coins) {
+      view.addCoin(coin);
+      this.viewCoins.push(coin);


Why is this ending different than resolvePreferred()? Do we already have these coins in chosen?

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

this.tx.addCoin will also create inputs in the tx. We don't want to do that here, because those inputs already exist in the this.tx. We are just updating view (Comment above)

pinheadmz · 2022-01-02T19:15:30Z

lib/primitives/mtx.js

+      return;
+
+    while (this.index < this.coins.length) {
+      const coin = this.coins[this.index++];


Is it possible that a coin would be added twice? If, for example, this.coins[1] was actually spent by one of the preferred or existing inputs, could that coin be mistakenly added a second time to fund the rest of the transaction? I actually am not even sure mtx.check() would catch a double-spend like this, we might need an explicit test

pinheadmz · 2022-01-02T19:42:27Z

The anyone can renew test can also be updated:

hsd/test/anyone-can-renew-test.js

Lines 309 to 312 in 06aabff

    
           // Have to add this last because wallet fund clears existing input data 
        
           const witness = new Stack(); 
        
           witness.pushData(script.encode()); 
        
           mtx.inputs[0].witness.fromStack(witness);

pinheadmz · 2022-03-01T16:16:41Z

@nodech checking in on this?

nodech · 2022-03-09T13:16:38Z

One thing that users will have to consider with these changes is that, coinview should always be supplied with coin information when using preferred inputs and existing inputs. If you don't add coins to the coinview, the code will try to fill them in from the coin list it gets, but if the coin list is big that will hit the performance. - That feature is there to help users just in case.

As for double spend question: It's not job of the CoinSelector to resolve double spents that happen because of the existing and preferred inputs. The coinselector itself wont double spend the coins if they are correctly applied to the coinview.

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.

nodech changed the title ~~MTX - use coinview to resolve preferred inputs~~ MTX.fund - Fixes Dec 3, 2021

nodech force-pushed the mtx-fund branch from 5c41f3d to 29f97c0 Compare December 3, 2021 17:51

nodech added enhancement general - improving existing feature primitives part of the codebase tests part of the codebase patch Release version bug general - Something isn't working labels Dec 3, 2021

pinheadmz reviewed Jan 2, 2022

View reviewed changes

lib/primitives/mtx.js Outdated Show resolved Hide resolved

pinheadmz reviewed Jan 2, 2022

View reviewed changes

lib/primitives/mtx.js Outdated Show resolved Hide resolved

pinheadmz reviewed Jan 2, 2022

View reviewed changes

handshake-org deleted a comment from Gksvxc7up Jan 31, 2022

Anunayj mentioned this pull request Mar 6, 2022

Wallet.fund() messes up existing inputs #639

Closed

nodech force-pushed the mtx-fund branch from 29f97c0 to 6bb7bb0 Compare March 9, 2022 13:36

Anunayj mentioned this pull request Apr 12, 2022

test: Turbo Blinds Documentation Tests #711

Closed

nodech and others added 3 commits August 22, 2024 14:43

mtx: use coinview to resolve preferred inputs.

9a947bd

Improved interactive-name-swap test

a11d4ab

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.

mtx: separate preferred and existing inputs.

7e69a76

nodech force-pushed the mtx-fund branch from 6bb7bb0 to 7e69a76 Compare August 22, 2024 10:44

mtx: rename existing inputs.

c9cc309

nodech mentioned this pull request Aug 26, 2024

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

Merged

nodech added breaking-major Backwards incompatible - Release version and removed patch Release version labels Aug 27, 2024

nodech closed this in #901 Aug 27, 2024

nodech deleted the mtx-fund branch August 27, 2024 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MTX.fund - Fixes #666

MTX.fund - Fixes #666

nodech commented Dec 3, 2021 •

edited

Loading

nodech commented Dec 3, 2021

coveralls commented Dec 3, 2021 •

edited

Loading

pinheadmz left a comment

pinheadmz Jan 2, 2022

pinheadmz Jan 2, 2022

nodech Mar 3, 2022

pinheadmz Jan 2, 2022

nodech Mar 3, 2022

pinheadmz Jan 2, 2022

nodech Mar 3, 2022

pinheadmz commented Jan 2, 2022

pinheadmz Jan 2, 2022

nodech Mar 3, 2022

pinheadmz Jan 2, 2022

pinheadmz commented Jan 2, 2022

pinheadmz commented Mar 1, 2022

nodech commented Mar 9, 2022


		// Ensure we have coins for the existing inputs.
		const view = this.tx.view;

MTX.fund - Fixes #666

MTX.fund - Fixes #666

Conversation

nodech commented Dec 3, 2021 • edited Loading

nodech commented Dec 3, 2021

coveralls commented Dec 3, 2021 • edited Loading

pinheadmz left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinheadmz commented Jan 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinheadmz commented Jan 2, 2022

pinheadmz commented Mar 1, 2022

nodech commented Mar 9, 2022

nodech commented Dec 3, 2021 •

edited

Loading

coveralls commented Dec 3, 2021 •

edited

Loading