From 6a4d8f7404aab036c8ca8cda3700a4b90502d94e Mon Sep 17 00:00:00 2001 From: Nicolas Dorier Date: Tue, 7 Feb 2023 16:43:31 +0900 Subject: [PATCH] Fix: Payjoin wasn't always properly choosing utxo for optimal change (#4600) * Fix: Payjoin wasn't always properly choosing utxo for optimal change * Update with timeout --- BTCPayServer.Tests/PayJoinTests.cs | 31 +++++++-- .../PayJoin/PayJoinEndpointController.cs | 65 ++++++++++--------- 2 files changed, 61 insertions(+), 35 deletions(-) diff --git a/BTCPayServer.Tests/PayJoinTests.cs b/BTCPayServer.Tests/PayJoinTests.cs index f566aff2d..2e6560b67 100644 --- a/BTCPayServer.Tests/PayJoinTests.cs +++ b/BTCPayServer.Tests/PayJoinTests.cs @@ -109,13 +109,12 @@ namespace BTCPayServer.Tests var network = tester.NetworkProvider.GetNetwork("BTC"); var controller = tester.PayTester.GetService(); - //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 @@ -124,7 +123,7 @@ namespace BTCPayServer.Tests 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) }; @@ -132,7 +131,31 @@ namespace BTCPayServer.Tests 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)); } diff --git a/BTCPayServer/Payments/PayJoin/PayJoinEndpointController.cs b/BTCPayServer/Payments/PayJoin/PayJoinEndpointController.cs index 11cabaa59..30d4ce98d 100644 --- a/BTCPayServer/Payments/PayJoin/PayJoinEndpointController.cs +++ b/BTCPayServer/Payments/PayJoin/PayJoinEndpointController.cs @@ -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; @@ -585,62 +586,64 @@ namespace BTCPayServer.Payments.PayJoin 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 otherOutputs) { if (availableUtxos.Length == 0) return (Array.Empty(), PayjoinUtxoSelectionType.Unavailable); - // Assume the merchant wants to get rid of the dust HashSet locked = new HashSet(); - // 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; } - 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(), PayjoinUtxoSelectionType.Unavailable); }