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.

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

Refactor coin selection to leverage NBitcoin #2116

Merged
merged 41 commits into from Aug 19, 2019

Conversation

@NicolasDorier
Copy link
Collaborator

commented Aug 13, 2019

Fix #2111

Nice feature included in NBitcoin:

  • Automatically spend all UTXO which have same addresses
  • Remove below dust outputs
  • Widely tested and used in prod

Tests pass. (Tested SendTestsFromHiddenWalletAsync)

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2019

As you can see the code is way more readable as well.

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:refactor/coinselection branch 7 times, most recently from 97dbdd8 to eee13f9 Aug 13, 2019

@lontivero
Copy link
Collaborator

left a comment

I totally agree the code is way more readable. I also like the fact that the TransactionBuilder allows to set a coin selection strategy.

The problem that I see here is that we have no a single unit test for this critical method that give us at least a minimum degree of confidence that the changes are not breaking anything. Additionally, we know this introduces some very small breaking changes, I mean, while the transaction builder has the concept of DustProtection enabled by default, Wasabi doesn't. So, the built transaction can be different.

I like a lot this kind of PRs but we need tests or at least a way we can prove the change in safe to merge.


Update: Okay, forget my comment about dust protection. I will see if SendTestsFromHiddenWalletAsync covers all this changes.

Could you @NicolasDorier write some tests for the new SmartCoinSelector? That would help.

WalletWasabi/Services/SmartCoinSelector.cs Outdated Show resolved Hide resolved
WalletWasabi/Services/SmartCoinSelector.cs Outdated Show resolved Hide resolved
WalletWasabi/Services/SmartCoinSelector.cs Outdated Show resolved Hide resolved
WalletWasabi/Services/SmartCoinSelector.cs Outdated Show resolved Hide resolved
WalletWasabi/Services/WalletService.cs Outdated Show resolved Hide resolved
WalletWasabi/Services/WalletService.cs Show resolved Hide resolved
WalletWasabi/Services/WalletService.cs Outdated Show resolved Hide resolved
WalletWasabi/Services/WalletService.cs Outdated Show resolved Hide resolved
WalletWasabi/Services/WalletService.cs Outdated Show resolved Hide resolved
@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

Tested it the following:

  • send two coins from faucet to the same address
  • in the send tab, select one of the two coins.
  • send half the value of the one coin to myself.

result is that only the one selected coin [not both address reuse coins] are in the input, the output is my destination address and my change address.

I'm not sure if this would test this PR though...

@benthecarman

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

Concept ACK, looks a lot cleaner

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

Concept ACK. RegTests pass. Will review after Lucas's review is addressed.

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:refactor/coinselection branch from eee13f9 to 6d80c1b Aug 15, 2019

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 15, 2019

@lontivero @yahiheb I fixed what I could.

@lontivero

This comment has been minimized.

Copy link
Collaborator

commented Aug 16, 2019

@NicolasDorier I have isolated (moved) the refactored method in order to write a couple of unit tests without so many dependencies. If you run these tests (I have to write a lot more test) you will see that there is one that fails because the refactored code changed the behavior.


Do not worry for my changes, you can removed them. My idea is after merging this PR, move this method to the TransactionProcessor class and add the UTs you see here and others.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2019

@lontivero so can I remove your changes? I think the tests are good enough, but your bug on #2116 (comment) should definitvely be tested is SendTestsFromHiddenWalletAsync.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2019

@lontivero so the exception is not a bug.

SendAll is failing because the coins you have are too small to fit in an output without being dust. So the destination of SendAll is ignored, and when I try to substract the fee then it gives the exception.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2019

The SendAll is also already tested.

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:refactor/coinselection branch from 116e385 to 6fdd38d Aug 16, 2019

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2019

I reverted your changes outside of the Guard. As there is no bug, and the exception you saw is expected. (The output created by SendAll is too small to support the fees)

nopara73 added 4 commits Aug 17, 2019
@nopara73
Copy link
Collaborator

left a comment

This PR is good to be merged as far as I'm concerned.

nopara73 added 2 commits Aug 18, 2019
@nopara73
Copy link
Collaborator

left a comment

If you want to consolidate many coins, then builder.Check(tx, feeRate); fails.

Reproduce: Send tab: select 50 coins, click MAX and send.

It fails because we asked for a fee rate of 144s/b, but the transaction builder somehow created 144.5s/b.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 18, 2019

@nopara73 this should not happen. I need to reproduce it. One sec.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 18, 2019

In NBitcoin:

if(expectedFees != null)
			{
				var fees = tx.GetFee(coins);
				if(fees != null)
				{
					Money margin = Money.Zero;
					if(DustPrevention)
						margin = GetDust() * 2;
					if(!fees.Almost(expectedFees, margin))
						exceptions.Add(new NotEnoughFundsPolicyError("Fees different than expected", expectedFees - fees));
				}
			}

Because I compare that the fees are more or less different than no more than Dust amount, if the feerate is high then it can happen.

Let me fix that in NBitcoin. (there is no way I can know for the user what is "near enough")

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 18, 2019

@nopara73 I rewrote the code for the fee check. It is more readable and more conservative. (It should not be possible to bypass your fee check)

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Aug 19, 2019

@NicolasDorier You have a Money.Almost function, you could have it for the FeeRate class, too. Or configure the Check function with the expected inaccuracy. There are many of syntactical things we can do.

Zooming out from the micro, the question is more like why the txbuilder builds transaction with different feerate? Is it some fundamental issue or just a bug?

var parts = Guard.Correct(label).Split(',', StringSplitOptions.RemoveEmptyEntries);
foreach (var corrected in parts.Select(Guard.Correct))
{
if (corrected == "")

This comment has been minimized.

Copy link
@lontivero

lontivero Aug 19, 2019

Collaborator

I think this is not possible.

This comment has been minimized.

Copy link
@nopara73

nopara73 Aug 19, 2019

Collaborator

Guard.Correct corrects everything to "". Whitespaces, nulls. So it should be possible.

@lontivero
Copy link
Collaborator

left a comment

Tested. Works okay in all the tested scenarios.

@lontivero lontivero merged commit 2f7d769 into zkSNACKs:master Aug 19, 2019

4 checks passed

CodeFactor No issues found.
Details
Wasabi.Linux #20190819.4 succeeded
Details
Wasabi.Osx #20190819.4 succeeded
Details
Wasabi.Windows #20190819.4 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.