-
Notifications
You must be signed in to change notification settings - Fork 492
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
Coin selection improvement #10096
base: master
Are you sure you want to change the base?
Coin selection improvement #10096
Changes from all commits
164c30c
11fa485
54f3cd4
3cd7ea2
6a691af
ed7d6cc
96d01af
ae6205e
f5a2788
af6f9b3
96056fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -151,6 +151,35 @@ public void SelectTwoNonPrivateCoinsFromTwoCoinsSetOfCoinsConsolidationMode() | |||||||||||||||||||||||||||||||||||||||
Assert.Equal(2, coins.Count); | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
[Fact] | ||||||||||||||||||||||||||||||||||||||||
public void DoNotSelectCoinsWithBigAnonymityLoss() | ||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||
// This test ensures that we do not select coins whose anonymity could be lowered a lot. | ||||||||||||||||||||||||||||||||||||||||
const int AnonymitySet = 10; | ||||||||||||||||||||||||||||||||||||||||
var km = KeyManager.CreateNew(out _, "", Network.Main); | ||||||||||||||||||||||||||||||||||||||||
var bigCoinWithSmallAnonymity1 = BitcoinFactory.CreateSmartCoin(BitcoinFactory.CreateHdPubKey(km), Money.Coins(1m), anonymitySet: 1); | ||||||||||||||||||||||||||||||||||||||||
var bigCoinWithSmallAnonymity2 = BitcoinFactory.CreateSmartCoin(BitcoinFactory.CreateHdPubKey(km), Money.Coins(1m), anonymitySet: 2); | ||||||||||||||||||||||||||||||||||||||||
var smallCoinWithBigAnonymity = BitcoinFactory.CreateSmartCoin(BitcoinFactory.CreateHdPubKey(km), Money.Coins(0.1m), anonymitySet: 6); | ||||||||||||||||||||||||||||||||||||||||
var coinsToSelectFrom = Enumerable | ||||||||||||||||||||||||||||||||||||||||
.Empty<SmartCoin>() | ||||||||||||||||||||||||||||||||||||||||
.Append(bigCoinWithSmallAnonymity1) | ||||||||||||||||||||||||||||||||||||||||
.Append(bigCoinWithSmallAnonymity2) | ||||||||||||||||||||||||||||||||||||||||
.Append(smallCoinWithBigAnonymity) | ||||||||||||||||||||||||||||||||||||||||
.ToList(); | ||||||||||||||||||||||||||||||||||||||||
Comment on lines
+163
to
+168
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An alternative would be:
Suggested change
or
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
var coins = CoinJoinClient.SelectCoinsForRound( | ||||||||||||||||||||||||||||||||||||||||
coins: coinsToSelectFrom, | ||||||||||||||||||||||||||||||||||||||||
UtxoSelectionParameters.FromRoundParameters(CreateMultipartyTransactionParameters()), | ||||||||||||||||||||||||||||||||||||||||
consolidationMode: true, | ||||||||||||||||||||||||||||||||||||||||
anonScoreTarget: AnonymitySet, | ||||||||||||||||||||||||||||||||||||||||
semiPrivateThreshold: 0, | ||||||||||||||||||||||||||||||||||||||||
liquidityClue: Money.Coins(0.5m), | ||||||||||||||||||||||||||||||||||||||||
ConfigureRng(1)); | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
Assert.False(coins.Contains(bigCoinWithSmallAnonymity1) && coins.Contains(smallCoinWithBigAnonymity)); | ||||||||||||||||||||||||||||||||||||||||
Assert.False(coins.Contains(bigCoinWithSmallAnonymity2) && coins.Contains(smallCoinWithBigAnonymity)); | ||||||||||||||||||||||||||||||||||||||||
Comment on lines
+179
to
+180
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two checks make me think that the asserts are more general because we use random number generator but the rng returns a fixed number. Would it make sense to run this test, eg, 100 times with random RNG seed (i.e. make it random instead of deterministic) so that the test can actually fail if something is not as expected? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it would make sense. And I suppose the same holds true for other tests in this file. For this reason I suggest you to address it in another issue or pull request. |
||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
private static WasabiRandom ConfigureRng(int returnValue) | ||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||
var mockWasabiRandom = new Mock<WasabiRandom>(); | ||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -209,11 +209,48 @@ public static IEnumerable<T> TakeUntil<T>(this IEnumerable<T> list, Func<T, bool | |||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
public static double WeightedAverage<T>(this IEnumerable<T> source, Func<T, double> value, Func<T, double> weight) | ||||||||||||||
public static double WeightedMean<T>(this IEnumerable<T> source, Func<T, double> value, Func<T, double> weight) | ||||||||||||||
{ | ||||||||||||||
return source.Select(x => value(x) * weight(x)).Sum() / source.Select(weight).Sum(); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
public static double GeneralizedWeightedMean<T>(this IEnumerable<T> source, Func<T, double> value, Func<T, double> weight, double p) | ||||||||||||||
{ | ||||||||||||||
// See https://en.wikipedia.org/wiki/Generalized_mean | ||||||||||||||
// Basic properties: | ||||||||||||||
// * GeneralizedWeightedAverage(source, value, weight, 1) = WeightedAverage(source, value, weight) | ||||||||||||||
// * GeneralizedWeightedAverage(source, value, weight, p) goes to Max(source, value) as p goes to the infinity | ||||||||||||||
// * GeneralizedWeightedAverage(source, value, weight, p) goes to Min(source, value) as p goes to the minus infinity | ||||||||||||||
// * GeneralizedWeightedAverage(source, value, weight, p) <= GeneralizedWeightedAverage(source, value, weight, q) provided p < q | ||||||||||||||
|
||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would check the parameters more:
Suggested change
If this is not added, it will crash later in because 1 / 0 is not defined (AFAIK). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 3cd7ea2. |
||||||||||||||
if (!source.Any()) | ||||||||||||||
kiminuo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
{ | ||||||||||||||
throw new ArgumentException("Cannot be empty.", nameof(source)); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
if (source.Any(x => value(x) < 0)) | ||||||||||||||
{ | ||||||||||||||
throw new ArgumentException("Cannot be negative.", nameof(value)); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
if (source.Any(x => weight(x) < 0)) | ||||||||||||||
{ | ||||||||||||||
throw new ArgumentException("Cannot be negative.", nameof(weight)); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
if (source.All(x => weight(x) == 0)) | ||||||||||||||
{ | ||||||||||||||
throw new ArgumentException("Cannot be all zero.", nameof(weight)); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
if (p == 0) | ||||||||||||||
{ | ||||||||||||||
throw new ArgumentException("Cannot be zero.", nameof(p)); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
return Math.Pow(source.Select(x => Math.Pow(value(x), p) * weight(x)).Sum() / source.Select(weight).Sum(), 1 / p); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
public static int MaxOrDefault(this IEnumerable<int> me, int defaultValue) => | ||||||||||||||
me.DefaultIfEmpty(defaultValue).Max(); | ||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to switch from
[Fact]
to[Theory]
with multipleAnonymitySet
values to make the test more robust?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would make sense. Nevertheless, it is not easy to generate such test vectors, because the coin selection algorithm is very complex and doesn't behave deterministically.