lightninglabs / lightninglabs/loop

staticaddr/withdraw: make withdrawal recovery deterministic and atomic

Open
#1,106 0 comments 0 reactions 1 assignee Claimed by @hieblmi View on GitHub
bug
Dominant language
Go
Stars
595
Forks
135
Avg merge
1d 8h
Merged PRs (30d)
12

Description

## Summary

Static address withdrawal recovery currently infers in-flight withdrawals from deposit state. That is fragile: a restart can leave deposits in `Withdrawing` without a durable authoritative withdrawal record, and recovery then has to reconstruct cluster membership from `FinalizedWithdrawalTx` pointers stored on individual deposits.

PR #1105 addresses a real bug in this area: some deposits can end up in `Withdrawing` with a missing `FinalizedWithdrawalTx`, which means restart recovery only reinstates the subset that still has a tx pointer. However, the deeper problem is that withdrawal identity is still duplicated across deposit rows, so recovery remains heuristic instead of authoritative.

## Current issue

- A single withdrawal is represented indirectly by copying the same `FinalizedWithdrawalTx` onto every participating deposit.
- Restart recovery groups `Withdrawing` deposits by tx hash and may need to reconstruct missing pointers.
- Partial persistence can leave only some deposits updated.
- Recovery of nil-pointer deposits is ambiguous unless the code can prove that a given tx actually spends a given deposit outpoint.
- Fee bumps are modeled by mutating per-deposit tx pointers instead of advancing a withdrawal-level record.
- Publish/persist ordering is hard to make robust because there is no single in-flight withdrawal object that can be resumed deterministically.

## Why this matters

- A crash or DB failure can leave an in-flight withdrawal only partially represented in the database.
- Recovery logic has to guess cluster membership instead of loading it from an authoritative record.
- Wrong recovery guesses can permanently corrupt deposit-to-withdrawal association if they are written back.
- Follow-up work like RBF handling, retry semantics, or reorg recovery stays harder than it needs to be.

## Option 1: narrower fix

Keep the current general model, but make it safer and atomic.

- Add a batch deposit update path so all selected deposits are updated in a single DB transaction.
- Persist `FinalizedWithdrawalTx` to all selected deposits before the state transition to `Withdrawing`.
- During recovery, only use a fallback tx for nil-pointer deposits when the association can be validated from tx inputs.
- Keep recovery non-destructive for ambiguous cases: if there are zero known txs, or multiple known tx clusters, skip nil-pointer deposits and log warnings instead of guessing.
- Let the normal confirmed-withdrawal path persist final state once the withdrawal confirms.

Pros:

- Smaller change set.
- No schema migration beyond batch update plumbing, if any.
- Fixes the immediate restart bug and avoids destructive heuristic persistence.

Cons:

- Withdrawal identity is still duplicated across deposit rows.
- Recovery still depends on deposit-level reconstruction.
- Publish/persist coordination remains subtle.

## Option 2: bigger refactor with a withdrawal aggregate / outbox-style publish path

Make withdrawals first-class persisted workflows and stop reconstructing them from deposits.

- Introduce an authoritative in-flight withdrawal record with a stable withdrawal ID, member deposits, raw finalized tx bytes, destination metadata, and an explicit status such as `ReadyToPublish`, `Published`, `Confirmed`, `Failed`.
- Persist the withdrawal record and deposit membership atomically in one DB transaction before any broadcast attempt.
- Publish from a separate resume-safe step after commit, ideally using an outbox-style worker/loop.
- On restart, recover by loading non-final withdrawals and resuming publish / spend / confirmation tracking from the withdrawal record.
- Model fee bumps as new attempts or replacement txs on the same logical withdrawal instead of mutating every deposit row independently.

Pros:

- Recovery becomes deterministic.
- No heuristic clustering by per-deposit tx pointer.
- Clear atomic boundary between DB state and external side effects.
- Better foundation for fee bumps, retries, and reorg handling.

Cons:

- Bigger schema and manager refactor.
- More migration and testing work.

## Suggested next step

If we want the smallest safe change, land option 1 first.

If we want to stop revisiting this class of bug, option 2 is the better long-term model: withdrawals should be authoritative persisted workflows, not something recovered indirectly from deposits in `Withdrawing`.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.