From 42152050a3786bd3db103220e1a45b60c9fcddaf Mon Sep 17 00:00:00 2001 From: "nicolas.dorier" Date: Wed, 11 Mar 2020 20:46:37 +0900 Subject: [PATCH] Simplify RBF handling, and handle case of double spend happening outside of wallet (Fix #1375) --- BTCPayServer.Tests/UnitTest1.cs | 39 +++++ .../Payments/Bitcoin/NBXplorerListener.cs | 134 +++++++----------- 2 files changed, 87 insertions(+), 86 deletions(-) diff --git a/BTCPayServer.Tests/UnitTest1.cs b/BTCPayServer.Tests/UnitTest1.cs index 5fd023d7d..c739427ba 100644 --- a/BTCPayServer.Tests/UnitTest1.cs +++ b/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); + }); } } diff --git a/BTCPayServer/Payments/Bitcoin/NBXplorerListener.cs b/BTCPayServer/Payments/Bitcoin/NBXplorerListener.cs index eb4fc5cd5..72f59bed5 100644 --- a/BTCPayServer/Payments/Bitcoin/NBXplorerListener.cs +++ b/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 { @@ -38,7 +39,7 @@ namespace BTCPayServer.Payments.Bitcoin public NBXplorerListener(ExplorerClientProvider explorerClients, BTCPayWalletProvider wallets, InvoiceRepository invoiceRepository, - EventAggregator aggregator, + EventAggregator aggregator, IHostApplicationLifetime lifetime) { PollInterval = TimeSpan.FromMinutes(1.0); @@ -150,28 +151,28 @@ namespace BTCPayServer.Payments.Bitcoin CryptoCode = wallet.Network.CryptoCode, NewTransactionEvent = evt }); - foreach (var output in network.GetValidOutputs(evt)) + foreach (var output in network.GetValidOutputs(evt)) { - var key = output.Item1.ScriptPubKey.Hash + "#" + network.CryptoCode.ToUpperInvariant(); - var invoice = (await _InvoiceRepository.GetInvoicesFromAddresses(new [] {key})).FirstOrDefault(); - if (invoice != null) + var key = output.Item1.ScriptPubKey.Hash + "#" + network.CryptoCode.ToUpperInvariant(); + var invoice = (await _InvoiceRepository.GetInvoicesFromAddresses(new[] { key })).FirstOrDefault(); + if (invoice != null) + { + var address = network.NBXplorerNetwork.CreateAddress(evt.DerivationStrategy, + output.Item1.KeyPath, output.Item1.ScriptPubKey); + var paymentData = new BitcoinLikePaymentData(address, output.matchedOutput.Value, output.outPoint, evt.TransactionData.Transaction.RBF); + var alreadyExist = GetAllBitcoinPaymentData(invoice).Where(c => c.GetPaymentId() == paymentData.GetPaymentId()).Any(); + if (!alreadyExist) { - var address = network.NBXplorerNetwork.CreateAddress(evt.DerivationStrategy, - output.Item1.KeyPath, output.Item1.ScriptPubKey); - var paymentData = new BitcoinLikePaymentData(address, output.matchedOutput.Value, output.outPoint, evt.TransactionData.Transaction.RBF); - var alreadyExist = GetAllBitcoinPaymentData(invoice).Where(c => c.GetPaymentId() == paymentData.GetPaymentId()).Any(); - if (!alreadyExist) - { - var payment = await _InvoiceRepository.AddPayment(invoice.Id, DateTimeOffset.UtcNow, paymentData, network); - if(payment != null) - await ReceivedPayment(wallet, invoice, payment, evt.DerivationStrategy); - } - else - { - await UpdatePaymentStates(wallet, invoice.Id); - } + var payment = await _InvoiceRepository.AddPayment(invoice.Id, DateTimeOffset.UtcNow, paymentData, network); + if (payment != null) + await ReceivedPayment(wallet, invoice, payment, evt.DerivationStrategy); } - + else + { + await UpdatePaymentStates(wallet, invoice.Id); + } + } + } break; default: @@ -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); - var accounted = txConflict == null || txConflict.IsWinner(txId); + bool accounted = true; + 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 Transactions { get; set; } = new Dictionary(); - - - uint256 _Winner; - public bool IsWinner(uint256 txId) - { - if (_Winner == null) - { - var confirmed = Transactions.FirstOrDefault(t => t.Value.Confirmations >= 1); - if (!confirmed.Equals(default(KeyValuePair))) - { - _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 - { - public TransactionConflicts(IEnumerable collection) : base(collection) - { - - } - - public TransactionConflict GetConflict(uint256 txId) - { - return this.FirstOrDefault(c => c.Transactions.ContainsKey(txId)); - } - } - private TransactionConflicts GetConflicts(IEnumerable transactions) - { - Dictionary conflictsByOutpoint = new Dictionary(); - 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 FindPaymentViaPolling(BTCPayWallet wallet, BTCPayNetwork network) { int totalPayment = 0; @@ -339,17 +301,17 @@ namespace BTCPayServer.Payments.Bitcoin foreach (var coin in coins.Where(c => !alreadyAccounted.Contains(c.OutPoint))) { var transaction = await wallet.GetTransactionAsync(coin.OutPoint.Hash); - + var address = network.NBXplorerNetwork.CreateAddress(strategy, coin.KeyPath, coin.ScriptPubKey); var paymentData = new BitcoinLikePaymentData(address, coin.Value, coin.OutPoint, transaction.Transaction.RBF); - + var payment = await _InvoiceRepository.AddPayment(invoice.Id, coin.Timestamp, paymentData, network).ConfigureAwait(false); alreadyAccounted.Add(coin.OutPoint); if (payment != null) { invoice = await ReceivedPayment(wallet, invoice, payment, strategy); - if(invoice == null) + if (invoice == null) continue; totalPayment++; } @@ -385,7 +347,7 @@ namespace BTCPayServer.Payments.Bitcoin invoice.SetPaymentMethod(paymentMethod); } wallet.InvalidateCache(strategy); - _Aggregator.Publish(new InvoiceEvent(invoice, 1002, InvoiceEvent.ReceivedPayment){Payment = payment}); + _Aggregator.Publish(new InvoiceEvent(invoice, 1002, InvoiceEvent.ReceivedPayment) { Payment = payment }); return invoice; } public async Task StopAsync(CancellationToken cancellationToken)