Fix unnecessary inputs bug #9536

adamPetho · 2022-11-15T11:36:20Z

yahiheb

CI is failing.

WalletWasabi/Blockchain/TransactionBuilding/SmartCoinSelector.cs

lontivero

LGTM. The only doubt that I have is in the case of lesser coins vs exact amount. If I want to pay 0.3btc and I have [0.2, 0.2, 0.1, 0.1] it prefers [0.2, 0.2] over [0.2, 0.1], what doesn't look 100% okay.

Anyway, the code makes sense. Just make sure this works okay with the BnB preselection.

adamPetho · 2022-11-17T14:39:53Z

f I want to pay 0.3btc and I have [0.2, 0.2, 0.1, 0.1] it prefers [0.2, 0.2] over [0.2, 0.1], what doesn't look 100% okay.

Oh boy, you are right. It doesn't look right, indeed. I will see what can be done about this.

it prefers [0.2, 0.2] over [0.2, 0.1]

The problem seems like that the [0.2, 0.1] combination is not calculated at all.

It calculates [0.1, 0.1], [0.2, 0.2] and [all the coins] combination IF the unspent coins looks like this

BUT if we change the order of the avaliableCoins

we suddenly find much more combinations

I will investigate this further.

adamPetho · 2022-11-21T12:29:35Z

Just make sure this works okay with the BnB preselection.

I checked it, and AFAIK BnB logic is untouched by these changes.

I want to pay 0.3btc and I have [0.2, 0.2, 0.1, 0.1] it prefers [0.2, 0.2] over [0.2, 0.1]

So what I found about this scenario is:

Order of the coins don't matter.

In the test if we generate our coins this way:

var availableCoins = GenerateSmartCoins(
	Enumerable.Range(0, 1).Select(i => ("Juan", 0.1m)))
	.ToList();

availableCoins.Add(GenerateSmartCoins(
	Enumerable.Range(0, 1).Select(i => ("Juan", 0.2m)))
	.ToList());

availableCoins.Add(GenerateSmartCoins(
	Enumerable.Range(0, 1).Select(i => ("Juan", 0.1m)))
	.ToList());

availableCoins.Add(GenerateSmartCoins(
	Enumerable.Range(0, 1).Select(i => ("Juan", 0.2m)))
	.ToList());

It calculates and chooses the [0.2, 0.1] combination.

But if we generate our coins this way:

var availableCoins = GenerateSmartCoins(
	Enumerable.Range(0, 2).Select(i => ("Juan", 0.1m)))
	.ToList();

availableCoins.Add(GenerateSmartCoins(
	Enumerable.Range(0, 2).Select(i => ("Juan", 0.2m)))
	.ToList());

The [0.2, 0.1] combination doesn't get calculated and selector chooses [0.2, 0.2].

So, while the order of the coins don't matter, how we call GenerateSmartCoins() DOES matter.
That method creates cluster-grouped keys and filles them up here.
So, when we call

availableCoins.Add(GenerateSmartCoins(
	Enumerable.Range(0, 2).Select(i => ("Juan", 0.2m)))
	.ToList());

the two newly generated coins will be linked together and SmartCoinSelector will try to spend them together.

So it works as intended IMO.

WalletWasabi.Tests/UnitTests/Wallet/SmartCoinSelectorTests.cs

kiminuo · 2022-11-24T13:59:34Z

WalletWasabi/Blockchain/TransactionBuilding/SmartCoinSelector.cs

@@ -88,20 +88,21 @@ public IEnumerable<ICoin> Select(IEnumerable<ICoin> suggestion, IMoney target)
 		var coinsInBestClusterByScript = bestCoinCluster
 			.GroupBy(c => c.ScriptPubKey)
 			.Select(group => (ScriptPubKey: group.Key, Coins: group.ToList()))
-			.OrderBy(x => x.Coins.Sum(c => c.Amount))
+			.OrderByDescending(x => x.Coins.Sum(c => c.Amount))


Given how SmartCoinSelector is commented, I would add a comment why descending ordering is the good thing to do here.

I'm thinking about an answer for this, and I just can't get it.

Short answer: it makes the tests pass.

Long answer: Something is fishy and I don't yet understand what.

WalletWasabi.Tests/UnitTests/Wallet/SmartCoinSelectorTests.cs

WalletWasabi.Tests/UnitTests/Transactions/TransactionFactoryTests.cs

kiminuo · 2022-11-24T14:05:35Z

#9536 (comment) - this sounds like a great thing to add to a single test. WDYT?

kiminuo · 2022-11-24T20:31:02Z

LGTM.

We can add short test summaries so that we won't spend time next time finding out what is covered and isn't. Up to you though.

adamPetho · 2022-11-25T11:04:24Z

Sorry @kiminuo I'm not ignoring your questions and feedbacks, they are just hard to answer. 😅

#9536 (comment) - this sounds like a great thing to add to a single test. WDYT?

The reason I didn't add this test (yet), because I don't fully understand why one scenario is failing and the other isn't.

kiminuo · 2022-11-25T12:33:58Z

Sorry @kiminuo I'm not ignoring your questions and feedbacks, they are just hard to answer. 😅

#9536 (comment) - this sounds like a great thing to add to a single test. WDYT?

The reason I didn't add this test (yet), because I don't fully understand why one scenario is failing and the other isn't.

If you share those scenarios with me (ideally as two tests), then I can maybe help.

adamPetho · 2022-11-25T15:49:07Z

If you share those scenarios with me (ideally as two tests), then I can maybe help.

So the main issue is this:

If I want to pay 0.3btc and I have [0.2, 0.2, 0.1, 0.1] it prefers [0.2, 0.2] over [0.2, 0.1], what doesn't look 100% okay.

And today, I just can't make a passing test for this...

The failing test looks like this:

[Fact]
public void FailingCase()
{
	Money target = Money.Coins(0.3m);

	List<SmartCoin> availableCoins = GenerateSmartCoins(Enumerable.Range(0, 2).Select(i => ("Juan", 0.1m))).ToList();

	availableCoins.Add(GenerateSmartCoins(Enumerable.Range(0, 2).Select(i => ("Juan", 0.2m))));

	SmartCoinSelector selector = new(availableCoins);
	List<Coin> coinsToSpend = selector.Select(Enumerable.Empty<Coin>(), target).Cast<Coin>().ToList();

	Assert.Equal(target, Money.Satoshis(coinsToSpend.Sum(x => x.Amount))); // [0.2, 0.2] gets chosen, so it fails.
}

Another failing test, where the selector would rather choose a 5 BTC amount coin than 3 0.1 and be exact.
(I know we have a test for prefering less coins over exact amount, but come on...)
I don't know if I'm chasing a ghost (feature) or an actual issue.

[Fact]
public void FailingCase()
{
	Money target = Money.Coins(0.3m);

	List<SmartCoin> availableCoins = GenerateSmartCoins(Enumerable.Range(0, 4).Select(i => ("Juan", 0.1m))).ToList();

	availableCoins.Add(GenerateSmartCoins(Enumerable.Range(0, 4).Select(i => ("Juan", 5m))));

	SmartCoinSelector selector = new(availableCoins);
	List<Coin> coinsToSpend = selector.Select(Enumerable.Empty<Coin>(), target).Cast<Coin>().ToList();

	Assert.Equal(target, Money.Satoshis(coinsToSpend.Sum(x => x.Amount))); // fails with [5.0] selection
}

lontivero · 2022-11-28T12:28:40Z

Yes! That's the answer. It selects the [0.2; 0.2] to prevent mixing unnecessary clusters and then the code was right before. The tests look good.

adamPetho · 2022-11-28T12:42:37Z

Yes! That's the answer. It selects the [0.2; 0.2] to prevent mixing unnecessary clusters

Yep, took me some time to figure it out. 😅

Do the changes look good in SmartCoinSelector? @lontivero @kiminuo

The tests are passing, but we don't aim to satisfy the tests, we aim to have a nicely working coin selector.

kiminuo · 2022-11-28T13:01:51Z

I plan to check it in an hour or so. Thanks!

WalletWasabi.Tests/UnitTests/Wallet/SmartCoinSelectorTests.cs

kiminuo · 2022-11-28T15:20:25Z

Look good to me.

Personally, I would clean up those SmartCoinSelectorTests tests. Mostly it's unnecessarily hard to read but that's not introduced by this PR.

adamPetho · 2022-11-28T15:59:20Z

Personally, I would clean up those SmartCoinSelectorTests tests.

I will clean them up tomorrow.

kiminuo · 2022-11-29T08:06:28Z

Personally, I would simplify & unify it as follows:

using DynamicData;
using NBitcoin;
using System.Collections.Generic;
using System.Linq;
using WalletWasabi.Blockchain.Analysis.Clustering;
using WalletWasabi.Blockchain.Keys;
using WalletWasabi.Blockchain.TransactionBuilding;
using WalletWasabi.Blockchain.TransactionOutputs;
using WalletWasabi.Tests.Helpers;
using Xunit;

namespace WalletWasabi.Tests.UnitTests.Wallet;

public class SmartCoinSelectorTests
{
	public SmartCoinSelectorTests()
	{
		KeyManager = KeyManager.Recover(
			new Mnemonic("all all all all all all all all all all all all"),
			"",
			Network.Main,
			KeyManager.GetAccountKeyPath(Network.Main));
	}

	private KeyManager KeyManager { get; }
	private static IEnumerable<Coin> EmptySuggestion { get; } = Enumerable.Empty<Coin>();

	[Fact]
	public void SelectsOnlyOneCoinWhenPossible()
	{
		Money target = Money.Coins(0.3m);
		var availableCoins = GenerateSmartCoins(Enumerable.Range(0, 9).Select(i => ("Juan", 0.1m * (i + 1))));

		var selector = new SmartCoinSelector(availableCoins);		
		var coinsToSpend = selector.Select(suggestion: EmptySuggestion, target);

		var theOnlyOne = Assert.Single(coinsToSpend.Cast<Coin>());
		Assert.Equal(0.3m, theOnlyOne.Amount.ToUnit(MoneyUnit.BTC));
	}

	[Fact]
	public void DontSelectUnnecessaryInputs()
	{
		Money target = Money.Coins(4m);
		List<SmartCoin> availableCoins = GenerateSmartCoins(Enumerable.Range(0, 10).Select(i => ("Juan", 0.1m * (i + 1))));

		SmartCoinSelector selector = new(availableCoins);
		List<Coin> coinsToSpend = selector.Select(suggestion: EmptySuggestion, target).Cast<Coin>().ToList();

		Assert.Equal(5, coinsToSpend.Count);
		Assert.Equal(target, Money.Satoshis(coinsToSpend.Sum(x => x.Amount)));
	}

	[Fact]
	public void PreferSameClusterOverExactAmount()
	{
		Money target = Money.Coins(0.3m);
		List<SmartCoin> availableCoins = GenerateSmartCoins(("Besos", 0.2m), ("Besos", 0.2m), ("Juan", 0.1m), ("Juan", 0.1m));

		SmartCoinSelector selector = new(availableCoins);
		List<Coin> coinsToSpend = selector.Select(suggestion: EmptySuggestion, target).Cast<Coin>().ToList();

		// One might expect that we get an exact match of 0.3 BTC here - i.e. the combination ("Besos", 0.2m) and ("Juan", 0.1m), but that's
		// not the case as we prefer not to mix clusters.
		Assert.Equal(Money.Coins(0.4m), Money.Satoshis(coinsToSpend.Sum(x => x.Amount)));
	}

	[Fact]
	public void PreferExactAmountWhenClustersAreDifferent()
	{
		Money target = Money.Coins(0.3m);
		List<SmartCoin> availableCoins = GenerateSmartCoins(("Besos", 0.2m), ("Juan", 0.1m), ("Adam", 0.2m), ("Eve", 0.1m));

		SmartCoinSelector selector = new(availableCoins);
		List<Coin> coinsToSpend = selector.Select(suggestion: EmptySuggestion, target).Cast<Coin>().ToList();

		Assert.Equal(2, coinsToSpend.Count);
		Assert.Equal(Money.Coins(0.3m), Money.Satoshis(coinsToSpend.Sum(x => x.Amount)));       // Cluster-privacy is indifferent, so aim for exact amount.
	}

	[Fact]
	public void DontUseTheWholeClusterIfNotNecessary()
	{
		Money target = Money.Coins(0.3m);
		List<SmartCoin> availableCoins = GenerateDuplicateSmartCoins(("Juan", 0.1m), count: 10);

		SmartCoinSelector selector = new(availableCoins);
		List<Coin> coinsToSpend = selector.Select(suggestion: EmptySuggestion, target).Cast<Coin>().ToList();

		Assert.Equal(3, coinsToSpend.Count);
		Assert.Equal(target, Money.Satoshis(coinsToSpend.Sum(x => x.Amount)));
	}

	[Fact]
	public void PreferLessCoinsOnSameAmount()
	{
		Money target = Money.Coins(1m);
		List<SmartCoin> availableCoins = GenerateDuplicateSmartCoins(("Juan", 0.1m), count: 11);
		availableCoins.Add(GenerateDuplicateSmartCoins(("Beto", 0.2m), count: 5));

		SmartCoinSelector selector = new(availableCoins);
		List<Coin> coinsToSpend = selector.Select(suggestion: EmptySuggestion, target).Cast<Coin>().ToList();

		Assert.Equal(5, coinsToSpend.Count);
		Assert.Equal(target, Money.Satoshis(coinsToSpend.Sum(x => x.Amount)));
	}

	[Fact]
	public void PreferLessCoinsOverExactAmount()
	{
		Money target = Money.Coins(0.41m);
		List<SmartCoin> smartCoins = GenerateSmartCoins(Enumerable.Range(0, 10).Select(i => ("Juan", 0.1m * (i + 1))));
		smartCoins.Add(BitcoinFactory.CreateSmartCoin(smartCoins[0].HdPubKey, 0.11m));
		var someCoins = smartCoins.Select(x => x.Coin);

		var selector = new SmartCoinSelector(smartCoins);
		var coinsToSpend = selector.Select(suggestion: someCoins, target);

		var theOnlyOne = Assert.Single(coinsToSpend.Cast<Coin>());
		Assert.Equal(0.5m, theOnlyOne.Amount.ToUnit(MoneyUnit.BTC));
	}

	[Fact]
	public void PreferSameScript()
	{
		Money target = Money.Coins(0.31m);
		var smartCoins = GenerateSmartCoins(Enumerable.Repeat(("Juan", 0.2m), 12));
		smartCoins.Add(BitcoinFactory.CreateSmartCoin(smartCoins[0].HdPubKey, 0.11m));

		var selector = new SmartCoinSelector(smartCoins);
		var coinsToSpend = selector.Select(suggestion: EmptySuggestion, target).Cast<Coin>().ToList();

		Assert.Equal(2, coinsToSpend.Count);
		Assert.Equal(coinsToSpend[0].ScriptPubKey, coinsToSpend[1].ScriptPubKey);
		Assert.Equal(0.31m, coinsToSpend.Sum(x => x.Amount.ToUnit(MoneyUnit.BTC)));
	}

	[Fact]
	public void PreferMorePrivateClusterScript()
	{
		Money target = Money.Coins(0.3m);
		List<SmartCoin> coinsKnownByJuan = GenerateSmartCoins(("Juan", 0.2m), ("Juan", 0.2m), ("Juan", 0.2m), ("Juan", 0.2m), ("Juan", 0.2m));
		List<SmartCoin> coinsKnownByBeto = GenerateSmartCoins(("Beto", 0.2m), ("Beto", 0.2m));

		var selector = new SmartCoinSelector(coinsKnownByJuan.Concat(coinsKnownByBeto).ToList());
		var coinsToSpend = selector.Select(suggestion: EmptySuggestion, target).Cast<Coin>().ToList();

		Assert.Equal(2, coinsToSpend.Count);
		Assert.Equal(0.4m, coinsToSpend.Sum(x => x.Amount.ToUnit(MoneyUnit.BTC)));
	}

	private List<SmartCoin> GenerateDuplicateSmartCoins((string Cluster, decimal amount) coin, int count)
		=> GenerateSmartCoins(Enumerable.Range(start: 0, count).Select(x => coin));

	private List<SmartCoin> GenerateSmartCoins(params (string Cluster, decimal amount)[] coins)
		=> GenerateSmartCoins((IEnumerable<(string Cluster, decimal amount)>)coins);

	private List<SmartCoin> GenerateSmartCoins(IEnumerable<(string Cluster, decimal amount)> coins)
	{
		Dictionary<string, List<(HdPubKey key, decimal amount)>> generatedKeyGroup = new();

		// Create cluster-grouped keys
		foreach (var targetCoin in coins)
		{
			var key = KeyManager.GenerateNewKey(new SmartLabel(targetCoin.Cluster), KeyState.Clean, false);

			if (!generatedKeyGroup.ContainsKey(targetCoin.Cluster))
			{
				generatedKeyGroup.Add(targetCoin.Cluster, new());
			}

			generatedKeyGroup[targetCoin.Cluster].Add((key, targetCoin.amount));
		}

		var coinPairClusters = generatedKeyGroup.GroupBy(x => x.Key)
			.Select(x => x.Select(y => y.Value)) // Group the coin pairs into clusters.
			.SelectMany(x => x
				.Select(coinPair => (coinPair,
					cluster: new Cluster(coinPair.Select(z => z.key))))).ToList();

		// Set each key with its corresponding cluster object.
		foreach (var x in coinPairClusters)
		{
			foreach (var y in x.coinPair)
			{
				y.key.Cluster = x.cluster;
			}
		}

		return coinPairClusters.Select(x => x.coinPair)
			.SelectMany(x => x.Select(y => BitcoinFactory.CreateSmartCoin(y.key, y.amount)))
			.ToList(); // Generate the final SmartCoins.
	}
}

* add failing test * fix the failing + existing tests * clean up the test * fix CI * use ThenBy() * add test * more precise tests * improve readability in tests * add more tests * fix tests * clean up and unify the tests

adamPetho added 3 commits November 15, 2022 12:32

add failing test

4b38b66

fix the failing + existing tests

4972d48

clean up the test

c6150f0

pull-request-size bot added the size/S label Nov 15, 2022

adamPetho requested review from lontivero and yahiheb November 15, 2022 11:36

yahiheb requested changes Nov 15, 2022

View reviewed changes

fix CI

1961718

kiminuo reviewed Nov 16, 2022

View reviewed changes

WalletWasabi/Blockchain/TransactionBuilding/SmartCoinSelector.cs Outdated Show resolved Hide resolved

adamPetho added 2 commits November 16, 2022 11:02

use ThenBy()

247a2dd

add test

8ac0065

pull-request-size bot added size/M and removed size/S labels Nov 16, 2022

lontivero previously approved these changes Nov 17, 2022

View reviewed changes

adamPetho added 2 commits November 23, 2022 11:52

Merge branch 'master' into unnecessary_inputs

b1d741c

more precise tests

4f2b570

adamPetho dismissed lontivero’s stale review via 4f2b570 November 23, 2022 11:01

kiminuo reviewed Nov 24, 2022

View reviewed changes

WalletWasabi.Tests/UnitTests/Wallet/SmartCoinSelectorTests.cs Outdated Show resolved Hide resolved

kiminuo reviewed Nov 24, 2022

View reviewed changes

WalletWasabi.Tests/UnitTests/Wallet/SmartCoinSelectorTests.cs Outdated Show resolved Hide resolved

kiminuo reviewed Nov 24, 2022

View reviewed changes

WalletWasabi.Tests/UnitTests/Transactions/TransactionFactoryTests.cs Show resolved Hide resolved

improve readability in tests

34acc12

add more tests

dab51d5

fix tests

f64eada

pull-request-size bot added size/L and removed size/M labels Nov 28, 2022

kiminuo reviewed Nov 28, 2022

View reviewed changes

WalletWasabi.Tests/UnitTests/Wallet/SmartCoinSelectorTests.cs Outdated Show resolved Hide resolved

clean up and unify the tests

e03cc1c

kiminuo approved these changes Nov 29, 2022

View reviewed changes

lontivero merged commit e78e8c9 into WalletWasabi:master Nov 29, 2022

adamPetho mentioned this pull request Nov 29, 2022

Transaction includes unnecessary inputs #9352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unnecessary inputs bug #9536

Fix unnecessary inputs bug #9536

adamPetho commented Nov 15, 2022

yahiheb left a comment

lontivero left a comment

adamPetho commented Nov 17, 2022 •

edited

adamPetho commented Nov 21, 2022 •

edited

kiminuo Nov 24, 2022

adamPetho Nov 25, 2022

kiminuo commented Nov 24, 2022

kiminuo commented Nov 24, 2022

adamPetho commented Nov 25, 2022

kiminuo commented Nov 25, 2022

adamPetho commented Nov 25, 2022 •

edited

lontivero commented Nov 28, 2022

adamPetho commented Nov 28, 2022

kiminuo commented Nov 28, 2022

kiminuo commented Nov 28, 2022

adamPetho commented Nov 28, 2022

kiminuo commented Nov 29, 2022 •

edited

Fix unnecessary inputs bug #9536

Fix unnecessary inputs bug #9536

Conversation

adamPetho commented Nov 15, 2022

yahiheb left a comment

Choose a reason for hiding this comment

lontivero left a comment

Choose a reason for hiding this comment

adamPetho commented Nov 17, 2022 • edited

adamPetho commented Nov 21, 2022 • edited

kiminuo Nov 24, 2022

Choose a reason for hiding this comment

adamPetho Nov 25, 2022

Choose a reason for hiding this comment

kiminuo commented Nov 24, 2022

kiminuo commented Nov 24, 2022

adamPetho commented Nov 25, 2022

kiminuo commented Nov 25, 2022

adamPetho commented Nov 25, 2022 • edited

lontivero commented Nov 28, 2022

adamPetho commented Nov 28, 2022

kiminuo commented Nov 28, 2022

kiminuo commented Nov 28, 2022

adamPetho commented Nov 28, 2022

kiminuo commented Nov 29, 2022 • edited

adamPetho commented Nov 17, 2022 •

edited

adamPetho commented Nov 21, 2022 •

edited

adamPetho commented Nov 25, 2022 •

edited

kiminuo commented Nov 29, 2022 •

edited