Skip to content

Keep replaced withdrawals pending until their RBF replacement confirms - #645

Closed
markettes wants to merge 3 commits into
fix/failed-rbf-original-confirmedfrom
refactor/bumped-terminal-only
Closed

markettes wants to merge 3 commits into
fix/failed-rbf-original-confirmedfrom
refactor/bumped-terminal-only

Conversation

@markettes

@markettes markettes commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #642.

What changes

A withdrawal request's status now only describes its own transaction. Creating, executing or abandoning an RBF replacement never writes the request it replaces; "replaced" is derived from the replacement through the existing BumpingWalletWithdrawalRequestId link, and Bumped becomes terminal: written only by the monitor once a replacement confirms.

Why: Bumped meant two things at once ("replacement in flight" and "superseded, done"), and that dual meaning is behind the bugs the RBF work keeps hitting. A bumped original was not polled, so its confirmation was only noticed through the replacement's row; whenever the replacement was abandoned without a revert (RPC MarkWithdrawalRequestCancelled, the UI's dismissed confirmation modal, a failed revert write) the original sat Bumped forever with a transaction that could still confirm, by-id clients saw WITHDRAWAL_BUMPED forever, and its inputs stopped counting as locked. The revert itself was the cross-row write Copilot kept flagging on #642.

Event Original in DB Original by gRPC id Replacement
Replacement created (cold: awaiting approval) OnChainConfirmationPending WITHDRAWAL_PENDING_CONFIRMATION, replaced_by_request_id set live
Replacement broadcast untouched WITHDRAWAL_BUMPED / WITHDRAWAL_CANCELLED (derived) OnChainConfirmationPending
Replacement abandoned untouched WITHDRAWAL_PENDING_CONFIRMATION again terminal
Replacement confirms Bumped / Cancelled + reason, by the monitor same OnChainConfirmed
Original mined instead OnChainConfirmed, the monitor polls it itself WITHDRAWAL_SETTLED Failed "Withdrawal request A was mined instead", every replacement, a cold one awaiting approval included
  • Model: Replacements navigation, EffectiveStatus() (single derivation point for the RPC and the Withdrawals page), LiveReplacement / BroadcastReplacement. The AutoMapper self-map ignores both RBF navigations so Repository.Update never writes a neighbour.
  • One live replacement per request: service guard plus partial unique index IX_WalletWithdrawalRequests_LiveReplacement; the loser of a race gets FAILED_PRECONDITION, not INTERNAL.
  • Monitor: null-safe NBXplorer result, per-row errors instead of aborting the batch, a floor of one confirmation (replaced originals are polled again and an evicted one sits at 0), then ReconcileReplacementChainsAsync gives each live linked row its final status from the database alone. Each row writes only itself, so a failed write is retried next run. Path rule: a bump of a cancellation still cancels the first original.
  • gRPC: GetWithdrawalsRequestStatusByReferenceIds now returns every request carrying the reference id, oldest first, each with its own status. WithdrawalRequest gains replaced_by_request_id (newest non-abandoned replacement, set even while awaiting approval). WITHDRAWAL_BUMPED stays, derived while in flight, stored once settled.
  • UI: bump / cancel gated on "pending and no live replacement", status cell shows the stored status plus "replaced by #N" (the page has no NBXplorer input, so it does not derive); the mark / revert paths are gone.
  • Removed: MarkOriginalAsReplacedAsync, RevertOriginalAsync, SettleMinedOriginalAsync, the "Cancelled by RBF replacement request" marker comparison.

Migration BumpedIsTerminalOnly

Repairs legacy rows in SQL before creating the index: a Bumped or RBF-cancelled request none of whose replacements (recursively) confirmed goes back to OnChainConfirmationPending; rows whose replacement confirmed are already final. Then a guard raises a readable error if any request has more than one live replacement. Before deploying to prod run:

SELECT "BumpingWalletWithdrawalRequestId", count(*) FROM "WalletWithdrawalRequests"
WHERE "BumpingWalletWithdrawalRequestId" IS NOT NULL AND "Status" IN (0, 3, 4, 7)
GROUP BY 1 HAVING count(*) > 1;   -- expected: no rows

Tests

  • Unit: 823 passing (Category!=E2E), just check-migrations clean. New coverage for the guard, the race on insert, the chain reconciliation (bump of a bump, bump of a cancellation, cancellation of a bump, cold replacement awaiting approval, finalizing rows skipped, both directions confirmed), the derived statuses over gRPC, the by-reference chain response, and the mapper.
  • E2E: expectations updated (BumpOfABump… now expects B and C failed; the by-reference assertion expects the whole chain). Not run locally yet: add the run-e2e-tests label.

A withdrawal request's status now only describes its own transaction.
Creating, executing or abandoning an RBF replacement never writes the
request it replaces: the API derives WITHDRAWAL_BUMPED / _CANCELLED from
a broadcast replacement, and the monitor stores Bumped / Cancelled only
once a replacement confirms, or settles the request itself when it was
mined instead. Bumped is terminal.

- Replacements navigation, EffectiveStatus and the one-live-replacement
  guard (service check plus partial unique index) on WalletWithdrawalRequest
- MonitorWithdrawalsJob: null-safe, per-row errors, floor of one
  confirmation, then ReconcileReplacementChainsAsync writes each linked
  row from its chain alone (fails every replacement of a mined original,
  a bump of a bump included; a bump of a cancellation still cancels)
- GetWithdrawalsRequestStatusByReferenceIds returns the whole chain, and
  WithdrawalRequest gains replaced_by_request_id
- Migration repairs legacy Bumped / RBF-cancelled rows with no confirmed
  replacement back to pending and guards the index creation
- Withdrawals page gates bump / cancel on no live replacement, shows the
  derived status, and the revert paths are gone
@markettes
markettes added this pull request to stack #646 October 19, 2026 13:41
- One Ancestors iterator replaces the two hand-rolled upward walks; the
  chain is cached as a dictionary and the live rows are fetched as ids
- One "live" status set: the repository and the index filter derive from
  WalletWithdrawalRequest.LiveStatuses; the FinalizingPSBT skip lives in
  the service only
- SupersededBy gives the derived and the stored Bumped / Cancelled the
  same (status, reason), and the reason builders share one tx suffix
- Repository.AddAsync returns the database's message like AddRangeAsync,
  so the unique-index race is recognised from it and
  HasLiveReplacementAsync goes away
- GetRbfChainAsync walks up to the root, then down level by level, no
  re-fetching; the status RPC looks confirmations up in parallel
- TRANSACTION_CONFIRMATION_MINIMUM_BLOCKS is clamped at its source
- The Withdrawals page shows the stored status plus the replacement; the
  migration keys RBF-cancelled rows on the link, not on the reason text
@markettes
markettes force-pushed the refactor/bumped-terminal-only branch from 6cca56d to e51cc24 Compare October 19, 2026 16:51
@markettes
markettes force-pushed the fix/failed-rbf-original-confirmed branch from c06d7b8 to 01b77a0 Compare October 19, 2026 17:32
@markettes

Copy link
Copy Markdown
Contributor Author

Folded into #642.

@markettes markettes closed this Oct 19, 2026
@markettes
markettes deleted the refactor/bumped-terminal-only branch October 19, 2026 17:32
@markettes
markettes removed this pull request from stack #646 October 19, 2026 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant