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.

Sign up for GitHub

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

Jump to bottom

Fix: Payjoin wasn't always properly choosing utxo for optimal change #4600

Merged
merged 2 commits into from
Feb 7, 2023
Merged

Fix: Payjoin wasn't always properly choosing utxo for optimal change #4600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
967f76d
Fix: Payjoin wasn't always properly choosing utxo for optimal change
NicolasDorier Feb 6, 2023
7f9dd1c
Update with timeout
NicolasDorier Feb 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 27 additions & 4 deletions BTCPayServer.Tests/PayJoinTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,12 @@ public async Task ChooseBestUTXOsForPayjoin()
var network = tester.NetworkProvider.GetNetwork<BTCPayNetwork>("BTC");
var controller = tester.PayTester.GetService<PayJoinEndpointController>();

//Only one utxo, so obvious result
var utxos = new[] { FakeUTXO(1.0m) };
var utxos = new[] { FakeUTXO(1m) };
var paymentAmount = 0.5m;
var otherOutputs = new[] { 0.5m };
var inputs = new[] { 1m };
var result = await controller.SelectUTXO(network, utxos, inputs, paymentAmount, otherOutputs);
Assert.Equal(PayJoinEndpointController.PayjoinUtxoSelectionType.Ordered, result.selectionType);
Assert.Equal(PayJoinEndpointController.PayjoinUtxoSelectionType.HeuristicBased, result.selectionType);
Assert.Contains(result.selectedUTXO, utxo => utxos.Contains(utxo));

//no matter what here, no good selection, it seems that payment with 1 utxo generally makes payjoin coin selection unperformant
Expand All @@ -124,15 +123,39 @@ public async Task ChooseBestUTXOsForPayjoin()
otherOutputs = new[] { 0.5m };
inputs = new[] { 1m };
result = await controller.SelectUTXO(network, utxos, inputs, paymentAmount, otherOutputs);
Assert.Equal(PayJoinEndpointController.PayjoinUtxoSelectionType.Ordered, result.selectionType);
Assert.Equal(PayJoinEndpointController.PayjoinUtxoSelectionType.HeuristicBased, result.selectionType);

//when there is no change, anything works
utxos = new[] { FakeUTXO(1), FakeUTXO(0.1m), FakeUTXO(0.001m), FakeUTXO(0.003m) };
paymentAmount = 0.5m;
otherOutputs = new decimal[0];
inputs = new[] { 0.03m, 0.07m };
result = await controller.SelectUTXO(network, utxos, inputs, paymentAmount, otherOutputs);


// We want to make a transaction such that
// min(out) < min(in)

// Original transaction is:
// 0.5 -> 0.3 , 0.1
// When chosing a new utxo x, we have the modified tx
// 0.5 , x -> 0.3 , (0.1+x)
// We need:
// min(0.3, 0.1+x) < min(0.5, x)
// Any x > 0.3 should be fine
utxos = new[] { FakeUTXO(0.2m), FakeUTXO(0.3m), FakeUTXO(0.31m) };
paymentAmount = 0.1m;
otherOutputs = new decimal[] { 0.3m };
inputs = new[] { 0.5m };
result = await controller.SelectUTXO(network, utxos, inputs, paymentAmount, otherOutputs);
Assert.Equal(PayJoinEndpointController.PayjoinUtxoSelectionType.HeuristicBased, result.selectionType);
Assert.Equal(0.31m, result.selectedUTXO[0].Value.GetValue(network));

// If the 0.31m wasn't available, no selection heuristic based
utxos = new[] { FakeUTXO(0.2m), FakeUTXO(0.3m) };
result = await controller.SelectUTXO(network, utxos, inputs, paymentAmount, otherOutputs);
Assert.Equal(PayJoinEndpointController.PayjoinUtxoSelectionType.Ordered, result.selectionType);
Assert.Equal(0.2m, result.selectedUTXO[0].Value.GetValue(network));
}


Expand Down
65 changes: 34 additions & 31 deletions BTCPayServer/Payments/PayJoin/PayJoinEndpointController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#nullable enable
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Linq;
Expand Down Expand Up @@ -585,62 +586,64 @@ public enum PayjoinUtxoSelectionType
Ordered
}
[NonAction]
public async Task<(UTXO[] selectedUTXO, PayjoinUtxoSelectionType selectionType)> SelectUTXO(BTCPayNetwork network, UTXO[] availableUtxos, decimal[] otherInputs, decimal mainPaymentOutput,
public async Task<(UTXO[] selectedUTXO, PayjoinUtxoSelectionType selectionType)> SelectUTXO(BTCPayNetwork network, UTXO[] availableUtxos, decimal[] otherInputs, decimal originalPaymentOutput,
IEnumerable<decimal> otherOutputs)
{
if (availableUtxos.Length == 0)
return (Array.Empty<UTXO>(), PayjoinUtxoSelectionType.Unavailable);
// Assume the merchant wants to get rid of the dust
HashSet<OutPoint> locked = new HashSet<OutPoint>();
// We don't want to make too many db roundtrip which would be inconvenient for the sender
int maxTries = 30;
int currentTry = 0;
// UIH = "unnecessary input heuristic", basically "a wallet wouldn't choose more utxos to spend in this scenario".
//
// "UIH1" : one output is smaller than any input. This heuristically implies that that output is not a payment, and must therefore be a change output.
//
// "UIH2": one input is larger than any output. This heuristically implies that no output is a payment, or, to say it better, it implies that this is not a normal wallet-created payment, it's something strange/exotic.
//src: https://gist.github.com/AdamISZ/4551b947789d3216bacfcb7af25e029e#gistcomment-2796539

TimeSpan timeout = TimeSpan.FromSeconds(30.0);
Stopwatch watch = new Stopwatch();
watch.Start();

// BlockSci UIH1 and UIH2:
// if min(out) < min(in) then UIH1 else UIH2
// https://eprint.iacr.org/2022/589.pdf


var origMinOut = otherOutputs.Concat(new[] { decimal.MaxValue }).Min();
var origMinIn = otherInputs.Concat(new[] { decimal.MaxValue }).Min();

foreach (var availableUtxo in availableUtxos)
{
if (currentTry >= maxTries)
if (watch.Elapsed > timeout)
break;

var invalid = false;
foreach (var input in otherInputs.Concat(new[] { availableUtxo.Value.GetValue(network) }))
var utxoValue = availableUtxo.Value.GetValue(network);
var newPaymentOutput = originalPaymentOutput + utxoValue;
var minOut = Math.Min(origMinOut, newPaymentOutput);
var minIn = Math.Min(origMinIn, utxoValue);

if (minOut < minIn)
{
var computedOutputs =
otherOutputs.Concat(new[] { mainPaymentOutput + availableUtxo.Value.GetValue(network) });
if (computedOutputs.Any(output => input > output))
// UIH1: Optimal change, smallest output is the change address.
if (await _utxoLocker.TryLock(availableUtxo.Outpoint))
{
//UIH 1 & 2
invalid = true;
break;
return (new[] { availableUtxo }, PayjoinUtxoSelectionType.HeuristicBased);
}
else
{
locked.Add(availableUtxo.Outpoint);
continue;
}
}

if (invalid)
else
{
// UIH2: Unnecessary input, let's try to get an optimal change by using a different output
continue;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will loop without getting any closer to maxTries because it does not add to locked. Therefore maxTries is not the hard cap on tries it once was

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was on purpose, as this line doesn't make any DB roundtrip.
Irrelevant now, as I use a timeout.

}
if (await _utxoLocker.TryLock(availableUtxo.Outpoint))
{
return (new[] { availableUtxo }, PayjoinUtxoSelectionType.HeuristicBased);
}

locked.Add(availableUtxo.Outpoint);
currentTry++;
}

foreach (var utxo in availableUtxos.Where(u => !locked.Contains(u.Outpoint)))
{
if (currentTry >= maxTries)
if (watch.Elapsed > timeout)
break;
if (await _utxoLocker.TryLock(utxo.Outpoint))
{
return (new[] { utxo }, PayjoinUtxoSelectionType.Ordered);
}
currentTry++;
locked.Add(utxo.Outpoint);
}
return (Array.Empty<UTXO>(), PayjoinUtxoSelectionType.Unavailable);
}
Expand Down