From 90af9b135fb6f3d367b3a521700310e64d5a1e00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20A=2EP?= <53834183+Jossec101@users.noreply.github.com> Date: Thu, 19 Jun 2025 15:14:14 +0200 Subject: [PATCH 1/5] Add support for multiple withdrawal destinations and deprecate old fields --- src/Proto/nodeguard.proto | 13 ++++++- src/Rpc/NodeGuardService.cs | 67 +++++++++++++++++++++------------- src/Services/BitcoinService.cs | 58 ++++++++++++++++++++++++----- 3 files changed, 101 insertions(+), 37 deletions(-) diff --git a/src/Proto/nodeguard.proto b/src/Proto/nodeguard.proto index fa69694a..a511f37e 100644 --- a/src/Proto/nodeguard.proto +++ b/src/Proto/nodeguard.proto @@ -123,11 +123,18 @@ message GetNewWalletAddressResponse { string address = 1; } +message Destination { + // BTC address to send the funds to + string address = 1; + // Amount in satoshis + int64 amount_sats = 2; +} + message RequestWithdrawalRequest { int32 wallet_id = 1; - string address = 2; + string address = 2 [deprecated = true]; // Amount in satoshis - int64 amount = 3; + int64 amount = 3 [deprecated = true]; string description = 4; // in JSON format string request_metadata = 5; @@ -141,6 +148,8 @@ message RequestWithdrawalRequest { optional int32 custom_fee_rate = 9; // External reference id for the withdrawal request optional string reference_id = 10; + // Destinations for the withdrawal + repeated Destination destinations = 11; } message RequestWithdrawalResponse { diff --git a/src/Rpc/NodeGuardService.cs b/src/Rpc/NodeGuardService.cs index 10efdd96..bb251aac 100644 --- a/src/Rpc/NodeGuardService.cs +++ b/src/Rpc/NodeGuardService.cs @@ -170,6 +170,34 @@ public override async Task GetNewWalletAddress(GetN return getNewWalletAddressResponse; } + private void ValidateWithdrawalDestinations(IList destinations, bool isChangeless = false) + { + if (destinations == null || destinations.Count == 0) + { + throw new RpcException(new Status(StatusCode.InvalidArgument, "At least one destination must be provided")); + } + + // Changeless transactions can only have one destination + if (isChangeless && destinations.Count > 1) + { + throw new RpcException(new Status(StatusCode.InvalidArgument, "Changeless transactions can only have one destination")); + } + + foreach (var destination in destinations) + { + if (destination.AmountSats <= 0) + { + throw new RpcException(new Status(StatusCode.InvalidArgument, "Amount must be greater than 0")); + } + + if (string.IsNullOrEmpty(destination.Address)) + { + throw new RpcException(new Status(StatusCode.InvalidArgument, + "A destination address must be provided")); + } + } + } + public override async Task RequestWithdrawal(RequestWithdrawalRequest request, ServerCallContext context) { @@ -184,19 +212,8 @@ public override async Task RequestWithdrawal(RequestW throw new RpcException(new Status(StatusCode.NotFound, "Wallet not found")); } - if (request.Amount <= 0) - { - throw new RpcException(new Status(StatusCode.InvalidArgument, "Amount must be greater than 0")); - } - - if (request.Address == "") - { - throw new RpcException(new Status(StatusCode.InvalidArgument, - "A destination address must be provided")); - } - - //Create withdrawal request - var amount = new Money(request.Amount, MoneyUnit.Satoshi).ToUnit(MoneyUnit.BTC); + // Validate destinations + ValidateWithdrawalDestinations(request.Destinations, request.Changeless); var outpoints = new List(); var utxos = new List(); @@ -215,20 +232,20 @@ public override async Task RequestWithdrawal(RequestW throw new RpcException(new Status(StatusCode.Internal, "Derivation strategy not found")); utxos = await _coinSelectionService.GetUTXOsByOutpointAsync(derivationStrategyBase, outpoints); - amount = utxos.Sum(u => ((Money)u.Value).ToUnit(MoneyUnit.BTC)); + } + // Create destination objects for the withdrawal request + var withdrawalDestinations = request.Destinations.Select(d => new WalletWithdrawalRequestDestination() + { + Address = d.Address, + Amount = new Money(d.AmountSats, MoneyUnit.Satoshi).ToDecimal(MoneyUnit.BTC), + }).ToList(); + withdrawalRequest = new WalletWithdrawalRequest() { WalletId = request.WalletId, - WalletWithdrawalRequestDestinations = - [ - new WalletWithdrawalRequestDestination() - { - Address = request.Address, - Amount = amount, - } - ], + WalletWithdrawalRequestDestinations = withdrawalDestinations, Description = request.Description, Status = wallet.IsHotWallet ? WalletWithdrawalRequestStatus.PSBTSignaturesPending @@ -261,7 +278,7 @@ await _coinSelectionService.LockUTXOs(utxos, withdrawalRequest, BitcoinRequestType.WalletWithdrawal); } - //Update to refresh from db + // Update to refresh from db withdrawalRequest = await _walletWithdrawalRequestRepository.GetById(withdrawalRequest.Id); if (!withdrawalSaved.Item1) @@ -270,12 +287,12 @@ await _coinSelectionService.LockUTXOs(utxos, withdrawalRequest, throw new RpcException(new Status(StatusCode.Internal, "Error saving withdrawal request for wallet")); } - //Template PSBT generation with SIGHASH_ALL + // Template PSBT generation with SIGHASH_ALL var psbt = await _bitcoinService.GenerateTemplatePSBT(withdrawalRequest ?? throw new ArgumentException( nameof(withdrawalRequest))); - //If the wallet is hot, we send the withdrawal request to the node + // If the wallet is hot, we send the withdrawal request to the embedded or remote signer if (wallet.IsHotWallet) { var map = new JobDataMap(); diff --git a/src/Services/BitcoinService.cs b/src/Services/BitcoinService.cs index 31455521..9e06b8fe 100644 --- a/src/Services/BitcoinService.cs +++ b/src/Services/BitcoinService.cs @@ -81,7 +81,12 @@ public async Task GenerateTemplatePSBT(WalletWithdrawalRequest walletWithd { if (walletWithdrawalRequest == null) throw new ArgumentNullException(nameof(walletWithdrawalRequest)); - walletWithdrawalRequest = await _walletWithdrawalRequestRepository.GetById(walletWithdrawalRequest.Id); + var fetchedWithdrawalRequest = await _walletWithdrawalRequestRepository.GetById(walletWithdrawalRequest.Id); + if (fetchedWithdrawalRequest == null) + { + throw new InvalidOperationException($"Withdrawal request with Id {walletWithdrawalRequest.Id} not found."); + } + walletWithdrawalRequest = fetchedWithdrawalRequest; if (walletWithdrawalRequest.Status != WalletWithdrawalRequestStatus.Pending && walletWithdrawalRequest.Status != WalletWithdrawalRequestStatus.PSBTSignaturesPending) @@ -228,13 +233,11 @@ await _coinSelectionService.GetTxInputCoins(availableUTXOs, walletWithdrawalRequ builder.AddCoins(scriptCoins); var changelessAmount = selectedUTXOs.Sum(u => (Money)u.Value); - var amount = new Money(walletWithdrawalRequest.SatsAmount, MoneyUnit.Satoshi); - var destinationAddress = walletWithdrawalRequest.WalletWithdrawalRequestDestinations?.FirstOrDefault()?.Address; - if (string.IsNullOrEmpty(destinationAddress)) + var destinations = walletWithdrawalRequest.WalletWithdrawalRequestDestinations?.ToList(); + if (destinations == null || !destinations.Any()) { - throw new ArgumentException("Destination address is null or empty."); + throw new ArgumentException("No destination addresses provided."); } - var destination = BitcoinAddress.Create(destinationAddress, nbXplorerNetwork); builder.SetSigningOptions(SigHash.All) .SetChange(changeAddress.Address) @@ -246,14 +249,49 @@ await _coinSelectionService.GetTxInputCoins(availableUTXOs, walletWithdrawalRequ if (walletWithdrawalRequest.WithdrawAllFunds) { - builder.SendAll(destination); + // For withdraw all funds, send to the first destination only and check there's only one destination + if (destinations.Count > 1) + { + throw new ArgumentException("Withdraw all funds can only have one destination address."); + } + + var firstDestination = destinations.First(); + if (string.IsNullOrEmpty(firstDestination.Address)) + { + throw new ArgumentException("First destination address is null or empty."); + } + var firstDestinationAddress = BitcoinAddress.Create(firstDestination.Address, nbXplorerNetwork); + builder.SendAll(firstDestinationAddress); + } + else if (walletWithdrawalRequest.Changeless) + { + // Changeless transactions can only have one destination + if (destinations.Count > 1) + { + throw new ArgumentException("Changeless transactions can only have one destination address."); + } + + var destination = destinations.First(); + if (string.IsNullOrEmpty(destination.Address)) + { + throw new ArgumentException("Destination address is null or empty."); + } + var destinationAddress = BitcoinAddress.Create(destination.Address, nbXplorerNetwork); + builder.Send(destinationAddress, changelessAmount); + builder.SubtractFees(); } else { - builder.Send(destination, walletWithdrawalRequest.Changeless ? changelessAmount : amount); - if (walletWithdrawalRequest.Changeless) + // Handle multiple destinations for regular transactions + foreach (var dest in destinations) { - builder.SubtractFees(); + if (string.IsNullOrEmpty(dest.Address)) + { + throw new ArgumentException("Destination address is null or empty."); + } + var destinationAddress = BitcoinAddress.Create(dest.Address, nbXplorerNetwork); + var amount = new Money(dest.Amount, MoneyUnit.BTC); + builder.Send(destinationAddress, amount); } builder.SendAllRemainingToChange(); From b468eb25212c001bc45a5536da03ca1b148aad0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20A=2EP?= <53834183+Jossec101@users.noreply.github.com> Date: Thu, 19 Jun 2025 15:51:29 +0200 Subject: [PATCH 2/5] Refactor withdrawal request handling to throw an exception for null requests and update test to include withdrawal destinations --- src/Rpc/NodeGuardService.cs | 3 +-- test/NodeGuard.Tests/Rpc/NodeGuardServiceTests.cs | 8 ++++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Rpc/NodeGuardService.cs b/src/Rpc/NodeGuardService.cs index bb251aac..548261f0 100644 --- a/src/Rpc/NodeGuardService.cs +++ b/src/Rpc/NodeGuardService.cs @@ -289,8 +289,7 @@ await _coinSelectionService.LockUTXOs(utxos, withdrawalRequest, // Template PSBT generation with SIGHASH_ALL var psbt = await _bitcoinService.GenerateTemplatePSBT(withdrawalRequest ?? - throw new ArgumentException( - nameof(withdrawalRequest))); + throw new ArgumentException(nameof(withdrawalRequest))); // If the wallet is hot, we send the withdrawal request to the embedded or remote signer if (wallet.IsHotWallet) diff --git a/test/NodeGuard.Tests/Rpc/NodeGuardServiceTests.cs b/test/NodeGuard.Tests/Rpc/NodeGuardServiceTests.cs index 68e5f624..8a8d1345 100644 --- a/test/NodeGuard.Tests/Rpc/NodeGuardServiceTests.cs +++ b/test/NodeGuard.Tests/Rpc/NodeGuardServiceTests.cs @@ -632,9 +632,13 @@ public async Task RequestWithdrawal_Success() var requestWithdrawalRequest = new RequestWithdrawalRequest { WalletId = wallet.Id, - Amount = 100, Description = $"Request Withdrawal Test {DateTime.Now}", - Address = "bcrt1q590shaxaf5u08ml8jwlzghz99dup3z9592vxal", + Destinations = { new Destination + { + Address = "bcrt1q590shaxaf5u08ml8jwlzghz99dup3z9592vxal", + AmountSats = new Money(1, MoneyUnit.BTC).Satoshi + } + } }; //Act From a19cbaae6d80b442ef5bf4c3271c635e83476dec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20A=2EP?= <53834183+Jossec101@users.noreply.github.com> Date: Thu, 19 Jun 2025 16:09:33 +0200 Subject: [PATCH 3/5] Add unit test for GenerateTemplatePSBT with multiple withdrawal destinations --- .../Services/BitcoinServiceTests.cs | 102 +++++++++++++++++- 1 file changed, 101 insertions(+), 1 deletion(-) diff --git a/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs b/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs index 65e2afa1..12e450a5 100644 --- a/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs +++ b/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs @@ -8,7 +8,6 @@ using NBitcoin; using NBXplorer.DerivationStrategy; using NBXplorer.Models; -using NSubstitute; using NSubstitute.Exceptions; using Key = NodeGuard.Data.Models.Key; @@ -987,4 +986,105 @@ async Task PerformWithdrawal_LegacyMultiSigSucceeds() // Assert await act.Should().NotThrowAsync(); } + + [Fact] + async Task GenerateTemplatePSBT_MultipleDestinations_SingleSigSucceeds() + { + // Arrange + var wallet = CreateWallet.SingleSig(_internalWallet); + var withdrawalRequest = new WalletWithdrawalRequest() + { + Id = 1, + Status = WalletWithdrawalRequestStatus.Pending, + Wallet = wallet, + WalletWithdrawalRequestPSBTs = new List(), + WalletWithdrawalRequestDestinations = new List + { + new WalletWithdrawalRequestDestination + { + Address = "bcrt1qmde5y02qx2mywuzn05r50xkn9l6sv8h7646zyk", + Amount = 0.005m + }, + new WalletWithdrawalRequestDestination + { + Address = "bcrt1q9vzcaxm4xsq6p8rp8at7xsa2ehxncxdkdlrrwp", + Amount = 0.003m + }, + new WalletWithdrawalRequestDestination + { + Address = "bcrt1qpq9v4xhks7x5lgs7d54wzednkphan5uzqp6jw8", + Amount = 0.002m + } + } + }; + + var walletWithdrawalRequestRepository = new Mock(); + var walletWithdrawalRequestPsbtRepository = new Mock(); + var fmutxoRepository = new Mock(); + var nbXplorerService = new Mock(); + var utxoTagRepository = new Mock(); + var mapper = new Mock(); + walletWithdrawalRequestRepository + .Setup((w) => w.GetById(It.IsAny())) + .ReturnsAsync(withdrawalRequest); + walletWithdrawalRequestRepository + .Setup((w) => w.AddUTXOs(It.IsAny(), It.IsAny>())) + .ReturnsAsync((true, null)); + walletWithdrawalRequestPsbtRepository + .Setup((w) => w.AddAsync(It.IsAny())) + .ReturnsAsync((true, null)); + nbXplorerService + .Setup(x => x.GetStatusAsync(default)) + .ReturnsAsync(new StatusResult() { IsFullySynched = true }); + nbXplorerService + .Setup(x => x.GetUnusedAsync(It.IsAny(), DerivationFeature.Change, 0, false, default)) + .ReturnsAsync(new KeyPathInformation() { Address = BitcoinAddress.Create("bcrt1qhkvrjg9wa7h3sasl7260ehstwtcgq62a3udy5p", Network.RegTest) }); + nbXplorerService + .Setup(x => x.GetUTXOsAsync(It.IsAny(), default)) + .ReturnsAsync(new UTXOChanges() + { + Confirmed = new UTXOChange() + { + UTXOs = new List() + { + new UTXO() + { + Value = new Money((long)20000000), // 0.2 BTC - enough for multiple outputs plus fees + ScriptPubKey = wallet.GetDerivationStrategy().GetDerivation(KeyPath.Parse("0/0")).ScriptPubKey, + KeyPath = KeyPath.Parse("0/0") + } + } + } + }); + fmutxoRepository + .Setup(x => x.GetLockedUTXOs(null, null)) + .ReturnsAsync(new List()); + utxoTagRepository + .Setup(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + + var coinSelectionService = new CoinSelectionService(_logger, mapper.Object, fmutxoRepository.Object, nbXplorerService.Object, null, walletWithdrawalRequestRepository.Object, utxoTagRepository.Object); + + var bitcoinService = new BitcoinService(_logger, mapper.Object, walletWithdrawalRequestRepository.Object, walletWithdrawalRequestPsbtRepository.Object, null, null, nbXplorerService.Object, coinSelectionService); + + // Act + var result = await bitcoinService.GenerateTemplatePSBT(withdrawalRequest); + + // Assert + result.Should().NotBeNull(); + + // Verify that the PSBT has the expected number of outputs + // 3 destination outputs + 1 change output = 4 total outputs + result.Outputs.Count.Should().Be(4); + + // Verify destination amounts are correct + var destinationOutputs = result.Outputs.Take(3).ToList(); + destinationOutputs[0].Value.Should().Be(new Money(0.005m, MoneyUnit.BTC)); + destinationOutputs[1].Value.Should().Be(new Money(0.003m, MoneyUnit.BTC)); + destinationOutputs[2].Value.Should().Be(new Money(0.002m, MoneyUnit.BTC)); + + // Verify that there is a change output (the 4th output) + var changeOutput = result.Outputs[3]; + changeOutput.Value.Satoshi.Should().BeGreaterThan(0); + } } From 50987a3e7143a2ff58eeb311bc77b495e99f4c03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20A=2EP?= <53834183+Jossec101@users.noreply.github.com> Date: Thu, 19 Jun 2025 16:57:23 +0200 Subject: [PATCH 4/5] Enhance GenerateTemplatePSBT tests for withdrawal requests with multiple destinations and validate withdraw all funds scenarios --- .../Services/BitcoinServiceTests.cs | 365 +++++++++++++++++- 1 file changed, 357 insertions(+), 8 deletions(-) diff --git a/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs b/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs index 12e450a5..9a51af49 100644 --- a/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs +++ b/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs @@ -1077,14 +1077,363 @@ async Task GenerateTemplatePSBT_MultipleDestinations_SingleSigSucceeds() // 3 destination outputs + 1 change output = 4 total outputs result.Outputs.Count.Should().Be(4); - // Verify destination amounts are correct - var destinationOutputs = result.Outputs.Take(3).ToList(); - destinationOutputs[0].Value.Should().Be(new Money(0.005m, MoneyUnit.BTC)); - destinationOutputs[1].Value.Should().Be(new Money(0.003m, MoneyUnit.BTC)); - destinationOutputs[2].Value.Should().Be(new Money(0.002m, MoneyUnit.BTC)); + // Verify destination amounts are present (order may vary due to shuffling) + var outputValues = result.Outputs.Select(o => o.Value).ToList(); + outputValues.Should().Contain(new Money(0.005m, MoneyUnit.BTC)); + outputValues.Should().Contain(new Money(0.003m, MoneyUnit.BTC)); + outputValues.Should().Contain(new Money(0.002m, MoneyUnit.BTC)); - // Verify that there is a change output (the 4th output) - var changeOutput = result.Outputs[3]; - changeOutput.Value.Satoshi.Should().BeGreaterThan(0); + // Verify that there is a change output (should be greater than the destination amounts) + var changeOutput = outputValues.Where(v => v > new Money(0.005m, MoneyUnit.BTC)).FirstOrDefault(); + changeOutput.Should().NotBeNull(); + changeOutput!.Satoshi.Should().BeGreaterThan(0); + } + + + [Fact] + async Task GenerateTemplatePSBT_WithdrawAllFunds_SingleSigSucceeds() + { + // Arrange + var wallet = CreateWallet.SingleSig(_internalWallet); + var withdrawalRequest = new WalletWithdrawalRequest() + { + Id = 1, + Status = WalletWithdrawalRequestStatus.Pending, + Wallet = wallet, + WithdrawAllFunds = true, + WalletWithdrawalRequestPSBTs = new List(), + WalletWithdrawalRequestDestinations = new List + { + new WalletWithdrawalRequestDestination + { + Address = "bcrt1q8k3av6q5yp83rn332lx8a90k6kukhg28hs5qw7krdq95t629hgsqk6ztmf", + Amount = 0m // Will be set to full balance + } + } + }; + + var walletWithdrawalRequestRepository = new Mock(); + var walletWithdrawalRequestPsbtRepository = new Mock(); + var fmutxoRepository = new Mock(); + var nbXplorerService = new Mock(); + var utxoTagRepository = new Mock(); + var mapper = new Mock(); + + walletWithdrawalRequestRepository + .Setup((w) => w.GetById(It.IsAny())) + .ReturnsAsync(withdrawalRequest); + walletWithdrawalRequestRepository + .Setup((w) => w.Update(It.IsAny())) + .Returns((true, null)); + walletWithdrawalRequestRepository + .Setup((w) => w.AddUTXOs(It.IsAny(), It.IsAny>())) + .ReturnsAsync((true, null)); + walletWithdrawalRequestPsbtRepository + .Setup((w) => w.AddAsync(It.IsAny())) + .ReturnsAsync((true, null)); + nbXplorerService + .Setup(x => x.GetStatusAsync(default)) + .ReturnsAsync(new StatusResult() { IsFullySynched = true }); + nbXplorerService + .Setup(x => x.GetBalanceAsync(It.IsAny(), default)) + .ReturnsAsync(new GetBalanceResponse() { Confirmed = new Money((long)50000000) }); // 0.5 BTC + nbXplorerService + .Setup(x => x.GetUnusedAsync(It.IsAny(), DerivationFeature.Change, 0, false, default)) + .ReturnsAsync(new KeyPathInformation() { Address = BitcoinAddress.Create("bcrt1q83ml8tve8vh672wsm83getxfzetaquq352jr6t423tdwjvdz3f3qe4r4t7", Network.RegTest) }); + nbXplorerService + .Setup(x => x.GetUTXOsAsync(It.IsAny(), default)) + .ReturnsAsync(new UTXOChanges() + { + Confirmed = new UTXOChange() + { + UTXOs = new List() + { + new UTXO() + { + Value = new Money((long)50000000), + ScriptPubKey = wallet.GetDerivationStrategy().GetDerivation(KeyPath.Parse("0/0")).ScriptPubKey, + KeyPath = KeyPath.Parse("0/0") + } + } + } + }); + fmutxoRepository + .Setup(x => x.GetLockedUTXOs(null, null)) + .ReturnsAsync(new List()); + utxoTagRepository + .Setup(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + + var coinSelectionService = new CoinSelectionService(_logger, mapper.Object, fmutxoRepository.Object, nbXplorerService.Object, null, walletWithdrawalRequestRepository.Object, utxoTagRepository.Object); + var bitcoinService = new BitcoinService(_logger, mapper.Object, walletWithdrawalRequestRepository.Object, walletWithdrawalRequestPsbtRepository.Object, null, null, nbXplorerService.Object, coinSelectionService); + + // Act + var result = await bitcoinService.GenerateTemplatePSBT(withdrawalRequest); + + // Assert + result.Should().NotBeNull(); + // The destination amount should have been updated to the full balance + withdrawalRequest.WalletWithdrawalRequestDestinations.First().Amount.Should().Be(0.5m); + // For withdraw all funds, verify that all available funds are being sent + result.Outputs.Should().HaveCountGreaterOrEqualTo(1); + } + + [Fact] + async Task GenerateTemplatePSBT_WithdrawAllFunds_MultipleDestinations_ShouldFail() + { + // Arrange + var wallet = CreateWallet.SingleSig(_internalWallet); + var withdrawalRequest = new WalletWithdrawalRequest() + { + Id = 1, + Status = WalletWithdrawalRequestStatus.Pending, + Wallet = wallet, + WithdrawAllFunds = true, + WalletWithdrawalRequestPSBTs = new List(), + WalletWithdrawalRequestDestinations = new List + { + new WalletWithdrawalRequestDestination + { + Address = "bcrt1q8k3av6q5yp83rn332lx8a90k6kukhg28hs5qw7krdq95t629hgsqk6ztmf", + Amount = 0m + }, + new WalletWithdrawalRequestDestination + { + Address = "bcrt1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh", + Amount = 0m + } + } + }; + + var walletWithdrawalRequestRepository = new Mock(); + var walletWithdrawalRequestPsbtRepository = new Mock(); + var fmutxoRepository = new Mock(); + var nbXplorerService = new Mock(); + var utxoTagRepository = new Mock(); + var mapper = new Mock(); + + walletWithdrawalRequestRepository + .Setup((w) => w.GetById(It.IsAny())) + .ReturnsAsync(withdrawalRequest); + walletWithdrawalRequestRepository + .Setup((w) => w.Update(It.IsAny())) + .Returns((true, null)); + walletWithdrawalRequestRepository + .Setup((w) => w.AddUTXOs(It.IsAny(), It.IsAny>())) + .ReturnsAsync((true, null)); + walletWithdrawalRequestPsbtRepository + .Setup((w) => w.AddAsync(It.IsAny())) + .ReturnsAsync((true, null)); + nbXplorerService + .Setup(x => x.GetStatusAsync(default)) + .ReturnsAsync(new StatusResult() { IsFullySynched = true }); + nbXplorerService + .Setup(x => x.GetBalanceAsync(It.IsAny(), default)) + .ReturnsAsync(new GetBalanceResponse() { Confirmed = new Money((long)50000000) }); + nbXplorerService + .Setup(x => x.GetUnusedAsync(It.IsAny(), DerivationFeature.Change, 0, false, default)) + .ReturnsAsync(new KeyPathInformation() { Address = BitcoinAddress.Create("bcrt1q83ml8tve8vh672wsm83getxfzetaquq352jr6t423tdwjvdz3f3qe4r4t7", Network.RegTest) }); + nbXplorerService + .Setup(x => x.GetUTXOsAsync(It.IsAny(), default)) + .ReturnsAsync(new UTXOChanges() + { + Confirmed = new UTXOChange() + { + UTXOs = new List() + { + new UTXO() + { + Value = new Money((long)50000000), + ScriptPubKey = wallet.GetDerivationStrategy()!.GetDerivation(KeyPath.Parse("0/0")).ScriptPubKey, + KeyPath = KeyPath.Parse("0/0") + } + } + } + }); + fmutxoRepository + .Setup(x => x.GetLockedUTXOs(null, null)) + .ReturnsAsync(new List()); + utxoTagRepository + .Setup(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + + var coinSelectionService = new CoinSelectionService(_logger, mapper.Object, fmutxoRepository.Object, nbXplorerService.Object, null, walletWithdrawalRequestRepository.Object, utxoTagRepository.Object); + var bitcoinService = new BitcoinService(_logger, mapper.Object, walletWithdrawalRequestRepository.Object, walletWithdrawalRequestPsbtRepository.Object, null, null, nbXplorerService.Object, coinSelectionService); + + // Act + var act = () => bitcoinService.GenerateTemplatePSBT(withdrawalRequest); + + // Assert + await act + .Should() + .ThrowAsync() + .WithMessage("Withdraw all funds can only have one destination address."); + } + + [Fact] + async Task GenerateTemplatePSBT_Changeless_MultipleDestinations_ShouldFail() + { + // Arrange + var wallet = CreateWallet.SingleSig(_internalWallet); + var withdrawalRequest = new WalletWithdrawalRequest() + { + Id = 1, + Status = WalletWithdrawalRequestStatus.Pending, + Wallet = wallet, + Changeless = true, + WalletWithdrawalRequestPSBTs = new List(), + WalletWithdrawalRequestDestinations = new List + { + new WalletWithdrawalRequestDestination + { + Address = "bcrt1q8k3av6q5yp83rn332lx8a90k6kukhg28hs5qw7krdq95t629hgsqk6ztmf", + Amount = 0.01m + }, + new WalletWithdrawalRequestDestination + { + Address = "bcrt1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh", + Amount = 0.005m + } + } + }; + + var walletWithdrawalRequestRepository = new Mock(); + var walletWithdrawalRequestPsbtRepository = new Mock(); + var fmutxoRepository = new Mock(); + var nbXplorerService = new Mock(); + var utxoTagRepository = new Mock(); + var mapper = new Mock(); + + walletWithdrawalRequestRepository + .Setup((w) => w.GetById(It.IsAny())) + .ReturnsAsync(withdrawalRequest); + walletWithdrawalRequestRepository + .Setup((w) => w.AddUTXOs(It.IsAny(), It.IsAny>())) + .ReturnsAsync((true, null)); + walletWithdrawalRequestPsbtRepository + .Setup((w) => w.AddAsync(It.IsAny())) + .ReturnsAsync((true, null)); + nbXplorerService + .Setup(x => x.GetStatusAsync(default)) + .ReturnsAsync(new StatusResult() { IsFullySynched = true }); + nbXplorerService + .Setup(x => x.GetUnusedAsync(It.IsAny(), DerivationFeature.Change, 0, false, default)) + .ReturnsAsync(new KeyPathInformation() { Address = BitcoinAddress.Create("bcrt1q83ml8tve8vh672wsm83getxfzetaquq352jr6t423tdwjvdz3f3qe4r4t7", Network.RegTest) }); + nbXplorerService + .Setup(x => x.GetUTXOsAsync(It.IsAny(), default)) + .ReturnsAsync(new UTXOChanges() + { + Confirmed = new UTXOChange() + { + UTXOs = new List() + { + new UTXO() + { + Value = new Money((long)20000000), + ScriptPubKey = wallet.GetDerivationStrategy()!.GetDerivation(KeyPath.Parse("0/0")).ScriptPubKey, + KeyPath = KeyPath.Parse("0/0") + } + } + } + }); + fmutxoRepository + .Setup(x => x.GetLockedUTXOs(null, null)) + .ReturnsAsync(new List()); + utxoTagRepository + .Setup(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + + // Mock to return UTXOs for changeless validation + walletWithdrawalRequestRepository + .Setup(x => x.GetUTXOs(It.IsAny())) + .ReturnsAsync((true, new List + { + new FMUTXO + { + TxId = "abc123", + OutputIndex = 0, + SatsAmount = 20000000 // 0.2 BTC + } + })); + + var coinSelectionService = new CoinSelectionService(_logger, mapper.Object, fmutxoRepository.Object, nbXplorerService.Object, null, walletWithdrawalRequestRepository.Object, utxoTagRepository.Object); + var bitcoinService = new BitcoinService(_logger, mapper.Object, walletWithdrawalRequestRepository.Object, walletWithdrawalRequestPsbtRepository.Object, null, null, nbXplorerService.Object, coinSelectionService); + + // Act + var act = () => bitcoinService.GenerateTemplatePSBT(withdrawalRequest); + + // Assert + await act + .Should() + .ThrowAsync() + .WithMessage("Changeless transactions can only have one destination address."); + } + + [Fact] + async Task GenerateTemplatePSBT_ReuseTemplatePSBT_WhenUTXOsStillValid() + { + // Arrange + var wallet = CreateWallet.SingleSig(_internalWallet); + var existingTemplatePSBT = "cHNidP8BAIkBAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wD/////AkBCDwAAAAAAIgAgPaPWaBQgTxHOMVfMfpX21blroUe8KAd6w2gLRelFuiCsUYkAAAAAACIAIDx3862ZOy+vKdDZ4oysyRZX0HARoqQ9LqqK2ukxoopiAAAAAE8BBDWHzwN9uUaNAAAAAYPR/OiA1LbTzxbLPvbXvtAwckIG3g+0T1zblR/ZodaiA5zBFsigPpL8htN/KJ/Ph8SPvQA/K+mSNXTSA0hgvPNuEO0CEMgwAACAAQAAgAEAAAAAAQEfgJaYAAAAAAAWABTpOvUBMqNMfl7P81etji6x4fXrMyIGA3uD9HVjgF5E+eQhHp+Na6femVYpc4bCA4DmimehAdWcGO0CEMgwAACAAQAAgAEAAAAAAAAAAAAAAAAAAA=="; + var withdrawalRequest = new WalletWithdrawalRequest() + { + Id = 1, + Status = WalletWithdrawalRequestStatus.Pending, + Wallet = wallet, + WalletWithdrawalRequestPSBTs = new List + { + new WalletWithdrawalRequestPSBT + { + Id = 1, + IsTemplatePSBT = true, + PSBT = existingTemplatePSBT + } + }, + WalletWithdrawalRequestDestinations = new List + { + new WalletWithdrawalRequestDestination + { + Address = "bcrt1q8k3av6q5yp83rn332lx8a90k6kukhg28hs5qw7krdq95t629hgsqk6ztmf", + Amount = 0.01m + } + } + }; + + var walletWithdrawalRequestRepository = new Mock(); + var nbXplorerService = new Mock(); + + walletWithdrawalRequestRepository + .Setup((w) => w.GetById(It.IsAny())) + .ReturnsAsync(withdrawalRequest); + nbXplorerService + .Setup(x => x.GetStatusAsync(default)) + .ReturnsAsync(new StatusResult() { IsFullySynched = true }); + nbXplorerService + .Setup(x => x.GetUTXOsAsync(It.IsAny(), default)) + .ReturnsAsync(new UTXOChanges() + { + Confirmed = new UTXOChange() + { + UTXOs = new List() + { + new UTXO() + { + Outpoint = new OutPoint(), // This matches the PSBT input + Value = new Money((long)10000000), + ScriptPubKey = wallet.GetDerivationStrategy().GetDerivation(KeyPath.Parse("0/0")).ScriptPubKey, + KeyPath = KeyPath.Parse("0/0") + } + } + } + }); + + var bitcoinService = new BitcoinService(_logger, null, walletWithdrawalRequestRepository.Object, null, null, null, nbXplorerService.Object, null); + + // Act + var result = await bitcoinService.GenerateTemplatePSBT(withdrawalRequest); + + // Assert + result.Should().NotBeNull(); + result.ToBase64().Should().Be(existingTemplatePSBT); // Should return the existing template PSBT } } From 784160b36918f79f67933b1adf8f695beaa8ebde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20A=2EP?= <53834183+Jossec101@users.noreply.github.com> Date: Thu, 19 Jun 2025 17:00:49 +0200 Subject: [PATCH 5/5] Fix changeless txs and throw error on the method --- src/Services/BitcoinService.cs | 166 +++++++++++++++------------------ 1 file changed, 77 insertions(+), 89 deletions(-) diff --git a/src/Services/BitcoinService.cs b/src/Services/BitcoinService.cs index 9e06b8fe..77cf0865 100644 --- a/src/Services/BitcoinService.cs +++ b/src/Services/BitcoinService.cs @@ -177,13 +177,6 @@ public async Task GenerateTemplatePSBT(WalletWithdrawalRequest walletWithd await _coinSelectionService.GetLockedUTXOsForRequest(walletWithdrawalRequest, BitcoinRequestType.WalletWithdrawal); - if (walletWithdrawalRequest.Changeless && previouslyLockedUTXOs.Count == 0) - { - _logger.LogError( - "Cannot generate base template PSBT for withdrawal request: {RequestId}, no UTXOs were provided for the changeless operation", - walletWithdrawalRequest.Id); - throw new ArgumentException(); - } var availableUTXOs = previouslyLockedUTXOs.Count > 0 ? previouslyLockedUTXOs @@ -204,109 +197,104 @@ await _coinSelectionService.GetTxInputCoins(availableUTXOs, walletWithdrawalRequ var nbXplorerNetwork = CurrentNetworkHelper.GetCurrentNetwork(); PSBT? originalPSBT = null; - try + + //We got enough inputs to fund the TX so time to build the PSBT + + var network = CurrentNetworkHelper.GetCurrentNetwork(); + var txBuilder = nbXplorerNetwork.CreateTransactionBuilder(); + + //We get the mempool.space recommended fees, the custom or use the bitcoin core (nbxplorer) one if not available + var feerate = await _nbXplorerService.GetFeesByType(walletWithdrawalRequest.MempoolRecommendedFeesType) + ?? walletWithdrawalRequest.CustomFeeRate + ?? (await LightningHelper.GetFeeRateResult(network, _nbXplorerService)).FeeRate + .SatoshiPerByte; + var feeRateResult = new FeeRate(feerate); + + var changeAddress = await _nbXplorerService.GetUnusedAsync(derivationStrategy, DerivationFeature.Change, + 0, false, default); + + if (changeAddress == null) { - //We got enough inputs to fund the TX so time to build the PSBT + _logger.LogError("Change address was not found for wallet: {WalletId}", + walletWithdrawalRequest.Wallet.Id); + throw new ArgumentNullException( + $"Change address was not found for wallet: {walletWithdrawalRequest.Wallet.Id}"); + } - var network = CurrentNetworkHelper.GetCurrentNetwork(); - var txBuilder = nbXplorerNetwork.CreateTransactionBuilder(); + var builder = txBuilder; + builder.AddCoins(scriptCoins); - //We get the mempool.space recommended fees, the custom or use the bitcoin core (nbxplorer) one if not available - var feerate = await _nbXplorerService.GetFeesByType(walletWithdrawalRequest.MempoolRecommendedFeesType) - ?? walletWithdrawalRequest.CustomFeeRate - ?? (await LightningHelper.GetFeeRateResult(network, _nbXplorerService)).FeeRate - .SatoshiPerByte; - var feeRateResult = new FeeRate(feerate); + var changelessAmount = selectedUTXOs.Sum(u => (Money)u.Value); + var destinations = walletWithdrawalRequest.WalletWithdrawalRequestDestinations?.ToList(); + if (destinations == null || !destinations.Any()) + { + throw new ArgumentException("No destination addresses provided."); + } + + builder.SetSigningOptions(SigHash.All) + .SetChange(changeAddress.Address) + .SendEstimatedFees(feeRateResult); - var changeAddress = await _nbXplorerService.GetUnusedAsync(derivationStrategy, DerivationFeature.Change, - 0, false, default); + // We preserve the output order when testing so the psbt doesn't change + var command = Assembly.GetEntryAssembly()?.GetName().Name?.ToLowerInvariant(); + builder.ShuffleOutputs = command != "ef" && (command != null && !command.Contains("test")); - if (changeAddress == null) + if (walletWithdrawalRequest.WithdrawAllFunds) + { + // For withdraw all funds, send to the first destination only and check there's only one destination + if (destinations.Count > 1) { - _logger.LogError("Change address was not found for wallet: {WalletId}", - walletWithdrawalRequest.Wallet.Id); - throw new ArgumentNullException( - $"Change address was not found for wallet: {walletWithdrawalRequest.Wallet.Id}"); + throw new ArgumentException("Withdraw all funds can only have one destination address."); } - var builder = txBuilder; - builder.AddCoins(scriptCoins); - - var changelessAmount = selectedUTXOs.Sum(u => (Money)u.Value); - var destinations = walletWithdrawalRequest.WalletWithdrawalRequestDestinations?.ToList(); - if (destinations == null || !destinations.Any()) + var firstDestination = destinations.First(); + if (string.IsNullOrEmpty(firstDestination.Address)) { - throw new ArgumentException("No destination addresses provided."); + throw new ArgumentException("First destination address is null or empty."); + } + var firstDestinationAddress = BitcoinAddress.Create(firstDestination.Address, nbXplorerNetwork); + builder.SendAll(firstDestinationAddress); + } + else if (walletWithdrawalRequest.Changeless) + { + // Changeless transactions can only have one destination + if (destinations.Count > 1) + { + throw new ArgumentException("Changeless transactions can only have one destination address."); } - builder.SetSigningOptions(SigHash.All) - .SetChange(changeAddress.Address) - .SendEstimatedFees(feeRateResult); - - // We preserve the output order when testing so the psbt doesn't change - var command = Assembly.GetEntryAssembly()?.GetName().Name?.ToLowerInvariant(); - builder.ShuffleOutputs = command != "ef" && (command != null && !command.Contains("test")); - - if (walletWithdrawalRequest.WithdrawAllFunds) + var destination = destinations.First(); + if (string.IsNullOrEmpty(destination.Address)) { - // For withdraw all funds, send to the first destination only and check there's only one destination - if (destinations.Count > 1) - { - throw new ArgumentException("Withdraw all funds can only have one destination address."); - } - - var firstDestination = destinations.First(); - if (string.IsNullOrEmpty(firstDestination.Address)) - { - throw new ArgumentException("First destination address is null or empty."); - } - var firstDestinationAddress = BitcoinAddress.Create(firstDestination.Address, nbXplorerNetwork); - builder.SendAll(firstDestinationAddress); + throw new ArgumentException("Destination address is null or empty."); } - else if (walletWithdrawalRequest.Changeless) + var destinationAddress = BitcoinAddress.Create(destination.Address, nbXplorerNetwork); + builder.Send(destinationAddress, changelessAmount); + builder.SubtractFees(); + } + else + { + // Handle multiple destinations for regular transactions + foreach (var dest in destinations) { - // Changeless transactions can only have one destination - if (destinations.Count > 1) - { - throw new ArgumentException("Changeless transactions can only have one destination address."); - } - - var destination = destinations.First(); - if (string.IsNullOrEmpty(destination.Address)) + if (string.IsNullOrEmpty(dest.Address)) { throw new ArgumentException("Destination address is null or empty."); } - var destinationAddress = BitcoinAddress.Create(destination.Address, nbXplorerNetwork); - builder.Send(destinationAddress, changelessAmount); - builder.SubtractFees(); + var destinationAddress = BitcoinAddress.Create(dest.Address, nbXplorerNetwork); + var amount = new Money(dest.Amount, MoneyUnit.BTC); + builder.Send(destinationAddress, amount); } - else - { - // Handle multiple destinations for regular transactions - foreach (var dest in destinations) - { - if (string.IsNullOrEmpty(dest.Address)) - { - throw new ArgumentException("Destination address is null or empty."); - } - var destinationAddress = BitcoinAddress.Create(dest.Address, nbXplorerNetwork); - var amount = new Money(dest.Amount, MoneyUnit.BTC); - builder.Send(destinationAddress, amount); - } - builder.SendAllRemainingToChange(); - } + builder.SendAllRemainingToChange(); + } - originalPSBT = builder.BuildPSBT(false); + originalPSBT = builder.BuildPSBT(false); + + //Additional fields to support PSBT signing with a HW or the Remote Signer + originalPSBT = LightningHelper.AddDerivationData(walletWithdrawalRequest.Wallet, originalPSBT, + selectedUTXOs, scriptCoins, _logger); - //Additional fields to support PSBT signing with a HW or the Remote Signer - originalPSBT = LightningHelper.AddDerivationData(walletWithdrawalRequest.Wallet, originalPSBT, - selectedUTXOs, scriptCoins, _logger); - } - catch (Exception e) - { - _logger.LogError(e, "Error while generating base PSBT"); - } // We "lock" the PSBT to the channel operation request by adding to its UTXOs collection for later checking var utxos = selectedUTXOs.Select(x => _mapper.Map(x)).ToList();