Simplify RBF handling, and handle case of double spend happening outside of wallet (Fix #1375)

BTCPayServer.Tests/UnitTest1.cs

@@ -1101,6 +1101,45 @@ namespace BTCPayServer.Tests
                     Assert.Equal(payment2, invoice.BtcPaid);
                     Assert.Equal("False", invoice.ExceptionStatus.ToString());
                 });
+
+
+                Logs.Tester.LogInformation($"Let's test out rbf payments where the payment gets sent elsehwere instead");
+                var invoice2 = user.BitPay.CreateInvoice(new Invoice()
+                {
+                    Price = 0.01m,
+                    Currency = "BTC"
+                }, Facade.Merchant);
+
+                var invoice2Address = BitcoinAddress.Create(invoice2.BitcoinAddress, user.SupportedNetwork.NBitcoinNetwork);
+                uint256 invoice2tx1Id = await tester.ExplorerNode.SendToAddressAsync(invoice2Address, invoice2.BtcDue, replaceable: true);
+                Transaction invoice2Tx1 = null;
+                TestUtils.Eventually(() =>
+                {
+                    invoice2 = user.BitPay.GetInvoice(invoice2.Id);
+                    Assert.Equal("paid", invoice2.Status);
+                    invoice2Tx1 = tester.ExplorerNode.GetRawTransaction(new uint256(invoice2tx1Id));
+                });
+                var invoice2Tx2 = invoice2Tx1.Clone();
+                foreach (var input in invoice2Tx2.Inputs)
+                {
+                    input.ScriptSig = Script.Empty; //Strip signatures
+                    input.WitScript = WitScript.Empty; //Strip signatures
+                }
+
+                output = invoice2Tx2.Outputs.First(o =>
+                    o.ScriptPubKey == invoice2Address.ScriptPubKey);
+                output.Value -= new Money(10_000, MoneyUnit.Satoshi);
+                output.ScriptPubKey = new Key().ScriptPubKey;
+                invoice2Tx2 = await tester.ExplorerNode.SignRawTransactionAsync(invoice2Tx2);
+                await tester.ExplorerNode.SendRawTransactionAsync(invoice2Tx2);
+                tester.ExplorerNode.Generate(1);
+                await TestUtils.EventuallyAsync(async () =>
+                {
+                    var i = await tester.PayTester.InvoiceRepository.GetInvoice(invoice2.Id);
+                    Assert.Equal(InvoiceStatus.New, i.Status);
+                    Assert.Single(i.GetPayments());
+                    Assert.False(i.GetPayments().First().Accounted);
+                });
             }
         }
 

BTCPayServer/Payments/Bitcoin/NBXplorerListener.cs

@@ -19,6 +19,7 @@ using NBXplorer.Models;
 using BTCPayServer.Payments;
 using BTCPayServer.HostedServices;
 using NBitcoin.Altcoins.Elements;
+using NBitcoin.RPC;
 
 namespace BTCPayServer.Payments.Bitcoin
 {
@@ -216,7 +217,6 @@ namespace BTCPayServer.Payments.Bitcoin
             var transactions = await wallet.GetTransactions(GetAllBitcoinPaymentData(invoice)
                     .Select(p => p.Outpoint.Hash)
                     .ToArray());
-            var conflicts = GetConflicts(transactions.Select(t => t.Value));
             foreach (var payment in invoice.GetPayments(wallet.Network))
             {
                 if (payment.GetPaymentMethodId().PaymentType != PaymentTypes.BTCLike)
@@ -225,8 +225,29 @@ namespace BTCPayServer.Payments.Bitcoin
                 if (!transactions.TryGetValue(paymentData.Outpoint.Hash, out TransactionResult tx))
                     continue;
                 var txId = tx.Transaction.GetHash();
-                var txConflict = conflicts.GetConflict(txId);
+                bool accounted = true;
-                var accounted = txConflict == null || txConflict.IsWinner(txId);
+                if (tx.Confirmations == 0)
+                {
+                    // Let's check if it was orphaned by broadcasting it again
+                    var explorerClient = _ExplorerClients.GetExplorerClient(wallet.Network);
+                    try
+                    {
+                        var result = await explorerClient.BroadcastAsync(tx.Transaction, _Cts.Token);
+                        accounted = result.Success ||
+                                    result.RPCCode == RPCErrorCode.RPC_TRANSACTION_ALREADY_IN_CHAIN ||
+                                    !(
+                                    // Happen if a blocks mined a replacement
+                                    // Or if the tx is a double spend of something already in the mempool without rbf
+                                    result.RPCCode == RPCErrorCode.RPC_TRANSACTION_ERROR ||
+                                    // Happen if RBF is on and fee insufficient
+                                    result.RPCCode == RPCErrorCode.RPC_TRANSACTION_REJECTED);
+                    }
+                    // RPC might be unavailable, we can't check double spend so let's assume there is none
+                    catch
+                    {
+
+                    }
+                }
 
                 bool updated = false;
                 if (accounted != payment.Accounted)
@@ -258,65 +279,6 @@ namespace BTCPayServer.Payments.Bitcoin
             return invoice;
         }
 
-        class TransactionConflict
-        {
-            public Dictionary<uint256, TransactionResult> Transactions { get; set; } = new Dictionary<uint256, TransactionResult>();
-
-
-            uint256 _Winner;
-            public bool IsWinner(uint256 txId)
-            {
-                if (_Winner == null)
-                {
-                    var confirmed = Transactions.FirstOrDefault(t => t.Value.Confirmations >= 1);
-                    if (!confirmed.Equals(default(KeyValuePair<uint256, TransactionResult>)))
-                    {
-                        _Winner = confirmed.Key;
-                    }
-                    else
-                    {
-                        // Take the most recent (bitcoin node would not forward a conflict without a successful RBF)
-                        _Winner = Transactions
-                                .OrderByDescending(t => t.Value.Timestamp)
-                                .First()
-                                .Key;
-                    }
-                }
-                return _Winner == txId;
-            }
-        }
-        class TransactionConflicts : List<TransactionConflict>
-        {
-            public TransactionConflicts(IEnumerable<TransactionConflict> collection) : base(collection)
-            {
-
-            }
-
-            public TransactionConflict GetConflict(uint256 txId)
-            {
-                return this.FirstOrDefault(c => c.Transactions.ContainsKey(txId));
-            }
-        }
-        private TransactionConflicts GetConflicts(IEnumerable<TransactionResult> transactions)
-        {
-            Dictionary<OutPoint, TransactionConflict> conflictsByOutpoint = new Dictionary<OutPoint, TransactionConflict>();
-            foreach (var tx in transactions)
-            {
-                var hash = tx.Transaction.GetHash();
-                foreach (var input in tx.Transaction.Inputs)
-                {
-                    TransactionConflict conflict = new TransactionConflict();
-                    if (!conflictsByOutpoint.TryAdd(input.PrevOut, conflict))
-                    {
-                        conflict = conflictsByOutpoint[input.PrevOut];
-                    }
-                    if (!conflict.Transactions.ContainsKey(hash))
-                        conflict.Transactions.Add(hash, tx);
-                }
-            }
-            return new TransactionConflicts(conflictsByOutpoint.Where(c => c.Value.Transactions.Count > 1).Select(c => c.Value));
-        }
-
         private async Task<int> FindPaymentViaPolling(BTCPayWallet wallet, BTCPayNetwork network)
         {
             int totalPayment = 0;