diff --git a/docker/e2e/docker-compose.yml b/docker/e2e/docker-compose.yml index 5081414c..a2be4efb 100644 --- a/docker/e2e/docker-compose.yml +++ b/docker/e2e/docker-compose.yml @@ -111,6 +111,7 @@ services: BITCOIND_RPC_USER: "polaruser" BITCOIND_RPC_PASS: "polarpass" BITCOIND_RPC_WALLET: "default" + NBXPLORER_URI: "http://nbxplorer:32838" E2E_HOT_WALLET_ID: "3" # The HTLC-reconnect test reads persisted ForwardingHtlcEvents straight from Postgres # (there is no gRPC to list them) and restarts a forwarding LND node mid-test. diff --git a/src/Data/Repositories/FUTXORepository.cs b/src/Data/Repositories/FUTXORepository.cs index 2220e56c..4e944ad7 100644 --- a/src/Data/Repositories/FUTXORepository.cs +++ b/src/Data/Repositories/FUTXORepository.cs @@ -126,10 +126,10 @@ public async Task> GetLockedUTXOs(int? ignoredWalletWithdrawalReque walletWithdrawalRequestsLockedUTXOs = await applicationDbContext.WalletWithdrawalRequests .Include(x => x.UTXOs) .Where(x => x.Id != ignoredWalletWithdrawalRequestId - && x.Status == WalletWithdrawalRequestStatus.Pending || + && (x.Status == WalletWithdrawalRequestStatus.Pending || x.Status == WalletWithdrawalRequestStatus.PSBTSignaturesPending || x.Status == WalletWithdrawalRequestStatus.FinalizingPSBT || - x.Status == WalletWithdrawalRequestStatus.OnChainConfirmationPending) + x.Status == WalletWithdrawalRequestStatus.OnChainConfirmationPending)) .SelectMany(x => x.UTXOs).ToListAsync(); } @@ -148,10 +148,10 @@ public async Task> GetLockedUTXOs(int? ignoredWalletWithdrawalReque { channelOperationRequestsLockedUTXOs = await applicationDbContext.ChannelOperationRequests.Include(x => x.Utxos) .Where(x => x.Id != ignoredChannelOperationRequestId - && x.Status == ChannelOperationRequestStatus.Pending || + && (x.Status == ChannelOperationRequestStatus.Pending || x.Status == ChannelOperationRequestStatus.PSBTSignaturesPending || x.Status == ChannelOperationRequestStatus.FinalizingPSBT || - x.Status == ChannelOperationRequestStatus.OnChainConfirmationPending) + x.Status == ChannelOperationRequestStatus.OnChainConfirmationPending)) .SelectMany(x => x.Utxos).ToListAsync(); } diff --git a/src/Helpers/CustomExceptions.cs b/src/Helpers/CustomExceptions.cs index baa4841f..f7378005 100644 --- a/src/Helpers/CustomExceptions.cs +++ b/src/Helpers/CustomExceptions.cs @@ -46,6 +46,13 @@ public class BumpingException : Exception public BumpingException(string? message = null): base(message) {} } +// A ShowToUserException: picking a UTXO someone else already took is a normal thing for a user to +// hit, so it should reach them as a message rather than as an unhandled error. +public class UtxoAlreadyLockedException : ShowToUserException +{ + public UtxoAlreadyLockedException(string? message = null): base(message) {} +} + public class CustomArgumentNullException : ArgumentNullException { public static void ThrowIfNull([NotNull] object? obj, string paramName, string message, params object[] args) diff --git a/src/Pages/ChannelRequests.razor b/src/Pages/ChannelRequests.razor index 7ba98146..6705d5ae 100644 --- a/src/Pages/ChannelRequests.razor +++ b/src/Pages/ChannelRequests.razor @@ -905,7 +905,14 @@ if (_selectedUTXOs.Count > 0) { - await CoinSelectionService.LockUTXOs(_selectedUTXOs, request, BitcoinRequestType.ChannelOperation); + try + { + await CoinSelectionService.LockUTXOs(_selectedUTXOs, request, BitcoinRequestType.ChannelOperation); + } + catch (ShowToUserException e) + { + ToastService.ShowError(e.Message); + } } } else diff --git a/src/Pages/Withdrawals.razor b/src/Pages/Withdrawals.razor index 8ac8c92f..c043c95a 100644 --- a/src/Pages/Withdrawals.razor +++ b/src/Pages/Withdrawals.razor @@ -751,7 +751,14 @@ if (_selectedUTXOs.Count > 0) { - await CoinSelectionService.LockUTXOs(_selectedUTXOs, arg.Item, BitcoinRequestType.WalletWithdrawal); + try + { + await CoinSelectionService.LockUTXOs(_selectedUTXOs, arg.Item, BitcoinRequestType.WalletWithdrawal); + } + catch (ShowToUserException e) + { + ToastService.ShowError(e.Message); + } } _utxoSelectorModalRef.ClearModal(); @@ -1434,7 +1441,11 @@ if (_selectedUTXOs.Count > 0) { - await CoinSelectionService.LockUTXOs(_selectedUTXOs, _selectedRequest, BitcoinRequestType.WalletWithdrawal); + // Null for a plain withdrawal (nothing to exempt); when this save is a fee + // bump, it's the original request's id, so reusing its already-locked UTXO(s) + // here is not treated as a conflict with itself. + await CoinSelectionService.LockUTXOs(_selectedUTXOs, _selectedRequest, BitcoinRequestType.WalletWithdrawal, + previousRequestIdAllowedToShareUtxos: _selectedRequest.BumpingWalletWithdrawalRequestId); } var templatePsbt = await BitcoinService.GenerateTemplatePSBT(_selectedRequest); diff --git a/src/Rpc/NodeGuardService.cs b/src/Rpc/NodeGuardService.cs index fd1ac3a7..f3c0a5f1 100644 --- a/src/Rpc/NodeGuardService.cs +++ b/src/Rpc/NodeGuardService.cs @@ -259,14 +259,14 @@ public override async Task RequestWithdrawal(RequestW outpoints.Add(OutPoint.Parse(outpoint)); } - // Search the utxos and lock them + // Search the utxos (not locked yet - LockUTXOs below is what checks and commits + // the lock atomically, since this is a pure, unprotected read) var derivationStrategyBase = wallet.GetDerivationStrategy(); if (derivationStrategyBase == null) throw new RpcException(new Status(StatusCode.Internal, "Derivation strategy not found")); utxos = await _coinSelectionService.GetUTXOsByOutpointAsync(derivationStrategyBase, outpoints); - } // Create destination objects for the withdrawal request @@ -307,7 +307,7 @@ public override async Task RequestWithdrawal(RequestW if (request.Changeless) { - // Lock the utxos + // Checks the utxos aren't already locked/frozen and locks them, atomically per wallet await _coinSelectionService.LockUTXOs(utxos, withdrawalRequest, BitcoinRequestType.WalletWithdrawal); } @@ -315,12 +315,6 @@ await _coinSelectionService.LockUTXOs(utxos, withdrawalRequest, // Update to refresh from db withdrawalRequest = await _walletWithdrawalRequestRepository.GetById(withdrawalRequest.Id); - if (!withdrawalSaved.Item1) - { - _logger.LogError("Error saving withdrawal request for wallet with id {walletId}", request.WalletId); - throw new RpcException(new Status(StatusCode.Internal, "Error saving withdrawal request for wallet")); - } - // Template PSBT generation with SIGHASH_ALL var psbt = await _bitcoinService.GenerateTemplatePSBT(withdrawalRequest ?? throw new ArgumentException(nameof(withdrawalRequest))); @@ -354,6 +348,12 @@ await _coinSelectionService.LockUTXOs(utxos, withdrawalRequest, _logger.LogError(e.Message); throw new RpcException(new Status(StatusCode.ResourceExhausted, e.Message)); } + catch (UtxoAlreadyLockedException e) + { + CancelWithdrawalRequest(withdrawalRequest); + _logger.LogError(e.Message); + throw new RpcException(new Status(StatusCode.FailedPrecondition, e.Message)); + } catch (RpcException e) { CancelWithdrawalRequest(withdrawalRequest); @@ -382,6 +382,20 @@ private void CancelWithdrawalRequest(WalletWithdrawalRequest? withdrawalRequest) } } + private void CancelChannelOperationRequest(ChannelOperationRequest? channelOperationRequest) + { + if (channelOperationRequest != null) + { + channelOperationRequest.Status = ChannelOperationRequestStatus.Failed; + var (success, error) = _channelOperationRequestRepository.Update(channelOperationRequest); + if (!success) + { + _logger?.LogError(error, "Error updating status of channel operation request {RequestId} for wallet {WalletId}", + channelOperationRequest.Id, channelOperationRequest.WalletId); + } + } + } + public override async Task GetAvailableWallets(GetAvailableWalletsRequest request, ServerCallContext context) { @@ -561,6 +575,7 @@ public override async Task OpenChannel(OpenChannelRequest r } int requestId; + ChannelOperationRequest? channelOperationRequest = null; try { @@ -574,7 +589,8 @@ public override async Task OpenChannel(OpenChannelRequest r outpoints.Add(OutPoint.Parse(outpoint)); } - // Search the utxos and lock them + // Search the utxos (not locked yet - LockUTXOs below is what checks and commits + // the lock atomically, since this is a pure, unprotected read) var derivationStrategy = wallet.GetDerivationStrategy(); if (derivationStrategy == null) { @@ -601,7 +617,7 @@ public override async Task OpenChannel(OpenChannelRequest r throw new RpcException(new Status(StatusCode.NotFound, "Custom fee rate is required")); } - var channelOperationRequest = new ChannelOperationRequest + channelOperationRequest = new ChannelOperationRequest { SatsAmount = request.SatsAmount, Description = $"Channel open from {sourceNode.PubKey} to {destNode.PubKey} (API)", @@ -629,7 +645,7 @@ public override async Task OpenChannel(OpenChannelRequest r if (request.Changeless) { - // Lock the utxos + // Checks the utxos aren't already locked/frozen and locks them, atomically per wallet await _coinSelectionService.LockUTXOs(utxos, channelOperationRequest, BitcoinRequestType.ChannelOperation); } @@ -673,8 +689,15 @@ await _coinSelectionService.LockUTXOs(utxos, channelOperationRequest, requestId = channelOperationRequest.Id; } + catch (UtxoAlreadyLockedException e) + { + CancelChannelOperationRequest(channelOperationRequest); + _logger?.LogError(e.Message); + throw new RpcException(new Status(StatusCode.FailedPrecondition, e.Message)); + } catch (Exception e) { + CancelChannelOperationRequest(channelOperationRequest); _logger?.LogError(e, "Error opening channel through gRPC"); throw new RpcException(new Status(StatusCode.Internal, e.Message)); } diff --git a/src/Services/BitcoinService.cs b/src/Services/BitcoinService.cs index 7c634d4a..90c9753c 100644 --- a/src/Services/BitcoinService.cs +++ b/src/Services/BitcoinService.cs @@ -183,20 +183,8 @@ await _coinSelectionService.GetLockedUTXOsForRequest(walletWithdrawalRequest, } } - // Edge case: If you bumped a multisig transaction and a block was mined in between, the utxo is now unlocked, so we need to fail here - // So a new withdrawal isn't performed with a new utxo - if (previouslyLockedUTXOs.Count == 0 && walletWithdrawalRequest.BumpingWalletWithdrawalRequestId != null) - { - throw new ShowToUserException($"Cannot generate a template PSBT for an already confirmed bumped transaction. The UTXO for request {walletWithdrawalRequest.BumpingWalletWithdrawalRequestId} is already confirmed"); - } - - - var availableUTXOs = previouslyLockedUTXOs.Count > 0 - ? previouslyLockedUTXOs - : await _coinSelectionService.GetAvailableUTXOsAsync(derivationStrategy); - var (scriptCoins, selectedUTXOs) = - await _coinSelectionService.GetTxInputCoins(availableUTXOs, walletWithdrawalRequest, - derivationStrategy); + var (scriptCoins, selectedUTXOs) = await _coinSelectionService.SelectAndLockUTXOsAsync( + walletWithdrawalRequest, BitcoinRequestType.WalletWithdrawal, derivationStrategy); if (scriptCoins == null || !scriptCoins.Any()) { @@ -310,18 +298,6 @@ await _coinSelectionService.GetTxInputCoins(availableUTXOs, walletWithdrawalRequ selectedUTXOs, scriptCoins, _logger); - // 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(); - - var addUTXOSOperation = await _walletWithdrawalRequestRepository.AddUTXOs(walletWithdrawalRequest, utxos); - if (!addUTXOSOperation.Item1) - { - var message = - $"Could not add the following utxos({utxos.Humanize()}) to op request:{walletWithdrawalRequest.Id}"; - _logger.LogError(message); - throw new Exception(message); - } - if (originalPSBT == null) { throw new Exception("Error while generating base PSBT"); diff --git a/src/Services/CoinSelectionService.cs b/src/Services/CoinSelectionService.cs index 927065b0..1e28ce6f 100644 --- a/src/Services/CoinSelectionService.cs +++ b/src/Services/CoinSelectionService.cs @@ -55,11 +55,42 @@ public interface ICoinSelectionService public Task> GetUTXOsByOutpointAsync(DerivationStrategyBase derivationStrategy, List outPoints); /// - /// Locks the UTXOs for using in a specific transaction + /// Locks the UTXOs for using in a specific transaction. Serializes with other selections and + /// throws if any of the given UTXOs + /// is already locked by a different active request or frozen - this is the only guard for + /// explicitly/manually selected UTXOs, since (unlike automatic coin selection) nothing else + /// filters them beforehand. /// /// - /// - public Task LockUTXOs(List selectedUTXOs, IBitcoinRequest bitcoinRequest, BitcoinRequestType requestType); + /// + /// + /// + /// If set, a UTXO already locked by this specific request id (of the same ) + /// is not treated as a conflict. Used for fee-bump (RBF): a bump intentionally reuses the exact + /// UTXO(s) of the request it replaces, so that specific, known-related lock must be allowed + /// while a lock from any other, unrelated request must still be rejected. + /// + /// + /// Use this when the caller already knows which UTXOs it wants (explicit/manual selection); use + /// to have them selected too. Both hold the selection mutex + /// for their whole duration, so neither may be called from inside the other. + /// + public Task LockUTXOs(List selectedUTXOs, IBitcoinRequest bitcoinRequest, BitcoinRequestType requestType, + int? previousRequestIdAllowedToShareUtxos = null); + + /// + /// Atomically picks the UTXOs to fund and locks them to it: reads the + /// wallet's available UTXOs, selects enough to cover the amount and records them as locked to it, + /// all under the selection mutex. That makes read -> select -> claim indivisible, so two concurrent + /// requests can never select the same UTXO (which would make one transaction double-spend/RBF-replace + /// the other). The mutex is released before returning, so the caller builds its (comparatively slow) + /// PSBT without holding it. + /// If the request already has UTXOs locked to it (a retry/resume), those are reused instead of + /// selecting and locking a second, different set. + /// Returns empty collections when the wallet has no UTXOs that can fund the request. + /// + public Task<(List coins, List selectedUTXOs)> SelectAndLockUTXOsAsync( + IBitcoinRequest request, BitcoinRequestType requestType, DerivationStrategyBase derivationStrategy); /// /// Gets the locked UTXOs from a request @@ -81,6 +112,17 @@ public interface ICoinSelectionService public class CoinSelectionService: ICoinSelectionService { + // Guards UTXO selection so only one request at a time can go from "read what is available" to + // "claim what it picked". A selection is a handful of short DB/NBXplorer reads and these are + // human-initiated operations, so one mutex for all wallets is plenty and avoids the bookkeeping a + // per-wallet mutex table would need. + // Not reentrant: LockUTXOs and SelectAndLockUTXOsAsync each hold it for their whole duration, so + // neither may be called from inside the other. + private static readonly SemaphoreSlim SelectionMutex = new(1, 1); + + // Bounds the wait so a mutex bug surfaces as a failed request instead of an indefinite hang. + private static readonly TimeSpan SelectionMutexTimeout = TimeSpan.FromMinutes(2); + private readonly ILogger _logger; private readonly IMapper _mapper; private readonly IFMUTXORepository _fmutxoRepository; @@ -118,8 +160,104 @@ private IBitcoinRequestRepository GetRepository(BitcoinRequestType requestType) }; } - public async Task LockUTXOs(List selectedUTXOs, IBitcoinRequest bitcoinRequest, BitcoinRequestType requestType) + /// + /// Runs with the selection mutex held. + /// + private static async Task WithSelectionMutexAsync(Func> body) + { + if (!await SelectionMutex.WaitAsync(SelectionMutexTimeout)) + { + throw new TimeoutException( + $"Timed out after {SelectionMutexTimeout.TotalSeconds}s waiting for the UTXO selection mutex"); + } + + try + { + return await body(); + } + finally + { + SelectionMutex.Release(); + } + } + + public Task LockUTXOs(List selectedUTXOs, IBitcoinRequest bitcoinRequest, BitcoinRequestType requestType, + int? previousRequestIdAllowedToShareUtxos = null) + { + return WithSelectionMutexAsync(() => + ClaimUTXOsForRequestAsync(selectedUTXOs, bitcoinRequest, requestType, + previousRequestIdAllowedToShareUtxos)); + } + + public Task<(List coins, List selectedUTXOs)> SelectAndLockUTXOsAsync( + IBitcoinRequest request, BitcoinRequestType requestType, DerivationStrategyBase derivationStrategy) { + return WithSelectionMutexAsync(async () => + { + // A request that already owns UTXOs is being retried/resumed, so reuse exactly those + // instead of selecting (and locking) a second, different set. + var previouslyLockedUTXOs = await GetLockedUTXOsForRequest(request, requestType); + + // A fee bump must spend the same input as the transaction it replaces. Owning no UTXOs at + // this point means that input was already confirmed and released, so selecting below would + // silently bump onto a different one. Checked here, with the read it depends on, so a + // confirmation landing mid-selection cannot slip past it. + if (previouslyLockedUTXOs.Count == 0 + && request is WalletWithdrawalRequest { BumpingWalletWithdrawalRequestId: not null } bump) + { + throw new ShowToUserException( + $"Cannot generate a template PSBT for an already confirmed bumped transaction. The UTXO for request {bump.BumpingWalletWithdrawalRequestId} is already confirmed"); + } + + var availableUTXOs = previouslyLockedUTXOs.Count > 0 + ? previouslyLockedUTXOs + : await GetAvailableUTXOsAsync(derivationStrategy); + + var (coins, selectedUTXOs) = await GetTxInputCoins(availableUTXOs, request, derivationStrategy); + + if (coins.Count > 0 && previouslyLockedUTXOs.Count == 0 + && !await ClaimUTXOsForRequestAsync(selectedUTXOs, request, requestType, null)) + { + throw new InvalidOperationException( + $"Could not lock the selected UTXOs to {requestType} request {request.Id}"); + } + + return (coins, selectedUTXOs); + }); + } + + /// + /// Verifies none of is already locked by another active request + /// or frozen, then records them as locked to this one. Must be called with the selection mutex + /// held, which is what makes that check-then-record atomic. Returns false if the record failed. + /// + private async Task ClaimUTXOsForRequestAsync(List selectedUTXOs, IBitcoinRequest bitcoinRequest, + BitcoinRequestType requestType, int? previousRequestIdAllowedToShareUtxos) + { + // Explicitly/manually selected UTXOs never went through the automatic-selection + // filtering (GetAvailableUTXOsAsync), so this is the only place that checks they + // aren't already locked by someone else before committing to them - a UTXO already + // locked by the specific request being bumped is allowed through, since fee-bumping + // intentionally reuses the same input(s) as the request it replaces. + var ignoredWalletWithdrawalRequestId = requestType == BitcoinRequestType.WalletWithdrawal + ? previousRequestIdAllowedToShareUtxos : null; + var ignoredChannelOperationRequestId = requestType == BitcoinRequestType.ChannelOperation + ? previousRequestIdAllowedToShareUtxos : null; + + var lockedUtxos = await _fmutxoRepository.GetLockedUTXOs(ignoredWalletWithdrawalRequestId, ignoredChannelOperationRequestId); + var lockedOutpoints = lockedUtxos.Select(u => $"{u.TxId}-{u.OutputIndex}").ToHashSet(); + var frozenOutpoints = await GetFrozenUTXOs(); + + var conflictingOutpoints = selectedUTXOs + .Select(u => u.Outpoint.ToString()) + .Where(outpoint => lockedOutpoints.Contains(outpoint) || frozenOutpoints.Contains(outpoint)) + .ToList(); + if (conflictingOutpoints.Any()) + { + throw new UtxoAlreadyLockedException( + $"UTXO(s) already locked by another request or frozen: {string.Join(", ", conflictingOutpoints)}"); + } + // 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(); @@ -128,7 +266,10 @@ public async Task LockUTXOs(List selectedUTXOs, IBitcoinRequest bitcoinReq { _logger.LogError( $"Could not add the following utxos({utxos.Humanize()}) to op request:{bitcoinRequest.Id}"); + return false; } + + return true; } public async Task> GetLockedUTXOsForRequest(IBitcoinRequest bitcoinRequest, BitcoinRequestType requestType) diff --git a/src/Services/LightningService.cs b/src/Services/LightningService.cs index 0bc04f82..670a6de0 100644 --- a/src/Services/LightningService.cs +++ b/src/Services/LightningService.cs @@ -1193,15 +1193,13 @@ public void CancelPendingChannel(Node source, byte[] pendingChannelId, Lightning } } - var previouslyLockedUTXOs = - await _coinSelectionService.GetLockedUTXOsForRequest(channelOperationRequest, - BitcoinRequestType.ChannelOperation); - var availableUTXOs = previouslyLockedUTXOs.Count > 0 - ? previouslyLockedUTXOs - : await _coinSelectionService.GetAvailableUTXOsAsync(derivationStrategy); + // Selects the inputs and locks them to this request as one indivisible step, so a + // concurrent PSBT generation for the same wallet cannot pick the same UTXOs (which would + // let both transactions conflict/RBF each other on broadcast). The per-wallet lock is + // released before we build the PSBT below, so that slower work never holds it. var (multisigCoins, selectedUtxOs) = - await _coinSelectionService.GetTxInputCoins(availableUTXOs, channelOperationRequest, - derivationStrategy); + await _coinSelectionService.SelectAndLockUTXOsAsync(channelOperationRequest, + BitcoinRequestType.ChannelOperation, derivationStrategy); if (multisigCoins == null || !multisigCoins.Any()) { @@ -1279,12 +1277,6 @@ await _coinSelectionService.GetTxInputCoins(availableUTXOs, channelOperationRequ _logger.LogError(e, "Error while generating base PSBT"); } - if (previouslyLockedUTXOs.Count == 0) - { - await _coinSelectionService.LockUTXOs(selectedUtxOs, channelOperationRequest, - BitcoinRequestType.ChannelOperation); - } - // The template PSBT is saved for later reuse if (result.Item1 != null) { diff --git a/src/Shared/BumpfeeModal.razor b/src/Shared/BumpfeeModal.razor index d0959487..c2497e7a 100644 --- a/src/Shared/BumpfeeModal.razor +++ b/src/Shared/BumpfeeModal.razor @@ -167,7 +167,8 @@ var outpoints = mUTXOs.Select(u => OutPoint.Parse($"{u.TxId}:{u.OutputIndex}")).ToList(); var UTXOs = await CoinSelectionService.GetUTXOsByOutpointAsync(WithdrawalRequest.Wallet.GetDerivationStrategy()!, outpoints); - await CoinSelectionService.LockUTXOs(UTXOs, newRequest, BitcoinRequestType.WalletWithdrawal); + await CoinSelectionService.LockUTXOs(UTXOs, newRequest, BitcoinRequestType.WalletWithdrawal, + previousRequestIdAllowedToShareUtxos: WithdrawalRequest.Id); var updateResult = WalletWithdrawalRequestRepository.Update(newRequest); if (!updateResult.Item1) diff --git a/test/NodeGuard.Tests/Data/Repositories/FUTXORepositoryTests.cs b/test/NodeGuard.Tests/Data/Repositories/FUTXORepositoryTests.cs index 844b2a05..2a69b9ce 100644 --- a/test/NodeGuard.Tests/Data/Repositories/FUTXORepositoryTests.cs +++ b/test/NodeGuard.Tests/Data/Repositories/FUTXORepositoryTests.cs @@ -125,6 +125,91 @@ public async Task GetLockedUTXOs_ignoreChannels() result[0].Id.Should().Be(1); } + [Theory] + [InlineData(WalletWithdrawalRequestStatus.Pending)] + [InlineData(WalletWithdrawalRequestStatus.PSBTSignaturesPending)] + [InlineData(WalletWithdrawalRequestStatus.FinalizingPSBT)] + [InlineData(WalletWithdrawalRequestStatus.OnChainConfirmationPending)] + public async Task GetLockedUTXOs_ignoreWithdrawals_StillIgnoredForEveryActiveStatus(WalletWithdrawalRequestStatus status) + { + // Regression test: the ignoredWalletWithdrawalRequestId branch must exclude the ignored + // request regardless of its (active) status, not just when it happens to be Pending (an + // operator precedence bug used to make the exclusion apply only to the Pending case). + var dbContextFactory = SetupDbContextFactory(); + var futxoRepository = new FUTXORepository(null!, null!, dbContextFactory.Object); + + var context = dbContextFactory.Object.CreateDbContext(); + + context.WalletWithdrawalRequests.Add(new WalletWithdrawalRequest + { + Id = 1, + Description = "1", + Status = status, + WalletWithdrawalRequestDestinations = new List + { + new WalletWithdrawalRequestDestination + { + Address = "1", + Amount = 0.01m + } + }, + UTXOs = new List { new () { TxId = "1"} } + }); + context.ChannelOperationRequests.Add(new ChannelOperationRequest + { + Id = 2, + Status = ChannelOperationRequestStatus.Pending, + Utxos = new List { new () { TxId = "2"} } + }); + await context.SaveChangesAsync(); + + var result = await futxoRepository.GetLockedUTXOs(1); + result.Should().HaveCount(1); + result[0].Id.Should().Be(2); + } + + [Theory] + [InlineData(ChannelOperationRequestStatus.Pending)] + [InlineData(ChannelOperationRequestStatus.PSBTSignaturesPending)] + [InlineData(ChannelOperationRequestStatus.FinalizingPSBT)] + [InlineData(ChannelOperationRequestStatus.OnChainConfirmationPending)] + public async Task GetLockedUTXOs_ignoreChannels_StillIgnoredForEveryActiveStatus(ChannelOperationRequestStatus status) + { + // Regression test: same operator precedence bug as above, for the + // ignoredChannelOperationRequestId branch. + var dbContextFactory = SetupDbContextFactory(); + var futxoRepository = new FUTXORepository(null!, null!, dbContextFactory.Object); + + var context = dbContextFactory.Object.CreateDbContext(); + + context.WalletWithdrawalRequests.Add(new WalletWithdrawalRequest + { + Id = 1, + Description = "1", + Status = WalletWithdrawalRequestStatus.Pending, + WalletWithdrawalRequestDestinations = new List + { + new WalletWithdrawalRequestDestination + { + Address = "1", + Amount = 0.01m + } + }, + UTXOs = new List { new () { TxId = "1"} } + }); + context.ChannelOperationRequests.Add(new ChannelOperationRequest + { + Id = 2, + Status = status, + Utxos = new List { new () { TxId = "2"} } + }); + await context.SaveChangesAsync(); + + var result = await futxoRepository.GetLockedUTXOs(null, 2); + result.Should().HaveCount(1); + result[0].Id.Should().Be(1); + } + [Fact] public async Task GetLockedUTXOs_failedChannels() { diff --git a/test/NodeGuard.Tests/E2E/ConcurrentWithdrawalE2ETests.cs b/test/NodeGuard.Tests/E2E/ConcurrentWithdrawalE2ETests.cs new file mode 100644 index 00000000..808b7e16 --- /dev/null +++ b/test/NodeGuard.Tests/E2E/ConcurrentWithdrawalE2ETests.cs @@ -0,0 +1,377 @@ +/* + * NodeGuard + * Copyright (C) 2023 Elenpay + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see http://www.gnu.org/licenses/. + * + */ + +using System.Net; +using System.Net.Http.Json; +using FluentAssertions; +using Grpc.Core; +using Grpc.Net.Client; +using NBitcoin; +using NBitcoin.RPC; +using Nodeguard; +using Xunit.Abstractions; + +namespace NodeGuard.Tests.E2E; + +/// +/// Coin selection must be safe for concurrent withdrawals: multiple withdrawal requests from the +/// SAME wallet to DIFFERENT destinations, submitted at the same time, must each end up with their +/// own disjoint set of UTXOs. If two of them selected the same UTXO, one transaction would +/// double-spend/RBF-replace the other on the network. This covers both how a UTXO can end up +/// selected: automatically (coin selection picks it) and explicitly (a caller names its outpoint). +/// The automatic-selection test funds the wallet with one distinct confirmed UTXO per withdrawal, +/// fires all the withdrawal requests concurrently, and decodes each resulting transaction's inputs +/// straight from the mempool (no mining/confirmation needed, since NodeGuard signs and broadcasts +/// hot-wallet withdrawals synchronously) to assert no input is ever picked by two of them. The +/// explicit-selection test funds a single UTXO and fires two concurrent requests that both name +/// its exact outpoint, asserting exactly one wins and the other is rejected. +/// Exercised against a LIVE NodeGuard instance + bitcoind. +/// Gated by (RUN_E2E_TESTS=1). Connection via env: +/// NODEGUARD_GRPC_ENDPOINT default http://localhost:50051 (h2c) +/// NODEGUARD_API_TOKEN default the dev "Liquidator" token +/// BITCOIND_RPC_URL/USER/PASS/WALLET default http://localhost:18443 / polaruser / polarpass / default +/// NBXPLORER_URI default http://localhost:32838, used to poll NBXplorer's own sync status +/// E2E_HOT_WALLET_ID NodeGuard hot wallet to withdraw from (default 3, shared with the +/// other E2E tests in this collection). This test sweeps its own change +/// outputs to an external address once it's done, so it doesn't leave +/// behind UTXOs that could affect other tests' assertions. +/// +[Trait("Category", "E2E")] +[Collection("E2E")] +public class ConcurrentWithdrawalE2ETests +{ + private const string DefaultDevToken = "8rvSsUGeyXXdDQrHctcTey/xtHdZQEn945KHwccKp9Q="; + private const int ConcurrentWithdrawals = 10; + private const long AmountPerWithdrawalSats = 1_000_000; + + private readonly ITestOutputHelper _output; + + public ConcurrentWithdrawalE2ETests(ITestOutputHelper output) + { + _output = output; + AppContext.SetSwitch("System.Net.Http.SocketsHttpHandler.Http2UnencryptedSupport", true); + } + + [E2EFact] + public async Task ManyConcurrentWithdrawals_FromSameWallet_NeverSelectTheSameUtxo() + { + var client = CreateClient(out var headers); + var rpc = CreateBitcoindRpc(); + var walletId = int.Parse(Env("E2E_HOT_WALLET_ID", "3")); + + await RetryAsync(async () => + { + var resp = await client.GetNodesAsync(new GetNodesRequest(), headers); + if (resp.Nodes.Count == 0) throw new InvalidOperationException("no nodes seeded yet"); + return true; + }, attempts: 90, delay: TimeSpan.FromSeconds(4), what: "GetNodes (NodeGuard readiness)"); + + // Give the wallet one distinct confirmed UTXO per withdrawal: coin selection picks the + // oldest UTXO(s) first, so with only a single large UTXO every concurrent request would + // just contend for that one input (and correctly fail for all but one), rather than each + // picking its own input as they would with a realistic, multi-UTXO wallet. + for (var i = 0; i < ConcurrentWithdrawals; i++) + { + var fundingAddressResponse = await client.GetNewWalletAddressAsync( + new GetNewWalletAddressRequest { WalletId = walletId, Skip = 0, Reserve = true }, headers); + await rpc.SendToAddressAsync( + BitcoinAddress.Create(fundingAddressResponse.Address, Network.RegTest), + Money.Satoshis(AmountPerWithdrawalSats * 3)); + } + await MineAsync(rpc, 6); + + await RetryAsync(async () => + { + var available = await client.GetAvailableUtxosAsync( + new GetAvailableUtxosRequest { WalletId = walletId, Amount = AmountPerWithdrawalSats * ConcurrentWithdrawals }, + headers); + if (available.Confirmed.Count < ConcurrentWithdrawals) + throw new InvalidOperationException( + $"expected {ConcurrentWithdrawals} distinct confirmed UTXOs, got {available.Confirmed.Count}"); + return true; + }, attempts: 60, delay: TimeSpan.FromSeconds(4), what: "GetAvailableUtxos (hot wallet funded with distinct UTXOs)"); + + await WaitForNbxplorerFullySynchedAsync(); + + // Fire all withdrawal requests concurrently so their coin selections overlap in time. + var requestTasks = new List>(); + var destinations = new List(); + for (var i = 0; i < ConcurrentWithdrawals; i++) + { + var destination = await rpc.GetNewAddressAsync(); + destinations.Add(destination); + requestTasks.Add(client.RequestWithdrawalAsync(new RequestWithdrawalRequest + { + WalletId = walletId, + Description = $"E2E concurrent withdrawal {i}", + Destinations = { new Destination { Address = destination.ToString(), AmountSats = AmountPerWithdrawalSats } }, + MempoolFeeRate = FEES_TYPE.CustomFee, + CustomFeeRate = 2, + }, headers).ResponseAsync); + } + + var withdrawals = await Task.WhenAll(requestTasks); + foreach (var withdrawal in withdrawals) + { + _output.WriteLine($"withdrawal {withdrawal.RequestId} -> txid {withdrawal.Txid}"); + withdrawal.IsHotWallet.Should().BeTrue(); + } + + withdrawals.Select(w => w.Txid).Distinct().Should().HaveCount(ConcurrentWithdrawals, + "every concurrent withdrawal must produce its own transaction, not replace another's"); + + // NodeGuard signs and broadcasts hot-wallet withdrawals synchronously, so the txid is + // already a valid, decodable mempool transaction - no need to mine/confirm anything to + // inspect its inputs. + var withdrawalTxs = await Task.WhenAll(withdrawals.Select(async w => + { + var tx = await RetryAsync(async () => + { + var t = await rpc.GetRawTransactionAsync(uint256.Parse(w.Txid), throwIfNotFound: false); + return t ?? throw new InvalidOperationException($"withdrawal {w.RequestId} tx not broadcast yet"); + }, attempts: 30, delay: TimeSpan.FromSeconds(4), what: $"GetRawTransaction (withdrawal {w.RequestId})"); + return tx; + })); + + // Across ALL concurrent withdrawals, no prevout may ever be spent twice. A shared input + // between any two of them is exactly what would let bitcoind treat one as an RBF + // replacement of the other. + var allInputs = withdrawalTxs.SelectMany(tx => tx.Inputs.Select(input => input.PrevOut)).ToList(); + allInputs.Should().OnlyHaveUniqueItems( + "concurrent withdrawals from the same wallet must select disjoint UTXOs"); + + await SweepChangeOutputsAsync(client, headers, rpc, walletId, withdrawalTxs, destinations); + } + + /// + /// Manual/explicit UTXO selection (Changeless withdrawals) is a completely different code + /// path from the automatic coin selection covered above - it never went through + /// GetAvailableUTXOsAsync's filtering, so CoinSelectionService.LockUTXOs itself has to check + /// an explicitly-named outpoint isn't already locked before committing to it. This test funds a + /// single UTXO and fires two concurrent Changeless withdrawals that both explicitly name that + /// same outpoint: exactly one must succeed, and the other must fail with FailedPrecondition + /// rather than both silently locking the same UTXO to two different requests. + /// + [E2EFact] + public async Task TwoConcurrentWithdrawals_ExplicitlySelectingTheSameUtxo_OnlyOneSucceeds() + { + var client = CreateClient(out var headers); + var rpc = CreateBitcoindRpc(); + var walletId = int.Parse(Env("E2E_HOT_WALLET_ID", "3")); + const long amountSats = 1_000_000; + + await RetryAsync(async () => + { + var resp = await client.GetNodesAsync(new GetNodesRequest(), headers); + if (resp.Nodes.Count == 0) throw new InvalidOperationException("no nodes seeded yet"); + return true; + }, attempts: 90, delay: TimeSpan.FromSeconds(4), what: "GetNodes (NodeGuard readiness)"); + + var fundingAddressResponse = await client.GetNewWalletAddressAsync( + new GetNewWalletAddressRequest { WalletId = walletId, Skip = 0, Reserve = true }, headers); + var fundingTxId = await rpc.SendToAddressAsync( + BitcoinAddress.Create(fundingAddressResponse.Address, Network.RegTest), Money.Satoshis(amountSats)); + await MineAsync(rpc, 6); + + string outpoint = null!; + await RetryAsync(async () => + { + var available = await client.GetAvailableUtxosAsync( + new GetAvailableUtxosRequest { WalletId = walletId, Amount = amountSats }, headers); + var match = available.Confirmed.FirstOrDefault(u => u.Outpoint.StartsWith(fundingTxId.ToString())); + if (match == null) throw new InvalidOperationException("funded UTXO not confirmed/indexed yet"); + outpoint = match.Outpoint; + return true; + }, attempts: 30, delay: TimeSpan.FromSeconds(4), what: "GetAvailableUtxos (dedicated UTXO funded)"); + + await WaitForNbxplorerFullySynchedAsync(); + + var destinationA = await rpc.GetNewAddressAsync(); + var destinationB = await rpc.GetNewAddressAsync(); + + // Both requests explicitly name the exact same outpoint. + var requestA = TryRequestWithdrawalAsync(client, headers, walletId, outpoint, destinationA, amountSats, "A"); + var requestB = TryRequestWithdrawalAsync(client, headers, walletId, outpoint, destinationB, amountSats, "B"); + + var (resultA, resultB) = (await requestA, await requestB); + _output.WriteLine($"request A: {(resultA.Response != null ? $"txid {resultA.Response.Txid}" : resultA.Error!.Status)}"); + _output.WriteLine($"request B: {(resultB.Response != null ? $"txid {resultB.Response.Txid}" : resultB.Error!.Status)}"); + + var successes = new[] { resultA, resultB }.Where(r => r.Response != null).ToList(); + var failures = new[] { resultA, resultB }.Where(r => r.Response == null).ToList(); + + successes.Should().ContainSingle( + "exactly one of the two requests explicitly naming the same outpoint must win the lock"); + failures.Should().ContainSingle(); + failures[0].Error!.StatusCode.Should().Be(StatusCode.FailedPrecondition, + "the losing request must be rejected for the outpoint already being locked, not fail some other way"); + + // The winning withdrawal spends the UTXO changelessly to an external address, so nothing + // is left behind in the shared wallet for other tests to trip over. + await RetryAsync(async () => + { + var tx = await rpc.GetRawTransactionAsync(uint256.Parse(successes[0].Response!.Txid), throwIfNotFound: false); + return tx ?? throw new InvalidOperationException("winning withdrawal tx not broadcast yet"); + }, attempts: 30, delay: TimeSpan.FromSeconds(4), what: "GetRawTransaction (winning withdrawal broadcast)"); + await MineAsync(rpc, 6); + } + + private async Task<(RequestWithdrawalResponse? Response, RpcException? Error)> TryRequestWithdrawalAsync( + NodeGuardService.NodeGuardServiceClient client, Metadata headers, int walletId, string outpoint, + BitcoinAddress destination, long amountSats, string label) + { + try + { + var response = await client.RequestWithdrawalAsync(new RequestWithdrawalRequest + { + WalletId = walletId, + Description = $"E2E explicit-outpoint conflict test {label}", + Changeless = true, + UtxosOutpoints = { outpoint }, + Destinations = { new Destination { Address = destination.ToString(), AmountSats = amountSats } }, + MempoolFeeRate = FEES_TYPE.CustomFee, + CustomFeeRate = 2, + }, headers); + return (response, null); + } + catch (RpcException e) + { + return (null, e); + } + } + + // The wallet used above is shared with other E2E tests in this collection, so the change + // outputs these withdrawals produced need to be swept out to an external address rather than + // left behind - otherwise later tests could see UTXOs of a shape they don't expect. + private async Task SweepChangeOutputsAsync( + NodeGuardService.NodeGuardServiceClient client, Metadata headers, RPCClient rpc, int walletId, + Transaction[] withdrawalTxs, List destinations) + { + var destinationScripts = destinations.Select(d => d.ScriptPubKey).ToHashSet(); + var changeOutpoints = withdrawalTxs + .SelectMany(tx => tx.Outputs.AsIndexedOutputs() + .Where(o => !destinationScripts.Contains(o.TxOut.ScriptPubKey)) + .Select(o => new OutPoint(tx.GetHash(), o.N))) + .ToList(); + changeOutpoints.Should().HaveCount(ConcurrentWithdrawals, "each withdrawal must have produced exactly one change output"); + + await MineAsync(rpc, 6); + await RetryAsync(async () => + { + var available = await client.GetAvailableUtxosAsync( + new GetAvailableUtxosRequest { WalletId = walletId, Amount = changeOutpoints.Count }, headers); + var confirmedOutpoints = available.Confirmed.Select(u => u.Outpoint).ToHashSet(); + if (!changeOutpoints.All(o => confirmedOutpoints.Contains(o.ToString()))) + throw new InvalidOperationException("change outputs not confirmed/indexed yet"); + return true; + }, attempts: 30, delay: TimeSpan.FromSeconds(4), what: "GetAvailableUtxos (change outputs confirmed for sweep)"); + + // Same NBXplorer post-mining sync race as after the initial funding round above. + await WaitForNbxplorerFullySynchedAsync(); + + var sweepAddress = await rpc.GetNewAddressAsync(); + var sweepResponse = await client.RequestWithdrawalAsync(new RequestWithdrawalRequest + { + WalletId = walletId, + Description = "E2E concurrent withdrawal cleanup sweep", + Changeless = true, + UtxosOutpoints = { changeOutpoints.Select(o => o.ToString()) }, + Destinations = { new Destination { Address = sweepAddress.ToString(), AmountSats = 1 } }, + MempoolFeeRate = FEES_TYPE.CustomFee, + CustomFeeRate = 2, + }, headers); + _output.WriteLine($"cleanup sweep -> txid {sweepResponse.Txid}"); + + await RetryAsync(async () => + { + var tx = await rpc.GetRawTransactionAsync(uint256.Parse(sweepResponse.Txid), throwIfNotFound: false); + return tx ?? throw new InvalidOperationException("sweep tx not broadcast yet"); + }, attempts: 30, delay: TimeSpan.FromSeconds(4), what: "GetRawTransaction (cleanup sweep broadcast)"); + await MineAsync(rpc, 6); + } + + // ---- helpers ----------------------------------------------------------------------------- + + private static readonly HttpClient NbxplorerHttp = new(); + + // NBXplorer can still be finishing a background rescan of blocks just mined even though it + // already has enough indexed to satisfy a GetAvailableUtxos check, so a withdrawal request + // fired immediately after mining can race NBXplorer's own "fully synced" flag and fail with + // NBXplorerNotFullySyncedException. Poll NBXplorer's own status endpoint directly - the same + // flag GenerateTemplatePSBT itself checks server-side - rather than inferring it. Call this + // after every MineAsync that precedes a withdrawal request. + private async Task WaitForNbxplorerFullySynchedAsync() + { + await RetryAsync(async () => + { + var isFullySynched = await IsNbxplorerFullySynchedAsync(); + if (!isFullySynched) + throw new InvalidOperationException("NBXplorer is not fully synched yet"); + return true; + }, attempts: 30, delay: TimeSpan.FromSeconds(2), what: "NBXplorer status (fully synched)"); + } + + private async Task IsNbxplorerFullySynchedAsync() + { + var baseUrl = Env("NBXPLORER_URI", "http://localhost:32838"); + var status = await NbxplorerHttp.GetFromJsonAsync($"{baseUrl}/v1/cryptos/btc/status"); + return status?.IsFullySynched ?? false; + } + + private sealed class NbxplorerStatus + { + public bool IsFullySynched { get; set; } + } + + private async Task MineAsync(RPCClient rpc, int blocks) + { + var addr = await rpc.GetNewAddressAsync(); + await rpc.GenerateToAddressAsync(blocks, addr); + } + + private async Task RetryAsync(Func> action, int attempts, TimeSpan delay, string what) + { + Exception? last = null; + for (var i = 0; i < attempts; i++) + { + try { return await action(); } + catch (Exception ex) { last = ex; _output.WriteLine($"{what} attempt {i + 1}/{attempts} failed: {ex.Message}"); } + await Task.Delay(delay); + } + throw new InvalidOperationException($"{what} did not succeed after {attempts} attempts", last); + } + + private static NodeGuardService.NodeGuardServiceClient CreateClient(out Metadata headers) + { + var endpoint = Env("NODEGUARD_GRPC_ENDPOINT", "http://localhost:50051"); + headers = new Metadata { { "auth-token", Env("NODEGUARD_API_TOKEN", DefaultDevToken) } }; + return new NodeGuardService.NodeGuardServiceClient(GrpcChannel.ForAddress(endpoint)); + } + + private static RPCClient CreateBitcoindRpc() + { + var url = Env("BITCOIND_RPC_URL", "http://localhost:18443"); + var cred = new NetworkCredential(Env("BITCOIND_RPC_USER", "polaruser"), Env("BITCOIND_RPC_PASS", "polarpass")); + var rpc = new RPCClient(cred, new Uri(url), Network.RegTest); + return rpc.SetWalletContext(Env("BITCOIND_RPC_WALLET", "default")); + } + + private static string Env(string name, string fallback) + => Environment.GetEnvironmentVariable(name) is { Length: > 0 } v ? v : fallback; +} diff --git a/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs b/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs index 439166b6..54b4aeb7 100644 --- a/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs +++ b/test/NodeGuard.Tests/Services/BitcoinServiceTests.cs @@ -517,7 +517,7 @@ async Task GenerateTemplatePSBT_SingleSigSuccessManuallyUnfrozenUTXO() .Setup(x => x.GetLockedUTXOs(null, null)) .ReturnsAsync(new List()); utxoTagRepository - .SetupSequence(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .Setup(x => x.GetByKeyValue(Constants.IsFrozenTag, "true")) .ReturnsAsync(new List() { new UTXOTag() @@ -526,8 +526,12 @@ async Task GenerateTemplatePSBT_SingleSigSuccessManuallyUnfrozenUTXO() Value = "false", Outpoint = "00000000000000000000000000000000000000000000000000000000000004d2-1" } - }) - .ReturnsAsync(new List()) + }); + utxoTagRepository + .Setup(x => x.GetByKeyValue(Constants.IsManuallyFrozenTag, "true")) + .ReturnsAsync(new List()); + utxoTagRepository + .Setup(x => x.GetByKeyValue(Constants.IsManuallyFrozenTag, "false")) .ReturnsAsync(new List() { new UTXOTag() diff --git a/test/NodeGuard.Tests/Services/CoinSelectionServiceTests.cs b/test/NodeGuard.Tests/Services/CoinSelectionServiceTests.cs index a9e9bae8..7bc740a0 100644 --- a/test/NodeGuard.Tests/Services/CoinSelectionServiceTests.cs +++ b/test/NodeGuard.Tests/Services/CoinSelectionServiceTests.cs @@ -20,6 +20,7 @@ using FluentAssertions; using NodeGuard.Data.Models; using NodeGuard.Data.Repositories.Interfaces; +using NodeGuard.Helpers; using NodeGuard.TestHelpers; using Microsoft.Extensions.Logging; using NBitcoin; @@ -286,4 +287,316 @@ public async Task GetAvailableUTXOsAsync_ExcludesDustAndLockedUTXOs() availableUTXOs.Should().ContainSingle(); availableUTXOs[0].Outpoint.Should().Be(availableUtxo.Outpoint); } + + private CoinSelectionService CreateServiceForLockUTXOs( + out Mock walletWithdrawalRequestRepository, + List? lockedUtxos = null) + { + var fmutxoRepository = new Mock(); + fmutxoRepository.Setup(x => x.GetLockedUTXOs(It.IsAny(), It.IsAny())) + .ReturnsAsync(lockedUtxos ?? new List()); + var utxoTagRepository = new Mock(); + utxoTagRepository.Setup(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + walletWithdrawalRequestRepository = new Mock(); + walletWithdrawalRequestRepository + .Setup(x => x.AddUTXOs(It.IsAny(), It.IsAny>())) + .ReturnsAsync((true, (string?)null)); + var mapper = new Mock(); + mapper.Setup(m => m.Map(It.IsAny())).Returns(new FMUTXO()); + + return new CoinSelectionService(_logger, mapper.Object, fmutxoRepository.Object, + new Mock().Object, new Mock().Object, + walletWithdrawalRequestRepository.Object, utxoTagRepository.Object); + } + + private CoinSelectionService CreateServiceForSelectAndLock( + List confirmedUtxos, + out Mock walletWithdrawalRequestRepository, + List? alreadyLockedToThisRequest = null) + { + var fmutxoRepository = new Mock(); + fmutxoRepository.Setup(x => x.GetLockedUTXOs(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + var utxoTagRepository = new Mock(); + utxoTagRepository.Setup(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + + walletWithdrawalRequestRepository = new Mock(); + walletWithdrawalRequestRepository + .Setup(x => x.GetUTXOs(It.IsAny())) + .ReturnsAsync((true, alreadyLockedToThisRequest ?? new List())); + walletWithdrawalRequestRepository + .Setup(x => x.AddUTXOs(It.IsAny(), It.IsAny>())) + .ReturnsAsync((true, (string?)null)); + + var nbXplorerService = new Mock(); + nbXplorerService + .Setup(x => x.GetUTXOsAsync(It.IsAny(), default)) + .ReturnsAsync(new UTXOChanges { Confirmed = new UTXOChange { UTXOs = confirmedUtxos } }); + + var mapper = new Mock(); + mapper.Setup(m => m.Map(It.IsAny())).Returns(new FMUTXO()); + + return new CoinSelectionService(_logger, mapper.Object, fmutxoRepository.Object, + nbXplorerService.Object, new Mock().Object, + walletWithdrawalRequestRepository.Object, utxoTagRepository.Object); + } + + [Fact] + public async Task SelectAndLockUTXOsAsync_ReleasesTheWalletLockBeforeReturning() + { + // The point of doing select+lock as one owned step is that the per-wallet lock is held only + // for that, and is already released by the time the caller goes on to build its PSBT. If it + // were still held, the caller's next call for the same wallet (or its own nested locking) + // would block forever - which is exactly the deadlock this design removes. + var wallet = CreateWallet.SingleSig(_internalWallet); + var service = CreateServiceForSelectAndLock(new List { CreateUtxo(1, 100_000) }, out _); + var request = new WalletWithdrawalRequest + { + Id = 99, WalletId = wallet.Id, Wallet = wallet, + WalletWithdrawalRequestDestinations = new List + { + new() { Address = "1", Amount = 0.0001m } + } + }; + + await service.SelectAndLockUTXOsAsync(request, BitcoinRequestType.WalletWithdrawal, + wallet.GetDerivationStrategy()); + + // A second call for the same wallet can only complete promptly if the first one released. + var again = service.SelectAndLockUTXOsAsync(request, BitcoinRequestType.WalletWithdrawal, + wallet.GetDerivationStrategy()); + var winner = await Task.WhenAny(again, Task.Delay(TimeSpan.FromSeconds(5))); + winner.Should().Be(again, + "the wallet lock must be released before returning, so the PSBT build never holds it"); + await again; + } + + [Fact] + public async Task SelectAndLockUTXOsAsync_ForABumpWhoseUTXOIsAlreadyConfirmed_Throws() + { + // A fee bump has to spend the same input as the transaction it replaces. Owning no UTXOs means + // that input already confirmed and was released, so selecting would silently bump onto a + // different one - refuse instead. Enforced here, next to the read it depends on, so a + // confirmation landing mid-selection cannot slip past it. + var wallet = CreateWallet.SingleSig(_internalWallet); + var service = CreateServiceForSelectAndLock(new List { CreateUtxo(1, 100_000) }, + out var walletWithdrawalRequestRepository); + var bumpRequest = new WalletWithdrawalRequest + { + Id = 100, + WalletId = wallet.Id, + Wallet = wallet, + BumpingWalletWithdrawalRequestId = 42, + WalletWithdrawalRequestDestinations = new List + { + new() { Address = "1", Amount = 0.0001m } + } + }; + + var act = () => service.SelectAndLockUTXOsAsync(bumpRequest, BitcoinRequestType.WalletWithdrawal, + wallet.GetDerivationStrategy()); + + await act.Should().ThrowAsync(); + walletWithdrawalRequestRepository.Verify( + x => x.AddUTXOs(It.IsAny(), It.IsAny>()), Times.Never); + } + + [Fact] + public async Task SelectAndLockUTXOsAsync_WhenRequestAlreadyOwnsUTXOs_DoesNotLockASecondSet() + { + // A retried/resumed request must reuse the UTXOs already locked to it rather than selecting + // and locking a second, different set. + var wallet = CreateWallet.SingleSig(_internalWallet); + var utxo = CreateUtxo(1, 100_000); + var alreadyLocked = new List + { + new() { TxId = utxo.Outpoint.Hash.ToString(), OutputIndex = utxo.Outpoint.N, SatsAmount = 100_000 } + }; + var service = CreateServiceForSelectAndLock(new List { utxo }, + out var walletWithdrawalRequestRepository, alreadyLocked); + var request = new WalletWithdrawalRequest + { + Id = 99, WalletId = wallet.Id, Wallet = wallet, + WalletWithdrawalRequestDestinations = new List + { + new() { Address = "1", Amount = 0.0001m } + } + }; + + await service.SelectAndLockUTXOsAsync(request, BitcoinRequestType.WalletWithdrawal, + wallet.GetDerivationStrategy()); + + walletWithdrawalRequestRepository.Verify( + x => x.AddUTXOs(It.IsAny(), It.IsAny>()), Times.Never); + } + + [Fact] + public async Task ConcurrentSelectAndLockForTheSameWallet_NeverOverlap() + { + // The whole point of the lock: read-available -> select -> lock must be indivisible, so two + // concurrent requests for the same wallet can never both be inside it (and therefore can + // never both select the same UTXO). Observed by counting how many callers are inside the + // critical section at once, via a deliberately slow repository read. + var wallet = CreateWallet.SingleSig(_internalWallet); + var inFlight = 0; + var maxObservedInFlight = 0; + + var fmutxoRepository = new Mock(); + fmutxoRepository.Setup(x => x.GetLockedUTXOs(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + var utxoTagRepository = new Mock(); + utxoTagRepository.Setup(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + + var walletWithdrawalRequestRepository = new Mock(); + walletWithdrawalRequestRepository + .Setup(x => x.GetUTXOs(It.IsAny())) + .Returns(async () => + { + maxObservedInFlight = Math.Max(maxObservedInFlight, Interlocked.Increment(ref inFlight)); + await Task.Delay(200); + Interlocked.Decrement(ref inFlight); + return (true, new List()); + }); + walletWithdrawalRequestRepository + .Setup(x => x.AddUTXOs(It.IsAny(), It.IsAny>())) + .ReturnsAsync((true, (string?)null)); + + var nbXplorerService = new Mock(); + nbXplorerService + .Setup(x => x.GetUTXOsAsync(It.IsAny(), default)) + .ReturnsAsync(new UTXOChanges + { + Confirmed = new UTXOChange { UTXOs = new List { CreateUtxo(1, 100_000) } } + }); + + var mapper = new Mock(); + mapper.Setup(m => m.Map(It.IsAny())).Returns(new FMUTXO()); + + var service = new CoinSelectionService(_logger, mapper.Object, fmutxoRepository.Object, + nbXplorerService.Object, new Mock().Object, + walletWithdrawalRequestRepository.Object, utxoTagRepository.Object); + + WalletWithdrawalRequest NewRequest(int id) => new() + { + Id = id, WalletId = wallet.Id, Wallet = wallet, + WalletWithdrawalRequestDestinations = new List + { + new() { Address = "1", Amount = 0.0001m } + } + }; + + await Task.WhenAll( + service.SelectAndLockUTXOsAsync(NewRequest(1), BitcoinRequestType.WalletWithdrawal, wallet.GetDerivationStrategy()), + service.SelectAndLockUTXOsAsync(NewRequest(2), BitcoinRequestType.WalletWithdrawal, wallet.GetDerivationStrategy())); + + maxObservedInFlight.Should().Be(1, + "the per-wallet lock must keep the select+lock sequences from overlapping"); + } + + [Fact] + public async Task LockUTXOs_ConflictingOutpoint_ThrowsUtxoAlreadyLockedException() + { + // Arrange + var utxo = CreateUtxo(1, 10_000); + var fmutxoRepository = new Mock(); + fmutxoRepository + .Setup(x => x.GetLockedUTXOs(null, null)) + .ReturnsAsync(new List + { + new() { TxId = utxo.Outpoint.Hash.ToString(), OutputIndex = utxo.Outpoint.N } + }); + var utxoTagRepository = new Mock(); + utxoTagRepository.Setup(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + var walletWithdrawalRequestRepository = new Mock(); + var mapper = new Mock(); + + var coinSelectionService = new CoinSelectionService(_logger, mapper.Object, fmutxoRepository.Object, + new Mock().Object, new Mock().Object, + walletWithdrawalRequestRepository.Object, utxoTagRepository.Object); + + var withdrawalRequest = new WalletWithdrawalRequest { Id = 99, Wallet = new Wallet { Id = 1 } }; + + // Act + var act = () => coinSelectionService.LockUTXOs(new List { utxo }, withdrawalRequest, + BitcoinRequestType.WalletWithdrawal); + + // Assert + await act.Should().ThrowAsync(); + walletWithdrawalRequestRepository.Verify( + x => x.AddUTXOs(It.IsAny(), It.IsAny>()), Times.Never); + } + + [Fact] + public async Task LockUTXOs_NoConflict_LocksTheUtxo() + { + // Arrange + var utxo = CreateUtxo(1, 10_000); + var fmutxoRepository = new Mock(); + fmutxoRepository + .Setup(x => x.GetLockedUTXOs(null, null)) + .ReturnsAsync(new List()); + var utxoTagRepository = new Mock(); + utxoTagRepository.Setup(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + var walletWithdrawalRequestRepository = new Mock(); + walletWithdrawalRequestRepository + .Setup(x => x.AddUTXOs(It.IsAny(), It.IsAny>())) + .ReturnsAsync((true, (string?)null)); + var mapper = new Mock(); + mapper.Setup(m => m.Map(It.IsAny())).Returns(new FMUTXO()); + + var coinSelectionService = new CoinSelectionService(_logger, mapper.Object, fmutxoRepository.Object, + new Mock().Object, new Mock().Object, + walletWithdrawalRequestRepository.Object, utxoTagRepository.Object); + + var withdrawalRequest = new WalletWithdrawalRequest { Id = 99, Wallet = new Wallet { Id = 1 } }; + + // Act + await coinSelectionService.LockUTXOs(new List { utxo }, withdrawalRequest, + BitcoinRequestType.WalletWithdrawal); + + // Assert + walletWithdrawalRequestRepository.Verify( + x => x.AddUTXOs(withdrawalRequest, It.IsAny>()), Times.Once); + } + + [Fact] + public async Task LockUTXOs_PreviousRequestIdAllowedToShareUtxos_DoesNotConflictWithBumpedRequest() + { + // Arrange: the UTXO is locked by request 42, which is the specific request being bumped, + // so it must be allowed through rather than treated as a conflict. + var utxo = CreateUtxo(1, 10_000); + var fmutxoRepository = new Mock(); + fmutxoRepository + .Setup(x => x.GetLockedUTXOs(42, null)) + .ReturnsAsync(new List()); + var utxoTagRepository = new Mock(); + utxoTagRepository.Setup(x => x.GetByKeyValue(It.IsAny(), It.IsAny())) + .ReturnsAsync(new List()); + var walletWithdrawalRequestRepository = new Mock(); + walletWithdrawalRequestRepository + .Setup(x => x.AddUTXOs(It.IsAny(), It.IsAny>())) + .ReturnsAsync((true, (string?)null)); + var mapper = new Mock(); + mapper.Setup(m => m.Map(It.IsAny())).Returns(new FMUTXO()); + + var coinSelectionService = new CoinSelectionService(_logger, mapper.Object, fmutxoRepository.Object, + new Mock().Object, new Mock().Object, + walletWithdrawalRequestRepository.Object, utxoTagRepository.Object); + + var bumpRequest = new WalletWithdrawalRequest { Id = 100, Wallet = new Wallet { Id = 1 } }; + + // Act + await coinSelectionService.LockUTXOs(new List { utxo }, bumpRequest, + BitcoinRequestType.WalletWithdrawal, previousRequestIdAllowedToShareUtxos: 42); + + // Assert + fmutxoRepository.Verify(x => x.GetLockedUTXOs(42, null), Times.Once); + walletWithdrawalRequestRepository.Verify( + x => x.AddUTXOs(bumpRequest, It.IsAny>()), Times.Once); + } }