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
Merged
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
6d80c1b
Refactor coin selection to leverage NBitcoin
NicolasDorier Aug 13, 2019
6fdd38d
Use Guard to check ctor parameter
lontivero Aug 15, 2019
ac9f252
Merge branch 'master' into pr/2116-refactor-coin-selection-to-leverag…
nopara73 Aug 16, 2019
f4642f5
codemaid + fix nullcheck
nopara73 Aug 16, 2019
9c2928a
use property
nopara73 Aug 16, 2019
b1399e5
Set SmartCoinsByOutpoint
nopara73 Aug 16, 2019
b33bbc1
Restore an original line for readability.
nopara73 Aug 16, 2019
96b914b
Improve comment.
nopara73 Aug 16, 2019
e8d0a8c
Use != null for readability
nopara73 Aug 16, 2019
1bc0a4e
Invert if (so it'll be closer to how we used in the original code.)
nopara73 Aug 16, 2019
efdbd7e
Introduce PaymentIntent instead of operation.
nopara73 Aug 16, 2019
f58ee19
When sending, do not create change if not needed
NicolasDorier Aug 16, 2019
e568f06
Send proper error message if we send a too small amount
NicolasDorier Aug 16, 2019
fa40150
Fix change labeling.
nopara73 Aug 16, 2019
d5d0a75
Merge branch 'refactor/coinselection' of https://github.com/NicolasDo…
nopara73 Aug 16, 2019
ad739f0
Use calculated property.
nopara73 Aug 16, 2019
7c7b598
fix typo
nopara73 Aug 16, 2019
d6ceb00
Improve grammar
NicolasDorier Aug 17, 2019
f9bd25e
Use NBitcoin SendAllRemaining instead of SendAll
NicolasDorier Aug 17, 2019
5bb0c48
bump NBitcoin
NicolasDorier Aug 17, 2019
9479142
Merge branch 'master' into pr/2116-refactor-coin-selection-to-leverag…
nopara73 Aug 17, 2019
7f6c168
fix fee percent calculation
nopara73 Aug 17, 2019
6772692
Make sure there's difference between change and all remaining.
nopara73 Aug 17, 2019
db3fca4
Remove leftover numbering comments
nopara73 Aug 17, 2019
6705cfa
Introduce FeeStrategy
nopara73 Aug 17, 2019
0e9e8ea
Make sure to add to "allowedcoins" even if they're not selected.
nopara73 Aug 17, 2019
0f3b168
fix off by one bug, make sure with the "max" button we cannot acciden…
nopara73 Aug 17, 2019
68b4b37
Implement address reuse consolidation
nopara73 Aug 17, 2019
bb98c40
Fix some datadir issues
nopara73 Aug 17, 2019
bc5a813
Don't address reuse in test
nopara73 Aug 17, 2019
592409a
Improve labeling
nopara73 Aug 17, 2019
ee8886a
Fix naming
nopara73 Aug 17, 2019
4d3be06
fix naming in other places, too
nopara73 Aug 17, 2019
83aa0cc
Use FeeStrategy.TwentyMinutesConfirmationTargetStrategy
nopara73 Aug 17, 2019
23ccb13
target -> rate
nopara73 Aug 17, 2019
ce81960
Improve exception message.
nopara73 Aug 18, 2019
5942c5d
Remove ToArray
nopara73 Aug 18, 2019
dd81c48
Improve readability
nopara73 Aug 18, 2019
c07befc
Fix checkresult
nopara73 Aug 18, 2019
3b7d977
Make fee check explicit on WasabiWallet side
NicolasDorier Aug 18, 2019
f53c454
satoshi -> sat
nopara73 Aug 19, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Next

Refactor coin selection to leverage NBitcoin

  • Loading branch information...
NicolasDorier committed Aug 13, 2019
commit 6d80c1b4791a84752516f6e93e518930224195ff
@@ -0,0 +1,89 @@
using NBitcoin;
using System.Linq;
using System;
using System.Collections.Generic;
using System.Text;
using WalletWasabi.Models;
using WalletWasabi.Exceptions;

namespace WalletWasabi.Services
{
internal class SmartCoinSelector : ICoinSelector
{
private Dictionary<OutPoint, SmartCoin> _smartCoinsByOutpoint;

public SmartCoinSelector(Dictionary<OutPoint, SmartCoin> smartCoinsByOutpoint)
{
_smartCoinsByOutpoint = smartCoinsByOutpoint ?? throw new ArgumentNullException(nameof(smartCoinsByOutpoint));
}

public IEnumerable<ICoin> Select(IEnumerable<ICoin> coins, IMoney target)
{
var coinsByOutpoint = coins.ToDictionary(c => c.Outpoint);
var totalOutAmount = (Money)target;
var unspentCoins = coins.Select(c => _smartCoinsByOutpoint[c.Outpoint]).ToArray();
var coinsToSpend = new HashSet<SmartCoin>();
var unspentConfirmedCoins = new List<SmartCoin>();
var unspentUnconfirmedCoins = new List<SmartCoin>();
foreach (SmartCoin coin in unspentCoins)
{
if (coin.Confirmed)
{
unspentConfirmedCoins.Add(coin);
}
else
{
unspentUnconfirmedCoins.Add(coin);
}
}

bool haveEnough = TrySelectCoins(coinsToSpend, totalOutAmount, unspentConfirmedCoins);
if (!haveEnough)
{
haveEnough = TrySelectCoins(coinsToSpend, totalOutAmount, unspentUnconfirmedCoins);
}

if (!haveEnough)
{
throw new InsufficientBalanceException(totalOutAmount, unspentConfirmedCoins.Select(x => x.Amount).Sum() + unspentUnconfirmedCoins.Select(x => x.Amount).Sum());
This conversation was marked as resolved by lontivero

This comment has been minimized.

Copy link
@lontivero

lontivero Aug 13, 2019

Collaborator

Do you think we can make it more readable with something like the follow:

Suggested change
throw new InsufficientBalanceException(totalOutAmount, unspentConfirmedCoins.Select(x => x.Amount).Sum() + unspentUnconfirmedCoins.Select(x => x.Amount).Sum());
var actualAmount = unspentConfirmedCoins.Concat(unspentUnconfirmedCoins).Sum(x => x.Amount);
throw new InsufficientBalanceException(totalOutAmount, actualAmount);

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Aug 15, 2019

Author Collaborator

probably, I copy/pasta the previous code to facilitate review.

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Aug 15, 2019

Author Collaborator
unspentConfirmedCoins.Concat(unspentUnconfirmedCoins).Sum(x => x.Amount)

This is incorrect, because this will returns a decimal while my current version returns a Money

}

return coinsToSpend.Select(c => coinsByOutpoint[c.GetOutPoint()]);
}

/// <returns>If the selection was successful. If there's enough coins to spend from.</returns>
private bool TrySelectCoins(HashSet<SmartCoin> coinsToSpend, Money totalOutAmount, IEnumerable<SmartCoin> unspentCoins)
{
// If there's no need for input merging, then use the largest selected.
// Do not prefer anonymity set. You can assume the user prefers anonymity set manually through the GUI.
SmartCoin largestCoin = unspentCoins.OrderByDescending(x => x.Amount).FirstOrDefault();
if (largestCoin == default)
{
return false; // If there's no coin then unsuccessful selection.
}
else // Check if we can do without input merging.
{
if (largestCoin.Amount >= totalOutAmount)
{
coinsToSpend.Add(largestCoin);
return true;
}
}

// If there's a need for input merging.
foreach (var coin in unspentCoins
.OrderByDescending(x => x.AnonymitySet) // Always try to spend/merge the largest anonset coins first.
.ThenByDescending(x => x.Amount)) // Then always try to spend by amount.
{
coinsToSpend.Add(coin);
// If reaches the amount, then return true, else just go with the largest coin.
if (coinsToSpend.Select(x => x.Amount).Sum() >= totalOutAmount)
This conversation was marked as resolved by lontivero

This comment has been minimized.

Copy link
@lontivero

lontivero Aug 13, 2019

Collaborator

I think it could be more readable in this way (but it is not really important, btw):

Suggested change
if (coinsToSpend.Select(x => x.Amount).Sum() >= totalOutAmount)
if (coinsToSpend.Sum(x => x.Amount) >= totalOutAmount)

This comment has been minimized.

Copy link
@lontivero

lontivero Aug 13, 2019

Collaborator

If well this will never be a problem for performance, it worth to note that summing inside the foreach loop can be eliminated by using an accumulator.

{
return true;
}
}

return false;
}
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.