Stop concurrent withdrawals from grabbing the same UTXO

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.

src/Data/Repositories/FUTXORepository.cs

@@ -126,10 +126,10 @@ public async Task<List<FMUTXO>> 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<List<FMUTXO>> 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();
             }
 

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)

src/Pages/ChannelRequests.razor

@@ -555,7 +555,7 @@
     TemplatePsbtString="@_templatePSBTString"
     SignedPSBT="@_psbt"/>
 
 <CancelOrRejectPopup
     @ref=@_rejectCancelModalRef
     Title='@(_selectedStatusActionString + " operation: " + _selectedRequest?.Id)'
     Validator="@RejectReasonValidator"
@@ -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

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);

src/Rpc/NodeGuardService.cs

@@ -259,14 +259,14 @@ public override async Task<RequestWithdrawalResponse> 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,20 +307,14 @@ public override async Task<RequestWithdrawalResponse> 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);
             }
 
             // 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<GetAvailableWalletsResponse> GetAvailableWallets(GetAvailableWalletsRequest request,
         ServerCallContext context)
     {
@@ -561,6 +575,7 @@ public override async Task<OpenChannelResponse> OpenChannel(OpenChannelRequest r
         }
 
         int requestId;
+        ChannelOperationRequest? channelOperationRequest = null;
 
         try
         {
@@ -574,7 +589,8 @@ public override async Task<OpenChannelResponse> 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<OpenChannelResponse> 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<OpenChannelResponse> 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));
         }

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<UTXO, FMUTXO>(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");