From 8d1ff01ee882a76b0910f4960be7d2303e18dad5 Mon Sep 17 00:00:00 2001 From: Nicolas Dorier Date: Sun, 17 May 2020 05:07:24 +0900 Subject: [PATCH] Adapt payjoin implementation to the BIP (#1569) --- BTCPayServer.Tests/FakeServer.cs | 74 ++++++ BTCPayServer.Tests/PayJoinTests.cs | 231 ++++++++++++++---- BTCPayServer.Tests/TestAccount.cs | 6 + BTCPayServer/Controllers/WalletsController.cs | 2 +- .../PayJoin/PayJoinEndpointController.cs | 9 +- BTCPayServer/Services/PayjoinClient.cs | 48 +++- BTCPayServer/Views/Wallets/WalletSend.cshtml | 4 +- 7 files changed, 302 insertions(+), 72 deletions(-) create mode 100644 BTCPayServer.Tests/FakeServer.cs diff --git a/BTCPayServer.Tests/FakeServer.cs b/BTCPayServer.Tests/FakeServer.cs new file mode 100644 index 000000000..52f8f0571 --- /dev/null +++ b/BTCPayServer.Tests/FakeServer.cs @@ -0,0 +1,74 @@ +using System.Threading.Channels; +using System; +using System.Collections.Generic; +using System.Linq; +using System.Net.Http; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Hosting.Server.Features; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; + +namespace BTCPayServer.Tests +{ + public class FakeServer : IDisposable + { + IWebHost webHost; + SemaphoreSlim semaphore; + CancellationTokenSource cts = new CancellationTokenSource(); + public FakeServer() + { + _channel = Channel.CreateUnbounded(); + semaphore = new SemaphoreSlim(0); + } + + Channel _channel; + public async Task Start() + { + webHost = new WebHostBuilder() + .UseKestrel() + .UseUrls("http://127.0.0.1:0") + .Configure(appBuilder => + { + appBuilder.Run(async ctx => + { + await _channel.Writer.WriteAsync(ctx); + await semaphore.WaitAsync(cts.Token); + }); + }) + .Build(); + await webHost.StartAsync(); + var port = new Uri(webHost.ServerFeatures.Get().Addresses.First(), UriKind.Absolute) + .Port; + ServerUri = new Uri($"http://127.0.0.1:{port}/"); + } + + public Uri ServerUri { get; set; } + + public void Done() + { + semaphore.Release(); + } + + public async Task Stop() + { + await webHost.StopAsync(); + } + public void Dispose() + { + cts.Dispose(); + webHost?.Dispose(); + semaphore.Dispose(); + } + + public async Task GetNextRequest() + { + return await _channel.Reader.ReadAsync(); + } + } +} diff --git a/BTCPayServer.Tests/PayJoinTests.cs b/BTCPayServer.Tests/PayJoinTests.cs index f9a2c38b0..0e72be229 100644 --- a/BTCPayServer.Tests/PayJoinTests.cs +++ b/BTCPayServer.Tests/PayJoinTests.cs @@ -19,6 +19,7 @@ using BTCPayServer.Services.Invoices; using BTCPayServer.Services.Wallets; using BTCPayServer.Tests.Logging; using BTCPayServer.Views.Wallets; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; @@ -27,6 +28,7 @@ using NBitcoin; using NBitcoin.Altcoins; using NBitcoin.Payment; using NBitpayClient; +using NBXplorer.DerivationStrategy; using NBXplorer.Models; using Newtonsoft.Json.Linq; using OpenQA.Selenium; @@ -41,7 +43,7 @@ namespace BTCPayServer.Tests public PayJoinTests(ITestOutputHelper helper) { - Logs.Tester = new XUnitLog(helper) {Name = "Tests"}; + Logs.Tester = new XUnitLog(helper) { Name = "Tests" }; Logs.LogProvider = new XUnitLogProvider(helper); } @@ -76,16 +78,16 @@ namespace BTCPayServer.Tests var network = tester.NetworkProvider.GetNetwork("BTC"); var repo = tester.PayTester.GetService(); var outpoint = RandomOutpoint(); - + // Should not be locked Assert.False(await repo.TryUnlock(outpoint)); - + // Can lock input - Assert.True(await repo.TryLockInputs(new [] { outpoint })); + Assert.True(await repo.TryLockInputs(new[] { outpoint })); // Can't twice - Assert.False(await repo.TryLockInputs(new [] { outpoint })); + Assert.False(await repo.TryLockInputs(new[] { outpoint })); Assert.False(await repo.TryUnlock(outpoint)); - + // Lock and unlock outpoint utxo Assert.True(await repo.TryLock(outpoint)); Assert.True(await repo.TryUnlock(outpoint)); @@ -104,33 +106,33 @@ namespace BTCPayServer.Tests var controller = tester.PayTester.GetService(); //Only one utxo, so obvious result - var utxos = new[] {FakeUTXO(1.0m)}; + var utxos = new[] { FakeUTXO(1.0m) }; var paymentAmount = 0.5m; - var otherOutputs = new[] {0.5m}; - var inputs = new[] {1m}; + 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)); - + 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)}; + utxos = new[] { FakeUTXO(0.3m), FakeUTXO(0.7m) }; paymentAmount = 0.5m; - otherOutputs = new[] {0.5m}; - inputs = new[] {1m}; + 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)}; + 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}; + 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) { @@ -172,18 +174,18 @@ namespace BTCPayServer.Tests var unsupportedFormats = Enum.GetValues(typeof(ScriptPubKeyType)) .AssertType() .Where(type => !PayjoinClient.SupportedFormats.Contains(type)); - - + + foreach (ScriptPubKeyType senderAddressType in Enum.GetValues(typeof(ScriptPubKeyType))) { var senderUser = tester.NewAccount(); senderUser.GrantAccess(true); senderUser.RegisterDerivationScheme("BTC", senderAddressType); - + foreach (ScriptPubKeyType receiverAddressType in Enum.GetValues(typeof(ScriptPubKeyType))) { var senderCoin = await senderUser.ReceiveUTXO(Money.Satoshis(100000), network); - + Logs.Tester.LogInformation($"Testing payjoin with sender: {senderAddressType} receiver: {receiverAddressType}"); var receiverUser = tester.NewAccount(); receiverUser.GrantAccess(true); @@ -196,15 +198,16 @@ namespace BTCPayServer.Tests if (unsupportedFormats.Contains(receiverAddressType)) { errorCode = "unsupported-inputs"; - }else if (receiverAddressType != senderAddressType) + } + else if (receiverAddressType != senderAddressType) { errorCode = "out-of-utxos"; } - var invoice = receiverUser.BitPay.CreateInvoice(new Invoice() {Price = 50000, Currency = "sats", FullNotifications = true}); - + var invoice = receiverUser.BitPay.CreateInvoice(new Invoice() { Price = 50000, Currency = "sats", FullNotifications = true }); + var invoiceAddress = BitcoinAddress.Create(invoice.BitcoinAddress, cashCow.Network); var txBuilder = network.NBitcoinNetwork.CreateTransactionBuilder(); - + txBuilder.AddCoins(senderCoin); txBuilder.Send(invoiceAddress, invoice.BtcDue); txBuilder.SetChange(await senderUser.GetNewAddress(network)); @@ -214,12 +217,12 @@ namespace BTCPayServer.Tests var pj = await senderUser.SubmitPayjoin(invoice, psbt, errorCode, clientShouldError); } } - + } } - [Fact] - [Trait("Selenium", "Selenium")] + [Fact] + [Trait("Selenium", "Selenium")] public async Task CanUsePayjoinViaUI() { using (var s = SeleniumTester.Create()) @@ -346,7 +349,7 @@ namespace BTCPayServer.Tests .FindElement(By.ClassName("payment-value")); Assert.False(paymentValueRowColumn.Text.Contains("payjoin", StringComparison.InvariantCultureIgnoreCase)); - + TestUtils.Eventually(() => { s.GoToWallet(receiverWalletId, WalletsNavPages.Transactions); @@ -359,6 +362,126 @@ namespace BTCPayServer.Tests } } + [Fact] + [Trait("Integration", "Integration")] + public async Task CanUsePayjoin2() + { + using (var tester = ServerTester.Create()) + { + await tester.StartAsync(); + var pjClient = tester.PayTester.GetService(); + var nbx = tester.PayTester.GetService().GetExplorerClient("BTC"); + var notifications = await nbx.CreateWebsocketNotificationSessionAsync(); + var alice = tester.NewAccount(); + await alice.RegisterDerivationSchemeAsync("BTC", ScriptPubKeyType.Segwit, true); + await notifications.ListenDerivationSchemesAsync(new[] { alice.DerivationScheme }); + var address = (await nbx.GetUnusedAsync(alice.DerivationScheme, DerivationFeature.Deposit)).Address; + tester.ExplorerNode.SendToAddress(address, Money.Coins(1.0m)); + await notifications.NextEventAsync(); + var psbt = (await nbx.CreatePSBTAsync(alice.DerivationScheme, new CreatePSBTRequest() + { + Destinations = + { + new CreatePSBTDestination() + { + Amount = Money.Coins(0.5m), + Destination = new Key().PubKey.GetAddress(ScriptPubKeyType.Legacy, Network.RegTest) + } + }, + FeePreference = new FeePreference() + { + ExplicitFee = Money.Satoshis(3000) + } + })).PSBT; + var derivationSchemeSettings = alice.GetController().GetDerivationSchemeSettings(new WalletId(alice.StoreId, "BTC")); + var signingAccount = derivationSchemeSettings.GetSigningAccountKeySettings(); + psbt.SignAll(derivationSchemeSettings.AccountDerivation, alice.GenerateWalletResponseV.AccountHDKey, signingAccount.GetRootedKeyPath()); + var changeIndex = Array.FindIndex(psbt.Outputs.ToArray(), (PSBTOutput o) => o.ScriptPubKey.IsScriptType(ScriptType.P2WPKH)); + using var fakeServer = new FakeServer(); + await fakeServer.Start(); + var requesting = pjClient.RequestPayjoin(fakeServer.ServerUri, derivationSchemeSettings, psbt, default); + var request = await fakeServer.GetNextRequest(); + Assert.Equal("1", request.Request.Query["v"][0]); + Assert.Equal(changeIndex.ToString(), request.Request.Query["feebumpindex"][0]); + Assert.Equal("3000", request.Request.Query["maxfeebumpcontribution"][0]); + + + Logs.Tester.LogInformation("The payjoin receiver tries to make us pay lots of fee"); + var originalPSBT = await ParsePSBT(request); + var proposalTx = originalPSBT.GetGlobalTransaction(); + proposalTx.Outputs[changeIndex].Value -= Money.Satoshis(3001); + await request.Response.WriteAsync(PSBT.FromTransaction(proposalTx, Network.RegTest).ToBase64(), Encoding.UTF8); + fakeServer.Done(); + var ex = await Assert.ThrowsAsync(async () => await requesting); + Assert.Contains("too much fee", ex.Message); + + Logs.Tester.LogInformation("The payjoin receiver tries to send money to himself"); + requesting = pjClient.RequestPayjoin(fakeServer.ServerUri, derivationSchemeSettings, psbt, default); + request = await fakeServer.GetNextRequest(); + originalPSBT = await ParsePSBT(request); + proposalTx = originalPSBT.GetGlobalTransaction(); + proposalTx.Outputs.Where((o, i) => i != changeIndex).First().Value += Money.Satoshis(1); + proposalTx.Outputs[changeIndex].Value -= Money.Satoshis(1); + await request.Response.WriteAsync(PSBT.FromTransaction(proposalTx, Network.RegTest).ToBase64(), Encoding.UTF8); + fakeServer.Done(); + ex = await Assert.ThrowsAsync(async () => await requesting); + Assert.Contains("money to himself", ex.Message); + + Logs.Tester.LogInformation("The payjoin receiver can't increase the fee rate too much"); + requesting = pjClient.RequestPayjoin(fakeServer.ServerUri, derivationSchemeSettings, psbt, default); + request = await fakeServer.GetNextRequest(); + originalPSBT = await ParsePSBT(request); + proposalTx = originalPSBT.GetGlobalTransaction(); + proposalTx.Outputs[changeIndex].Value -= Money.Satoshis(3000); + await request.Response.WriteAsync(PSBT.FromTransaction(proposalTx, Network.RegTest).ToBase64(), Encoding.UTF8); + fakeServer.Done(); + ex = await Assert.ThrowsAsync(async () => await requesting); + Assert.Contains("increased the fee rate", ex.Message); + + Logs.Tester.LogInformation("Make sure the receiver implementation do not take more fee than allowed"); + var bob = tester.NewAccount(); + await bob.GrantAccessAsync(); + await bob.RegisterDerivationSchemeAsync("BTC", ScriptPubKeyType.Segwit, true); + await notifications.ListenDerivationSchemesAsync(new[] { bob.DerivationScheme }); + address = (await nbx.GetUnusedAsync(bob.DerivationScheme, DerivationFeature.Deposit)).Address; + tester.ExplorerNode.SendToAddress(address, Money.Coins(1.1m)); + await notifications.NextEventAsync(); + bob.ModifyStore(s => s.PayJoinEnabled = true); + var invoice = bob.BitPay.CreateInvoice( + new Invoice() { Price = 0.1m, Currency = "BTC", FullNotifications = true }); + var invoiceBIP21 = new BitcoinUrlBuilder(invoice.CryptoInfo.First().PaymentUrls.BIP21, + tester.ExplorerClient.Network.NBitcoinNetwork); + psbt = (await nbx.CreatePSBTAsync(alice.DerivationScheme, new CreatePSBTRequest() + { + Destinations = + { + new CreatePSBTDestination() + { + Amount = invoiceBIP21.Amount, + Destination = invoiceBIP21.Address + } + }, + FeePreference = new FeePreference() + { + ExplicitFee = Money.Satoshis(3001) + } + })).PSBT; + psbt.SignAll(derivationSchemeSettings.AccountDerivation, alice.GenerateWalletResponseV.AccountHDKey, signingAccount.GetRootedKeyPath()); + var endpoint = TestAccount.GetPayjoinEndpoint(invoice, Network.RegTest); + pjClient.MaxFeeBumpContribution = Money.Satoshis(50); + var proposal = await pjClient.RequestPayjoin(endpoint, derivationSchemeSettings, psbt, default); + Assert.True(proposal.TryGetFee(out var newFee)); + Assert.Equal(Money.Satoshis(3001 + 50), newFee); + } + } + + private static async Task ParsePSBT(Microsoft.AspNetCore.Http.HttpContext request) + { + var bytes = await request.Request.Body.ReadBytesAsync(int.Parse(request.Request.Headers["Content-Length"].First())); + var str = Encoding.UTF8.GetString(bytes); + return PSBT.Parse(str, Network.RegTest); + } + [Fact] [Trait("Integration", "Integration")] public async Task CanUsePayjoinFeeCornerCase() @@ -389,7 +512,7 @@ namespace BTCPayServer.Tests async Task RunVector(bool skipLockedCheck = false) { var coin = await senderUser.ReceiveUTXO(vector.SpentCoin, network); - var invoice = receiverUser.BitPay.CreateInvoice(new Invoice() {Price = vector.InvoiceAmount.ToDecimal(MoneyUnit.BTC), Currency = "BTC", FullNotifications = true}); + var invoice = receiverUser.BitPay.CreateInvoice(new Invoice() { Price = vector.InvoiceAmount.ToDecimal(MoneyUnit.BTC), Currency = "BTC", FullNotifications = true }); lastInvoiceId = invoice.Id; var invoiceAddress = BitcoinAddress.Create(invoice.BitcoinAddress, cashCow.Network); var txBuilder = network.NBitcoinNetwork.CreateTransactionBuilder(); @@ -447,7 +570,7 @@ namespace BTCPayServer.Tests vector = (SpentCoin: Money.Satoshis(810), InvoiceAmount: Money.Satoshis(700), Paid: Money.Satoshis(700), Fee: Money.Satoshis(110), InvoicePaid: true, ExpectedError: "not-enough-money"); await RunVector(); await LockAllButReceiverCoin(); - + Logs.Tester.LogInformation("We don't pay enough"); vector = (SpentCoin: Money.Satoshis(810), InvoiceAmount: Money.Satoshis(700), Paid: Money.Satoshis(690), Fee: Money.Satoshis(110), InvoicePaid: false, ExpectedError: "invoice-not-fully-paid"); await RunVector(); @@ -456,14 +579,14 @@ namespace BTCPayServer.Tests vector = (SpentCoin: Money.Satoshis(810), InvoiceAmount: Money.Satoshis(500), Paid: Money.Satoshis(500), Fee: Money.Satoshis(110), InvoicePaid: true, ExpectedError: null as string); await RunVector(); await LockAllButReceiverCoin(); - + Logs.Tester.LogInformation("We pay a little bit more the invoice with enough fees to support additional input\n" + "The receiver should have added a fake output"); vector = (SpentCoin: Money.Satoshis(910), InvoiceAmount: Money.Satoshis(500), Paid: Money.Satoshis(700), Fee: Money.Satoshis(110), InvoicePaid: true, ExpectedError: null as string); var proposedPSBT = await RunVector(); Assert.Equal(2, proposedPSBT.Outputs.Count); await LockAllButReceiverCoin(); - + Logs.Tester.LogInformation("We pay correctly, but no utxo\n" + "However, this has the side effect of having the receiver broadcasting the original tx"); await payjoinRepository.TryLock(receiverCoin.Outpoint); @@ -511,10 +634,10 @@ namespace BTCPayServer.Tests } else { - senderUser = originalSenderUser; + senderUser = originalSenderUser; } - - + + // Same as above. Except the sender send one satoshi less, so the change // output would get below dust and would be removed completely. // So we remove as much fee as we can, and still accept the transaction because it is above minrelay fee @@ -532,7 +655,7 @@ namespace BTCPayServer.Tests Assert.True(result.Success); } } - + [Fact(Timeout = TestTimeout)] [Trait("Integration", "Integration")] public async Task CanUsePayjoin() @@ -540,7 +663,7 @@ namespace BTCPayServer.Tests using (var tester = ServerTester.Create()) { await tester.StartAsync(); - + ////var payJoinStateProvider = tester.PayTester.GetService(); var btcPayNetwork = tester.NetworkProvider.GetNetwork("BTC"); var btcPayWallet = tester.PayTester.GetService().GetWallet(btcPayNetwork); @@ -552,7 +675,7 @@ namespace BTCPayServer.Tests senderUser.RegisterDerivationScheme("BTC", ScriptPubKeyType.Segwit, true); var invoice = senderUser.BitPay.CreateInvoice( - new Invoice() {Price = 100, Currency = "USD", FullNotifications = true}); + new Invoice() { Price = 100, Currency = "USD", FullNotifications = true }); //payjoin is not enabled by default. Assert.DoesNotContain($"{PayjoinClient.BIP21EndpointKey}", invoice.CryptoInfo.First().PaymentUrls.BIP21); cashCow.SendToAddress(BitcoinAddress.Create(invoice.BitcoinAddress, cashCow.Network), @@ -565,7 +688,7 @@ namespace BTCPayServer.Tests await receiverUser.EnablePayJoin(); // payjoin is enabled, with a segwit wallet, and the keys are available in nbxplorer invoice = receiverUser.BitPay.CreateInvoice( - new Invoice() {Price = 0.02m, Currency = "BTC", FullNotifications = true}); + new Invoice() { Price = 0.02m, Currency = "BTC", FullNotifications = true }); cashCow.SendToAddress(BitcoinAddress.Create(invoice.BitcoinAddress, cashCow.Network), Money.Coins(0.06m)); var receiverWalletId = new WalletId(receiverUser.StoreId, "BTC"); @@ -587,7 +710,7 @@ namespace BTCPayServer.Tests //Let's start the harassment invoice = receiverUser.BitPay.CreateInvoice( - new Invoice() {Price = 0.02m, Currency = "BTC", FullNotifications = true}); + new Invoice() { Price = 0.02m, Currency = "BTC", FullNotifications = true }); // Bad version should throw incorrect version var endpoint = TestAccount.GetPayjoinEndpoint(invoice, btcPayNetwork.NBitcoinNetwork); var response = await tester.PayTester.HttpClient.PostAsync(endpoint.AbsoluteUri + "?v=2", @@ -601,7 +724,7 @@ namespace BTCPayServer.Tests tester.ExplorerClient.Network.NBitcoinNetwork); var invoice2 = receiverUser.BitPay.CreateInvoice( - new Invoice() {Price = 0.02m, Currency = "BTC", FullNotifications = true}); + new Invoice() { Price = 0.02m, Currency = "BTC", FullNotifications = true }); var secondInvoiceParsedBip21 = new BitcoinUrlBuilder(invoice2.CryptoInfo.First().PaymentUrls.BIP21, tester.ExplorerClient.Network.NBitcoinNetwork); @@ -684,7 +807,7 @@ namespace BTCPayServer.Tests //Result: Reject Tx1 but accept tx 2 as its inputs were never accepted by invoice 1 await senderUser.SubmitPayjoin(invoice2, Invoice2Coin1, btcPayNetwork, "inputs-already-used"); var Invoice2Coin2ResponseTx = await senderUser.SubmitPayjoin(invoice2, Invoice2Coin2, btcPayNetwork); - + var contributedInputsInvoice2Coin2ResponseTx = Invoice2Coin2ResponseTx.Inputs.Where(txin => coin2.OutPoint != txin.PrevOut); Assert.Single(contributedInputsInvoice2Coin2ResponseTx); @@ -693,13 +816,13 @@ namespace BTCPayServer.Tests //Result: reject on 4: the protocol should not worry about this complexity var invoice3 = receiverUser.BitPay.CreateInvoice( - new Invoice() {Price = 0.01m, Currency = "BTC", FullNotifications = true}); + new Invoice() { Price = 0.01m, Currency = "BTC", FullNotifications = true }); var invoice3ParsedBip21 = new BitcoinUrlBuilder(invoice3.CryptoInfo.First().PaymentUrls.BIP21, tester.ExplorerClient.Network.NBitcoinNetwork); var invoice4 = receiverUser.BitPay.CreateInvoice( - new Invoice() {Price = 0.01m, Currency = "BTC", FullNotifications = true}); + new Invoice() { Price = 0.01m, Currency = "BTC", FullNotifications = true }); var invoice4ParsedBip21 = new BitcoinUrlBuilder(invoice4.CryptoInfo.First().PaymentUrls.BIP21, tester.ExplorerClient.Network.NBitcoinNetwork); @@ -720,7 +843,7 @@ namespace BTCPayServer.Tests //Result: proposed tx consolidates the outputs var invoice5 = receiverUser.BitPay.CreateInvoice( - new Invoice() {Price = 0.01m, Currency = "BTC", FullNotifications = true}); + new Invoice() { Price = 0.01m, Currency = "BTC", FullNotifications = true }); var invoice5ParsedBip21 = new BitcoinUrlBuilder(invoice5.CryptoInfo.First().PaymentUrls.BIP21, tester.ExplorerClient.Network.NBitcoinNetwork); @@ -745,7 +868,7 @@ namespace BTCPayServer.Tests new Money(0.1m, MoneyUnit.BTC))); var invoice6 = receiverUser.BitPay.CreateInvoice( - new Invoice() {Price = 0.01m, Currency = "BTC", FullNotifications = true}); + new Invoice() { Price = 0.01m, Currency = "BTC", FullNotifications = true }); var invoice6ParsedBip21 = new BitcoinUrlBuilder(invoice6.CryptoInfo.First().PaymentUrls.BIP21, tester.ExplorerClient.Network.NBitcoinNetwork); @@ -760,7 +883,7 @@ namespace BTCPayServer.Tests var invoice6Coin5 = invoice6Coin5TxBuilder .BuildTransaction(true); - var Invoice6Coin5Response1Tx =await senderUser.SubmitPayjoin(invoice6, invoice6Coin5, btcPayNetwork); + var Invoice6Coin5Response1Tx = await senderUser.SubmitPayjoin(invoice6, invoice6Coin5, btcPayNetwork); var Invoice6Coin5Response1TxSigned = invoice6Coin5TxBuilder.SignTransaction(Invoice6Coin5Response1Tx); //broadcast the first payjoin await tester.ExplorerClient.BroadcastAsync(Invoice6Coin5Response1TxSigned); @@ -788,7 +911,7 @@ namespace BTCPayServer.Tests new Money(0.1m, MoneyUnit.BTC))); var invoice7 = receiverUser.BitPay.CreateInvoice( - new Invoice() {Price = 0.01m, Currency = "BTC", FullNotifications = true}); + new Invoice() { Price = 0.01m, Currency = "BTC", FullNotifications = true }); var invoice7ParsedBip21 = new BitcoinUrlBuilder(invoice7.CryptoInfo.First().PaymentUrls.BIP21, tester.ExplorerClient.Network.NBitcoinNetwork); @@ -808,14 +931,14 @@ namespace BTCPayServer.Tests var Invoice7Coin6Response1TxSigned = invoice7Coin6TxBuilder.SignTransaction(invoice7Coin6Response1Tx); var contributedInputsInvoice7Coin6Response1TxSigned = Invoice7Coin6Response1TxSigned.Inputs.Single(txin => coin6.OutPoint != txin.PrevOut); - - + + ////var receiverWalletPayJoinState = payJoinStateProvider.Get(receiverWalletId); ////Assert.Contains(receiverWalletPayJoinState.GetRecords(), item => item.InvoiceId == invoice7.Id); //broadcast the payjoin var res = (await tester.ExplorerClient.BroadcastAsync(Invoice7Coin6Response1TxSigned)); Assert.True(res.Success); - + // Paid with coinjoin await TestUtils.EventuallyAsync(async () => { diff --git a/BTCPayServer.Tests/TestAccount.cs b/BTCPayServer.Tests/TestAccount.cs index 300426ecb..5911d0a59 100644 --- a/BTCPayServer.Tests/TestAccount.cs +++ b/BTCPayServer.Tests/TestAccount.cs @@ -145,6 +145,10 @@ namespace BTCPayServer.Tests public async Task CreateStoreAsync() { + if (UserId is null) + { + await RegisterAsync(); + } var store = this.GetController(); await store.CreateStore(new CreateStoreViewModel() {Name = "Test Store"}); StoreId = store.CreatedStoreId; @@ -161,6 +165,8 @@ namespace BTCPayServer.Tests public async Task RegisterDerivationSchemeAsync(string cryptoCode, ScriptPubKeyType segwit = ScriptPubKeyType.Legacy, bool importKeysToNBX = false) { + if (StoreId is null) + await CreateStoreAsync(); SupportedNetwork = parent.NetworkProvider.GetNetwork(cryptoCode); var store = parent.PayTester.GetController(UserId, StoreId); GenerateWalletResponseV = await parent.ExplorerClient.GenerateWalletAsync(new GenerateWalletRequest() diff --git a/BTCPayServer/Controllers/WalletsController.cs b/BTCPayServer/Controllers/WalletsController.cs index 48c710617..cc579c71c 100644 --- a/BTCPayServer/Controllers/WalletsController.cs +++ b/BTCPayServer/Controllers/WalletsController.cs @@ -1023,7 +1023,7 @@ namespace BTCPayServer.Controllers } } - private DerivationSchemeSettings GetDerivationSchemeSettings(WalletId walletId) + internal DerivationSchemeSettings GetDerivationSchemeSettings(WalletId walletId) { var paymentMethod = CurrentStore .GetSupportedPaymentMethods(NetworkProvider) diff --git a/BTCPayServer/Payments/PayJoin/PayJoinEndpointController.cs b/BTCPayServer/Payments/PayJoin/PayJoinEndpointController.cs index 6a7ac61e5..6ce47cbd3 100644 --- a/BTCPayServer/Payments/PayJoin/PayJoinEndpointController.cs +++ b/BTCPayServer/Payments/PayJoin/PayJoinEndpointController.cs @@ -117,7 +117,7 @@ namespace BTCPayServer.Payments.PayJoin [MediaTypeConstraint("text/plain")] [RateLimitsFilter(ZoneLimits.PayJoin, Scope = RateLimitsScope.RemoteAddress)] public async Task Submit(string cryptoCode, - bool noadjustfee = false, + long maxfeebumpcontribution = -1, int feebumpindex = -1, int v = 1) { @@ -130,6 +130,7 @@ namespace BTCPayServer.Payments.PayJoin new JProperty("message", "This version of payjoin is not supported.") }); } + Money allowedFeeBumpContribution = Money.Satoshis(maxfeebumpcontribution >= 0 ? maxfeebumpcontribution : long.MaxValue); var network = _btcPayNetworkProvider.GetNetwork(cryptoCode); if (network == null) { @@ -415,7 +416,7 @@ namespace BTCPayServer.Payments.PayJoin Money expectedFee = txBuilder.EstimateFees(newTx, originalFeeRate); Money actualFee = newTx.GetFee(txBuilder.FindSpentCoins(newTx)); Money additionalFee = expectedFee - actualFee; - if (additionalFee > Money.Zero && !noadjustfee) + if (additionalFee > Money.Zero) { // If the user overpaid, taking fee on our output (useful if sender dump a full UTXO for privacy) for (int i = 0; i < newTx.Outputs.Count && additionalFee > Money.Zero && due < Money.Zero; i++) @@ -433,7 +434,7 @@ namespace BTCPayServer.Payments.PayJoin } // The rest, we take from user's change - for (int i = 0; i < newTx.Outputs.Count && additionalFee > Money.Zero; i++) + for (int i = 0; i < newTx.Outputs.Count && additionalFee > Money.Zero && allowedFeeBumpContribution > Money.Zero; i++) { if (preferredFeeBumpOutput is TxOut && preferredFeeBumpOutput != newTx.Outputs[i]) @@ -443,8 +444,10 @@ namespace BTCPayServer.Payments.PayJoin var outputContribution = Money.Min(additionalFee, newTx.Outputs[i].Value); outputContribution = Money.Min(outputContribution, newTx.Outputs[i].Value - newTx.Outputs[i].GetDustThreshold(minRelayTxFee)); + outputContribution = Money.Min(outputContribution, allowedFeeBumpContribution); newTx.Outputs[i].Value -= outputContribution; additionalFee -= outputContribution; + allowedFeeBumpContribution -= outputContribution; } } diff --git a/BTCPayServer/Services/PayjoinClient.cs b/BTCPayServer/Services/PayjoinClient.cs index da546d568..658a5f69d 100644 --- a/BTCPayServer/Services/PayjoinClient.cs +++ b/BTCPayServer/Services/PayjoinClient.cs @@ -41,6 +41,13 @@ namespace BTCPayServer.Services } } + public class PayjoinClientParameters + { + public Money MaxFeeBumpContribution { get; set; } + public int? FeeBumpIndex { get; set; } + public int Version { get; set; } = 1; + } + public class PayjoinClient { public const string PayjoinOnionNamedClient = "payjoin.onion"; @@ -64,6 +71,8 @@ namespace BTCPayServer.Services _httpClientFactory = httpClientFactory; } + public Money MaxFeeBumpContribution { get; set; } + public async Task RequestPayjoin(Uri endpoint, DerivationSchemeSettings derivationSchemeSettings, PSBT originalTx, CancellationToken cancellationToken) { @@ -75,13 +84,17 @@ namespace BTCPayServer.Services throw new ArgumentNullException(nameof(originalTx)); if (originalTx.IsAllFinalized()) throw new InvalidOperationException("The original PSBT should not be finalized."); - + var clientParameters = new PayjoinClientParameters(); var type = derivationSchemeSettings.AccountDerivation.ScriptPubKeyType(); if (!SupportedFormats.Contains(type)) { throw new PayjoinSenderException($"The wallet does not support payjoin"); } var signingAccount = derivationSchemeSettings.GetSigningAccountKeySettings(); + var changeOutput = originalTx.Outputs.CoinsFor(derivationSchemeSettings.AccountDerivation, signingAccount.AccountKey, signingAccount.GetRootedKeyPath()) + .FirstOrDefault(); + if (changeOutput is PSBTOutput o) + clientParameters.FeeBumpIndex = (int)o.Index; var sentBefore = -originalTx.GetBalance(derivationSchemeSettings.AccountDerivation, signingAccount.AccountKey, signingAccount.GetRootedKeyPath()); @@ -89,11 +102,10 @@ namespace BTCPayServer.Services if (!originalTx.TryGetEstimatedFeeRate(out var originalFeeRate) || !originalTx.TryGetVirtualSize(out var oldVirtualSize)) throw new ArgumentException("originalTx should have utxo information", nameof(originalTx)); var originalFee = originalTx.GetFee(); + + clientParameters.MaxFeeBumpContribution = MaxFeeBumpContribution is null ? originalFee : MaxFeeBumpContribution; var cloned = originalTx.Clone(); - if (!cloned.TryFinalize(out var errors)) - { - return null; - } + cloned.Finalize(); // We make sure we don't send unnecessary information to the receiver foreach (var finalized in cloned.Inputs.Where(i => i.IsFinalized())) @@ -107,6 +119,8 @@ namespace BTCPayServer.Services } cloned.GlobalXPubs.Clear(); + + endpoint = ApplyOptionalParameters(endpoint, clientParameters); using HttpClient client = CreateHttpClient(endpoint); var bpuresponse = await client.PostAsync(endpoint, new StringContent(cloned.ToHex(), Encoding.UTF8, "text/plain"), cancellationToken); @@ -206,11 +220,6 @@ namespace BTCPayServer.Services if (ourInputCount < originalTx.Inputs.Count) throw new PayjoinSenderException("The payjoin receiver removed some of our inputs"); - // We limit the number of inputs the receiver can add - var addedInputs = newPSBT.Inputs.Count - originalTx.Inputs.Count; - if (addedInputs == 0) - throw new PayjoinSenderException("The payjoin receiver did not added any input"); - var sentAfter = -newPSBT.GetBalance(derivationSchemeSettings.AccountDerivation, signingAccount.AccountKey, signingAccount.GetRootedKeyPath()); @@ -224,8 +233,8 @@ namespace BTCPayServer.Services var additionalFee = newPSBT.GetFee() - originalFee; if (overPaying > additionalFee) throw new PayjoinSenderException("The payjoin receiver is sending more money to himself"); - if (overPaying > originalFee) - throw new PayjoinSenderException("The payjoin receiver is making us pay more than twice the original fee"); + if (overPaying > clientParameters.MaxFeeBumpContribution) + throw new PayjoinSenderException("The payjoin receiver is making us pay too much fee"); // Let's check the difference is only for the fee and that feerate // did not changed that much @@ -239,6 +248,21 @@ namespace BTCPayServer.Services return newPSBT; } + private static Uri ApplyOptionalParameters(Uri endpoint, PayjoinClientParameters clientParameters) + { + var requestUri = endpoint.AbsoluteUri; + if (requestUri.IndexOf('?', StringComparison.OrdinalIgnoreCase) is int i && i != -1) + requestUri = requestUri.Substring(0, i); + List parameters = new List(3); + parameters.Add($"v={clientParameters.Version}"); + if (clientParameters.FeeBumpIndex is int feeBumpIndex) + parameters.Add($"feebumpindex={feeBumpIndex}"); + if (clientParameters.MaxFeeBumpContribution is Money maxFeeBumpContribution) + parameters.Add($"maxfeebumpcontribution={maxFeeBumpContribution.Satoshi}"); + endpoint = new Uri($"{requestUri}?{string.Join('&', parameters)}"); + return endpoint; + } + private HttpClient CreateHttpClient(Uri uri) { if (uri.IsOnion()) diff --git a/BTCPayServer/Views/Wallets/WalletSend.cshtml b/BTCPayServer/Views/Wallets/WalletSend.cshtml index 7d8308893..ec4e9c446 100644 --- a/BTCPayServer/Views/Wallets/WalletSend.cshtml +++ b/BTCPayServer/Views/Wallets/WalletSend.cshtml @@ -144,11 +144,11 @@ { var feeRateOption = Model.RecommendedSatoshiPerByte[index]; + + }