Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Persistence failures can still leave originals terminally stuck, and one settlement path reports success after an incomplete update.
3 open findings
What changed in this PR
Improves RBF withdrawal handling when an original transaction confirms instead of its replacement.
Changes:
- Detects and settles confirmed originals while failing losing replacements.
- Safely reverts originals after replacement failures.
- Adds repository, job, service, and E2E coverage.
| File | Description |
|---|---|
src/Services/WithdrawalRequestService.cs |
Adds confirmed-original settlement logic. |
src/Jobs/MonitorWithdrawalsJob.cs |
Checks unconfirmed replacements for confirmed ancestors. |
src/Jobs/PerformWithdrawalJob.cs |
Reverts originals after execution failures. |
src/Pages/Withdrawals.razor |
Reorders replacement marking and scheduling. |
src/Data/Repositories/WalletWithdrawalRequestRepository.cs |
Resolves references to the transaction that settled. |
test/NodeGuard.Tests/TestHelpers/TestEnvironment.cs |
Configures deterministic confirmation depth. |
test/NodeGuard.Tests/Services/WithdrawalRequestServiceTests.cs |
Tests settlement and replacement chains. |
test/NodeGuard.Tests/Jobs/PerformWithdrawalJobTests.cs |
Tests failed replacement execution. |
test/NodeGuard.Tests/Jobs/MonitorWithdrawalsJobTests.cs |
Tests monitoring decisions. |
test/NodeGuard.Tests/E2E/WithdrawalRbfBumpE2ETests.cs |
Covers mined-original RBF races end-to-end. |
test/NodeGuard.Tests/Data/Repositories/WalletWithdrawalRequestRepositoryTests.cs |
Tests reference-ID chain resolution. |
docker/e2e/README.md |
Documents the new E2E scenarios. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Jossec101
left a comment
Jossec101
left a comment
There was a problem hiding this comment.
LGTM check the comments.
e97e922 to
ecdcba2
Compare
| if (request.Status == WalletWithdrawalRequestStatus.OnChainConfirmed) return request; | ||
|
|
||
| // A replacement is always newer than what it replaces, so ids only go down and the walk ends. | ||
| request = request.BumpingWalletWithdrawalRequestId is { } replacedId && replacedId < request.Id | ||
| ? byId.GetValueOrDefault(replacedId) | ||
| : null; | ||
| } | ||
|
|
||
| return latest; |
There was a problem hiding this comment.
I think this could lead to a situation where, while the winner tx is being confirmed, the failed replacement (the latest) can be returned, which in elenpay side would be difficult to differentiate between "the whole settlement failed" and just the replacement failed....
What about just serving everything as an array instead of replacing? The grpc is already built that way. If all the txs participants in a rbf are returned in a array per refid, we could just listen until one is confirmed
Another option is to change the status to be OnchainConfirmationPending instead of OnchainConfirmed but in my opinion, the array one is the most healthy and less risky one (because you are not overwriting)
There was a problem hiding this comment.
If you guys want my opinion, I think that this is correct in the way @markettes announced in the EP channel: What it is returned, is the tx mined (or failed, if both fails). That's the part done in lines 112-115 right? 🤔
To us that would suit us well: The latest request returned by the grpc call is actually what it gets promoted if that's the case.
Said that, architectural wise is a bit confusing (philosoraptor from this point on; just thinking out loud but not blocking the PR):
- WD request is not actually a request in the way it's a registered petition from a caller. It is actually a WD transaction.
- Returning the history is a way to track what happens for that tx and having multiple "requests" on the response means, there could be a jump that generated a new tx.
- What it is actually blocking us is the current "jump" happening on the tx replaced, that is position on top of the history. Following the list approach would require that such change in the WD ordering does not happen, so that the replaced remains before the replacement.
There was a problem hiding this comment.
I have changed few things, this was a mess tbh. rn it is returning every request with the reference Id as you requested. I have clarify a bit the flow.
ecdcba2 to
c06d7b8
Compare
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. - MonitorWithdrawalsJob: null-safe, per-row errors, then ReconcileReplacementChainsAsync gives each live RBF-linked row its final status from its chain alone: Failed when what it replaced confirmed or a sibling branch did, Bumped / Cancelled when a replacement of it did - The replaced request is never marked or reverted; creating a replacement is refused while one is live; the replacements of a request come from one repository lookup, not a navigation - 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 - Withdrawals page gates bump / cancel on no live replacement and names the replacement next to the stored status - E2E: the original mined instead of its replacement (bump, cancel, bump of a bump) and the by-reference chain
c06d7b8 to
01b77a0
Compare
markettes
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Replacement-link selection, status derivation after failed writes, and UI cache refresh still have correctness issues.
7 open issues
4 resolved since last review
1 file not reviewed
src/Migrations/20261019172941_BumpedIsTerminalOnly.Designer.cs: Generated file
Review effort: Balanced
…action competition
…peting replacements
|
Hey @markettes ! One thing, in the table on the PR description, you mentioned that a fee bump where the original tx is mined instead of the replacement, this change will return "failed but other mined" . I think it'd be better to be returned as a success but including the promoted hash, as eitherway, from the caller perspective, the requested withdrawal is ongoing and In any case, I applaude your initiative on this 💪 , but unfortunately this topic requires strong consensus from EP and custody. From EP perspective, it's a really important thing that could bring us relief if we get a real, comprehensible snapshot of what happened with the withdrawal, and our processing depends on this. Is that ok for you guys to have a meeting @daliclovr @markettes @Jossec101 @RodriFS about this? Sorry if it sounds too bureaucratic, but we could benefit a lot. " |
| optional string request_metadata = 7; | ||
| // Newest RBF replacement of this request that was not abandoned, set as soon as it exists, even while it is still | ||
| // awaiting approval. Follow it to find the transaction that superseded (or will supersede) this one. | ||
| optional int32 replaced_by_request_id = 8; |
There was a problem hiding this comment.
I dont think we need this?
Integrators can use txid to monitor on their and they can link rbfs via their node?
There was a problem hiding this comment.
though if it helps e2e why not
| /// replacement confirmed are already in their final status and are left alone. No schema change. | ||
| /// </para> | ||
| /// </summary> | ||
| public partial class BumpedIsTerminalOnly : Migration |
There was a problem hiding this comment.
@daliclovr @imclvr can this affect you somehow? do you need this fix?
There was a problem hiding this comment.
For context:
Data only, no schema change. A Bumped or RBF-cancelled request none of whose replacements (recursively) confirmed goes back to OnChainConfirmationPending; rows whose replacement confirmed are already final and are left alone.
What it does in prod (checked read-only through Teleport on 2026-10-20, with the migration's own query run as a SELECT): no replacement in flight, no RBF cancellation ever used, 14 rows stored Bumped. 13 have a confirmed replacement and are left alone. One is revived: request 1489, marked Bumped on 2025-08-21 when its bump, request 1492, was created; that bump was cancelled before it was ever broadcast, so nothing reverted 1489, and its own transaction confirmed half an hour later in block 911022. It has been reported as WITHDRAWAL_BUMPED ever since, which is the bug this PR fixes. After the deploy the first MonitorWithdrawalsJob run stores it OnChainConfirmed; the log line to look for is Updating wallet withdrawal: 1489 to status: OnChainConfirmed.
| /// request's own transaction only: while a replacement's transaction is in the mempool and this one has no | ||
| /// confirmations, the request is reported with the status it will be stored with once that replacement settles. | ||
| /// </summary> | ||
| public (WalletWithdrawalRequestStatus Status, string? Reason, int? ReplacedById) EffectiveStatus(ulong ownConfirmations, |
There was a problem hiding this comment.
The API and the DB should agree: BUMPED and CANCELLED are final.
In the DB they already are: the monitor only writes Bumped / Cancelled after the replacement confirms. But the API reports them earlier, as soon as the replacement is in the mempool (this method). If the original then gets mined instead, the same request flips to SETTLED. The E2E test Replacement_WhenTheOriginalIsMinedInstead... shows exactly that. An integrator that refunds when it reads CANCELLED would pay twice.
Proposal: only confirmations change an RBF chain, and the API returns the stored status.
- Drop
EffectiveStatusand return the stored status. - Every tx in the chain is monitored. This already happens: a replaced request stays OnChainConfirmationPending, so the monitor keeps polling its txid.
- The first tx to confirm wins and becomes SETTLED. Every other request in the chain gets a final status: BUMPED or CANCELLED for the ones it replaced, FAILED for the rest.
ReconcileReplacementChainsAsyncalready does this. - Until something confirms, the replaced requests read PENDING_CONFIRMATION, with
replaced_by_request_idpointing at the next one. No new "bump pending" status is needed: the UI already shows "replaced by #N" and hides Bump / Cancel while a replacement is live, and API clients can usereplaced_by_request_idthe same way.
About txs that disappear from the mempool: I wouldn't mark them FAILED just for that. A tx missing from our mempool can still be in a miner's mempool, or be rebroadcast, and then confirm. FAILED should only come from a confirmation that proves the tx can never confirm, meaning another tx spending the same inputs confirmed. If a whole chain drops out of the mempool, it should stay pending and raise an alert so someone can rebroadcast or bump it. That can be a follow-up.
Also to update: the proto docs for BumpWithdrawal, CancelWithdrawal and WITHDRAWAL_BUMPED, which describe the early reporting; the NodeGuardService status tests that expect BUMPED / CANCELLED while the replacement is in the mempool; and the matching E2E polls. They should expect PENDING_CONFIRMATION with replaced_by_request_id set.
| // WalletWithdrawalRequestStatus: Cancelled = 1, OnChainConfirmationPending = 4, OnChainConfirmed = 5, Bumped = 8 | ||
| // A Cancelled request with an RBF-cancellation replacement was cancelled by it: humans can only cancel a | ||
| // request that was never broadcast, and only a broadcast one gets replacements. | ||
| // Internal so BumpedIsTerminalOnlyMigrationE2ETests can run it against the stack's Postgres. |
There was a problem hiding this comment.
This mentions BumpedIsTerminalOnlyMigrationE2ETests, but that test doesn't exist. Let's drop the comment and make Repair private, or add the test.
There was a problem hiding this comment.
sorry, bad comment, I did run a test to make sure about this
… pending until a confirmation.
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; the replacement is reachable through the existing
BumpingWalletWithdrawalRequestIdlink, only a confirmation changes a chain, andBumpedbecomes terminal: written only by the monitor once a replacement confirms. The API reports the stored status and nothing derived, soWITHDRAWAL_SETTLED,WITHDRAWAL_BUMPED,WITHDRAWAL_CANCELLEDandWITHDRAWAL_FAILEDare final; while a chain is in flight a client followsreplaced_by_request_id.Why:
Bumpedmeant two things at once ("replacement in flight" and "superseded, done"). 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 (RPCMarkWithdrawalRequestCancelled, the UI's dismissed confirmation modal, a failed revert write) the original satBumpedforever with a transaction that could still confirm, by-id clients sawWITHDRAWAL_BUMPEDforever, and its inputs stopped counting as locked. The revert itself was a cross-row write with a race against the broadcast.OnChainConfirmationPendingWITHDRAWAL_PENDING_CONFIRMATION,replaced_by_request_idsetWITHDRAWAL_PENDING_CONFIRMATION,replaced_by_request_idsetOnChainConfirmationPendingWITHDRAWAL_PENDING_CONFIRMATION, no next hopBumped/Cancelled+ reason, by the monitorWITHDRAWAL_BUMPED/WITHDRAWAL_CANCELLEDOnChainConfirmedOnChainConfirmed, the monitor polls it itselfWITHDRAWAL_SETTLEDFailed"Withdrawal request A was mined instead", every replacement, a cold one awaiting approval includedChanged after review: the first version reported
WITHDRAWAL_BUMPED/WITHDRAWAL_CANCELLEDas soon as the replacement was in the mempool, which flipped toWITHDRAWAL_SETTLEDwhen the original was mined instead (an integrator refunding onCANCELLEDwould have paid twice). Clients that switched to the replacement onWITHDRAWAL_BUMPEDmust followreplaced_by_request_idinstead. A transaction that drops out of the mempool is not failed for that:FAILEDonly comes from a confirmation that proves it can never confirm; alerting on a chain that left the mempool is a follow-up.ReconcileReplacementChainsAsyncgives each live RBF-linked row its final status from the database alone. Every member of a chain spends the original's inputs, so one confirming decides the rest: a confirmed replacement of a request makes itBumped(orCancelledwhen a cancellation sits on the path), any other confirmed member (what it replaced, or a sibling branch from a double submit) fails it. Each row writes only itself, so a failed write is retried next run.GetReplacementsAsync(ids)returns the replacements of a set of requests in one query;RbfReplacementsanswers "live" (gates a further replacement) and "newest" (the next hop) over them. No navigation on the entity, so a caller cannot forget to load it.FAILED_PRECONDITION).MarkOriginalAsReplacedAsync,RevertOriginalAsyncand the"Cancelled by RBF replacement request"marker comparison are gone.GetWithdrawalsRequestStatusByReferenceIdsreturns every request carrying the reference id, oldest first, each with its own status.WithdrawalRequestgainsreplaced_by_request_id(newest non-abandoned replacement, set even while awaiting approval).WITHDRAWAL_BUMPED/WITHDRAWAL_CANCELLEDare reported only once stored, i.e. once the replacement confirmed; until then the replaced request readsWITHDRAWAL_PENDING_CONFIRMATIONwithreplaced_by_request_idset. Confirmations are looked up in parallel.TRANSACTION_CONFIRMATION_MINIMUM_BLOCKSis clamped to at least one at its source (an evicted transaction sits at 0 and must never settle); the generic repository'sAddAsyncreturns the database's message likeAddRangeAsyncalready did; the AutoMapper self-map ignores the replaced-request navigation soUpdatenever writes a neighbour.Migration
BumpedIsTerminalOnlyData only, no schema change. A
Bumpedor RBF-cancelled request none of whose replacements (recursively) confirmed goes back toOnChainConfirmationPending; rows whose replacement confirmed are already final and are left alone.What it does in prod (checked read-only through Teleport on 2026-10-20, with the migration's own query run as a SELECT): no replacement in flight, no RBF cancellation ever used, 14 rows stored
Bumped. 13 have a confirmed replacement and are left alone. One is revived: request 1489, markedBumpedon 2025-08-21 when its bump, request 1492, was created; that bump was cancelled before it was ever broadcast, so nothing reverted 1489, and its own transaction confirmed half an hour later in block 911022. It has been reported asWITHDRAWAL_BUMPEDever since, which is the bug this PR fixes. After the deploy the firstMonitorWithdrawalsJobrun stores itOnChainConfirmed; the log line to look for isUpdating wallet withdrawal: 1489 to status: OnChainConfirmed.Tests
Category!=E2E),just check-migrationsclean. Coverage for the guard, the chain reconciliation (bump of a bump, bump of a cancellation, cancellation of a bump, sibling branch, cold replacement awaiting approval, finalizing rows skipped, both directions confirmed), the stored statuses and next hop over gRPC, the by-reference chain response, the replacements lookup and the mapper.WithdrawalRbfBumpE2ETests): fee bump, RBF cancel, the original mined instead of its replacement (bump and cancel), bump of a bump with the first mined (A settles, B and C fail), and the by-reference chain. Every in-flight poll expectsWITHDRAWAL_PENDING_CONFIRMATIONwithreplaced_by_request_idset, and the final statuses only after mining. Runs in CI under therun-e2e-testslabel.