Keep replaced withdrawals pending until their RBF replacement confirms

docker/e2e/README.md

@@ -70,9 +70,15 @@ tests (`DustUtxoWithdrawalE2ETests`, `WithdrawalRbfBumpE2ETests`, `GetNewWalletA
   those two in sync between the `nodeguard` and `e2e-runner` services**, or the test asserts against numbers
   NodeGuard didn't plan with.
 - **RBF bump scenario** (`WithdrawalRbfBumpE2ETests`): `RequestWithdrawal` on the hot wallet, then `BumpWithdrawal`
-  over gRPC, asserting bitcoind evicts the original for the higher-fee replacement, NodeGuard marks the original
-  `WITHDRAWAL_BUMPED`, and the replacement settles once mined. It relies on `MONITOR_WITHDRAWALS_CRON` being fast on
-  the `nodeguard` service (10s here; prod keeps the every-5-minutes default) and never mines between the two calls.
+  over gRPC, asserting bitcoind evicts the original for the higher-fee replacement, NodeGuard keeps reporting the
+  original `WITHDRAWAL_PENDING_CONFIRMATION` with `replaced_by_request_id` set while the replacement is in the mempool
+  (only a confirmation changes an RBF chain), and once the replacement is mined it settles and the original is stored
+  as `WITHDRAWAL_BUMPED`. It relies on
+  `MONITOR_WITHDRAWALS_CRON` being fast on the `nodeguard` service (10s here; prod keeps the every-5-minutes default)
+  and never mines between the two calls. The mined-original theory mines the ORIGINAL with `generateblock` (raw tx,
+  bypassing the mempool) after a bump or cancel, and asserts NodeGuard settles the original and fails the replacement.
+  The bump-of-a-bump test does the same with three transactions (A bumped to B, B to C, A mined): A settles, B and C
+  fail. The by-reference lookup returns the whole chain, each request with its own status.
 - **Adding an e2e test**: put it in `[Collection("E2E")]` (so it serialises with the others on the one
   regtest chain — without it the class runs in parallel and they interfere), have it provision the
   resources it needs and reset its own state, and gate it with `[E2EFact]`.

src/Automapper/MapperProfile.cs

@@ -60,7 +60,9 @@ public MapperProfile()
                 .ForMember(x => x.Wallet, opt => opt.Ignore())
                 .ForMember(x => x.WalletWithdrawalRequestPSBTs, opt => opt.Ignore())
                 .ForMember(x => x.UserRequestor, opt => opt.Ignore())
-                .ForMember(x => x.UTXOs, opt => opt.Ignore());
+                .ForMember(x => x.UTXOs, opt => opt.Ignore())
+                // Update attaches the whole mapped graph as Modified: the replaced request must never ride along.
+                .ForMember(x => x.BumpingWalletWithdrawalRequest, opt => opt.Ignore());
 
             CreateMap<UTXO, FMUTXO>()
                 .ForMember(x => x.TxId, opt => opt.MapFrom(x => x.Outpoint.Hash.ToString()))

src/Data/Models/WalletWithdrawalRequest.cs

@@ -66,7 +66,8 @@ public enum WalletWithdrawalRequestStatus
         FinalizingPSBT = 7,
 
         /// <summary>
-        /// The tx was bumped by Replace by Fee (RBF) method
+        /// Superseded: an RBF replacement of this request (or of one of its replacements) confirmed. Terminal, written
+        /// only by the monitor. While a replacement is merely in flight the request keeps its own status.
         /// </summary>
         Bumped = 8
     }
@@ -213,24 +214,69 @@ public bool Equals(WalletWithdrawalRequest? other)
         /// </summary>
         public string? UserRequestorId { get; set; }
 
-        public ApplicationUser UserRequestor { get; set; }
+        public ApplicationUser? UserRequestor { get; set; }
         public int WalletId { get; set; }
 
-        public Wallet Wallet { get; set; }
+        public Wallet? Wallet { get; set; }
 
         public int? BumpingWalletWithdrawalRequestId { get; set; }
 
         public WalletWithdrawalRequest? BumpingWalletWithdrawalRequest { get; set; }
 
         /// <summary>
         /// True when this request replaces <see cref="BumpingWalletWithdrawalRequest"/> to CANCEL it (RBF): same inputs,
-        /// a single output back to the wallet, and the replaced request ends up Cancelled instead of Bumped.
+        /// a single output back to the wallet, and once this one confirms the replaced request ends up Cancelled
+        /// instead of Bumped.
         /// </summary>
         public bool IsRbfCancellation { get; set; }
 
-        public List<WalletWithdrawalRequestPSBT> WalletWithdrawalRequestPSBTs { get; set; }
+        /// <summary>
+        /// Statuses in which a request can still put (or has put) a transaction in the mempool. A replacement in any of
+        /// them blocks a second replacement of the same request.
+        /// </summary>
+        public static readonly WalletWithdrawalRequestStatus[] LiveStatuses =
+        {
+            WalletWithdrawalRequestStatus.Pending,
+            WalletWithdrawalRequestStatus.PSBTSignaturesPending,
+            WalletWithdrawalRequestStatus.FinalizingPSBT,
+            WalletWithdrawalRequestStatus.OnChainConfirmationPending,
+        };
+
+        [NotMapped]
+        public bool IsLive => LiveStatuses.Contains(Status);
+
+        /// <summary>
+        /// Never put a transaction in the mempool, or lost to another one. A request cancelled by its own RBF
+        /// replacement (Cancelled with a txid) was superseded, not abandoned: it is a hop of the chain.
+        /// </summary>
+        [NotMapped]
+        public bool IsAbandoned => Status is WalletWithdrawalRequestStatus.Failed or WalletWithdrawalRequestStatus.Rejected
+            || (Status == WalletWithdrawalRequestStatus.Cancelled && string.IsNullOrEmpty(TxId));
+
+        public static string RbfCancellationReason(int replacementId, string? txId) =>
+            WithTx($"Cancelled by RBF replacement request {replacementId}", txId);
+
+        public static string ReplacedReason(int replacementId, string? txId) =>
+            WithTx($"Replaced by request {replacementId}", txId);
+
+        public static string MinedInsteadReason(int originalId, string? txId) =>
+            WithTx($"Withdrawal request {originalId} was mined instead", txId);
+
+        private static string WithTx(string reason, string? txId) =>
+            string.IsNullOrEmpty(txId) ? reason : $"{reason} (tx {txId})";
+
+        /// <summary>
+        /// What a request superseded by <paramref name="replacement"/> becomes: Cancelled when a cancellation sits
+        /// on the way to it, otherwise Bumped.
+        /// </summary>
+        public static (WalletWithdrawalRequestStatus Status, string Reason) SupersededBy(WalletWithdrawalRequest replacement, bool cancellation) =>
+            cancellation
+                ? (WalletWithdrawalRequestStatus.Cancelled, RbfCancellationReason(replacement.Id, replacement.TxId))
+                : (WalletWithdrawalRequestStatus.Bumped, ReplacedReason(replacement.Id, replacement.TxId));
+
+        public List<WalletWithdrawalRequestPSBT>? WalletWithdrawalRequestPSBTs { get; set; }
 
-        public List<FMUTXO> UTXOs { get; set; }
+        public List<FMUTXO>? UTXOs { get; set; }
 
         /// <summary>
         /// This is a optional field that you can used to link withdrawals with externally-generated IDs (e.g. a withdrawal/settlement that belongs to an elenpay store)
@@ -259,4 +305,31 @@ public override int GetHashCode()
             return !Equals(left, right);
         }
     }
+
+    /// <summary>
+    /// The requests that replace one request through RBF (fee bumps and cancellations), as the repository's
+    /// GetReplacementsAsync returns them. A request's status only ever describes its own transaction and only a
+    /// confirmation changes it: a live replacement blocks another one, and the newest not abandoned is the next hop
+    /// the API and the UI point at while the chain is in flight.
+    /// </summary>
+    public sealed class RbfReplacements
+    {
+        public static readonly RbfReplacements None = new(Enumerable.Empty<WalletWithdrawalRequest>());
+
+        private readonly List<WalletWithdrawalRequest> _requests;
+
+        public RbfReplacements(IEnumerable<WalletWithdrawalRequest> requests)
+        {
+            _requests = requests.ToList();
+        }
+
+        /// <summary>
+        /// The one still in flight, if any. Creating a replacement is refused while one exists; should a race let two
+        /// through, the newest is the one that had to outbid the other to get into the mempool.
+        /// </summary>
+        public WalletWithdrawalRequest? Live => _requests.Where(r => r.IsLive).MaxBy(r => r.Id);
+
+        /// <summary>The newest one that was not abandoned: the next hop when following a chain by id.</summary>
+        public WalletWithdrawalRequest? Newest => _requests.Where(r => !r.IsAbandoned).MaxBy(r => r.Id);
+    }
 }

src/Data/Repositories/Interfaces/IWalletWithdrawalRequestRepository.cs

@@ -58,4 +58,19 @@ public interface IWalletWithdrawalRequestRepository : IBitcoinRequestRepository
     Task<List<WalletWithdrawalRequest>> GetPendingRequests();
 
     Task<List<WalletWithdrawalRequest>> GetOnChainPendingWithdrawals();
+
+    /// <summary>
+    /// Every request connected to this one through RBF links, in both directions, the request itself included.
+    /// Untracked, no navigations loaded.
+    /// </summary>
+    Task<List<WalletWithdrawalRequest>> GetRbfChainAsync(int requestId);
+
+    /// <summary>Ids of the live requests that replace another request or are replaced by one.</summary>
+    Task<List<int>> GetLiveRbfLinkedRequestIdsAsync();
+
+    /// <summary>
+    /// The RBF replacements of each of <paramref name="requestIds"/>, keyed by the request they replace. A request
+    /// with none is absent from the lookup. Untracked.
+    /// </summary>
+    Task<ILookup<int, WalletWithdrawalRequest>> GetReplacementsAsync(IEnumerable<int> requestIds);
 }

src/Data/Repositories/Repository.cs

@@ -60,6 +60,8 @@ public async Task<List<T>> GetAll(ApplicationDbContext applicationDbContext)
             catch (Exception e)
             {
                 _logger.LogError(e, "Error on repository");
+                // The innermost message is the database's own (e.g. the constraint a unique index rejected).
+                return (false, e.GetBaseException().Message);
             }
 
             return (rowsChanged, null);

src/Data/Repositories/WalletWithdrawalRequestRepository.cs

@@ -82,16 +82,12 @@ public async Task<List<WalletWithdrawalRequest>> GetByReferenceIds(List<string>
         {
             await using var applicationDbContext = await _dbContextFactory.CreateDbContextAsync();
 
-            // RBF replacements inherit the reference id of the request they replace, so a reference id can match a whole
-            // chain of requests. Only the latest one of each chain reflects the current status of the withdrawal.
-            var requests = await applicationDbContext.WalletWithdrawalRequests
+            // RBF replacements inherit the reference id of the request they replace, so a reference id matches a whole
+            // chain. Every request of it is returned, oldest first, each reporting its own (derived) status.
+            return await applicationDbContext.WalletWithdrawalRequests
                 .Where(wr => !string.IsNullOrEmpty(wr.ReferenceId) && referenceIds.Contains(wr.ReferenceId))
+                .OrderBy(wr => wr.Id)
                 .ToListAsync();
-
-            return requests
-                .GroupBy(wr => wr.ReferenceId!)
-                .Select(g => g.OrderByDescending(wr => wr.Id).First())
-                .ToList();
         }
 
         public async Task<WalletWithdrawalRequest?> GetByTxHash(string txHash)
@@ -382,6 +378,73 @@ public async Task<List<WalletWithdrawalRequest>> GetPendingRequests()
             return walletWithdrawalRequests;
         }
 
+        public async Task<List<WalletWithdrawalRequest>> GetRbfChainAsync(int requestId)
+        {
+            await using var applicationDbContext = await _dbContextFactory.CreateDbContextAsync();
+            var requests = applicationDbContext.WalletWithdrawalRequests.AsNoTracking();
+
+            // Up to the first request of the chain, then down through every replacement, level by level. One query
+            // per hop; chains are a handful of rows.
+            var root = await requests.SingleOrDefaultAsync(x => x.Id == requestId);
+            if (root == null) return new List<WalletWithdrawalRequest>();
+
+            var chain = new Dictionary<int, WalletWithdrawalRequest> { [root.Id] = root };
+            while (root.BumpingWalletWithdrawalRequestId is { } replacedId && !chain.ContainsKey(replacedId))
+            {
+                var replaced = await requests.SingleOrDefaultAsync(x => x.Id == replacedId);
+                if (replaced == null) break;
+
+                chain[replaced.Id] = replaced;
+                root = replaced;
+            }
+
+            var expanded = new HashSet<int> { root.Id };
+            var frontier = new List<int> { root.Id };
+            while (frontier.Count > 0)
+            {
+                var parents = frontier;
+                var children = await requests
+                    .Where(x => x.BumpingWalletWithdrawalRequestId != null && parents.Contains(x.BumpingWalletWithdrawalRequestId.Value))
+                    .ToListAsync();
+
+                frontier = new List<int>();
+                foreach (var child in children)
+                {
+                    chain.TryAdd(child.Id, child);
+                    if (expanded.Add(child.Id)) frontier.Add(child.Id);
+                }
+            }
+
+            return chain.Values.OrderBy(x => x.Id).ToList();
+        }
+
+        public async Task<List<int>> GetLiveRbfLinkedRequestIdsAsync()
+        {
+            await using var applicationDbContext = await _dbContextFactory.CreateDbContextAsync();
+
+            var live = WalletWithdrawalRequest.LiveStatuses;
+            var requests = applicationDbContext.WalletWithdrawalRequests;
+            return await requests
+                .Where(x => live.Contains(x.Status)
+                            && (x.BumpingWalletWithdrawalRequestId != null
+                                || requests.Any(r => r.BumpingWalletWithdrawalRequestId == x.Id)))
+                .Select(x => x.Id)
+                .ToListAsync();
+        }
+
+        public async Task<ILookup<int, WalletWithdrawalRequest>> GetReplacementsAsync(IEnumerable<int> requestIds)
+        {
+            await using var applicationDbContext = await _dbContextFactory.CreateDbContextAsync();
+
+            var ids = requestIds.Distinct().ToList();
+            var replacements = await applicationDbContext.WalletWithdrawalRequests
+                .AsNoTracking()
+                .Where(x => x.BumpingWalletWithdrawalRequestId != null && ids.Contains(x.BumpingWalletWithdrawalRequestId.Value))
+                .ToListAsync();
+
+            return replacements.ToLookup(x => x.BumpingWalletWithdrawalRequestId!.Value);
+        }
+
         public async Task<List<WalletWithdrawalRequest>> GetOnChainPendingWithdrawals()
         {
             await using var applicationDbContext = await _dbContextFactory.CreateDbContextAsync();

src/Helpers/Constants.cs

@@ -526,8 +526,9 @@ static Constants()
         var environmentVariableMax = Environment.GetEnvironmentVariable("MAXIMUM_WITHDRAWAL_BTC_AMOUNT");
         if (environmentVariableMax != null) MAXIMUM_WITHDRAWAL_BTC_AMOUNT = decimal.Parse(environmentVariableMax, NumberStyles.AllowDecimalPoint, CultureInfo.InvariantCulture);
 
+        // At least one: a transaction evicted from the mempool sits at 0 confirmations and must never count as mined.
         var transactionConfBlocks = GetEnvironmentalVariableOrThrowIfNotTesting("TRANSACTION_CONFIRMATION_MINIMUM_BLOCKS");
-        if (transactionConfBlocks != null) TRANSACTION_CONFIRMATION_MINIMUM_BLOCKS = int.Parse(transactionConfBlocks);
+        TRANSACTION_CONFIRMATION_MINIMUM_BLOCKS = Math.Max(1, transactionConfBlocks != null ? int.Parse(transactionConfBlocks) : 1);
 
         var defaultChannelFeePolicyTimelockDeltaBlocks = Environment.GetEnvironmentVariable("DEFAULT_CHANNEL_FEE_POLICY_TIMELOCK_DELTA_BLOCKS");
         if (defaultChannelFeePolicyTimelockDeltaBlocks != null) DEFAULT_CHANNEL_FEE_POLICY_TIMELOCK_DELTA_BLOCKS = int.Parse(defaultChannelFeePolicyTimelockDeltaBlocks);