Payjoin: Better UIH1 & UIH2 based selection (#1473)

* Try to make SelectUTXO care about all inputs and outputs

* wip

* wip

* Add test and fix seelctor

* remove space

* review changes

* revert back to index check

BTCPayServer.Tests/PayJoinTests.cs

@@ -26,6 +26,7 @@ using NBitcoin;
 using NBitcoin.Altcoins;
 using NBitcoin.Payment;
 using NBitpayClient;
+using NBXplorer.Models;
 using OpenQA.Selenium;
 using Xunit;
 using Xunit.Abstractions;
@@ -90,6 +91,45 @@ namespace BTCPayServer.Tests
             }
         }
 
+        [Fact]
+        [Trait("Integration", "Integration")]
+        public async Task ChooseBestUTXOsForPayjoin()
+        {
+            using (var tester = ServerTester.Create())
+            {
+                await tester.StartAsync();
+                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 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.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
+                utxos = new[] {FakeUTXO(0.3m),FakeUTXO(0.7m)};
+                paymentAmount = 0.5m;
+                otherOutputs = new[] {0.5m};
+                inputs = new[] {1m};
+                result = await controller.SelectUTXO(network, utxos, inputs, paymentAmount, otherOutputs);
+                Assert.Equal(PayJoinEndpointController.PayjoinUtxoSelectionType.Ordered, 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);
+                Assert.Equal(PayJoinEndpointController.PayjoinUtxoSelectionType.HeuristicBased, result.selectionType);
+            }
+        }
+        
+        
+
         private Transaction RandomTransaction(BTCPayNetwork network)
         {
             var tx = network.NBitcoinNetwork.CreateTransaction();
@@ -98,6 +138,15 @@ namespace BTCPayServer.Tests
             return tx;
         }
 
+        private UTXO FakeUTXO(decimal amount)
+        {
+            return new UTXO()
+            {
+                Value = new Money(amount, MoneyUnit.BTC),
+                Outpoint = RandomOutpoint()
+            };
+        }
+
         private OutPoint RandomOutpoint()
         {
             return new OutPoint(RandomUtils.GetUInt256(), 0);

BTCPayServer/Payments/PayJoin/PayJoinEndpointController.cs

@@ -275,8 +275,9 @@ namespace BTCPayServer.Payments.PayJoin
                 var prevOuts = originalTx.Inputs.Select(o => o.PrevOut).ToHashSet();
                 utxos = utxos.Where(u => !prevOuts.Contains(u.Outpoint)).ToArray();
                 Array.Sort(utxos, UTXODeterministicComparer.Instance);
-                foreach (var utxo in await SelectUTXO(network, utxos, output.Value,
+                
-                    psbt.Outputs.Where(o => o.Index != output.Index).Select(o => o.Value).ToArray()))
+                foreach (var utxo in (await SelectUTXO(network, utxos, psbt.Inputs.Select(input => input.WitnessUtxo.Value.ToDecimal(MoneyUnit.BTC)),  output.Value.ToDecimal(MoneyUnit.BTC),
+                    psbt.Outputs.Where(psbtOutput => psbtOutput.Index != output.Index).Select(psbtOutput => psbtOutput.Value.ToDecimal(MoneyUnit.BTC)))).selectedUTXO)
                 {
                     selectedUTXOs.Add(utxo.Outpoint, utxo);
                 }
@@ -505,41 +506,57 @@ namespace BTCPayServer.Payments.PayJoin
             return o;
         }
 
-        private async Task<UTXO[]> SelectUTXO(BTCPayNetwork network, UTXO[] availableUtxos, Money paymentAmount,
+        public enum PayjoinUtxoSelectionType
-            Money[] otherOutputs)
+        {
+            Unavailable,
+            HeuristicBased,
+            Ordered
+        }
+        [NonAction]
+        public async Task<(UTXO[] selectedUTXO, PayjoinUtxoSelectionType selectionType)> SelectUTXO(BTCPayNetwork network, UTXO[] availableUtxos, IEnumerable<decimal> otherInputs, decimal mainPaymentOutput,
+            IEnumerable<decimal> otherOutputs)
         {
             if (availableUtxos.Length == 0)
-                return Array.Empty<UTXO>();
+                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;
-            List<UTXO> utxosByPriority = new List<UTXO>();
             // 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
-
+            
             foreach (var availableUtxo in availableUtxos)
             {
                 if (currentTry >= maxTries)
                     break;
-                //we can only check against our input as we dont know the value of the rest.
+
-                var input = (Money)availableUtxo.Value;
+                var invalid = false;
-                var paymentAmountSum = input + paymentAmount;
+                foreach (var input in otherInputs.Concat(new[] {availableUtxo.Value.GetValue(network)}))
-                if (otherOutputs.Concat(new[] {paymentAmountSum}).Any(output => input > output))
                 {
-                    //UIH 1 & 2
+                    var computedOutputs =
-                    continue;
+                        otherOutputs.Concat(new[] {mainPaymentOutput + availableUtxo.Value.GetValue(network)});
+                    if (computedOutputs.Any(output => input > output))
+                    {
+                        //UIH 1 & 2
+                        invalid = true;
+                        break;
+                    }
                 }
 
+                if (invalid)
+                {
+                    continue;
+                }
                 if (await _payJoinRepository.TryLock(availableUtxo.Outpoint))
                 {
-                    return new UTXO[] { availableUtxo };
+                    return (new[] {availableUtxo}, PayjoinUtxoSelectionType.HeuristicBased);
                 }
+
                 locked.Add(availableUtxo.Outpoint);
                 currentTry++;
             }
@@ -549,11 +566,11 @@ namespace BTCPayServer.Payments.PayJoin
                     break;
                 if (await _payJoinRepository.TryLock(utxo.Outpoint))
                 {
-                    return new UTXO[] { utxo };
+                    return (new[] {utxo}, PayjoinUtxoSelectionType.Ordered);
                 }
                 currentTry++;
             }
-            return Array.Empty<UTXO>();
+            return (Array.Empty<UTXO>(), PayjoinUtxoSelectionType.Unavailable);
         }
     }
 }