Skip to content

staticaddr/withdraw: prepare deposits before publish #1176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hieblmi wants to merge 1 commit into
base: master
Choose a base branch
from
Open

staticaddr/withdraw: prepare deposits before publish #1176

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 48 additions & 36 deletions staticaddr/withdraw/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,19 +439,37 @@ func (m *Manager) WithdrawDeposits(ctx context.Context,
return "", "", err
}

published, err := m.publishFinalizedWithdrawalTx(ctx, finalizedTx)
withdrawalPkScript, err := txscript.PayToAddrScript(withdrawalAddress)
if err != nil {
return "", "", err
return "", "", fmt.Errorf("could not get withdrawal "+
"pkscript: %w", err)
}

if !published {
return "", "", nil
// Attach the finalized withdrawal transaction before transitioning the
// deposits. The state transition persists it, allowing recovery to
// republish the transaction if publishing blocks or the client restarts.
previousWithdrawalTxns := make([]*wire.MsgTx, len(deposits))
for i, d := range deposits {
d.Lock()
previousWithdrawalTxns[i] = d.FinalizedWithdrawalTx
d.FinalizedWithdrawalTx = finalizedTx
d.Unlock()
}

withdrawalPkScript, err := txscript.PayToAddrScript(withdrawalAddress)
// Transition before publishing so wallet reconciliation can't remove a
// spent deposit from the active set while publication is in progress.
err = m.cfg.DepositManager.TransitionDeposits(

@starius starius Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we ensure persistence errors are observable here? TransitionDeposits returns success even when the FSM’s Store.UpdateDeposit fails, because that error is only logged. Since the explicit update now only runs for fee bumps, an initial withdrawal can reach publication while the database still says Deposited and has no finalized transaction. That recreates the restart/reconciliation race this change is intended to fix.

ctx, deposits, deposit.OnWithdrawInitiated, deposit.Withdrawing,
)
if err != nil {
return "", "", fmt.Errorf("could not get withdrawal "+
"pkscript: %w", err)
for i, d := range deposits {

@starius starius Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this rollback safe for multiple deposits? TransitionDeposits processes FSMs sequentially, so an earlier deposit may already be Withdrawing and persisted when a later transition fails. Restoring only FinalizedWithdrawalTx would leave that deposit in Withdrawing with its old or nil transaction, while the cluster may have mixed states. We likely need atomic persistence/transition or a rollback that also restores state and database records.

d.Lock()
d.FinalizedWithdrawalTx = previousWithdrawalTxns[i]
d.Unlock()
}

return "", "", fmt.Errorf("failed to transition deposits %w",
err)
}

// If this is the first time this cluster of deposits is withdrawn, we
Expand Down Expand Up @@ -479,49 +497,43 @@ func (m *Manager) WithdrawDeposits(ctx context.Context,
// If a previous withdrawal existed across the selected deposits, and
// it isn't the same as the new withdrawal, we remove it from the
// finalized withdrawals to stop republishing it on block arrivals.
deposits[0].Lock()
prevTx := deposits[0].FinalizedWithdrawalTx
deposits[0].Unlock()
previousWithdrawalTx := previousWithdrawalTxns[0]

@starius starius Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumption is pre-existing, but could we fix it while touching this logic? The fee-bump path verifies that all Withdrawing deposits reference the same previous transaction, but that invariant is not enforced for Deposited deposits or inconsistent recovery states.

Since we now retain every previous transaction, consider deleting every distinct non-nil previous hash other than finalizedTx, or explicitly validating that all entries match. Otherwise, a stale transaction associated with a deposit other than index 0 could remain in finalizedWithdrawalTxns and continue being republished. A multi-deposit regression test would be useful here.

if previousWithdrawalTx != nil &&
previousWithdrawalTx.TxHash() != finalizedTx.TxHash() {

if prevTx != nil && prevTx.TxHash() != finalizedTx.TxHash() {
m.mu.Lock()
delete(m.finalizedWithdrawalTxns, prevTx.TxHash())
delete(m.finalizedWithdrawalTxns, previousWithdrawalTx.TxHash())
m.mu.Unlock()
}

// Attach the finalized withdrawal tx to the deposits. After a client
// restart we can use this address as an indicator to republish the
// withdrawal tx and continue the withdrawal.
// Deposits with the same withdrawal tx are part of the same withdrawal.
for _, d := range deposits {
d.Lock()
d.FinalizedWithdrawalTx = finalizedTx
d.Unlock()
}

// Add the new withdrawal tx to the finalized withdrawals to republish
// it on block arrivals.
m.mu.Lock()

@starius starius Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add the replacement to this map only after all deposit updates succeed? If an UpdateDeposit below fails, the method returns but the old transaction has already been removed and the replacement remains eligible for block-triggered republication. That can broadcast a transaction whose database records are old or only partially updated.

m.finalizedWithdrawalTxns[finalizedTx.TxHash()] = finalizedTx
m.mu.Unlock()

// Transition the deposits to the withdrawing state. If the user fee
// bumped a withdrawal this results in a NOOP transition.
err = m.cfg.DepositManager.TransitionDeposits(
ctx, deposits, deposit.OnWithdrawInitiated, deposit.Withdrawing,
)
// A fee bump is a self-transition, which the deposit FSM doesn't
// persist. Explicitly store the replacement transaction in that case.
if allWithdrawing {
for _, d := range deposits {
err = m.cfg.DepositManager.UpdateDeposit(ctx, d)
if err != nil {
return "", "", fmt.Errorf("failed to update "+
"deposit %w", err)
}
}
}

// Publish only after the finalized transaction is durable and local
// tracking is set up. Keep that preparation on publication errors because
// the wallet RPC outcome can be ambiguous.
published, err := m.publishFinalizedWithdrawalTx(ctx, finalizedTx)
if err != nil {
return "", "", fmt.Errorf("failed to transition deposits %w",
err)
return "", "", err
}

// Update the deposits in the database.
for _, d := range deposits {
err = m.cfg.DepositManager.UpdateDeposit(ctx, d)
if err != nil {
return "", "", fmt.Errorf("failed to update "+
"deposit %w", err)
}
if !published {
return "", "", nil
}

return finalizedTx.TxID(), withdrawalAddress.String(), nil
Expand Down
Loading